Event feed connector: the run loop and tier-2 driver (2/3)

[jeremy]

This PR has been split and force-pushed. It now carries the state machine
only, and is stacked on #777 (foundations). #778 (conformance corrections)
stacks on this. The pre-split head is preserved at tag pre-split/705-head;
the exact commit this PR pointed at before the force-push is
pre-split/705-remote-head (d379f2e11). All 63 review threads are intact,
but line anchors on foundation files now resolve against #777.

Why: eight bot rounds here did not converge (12→3→5→2→2→1→3 threads, with late
findings in files no earlier round had touched), and a review pass then found a
P1 credential defect all eight missed because it composes two files across a
package boundary. That defect is fixed in #777.

The Go reference implementation of the SPEC.md §23 Event Feed connector — BC3's
account-wide event feed over Action Cable push plus polling catch-up — and the
tier-2 conformance driver. All 22 fixtures pass (23 with #778's addition).

Layer 2 only: the connector reaches the wire through TicketMinter /
PollSource seams and one sanctioned cable dial (AGENTS.md Hard Rule 2). The
Layer-1 adapters are deferred to G1b, so the package is experimental and a
consumer supplies the seams today.

What is here

connector.go (New, the options, Events, Close), loop.go (states,
transitions, timers, live buffer), catchup.go (the poll walk, its page
boundary, the drain), recovery.go (the 400/409/410 matrix), and the tier-2
driver with its fixture model, harness and self-tests.

The foundations — seams, wire types, transports, filters, checkpoint identity,
the file store, dedupe, backoff, clock, cable codec, feedtest/ — are #777.

Rebase note

The 29 commits were re-cut, not rebased. Rebasing onto #777 was attempted
and abandoned: 8 of them touch only foundation files and would replay empty, and
21 of the remaining 22 mix both halves, so every one conflicts against the four
fixes #777 applies. Resolving 22 interleaved conflicts by hand is a worse
guarantee than re-cutting, which is byte-exact by construction — the foundation
paths come from #777's tip, these 17 from the pre-split head, verified with an
empty git diff against both.

The five fixes on top

Blocker 2 — a poll page carrying no position is malformed

The walk took page.Position on trust, and an empty one silently skipped
history in two different ways.

Position-resume: acceptPosition("") sets l.position = "", and
entryCursor selects on l.position != "" — so it does not preserve the old
cursor, it falls through to a bare present entry. The feed resumes at the
server's head with everything between skipped, reporting nothing.

Present-class is worse. held uses "" as its sentinel for "the final entry
was not present-class", so an empty position is not saved as empty — it
collapses into the sentinel, the held != "" guard skips acceptPosition and
saveCheckpoint outright, and caught_up announces anyway. The position is
discarded, with the drain's deliveries already handed to the consumer.

Refused before delivery, counter resets, and every mutation. That placement is
what the mutation check exercises: moving the guard after the delivery loop
fails three of four subtests on delivered ids = [101], want [].

Blocker 6 — narrow the promise, and make Wait the quiescence point

Close's doc claimed "no seam call and no delivery can begin after Close has
returned". That is not true and cannot be made true: the run goroutine checks
its context at each dispatch point and then acts, so a Close landing in between
cannot stop the call from starting — only from starting on a live context.
Closing that window means holding a lock across arbitrary host code, trading a
benign race for a deadlock reachable from any callback.

The one effect that outlives the process is the checkpoint save, and this PR
deliberately does not gate it. An accepted page's position saves even when
Close lands first — its events were already delivered, and dropping the write
would silently re-deliver them — and the save runs under a context detached
from the run's cancellation (context.WithoutCancel: values kept, cancellation
dropped), because a store that honors its ctx is compliant and would
otherwise lose the position Close raced. A save decided just before Close can
land just after it; that is intended, not residual.

Ordering a second connector over the same store is therefore the consumer's
to do, and Wait is the tool: it blocks until the run goroutine has exited, so
no save can be in flight — by construction, not by a narrow window. Await the
iterator's termination, or Wait, before opening a second connector over the
same checkpoint store. (An earlier revision ordered saves through a
durableGate claimed inside Close and tracked the unclosable [claim, write]
residual in #784; the gate is gone, Wait replaces it, and #784 is closed with
it.)

Also: a cancelled checkpoint load is no longer Terminal(checkpoint_load).

Blocker 7 — observers see origins only

