Give cursor-paginated operations no page parameter - #732
Merged
Merged
Conversation
basecamp-sdk#914 declares the event feed's two poll lanes, PollEvents and PollInbox, as cursor-style pagination. They page by an opaque position the response body carries. Bumping the SDK to 12cbad5b (the revision #729 needs for eventfeed.NewLive) and re-syncing the vendored model brings that declaration in, and synthesizePageParams then gave both operations a Link-style `page` parameter. For those two operations a page number means nothing. BC3 ignores it, so a caller passing page=2 is served the first page again, with nothing in the answer to say so. describe would still advertise the parameter as if it did something. Three tests caught it: TestFeedLaneParamsMatchTheModel, TestCatalogPaginatedActionsTakePage (61 expected, 63 found) and TestCatalogSnapshot. `page` is now synthesized only for Link-style operations. The decision reads the style the behavior model declares, not any operation's name, because the toolkit catalog records only whether an operation is paginated, not how. Cursor-style operations keep exactly the parameters the model gives them: since and position on the feed lanes. A paginated operation with a style the loader does not recognize now stops the load. Either default would be a silent guess: a page parameter that does nothing, or a listing whose later pages cannot be reached. So a model sync that brings a new style has to be decided about here. No other operation gained or lost `page`. The set of operations declaring it is identical, name for name, to main's (62), and differs from the unfixed re-sync only by PollEvents and PollInbox. The snapshot changes only by marking the two feed lanes "(RO, paginated)", which the model now says they are. The paginated-operation test now counts Link-style operations (61) and says why the model's total is 63. Two new tests, each red against the code they guard: before this change (cursor operations given page): --- FAIL: TestCatalogCursorPaginatedActionsTakeNoPage cursor-style operation "PollEvents" must not take a Link-style page parameter cursor-style operation "PollInbox" must not take a Link-style page parameter undecided style defaulted instead of refused: --- FAIL: TestCatalogRefusesAnUndecidedPaginationStyle an undecided style must stop the load The SDK bump, model re-sync and Nix vendorHash come from the repo's own tooling: scripts/bump-sdk.sh, scripts/sync-mcp-model.sh against a basecamp-sdk checkout at 12cbad5b, and make update-nix-hash, which verified the Docker Nix build.
github-actions
Bot
added
sdk
Contributor
There was a problem hiding this comment.
🟢 Approved
The implementation matches the model contract and includes focused regression coverage for all affected pagination styles.
Pull request overview
Updates the MCP catalog to distinguish Link-style and cursor-style pagination after syncing the latest SDK model.
Changes:
- Synthesizes
pageonly for Link-style operations and rejects unknown styles. - Marks event-feed polling as cursor-paginated without exposing
page. - Updates SDK provenance, models, tests, snapshots, and Nix dependency hash.
[!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 |
|---|---|
go.mod |
Bumps the Basecamp SDK. |
go.sum |
Updates SDK checksums. |
nix/package.nix |
Refreshes the Go vendor hash. |
internal/version/sdk-provenance.json |
Records the new SDK revision. |
internal/mcpserver/model/PROVENANCE.json |
Records the synchronized model source. |
internal/mcpserver/model/openapi.json |
Declares cursor pagination for feed lanes. |
internal/mcpserver/model/behavior-model.json |
Adds cursor pagination metadata. |
internal/mcpserver/catalog.go |
Applies pagination behavior by declared style. |
internal/mcpserver/catalog_test.go |
Covers Link, cursor, and unknown styles. |
internal/mcpserver/testdata/catalog_snapshot.txt |
Updates feed pagination labels. |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 0
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
basecamp-sdk#919 (bounded connector capacities) merged after the first bump here, and a release is being cut. No release tag exists yet, and the latest is go/v0.18.0. Waiting for one would hold up #729, so this pins the newest basecamp-sdk main, 4523eac, which contains both #919 and 12cbad5b. #729 and the connector work then inherit #919 through this PR. The vendored model is byte-identical between 12cbad5b and 4523eac. The re-sync moves only provenance, so the pagination change in the previous commit is unaffected. The Nix vendorHash is refreshed and verified through the Docker Nix build.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 16, 2026 21:08
Contributor
There was a problem hiding this comment.
🟢 Approved
The model, catalog behavior, dependency provenance, Nix hash, and regression coverage are consistent with the cursor-pagination contract.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced (auto)
Note
Copilot is running an experiment and ran this review at Balanced.
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.
Rebasing #729 needs basecamp-sdk
12cbad5bforeventfeed.NewLive.SDK pin: this PR pins basecamp-sdk main at
4523eac, not a release. That revision contains12cbad5band basecamp-sdk#919 (bounded connector capacities). A release carrying #919 is being cut, but no tag existed when this was pushed (the latest isgo/v0.18.0), and waiting would hold up #729. Moving to the tag is a one-line bump once it exists. The vendored model is byte-identical between12cbad5band4523eac. Every SDK revision that has it also carries basecamp-sdk#914. That PR declares the event feed's two poll lanes,PollEventsandPollInbox, as cursor-style pagination, meaning they page by an opaque position the response body carries.Re-syncing the vendored model brings that declaration in.
synthesizePageParamsthen gives both operations a Link-stylepageparameter.A page number means nothing to those two. BC3 ignores it, so a caller passing
page=2gets the first page again and nothing in the answer says so.describewould still advertise the parameter as working. Three tests go red:TestFeedLaneParamsMatchTheModelTestCatalogPaginatedActionsTakePage(61 expected, 63 found)TestCatalogSnapshotThis PR carries the bump and re-sync that expose the problem, plus the fix.
The fix
pageis now synthesized only for Link-style operations. The decision reads the style the behavior model declares, not operation names, because the toolkit catalog records only whether an operation is paginated, not how. Cursor-style operations keep exactly the parameters the model gives them, which on the feed lanes aresinceandposition.A paginated operation whose style the loader doesn't recognize stops the load. Either default would be a silent guess: a page parameter that does nothing, or a listing whose later pages can't be reached. So a future model sync that brings a new style forces a decision here.
What it touches
page. I dumped the set of operations declaring it: identical, name for name, to main's (62), and different from the unfixed re-sync only byPollEventsandPollInbox.(RO, paginated), which the model now says they are. Parameters aren't part of the snapshot.TestCatalogPaginatedActionsTakePagenow counts Link-style operations (61), and its comment explains why the model's total is 63.Tests
Each new test fails on the code it guards when the fix is reverted:
TestCatalogCursorPaginatedActionsTakeNoPagefails before this change:cursor-style operation "PollEvents" must not take a Link-style page parameter, and the same forPollInbox.TestCatalogRefusesAnUndecidedPaginationStylefails if an unknown style is defaulted instead of refused.Mechanics
The SDK bump, model re-sync and Nix
vendorHashcome from the repo's own tooling:scripts/bump-sdk.sh 4523eac74cfascripts/sync-mcp-model.shagainst a basecamp-sdk checkout (first at12cbad5b, then at4523eac; the model files did not change between the two), which addedpaginationto the two operations and nothing else in the behavior modelmake update-nix-hash, which verified the Docker Nix build