MCP follow-up: converge the Codex review of #662

Merged
JE
2mo ago
+178-15

Follow-up to #662, which merged before the Codex review landed. Triage of the three findings (each answered in-thread on #662):

  • P1 — Nix vendorHash (go.mod): already fixed before the merge — the branch's Refresh the Nix vendorHash commit rode the squash into db1ff0c, and the Nix flake job was green on the merged head. No action here.
  • P2 — paginated actions without a page parameter (dispatch): confirmed. Six operations (ListWebhooks, ListChatbots, ListMessageTypes, ListPingablePeople, ListQuestionAnswerers, ListUploadVersions) are marked paginated by the behavior model but declare no page query parameter, so the next_page a listing returns was rejected as an unknown parameter and pages past the first were unreachable. Fixed by synthesizing the parameter at catalog load from the paginated trait — trait-driven rather than a name table, so it covers whatever the model marks paginated and no-ops once the SDK export declares the parameter itself. Pinned by a catalog invariant (every paginated operation declares exactly one integer page query param) and a round-trip test through list_webhooks.
  • P2 — MCP command errors on the protocol stream (commands/mcp.go): confirmed. Errors from the command's RunE rendered through cli.Execute's stdout writers, landing a CLI error envelope in the JSON-RPC transport where it reads as a malformed protocol message. Fixed with a stdout_wire annotation on the command: Execute reports errors for wire commands on stderr — plain lines, hint included when the message doesn't already carry it, same exit codes.

Also picks up Copilot's suppressed nit from the same review pass: next_page was emitted as a string ("2") while every page parameter advertises an integer schema; it's now a number, and the pagination round-trip test covers passing it straight back.

The remaining Copilot/Codex-adjacent thread on #662 (raw AccountClient verbs bypassing OnOperationGate) stays declined for the reasons on the thread: the right fix is new basecamp-sdk surface (Do(ctx, OperationInfo, ...)) that both basecamp api and basecamp mcp ride, not a 250-entry dispatch table here.

Summary by cubic

Fixes the two P2 findings from the Codex review on #662 plus a related schema mismatch.

  • Synthesizes a page query parameter for operations the model marks paginated but the SDK export leaves undeclared, so next_page values are accepted by the dispatcher.
  • Routes basecamp mcp errors to stderr instead of stdout, keeping CLI error envelopes out of the JSON-RPC transport.
  • Emits next_page as a number, matching the page parameter's integer schema.

The declined thread about raw AccountClient verbs is intentionally not addressed here; the right fix is new basecamp-sdk surface.