Close the malformed-trait pass in the two generators that still had it - #910
Merged
Merged
Conversation
"All six generators refuse an unrecognised style by name" was true for a
well-formed trait and false for a malformed one, in two of the six. Both
holes are the same shape as the one this branch already closed in Swift,
which is the reason to distrust the claim rather than the code: the fix
was applied where a reviewer pointed, not everywhere it belonged.
Kotlin gated the refusal on `as? JsonObject`, which answers null for a
present-but-non-object trait. A bare `"x-basecamp-pagination": "page"`
skipped the refusal entirely and generated an unpaginated method for a
spec that plainly said "page" — the exact silent pass the refusal exists
to close. Presence is now tested before the cast, as Swift does, and a
non-primitive `style` is refused by name instead of throwing the cast
error `jsonPrimitive` would.
TypeScript gated it on truthiness, so `false`, `0` and `""` were exempt
and read as unpaginated. It now narrows through `unknown`: absent and a
literal null are unpaginated, anything else is declared, and a declared
trait that is not an object has no style to accept. The key is read off
the same narrowed value, so it cannot outlive the style check.
Neither generator changes its answer for any operation in the spec: all
61 paginated operations carry a well-formed `{"style": "link"}`, and both
drift gates report no drift.
Tests in both languages, and they fail when the guard is reverted — I
checked, because the last test on this branch that re-implemented the
predicate it was guarding stayed green when the guard was deleted.
`parseOperation` is exported from the TypeScript generator to reach it;
the Kotlin test drives OperationParser against an in-memory spec.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
typescript
There was a problem hiding this comment.
🟡 Changes recommended
One Kotlin regression test cannot distinguish the intended validation error from the old cast failure.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Hardens Kotlin and TypeScript pagination-trait parsing so malformed declarations fail generation instead of silently disabling pagination.
[!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.
Changes:
- Explicitly distinguishes absent/null traits from malformed declarations.
- Safely validates pagination styles and keys.
- Adds parser-level regression coverage.
File summaries
| File | Description |
|---|---|
typescript/scripts/generate-services.ts |
Hardens trait parsing and exports parser types. |
typescript/tests/generator/pagination-style.test.ts |
Covers supported and malformed traits. |
kotlin/generator/src/main/kotlin/com/basecamp/sdk/generator/OperationParser.kt |
Refuses malformed traits safely. |
kotlin/generator/src/test/kotlin/com/basecamp/sdk/generator/PaginationStyleTest.kt |
Adds Kotlin parser regression tests. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Copilot, and it is the same defect this PR exists to close, one level up. `styleFailure` answers the exception's message, and the named-style tests assert against it -- but the five malformed-trait tests threw the answer away and checked only that an IllegalArgumentException came out. `jsonPrimitive` throws that same type, so `a non-primitive style is refused by name rather than throwing a cast error` could not tell a refusal from the cast error it is named after: revert the parser and it stayed green. All five now assert the message. Proven by reverting the parser to its pre-fix shape and re-running: five failures, and the fifth lands on the message assertion rather than on the exception type, which is the distinction that was missing.
Member
Author
|
🤖 Right, and it is this PR's own defect one level up — a test that cannot fail for the reason it is named after.
All five now assert the message, through a helper whose comment says why the type alone is not enough. Proven rather than asserted. I reverted The line numbers are the point. The first four fail at Restored, green again: |
jorgemanrubia
added a commit
that referenced
this pull request
Sep 16, 2026
PR 898 modelled the event feed's paging as an absence: PollEvents and PollInbox carried no @basecampPagination trait, because the only thing the trait could mean was the Link-following walk, and following that walk would flatten the pages and swallow the per-page `position` a consumer resumes from. Declaring nothing was the only way to not declare the wrong thing. PR 905 gave the trait a second mode — style: "cursor", which declares the paging and generates no walk — and #910 closed the two malformed-trait passes it left. So the feed can now say what it does. Both poll lanes declare it: `key` names the envelope member holding the page (`events`, `items`) and `maxPageSize` is bc3's PAGE_SIZE of 100 on both Event::Feed and Event::Inbox. CreateStreamTicket declares nothing — it mints one ticket per call and pages in no sense. No generated method changes. All six service generators read the cursor style as "no walk", which is what the absent trait produced, so the only diff in the SDKs is the doc comment. What does change is the catalogue: openapi.json, behavior-model.json, the Ruby and TypeScript metadata, and the Rust route table, which now says Cursor rather than the None that meant "a single answer". The route table is where it is pinned. guarantees.rs asserts the two lanes are the cursor set, with their keys, and that CreateStreamTicket still pages not at all; the Link-paginated count stays 61, which is the assertion that the declaration did not quietly enrol them in the walk. The TypeScript test serving a Link: rel="next" header is a tripwire against a future style flip, not a proof of this change — an undeclared operation makes one request too. It goes from 1 request to 10,000 if the style is changed to "link".
jorgemanrubia
added a commit
that referenced
this pull request
Sep 16, 2026
* Say the feed pages by cursor, now that the trait can say it PR 898 modelled the event feed's paging as an absence: PollEvents and PollInbox carried no @basecampPagination trait, because the only thing the trait could mean was the Link-following walk, and following that walk would flatten the pages and swallow the per-page `position` a consumer resumes from. Declaring nothing was the only way to not declare the wrong thing. PR 905 gave the trait a second mode — style: "cursor", which declares the paging and generates no walk — and #910 closed the two malformed-trait passes it left. So the feed can now say what it does. Both poll lanes declare it: `key` names the envelope member holding the page (`events`, `items`) and `maxPageSize` is bc3's PAGE_SIZE of 100 on both Event::Feed and Event::Inbox. CreateStreamTicket declares nothing — it mints one ticket per call and pages in no sense. No generated method changes. All six service generators read the cursor style as "no walk", which is what the absent trait produced, so the only diff in the SDKs is the doc comment. What does change is the catalogue: openapi.json, behavior-model.json, the Ruby and TypeScript metadata, and the Rust route table, which now says Cursor rather than the None that meant "a single answer". The route table is where it is pinned. guarantees.rs asserts the two lanes are the cursor set, with their keys, and that CreateStreamTicket still pages not at all; the Link-paginated count stays 61, which is the assertion that the declaration did not quietly enrol them in the walk. The TypeScript test serving a Link: rel="next" header is a tripwire against a future style flip, not a proof of this change — an undeclared operation makes one request too. It goes from 1 request to 10,000 if the style is changed to "link". * Give the inbox the cursor tripwire the feed already had Adversarial review of the commit before this one, over two passes. The runtime tripwire was on the wrong lane. The TypeScript test serving a Link: rel="next" header duplicated a conformance case that already did that for PollEvents in all seven SDKs, asserting one request. PollInbox had nothing: its envelope case serves no `next` and no Link header, so the cursor style declared on the inbox had no runtime assertion anywhere. The TypeScript test is gone and the inbox gets its own conformance case, serving both continuation signals and asserting a single request with `next` decoded. One fixture covers seven languages. Both cases now serve a relative Link, as pagination.json's do. The feed case's absolute one meant a generated walk was caught in most runners only by the walker's cross-origin refusal. Each runner mocks its own origin, so an absolute production URL is cross-origin everywhere except Ruby and Python. A wrapper that walked same-origin links and stopped at foreign ones would have passed while walking in production, where `next` is same-origin. Resolved against the request, the relative link is same-origin in every runner, so a walk is caught by the request count itself. The body's `next` stays absolute, as on the wire. Proven: flipping both lanes to style "link" and regenerating turns both cases red in the TypeScript and Python runners with "Expected 1 requests, got 2". With the absolute Link, TypeScript failed earlier, at the origin guard. The trait's own doc said a cursor operation's Go wrapper is guarded by nothing, "and no build will say so". Conformance says so, since Go's runner executes these fixtures like the other six. The doc now names that guard and says a new cursor operation owes the same fixture. SPEC §8 listed the operations that carry the pagination trait without a `page` parameter. The two lanes now belong on that list, for a different reason than the six already there, so they get their own carve-out. SPEC §23's fixture paragraph and the fixture-section map now count thirteen cases and name the new one. `key`'s member doc still read purely as link-walk framing, though every service generator withholds it for the cursor style. It now says so, and that on a cursor operation it is catalogue only. A correction to the previous commit's message: the Link-paginated count staying at 61 does not show the declaration kept these lanes out of the walk. That filter already matched Link routes alone, so a cursor declaration leaves it at 61 by construction. What it catches is a "link" typo. * Say how a generated walk goes red, not only that it would Two wording corrections from the converged review of the rebased head. SPEC §23 said both cursor lanes are "caught by count" if a walk is ever generated, and the trait doc said a walking Go wrapper fails "exactly as the other six". Count is the mechanism where the return type survives a flip to the link style: TypeScript, Python, Ruby, Go and Kotlin. In Rust and Swift the flip changes the generated return type, so the runner fails to compile before it counts anything. The walk still goes red, just not by count, and both sentences now say so. The feed's envelope case serves a relative Link beside an absolute body `next`, unlike bc3's wire, and only the inbox case explained why. The feed case now says it too, so nobody "fixes" it back to absolute. * Scope the Go guard to the walk it actually catches A Go wrapper that follows the Link walk fails conformance on the request count. One that chased the body's absolute `next` would be refused at the runner's mock origin and still count one request. That walker is contrived, but the trait doc claimed more than the fixture guards, so it now names the walk it catches and the one it does not. * Rust passes these cases on a flip; say what guards it there Review of the previous two commits, which set out to state exactly how a generated walk goes red and still got one SDK wrong. A lane flipped to the link style does not fail Rust's conformance runner at compile, or at all. Rust's link-style method calls send_page, which fetches one page and leaves the walk to collect_all and items. Page<T> derefs to T, so the runner's summarizer still compiles. One request, every assertion green. What pins the style in Rust is the route-table test in guarantees.rs, and SPEC §23, the trait doc and both fixture descriptions now say so. The rest of the split is now stated as each runner behaves: request count in TypeScript (its runner is not type-checked), Python, Ruby and a Link-following Go wrapper; compile in Kotlin and Swift. A Go wrapper that chased the absolute body `next` fails on the cross-origin error rather than on count. Only one that swallowed the error slips through, and the trait doc names it. * Ruby fails on an error, not on count; and the client refuses, not the mock Review of the previous commit, which converged with three wording nits. Two of them corrected claims that were false. Ruby's link-style method fetches page one eagerly and places a lazy enumerator under the key. The runner's summarizer calls `.length`, which Enumerator lacks, so the case fails on the NoMethodError after one request, not on the request count. SPEC §23 now says so. In Go an absolute `next` is refused by the client, whose base URL is the runner's loopback mock. The mock never sees that request. Also untangles two parentheticals in the inbox case's description and a doubled "so" in the feed case's.
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.
#905 made
basecampPagination.styledecide whether a generator emits a pagination walk, and every generator refuse a value it does not implement. It said so in two shipped places —spec/basecamp-traits.smithy("The six service generators reject any other value rather than reading it as 'not paginated'") andSPEC.md§8 ("Every service generator refuses any other value"). In two of the six that was not true.Kotlin gated the refusal on the object cast rather than on presence.
operation["x-basecamp-pagination"] as? JsonObjectanswers null for a trait that is present but not an object — a bare"x-basecamp-pagination": "page", or a number, or an array — sorequireshort-circuited true and the operation shipped as silently unpaginated.TypeScript gated it on truthiness.
if (operation["x-basecamp-pagination"] && …)exempts a present-but-falsy trait —false,0,""— from the refusal, same silent pass by a narrower vector.Both are the exact failure mode #905 exists to close, and Swift already had the fix thirty lines away with a comment naming it: "answers nil for a trait that is present but not an object, which would skip the refusal below and ship the operation as silently unpaginated." It was written for Swift and not carried to the other two.
The change
Presence is tested before any cast, in both, matching Swift and Rust: absent and a literal null read as unpaginated; anything else is declared, and a declared trait that is not an object has no style to accept, so it is refused by name. Kotlin additionally reads
stylethroughas? JsonPrimitiveso a non-primitive style is refused by name rather than throwing the confusing cast errorjsonPrimitiveraises.parseOperationis now exported from the TypeScript generator. #905's TypeScript tests hand-built aParsedOperationand characterisedbuildReturnType, so they could not reach the parser at all — reverting the key-withholding left them green. These tests drive the real parser.Tests land in both generators' own test modules —
kotlin/generator/src/test/…/PaginationStyleTest.ktandtypescript/tests/generator/pagination-style.test.ts— covering link, cursor, absent, literal null, the retiredpage, a typo, case sensitivity, a declared trait with no style, and the non-object and falsy vectors above. Kotlin's generator test module had no coverage from #905 at all.No generated output changes: no operation declares a malformed trait, so this only alters what happens to one that would.
Provenance
Found by an adversarial review of #905's merge commit — which arrived after #905 had already merged, because I armed auto-merge while that review was still running. That was my error and this PR is its consequence; the defects were on
mainfor the interval.The fix itself is a concurrent session's commit
a106b26c, cherry-picked onto currentmainrather than reimplemented. It was pushed to #905's branch after that PR merged, so it was stranded where nothing would pick it up.Raised on Pagination trait: style "cursor" is documented but unimplemented.