Event feed connector: foundations (1/3)

[jeremy]

First half of the SPEC.md §23 event feed connector, split out of #705 so the
state machine can be reviewed on its own. #705 keeps the run loop, catch-up,
recovery and the tier-2 driver, and now stacks on this.

Eight bot rounds on #705 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 found a P1
credential defect that all eight missed because it composes two files across a
package boundary. Splitting is the response to that shape.

Size: ~2.7k lines of production code, ~7k with tests.

What is here

Everything the run loop is built from and nothing that runs — each piece
testable without starting a feed:

seams.go	TicketMinter / PollSource / CableTransport / CableConn
event.go	Event, Cursor, Page, Signal, Disposition, Observer
errors.go	TerminalError and its reason codes
filters.go, digest.go	the filter set, its fail-closed validation, the srv1 key
checkpoint.go	CheckpointKey / FlatKey / CanonicalOrigin, the store seam
continuation.go	the §8 same-origin validation algorithm
filestore.go	the built-in FileCheckpointStore
dedupe.go, backoff.go, clock.go, cable.go	LRU, reconnect schedule, timers, frame codec
transport.go, websocket_transport.go	cable-URL policy, the default transport
feedtest/	host-supplied fakes for all four seams

There is no consumer entry point yet: New and the loop are on #705.

The four fixes

1. The cable dial takes no credential it was not given (P1)

The cable origin is chosen by the server — the mint returns a url and the
connector dials it verbatim, cross-host by design — and the short-lived ticket
in its query is the only credential that origin is entitled to. Two paths handed
it more.

WebSocketTransport.HTTPClient, or http.DefaultClient when nil. An
*http.Client carries three credentials invisible at the call site: a
RoundTripper may inject Authorization, a Jar attaches cookies, a
TLSClientConfig may present a client certificate. The DefaultClient fallback
is the same hazard with no call site at all. Deleted rather than validated —
a RoundTripper is opaque, so no runtime inspection could accept one client and
refuse another. Handshakes now run on a package-owned client. (The field had no
callers anywhere, so nothing regressed with it.)

URL userinfo. net/http's send() turns it into a Basic Authorization
header, so a mint whose url carried userinfo made the connector authenticate
to a server-nominated origin with a credential the server chose. Refused before
any network I/O.

The proxy. A wss:// handshake reaches a proxy as CONNECT host:port, so
the ticket stays inside the tunnel; a ws:// handshake is forwarded in absolute
form, putting /cable?ticket=… in the proxy's request line and access log in
the clear. Reachable, not theoretical: §9 admits ws:// for *.localhost, and
net/http's proxy rules exempt the literal localhost and loopback IPs but
not .localhost subdomains. Cleartext dials no longer proxy; TLS dials
still do.

Pre-fix transcript, against un-fixed code:

handshake 0 sent Authorization: "Bearer caller-bearer-token"
handshake 0 sent Proxy-Authorization: "Basic cHJveHk6c2VjcmV0"
handshake 0 sent Cookie: "session=caller-session-cookie"
the server accepted 1 connection(s); the refusal must precede all network I/O

and from the proxy sentinel with the cleartext exemption removed:

a cleartext cable dial reached the proxy 1 time(s):
  ["GET http://app.localhost:9/cable?ticket=sekrit-ticket-value"]

TestCableHTTPClient_IsWiredShut exists because the first mutant written
against the proxy fix survived: Proxy: proxyFromEnvironment captures the
var's value at init, so the behavior tests' sentinel never reached it — meaning
both would pass a regression to Proxy: http.ProxyFromEnvironment. The wiring
assertion holds the shape they cannot observe.

2. A suppressed duplicate is not a delivery

§23 defines the LRU as "actually-delivered event ids", recorded by every
delivery. Seen refreshed recency on a hit, which is the case where the
event is suppressed and no delivery happens. §23 says to expect poll-vs-push
duplication continuously, so a hot id was pinned at the front and evicted ids
delivered once and never seen again — which become eligible for exactly the
re-delivery the LRU prevents.

