From 0f8de3b0d44207695b4c846ffd1da687384e84f4 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 10:30:45 -0700 Subject: [PATCH 1/3] Synthesize the page parameter paginated operations leave undeclared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/mcpserver/catalog.go | 34 ++++++++++++++++++++++++++++++ internal/mcpserver/catalog_test.go | 29 +++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/internal/mcpserver/catalog.go b/internal/mcpserver/catalog.go index e75ab089..1b230345 100644 --- a/internal/mcpserver/catalog.go +++ b/internal/mcpserver/catalog.go @@ -42,6 +42,7 @@ func loadCatalog() (*catalog.Catalog, error) { if err := rescopeToAccount(cat); err != nil { return nil, err } + synthesizePageParams(cat) return cat, nil } @@ -77,3 +78,36 @@ func rescopeToAccount(cat *catalog.Catalog) error { } return nil } + +// synthesizePageParams gives every paginated operation a page query +// parameter. The SDK export marks a handful of operations paginated without +// declaring one (ListWebhooks, ListChatbots, ...); left alone, that makes +// every page after the first unreachable over MCP — the dispatcher rejects +// parameters an operation does not declare, so the next_page value a listing +// returns could never be passed back. Synthesizing from the paginated trait +// covers whatever the model marks, and no-ops once the export declares the +// parameter itself. +func synthesizePageParams(cat *catalog.Catalog) { + for _, d := range cat.Domains { + for _, op := range d.Operations { + if !op.Paginated || declaresPage(op) { + continue + } + op.Params = append(op.Params, catalog.Param{ + Name: "page", + In: "query", + Description: "Page number for paginating through results. Defaults to 1.", + Schema: map[string]any{"type": "integer"}, + }) + } + } +} + +func declaresPage(op *catalog.Operation) bool { + for _, p := range op.Params { + if p.In == "query" && p.Name == "page" { + return true + } + } + return false +} diff --git a/internal/mcpserver/catalog_test.go b/internal/mcpserver/catalog_test.go index b57a49b8..28f80cbf 100644 --- a/internal/mcpserver/catalog_test.go +++ b/internal/mcpserver/catalog_test.go @@ -80,6 +80,35 @@ func TestCatalogIsAccountScoped(t *testing.T) { } } +// TestCatalogPaginatedActionsTakePage pins the synthesized page parameter: +// every operation the behavior model marks paginated must declare a page +// query parameter, whether the OpenAPI export supplies it or loadCatalog +// synthesizes it. Otherwise the next_page value a listing returns could +// never be passed back — the dispatcher rejects undeclared parameters. +func TestCatalogPaginatedActionsTakePage(t *testing.T) { + cat := loadForTest(t) + paginated := 0 + for _, d := range cat.Domains { + for _, op := range d.Operations { + if !op.Paginated { + continue + } + paginated++ + pages := 0 + for _, p := range op.Params { + if p.In != "query" || p.Name != "page" { + continue + } + pages++ + assert.Equal(t, "integer", p.Schema["type"], "operation %q page schema", op.ID) + assert.NotEmpty(t, p.Description, "operation %q page description", op.ID) + } + assert.Equal(t, 1, pages, "operation %q must declare exactly one page query parameter", op.ID) + } + } + assert.Equal(t, 61, paginated, "paginated operation count") +} + // TestCatalogSnapshot renders the full served surface — every tool // description, action, and flag — so a model sync or curation change shows // its whole effect as a reviewable diff. Regenerate with -update. From a0f4a9c46c505359f42f265ebd2e0777ba14e553 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 10:30:45 -0700 Subject: [PATCH 2/3] Surface next_page as a number, matching the page parameter schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/mcpserver/dispatch.go | 17 +++++------ internal/mcpserver/dispatch_test.go | 11 ++++---- internal/mcpserver/server_test.go | 44 +++++++++++++++++++++++++++-- 3 files changed, 57 insertions(+), 15 deletions(-) diff --git a/internal/mcpserver/dispatch.go b/internal/mcpserver/dispatch.go index 92d0e4d1..fa4333dd 100644 --- a/internal/mcpserver/dispatch.go +++ b/internal/mcpserver/dispatch.go @@ -71,7 +71,7 @@ func (d dispatcher) handle(ctx context.Context, dom gateway.Domain, op gateway.O } return gateway.JSONResult(result) } - if next := nextPage(resp.Headers); next != "" { + if next := nextPage(resp.Headers); next > 0 { // Paginated listings surface the Link rel="next" page to pass back // as the action's page parameter. wrapped, err := json.Marshal(map[string]any{"next_page": next, "results": resp.Data}) @@ -82,11 +82,12 @@ func (d dispatcher) handle(ctx context.Context, dom gateway.Domain, op gateway.O return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: string(resp.Data)}}}, nil } -// nextPage extracts the page parameter from a geared_pagination Link -// rel="next" header. Basecamp pages by number: when a listing has more, the -// result is wrapped as {"next_page": N, "results": ...} and the caller -// passes N back as the action's page parameter. -func nextPage(headers http.Header) string { +// nextPage extracts the page number from a geared_pagination Link +// rel="next" header, 0 when there is none. Basecamp pages by number: when a +// listing has more, the result is wrapped as {"next_page": N, "results": ...} +// and the caller passes N back as the action's page parameter — a number, to +// match the page parameter's integer schema. +func nextPage(headers http.Header) int { for _, link := range headers.Values("Link") { for part := range strings.SplitSeq(link, ",") { if !strings.Contains(part, `rel="next"`) { @@ -101,12 +102,12 @@ func nextPage(headers http.Header) string { if err != nil { continue } - if page := u.Query().Get("page"); page != "" { + if page, err := strconv.Atoi(u.Query().Get("page")); err == nil && page > 0 { return page } } } - return "" + return 0 } func (d dispatcher) call(ctx context.Context, method, path string, body any) (*basecamp.Response, error) { diff --git a/internal/mcpserver/dispatch_test.go b/internal/mcpserver/dispatch_test.go index 3acd5daa..69edeb97 100644 --- a/internal/mcpserver/dispatch_test.go +++ b/internal/mcpserver/dispatch_test.go @@ -180,11 +180,12 @@ func TestNextPage(t *testing.T) { return h } - assert.Equal(t, "4", + assert.Equal(t, 4, nextPage(link(`; rel="next"`))) - assert.Equal(t, "2", + assert.Equal(t, 2, nextPage(link(`; rel="prev", ; rel="next"`))) - assert.Empty(t, nextPage(link(`; rel="prev"`)), "no next link") - assert.Empty(t, nextPage(link(`; rel="next"`)), "next link without page") - assert.Empty(t, nextPage(http.Header{}), "no Link header") + assert.Zero(t, nextPage(link(`; rel="prev"`)), "no next link") + assert.Zero(t, nextPage(link(`; rel="next"`)), "next link without page") + assert.Zero(t, nextPage(link(`; rel="next"`)), "non-numeric page") + assert.Zero(t, nextPage(http.Header{}), "no Link header") } diff --git a/internal/mcpserver/server_test.go b/internal/mcpserver/server_test.go index 4711a765..74bf4996 100644 --- a/internal/mcpserver/server_test.go +++ b/internal/mcpserver/server_test.go @@ -158,14 +158,54 @@ func TestServerSurfacesPagination(t *testing.T) { }) require.False(t, isError, "list_projects failed: %s", text) var wrapped struct { - NextPage string `json:"next_page"` + NextPage int `json:"next_page"` Results json.RawMessage `json:"results"` } require.NoError(t, json.Unmarshal([]byte(text), &wrapped)) - assert.Equal(t, "2", wrapped.NextPage) + assert.Equal(t, 2, wrapped.NextPage, "next_page is a number, matching the page parameter's integer schema") assert.JSONEq(t, `[{"id":1}]`, string(wrapped.Results)) } +// TestServerAcceptsSynthesizedPageParam drives the pagination round trip +// through list_webhooks, one of the operations the model marks paginated +// without declaring a page parameter: the next_page a listing returns must +// be acceptable as the follow-up call's page parameter. +func TestServerAcceptsSynthesizedPageParam(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "/999/buckets/1/webhooks.json", r.URL.Path) + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write([]byte(`[{"id":2}]`)) + return + } + w.Header().Set("Link", `; rel="next"`) + _, _ = w.Write([]byte(`[{"id":1}]`)) + })) + t.Cleanup(upstream.Close) + + srv, err := New(newTestAPI(upstream), Config{}) + require.NoError(t, err) + session := mcptest.Connect(t, srv.BuildMCPServer(slog.New(slog.DiscardHandler))) + + text, isError := mcptest.CallText(t, session, "basecamp_automation", map[string]any{ + "action": "list_webhooks", + "params": map[string]any{"bucketId": "1"}, + }) + require.False(t, isError, "list_webhooks failed: %s", text) + var wrapped struct { + NextPage int `json:"next_page"` + } + require.NoError(t, json.Unmarshal([]byte(text), &wrapped)) + require.Equal(t, 2, wrapped.NextPage) + + text, isError = mcptest.CallText(t, session, "basecamp_automation", map[string]any{ + "action": "list_webhooks", + "params": map[string]any{"bucketId": "1", "page": wrapped.NextPage}, + }) + require.False(t, isError, "passing next_page back must dispatch, got: %s", text) + assert.JSONEq(t, `[{"id":2}]`, text) +} + func TestServerSurfacesAPIErrorsInBand(t *testing.T) { upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Error(w, `{"error":"Record not found"}`, http.StatusNotFound) From 3632b131580530667bcae57d1043816efe869944 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 28 Aug 2026 10:31:55 -0700 Subject: [PATCH 3/3] Keep basecamp mcp errors off the MCP wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- internal/cli/root.go | 25 +++++++++++++++++++++++++ internal/cli/root_test.go | 24 ++++++++++++++++++++++++ internal/commands/mcp.go | 4 ++++ internal/commands/mcp_test.go | 5 +++++ 4 files changed, 58 insertions(+) diff --git a/internal/cli/root.go b/internal/cli/root.go index a4267300..3ab3b7ef 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "io" "os" "path/filepath" "regexp" @@ -392,6 +393,14 @@ func Execute() { // Convert error to structured output apiErr := output.AsError(err) + // Commands whose stdout speaks a wire protocol (basecamp mcp: + // JSON-RPC) keep errors off stdout entirely — an error envelope + // there is a malformed protocol message that hides the real failure + // behind the client's parse error. Report on stderr and exit. + if executedCmd.Annotations["stdout_wire"] != "" { + os.Exit(reportWireError(os.Stderr, err)) + } + // jq-related errors (validation failures, unsupported commands, conflicts) // must never be fed through the jq filter. Skip app.Err() entirely and // render with a plain writer. @@ -479,6 +488,22 @@ func Execute() { } } +// reportWireError renders err for a command whose stdout speaks a wire +// protocol: plain lines on w (stderr), nothing on stdout. Returns the +// process exit code for the error. 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. +func reportWireError(w io.Writer, err error) int { + apiErr := output.AsError(err) + message := richtext.SanitizeSingleLine(apiErr.Message) + hint := richtext.SanitizeSingleLine(apiErr.Hint) + fmt.Fprintln(w, "Error: "+message) + if hint != "" && !strings.Contains(message, hint) { + fmt.Fprintln(w, hint) + } + return output.ExitCodeFor(apiErr.Code) +} + // jqUsable reports whether a filter parses and compiles. Only these failures // are knowable before any output is produced, which is what makes clearing the // filter safe: a filter that fails at runtime may already have written. diff --git a/internal/cli/root_test.go b/internal/cli/root_test.go index e6ec8140..81887b21 100644 --- a/internal/cli/root_test.go +++ b/internal/cli/root_test.go @@ -13,6 +13,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/commands" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/version" ) @@ -370,3 +371,26 @@ func stubTerminalStdio(t *testing.T) { pty.Close() }) } + +// TestReportWireError pins the error rendering for wire commands (see the +// stdout_wire annotation): plain lines suitable for an MCP client's stderr +// log, with the structured error's hint when it has one, and the same exit +// code the envelope path would produce. A hint the message already carries +// (ErrAuth bakes its own into the message) is not repeated. +func TestReportWireError(t *testing.T) { + var buf bytes.Buffer + code := reportWireError(&buf, output.ErrAuth("Not authenticated. Run: basecamp auth login")) + assert.Equal(t, "Error: Not authenticated. Run: basecamp auth login\n", buf.String()) + assert.Equal(t, output.ExitCodeFor(output.CodeAuth), code) + + buf.Reset() + code = reportWireError(&buf, output.ErrUsageHint("subcommand required", "Usage: basecamp mcp")) + assert.Equal(t, "Error: subcommand required\nUsage: basecamp mcp\n", buf.String()) + assert.Equal(t, output.ExitCodeFor(output.CodeUsage), code) + + // Server- or transport-controlled text is sanitized to a single + // terminal-safe line, like the styled error renderer does. + buf.Reset() + reportWireError(&buf, output.ErrAPI(502, "bad\x1b[31mgateway\r\ninjected")) + assert.Equal(t, "Error: badgateway injected\n", buf.String()) +} diff --git a/internal/commands/mcp.go b/internal/commands/mcp.go index 811a6342..3cc47790 100644 --- a/internal/commands/mcp.go +++ b/internal/commands/mcp.go @@ -37,6 +37,10 @@ func NewMCPCmd() *cobra.Command { Args: cobra.NoArgs, Annotations: map[string]string{ "agent_notes": "Long-running server; stdout speaks the MCP wire protocol. Not for interactive use.", + // cli.Execute keeps errors off stdout for wire commands: an + // error envelope there would be a malformed JSON-RPC message, + // hiding the real failure behind the client's parse error. + "stdout_wire": "mcp", }, RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) diff --git a/internal/commands/mcp_test.go b/internal/commands/mcp_test.go index 8df1485d..4a489237 100644 --- a/internal/commands/mcp_test.go +++ b/internal/commands/mcp_test.go @@ -25,6 +25,11 @@ func TestMCPCommandFlags(t *testing.T) { require.NotNil(t, readOnly) assert.Equal(t, "false", readOnly.DefValue, "full surface is the default, matching basecamp-mcp-server") require.NotNil(t, cmd.Flags().Lookup("domains")) + + // Stdout is the MCP JSON-RPC transport: the stdout_wire annotation makes + // cli.Execute report this command's errors on stderr instead of writing + // an error envelope into the protocol stream. + assert.NotEmpty(t, cmd.Annotations["stdout_wire"], "basecamp mcp must keep errors off the MCP wire") } // setupMCPTestApp builds the app the way the root command would: real