test: Pin sync/task tool-call contracts before dispatch dedup#1073
test: Pin sync/task tool-call contracts before dispatch dedup#1073jirispilka wants to merge 2 commits into
Conversation
Umbrella #658 will merge the two near-duplicate tool-call paths in src/mcp/server.ts; five handler-level contracts had no tests, so the refactor could silently change them. Pins, table-driven per failure class (402 / permission-approval / generic): task terminal-status mapping, telemetry property shapes on both paths (the only guard for these — dashboards consume them with no CI check), sync error result shapes, the taskSupport pre-dispatch gate, and the ACTOR_MCP-in-task {} gap (pinned as-is, flip when #1063 lands). Also lifts the copy-pasted test harness into tests/unit/helpers/mcp_server.ts and migrates the three existing mcp.server suites onto it, assertions unchanged. Alternative — copying the harness locally — was rejected in review to stop the duplication spreading further. Closes #1061 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LUZNJQKNCn7ntZCbYsFer5
vojtechj-apify
left a comment
vojtechj-apify
left a comment
There was a problem hiding this comment.
Nits found by Claude during a review pass — nothing serious, all cosmetic. The substance checks out: every pinned contract assertion was traced back to the actual behavior in src/mcp/server.ts (terminal-status mapping, per-failure-class telemetry shapes on both paths, taskId absent from Segment properties, the taskSupport gate, the ACTOR_MCP task-mode gap) and is accurate. Harness extraction is faithful, the wait-for-spy in the task telemetry tests is genuinely required, and type-check/lint/unit suite (972 passed) are all green.
| * `CallToolRequestSchema` catch and the `executeToolAndUpdateTask` catch). Failure classes are | ||
| * fabricated by throwing from a fake tool's `call`; both the sync path (result shapes) and the task | ||
| * path (terminal status mapping) assert the same source-of-truth per class. See DESIGN | ||
| * `.devforge/2-design.md`. |
There was a problem hiding this comment.
nit (Claude): .devforge/2-design.md is gitignored per-run scratch (the .devforge/.gitignore whitelists only config.json/registry.json), so this reference is dead for anyone reading the file later. That context fits better in the PR description than the code.
| run: (server: ActorsMcpServer) => Promise<T>, | ||
| options?: Partial<ActorsMcpServerOptions>, | ||
| ): Promise<T> { | ||
| const { ActorsMcpServer } = await import('../../../src/mcp/server.js'); |
There was a problem hiding this comment.
nit (Claude): the dynamic await import(...) isn't needed — none of the four consumer suites use vi.mock on this module, and the file already type-imports ActorsMcpServer. Looks like cruft carried over from the old progress_wiring harness; a static value import is simpler.
| ); | ||
|
|
||
| const handler = getHandler(server as never, 'tasks/result'); | ||
| const handler = getRequestHandler(server, 'tasks/result'); |
There was a problem hiding this comment.
nit (Claude): these two tasks/result tests migrated onto getRequestHandler but kept the rest of the hand-rolled bootstrap: a dynamic await import('../../src/mcp/server.js') of a module this file already imports statically (line 6), manual new ActorsMcpServer, and await server.close() outside a finally. withServer + server.taskStore would cover both tests and finish the migration this PR started.
vojtechj-apify
left a comment
vojtechj-apify
left a comment
There was a problem hiding this comment.
LGTM — contracts verified against src/mcp/server.ts behavior. The nits from the earlier review pass are non-blocking.
4 participants
What we're solving
Umbrella #658 will merge the two near-duplicate tool-call paths in
src/mcp/server.ts(syncCallToolRequestSchemahandler andexecuteToolAndUpdateTask). Five handler-level contracts had no tests, so the refactor could silently change them. The riskiest is telemetry property shapes: nothing in CI guards them — the internal repo consumes none of them in code, only analytics dashboards do. Closes #1061 (step 1 of #658).How
New
tests/unit/mcp.server.tool_call_contracts.test.ts, table-driven per failure class (402 / permission-approval / generic execution error), driving the real handler with synthetic tools that throw the class fixture:completed; generic storedfailed+internalToolStatus.vi.spyOnontrackToolCall; telemetry enabled with no token +allowUnauthMode, so no network).taskSupport→InvalidParams, tool never invoked.{}ascompleted— pinned as current behavior with a comment to flip when refactor: Extract dispatchToolCall with exhaustive TOOL_TYPE switch #1063 closes it.Also lifts the harness the existing
mcp.server.*suites copy-pasted intotests/unit/helpers/mcp_server.tsand migratesmcp.server.error_handling,mcp.server.progress_wiring,mcp.task_notificationsonto it — assertion text unchanged, plumbing only.mcp.server.capability_gatingdeliberately not migrated (its harness differs).Alternatives considered
vi.mockof the telemetry module — the lighter namespace spy intercepts fine, so the full mock wasn't needed.Run evidence
type-check,lint,test:unit(972 passed / 2 skipped, deterministic across repeated runs),format,check:agentsall green.failure_http_status) → tests failed → restored.Notes for
apify-mcp-server-internalNo internal change needed: internal consumes none of the tool-call telemetry shapes in code (verified) — this suite is the only guard for them. Result shapes /
_metaare pinned, not changed.🤖 Generated with Claude Code
https://claude.ai/code/session_01LUZNJQKNCn7ntZCbYsFer5
Generated by Claude Code