TestDedupe_HitRefreshesRecency is inverted to ..._HitDoesNotRefreshRecency,
and I am calling that out rather than letting it look like a test edited to
accept a fix. It asserted the negation of the contract; nothing short of
inverting it is honest.

3. The checkpoint store reads a bounded regular file, under one lock

#761: the lock registry was keyed on the exact path spelling. On APFS or
NTFS feed.json and Feed.json are one file, so two stores took two mutexes —
the lost update the registry exists to prevent, reached by two call sites
disagreeing about capitalization. The lock key is now case-folded; the path each
store reads and writes is not.

The read followed whatever the path named. Against the pre-fix read, all four
cases fail:

directory       ... reported "is a directory", not a store-type refusal
FIFO            ... did not return (open blocks until a writer appears)
/dev/zero       ... did not return (reads without end)
8 MiB + 1 file  ... read whole, then failed on a JSON parse error

The FIFO and device cases are hangs, so every assertion runs under a bound. Not
defensive dressing: the first draft bounded only the FIFO, and /dev/zero took
the package's 45s timeout with it, naming nothing.

4. One observer-safe URL redactor

The primitive #705 applies to every URL-bearing observer surface. Reduction is
via CanonicalOrigin rather than truncation at ?, which matters for the case
a naive redactor misses: userinfo is a credential in the authority, so
https://attacker:hunter2@evil.example/steal?ticket=… survives query-stripping
intact and does not survive this.

Verification

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

• go build / go vet / go test -race -count=1 / -count=5 — all pass
• make go-lint — 0 issues
• gosec -severity high -exclude-dir=pkg/generated on the CI-pinned v2.23.0
  (module hash verified, not a scratchpad binary) — 0 issues
• full make check — exit 0

Every fix was red-proven before it was written, and every test mutation-checked
after. Three tests were rewritten because mutation showed them vacuous.

One preparatory commit

1646f2c1f relocated the run-coupled declarations out of checkpoint.go and
continuation.go so the two halves fall on file boundaries. The moved function
bodies are byte-identical. It also added direct tests for checkContinuation
and Filters.clone, both of which were only reachable through a full run.

Development history, review threads and proof lineage for every file here are on
#705, preserved at tag pre-split/705-head.

Five more from review

Copilot's rounds on this branch found five further defects; all five are fixed, each
red-proven against the un-fixed code first.

6c6e14f16	A null type is not a broadcast. A *string gives the same nil for an absent key and a JSON null, so {"type":null} was liveness-only while {"type":null,"identifier":…,"message":…} was delivered as an event — one wire value in two classes depending on its siblings. Presence is now decoded separately. A present-but-null type takes the ignore branch, not the reject branch: it names no type to recognize, which is §23's unrecognized-type case. BC3's push lane sends {identifier, message} with no type key at all, checked against the current head of bc3 #9659, so the narrowing drops nothing real.
fae998b16	Close bounds the read, not just itself. closeGraceBudget stopped Close from waiting out the close handshake, but the socket is what releases a parked read and coder/websocket does not tear it down until its own 5s+5s ends — so a pending ReadFrame stayed blocked four seconds after Close returned. Worse, a background read was uncancellable: the library installs its cancellation hook only when the read context has a Done channel, and ReadFrame(context.Background()) is how a run loop parks a pump. The connection now owns a lifetime context every read and write derives from, cancelled once Close is done waiting — after the budget, never before, so the close frame is still written.
60a28700d	The package docs described the finished connector. There is no Connector, no constructor and no run loop here, and the docs said "runs the whole protocol". Now: foundations only, what has landed, both pending pieces, and that everything below documents the architecture they implement. AGENTS.md's row carried the same overclaim.
791143207	The store's write is bound like its read. The read refuses a file past 8 MiB; the write did not, so a Save crossing the cap renamed into place a file nothing can read again — and since Save reads before it writes, Save too. No in-band recovery; the operator must delete the file, discarding every other lineage's cursor. Reaching the cap is accretion, not an adversary: there is no delete, so a filter change leaves the old lineage in the file forever. Refusing degrades to the documented failed-save outcome instead of an unrecoverable one.
b15e8d031	Cancellation outranks a local close on entry too. ReadFrame documents the precedence and honored it on the way out but not on the way in, so a cancelled read over a closed connection reported a connection failure — the shutdown a run loop performs. WriteFrame already checked the context first; the two disagreed. The assertion went in the shared transport contract, where the feedtest fake already passed it and the real transport did not.