#777's redactor applied to Observer.Gap and both CatchUpStarted sites. An
accepted 410 latches the server's resume URL as reconnect state, so the
reconnect announces its walk carrying it — redacting Gap alone would have left
the identical URL leaving through a different callback one reconnect later.

Observer.Disconnected is redacted, and this paragraph used to say the
opposite. The original reasoning — an error is opaque text, and stripping a
credential out of arbitrary text means modelling the credential, which §23's
"opaque bearer" contract forbids — argued for leaving it alone. A later round
found a ticket reaching the callback through the raw seam read error, which no
seam obligation can repair from the connector's side, so both arguments now go
through closed vocabularies: observableDisconnectReason maps every
unrecognized peer reason to "other", and observableSocketError reduces every
cause to one the connector owns, degrading anything unrecognized to
errSocketFailed.

"Reduces" is load-bearing and was the last hole: matching a sentinel is not the
same as being one, so a seam returning fmt.Errorf("read %s: %w", cableURL, context.Canceled) matched a recognized arm while its text carried the ticket.
Every arm now returns the connector's own value rather than its argument;
errors.Is still matches, and the wrapper does not survive.

The seam obligation remains on Dial, ReadFrame and WriteFrame — it is what
keeps a preserved typed error safe — but the connector no longer depends on it
being honored.

#763 — staleness arms at socket open

Observer.Connected fired between the socket opening and the window that
measures silence on it. Whatever a host's callback spent was time the window
never counted. Observed from inside the callback, which is the only place the
ordering is visible.

#760 — an occupied deferral slot no longer blinds the drain's fatal scan

drainScan returned the moment it found the slot occupied, on the reasoning
that everything queued "arrived behind this one". That is a claim about arrival
order, where §23's carve-out is a claim about which verdict governs. With
any non-fatal outcome parked ahead of it the scan looked at nothing — and the
budget it runs under is pumpDepth+1, sized in drain's own comment to reach
every frame the pump had already read. An occupied slot spent none of it.

The planned fix was a bounded deferral queue carved out of pumpDepth; that
turned out to be unnecessary. Only one deferral is ever dispatched —
pumpExited, dispatchDisconnect and the invalid-frame teardown all end the
cycle — so a queue would be a buffer sized for a delivery that cannot happen.
The scan keeps the first outcome and discards the rest as it passes them, which
removes the capacity question entirely: no share of pumpDepth, no channel
resize, no change to the published memory bound, no fixture sweep.

The new subtest needs both halves of the two around it: deferring during the
entry poll occupies the slot, and queuing the fatal frame mid-drain puts it
where only the scan can find it. Queued earlier, the ownership cut consumes it
first — which is how the first draft passed against un-fixed code.

On #758/#759: I could not reproduce the missing-wake-source hang. Every path
leaves a wake. The overshoot is real (close to two staleness windows) and
provably cannot exceed two, so the bound is documented in place rather than
patched, and carried to bc3 as an open §23 contract question.

Verification

Pristine worktree, one pass, clean tree before and after:

• go build / go vet / -race -count=1 / -count=5 — pass
• make go-lint 0 issues; gosec on the CI-pinned v2.23.0 (hash-verified) 0 issues
• full make check — exit 0
• 22/22 fixtures
• TestWalkFailureBetweenPages both subtests — the invariant that killed the
  reviewers' one-liner survives
• staleness soak 8 × 500 under -race: 0 failures, 0 data races, re-earned
  because this PR rewrites the files that wait

Kill-matrix correction

Row 15's claim was inherited from the family README and is wrong; #778
corrects it. Tier 2 cannot prove zero egress to a foreign redirect target,
because the driver is the seam and manufactures the verdict. That is a
Layer-1 property, tracked for G1b.

[Copilot]

Pull request overview

Adds the experimental Go Event Feed reference connector, deterministic conformance infrastructure, WebSocket transport, and checkpoint persistence.

Changes:

• Implements the push/poll state machine, recovery, deduplication, and checkpointing.
• Adds real and fake transports plus tier-2/tier-3 conformance tests.
• Documents the experimental API and architecture exception.

Tip

If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

Reviewed changes

