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

Foundations for SPEC §23 Event Feed connector with hardened security invariants; no run loop yet. Splits out seams, models, codecs, persistence, and a default WebSocketTransport so the state machine can be reviewed independently.

• Highlights

  • Seams and models: TicketMinter, PollSource, CableTransport, CheckpointStore, Clock; event/filter models; Action Cable frame parser and command encoder; backoff envelopes and named timers; continuation/resume URL validation; deterministic feedtest fakes.
  • Security invariants: closed-set URL redaction; invalid-frame errors never render frame contents; bounded inbound frame size with ErrFrameOversize; bounded error renderings elsewhere; dedupe records only actually-delivered ids.
  • Cable policy and default transport: refuse userinfo; require wss:// (allow ws:// only for localhost/loopback); never proxy cleartext ws://; refuse redirects; negotiate Action Cable subprotocol; send no Origin; enforce positive max-frame limits; redact tickets in errors. Default WebSocketTransport uses github.com/coder/websocket.
  • Persistence: FileCheckpointStore reads/writes a single ≤8 MiB JSON file, requires a regular file, case-folds lock keys, preserves atomic writes, and saves through symlinks by resolving the final link and renaming onto its target.
  • Behavior notes: cancellation outranks local close on entry and exit; a concurrent second Close returns early by design; package docs state “foundations only.”
  • Lint-only: annotate resolveStorePath’s deliberate nilerr fall-through; no behavior change.

• Migration

  • Custom CableTransport implementations must return ErrFrameOversize for over-limit frames and must refuse non-positive max-frame limits as usage errors.

