Repository navigation
Event feed: split the poll lanes' 410s and give their 400 a reason - #912
Merged
Merged
Conversation
PollEvents answers FeedPositionGoneError {error, epoch_after_id (required),
resume re-entering at the epoch}; PollInbox answers a distinct
InboxPositionGoneError {error, resume re-entering at since=0} — two shapes on
two operations, so a consumer cannot handle one lane's 410 with the other's
arm. Both lanes' 400 is a dedicated FeedRequestError {error, reason?}: bc3
because a server that predates it sends the flat body, in which case the 400
is undifferentiated and is surfaced rather than guessed at. Fixtures cover the
400 with and without reason on both lanes. The Go wrapper returns
*FeedRequestError (with Reason), *FeedPositionGoneError (feed only) and
*InboxPositionGoneError (inbox only). Also regenerates the Ruby and TypeScript
metadata the BareForbiddenError commits left stale. CreateStreamTicket keeps
@basecampIdempotent(natural: true): Ruby, TypeScript and Go classify retry off
that flag, and dropping it fails check-idempotency-parity.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
typescript
This was referenced Sep 16, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Validate the required epoch_after_id before exposing a typed FeedPositionGoneError.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR refines event-feed error contracts by separating feed and inbox 410 responses and adding optional 400 error reasons across SDKs and specifications.
Changes:
- Adds
FeedRequestErrorwith optional invalid-position/filter reasons. - Splits
FeedPositionGoneErrorandInboxPositionGoneError. - Regenerates SDKs, OpenAPI artifacts, conformance fixtures, and documentation.
File summaries
| File | Summary |
|---|---|
typescript/src/generated/schema.d.ts |
Regenerated TypeScript schemas. |
typescript/src/generated/openapi-stripped.json |
Updated stripped OpenAPI artifact. |
typescript/src/generated/metadata.ts |
Updated TypeScript metadata. |
spec/basecamp.smithy |
Defines revised error contracts and operation mappings. |
spec/api-gaps/event-feed.md |
Updates event-feed registry references. |
SPEC.md |
Documents revised wire operations. Nit (2 votes): complete the dangling “the feed’s” reference with FeedPositionGoneError. |
rust/basecamp-sdk/src/generated/types.rs |
Regenerated Rust types. |
rust/basecamp-sdk/src/generated/services/event_feed.rs |
Updated Rust service bindings. |
ruby/lib/basecamp/generated/types.rb |
Regenerated Ruby types. |
ruby/lib/basecamp/generated/metadata.json |
Updated Ruby metadata. |
python/src/basecamp/generated/types.py |
Regenerated Python types. |
python/src/basecamp/generated/services/event_feed.py |
Updated Python service bindings. |
openapi.json |
Regenerated canonical OpenAPI contract. |
go/pkg/generated/client.gen.go |
Regenerated Go response types. |
go/pkg/basecamp/event_feed.go |
Adds typed Go error mapping. Critical (2 votes): required epoch_after_id presence must be validated before constructing the typed error. |
go/pkg/basecamp/event_feed_test.go |
Tests revised Go error behavior. |
conformance/tests/event_feed.json |
Adds lane-specific and reasoned error cases. |
Review details
- Files reviewed: 7/17 changed files
- Comments generated: 2
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Decoding into the generated int64 would leave a missing or null epoch_after_id at 0 and hand callers a FeedPositionGoneError with a fabricated boundary; decode through a pointer and keep the canonical error when the epoch is absent. Also finish the SPEC table sentence that contrasted the inbox's 410 with the feed's.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ae5331f4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🔵 Needs a closer look
Address the two moderate Go validation issues, constrain or clarify reason, and update the PollInbox status listing.
Review details
Suppressed comments (4)
SPEC.md:3124
- This Errors column otherwise enumerates the operation's statuses, but the new PollInbox row omits the shared 401, 429, and 500 responses that remain in
PollInbox's Smithy error list. A reader could infer that the inbox does not return authentication, rate-limit, or server errors; list those statuses (or state that all other PollEvents errors are inherited).
| `PollInbox` | `GET /{accountId}/inbox.json` | `@readonly`; retry 429/503 ×3 | 400 `FeedRequestError` and 409 as `PollEvents`; 403 `BareForbiddenError` — a bodyless `head :forbidden` — for every non-agent principal; 410 `InboxPositionGoneError` (re-enters at `since=0`), a distinct shape from the feed's `FeedPositionGoneError` |
go/pkg/basecamp/event_feed.go:481
InboxPositionGoneErrordeclareserroras required, but this guard validates onlyresume. A 410 body containing justresumeis therefore exposed as a typed error without the required server message (the canonical*Errorhas only its generic status fallback); trackerror's presence before constructing this wrapper.
if json.Unmarshal(body, &gone) == nil && gone.Resume != "" {
go/pkg/basecamp/event_feed.go:493
- The new feed 410 guard checks the required epoch and
resume, but still does not validate the requirederrormember. A body with only those two fields is typed asFeedPositionGoneErrorwithout the required server message, leaving the canonical*Errorwith only its generic status fallback; trackError's presence in the auxiliary decode and require it here as well.
if json.Unmarshal(body, &gone) == nil && gone.Resume != "" && gone.EpochAfterID != nil {
spec/basecamp.smithy:540
reasonis documented here as onlyinvalid_positionorinvalid_filter, and the PR description treats it as a two-value discriminator, but declaring it as plainStringdrops that constraint from OpenAPI and the generated SDK types (for example, TypeScript exposesreason?: string). Consumers cannot use the generated contract to distinguish the valid cases exhaustively. Model the optional member with a dedicated enum and regenerate, or explicitly document this as an intentionally open string with unknown-value handling.
reason: String
- Files reviewed: 7/17 changed files
- Comments generated: 0 new
- Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
Member
Author
|
Review threads: 3 resolved (2 fixed, 1 declined with the reasoning in the thread). Declined:
CI green on 0ae5331 with the |
jorgemanrubia
added a commit
that referenced
this pull request
Sep 16, 2026
…eeded An adversarial review of the previous head found the TypeScript string-id tests could not fail. On this base the normalizer walk still covers `assignees`, so it had already turned those ids into numbers before the guard saw them: a guard that refused every string passed the whole suite. They now run against `writableIdList` directly, with no walk in front, and that mutation fails 14 of them. The same review found claims wider than the evidence: - None of the person objects in `spec/fixtures` carries a string id. What the fixtures show is that the `personable_type` marker the walk keys on is often missing (3 of 7 `assignees` people), not that those ids arrive as strings. The comments now say that, and say why the guard must not lean on the walk either way: which people the walk finds is the walk's rule. - Comments on this branch's person-id modules said the merge-safe guard "requires an int", that it refuses a string assignee id "tracked as its own unit of work", and that "two places" read a string id. This is that unit of work, and there are three. - The guard's docs cited #912, which is an unrelated event-feed change. Python bounds a plain `int` with a direct range check instead of `parse_int64(str(value))`: `str` of an `int` past 4300 digits raises a raw ValueError under Python's default limit, and is quadratic where a host has lifted it. The corpus gains the rows the review said it lacked — a JSON number and a string between 2^53 and 2^63, and `1e3` both ways. Python and Ruby are 0 of 39 at every site; TypeScript is 6, all of them the JavaScript number boundary: four ids past 2^53 it refuses rather than rounds, and `1024.0` and `1e3`, which `JSON.parse` has already made indistinguishable from integers.
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Sep 16, 2026
basecamp/basecamp-sdk#912 splits what this code had to treat as one shape. Each lane's 410 is now its own type — the feed's names the epoch, the inbox's does not have one — so a lane can no longer read the other's recovery by accident. Each lane owns the arm that recognizes its own, and the epoch arrives as a plain int64 rather than a pointer that stood for "this is the inbox". The 400 now carries a reason, and the two cases it separates have opposite remedies: a malformed position wants a re-entry, a malformed filter wants the filters fixed and would be untouched by any position reset. Each gets the remedy its reason names. A 400 with no reason — an older server — still gets the server's message and no hint at all, because guessing between the two sends the caller to the wrong one.
jorgemanrubia
added a commit
that referenced
this pull request
Sep 16, 2026
…eeded An adversarial review of the previous head found the TypeScript string-id tests could not fail. On this base the normalizer walk still covers `assignees`, so it had already turned those ids into numbers before the guard saw them: a guard that refused every string passed the whole suite. They now run against `writableIdList` directly, with no walk in front, and that mutation fails 14 of them. The same review found claims wider than the evidence: - None of the person objects in `spec/fixtures` carries a string id. What the fixtures show is that the `personable_type` marker the walk keys on is often missing (3 of 7 `assignees` people), not that those ids arrive as strings. The comments now say that, and say why the guard must not lean on the walk either way: which people the walk finds is the walk's rule. - Comments on this branch's person-id modules said the merge-safe guard "requires an int", that it refuses a string assignee id "tracked as its own unit of work", and that "two places" read a string id. This is that unit of work, and there are three. - The guard's docs cited #912, which is an unrelated event-feed change. Python bounds a plain `int` with a direct range check instead of `parse_int64(str(value))`: `str` of an `int` past 4300 digits raises a raw ValueError under Python's default limit, and is quadratic where a host has lifted it. The corpus gains the rows the review said it lacked — a JSON number and a string between 2^53 and 2^63, and `1e3` both ways. Python and Ruby are 0 of 39 at every site; TypeScript is 6, all of them the JavaScript number boundary: four ids past 2^53 it refuses rather than rounds, and `1024.0` and `1e3`, which `JSON.parse` has already made indistinguishable from integers.
jorgemanrubia
added a commit
that referenced
this pull request
Sep 16, 2026
* Read a person id at the write guard the way the reference reads one A merge-safe update/edit GETs a record, reads every writable field and PUTs the whole representation back, so the id lists it reads are the id lists it writes. Three of those fields are a list of embedded people projected to ids: a todo's `assignees` and `completion_subscribers`, and a schedule entry's `participants`. The guard that reads them refused a body the reference accepts. The reference reads each element through the generated `Person`, whose `Id` is the one field in the whole model decoded FLEXIBLY (`types.FlexibleInt64`), and `fieldsFromTodo` appends what that produced with no filter at all — 0 included. So a string id is the number `strconv.ParseInt(s, 10, 64)` spells, a string that is not a number is the system actor 0, an absent id is the zero value and a null ELEMENT is the zero Person. All of it measured through the reference's own Update and EditEntry composites over a 35-row corpus; the three sites answer identically, row for row. The shapes are live rather than theoretical: across `spec/fixtures`, 3 of 7 `assignees` people carry no `personable_type`, which is exactly what keeps the pre-decode normalizer from reaching them. Not a widened normalizer walk, and not a sweep. Go covers this site with its DECODER; a port's walk that grew an `assignees` key would be inventing a rule the reference does not have. And `TemplateLibraryConfirmationPerson.ID` (`templates.go:60`) is a plain `int64` whose string id correctly drops the projection, so "make every person id flexible" would break it. The fix is at each port's reader, field by field. No port spells the grammar itself. Each reader calls the scan this branch already shares between the walk and the decode — `parse_int64` in Python, `Ids.person_from_wire` in Ruby, `scanPersonId`/`personIdNumber` in TypeScript — so a person the walk reached and a person it did not land on the same id. That matters directly: the walk is being narrowed back to the reference's two positional surfaces, and when `assignees` leaves it, the reader is what covers those people. It already does. Two rows are the reader's alone and no walk can reach them, measured in every decoder-less SDK: an ABSENT id (the zero value, no error) and a null ELEMENT. They are the only rows that survive the walk everywhere, and they survive in Kotlin, Swift and Rust too. Divergence counts against the reference, per site, over the 35-row corpus: Python 18 -> 0 and Ruby 18 -> 0 at the todo sites, 3 -> 0 at participants; TypeScript 6 -> 3. TypeScript's three are the JavaScript number boundary — an id past 2^53 is refused rather than silently rounded, the resolution `personIdNumber` already documents, and `1024.0` cannot be told from `1024` after `JSON.parse`. * Pin the TypeScript guard on its own, and stop overstating why it is needed An adversarial review of the previous head found the TypeScript string-id tests could not fail. On this base the normalizer walk still covers `assignees`, so it had already turned those ids into numbers before the guard saw them: a guard that refused every string passed the whole suite. They now run against `writableIdList` directly, with no walk in front, and that mutation fails 14 of them. The same review found claims wider than the evidence: - None of the person objects in `spec/fixtures` carries a string id. What the fixtures show is that the `personable_type` marker the walk keys on is often missing (3 of 7 `assignees` people), not that those ids arrive as strings. The comments now say that, and say why the guard must not lean on the walk either way: which people the walk finds is the walk's rule. - Comments on this branch's person-id modules said the merge-safe guard "requires an int", that it refuses a string assignee id "tracked as its own unit of work", and that "two places" read a string id. This is that unit of work, and there are three. - The guard's docs cited #912, which is an unrelated event-feed change. Python bounds a plain `int` with a direct range check instead of `parse_int64(str(value))`: `str` of an `int` past 4300 digits raises a raw ValueError under Python's default limit, and is quadratic where a host has lifted it. The corpus gains the rows the review said it lacked — a JSON number and a string between 2^53 and 2^63, and `1e3` both ways. Python and Ruby are 0 of 39 at every site; TypeScript is 6, all of them the JavaScript number boundary: four ids past 2^53 it refuses rather than rounds, and `1024.0` and `1e3`, which `JSON.parse` has already made indistinguishable from integers. * Two comments the review found still said the old thing The Ruby schedules test described the merge-safe guard as requiring an Integer, in the present tense, and the new TypeScript guard tests claimed every expectation was the reference's while asserting the one refusal this change makes deliberately: an id past 2^53, which JavaScript has already rounded. * Flip the tests main pinned "to flip when #913 lands", and say what it closes PR 908 narrowed the positional normalizer to the reference's two surfaces and, rather than leave the gap unstated, pinned the refusal it reopened as tests: a string id in `assignees`, `completion_subscribers` or a schedule entry's `participants` was asserted to fail the merge-safe update, with a note that these were the tests to flip when this landed. They now assert what the reference does — the update completes, `"007"` and `"+007"` are person 7, and a `"basecamp"` participant is the system actor 0, read through the flexible decoder's rule at exactly that field (confirmed with the operator on card 42). The comments and SPEC that handed this work over said it would close "both halves". It closes one. The WRITE path is closed: the guard reads the id by the shared scan. A plain generated read in Ruby, Python and TypeScript still returns an untagged person's string id as the string, where the reference's decoder gives the number; SPEC section 10 now says so instead of pointing here. * Say where the write path is still not the reference, in TypeScript A fresh review of the rebased head found the docs claiming more than the measurements. SPEC section 10 said the write path is closed; in TypeScript it is closed except on the JavaScript number boundary, in both directions — an id past 2^53 is refused where the reference reads it, and a JSON number spelled `1024.0` or `1e3` is accepted where the reference refuses it. SPEC now names both, the guard's docblock stops describing the `Number.isInteger` check it no longer uses, and the guard tests pin the `1024.0`/`1e3` acceptance as a residual so a change in either direction is a decision. Also corrected: a Python test comment that still said the normalizer converts a participant's string id before the guard sees it, which stopped being true when the positional pass was narrowed; SPEC's "Both halves" had lost its antecedent; and a reflowed docstring line in `_person_id.py`. Documentation and one pinning test only; no reader behaviour changes.
jeremy
added a commit
to basecamp/basecamp-cli
that referenced
this pull request
Sep 16, 2026
…ed (#726) * Pin the SDK to the main commit carrying the event-feed operations The account event feed's three operations — poll events, poll the inbox, mint a stream ticket — are not in an SDK release yet, so the pin is a basecamp-sdk main pseudo-version until the next tag. Re-syncing the vendored MCP model brings the EventFeed tag with it, and an SDK tag no DomainSpec claims fails the catalog build. It gets its own domain rather than joining the account grab-bag: the feed is a resumable lane with its own contract — a position and a continuation URL instead of page numbers — and the blurb is where an MCP client learns that. * Add events poll, inbox, and stream-ticket commands on the account event feed The account-wide event feed is how an agent hears about activity it did not cause. It is a different resource from `basecamp events <id>`, which is one recording's audit trail, so it lands as subcommands under the same noun: `events poll` for the catch-up poll lane and `events ticket` to mint a live-stream credential, plus `inbox` at the top level for the addressed-items lane, which is its own endpoint rather than a filter over the feed. Pagination here is the response body, not the Link-header page walk every other listing in this CLI uses: a page carries a durable position to resume from and a continuation URL while the current walk has more to serve. One call is one page; --all walks the continuation to the end of that walk, bounded by --max-pages, and says so when it stops at the bound rather than at the end. That is not a live tail — the live lane is a WebSocket the ticket opens, and connecting it would mean taking a WebSocket dependency the CLI does not have today. The three documented failures get the taxonomy they deserve. A 409 is the caller's to fix, so it exits as usage and names both filter digests. A 410 is not, so it exits as not-found carrying the lane's own re-entry point: the feed re-enters at the epoch the body names, the inbox at the earliest retained item. A 400 keeps the server's message, because a malformed position and a malformed filter have different fixes and only the body knows which. The inbox's 403 says what it means — the lane is served to agent principals only for now. The ticket and its URL are redacted unless --show-secret: the URL embeds the ticket, both are bearers for the ~2 minute window, and a mint that printed them by default would put a credential in every terminal scrollback and log. Bare `basecamp events` now shows the group's help instead of refusing for a missing id, so the subcommands are discoverable. Passing an id still does what it did. * Name the lane's own identifier in an invalid --since, and re-hash the flake The two poll lanes number different things: --since takes an event id on the feed and an addressed-item id on the inbox. One shared remedy string sent an inbox caller looking for the wrong identifier, so the hint now comes from the lane, which already carried the rest of its per-lane text. The SDK pin also moved vendored dependencies, which invalidates the Nix flake's vendorHash. Recomputed and verified against a Docker nix build. * Preserve filters when re-entering a gone position, and count feed rows Three review findings, all real. A 410's resume URL preserves the request's canonical filters, and the hint threw them away: re-entering with a bare --since drops --types, --buckets and the --exclude-performers self loop guard, so an agent recovering from an expired position would start consuming its own activity. The hint now renders the filters back out of the resume URL through the lane's own parser. A resume URL that will not parse, or one carrying a value outside srv2's alphabet, falls back to the bare re-entry — a hint is meant to be pasted, and half-rendered server text is not. The continuation origin check compared raw scheme and host strings, so a base URL written with an explicit default port, or a host in different case, rejected its own continuations and broke --all after the first page. It now compares the scheme case-insensitively and reuses this package's sameHostPort, which normalizes the omitted port to the scheme's default. Both poll commands handed the whole envelope to the output writer, which is one object: --count answered 1 for every page and --ids-only found no id to print. The rows go through WithDisplayData, the seam the account-wide listings already use for exactly this. An inbox row's identity is its addressing id, because applying the feed's dedupe-by-event-id rule to the inbox would discard every reason but one. * Run the event-feed smoke suite The file was written but named in no level array, so `make smoke` never ran it while the static coverage check — which scans files, not the arrays — reported it covered. It joins Level 0: the ticket mint is a POST, but it writes no record and leaves nothing behind for a parallel suite. The inbox case is allowlisted as an accepted gap. The lane is served to agent principals only, so a person's smoke token gets the 403 the test asserts before marking the gap, and an unallowlisted unverifiable blocks the run. * Carry the poll's own filters into every re-entry it advertises A position is bound to the filter set it was minted for, so every command this code hands back has to carry the filters or it is a command that fails. Three places did not. The resume breadcrumb offered `--position <position>` alone. On a filtered poll that is a different filter set, and the server answers 409 — the breadcrumb advertised its own failure. It now renders the filters the poll used, on both lanes. The 409 hint did the same thing in the other direction: the caller has just changed filters, and the remedy told them to re-enter without any. It now repeats the filters they presented. And a 410's resume URL is server text that this code displays as authoritative and reads filters out of, which is acting on it — so it now faces the same origin check the walk applies to a continuation. A resume URL pointing elsewhere is neither shown nor parsed, and the re-entry falls back to the filters the request was invoked with. The filter rendering is one helper now, fed either by the lane's continuation parser or by the request's own options. The skill's two project-scope exception lists had drifted apart, and the feed section described the inbox as the event filter set plus --reasons. It is not a superset: --creators, --performers, --exclude-performers and --actor-types are not flags there, and generated calls using them would fail. Both lanes now list their own filters. * Adopt the SDK's per-lane 410 and the 400's reason basecamp/basecamp-sdk#912 splits what this code had to treat as one shape. Each lane's 410 is now its own type — the feed's names the epoch, the inbox's does not have one — so a lane can no longer read the other's recovery by accident. Each lane owns the arm that recognizes its own, and the epoch arrives as a plain int64 rather than a pointer that stood for "this is the inbox". The 400 now carries a reason, and the two cases it separates have opposite remedies: a malformed position wants a re-entry, a malformed filter wants the filters fixed and would be untouched by any position reset. Each gets the remedy its reason names. A 400 with no reason — an older server — still gets the server's message and no hint at all, because guessing between the two sends the caller to the wrong one. * Keep the stream-ticket mint off the MCP surface The mint returns a replayable bearer and a URL that embeds it, and the MCP dispatcher hands a result straight into a model transcript with no equivalent of the CLI's --show-secret gate. Nothing on that surface could use the ticket anyway: opening the stream needs a WebSocket, which a JSON tool call is not. The sync script's existing exclusion list is held to a "still not JSON" check — it exists for bodies the toolkit refuses to load, and that check is worth keeping — so the policy exclusion is its own list, named for the reason it exists. The skill said these commands do not take --in. They do take it, as a global flag, and ignore it; saying otherwise would have an agent expect Cobra to reject it. * Hand the position back to the person, not just the machine The rendered output is the page's rows, and the generic renderer drops a field named "position" from objects anyway — on a to-do or a card that name means an ordering integer. So someone polling without --json had no way to persist or resume their cursor. The notice carries the whole resume command now, filters included, in every format and whether or not hints are on. A walk that stopped at its page cap says both things in one sentence: more remains, and here is where to continue from. The resume breadcrumb renders the same command rather than a <position> placeholder it could not fill. * Quote the position in the command it hands back A position is opaque server text going into a command somebody pastes, and nothing about it promises to be a bare word. It goes through this package's shellQuote, as every other emitted command's interpolated value does. The filter values beside it are not quoted because they are checked against srv2's alphabet first, and a value outside it drops the whole suffix rather than being rendered — a whitelist, where the position can only have a blacklist. * Re-hash the flake for the SDK re-pin * Say the walk was capped in --ids-only and --count too Those two formats print rows alone: Writer.write routes them straight to writeIDs/writeCount, so no envelope field survives and the notice carrying the cap warning and the resume position was destroyed rather than relocated. A capped --all walk handed back an id list or a count that read as the complete answer. Rows-only formats now print the notice on stderr, where it cannot corrupt the stream a script is reading. Quiet mode keeps its diagnostic-only gate: its stdout is the whole Data object, position and next included. * Name the set the stream-ticket mint is actually excluded by The generated provenance credited EXCLUDED_OPERATIONS, but the mint is in POLICY_EXCLUDED_OPERATIONS. The two sets answer different questions — the toolkit cannot load a non-JSON body, versus we decline to serve an operation that would load fine — and naming the wrong one erases the reason the mint is absent for whoever reads the file to find out. Fixed in the generator and regenerated; the model files came back byte-identical. * Withhold a cable URL that still carries the ticket anywhere Blanking the ticket parameter is not the same as redacting the ticket. net/url preserved the rest of the response verbatim, so a URL whose fragment, userinfo, path or second parameter repeated the credential printed it without --show-secret — against the function's own stated posture that any component can be carrying it. The display URL is now built from the components a cable URL needs, which drops userinfo and fragment outright, and the result is withheld whole if the ticket survives in it. * Resume from the filters the last page was minted under The walk refreshed its request options from each continuation but not the filter string the resume command and breadcrumb are rendered from, so a continuation carrying its own filters produced a re-entry describing the first call's flags. A position is bound to the set it was minted for, so that re-entry is a different set and the server answers 409. * Suppress --project when the id-or-url positional is optional too The classifier matched the required spelling only. events grew subcommands this release and so spells its positional [id|url], which put the irrelevant inherited --project back into its agent help. * Say 195 endpoints and name the event feed The skill's coverage header and the API-COVERAGE version label both described an earlier surface than the one this release ships, which is the sort of contradiction an agent reads as authoritative. The counts now come from the coverage matrix and the version label defers to the pin. * Build the redacted cable URL's query instead of editing it Carrying the server's other parameters through meant asking whether a copy of the ticket was hiding in one, and that question has no stable answer: the rendering escapes as it sees fit, so a ticket "abc/def" echoed as "abc%2Fdef" matched neither a search for the raw ticket nor one for the escaped form. The query is now rebuilt from a constant, which leaves only scheme, host and path to vet — and those are compared decoded. * Project the feed smoke reads down to what they assert bats keeps $output and a failing assertion prints it in full, so capturing a whole poll envelope put a signed resume position in logs somebody keeps — which is what this suite's own header said it did not do. Each read now asks for the fields it checks and nothing else. * Say that the redacted cable URL is a diagnostic It carries ticket=[REDACTED] and so opens nothing whatever else it keeps. The connectable URL is the mint's own, which --show-secret returns verbatim and which does not pass through the redactor at all — so dropping the server's other parameters from the diagnostic costs no artifact anyone connects with, including a parameter the mint might start sending. Pinned with a test over --show-secret. * Name v0.18.0 as the release the pin builds on The pseudo-version builds on v0.18.0, the latest release, not on v0.16.0 — which keeps its own sentence as the release that added the template operations. * Capture the redaction verdict, not the ticket it checks Projecting the ticket and comparing it in bats meant the one regression this test exists to catch — redaction failing — was also the one that put a live bearer into the failure output. The comparison now happens inside --jq, so only its boolean leaves the process, and the filter stays null-safe so a failed mint reports its error rather than a jq error. * Vet the redacted URL as rendered, not component by component The scheme was not on the list of components checked, so a response that echoed the ticket there printed it. A list of components to check is a list to be caught out by; the finished string is checked instead, alongside the decoded path, which is the one place an escaped copy could hide from it. * Refuse an entry flag given an empty value The documented way to resume is --position "$POSITION". An unset variable makes that an empty string, which read the same as omitting the flag and entered at the present — silently skipping the backlog the caller was resuming. Both lanes now say so instead, following the blank-flag pattern checkins already uses. * Write down why events keeps RunE alongside its subcommands The allowlist gained events without the documented exception list gaining it, so the enforced convention and the written one disagreed. events is also the one plural noun in that set, so appending it to a list of commands "without a sibling plural group" would have moved the contradiction rather than fixed it. * State the redaction assertion's coverage component by component Asserting on the finished string is what makes the class dead — it covers components nobody enumerated, which is how the scheme was missed. But what is known to be covered still has to be readable, so there is now a case per component: scheme, userinfo, host, port, path, escaped path, a second parameter, a repeated one, and the fragment. Each also checks the URL was withheld whole rather than partially scrubbed. * Treat every ticket the response presents as the secret The assertion checked the body's ticket field only. A response whose URL carries a different ticket than its body makes the URL's the operative bearer — the one that would actually open the stream — and it survived in the path because nothing was looking for it. The check now runs over the body's ticket and the URL's own ticket values alike. * Check the redacted URL's decoding, not its components url.URL.String escapes per component and picks the encoding itself, so a search for the raw secret misses whichever component it re-spelled: the scheme got through that way, then a non-ASCII host. Decoding the finished string normalizes every component at once, including the ones nobody has named, and replaces the hand-placed path check rather than joining it. * Refuse a list filter given an empty value An unset variable marks the flag provided but leaves it carrying nothing, and dropping it silently widened the request: --buckets to account-wide, and --exclude-performers to no loop guard at all, so an agent starts reacting to its own activity. The guard reads the flags the caller set rather than a list of names to keep in step, so a filter added later is covered without being enrolled. A trailing comma after a real value is still fine.
Merged
jeremy
added a commit
that referenced
this pull request
Sep 16, 2026
Promotes MIGRATING.md's Unreleased section to v0.19.0 and carries the version across the eleven version files and eight lockfiles. Adds one migration note the promotion did not cover. #912 carries the breaking label, so it will appear under the release notes' Breaking Changes heading, and MIGRATING.md had nothing to match it. The EventFeed operations reach a tagged release for the first time here — PollEvents and PollInbox do not exist in v0.18.0 — so the error shapes #912 replaced were never released and there is nothing for a consumer to migrate. That is worth a section rather than a silence: the release notes are generated from labels and cannot say it.
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.
Summary
Follow-up to #898 carrying the two review findings that were decided after it merged (the third,
BareForbiddenErroronPollInbox, landed in #898 itself):PollEventsanswersFeedPositionGoneError {error, epoch_after_id, resume}—epoch_after_idnow required,resumere-entering atsince=<epoch_after_id>so the servable history above the fence is not skipped.PollInboxanswers a distinctInboxPositionGoneError {error, resume}— no epoch,resumere-entering atsince=0, the earliest retained item. They were one shape with an optionalepoch_after_id; a consumer could write oneerrors.Asarm that was silently wrong on one lane. The recoveries are not interchangeable, and the spec now says so.FeedRequestError {error, reason?}on both poll operations replaces the sharedBadRequestErrorthere. bc3 #13362 (merged on master, deploy pending) addsreason: "invalid_position" | "invalid_filter"besideerror;reasonis optional, and when it is absent the 400 is undifferentiated — a consumer surfaces it rather than guessing between re-entering withsince=and stopping. Fixtures cover the 400 withreasonon both lanes and the reason-less fallback.CreateStreamTicketkeeps@basecampIdempotent(natural: true): Ruby, TypeScript and Go classify retry off that flag (TypeScript's gate is literallyopMeta?.idempotent?.natural), so dropping it failsmake check-idempotency-parity— the doc comment already disclaims "same input, same result". Thestyle: "cursor"pagination mode from #905 is not applied to these operations here; that is its own follow-up.What changed
spec/basecamp.smithy—FeedRequestError(400),FeedPositionGoneError(410,PollEvents,epoch_after_idrequired),InboxPositionGoneError(410,PollInbox); the two poll operations' error lists and docs.openapi.jsonregenerated.make generate). Generated Go:PollEventsResponse.JSON400/PollInboxResponse.JSON400are*FeedRequestErrorResponseContent;PollInboxResponse.JSON410is*InboxPositionGoneErrorResponseContent;FeedPositionGoneErrorResponseContent.EpochAfterIdisint64(was*int64).*FeedRequestError{Err, Reason}withFeedReasonInvalidPosition/FeedReasonInvalidFilterconstants (Reason == ""is the undifferentiated case),*FeedPositionGoneError{Err, EpochAfterID int64, Resume}returned byPollEventsonly,*InboxPositionGoneError{Err, Resume}returned byPollInboxonly; each unwraps to the canonical*Error. Tests cover the reasoned and reason-less 400, the feed 410, and that an inbox 410 never types as the feed's.conformance/tests/event_feed.jsongains the 400-with-reason case on each lane (12 cases, all seven runners); the 410 and 403 descriptions name the shapes.spec/api-gaps/event-feed.mdsmithy_refs.Verification
Run locally on macOS, rebased on
mainat e58b582 (every exit code 0):make generate(no diff), all 46 static gates ofmake check-targetsindividually (smithy-check through conformance-fixtures-check, includingcheck-idempotency-parity),make go-check,make ts-check,make py-check,make rb-check,make rs-check,make kt-check,make swift-check, andmake check-fixture-execution(all seven runners execute the 289 cases).Breaking
Generated Go types change shape (
EpochAfterIdpointer → value; the poll responses'JSON400/JSON410types;PollInboxResponseloses itsJSON403body), so the API Compatibility gate flags this the way it flagged #898's additions. The typed feed surface shipped in #898 has not been released, soMIGRATING.mdcarries no entry.Release notes: Features (
enhancement).Summary by cubic
Splits the two poll lanes' 410 into distinct error shapes and gives their 400 a dedicated shape with an optional
reason, fixing two places where consumers could misread an error body and guess wrong recovery.PollEventsnow answersFeedPositionGoneErrorwith requiredepoch_after_idand aresumethat re-enters at the epoch;PollInboxanswersInboxPositionGoneErrorwith no epoch and aresumethat re-enters atsince=0. They were one shape with an optionalepoch_after_id, so a singleerrors.Asarm could silently handle the wrong lane. A feed 410 that omits the now-required epoch is not typed at all — it stays the canonical error instead of gaining a fabricated boundary. Both lanes' 400 is nowFeedRequestError {error, reason?}wherereasonisinvalid_positionorinvalid_filter; when the server predates bc3 #13362 and omitsreason, the 400 stays undifferentiated and the consumer surfaces it.Breaking
EpochAfterIdbecomes a value, the poll responses'JSON400/JSON410types change, andPollInboxResponseloses itsJSON403body.MIGRATING.mdentry is needed.Written for commit 0ae5331. Summary will update on new commits.