Two findings were declined on merit with the reasoning in a comment rather than left open, and a third — the symlinked store path versus atomic rename — is flagged for a human call: it is the third round on one file, and every candidate remedy trades away a different documented property of what a store file's identity is.

---

Summary by cubic

Lays the foundations for SPEC §23 Event Feed: seams, wire types, default github.com/coder/websocket transport, a file-backed checkpoint store, and deterministic fakes. This also hardens transport and storage paths to avoid leaks and torn files.

• Bug Fixes

  • Load returns a usage-coded error when any key component is invalid before lookup.
  • Saves fsync the staged file; directory fsync runs on non-Windows only.
  • Refused subprotocol comparisons are exact; a wrong-case or unoffered selection is now a policy refusal instead of a transient retry.
  • The file checkpoint store treats duplicate JSON keys as corruption (counts members), not last-wins; null positions no longer mask duplicates.
  • The store’s lock registry key uses Unicode case folding (SimpleFold) to prevent split locks for differently cased spellings on case-insensitive filesystems.
  • Absolute-path resolution failures now surface immediately; Load/Save report the error and avoid filesystem I/O to prevent identity drift across chdir.
  • Clarifies and pins that raw read failures are not flattened and never render dialed URL components, with a tripwire test that ensures no ticket leakage from read errors.

• Migration

  • If you require proxy egress, implement a custom CableTransport; the default transport does not use proxies.

^{Written for commit ead96a5. Summary will update on new commits.}

[Copilot]

Pull request overview

Introduces the foundational Go event-feed components required by the forthcoming connector run loop.

Changes:

• Adds event models, seams, codecs, filtering, deduplication, timing, and checkpoint persistence.
• Adds a credential-isolated WebSocket transport and URL security policies.
• Adds deterministic test fakes and extensive contract/unit coverage.

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 41 out of 42 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
AGENTS.md	Registers the sanctioned event-feed architecture.
go/go.mod	Adds the WebSocket dependency.
go/go.sum	Records dependency checksums.
eventfeed/backoff.go	Implements retry and repair jitter.
eventfeed/backoff_test.go	Tests timing boundaries and saturation.
eventfeed/cable.go	Implements Action Cable frame codecs.
eventfeed/cable_test.go	Tests frame parsing and commands.
eventfeed/checkpoint.go	Defines checkpoint identity and store seam.
eventfeed/clock.go	Provides the timer abstraction and system clock.
eventfeed/clock_test.go	Tests system timer registration.
eventfeed/continuation.go	Validates continuation origins.
eventfeed/continuation_test.go	Tests continuation security policy.
eventfeed/dedupe.go	Implements delivered-event LRU deduplication.
eventfeed/dedupe_test.go	Tests deduplication and eviction.
eventfeed/digest.go	Implements canonical filter digests.
eventfeed/digest_test.go	Verifies shared digest fixtures.
eventfeed/doc.go	Documents the package architecture.
eventfeed/errors.go	Defines terminal errors and reasons.
eventfeed/errors_test.go	Tests error taxonomy and rendering.
eventfeed/event.go	Defines event payloads.
eventfeed/event_test.go	Tests payload presence semantics.
eventfeed/filestore.go	Implements bounded atomic checkpoint storage.
eventfeed/filestore_test.go	Tests persistence, locking, and file safety.
eventfeed/filters.go	Defines and validates feed filters.
eventfeed/filters_test.go	Tests validation and cloning.
eventfeed/redact.go	Redacts observer-facing URLs.
eventfeed/redact_test.go	Tests credential-safe URL rendering.
eventfeed/seams.go	Defines connector interfaces and public types.
eventfeed/transport.go	Implements cable URL policy.
eventfeed/transport_test.go	Tests URL and proxy policy.
eventfeed/transport_contract_test.go	Defines the shared transport contract.
eventfeed/websocket_transport.go	Implements the default WebSocket transport.
eventfeed/websocket_transport_test.go	Tests real transport behavior and security.
eventfeed/feedtest/clock.go	Provides deterministic virtual time.
eventfeed/feedtest/clock_test.go	Tests virtual timer behavior.
eventfeed/feedtest/minter.go	Provides a scripted ticket minter.
eventfeed/feedtest/minter_test.go	Tests minter scripting and cancellation.
eventfeed/feedtest/polls.go	Provides a scripted poll source.
eventfeed/feedtest/polls_test.go	Tests poll scripting and cancellation.
eventfeed/feedtest/store.go	Provides a scripted checkpoint store.
eventfeed/feedtest/transport.go	Provides a scripted cable transport.
eventfeed/feedtest/transport_test.go	Tests fake connection behavior.

