Serve the account event feed over MCP - #725
Merged
Merged
Conversation
github-actions
Bot
added
commands
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The SDK sync is incomplete, the Nix vendor hash is stale, and the feed capability contract can advertise unusable actions.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds account event-feed polling, agent inbox polling, and stream-ticket creation to the MCP server, preserving structured recovery errors.
Changes:
- Adds the
basecamp_eventsMCP domain and typed dispatch. - Preserves 400/409/410 recovery semantics.
- Resyncs the SDK model and expands MCP tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
internal/version/sdk-provenance.json |
Updates SDK provenance. |
internal/mcpserver/testdata/catalog_snapshot.txt |
Records the events domain. |
internal/mcpserver/server_test.go |
Updates tool count. |
internal/mcpserver/model/PROVENANCE.json |
Updates model provenance. |
internal/mcpserver/model/openapi.json |
Adds feed operations and schemas. |
internal/mcpserver/model/behavior-model.json |
Adds feed behavior metadata. |
internal/mcpserver/feed.go |
Implements typed feed dispatch and errors. |
internal/mcpserver/feed_test.go |
Tests requests, errors, and read-only behavior. |
internal/mcpserver/domains.go |
Registers the events domain. |
internal/mcpserver/dispatch.go |
Routes feed operations through the SDK service. |
internal/mcpserver/catalog.go |
Marks stream-ticket creation readable. |
internal/mcpserver/catalog_test.go |
Updates catalog expectations. |
internal/commands/mcp_test.go |
Updates command tool count. |
go.sum |
Updates SDK checksums. |
go.mod |
Bumps the Basecamp SDK. |
Review details
- Files reviewed: 14/15 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Malformed filters can widen into unrestricted feed requests, and the SDK bump omits required CLI coverage artifacts.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/mcpserver/model/openapi.json:33874
- The PR description says the model resync brings only the three EventFeed operations, but this also changes five existing
Personfields (email_address,title,bio,tagline, andlocation) from string-only to nullable. Please either confirm and document this additional compatibility change in the PR scope or resync from a revision that contains only the intended feed model changes.
go.mod:9
- This SDK bump introduces
EventFeed.PollEvents,PollInbox, andCreateStreamTicket, but there are no corresponding CLI commands, command catalog/root registrations, command tests, orAPI-COVERAGE.mdentries. The repository's SDK completeness bar requires each of these for every new SDK service method (AGENTS.md:160-164), so please add the missing CLI surface and coverage tracking.
github.com/basecamp/basecamp-sdk/go v0.18.1-0.20260916110029-482c7f6ef37e
- Files reviewed: 15/16 changed files
- Comments generated: 1
- Review effort level: Balanced
jorgemanrubia
force-pushed
the
mcp-event-feed
branch
from
September 16, 2026 12:20
d785d21 to
ce03d88
Compare
jorgemanrubia
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
CLI continuations and errors currently discard filter-bound and recovery information needed to resume feed consumption.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
internal/commands/feed.go:206
- This also flattens the inbox's typed 409/410 errors through
convertSDKError. In particular, a retention-expired position loses theresumeURL that re-enters atsince=0, so the documented “way back in” is unavailable to CLI callers. Preserve the typed feed recovery fields before invoking the generic converter.
if err != nil {
return convertSDKError(err)
}
- Files reviewed: 22/23 changed files
- Comments generated: 2
- Review effort level: Balanced
jorgemanrubia
changed the title
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The account-feed 410 test currently validates a resume URL that skips recoverable history instead of re-entering at the reported epoch.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/mcpserver/feed_test.go:138
- This fixture contradicts the account-lane 410 contract:
since=nowskips all still-servable events, while the vendored schema atinternal/mcpserver/model/openapi.json:31396requires re-entry atsince=<epoch_after_id>. Because the assertion repeats the same invalid URL, the test currently blesses a recovery instruction that loses history; use the epoch value in both places.
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new credential-minting surface and recovery-error compatibility contract warrant final human validation.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
internal/mcpserver/domains.go:94
- Adding this domain leaves the public MCP documentation stale:
README.md:320-325still says there are sixteen tools and omitsbasecamp_eventsfrom the complete list. Please update that count and list so users can discover and select the neweventsdomain.
internal/mcpserver/feed.go:55 - This still claims “one contract, two servers,” although the PR description now explicitly documents observable differences in error data, 403 classification, and 5xx handling. Narrow the comment to the shared error vocabulary so it does not reintroduce the overclaim the description corrected.
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Member
Author
|
🤖 Good catch, and fixed in 9e80291. You are right that the fixture contradicted the lane. Both places now use the epoch, and the assertion says why. Nothing in the dispatcher changed — it passes Worth noting this is the direction basecamp-sdk#912 is formalising: it makes |
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
A human should confirm the deliberate policy of exposing credential-minting as read-only; only minor documentation corrections were otherwise found.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/mcpserver/feed.go:55
- This “one contract, two servers” claim reintroduces the overstatement the PR description explicitly retracts. The servers share recovery vocabulary, but deliberately differ in preserved
data, 403 detection, 5xx handling, and action names; describe only the shared vocabulary here so future maintainers do not assume full interchangeability.
This issue also appears on line 453 of the same file.
internal/mcpserver/feed.go:457
- The same full-contract overclaim remains here even though the PR description narrows the promise to shared error vocabulary. State the concrete reason for typing local argument errors without implying that every aspect of the two servers' contracts is identical.
// 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.
- Files reviewed: 15/16 changed files
- Comments generated: 0 new
- Review effort level: Balanced
#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.
jorgemanrubia
force-pushed
the
mcp-event-feed
branch
from
September 16, 2026 20:39
9e80291 to
78898a4
Compare
github-actions
Bot
removed
commands
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: {}
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 16, 2026 20:47
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#726 put the account event feed on
basecamp mcp:poll_eventsandpoll_inboxin theeventfeeddomain, through the mechanical dispatch every other operation uses. That dispatch turns a failed call into its message and nothing more, because the SDK's canonical 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, and a 400 says whether to re-enter or to fix the filters. A consumer that gets those as prose has lost the lane it was following.
This PR sends the two poll lanes through
EventFeedServiceinstead. Its typed errors carry those members back out, and they come back as typed in-band errors. The request is still built from the vendored model, so parameters, validation and thedescribeschema stay the model's. The diff is three files ininternal/mcpserver/.This is originally tracked in 11 Feed operations in both MCP servers. The companion change in the standalone server is basecamp-mcp-server#162.
Aligned with basecamp-sdk#912, which main now pins
basecamp-sdk#912 split the refusals by lane, and each gets its own arm here:
*FeedPositionGoneErrorstale_positionwithresumeandepoch_after_id*InboxPositionGoneErrorstale_positionwithresumeonly. The inbox fences on 30-day retention, has no epoch, and re-enters atsince=0*FeedRequestErrorinvalid_position/invalid_filterfromreason. The remedy sentence is read only for a deploy that predates the reason. An unknown reason isbad_request, not guessed at*FeedFilterMismatchErrorfilter_mismatch, only when both digests are presentagents_only. A 403 with a reason (scope, access) is not claimedWorth reviewing: a missing arm fails silently. Every one of these types unwraps to the canonical error, so leaving an arm out still compiles. The refusal would degrade to
request_failedand drop the member a consumer recovers from. The tests drive each shape through the real SDK, so removing the inbox arm turns two tests red rather than passing quietly.Also carried from review of the earlier revision
buckets=,,types=,) is refused, not dropped. Dropped, it left the dimension unset, which on the wire is no filter, so a malformed filter returned the whole account feed.invalid_argumentsrather than prose: a filter that narrows nothing,sincetogether withposition, a parameter the model rejects.EventFeed()is part of the required dispatcher API, so a client that cannot reach the feed cannot build a server that advertises it.Two more ways a read could widen silently, now refused
buildRequestanddescribeaccept them. The SDK's options are typed, so only names this file knows are passed on. A model sync can add a filter before this file learns it, and the caller would then get the unfiltered lane. Each lane now has a table of the parameters it passes. Anything outside the table is refused, and a test pins the table to the model.sinceorposition. The SDK reads an empty value as no entry point, which means the present. A consumer whose stored position came back empty would silently skip history. That is refused now, the same way an empty filter is.A change from #726 on the success path
A page now comes back through the SDK's modeled types, not as BC3's bytes, so it differs from what #726's generic dispatch returned:
"details": nulland"next": nullare omitted rather than written..120becomes.12).#726 is not in a release, so no released caller depends on the earlier shape. A test now reads a page back on both lanes; before, only the request side was asserted.
Not served: the stream-ticket mint
It 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 on this surface could open the WebSocket it is for.
An earlier revision of this PR served the mint read-only, to match the standalone server, which does serve it. That is not carried forward. Reversing a credential-exposure decision is not something to fold into a rebase. If the two servers should agree on it, that is its own decision.
How close this is to the standalone server
The type strings, the envelope and the recovery members are the standalone server's. Three differences remain, all forced by reading the feed through the SDK's typed errors rather than the wire:
datacarries the members named here rather than BC3's whole body. A member neither server names yet reaches the standalone server's callers and not these.agents_onlyis recognised by the SDK's substitute text for a bodyless 403. A 403 whose body said exactly "access denied" would be mis-claimed.The first two close the day the SDK carries the raw error body.
History
This PR originally carried its own SDK bump, model resync, Nix
vendorHashand, briefly, CLI commands. #726 landed all of those, so the branch was rebuilt on main with only the MCP error path.