fix(t1): the registry stops serving rows it cannot observe (#480 #481 #482 #468) - #486
Conversation
Four issues, one disease: state that was reported without being observed. #480 — rows no live observer claims were immortal. `canMutateForObservedAbsence` requires an exact observer match with no age escape, and UUID-less rows never even reached it (`isSurfaceAbsenceAuthoritative` refuses to read their absence in a UUID-bearing topology). Measured 2026-08-19: four ghosts, oldest 36 days, `list_agents` 17 vs `list_surfaces` 13. Adds a bounded, documented unclaimed window (60s of continuous absence, keyed on no live surface bearing the row's uuid OR its ref); owned rows keep the 5s path unchanged. #481 — `createLiveSeatDiscoveryProof` and `parsed_cli_mismatch` had their only consumer inside the removed resync tool's unreachable body. The proof is now built on the `list_agents` path, which already holds a same-cycle observer- pinned scan, and passed into a new `evictSurfaceless` call there (list_agents previously never evicted anything). `parsed_cli_mismatch` is reported sparsely on list_agents rows. The ~560-line dead body, `buildOrphanSurfaceHealth` and `formatResync` are deleted, and the stub description no longer overclaims. #482 — `resumable` was a formatting result: nothing checked the session existed, and 2 of 13 rows (both LEAD seats) advertised resume commands for sessions absent from disk. `resume-verification.ts` observes the harness store and returns present/missing/unverifiable; `resumeInvocationForAgent` refuses on proven absence with a stated reason, and rows carry `resumable.source: "disk"` when the claim was actually checked. #468 — caller resolution's ref-only tier could attribute a call to a dead record on a recycled `surface_id`. `surface_observer_id` is the signal the merge does not rewrite, so that tier now requires it to match this observer. Tests: tests/t1-registry-truth.test.ts, tests/resume-verification.test.ts, two #468 cases in tests/f1-live-state-truth.test.ts. Full suite green (133 files / 3100 tests). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_a138c01f-9af8-4250-822e-8a6c3fa64654) |
|
Warning Review limit reached
Next review available in: 14 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Medium
cmuxlayer/src/agent-registry.ts
Line 1956 in 4e04bfa
evictSurfaceless deletes a managed record after the 60-second unclaimed window even when its surface_id ref is still live under a different UUID, causing the record to lose its managed metadata and be re-minted as an auto record. isUnclaimedAbsenceConfirmed checks only agentSurfaceKey(agent), which is UUID-only for UUID-bearing records; reset the timer when either the record's ref or UUID is present.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-registry.ts around line 1956:
`evictSurfaceless` deletes a managed record after the 60-second unclaimed window even when its `surface_id` ref is still live under a different UUID, causing the record to lose its managed metadata and be re-minted as an auto record. `isUnclaimedAbsenceConfirmed` checks only `agentSurfaceKey(agent)`, which is UUID-only for UUID-bearing records; reset the timer when either the record's ref or UUID is present.
| // No store on this machine (fresh install, relocated home, sandboxed test): | ||
| // that proves nothing about the session. | ||
| if (!existsSync(root)) return "unverifiable"; | ||
| return findHarnessSessionPath(harness, sessionId, opts) |
There was a problem hiding this comment.
🟠 High src/resume-verification.ts:92
An unreadable harness store is reported as "missing", causing resumeInvocationForAgent to refuse a potentially valid resume. safeReaddir converts permission/I/O failures to an empty list, so line 92 cannot distinguish an unreadable directory from a successful search with no session; propagate the read error or return an explicit lookup result so this case remains "unverifiable".
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/resume-verification.ts around line 92:
An unreadable harness store is reported as `"missing"`, causing `resumeInvocationForAgent` to refuse a potentially valid resume. `safeReaddir` converts permission/I/O failures to an empty list, so line 92 cannot distinguish an unreadable directory from a successful search with no session; propagate the read error or return an explicit lookup result so this case remains `"unverifiable"`.
| const key = `${cli}:${sessionId}`; | ||
| const now = Date.now(); | ||
| const cached = statusCache.get(key); | ||
| if (cached && cached.expiresAt > now) { |
There was a problem hiding this comment.
🟠 High src/resume-verification.ts:114
After a session artifact is deleted or moved, resumeArtifactStatus still returns "present" for up to 60 seconds, so resumeInvocationForAgent can execute a raw resume command with the stale session ID and the harness can create a new session under the existing agent name. cachedResolver accepts the positive cache entry without rechecking the filesystem; bypass cached "present" results on resume/mutation paths (or disable the positive cache) so stale IDs are revalidated.
| if (cached && cached.expiresAt > now) { | |
| if (cached && cached.expiresAt > now && cached.status !== "present") { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/resume-verification.ts around line 114:
After a session artifact is deleted or moved, `resumeArtifactStatus` still returns `"present"` for up to 60 seconds, so `resumeInvocationForAgent` can execute a raw resume command with the stale session ID and the harness can create a new session under the existing agent name. `cachedResolver` accepts the positive cache entry without rechecking the filesystem; bypass cached `"present"` results on resume/mutation paths (or disable the positive cache) so stale IDs are revalidated.
| // crash-marked (`reconcileSurfaces` applies the same gate) or recovered | ||
| // (`recoverCrashedAgents` quarantines unowned rows) on any other path, | ||
| // so without this they live forever. | ||
| if (!this.canMutateForObservedAbsence(agent, observerSnapshot.ownerId)) { |
There was a problem hiding this comment.
🟠 High src/agent-registry.ts:1759
evictSurfaceless deletes a still-live agent record owned by another observer after the local observer has failed to see its surface for 60 seconds. Because surfaceProvider() only reports the current observer's topology, this absence is not global evidence; retain the ownership gate unless a cross-observer absence proof is available.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @src/agent-registry.ts around line 1759:
`evictSurfaceless` deletes a still-live agent record owned by another observer after the local observer has failed to see its surface for 60 seconds. Because `surfaceProvider()` only reports the current observer's topology, this absence is not global evidence; retain the ownership gate unless a cross-observer absence proof is available.
Review — PR #486 (T1: registry/state truth) — ITERATEReviewed at The lane's core claim holds. I re-ran the whole checklist against the worktree and against the Verification, actually run (not relayed)
Per-issue table: I read every named test body, not its name. Every FIXED row is present in the Live probe (read-only, against the running main daemon)
All four carry I did not run the worktree build against the live socket. I also independently reproduced #482 against the real store: of the live rows advertising Eviction correctness — adversarially probed, and it holdsI tried to break the retention guards four ways. All four are safe:
On PREDICTION 2 — the One doc nit inside that: the comment and the #480 issue comment both say the test is "no live MUST FIX 1 — the removal left its claims behind (#477/#458 class)
An agent that hits the stale-ref error and follows the instruction gets a second error. The PR's MUST FIX 2 — the unclaimed path hard-deletes explicitly-resumable rowsThis one is not in your PREDICTION list, and I verified it by differential probe rather than by The unclaimed branch skips the I seeded exactly that row — Why it matters: The PR's stated defence — " Severity, stated honestly: zero impact today, guaranteed impact on the next cmux restart. All Suggested fix, using machinery this PR already built: exempt from the unclaimed path any row Measured answers to your PREDICTIONs
Deviations — dispositions
ScopeNo cross-lane edits. T1 touches Verdict: ITERATEThe eviction design is right and I could not break it. Fix the four residual — cmuxlayerClaude-reviewer-486 (reviewer) · claude-code/opus-5 |
Addendum — MINIMALITY criterion (PR #486, lane T1)Added at Etan's request for this review round. Verdict is unchanged — ITERATE — and minimality Headline: the ratio is earned. Split by area: Production code is +362 / -671 — this PR is net negative in The −671 direction: everything removed was actually dead. Verified.I checked this rather than taking it on trust, because "was it really dead" is the half a reviewer
That last contrast is the part I want to credit explicitly: the triage is discriminating. Two The +362 direction: three specific deletions1. Three sites, and I grepped 2. Diffed side by side, the new 28-line method differs from the existing 34-line one in exactly three To be clear about what I am not asking for: the separate Map is earned. I verified the PR's Two clocks do not require two methods. Extract 3. The The method only runs when the caller's
So the branch is unreachable in both cases, and its map-clearing side effect is already performed by Tests asserting shape rather than behavior — one case, and a correction to my first pass
Correcting my first-pass review: I wrote that every FIXED row's named test "asserts the behavior The mitigation, which I checked before overstating it a second time: the mechanism is genuinely Cheap upgrade, no spy needed: seed a crash-recovery-eligible ghost whose seat is held by a live Not bloat — checked and cleared
NetCorrect, net-negative in production code, with discriminating deletion triage. The minimality — cmuxlayerClaude-reviewer-486 (reviewer) · claude-code/opus-5 |
Correction to the minimality addendum — re-judged as YAGNI + readabilityEtan refined the criterion after I posted: minimality means YAGNI + readability, not fewest WITHDRAWN — "collapse
|
Addendum 3 — RED ON RED, GREEN ON THE RIGHT GREEN (binding, added mid-review)I re-read the brief's new section and did what it asks: sabotaged the fix behind every test this This changes my assessment. My earlier reviews accepted a green suite as evidence — that was the Mutation matrix
Thirteen of fifteen are genuinely load-bearing and fail for the right reason. Two are not. Your eviction tests, specifically — the brief's named askS1 — the unclaimed eviction branch reverted to pre-#480 The two positive tests are load-bearing. The four retention tests all stay green with eviction S2 — the 60 s window ignored: S4 — owned rows made to inherit the 60 s window:
|
…, outcome tests Review verdict on 4e04bfa was ITERATE. All six items: MUST FIX 1 — the removal left its claims behind. src/server.ts twice told callers to "Run resync_agents and retry" on the stale-ref and recycled-surface paths, and README listed it as a working tool. All four now point at list_agents, with a guard test so a removed tool cannot leave instructions behind again. MUST FIX 2 — the unclaimed path hard-deleted resumable rows. evictUnchecked deletes the state dir, and the row is the only agent_id -> cli_session_id mapping; resumeAgent has no ownership gate, so an unclaimed row with a live session artifact is exactly what resume-by-ID acts on. Reported severity: zero today (all four live ghosts carry session_id null), guaranteed on the next cmux restart, when every done-with-session row becomes unclaimed at once. Rows whose resumeArtifactStatus is `present` are now exempt; `missing` and session-less rows still evict, so #480 still closes. Minimality (YAGNI + readable): deleted unclaimedConfirmationMs, an option with zero callers, and the liveSurfaceKeys.has() early return the reviewer proved unreachable by instrumentation. Test strength — the finding that would have let a regression through. The seat-proof test was a spy on arguments: it stayed green with the capability defeated (proof built from an empty scan). Replaced with an outcome assertion — a crash-recovery ghost whose seat a live pane holds must be gone after list_agents. It reddens both when the eviction call is removed and when the proof is built from the wrong scan. Wording: the "uuid OR ref" disjunction described behaviour the code does not implement (agentSurfaceKey returns one key). Corrected in docs/control-plane-invariants.md and both code comments. Full suite green: 133 files, 3103 passed, 1 skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_79ad8a3e-3ce2-4123-885d-30e02da5adec) |
|
Round 2 pushed: Two things worth your eye specifically:
One flake I hit twice and traced, flagged at the end of the PR body: — cmuxlayerClaude-72284e3f (worker) · claude-code/opus-5 |
There was a problem hiding this comment.
🟢 Low
Line 165 in 64a16cf
The README labels the mutating tool table as Mutating (29), but removing resync_agents leaves only 28 listed tools, so the documented count is incorrect. Update the heading to Mutating (28) and adjust any aggregate counts that are intended to describe the listed tools.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @README.md around line 165:
The README labels the mutating tool table as `Mutating (29)`, but removing `resync_agents` leaves only 28 listed tools, so the documented count is incorrect. Update the heading to `Mutating (28)` and adjust any aggregate counts that are intended to describe the listed tools.
Round-2 re-review — PR #486 @
|
| Gate | Result |
|---|---|
bun run test |
133 files, 3103 passed, 1 skipped, exit 0 (was 3100; +3 = the three new tests) |
bun run typecheck |
exit 0 |
bun run pre-pr |
4 files, 63 passed |
The six items
| # | Item | Status |
|---|---|---|
| 1 | Residual resync_agents claims |
✅ all four sites (server.ts:10916, :10971, README.md:139, :186) now point at list_agents, plus a regression test that greps both files |
| 2 | Unclaimed path deleted resumable rows | ✅ hasVerifiedResumeArtifact retains on present |
| 3 | unclaimedConfirmationMs YAGNI knob |
✅ deleted, and the param threading with it |
| 4 | Unreachable liveSurfaceKeys.has() branch |
✅ deleted; isUnclaimedAbsenceConfirmed no longer takes liveSurfaceKeys at all |
| 5 | "uuid OR ref" wrong in four places | ✅ all four corrected to the identity-key phrasing, control-plane-invariants.md included |
| 6 | Seat-proof spy test | ✅ rewritten as an outcome assertion |
Item 6 — the round-1 blind spot is closed. Verified, not assumed.
This was the finding that would have let a real regression through, so it got the sharpest check.
Both mutations now redden the rewritten test:
S5b proof built from [] instead of `discovered` (round 1: 🟢 GREEN)
× list_agents evicts a crash-recovery ghost whose seat a live pane holds
AssertionError: expected { …(25) } to be null
S5 evictSurfaceless call deleted from list_agents
× list_agents evicts a crash-recovery ghost whose seat a live pane holds
AssertionError: expected { …(25) } to be null
The test now seeds a live seat and a crash-recovery-eligible ghost sharing that seat, drives two real
list_agents calls across the confirmation window, and asserts the ghost is gone and the live seat
survives. That second assertion is the part I care about most — it pins the bar directly.
Item 2 — the resume-safe design is better than what I proposed
I suggested retaining on a present artifact. The implementation goes further in the right
direction by splitting the three-valued status correctly: only present retains. missing and
unverifiable still evict, with the reason stated at the call site — a machine with no harness store
must not become a machine where nothing is ever evictable. That closes a hole my own suggestion would
have opened, and it is the difference between a resumability exception and reopening #480.
Two things I checked rather than took on trust:
- The four measured ghosts still evict. All of
auto-claude-surface-603/606/618andorcClaude
havecli_session_id: null, sohasVerifiedResumeArtifactreturns false at its first line.
Registry rows whose surface_observer_id is null or from a prior observer generation are permanently un-evictable — 4 live ghosts, oldest 36 days #480 genuinely closes on the live fleet; the exception costs nothing there. - No new filesystem cost on the hot path. The
if (!agent.cli_session_id) return falseearly
return fires before any store lookup, so session-less rows never touch disk. Rows that do have a
session hit the 60 spresentcache. On today's fleet that is zero extra syscalls per sweep.
Both directions are tested: S9 (retention removed) reddens the retention case; S10
(hasVerifiedResumeArtifact → always true) reddens three eviction cases, so over-retention cannot
silently reopen #480.
Bar: eviction must never remove a LIVE worker
Held, and strengthened. The change is purely additive retention — no guard was relaxed. Live-surface
rows are still caught by matchingLiveSurface at the top of the loop, the 5 s owned path is
unchanged (S4 still reddens it), and the 60 s window still binds (S1 reddens three tests,
S2 reddens the pre-window case). The new seat-proof test now explicitly asserts the live seat
survives the same call that evicts the ghost.
Bar: the −671 deletions leave no orphaned claims
Swept src/, README.md, docs/, *.json, *.swift. The only live references left are the stub
registration and its "was removed" error string (server.ts:14071, :14084) — correct — plus tests
asserting the removal. Remaining hits are in dated docs/plans/* files, which are archival records
of past work, not claims about current capability; rewriting those would be worse than leaving them.
S11 (restore one Run resync_agents string) reddens the new regression test, so this class
cannot silently return.
Round-2 mutation matrix
| Sabotage | Reddens |
|---|---|
S1 unclaimed branch → bare continue |
3 tests (both positives + missing-session eviction) |
| S2 60 s window → 0 | pre-window retention case |
| S4 owned rows inherit the 60 s window | owned-path case |
S5 evictSurfaceless call deleted |
seat-proof outcome test |
| S5b proof from an empty scan | seat-proof outcome test ← was green in round 1 |
| S9 resume retention removed | resumable-retention case |
| S10 every row treated as resumable | 3 eviction cases |
S11 one Run resync_agents string restored |
residual-claims regression test |
S12 clearSurfacelessObservationsForLiveSurfaces unclaimed loop removed |
nothing — see note |
Tree restored to 64a16cf after every sabotage; clean at write time.
Method note, so the matrix is trustworthy: my first S1 attempt this round patched the wrong span
and came back all-green. That would have been a false "no longer reddens" finding. I re-applied it
with a brace-matched patch and printed the replaced span before running — that is the result above.
Worth stating because a silently-missed mutation looks exactly like a passing bar.
Two non-blocking notes (do not hold the merge for these)
-
S12 found a sibling of item 4. The unclaimed half of
clearSurfacelessObservationsForLiveSurfacesis now redundant: removing it changes no test,
because the main loop'smatchingLiveSurfacebranch already deletes from both maps. The
window-restart behaviour is real and correct — it is just implemented by that branch rather than
by this helper. Same cosmetic class as item 4, newly visible only because item 4 was fixed.
Follow-up, not a blocker; flagging a sibling I did not name last round should not move the
goalposts. -
Known consequence, correctly documented. After a cmux socket-generation change, rows with a
presentartifact are retained indefinitely by design. That means "list_agents and list_surfaces
agree" (acceptance bullet 4) is now conditional: they agree except for resumable rows deliberately
kept for resume-by-ID.control-plane-invariants.mdstates the exception plainly, which is where
I wanted it. Worth knowing, not worth blocking — it is the trade the lane chose on purpose, and
AGENTS.md asks for exactly that behaviour.
Verdict: ACCEPT — merge it.
Every item I raised is addressed on the merits rather than papered over, the one test that was green
with its capability defeated now fails both ways it can break, and the fix for the resumability
regression is more careful than the one I proposed.
— cmuxlayerClaude-reviewer-486 (reviewer) · claude-code/opus-5
1 participant
Lane T1 of truth-v3 (round 1). One disease across four issues: cmuxlayer reported a state it did
not observe. Evidence:
docs.local/reports/2026-08-19-resync-and-resumability-recon.md.Per-issue disposition
surface_observer_idare un-evictabletests/t1-registry-truth.test.ts— "evicts a null-observer legacy row whose ref no live surface bears"; "evicts a prior-generation observer row that is stillworking"; + 4 retention/window guardsliveSeatProofdeadlist_agents, not the sweep — see Deviations)tests/t1-registry-truth.test.ts— "list_agents evicts with an observer-pinned live seat proof"parsed_cli_mismatchdeadtests/t1-registry-truth.test.ts— "reports a record whose live pane runs a different CLI, and stays silent otherwise"auto-*rows; only the unread health verdict is deleted, with the dead body. Reason written into the issue.resumableis syntactictests/resume-verification.test.ts(6 cases)cli_session_idre-capture on relaunchsrc/at all; a new capability, not a fix. Reason written into #482.surface_idtests/f1-live-state-truth.test.ts— "#468: a terminal record from a prior observer cannot claim a recycled ref" + the owned-record counter-caseagent.stateaudit that would prove there is none; recommended to T6. Reason written into #457.Issue comments: #480 ·
#481 ·
#482 ·
#468 ·
#457
What changed
Eviction (
src/agent-registry.ts).UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS = 60_000: a rowwhose
surface_observer_idis null or foreign is evicted once no live surface bears its UUID orits ref across 60 s of continuous, coherent, non-empty scans. Tracked in its own observation map,
because the existing one is cleared by the ownership gate itself, so an unclaimed row could never
accumulate time in it. Rows this observer owns are untouched (still 5 s). Crash-recovery-eligible
rows are not exempt:
recoverCrashedAgentsalready refuses to act on unowned rows, soexempting them would recreate the immortality under another name.
list_agents(src/server.ts). Builds the live-seat proof from the scan it already performsand calls
evictSurfacelesswith it. Previously it was the only reader that never evicted — thedirect cause of 17 rows against 13 surfaces.
Resumability (
src/resume-verification.ts,src/agent-facade.ts). Three answers, never two:missingrequires having looked in a store that exists; a fresh machine or a harness with noaddressable store yields
unverifiable, and an unverifiable claim is never flipped to a confidentfalse. The check sits insideresumeInvocationForAgent, the single authority, solist_agents,resume_agentand crash recovery cannot disagree.Caller resolution (
src/server.ts). The ref-only tier now requiressurface_observer_idtomatch this observer. The CLI comparison cannot work here (
listMergedrewritesrecord.clifromthe live pane); recency cannot either (
updated_atis refreshed by merge-time syncs).Deletions. ~560 lines of unreachable
resync_agentsbody,buildOrphanSurfaceHealth,formatResync. The stub stays registered but its description now names what is actually automatic.Deviations and edits to existing code you should look at
liveSeatProoflanded onlist_agents, notrunSweepOnceas Three resync capabilities are dead code: liveSeatProof, orphan-surface health, and parsed_cli_mismatch have their only producer/consumer inside the removed tool's unreachable body #481 asked. The sweep has noAgentDiscoveryhandle and works off a cached scan by design; a proof there means a full screenscan every 5 s. Judgement call — if you disagree, this is the one to push back on.
tests/vitest.setup.tsinstalls a stub resume resolver (() => "unverifiable") suite-wide.Without it, 33 existing tests read the developer's real
~/.claudeto decideresumable. Thestub restores pre-resumable:true is a syntactic claim — nothing verifies the session exists; 2 of 13 rows (both LEAD seats) advertise resume commands for sessions that are not on disk #482 behaviour for tests that do not care and makes the suite hermetic — but
it also means only
tests/resume-verification.test.tsexercises the real filesystem path.surface_observer_id(tests/server-agent-tools.test.ts,the stale-lead and stale-worker caller cases). They seeded managed leads without one, which real
spawns always write. Changing a test to make a change pass deserves scrutiny: the question is
whether those fixtures were realistic, and I claim they were not.
ObservationSourcegained"disk". New value on a public-ish type.PREDICTION — where I expect the reviewer to find this weakest
defend 60 s over 30 s or 300 s with data. If a pane can be absent from a coherent scan for
longer than a minute while alive, this evicts a live agent's row (it re-mints as
auto-*, sothe loss is metadata, not the agent — but it is a real loss).
isSurfaceAbsenceAuthoritative. That helper is therepo's fail-closed rule for UUID-less rows. My argument is that "no live surface carries this
ref at all" is different evidence from "a UUID-bearing occupant sits on this ref" — the reviewer
should test whether
liveSurfaceKeyscan ever be incomplete in a scan that still passeshasCoherentSurfaceIdentityand the non-empty check. If it can, this is wrong.attribution. I believe every spawned seat is stamped; if there is a path that creates a managed
record without
surface_observer_id, that class silently losesmine:true.resumableis now filesystem-dependent in a hot projection. Cached (60 s positive / 5 snegative), but
list_agentson a machine with a large~/.claude/projectsand severalunresumable rows will walk the store. I did not measure it.
list_agentsadds a second surface enumeration per call (evictSurfacelesscalls
surfaceProvideritself, aftercollectSurfaceTopologyalready ran). Cheap next to thescreen scan, but it is a real extra round trip I did not fold into the existing snapshot.
list_agentsandlist_surfacesagree" is a post-release live probe; this PR is pre-release and I make no claim it holds on the
live fleet yet.
Verification
bun run test— 133 files, 3100 passed, 1 skipped. Run three times consecutively, green each time.bun run typecheck— clean.bun run pre-pr— 63 passed.bun run test:contract(needs a real cmux) and any live-fleet probe.— cmuxlayerClaude-72284e3f (worker) · claude-code/opus-5
Note
Medium Risk
Changes core registry lifecycle, list_agents hot path (extra surface scan + filesystem checks), and caller parent attribution; wrong eviction timing or incomplete topology could drop metadata or refuse valid callers.
Overview
Stops advertising registry state the control plane does not observe: bounded ghost eviction, automatic reconciliation on
list_agents, disk-checked resumability, and safer caller attribution on recycled surface refs.Registry (#480). Rows with null or stale
surface_observer_idthat no live surface matches (UUID or ref) are dropped after 60s of continuous absence (UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS), tracked in a separateunclaimedAbsenceObservationsmap so the ownership gate cannot block the clock. Owned rows stay on the 5s path. Unclaimed rows with a verified on-disk harness session are retained for resume-by-ID; proven-missing sessions still evict.list_agents(#481). Each live refresh now builds a live-seat discovery proof, runsevictSurfacelesswith it, and surfacesparsed_cli_mismatchwhen the pane’s parsed CLI disagrees with the record. The largeresync_agentsimplementation,formatResync, and orphan-surface health helpers are removed; the stub errors and docs point callers atlist_agents.Resume (#482). New
resume-verificationchecks harness session stores (present/missing/unverifiable).resumeInvocationForAgentrefuses resume when the artifact is missing; public rows markresumablewithdiskprovenance when verified. Suite default stubs the resolver invitest.setupso tests do not read real~/.claude.Caller resolution (#468). Terminal records matched only by recyclable
surface_idmust be owned by the current observer’ssurface_observer_id; prior-generation corpses no longer winmineattribution.Docs. Control-plane invariants document the eviction windows; README drops
resync_agentsfrom the tool lists.Reviewed by Cursor Bugbot for commit 64a16cf. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix registry to stop serving rows it cannot observe by evicting unclaimed absent agents
UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS), tracked in a newisUnclaimedAbsenceConfirmedhelper in agent-registry.ts.cli_session_idthat still resolves to a present on-disk session artifact are retained regardless of the eviction window; rows with missing artifacts are evicted normally.resumeInvocationForAgentnow refuses to build a resume command when the session artifact is missing;toObservedPublicAgentreportsresumablewith source'disk'when the artifact check is conclusive.list_agentsnow triggers surfaceless eviction using an observer-pinned live seat proof before returning results, and may includeparsed_cli_mismatch: trueon agents where the observed CLI disagrees with the record.resync_agentstool is removed from the server, docs, and README; callers are directed tolist_agentsinstead.surface_observer_idto match the current observer, rejecting terminal records from prior observer generations.Macroscope summarized 64a16cf.
Round 2 — review findings landed (
64a16cf)All six ITERATE items are in. Every test added or changed this round is shown failing against the
pre-fix code first (red-on-red), then passing.
resync_agentsclaimsserver.ts×2 +README.md×2 now point atlist_agentspresentare exemptunclaimedConfirmationMsliveSurfaceKeys.has()control-plane-invariants.md+ both code commentsRED 1 — MUST FIX 1 guard, against pre-fix
src/server.ts+README.mdNew test: "keeps the removed tool out of runtime guidance and the README".
RED 2 — MUST FIX 2, against pre-fix
evictSurfacelessNew test: "keeps an unclaimed row whose captured session is still on disk" — your exact scenario, a
terminal row with a real
cli_session_idand a prior-generation observer.RED 3 — the counter-case is load-bearing too (over-eager retention)
The exemption is
=== "present", not "has a session id". S9: widen it to retain on any status —So the pair pins both directions:
presentretains,missingstill evicts.unverifiableevicts bydesign — making eviction depend on a store directory existing would reopen #480 on any machine
without one. That trade is now stated in the code and in
control-plane-invariants.md.RED 4 / RED 5 — the seat-proof test, now an outcome assertion
Replaced the spy with: seed a crash-recovery-eligible ghost whose seat a live pane holds, drive
list_agentsacross the 5 s confirmation window, assert the ghost is gone and the live seat is not.S5 — eviction call removed (the mutation the old spy test caught):
S5b — proof built from an empty scan (the mutation the old spy test missed):
Both red. Finding A closed.
Finding B — the live-ref retention test
Not rewritten, per your call. It now carries a comment saying exactly what you measured: the property
is upheld upstream (
matchingLiveSurface+clearSurfacelessObservationsForLiveSurfaces), the casewould pass against
main, and it is kept because "never evict a live row" is the property most worthpinning — not because it isolates new code. With minimality item 4 applied, the third guard it also
covered no longer exists.
Verification
bun run test— 133 files, 3103 passed, 1 skipped, exit 0.bun run typecheck— clean.bun run pre-pr— 63 passed. Pre-push harness green.One flake worth the fleet's attention, not caused by this branch. Two suite runs mid-round failed
with
ENOTEMPTY/ENOENT: rename …/T/cmux-agents-test-engine/state.json.tmp. Cause:TEST_DIRisjoin(tmpdir(), "cmux-agents-test-engine")— a fixed path, identical across every worktree, so twolanes running the suite at once fight over the same directory. I confirmed a second
npx vitest run tests/agent-engine.test.tsfrom another lane was live at the time, waited for it toexit, and the suite passed clean. Not fixed here (it touches many test files and is outside T1), but
it will keep biting the fleet tonight — a per-process suffix on those constants is the one-line fix.