^{Written for commit e7114fc. 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.

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

go/pkg/basecamp/eventfeed/errors.go:88

• TerminalError.Error can render unbounded server-controlled text. For example, checkContinuation places the parsed scheme/origin in Msg, and url.Parse accepts an arbitrarily long scheme; logging that terminal error bypasses §9's 500-byte rendering cap. Truncate the composed message as DialError and CloseError do, and add a long-continuation regression case.

	return msg

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

• An oversized frame reaches this branch as coder/websocket's untyped read-limit error. The stacked run loop only preserves package-owned sentinels and invalidFrameError; it reduces this error to generic event feed socket failed, so Observer.Disconnected loses the invalid-frame indication required by SPEC §23. Expose a stable package-owned size-violation classification from the transport and preserve it in the observer sanitizer.
  go/pkg/basecamp/eventfeed/feedtest/polls.go:113
• call and the ledger entry share the cloned filter backing arrays. If an OnCall callback mutates or retains call.Filters, it can rewrite p.calls despite Calls promising immutable snapshots. Give the callback its own filter clone.

	if onCall != nil {
		onCall(call)
	}

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

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

• A present "reconnect": null is silently treated as an absent flag because unmarshalling JSON null into *bool succeeds with nil. This contradicts the parser contract that wrong-typed fields used by a recognized frame are invalid and collapses the documented absent/boolean wire distinction. Preserve presence separately and reject null while continuing to allow the key to be absent.

			var env struct {
				Reason    string `json:"reason"`
				Reconnect *bool  `json:"reconnect"`
			}

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

go/pkg/basecamp/eventfeed/checkpoint.go:98

• url.URL.Port() preserves leading zeros, so an explicit default port such as https://example.com:0443 is not stripped. That URL reaches the same TCP origin as https://example.com, but continuation validation rejects it as cross-origin and checkpoint identity splits it into a separate lineage, contrary to the numeric default-port normalization in SPEC §8. Canonicalize the port spelling before comparing it with the defaults.

	port := u.Port()
	if (scheme == "https" && port == "443") || (scheme == "http" && port == "80") {
		port = ""

go/pkg/basecamp/eventfeed/backoff.go:104

• A valid positive repair interval of 1 ns produces a zero duration for every downward jitter draw because the float is truncated when converted to time.Duration. The stacked connector accepts any positive interval and schedules this result directly, so these draws arm an immediate repair timer and can create a hot repair-poll loop. Clamp a positive jittered result to at least 1 ns (or reject intervals too small to jitter).

	jittered := float64(interval) * (1 + repairJitterFraction*span)
	if jittered >= float64(math.MaxInt64) {
		return time.Duration(math.MaxInt64)
	}
	return time.Duration(jittered)

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

• Renaming onto s.path replaces the directory entry, not the target of a symlink. Thus a store path that is a symlink to a regular file reads the target, but its first successful Save replaces the symlink while leaving the target stale; later loads silently switch to the new file. This contradicts the documented symlink support above read. Either reject symlink store paths or preserve a stable target identity for writes.

[Copilot]

Pull request overview

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

Suppressed comments (3)

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

go/pkg/basecamp/eventfeed/cable_test.go:590

• This assertion comment still describes the superseded “truncate frame contents” rule. §23 now requires invalid-frame errors to render no frame contents at all, which is exactly the behavior this test verifies.

	// §23 Security Invariants: error renderings of frame contents are
	// bounded. The codec goes further — it never embeds frame bytes at all.

go/pkg/basecamp/eventfeed/dedupe.go:17

• A hit in Seen deliberately leaves this list unchanged, so this is delivery recency rather than observation recency. Calling it “most-recently-seen” contradicts the new dedupe contract and can invite reintroducing the hit-refresh behavior.

	// order holds ids most-recently-seen first.

go/pkg/basecamp/eventfeed/errors.go:104

• The updated §23 rule explicitly forbids invalid-frame errors from carrying frame contents; it no longer applies the truncation cap to them. This comment states the superseded rule and could encourage restoring the data leak that invalidFrameError now prevents.

// maxErrorMessageBytes bounds every error rendering that can carry
// frame-derived text (SPEC.md §9 "Error Message Truncation",
// MAX_ERROR_MESSAGE_LENGTH = 500; §23's Security Invariants apply it to frame
// contents). Go's unit is bytes, matching the main client's
// MaxErrorMessageBytes.

[jeremy]

Covering note for the three suppressed findings in Copilot's review of f524c9db2, all taken in 36733290a — one class, comment drift against the strengthened §23 rule:

• cable_test.go:590 — the assertion comment still described the superseded truncate-frame-contents rule; total suppression is the required rule ("bounding is not redacting"), not the codec exceeding a cap. The comment now states the absence as the rule.
• dedupe.go:17 — the order field comment said "most-recently-seen first", the exact wording that invites reintroducing hit-refresh; Seen's own doc forbids it. Now "most-recently-delivered first: a Seen hit is a suppression, not a delivery."
• errors.go:104 — maxErrorMessageBytes' doc claimed §23 applies the cap to frame contents; frame contents are forbidden outright and the cap's real clients are the other renderings. Rewritten to name what it binds.

Comment-only — no behaviour or assertion moved, so no red proofs attach; verified against the SPEC sentences and code paths each comment describes.

[Copilot]

Pull request overview

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

Suppressed comments (4)

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

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

• strings.EqualFold accepts a server response such as ACTIONCABLE-V1-JSON, although the client offered only the exact actioncable-v1-json token. WebSocket subprotocol values are selected from the client's offered values, not compared as case-insensitive HTTP header names; accepting a different token also diverges from SPEC §23's literal protocol requirement. Enforce exact equality here (including the missing-protocol case).
  go/pkg/basecamp/eventfeed/websocket_transport.go:112
• This package-owned transport does not actually mirror http.DefaultTransport: leaving MaxIdleConns and IdleConnTimeout at zero means no global idle-connection limit and no idle expiry, versus the standard defaults of 100 and 90 seconds. Because this client is global and callers cannot close its idle pool, failed handshakes across changing server-selected origins can retain connections indefinitely. Set both safe defaults explicitly.
  go/pkg/basecamp/eventfeed/cable.go:206
• encoding/json matches struct tags case-insensitively, so this does not test the presence of the exact JSON member type. For example, {"type":"ping","TYPE":"welcome"} overwrites the same field and is classified as a welcome, while a frame with only TYPE is treated as typed rather than as the typeless broadcast shape. Decode member names through map[string]json.RawMessage (and apply the same exact-name handling to the other envelope fields) so the wire grammar remains case-sensitive.

	var envType struct {
		Type json.RawMessage `json:"type"`
	}

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

• These required-field checks are not exact: Go's JSON decoder accepts ID, Kind, and other case variants for the tagged fields. A payload can therefore omit the schema's required lowercase key yet still be delivered, contrary to the stated nine-key push contract. Validate exact member names from a raw-message map before decoding values.

		ID               *int64     `json:"id"`

[Copilot]

Pull request overview

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

Suppressed comments (1)

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

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

• A broadcast with "identifier": null is silently classified as frameUnknown: encoding/json accepts JSON null into a string and leaves it empty, so the condition below fails. This contradicts this parser's invalid-frame rule for wrong-typed fields and can silently discard an event instead of taking the socket-failure path that triggers poll repair. Decode identifier presence/value separately so absent remains non-broadcast while present null is rejected.

	var env struct {
		Identifier string          `json:"identifier"`
		Message    json.RawMessage `json:"message"`
	}
	if err := json.Unmarshal(data, &env); err != nil {
		return frame{}, newInvalidFrameError(invalidFrameParse)
	}
	if env.Identifier != "" && len(env.Message) > 0 {
		return frame{kind: frameMessage, identifier: env.Identifier, message: env.Message}, nil