From 78898a48c4c1ca7d9294dd9f67c0d9f2aaa3a77e Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 16 Sep 2026 22:39:18 +0200 Subject: [PATCH 1/2] Serve the event feed's refusals as typed in-band errors over MCP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #726 put the account event feed on this server: poll_events and poll_inbox in the eventfeed domain, through the mechanical dispatch every other operation uses. That dispatch turns a failed call into its message and nothing more, because the canonical SDK error keeps only the message, the hint and a validation body's field errors. For the feed that throws the contract away. Its refusals are the recovery instructions — a 410 names where to re-enter, a 409 names the two filter digests that disagree, a 400 says whether to re-enter or to fix the filters — and a consumer that receives them as prose has lost the lane it was following. So the two poll lanes now run through EventFeedService, whose typed errors carry those members back out, and they come back as typed in-band errors in the vocabulary basecamp-mcp-server serves. The request is still built from the vendored model, so parameters, validation and the describe schema stay the model's. Rebuilt on #726 and on basecamp-sdk#912, which it pins. 912 split the refusals by lane, and each gets its own arm here: - The feed's 410, *FeedPositionGoneError, carries resume and epoch_after_id. - The inbox's 410, *InboxPositionGoneError, carries resume and never an epoch: that lane fences on 30-day retention and re-enters at since=0. - Either lane's 400, *FeedRequestError, is typed from its reason. The remedy sentence is read only when the deploy predates the reason, and an unknown reason is not guessed at through the sentence. - A 409 is filter_mismatch only when both digests are present. These types unwrap to the canonical error, so a missing arm is not a compile error: the refusal degrades to request_failed and silently drops the member a consumer recovers from. That is the trap 912 sets for any consumer, and the tests close it: inbox 410 arm removed: --- FAIL: TestFeedStalePositionsKeepTheirOwnRecoveryData/the_inbox_lane_resumes_at_since=0_with_no_epoch the 410 must carry its recovery data --- FAIL: TestFeedInbox410NeverCarriesAnEpoch 400 reason ignored: --- FAIL: TestFeedTellsTheTwo400sApart/the_reason_names_a_filter_error_even_when_the_sentence_says_otherwise --- FAIL: TestFeedTellsTheTwo400sApart/the_reason_names_a_position_error_on_the_inbox unknown reason guessed through the sentence: --- FAIL: TestFeedTellsTheTwo400sApart/a_reason_this_code_does_not_know_is_not_guessed_at_through_the_sentence Carried from review of the earlier revision of this PR, each proven red on the old code there: - A filter that normalizes to nothing (`buckets=,`, `types=,`) is refused, not dropped. Dropped, it left the dimension unset, which on the wire is no filter, and the caller got the whole account feed. - agents_only is claimed only on the bodyless 403 the inbox's principal guard sends, never on a scope or access refusal a caller could repair. - Argument refusals (a filter that narrows nothing, both entry points at once, a parameter buildRequest rejects) are typed invalid_arguments rather than prose. - EventFeed() is part of the required dispatcher API, so a client that cannot reach the feed cannot build a server that advertises it. The stream-ticket mint stays excluded, as #726 left it. Its result is a replayable bearer and a URL embedding it, this dispatcher returns results verbatim into a model transcript with no redaction, and nothing here could open the WebSocket it is for. The earlier revision of this PR served it read-only; that is not carried forward. Reversing a credential-exposure decision is not something to do inside a rebase. --- internal/mcpserver/dispatch.go | 22 +- internal/mcpserver/feed.go | 485 +++++++++++++++++++++++++++ internal/mcpserver/feed_test.go | 565 ++++++++++++++++++++++++++++++++ 3 files changed, 1069 insertions(+), 3 deletions(-) create mode 100644 internal/mcpserver/feed.go create mode 100644 internal/mcpserver/feed_test.go diff --git a/internal/mcpserver/dispatch.go b/internal/mcpserver/dispatch.go index f16fa66e6..3383ec754 100644 --- a/internal/mcpserver/dispatch.go +++ b/internal/mcpserver/dispatch.go @@ -23,9 +23,18 @@ import ( // refresh, retry, account scoping, and base URL resolution, so the // dispatcher only assembles paths and bodies. // -// The four verbs serve every model operation. The two services serve the -// composite actions (see composite.go), which are SDK compositions rather -// than single requests and so cannot be assembled from a method and a path. +// The four verbs serve every model operation but the feed's two poll lanes. +// The recordings and comments services serve the composite actions (see +// composite.go), which are SDK compositions rather than single requests and so +// cannot be assembled from a method and a path. The event feed service serves +// poll_events and poll_inbox, whose refusals carry the data a consumer resumes +// from and whose bodies the verbs above throw away (see feed.go). +// +// Every one of them is required rather than probed for. The catalog derives +// from the model and always carries these actions, so a client that satisfied +// only the verbs would advertise actions it then failed as internal errors — +// a promise nothing keeps. Required here, that mismatch cannot exist, and the +// compiler is what says so. type API interface { Get(ctx context.Context, path string) (*basecamp.Response, error) Post(ctx context.Context, path string, body any) (*basecamp.Response, error) @@ -34,6 +43,7 @@ type API interface { Recordings() *basecamp.RecordingsService Comments() *basecamp.CommentsService + EventFeed() *basecamp.EventFeedService } // The CLI hands its account-scoped client straight to New. @@ -71,6 +81,12 @@ func (d dispatcher) handle(ctx context.Context, dom gateway.Domain, op gateway.O return gateway.ErrorResult("%v", err), nil } + if isFeedOperation(full.ID) { + // The feed's refusals carry the data a consumer resumes from, which + // the raw path throws away; see feed.go. + return d.dispatchFeed(ctx, full, params), nil + } + path, body, err := buildRequest(full, params) if err != nil { return gateway.ErrorResult("%v", err), nil diff --git a/internal/mcpserver/feed.go b/internal/mcpserver/feed.go new file mode 100644 index 000000000..7c749bd98 --- /dev/null +++ b/internal/mcpserver/feed.go @@ -0,0 +1,485 @@ +package mcpserver + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/url" + "strconv" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" + + "github.com/basecamp/mcp/catalog" + "github.com/basecamp/mcp/gateway" +) + +// The event feed's two poll lanes are the one place this dispatcher does not +// go through the raw account client. +// +// Everywhere else a failed call becomes its message and nothing more, which is +// all the raw path can offer: the SDK's canonical *Error keeps the message, +// the hint and a validation body's field errors, and drops every other member +// the server wrote. For the feed that would throw away the contract. Its +// documented refusals are recovery instructions — a 410 names the epoch to +// re-enter after, or the URL to re-enter at; a 409 names the two filter +// digests that disagree — and a consumer that receives them as prose has lost +// the lane. +// +// basecamp-sdk models exactly that, so these operations run through +// EventFeedService, whose typed errors carry the members back out. The wire +// request is still built by buildRequest from the vendored model, so +// parameters, validation and the describe schema stay the model's, as they are +// for every other operation. +// +// The stream-ticket mint is not here because it is not served at all: the +// sync script drops it by policy (POLICY_EXCLUDED_OPERATIONS), since its +// result is a replayable bearer this surface would hand verbatim to a model +// transcript and could not use — opening the stream needs a WebSocket. +// +// The trade is on the success path: a page comes back through the SDK's +// modeled shapes rather than as the bytes BC3 wrote, so a member BC3 starts +// serving before the SDK regenerates would not reach a caller here. +// basecamp-mcp-server, which reads the wire itself, passes those through. The +// two agree the moment the SDK carries the raw body on its errors — which is +// also the day this file goes away. +// +// The refusals are read through the typed errors basecamp-sdk#912 split them +// into, one per lane where the lanes differ: *FeedPositionGoneError is the +// feed's 410 (an epoch and a resume at it), *InboxPositionGoneError is the +// inbox's (a resume at since=0 and no epoch at all), and *FeedRequestError is +// either lane's 400, with a reason. Each has its own arm below. The types +// unwrap to the canonical *basecamp.Error, so a missing arm is not a compile +// error — the refusal would quietly degrade to request_failed and lose the +// member a consumer recovers from. The tests drive each shape through the real +// SDK for exactly that reason. + +// Feed operation ids, as basecamp-sdk names them. +const ( + opPollEvents = "PollEvents" + opPollInbox = "PollInbox" +) + +// Feed error types: the vocabulary basecamp-mcp-server serves for the same +// operations. The type strings, the envelope and the recovery members are the +// same; what differs, and why, is in the PR that introduced this file +// (basecamp-cli#725). +const ( + feedErrInvalidPosition = "invalid_position" + feedErrInvalidFilter = "invalid_filter" + feedErrBadRequest = "bad_request" + feedErrFilterMismatch = "filter_mismatch" + feedErrStalePosition = "stale_position" + feedErrAgentsOnly = "agents_only" + feedErrInvalidArguments = "invalid_arguments" + feedErrUnexpected = "request_failed" +) + +// The two sentences BC3 renders for its two 400s. A malformed position is +// recovered by re-entering with since=, a bad filter by fixing the filters, and +// the instructions are opposites. BC3 names the case in a `reason` member +// (bc3#13362), which the SDK carries on *FeedRequestError and which is read +// first. These sentences are the fallback for a deploy that predates the +// member, where the SDK's advice is to surface the 400 as undifferentiated — +// the companion server keeps the same fallback, deliberately, so the two +// answer an older deploy alike. +const ( + feedFilterRemedy = "a position reset won't help" + feedPositionRemedy = "Resume with since=" +) + +// feedBodylessForbidden is what the SDK renders a 403 as when the response +// carried no message of its own (helpers.go: msgOrDefault(serverMsg, ...)). +// It is the only trace of a bodyless 403 that survives into the typed error, +// and the inbox's principal guard is the one refusal that arrives that way. +const feedBodylessForbidden = "access denied" + +// feedError is the typed in-band error, shaped as basecamp-mcp-server shapes +// it. Data carries the members the refusal offers for recovery. +type feedError struct { + Type string `json:"type"` + HTTPStatus int `json:"http_status,omitempty"` + Retryable bool `json:"retryable"` + RetryAfter int `json:"retry_after,omitempty"` + RequestID string `json:"request_id,omitempty"` + Message string `json:"message,omitempty"` + Data json.RawMessage `json:"data,omitempty"` +} + +type feedErrorPayload struct { + Error feedError `json:"error"` +} + +// isFeedOperation reports whether the operation runs through EventFeedService. +func isFeedOperation(id string) bool { + switch id { + case opPollEvents, opPollInbox: + return true + } + return false +} + +// dispatchFeed runs one feed operation. The path and query come from +// buildRequest, so the caller's parameters were checked against the same model +// schema describe advertises; only the answer is handled differently. +func (d dispatcher) dispatchFeed(ctx context.Context, op *catalog.Operation, params map[string]any) *mcp.CallToolResult { + // An unknown parameter or a non-scalar value is the caller's mistake in + // the same sense a filter that narrows nothing is, so it carries the same + // type. The describe schema makes these rare; rare is not a reason to be + // the one refusal on this path that arrives untyped. + path, _, err := buildRequest(op, params) + if err != nil { + return feedUsageResult("%v", err) + } + // Not the caller's doing: the path was assembled here. + query, err := feedQuery(path) + if err != nil { + return gateway.ErrorResult("internal error: %v", err) + } + + // Filters are read through a reader that refuses one which normalizes to + // nothing, because on the wire that means unfiltered — the whole account + // feed, handed back as if it were the answer to the question that was + // asked. A malformed filter must narrow to a refusal, never widen to + // everything. + filters := feedFilters{params: params, query: query} + + if since, position := query.Get("since"), query.Get("position"); since != "" && position != "" { + return feedUsageResult("since and position are two ways to enter the lane; pass one, not both") + } + + service := d.api.EventFeed() + var result any + var callErr error + switch op.ID { + case opPollEvents: + options := &basecamp.PollEventsOptions{ + Since: query.Get("since"), + Position: query.Get("position"), + } + if err := filters.all( + filters.strings("types", &options.Types), + filters.ids("buckets", &options.Buckets), + filters.ids("creators", &options.Creators), + filters.strings("performers", &options.Performers), + filters.strings("exclude_performers", &options.ExcludePerformers), + filters.strings("actor_types", &options.ActorTypes), + ); err != nil { + return feedUsageResult("%v", err) + } + result, callErr = service.PollEvents(ctx, options) + case opPollInbox: + options := &basecamp.PollInboxOptions{ + Since: query.Get("since"), + Position: query.Get("position"), + } + if err := filters.all( + filters.strings("reasons", &options.Reasons), + filters.strings("types", &options.Types), + filters.ids("buckets", &options.Buckets), + ); err != nil { + return feedUsageResult("%v", err) + } + result, callErr = service.PollInbox(ctx, options) + default: + return gateway.ErrorResult("internal error: %q is not an event feed operation", op.ID) + } + if callErr != nil { + return feedErrorResult(op.ID, callErr) + } + + encoded, err := json.MarshalIndent(result, "", " ") + if err != nil { + return gateway.ErrorResult("internal error: encode result") + } + return &mcp.CallToolResult{Content: []mcp.Content{&mcp.TextContent{Text: string(encoded)}}} +} + +// feedQuery reads back the query buildRequest assembled, so the filters go out +// exactly as the model says to spell them. +func feedQuery(path string) (url.Values, error) { + parsed, err := url.Parse(path) + if err != nil { + return nil, fmt.Errorf("built an unparsable feed path: %w", err) + } + return parsed.Query(), nil +} + +// feedFilters reads the filter dimensions off one request. +// +// It exists because the obvious reading is a fail-open. A filter the caller +// named but which arrives as nothing usable — "," or ",," or an empty string — +// would, if its components were simply dropped, leave the dimension unset, and +// an unset dimension on the wire means unfiltered. The caller asked to narrow +// and would be served the whole account feed, with nothing in the answer to +// say the filter had been thrown away. So a filter that is present and +// normalizes to nothing is refused, and one that is merely partly malformed +// keeps its components verbatim for BC3 to refuse by name. +type feedFilters struct { + params map[string]any + query url.Values +} + +// all returns the first error among the readers, so one malformed filter is +// reported rather than the last one. +func (f feedFilters) all(errs ...error) error { + for _, err := range errs { + if err != nil { + return err + } + } + return nil +} + +// strings reads a list-of-strings filter into dst. +func (f feedFilters) strings(name string, dst *[]string) error { + values, err := f.values(name) + if err != nil { + return err + } + *dst = values + return nil +} + +// ids reads a list-of-ids filter into dst. Every component must be a decimal +// id, empty ones included: the SDK's options are typed, so a component that is +// not a number has nowhere to go, and dropping it would narrow the filter +// silently — the mirror of the widening this type exists to prevent. +func (f feedFilters) ids(name string, dst *[]int64) error { + values, err := f.values(name) + if err != nil { + return err + } + if values == nil { + return nil + } + ids := make([]int64, 0, len(values)) + for _, value := range values { + id, parseErr := strconv.ParseInt(value, 10, 64) + if parseErr != nil { + return fmt.Errorf("%s takes decimal ids; got %q", name, value) + } + ids = append(ids, id) + } + *dst = ids + return nil +} + +// values splits one filter, or reports that the caller named it and gave it +// nothing to narrow by. A dimension the caller did not name at all is nil, +// which is the only way this returns an unset filter. +func (f feedFilters) values(name string) ([]string, error) { + if _, named := f.params[name]; !named { + return nil, nil + } + parts := feedSplit(f.query.Get(name)) + usable := 0 + for _, part := range parts { + if part != "" { + usable++ + } + } + if usable == 0 { + return nil, fmt.Errorf("%s had no usable values; omit it rather than passing an empty filter, which means unfiltered", name) + } + return parts, nil +} + +// feedSplit undoes the comma joining the wire uses, since the SDK's options +// take the values apart and rejoin them itself. Surrounding whitespace goes; +// an empty component stays, because dropping it is how "," turns into no +// filter at all. +func feedSplit(value string) []string { + if value == "" { + return nil + } + parts := strings.Split(value, ",") + out := make([]string, 0, len(parts)) + for _, part := range parts { + out = append(out, strings.TrimSpace(part)) + } + return out +} + +// feedErrorResult turns a feed failure into the typed in-band error. +// +// A type is claimed only on evidence — both digests on a 409, a resume (and, +// on the feed, an epoch) on a 410, a reason or a remedy on a 400 — because the +// type is a promise about how to recover, and a refusal that offers no +// recovery must not be dressed as one. What cannot be recognized keeps its +// status and says no more than that. +func feedErrorResult(operation string, err error) *mcp.CallToolResult { + // The SDK types a 409 on either digest; the claim needs both. The two + // digests are a comparison, and half of one does not say which side moved + // — while a digest written as "" is a value a consumer compares against + // and finds unequal. With one digest this is a refusal we cannot explain, + // so it is served as one. + var mismatch *basecamp.FeedFilterMismatchError + if errors.As(err, &mismatch) && mismatch.PositionDigest != "" && mismatch.FiltersDigest != "" { + return feedErrorPayloadResult(feedDetail(mismatch.Err, feedErrFilterMismatch, map[string]any{ + "position_digest": mismatch.PositionDigest, + "filters_digest": mismatch.FiltersDigest, + })) + } + + // The two 410s. The lanes fence on different things, so the SDK gives + // each its own type and neither can stand in for the other: the feed + // names the epoch it re-enters at, the inbox fences on a 30-day retention + // window, has no epoch at all, and re-enters at the earliest item it still + // holds. Both are checked against the lane that answered as well, so a + // type arriving on the wrong lane is not believed. + var feedGone *basecamp.FeedPositionGoneError + if operation == opPollEvents && errors.As(err, &feedGone) { + return feedErrorPayloadResult(feedDetail(feedGone.Err, feedErrStalePosition, map[string]any{ + "resume": feedGone.Resume, + "epoch_after_id": feedGone.EpochAfterID, + })) + } + var inboxGone *basecamp.InboxPositionGoneError + if operation == opPollInbox && errors.As(err, &inboxGone) { + return feedErrorPayloadResult(feedDetail(inboxGone.Err, feedErrStalePosition, map[string]any{ + "resume": inboxGone.Resume, + })) + } + + // The 400, with its reason when the server gave one. + var request *basecamp.FeedRequestError + if errors.As(err, &request) { + return feedErrorPayloadResult(feedDetail(request.Err, feedRequestErrorType(request), nil)) + } + + var apiErr *basecamp.Error + if !errors.As(err, &apiErr) { + // Transport, cancellation, a credential that could not be obtained: + // not the feed answering, and nothing to recover from in band. + return gateway.ErrorResult("%v", err) + } + + detail := feedDetail(apiErr, feedErrorType(operation, apiErr), nil) + if detail.Type == feedErrUnexpected { + // Repeating a request the server refused on its merits cannot repair + // it; a rate limit is worth another attempt after the wait it named. + detail.Retryable = apiErr.Retryable + } + if detail.Type == feedErrAgentsOnly { + // The message this replaces is the SDK's stand-in for a body that was + // not there. "access denied" says nothing a caller can act on; the + // guard this claim names does. + detail.Message = "the event inbox is served to agent principals only" + } + return feedErrorPayloadResult(detail) +} + +// feedDetail is the envelope every feed refusal shares: the canonical error's +// status, request id, wait and message, under the type claimed for it, with +// the recovery members it carries. +func feedDetail(base *basecamp.Error, errType string, data map[string]any) feedError { + detail := feedError{ + Type: errType, + HTTPStatus: base.HTTPStatus, + RequestID: base.RequestID, + RetryAfter: base.RetryAfter, + Message: base.Message, + } + if data != nil { + detail.Data = feedData(data) + } + return detail +} + +// feedRequestErrorType names a 400 from its reason, or from the remedy +// sentence when the deploy predates the reason, or declines to. +// +// An unrecognized reason is not guessed at through the sentence: the server +// answered, in a vocabulary this code does not yet know, and reading the prose +// over the top of that would be claiming to understand it better. +func feedRequestErrorType(request *basecamp.FeedRequestError) string { + switch request.Reason { + case basecamp.FeedReasonInvalidPosition: + return feedErrInvalidPosition + case basecamp.FeedReasonInvalidFilter: + return feedErrInvalidFilter + case "": + return feedRemedyErrorType(request.Err.Message) + } + return feedErrBadRequest +} + +// feedRemedyErrorType is the pre-reason reading of a 400: the one sentence +// that tells its two cases apart. +func feedRemedyErrorType(message string) string { + switch { + case strings.Contains(message, feedFilterRemedy): + return feedErrInvalidFilter + case strings.Contains(message, feedPositionRemedy): + return feedErrInvalidPosition + } + return feedErrBadRequest +} + +// feedErrorType names a refusal the SDK did not type, or declines to. +func feedErrorType(operation string, err *basecamp.Error) string { + switch err.HTTPStatus { + case 400: + // A 400 whose body the SDK could not read as the feed's own — no + // error member at all. Still the feed refusing the request. + return feedRemedyErrorType(err.Message) + case 403: + // The inbox is agents-only for now and refuses everyone else with a + // bodyless 403. Naming it keeps a caller from retrying a principal + // that will never be admitted — but only the bodyless one may be + // claimed: a 403 BC3 gave a reason for came from scope or access, and + // those are fixable, so typing them agents_only would tell a caller + // to stop trying about a condition it could repair. + // + // The evidence available here is thinner than the companion server's. + // That one reads the wire and tests the body for emptiness directly; + // by the time a refusal reaches this file the SDK has already + // substituted its own text for an absent message, so a bodyless 403 + // and one whose body said exactly "access denied" are identical. This + // claims the narrower thing it can actually see, and BC3 sending that + // precise phrase is the one case it would still get wrong. + if operation == opPollInbox && err.Message == feedBodylessForbidden { + return feedErrAgentsOnly + } + } + return feedErrUnexpected +} + +// feedUsageResult is a refusal this server made on the caller's arguments, +// served in the same typed shape as the ones BC3 makes. +// +// These are the refusals a caller hits most — a filter that narrows nothing, +// both entry points at once — and serving them as bare prose while every wire +// refusal carries a type would put the untyped answer on the common path and +// make "one contract, two servers" true only of the rare cases. The companion +// server answers the identical conditions as invalid_arguments; so does this. +func feedUsageResult(format string, args ...any) *mcp.CallToolResult { + return feedErrorPayloadResult(feedError{ + Type: feedErrInvalidArguments, + Message: fmt.Sprintf(format, args...), + }) +} + +func feedData(members map[string]any) json.RawMessage { + encoded, err := json.Marshal(members) + if err != nil { + return nil + } + return encoded +} + +func feedErrorPayloadResult(detail feedError) *mcp.CallToolResult { + payload := feedErrorPayload{Error: detail} + encoded, err := json.MarshalIndent(payload, "", " ") + if err != nil { + return gateway.ErrorResult("internal error: encode event feed error") + } + return &mcp.CallToolResult{ + IsError: true, + Content: []mcp.Content{&mcp.TextContent{Text: string(encoded)}}, + } +} diff --git a/internal/mcpserver/feed_test.go b/internal/mcpserver/feed_test.go new file mode 100644 index 000000000..691b31a33 --- /dev/null +++ b/internal/mcpserver/feed_test.go @@ -0,0 +1,565 @@ +package mcpserver + +import ( + "encoding/json" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/basecamp/mcp/mcptest" +) + +// feedSession serves one canned answer to whatever the feed asks for, and +// records the request that asked. +func feedSession(t *testing.T, status int, body string, seen *http.Request) *mcp.ClientSession { + t.Helper() + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if seen != nil { + *seen = *r + } + w.Header().Set("Content-Type", "application/json") + w.Header().Set("X-Request-Id", "req-feed-1") + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(upstream.Close) + + srv, err := New(newTestAPI(upstream), Config{}) + require.NoError(t, err) + return mcptest.Connect(t, srv.BuildMCPServer(slog.New(slog.DiscardHandler))) +} + +// feedErrorOf reads the typed in-band error off a failed feed call. +func feedErrorOf(t *testing.T, text string) map[string]any { + t.Helper() + var payload map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &payload), "the error must arrive as JSON: %s", text) + detail, ok := payload["error"].(map[string]any) + require.True(t, ok, "the error payload must be an object: %s", text) + return detail +} + +func callFeed(t *testing.T, session *mcp.ClientSession, action string, params map[string]any) (string, bool) { + t.Helper() + return mcptest.CallText(t, session, "basecamp_eventfeed", map[string]any{ + "action": action, + "params": params, + }) +} + +// The feed's filters go out comma-joined on one key per dimension, the form +// BC3 reads — never as repeated bare keys, which it does not. +func TestFeedPollBuildsTheAccountScopedRequest(t *testing.T) { + var seen http.Request + session := feedSession(t, http.StatusOK, `{"events":[],"position":"aBcD"}`, &seen) + + text, isError := callFeed(t, session, "poll_events", map[string]any{ + "since": "1071915468", + "types": "message.created,comment.created", + "buckets": "2085958499", + "performers": "self", + "exclude_performers": "1049715945,self", + "actor_types": "agent,person", + }) + require.False(t, isError, text) + + assert.Equal(t, http.MethodGet, seen.Method) + assert.Equal(t, "/999/events.json", seen.URL.Path) + query := seen.URL.Query() + assert.Equal(t, "1071915468", query.Get("since")) + assert.Equal(t, "message.created,comment.created", query.Get("types")) + assert.Equal(t, "2085958499", query.Get("buckets")) + assert.Equal(t, "self", query.Get("performers")) + assert.Equal(t, "1049715945,self", query.Get("exclude_performers")) + assert.Equal(t, "agent,person", query.Get("actor_types")) + assert.Len(t, query["types"], 1) +} + +// The inbox is its own resource, not a filter over the feed: a different path, +// a different ledger, and positions that are never interchangeable. +func TestFeedInboxPollsItsOwnResource(t *testing.T) { + var seen http.Request + session := feedSession(t, http.StatusOK, `{"items":[],"position":"aBcD"}`, &seen) + + text, isError := callFeed(t, session, "poll_inbox", map[string]any{ + "since": "0", + "reasons": "mentioned,assigned", + "buckets": "2085958499", + }) + require.False(t, isError, text) + + assert.Equal(t, "/999/inbox.json", seen.URL.Path) + assert.Equal(t, "0", seen.URL.Query().Get("since")) + assert.Equal(t, "mentioned,assigned", seen.URL.Query().Get("reasons")) + assert.Equal(t, "2085958499", seen.URL.Query().Get("buckets")) +} + +// The decisive check on the whole lane. A 410 is a recovery instruction, not a +// failure message: the account lane fences on the feed's epoch and names it, +// the inbox lane fences on the 30-day retention window, has no epoch at all, +// and its resume re-enters at since=0 — the earliest retained item. Flattening +// the two into one error takes the way back in away from the consumer. +func TestFeedStalePositionsKeepTheirOwnRecoveryData(t *testing.T) { + // The account lane's resume re-enters *after the epoch*, not at the + // present. since=now would skip every event still servable above the + // fence — history the caller is entitled to and asked for — so a fixture + // written that way would have this test bless a recovery instruction that + // silently loses data. The vendored schema says since= + // (model/openapi.json), and basecamp-sdk#912 makes the epoch required for + // this reason. + t.Run("the account lane names the epoch", func(t *testing.T) { + session := feedSession(t, http.StatusGone, + `{"error":"That position predates this feed's epoch.","epoch_after_id":1071900000,`+ + `"resume":"https://3.basecampapi.com/999/events.json?since=1071900000"}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"position": "stale"}) + require.True(t, isError, "a 410 is an in-band error") + + detail := feedErrorOf(t, text) + assert.Equal(t, "stale_position", detail["type"]) + assert.Equal(t, float64(http.StatusGone), detail["http_status"]) + assert.Equal(t, "req-feed-1", detail["request_id"]) + + data, ok := detail["data"].(map[string]any) + require.True(t, ok, "the 410 must carry its recovery data") + assert.Equal(t, float64(1071900000), data["epoch_after_id"]) + assert.Equal(t, "https://3.basecampapi.com/999/events.json?since=1071900000", data["resume"], + "the way back in re-enters after the epoch, not at the present") + }) + + t.Run("the inbox lane resumes at since=0 with no epoch", func(t *testing.T) { + session := feedSession(t, http.StatusGone, + `{"error":"That position predates the inbox's retention window.",`+ + `"resume":"https://3.basecampapi.com/999/inbox.json?since=0"}`, nil) + + text, isError := callFeed(t, session, "poll_inbox", map[string]any{"position": "stale"}) + require.True(t, isError) + + detail := feedErrorOf(t, text) + assert.Equal(t, "stale_position", detail["type"]) + + data, ok := detail["data"].(map[string]any) + require.True(t, ok, "the 410 must carry its recovery data") + assert.Equal(t, "https://3.basecampapi.com/999/inbox.json?since=0", data["resume"]) + assert.NotContains(t, data, "epoch_after_id", + "the inbox fences on retention, not on an epoch — a zero here would read as one") + }) +} + +// A 409 is the only refusal that says which side of the filter set moved, and +// it says it in two digests. Without both, a caller cannot tell whether to +// re-enter or to fix its own configuration. +func TestFeedFilterMismatchKeepsBothDigests(t *testing.T) { + session := feedSession(t, http.StatusConflict, + `{"error":"Positions are bound to the filter set they were minted for.",`+ + `"position_digest":"0123456789abcdef","filters_digest":"fedcba9876543210"}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"position": "aBcD"}) + require.True(t, isError) + + detail := feedErrorOf(t, text) + assert.Equal(t, "filter_mismatch", detail["type"]) + assert.Equal(t, float64(http.StatusConflict), detail["http_status"]) + data, ok := detail["data"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "0123456789abcdef", data["position_digest"]) + assert.Equal(t, "fedcba9876543210", data["filters_digest"]) +} + +// The two 400s carry opposite instructions — re-enter with since=, or fix the +// filters and stop resetting — and the remedy BC3 renders is what tells them +// apart. The reason member (bc3#13362) is read first and wins over the +// sentence; the sentence is the reading for a deploy that predates it. A 400 +// that offers neither is not dressed as either. +func TestFeedTellsTheTwo400sApart(t *testing.T) { + cases := []struct { + name string + action string + body string + want string + }{ + { + name: "a filter error that sounds internal is still a filter error", + action: "poll_events", + body: `{"error":"The types filter has an unknown type: connection timeout. Fix the filters; a position reset won't help."}`, + want: "invalid_filter", + }, + { + name: "an unrecognized position is its own answer", + action: "poll_events", + body: `{"error":"Unrecognized position. Resume with since= or since=now."}`, + want: "invalid_position", + }, + { + name: "the inbox tells them apart the same way", + action: "poll_inbox", + body: `{"error":"Unrecognized position. Resume with since= or since=now."}`, + want: "invalid_position", + }, + { + name: "a 400 offering no remedy is not given one", + action: "poll_events", + body: `{"error":"Something went wrong."}`, + want: "bad_request", + }, + { + name: "the reason names a filter error even when the sentence says otherwise", + action: "poll_events", + body: `{"error":"Unrecognized position. Resume with since=now.","reason":"invalid_filter"}`, + want: "invalid_filter", + }, + { + name: "the reason names a position error on the inbox", + action: "poll_inbox", + body: `{"error":"Something about the request.","reason":"invalid_position"}`, + want: "invalid_position", + }, + { + name: "a reason this code does not know is not guessed at through the sentence", + action: "poll_events", + body: `{"error":"Unrecognized position. Resume with since=now.","reason":"invalid_cursor_epoch"}`, + want: "bad_request", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + session := feedSession(t, http.StatusBadRequest, tc.body, nil) + + text, isError := callFeed(t, session, tc.action, map[string]any{}) + require.True(t, isError) + assert.Equal(t, tc.want, feedErrorOf(t, text)["type"]) + }) + } +} + +// The inbox is agents-only for now and refuses everyone else. Naming that +// keeps a caller from retrying a principal that will never be admitted; the +// feed lane has no principal guard, so nothing there claims it. +func TestFeedInboxNamesTheAgentsOnlyRefusal(t *testing.T) { + session := feedSession(t, http.StatusForbidden, "", nil) + text, isError := callFeed(t, session, "poll_inbox", map[string]any{}) + require.True(t, isError) + assert.Equal(t, "agents_only", feedErrorOf(t, text)["type"]) + + session = feedSession(t, http.StatusForbidden, `{"error":"access denied"}`, nil) + text, isError = callFeed(t, session, "poll_events", map[string]any{}) + require.True(t, isError) + assert.Equal(t, "request_failed", feedErrorOf(t, text)["type"]) +} + +// A refusal this server cannot recognize keeps its status and says no more +// than that — and a rate limit keeps the delay the server named, because +// retrying is the consumer's own loop to drive. +func TestFeedUnrecognizedRefusalsSayOnlyWhatTheyKnow(t *testing.T) { + session := feedSession(t, http.StatusTooManyRequests, `{"error":"rate limited - try again later"}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{}) + require.True(t, isError) + + detail := feedErrorOf(t, text) + assert.Equal(t, "request_failed", detail["type"]) + assert.Equal(t, float64(http.StatusTooManyRequests), detail["http_status"]) + assert.Equal(t, true, detail["retryable"]) + assert.NotContains(t, detail, "data", "no recovery data was offered, so none is claimed") +} + +// Both poll lanes are reads, and --read-only must keep them: a read-scoped +// consumer following the feed needs nothing else from this surface. The +// stream-ticket mint is not served in either mode — the sync script drops it by +// policy, because its result is a bearer credential this surface would hand to +// a model transcript and could not use. +func TestFeedReadOnlyServesBothPollLanes(t *testing.T) { + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("unexpected HTTP request: %s %s", r.Method, r.URL.Path) + })) + t.Cleanup(upstream.Close) + + srv, err := New(newTestAPI(upstream), Config{ReadOnly: true}) + require.NoError(t, err) + session := mcptest.Connect(t, srv.BuildMCPServer(slog.New(slog.DiscardHandler))) + + tools := mcptest.ListTools(t, session) + require.Contains(t, tools, "basecamp_eventfeed") + description := tools["basecamp_eventfeed"].Description + for _, action := range []string{"poll_events", "poll_inbox"} { + assert.Contains(t, description, action, "read-only mode must still serve %s", action) + } + assert.NotContains(t, description, "create_stream_ticket", "the mint is not served on this surface") +} + +// A filter the caller named but gave nothing usable must be refused, never +// dropped. Dropping it leaves the dimension unset, and an unset dimension on +// the wire means unfiltered — so a malformed filter would come back as the +// whole account feed, with nothing in the answer to say the filter had been +// thrown away. Narrow to a refusal, never widen to everything. +func TestFeedRefusesAFilterThatNarrowsNothing(t *testing.T) { + cases := []struct { + name string + action string + params map[string]any + }{ + {name: "buckets is a bare comma", action: "poll_events", params: map[string]any{"buckets": ","}}, + {name: "types is a bare comma", action: "poll_events", params: map[string]any{"types": ","}}, + {name: "types is empty", action: "poll_events", params: map[string]any{"types": ""}}, + {name: "buckets is empty", action: "poll_events", params: map[string]any{"buckets": ""}}, + {name: "types is only separators", action: "poll_events", params: map[string]any{"types": ",,"}}, + {name: "types is only whitespace", action: "poll_events", params: map[string]any{"types": " , "}}, + {name: "performers is a bare comma", action: "poll_events", params: map[string]any{"performers": ","}}, + {name: "actor_types is a bare comma", action: "poll_events", params: map[string]any{"actor_types": ","}}, + {name: "reasons is a bare comma", action: "poll_inbox", params: map[string]any{"reasons": ","}}, + {name: "inbox buckets is a bare comma", action: "poll_inbox", params: map[string]any{"buckets": ","}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var reached bool + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.Header().Set("Content-Type", "application/json") + // The page a widened read would have returned. If the + // refusal regresses, this is what the caller gets — which is + // why the assertion below is on the text, not merely on + // isError. + _, _ = w.Write([]byte(`{"events":[],"items":[],"position":"aBcD"}`)) + })) + 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 := callFeed(t, session, tc.action, tc.params) + + require.True(t, isError, "a filter that narrows nothing must be refused, got: %s", text) + assert.Contains(t, text, "had no usable values", + "the refusal must name the fail-open it prevented, not merely fail") + assert.False(t, reached, "a refused filter must not reach Basecamp as an unfiltered read") + }) + } +} + +// A filter that is only partly malformed keeps its components verbatim, so BC3 +// refuses it by name rather than this server guessing which component was +// meant. What must not happen is the empty component vanishing and the rest +// being served as if the caller had written it that way. +func TestFeedKeepsMalformedFilterComponentsVerbatim(t *testing.T) { + t.Run("a string filter reaches BC3 as written", func(t *testing.T) { + var seen http.Request + session := feedSession(t, http.StatusOK, `{"events":[],"position":"aBcD"}`, &seen) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"types": "message.created,,comment.created"}) + require.False(t, isError, text) + assert.Equal(t, "message.created,,comment.created", seen.URL.Query().Get("types"), + "the empty component must survive so BC3 judges the filter, not this server") + }) + + t.Run("an id filter is refused rather than silently narrowed", func(t *testing.T) { + var reached bool + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"events":[],"position":"aBcD"}`)) + })) + t.Cleanup(upstream.Close) + + srv, err := New(newTestAPI(upstream), Config{}) + require.NoError(t, err) + session := mcptest.Connect(t, srv.BuildMCPServer(slog.New(slog.DiscardHandler))) + + for _, value := range []string{",,1", "1,", "1,abc"} { + text, isError := callFeed(t, session, "poll_events", map[string]any{"buckets": value}) + require.True(t, isError, "buckets=%q must be refused, got: %s", value, text) + assert.Contains(t, text, "takes decimal ids", + "the refusal must say what buckets takes") + } + assert.False(t, reached, "a refused filter must not reach Basecamp") + }) +} + +// The filters a caller does give still travel whole — the refusals above must +// not be paid for by a dimension that silently stops working. +func TestFeedFiltersStillReachTheWire(t *testing.T) { + var seen http.Request + session := feedSession(t, http.StatusOK, `{"events":[],"position":"aBcD"}`, &seen) + + text, isError := callFeed(t, session, "poll_events", map[string]any{ + "types": " message.created , comment.created ", + "buckets": "1,2", + "creators": "3", + "performers": "self", + "exclude_performers": "4,self", + "actor_types": "agent", + }) + require.False(t, isError, text) + + query := seen.URL.Query() + assert.Equal(t, "message.created,comment.created", query.Get("types")) + assert.Equal(t, "1,2", query.Get("buckets")) + assert.Equal(t, "3", query.Get("creators")) + assert.Equal(t, "self", query.Get("performers")) + assert.Equal(t, "4,self", query.Get("exclude_performers")) + assert.Equal(t, "agent", query.Get("actor_types")) +} + +// A refusal this server makes on the caller's arguments must arrive in the +// same typed shape as the ones BC3 makes. These are the refusals a caller +// hits most; serving them as prose while every wire refusal carries a type +// would leave the common path untyped and make one contract true only of the +// rare cases. +// +// The assertion is on the shape, not on the words — an earlier test read only +// the message text, which prose satisfies just as well as a payload does, so +// it could not have seen this. +func TestFeedArgumentRefusalsAreTypedToo(t *testing.T) { + cases := []struct { + name string + action string + params map[string]any + says string + }{ + { + name: "a filter that narrows nothing", + action: "poll_events", + params: map[string]any{"types": ","}, + says: "had no usable values", + }, + { + name: "an id filter that is not decimal", + action: "poll_events", + params: map[string]any{"buckets": "abc"}, + says: "takes decimal ids", + }, + { + name: "both entry points at once", + action: "poll_events", + params: map[string]any{"since": "0", "position": "aBcD"}, + says: "two ways to enter", + }, + { + name: "both entry points at once on the inbox", + action: "poll_inbox", + params: map[string]any{"since": "0", "position": "aBcD"}, + says: "two ways to enter", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + session := feedSession(t, http.StatusOK, `{"events":[],"items":[],"position":"aBcD"}`, nil) + + text, isError := callFeed(t, session, tc.action, tc.params) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.Equal(t, "invalid_arguments", detail["type"], + "an argument refusal carries the same vocabulary as a wire refusal") + assert.Contains(t, detail["message"], tc.says) + }) + } +} + +// The 409 is a comparison of two digests. With only one of them a caller +// cannot tell which side moved, so the type is not claimed — and the missing +// digest is never written as "", which is a value a consumer would compare +// against and find unequal. Same fabricated-zero defect the epoch avoids, one +// refusal over. +func TestFeedFilterMismatchNeedsBothDigests(t *testing.T) { + for _, body := range []string{ + `{"error":"filters changed","position_digest":"abc123"}`, + `{"error":"filters changed","filters_digest":"def456"}`, + } { + session := feedSession(t, http.StatusConflict, body, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"since": "0"}) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.NotEqual(t, "filter_mismatch", detail["type"], + "a half-populated 409 is not the documented filter mismatch: %s", body) + + data, _ := detail["data"].(map[string]any) + assert.NotContains(t, data, "position_digest", "no digest is better than an empty one") + assert.NotContains(t, data, "filters_digest") + } +} + +// The inbox has no epoch. It fences on a 30-day retention window, and the +// lane is what says so — not whether the SDK's shared 410 type happened to +// carry one. A consumer handed an epoch_after_id on this lane would re-enter +// at a fence the inbox does not have. +// +// basecamp-sdk#912 splits the type in two for this reason; keyed on the lane, +// this already writes the two payloads that split produces. +func TestFeedInbox410NeverCarriesAnEpoch(t *testing.T) { + // An epoch on the wire the inbox lane must not pass on, even though the + // shared type has somewhere to put it. + const body = `{"error":"position expired","resume":"https://3.basecampapi.com/999/inbox.json?since=0","epoch_after_id":991}` + session := feedSession(t, http.StatusGone, body, nil) + + text, isError := callFeed(t, session, "poll_inbox", map[string]any{"position": "aBcD"}) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.Equal(t, "stale_position", detail["type"]) + + data, ok := detail["data"].(map[string]any) + require.True(t, ok, "the recovery data must travel: %s", text) + assert.Equal(t, "https://3.basecampapi.com/999/inbox.json?since=0", data["resume"], + "the inbox's way back in is the earliest retained item") + assert.NotContains(t, data, "epoch_after_id", + "the inbox fences on retention, not on an epoch") +} + +// agents_only is a claim that a principal will never be admitted, so it may +// only be made on the refusal that means that. A 403 BC3 gave a reason for +// came from scope or access; those are fixable, and telling a caller to stop +// trying about a repairable condition is a false promise. +func TestFeedAgentsOnlyIsClaimedOnlyOnTheBodylessRefusal(t *testing.T) { + t.Run("a bodyless 403 names the guard", func(t *testing.T) { + session := feedSession(t, http.StatusForbidden, ``, nil) + + text, isError := callFeed(t, session, "poll_inbox", nil) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.Equal(t, "agents_only", detail["type"]) + assert.Equal(t, "the event inbox is served to agent principals only", detail["message"], + "the claim must say what the guard is, not repeat the SDK's stand-in text") + }) + + t.Run("a 403 with a reason of its own is not claimed", func(t *testing.T) { + session := feedSession(t, http.StatusForbidden, `{"error":"your token lacks the read scope"}`, nil) + + text, isError := callFeed(t, session, "poll_inbox", nil) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.NotEqual(t, "agents_only", detail["type"], + "a scope refusal is fixable and must not be typed as a permanent one") + assert.Contains(t, detail["message"], "read scope", + "the server's own reason must survive") + }) +} + +// A feed 410 is a recovery only when it names the epoch it re-enters at. One +// that carries a resume and no epoch is not the documented refusal, and typing +// it stale_position would promise a fence the caller has no way to place — so +// it is served as a refusal we cannot explain. The resume does not travel +// either: re-entering without knowing where the history begins is a guess. +func TestFeedGoneWithoutAnEpochIsNotAStalePosition(t *testing.T) { + session := feedSession(t, http.StatusGone, + `{"error":"That position predates this feed's epoch.",`+ + `"resume":"https://3.basecampapi.com/999/events.json?since=1071900000"}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"position": "stale"}) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.Equal(t, "request_failed", detail["type"]) + assert.Equal(t, float64(http.StatusGone), detail["http_status"]) + assert.NotContains(t, detail, "data", "no epoch, so no recovery is claimed") +} From a2391085b43c45f08a89d01b873c1614e1fcb969 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Wed, 16 Sep 2026 22:47:45 +0200 Subject: [PATCH 2/2] Refuse the two remaining ways a feed read widens silently Adversarial review of the rebased head found no defect in the 400/409/410 mapping. It did find two more instances of the failure this file already refuses for filters: a caller asks for less, is served more, and nothing in the answer says so. A model parameter this file does not pass on was dropped. buildRequest accepts every query parameter the vendored model declares, and describe advertises them. The SDK options are typed, so dispatchFeed copies only the names it knows. The model sync and the SDK pin are separate bumps, so a lane can gain a filter before this switch learns it. The caller would name the filter, the switch would drop it, and the feed would come back unfiltered. Each lane now has a table of the parameters it passes on, and anything outside it is refused. A test pins the table to the model, so the day they disagree the build fails rather than the read widening. An empty entry point skipped history. The SDK reads an empty since or position as no entry point, and no entry point is the present. A consumer whose stored position came back empty would enter at now and never see what it had not yet read. A named and empty entry point is now refused, as a named and empty filter already was. The same pass closed three gaps in the tests and one dishonest branch: - A 400 whose body is not the feed's own, with no error member, read a recovery out of whatever text the SDK fell back to. BC3's feed did not write that text, so it is bad_request now. - A page's contents were never asserted. The request-side tests would have passed against an encoder returning {}. Both lanes' pages are now read back. - A 409 on the inbox, a feed 410 with an epoch and no resume, and an inbox 410 with no resume had no test. Each lands on its status with no recovery claimed. - The comment on the inbox-epoch test still described the pre-912 shared type. Each new test fails against the code it guards, with that one fix reverted: parameter refusal removed: --- FAIL: TestFeedRefusesAParameterItWouldDrop a filter this server cannot pass on must be refused, got: { model gains actor_types, table does not: --- FAIL: TestFeedLaneParamsMatchTheModel PollEvents: the model's query parameters and the ones handed to the SDK must be the same set empty entry-point refusal removed: --- FAIL: TestFeedRefusesAnEmptyEntryPoint/poll_events_position an empty position must be refused, got: { remedy read out of a foreign 400: --- FAIL: TestFeedDoesNotReadARemedyOutOfAForeign400 encoder returns {}: --- FAIL: TestFeedServesThePageItFetched/feed the page's events must travel: {} --- internal/mcpserver/feed.go | 44 ++++++- internal/mcpserver/feed_test.go | 203 ++++++++++++++++++++++++++++++-- 2 files changed, 235 insertions(+), 12 deletions(-) diff --git a/internal/mcpserver/feed.go b/internal/mcpserver/feed.go index 7c749bd98..2ffdcfa17 100644 --- a/internal/mcpserver/feed.go +++ b/internal/mcpserver/feed.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "net/url" + "slices" "strconv" "strings" @@ -113,6 +114,22 @@ type feedErrorPayload struct { Error feedError `json:"error"` } +// feedLaneParams are the query parameters each lane hands to the SDK, which is +// every one this file knows how to pass on. +// +// The request is assembled by buildRequest from the vendored model, and the +// SDK's options are typed, so the two can drift: a model sync can declare a +// filter before this file learns to copy it across. A parameter the model +// accepts and this file drops is the widest fail-open on the lane — the caller +// asks to narrow and is served the unfiltered feed, and describe even told them +// the filter existed. So a parameter outside this table is refused, and +// TestFeedLaneParamsMatchTheModel fails the build the day the model and this +// table disagree. +var feedLaneParams = map[string][]string{ + opPollEvents: {"since", "position", "types", "buckets", "creators", "performers", "exclude_performers", "actor_types"}, + opPollInbox: {"since", "position", "reasons", "types", "buckets"}, +} + // isFeedOperation reports whether the operation runs through EventFeedService. func isFeedOperation(id string) bool { switch id { @@ -147,6 +164,24 @@ func (d dispatcher) dispatchFeed(ctx context.Context, op *catalog.Operation, par // everything. filters := feedFilters{params: params, query: query} + for name := range query { + if !slices.Contains(feedLaneParams[op.ID], name) { + return feedUsageResult("%s is not yet passed to the event feed by this server; it is refused rather than dropped, "+ + "because a dropped filter would serve the lane unfiltered", name) + } + } + + // An entry point that is named and empty is refused for the same reason a + // filter is. The SDK reads an empty since or position as no entry point, + // and no entry point means the present: a consumer whose stored position + // came back empty would silently skip every event it had not yet read. + for _, name := range []string{"since", "position"} { + if _, named := params[name]; named && query.Get(name) == "" { + return feedUsageResult("%s was passed empty, which the feed reads as entering at the present and skipping history; "+ + "omit it to enter at the present on purpose", name) + } + } + if since, position := query.Get("since"), query.Get("position"); since != "" && position != "" { return feedUsageResult("since and position are two ways to enter the lane; pass one, not both") } @@ -424,9 +459,12 @@ func feedRemedyErrorType(message string) string { func feedErrorType(operation string, err *basecamp.Error) string { switch err.HTTPStatus { case 400: - // A 400 whose body the SDK could not read as the feed's own — no - // error member at all. Still the feed refusing the request. - return feedRemedyErrorType(err.Message) + // A 400 whose body the SDK could not read as the feed's own: no error + // member at all. The message here is the SDK's placeholder or some + // other member it fell back to, not BC3's feed refusal, so no remedy + // is read out of it — that would be claiming a recovery from text the + // feed did not write. + return feedErrBadRequest case 403: // The inbox is agents-only for now and refuses everyone else with a // bodyless 403. Naming it keeps a caller from retrying a principal diff --git a/internal/mcpserver/feed_test.go b/internal/mcpserver/feed_test.go index 691b31a33..de1ed5eb7 100644 --- a/internal/mcpserver/feed_test.go +++ b/internal/mcpserver/feed_test.go @@ -5,6 +5,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "slices" "testing" "github.com/stretchr/testify/assert" @@ -487,16 +488,14 @@ func TestFeedFilterMismatchNeedsBothDigests(t *testing.T) { } } -// The inbox has no epoch. It fences on a 30-day retention window, and the -// lane is what says so — not whether the SDK's shared 410 type happened to -// carry one. A consumer handed an epoch_after_id on this lane would re-enter -// at a fence the inbox does not have. -// -// basecamp-sdk#912 splits the type in two for this reason; keyed on the lane, -// this already writes the two payloads that split produces. +// The inbox has no epoch. It fences on a 30-day retention window, and its 410 +// is its own type in the SDK, with no field an epoch could ride in. An epoch +// that turns up on the wire anyway must not reach the caller: a consumer +// handed epoch_after_id on this lane would re-enter at a fence the inbox does +// not have. The SDK drops it today; this pins that nothing between the SDK +// and the payload puts one back. func TestFeedInbox410NeverCarriesAnEpoch(t *testing.T) { - // An epoch on the wire the inbox lane must not pass on, even though the - // shared type has somewhere to put it. + // An epoch on the wire the inbox lane must not pass on. const body = `{"error":"position expired","resume":"https://3.basecampapi.com/999/inbox.json?since=0","epoch_after_id":991}` session := feedSession(t, http.StatusGone, body, nil) @@ -563,3 +562,189 @@ func TestFeedGoneWithoutAnEpochIsNotAStalePosition(t *testing.T) { assert.Equal(t, float64(http.StatusGone), detail["http_status"]) assert.NotContains(t, detail, "data", "no epoch, so no recovery is claimed") } + +// The lane tables name every parameter this server hands to the feed, and the +// model names every parameter describe advertises and buildRequest accepts. +// Where the two disagree, a filter a caller was told exists would be refused +// at call time — or, before the refusal existed, silently dropped. This fails +// the day a model sync gives a lane a parameter the dispatcher does not pass. +func TestFeedLaneParamsMatchTheModel(t *testing.T) { + cat := loadForTest(t) + seen := map[string]bool{} + for _, d := range cat.Domains { + for _, op := range d.Operations { + if !isFeedOperation(op.ID) { + continue + } + seen[op.ID] = true + var model []string + for _, p := range op.Params { + if p.In == "query" { + model = append(model, p.Name) + } + } + assert.ElementsMatch(t, model, feedLaneParams[op.ID], + "%s: the model's query parameters and the ones handed to the SDK must be the same set", op.ID) + } + } + assert.True(t, seen[opPollEvents] && seen[opPollInbox], "both poll lanes must be in the catalog") +} + +// A parameter the model accepts and this server does not pass to the SDK is +// refused, not dropped. Dropped, the caller who asked to narrow is served the +// unfiltered lane. The table is shrunk here to stand in for a model sync that +// got ahead of the dispatcher. +func TestFeedRefusesAParameterItWouldDrop(t *testing.T) { + original := feedLaneParams[opPollEvents] + feedLaneParams[opPollEvents] = slices.DeleteFunc(slices.Clone(original), func(name string) bool { return name == "creators" }) + t.Cleanup(func() { feedLaneParams[opPollEvents] = original }) + + var reached bool + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"events":[],"position":"aBcD"}`)) + })) + 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 := callFeed(t, session, "poll_events", map[string]any{"creators": "1049715914"}) + require.True(t, isError, "a filter this server cannot pass on must be refused, got: %s", text) + detail := feedErrorOf(t, text) + assert.Equal(t, "invalid_arguments", detail["type"]) + assert.Contains(t, detail["message"], "creators") + assert.False(t, reached, "a dropped filter must not reach Basecamp as an unfiltered read") +} + +// An entry point passed empty is refused. The SDK reads it as no entry point, +// which is the present, so a consumer whose stored position came back empty +// would skip everything it had not yet read and be told nothing. +func TestFeedRefusesAnEmptyEntryPoint(t *testing.T) { + for _, tc := range []struct{ action, name string }{ + {"poll_events", "position"}, + {"poll_events", "since"}, + {"poll_inbox", "position"}, + {"poll_inbox", "since"}, + } { + t.Run(tc.action+" "+tc.name, func(t *testing.T) { + var reached bool + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"events":[],"items":[],"position":"aBcD"}`)) + })) + 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 := callFeed(t, session, tc.action, map[string]any{tc.name: ""}) + require.True(t, isError, "an empty %s must be refused, got: %s", tc.name, text) + assert.Equal(t, "invalid_arguments", feedErrorOf(t, text)["type"]) + assert.False(t, reached, "an empty entry point must not reach Basecamp as a read from the present") + }) + } +} + +// A 400 whose body is not the feed's own is not given the feed's recovery. +// The SDK falls back to a `message` member when there is no `error`, so text +// that happens to read like BC3's remedy can arrive here from a body BC3's +// feed did not write; reading a recovery out of it would be a guess. +func TestFeedDoesNotReadARemedyOutOfAForeign400(t *testing.T) { + session := feedSession(t, http.StatusBadRequest, `{"message":"Unrecognized position. Resume with since=now."}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"position": "aBcD"}) + require.True(t, isError, text) + assert.Equal(t, "bad_request", feedErrorOf(t, text)["type"]) +} + +// The refusals the SDK leaves untyped land somewhere honest: their status, no +// recovery claimed, and no recovery data invented. +func TestFeedUntypedRefusalsClaimNoRecovery(t *testing.T) { + cases := []struct { + name string + action string + status int + body string + }{ + {"a 409 on the inbox with one digest", "poll_inbox", http.StatusConflict, `{"error":"filters changed","filters_digest":"def456"}`}, + {"a feed 410 with an epoch and no resume", "poll_events", http.StatusGone, `{"error":"gone","epoch_after_id":1071900000}`}, + {"an inbox 410 with no resume", "poll_inbox", http.StatusGone, `{"error":"gone"}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + session := feedSession(t, tc.status, tc.body, nil) + + text, isError := callFeed(t, session, tc.action, map[string]any{"position": "aBcD"}) + require.True(t, isError, text) + + detail := feedErrorOf(t, text) + assert.Equal(t, "request_failed", detail["type"]) + assert.Equal(t, float64(tc.status), detail["http_status"]) + assert.NotContains(t, detail, "data", "no recovery was offered, so none is claimed") + }) + } + t.Run("a 409 on the inbox with both digests is a filter mismatch there too", func(t *testing.T) { + session := feedSession(t, http.StatusConflict, + `{"error":"filters changed","position_digest":"abc123","filters_digest":"def456"}`, nil) + + text, isError := callFeed(t, session, "poll_inbox", map[string]any{"position": "aBcD"}) + require.True(t, isError, text) + detail := feedErrorOf(t, text) + assert.Equal(t, "filter_mismatch", detail["type"]) + assert.Equal(t, map[string]any{"position_digest": "abc123", "filters_digest": "def456"}, detail["data"]) + }) +} + +// A page comes back with what it carried: the rows, the durable position, and +// the continuation. The request-side tests would pass against an encoder that +// returned {}, so the answer is read here. +func TestFeedServesThePageItFetched(t *testing.T) { + t.Run("feed", func(t *testing.T) { + session := feedSession(t, http.StatusOK, `{ + "events":[{"id":11,"kind":"message_created","action":"created","event_type":"message.created", + "bucket_id":2085958499,"creator_id":1049715914,"performed_by_id":52007412, + "recording_id":9007199254,"created_at":"2026-09-16T09:00:00Z","details":{"column_id":7}}], + "position":"cG9zOjEx", + "next":"https://3.basecampapi.com/999/events.json?position=cG9zOjEx"}`, nil) + + text, isError := callFeed(t, session, "poll_events", map[string]any{"since": "0"}) + require.False(t, isError, text) + + var page map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &page), text) + assert.Equal(t, "cG9zOjEx", page["position"]) + assert.Equal(t, "https://3.basecampapi.com/999/events.json?position=cG9zOjEx", page["next"]) + events, ok := page["events"].([]any) + require.True(t, ok && len(events) == 1, "the page's events must travel: %s", text) + event := events[0].(map[string]any) + assert.Equal(t, float64(11), event["id"]) + assert.Equal(t, "message.created", event["event_type"]) + assert.Equal(t, float64(52007412), event["performed_by_id"]) + assert.Equal(t, float64(9007199254), event["recording_id"]) + assert.Equal(t, map[string]any{"column_id": float64(7)}, event["details"]) + }) + + t.Run("inbox", func(t *testing.T) { + session := feedSession(t, http.StatusOK, `{ + "items":[{"addressing_id":801,"reason":"mentioned","addressed_at":"2026-09-16T09:00:00Z", + "event":{"id":11,"event_type":"comment.created","bucket_id":2085958499,"creator_id":1049715914, + "performed_by_id":null,"recording_id":9007199254,"created_at":"2026-09-16T09:00:00Z"}}], + "position":"aW5ib3g6ODAx"}`, nil) + + text, isError := callFeed(t, session, "poll_inbox", map[string]any{"since": "0"}) + require.False(t, isError, text) + + var page map[string]any + require.NoError(t, json.Unmarshal([]byte(text), &page), text) + assert.Equal(t, "aW5ib3g6ODAx", page["position"]) + items, ok := page["items"].([]any) + require.True(t, ok && len(items) == 1, "the page's items must travel: %s", text) + item := items[0].(map[string]any) + assert.Equal(t, float64(801), item["addressing_id"]) + assert.Equal(t, "mentioned", item["reason"]) + assert.Equal(t, float64(11), item["event"].(map[string]any)["id"]) + }) +}