Repository navigation
Event feed connector: the inbox lane (4/4) - #897
Conversation
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. |
There was a problem hiding this comment.
🟡 Changes recommended
Inbox file checkpointing is currently broken, and the decoder and conformance contract have schema-valid cases they mishandle.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds inbox-lane support to the Go event-feed connector and its tier-2 conformance suite.
Changes:
- Adds lane-aware filtering, subscription decoding, item identity, and checkpoint keys.
- Adds inbox retention recovery and addressing-ID deduplication.
- Adds conformance schema, fixtures, documentation, and 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 |
|---|---|
SPEC.md |
Defines the inbox-lane contract. |
go/README.md |
Documents inbox usage. |
go/pkg/basecamp/eventfeed/scenario_harness_test.go |
Configures fixture lanes and reasons. |
go/pkg/basecamp/eventfeed/scenario_fixture_test.go |
Models and validates inbox fixture data. |
go/pkg/basecamp/eventfeed/scenario_conformance_test.go |
Drives inbox subscription and polling. |
go/pkg/basecamp/eventfeed/recovery.go |
Adds inbox-specific gap messaging. |
go/pkg/basecamp/eventfeed/loop.go |
Applies lane identity and subscriptions. |
go/pkg/basecamp/eventfeed/inbox_test.go |
Tests inbox behavior end-to-end. |
go/pkg/basecamp/eventfeed/filters.go |
Enforces lane-specific dimensions. |
go/pkg/basecamp/eventfeed/export_test.go |
Exposes inbox test helpers. |
go/pkg/basecamp/eventfeed/event.go |
Adds addressing metadata and keys. |
go/pkg/basecamp/eventfeed/doc.go |
Updates package documentation. |
go/pkg/basecamp/eventfeed/connector.go |
Adds the public lane option. |
go/pkg/basecamp/eventfeed/checkpoint.go |
Adds lane-aware checkpoint keys. |
go/pkg/basecamp/eventfeed/catchup.go |
Uses lane-specific event identity. |
go/pkg/basecamp/eventfeed/cable.go |
Adds inbox subscription and decoding. |
go/pkg/basecamp/eventfeed/cable_test.go |
Tests inbox cable behavior. |
conformance/event-feed/schema.json |
Extends the scenario schema. |
conformance/event-feed/README.md |
Documents fixtures 32 and 33. |
conformance/event-feed/fixtures/33-inbox-gap-410-retention-resume.json |
Covers inbox 410 recovery. |
conformance/event-feed/fixtures/32-inbox-lane-dedupes-by-addressing-id.json |
Covers inbox identity and deduplication. |
Review details
- Files reviewed: 21/21 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 3f28b08ae2
ℹ️ 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".
3f28b08 to
8a7d063
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a7d0636ba
ℹ️ 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".
8a7d063 to
225cd47
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 225cd47248
ℹ️ 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".
225cd47 to
9946e17
Compare
9946e17 to
8698396
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8698396102
ℹ️ 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".
8698396 to
ce0899c
Compare
|
Review threads: 49 resolved (47 fixed, 1 declined with the reasoning in the thread, 1 pointed at #898), 0 open. Head cdef1b8, rebased onto main at a804626 (#912 merged) — no merge commits; the seam contract names the reason-keyed 400 and the two 410 shapes; the normative dedupe contract and the capacity table are in the lane's identity. Copilot and Codex both reported on 650f1c4; this head changes SPEC prose and one schema pattern only. Since the first rebase: the seam contract is parameterized by lane; the checkpoint identity is five components everywhere it is stated and the store admits only the two lane names; the inbox's 410 carries no epoch (fixture 33, schema, connector message, docs); every fixture timestamp validates as a calendar instant; the overflow and dedupe contracts name the lane's identity, with an inbox overflow test; fixture 32 follows an inbox continuation that carries the walk's filters; a polled inbox item's event is the poll row and a live item's is push-shaped, named apart in the schema; the README matrix rows match the schema and decoder. Declined:
|
ce0899c to
8885d4e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8885d4e1bf
ℹ️ 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".
8885d4e to
238f12f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 238f12fc1f
ℹ️ 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".
238f12f to
76bc64b
Compare
The shipped feed has a second resource: GET /inbox.json, the principal's
addressed items — the low-noise "someone addressed you" lane, served to
agent principals only. The connector consumes it with the same protocol,
selected by WithLane(InboxLane).
Identity is the addressing id. An item is a first-class delivery: the
same event can address the principal for several reasons and each reason
is its own item, so the lane deduplicates by addressing id, never by
event id — the feed's rule would discard every reason but one — and the
same key positions the reset cursor (since walks item ids there), names
the ids a BufferOverflow reports, and is what Event.Key returns. Every
delivered event carries an Addressing; account-lane events carry none.
The lane's dimensions are reasons, narrowed by types and buckets, and
nothing else: construction refuses the account feed's other dimensions
on the inbox and reasons on the feed. The live subscription carries
`inbox: true` as a JSON boolean after the channel key, and each live
frame is one item in the poll shape — the four envelope keys around an
event decoded to the poll row's contract, the push-only transport fields
accepted when present. Inbox positions are never interchangeable with
feed positions, so the checkpoint identity carries the lane: the flat
key gains a fifth element, and the account lane's key is unchanged.
The inbox's 410 is the retention window rather than the epoch; the
resume URL re-enters at since=0, the earliest retained item, as the same
position-resume entry the feed's does. A non-agent principal's 403 rides
the shared authorization counter.
The tier-2 family gains the lane: config.lane and config.reasons, the
`inbox` subscription pin, item frames, the {items, position, next}
envelope, and fixtures 32 (subscription, items, dedupe by addressing id
across poll and live) and 33 (the retention 410 resumed). SPEC §23 has
the lane's subsection.
…and lane-conditioned fixtures The file store accepted only the four-element flat key, so the first inbox save wrote a key every later load rejected as corruption; it now round-trips the five-element inbox form (an empty fifth element stays malformed) and validates the lane's text like the other identity parts. An inbox item's event may omit the two push-only transport keys, but a present null is a wrong-typed value, not absence, and takes the invalid-frame path. The tier-2 driver decodes and forwards those keys on inbox rows, and the scenario schema conditions the filter dimensions on the lane so a fixture no driver could construct cannot be certified. Inbox positions are bound to the principal; SPEC §23 and the lane's documentation say the consumer namespace is where that identity lives.
…e server's vocabulary
The scenario schema conditions the page envelope and the live frame on
the configured lane — an inbox scenario serves {items, position, next}
pages and item frames, an account scenario events and event frames — and
the driver refuses the other lane's envelope at the seam, so a fixture
cannot certify the wrong wire shape. Addressing reasons are constrained
strings rather than a closed enumeration, matching Filters.Validate: the
vocabulary is server-owned, and a scenario may name an unknown reason to
draw the filter 400. StartAfter says which id the inbox lane takes.
…e server's vocabulary on the wire too, and the decoded addressed_at is asserted
…by five The PollSource seam names PollEvents on the account lane and PollInbox on the inbox lane, and PollPage rows are ordered by the lane's identity. CheckpointKey carries Lane everywhere the identity contract is stated, naming the collision a four-part store would allow. The README matrix rows for 409/410 are consolidated and its push-key rule matches the schema and decoder for inbox item events; the schema constrains addressed_at to RFC 3339 and conditions the poll-query pins on the lane.
The inbox lane's fence is its retention window, not an epoch: its 410 body carries resume only. Fixture 33 serves that shape; the schema requires epoch_after_id on the account lane and forbids it on the inbox, where the gap's epochAfterId is 0, the fixed value for none; the inbox's terminal message names no item, and the seam and signal records document the 0.
…e names, and the seam docs follow the lane Every fixture timestamp — a row's created_at on either lane and an item's addressed_at — is validated as a calendar instant (format) as well as the exact spelling every driver parses (pattern), so a schema-valid scenario cannot fail inside a driver's parse. Fixture 32's page is in strict addressing-id order; the checkpoint lane component admits only the account lane's empty spelling and the inbox's name, at Load, Save and in a stored key; and the Go seam and package docs say which operation backs PollSource on each lane.
…ressing ids, and an item's polled event is the poll row
Fixture 32 follows an inbox next URL so the {items, position, next} envelope
is exercised, and joins the frozen-head row with 33. An inbox overflow test
pins DroppedIDs as addressing ids. The inbox item's polled event is the poll
row's nine keys — the generated FeedEvent carries neither transport key, so
the schema and the driver stop accepting them there — while an item frame's
event stays push-shaped. FeedGap's summary is lane-neutral.
…ion continues the lane's operation The item frame's event may carry the push transport keys while a polled item's event is the poll row, so the schema names the two shapes apart; the driver's NEXT placeholder continues inbox.json on the inbox lane.
…es the two 410 shapes
99f7536 to
0fb8eaf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fb8eaf705
ℹ️ 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".
… a live item's transport keys are optional in the README
|
@codex review |
There was a problem hiding this comment.
🔵 Needs a closer look
The stacked change alters protocol decoding, identity, persistence, and recovery semantics across a concurrent state machine and needs final human review.
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 650f1c420b
ℹ️ 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".
…and an inbound actor type is any non-empty string
There was a problem hiding this comment.
🔵 Needs a closer look
The schema constraints need regression probes, and two lane-related messages remain inconsistent.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
conformance/event-feed/schema.json:714
- This new lane-dependent
allOfis only covered by valid scenarios. The repository’s schema pin gate currently has only the two default-handler probes, so removing or widening these negative constraints (for example, allowingreasonson the account lane orcreatorson inbox) would leave every existing check green. Add derived pin probes for both lane arms so the shared conformance contract is regression-tested.
go/pkg/basecamp/eventfeed/connector.go:413 - With
InboxLanenow supported, the laterStartAftervalidation still reports “positive event id.” For inbox callers this value is an addressing ID, so theReasonUsagemessage contradicts the new API contract. Make the message lane-neutral.
go/pkg/basecamp/eventfeed/loop.go:2004 - The nearby
checkpointKeycomment still says this identity has “all four parts,” but the newLanefield makes the struct five-part even though account flat keys omit the empty fifth element. Update the comment so maintainers do not accidentally treat lane as optional identity data.
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Last of the event-feed stack (#777 → #705 → #778 → #897), and the change that makes the connector reach the wire on its own. ## What this is `eventfeed.NewLive` binds the connector's two seams to the generated operations through the `basecamp.EventFeedService` wrapper: `Live.Minter()` over `CreateStreamTicket`, `Live.Polls()` over `PollEvents` (account lane) or `PollInbox` (inbox lane), and `Live.Connect(opts...)` builds the connector over them. One seam call is one fully-governed generated call — the operation keeps its SPEC §7 retry budget, backoff and Retry-After inside the seam — and each adapter maps every §6/§7 outcome onto exactly one seam error kind: - 401/403 → `unauthorized`; a parsed Retry-After → `throttled(retry_after)`; a retryable or transport-level failure → `transient`; context cancellation passes through untouched. - The feed's matrix: 409 → `filter_changed` with both digests; 410 → `gone` with the resume URL, and the two lanes own their own 410 shapes — the feed's carries `epoch_after_id`, the inbox's re-enters at `since=0` — so one lane can no longer read the other's recovery. A 400 is keyed by the server's `reason`, falling back to the legacy message classification only when no reason is present; anything else non-retryable → `unrecoverable`. - A continuation or resume URL the connector has validated is followed by re-issuing the operation with the query the server wrote into it (`PollEventsOptionsFromURL` / `PollInboxOptionsFromURL`), never by fetching the URL raw — so "never construct API paths manually" and "one seam call is one generated call" both hold. A continuation must continue at a `position` and a resume must re-enter at a `since`; a URL carrying the other lane's cursor kind, a repeated key, or filters that differ as a set from the lane's own canonical filters is a malformed page and is refused. ## The redirect policy, and why the adapters build their own client The client's default redirect policy follows a cross-origin hop with the bearer stripped — which still egresses to the foreign origin, and §23 "Continuation and Resume URL Validation" requires none. A per-operation override isn't reachable through the wrapper, so this adds one client option, `basecamp.WithCheckRedirect`, and `NewLive` builds the feed's client from the host's `Config`, `TokenProvider` and any `ClientOption`s with a policy installed last that validates every hop's resolved `Location` under §8's same-origin rule plus downgrade rejection before any request is issued. A refused hop reaches the connector as the seam's `redirect_refused` kind carrying only the Location's origin: the `url.Error` net/http wraps the refusal in renders the refused URL whole, which is exactly the value a hostile redirect can put the bearer into, so the typed error alone travels. Where a host hook replaces the request context and the connector loses its own record of the hop, the origin reports as `unrecorded` rather than borrowing `unparsable`, which §9 reserves for a Location that is absent or genuinely unparseable — an observability surface should not state something false about the server. `NewLive` refuses a base URL whose escaped path is not the canonical encoding of its decoded one, alongside the dot-segment and doubled-slash rules it already applied. A base URL ending in `%2f` otherwise decodes to a canonical-looking path, anchors the account segment one level off, and takes the feed's own request outside the guard's recognition — with a 302 then followed and the bearer delivered to the foreign origin. The reachable version of that is a host whose config templating or an encoded environment variable leaves an escape in its own base URL, which is what the precondition exists to catch. The rule is deliberately fail-closed at one edge: a base path carrying non-ASCII also sets `RawPath` and is refused, since Go's canonical encoding differs from the literal. No deployment uses one, and `/a%20b` has a test showing where the line falls. This is the Layer-1 302 test the tier-2 family assigned to this layer: `TestLivePolls_RefusesACrossOriginRedirectWithZeroEgress` has a real generated `PollEvents` call meet a real 302, and a sentinel listener behind the foreign Location records zero hits. The deferrals that pointed at it (SPEC §23, the README's row-15 note and rows 15/30, fixture 30 and its schema variant, the four `spec/tracking-issues.yml` entries) are discharged. ## Tests `live_test.go` drives the adapters against a loopback API through a real `basecamp.NewClient`: the mint (success, 401/403, 429 with Retry-After, 503, 404, a malformed success), the poll query for every dimension and the page mapping (`performed_by_id`, verbatim `details`), continuation-following through the operation, the full error matrix on both lanes, inbox items with their addressing, the cross-origin 302 refusal with zero egress, a same-origin 302 followed with the bearer intact, an escaped base path refused while `/api/v1` and `/a%20b` still build and still refuse a 302 at the wire, cancellation pass-through, and `NewLive`'s construction validation. ## What is deliberately left to #915 Cross-field consistency checks over a malformed response body — a `next` whose position diverges from the page's own, a digest outside the 16-hex alphabet, a `reason` outside its enum — belong at the generated layer's decode, where every SDK gets them, rather than as another condition in this adapter. #915 tracks that. An inbox 410 carrying the feed's `epoch_after_id` is inert here by construction: the response type declares no such member, and the control that matters is the fence, which is pinned to `since=0`. ## Verification `go build`, `go vet`, `go test -race -count=1 ./pkg/basecamp/eventfeed/...`, `go test ./pkg/basecamp/`, `golangci-lint run` (0 issues), `make event-feed-fixtures-check`, `make doc-constants-check` and `make check-known-defect-issues-open` — all clean.
Fourth of the stack. Stacked on #778, which is stacked on #705 and #777 — review after those. The shipped feed's second resource,
GET /inbox.json, consumed by the same connector.Why a fourth PR rather than folding it into #777/#705
The inbox lane cuts across all three layers of the stack — the seams, filters and cable codec in #777, the run loop's identity key in #705, the schema, driver and fixtures in #778 — and those three PRs carry an eight-round review history. Their last commits reconcile them with the shipped contract's changed spellings (srv2, the new dimensions, the event keys, the epoch resume); adding a new resource to each of them would have widened every diff that reviewers had already converged on. One single-purpose PR keeps each review scope what its title says, and gives the lane its own.
What the lane is
WithLane(InboxLane)selects the principal's addressed items — the low-noise "someone addressed you" lane, served to agent principals only. Same protocol: mint, dial, subscribe, confirm, catch-up, drain, stream, repair.addressing_id, never by event id (the feed's rule would discard every reason but one), positions its reset cursor by it (sincewalks item ids), reports it inBufferOverflow.DroppedIDs, andEvent.Key()returns it. Every deliveredEventcarries anAddressing{ID, Reason, AddressedAt}; account-lane events carry none — the iteration element stays one type across lanes.reasons, narrowed bytypesandbuckets, and nothing else: construction refuses the account feed's other dimensions on the inbox lane andreasonson the account lane (usageerrors). Items are never self-addressed, so the lane needs no loop guard.{"channel":"EventsChannel","inbox":true[,"types":…][,"buckets":…][,"reasons":…]}—inboxa JSON boolean right after the channel, as documented. Each live frame is one item in the poll shape: the four envelope keys around an event decoded to the poll row's contract, with the push-only transport fields accepted when present (the doc says "same shape as above", meaning the poll item).CheckpointKey.Laneis"inbox"there and the flat key gains a fifth element; the account lane's four-element key is unchanged.resumere-enters atsince=0, the earliest retained item, as the same position-resume entry the feed's epoch resume is. A non-agent principal's 403 rides the shared authorization counter toauthorization_failed.The
PollSourceseam is unchanged: an inbox adapter over the generatedPollInboxoperation maps each item onto anEventwith itsAddressingset. That adapter, with the feed's, is the next PR once the spec operations land.Conformance
The tier-2 family gains
config.lane/config.reasons, theinboxsubscription pin, item frames, and the{items, position, next}envelope; fixtures 32 (subscription spelling, items envelope, two items over one event both delivered, a repeated addressing id suppressed across the poll and live lanes) and 33 (the retention 410 accepted and resumed as a position-resume entry). SPEC §23 has the lane's subsection and theWithLaneoption row.Gates run locally:
go test ./pkg/basecamp/eventfeed/...,golangci-lint run,make event-feed-fixtures-check,make doc-constants-check— all clean.Summary by cubic
Adds
WithLane(InboxLane)support forGET /inbox.jsonand its live subscription. The existing account lane remains the default and keeps its behavior; inbox deliveries exposeAddressingand use addressing IDs, rather than event IDs, for deduplication and positioning.Inbox lane
reasons, narrowed bytypesandbuckets, and rejects account-only filters at construction.{items, position, next}pages, andinbox: truesubscriptions.since=0; non-agent 403 responses use the shared authorization counter.Conformance
Written for commit cdef1b8. Summary will update on new commits.