From 4e04bfa86d57524867a42dde6b9fb2332de7a5db Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 19:57:26 +0300 Subject: [PATCH 1/2] fix(t1): the registry stops serving rows it cannot observe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/control-plane-invariants.md | 15 + src/agent-facade.ts | 26 +- src/agent-registry.ts | 126 +++++- src/agent-types.ts | 4 +- src/format.ts | 17 - src/resume-verification.ts | 143 ++++++ src/server.ts | 717 +++--------------------------- tests/f1-live-state-truth.test.ts | 67 +++ tests/resume-verification.test.ts | 149 +++++++ tests/server-agent-tools.test.ts | 6 + tests/t1-registry-truth.test.ts | 457 +++++++++++++++++++ tests/vitest.setup.ts | 9 + 12 files changed, 1065 insertions(+), 671 deletions(-) create mode 100644 src/resume-verification.ts create mode 100644 tests/resume-verification.test.ts create mode 100644 tests/t1-registry-truth.test.ts diff --git a/docs/control-plane-invariants.md b/docs/control-plane-invariants.md index f73b7f43..cd990022 100644 --- a/docs/control-plane-invariants.md +++ b/docs/control-plane-invariants.md @@ -37,6 +37,21 @@ recovery, and sidebar/reporting decisions. An empty/failed surface listing is inconclusive (`unknown`); only a non-empty topology lacking a specific surface proves absence; stale records reap on the next non-empty scan. +### Bounded eviction windows (#480) + +A registry row is dropped once its absence is confirmed for a window, and the window depends on +whether a live observer claims the row. Neither window is unbounded, which is the invariant the +measured 36-day ghosts violated. + +| Row | Window | Constant | Path | +| --- | --- | --- | --- | +| `surface_observer_id` equals the current observer | 5 s of continuous absence | `SURFACE_EVICTION_CONFIRMATION_MS` (`src/agent-registry.ts`) | sweep + `list_agents` | +| `surface_observer_id` is null or from a prior observer generation | 60 s of continuous absence, where absence means no live surface bears the row's UUID **or** its ref | `UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS` (`src/agent-registry.ts`) | sweep + `list_agents` | + +Ownership stops one observer mutating another's *live* row; it is not a claim on a row that no +live surface bears. An unclaimed row is evicted, never crash-marked: eviction is the reversible +direction, because `listMerged` re-mints a row from discovery if the pane turns out to be alive. + ## Allowed Transitions Allowed transitions are intentionally narrower than current ad hoc state movement: diff --git a/src/agent-facade.ts b/src/agent-facade.ts index e9dd408d..34fe4611 100644 --- a/src/agent-facade.ts +++ b/src/agent-facade.ts @@ -13,6 +13,10 @@ import { rawResumeNeedsCwd, rawResumeSupported, } from "./agent-command.js"; +import { + resumeArtifactStatus, + type ResumeArtifactStatus, +} from "./resume-verification.js"; export type AgentStatePayload = AgentRecord & { resumable: boolean; @@ -57,6 +61,19 @@ export function resumeInvocationForAgent( if (!record.cli_session_id) { return { command: null, reason: "no CLI session has been captured" }; } + // #482: a formattable id is not a resumable agent. A seat that survived a + // restart keeps the OLD id, so the command would open a fresh session + // wearing the seat's name. Refuse on proof of absence only -- an + // unverifiable store leaves the claim standing. + if (resumeArtifactStatus(record.cli, record.cli_session_id) === "missing") { + return { + command: null, + reason: + `captured ${record.cli} session ${record.cli_session_id} is not in ` + + `the harness session store; resuming it would start a NEW session ` + + `under this agent's name, not restore it`, + }; + } const cwd = resumeCwdForAgent(record); if (!record.launcher_name) { if (!cwd && rawResumeNeedsCwd(record.cli)) { @@ -147,6 +164,13 @@ export function toObservedPublicAgent( const registryObservedAtMs = derivedAtMs; const resumeCommand = resumeCommandForAgent(record); const resumable = !!resumeCommand; + // #482 provenance: `disk` means a session artifact was looked for and + // found (or proven absent). `registry` means the claim is unverified. + const artifactStatus: ResumeArtifactStatus = record.cli_session_id + ? resumeArtifactStatus(record.cli, record.cli_session_id) + : "unverifiable"; + const resumableSource: ObservationSource = + artifactStatus === "unverifiable" ? "registry" : "disk"; const hasScreenModelObservation = opts.screenObservedAtMs !== undefined && opts.screenModel != null; const model = hasScreenModelObservation @@ -178,7 +202,7 @@ export function toObservedPublicAgent( "registry", registryObservedAtMs, ), - resumable: observed(resumable, "registry", registryObservedAtMs), + resumable: observed(resumable, resumableSource, registryObservedAtMs), submit_verified: observed( record.submit_verified ?? null, "registry", diff --git a/src/agent-registry.ts b/src/agent-registry.ts index 4322e1fe..e3c2115d 100644 --- a/src/agent-registry.ts +++ b/src/agent-registry.ts @@ -61,6 +61,11 @@ interface SurfacelessEvictionOptions extends SurfaceAbsenceOptions { * still owns the shell, so crash-recovery rows may only yield to this proof. */ liveSeatProof?: LiveSeatDiscoveryProof | null; + /** + * Continuous-absence window before a row that no live observer claims is + * dropped (#480). Defaults to `UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS`. + */ + unclaimedConfirmationMs?: number; } export interface AgentRegistryOptions { @@ -105,6 +110,23 @@ export function deriveSurfaceObserverId( export const SURFACE_EVICTION_CONFIRMATION_MS = 5_000; +/** + * Absence window for rows NO live observer claims (#480). + * + * `canMutateForObservedAbsence` requires an exact observer match, so a row + * whose `surface_observer_id` is null (pre-observer-identity) or belongs to a + * dead socket generation could never be evicted, never be crash-marked, and + * never be purged: worst-case survival was unbounded. Measured 2026-08-19: + * four such rows, oldest 36 days, `list_agents` 17 vs `list_surfaces` 13. + * + * Ownership exists to stop one observer mutating another's LIVE row. It is not + * a claim on a row that no live surface bears — by uuid or by ref — across a + * continuous absence window. This constant is that window: twelve consecutive + * 5 s sweeps of proven absence before an unclaimed row is dropped, so the + * worst case is bounded and documented rather than infinite. + */ +export const UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS = 60_000; + export interface AgentFilter { state?: AgentState; repo?: string; @@ -501,6 +523,16 @@ export class AgentRegistry { string, { surfaceId: string; firstObservedAt: number } >(); + /** + * Absence clock for rows this observer does not own (#480). Kept separate + * from `surfacelessObservations` on purpose: that map is cleared by the + * ownership gate itself (`isSurfacelessConfirmed`), so an unclaimed row can + * never accumulate time in it. + */ + private unclaimedAbsenceObservations = new Map< + string, + { surfaceId: string; firstObservedAt: number } + >(); private stateMgr: StateManager; private surfaceProvider: SurfaceProvider; private observerId: string | null; @@ -645,7 +677,7 @@ export class AgentRegistry { async reconstitute(opts: SurfaceAbsenceOptions = {}): Promise> { this.agents.clear(); this.aliases.clear(); - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); const stateFiles = this.stateMgr.listStates(); for (const record of stateFiles) { @@ -749,7 +781,7 @@ export class AgentRegistry { // Incomplete or contradictory identity evidence can prove neither // presence nor absence. // Reset pending absence timers so a later valid scan starts fresh. - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return new Set(); } const liveSurfaceKeys = this.liveSurfaceKeys(surfaces); @@ -909,6 +941,7 @@ export class AgentRegistry { const aliases = this.aliasesResolvingTo(resolved); this.agents.delete(resolved); this.surfacelessObservations.delete(resolved); + this.unclaimedAbsenceObservations.delete(resolved); this.aliases.delete(agentId); this.aliases.delete(resolved); for (const alias of aliases) { @@ -1013,7 +1046,7 @@ export class AgentRegistry { if (!discoveryIsBijective || discoveryHasMixedIdentity) { // A degraded discovery scan must break any pending negative-evidence // streak even when exact UUID matches remain safe for positive sync. - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); } if (!discoveryIsBijective) { return this.list(opts?.filter).map((record) => ({ @@ -1237,7 +1270,7 @@ export class AgentRegistry { const discoveryHasMixedIdentity = hasMixedDiscoveryIdentityCoverage(discovered); if (!discoveryIsBijective || discoveryHasMixedIdentity) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); } if (!discoveryIsBijective) { return opts?.agentId ? this.get(opts.agentId) : null; @@ -1629,6 +1662,12 @@ export class AgentRegistry { if (surfacelessObservation?.surfaceId === this.agentSurfaceKey(record)) { this.surfacelessObservations.set(newAgentId, surfacelessObservation); } + const unclaimedObservation = + this.unclaimedAbsenceObservations.get(oldAgentId); + this.unclaimedAbsenceObservations.delete(oldAgentId); + if (unclaimedObservation?.surfaceId === this.agentSurfaceKey(record)) { + this.unclaimedAbsenceObservations.set(newAgentId, unclaimedObservation); + } for (const [alias, target] of this.aliases) { if (target === oldAgentId) { this.aliases.set(alias, newAgentId); @@ -1693,7 +1732,7 @@ export class AgentRegistry { return []; } if (!hasCoherentSurfaceIdentity(surfaces)) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return []; } @@ -1704,10 +1743,29 @@ export class AgentRegistry { for (const [id, agent] of [...this.agents.entries()]) { if (agent.transcript_session_capture_deferred === true) { this.surfacelessObservations.delete(agent.agent_id); + this.unclaimedAbsenceObservations.delete(agent.agent_id); continue; } if (this.matchingLiveSurface(agent, surfaces)) { this.surfacelessObservations.delete(agent.agent_id); + this.unclaimedAbsenceObservations.delete(agent.agent_id); + continue; + } + // #480: rows no live observer claims take the bounded unclaimed path + // instead of dying at the ownership gate below. They cannot be evicted, + // 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)) { + if ( + !this.isUnclaimedAbsenceConfirmed(agent, liveSurfaceKeys, opts) + ) { + continue; + } + const removedUnclaimedId = this.evictUnchecked(id); + if (removedUnclaimedId) { + evicted.push(removedUnclaimedId); + } continue; } if (!this.isSurfaceAbsenceAuthoritative(agent, surfaces)) { @@ -1874,6 +1932,50 @@ export class AgentRegistry { return now - observation.firstObservedAt >= confirmationMs; } + /** + * Continuous, ref-AND-uuid absence of a row that this observer does not own + * (#480). Deliberately does NOT consult `isSurfaceAbsenceAuthoritative`: + * that helper refuses to read a UUID-less row's absence in a UUID-bearing + * topology because a live occupant on the same mutable ref proves nothing + * about the row. Here the ref is not occupied at all -- no live surface + * carries the row's uuid OR its ref -- across a coherent, non-empty scan. + * That is real absence evidence, and it is the only evidence an unclaimed + * row can ever produce. + * + * Eviction, not crash-marking: dropping the registry row is the reversible + * direction. If the pane were somehow alive, `listMerged` re-mints it from + * discovery on the next call; marking a live agent `error` would not + * self-correct. + */ + private isUnclaimedAbsenceConfirmed( + agent: AgentRecord, + liveSurfaceKeys: ReadonlySet, + opts: { unclaimedConfirmationMs?: number; now?: number }, + ): boolean { + const surfaceKey = this.agentSurfaceKey(agent); + if (liveSurfaceKeys.has(surfaceKey)) { + this.unclaimedAbsenceObservations.delete(agent.agent_id); + return false; + } + const confirmationMs = Math.max( + 0, + opts.unclaimedConfirmationMs ?? UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS, + ); + if (confirmationMs === 0) { + return true; + } + const now = opts.now ?? Date.now(); + const observation = this.unclaimedAbsenceObservations.get(agent.agent_id); + if (!observation || observation.surfaceId !== surfaceKey) { + this.unclaimedAbsenceObservations.set(agent.agent_id, { + surfaceId: surfaceKey, + firstObservedAt: now, + }); + return false; + } + return now - observation.firstObservedAt >= confirmationMs; + } + private canMutateForObservedAbsence( agent: AgentRecord, observerEpoch?: string | null, @@ -1895,6 +1997,11 @@ export class AgentRegistry { return !owner || Boolean(observerId && owner === observerId); } + private clearAbsenceObservations(): void { + this.surfacelessObservations.clear(); + this.unclaimedAbsenceObservations.clear(); + } + private clearSurfacelessObservationsForLiveSurfaces( liveSurfaceKeys: ReadonlySet, ): void { @@ -1903,6 +2010,11 @@ export class AgentRegistry { this.surfacelessObservations.delete(agentId); } } + for (const [agentId, observation] of this.unclaimedAbsenceObservations) { + if (liveSurfaceKeys.has(observation.surfaceId)) { + this.unclaimedAbsenceObservations.delete(agentId); + } + } } repairFromDiscovery( @@ -1916,7 +2028,7 @@ export class AgentRegistry { !hasBijectiveDiscoveryIdentity(discovered) || hasMixedDiscoveryIdentityCoverage(discovered) ) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return { repaired: [], evicted: [], skipped: [] }; } const repaired: RegistryRepairEntry[] = []; @@ -2463,7 +2575,7 @@ export class AgentRegistry { return 0; } if (!hasCoherentSurfaceIdentity(surfaces)) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return 0; } const liveSurfaceKeys = this.liveSurfaceKeys(surfaces); diff --git a/src/agent-types.ts b/src/agent-types.ts index b3dbf368..49250e9f 100644 --- a/src/agent-types.ts +++ b/src/agent-types.ts @@ -21,7 +21,9 @@ export type AgentFunction = "implementor" | "reviewer" | "gatherer"; export type AgentPlacement = "left" | "right"; export type SurfaceProvenance = "cmuxlayer_spawn" | "unknown"; export type SeatIdentityStatus = "ok" | "mismatch" | "unknown"; -export type ObservationSource = "screen" | "registry" | "process"; +// `disk` is a filesystem observation (today: the harness session artifact +// behind `resumable`, #482), as opposed to a remembered registry field. +export type ObservationSource = "screen" | "registry" | "process" | "disk"; export type AgentReviveOutcome = "pending" | "failed" | "revived" | "unrecoverable"; export type AgentHaltType = diff --git a/src/format.ts b/src/format.ts index c47eecff..e805430b 100644 --- a/src/format.ts +++ b/src/format.ts @@ -307,20 +307,3 @@ export function formatDelivery( submit = " \u00b7 submit_verified=null (not attempted)"; return `\u2714 ${action} \u2500 ${head}${submit}`; } - -export function formatResync(diff: { - added: string[]; - evicted: string[]; - repaired?: unknown[]; - reflowed?: unknown[]; - mismatches: string[]; - orphaned?: string[]; -}): string { - const added = diff.added.length; - const evicted = diff.evicted.length; - const repaired = diff.repaired?.length ?? 0; - const reflowed = diff.reflowed?.length ?? 0; - const mismatches = diff.mismatches.length; - const orphaned = diff.orphaned?.length ?? 0; - return `✔ resync_agents — added: ${added} repaired: ${repaired} reflowed: ${reflowed} evicted: ${evicted} mismatches: ${mismatches} orphaned: ${orphaned}`; -} diff --git a/src/resume-verification.ts b/src/resume-verification.ts new file mode 100644 index 00000000..b492df45 --- /dev/null +++ b/src/resume-verification.ts @@ -0,0 +1,143 @@ +import { existsSync } from "node:fs"; +import { homedir } from "node:os"; +import { join } from "node:path"; +import type { CliType } from "./agent-types.js"; +import { findHarnessSessionPath, type Harness } from "./harness-session.js"; + +/** + * AIDEV-NOTE (#482): `resumable` used to mean "we can format a string". + * `buildResumeCommand` validates only that the captured id looks like a UUID, + * so a lead that survived a restart kept a stale `cli_session_id` and was + * advertised as recoverable while its session existed nowhere on disk (2 of 13 + * rows, both LEAD seats, measured 2026-08-19). This module is the observation + * that claim was missing. + * + * Three answers, never two: `missing` requires having LOOKED in a store that + * exists. When the harness keeps no addressable session store, or this machine + * has none, the answer is `unverifiable` — the claim stays unverified rather + * than being flipped to a confident false. + */ +export type ResumeArtifactStatus = "present" | "missing" | "unverifiable"; + +export type ResumeArtifactResolver = ( + cli: CliType, + sessionId: string, +) => ResumeArtifactStatus; + +export interface ResumeArtifactOptions { + /** Home directory holding the harness stores. Defaults to `os.homedir()`. */ + home?: string; + /** Codex root override, mirroring `findHarnessSessionPath`. */ + codexHome?: string; +} + +/** Harnesses whose session store cmuxlayer can address by session id. */ +function harnessForCli(cli: CliType): Harness | null { + switch (cli) { + case "claude": + return "claude"; + case "codex": + return "codex"; + case "cursor": + return "cursor"; + // gemini has no UUID-addressable store; kiro's is not readable here. + default: + return null; + } +} + +/** + * Same override contract the session-capture paths already use + * (`agent-engine.ts`, `server.ts`): `CMUXLAYER_HARNESS_HOME` relocates the + * whole harness home, `CODEX_HOME` relocates codex's. + */ +function resolveOptionsFromEnv( + opts: ResumeArtifactOptions, +): ResumeArtifactOptions { + return { + ...(process.env.CMUXLAYER_HARNESS_HOME + ? { home: process.env.CMUXLAYER_HARNESS_HOME } + : {}), + ...(process.env.CODEX_HOME ? { codexHome: process.env.CODEX_HOME } : {}), + ...opts, + }; +} + +function storeRoot(harness: Harness, opts: ResumeArtifactOptions): string { + const home = opts.home ?? homedir(); + switch (harness) { + case "claude": + return join(home, ".claude", "projects"); + case "cursor": + return join(home, ".cursor", "projects"); + case "codex": + return join(opts.codexHome ?? join(home, ".codex"), "sessions"); + } +} + +/** The real filesystem observation. Cheap: a bounded walk of one store root. */ +export function resolveResumeArtifact( + cli: CliType, + sessionId: string, + callerOpts: ResumeArtifactOptions = {}, +): ResumeArtifactStatus { + if (!sessionId) return "unverifiable"; + const harness = harnessForCli(cli); + if (!harness) return "unverifiable"; + const opts = resolveOptionsFromEnv(callerOpts); + const root = storeRoot(harness, opts); + // 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) + ? "present" + : "missing"; +} + +const PRESENT_TTL_MS = 60_000; +const NEGATIVE_TTL_MS = 5_000; +const statusCache = new Map< + string, + { status: ResumeArtifactStatus; expiresAt: number } +>(); + +/** + * Default resolver: the filesystem check, memoised. `list_agents` asks once + * per row, so an uncached miss would re-walk the store for every row on every + * call. A `present` answer is stable enough to hold for a minute; a `missing` + * one expires fast because a resume creates the file. + */ +function cachedResolver(cli: CliType, sessionId: string): ResumeArtifactStatus { + const key = `${cli}:${sessionId}`; + const now = Date.now(); + const cached = statusCache.get(key); + if (cached && cached.expiresAt > now) { + return cached.status; + } + const status = resolveResumeArtifact(cli, sessionId); + statusCache.set(key, { + status, + expiresAt: now + (status === "present" ? PRESENT_TTL_MS : NEGATIVE_TTL_MS), + }); + return status; +} + +let resolver: ResumeArtifactResolver = cachedResolver; + +/** Test and embedding seam; production uses the cached filesystem resolver. */ +export function setResumeArtifactResolver(next: ResumeArtifactResolver): void { + resolver = next; + statusCache.clear(); +} + +export function resetResumeArtifactResolver(): void { + resolver = cachedResolver; + statusCache.clear(); +} + +export function resumeArtifactStatus( + cli: CliType, + sessionId: string, +): ResumeArtifactStatus { + return resolver(cli, sessionId); +} diff --git a/src/server.ts b/src/server.ts index 78438694..157cbd5e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -105,10 +105,8 @@ import { toObservedPublicAgent, } from "./agent-facade.js"; import { - DEFAULT_AGENT_HEALTH_ISSUE_SEVERITY, evaluateAgentHealth, type AgentHealth, - type AgentHealthIssueCode, } from "./agent-health.js"; import { AGENT_HEALTH_DISPATCH_ACK_TIMEOUT_MS, @@ -140,7 +138,6 @@ import { formatAgentState, formatOk, formatDelivery, - formatResync, } from "./format.js"; import { cleanScreenText, @@ -3621,23 +3618,41 @@ export function createServer(opts?: CreateServerOptions): McpServer { // is the best available caller identity even when the record is stale -- // the call itself is the liveness evidence. The live-first ordering still // lets a genuinely live record win a recycled surface (#378 MEDIUM-A). - // AIDEV-TODO (F1 review, finding 2): the LAST tier below matches a terminal - // record by `surface_id`, and `surface_id` is a RECYCLABLE ref -- a dead - // worker's record whose ref got reused can claim to be the caller, and #378 - // then forces the new pane's children to worker/right off a corpse. The - // obvious guard -- compare the live pane's CLI to the record's, as + // AIDEV-NOTE (#468): the LAST tier matches a TERMINAL record by + // `surface_id`, and `surface_id` is a RECYCLABLE ref -- a dead worker's + // record whose ref got reused could claim to be the caller, and #378 then + // forced the new pane's children to worker/right off a corpse. The obvious + // guard -- compare the live pane's CLI to the record's, as // deliverAgentInput does -- does NOT work here: `registry.listMerged` // rewrites `record.cli` from the live pane, so by the time caller // resolution runs, a recycled record already claims the new occupant's CLI. - // Needs a signal the merge does not overwrite. Tiers 1 and 3 (UUID) are - // unaffected. Tracked in #468. + // + // `surface_observer_id` IS a signal the merge does not overwrite: it is + // stamped when this observer binds the surface and only ever replaced by + // another binding. A ref stamped by a dead socket generation (or never + // stamped at all) proves nothing about who occupies that ref now, so those + // records are refused at the ref-only tier. Tiers 1 and 3 (UUID) are + // unaffected -- a UUID is not recyclable -- and a record this observer owns + // still resolves, which is what keeps U6 working for #408-poisoned rows. + // + // Cost, stated plainly: a caller whose record predates observer identity + // gets no attribution and sees an explicit refusal instead of a wrong + // parent. That is the trade this repo already makes everywhere else + // absence is ambiguous. + const observerOwnerId = context.surfaceObserverId?.trim() || null; + const ownsRefBinding = (agent: AgentRecord): boolean => + Boolean( + observerOwnerId && agent.surface_observer_id === observerOwnerId, + ); const live = (agent: AgentRecord): boolean => !isLiveTerminal(liveStateFor(agent)); return ( records.find((agent) => matchesUuid(agent) && live(agent)) ?? records.find((agent) => matchesSurfaceId(agent) && live(agent)) ?? records.find(matchesUuid) ?? - records.find(matchesSurfaceId) ?? + records.find( + (agent) => matchesSurfaceId(agent) && ownsRefBinding(agent), + ) ?? null ); }; @@ -7329,65 +7344,6 @@ export function createServer(opts?: CreateServerOptions): McpServer { fallback?: string, ): string | undefined => result.workspace_id || fallback; - const isLeadLikeSurfaceTitle = (title: string): boolean => - /\b(?:lead|orchestrator|coordinator|coord)\b/i.test(title); - - const buildOrphanSurfaceHealth = (surface: DiscoveredAgent) => { - const issueCodes: AgentHealthIssueCode[] = []; - const issues: string[] = []; - if (surface.has_agent) { - issueCodes.push("auto_discovered_agent"); - issues.push( - "live agent surface has no managed registry seat; repair/register the seat or leave it visible as an unresolved orphan", - ); - } - if (isLeadLikeSurfaceTitle(surface.surface_title)) { - issueCodes.push("missing_managed_lead_agent_id"); - issues.push( - "lead/coordinator surface has no managed agent_id; recover/register or replace with a managed lead", - ); - } - - const title = surface.surface_title.trim().toLowerCase(); - if ( - title === "" || - title === "gits" || - title === "git" || - title === "repos" || - title === "projects" || - title === "workspace" - ) { - issueCodes.push("ambiguous_repo_cwd_label"); - issues.push( - "orphan terminal surface has an ambiguous repo/cwd label; tab title is not lane ownership", - ); - } - - const issueSeverities = Object.fromEntries( - issueCodes.map((code) => [ - code, - DEFAULT_AGENT_HEALTH_ISSUE_SEVERITY[code], - ]), - ); - const hasBlockingIssue = issueCodes.some( - (code) => DEFAULT_AGENT_HEALTH_ISSUE_SEVERITY[code] === "blocking", - ); - return { - surface_id: surface.surface_id, - surface_title: surface.surface_title, - workspace_id: surface.workspace_id ?? null, - status: - issueCodes.length === 0 - ? "unknown" - : hasBlockingIssue - ? "unhealthy" - : "degraded", - issue_codes: issueCodes, - issues, - ...(issueCodes.length > 0 ? { issue_severities: issueSeverities } : {}), - }; - }; - const collectDeliveryEvidence = async (agentId: string) => { const agent = context.lifecycleSweepEngine?.getAgentState(agentId) ?? null; if (!agent) { @@ -13637,7 +13593,9 @@ export function createServer(opts?: CreateServerOptions): McpServer { return okFormatted(formatted, data); }; const buildListAgentsResponse = async ( - records: AgentRecord[], + // `listMerged` hands back MergedAgent rows; the merge-only fields are + // optional so cached/registry-only callers still type-check. + records: Array, topology: SurfaceTopologySnapshot | null, topologySignature: string, liveDiscovery?: { @@ -13728,6 +13686,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { }), surface_id: agent.surface_id, send_via: "send_to" as const, + // #481: computed on every listMerged, read only by the + // removed resync tool's dead body -- so a pane whose + // observed CLI disagreed with its record was silently + // un-surfaced. Sparse on purpose: agreement is the normal + // case and must cost no payload. + ...(agent.parsed_cli_mismatch === true + ? { parsed_cli_mismatch: true } + : {}), // P11 Constraint 3: at DEFAULT detail, so a lead can tell a // deadlocked child (done, no artifact -> act) from a busy one // (pending -> wait) WITHOUT a second full-detail call. A bare @@ -13805,6 +13771,28 @@ export function createServer(opts?: CreateServerOptions): McpServer { seatRegistry, orphansOnly: true, }); + // #481: `createLiveSeatDiscoveryProof` had exactly one call site -- + // inside the removed resync tool's unreachable body -- so + // `hasLiveManagedSeatSibling` returned false unconditionally and + // every crash-recovery-eligible ghost was retained forever. This is + // the live path that already holds a same-cycle, observer-pinned + // scan, so the proof belongs here. + // #480: it is also the only reconciliation callers actually + // trigger. Without an eviction here `list_agents` was the one + // reader that never dropped a row: 17 agents against 13 surfaces. + const liveSeatProof = registry.createLiveSeatDiscoveryProof( + discovered, + { + seatRegistry, + expectedObserverId: registry.getObserverId(), + expectedObserverEpoch: registry.getObserverEpoch(), + }, + ); + await registry.evictSurfaceless({ + confirmationMs: SURFACE_EVICTION_CONFIRMATION_MS, + now: observedAtMs, + liveSeatProof, + }); const merged = await registry.listMerged(discovery, { filter, force: true, @@ -14080,582 +14068,21 @@ export function createServer(opts?: CreateServerOptions): McpServer { server.tool( "resync_agents", - "Removed compatibility stub. Agent discovery and reconciliation now happen automatically on list_agents; callers must not resync manually.", + "Removed. Reconciliation runs automatically on list_agents: fresh discovery, orphan repair, and ghost eviction carrying a same-cycle live-seat proof. Role reflow runs on the periodic sweep. Call list_agents.", {}, ANNOTATIONS.readOnly, - async () => { - const compatibilityStubRemoved: boolean = true; - if (compatibilityStubRemoved) { - return err( - new Error( - "resync_agents was removed; call list_agents for an automatically refreshed live view", - ), - ); - } - /* c8 ignore start -- retained for one release as unreachable rollback reference */ - await awaitLifecycleStart(); - return engine.runLifecycleMutation(async () => { - try { - const beforeIds = new Set( - registry.list().map((agent) => agent.agent_id), - ); - const surfaceAbsenceConfirmation = { - confirmationMs: SURFACE_EVICTION_CONFIRMATION_MS, - }; - await registry.reconcile(surfaceAbsenceConfirmation); - for (const agent of registry.list()) { - beforeIds.add(agent.agent_id); - } - discovery.invalidate(); - const discoveredBeforeRepair = await discovery.scan(true); - const repair = registry.repairFromDiscovery( - discoveredBeforeRepair, - { - seatRegistry, - }, - ); - const liveSeatProofObserverId = registry.getObserverId(); - const liveSeatProofObserverEpoch = registry.getObserverEpoch(); - discovery.invalidate(); - const discoveredAfterRepair = await discovery.scan(true); - const liveSeatProof = registry.createLiveSeatDiscoveryProof( - discoveredAfterRepair, - { - seatRegistry, - expectedObserverId: liveSeatProofObserverId, - expectedObserverEpoch: liveSeatProofObserverEpoch, - }, - ); - await registry.listMerged(discovery, { - force: true, - discovered: discoveredAfterRepair, - }); - const surfacelessEvicted = await registry.evictSurfaceless({ - ...surfaceAbsenceConfirmation, - liveSeatProof, - }); - engine.evictDeadProcessAgents(); - discovery.invalidate(); - let after = await registry.listMerged(discovery, { force: true }); - const reflowObserverEpoch = captureObserverEpoch( - surfaceObserverEpochProvider(), - ); - const topologyBeforeReflow = await collectSurfaceTopology(); - const topologyIsCoherent = ( - topology: SurfaceTopologySnapshot | null, - ): topology is SurfaceTopologySnapshot => { - const surfaceCount = topology?.workspaceBySurface.size ?? 0; - const uuidCount = topology?.surfaceIdByRef.size ?? 0; - return ( - topology?.complete === true && - surfaceCount > 0 && - (uuidCount === 0 || uuidCount === surfaceCount) - ); - }; - const topologyBeforeReflowIsCoherent = - topologyIsCoherent(topologyBeforeReflow); - const reflowed: Array<{ - agent_id: string; - surface_id: string; - from_column: number; - to_column: number; - pane: string; - }> = []; - type ReflowOperation = - "new_split" | "move_surface" | "verify_reflow" | "close_surface"; - const reflowSkipped: Array<{ - agent_id: string; - surface_id: string; - operation: ReflowOperation; - reason: string; - }> = []; - const recordReflowSkip = ( - agent: AgentRecord, - surfaceId: string, - operation: ReflowOperation, - error: unknown, - ): void => { - reflowSkipped.push({ - agent_id: agent.agent_id, - surface_id: surfaceId, - operation, - reason: error instanceof Error ? error.message : String(error), - }); - }; - const assertCurrentReflowAuthority = ( - agent: AgentRecord, - expectedWorkspace: string, - operation: ReflowOperation, - ): AgentRecord => { - const currentAgent = - registry.get(agent.agent_id) ?? - stateMgr.readState(agent.agent_id); - const expectedUuid = agent.surface_uuid?.trim().toLowerCase(); - const currentUuid = currentAgent?.surface_uuid - ?.trim() - .toLowerCase(); - if ( - !currentAgent || - currentAgent.surface_provenance !== "cmuxlayer_spawn" || - inferRecordRoleOrNull(currentAgent) !== "worker" || - (currentAgent.state !== "idle" && - !TERMINAL_AGENT_STATES.has(currentAgent.state)) || - !expectedUuid || - currentUuid !== expectedUuid || - (currentAgent.workspace_id ?? null) !== expectedWorkspace - ) { - throw new Error( - `Agent ${agent.agent_id} provenance, role, state, or stable ` + - `binding changed before ${operation}; refusing to move a busy or unowned pane.`, - ); - } - return currentAgent; - }; - const resolveFreshReflowBinding = async ( - agent: AgentRecord, - expectedSurfaceRef: string, - expectedWorkspace: string, - operation: ReflowOperation, - ) => { - assertCurrentReflowAuthority(agent, expectedWorkspace, operation); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - `resync_agents ${operation}`, - ); - const topology = await collectSurfaceTopology(); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - `resync_agents ${operation}`, - ); - if (!topologyIsCoherent(topology)) { - throw new Error( - `Fresh topology is incomplete before ${operation}; refusing reflow mutation.`, - ); - } - const currentAgent = assertCurrentReflowAuthority( - agent, - expectedWorkspace, - operation, - ); - const binding = resolveAgentSurfaceBinding( - currentAgent, - topology, - ); - if (!binding) { - throw new Error( - `Stable surface UUID ${agent.surface_uuid ?? "unavailable"} is not uniquely bound before ${operation}; refusing reflow mutation.`, - ); - } - const observedUuid = - topology.surfaceIdByRef.get(binding.surfaceRef) ?? null; - if (!registry.canUseObservedBinding(currentAgent, observedUuid)) { - throw new Error( - `Fresh binding ${binding.surfaceRef} is not owned by the current observer before ${operation}; refusing reflow mutation.`, - ); - } - const workspace = - topology.workspaceBySurface.get(binding.surfaceRef) ?? - binding.workspaceId; - if ( - binding.surfaceRef !== expectedSurfaceRef || - workspace !== expectedWorkspace - ) { - throw new Error( - `Surface binding changed before ${operation} ` + - `(${expectedSurfaceRef}@${expectedWorkspace} -> ` + - `${binding.surfaceRef}@${workspace ?? "unknown"}); refusing to mutate a recycled ref.`, - ); - } - const current = topology.topologyBySurface.get( - binding.surfaceRef, - ); - if (current?.column !== 0) { - throw new Error( - `Stable surface UUID ${agent.surface_uuid ?? "unavailable"} no longer needs left-column reflow before ${operation}.`, - ); - } - return { binding, current, topology, workspace }; - }; - - if (topologyBeforeReflow && topologyBeforeReflowIsCoherent) { - const panesByWorkspace = new Map< - string, - Awaited> - >(); - - for (const agent of after) { - if (inferRecordRoleOrNull(agent) !== "worker") continue; - if (agent.surface_provenance !== "cmuxlayer_spawn") continue; - if ( - agent.state !== "idle" && - !TERMINAL_AGENT_STATES.has(agent.state) - ) { - continue; - } - const binding = resolveAgentSurfaceBinding( - agent, - topologyBeforeReflow, - ); - if (!binding) continue; - const observedUuid = - topologyBeforeReflow.surfaceIdByRef.get(binding.surfaceRef) ?? - null; - if (!registry.canUseObservedBinding(agent, observedUuid)) { - continue; - } - const liveSurfaceRef = binding.surfaceRef; - const current = - topologyBeforeReflow.topologyBySurface.get(liveSurfaceRef); - if (current?.column !== 0) continue; - - let seededSurface: string | null = null; - let seededSurfaceUuid: string | null = null; - let workspace: string | null = null; - let attemptedOperation: ReflowOperation = - (current.column_count ?? 0) < 2 - ? "new_split" - : "move_surface"; - try { - workspace = - topologyBeforeReflow.workspaceBySurface.get( - liveSurfaceRef, - ) ?? - agent.workspace_id ?? - null; - if (!workspace) continue; - - let targetPane: string | null = null; - if ((current.column_count ?? 0) < 2) { - attemptedOperation = "new_split"; - await resolveFreshReflowBinding( - agent, - liveSurfaceRef, - workspace, - attemptedOperation, - ); - await assertWorkspaceMutationAllowed( - "new_split", - workspace, - ); - await withSurfaceWrite( - liveSurfaceRef, - async () => { - const immediate = await resolveFreshReflowBinding( - agent, - liveSurfaceRef, - workspace!, - attemptedOperation, - ); - await assertWorkspaceMutationAllowed( - "new_split", - immediate.workspace ?? workspace!, - ); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents new_split", - ); - assertCurrentReflowAuthority( - agent, - immediate.workspace ?? workspace!, - attemptedOperation, - ); - const seed = await client.newSplit("right", { - workspace: immediate.workspace, - surface: immediate.binding.surfaceRef, - type: "terminal", - }); - seededSurface = seed.surface; - seededSurfaceUuid = seed.surface_id ?? null; - targetPane = seed.pane; - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents new_split", - ); - }, - { - owner: `resync-reflow:new_split:${agent.agent_id}`, - stableSurfaceIdentity: agent.surface_uuid, - }, - ); - panesByWorkspace.delete(workspace); - } else { - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents move_surface pane selection", - ); - let panes = panesByWorkspace.get(workspace); - if (!panes) { - panes = await client.listPanes({ workspace }); - panesByWorkspace.set(workspace, panes); - } - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents move_surface pane selection", - ); - targetPane = - topPaneInRoleColumn(panes.panes, "worker")?.ref ?? null; - } - if (!targetPane) continue; - - attemptedOperation = "move_surface"; - const freshBeforeMove = await resolveFreshReflowBinding( - agent, - liveSurfaceRef, - workspace, - attemptedOperation, - ); - await withSurfaceWrite( - freshBeforeMove.binding.surfaceRef, - async () => { - const immediate = await resolveFreshReflowBinding( - agent, - liveSurfaceRef, - workspace!, - attemptedOperation, - ); - await assertSurfaceMutationAllowed( - "move_surface", - immediate.binding.surfaceRef, - immediate.workspace ?? workspace!, - ); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents move_surface", - ); - assertCurrentReflowAuthority( - agent, - immediate.workspace ?? workspace!, - attemptedOperation, - ); - await client.moveSurface({ - surface: immediate.binding.surfaceRef, - pane: targetPane!, - workspace: immediate.workspace, - focus: false, - }); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents move_surface", - ); - }, - { - toolName: "move_surface", - workspace: freshBeforeMove.workspace ?? workspace, - owner: `resync-reflow:move_surface:${agent.agent_id}`, - stableSurfaceIdentity: agent.surface_uuid, - }, - ); - panesByWorkspace.delete(workspace); - - attemptedOperation = "verify_reflow"; - const topologyAfterMove = - await collectSurfaceTopology(workspace); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents verify_reflow", - ); - if (!topologyIsCoherent(topologyAfterMove)) { - throw new Error( - "Post-move topology is incomplete; reflow could not be verified.", - ); - } - const bindingAfterMove = resolveAgentSurfaceBinding( - agent, - topologyAfterMove, - ); - if (!bindingAfterMove) { - throw new Error( - "Post-move stable UUID binding is unavailable; reflow could not be verified.", - ); - } - const actual = topologyAfterMove.topologyBySurface.get( - bindingAfterMove.surfaceRef, - ); - if (actual?.column !== 1) { - throw new Error( - "Post-move topology does not place the worker in canonical column 1.", - ); - } - - reflowed.push({ - agent_id: agent.agent_id, - surface_id: bindingAfterMove.surfaceRef, - from_column: current.column, - to_column: actual.column, - pane: targetPane, - }); - } catch (error) { - // Reflow is self-healing best effort. One stale workspace, pane, - // or topology read must not abort the registry-wide resync. - recordReflowSkip( - agent, - liveSurfaceRef, - attemptedOperation, - error, - ); - } finally { - if (seededSurface) { - try { - const cleanupSeedUuid = seededSurfaceUuid as - string | null; - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents close_surface", - ); - if (!cleanupSeedUuid) { - throw new Error( - `Seed ${seededSurface} has no stable UUID; refusing cleanup by mutable ref.`, - ); - } - const cleanupTopology = await collectSurfaceTopology(); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents close_surface", - ); - if (!topologyIsCoherent(cleanupTopology)) { - throw new Error( - "Fresh topology is incomplete before seed cleanup; refusing close_surface.", - ); - } - const seedUuidKey = cleanupSeedUuid.toLowerCase(); - const freshSeedRef = [ - ...cleanupTopology.surfaceRefById, - ].find( - ([surfaceUuid]) => - surfaceUuid.toLowerCase() === seedUuidKey, - )?.[1]; - if (!freshSeedRef) { - throw new Error( - `Seed UUID ${cleanupSeedUuid} is no longer uniquely bound; refusing close_surface.`, - ); - } - const cleanupWorkspace = - cleanupTopology.workspaceBySurface.get(freshSeedRef) ?? - workspace ?? - undefined; - await withSurfaceWrite( - freshSeedRef, - async () => { - const immediateTopology = - await collectSurfaceTopology(); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents close_surface", - ); - if (!topologyIsCoherent(immediateTopology)) { - throw new Error( - "Immediate topology is incomplete before seed cleanup; refusing close_surface.", - ); - } - const immediateSeedRef = [ - ...immediateTopology.surfaceRefById, - ].find( - ([surfaceUuid]) => - surfaceUuid.toLowerCase() === seedUuidKey, - )?.[1]; - if (immediateSeedRef !== freshSeedRef) { - throw new Error( - `Seed binding changed before close_surface (${freshSeedRef} -> ${immediateSeedRef ?? "missing"}); refusing to close a recycled ref.`, - ); - } - const immediateCleanupWorkspace = - immediateTopology.workspaceBySurface.get( - freshSeedRef, - ) ?? cleanupWorkspace; - await assertSurfaceMutationAllowed( - "close_surface", - freshSeedRef, - immediateCleanupWorkspace, - ); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents close_surface", - ); - await client.closeSurface(freshSeedRef, { - ...(immediateCleanupWorkspace - ? { workspace: immediateCleanupWorkspace } - : {}), - }); - assertSurfaceObserverEpochCurrent( - reflowObserverEpoch, - "resync_agents close_surface", - ); - }, - { - toolName: "close_surface", - workspace: cleanupWorkspace, - owner: `resync-reflow:close_surface:${agent.agent_id}`, - stableSurfaceIdentity: cleanupSeedUuid, - }, - ); - } catch (error) { - // A seed cleanup race is isolated to this worker as well. - recordReflowSkip( - agent, - seededSurface, - "close_surface", - error, - ); - } - } - } - } - } - - if (reflowed.length > 0) { - discovery.invalidate(); - after = await registry.listMerged(discovery, { force: true }); - } - const discovered = await discovery.scan(); - const afterIds = new Set(after.map((agent) => agent.agent_id)); - const managedSurfaceIds = new Set( - registry - .list() - .filter((agent) => !agent.agent_id.startsWith("auto-")) - .map((agent) => agent.surface_id), - ); - const orphanedSurfaces = discovered.filter( - (surface) => - !surface.read_error && - !managedSurfaceIds.has(surface.surface_id), - ); - const orphanedHealth = orphanedSurfaces.map( - buildOrphanSurfaceHealth, - ); - const evicted = [ - ...new Set([ - ...repair.evicted, - ...surfacelessEvicted, - ...[...beforeIds].filter((id) => !afterIds.has(id)), - ]), - ]; - const diff = { - added: [...afterIds].filter((id) => !beforeIds.has(id)), - evicted, - repaired: repair.repaired, - repair_skipped: repair.skipped, - reflowed, - reflow_skipped: reflowSkipped, - mismatches: after - .filter((agent) => agent.parsed_cli_mismatch) - .map((agent) => agent.agent_id), - orphaned: orphanedSurfaces.map((surface) => surface.surface_id), - orphaned_health: orphanedHealth, - health_failures: orphanedHealth.filter( - (health) => health.status === "unhealthy", - ), - }; - - return okFormatted(formatResync(diff), { - diff, - count: after.length, - }); - } catch (e) { - return err(e); - } - }); - /* c8 ignore stop */ - }, + // AIDEV-NOTE (#481): the original body was kept here behind an early + // return as an unreachable rollback reference, which made three + // capabilities look covered while their only producer/consumer sat in + // dead code. It is deleted; `liveSeatProof` and `parsed_cli_mismatch` + // now run on the live list_agents path, and orphan surfaces are already + // visible there as auto-discovered rows. + async () => + err( + new Error( + "resync_agents was removed; call list_agents for an automatically refreshed live view", + ), + ), ); // 16. stop_agent diff --git a/tests/f1-live-state-truth.test.ts b/tests/f1-live-state-truth.test.ts index c58febca..f9c36c9f 100644 --- a/tests/f1-live-state-truth.test.ts +++ b/tests/f1-live-state-truth.test.ts @@ -356,6 +356,73 @@ describe("F1 — live state, not the stale registry record", () => { ]); }); + it("#468: a terminal record from a prior observer cannot claim a recycled ref", async () => { + // `surface_id` is a RECYCLABLE ref. A dead worker's record whose ref was + // reused by a new pane used to win the last resolution tier and become the + // caller -- and the #378 guard then forced the new pane's children to + // worker/right off a corpse. The record's own CLI cannot arbitrate + // (`listMerged` rewrites it from the live pane), but `surface_observer_id` + // is not rewritten by the merge: a ref stamped by a dead socket generation + // says nothing about who is on that ref now. + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-corpse", + surface_id: client.idleSurface, + state: "done", + surface_observer_id: "cmux:/tmp/cmux-f1-previous-generation.sock", + }), + ); + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-childofcorpse", + surface_id: "surface:childofcorpse", + parent_agent_id: "cmuxlayerCodex-corpse", + }), + ); + + const parsed = await runWithCallerContext( + { workspaceId: "workspace:1", surfaceId: client.idleSurface }, + async () => parseResult(await callTool(server, "list_agents", { mine: true })), + ); + + // An explicit refusal, not a confident wrong answer. + expect(parsed.ok).toBe(false); + expect(JSON.stringify(parsed)).toContain("managed calling agent identity"); + }); + + it("#468: a terminal record this observer owns still resolves as the caller", async () => { + // Tier 4 exists because #408 flips live agents to `done`; the guard must + // narrow it to recycling-proof records, not delete it (that re-breaks U6). + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-ownedterminal", + surface_id: client.idleSurface, + state: "done", + }), + ); + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-childofowned", + surface_id: "surface:childofowned", + parent_agent_id: "cmuxlayerCodex-ownedterminal", + }), + ); + + const parsed = await runWithCallerContext( + { workspaceId: "workspace:1", surfaceId: client.idleSurface }, + async () => parseResult(await callTool(server, "list_agents", { mine: true })), + ); + + expect(parsed.ok, JSON.stringify(parsed)).toBe(true); + expect(parsed.agents.map((agent: any) => agent.agent_id)).toEqual([ + "cmuxlayerCodex-childofowned", + ]); + }); + it("P11 closure reads pending, not artifact_missing, on a screen-working agent", async () => { registerAgent( server, diff --git a/tests/resume-verification.test.ts b/tests/resume-verification.test.ts new file mode 100644 index 00000000..153a45ad --- /dev/null +++ b/tests/resume-verification.test.ts @@ -0,0 +1,149 @@ +/** + * Lane T1 — #482: `resumable` must be an observation, not a formatting result. + * + * Measured 2026-08-19: 13 rows advertised `resumable: true`; 2 of them (both + * LEAD seats) pointed at session files that exist nowhere on disk, while the + * live pane was writing a different session. Running those `resume_command`s + * restores nothing — at best it opens a fresh session wearing a lead's name. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdirSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + resolveResumeArtifact, + setResumeArtifactResolver, + resetResumeArtifactResolver, +} from "../src/resume-verification.js"; +import { + resumeInvocationForAgent, + toObservedPublicAgent, + toPublicAgent, +} from "../src/agent-facade.js"; +import type { AgentRecord } from "../src/agent-types.js"; + +const TEST_HOME = join(tmpdir(), "cmux-resume-verification-home"); +const PRESENT_SESSION = "b9e7f86f-f96c-43a3-a35b-e1ff0d3ef8a9"; +const MISSING_SESSION = "3c37f59c-6604-4892-9179-66a422102dbe"; + +function makeRecord(overrides: Partial = {}): AgentRecord { + return { + agent_id: "brainClaude", + surface_id: "surface:1", + state: "done", + repo: "brainlayer", + model: "claude", + cli: "claude", + cli_session_id: PRESENT_SESSION, + launcher_name: "brainlayerClaude", + task_summary: "t1", + pid: null, + version: 1, + created_at: "2026-08-19T10:00:00.000Z", + updated_at: "2026-08-19T10:00:00.000Z", + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "orchestrator", + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + ...overrides, + } as AgentRecord; +} + +describe("T1 #482 — resumable is verified against the session artifact", () => { + beforeEach(() => { + rmSync(TEST_HOME, { recursive: true, force: true }); + mkdirSync(join(TEST_HOME, ".claude", "projects", "-Users-x-brainlayer"), { + recursive: true, + }); + writeFileSync( + join( + TEST_HOME, + ".claude", + "projects", + "-Users-x-brainlayer", + `${PRESENT_SESSION}.jsonl`, + ), + "{}\n", + ); + setResumeArtifactResolver((cli, sessionId) => + resolveResumeArtifact(cli, sessionId, { home: TEST_HOME }), + ); + }); + + afterEach(() => { + resetResumeArtifactResolver(); + rmSync(TEST_HOME, { recursive: true, force: true }); + }); + + it("reports present / missing / unverifiable from the harness store", () => { + expect( + resolveResumeArtifact("claude", PRESENT_SESSION, { home: TEST_HOME }), + ).toBe("present"); + expect( + resolveResumeArtifact("claude", MISSING_SESSION, { home: TEST_HOME }), + ).toBe("missing"); + // No harness store on this machine at all: absence of evidence is not + // evidence of absence, so the claim stays unverified rather than false. + expect( + resolveResumeArtifact("claude", MISSING_SESSION, { + home: join(TEST_HOME, "no-such-home"), + }), + ).toBe("unverifiable"); + // gemini/kiro sessions are not stored anywhere cmuxlayer can read. + expect( + resolveResumeArtifact("gemini", PRESENT_SESSION, { home: TEST_HOME }), + ).toBe("unverifiable"); + }); + + it("refuses a resume invocation whose session file is not on disk", () => { + const invocation = resumeInvocationForAgent( + makeRecord({ cli_session_id: MISSING_SESSION }), + ); + expect(invocation.command).toBeNull(); + expect(invocation.reason).toMatch(/session/i); + expect(invocation.reason).toContain(MISSING_SESSION); + }); + + it("keeps advertising a resume whose session file exists", () => { + const invocation = resumeInvocationForAgent(makeRecord()); + expect(invocation.reason).toBeNull(); + expect(invocation.command).toBe( + `brainlayerClaude -s --resume ${PRESENT_SESSION}`, + ); + }); + + it("downgrades resumable to false with disk provenance in list_agents rows", () => { + const observed = toObservedPublicAgent( + makeRecord({ cli_session_id: MISSING_SESSION }), + ); + expect(observed.resumable.value).toBe(false); + expect(observed.resumable.source).toBe("disk"); + expect(observed.resume_command).toBeUndefined(); + + const publicAgent = toPublicAgent( + makeRecord({ cli_session_id: MISSING_SESSION }), + ); + expect(publicAgent.resumable).toBe(false); + expect(publicAgent.resume_command).toBeUndefined(); + }); + + it("marks a verified resume with disk provenance", () => { + const observed = toObservedPublicAgent(makeRecord()); + expect(observed.resumable.value).toBe(true); + expect(observed.resumable.source).toBe("disk"); + expect(observed.resume_command).toBe( + `brainlayerClaude -s --resume ${PRESENT_SESSION}`, + ); + }); + + it("does not downgrade a claim it cannot check", () => { + setResumeArtifactResolver(() => "unverifiable"); + const observed = toObservedPublicAgent(makeRecord()); + expect(observed.resumable.value).toBe(true); + // Provenance stays `registry`: nothing on disk confirmed this. + expect(observed.resumable.source).toBe("registry"); + }); +}); diff --git a/tests/server-agent-tools.test.ts b/tests/server-agent-tools.test.ts index 1a1f74af..664a0669 100644 --- a/tests/server-agent-tools.test.ts +++ b/tests/server-agent-tools.test.ts @@ -3048,6 +3048,10 @@ describe("agent lifecycle tool handlers", () => { cli: "claude", role: "orchestrator", task_done_detected_at: "2026-08-18T00:00:00Z", + // #468: a managed seat carries this observer's stamp -- spawn writes it. + // The ref-only caller tier now requires it, because a ref stamped by a + // dead generation (or never stamped) cannot prove who occupies it now. + surface_observer_id: "cmux:/tmp/cmuxlayer-test.sock", }); engine.stateMgr.writeState(staleLead); engine.getRegistry().set(staleLead.agent_id, staleLead); @@ -3091,6 +3095,8 @@ describe("agent lifecycle tool handlers", () => { cli: "codex", role: "worker", task_done_detected_at: "2026-08-18T00:00:00Z", + // #468: see the note on the stale-lead fixture above. + surface_observer_id: "cmux:/tmp/cmuxlayer-test.sock", }); engine.stateMgr.writeState(staleWorker); engine.getRegistry().set(staleWorker.agent_id, staleWorker); diff --git a/tests/t1-registry-truth.test.ts b/tests/t1-registry-truth.test.ts new file mode 100644 index 00000000..a23a4449 --- /dev/null +++ b/tests/t1-registry-truth.test.ts @@ -0,0 +1,457 @@ +/** + * Lane T1 — registry/state truth. + * + * #480: a row whose `surface_observer_id` is null or from a prior observer + * generation is structurally un-evictable today: `canMutateForObservedAbsence` + * requires an exact observer match and has no age escape hatch. Measured live + * on 2026-08-19: four such rows, the oldest 36 days, `list_agents` reporting 17 + * agents against 13 live surfaces. + * + * The rule these tests pin: observer ownership protects a row that a LIVE + * observer claims. It must not protect a row that no live surface bears — by + * uuid OR by ref — for a bounded, documented window. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + AgentRegistry, + SURFACE_EVICTION_CONFIRMATION_MS, + UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS, +} from "../src/agent-registry.js"; +import { createServer } from "../src/server.js"; +import { StateManager } from "../src/state-manager.js"; +import type { AgentRecord } from "../src/agent-types.js"; +import type { CmuxSurface } from "../src/types.js"; + +const TEST_DIR = join(tmpdir(), "cmux-agents-test-t1-registry-truth"); +const OBSERVER = "cmux:/tmp/cmux-t1-live.sock#socket=16777229"; +const DEAD_OBSERVER = "cmux:/tmp/cmux-t1-live.sock#socket=16777232"; + +function makeRecord(overrides: Partial = {}): AgentRecord { + return { + agent_id: "cmuxlayerClaude-t1", + surface_id: "surface:42", + surface_uuid: null, + surface_observer_id: OBSERVER, + state: "idle", + repo: "cmuxlayer", + model: "claude", + cli: "claude", + cli_session_id: null, + task_summary: "t1", + pid: null, + version: 1, + created_at: "2026-07-14T13:07:16.765Z", + updated_at: "2026-07-14T13:07:16.765Z", + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "orchestrator", + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + ...overrides, + } as AgentRecord; +} + +function makeSurface(ref: string, id?: string): CmuxSurface { + return { + ref, + title: `Agent on ${ref}`, + type: "terminal", + index: 0, + selected: false, + ...(id ? { id } : {}), + } as CmuxSurface; +} + +function makeRegistry( + stateMgr: StateManager, + surfaces: CmuxSurface[], +): AgentRegistry { + return new AgentRegistry(stateMgr, async () => surfaces, { + observerId: OBSERVER, + observerEpochProvider: () => `${OBSERVER}@epoch-1`, + }); +} + +/** Two ticks: the first records the absence, the second clears the window. */ +async function evictAcrossWindow( + registry: AgentRegistry, + opts: { elapsedMs: number; startedAt?: number } = { elapsedMs: 0 }, +): Promise { + const startedAt = opts.startedAt ?? 1_000_000; + await registry.evictSurfaceless({ + confirmationMs: 5_000, + now: startedAt, + }); + return registry.evictSurfaceless({ + confirmationMs: 5_000, + now: startedAt + opts.elapsedMs, + }); +} + +describe("T1 #480 — unclaimed rows are evictable within a bounded window", () => { + let stateMgr: StateManager; + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + stateMgr = new StateManager(TEST_DIR); + }); + + afterEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("evicts a null-observer legacy row whose ref no live surface bears", async () => { + // The exact shape measured on 2026-08-19: auto-claude-surface-603, no + // surface_uuid, no surface_observer_id, ref absent from a UUID-bearing + // topology. Immortal today on two counts (absence not "authoritative" for + // a UUID-less row, and the observer gate). + stateMgr.writeState( + makeRecord({ + agent_id: "auto-claude-surface-603", + surface_id: "surface:603", + surface_uuid: null, + surface_observer_id: null, + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS + 1, + }), + ).resolves.toEqual(["auto-claude-surface-603"]); + expect(registry.get("auto-claude-surface-603")).toBeNull(); + expect(stateMgr.readState("auto-claude-surface-603")).toBeNull(); + }); + + it("evicts a prior-generation observer row that is still `working`", async () => { + // orcClaude: state `working`, observer from a dead socket generation. It + // cannot be evicted, cannot be crash-marked, and therefore cannot be + // resumed either (resumeAgent requires a terminal state). + stateMgr.writeState( + makeRecord({ + agent_id: "orcClaude", + surface_id: "surface:478", + surface_uuid: "F317D8AB-ED5E-426D-9CE7-1D846666E532", + surface_observer_id: DEAD_OBSERVER, + state: "working", + seat_id: "orcClaude", + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS + 1, + }), + ).resolves.toEqual(["orcClaude"]); + }); + + it("does not evict an unclaimed row before the window closes", async () => { + stateMgr.writeState( + makeRecord({ + agent_id: "auto-claude-surface-606", + surface_id: "surface:606", + surface_observer_id: null, + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS - 1, + }), + ).resolves.toEqual([]); + expect(registry.get("auto-claude-surface-606")).not.toBeNull(); + }); + + it("keeps an unclaimed row whose ref is still live (recycled or not)", async () => { + // The ownership gate's real job: a foreign/legacy row on a ref a live + // surface still bears must never be evicted by this observer. Eviction is + // for rows NO live surface bears. + stateMgr.writeState( + makeRecord({ + agent_id: "legacy-on-live-ref", + surface_id: "surface:603", + surface_observer_id: null, + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:603", "BBBB-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS * 10, + }), + ).resolves.toEqual([]); + expect(registry.get("legacy-on-live-ref")).not.toBeNull(); + }); + + it("restarts the window when the row is observed live again", async () => { + stateMgr.writeState( + makeRecord({ + agent_id: "auto-claude-surface-618", + surface_id: "surface:618", + surface_observer_id: null, + }), + ); + let surfaces: CmuxSurface[] = [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]; + const registry = new AgentRegistry(stateMgr, async () => surfaces, { + observerId: OBSERVER, + observerEpochProvider: () => `${OBSERVER}@epoch-1`, + }); + await registry.reconstitute(); + + await registry.evictSurfaceless({ confirmationMs: 5_000, now: 1_000_000 }); + // The pane comes back mid-window: the absence clock must reset, not carry. + surfaces = [ + makeSurface("surface:700", "AAAA-live-uuid"), + makeSurface("surface:618", "CCCC-live-uuid"), + ]; + await registry.evictSurfaceless({ confirmationMs: 5_000, now: 1_010_000 }); + surfaces = [makeSurface("surface:700", "AAAA-live-uuid")]; + await expect( + registry.evictSurfaceless({ + confirmationMs: 5_000, + now: 1_010_000 + UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS, + }), + ).resolves.toEqual([]); + expect(registry.get("auto-claude-surface-618")).not.toBeNull(); + }); + + it("leaves rows this observer owns on the existing 5s confirmation path", async () => { + // The owned path must not silently inherit the longer unclaimed window. + stateMgr.writeState( + makeRecord({ + agent_id: "owned-ghost", + surface_id: "surface:owned", + surface_uuid: "DDDD-owned-uuid", + surface_observer_id: OBSERVER, + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { elapsedMs: 5_001 }), + ).resolves.toEqual(["owned-ghost"]); + }); +}); + +/** + * #481: `createLiveSeatDiscoveryProof` had exactly one call site in the repo — + * inside the removed `resync_agents` tool's unreachable body. Without a proof + * `hasLiveManagedSeatSibling` returns false unconditionally, so every + * crash-recovery-eligible ghost is retained forever, including the case the + * guard exists for: a live replacement already holding that row's seat. + * + * It also pins the other half of the recon finding: `list_agents` was the only + * caller that never evicted anything, which is why it reported 17 agents while + * `list_surfaces` reported 13. + */ +const SERVER_DIR = join(tmpdir(), "cmux-t1-list-agents-eviction"); +const SERVER_OBSERVER = "cmux:/tmp/cmux-t1-list-agents.sock"; + +class TwoSurfaceClient { + readonly workspace = "workspace:1"; + readonly screens: Record = { + "surface:lead": "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer\ncodex>", + }; + + async listWorkspaces() { + return { + workspaces: [ + { + ref: this.workspace, + title: "Main", + index: 0, + selected: true, + pinned: false, + }, + ], + }; + } + + async listPanes() { + return { + workspace_ref: this.workspace, + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:lead"], + selected_surface_ref: "surface:lead", + }, + ], + }; + } + + async listPaneSurfaces() { + return { + workspace_ref: this.workspace, + window_ref: "window:1", + pane_ref: "pane:1", + surfaces: [ + { + ref: "surface:lead", + id: "LEAD-UUID", + title: "cmuxlayerCodex-lead", + type: "terminal", + index: 0, + selected: true, + }, + ], + }; + } + + async readScreen(surface: string, opts?: { lines?: number }) { + const text = this.screens[surface]; + if (text == null) throw new Error(`Unknown surface: ${surface}`); + return { surface, text, lines: opts?.lines ?? 30, scrollback_used: false }; + } + + async send() {} + async sendKey() {} + async renameTab() {} +} + +describe("T1 #481 — the seat proof reaches a path callers actually use", () => { + let server: any; + + beforeEach(() => { + rmSync(SERVER_DIR, { recursive: true, force: true }); + mkdirSync(SERVER_DIR, { recursive: true }); + server = createServer({ + client: new TwoSurfaceClient() as any, + stateDir: SERVER_DIR, + disableSpawnPreflight: true, + surfaceObserverOwnerIdProvider: () => SERVER_OBSERVER, + surfaceObserverEpochProvider: () => `${SERVER_OBSERVER}@test`, + } as any); + }); + + afterEach(() => { + const engine = server?._registeredTools?.interact?._engine; + if (engine && typeof engine.dispose === "function") engine.dispose(); + rmSync(SERVER_DIR, { recursive: true, force: true }); + }); + + it("list_agents evicts with an observer-pinned live seat proof", async () => { + const engine = server._registeredTools["interact"]._engine; + const registry = engine.getRegistry(); + const evictSpy = vi.spyOn(registry, "evictSurfaceless"); + + const tool = server._registeredTools["list_agents"]; + await tool.handler({}, {} as any); + + expect(evictSpy).toHaveBeenCalled(); + const opts = evictSpy.mock.calls.at(-1)?.[0] as any; + expect(opts?.confirmationMs).toBe(SURFACE_EVICTION_CONFIRMATION_MS); + expect(opts?.liveSeatProof).not.toBeNull(); + expect(opts?.liveSeatProof?.observer_id).toBe(SERVER_OBSERVER); + expect(opts?.liveSeatProof?.observer_epoch).toBe( + `${SERVER_OBSERVER}@test`, + ); + }); +}); + +describe("T1 #481 — parsed_cli_mismatch reaches a reader again", () => { + let server: any; + + beforeEach(() => { + rmSync(SERVER_DIR, { recursive: true, force: true }); + mkdirSync(SERVER_DIR, { recursive: true }); + }); + + afterEach(() => { + const engine = server?._registeredTools?.interact?._engine; + if (engine && typeof engine.dispose === "function") engine.dispose(); + rmSync(SERVER_DIR, { recursive: true, force: true }); + }); + + it("reports a record whose live pane runs a different CLI, and stays silent otherwise", async () => { + // `parsed_cli_mismatch` was computed on every listMerged and read by + // exactly one call site: the removed resync tool's dead body. A pane whose + // observed CLI disagrees with its record was silently un-surfaced. + const client = new TwoSurfaceClient(); + client.screens["surface:lead"] = [ + "✻ Welcome to Claude Code", + "bypass permissions on", + "> ", + ].join("\n"); + server = createServer({ + client: client as any, + stateDir: SERVER_DIR, + disableSpawnPreflight: true, + surfaceObserverOwnerIdProvider: () => SERVER_OBSERVER, + surfaceObserverEpochProvider: () => `${SERVER_OBSERVER}@test`, + } as any); + const engine = server._registeredTools["interact"]._engine; + const record = { + agent_id: "cmuxlayerCodex-lead", + surface_id: "surface:lead", + surface_uuid: "LEAD-UUID", + surface_observer_id: SERVER_OBSERVER, + workspace_id: "workspace:1", + state: "idle", + repo: "cmuxlayer", + model: "gpt-5.5", + cli: "codex", + cli_session_id: null, + task_summary: "mismatch fixture", + pid: null, + version: 1, + created_at: "2026-08-19T10:00:00.000Z", + updated_at: "2026-08-19T10:00:00.000Z", + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "orchestrator", + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + } as unknown as AgentRecord; + engine.stateMgr.writeState(record); + engine.getRegistry().set(record.agent_id, record); + + const result = await server._registeredTools["list_agents"].handler( + {}, + {} as any, + ); + const parsed = result.structuredContent ?? JSON.parse(result.content[0].text); + const row = parsed.agents.find( + (agent: any) => agent.agent_id === "cmuxlayerCodex-lead", + ); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + expect(row.parsed_cli_mismatch).toBe(true); + // Agreement costs no payload: the field is absent, not `false`. + for (const other of parsed.agents) { + if (other.agent_id === "cmuxlayerCodex-lead") continue; + expect(other).not.toHaveProperty("parsed_cli_mismatch"); + } + }); +}); diff --git a/tests/vitest.setup.ts b/tests/vitest.setup.ts index b72bbac7..de4b7980 100644 --- a/tests/vitest.setup.ts +++ b/tests/vitest.setup.ts @@ -1,3 +1,12 @@ +import { setResumeArtifactResolver } from "../src/resume-verification.js"; + // The release script bumps package.json before its pre-push Vitest rerun. // Keep unit tests from comparing that temporary version with the host brew tree. process.env.CMUXLAYER_DEV = "1"; + +// #482: `resumable` is now an observation of the harness session store. The +// suite must never read the developer's real ~/.claude to decide it, so the +// default here is the honest "I did not look" answer — which is exactly the +// pre-#482 behaviour. Tests that exercise verification install their own +// resolver (see tests/resume-verification.test.ts). +setResumeArtifactResolver(() => "unverifiable"); From 64a16cf3701a6d9516dcfc4534b22818e1f2c9d8 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 22:37:46 +0300 Subject: [PATCH 2/2] =?UTF-8?q?fix(t1):=20land=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20resume-safe=20eviction,=20residual=20claims,=20outc?= =?UTF-8?q?ome=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- README.md | 3 +- docs/control-plane-invariants.md | 8 +- src/agent-registry.ts | 71 ++++++----- src/server.ts | 5 +- tests/t1-registry-truth.test.ts | 200 +++++++++++++++++++++++++++---- 5 files changed, 230 insertions(+), 57 deletions(-) diff --git a/README.md b/README.md index 1fc52822..937aac97 100644 --- a/README.md +++ b/README.md @@ -136,7 +136,7 @@ The other 30 definitions, including `interact`, are interim ToolSearch-deferred **Terminal control** — `list_surfaces` `control_health` `select_workspace` `create_workspace` `delete_workspace` `new_split` `new_surface` `move_surface` `send_input` `send_command` `send_key` `read_screen` `rename_tab` `close_surface` `browser_surface` -**Agent lifecycle** — `spawn_agent` `new_worktree_split` `spawn_in_workspace` `resync_agents` `send_to` `send_to_agent` `wait_for` `wait_for_all` `interact` `stop_agent` `kill` `supersede_agent_goal` `broadcast` +**Agent lifecycle** — `spawn_agent` `new_worktree_split` `spawn_in_workspace` `send_to` `send_to_agent` `wait_for` `wait_for_all` `interact` `stop_agent` `kill` `supersede_agent_goal` `broadcast` **Metacomm (agent inbox)** — `dispatch_to_agent` `inbox_check` @@ -183,7 +183,6 @@ The other 30 definitions, including `interact`, are interim ToolSearch-deferred | `spawn_agent` | Spawn a CLI agent and return an `agent_id` for routing | | `new_worktree_split` | Deprecated one-release alias; use `spawn_agent(worktree:true, placement:"worker")` | | `spawn_in_workspace` | Deprecated one-release alias; create/reuse a workspace and call `spawn_agent` for each managed agent | -| `resync_agents` | Re-sync the agent registry from live surfaces | | `dispatch_to_agent` | Append a task to an agent's inbox file (deterministic write channel) | | `send_to` | Send by agent ID or raw surface using `mode:"agent"|"surface"|"command"|"key"` | | `send_to_agent` | Deprecated one-release alias for `send_to(mode:"agent")` | diff --git a/docs/control-plane-invariants.md b/docs/control-plane-invariants.md index cd990022..9445455c 100644 --- a/docs/control-plane-invariants.md +++ b/docs/control-plane-invariants.md @@ -46,12 +46,18 @@ measured 36-day ghosts violated. | Row | Window | Constant | Path | | --- | --- | --- | --- | | `surface_observer_id` equals the current observer | 5 s of continuous absence | `SURFACE_EVICTION_CONFIRMATION_MS` (`src/agent-registry.ts`) | sweep + `list_agents` | -| `surface_observer_id` is null or from a prior observer generation | 60 s of continuous absence, where absence means no live surface bears the row's UUID **or** its ref | `UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS` (`src/agent-registry.ts`) | sweep + `list_agents` | +| `surface_observer_id` is null or from a prior observer generation | 60 s of continuous absence, where absence means no live surface bears the row's identity key — its UUID when it has one, else its ref | `UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS` (`src/agent-registry.ts`) | sweep + `list_agents` | Ownership stops one observer mutating another's *live* row; it is not a claim on a row that no live surface bears. An unclaimed row is evicted, never crash-marked: eviction is the reversible direction, because `listMerged` re-mints a row from discovery if the pane turns out to be alive. +One exception, and it is deliberate: a row whose captured `cli_session_id` still resolves to a +session artifact on disk is retained regardless of the window. `resumeAgent` has no ownership gate, +so that row is the record resume-by-ID acts on, and the registry is the only `agent_id` → +`cli_session_id` mapping. `missing` and session-less rows still evict, so the ghost class #480 was +filed about still closes. + ## Allowed Transitions Allowed transitions are intentionally narrower than current ad hoc state movement: diff --git a/src/agent-registry.ts b/src/agent-registry.ts index e3c2115d..9db1b9a4 100644 --- a/src/agent-registry.ts +++ b/src/agent-registry.ts @@ -36,6 +36,7 @@ import { import { validateSurfaceIdentityBijection } from "./surface-topology.js"; import { deriveCmuxObserverOwnerId } from "./cmux-observer-identity.js"; import { inferRepoFromDirectory } from "./repo-workspace.js"; +import { resumeArtifactStatus } from "./resume-verification.js"; export type SurfaceProvider = () => Promise; @@ -61,11 +62,6 @@ interface SurfacelessEvictionOptions extends SurfaceAbsenceOptions { * still owns the shell, so crash-recovery rows may only yield to this proof. */ liveSeatProof?: LiveSeatDiscoveryProof | null; - /** - * Continuous-absence window before a row that no live observer claims is - * dropped (#480). Defaults to `UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS`. - */ - unclaimedConfirmationMs?: number; } export interface AgentRegistryOptions { @@ -120,8 +116,8 @@ export const SURFACE_EVICTION_CONFIRMATION_MS = 5_000; * four such rows, oldest 36 days, `list_agents` 17 vs `list_surfaces` 13. * * Ownership exists to stop one observer mutating another's LIVE row. It is not - * a claim on a row that no live surface bears — by uuid or by ref — across a - * continuous absence window. This constant is that window: twelve consecutive + * a claim on a row that no live surface bears — matched on the row's UUID when + * it has one, else on its ref — across a continuous absence window. This constant is that window: twelve consecutive * 5 s sweeps of proven absence before an unclaimed row is dropped, so the * worst case is bounded and documented rather than infinite. */ @@ -1757,9 +1753,20 @@ export class AgentRegistry { // (`recoverCrashedAgents` quarantines unowned rows) on any other path, // so without this they live forever. if (!this.canMutateForObservedAbsence(agent, observerSnapshot.ownerId)) { - if ( - !this.isUnclaimedAbsenceConfirmed(agent, liveSurfaceKeys, opts) - ) { + // The row is the only `agent_id` -> `cli_session_id` mapping, and + // `resumeAgent` is the one recovery path with no ownership gate. So a + // row whose captured session is still on disk is not a ghost: it is + // the record resume-by-ID acts on, and a successful resume re-stamps + // it with this observer. Deleting it would strand a live transcript. + // Only a PRESENT artifact retains: `missing` restores nothing, and + // `unverifiable` (no store on this machine) must not make eviction + // depend on a directory's existence -- that would reopen #480 wherever + // the harness store is absent. + if (this.hasVerifiedResumeArtifact(agent)) { + this.unclaimedAbsenceObservations.delete(agent.agent_id); + continue; + } + if (!this.isUnclaimedAbsenceConfirmed(agent, opts)) { continue; } const removedUnclaimedId = this.evictUnchecked(id); @@ -1933,14 +1940,27 @@ export class AgentRegistry { } /** - * Continuous, ref-AND-uuid absence of a row that this observer does not own - * (#480). Deliberately does NOT consult `isSurfaceAbsenceAuthoritative`: - * that helper refuses to read a UUID-less row's absence in a UUID-bearing - * topology because a live occupant on the same mutable ref proves nothing - * about the row. Here the ref is not occupied at all -- no live surface - * carries the row's uuid OR its ref -- across a coherent, non-empty scan. - * That is real absence evidence, and it is the only evidence an unclaimed - * row can ever produce. + * #480/#482: a captured session this machine can still see on disk — the + * one thing an unclaimed row still protects, since `resumeAgent` has no + * ownership gate and the registry holds the only agent_id -> session map. + */ + private hasVerifiedResumeArtifact(agent: AgentRecord): boolean { + if (!agent.cli_session_id) return false; + return resumeArtifactStatus(agent.cli, agent.cli_session_id) === "present"; + } + + /** + * Continuous absence of a row that this observer does not own (#480). + * + * The row's identity is ONE key: its UUID when it has one, else its ref + * (`agentSurfaceKey`). Absence means the caller's `matchingLiveSurface` + * found nothing for that key in a coherent, non-empty scan. + * + * Deliberately does NOT consult `isSurfaceAbsenceAuthoritative`: that helper + * refuses to read a UUID-less row's absence in a UUID-bearing topology, + * because a live occupant sitting ON the same mutable ref proves nothing + * about the row. Here the ref is not occupied at all. That is real absence + * evidence, and it is the only evidence an unclaimed row can ever produce. * * Eviction, not crash-marking: dropping the registry row is the reversible * direction. If the pane were somehow alive, `listMerged` re-mints it from @@ -1949,21 +1969,10 @@ export class AgentRegistry { */ private isUnclaimedAbsenceConfirmed( agent: AgentRecord, - liveSurfaceKeys: ReadonlySet, - opts: { unclaimedConfirmationMs?: number; now?: number }, + opts: { now?: number }, ): boolean { const surfaceKey = this.agentSurfaceKey(agent); - if (liveSurfaceKeys.has(surfaceKey)) { - this.unclaimedAbsenceObservations.delete(agent.agent_id); - return false; - } - const confirmationMs = Math.max( - 0, - opts.unclaimedConfirmationMs ?? UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS, - ); - if (confirmationMs === 0) { - return true; - } + const confirmationMs = UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS; const now = opts.now ?? Date.now(); const observation = this.unclaimedAbsenceObservations.get(agent.agent_id); if (!observation || observation.surfaceId !== surfaceKey) { diff --git a/src/server.ts b/src/server.ts index 157cbd5e..769b10a6 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10916,7 +10916,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { throw new Error( `Agent "${args.agent_id}" no longer maps to a live surface ` + `(stale surface ref); its pane likely closed or was recycled. ` + - `Run resync_agents and retry.`, + `Call list_agents for a refreshed live view and retry.`, ); } route = reresolved; @@ -10971,7 +10971,8 @@ export function createServer(opts?: CreateServerOptions): McpServer { throw new Error( `Agent "${args.agent_id}" (${expectedCli}) no longer occupies ` + `surface ${route.surface_id} — it now hosts a ${freshOccupant?.cli} ` + - `agent (surface recycled). Run resync_agents and retry.`, + `agent (surface recycled). Call list_agents for a refreshed ` + + `live view and retry.`, ); } } diff --git a/tests/t1-registry-truth.test.ts b/tests/t1-registry-truth.test.ts index a23a4449..6aebc4e2 100644 --- a/tests/t1-registry-truth.test.ts +++ b/tests/t1-registry-truth.test.ts @@ -8,11 +8,13 @@ * agents against 13 live surfaces. * * The rule these tests pin: observer ownership protects a row that a LIVE - * observer claims. It must not protect a row that no live surface bears — by - * uuid OR by ref — for a bounded, documented window. + * observer claims. It must not protect a row that no live surface bears on its + * identity key (its UUID when it has one, else its ref) for a bounded, + * documented window — unless the row still carries a session artifact that + * resume-by-ID can act on. */ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; -import { mkdirSync, rmSync } from "node:fs"; +import { mkdirSync, readFileSync, rmSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { @@ -22,6 +24,7 @@ import { } from "../src/agent-registry.js"; import { createServer } from "../src/server.js"; import { StateManager } from "../src/state-manager.js"; +import { setResumeArtifactResolver } from "../src/resume-verification.js"; import type { AgentRecord } from "../src/agent-types.js"; import type { CmuxSurface } from "../src/types.js"; @@ -103,6 +106,8 @@ describe("T1 #480 — unclaimed rows are evictable within a bounded window", () }); afterEach(() => { + // Restore the suite-wide stub from tests/vitest.setup.ts. + setResumeArtifactResolver(() => "unverifiable"); rmSync(TEST_DIR, { recursive: true, force: true }); }); @@ -184,6 +189,13 @@ describe("T1 #480 — unclaimed rows are evictable within a bounded window", () // The ownership gate's real job: a foreign/legacy row on a ref a live // surface still bears must never be evicted by this observer. Eviction is // for rows NO live surface bears. + // + // Belt-and-braces on purpose: the property is upheld UPSTREAM of the code + // this lane added — `matchingLiveSurface` catches the row at the top of + // the loop, and `clearSurfacelessObservationsForLiveSurfaces` wipes its + // absence clock every tick. This case therefore passes against `main` + // too. It is here because "never evict a live row" is the property most + // worth pinning, not because it isolates the new branch. stateMgr.writeState( makeRecord({ agent_id: "legacy-on-live-ref", @@ -238,6 +250,64 @@ describe("T1 #480 — unclaimed rows are evictable within a bounded window", () expect(registry.get("auto-claude-surface-618")).not.toBeNull(); }); + it("keeps an unclaimed row whose captured session is still on disk", async () => { + // The row is the ONLY agent_id -> cli_session_id mapping, and + // `resumeAgent` has no ownership gate: an unclaimed row with a live + // session artifact is the one thing resume-by-ID can still act on + // (AGENTS.md: "a worker got killed because its pane broke"). Evicting it + // would delete the mapping and strand the transcript. Retention here is + // not the old immortality: the row is retained because it is usable, and + // a successful resume re-stamps it with the current observer. + setResumeArtifactResolver(() => "present"); + stateMgr.writeState( + makeRecord({ + agent_id: "killed-but-resumable", + surface_id: "surface:gone", + surface_uuid: "GONE-UUID", + surface_observer_id: DEAD_OBSERVER, + state: "done", + cli_session_id: "5f1d0c6a-1f2b-4a3c-8d4e-9f0a1b2c3d4e", + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS * 10, + }), + ).resolves.toEqual([]); + expect(registry.get("killed-but-resumable")).not.toBeNull(); + }); + + it("evicts an unclaimed row whose captured session is gone from disk", async () => { + // The counter-case: a session id that resolves to nothing restores + // nothing, so the row protects no capability and #480 still closes. + setResumeArtifactResolver(() => "missing"); + stateMgr.writeState( + makeRecord({ + agent_id: "killed-and-unrecoverable", + surface_id: "surface:gone", + surface_uuid: "GONE-UUID", + surface_observer_id: DEAD_OBSERVER, + state: "done", + cli_session_id: "5f1d0c6a-1f2b-4a3c-8d4e-9f0a1b2c3d4e", + }), + ); + const registry = makeRegistry(stateMgr, [ + makeSurface("surface:700", "AAAA-live-uuid"), + ]); + await registry.reconstitute(); + + await expect( + evictAcrossWindow(registry, { + elapsedMs: UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS + 1, + }), + ).resolves.toEqual(["killed-and-unrecoverable"]); + }); + it("leaves rows this observer owns on the existing 5s confirmation path", async () => { // The owned path must not silently inherit the longer unclaimed window. stateMgr.writeState( @@ -270,14 +340,36 @@ describe("T1 #480 — unclaimed rows are evictable within a bounded window", () * caller that never evicted anything, which is why it reported 17 agents while * `list_surfaces` reported 13. */ +const SEAT_REGISTRY = { + cmuxlayerClaude: { + repo: "cmuxlayer", + launchers: { claude: "cmuxlayerClaude" }, + lane: "cmuxlayer", + role: "lead", + }, +} as const; + const SERVER_DIR = join(tmpdir(), "cmux-t1-list-agents-eviction"); const SERVER_OBSERVER = "cmux:/tmp/cmux-t1-list-agents.sock"; +const CLAUDE_SCREEN = [ + "✻ Welcome to Claude Code", + "bypass permissions on", + "> ", +].join("\n"); + class TwoSurfaceClient { readonly workspace = "workspace:1"; - readonly screens: Record = { - "surface:lead": "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer\ncodex>", - }; + readonly title: string; + readonly screens: Record; + + constructor(opts: { title?: string; screen?: string } = {}) { + this.title = opts.title ?? "cmuxlayerCodex-lead"; + this.screens = { + "surface:lead": + opts.screen ?? "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer\ncodex>", + }; + } async listWorkspaces() { return { @@ -319,7 +411,7 @@ class TwoSurfaceClient { { ref: "surface:lead", id: "LEAD-UUID", - title: "cmuxlayerCodex-lead", + title: this.title, type: "terminal", index: 0, selected: true, @@ -360,22 +452,73 @@ describe("T1 #481 — the seat proof reaches a path callers actually use", () => rmSync(SERVER_DIR, { recursive: true, force: true }); }); - it("list_agents evicts with an observer-pinned live seat proof", async () => { + it("list_agents evicts a crash-recovery ghost whose seat a live pane holds", async () => { + // Outcome, not wiring: `hasLiveManagedSeatSibling` returns false for every + // row unless it is handed a proof built from THIS cycle's scan, so a + // crash-recovery-eligible ghost is retained forever without one. Asserting + // the ghost is gone fails both ways a regression can happen -- the call + // removed, and a proof built from the wrong (or empty) scan. + server = createServer({ + client: new TwoSurfaceClient({ + title: "cmuxlayerClaude", + screen: CLAUDE_SCREEN, + }) as any, + stateDir: SERVER_DIR, + disableSpawnPreflight: true, + seatRegistry: SEAT_REGISTRY, + surfaceObserverOwnerIdProvider: () => SERVER_OBSERVER, + surfaceObserverEpochProvider: () => `${SERVER_OBSERVER}@test`, + } as any); const engine = server._registeredTools["interact"]._engine; + const seatFields = { + repo: "cmuxlayer", + cli: "claude", + launcher_name: "cmuxlayerClaude", + seat_id: "cmuxlayerClaude", + role: "orchestrator", + surface_observer_id: SERVER_OBSERVER, + workspace_id: "workspace:1", + }; + for (const record of [ + makeRecord({ + ...seatFields, + agent_id: "cmuxlayerClaude-live", + state: "working", + surface_id: "surface:lead", + surface_uuid: "LEAD-UUID", + }), + makeRecord({ + ...seatFields, + agent_id: "cmuxlayerClaude-ghost", + state: "error", + surface_id: "surface:gone", + surface_uuid: "GONE-UUID", + crash_recover: true, + cli_session_id: "5f1d0c6a-1f2b-4a3c-8d4e-9f0a1b2c3d4e", + error: "Surface surface:gone disappeared", + }), + ]) { + engine.stateMgr.writeState(record); + engine.getRegistry().set(record.agent_id, record); + } + + const nowSpy = vi.spyOn(Date, "now"); + try { + // First call observes the absence; the 5 s confirmation window is the + // owned path's, unchanged by this lane. + nowSpy.mockReturnValue(1_000_000); + await server._registeredTools["list_agents"].handler({}, {} as any); + nowSpy.mockReturnValue(1_006_000); + await server._registeredTools["list_agents"].handler({}, {} as any); + } finally { + nowSpy.mockRestore(); + } + const registry = engine.getRegistry(); - const evictSpy = vi.spyOn(registry, "evictSurfaceless"); - - const tool = server._registeredTools["list_agents"]; - await tool.handler({}, {} as any); - - expect(evictSpy).toHaveBeenCalled(); - const opts = evictSpy.mock.calls.at(-1)?.[0] as any; - expect(opts?.confirmationMs).toBe(SURFACE_EVICTION_CONFIRMATION_MS); - expect(opts?.liveSeatProof).not.toBeNull(); - expect(opts?.liveSeatProof?.observer_id).toBe(SERVER_OBSERVER); - expect(opts?.liveSeatProof?.observer_epoch).toBe( - `${SERVER_OBSERVER}@test`, - ); + expect(registry.get("cmuxlayerClaude-ghost")).toBeNull(); + expect(engine.stateMgr.readState("cmuxlayerClaude-ghost")).toBeNull(); + // The live seat itself must survive: the proof identifies it, it is not a ghost. + expect(registry.get("cmuxlayerClaude-live")).not.toBeNull(); }); }); @@ -455,3 +598,18 @@ describe("T1 #481 — parsed_cli_mismatch reaches a reader again", () => { } }); }); + +/** + * #481/#477 class: a removal that leaves its instructions behind is not a + * removal. `resync_agents` errors unconditionally, so any surface that still + * tells a caller to run it hands them a second error. + */ +describe("T1 #481 — nothing still instructs callers to run resync_agents", () => { + const read = (relative: string) => + readFileSync(new URL(`../${relative}`, import.meta.url), "utf8"); + + it("keeps the removed tool out of runtime guidance and the README", () => { + expect(read("src/server.ts")).not.toContain("Run resync_agents"); + expect(read("README.md")).not.toContain("resync_agents"); + }); +});