MCP follow-up: converge the Codex review of #662 - #663
Merged
Conversation
The SDK export marks six operations paginated — ListWebhooks, ListChatbots, ListMessageTypes, ListPingablePeople, ListQuestionAnswerers, ListUploadVersions — without declaring a page query parameter. The dispatcher rejects parameters an operation does not declare, so the next_page value those listings return could never be passed back: every page after the first was unreachable over MCP. Synthesize the parameter at catalog load from the paginated trait, next to the account rescope. Trait-driven rather than a name table: it covers whatever the model marks paginated and no-ops once the export declares the parameter itself. Pinned by a catalog test asserting every paginated operation declares exactly one integer page query parameter.
Every advertised page parameter is an integer, and the documented
pagination wrapper is {"next_page": N, "results": ...} — but nextPage
returned the Link header's query value as a string, emitting
"next_page":"2". Clients copying that continuation value into the next
call would send a schema-invalid string. Parse the page number when
extracting it, treating a non-numeric value as no next page, the same as
geared_pagination treats pages.
The new round-trip test drives list_webhooks — one of the operations
whose page parameter is synthesized — through a full pagination cycle:
the next_page a listing returns is accepted as the follow-up call's page
parameter.
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
Fixes MCP pagination and keeps command errors off the JSON-RPC stdout stream.
Changes:
- Synthesizes missing integer
pageparameters for paginated operations. - Emits numeric
next_pagevalues with round-trip coverage. - Routes MCP command errors to stderr with preserved exit codes.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
internal/mcpserver/server_test.go |
Tests numeric pagination round trips. |
internal/mcpserver/dispatch.go |
Parses next-page values as integers. |
internal/mcpserver/dispatch_test.go |
Covers pagination parsing edge cases. |
internal/mcpserver/catalog.go |
Synthesizes missing page parameters. |
internal/mcpserver/catalog_test.go |
Enforces paginated catalog invariants. |
internal/commands/mcp.go |
Marks MCP stdout as a protocol stream. |
internal/commands/mcp_test.go |
Verifies the wire annotation. |
internal/cli/root.go |
Routes wire-command errors to stderr. |
internal/cli/root_test.go |
Tests wire-error formatting and exit codes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+495
to
+500
| apiErr := output.AsError(err) | ||
| fmt.Fprintln(w, "Error: "+apiErr.Message) | ||
| if apiErr.Hint != "" && !strings.Contains(apiErr.Message, apiErr.Hint) { | ||
| fmt.Fprintln(w, apiErr.Hint) | ||
| } | ||
| return output.ExitCodeFor(apiErr.Code) |
Member
Author
There was a problem hiding this comment.
Fixed in 3632b13: reportWireError now runs both message and hint through richtext.SanitizeSingleLine — the same treatment the styled renderer's sanitizeText applies (CR/CRLF normalized, escapes and controls stripped, collapsed to one line) — before writing and before the hint-containment check. Covered by a test case with ESC/CR/newline injection in the message.
Errors returned from the mcp command's RunE — unauthenticated launch, missing account, unknown domain, transport failure, session errors — flowed through cli.Execute's error rendering, whose writers all target stdout. For this command stdout is the MCP JSON-RPC transport, so the CLI error envelope landed as a malformed protocol message and the real failure hid behind the client's parse error. Mark the command stdout_wire, following the annotation convention, and have Execute report errors for wire commands on stderr: plain lines an MCP client's stderr log shows as-is, the structured error's hint when the message does not already carry it, and the same exit code the envelope path produces. Message and hint can carry SDK- or transport-controlled text, so both are sanitized to single terminal-safe lines, the same treatment the styled error renderer applies.
jeremy
force-pushed
the
mcp-codex-followup
branch
from
August 28, 2026 17:53
74a6b0d to
3632b13
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #662, which merged before the Codex review landed. Triage of the three findings (each answered in-thread on #662):
Refresh the Nix vendorHashcommit rode the squash into db1ff0c, and the Nix flake job was green on the merged head. No action here.pageparameter (dispatch): confirmed. Six operations (ListWebhooks, ListChatbots, ListMessageTypes, ListPingablePeople, ListQuestionAnswerers, ListUploadVersions) are marked paginated by the behavior model but declare nopagequery parameter, so thenext_pagea 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 integerpagequery param) and a round-trip test throughlist_webhooks.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 astdout_wireannotation 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_pagewas emitted as a string ("2") while everypageparameter 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
AccountClientverbs bypassingOnOperationGate) stays declined for the reasons on the thread: the right fix is new basecamp-sdk surface (Do(ctx, OperationInfo, ...)) that bothbasecamp apiandbasecamp mcpride, 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.
pagequery parameter for operations the model marks paginated but the SDK export leaves undeclared, sonext_pagevalues are accepted by the dispatcher.basecamp mcperrors to stderr instead of stdout, keeping CLI error envelopes out of the JSON-RPC transport.next_pageas a number, matching thepageparameter's integer schema.The declined thread about raw
AccountClientverbs is intentionally not addressed here; the right fix is new basecamp-sdk surface.Written for commit 3632b13. Summary will update on new commits.