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

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.

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())
+}

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())

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

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
+}

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.

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) {

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(`<https://3.basecampapi.com/999/projects.json?page=4>; rel="next"`)))
-	assert.Equal(t, "2",
+	assert.Equal(t, 2,
 		nextPage(link(`<https://x.test/a.json?page=1>; rel="prev", <https://x.test/a.json?page=2>; rel="next"`)))
-	assert.Empty(t, nextPage(link(`<https://x.test/a.json?page=1>; rel="prev"`)), "no next link")
-	assert.Empty(t, nextPage(link(`<https://x.test/a.json>; rel="next"`)), "next link without page")
-	assert.Empty(t, nextPage(http.Header{}), "no Link header")
+	assert.Zero(t, nextPage(link(`<https://x.test/a.json?page=1>; rel="prev"`)), "no next link")
+	assert.Zero(t, nextPage(link(`<https://x.test/a.json>; rel="next"`)), "next link without page")
+	assert.Zero(t, nextPage(link(`<https://x.test/a.json?page=bogus>; rel="next"`)), "non-numeric page")
+	assert.Zero(t, nextPage(http.Header{}), "no Link header")
 }

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", `<http://`+r.Host+`/999/buckets/1/webhooks.json?page=2>; 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)