diff --git a/AGENTS.md b/AGENTS.md index 74d9274..a78eca2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,6 +77,7 @@ Common variables (subset; see command READMEs for complete lists): - The MCP SDK validates a tool's `InputSchema` before the handler runs, so a parameter listed in `Required` can never be defaulted or aliased in Go. Widening a shipped tool's parameter while still accepting the old form (as `move_task_to_workflow_stage` does for the scalar `task_id` it advertised before it took `task_ids`) means leaving the new parameter out of `Required` and enforcing it in the handler. - Annotation hints are mandatory and must be explicit: every tool sets `ReadOnlyHint`, `DestructiveHint` and `OpenWorldHint`. The latter two are `*bool` in the SDK, so a nil value is omitted from `tools/list` and the spec then defaults it to `true`; OpenAI's app review rejects tools with missing hints. Use `new(false)` unless the tool truly destroys data (deletes, plus Desk ticket create/reply, which email the customer) or reaches outside the customer's Teamwork account (help-doc articles published to a public knowledge base, Desk ticket create/reply). `TestAnnotationHintsAreExplicit` in `cmd/docs-gen/main_test.go` guards this across all four products with `allowDelete=true`. - JSON-Schema gotcha (OpenAI Responses API): every `Type: "array"` node — including inside `AnyOf`/`OneOf`/`AllOf` branches — must declare `Items`. OpenAI rejects bare arrays at tool-registration time even with `strict: false`; Anthropic does not, so Claude Desktop hides the bug. `TestToolInputSchemasArrayItems` in `internal/twprojects/tools_test.go` guards this — if it fires, pick the right item schema rather than weakening the test. +- Any output schema built by reflection over an SDK response must be generated through `helpers.WithDateTypeSchema(...)`. `twapi.Date` and `twapi.OptionalDateTime` are defined over `time.Time`, so `jsonschema.For` describes them by `time.Time`'s unexported fields — an opaque `object` — while their `MarshalJSON` writes a string. Every response carrying one then fails output-schema validation at any validating client, on every call, for every row; non-validating clients see nothing wrong, so this ships silently. A response picks the types up transitively (`projects.Team` is the only model with an `OptionalDateTime`, but `SearchResponse` sideloads `Team`), so register the override rather than reasoning about which models are affected — pre-existing `TypeSchemas` entries are preserved, as `teams.go` does for `LegacyNumber`. Two traps when testing: the mocks in `*_test.go` usually reply `{}`, which leaves such fields nil and encodes as `null`, exercising neither the set nor the unset case; and neither the server nor `testutil.ExecuteToolRequest` validates structured content, so a mismatch is invisible in-process. `TestTeamDeletedDateValidatesAgainstOutputSchema` in `internal/twprojects/teams_test.go` validates the result against the tool's published schema by hand — copy that shape rather than assuming a green tool test means the wire shape is right. - Date and date-time parameters go through the binders in `internal/helpers/tool_parser.go`, which accept more than one layout (see `dateTimeLayouts` in `internal/helpers/datetime.go`): RFC 3339, an offset-less date-time, and a plain `YYYY-MM-DD`. Models emit the plain date by default when asked about a range, so a strict RFC 3339 parse costs a failed first call and a visible retry. Do not narrow this back. Use `helpers.DateTimeFilterSchema(...)` rather than an inline schema so every filter advertises both forms, and pass `helpers.EndOfDay()` to the binder for any *upper-bound* filter (`end_date`, `*_before`) — a date-only value there must resolve to the day's last second, or the range silently drops its closing day. Handlers that forward the value as a raw query-string parameter instead of binding it use `helpers.NormalizeDateTime(...)`. - The calendar events endpoint binds `fields[calendarsEvents]`, not `fields[events]` (SDK v1.20.8+ sends the right key via a `sparsefields:key` marker), and its server-side filtering was broken until an API fix in 2026-08. Verify against the live API before assuming an endpoint honours `fields[...]`. - `list_*` tools follow a specific contract — see `TaskList` in `internal/twprojects/tasks.go` as the canonical pattern: diff --git a/go.mod b/go.mod index 4922342..d27ae84 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/sonh/qs v0.7.0 github.com/teamwork/desksdkgo v1.1.0 github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb - github.com/teamwork/twapi-go-sdk v1.21.3 + github.com/teamwork/twapi-go-sdk v1.21.4 ) require ( diff --git a/go.sum b/go.sum index b487513..76e60e2 100644 --- a/go.sum +++ b/go.sum @@ -194,6 +194,8 @@ github.com/teamwork/twapi-go-sdk v1.21.2 h1:SsLaxc15Q+m1v+LO0UHWoxCJ+I4wf6uTftfV github.com/teamwork/twapi-go-sdk v1.21.2/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/teamwork/twapi-go-sdk v1.21.3 h1:xTE2l2Xfw9WQLIlWYUNazQM4y4N6K3Y34CJuUO0xTSM= github.com/teamwork/twapi-go-sdk v1.21.3/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= +github.com/teamwork/twapi-go-sdk v1.21.4 h1:ELySgyUMeSuyrZzuhU0+4bjBgf8X8pZEMAqVxm+7J5o= +github.com/teamwork/twapi-go-sdk v1.21.4/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/tinylib/msgp v1.6.3 h1:bCSxiTz386UTgyT1i0MSCvdbWjVW+8sG3PjkGsZQt4s= github.com/tinylib/msgp v1.6.3/go.mod h1:RSp0LW9oSxFut3KzESt5Voq4GVWyS+PSulT77roAqEA= github.com/tklauser/go-sysconf v0.3.16 h1:frioLaCQSsF5Cy1jgRBrzr6t502KIIwQ0MArYICU0nA= diff --git a/internal/helpers/schema_date.go b/internal/helpers/schema_date.go index 4c28aad..75b7996 100644 --- a/internal/helpers/schema_date.go +++ b/internal/helpers/schema_date.go @@ -7,14 +7,22 @@ import ( twapi "github.com/teamwork/twapi-go-sdk" ) -// WithDateTypeSchema registers a JSON-schema override for the twapi.Date type -// on the given generation options. twapi.Date is defined as `type Date -// time.Time`, so the reflection-based generator would otherwise emit a useless -// object schema for time.Time's unexported fields. This override forces it to a -// nullable, date-only string. +// WithDateTypeSchema registers JSON-schema overrides for the SDK's date types +// on the given generation options. Both twapi.Date and twapi.OptionalDateTime +// are defined over time.Time (`type Date time.Time`), so the reflection-based +// generator would otherwise emit a useless object schema for time.Time's +// unexported fields — while their MarshalJSON methods emit a string. Every +// response carrying one then fails output-schema validation on every call. The +// overrides force them to nullable strings matching what the marshaller +// actually writes. +// +// The types are covered here rather than at each call site because a response +// picks them up transitively: projects.Team is the only model with an +// OptionalDateTime field, but SearchResponse sideloads Team, so the search +// schema needs the same override. // // Use it whenever generating an output schema from a response type that carries -// (or sideloads) twapi.Date fields: +// (or sideloads) those fields: // // schema, err = jsonschema.For[Response](helpers.WithDateTypeSchema(&jsonschema.ForOptions{})) // @@ -33,5 +41,10 @@ func WithDateTypeSchema(opts *jsonschema.ForOptions) *jsonschema.ForOptions { Format: "date", Description: "Null or date-only date string", } + opts.TypeSchemas[reflect.TypeFor[twapi.OptionalDateTime]()] = &jsonschema.Schema{ + Types: []string{"null", "string"}, + Format: "date-time", + Description: "Null or RFC3339 date-time string. Null when the value is unset.", + } return opts } diff --git a/internal/twprojects/teams.go b/internal/twprojects/teams.go index ace6dae..97f298a 100644 --- a/internal/twprojects/teams.go +++ b/internal/twprojects/teams.go @@ -37,26 +37,26 @@ func init() { var err error // generate the output schemas only once - teamGetOutputSchema, err = jsonschema.For[projects.TeamGetResponse](&jsonschema.ForOptions{ + teamGetOutputSchema, err = jsonschema.For[projects.TeamGetResponse](helpers.WithDateTypeSchema(&jsonschema.ForOptions{ TypeSchemas: map[reflect.Type]*jsonschema.Schema{ reflect.TypeFor[projects.LegacyNumber](): { Type: "string", Description: "A numeric value that is returned as a string.", }, }, - }) + })) if err != nil { panic(fmt.Sprintf("failed to generate JSON schema for TeamGetResponse: %v", err)) } helpers.WithMetaWebLinkSchema(teamGetOutputSchema) - teamListOutputSchema, err = jsonschema.For[projects.TeamListResponse](&jsonschema.ForOptions{ + teamListOutputSchema, err = jsonschema.For[projects.TeamListResponse](helpers.WithDateTypeSchema(&jsonschema.ForOptions{ TypeSchemas: map[reflect.Type]*jsonschema.Schema{ reflect.TypeFor[projects.LegacyNumber](): { Type: "string", Description: "A numeric value that is returned as a string.", }, }, - }) + })) if err != nil { panic(fmt.Sprintf("failed to generate JSON schema for TeamListResponse: %v", err)) } diff --git a/internal/twprojects/teams_test.go b/internal/twprojects/teams_test.go index e25e091..9fc17ad 100644 --- a/internal/twprojects/teams_test.go +++ b/internal/twprojects/teams_test.go @@ -1,9 +1,12 @@ package twprojects_test import ( + "encoding/json" "net/http" "testing" + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/teamwork/mcp/internal/testutil" "github.com/teamwork/mcp/internal/twprojects" ) @@ -66,6 +69,117 @@ func TestTeamList(t *testing.T) { }) } +// TestTeamDeletedDateValidatesAgainstOutputSchema pins the JSON shape of the +// team `deletedDate` against the tools' published output schemas. It is the +// SDK's only twapi.OptionalDateTime field, and that type is defined over +// time.Time, so the reflected schema described it as an object while +// MarshalJSON emitted an RFC3339 string — every validating client discarded the +// whole response. +// +// The other team tests pass `{}` as the body, which leaves DeletedAt nil and +// encodes as null, so they never exercised either half. Both cases belong here: +// the empty string the API sends for every live team (encoding/json allocates +// the pointer before UnmarshalJSON runs, so it survives as a non-nil pointer to +// the zero time), and the timestamp it sends for a deleted one. +func TestTeamDeletedDateValidatesAgainstOutputSchema(t *testing.T) { + tests := []struct { + name string + deletedDate string + }{ + {name: "live team", deletedDate: `""`}, + {name: "deleted team", deletedDate: `"2026-01-02T03:04:05Z"`}, + {name: "null", deletedDate: `null`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + team := `{"id":"123","name":"Example","deletedDate":` + tt.deletedDate + `}` + + mcpServer := mcpServerMock(t, http.StatusOK, []byte(`{"team":`+team+`}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTeamGet.String(), map[string]any{ + "id": float64(123), + }, testutil.ExecuteToolRequestWithCheckMessage( + checkStructuredContentMatchesOutputSchema(twprojects.MethodTeamGet.String()), + )) + + mcpServer = mcpServerMock(t, http.StatusOK, []byte(`{"teams":[`+team+`]}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTeamList.String(), map[string]any{ + "page": float64(1), + "page_size": float64(10), + }, testutil.ExecuteToolRequestWithCheckMessage( + checkStructuredContentMatchesOutputSchema(twprojects.MethodTeamList.String()), + )) + }) + } +} + +// checkStructuredContentMatchesOutputSchema returns a check that validates a +// tool result's StructuredContent against the output schema the named tool +// publishes in tools/list. +// +// Neither the test harness nor the server validates this, so a mismatch is +// invisible in-process and only surfaces at a validating client, which then +// discards a response the server returned successfully. StructuredContent holds +// the Go value rather than decoded JSON, so it has to be round-tripped through +// encoding/json to see what the client will actually receive. +func checkStructuredContentMatchesOutputSchema(toolName string) func(t *testing.T, result mcp.Result) { + return func(t *testing.T, result mcp.Result) { + t.Helper() + + testutil.CheckMessage(t, result) + + toolResult, ok := result.(*mcp.CallToolResult) + if !ok { + t.Fatalf("unexpected result type: %T", result) + } + if toolResult.StructuredContent == nil { + t.Fatalf("tool %s returned no structured content", toolName) + } + + schema := outputSchemaFor(t, toolName) + resolved, err := schema.Resolve(nil) + if err != nil { + t.Fatalf("failed to resolve output schema for %s: %s", toolName, err) + } + + encoded, err := json.Marshal(toolResult.StructuredContent) + if err != nil { + t.Fatalf("failed to encode structured content for %s: %s", toolName, err) + } + var decoded any + if err := json.Unmarshal(encoded, &decoded); err != nil { + t.Fatalf("failed to decode structured content for %s: %s", toolName, err) + } + + if err := resolved.Validate(decoded); err != nil { + t.Errorf("tool %s: structured content does not match its output schema: %s\nbody: %s", + toolName, err, encoded) + } + } +} + +// outputSchemaFor looks up the output schema a registered tool publishes. +func outputSchemaFor(t *testing.T, toolName string) *jsonschema.Schema { + t.Helper() + + group := twprojects.DefaultToolsetGroup(false, true, testutil.ProjectsEngineMock(http.StatusOK, nil)) + for _, toolset := range group.Toolsets { + for _, tool := range toolset.GetAvailableTools() { + if tool.Tool.Name != toolName { + continue + } + schema, ok := tool.Tool.OutputSchema.(*jsonschema.Schema) + if !ok { + t.Fatalf("tool %s: OutputSchema is not *jsonschema.Schema (got %T)", + toolName, tool.Tool.OutputSchema) + } + return schema + } + } + t.Fatalf("tool %s is not registered", toolName) + return nil +} + func TestTeamListByCompany(t *testing.T) { mcpServer := mcpServerMock(t, http.StatusOK, []byte(`{}`)) testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTeamList.String(), map[string]any{