Take the account event feed into a durable ledger - #729
Conversation
github-actions
Bot
added
commands
There was a problem hiding this comment.
🟡 Changes recommended
Durable reset recovery, lifecycle handling, queue closure, and MCP feed error preservation contain blocking defects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds durable event-feed intake for the forthcoming basecamp connect, including SQLite-backed deduplication, checkpoints, overflow recovery, and lifecycle support.
Changes:
- Adds connector intake, queues, locking, recovery, and durable ledger storage.
- Adds SDK-backed feed adapters with extensive tests.
- Updates the SDK pin and MCP catalog for event-feed operations.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
go.mod |
Updates SDK and adds SQLite dependencies. |
go.sum |
Records updated dependency checksums. |
nix/package.nix |
Refreshes the Nix vendor hash. |
internal/version/sdk-provenance.json |
Records the new SDK revision. |
internal/mcpserver/model/PROVENANCE.json |
Updates model provenance. |
internal/mcpserver/model/behavior-model.json |
Adds event-feed operation behavior. |
internal/mcpserver/model/openapi.json |
Adds event-feed API schemas. |
internal/mcpserver/domains.go |
Registers the event-feed MCP domain. |
internal/mcpserver/testdata/catalog_snapshot.txt |
Updates the MCP catalog snapshot. |
internal/mcpserver/catalog_test.go |
Updates catalog expectations. |
internal/mcpserver/server_test.go |
Updates MCP tool-count coverage. |
internal/commands/mcp_test.go |
Updates command-level MCP expectations. |
internal/connector/doc.go |
Documents connector intake semantics. |
internal/connector/feed_adapter.go |
Adapts generated SDK feed operations. |
internal/connector/feed_adapter_test.go |
Tests adapter behavior and errors. |
internal/connector/intake.go |
Implements feed intake orchestration. |
internal/connector/intake_test.go |
Tests intake and recovery signals. |
internal/connector/intake_feed_test.go |
Tests feed integration and restarts. |
internal/connector/ledger.go |
Defines the SQLite ledger schema. |
internal/connector/ledger_events.go |
Implements event persistence and state. |
internal/connector/ledger_checkpoint.go |
Implements durable checkpoints. |
internal/connector/ledger_recovery.go |
Persists losses and feed gaps. |
internal/connector/ledger_test.go |
Tests ledger behavior. |
internal/connector/lock.go |
Adds per-account agent locking. |
internal/connector/lock_test.go |
Tests lock identity and release. |
internal/connector/queue.go |
Adds backlog-aware intake queues. |
internal/connector/queue_test.go |
Tests queue thresholds and cancellation. |
internal/connector/repair.go |
Implements overflow repair walks. |
internal/connector/repair_test.go |
Tests repair and reconciliation. |
internal/connector/shutdown.go |
Adds signal handling and exit codes. |
internal/connector/shutdown_test.go |
Tests signal-to-exit-code mapping. |
Review details
- Files reviewed: 30/31 changed files
- Comments generated: 10
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Redirect handling, cancellation classification, ledger permissions, and concurrent pause tracking have unresolved correctness and security issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
internal/connector/feed_adapter.go:243
- This classifier has no context, so caller-driven
context.Canceled/DeadlineExceedederrors are wrapped asPollTransient. The event-feed seam requires connector cancellation to pass through unchanged; otherwise reconnect and shutdown enter transport-retry handling. Passctxinto the classifier and return cancellation directly only whenctx.Err()is set, preserving client timeouts as transient.
This issue also appears on line 305 of the same file.
internal/connector/queue.go:74
pausedis a boolean even though intake and every open repair walk can callOfferconcurrently. If two offers block on a full queue, the first one to resume stores false and firesOnResumewhile the other (possibly the feed) is still blocked, soPaused()and the operator signal become incorrect. Track the number of blocked offers and emit pause/resume only on the 0→1 and 1→0 edges.
internal/connector/intake.go:27- At a 60-second cadence, a ten-minute window contains about ten repair intervals, not twenty. Correcting this avoids documenting twice the recovery attempts the implementation and tests provide.
internal/connector/feed_adapter.go:309
- The mint classifier similarly turns connector cancellation into
MintTransient. Pass the call context intomintErrorand return caller-driven cancellation unchanged, while continuing to classify a client-owned timeout as transient.
// mintError classifies a failed CreateStreamTicket call.
func mintError(err error) error {
if err == nil {
return nil
}
- Files reviewed: 31/32 changed files
- Comments generated: 2
- Review effort level: Balanced
|
🤖 On the four comments suppressed in the latest review, all fixed in c43c506:
|
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved redirect-data exposure, cursor validation, recovery, locking, and queue concurrency issues can cause leaks or missed events.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/queue.go:124
- The depth sample and
warnedtransition are not serialized with othernoteDepthcalls. A sender can sample a warning-level depth, then a receiver drains and observeswarned == false, after which the sender sets it true; the queue is now below threshold but recovery never fires unless another operation occurs. Serialize the warning-state transitions and callbacks so observations cannot be applied out of order.
internal/connector/feed_adapter.go:367
- As in the poll path, the redirect refusal is a
url.Errorcontaining the server-chosen target. Preserving it here can render that target in terminal logs; return onlyErrRedirectRefusedso the mint failure cannot disclose redirect data.
if errors.Is(err, ErrRedirectRefused) {
return &eventfeed.MintError{Kind: eventfeed.MintUnrecoverable, Err: err}
- Files reviewed: 33/34 changed files
- Comments generated: 5
- Review effort level: Balanced
|
🤖 On the two suppressed comments in the last review, both fixed in 18af1cc:
This head follows a design pass over One thing this PR cannot fix: re-entry after a refused position depends on the package's in-memory reset cursor, which a restart cannot seed. The connector ends the connection and remakes it from the ledger, which works but has to fail closed by hand at each branch. A seed for that cursor, or a disposition on PositionRejected, belongs in basecamp-sdk's eventfeed package. |
There was a problem hiding this comment.
🟡 Changes recommended
Rejected empty-page checkpoints can skip events, and the MCP domain currently exposes a replayable stream credential.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 34/35 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Queue notifications can be reordered, recovery can report false success, and pointer timestamps lose precision.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 32/33 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Text timestamp ordering can retain expired ledger content, and one test file violates enforced import formatting.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/round4_test.go:9
- Remove the blank line splitting the standard-library imports. The repository requires standard-library imports to remain in one alphabetized group (
STYLE.md:52-59), andgoimportswill rewrite this file, causing the formatting check to fail.
- Files reviewed: 33/34 changed files
- Comments generated: 1
- Review effort level: Balanced
|
🤖 On the comment suppressed in the last review (round4_test.go:9, a standard-library import group split by a blank line): fixed in f728b71. It was the only file in the package with that problem. |
There was a problem hiding this comment.
🟡 Changes recommended
Ledger path validation remains vulnerable to redirection, and repair failures can strand committed events without handing them to admission.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 44/45 changed files
- Comments generated: 2
- Review effort level: Balanced
Two findings. The ledger holds feed positions, which resume the account's feed, so it is a credential file — but it was vetted by name: os.Stat on the immediate directory only, following an existing symlink, with nothing said about the ancestors. A group-writable ancestor is enough to rename the checked 0700 directory away and put another in its place between the check and SQLite's open. It now goes through the same private-path check as the instance lock and the trust file: every ancestor owned by this user and unwritable by others, the file opened without following symlinks and inspected through that descriptor, and a file another user can so much as read refused. Its own extra rule stays, because SQLite writes -wal and -shm beside it: the directory must be 0700, not merely unwritable. And a handover that failed after the commit waited for a restart. That is a long wait for a connector that runs for weeks, and the repair walk is where it bites: the commit resolves the loss's missing id, so the reconciliation that follows sees nothing missing and closes, while the event was recorded, never judged and never mentioned again. Every commit already goes through one handover path; now a failure on it is remembered there, and the sweep that re-offers open losses re-offers those ids too. A crash still loses the list, and loses nothing with it: the next start offers every record still in seen, and clears what it has offered.
There was a problem hiding this comment.
🔵 Needs a closer look
The new concurrent, persistent recovery state machine and security-sensitive filesystem behavior warrant final human validation despite extensive tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/lock.go:26
- Use “a” before “flock”; “an flock” is grammatically incorrect.
- Files reviewed: 44/45 changed files
- Comments generated: 0 new
- Review effort level: Balanced
A writable handle was closed in a defer, so a close that failed said nothing and the caller went on to treat the file as one that exists. It is closed and reported explicitly now, and since what is durable about an empty file is its directory entry, the directory is synced before the create is called done. The other write in the package already syncs, closes and renames; SQLite answers for the ledger's own durability. And the start's re-queue cleared the whole stranded list, on the assumption that it had just offered everything in it. It had not necessarily: the repair walks start before the re-queue and serve older ids, so one stranded mid-pass — past the page the walk is on — was forgotten without ever being handed over, which is the wait this was meant to remove. It now clears only the ids it carried in.
|
🤖 Head is f022c1a. Three changes since 20eba38, plus the two before it answered on their threads. CodeQL alert 323 ( The start's re-queue cleared too much — found by my adversarial reviewer on 20eba38, and it was a real hole in the fix I had just made. The "an flock" nit (suppressed, One note from the same review, recorded rather than changed: On the "needs a closer look" verdict: agreed, and it is not something I can close from here. The PR is a concurrent, durable state machine touching credential-bearing files, and it wants a human read. |
There was a problem hiding this comment.
🟡 Changes recommended
Documented terminal ledger states can currently be transitioned back into active work states.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 44/45 changed files
- Comments generated: 1
- Review effort level: Balanced
completed and discarded are documented as the end of a record's life, but SetState updated the row whatever it held, so either could be moved back into the working states — an event dispatched a second time, or, once DropContent has taken the payload and left the tombstone, a row picked up as work with nothing in it. The lifecycle is now a table of edges rather than a sentence in a comment, and the refusal is in the UPDATE itself: a state is written only where the record is in a state that may enter it, so a check and a write cannot race. Writing the state a record already has stays allowed — that is a repeat, not a move, so a retry after a crash is not an error. A refused transition is told apart from a missing row. Migration 3 adds the same refusal as a trigger, for the two edges that matter most: whatever ever writes to this file, a terminal record cannot change state.
There was a problem hiding this comment.
🟡 Changes recommended
Repair can restore a stale pre-epoch cursor after a transient failure following a 410 resume.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 44/45 changed files
- Comments generated: 1
- Review effort level: Balanced
A 410 with an epoch says the history below it is gone for good, and the walk clears its cursor on disk when it takes the fence. But the pass kept its own copy of the position in a local, and returned that copy if the resumed poll then failed for any other reason — a connection reset was enough. The caller put it back in memory, and every later pass entered at a position the feed had already refused forever. The walk's memory of the cursor is now the loss's own field, kept equal to what the ledger holds at every point: saved first, remembered second, and whatever a failure wrote is what the pass returns. Two edges the lifecycle was missing, from the same spec the table came from: seen to queued, because admission commits an admitted verdict AS queued when the conversation is already live, and dispatched back to admitted, because a dispatched record whose worker never started has its exposure withdrawn. Without them the next card writes around SetState. And a repeat no longer touches updated_at, which is the retention clock: a re-written completion was silently restarting the window.
|
🤖 Head is 9c89aca. Two rounds since f022c1a, both answered on their threads. Terminal means terminal (3963d28). A pre-epoch cursor could come back (9c89aca). After a 410 the walk clears its cursor on disk, but the pass kept a copy in a local and handed it back if the resumed poll then failed transiently — so every later pass re-entered at a position the feed had refused forever. The local is gone; the walk's memory of the cursor is the loss's own field, kept equal to the ledger's at every point. Two things my adversarial reviewer found in the same rounds, both taken:
The invariant list gained two entries: A2, a cursor older than the feed's current epoch is never usable; E4, terminal states have no outgoing edges, and the allowed transitions are the table in
|
There was a problem hiding this comment.
🟡 Changes recommended
The admission line writer has a normal-path data race during lazy initialization.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 44/45 changed files
- Comments generated: 1
- Review effort level: Balanced
The line writer was built on the first verdict. Nothing in this package needs that: admission knows its sink before it starts, so the writer is built there and injected, and the struct keeps a writer and nothing else — no sink, no once, no first-use path for a pool of workers to take together. A stream whose lines stay whole rests on there being exactly one writer and one lock per sink. That is easier to keep true when there is no second place a writer can come into being.
There was a problem hiding this comment.
🔵 Needs a closer look
An uncapped Retry-After sleep can keep a loss open beyond its declared 24-hour absolute deadline.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/repair.go:115
- The 24-hour
maxLossLifetimeis not actually an upper bound while this sleep is in progress.PollThrottledcarries the server's uncappedRetry-After, so one response whose delay exceeds the remaining lifetime can leave the loss open—and occupy a repair worker—past the promised absolute deadline (the same uncapped sleep exists infinalPass). Cap the sleep at the time remaining untilDetectedAt + maxLossLifetime; waking then should close the loss without polling before the server-directed delay.
- Files reviewed: 45/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The absolute lifetime bounds the loss, but the waits between its attempts were unbounded: Retry-After is whatever the server names, so one response asking for thirty hours held the loss open — and its repair worker with it — well past the day the connector promises. Every repair wait is now capped at the time the loss has left. Woken at the deadline, the next turn around the loop sees it and closes the loss without polling first, which is the honest answer: the server asked for a wait this connector no longer has to give.
|
🤖 Head is 0d84ab6. Two rounds since 9c89aca. The admission line writer (f427ce7, answered on its thread). I could not reproduce the race and I set out why I think the specific claim does not hold — The uncapped The invariant list now reads: A2 a cursor older than the feed's current epoch is never usable; C, a loss is scheduled or closed, and no wait may carry it past its absolute deadline; E4 terminal states have no outgoing edges; H, exactly one NDJSON writer per sink, constructed once and injected.
|
There was a problem hiding this comment.
🟡 Changes recommended
A panicking pause callback leaves the queue’s depth and paused state permanently incorrect.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 45/46 changed files
- Comments generated: 1
- Review effort level: Balanced
A callback belongs to whoever built the queue, and one that panicked took the queue's own state with it: the pause callback runs before the offer's cleanup was in place, so the id's count and the wait it announced both stayed behind — a queue reporting an item nobody can take, paused with nothing able to lift it. Three changes, all of them about the same rule. The state a callback is told about is committed before it runs, so nothing it does can change it. An offer that leaves without sending takes its count back on every way out, not only on cancellation. And a panic in a callback is contained and logged: a reporting bug is not a reason to lose the feed.
There was a problem hiding this comment.
🔵 Needs a closer look
Account intake must reject inbox-only Reasons filters before repair workers can issue widened requests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/connector/intake.go:225
- This only applies syntax validation, but intake always uses the SDK's account lane, where
Reasonsis explicitly unsupported.eventfeed.Newwill eventually reject it, butRunstarts repair workers before constructing the feed, so an open legacy loss can poll throughPollsForwith this invalid filter first (and the adapter may omit the inbox-only dimension, widening the repair read). Reject non-emptyReasonshere so invalid configuration fails before any repair or wire work.
- Files reviewed: 45/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Reasons filter the inbox — why an event reached you — and the account feed does not carry them. The SDK refuses them when the feed is built, but that is too late: Run starts the repair workers first, so an open loss could walk under a filter set the lane cannot honor, and a dropped dimension widens a read rather than narrowing it. Configuration that cannot mean what it says is refused before any wire work. And intake now hands the queue its logger, so the callback panic the queue contains is reported somewhere. A queue built by a caller who did not think about it would have swallowed it silently.
|
🤖 Head is 5e448b8. The panicking pause callback (90e644a, answered on its thread). Real, and the ordering was exactly as described. The offer's cleanup is now registered before the pause is delivered, an offer that leaves without sending takes its count back on every way out through a single The inbox-only One thing from my adversarial reviewer in the same round: with no logger set, the contained panic was silent, and the queue is built by the caller rather than by intake. Intake now hands the queue its own logger — one owner, one logger — with
|
There was a problem hiding this comment.
🔵 Needs a closer look
The large durability-, concurrency-, security-, and recovery-sensitive state machine warrants final human review despite extensive tests and green checks.
Review details
- Files reviewed: 45/46 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The previous commit had intake write the queue's logger field while the callback path read it unlocked, and a queue can already be live when intake adopts it — admission takes from the same one. The race detector found it three times in three on a queue in use during construction. The logger is now held under the queue's own lock: SetLogger for a caller, an adopt that checks and sets in one step for intake, and the one read made with the lock released around the callback it reports on.
|
🤖 Head is d13fda8. One commit since 5e448b8, and it fixes a race that commit introduced — found by my adversarial reviewer (a separate Opus agent), not by a review here. 5e448b8 had intake write the queue's logger field while the callback path read it without the lock, and a queue can already be in use when intake takes it over: admission takes from the same queue. The logger is now held under the queue's own lock ( Where things stand on d13fda8: all 31 checks pass, |
basecamp connecthas no intake yet: nothing in the CLI can hold the account event feed, remember what it has seen across a restart, or recover what it missed. The Ruby connector it replaces keeps its dedupe in memory and resumes by forgetting history, so a crash loses every event that landed while it was down.Tracked in 14 basecamp connect intake: feed, ledger, checkpoint.
Why
Intake is the only work on the feed's delivery path, and the ledger and checkpoint are what turn a crash into a delay. Admission (card 15) and dispatch build on this; neither can start without a durable record of what was seen.
What changes
A new
internal/connectorpackage, with no command surface of its own yet —basecamp connectlands with setup (card 16):Three things in this feed look like "nothing more to read" and aren't: an empty page, a missing
next, and a quiet inbox. Each has handling and a test that fails without it.The two 410s
They mean different things and do not share a recovery path. Both are now told apart below this package, by the SDK's live seam from Event feed: split the poll lanes' 410s and give their 400 a reason:
FeedPositionGoneError, epoch required) becomes the feed-gap signal, and its resume is followed as served. A resume that does not re-enter at the declared fence is refused. In the repair walk it condemns only the ids behind the epoch.InboxPositionGoneError, no epoch,since=0) is refused on the account lane. It ends the feed rather than being flattened to "epoch 0". A 410 that omits the epoch is never typed with a fabricated one.A 400's
reasondecides between recovering and stopping. A reason the contract doesn't name is surfaced, not guessed.Tests at the real
eventfeed.NewLiveseam pin this contract, and each one fails against a scratch copy of the SDK with the handling removed.Built on the SDK's live seams
This PR started against the seam interfaces with a temporary local adapter, because card 09's SDK work was still open. That work has landed: Event feed connector: the inbox lane (4/4), Event feed connector: bind the seams to the generated operations (5/5) and 912. The adapter is deleted, and intake binds to
eventfeed.NewLivethroughLiveOptions.The SDK's seams own the redirect guard, the 410 and 400 mapping, the one-cursor rule for followed URLs, and cancellation pass-through. One check stays in intake: the repair walk follows
nextand resume URLs outside the SDK connector, whose same-origin validation is unexported, so the walk checks the origin itself.SDK pin and MCP
go/v0.19.0tag (6abe227), which carries 897, 899 and 912. The tag is one commit past the pseudo-versionmainused; that commit changes only version strings, and the vendored model files are identical.internal/mcpserverand registers nothing. Serve the account event feed over MCP serves the feed operations, and Give cursor-paginated operations no page parameter fixed their catalogue entries. The only files here are the provenance record and the NixvendorHash.internal/connector/ndjson, and both strip terminal controls from API text.Invariants
Four review rounds, Copilot and a separate Opus reviewer, ended in a design pass over the package as one state machine: 25 invariants across positions, ledger, recovery, error classification, lifecycle, queue, lock and confidentiality. The fixes are made against those invariants, and each unheld one has a test that failed before its fix. Two points shape the code:
eventfeedwould remove that workaround.New dependency:
modernc.org/sqlite, pure Go, so all five release targets still build without cgo.Not in this PR
The card's done-when needs the running process: 24 hours in
--shadowagainst production, kill and restart against the real feed, reconnect timings. That waits on the command.The kill-and-restart case, catch-up, streaming, reconnects and shutdown are covered end to end on the SDK's
feedtestfakes. The membership refresh is wired against an interface; the SDK-backed project lister comes with the command. A 1,000-event burst goes through intake in about 20ms.