---

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

[Copilot]

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

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

• A concurrent repeat Close returns as soon as closed is set, while the first call may still be waiting for the graceful handshake and has not canceled lifetime. Pending reads/writes can therefore remain blocked after that Close has returned, violating the CableConn.Close contract. Publish a shared completion channel/result so every concurrent caller waits for the first teardown to finish.
  go/pkg/basecamp/eventfeed/filestore.go:398
• This rename does not preserve the documented support for a symlink to a regular store file: atomic rename replaces the symlink itself, leaving its target unchanged. A later consumer opening the target sees the stale checkpoint, while this spelling sees a new unrelated file. Either reject symlink paths consistently or resolve and lock/write the target identity without breaking atomic replacement.
  go/pkg/basecamp/eventfeed/websocket_transport.go:310
• The method documents cancellation before local-close precedence, but this early return reverses it when both happen before entry. That can turn a canceled operation into a socket failure; WriteFrame already checks ctx.Err() first. Apply the same ordering here.

[jeremy]

Suppressed comments, round 2 — three findings, two verdicts and a stop

Copilot's review on 60a28700d put three findings in a Suppressed comments block, which never becomes a thread. Answering them here since there is nothing to reply to.

1. websocket_transport.go:310 — cancellation vs local close on entry — fixed (b15e8d031)

True, and the two methods disagreed with each other, not just with the doc: ReadFrame tested the local close first on entry while WriteFrame tested the context first. The case is reachable — cancel the run context, then close the connection is the shutdown a run loop performs — and it reported a cancelled read as a connection failure.

The assertion went into the shared transport contract, not this transport's own tests, because it is a statement about the seam every implementation owes. Red proof: the feedtest fake already satisfied it and passed; the real transport failed the same case (exit 1). That asymmetry is the finding in one line.

2. websocket_transport.go:414 — concurrent repeat Close — true, and deliberately not doing it

Correct as stated: a second concurrent Close returns as soon as closed is set, while the first may still be inside the graceful handshake and has not cancelled lifetime yet.

What that costs is now bounded by closeGraceBudget, one second, because the first caller cancels the lifetime in both arms of its select — the budget always fires. Before fae998b16 the same window was coder/websocket's 5s+5s. So the residual is: for at most a second, a second caller cannot yet assume reads are released, while the first caller is still waiting on exactly that.