Copilot reviewed 54 out of 55 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
AGENTS.md	Registers Event Feed infrastructure.
CONTRIBUTING.md	Documents conformance verification.
go/README.md	Adds Event Feed usage guidance.
go/go.mod	Adds WebSocket dependency.
go/go.sum	Locks WebSocket dependency.
eventfeed/backoff.go	Implements retry timing.
eventfeed/backoff_test.go	Tests retry timing.
eventfeed/cable.go	Implements cable framing.
eventfeed/cable_test.go	Tests cable framing.
eventfeed/catchup.go	Implements catch-up and streaming.
eventfeed/catchup_test.go	Tests catch-up behavior.
eventfeed/checkpoint.go	Defines checkpoint contracts.
eventfeed/clock.go	Implements production timers.
eventfeed/clock_test.go	Tests production timers.
eventfeed/connector.go	Defines the public connector.
eventfeed/connector_test.go	Tests connector construction.
eventfeed/continuation.go	Validates continuation URLs.
eventfeed/dedupe.go	Implements delivered-ID deduplication.
eventfeed/dedupe_test.go	Tests deduplication.
eventfeed/digest.go	Implements filter digests.
eventfeed/digest_test.go	Tests shared digest vectors.
eventfeed/doc.go	Documents the package contract.
eventfeed/errors.go	Defines terminal errors.
eventfeed/errors_test.go	Tests error taxonomy.
eventfeed/event.go	Defines feed events.
eventfeed/event_test.go	Tests event decoding.
eventfeed/export_test.go	Exposes test-only hooks.
eventfeed/filestore.go	Implements file checkpoints.
eventfeed/filestore_test.go	Tests file checkpoints.
eventfeed/filters.go	Defines filter validation.
eventfeed/filters_test.go	Tests filter validation.
eventfeed/loop.go	Implements connector lifecycle.
eventfeed/loop_test.go	Tests lifecycle behavior.
eventfeed/reconnect_test.go	Tests reconnect and staleness.
eventfeed/recovery.go	Implements poll recovery.
eventfeed/recovery_test.go	Tests recovery paths.
eventfeed/scenario_conformance_test.go	Replays conformance fixtures.
eventfeed/scenario_fixture_test.go	Decodes fixture contracts.
eventfeed/scenario_harness_test.go	Provides scenario harnessing.
eventfeed/scenario_selftest_test.go	Tests driver strictness.
eventfeed/seams.go	Defines connector seams.
eventfeed/transport.go	Enforces cable URL policy.
eventfeed/transport_contract_test.go	Defines transport contract tests.
eventfeed/transport_test.go	Tests cable URL policy.
eventfeed/websocket_transport.go	Implements WebSocket transport.
eventfeed/websocket_transport_test.go	Tests real WebSocket behavior.
eventfeed/feedtest/clock.go	Adds deterministic virtual time.
eventfeed/feedtest/clock_test.go	Tests virtual time.
eventfeed/feedtest/minter.go	Adds scripted ticket minting.
eventfeed/feedtest/minter_test.go	Tests scripted minting.
eventfeed/feedtest/polls.go	Adds scripted polling.
eventfeed/feedtest/polls_test.go	Tests scripted polling.
eventfeed/feedtest/store.go	Adds scripted checkpoints.
eventfeed/feedtest/transport.go	Adds scripted cable transport.
eventfeed/feedtest/transport_test.go	Tests scripted transport.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/filestore.go:230

• The rename is atomic but not durable without syncing the staged file and parent directory. After a system crash, the first checkpoint file can disappear; the next Load then reports Missing and starts at the present, which can skip history rather than merely replay from an older position. Since Save and the package advertise durable checkpointing, sync the file before rename and the directory after rename, or stop claiming crash durability and avoid treating disappearance as a safe present entry.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ceb8398f4c

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated no new comments.

Suppressed comments (4)

go/pkg/basecamp/eventfeed/websocket_transport.go:73

• The offered subprotocol is not verified after the handshake. coder/websocket v1.8.15 accepts a 101 response with an empty Sec-WebSocket-Protocol, so this can return a connection even though actioncable-v1-json was never negotiated, contrary to the CableTransport contract. Check conn.Subprotocol() and reject/close a missing selection; add a server case that intentionally selects none.
  go/pkg/basecamp/eventfeed/catchup.go:302