The proposed remedy — a shared completion channel every concurrent caller waits on — buys that second by inverting a property the type documents on purpose: repeats are no-ops that return at once. It is a mechanism that earns its keep only on a path where two goroutines close one connection simultaneously, and it makes the common repeat-close slower to fix a one-second staleness on the rare one. The contract clause is about the operation releasing blocked I/O, and the first Close does that within its budget; it does not promise that a second caller observes the release as already complete.

Declining on merit, written here rather than left open. If a run loop ever does need "closed and quiesced" as one answer, that wants an explicit await on the seam, not a lock inside Close.

3. filestore.go:398 — symlinked store path vs atomic rename — stopping here, not patching

The finding is real. read uses Stat, not Lstat, specifically so "a symlink to a regular file still works — an operator pointing the store through a symlink is ordinary", and writeAtomic's os.Rename then replaces the symlink itself. Read follows the link; write destroys it, and any other consumer of the target keeps reading a checkpoint that stopped advancing.

I am not writing this patch, and the reason is the shape rather than the merit. Counting rounds on this one file:

1. dcdd1d65b — the read must refuse non-regular files, bound its size, and fold the lock key's case.
2. This round, in the thread above — the write must enforce the bound the read does.
3. This — the write must preserve the identity the read follows.

Three rounds, each individually small, each individually true, all circling one question nobody has been asked: what is a store file's identity — the path spelling, the resolved target, or the inode? The existing code has already answered it three different ways in three places. canonicalStorePath deliberately does not resolve symlinks, and says why: resolution requires the file to exist, and this file is created on the first Save, so a resolved key changes identity mid-life. read deliberately does follow them. writeAtomic replaces them. Each choice is defensible alone; together they are not one contract.

Every candidate remedy trades a different documented property away:

• Reject symlinks in read (Lstat) — removes support the doc calls ordinary.
• Resolve and rename onto the target — breaks atomic replacement across filesystems, and reintroduces the mid-life identity change the lock key was written to avoid.
• Document that a symlinked path is replaced by a regular file on first Save — honest, free, and leaves the surprise in place.

That is an authoring decision about what the store is, not a review-round fix, and picking one here would be the fifth selector on an instrument nobody has sized. Flagging it for a human call rather than guessing.

---

Everything else from this round is on b15e8d031. make go-check (vet + golangci + tests) and go test -race are clean under LC_ALL=C.

[Copilot]

Pull request overview

Copilot reviewed 41 out of 42 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

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

• Concurrent callers do not observe completion of the same close. The first caller sets closed before starting/waiting for the handshake, so a second caller can return nil immediately while the connection lifetime is still active and pending I/O remains blocked for up to the close budget. Since Close is documented as safe from any goroutine and as unblocking reads/writes, make repeated callers wait on a shared close-completion signal (and return the completed result) instead of treating an in-progress close as complete.

[jeremy]

Stopping here: this is the fourth round on one question, not two more patches

Round 3 on b15e8d031 raises two findings that are the same finding, and it is a finding this branch has already paid three rounds for. Naming the pattern instead of writing the next patch.

The pattern

dialFailure's own doc comment records the history, in the code, on this branch:

Three review rounds found three different spellings that slipped a model in turn: a value below a length threshold, a percent-encoded form, and a query carrying the credential with no = in it at all. That is not three bugs, it is one control that has to anticipate its own input, and the peer chooses the input.

That round ended by replacing the redactor with a closed vocabulary keyed on error types — and generalised it to exactly one call site. Two other observer-facing renderings were left on the older model, "compose once and bound by §9's MAX_ERROR_MESSAGE_LENGTH":

• invalidFrameError (cable.go) — appends cause.Error(), and time.Time's decoder quotes the offending input.
• CloseError.Error() (seams.go) — renders the peer's close Reason.

Round 3's two comments are the observation that truncation is not redaction, applied to those two sites. That observation is correct, and it is the same observation the earlier three rounds made. Four rounds, one question, two models live in one package.