• This parks an already-observed overflow until the poll returns. SPEC §23 requires semantic signals at the first consumer-context opportunity after the condition arises (with “before the next save” only as the outer bound), so a stalled poll can postpone the handler forever even though this goroutine has received the dropping frame. Dispatch the overflow immediately here; an Accept disposition can continue awaiting the poll, while Terminate should cancel the attempt/poll.

			} else if over {
				// The buffer, not the socket: the call is unaffected and is
				// awaited to completion, and the drop's disposition runs
				// before this page's position moves anything durable.
				l.deferred = &deferredFrame{item: item, overflow: true}
				if l.hooks.frameDeferred != nil {
					l.hooks.frameDeferred(true)
				}
				r := <-done
				return r.page, false, r.err

go/pkg/basecamp/eventfeed/websocket_transport.go:73

• checkCableURL explicitly accepts case-insensitive schemes (the new unit test includes WSS://), but this passes that original spelling to coder/websocket. In v1.8.15 its handshake switch recognizes only lowercase ws/wss, so a URL accepted by policy fails as a transient dial and is retried indefinitely. Normalize only the scheme before dialing, while leaving the ticket-bearing remainder unchanged.

This issue also appears on line 70 of the same file.
go/pkg/basecamp/eventfeed/websocket_transport.go:183

• This synchronous graceful close can block teardown for several seconds: coder/websocket v1.8.15 waits up to 5 seconds to write the close frame and another 5 seconds for the peer response. Because dispose calls this before cancelling the attempt, caller cancellation, Connector.Close, terminal outcomes, and reconnects can all stall on a live peer that ignores the handshake. Use a bounded teardown strategy that preserves the required close frame without letting the library's full handshake timeout delay the universal Closed edge.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/catchup.go:504

• A live event admitted by drainScan is stranded when the buffer was empty at the start of this iteration: batch stays empty, so this returns even though the scan just repopulated l.buffer. Streaming never drains that buffer, delaying the event until a later repair walk and allowing caught_up (and a held save) to happen first. Continue the loop when the scan added events.

		batch := l.buffer.take()
		if out, done := l.drainScan(at, &budget); done {
			return out, true
		}
		if len(batch) == 0 {
			return cycleOutcome{}, false

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6eb58e6010

ℹ️ 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".

[jeremy]

Round-2 note on the suppressed comments, since three of them are load-bearing and one changed the design.

catchup.go:302 (overflow parked until the poll returns) — taken, and it reframed the visible finding above it. Rather than add a third dispatch site I deleted the overflow deferral outright; admitDuringPoll now dispatches at the drop's instant. Details in the thread. Fixture 22's own description already called drop-time dispatch normative and its post-poll rendezvous "scheduling slack only", so the implementation now matches what the merged fixture says it should do.

catchup.go:504 (live event stranded by drainScan) — real, and a defect introduced by my own round-1 change: drain returned on len(batch) == 0 even when the scan had just repopulated the buffer, so the event waited for a later repair walk while caught_up and the held save went first. Now gated on l.buffer.empty() too; red proof showed [event 41 save pos-1] where [event 41 event 42 save pos-1] was required.

websocket_transport.go:73 ×2 and :183 — all three taken and in progress: the negotiated subprotocol is never verified after the handshake, a WSS://-spelled URL passes policy but the library's dial switch is lowercase-only (so it fails transient and retries forever), and the graceful close can block teardown on a peer that ignores the handshake. That last one has to keep the close frame that nine fixtures pin — an earlier round caught the opposite bug — so it is getting a bound rather than a reordering.

Thanks for putting the sharp ones in the suppressed block; they've been the most useful part of both rounds.

[jeremy]

Closing out the remaining suppressed comments from round 2 (02fec5f). Two fixed, one declined with evidence.

Unverified subprotocol — real, fixed. Confirmed in the library rather than assumed: coder/websocket@v1.8.15/dial.go:270-283, verifySubprotocol returns nil when Sec-WebSocket-Protocol is absent, and the server side (accept.go, selectSubprotocol) sets no header at all when it selects nothing — so a 101 that negotiated nothing yielded a live connection. A mismatched selection the library does reject, leaving the empty case as the only reachable one, exactly as you said. Now conn.Subprotocol() must match actioncable-v1-json after the handshake; on mismatch the socket is torn down and the dial fails DialPolicy. Policy rather than transient is a deliberate call: a fresh mint returns a URL pointing at the same server, which will select the same nothing, so retrying forever against a server that cannot speak the protocol is the wrong shape — the redirect refusal already lands there for the same structural reason. Test includes the server-selects-none case you asked for.

Unbounded graceful close — real, fixed. close.go:99-128,157-228: Close writes the close frame under a hardcoded 5s context, then waits another 5s for the peer, then waitGoroutines. The phases can't be bounded separately (closeHandshake is unexported, no exported close-frame writer), and CloseNow is not an escape hatch once Close is in flight — casClosing has already flipped, so it just waits too. Bound is 1s, run off-caller: only the write is contractual (§23 needs the peer to see the frame, which is why dispose closes before cancelling — this is a bound, not the reordering an earlier round rejected), and that write is a control frame to an open socket bounded by the kernel send buffer, not by the peer. All 12 fixtures carrying expectClientClose still pass. Red proof: Close blocked past 3s against a peer that never answers; green returns at exactly the 1s budget, so it passes via the timeout path rather than a lucky response.

Case-insensitive scheme — not a defect, declined. net/url.Parse lowercases the scheme before anything downstream sees it ($(go env GOROOT)/src/net/url/url.go:454), and coder/websocket's dial switch runs on u.Scheme from its own url.Parse, so a WSS:// spelling arrives there already wss. Verified empirically, not just by reading: the new test dials a WS://-spelled loopback URL and passed against un-fixed code, with the ticket-bearing remainder byte-identical. No normalization added; the test stays as a regression pin binding checkCableURL's deliberate case-insensitivity to what actually dials.

[Copilot]

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated no new comments.

Suppressed comments (3)

go/pkg/basecamp/eventfeed/catchup.go:158

• A socket outcome deferred while this poll was in flight is skipped when the poll itself fails. For terminal poll branches, recoverPoll calls disposeAttempt, which clears l.deferred; for retryable failures, the deferred frame can remain undispatched through arbitrarily many retries. In particular, an invalid_event_stream_command observed during CatchingUp can be replaced by poll_failed/authorization_failed or delayed indefinitely, despite SPEC §23 requiring protocol-fatal to terminate from every socket-open state. Dispatch the already-observed socket outcome here before applying poll recovery; a failed poll has no successful page boundary left to finish.

		if p.err != nil {
			step, out, done := l.recoverPoll(at, cursor, p.err)

go/pkg/basecamp/eventfeed/transport.go:40

• Checking u.Host does not ensure that the URL has a hostname. For example, wss://:443/cable has Host == ":443" but an empty Hostname(), so it passes the policy check and is classified as a transient dial failure instead of terminal invalid_cable_url, causing repeated re-mints/dials for a structurally unusable URL.
  go/pkg/basecamp/eventfeed/cable.go:250
• The tier-2 push-event schema requires all nine keys, including presence-bearing visible_to_clients, and SPEC §23 treats a correlated message missing a required event key as an invalid frame. Omitting this field from the presence checks accepts both an absent value and JSON null, exposing a nil value on a push event instead of taking the socket-failure recovery edge.

		{"creator_id", p.CreatorID != nil},
		{"recording_id", p.RecordingID != nil},

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02fec5ff1f

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated no new comments.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/catchup.go:160

• A deferred socket outcome is skipped when the in-flight poll returns an error. recoverPoll may retry, increment authorization failures, or terminate, and disposal then clears l.deferred; this can even swallow an already-observed invalid_event_stream_command instead of producing protocol_fatal. Since no page succeeded, dispatch the deferred outcome before classifying the poll error (the finish-page ordering only applies to successful pages).

		if p.err != nil {
			step, out, done := l.recoverPoll(at, cursor, p.err)
			if done {
				return out, "", true

[jeremy]

Holding two findings open deliberately: the in-flight-poll mechanism has drawn four rounds

Two round-4 findings — the suspendable bound in awaitSupersededPoll (thread-adjacent) and the deferred socket outcome swallowed on the poll-error path (suppressed, catchup.go:160) — are not being fixed in this round. Both are correct. I am not writing the next patch on that mechanism until its shape is settled, because the pattern is now the finding.

The ledger on one mechanism, in order:

1. Round 1 (Codex P1): a stalled PollSource holds the consumer's goroutine when the socket dies → added awaitSupersededPoll, bounding the wait by the staleness window.
2. Round 2 (Copilot + Codex, P1): a deferred overflow dispatches after the save, and is dropped entirely on failed-poll paths → removed the overflow deferral; admitDuringPoll dispatches at the drop's instant.
3. Round 3 (Codex): the fatal-frame scan's budget was sized by the live-buffer capacity, so a fatal could hide behind one ping → rebounded on the pump queue's own depth.
4. Round 4 (Codex, this round): the staleness bound from step 1 is suspendable by the very pump backpressure that creates the problem, so a misbehaving peer can keep a compliant stalled poll alive indefinitely. Plus (suppressed): the socket deferral is cleared by disposal when the poll returns an error, which can swallow an already-observed invalid_event_stream_command instead of producing protocol_fatal.

Each fix was principled and each was verified, but four rounds of edge-findings on one structure is evidence about the structure. What they all orbit: the state machine both awaits a poll seam call and services the frame queue during it, parking one out-of-band frame in a single l.deferred slot, with a bound borrowed from a timer whose evaluation the queue itself can suspend.

The concurrency is not optional — fixtures 01 and 19 require a live frame admitted after confirm but before the entry page is served, and transition 21 requires the in-flight page to be finished before the dying socket is observed. So this is not a mechanism that can simply be deleted, which is precisely why it wants a decision rather than a fifth patch. The candidate shapes:

• (a) Patch in place — give the superseded-poll wait its own deadline from the injected clock (unsuspendable), and dispatch a deferred socket outcome before recoverPoll classifies a poll error, since the finish-page ordering only applies to pages that succeeded. Small, local, and both findings close.
• (b) One event loop — stop blocking on the poll at all: make the poll result just another case in the same select that reads frames, so there is no deferral slot, no superseded wait, and no borrowed bound. Larger, restructures the core of catchup.go, and dissolves this whole class.

I lean (a) plus a note, because the deferral that remains is the one SPEC explicitly sanctions (transition 21's deferred consumption) and (b) risks the delivery-ordering guarantees that nine fixtures and the save-ordering invariant pin. But it is a judgment call about this PR's core, so it goes to a human rather than to me.

The other three round-4 findings (synchronous cancellation on Close, visible_to_clients required on push frames, and the drain batch escaping the live-buffer ceiling) are independent of this and are being fixed now.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9103dd83a

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 54 out of 55 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

go/pkg/basecamp/eventfeed/loop.go:152

• ctx is the attempt context, which is a child of runCtx. Connector.Close (or caller cancellation) therefore cancels the pump immediately, before the state machine can reach dispose and call conn.Close. The default WebSocket read may abort the socket on that cancellation, so normal shutdown can still produce the abrupt disconnect that the close-before-cancel ordering is meant to prevent. Give the pump a cancellation scope that is canceled only by disposal after the socket close; HTTP seam calls can continue using the attempt context.
  go/README.md:615
• This says the connector performs no wire I/O, but the package directly performs the sanctioned WebSocket cable dial. That contradicts both the implementation and the architecture rule. Limit the claim to HTTP requests and explicitly name the cable dial exception.

**Experimental: the Layer-1 seam adapters have not landed yet.** The connector performs
no wire I/O of its own — every HTTP exchange reaches the wire through a seam backed by
a generated operation — and the adapters that build those seams over the generated
`CreateStreamTicket` and `PollEvents` operations are still to come. Until they do, a
consumer must supply the `TicketMinter` and `PollSource` implementations itself, and the
exported surface may still change as they land.

go/pkg/basecamp/eventfeed/loop.go:638

• Cancellation is not re-checked after Load. A custom store that honors the supplied context can return context.Canceled when the caller cancels or calls Close, and this path then yields checkpoint_load even though cancellation is documented to end iteration cleanly. Match the mint/poll paths by giving runCtx cancellation precedence over the load error.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 93a3d5a457

ℹ️ 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".

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 56adfee3b5

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 2 comments.

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Nice work!

Reviewed commit: 7d5632f1ff

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/scenario_conformance_test.go:1029

• RetryAfter is populated before the status is classified, so 401/403 and 404/422 responses produce unauthorized/unrecoverable seam errors carrying a retry delay. SPEC.md:3087 and 3568-3574 define retry_after only for throttled and explicitly say unauthorized carries none. Assign the field only in the throttled branch (and update the contradictory comment above) so the tier-2 adapter model matches the seam contract.

	mintErr := &eventfeed.MintError{RetryAfter: retryAfter, Err: fmt.Errorf("mint responded %d", *respond.Status)}

[jeremy]

@codex review

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/catchup.go:370

• Dispatch the parked socket outcome before checking staleness here. pollPage replaces the staleness timer with the grace wake and latches expired; if delivery, PageDelivered, or the checkpoint save lasts until that wake fires, socketCheck consumes it as a new stale verdict before the next loop iteration reaches dispatchDeferred. On a page with Next, this can turn an already-deferred invalid_event_stream_command into a reconnect instead of Terminal(protocol_fatal) (and similarly misreport other deferred outcomes). Probe and dispatch the deferral at this page boundary before running the ordinary socket check.

		if out, done := l.socketCheck(at); done {

[jeremy]

@codex review

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (5)

go/pkg/basecamp/eventfeed/catchup.go:631

• The result arm bypasses the fixed grace deadline. If the poll result and deadline wake are both ready, Go may select this arm and accept the page even though the deadline has elapsed, making the published abandonment bound nondeterministic. Recheck the clock in this arm so the deadline wins regardless of select ordering.

		case r := <-done:
			return pollAttempt{page: r.page, err: r.err}

go/pkg/basecamp/eventfeed/scenario_conformance_test.go:1032

• This attaches RetryAfter to 401/403 outcomes, but §23 defines MintUnauthorized as carrying no retry-after; the driver is supposed to model the Layer-1 seam contract. Parse and assign the header only for the retryable default branch (which also avoids attaching it to terminal 404/422 outcomes).

	retryAfter, present := d.retryAfterFrom(respond.Headers)
	mintErr := &eventfeed.MintError{RetryAfter: retryAfter, Err: fmt.Errorf("mint responded %d", *respond.Status)}
	switch *respond.Status {
	case 401, 403:
		mintErr.Kind = eventfeed.MintUnauthorized

go/pkg/basecamp/eventfeed/scenario_conformance_test.go:1019

• This comment contradicts the §23 contract: unauthorized mint outcomes carry no retry-after, so row 4 must use only the local backoff draw. Update it alongside the classification fix to avoid documenting the opposite behavior.

// drop it at another. Unauthorized keeps a parsed value too — row 4 floors
// the below-threshold reconnect delay on it.

go/pkg/basecamp/eventfeed/connector.go:598

• The “safe from anywhere else” claim is too broad. Wait also self-deadlocks when called from a host seam method the run is awaiting (for example TicketMinter.Mint, PollSource.Poll, or a checkpoint-store call), even though those are not consumer callbacks and Poll may run on another goroutine. Document that callers must use an independent coordinator rather than any code the connector is waiting to return.

// It is not callable from a consumer callback. Every callback — an observer, a
// signal handler, the loop body — runs ON the run goroutine, so waiting for
// that goroutine from inside one waits for itself. Close is the call that is
// safe from anywhere; this is the one that is safe from anywhere ELSE.

SPEC.md:3029

• This normative restriction is narrower than the actual deadlock boundary. A host seam method can call wait() while the run is awaiting that method, creating the same cycle even though it is not a consumer callback. Define the rule in terms of any execution the connector is waiting to return so all SDKs document the same safe usage.

  `wait()` is not callable from a consumer callback, for the reason `close()` does not wait.

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. 🚀

Reviewed commit: 1f069f9073

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

go/pkg/basecamp/eventfeed/catchup.go:523

• This frame arm does not recheck the staleness timer after select chooses a ready frame. If the deadline fired first but the frame became ready too, staleHolder latches the expiry, yet this path admits the frame into the loop-wide buffer before the next iteration observes staleness; that event can survive teardown and be delivered after reconnect. loop.go:1136-1155 explicitly guards the identical race in awaitConfirmation. Apply the same authoritative-expiry arbitration here before calling admitDuringPoll.

		case item, ok := <-at.lc.frames:
			if !ok {
				l.deferred = &deferredFrame{closed: true}
			} else {
				handled, out, ended := l.admitDuringPoll(at, item)

go/pkg/basecamp/eventfeed/catchup.go:792

• When pollPage already consumed and parked an authoritative staleness expiry, the new grace timer is not ready here, so this probe falls through to admissionPass. A queued event received after the expiry is then admitted before the next loop iteration dispatches the deferred stale outcome, allowing it to survive reconnect. Skip normal admission when a stale outcome is already deferred (while still preserving the protocol-fatal scan that outranks it).

	staleTimer, staleGen := at.lc.stale.current()
	select {
	case <-staleTimer.C():
		if age, ok := at.lc.stale.evaluate(staleGen); ok {
			l.disposeAttempt(at, nil)
			if l.cfg.observer.StaleConnection != nil {
				l.cfg.observer.StaleConnection(age)
			}
			l.observeDisconnected("", errStaleConnection)
			return cycleOutcome{kind: outcomeFailed}, true
		}
	default:
	}
	return l.admissionPass(at)

go/pkg/basecamp/eventfeed/catchup.go:1005

• fatalScan also admits ordinary live events even when the occupied deferral is an authoritative staleness expiry. On a final poll page, those post-expiry events are added during drain and immediately replayed as “already-admitted” buffered events before staleness is dispatched, contrary to §23's requirement that a stale socket be detected before further live-socket delivery. With a deferred stale verdict, this scan should retain only the protocol-fatal override and discard other newly dequeued frames.

	for *budget > 0 {
		select {
		case item, ok := <-at.lc.frames:
			if !ok {
				l.deferForDrain(&deferredFrame{closed: true})
				return cycleOutcome{}, false
			}
			*budget--
			handled, out, ended := l.admitDuringPoll(at, item)
			switch {
			case ended:
				return out, true
			case handled:
			default:
				if f, ok := protocolFatalFrame(item); ok {
					return l.terminateProtocolFatal(at, f), true
				}
				l.deferForDrain(&deferredFrame{item: item})
			}

go/pkg/basecamp/eventfeed/scenario_conformance_test.go:1032

• This driver attaches a parsed Retry-After to unauthorized and unrecoverable mint errors, although the seam contract permits it only on retryable transient/throttled outcomes; loop.go:1066-1070 also explicitly states that unauthorized carries none. That makes tier 2 manufacture an impossible Layer-1 result. Assign RetryAfter only in the retryable default branch and correct the contradictory comment.

	retryAfter, present := d.retryAfterFrom(respond.Headers)
	mintErr := &eventfeed.MintError{RetryAfter: retryAfter, Err: fmt.Errorf("mint responded %d", *respond.Status)}
	switch *respond.Status {
	case 401, 403:
		mintErr.Kind = eventfeed.MintUnauthorized

[jeremy]

@codex review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Bravo.

Reviewed commit: a8d2d1b790

ℹ️ 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".

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

[jeremy]

@codex review

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

Suppressed comments (1)

go/pkg/basecamp/eventfeed/catchup.go:995

• Once the deferral slot is occupied, later frames still pass through admitDuringPoll. A correlated message queued after a deferred disconnect or staleness expiry is therefore added to the loop-wide buffer (and may even trigger overflow), survives that attempt's teardown, and is delivered after reconnect. This breaks the stated stop-at-the-deferred-receive ordering; after the first deferred outcome, the scan should only look for the protocol-fatal carve-out and discard every other later item.

			handled, out, ended := l.admitDuringPoll(at, item)

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Keep them coming!

Reviewed commit: ecea7a3460

ℹ️ 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".

[jeremy]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ecea7a3460

ℹ️ 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".

[chatgpt-codex-connector]

Check staleness before accepting a subscribe write

When WriteFrame completes while the staleness timer is already ready, this select can take written first without evaluating the fired window. A write error is then reported as the socket failure instead of firing StaleConnection, while a successful first subscribe proceeds to allocate a confirmation timer and enter AwaitingConfirmation before the authoritative staleness edge from SPEC.md transitions 9/15 is handled; a blocking custom Clock.NewTimer can delay that teardown indefinitely. Re-probe and evaluate staleTimer in this arm before dispatching the write result.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new comments.

[jeremy]

Disposition for Copilot's suppressed comment on the exact-head round (catchup.go:995, "later frames still pass through admitDuringPoll after the deferral slot is occupied"): declined.

The "stop at the deferred receive" sentence it cites governs which OUTCOME is reported — the first parked socket outcome is the one dispatched, preserving arrival order for the cycle-ending verdict — not a freeze on admissions. The scan's continuation past an occupied slot is the point of the #760 fix: a protocol-fatal frame the pump had already read must govern even when a recoverable outcome is parked ahead of it, and ending the scan at the slot is exactly the hole that fix closed.

Admitting correlated events behind the parked outcome is the live buffer doing its stated job (they are the in-flight stragglers it exists to carry, bounded by liveBufferCapacity with overflow dispatched at drop time), and delivery after reconnect is covered by the loop-wide dedupe lane, so a straggler that also arrives through the next attempt's catch-up cannot double-deliver. The proposed remedy — discard every non-fatal item after the first deferral — would turn received events into silent data loss to enforce an ordering the contract does not state.