What I think the real question is

Is MAX_ERROR_MESSAGE_LENGTH still the rule for peer-derived text in observer-facing errors, or did the closed-vocabulary decision supersede it everywhere peer text can reach an observer?

Until that is answered, any patch here is the fourth selector on an instrument nobody has sized — and the two obvious local fixes are both wrong in an instructive way:

• Drop the cause from invalidFrameError. Most causes are ours and already closed — decodeMessageEvent emits missing required key %q with the key drawn from a fixed nine-element set. Deleting the append to suppress the one decoder that quotes input throws away the package's own diagnostics and leaves the class no better specified. The right shape is dialFailure's: classify causes by type, render from a fixed vocabulary. That is a design change, not an edit.
• Render only the status code in CloseError.Error(). Cleaner in isolation — Reason stays on the struct, so callers keep it. But §23 dispatches on the reason string and documents Observer.disconnected as carrying an invalid-frame indication, so what these errors are allowed to say is a §23 conformance surface, mirrored in conformance/event-feed/schema.json and owed by all six SDKs. Changing it in Go alone would diverge the reference implementation from the spec the other five will implement, silently.

So this is a SPEC §23/§9 decision with a conformance-schema and six-SDK blast radius, not a Go-file fix, and it should not be made inside a review round on the foundations PR. Flagging it for a human call; the threads carry the same reasoning and are resolved so the PR is not held open on a decision that is not mine.

On merit, for the record

The findings are true but the actor is narrow: the entity that can trigger either is the cable server the mint pointed us at, which already holds the ticket — it received it in the handshake URL. Nothing is disclosed to a party that did not have it; what is at stake is our own short-lived credential landing in the operator's log aggregator. Real, worth fixing, and not urgent enough to justify guessing at the spec.

Also in round 3

• filestore_test.go:824 (Windows / syscall.Mkfifo) — declined, with evidence, in the thread. GOOS=windows GOARCH=amd64 go build ./... on this head exits 0: the shipped library is Windows-clean; only the test file is not, and no workflow in this repo runs Windows.
• The suppressed comment on websocket_transport.go:423 is round 2's concurrent-Close finding re-raised verbatim. It was declined with reasoning and that decline stands; a bot repeating an answered finding is more evidence about the instrument than about the code.

[jeremy]

Tracked as #788, so the analysis above survives this PR's squash-merge rather than living only in a comment thread.

The issue carries the question as posed here — whether MAX_ERROR_MESSAGE_LENGTH still governs peer-derived text or the closed-vocabulary decision superseded it — plus both wrong local fixes and why, the narrow-actor severity read, and the two shapes that would close it. Filing it does not decide it; the deciding constraint stays that §23's reason-string dispatch is a cross-SDK conformance surface, so the answer belongs in SPEC before it belongs in Go.

Not holding this PR on it. The threads are resolved because the decision isn't this PR's to make.

[jeremy]

A stacked-PR failure mode worth writing down: a moving base can silently disable Copilot review

Recording this on the base PR because the diagnosis is not discoverable from the symptom, and the next person to hit it will be looking at #777's history rather than at the child PR.

Symptom. Copilot posts, in place of a review:

Copilot wasn't able to review this pull request because it exceeds the maximum number of lines (20,000). Try reducing the number of changed lines and requesting a review from Copilot again.

What actually happened. #705 is stacked on this branch. When event-feed-foundations was force-pushed and rebased onto a newer main, #705's own head still sat on the old tip, so GitHub computed its diff from a stale merge base — folding this branch's commits into the child's diff:

	additions	files
Against the stale merge base	20,678	59
Against the correct base	12,698	19

The child PR crossed a reviewer's size limit without a single line of its own changing. Rebasing --onto the new base restored the real numbers and Copilot reviewed normally on the next push.

Why it is worth a note rather than a shrug. The failure is silent in the direction that matters: gh pr checks stays green, the thread count does not move, and the review that never happened looks exactly like a review that found nothing. It cost a full round here to notice, and only because the review body was read rather than the thread count.

Diagnosis, for next time. If a stacked PR's reviewer goes quiet or refuses on size, compare what GitHub thinks the diff is against what the branch actually carries:

gh pr view <n> --json additions,changedFiles   # GitHub, from the merge base
git diff --shortstat origin/<base>...HEAD      # what the branch really adds

A large disagreement means the base moved. git merge-base --is-ancestor <old-base> origin/<base> then says whether a plain rebase suffices or whether --onto is needed because the base was rewritten.

Both of this branch's moves have now been absorbed downstream: a2875be2c → 60a28700d (rewritten, needed --onto) and 60a28700d → b15e8d031 (fast-forward). No action is needed here — this is a note for the pattern, not a request.

[Copilot]

Pull request overview

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

Suppressed comments (1)

go/pkg/basecamp/eventfeed/seams.go:339

• DialError.Error() bypasses the 500-byte cap that §23 says still applies to other error renderings. checkCableURL places the server-supplied scheme or explicit port in Reason, and net/url accepts arbitrarily long valid schemes, so a malformed mint response can produce an arbitrarily large observer/log message. Apply the package truncation helper to the composed result.

[chatgpt-codex-connector]

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

Reviewed commit: 26d397d56f

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

[Copilot]

Pull request overview

Copilot reviewed 43 out of 44 changed files in this pull request and generated 1 comment.

[jeremy]

@codex review

[Copilot]

Pull request overview

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

go/pkg/basecamp/eventfeed/cable.go:378

• json.Decoder.Token decodes numbers as float64 unless UseNumber is enabled. Therefore a valid forward-compatible frame such as {"type":"future","value":1e1000} passes the RawMessage unmarshal but fails this duplicate-key walk and is treated as an invalid frame, contrary to the contract that unknown parseable frame types are ignored. Enable UseNumber before tokenizing so the duplicate check accepts the full JSON number grammar.

	dec := json.NewDecoder(bytes.NewReader(data))

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

• Resolving the final symlink for writes while retaining a lock keyed by the configured spelling means a store opened through the link and one opened through its target mutate the same file under different mutexes. Concurrent read-modify-write saves can therefore silently drop one lineage, even though the symlink tests describe link- and target-addressed consumers as one store. The lock identity and write target need one consistent identity, or the concurrency guarantee must explicitly exclude this supported alias.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 02a9b076c8

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

[Copilot]

Pull request overview

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

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be970aafe4

ℹ️ 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. Already looking forward to the next diff.

Reviewed commit: ead96a52d5

ℹ️ 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 43 out of 44 changed files in this pull request and generated 1 comment.

[jeremy]

@codex review

[Copilot]

Pull request overview

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

[chatgpt-codex-connector]

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

Reviewed commit: ead96a52d5

ℹ️ 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]

Coordination note now that #802 has merged SPEC §9 and #837 carries the SDK conformance sweep: the boundary question that spent rounds 3–4 here (#788) is decided, and it lands lightly on this stack.

Under merged §9 the rule is credential-scoped — the closed set of secrets the SDK holds or requested, not a general theory of peer-derived text. For the connector that means:

• The dial failure is this stack's only credential site (the stream ticket rides in the mint URL's query, and Go's *url.Error renders the URL it failed on). dialFailure's existing typed-classification shape already satisfies the rule; §9 cites it as the pattern for the dial site each SDK owes as its connector is written.
• invalidFrameError and CloseError.Reason need no change. Both render Basecamp's own text — a decoder's quotation, a close reason — which §9 explicitly leaves to the truncation cap and declines to prescribe. The reason-string dispatch surface in §23 is untouched.

So no further reworking of those two renderings is owed by this PR, and future review rounds have a spec section to point at instead of re-litigating the boundary. #837 touches none of the §23 code.