From 3ef2d4181f6827060b8437a53d1a8924840c7dde Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 17:23:33 +0300 Subject: [PATCH 1/5] fix(f1b): wait_for and watch resolve from live state, not the raw record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit F1 (#466) converted callers, delivery and closure to `resolveLiveAgentState` and left the two paths a lead actually monitors with reading the registry record raw. #473 — `wait_for`'s terminal short-circuits read `registry.get()` directly, so a #408-poisoned `done` returned `{state:"done", error:"Agent has already completed", elapsed:0}` for an agent mid-`brew install`, while the same response's own health block said `reconciled_state:"working"`. Every termination decision in `waitFor` now reads the live-resolved state — the entry short-circuits, the retroactive evidence gate, the sweep's fail-fast, and the timeout report — and the top-level `state` carries the reconciled value. Gating only the entry would have moved the false completion one poll later, so the sweep is gated with it. With no live probe wired the resolution IS the record, so an unprobed engine is unchanged. #472 — `watchAgentObservation` answered "does this agent exist?" with one in-memory registry lookup AND a successful screen parse, so a transient read failure, an unreconstituted record, or a booting pane all became `exists:false` and a hard `WatchArmError` saying the agent does not exist — for an agent `send_to` delivered to and verified in the same second. The record now decides existence (registry, then the state dir), the screen only refines what the agent is doing, a read failure is retried once and reported as a read failure, and a booting or unparseable frame arms and lets the predicate resolve. Only positive evidence the surface is gone (dead, evicted, bare shell) returns `exists:false`, and the refusal names what was observed instead of asserting absence. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent-engine.ts | 171 +++++++++-- src/watch-spec.ts | 12 +- tests/f1b-wait-for-watch-live-state.test.ts | 314 ++++++++++++++++++++ 3 files changed, 467 insertions(+), 30 deletions(-) create mode 100644 tests/f1b-wait-for-watch-live-state.test.ts diff --git a/src/agent-engine.ts b/src/agent-engine.ts index 59cf8946..dbbce2f8 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -708,6 +708,8 @@ export type AgentLifecycleEvent = "spawned" | "done" | "errored" | "health"; const INTERACTIVE_STATES = new Set(["ready", "idle"]); const TERMINAL_STATES = new Set(["done", "error"]); const WAIT_FOR_SWEEP_INTERVAL_MS = 1000; +/** One retry: a watch observation must not read a transient failure as absence. */ +const WATCH_OBSERVATION_READ_ATTEMPTS = 2; const DEFAULT_SWEEP_ACTIVE_INTERVAL_MS = 5_000; const DEFAULT_SWEEP_IDLE_INTERVAL_MS = 15_000; const DEFAULT_SWEEP_IDLE_AFTER_SWEEPS = 3; @@ -2360,11 +2362,18 @@ export class AgentEngine { ); } + /** + * AIDEV-NOTE (F1b): `effectiveState` is the LIVE-resolved state when the + * caller has one. `wait_for` passes it so a record the screen contradicts + * can never be read as evidence the target state was reached; every other + * caller keeps the record's own value and behaves exactly as before. + */ private async getTargetStateEvidenceSource( agent: AgentRecord, targetState: AgentState, + effectiveState: AgentState = agent.state, ): Promise { - if (agent.state !== targetState) return null; + if (effectiveState !== targetState) return null; if (!this.requiresOutputDoneEvidence(targetState)) return "state"; if (await this.hasGroundTruthDone(agent)) return "transcript"; return this.hasRecordedOutputDoneEvidence(agent) || @@ -7354,29 +7363,118 @@ export class AgentEngine { } } + /** The on-disk record for an agent the in-memory registry has not bound. */ + private readPersistedAgentRecord(agentId: string): AgentRecord | null { + try { + return this.stateMgr.readState(agentId); + } catch { + return null; + } + } + + /** + * AIDEV-NOTE (F1b, #472): watch-target existence, and NOTHING more. + * + * This resolver used to answer "does this agent exist?" with "did one + * registry lookup hit AND did one screen read parse into a known CLI?", so a + * transient read failure, an unreconstituted record, or a pane still booting + * all collapsed into `exists:false` -> a hard `WatchArmError` saying the + * agent does not exist. Live, that denied a watch on `voicelayerClaude-2ac0d960` + * in the same second `send_to` delivered to it and verified submission. + * + * So: the record decides existence (registry first, then the state dir that + * `send_to` also resolves from), and the screen only refines what the agent + * is DOING. A read failure is reported as a read failure and retried once -- + * it is not evidence of absence. A booting or unparseable frame is a legal + * watch target: it arms, and the predicate resolves on a later sweep. Only + * positive evidence that the surface is gone -- dead, evicted, or fallen back + * to a bare shell -- returns `exists:false`, and it says which one. + */ private watchAgentObservation = async ( agentId: string, ): Promise => { - const agent = this.registry.get(agentId); + const agent = + this.registry.get(agentId) ?? this.readPersistedAgentRecord(agentId); const source = `screen:${agent?.surface_uuid ?? agent?.surface_id ?? agentId}`; - if (!agent) return { exists: false, state: null, source }; - try { - const screen = await this.client.readScreen(agent.surface_id, { - ...(agent.workspace_id ? { workspace: agent.workspace_id } : {}), - lines: 30, - }); - const parsed = parseScreen(cleanScreenText(screen.text)); + if (!agent) { return { - exists: - parsed.agent_type !== "unknown" && - parsed.control_state !== "dead" && - parsed.control_state !== "stale_surface", - state: parsed.status === "frozen" ? "error" : parsed.status, + exists: false, + state: null, source, + detail: `no registry or state record for ${agentId}`, }; - } catch { - return { exists: false, state: null, source }; } + + let screenText: string | null = null; + let readError: unknown = null; + for (let attempt = 0; attempt < WATCH_OBSERVATION_READ_ATTEMPTS; attempt++) { + try { + const screen = await this.client.readScreen(agent.surface_id, { + ...(agent.workspace_id ? { workspace: agent.workspace_id } : {}), + lines: 30, + }); + screenText = screen.text; + readError = null; + break; + } catch (error) { + readError = error; + } + } + + if (screenText === null) { + // A record we can read, on a surface we momentarily cannot. That is a + // read failure, not a missing agent -- report the record's own state. + return { + exists: true, + state: resolveLiveAgentState(agent, null).state, + source, + detail: `registry hit, screen unreadable after ${WATCH_OBSERVATION_READ_ATTEMPTS} attempts: ${ + readError instanceof Error ? readError.message : String(readError) + }`, + }; + } + + const parsed = parseScreen(cleanScreenText(screenText)); + if ( + parsed.control_state === "dead" || + parsed.control_state === "stale_surface" + ) { + return { + exists: false, + state: null, + source, + detail: `registry hit, screen shows ${parsed.control_state}`, + }; + } + const live = resolveLiveAgentState(agent, { + status: parsed.status, + agent_type: parsed.agent_type, + control_state: parsed.control_state, + }); + if (parsed.control_state === "shell" && parsed.agent_type === "unknown") { + return { + exists: false, + state: null, + source, + detail: "registry hit, surface fell back to a bare shell", + }; + } + if (parsed.agent_type === "unknown") { + // Mid-boot or an unparseable frame: the screen has no authority over the + // status here, and reading its default as an idle prompt would fire an + // `idle` predicate on an agent that has not started yet. + return { + exists: true, + state: live.state, + source, + detail: "registry hit, screen unparseable", + }; + } + return { + exists: true, + state: parsed.status === "frozen" ? "error" : parsed.status, + source, + }; }; private async sweepWatchesBestEffort(): Promise { @@ -8130,15 +8228,27 @@ export class AgentEngine { throw new Error(`Agent not found: ${agentId}`); } + // AIDEV-NOTE (F1b, #473): a wait must never terminate on a record state the + // screen contradicts. #408 flips live agents to `done` within minutes, and + // these short-circuits read that record raw -- so `wait_for(target:"idle")` + // returned `{state:"done", error:"Agent has already completed", elapsed:0}` + // for an agent mid-`brew install`, and the lead that trusted it reported a + // false completion. Every termination decision below reads the LIVE state + // instead, and the top-level `state` reports the reconciled value rather + // than the poisoned record. With no live probe wired the resolution IS the + // record, so an unprobed engine behaves exactly as it did before. + const initialLive = this.liveStateOf(initial); + // Retroactive check — already in target state with required evidence? const initialEvidence = await this.getTargetStateEvidenceSource( initial, targetState, + initialLive.state, ); if (initialEvidence) { return { matched: true, - state: initial.state, + state: initialLive.state, elapsed: Date.now() - start, source: initialEvidence === "state" ? "immediate" : initialEvidence, agent: toPublicAgent(initial), @@ -8146,10 +8256,10 @@ export class AgentEngine { } // Already in terminal error state and target isn't error? - if (initial.state === "error" && targetState !== "error") { + if (initialLive.state === "error" && targetState !== "error") { return { matched: false, - state: initial.state, + state: initialLive.state, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8158,10 +8268,10 @@ export class AgentEngine { } // Already in terminal done state and target isn't done? - if (initial.state === "done" && targetState !== "done") { + if (initialLive.state === "done" && targetState !== "done") { return { matched: false, - state: initial.state, + state: initialLive.state, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8185,7 +8295,7 @@ export class AgentEngine { const current = this.registry.get(agentId); finish({ matched: false, - state: current?.state ?? "error", + state: current ? this.liveStateOf(current).state : "error", elapsed, source: "timeout", agent: current ? toPublicAgent(current) : null, @@ -8219,15 +8329,21 @@ export class AgentEngine { ); current = refreshed.agent; + // The sweep runs the same live gate as the retroactive check: gating + // only the entry short-circuit would just move the false completion + // one poll interval later. + const live = this.liveStateOf(current); + const evidenceSource = await this.getTargetStateEvidenceSource( current, targetState, + live.state, ); if (evidenceSource) { clearInterval(checkInterval); finish({ matched: true, - state: current.state, + state: live.state, elapsed, source: refreshed.source ?? @@ -8238,19 +8354,16 @@ export class AgentEngine { } // Fail-fast on terminal error - if ( - TERMINAL_STATES.has(current.state) && - current.state !== targetState - ) { + if (TERMINAL_STATES.has(live.state) && live.state !== targetState) { clearInterval(checkInterval); finish({ matched: false, - state: current.state, + state: live.state, elapsed, source: "sweep", agent: toPublicAgent(current), error: - current.error ?? `Agent entered terminal state: ${current.state}`, + current.error ?? `Agent entered terminal state: ${live.state}`, }); } }, WAIT_FOR_SWEEP_INTERVAL_MS); diff --git a/src/watch-spec.ts b/src/watch-spec.ts index 506bfe89..3ca4085e 100644 --- a/src/watch-spec.ts +++ b/src/watch-spec.ts @@ -84,6 +84,14 @@ export interface WatchAgentObservation { exists: boolean; state: string | null; source: string; + /** + * AIDEV-NOTE (F1b, #472): what the observer actually saw, in the observer's + * own words ("registry hit, screen unparseable"). The arm refusal quotes it + * instead of asserting the agent "does not exist" -- a claim the observer + * cannot make from a failed screen read, and one that was demonstrably false + * for agents `send_to` was delivering to in the same second. + */ + detail?: string; } export interface WatchRegistryOptions { @@ -490,7 +498,9 @@ export async function armWatch( throw new WatchArmError( "watch_target_missing", target, - `Watch target agent does not exist: ${target}`, + `Watch target agent is not observable: ${target} (${ + agentObservation?.detail ?? "no observation was returned" + })`, ); } const source: WatchObservedSource = diff --git a/tests/f1b-wait-for-watch-live-state.test.ts b/tests/f1b-wait-for-watch-live-state.test.ts new file mode 100644 index 00000000..af60ccda --- /dev/null +++ b/tests/f1b-wait-for-watch-live-state.test.ts @@ -0,0 +1,314 @@ +import { mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { AgentEngine } from "../src/agent-engine.js"; +import { AgentRegistry } from "../src/agent-registry.js"; +import type { AgentRecord } from "../src/agent-types.js"; +import type { CmuxClient } from "../src/cmux-client.js"; +import { resolveLiveAgentState } from "../src/live-agent-state.js"; +import { StateManager } from "../src/state-manager.js"; +import { WatchArmError, readWatchRegistry } from "../src/watch-spec.js"; +import type { CmuxSurface } from "../src/types.js"; + +const TEST_DIR = join(tmpdir(), "cmuxlayer-f1b-wait-for-watch-live-state"); +const registryPath = () => join(TEST_DIR, "watches.json"); + +/** + * A Claude pane mid-turn: the exact live shape from the VoiceLayer report -- + * registry `done`, screen `working`. + */ +const WORKING_SCREEN = [ + "> brew install ffmpeg", + "", + "✻ Compiling… (esc to interrupt)", + "", + "╭──────────────────────────────────────────────╮", + "│ > │", + "╰──────────────────────────────────────────────╯", +].join("\n"); + +function makeMockClient(overrides?: Partial): CmuxClient { + return { + newSplit: vi.fn(), + newSurface: vi.fn(), + send: vi.fn(), + sendKey: vi.fn(), + readScreen: vi.fn().mockResolvedValue({ + surface: "surface:worker", + text: WORKING_SCREEN, + lines: 30, + scrollback_used: false, + }), + renameTab: vi.fn(), + setStatus: vi.fn(), + closeSurface: vi.fn(), + listWorkspaces: vi.fn().mockResolvedValue({ workspaces: [] }), + listPanes: vi.fn().mockResolvedValue({ panes: [] }), + listPaneSurfaces: vi.fn().mockResolvedValue({ surfaces: [] }), + selectWorkspace: vi.fn(), + clearStatus: vi.fn(), + setProgress: vi.fn(), + clearProgress: vi.fn(), + identify: vi.fn().mockResolvedValue({}), + browser: vi.fn().mockResolvedValue({}), + log: vi.fn(), + ...overrides, + } as unknown as CmuxClient; +} + +function makeSurface(ref: string): CmuxSurface { + return { ref, title: "", type: "terminal", index: 0, selected: false }; +} + +function makeRecord(overrides?: Partial): AgentRecord { + return { + agent_id: "voicelayerClaude-2ac0d960", + surface_id: "surface:worker", + state: "done", + repo: "voicelayer", + model: "opus", + cli: "claude", + cli_session_id: null, + task_summary: "F1b live-state fixture", + pid: null, + version: 0, + created_at: "2026-08-19T10:00:00.000Z", + updated_at: "2026-08-19T10:01:00.000Z", + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "worker", + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + crash_recover: false, + respawn_attempts: 0, + user_killed: false, + ...overrides, + }; +} + +/** The server's live probe, standing in for a fresh screen scan. */ +const workingScreenProbe = (agent: AgentRecord) => + resolveLiveAgentState(agent, { + status: "working", + agent_type: "claude", + control_state: "busy", + }); + +describe("F1b #473 — wait_for terminates on live state, never on a contradicted record", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (client?: CmuxClient): void => { + stateMgr = new StateManager(TEST_DIR); + liveSurfaces = [makeSurface("surface:worker")]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine(stateMgr, registry, client ?? makeMockClient(), { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + watchRegistryPath: registryPath(), + watchRegistryNow: () => 1_000, + }); + }; + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + buildEngine(); + }); + + afterEach(() => { + vi.useRealTimers(); + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("blocks when the registry says done and the screen says working", async () => { + vi.useFakeTimers(); + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(workingScreenProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + expect(result.matched).toBe(false); + // The wait must NOT have short-circuited: it ran to its own deadline. + expect(result.source).toBe("timeout"); + expect(result.elapsed).toBeGreaterThanOrEqual(1_500); + // Top-level state is the reconciled value, not the poisoned record. + expect(result.state).toBe("working"); + expect(result.error).not.toBe("Agent has already completed"); + }); + + it("blocks when the registry says error and the screen says working", async () => { + vi.useFakeTimers(); + stateMgr.writeState( + makeRecord({ state: "error", error: "stale registry error" }), + ); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(workingScreenProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + expect(result.matched).toBe(false); + expect(result.source).toBe("timeout"); + expect(result.state).toBe("working"); + expect(result.error).not.toBe("stale registry error"); + }); + + it("still short-circuits on a recorded done when no live evidence contradicts it", async () => { + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + + const result = await engine.waitFor( + "voicelayerClaude-2ac0d960", + "idle", + 1_500, + ); + + expect(result.matched).toBe(false); + expect(result.source).toBe("immediate"); + expect(result.state).toBe("done"); + expect(result.error).toBe("Agent has already completed"); + }); + + it("does not report a match from a record state the screen contradicts", async () => { + vi.useFakeTimers(); + stateMgr.writeState(makeRecord({ state: "idle" })); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(workingScreenProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + expect(result.matched).toBe(false); + expect(result.state).toBe("working"); + }); +}); + +describe("F1b #472 — a watch arms on any observable agent", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (client?: CmuxClient): void => { + stateMgr = new StateManager(TEST_DIR); + liveSurfaces = [makeSurface("surface:worker")]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine(stateMgr, registry, client ?? makeMockClient(), { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + watchRegistryPath: registryPath(), + watchRegistryNow: () => 1_000, + }); + }; + + const spec = (predicate: "idle" | "working" | "done") => ({ + owner: "voiceClaude", + target: "voicelayerClaude-2ac0d960", + predicate, + deadline: 60_000, + }); + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + }); + + afterEach(() => { + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("arms against an agent the in-memory registry has not reconstituted", async () => { + buildEngine(); + // Written to disk, never reconstituted: exactly the divergence that made + // the watch path deny an agent `send_to` was delivering to. + stateMgr.writeState(makeRecord({ state: "working" })); + expect(engine.getRegistry().get("voicelayerClaude-2ac0d960")).toBeNull(); + + const watch = await engine.armWatch(spec("idle")); + + expect(watch.state).toBe("armed"); + expect(watch.target).toBe("voicelayerClaude-2ac0d960"); + }); + + it("retries once and arms when the screen read fails", async () => { + const readScreen = vi + .fn() + .mockRejectedValueOnce(new Error("surface busy")) + .mockResolvedValue({ + surface: "surface:worker", + text: WORKING_SCREEN, + lines: 30, + scrollback_used: false, + }); + buildEngine(makeMockClient({ readScreen } as Partial)); + stateMgr.writeState(makeRecord({ state: "working" })); + await engine.getRegistry().reconstitute(); + + const watch = await engine.armWatch(spec("idle")); + + expect(readScreen).toHaveBeenCalledTimes(2); + expect(watch.state).toBe("armed"); + }); + + it("arms when the screen is unreadable, rather than calling the agent missing", async () => { + const readScreen = vi.fn().mockRejectedValue(new Error("surface busy")); + buildEngine(makeMockClient({ readScreen } as Partial)); + stateMgr.writeState(makeRecord({ state: "working" })); + await engine.getRegistry().reconstitute(); + + const watch = await engine.armWatch(spec("idle")); + + expect(watch.state).toBe("armed"); + expect(readScreen).toHaveBeenCalledTimes(2); + }); + + it("arms on a booting agent and leaves the predicate to resolve", async () => { + const readScreen = vi.fn().mockResolvedValue({ + surface: "surface:worker", + text: "\n\n", + lines: 30, + scrollback_used: false, + }); + buildEngine(makeMockClient({ readScreen } as Partial)); + stateMgr.writeState(makeRecord({ state: "booting" })); + await engine.getRegistry().reconstitute(); + + const watch = await engine.armWatch(spec("idle")); + expect(watch.state).toBe("armed"); + + // An unparseable frame must not be read as an idle prompt: the watch + // stays armed through a real sweep instead of firing on a booting pane. + const swept = await engine.waitForWatch(spec("idle"), 150); + expect(swept.matched).toBe(false); + expect(swept.watch.state).toBe("armed"); + expect( + readWatchRegistry({ registryPath: registryPath() }).watches.find( + (row) => row.watch_id === watch.watch_id, + )?.state, + ).toBe("armed"); + }); + + it("names what was observed when it must still refuse", async () => { + buildEngine(); + + await expect(engine.armWatch(spec("idle"))).rejects.toMatchObject({ + name: "WatchArmError", + code: "watch_target_missing", + }); + const error = await engine.armWatch(spec("idle")).catch((e) => e as WatchArmError); + expect(error.message).not.toContain("does not exist"); + expect(error.message).toContain("voicelayerClaude-2ac0d960"); + expect(error.message).toMatch(/no registry or state record/i); + }); +}); From 1e19376e12a7e843c3d8094b888d2939133ff236 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 19:48:26 +0300 Subject: [PATCH 2/5] fix(f1b): the wait buys its own live evidence instead of hoping the cache is warm Round 2, reviewer finding A (BLOCKING). Round 1 read the live-resolved state everywhere it decides, and then depended on `discovery.cachedScan()` for that state -- which is evidence-free once the scan is 2000ms old, and nothing on the `wait_for` path refreshes it. For a lead whose next action is `wait_for` the cache is ordinarily cold, so the entry short-circuit resolved to the poisoned record and returned the reported bug byte-for-byte; warm at entry, it moved to the sweep tick two seconds later. Mock-green, not live-green: the round-1 probe modelled the resolver's shape and never its availability. So the engine can now FORCE evidence. `setFreshLiveStateProbe` takes an async single-surface probe (server-wired to `discovery.scanTarget`, not a fleet `scan`), `refreshLiveState` reads one screen and memoizes the resolution for LIVE_EVIDENCE_TTL_MS, and `liveStateOf` answers from that memo -- dropping it when the record moves, so a wait never answers with evidence about the agent's past. `waitFor` buys evidence at entry, on a 2000ms sweep cadence, and once more at timeout. Payload: one screen read per agent at entry, one per 2s while waiting, one at timeout -- bounded and asserted, not one per 1000ms tick. That memo also closes the second symptom reported live: P11 closure reads `liveStateOf`, so a working child rendered `closure:"artifact_missing"` beside `state:"working"` in one payload. The closure in a wait's own reply is now computed from the evidence that wait bought. Only positive evidence of ACTIVITY may overturn a terminal record (`terminationStateOf`). A ready prompt is where a finished worker sits and a pane reclaimed by a bare shell says nothing about whether the task completed; without this rule `wait_for(done)` reported `error` for an agent that genuinely finished on a surface that was later reclaimed. Finding B: the ready-evidence gate no longer decides from the raw record -- it opens when either the record or the live state is in the pre-target state, and additionally requires the record to be able to REACH the target, so it does not buy a screen read per tick for a transition `VALID_TRANSITIONS` forbids. Stated plainly in the code: for a `done`-poisoned record the wait still runs to timeout, because `VALID_TRANSITIONS.done` is empty. It fails safe; the other half is #408. Nits: `live` is computed where it is used; the read-failure fallback in the watch observation is documented as a decision, not an accident. Adds the negative watch-arm coverage the review asked for (bare shell still refuses). Two pre-existing expectations encoded the pre-F1b contract for a registry-done agent whose screen shows work in progress; both are updated with the reason, and neither test's own subject changed. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent-engine.ts | 223 ++++++++++-- src/server.ts | 92 +++-- tests/f1b-wait-for-watch-live-state.test.ts | 373 +++++++++++++++++++- tests/painpoint-e2e.test.ts | 7 +- tests/server-agent-tools.test.ts | 10 +- 5 files changed, 654 insertions(+), 51 deletions(-) diff --git a/src/agent-engine.ts b/src/agent-engine.ts index dbbce2f8..d9b0c15b 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -52,6 +52,22 @@ import { /** Live-derived state for a record, injected by the server (F1). */ export type LiveStateResolver = (agent: AgentRecord) => LiveAgentState | null; +/** + * AIDEV-NOTE (F1b round 2): the FORCING counterpart to `LiveStateResolver`. + * + * The sync resolver reads whatever screen scan happens to be cached, and the + * cache is deliberately evidence-free once it is 2000ms old -- so a lead whose + * next action after a spawn is `wait_for` gets no live evidence at all, and + * every live gate in this file silently degrades to the poisoned record. This + * probe reads ONE agent's screen on demand, so a wait can obtain its own + * evidence instead of hoping somebody else scanned recently. Returns null when + * the read fails or the surface does not bind to the record: no evidence, which + * leaves the record unchallenged rather than inventing a state. + */ +export type FreshLiveStateProbe = ( + agent: AgentRecord, +) => Promise; + import { resumeCommandForAgent, resumeCwdForAgent, @@ -86,6 +102,7 @@ import { resolveBootPromptText, summarizeTaskSummary, type AgentRoute, + isValidTransition, type AgentRecord, type AgentAuthority, type AgentFunction, @@ -710,6 +727,19 @@ const TERMINAL_STATES = new Set(["done", "error"]); const WAIT_FOR_SWEEP_INTERVAL_MS = 1000; /** One retry: a watch observation must not read a transient failure as absence. */ const WATCH_OBSERVATION_READ_ATTEMPTS = 2; +/** + * How long a forced live-state observation stays usable, matched to + * `AgentDiscovery`'s own 2000ms TTL so nothing in the fleet trusts screen + * evidence longer than the scan cache would. + */ +const LIVE_EVIDENCE_TTL_MS = 2000; +/** + * Sweep cadence for re-forcing live evidence during a wait. One extra screen + * read per agent per interval, and never older than `LIVE_EVIDENCE_TTL_MS` -- + * the two are the same number on purpose: the memo covers exactly the gap + * between refreshes, so no tick ever decides on expired evidence. + */ +const WAIT_FOR_LIVE_EVIDENCE_INTERVAL_MS = LIVE_EVIDENCE_TTL_MS; const DEFAULT_SWEEP_ACTIVE_INTERVAL_MS = 5_000; const DEFAULT_SWEEP_IDLE_INTERVAL_MS = 15_000; const DEFAULT_SWEEP_IDLE_AFTER_SWEEPS = 3; @@ -1466,6 +1496,12 @@ export function resolveSpawnLaunchPlan( export class AgentEngine { private stateMgr: StateManager; private liveStateResolver: LiveStateResolver | null = null; + private freshLiveStateProbe: FreshLiveStateProbe | null = null; + /** Forced observations, TTL-bounded, keyed by agent id. */ + private freshLiveStates = new Map< + string, + { live: LiveAgentState; at: number } + >(); private registry: AgentRegistry; private client: AgentEngineClient; private spawnPreflight: ( @@ -1729,8 +1765,79 @@ export class AgentEngine { this.liveStateResolver = resolver; } + /** + * AIDEV-NOTE (F1b round 2): the probe that lets a path FORCE evidence instead + * of depending on an incidentally-warm scan cache. The server wires it to a + * single-surface discovery scan; without it `refreshLiveState` degrades to + * the sync resolver, and this engine behaves exactly as it did before. + */ + setFreshLiveStateProbe(probe: FreshLiveStateProbe | null): void { + this.freshLiveStateProbe = probe; + this.freshLiveStates.clear(); + } + + /** + * Read one agent's screen NOW and memoize the resolution for + * `LIVE_EVIDENCE_TTL_MS`. The memo is what makes the forced read pay for the + * whole tick: `liveStateOf` -- and so every live gate reached from the same + * turn, closure included -- sees the evidence this call bought. + */ + async refreshLiveState(agent: AgentRecord): Promise { + const probe = this.freshLiveStateProbe; + if (!probe) return this.liveStateOf(agent); + let live: LiveAgentState | null = null; + try { + live = await probe(agent); + } catch { + // A failed read is not evidence; fall back to whatever else is known. + live = null; + } + if (!live) return this.liveStateOf(agent); + const at = Date.now(); + for (const [agentId, entry] of this.freshLiveStates) { + if (at - entry.at >= LIVE_EVIDENCE_TTL_MS) { + this.freshLiveStates.delete(agentId); + } + } + this.freshLiveStates.set(agent.agent_id, { live, at }); + return live; + } + + /** + * The live state a WAIT may terminate on. + * + * AIDEV-NOTE (F1b round 2): F1's rule, applied to termination. Only positive + * evidence of ACTIVITY -- the screen showing the agent still working -- is + * strong enough to overturn a terminal record. A ready prompt is where a + * finished worker sits, and a pane reclaimed by a bare shell says nothing + * about whether the task completed; treating either as truth would fail an + * agent that genuinely finished (`wait_for(done)` reporting `error` because + * the pane was later reclaimed). The record keeps terminal states it earned; + * it only loses the ones the screen contradicts with work in progress. + */ + private terminationStateOf( + agent: AgentRecord, + live: LiveAgentState, + ): AgentState { + if (TERMINAL_STATES.has(agent.state) && !isLiveActive(live)) { + return agent.state; + } + return live.state; + } + /** Live state for one record, or the record's own state when unprobed. */ liveStateOf(agent: AgentRecord): LiveAgentState { + const memo = this.freshLiveStates.get(agent.agent_id); + // The memo is a reconciliation OF a specific record. If the record moved, + // the reconciliation is about a state that no longer exists -- drop it, or + // a wait would keep answering with evidence about the agent's past. + if ( + memo && + Date.now() - memo.at < LIVE_EVIDENCE_TTL_MS && + memo.live.registry_state === agent.state + ) { + return memo.live; + } return ( this.liveStateResolver?.(agent) ?? resolveLiveAgentState(agent, null) ); @@ -2386,6 +2493,7 @@ export class AgentEngine { agent: AgentRecord, targetState: AgentState, waitForReadyPatternMatches: Map, + effectiveState: AgentState = agent.state, ): Promise<{ agent: AgentRecord; source?: RefreshedTargetStateEvidenceSource; @@ -2395,6 +2503,7 @@ export class AgentEngine { agent, targetState, waitForReadyPatternMatches, + effectiveState, ); } if (!this.requiresOutputDoneEvidence(targetState)) return { agent }; @@ -2402,19 +2511,40 @@ export class AgentEngine { return { agent: (await this.maybeMarkTaskDone(agent, {})).agent }; } + /** + * AIDEV-NOTE (F1b round 2, reviewer finding B): this gate decides whether to + * READ the screen for ready-evidence, and it used to decide purely from the + * raw record -- so on a `done`-poisoned agent it bailed, and once the wait + * correctly stopped short-circuiting it could never MATCH either. + * + * It now opens when EITHER the record or the live state says the agent is in + * the pre-target state, so a poisoned record alone no longer closes it. But + * it also requires the record to be able to REACH the target, because the + * transition below writes from the record: `VALID_TRANSITIONS.done` is empty, + * so a `done` record cannot become `idle` no matter what the screen shows. + * That guard is what keeps the widened gate from buying a screen read per + * tick for a transition that would throw anyway. + * + * Consequence, stated plainly: for a `done`-poisoned agent the wait still + * runs to timeout. It fails safe -- a timeout is not a false completion -- + * and the remaining half is #408 itself (stop poisoning the record) or a + * deliberate repair path, both outside this lane. + */ private async refreshInteractiveTargetStateEvidence( agent: AgentRecord, targetState: "ready" | "idle", waitForReadyPatternMatches: Map, + effectiveState: AgentState = agent.state, ): Promise<{ agent: AgentRecord; source?: RefreshedTargetStateEvidenceSource; }> { + const inPreTargetState = (state: AgentState): boolean => + targetState === "ready" ? state === "booting" : state === "working"; const canTransition = - targetState === "ready" - ? agent.state === "booting" - : agent.state === "working"; - if (!canTransition || TERMINAL_STATES.has(agent.state)) { + (inPreTargetState(agent.state) || inPreTargetState(effectiveState)) && + isValidTransition(agent.state, targetState); + if (!canTransition || TERMINAL_STATES.has(effectiveState)) { waitForReadyPatternMatches.delete(agent.agent_id); return { agent }; } @@ -7424,6 +7554,13 @@ export class AgentEngine { if (screenText === null) { // A record we can read, on a surface we momentarily cannot. That is a // read failure, not a missing agent -- report the record's own state. + // + // AIDEV-NOTE (F1b, reviewer nit): this state IS the raw record, and a + // `done`-predicate watch on an unreadable pane will therefore fire on a + // #408-poisoned `done`. That is deliberate, not an oversight: the rule + // this lane enforces is that absence of evidence leaves the record + // unchallenged, and inventing a state for a pane nobody could read would + // break it in the other direction. The deadline is the backstop. return { exists: true, state: resolveLiveAgentState(agent, null).state, @@ -7446,11 +7583,6 @@ export class AgentEngine { detail: `registry hit, screen shows ${parsed.control_state}`, }; } - const live = resolveLiveAgentState(agent, { - status: parsed.status, - agent_type: parsed.agent_type, - control_state: parsed.control_state, - }); if (parsed.control_state === "shell" && parsed.agent_type === "unknown") { return { exists: false, @@ -7460,6 +7592,11 @@ export class AgentEngine { }; } if (parsed.agent_type === "unknown") { + const live = resolveLiveAgentState(agent, { + status: parsed.status, + agent_type: parsed.agent_type, + control_state: parsed.control_state, + }); // Mid-boot or an unparseable frame: the screen has no authority over the // status here, and reading its default as an idle prompt would fire an // `idle` predicate on an agent that has not started yet. @@ -8237,18 +8374,29 @@ export class AgentEngine { // instead, and the top-level `state` reports the reconciled value rather // than the poisoned record. With no live probe wired the resolution IS the // record, so an unprobed engine behaves exactly as it did before. - const initialLive = this.liveStateOf(initial); + // + // Round 2 (reviewer finding A): reading the live state is not enough if + // nothing GUARANTEES there is any. `screenObservationForRecord` reads + // `discovery.cachedScan()`, which returns null once the scan is 2000ms old + // -- and nothing on this path refreshes it, so for a lead whose next action + // is `wait_for` the cache is ordinarily cold and the resolution degrades to + // the poisoned record, reproducing the original bug byte-for-byte. So the + // wait BUYS its own evidence at entry, and again on a deliberate cadence + // below. Cost: one screen read per agent at entry, one more per + // WAIT_FOR_LIVE_EVIDENCE_INTERVAL_MS thereafter. + const initialLive = await this.refreshLiveState(initial); + const initialState = this.terminationStateOf(initial, initialLive); // Retroactive check — already in target state with required evidence? const initialEvidence = await this.getTargetStateEvidenceSource( initial, targetState, - initialLive.state, + initialState, ); if (initialEvidence) { return { matched: true, - state: initialLive.state, + state: initialState, elapsed: Date.now() - start, source: initialEvidence === "state" ? "immediate" : initialEvidence, agent: toPublicAgent(initial), @@ -8256,10 +8404,10 @@ export class AgentEngine { } // Already in terminal error state and target isn't error? - if (initialLive.state === "error" && targetState !== "error") { + if (initialState === "error" && targetState !== "error") { return { matched: false, - state: initialLive.state, + state: initialState, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8268,10 +8416,10 @@ export class AgentEngine { } // Already in terminal done state and target isn't done? - if (initialLive.state === "done" && targetState !== "done") { + if (initialState === "done" && targetState !== "done") { return { matched: false, - state: initialLive.state, + state: initialState, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8280,6 +8428,9 @@ export class AgentEngine { } const waitForReadyPatternMatches = new Map(); + // Entry already bought evidence, so the first sweep refresh is due one + // full interval in. + let lastForcedEvidenceElapsed = 0; // Polling sweep loop return new Promise((resolve) => { @@ -8293,9 +8444,20 @@ export class AgentEngine { if (elapsed >= timeoutMs) { clearInterval(checkInterval); const current = this.registry.get(agentId); + // The timeout answer is the one a lead acts on, and it lands after + // the memo from the last cadence refresh has expired -- so buy one + // final observation rather than reporting the record by default. + // It also leaves fresh evidence behind for whatever renders the + // reply (P11 closure reads it in the same turn). + const timeoutLive = current + ? await this.refreshLiveState(current) + : null; finish({ matched: false, - state: current ? this.liveStateOf(current).state : "error", + state: + current && timeoutLive + ? this.terminationStateOf(current, timeoutLive) + : "error", elapsed, source: "timeout", agent: current ? toPublicAgent(current) : null, @@ -8322,28 +8484,43 @@ export class AgentEngine { return; } + // Re-force evidence on a deliberate cadence. Between refreshes the + // memo from the last one answers, and it expires exactly when the next + // is due, so no tick ever decides on evidence older than the TTL. + if ( + elapsed - lastForcedEvidenceElapsed >= + WAIT_FOR_LIVE_EVIDENCE_INTERVAL_MS + ) { + lastForcedEvidenceElapsed = elapsed; + await this.refreshLiveState(current); + } + const refreshed = await this.refreshTargetStateEvidence( current, targetState, waitForReadyPatternMatches, + this.terminationStateOf(current, this.liveStateOf(current)), ); current = refreshed.agent; // The sweep runs the same live gate as the retroactive check: gating // only the entry short-circuit would just move the false completion - // one poll interval later. + // one poll interval later. Resolved AFTER the refresh, because a + // refresh that transitions the record is itself fresh screen evidence + // -- and `liveStateOf` drops a memo whose record has moved. const live = this.liveStateOf(current); + const liveState = this.terminationStateOf(current, live); const evidenceSource = await this.getTargetStateEvidenceSource( current, targetState, - live.state, + liveState, ); if (evidenceSource) { clearInterval(checkInterval); finish({ matched: true, - state: live.state, + state: liveState, elapsed, source: refreshed.source ?? @@ -8354,16 +8531,16 @@ export class AgentEngine { } // Fail-fast on terminal error - if (TERMINAL_STATES.has(live.state) && live.state !== targetState) { + if (TERMINAL_STATES.has(liveState) && liveState !== targetState) { clearInterval(checkInterval); finish({ matched: false, - state: live.state, + state: liveState, elapsed, source: "sweep", agent: toPublicAgent(current), error: - current.error ?? `Agent entered terminal state: ${live.state}`, + current.error ?? `Agent entered terminal state: ${liveState}`, }); } }, WAIT_FOR_SWEEP_INTERVAL_MS); diff --git a/src/server.ts b/src/server.ts index 78438694..f2a66686 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10333,34 +10333,43 @@ export function createServer(opts?: CreateServerOptions): McpServer { // reads the last screen scan only -- no I/O on the caller's path -- and // returns null when there is no fresh evidence, which degrades to the // registry record with honest `registry` provenance. - const screenObservationForRecord = ( + type ScreenObservationRow = { + surface_id: string; + surface_uuid?: string | null; + parsed_status?: string | null; + control_state?: string | null; + cli?: string | null; + read_error?: unknown; + }; + // Same binding rule list_agents uses: a UUID pair, or a surface_id match + // ONLY when neither side has a UUID and this observer owns the seat. + // A looser match would let an unrelated pane's screen decide an agent's + // state, which is a worse lie than the stale record it replaces. + const rowBindsToRecord = ( agent: AgentRecord, + row: ScreenObservationRow, + ): boolean => { + const uuidKey = (value: string | null | undefined): string | null => + value?.trim().toLowerCase() || null; + const agentUuid = uuidKey(agent.surface_uuid); + const surfaceUuid = uuidKey(row.surface_uuid); + return agentUuid && surfaceUuid + ? agentUuid === surfaceUuid + : Boolean( + !agentUuid && + !surfaceUuid && + agent.surface_observer_id && + agent.surface_observer_id === registry.getObserverId() && + row.surface_id === agent.surface_id, + ); + }; + const observationFromRow = ( + row: ScreenObservationRow | null | undefined, ): { status: string | null; agent_type: string | null; control_state: string | null; } | null => { - const cached = discovery.cachedScan(); - if (!cached) return null; - const uuidKey = (value: string | null | undefined): string | null => - value?.trim().toLowerCase() || null; - const agentUuid = uuidKey(agent.surface_uuid); - // Same binding rule list_agents uses: a UUID pair, or a surface_id match - // ONLY when neither side has a UUID and this observer owns the seat. - // A looser match would let an unrelated pane's screen decide an agent's - // state, which is a worse lie than the stale record it replaces. - const row = cached.rows.find((surface) => { - const surfaceUuid = uuidKey(surface.surface_uuid); - return agentUuid && surfaceUuid - ? agentUuid === surfaceUuid - : Boolean( - !agentUuid && - !surfaceUuid && - agent.surface_observer_id && - agent.surface_observer_id === registry.getObserverId() && - surface.surface_id === agent.surface_id, - ); - }); if (!row || row.read_error) return null; return { status: row.parsed_status ?? null, @@ -10368,8 +10377,46 @@ export function createServer(opts?: CreateServerOptions): McpServer { control_state: row.control_state ?? null, }; }; + const screenObservationForRecord = ( + agent: AgentRecord, + ): { + status: string | null; + agent_type: string | null; + control_state: string | null; + } | null => { + const cached = discovery.cachedScan(); + if (!cached) return null; + return observationFromRow( + cached.rows.find((row) => rowBindsToRecord(agent, row)), + ); + }; liveAgentStateProbe.current = (agent) => resolveLiveAgentState(agent, screenObservationForRecord(agent)); + /** + * AIDEV-NOTE (F1b round 2): the FORCING probe. `cachedScan()` is + * deliberately evidence-free once it is 2000ms old, and nothing on the + * `wait_for` path refreshes it -- so a wait that only read the cache + * degraded straight back to the poisoned record. This reads ONE surface on + * demand (`scanTarget`, not a fleet `scan`), applies the same binding rule, + * and returns null on a failed read or an unbound surface: no evidence, + * which leaves the record unchallenged rather than inventing a state. + */ + const freshLiveAgentStateProbe = async ( + agent: AgentRecord, + ): Promise => { + try { + const row = await discovery.scanTarget({ + surface_id: agent.surface_id, + surface_uuid: agent.surface_uuid ?? null, + }); + if (!row || !rowBindsToRecord(agent, row)) return null; + const observation = observationFromRow(row); + return observation ? resolveLiveAgentState(agent, observation) : null; + } catch { + // A failed or racing scan is not evidence of anything. + return null; + } + }; const awaitLifecycleStart = async (): Promise => { if (context.lifecycleStartPromise) { await context.lifecycleStartPromise; @@ -10702,6 +10749,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { // F1: closure, harvestability and the health report all resolve state // through the same live probe the caller/delivery paths use. engine.setLiveStateResolver(liveAgentStateProbe.current); + engine.setFreshLiveStateProbe(freshLiveAgentStateProbe); server.tool( "arm_watch", diff --git a/tests/f1b-wait-for-watch-live-state.test.ts b/tests/f1b-wait-for-watch-live-state.test.ts index af60ccda..77c577b3 100644 --- a/tests/f1b-wait-for-watch-live-state.test.ts +++ b/tests/f1b-wait-for-watch-live-state.test.ts @@ -6,6 +6,7 @@ import { AgentEngine } from "../src/agent-engine.js"; import { AgentRegistry } from "../src/agent-registry.js"; import type { AgentRecord } from "../src/agent-types.js"; import type { CmuxClient } from "../src/cmux-client.js"; +import type { LiveAgentState } from "../src/live-agent-state.js"; import { resolveLiveAgentState } from "../src/live-agent-state.js"; import { StateManager } from "../src/state-manager.js"; import { WatchArmError, readWatchRegistry } from "../src/watch-spec.js"; @@ -43,9 +44,48 @@ function makeMockClient(overrides?: Partial): CmuxClient { renameTab: vi.fn(), setStatus: vi.fn(), closeSurface: vi.fn(), - listWorkspaces: vi.fn().mockResolvedValue({ workspaces: [] }), - listPanes: vi.fn().mockResolvedValue({ panes: [] }), - listPaneSurfaces: vi.fn().mockResolvedValue({ surfaces: [] }), + // A complete topology: terminal I/O refuses to read a surface it cannot + // prove is live and uniquely bound, so a suite that asserts on screen + // reads has to describe the pane the record points at. + listWorkspaces: vi.fn().mockResolvedValue({ + workspaces: [ + { + ref: "workspace:fleet", + title: "Fleet", + index: 0, + selected: true, + pinned: false, + }, + ], + }), + listPanes: vi.fn().mockResolvedValue({ + workspace_ref: "workspace:fleet", + window_ref: "window:fleet", + panes: [ + { + ref: "pane:fleet", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:worker"], + }, + ], + }), + listPaneSurfaces: vi.fn().mockResolvedValue({ + workspace_ref: "workspace:fleet", + window_ref: "window:fleet", + pane_ref: "pane:fleet", + surfaces: [ + { + id: "uuid-worker", + ref: "surface:worker", + title: "cmuxlayerClaude", + type: "terminal", + index: 0, + selected: true, + }, + ], + }), selectWorkspace: vi.fn(), clearStatus: vi.fn(), setProgress: vi.fn(), @@ -312,3 +352,330 @@ describe("F1b #472 — a watch arms on any observable agent", () => { expect(error.message).toMatch(/no registry or state record/i); }); }); + + +/** + * The cold discovery cache, modelled honestly. + * + * `screenObservationForRecord` reads `discovery.cachedScan()`, which returns + * null once the scan is 2000ms old — so the SYNC resolver's ordinary answer on + * the `wait_for` path is "no evidence", not "working". A probe that returns + * live evidence forever models the resolver's shape and never its availability, + * which is how round 1 shipped a fix that reproduced the original bug live. + */ +const coldSyncResolver = (agent: AgentRecord): LiveAgentState => + resolveLiveAgentState(agent, null); + +/** A sync resolver that goes cold after N calls, as the real cache does. */ +function coldAfter(calls: number): { + resolve: (agent: AgentRecord) => LiveAgentState; + calls: () => number; +} { + let seen = 0; + return { + resolve: (agent) => { + seen += 1; + return seen <= calls ? workingScreenProbe(agent) : coldSyncResolver(agent); + }, + calls: () => seen, + }; +} + +/** The forcing probe: what a real single-surface screen read would return. */ +const workingFreshProbe = async ( + agent: AgentRecord, +): Promise => workingScreenProbe(agent); + +describe("F1b round 2 — the wait buys its own evidence instead of hoping the cache is warm", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (client?: CmuxClient): void => { + stateMgr = new StateManager(TEST_DIR); + liveSurfaces = [makeSurface("surface:worker")]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine(stateMgr, registry, client ?? makeMockClient(), { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + watchRegistryPath: registryPath(), + watchRegistryNow: () => 1_000, + }); + }; + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + buildEngine(); + }); + + afterEach(() => { + vi.useRealTimers(); + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("blocks with a COLD cache at entry — the reported bug, byte for byte", async () => { + vi.useFakeTimers(); + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + // Nothing scanned recently: the sync resolver has no evidence at all. + engine.setLiveStateResolver(coldSyncResolver); + engine.setFreshLiveStateProbe(workingFreshProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + expect(result.source).toBe("timeout"); + expect(result.elapsed).toBeGreaterThanOrEqual(1_500); + expect(result.state).toBe("working"); + expect(result.error).not.toBe("Agent has already completed"); + }); + + it("blocks when the cache goes cold mid-wait, not just at entry", async () => { + vi.useFakeTimers(); + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + const resolver = coldAfter(1); + engine.setLiveStateResolver(resolver.resolve); + engine.setFreshLiveStateProbe(workingFreshProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 3_500); + await vi.advanceTimersByTimeAsync(4_000); + const result = await pending; + + expect(result.source).toBe("timeout"); + expect(result.state).toBe("working"); + }); + + it("forces one read at entry and then only on the declared cadence", async () => { + vi.useFakeTimers(); + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(coldSyncResolver); + const probe = vi.fn(workingFreshProbe); + engine.setFreshLiveStateProbe(probe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 4_500); + await vi.advanceTimersByTimeAsync(5_000); + await pending; + + // Entry + one per 2000ms of a 4500ms wait: bounded and countable, not + // one screen read per 1000ms tick. + expect(probe.mock.calls.length).toBeLessThanOrEqual(4); + expect(probe.mock.calls.length).toBeGreaterThanOrEqual(3); + }); + + it("still short-circuits with a cold cache and NO forcing probe", async () => { + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(coldSyncResolver); + + const result = await engine.waitFor( + "voicelayerClaude-2ac0d960", + "idle", + 1_500, + ); + + // Unprobed, no evidence exists anywhere: the record stands, unchanged. + expect(result.source).toBe("immediate"); + expect(result.state).toBe("done"); + }); + + it("renders closure:pending — never artifact_missing — for a working child on a cold cache", async () => { + vi.useFakeTimers(); + stateMgr.writeState( + makeRecord({ + state: "done", + report_path: "/tmp/report.md", + done_marker: "DONE_WORKER", + }), + ); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(coldSyncResolver); + engine.setFreshLiveStateProbe(workingFreshProbe); + const record = engine.getAgentState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + + // Cold, before the wait: no evidence, so closure honestly reads the record. + expect(engine.assessHarvestability(record).closure).toBe( + "artifact_missing", + ); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + // P11 Contract B: the closure the lead reads in the wait's own reply is + // computed from the evidence that wait bought. Two fields of one payload + // must not disagree about whether the child is working. + expect(result.state).toBe("working"); + expect(engine.assessHarvestability(record).closure).toBe("pending"); + }); + + it("expires forced evidence rather than answering from a stale observation", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-19T10:00:00.000Z")); + stateMgr.writeState(makeRecord({ state: "done" })); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(coldSyncResolver); + engine.setFreshLiveStateProbe(workingFreshProbe); + const record = engine.getAgentState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + + await engine.refreshLiveState(record); + expect(engine.liveStateOf(record).state).toBe("working"); + + vi.setSystemTime(new Date("2026-08-19T10:00:03.000Z")); + // Three seconds later the observation is older than the discovery TTL, so + // it stops speaking for the agent instead of aging into a new lie. + expect(engine.liveStateOf(record).state).toBe("done"); + }); +}); + +describe("F1b round 2 — finding B: the ready-evidence gate no longer reads the raw record", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (readScreen: ReturnType): void => { + stateMgr = new StateManager(TEST_DIR); + // A UUID-bound surface: terminal I/O refuses a UUID-less ref it cannot + // prove ownership of, and this suite is about whether the read HAPPENS. + liveSurfaces = [{ ...makeSurface("surface:worker"), id: "uuid-worker" }]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine( + stateMgr, + registry, + makeMockClient({ readScreen } as Partial), + { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + }, + ); + }; + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + }); + + afterEach(() => { + vi.useRealTimers(); + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("reads the screen for a working record whose live state agrees", async () => { + vi.useFakeTimers(); + const readScreen = vi.fn().mockResolvedValue({ + surface: "surface:worker", + text: WORKING_SCREEN, + lines: 30, + scrollback_used: false, + }); + buildEngine(readScreen); + stateMgr.writeState( + makeRecord({ state: "working", surface_uuid: "uuid-worker" }), + ); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(workingScreenProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + await pending; + + expect(readScreen).toHaveBeenCalled(); + }); + + it("does not buy a screen read for a transition the record cannot make", async () => { + vi.useFakeTimers(); + const readScreen = vi.fn().mockResolvedValue({ + surface: "surface:worker", + text: WORKING_SCREEN, + lines: 30, + scrollback_used: false, + }); + buildEngine(readScreen); + // `VALID_TRANSITIONS.done` is empty: no screen can move this record to + // idle, so the widened gate must not spend a read per tick trying. + stateMgr.writeState( + makeRecord({ state: "done", surface_uuid: "uuid-worker" }), + ); + await engine.getRegistry().reconstitute(); + engine.setLiveStateResolver(workingScreenProbe); + + const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); + await vi.advanceTimersByTimeAsync(2_000); + const result = await pending; + + expect(readScreen).not.toHaveBeenCalled(); + // Fails safe: a timeout, never a false completion. Closing the other half + // means #408 stopping the poisoning, which is not this lane. + expect(result.source).toBe("timeout"); + expect(result.matched).toBe(false); + }); +}); + +describe("F1b — a watch still refuses when the surface is positively gone", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (screenText: string): void => { + stateMgr = new StateManager(TEST_DIR); + liveSurfaces = [makeSurface("surface:worker")]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine( + stateMgr, + registry, + makeMockClient({ + readScreen: vi.fn().mockResolvedValue({ + surface: "surface:worker", + text: screenText, + lines: 30, + scrollback_used: false, + }), + } as Partial), + { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + watchRegistryPath: registryPath(), + watchRegistryNow: () => 1_000, + }, + ); + }; + + const spec = { + owner: "voiceClaude", + target: "voicelayerClaude-2ac0d960", + predicate: "idle" as const, + deadline: 60_000, + }; + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + }); + + afterEach(() => { + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("refuses a bare shell, naming it — existence did not become unconditional", async () => { + buildEngine("etanheyman@Mac cmuxlayer % \n"); + stateMgr.writeState(makeRecord({ state: "working" })); + await engine.getRegistry().reconstitute(); + + const error = await engine + .armWatch(spec) + .then(() => null) + .catch((e: WatchArmError) => e); + + expect(error?.code).toBe("watch_target_missing"); + expect(error?.message).toMatch(/bare shell/i); + expect(error?.message).not.toContain("does not exist"); + }); +}); diff --git a/tests/painpoint-e2e.test.ts b/tests/painpoint-e2e.test.ts index 1ef36849..e32e6360 100644 --- a/tests/painpoint-e2e.test.ts +++ b/tests/painpoint-e2e.test.ts @@ -634,10 +634,15 @@ describe("Phase 10 painpoint e2e replay", () => { state: AgentState; }>(await pending); + // F1b (#473): the registry says `done`, the screen says `Working` -- + // so the wait blocks (unchanged) AND the state it reports is the + // reconciled value. Reporting the record's `done` here is the exact + // field a lead reads first, and reading `done` off a working agent is + // the false completion this lane exists to kill. expect(result).toMatchObject({ matched: false, source: "timeout", - state: "done", + state: "working", }); } finally { await closeServer(server); diff --git a/tests/server-agent-tools.test.ts b/tests/server-agent-tools.test.ts index 1a1f74af..471d8deb 100644 --- a/tests/server-agent-tools.test.ts +++ b/tests/server-agent-tools.test.ts @@ -12982,7 +12982,7 @@ codex> engine.getRegistry().set(agentId, doneState); const result = await waitFor.handler( - { agent_id: currentAgentId, timeout_ms: 5000 }, + { agent_id: currentAgentId, timeout_ms: 1500 }, {} as any, ); const parsed = @@ -12990,7 +12990,13 @@ codex> expect(parsed.ok).toBe(true); expect(parsed.agent_id).toBe(agentId); - expect(parsed.state).toBe("done"); + // F1b (#473): the omitted target still defaults to `done` -- but this + // fixture's pane shows `✻ Working` while the record was forced to `done`, + // and a wait may no longer terminate on a record the screen contradicts. + // So the default-target wait runs and reports the reconciled state instead + // of matching. Asserting `done` here would re-encode the false completion. + expect(parsed.state).toBe("working"); + expect(parsed.matched).toBe(false); expect(parsed.agent.session_id).toBeNull(); }, 10_000); From 692bea26684f3e2e81924f25815c20eb0ce4f5c1 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 20:02:32 +0300 Subject: [PATCH 3/5] =?UTF-8?q?fix(f1b):=20one=20row,=20one=20state=20rule?= =?UTF-8?q?=20=E2=80=94=20and=20artifact=5Fmissing=20takes=20evidence?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3, from golemsClaude's live report: five specimens on v0.4.47 with the F1 fix present, one spawned two minutes earlier, each rendering `closure:"artifact_missing"` while the same row's `state` said `ready`. Two rules were deciding one row. The row's `state` came from agent-health's reconciled state -- the raw `screenConfirmedAgentState` verdict -- while `closure` came from `isLiveActive(live) ? live.state : agent.state`, where `ready` may not overturn `done`. So a fresh agent at a live prompt whose record #408 had flipped published a live state and a terminal closure side by side, and the alarming one won. `closureStateOf` is now that one rule, in one place, with two carve-outs about EVIDENCE rather than about which field is rendering: activity always wins, and a `done` the agent EARNED survives a ready prompt so a genuinely finished worker's deadlock signal keeps working. What no longer survives is a `done` with nothing behind it. And `artifact_missing` now takes positive done evidence. It is not a description, it is an alarm -- P11's table reads it as "route a reviewer NOW" -- so `resolveClosureState` requires `doneEvidence`, sourced from the evidence channel this payload already reports (`done_source !== "none"`: a done signal seen on the screen or in the harness transcript). A record that flipped is not a task that finished. That closes the cold-cache shape too: with no live evidence anywhere, a bare `done` record can no longer fire the alarm on its own. The F1 fixture asserting artifact_missing at a ready prompt carried no done evidence, which made it indistinguishable from the live specimen; it now carries `task_done_detected_at`, and its sibling -- same screen, same missing report, no evidence -- asserts `state:"ready"` beside `closure:"pending"` through the real `list_agents` tool. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent-engine.ts | 45 ++++++- src/coordination-paths.ts | 19 ++- tests/coordination-paths.test.ts | 29 +++- tests/f1-live-state-truth.test.ts | 34 +++++ tests/f1b-wait-for-watch-live-state.test.ts | 141 +++++++++++++++++++- 5 files changed, 258 insertions(+), 10 deletions(-) diff --git a/src/agent-engine.ts b/src/agent-engine.ts index d9b0c15b..edf94476 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -1825,6 +1825,43 @@ export class AgentEngine { return live.state; } + /** + * Positive evidence the task ENDED: a done signal seen on the screen or in + * the harness transcript. Deliberately NOT "the record says done" -- that is + * the thing #408 fabricates. + */ + private hasPositiveDoneEvidence(agent: AgentRecord): boolean { + if (agent.task_done_detected_at) return true; + return this.loadGroundTruthSession(agent)?.state.done === true; + } + + /** + * The state CLOSURE reasons about, and the one rule a response may use. + * + * AIDEV-NOTE (F1b round 3): a list_agents row published its `state` from + * `agent-health`'s reconciled state (the raw screen verdict) while its + * `closure` came from `isLiveActive(live) ? live.state : agent.state` -- so + * a fresh agent at a `ready` prompt whose record had flipped to `done` + * rendered `state:"ready"` beside `closure:"artifact_missing"`. One row, two + * state rules, and the alarming one won. Five such specimens were observed + * live, one spawned two minutes earlier. + * + * This is that same rule, in one place, with two carve-outs that are about + * EVIDENCE rather than about which field is being rendered: + * 1. Activity always wins (F1): a screen showing work in progress overturns + * any record. + * 2. A `done` the agent EARNED survives a ready prompt -- a finished worker + * sits at one too, and its deadlock signal has to keep working. What + * does not survive is a `done` with nothing behind it. + */ + private closureStateOf(agent: AgentRecord, live: LiveAgentState): AgentState { + if (isLiveActive(live)) return live.state; + if (agent.state === "done" && this.hasPositiveDoneEvidence(agent)) { + return "done"; + } + return live.screen_state ?? agent.state; + } + /** Live state for one record, or the record's own state when unprobed. */ liveStateOf(agent: AgentRecord): LiveAgentState { const memo = this.freshLiveStates.get(agent.agent_id); @@ -1861,7 +1898,7 @@ export class AgentEngine { // worker sits at one too), and a dead/shell pane must not either: there // the record's `done` plus the missing artifact IS the story. const live = this.liveStateOf(agent); - const effectiveState = isLiveActive(live) ? live.state : agent.state; + const effectiveState = this.closureStateOf(agent, live); const neutralEvidenceChannel: HarvestabilityEvidenceChannel = { done_source: agent.task_done_detected_at ? "screen" : "none", degraded: false, @@ -1889,6 +1926,9 @@ export class AgentEngine { Boolean(agent.report_path && agent.done_marker) || Boolean(agent.goal_file), closureArtifactVerified: null, + // This branch is only reached for a non-done state (or an + // orchestrator), where the evidence question does not arise. + doneEvidence: false, }), closure_artifact_verified: null, report_path: preClosureGoal.reportPath, @@ -2008,6 +2048,9 @@ export class AgentEngine { role, contractIssued: Boolean(goal.reportPath && goal.doneMarker), closureArtifactVerified, + // The channel this payload already reports: `none` means nothing ever + // observed this task ending, so the `done` came from the record alone. + doneEvidence: evidenceChannel.done_source !== "none", }), closure_artifact_verified: closureArtifactVerified, report_path: goal.reportPath, diff --git a/src/coordination-paths.ts b/src/coordination-paths.ts index 3872a410..f8051ea5 100644 --- a/src/coordination-paths.ts +++ b/src/coordination-paths.ts @@ -135,13 +135,26 @@ export function resolveClosureState(input: { role?: string | null; contractIssued: boolean; closureArtifactVerified: boolean | null; + /** + * Positive evidence the task actually ENDED -- a done signal detected on the + * screen or in the harness transcript -- as opposed to a registry record + * that merely says `done`. + */ + doneEvidence: boolean; }): ClosureState { if (!input.contractIssued) return "not_applicable"; if (input.role === "orchestrator") return "not_applicable"; if (input.state !== "done") return "pending"; - return input.closureArtifactVerified === true - ? "verified" - : "artifact_missing"; + if (input.closureArtifactVerified === true) return "verified"; + // AIDEV-NOTE (F1b round 3): `artifact_missing` is not a description, it is an + // ALARM -- P11's table reads it as "route a reviewer NOW". Firing it takes + // positive evidence that the work ended, because #408 flips live records to + // `done` on its own: five agents were observed rendering `artifact_missing` + // while sitting at a fresh `ready` prompt, one of them spawned two minutes + // earlier. A record that flipped is not a task that finished, and the + // difference is exactly this evidence. + if (!input.doneEvidence) return "pending"; + return "artifact_missing"; } // --------------------------------------------------------------------------- diff --git a/tests/coordination-paths.test.ts b/tests/coordination-paths.test.ts index ae89bcae..d9735d6c 100644 --- a/tests/coordination-paths.test.ts +++ b/tests/coordination-paths.test.ts @@ -112,7 +112,9 @@ describe("P11 boot footer (Constraint 1: <=2 short lines, bytes declared)", () = }); describe("P11 closure state (Constraint 3: no bare boolean at default detail)", () => { - const issued = { contractIssued: true }; + // A genuinely finished worker: the contract was issued AND something observed + // the task ending. `doneEvidence:false` is its own case below. + const issued = { contractIssued: true, doneEvidence: true }; it("done + verified artifact => verified", () => { expect( @@ -161,12 +163,37 @@ describe("P11 closure state (Constraint 3: no bare boolean at default detail)", expect(deadlocked).not.toBe(working); }); + it("F1b: done WITHOUT done evidence => pending, never the artifact_missing alarm", () => { + // #408 flips live records to `done` on its own. `artifact_missing` means + // "route a reviewer NOW", so a record flip must not be able to fire it. + expect( + resolveClosureState({ + contractIssued: true, + state: "done", + closureArtifactVerified: false, + doneEvidence: false, + }), + ).toBe("pending"); + }); + + it("F1b: a verified artifact stands on its own, evidence channel or not", () => { + expect( + resolveClosureState({ + contractIssued: true, + state: "done", + closureArtifactVerified: true, + doneEvidence: false, + }), + ).toBe("verified"); + }); + it("no contract issued => not_applicable, never a falsey negative", () => { expect( resolveClosureState({ contractIssued: false, state: "done", closureArtifactVerified: null, + doneEvidence: true, }), ).toBe("not_applicable"); }); diff --git a/tests/f1-live-state-truth.test.ts b/tests/f1-live-state-truth.test.ts index c58febca..b9dc18a9 100644 --- a/tests/f1-live-state-truth.test.ts +++ b/tests/f1-live-state-truth.test.ts @@ -395,6 +395,11 @@ describe("F1 — live state, not the stale registry record", () => { state: "done", report_path: join(TEST_DIR, "reports", "missing.md"), done_marker: "### @cmuxlayerCodex-finished DONE", + // F1b round 3: this worker EARNED its done -- a done signal was + // detected on its screen. Without that, the fixture is + // indistinguishable from a #408 record flip, and the sibling test + // below is what that shape must produce. + task_done_detected_at: "2026-08-19T10:05:00.000Z", } as Partial as any), ); @@ -408,4 +413,33 @@ describe("F1 — live state, not the stale registry record", () => { // overturn a recorded done: the deadlock signal has to survive. expect(row.closure).toBe("artifact_missing"); }); + + it("F1b: a fresh agent at a ready prompt whose record flipped done reads pending", async () => { + client.screens["surface:idle"] = [ + "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer", + "codex>", + ].join("\n"); + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-flipped", + surface_id: client.idleSurface, + state: "done", + report_path: join(TEST_DIR, "reports", "missing.md"), + done_marker: "### @cmuxlayerCodex-flipped DONE", + } as Partial as any), + ); + + const result = await callTool(server, "list_agents", { detail: "full" }); + const parsed = parseResult(result); + const row = parsed.agents.find( + (agent: any) => agent.agent_id === "cmuxlayerCodex-flipped", + ); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + // Same screen, same missing report — and NOTHING ever observed this task + // ending. The row's own `state` says the agent is at a live prompt, so the + // closure beside it may not say the work is over and unaccounted for. + expect(row.state.value).toBe("ready"); + expect(row.closure).toBe("pending"); + }); }); diff --git a/tests/f1b-wait-for-watch-live-state.test.ts b/tests/f1b-wait-for-watch-live-state.test.ts index 77c577b3..c2657c05 100644 --- a/tests/f1b-wait-for-watch-live-state.test.ts +++ b/tests/f1b-wait-for-watch-live-state.test.ts @@ -7,7 +7,10 @@ import { AgentRegistry } from "../src/agent-registry.js"; import type { AgentRecord } from "../src/agent-types.js"; import type { CmuxClient } from "../src/cmux-client.js"; import type { LiveAgentState } from "../src/live-agent-state.js"; -import { resolveLiveAgentState } from "../src/live-agent-state.js"; +import { + resolveLiveAgentState, + screenConfirmedAgentState, +} from "../src/live-agent-state.js"; import { StateManager } from "../src/state-manager.js"; import { WatchArmError, readWatchRegistry } from "../src/watch-spec.js"; import type { CmuxSurface } from "../src/types.js"; @@ -498,10 +501,10 @@ describe("F1b round 2 — the wait buys its own evidence instead of hoping the c const record = engine.getAgentState("voicelayerClaude-2ac0d960"); if (!record) throw new Error("fixture record missing"); - // Cold, before the wait: no evidence, so closure honestly reads the record. - expect(engine.assessHarvestability(record).closure).toBe( - "artifact_missing", - ); + // Cold, before the wait. Round 3 also made this `pending` -- with no + // evidence anywhere, a bare `done` record may not fire the alarm either -- + // so what this test now pins is the STATE half: the wait's own evidence. + expect(engine.assessHarvestability(record).closure).toBe("pending"); const pending = engine.waitFor("voicelayerClaude-2ac0d960", "idle", 1_500); await vi.advanceTimersByTimeAsync(2_000); @@ -679,3 +682,131 @@ describe("F1b — a watch still refuses when the surface is positively gone", () expect(error?.message).not.toContain("does not exist"); }); }); + + + +/** A fresh agent sitting at a live prompt, waiting for its first instruction. */ +const readyScreenProbe = (agent: AgentRecord): LiveAgentState => + resolveLiveAgentState(agent, { + status: "idle", + agent_type: "claude", + control_state: "ready", + }); + +describe("F1b round 3 — one row, one state rule: closure cannot contradict the state beside it", () => { + let stateMgr: StateManager; + let engine: AgentEngine; + let liveSurfaces: CmuxSurface[]; + + const buildEngine = (): void => { + stateMgr = new StateManager(TEST_DIR); + liveSurfaces = [makeSurface("surface:worker")]; + const registry = new AgentRegistry(stateMgr, async () => liveSurfaces); + engine = new AgentEngine(stateMgr, registry, makeMockClient(), { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + }); + }; + + const contractRecord = (overrides?: Partial): AgentRecord => + makeRecord({ + report_path: join(TEST_DIR, "report.md"), + done_marker: "DONE_WORKER", + ...overrides, + }); + + beforeEach(() => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + buildEngine(); + }); + + afterEach(() => { + vi.useRealTimers(); + engine.dispose(); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("shape 1 — a READY agent whose record flipped done reads pending, not artifact_missing", () => { + // golemsClaude's live specimen: five agents, one spawned two minutes + // earlier, all on v0.4.47 with the F1 fix present. The screen said `ready`, + // the row said `ready`, and the closure beside it said `artifact_missing`. + stateMgr.writeState(contractRecord({ state: "done" })); + const record = stateMgr.readState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + engine.setLiveStateResolver(readyScreenProbe); + + const harvest = engine.assessHarvestability(record); + + expect(harvest.closure).toBe("pending"); + // And it says why, in the field that was already in the payload. + expect(harvest.evidence_channel.done_source).toBe("none"); + }); + + it("shape 2 — a WORKING agent on a cold cache reads pending, not artifact_missing", () => { + // Same alarm, different root: no live evidence at all, so the record's + // bare `done` is the only thing speaking. It still must not fire the alarm. + stateMgr.writeState(contractRecord({ state: "done" })); + const record = stateMgr.readState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + engine.setLiveStateResolver((agent) => resolveLiveAgentState(agent, null)); + + expect(engine.assessHarvestability(record).closure).toBe("pending"); + }); + + it("the deadlock alarm SURVIVES for a worker that earned its done", () => { + // The other half of the contract: a finished worker sits at a ready prompt + // too. With real done evidence and no report, `artifact_missing` must still + // fire -- otherwise this change trades a false alarm for a silent one. + stateMgr.writeState( + contractRecord({ + state: "done", + task_done_detected_at: "2026-08-19T10:05:00.000Z", + }), + ); + const record = stateMgr.readState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + engine.setLiveStateResolver(readyScreenProbe); + + const harvest = engine.assessHarvestability(record); + + expect(harvest.closure).toBe("artifact_missing"); + expect(harvest.evidence_channel.done_source).toBe("screen"); + }); + + it("a screen showing work in progress still outranks an earned done", () => { + // Ordering check: evidence that the agent is working NOW beats evidence + // that it finished earlier — a re-tasked worker is not a deadlocked one. + stateMgr.writeState( + contractRecord({ + state: "done", + task_done_detected_at: "2026-08-19T10:05:00.000Z", + }), + ); + const record = stateMgr.readState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + engine.setLiveStateResolver(workingScreenProbe); + + expect(engine.assessHarvestability(record).closure).toBe("pending"); + }); + + it("closure agrees with the state the same response reports", () => { + // The row publishes `reconciled_state ?? record.state`, derived from + // `screenConfirmedAgentState`. Closure now reads the same value, so the two + // fields of one row cannot disagree about whether the agent is done. + stateMgr.writeState(contractRecord({ state: "done" })); + const record = stateMgr.readState("voicelayerClaude-2ac0d960"); + if (!record) throw new Error("fixture record missing"); + engine.setLiveStateResolver(readyScreenProbe); + + const rowState = + screenConfirmedAgentState({ + status: "idle", + agent_type: "claude", + control_state: "ready", + }) ?? record.state; + + expect(rowState).toBe("ready"); + expect(engine.assessHarvestability(record).closure).toBe("pending"); + }); +}); From 0f1afba51bc053e8f7500618c681d60a4ef982ba Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Thu, 20 Aug 2026 07:11:18 +0300 Subject: [PATCH 4/5] merge(prepared): main (#494) into #478, conflicts resolved Prepared by cmuxlayerCodex-567a9d89, which could not commit or push from its sandbox (read-only shared .git, no DNS). Resolutions, per its report: - src/agent-engine.ts: main's assessHarvestability(agent,{live}) input and its isLiveActive(live) ? live.state : agent.state terminal rule, plus the merged positive-done evidence rule; #478's fresh-probe wait/watch kept. - src/coordination-paths.ts: main's concise form of the same behaviour, keeping the doneEvidence contract and the verified -> artifact_missing -> pending order. - tests/coordination-paths.test.ts: main's expanded fixtures, both polarities. Two real merge regressions fixed (t1b-closure-probe-divergence, sidebar-sync): #478's pre-merge closureStateOf made a ready-screen/record-done worker nonterminal before report verification; switched that one line to main's #488 rule. Co-Authored-By: cmuxlayerCodex-567a9d89 running gpt-5.6-sol Co-Authored-By: cmuxlayerClaude running claude-opus-5 --- .github/workflows/publish.yml | 6 +- README.md | 3 +- docs/control-plane-invariants.md | 21 + package.json | 2 +- scripts/release.sh | 56 +- src/agent-engine.ts | 229 +++- src/agent-facade.ts | 26 +- src/agent-registry.ts | 135 +- src/agent-types.ts | 4 +- src/cmux-client.ts | 21 + src/coordination-paths.ts | 29 +- src/format.ts | 17 - src/key-names.ts | 32 +- src/resume-verification.ts | 143 ++ src/seat-identity.ts | 11 +- src/server.ts | 1381 ++++++++++---------- tests/agent-engine.test.ts | 118 ++ tests/coordination-paths.test.ts | 29 +- tests/delivery-truth-t2.test.ts | 598 +++++++++ tests/f1-live-state-truth.test.ts | 69 +- tests/global-setup.ts | 20 + tests/live-topology-restart.test.ts | 6 +- tests/pre-pr-scripts.test.ts | 19 + tests/ram-watchdog-warn-only.test.ts | 4 +- tests/release-receipts.test.ts | 86 +- tests/resume-verification.test.ts | 149 +++ tests/seat-identity.test.ts | 29 + tests/send-to-v2-background-verify.test.ts | 277 +++- tests/server-agent-tools.test.ts | 20 +- tests/server.test.ts | 13 +- tests/t1-registry-truth.test.ts | 615 +++++++++ tests/t1b-closure-probe-divergence.test.ts | 503 +++++++ tests/t2b-silent-failures.test.ts | 504 +++++++ tests/vitest.setup.ts | 43 + tests/workflow-toolchain.test.ts | 81 ++ vitest.config.ts | 1 + 36 files changed, 4509 insertions(+), 791 deletions(-) create mode 100644 src/resume-verification.ts create mode 100644 tests/delivery-truth-t2.test.ts create mode 100644 tests/global-setup.ts create mode 100644 tests/resume-verification.test.ts create mode 100644 tests/t1-registry-truth.test.ts create mode 100644 tests/t1b-closure-probe-divergence.test.ts create mode 100644 tests/t2b-silent-failures.test.ts create mode 100644 tests/workflow-toolchain.test.ts diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 3d823bbe..4c03f295 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -16,10 +16,14 @@ jobs: - uses: actions/setup-node@v4 with: - node-version: 20 + node-version: 22 cache: npm registry-url: https://registry.npmjs.org + # The suite spawns `bun` (tests/fleet-sidebar.test.ts) and release.sh + # shells out to `bun run`. Without it this job fails on toolchain, not code. + - uses: oven-sh/setup-bun@v2 + - run: npm install --no-package-lock - run: npm run typecheck - run: npm test 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 f73b7f43..9445455c 100644 --- a/docs/control-plane-invariants.md +++ b/docs/control-plane-invariants.md @@ -37,6 +37,27 @@ 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 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/package.json b/package.json index 5da262f7..65cd06fe 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "cmuxlayer", - "version": "0.4.47", + "version": "0.4.48", "description": "Terminal multiplexer MCP server for AI agent workspace orchestration", "type": "module", "main": "./dist/lib.js", diff --git a/scripts/release.sh b/scripts/release.sh index b794f7d8..46846795 100755 --- a/scripts/release.sh +++ b/scripts/release.sh @@ -6,6 +6,7 @@ # scripts/release.sh 0.3.0 --yes # no confirmation prompt # scripts/release.sh 0.3.0 --dry-run # print every step, change nothing # scripts/release.sh 0.3.0 --require-contract # a skipped real-cmux gate aborts the release +# scripts/release.sh 0.3.0 --require-ci # a non-green CI on HEAD aborts the release # # Steps: clean-tree + green build/tests gate → bump package.json → commit + # push main → tag vX.Y.Z + push tag → update formula url+sha256 in the @@ -30,11 +31,15 @@ VERSION="${1:-}" YES=0 DRY=0 REQUIRE_CONTRACT=0 +REQUIRE_CI=0 +CI_CONCLUSION="unknown" +CI_COMMIT_LABEL="HEAD" for arg in "${@:2}"; do case "$arg" in --yes) YES=1 ;; --dry-run) DRY=1 ;; --require-contract) REQUIRE_CONTRACT=1 ;; + --require-ci) REQUIRE_CI=1 ;; *) echo "unknown flag: $arg" >&2; exit 2 ;; esac done @@ -51,6 +56,19 @@ trap cleanup EXIT die() { echo "release: $*" >&2; exit 1; } run() { if [ "$DRY" -eq 1 ]; then printf 'DRY %s\n' "$*"; else eval "$@"; fi; } +# In-place sed that works on BSD *and* GNU. `sed -i ''` is BSD-only: GNU sed +# reads the '' as the script and the expression as a filename, exits 2, and +# takes this script down with it — which is why every Linux CI run of the +# release-receipt tests failed while the same tests passed on a Mac. +# Writes back through the ORIGINAL file rather than mv-ing the tmpfile over it: +# mv would hand the target the tmpfile's 0600 and owner, a mode change `sed -i` +# never makes. +sed_inplace() { + local expression="$1" file="$2" tmp + tmp="$(mktemp)" + sed -E "$expression" "$file" >"$tmp" && cat "$tmp" >"$file" && rm -f "$tmp" +} + # Receipt writes are never allowed to fail a release: the ledger records the # release, it does not gate it. receipt() { @@ -86,6 +104,37 @@ if [ "$DRY" -ne 1 ]; then receipt_record "gates.require_contract" "$([ "$REQUIRE_CONTRACT" -eq 1 ] && echo true || echo false)" fi +# --- CI status of the commit being released (#490) ------------------------- +# Six tagged releases shipped while publish.yml failed on every single run and +# cmuxlayer never reached npm at all. Nothing in the release said so. The receipt +# now carries CI's verdict on the released commit, and the banner prints it, so +# "the release looked clean" can never again mean "nobody opened the log". +if [ "$DRY" -eq 1 ]; then + printf 'DRY %s\n' "read CI status for HEAD" +else + # `gh run list --commit` needs the FULL sha; an abbreviated one matches nothing + # and would read as `unknown`. Never loosen this to a short sha. + RELEASE_COMMIT="$(git rev-parse HEAD)" + CI_COMMIT_LABEL="$RELEASE_COMMIT" + # An unusable gh -- absent, unauthenticated, offline -- reads as unknown. + # Only a real `success` from a real run is allowed to look green. + CI_CONCLUSION="$(gh run list --commit "$RELEASE_COMMIT" --workflow ci.yml \ + --limit 1 --json conclusion --jq '.[0].conclusion' 2>/dev/null || true)" + [ -n "$CI_CONCLUSION" ] || CI_CONCLUSION="unknown" + receipt_record "gates.ci" "$CI_CONCLUSION" + # Name the commit the verdict is ABOUT. The read happens before the version + # bump, so this is the commit the release was cut from -- not the tag's commit. + # In the one file whose purpose is that a release cannot look cleaner than it + # is, "which commit" cannot be left to inference. + receipt_record "gates.ci_commit" "$RELEASE_COMMIT" + if [ "$CI_CONCLUSION" != "success" ]; then + if [ "$REQUIRE_CI" -eq 1 ]; then + die "--require-ci: CI for $RELEASE_COMMIT is $CI_CONCLUSION, not success" + fi + echo "release: WARNING — CI for $RELEASE_COMMIT is $CI_CONCLUSION; recorded in the receipt" + fi +fi + echo "release: gating on typecheck + tests…" run "bun run typecheck" receipt_record "gates.typecheck" "pass" @@ -159,7 +208,7 @@ if [ "$YES" -ne 1 ] && [ "$DRY" -ne 1 ]; then fi # --- bump + commit + tag (cmuxlayer) -------------------------------------- -run "sed -i '' -E 's/^( \"version\": \")[^\"]+(\",)\$/\\1$VERSION\\2/' package.json" +run "sed_inplace 's/^( \"version\": \")[^\"]+(\",)\$/\\1$VERSION\\2/' package.json" run "git commit -aqm 'chore: release $TAG'" run "git push origin main" run "git tag -a '$TAG' -m 'cmuxlayer $TAG'" @@ -189,8 +238,8 @@ receipt_record "artifact.url" "$URL" receipt_record "artifact.sha256" "$SHA" # --- bump formula (homebrew-layers) --------------------------------------- -run "sed -i '' -E 's|archive/refs/tags/v[0-9]+\.[0-9]+\.[0-9]+\.tar\.gz|archive/refs/tags/$TAG.tar.gz|' '$FORMULA'" -run "sed -i '' -E 's|^ sha256 \"[0-9a-f]{64}\"| sha256 \"$SHA\"|' '$FORMULA'" +run "sed_inplace 's|archive/refs/tags/v[0-9]+\.[0-9]+\.[0-9]+\.tar\.gz|archive/refs/tags/$TAG.tar.gz|' '$FORMULA'" +run "sed_inplace 's|^ sha256 \"[0-9a-f]{64}\"| sha256 \"$SHA\"|' '$FORMULA'" run "brew audit etanhey/layers/cmuxlayer || true" run "git -C '$TAP_DIR' commit -aqm 'cmuxlayer $TAG'" run "git -C '$TAP_DIR' push origin main" @@ -246,6 +295,7 @@ fi cat < { @@ -1897,8 +1926,8 @@ export class AgentEngine { // on an agent mid-turn. A `ready` prompt cannot overturn done (a finished // worker sits at one too), and a dead/shell pane must not either: there // the record's `done` plus the missing artifact IS the story. - const live = this.liveStateOf(agent); - const effectiveState = this.closureStateOf(agent, live); + const live = opts?.live ?? this.liveStateOf(agent); + const effectiveState = isLiveActive(live) ? live.state : agent.state; const neutralEvidenceChannel: HarvestabilityEvidenceChannel = { done_source: agent.task_done_detected_at ? "screen" : "none", degraded: false, @@ -1926,8 +1955,7 @@ export class AgentEngine { Boolean(agent.report_path && agent.done_marker) || Boolean(agent.goal_file), closureArtifactVerified: null, - // This branch is only reached for a non-done state (or an - // orchestrator), where the evidence question does not arise. + // Unreachable as a deadlock claim: this branch is not `done`. doneEvidence: false, }), closure_artifact_verified: null, @@ -1974,6 +2002,15 @@ export class AgentEngine { reportText !== null && reportFresh === true && reportFinalLine === goal.doneMarker; + // AIDEV-NOTE (T1b/#488): the POSITIVE done evidence `artifact_missing` + // now requires. `evidence_channel.done_source` is already the engine's + // answer to "what saw this agent finish" -- `screen` from + // task_done_detected_at, `transcript` from the harness JSONL -- and a + // screen that itself reads `done` counts. `none` means the only thing + // claiming done is the record, which #408 writes without observing + // anything. + const doneEvidence = + evidenceChannel.done_source !== "none" || live.screen_state === "done"; const keptOpen = reportText ? this.extractKeptOpenContract(reportText) : null; @@ -2048,9 +2085,7 @@ export class AgentEngine { role, contractIssued: Boolean(goal.reportPath && goal.doneMarker), closureArtifactVerified, - // The channel this payload already reports: `none` means nothing ever - // observed this task ending, so the `done` came from the record alone. - doneEvidence: evidenceChannel.done_source !== "none", + doneEvidence, }), closure_artifact_verified: closureArtifactVerified, report_path: goal.reportPath, @@ -5902,7 +5937,32 @@ export class AgentEngine { this.clearAgentLifecycleMemory(initialAgentId); continue; } - const harvestability = this.assessHarvestability(agent); + // AIDEV-NOTE (T1b/#488): the sweep is the third emitter -- its + // harvestability feeds the health input, the sidebar row's `report=` and + // the done notification, beside a state derived from the screen it reads + // below. When the done-detection pass already has this agent's screen + // text in hand, closure resolves from THAT rather than from the discovery + // cache, which may be cold on this path too. No new read: when there is + // no screen text, the injected probe is used exactly as before. + // The done-detection pass returns early for a record already at `done` + // -- exactly #488's shape -- so its screen text is absent precisely when + // closure needs it. `readSweepScreen` memoizes on `sweepCtx`, which the + // health input below reuses for this same agent, so this shares that + // read rather than adding one. + let sweepScreenText = taskDoneResult.screenText; + if (sweepScreenText === undefined) { + try { + sweepScreenText = (await this.readSweepScreen(agent, sweepCtx)).text; + } catch { + // No screen is no evidence; the injected probe answers as before. + } + } + const harvestability = this.assessHarvestability(agent, { + live: + sweepScreenText === undefined + ? null + : resolveLiveAgentState(agent, parseScreen(sweepScreenText)), + }); const healthScreenContexts = new Map(); let screenCurrentAction: string | null = null; const healthScreenContextFor = ( @@ -7067,36 +7127,34 @@ export class AgentEngine { let snapshot: DeliveryVerifySnapshot | null | undefined; if (this.deliverySnapshotReader) { if (!snapshots.has(snapshotKey)) { + // AIDEV-NOTE (T2 #450): the snapshot read must be inside the + // hang guard, not before it. SF8 hoisted the surface read out of + // the verifier and awaited it OUTSIDE SF7's race; the CLI + // fallback path has no subprocess timeout, so one wedged `cmux` + // held deliveryVerifyInFlight forever and every later verify + // pass short-circuited -- exactly the stall SF7 exists to + // prevent. A timed-out read yields a null snapshot, which the + // verifier already treats as "no evidence, stay pending". snapshots.set( snapshotKey, - await this.deliverySnapshotReader(receipt), + await this.withDeliveryVerifyTimeout( + this.deliverySnapshotReader(receipt), + "Delivery snapshot read", + ).catch(() => null), ); } snapshot = snapshots.get(snapshotKey) ?? null; } - let timeout: ReturnType | null = null; try { - observation = await Promise.race([ + observation = await this.withDeliveryVerifyTimeout( this.deliveryVerifier(receipt, snapshot), - new Promise((_resolve, reject) => { - timeout = setTimeout( - () => - reject( - new Error( - `Delivery verify timed out after ${this.deliveryVerifyTimeoutMs}ms`, - ), - ), - this.deliveryVerifyTimeoutMs, - ); - }), - ]); + "Delivery verify", + ); } catch (error) { observation = { outcome: "pending", reason: error instanceof Error ? error.message : String(error), }; - } finally { - if (timeout) clearTimeout(timeout); } receipt.verify_last_attempt_at = new Date().toISOString(); this.persistDeliveryReceipts(); @@ -7146,6 +7204,30 @@ export class AgentEngine { } } + /** Bound one delivery-verify side quest to the verify timeout. */ + private withDeliveryVerifyTimeout( + work: Promise, + label: string, + ): Promise { + let timeout: ReturnType | null = null; + return Promise.race([ + work, + new Promise((_resolve, reject) => { + timeout = setTimeout( + () => + reject( + new Error( + `${label} timed out after ${this.deliveryVerifyTimeoutMs}ms`, + ), + ), + this.deliveryVerifyTimeoutMs, + ); + }), + ]).finally(() => { + if (timeout) clearTimeout(timeout); + }); + } + private verifyReadIntervalMs( receipt: AgentDeliveryReceipt, now: number, @@ -7176,6 +7258,35 @@ export class AgentEngine { return since > 0 && since < this.verifyReadIntervalMs(receipt, now); } + /** + * A confirmed-failure verdict is worth an issue only when something was + * actually observed to go wrong with the message. + * + * AIDEV-NOTE (T2 #471/#443): `verify_deadline_elapsed` means the ENGINE + * stopped looking, and `target_gone` means there was nothing left to look + * at. Neither is evidence the message was lost, and auto-filing them + * produced issues #471 and #443 -- tracker noise describing cmuxlayer's own + * timers, not a defect. The local evidence ticket is still written either + * way, so the verdict keeps citing its evidence; only the escalation stops. + */ + private deliveryFailureEscalationDecline( + reason: string, + ): string | null { + if (reason === "verify_deadline_elapsed") { + return ( + "background verify ran out of deadline before observing an outcome; " + + "no evidence the message was lost" + ); + } + if (reason === "target_gone") { + return ( + "the target agent disappeared before an outcome could be observed; " + + "no evidence the message was lost" + ); + } + return null; + } + private async fileConfirmedFailureTicket( receipt: AgentDeliveryReceipt, reason: string, @@ -7210,13 +7321,39 @@ export class AgentEngine { dir: ticketDir, }); receipt.ticket_filed = true; - this.persistDeliveryReceipts(); - if (!written.created) return; - if (!this.deliveryIssueFiler) return; + + // AIDEV-NOTE (T2 B2): both fields are written from the SAME resolved + // outcome, at every exit, and never before the escalation is known. + // Stamping `escalated: true` up front and then returning early -- deduped + // signature, no filer configured, filer threw -- left receipts asserting + // an escalation that never happened. That is this lane's own disease: a + // receipt reporting something the engine did not observe. + const settleEscalation = (declined: string | null): void => { + receipt.ticket_escalated = declined === null; + receipt.ticket_escalation_declined_reason = declined; + this.persistDeliveryReceipts(); + }; + + const declineReason = this.deliveryFailureEscalationDecline(reason); + if (declineReason !== null) return settleEscalation(declineReason); + if (!written.created) { + return settleEscalation( + "an issue for this failure signature was already filed; " + + "this occurrence was appended to the existing ticket", + ); + } + if (!this.deliveryIssueFiler) { + return settleEscalation("no issue filer is configured"); + } try { await this.deliveryIssueFiler(ticket); - } catch { - // Local ticket is authoritative; GitHub is best-effort. + settleEscalation(null); + } catch (error) { + // Local ticket is authoritative; GitHub is best-effort -- but the + // receipt must say the escalation did not land. + settleEscalation( + `issue filer failed: ${error instanceof Error ? error.message : String(error)}`, + ); } } @@ -7237,9 +7374,36 @@ export class AgentEngine { this.appendDeliveryReceiptEventBestEffort(receipt); continue; } + // AIDEV-NOTE (T2 N1): a paused target deliberately does NOT age out. + // #467's bounded lifetime exists for a target that is failing to + // become interactive on its own; pausing is a human's resumable act, + // and expiring queued work under it would discard the message the + // pause was protecting. The receipt stays nonterminal, and the + // paused-target WARNING already tells the caller it is not delivered. if (agent.paused === true) { continue; } + // AIDEV-NOTE (T2 #467): a retryable refusal is nonterminal, but it is + // not unbounded. Without this, a target stuck `booting` retried behind + // a 30s-capped backoff forever and the caller's receipt never resolved + // -- a lead could wait on it indefinitely. The lifetime is stamped on + // the first retryable requeue below; when it elapses the caller gets a + // terminal answer that cites the gate reason that kept refusing. + if ( + receipt.queue_deadline_at && + Date.now() >= Date.parse(receipt.queue_deadline_at) + ) { + const gateReason = receipt.error ?? "no gate reason recorded"; + receipt.delivery_state = "failed_confirmed"; + receipt.terminal = true; + receipt.submit_verified = false; + receipt.resolved_at = new Date().toISOString(); + receipt.next_attempt_at = null; + receipt.error = `queue_deadline_elapsed after ${receipt.retry_count} retryable refusals; last gate reason: ${gateReason}`; + this.persistDeliveryReceipts(); + this.appendDeliveryReceiptEventBestEffort(receipt); + continue; + } if ( receipt.next_attempt_at && Date.parse(receipt.next_attempt_at) > Date.now() @@ -7305,6 +7469,9 @@ export class AgentEngine { if (error instanceof RetryableDeliveryError) { receipt.submission_started_at = null; receipt.retry_count += 1; + receipt.queue_deadline_at ??= new Date( + Date.now() + this.deliveryQueueDeadlineMs, + ).toISOString(); const backoffMs = Math.min( 30_000, 250 * 2 ** Math.min(receipt.retry_count - 1, 16), 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..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; @@ -105,6 +106,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 — 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. + */ +export const UNCLAIMED_SURFACE_EVICTION_CONFIRMATION_MS = 60_000; + export interface AgentFilter { state?: AgentState; repo?: string; @@ -501,6 +519,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 +673,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 +777,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 +937,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 +1042,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 +1266,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 +1658,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 +1728,7 @@ export class AgentRegistry { return []; } if (!hasCoherentSurfaceIdentity(surfaces)) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return []; } @@ -1704,10 +1739,40 @@ 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)) { + // 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); + if (removedUnclaimedId) { + evicted.push(removedUnclaimedId); + } continue; } if (!this.isSurfaceAbsenceAuthoritative(agent, surfaces)) { @@ -1874,6 +1939,52 @@ export class AgentRegistry { return now - observation.firstObservedAt >= confirmationMs; } + /** + * #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 + * discovery on the next call; marking a live agent `error` would not + * self-correct. + */ + private isUnclaimedAbsenceConfirmed( + agent: AgentRecord, + opts: { now?: number }, + ): boolean { + const surfaceKey = this.agentSurfaceKey(agent); + 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) { + 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 +2006,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 +2019,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 +2037,7 @@ export class AgentRegistry { !hasBijectiveDiscoveryIdentity(discovered) || hasMixedDiscoveryIdentityCoverage(discovered) ) { - this.surfacelessObservations.clear(); + this.clearAbsenceObservations(); return { repaired: [], evicted: [], skipped: [] }; } const repaired: RegistryRepairEntry[] = []; @@ -2463,7 +2584,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/cmux-client.ts b/src/cmux-client.ts index 2e2408bb..6bae8acc 100644 --- a/src/cmux-client.ts +++ b/src/cmux-client.ts @@ -28,6 +28,16 @@ import { parseCmuxStatusFrame } from "./cmux-status-frame.js"; import { isCmuxAccessControlDenied } from "./cmux-access-control.js"; const execFileAsync = promisify(execFile); +/** + * Hard ceiling on one CLI-fallback `cmux` invocation. + * + * AIDEV-NOTE (T2 #450): the socket transport is bounded by its own + * REQUEST_TIMEOUT_MS, but this fallback had none -- a wedged `cmux` + * subprocess held its caller (notably the delivery snapshot read) forever. + * Matches the socket client's 10s budget so neither transport can outlast the + * other. + */ +export const CMUX_CLI_EXEC_TIMEOUT_MS = 10_000; const STANDARD_BUNDLED_CMUX = "/Applications/cmux.app/Contents/Resources/bin/cmux"; @@ -55,6 +65,8 @@ interface CmuxClientOptions { bin?: string; env?: NodeJS.ProcessEnv; existsSync?: (path: string) => boolean; + /** Hard ceiling on one CLI-fallback exec; defaults to CMUX_CLI_EXEC_TIMEOUT_MS. */ + execTimeoutMs?: number; } interface CmuxIdentifyResult { @@ -75,6 +87,7 @@ export class CmuxClient { private bin?: string; private env?: NodeJS.ProcessEnv; private existsSync: (path: string) => boolean; + private execTimeoutMs: number; private observerTransportGeneration = 0; constructor(opts?: CmuxClientOptions) { @@ -82,6 +95,10 @@ export class CmuxClient { this.bin = opts?.bin; this.env = opts?.env; this.existsSync = opts?.existsSync ?? fs.existsSync; + this.execTimeoutMs = Math.max( + 1, + opts?.execTimeoutMs ?? CMUX_CLI_EXEC_TIMEOUT_MS, + ); } setEnv(env: NodeJS.ProcessEnv | undefined): void { @@ -121,6 +138,10 @@ export class CmuxClient { ) : await execFileAsync(bin, cliArgs, { ...(env ? { env } : {}), + timeout: this.execTimeoutMs, + // N2: a subprocess that ignores SIGTERM would still hang the + // promise the timeout exists to bound. + killSignal: "SIGKILL", }); return stdout; } catch (error) { diff --git a/src/coordination-paths.ts b/src/coordination-paths.ts index f8051ea5..08cf97fd 100644 --- a/src/coordination-paths.ts +++ b/src/coordination-paths.ts @@ -121,8 +121,15 @@ export function coordinationFooterBytes(contract: CoordinationContract): number * into one falsey value, the same hazard `paused` shipped with in v0.4.41 and * that v0.4.42 fixed by attaching provenance (types.ts pauseHonestyFields). * - * Invariant: `artifact_missing` is reachable ONLY from state "done", so it is - * an actionable deadlock signal on its own -- no cross-referencing required. + * Invariant: `artifact_missing` is reachable ONLY from state "done" AND from + * POSITIVE done evidence, so it is an actionable deadlock signal on its own -- + * no cross-referencing required. + * + * AIDEV-NOTE (T1b/#488): the second half of that invariant is the fix. A bare + * registry flip to `done` (#408 does this within minutes, with nothing having + * observed a done) used to be enough, so healthy mid-work children -- one of + * them two minutes old -- rendered the "route a reviewer NOW" signal. Absence + * of done evidence is `pending`, never a deadlock claim. */ export type ClosureState = | "verified" @@ -136,25 +143,19 @@ export function resolveClosureState(input: { contractIssued: boolean; closureArtifactVerified: boolean | null; /** - * Positive evidence the task actually ENDED -- a done signal detected on the - * screen or in the harness transcript -- as opposed to a registry record - * that merely says `done`. + * Something OBSERVED this agent finish: a done marker on screen + * (`task_done_detected_at`), a harness transcript that ended, or a verified + * report. Required -- not optional -- so every call site has to say which + * evidence it has rather than inheriting the record's word for it. */ doneEvidence: boolean; }): ClosureState { if (!input.contractIssued) return "not_applicable"; if (input.role === "orchestrator") return "not_applicable"; if (input.state !== "done") return "pending"; + // A verified artifact IS positive done evidence, so it is checked first. if (input.closureArtifactVerified === true) return "verified"; - // AIDEV-NOTE (F1b round 3): `artifact_missing` is not a description, it is an - // ALARM -- P11's table reads it as "route a reviewer NOW". Firing it takes - // positive evidence that the work ended, because #408 flips live records to - // `done` on its own: five agents were observed rendering `artifact_missing` - // while sitting at a fresh `ready` prompt, one of them spawned two minutes - // earlier. A record that flipped is not a task that finished, and the - // difference is exactly this evidence. - if (!input.doneEvidence) return "pending"; - return "artifact_missing"; + return input.doneEvidence ? "artifact_missing" : "pending"; } // --------------------------------------------------------------------------- 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/key-names.ts b/src/key-names.ts index 090e0ed4..7e0b35ee 100644 --- a/src/key-names.ts +++ b/src/key-names.ts @@ -1,13 +1,41 @@ const CTRL_C_ALIASES = new Set(["c-c", "ctrl-c", "ctrl+c", "^c"]); +// AIDEV-NOTE (#484): every one of these means "submit" to the target CLI, but +// the delivery engine used to test `key === "return"` by exact match. A lead +// pressing "Enter" therefore got a receipt claiming submit_attempted:false for +// a submit that really was dispatched — a success receipt whose own fields said +// nothing had been attempted. This set exists to make the RECEIPT truthful; it +// deliberately does NOT rewrite the key handed to cmux. Raw "\r"/"\n" are not +// listed: a newline is how a composer expresses shift+enter, so treating it as +// a submit would claim an attempt the caller did not make. +const SUBMIT_KEY_ALIASES = new Set([ + "return", + "enter", + "kpenter", + "kp_enter", + "kp-enter", + "c-m", + "ctrl-m", + "ctrl+m", + "^m", +]); + +function canonicalKeyToken(key: string): string { + return key.trim().toLowerCase().replace(/\s+/g, ""); +} + +/** True when this key name submits the composer on the target CLI. */ +export function isSubmitKey(key: string): boolean { + return SUBMIT_KEY_ALIASES.has(canonicalKeyToken(key)); +} + export function normalizeKeyName(key: string): string { const trimmed = key.trim(); if (!trimmed) { return trimmed; } - const normalized = trimmed.toLowerCase().replace(/\s+/g, ""); - if (CTRL_C_ALIASES.has(normalized)) { + if (CTRL_C_ALIASES.has(canonicalKeyToken(trimmed))) { return "ctrl-c"; } 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/seat-identity.ts b/src/seat-identity.ts index 917fac8b..7853f849 100644 --- a/src/seat-identity.ts +++ b/src/seat-identity.ts @@ -112,7 +112,16 @@ export function parseSeatRegistryConfig(raw: string): SeatRegistry { ); } -export function defaultSeatRegistryPath(): string { +/** + * The seat registry is a MACHINE file: it says which seats this operator runs. + * `CMUXLAYER_SEAT_REGISTRY_PATH` lets a caller — a test, a sandbox, a second + * fleet — state its own registry instead of inheriting whatever the host has. + */ +export function defaultSeatRegistryPath( + env: NodeJS.ProcessEnv = process.env, +): string { + const override = env.CMUXLAYER_SEAT_REGISTRY_PATH?.trim(); + if (override) return override; return join(homedir(), ".golems", "config.yaml"); } diff --git a/src/server.ts b/src/server.ts index f2a66686..1d7badec 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, @@ -210,7 +207,7 @@ import type { ControlMode, ParsedScreenResult, } from "./types.js"; -import { normalizeKeyName } from "./key-names.js"; +import { isSubmitKey, normalizeKeyName } from "./key-names.js"; import { currentCallerContext, type CallerContext } from "./caller-context.js"; import { CLI_INPUT_PROMPT_PREFIXES, @@ -474,6 +471,10 @@ export const SEND_INPUT_MAX_INLINE_CHARS = parseMaxInlineChars( DEFAULT_SEND_INPUT_MAX_INLINE_CHARS, ); const SEND_INPUT_SUBMIT_VERIFY_POLL_MS = 100; +// A bare submit key either takes effect on the next render or it did not take +// effect at all -- there is no chunked typing to wait out, so the key path uses +// a much shorter verification window than the text path (#484). +const SEND_KEY_SUBMIT_VERIFY_TIMEOUT_MS = 1500; // Busy relays are interjections into an already-running UI. Observe several // repaint frames, accept a correlated TUI queue, and bound exact-composer // recovery so fleet fan-out does not inherit the general 5s timeout. @@ -530,7 +531,12 @@ const SendToArgsSchema = z.object({ surface: z.string().optional(), text: z.string().optional(), command: z.string().optional(), - key: z.string().optional(), + key: z + .string() + .optional() + .describe( + 'Key name for mode="key". Submit aliases (return, enter, KPEnter, ctrl-m, a raw CR) are normalized to "return" and verified: the receipt reports key_dispatched, and a submit that leaves the composer populated fails instead of returning ok.', + ), workspace: z.string().optional(), chunk_size: z.number().int().min(1).optional().default(200), background: z.boolean().optional().default(false), @@ -955,6 +961,7 @@ export function buildPublicDeliveryReceipt(input: { evidencedState === "submitted" || evidencedState === "failed" || evidencedState === "failed_confirmed"; + const warning = input.WARNING ?? defaultNonDeliveryWarning(evidencedState); return { delivered: evidencedState === "submitted", terminal, @@ -966,10 +973,42 @@ export function buildPublicDeliveryReceipt(input: { ? { delivery: evidencedState, delivery_state: evidencedState } : {}), ...(input.delivery_id ? { delivery_id: input.delivery_id } : {}), - ...(input.WARNING ? { WARNING: input.WARNING } : {}), + ...(warning ? { WARNING: warning } : {}), }; } +/** + * One plain-language line a caller cannot honestly quote as "sent". + * + * AIDEV-NOTE (T2 #445): `ok:true` with `delivered:false` was routinely read as + * success -- a lead's own words: "I treated the first as evidence of the + * second." The booleans two levels down were correct and still misread, so the + * receipt now says it in words at the top level. Explicit callers keep their + * own WARNING (the paused-target line is more specific than this default). + */ +function defaultNonDeliveryWarning( + state: PublicDeliveryState | undefined, +): string | undefined { + switch (state) { + case "pending_verify": + case "queued": + case "queued_followup": + return ( + `NOT DELIVERED YET — state ${state}: the message has not been observed ` + + "to land. It resolves in the background; do not relay as sent. " + + "Query wait_for({delivery_id}) for the terminal outcome." + ); + case "failed": + case "failed_confirmed": + return ( + `NOT DELIVERED — terminal failure (${state}). The message did not ` + + "land and will not be retried; do not relay as sent." + ); + default: + return undefined; + } +} + export function pausedTargetWarning(source: string): string { return ( `WARNING — target pane is paused (source: ${source}) and cannot act. ` + @@ -1016,11 +1055,45 @@ class DeliveryError extends Error { } } +/** + * Why a submit could not be verified. This is the sentence a receipt shows the + * fleet when a delivery did not land, so it is spelled out rather than nested: + * the order is "what we saw" before "what we required", most specific first. + */ +function resolveSubmitVerificationFailureReason(observed: { + sawPendingInput: boolean; + sawReadableScreen: boolean; + sawBlankScreen: boolean; + bootConsumptionRefuted: boolean; + requireWorkingStatus: boolean; +}): SubmitVerificationFailureReason { + if (observed.sawPendingInput) return "input_still_pending"; + if (!observed.sawReadableScreen) { + return observed.sawBlankScreen + ? "surface_screen_empty" + : "surface_read_unavailable"; + } + if (observed.bootConsumptionRefuted) return "consumption_not_observed"; + if (observed.requireWorkingStatus) return "working_status_not_observed"; + return "submit_evidence_absent"; +} + type SubmitVerificationFailureReason = | "surface_read_unavailable" | "surface_screen_empty" | "input_still_pending" | "working_status_not_observed" + | "consumption_not_observed" + | "submit_evidence_absent"; + +/** + * Why a bare submit-key dispatch could not be confirmed. A key send carries no + * text, so the text path's evidence (does the screen still show what we typed?) + * does not apply; the composer region itself is the evidence. + */ +type SubmitKeyVerificationReason = + | "surface_read_unavailable" + | "composer_still_populated" | "submit_evidence_absent"; class SubmitVerificationError extends Error { @@ -2394,6 +2467,81 @@ function screenShowsPendingInput( ); } +/** + * The text sitting on the composer's OWN input line, or null when no composer + * prompt line is on screen. + * + * AIDEV-NOTE (T2 #442/B1): deliberately NOT `extractComposerInputRegion`. That + * one appends every following line until it recognises a chrome line, so any + * footer missing from `isComposerFooterOrChromeLine` -- `? for shortcuts`, + * `accept edits on`, `Working (2s * esc to interrupt)` -- reads as composer + * content. That is fine for its own callers, which ask "is the composer + * CLEAR", where a false non-empty just withholds submit evidence. It is not + * fine for the draft guard, where a false non-empty REFUSES a ready pane. So + * the guard reads only the prompt line: a human draft always begins there, + * and chrome never does. Widening the chrome whitelist instead is what put + * this hole in the first place. + */ +function composerPromptLineInput(screenText: string): string | null { + const lines = normalizeTerminalText(screenText).split("\n"); + const cli = inferComposerCli(screenText); + const start = currentComposerRegionStart(cli, lines); + let end = lines.length; + while (end > start && isComposerFooterOrChromeLine(lines[end - 1] ?? "")) { + end -= 1; + } + + for (let index = end - 1; index >= start; index -= 1) { + const line = lines[index] ?? ""; + const match = + matchComposerPromptLine(line) ?? matchLegacyClaudePromptLine(cli, line); + if (match) { + return normalizeKnownPlaceholderComposerInput(cli, match.input.trim()); + } + } + return null; +} + +/** + * True when the target composer holds text that this delivery did not put + * there -- a human's half-written draft, or an earlier message still unflushed. + * + * AIDEV-NOTE (T2 #442): a partially-typed payload (chunk 1 landed, chunk 2 + * pending) IS ours and must stay deliverable, so the line content is compared + * whitespace-insensitively against the payload rather than required to be + * empty. + */ +function composerHoldsForeignDraft( + screenText: string, + submittedText: string, +): boolean { + // AIDEV-NOTE (T2 #442): Cursor is deliberately exempt. Its composer RETAINS + // the accepted text after a submit (the "retained composer" state #441/#449 + // built evidence rules around), so a non-empty Cursor composer is the normal + // post-send screen, not an unsent draft -- and nothing on that screen + // distinguishes the two. Guarding it would refuse every legitimate second + // send to a Cursor pane. Claude and Codex clear on submit, so there a + // non-empty composer really does mean somebody's text is sitting unsent. + if (inferComposerCli(screenText) === "cursor") { + return false; + } + // No recognisable composer prompt line (bare shell, unreadable frame). The + // pre-existing gates own those cases; do not invent a refusal here. + const promptLine = composerPromptLineInput(screenText); + if (promptLine === null) { + return false; + } + const compactDraft = promptLine.replace(/\s+/g, ""); + if (!compactDraft) { + return false; + } + const compactPayload = submittedText.replace(/\s+/g, ""); + if (compactPayload.length === 0) { + return true; + } + return !compactPayload.includes(compactDraft); +} + function stripCodexQueueGutter(line: string): string { return line.replace(/^\s*[│┃║┆┊]\s?/, "").trimEnd(); } @@ -2756,6 +2904,7 @@ function parseRawSubmitEvidenceMetrics( export const __submitEvidenceTestHooks = { extractComposerInputRegion, screenShowsPendingInput, + composerHoldsForeignDraft, }; function hasRawSubmitEvidenceIncrease( @@ -3621,23 +3770,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 ); }; @@ -4674,11 +4841,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { } }; - const assertDeliveryTargetIsSafe = async ( - surface: string, - workspace?: string, - cli?: CliType, - ): Promise<{ text: string; parsed: ParsedScreenResult } | null> => { + const assertDeliveryTargetIsSafe = async (opts: { + surface: string; + workspace?: string; + cli?: CliType; + /** When set, also refuse a composer already holding someone else's text. */ + draftGuardText?: string; + }): Promise<{ text: string; parsed: ParsedScreenResult } | null> => { + const { surface, workspace, cli } = opts; const snapshot = await readParsedSurface(surface, workspace, { throwOnSurfaceGone: true, }); @@ -4700,6 +4870,32 @@ export function createServer(opts?: CreateServerOptions): McpServer { ); } + // AIDEV-NOTE (T2 #442): a composer that already holds text nobody in this + // delivery wrote is a human (or another agent) mid-draft. Typing into it + // concatenates, and the Return that follows SUBMITS their words. The + // picker/permission gates above never covered this: the screen is a + // perfectly ordinary ready composer, it just is not empty. Refuse before + // the first keystroke -- a refused send is recoverable, a submitted draft + // is not. + // + // RETRYABLE, not terminal (T2 B1a): the same screen is produced by this + // delivery's OWN unflushed prior message (the queued_followup shape), and + // the guard cannot tell that from a human draft. For both, the right + // answer is "wait for the composer to flush", which the v2 queue already + // expresses -- and #467's bounded queue lifetime guarantees the caller + // still gets a terminal answer if it never does. A terminal `failed` here + // would be this PR's own disease: a verdict the engine did not observe. + if ( + opts.draftGuardText !== undefined && + composerHoldsForeignDraft(snapshot.text, opts.draftGuardText) + ) { + throw new RetryableDeliveryError( + "target composer already holds text this delivery did not write; " + + "refused to type (typing + Return would submit or mutate it). " + + "Delivery stays queued until the composer is clear.", + ); + } + return snapshot; }; @@ -4780,6 +4976,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { let retriedAt: number | null = null; let sawReadableScreen = false; let sawBlankScreen = false; + let lastBootConsumptionRefuted = false; const screenIncludesSubmittedText = (screenText: string): boolean => { const trimmed = opts.text.trim(); if (!trimmed) { @@ -4842,6 +5039,18 @@ export function createServer(opts?: CreateServerOptions): McpServer { delivery: "queued_followup", }; } + // AIDEV-NOTE (T2 #427): `0 tokens` is a definitive negative. An agent + // handed a prompt that has consumed nothing did not receive it, whatever + // the composer looks like -- a slow boot can render a working-looking + // banner while the CLI is still initialising, and that race produced + // `submit_verified: true` receipts for prompts that never left the + // composer. A NULL token count stays inconclusive on purpose: several + // CLIs never report one, and treating unknown as zero would turn this + // guard into a fleet-wide false negative. + const bootConsumptionRefuted = + opts.require_working_status === true && + snapshot.parsed.token_count === 0; + lastBootConsumptionRefuted = bootConsumptionRefuted; const screenCli = inferComposerCli(snapshot.text, snapshot.parsed); const cursorShowsSubmittedResponse = screenCli === "cursor" && @@ -4856,6 +5065,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { const composerInput = extractComposerInputRegion(snapshot.text); if ( !hasPendingSubmitEvidence && + !bootConsumptionRefuted && (isSubmitVerifiedStatus(snapshot.parsed.status) || cursorShowsSubmittedResponse) ) { @@ -4870,6 +5080,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { composerInput !== null && composerInput.trim() === "" && !hasPendingSubmitEvidence && + !bootConsumptionRefuted && screenHasAnyAgentIdentity(snapshot.text, snapshot.parsed); if (hasClearedAgentComposer) { sawClearedComposerEvidence = true; @@ -4997,17 +5208,16 @@ export function createServer(opts?: CreateServerOptions): McpServer { ? false : noSubmitEvidenceResult; const failureReason: SubmitVerificationFailureReason | null = - submitVerified !== false - ? null - : lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput - ? "input_still_pending" - : !sawReadableScreen - ? sawBlankScreen - ? "surface_screen_empty" - : "surface_read_unavailable" - : opts.require_working_status - ? "working_status_not_observed" - : "submit_evidence_absent"; + submitVerified === false + ? resolveSubmitVerificationFailureReason({ + sawPendingInput: + lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput, + sawReadableScreen, + sawBlankScreen, + bootConsumptionRefuted: lastBootConsumptionRefuted, + requireWorkingStatus: opts.require_working_status === true, + }) + : null; const allowPendingVerify = opts.source_event === "send_to" || opts.source_event === "dispatch_nudge"; if (allowPendingVerify && submitVerified === false) { @@ -5026,6 +5236,82 @@ export function createServer(opts?: CreateServerOptions): McpServer { }; }; + /** + * AIDEV-NOTE (#484): a bare submit key used to return ok:true with + * submit_verified:null and no evidence of any kind -- the documented + * "type, then press Return" recovery reported success for a no-op and two + * leads silently stopped being able to talk to each other. A submit key now + * has to answer the only question that matters: did the composer let go of + * its contents? Positive evidence either way is reported; absence of + * evidence is reported as absence, never as success. + */ + const verifySubmitKeyOutcome = async (opts: { + surface: string; + workspace?: string; + }): Promise<{ + submit_verified: boolean | null; + submit_verification_reason: SubmitKeyVerificationReason | null; + }> => { + const startedAt = Date.now(); + let sawReadableScreen = false; + let composerStillPopulated = false; + + while (Date.now() - startedAt < SEND_KEY_SUBMIT_VERIFY_TIMEOUT_MS) { + const snapshot = await readParsedSurface(opts.surface, opts.workspace); + if (!snapshot || !snapshot.text.trim()) { + await delay(SEND_INPUT_SUBMIT_VERIFY_POLL_MS); + continue; + } + sawReadableScreen = true; + const composerInput = extractComposerInputRegion(snapshot.text); + composerStillPopulated = + composerInput !== null && composerInput.trim() !== ""; + + // AIDEV-NOTE (#484 review): a populated composer VETOES every other + // signal. A busy target is the reported scenario -- the recipient was + // working on its previous turn while the relayed message sat unsent in + // its composer -- so reading "working" as proof of submit reported + // submit_verified:true for a message still visible on screen. That is + // worse than the null it replaced: null admits ignorance, true asserts + // an observation contradicted by the pane. The text path already gets + // this right by gating the same status branch on !hasPendingSubmitEvidence. + if (composerStillPopulated) { + await delay(SEND_INPUT_SUBMIT_VERIFY_POLL_MS); + continue; + } + if (composerInput !== null) { + // The composer is readable and empty: it let go of its contents. This + // is the ONLY positive proof available to a key send. A "working" + // status deliberately does not count: the reported target was already + // working on its previous turn, so status cannot distinguish "my + // submit started a turn" from "a turn was already running" -- and a + // composer that renders boxed reads as unreadable here, so accepting + // status would resurrect the same false-true through a blind spot. + return { submit_verified: true, submit_verification_reason: null }; + } + await delay(SEND_INPUT_SUBMIT_VERIFY_POLL_MS); + } + + if (!sawReadableScreen) { + return { + submit_verified: null, + submit_verification_reason: "surface_read_unavailable", + }; + } + if (composerStillPopulated) { + // The key reached the pane and the composer still holds its contents: + // that is a submit that did not land, not a submit we failed to observe. + return { + submit_verified: false, + submit_verification_reason: "composer_still_populated", + }; + } + return { + submit_verified: null, + submit_verification_reason: "submit_evidence_absent", + }; + }; + const executeDeliveryEngine = async (opts: { surface: string; workspace?: string; @@ -5044,7 +5330,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { submit_verify_timeout_ms?: number; stableSurfaceIdentity?: string | null; beforeMutation?: () => Promise; - }): Promise => { + }): Promise< + PublicDeliveryReceipt & { + bytes: number; + /** Present only on the key path: the key really reached the pane. */ + key_dispatched?: boolean; + submit_verification_reason?: SubmitKeyVerificationReason | null; + } + > => { await opts.beforeMutation?.(); if (opts.key !== undefined) { if (opts.chunks.length > 0 || opts.press_enter) { @@ -5053,16 +5346,26 @@ export function createServer(opts?: CreateServerOptions): McpServer { ); } const key = normalizeKeyName(opts.key); + // sendKeyWithRetry throws when nothing reached the pane, so reaching the + // next line is the dispatch evidence the receipt was missing (#484). await sendKeyWithRetry( opts.surface, key, opts.workspace, opts.beforeMutation, ); + const submitAttempted = isSubmitKey(key); + const verification = + submitAttempted && opts.verify_submit + ? await verifySubmitKeyOutcome({ + surface: opts.surface, + workspace: opts.workspace, + }) + : { submit_verified: null, submit_verification_reason: null }; const receipt = buildPublicDeliveryReceipt({ typed: false, - submit_attempted: key === "return", - submit_verified: null, + submit_attempted: submitAttempted, + submit_verified: verification.submit_verified, retry_count: 0, }); if (opts.source_event) { @@ -5071,17 +5374,36 @@ export function createServer(opts?: CreateServerOptions): McpServer { source_agent: opts.source_agent ?? null, target_surface: opts.surface, bytes: 0, - press_enter: key === "return", - submit_verified: null, + press_enter: submitAttempted, + submit_verified: verification.submit_verified, retry_count: 0, }); } - return { ...receipt, bytes: 0 }; + return { + ...receipt, + key_dispatched: true, + submit_verification_reason: verification.submit_verification_reason, + bytes: 0, + }; } - const deliverySafetySnapshot = await assertDeliveryTargetIsSafe( - opts.surface, - opts.workspace, - ); + // AIDEV-NOTE (T2 #442): the draft guard covers the caller-initiated relay + // paths, where refusing is cheap and a foreign draft is a live risk. Boot + // and cwd delivery run against a pane cmuxlayer just launched, where the + // only text on screen is the launcher's own echo -- refusing there would + // break spawn, not protect a human. + const draftGuardedEvent = + opts.source_event === "send_to" || + opts.source_event === "send_to_agent" || + opts.source_event === "send_input" || + opts.source_event === "dispatch_nudge"; + const draftGuardText = opts.chunks.join(""); + const deliverySafetySnapshot = await assertDeliveryTargetIsSafe({ + surface: opts.surface, + workspace: opts.workspace, + ...(draftGuardedEvent && draftGuardText.trim().length > 0 + ? { draftGuardText } + : {}), + }); const deliveryBatches = buildInputDeliveryBatches(opts.chunks); const shouldPaste = shouldPasteInputDelivery( opts.chunks, @@ -5489,7 +5811,17 @@ export function createServer(opts?: CreateServerOptions): McpServer { }); if (snapshot) { lastText = snapshot.text; - if (isSubmitVerifiedStatus(snapshot.parsed.status)) { + const metrics = parseRawSubmitEvidenceMetrics(snapshot.text); + // AIDEV-NOTE (T2 #427): `0 tokens` is a definitive negative -- an agent + // handed a prompt that has consumed nothing did not receive it. A slow + // boot (MCP servers still connecting, banner mid-render) can present a + // working-looking status while the prompt is still sitting in the + // composer, and accepting that status produced fully-verified receipts + // for workers that sat at `0 tokens` with their entire brief unsent. + // A NULL count stays inconclusive on purpose: several CLIs never + // report one, and reading unknown as zero would break every boot. + const consumptionRefuted = metrics.tokenCount === 0; + if (!consumptionRefuted && isSubmitVerifiedStatus(snapshot.parsed.status)) { return; } @@ -5501,10 +5833,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { if ( composerInput !== null && !hasPendingInput && - hasRawSubmitEvidenceIncrease( - parseRawSubmitEvidenceMetrics(snapshot.text), - opts.baseline_metrics, - ) + hasRawSubmitEvidenceIncrease(metrics, opts.baseline_metrics) ) { return; } @@ -5515,6 +5844,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { !hasPendingInput; if ( composerCleared && + !consumptionRefuted && screenHasAnyAgentIdentity(snapshot.text, snapshot.parsed) ) { if (composerInput === lastClearedComposerInput) { @@ -7212,6 +7542,56 @@ export function createServer(opts?: CreateServerOptions): McpServer { } : parsed; + /** + * AIDEV-NOTE (T1b/#488): ONE screen observation for a response that emits + * `closure` and `state`/health together. `list_agents` threads its own scan; + * `get_agent_state` and `wait_for` have no scan, so they read the surface ONCE + * here and hand the same observation to both consumers -- the health block via + * the screen_* overrides it already accepts (which stop it re-reading), and + * closure via `assessHarvestability(agent, { live })`. Without this, closure + * fell back to `cachedScan()`, null past 2000ms, and the same response could + * say `working` and `artifact_missing` at once. Read count is unchanged: the + * read moves out of the health call, it is not added to it. + */ + const observeAgentOnce = async ( + agent: AgentRecord, + topology: SurfaceTopologySnapshot | null, + ): Promise<{ + screenOverrides: AgentHealthInputOverrides; + live: LiveAgentState; + }> => { + const binding = resolveAuthorizedAgentSurfaceBinding(agent, topology); + if (!binding) { + // No authorized surface is no evidence -- the same answer the health path + // reaches on its own, and `resolveLiveAgentState` records it as + // `source: "registry"` rather than passing the record off as observed. + return { screenOverrides: {}, live: resolveLiveAgentState(agent, null) }; + } + const parsed = await readParsedSurface( + binding.surfaceRef, + binding.workspaceId ?? undefined, + { agent }, + ); + return { + screenOverrides: { + screen_status: parsed?.parsed.status ?? null, + screen_agent_type: parsed?.parsed.agent_type ?? null, + screen_control_state: parsed?.parsed.control_state ?? null, + screen_actions: parsed?.parsed.actions ?? null, + }, + live: resolveLiveAgentState( + agent, + parsed + ? { + status: parsed.parsed.status, + agent_type: parsed.parsed.agent_type, + control_state: parsed.parsed.control_state, + } + : null, + ), + }; + }; + const evaluateServerAgentHealth = async ( agent: AgentRecord, overrides?: AgentHealthInputOverrides, @@ -7329,65 +7709,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) { @@ -8988,6 +9309,9 @@ export function createServer(opts?: CreateServerOptions): McpServer { chunk_size: 0, chunk_delay_ms: 0, press_enter: false, + // A submit key is the documented recovery for a typed-but-unsent + // message. It has to prove it landed rather than assert it. + verify_submit: true, stableSurfaceIdentity: route.stableSurfaceIdentity, source_event: "send_key", beforeMutation: route.assertCurrent, @@ -9006,6 +9330,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { ...delivery, ...remapFields(route), }; + if (delivery.submit_verified === false) { + return err( + new Error( + `send_key ${key} reached ${route.surface} but the submit did not land (${delivery.submit_verification_reason}). The composer still holds its unsent contents — nothing was delivered. Read the surface and resolve the pending input before relaying this as sent.`, + ), + data, + ); + } return okFormatted(formatOk("send_key", data), data); } catch (e) { return err(e); @@ -9469,7 +9801,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { // 10. close_surface server.tool( "close_surface", - "Close one surface, managed agent, or workspace with live-agent guards.", + "Close one surface, managed agent, or workspace with live-agent guards. scope=\"agent\" stops the agent AND closes its pane, and reports the two halves separately (agent_stopped, surface_closed) so a pane that survives is never reported as closed. The pane close obeys the same live-agent guard as scope=\"surface\": without force:true a still-live agent keeps its pane, and the receipt says so.", { scope: z .enum(["surface", "agent", "workspace"]) @@ -9496,17 +9828,119 @@ export function createServer(opts?: CreateServerOptions): McpServer { const handler = toolHandlersByName.get("stop_agent"); if (!handler) throw new Error("Internal agent close adapter unavailable"); + // AIDEV-NOTE (#485): this used to stop the agent and hand back + // stop_agent's receipt verbatim -- ok:true, state:"done" -- for a + // tool named close_surface, while the pane stayed open. A lead + // harvesting panes got success and kept every one of them. Resolve + // the bound surface BEFORE stopping (the stop can evict the record), + // stop, then close the pane for real and say which halves happened. + const boundAgent = context.lifecycleRegistry?.get(args.agent_id) ?? null; + const boundSurface = boundAgent?.surface_id?.trim() || null; + const boundWorkspace = boundAgent?.workspace_id ?? undefined; const result = await handler( { agent_id: args.agent_id, force: args.force }, {}, ); - return { - ...result, - structuredContent: { - ...(result.structuredContent ?? {}), + const agentStopped = result.isError !== true; + const rawStopContent = (result.structuredContent ?? {}) as Record< + string, + unknown + >; + // Drop the stop receipt's own envelope fields; this response owns + // ok/error, and letting stop_agent's ok:true through was exactly how + // a half-done close reported success. + const { ok: _stopOk, error: _stopError, ...stopContent } = + rawStopContent; + if (!agentStopped) { + // The stop itself failed: keep its verbatim ok:false/error and add + // the surface half, which was never attempted. + return { + ...result, + structuredContent: { + ...rawStopContent, + scope: "agent", + agent_stopped: false, + surface_closed: false, + surface_close_skipped: "agent_stop_failed", + }, + }; + } + if (!boundSurface) { + const data = { + ...stopContent, scope: "agent", + agent_stopped: true, + surface_closed: false, + surface_close_skipped: "no_surface_bound", + }; + return okFormatted( + `close_surface scope=agent — agent ${args.agent_id} stopped; no surface was bound, so no pane was closed`, + data, + ); + } + const closeHandler = toolHandlersByName.get("close_surface"); + if (!closeHandler) { + throw new Error("Internal surface close adapter unavailable"); + } + const closeResult = await closeHandler( + { + scope: "surface", + surface: boundSurface, + workspace: boundWorkspace, + // AIDEV-NOTE (#485 review): pass the caller's own force through + // rather than forcing unconditionally. stop_agent has no liveness + // refusal of its own, so forcing here would let an unforced + // close_surface(scope:"agent") tear down a still-live agent's + // pane through a guard that could never fire for this scope -- + // a silent escalation of destructiveness. If the guard refuses, + // the receipt says the agent stopped and the pane did not close. + force: args.force ?? false, }, + {}, + ); + const closeContent = (closeResult.structuredContent ?? {}) as Record< + string, + unknown + >; + if (closeResult.isError === true) { + const reason = + typeof closeContent.error === "string" + ? closeContent.error + : "close failed"; + return err( + new Error( + `close_surface scope=agent: agent ${args.agent_id} was stopped but surface ${boundSurface} is still open — ${reason}`, + ), + { + ...stopContent, + scope: "agent", + agent_stopped: true, + surface: boundSurface, + surface_closed: false, + surface_close_error: reason, + }, + ); + } + // Forward the surface path's OBSERVATION of whether the pane is + // gone; never restate a returned call as a completed close. + const surfaceClosed = closeContent.surface_closed === true; + const data = { + ...stopContent, + scope: "agent", + agent_stopped: true, + surface: boundSurface, + surface_closed: surfaceClosed, + ...(closeContent.WARNING ? { WARNING: closeContent.WARNING } : {}), + ...(closeContent.collapse_pane !== undefined + ? { collapse_pane: closeContent.collapse_pane } + : {}), }; + return okFormatted( + surfaceClosed + ? `close_surface scope=agent — agent ${args.agent_id} stopped and surface ${boundSurface} closed` + : `close_surface scope=agent — agent ${args.agent_id} stopped, but surface ${boundSurface} is STILL LISTED — not closed`, + data, + ); } if (args.scope === "workspace") { if (!args.workspace) { @@ -9520,11 +9954,17 @@ export function createServer(opts?: CreateServerOptions): McpServer { { workspace: args.workspace, force: args.force }, {}, ); + // Cross-check for the #485 class: unlike scope=agent, this delegate + // really does perform the action the scope names -- delete_workspace + // tears the tab and its panes down and surfaces its own failures. + // State the outcome explicitly rather than leaving the caller to + // infer it from a bolted-on scope field. return { ...result, structuredContent: { ...(result.structuredContent ?? {}), scope: "workspace", + workspace_deleted: result.isError !== true, }, }; } @@ -9761,6 +10201,13 @@ export function createServer(opts?: CreateServerOptions): McpServer { stableSurfaceIdentity: route.stableSurfaceIdentity, }, ); + // AIDEV-NOTE (#485): confirm the pane is actually gone rather than + // inferring it from the CLI returning. One observation, no waiting -- + // the "eventually consistent" theory was withdrawn once the mechanism + // turned out to be the scope argument, so there is no window to sit + // through. If cmux still lists the surface, the receipt says so. + const surfaceStillPresent = + (await findSurfaceByRef(route.surface, route.workspace)) !== null; for (const record of stateMgr.listStates()) { // Stable identity wins whenever cmux exposes it. On a ref-only or // unavailable observation, preserve the explicit close intent by @@ -9780,6 +10227,16 @@ export function createServer(opts?: CreateServerOptions): McpServer { user_killed: true, }); context.lifecycleRegistry?.set(record.agent_id, terminal); + // AIDEV-NOTE (#485 reframe): marking user_killed left the RECORD's + // state untouched, so list_agents kept reporting a closed agent as + // "working" after its close was acknowledged -- reported live by + // golemsClaude, and independent of which scope was used. + // An acknowledged close means this agent is not running any more; + // say so in the same breath as accepting the close. + if (!TERMINAL_AGENT_STATES.has(terminal.state)) { + const stopped = stateMgr.transition(record.agent_id, "done"); + context.lifecycleRegistry?.set(record.agent_id, stopped); + } } catch (error) { if ( error instanceof Error && @@ -9804,9 +10261,20 @@ export function createServer(opts?: CreateServerOptions): McpServer { surface: route.surface, pane: closePolicy?.pane ?? undefined, collapse_pane: collapsePane, + surface_closed: !surfaceStillPresent, stale_registry_done_consolidated: staleRegistryDoneConsolidated, + ...(surfaceStillPresent + ? { + WARNING: `cmux accepted the close but ${route.surface} is STILL listed. Do not relay this as closed.`, + } + : {}), }; - return okFormatted(formatOk("close_surface", data), data); + return okFormatted( + surfaceStillPresent + ? `close_surface accepted — ${route.surface} is still listed; NOT closed` + : formatOk("close_surface", data), + data, + ); } catch (e) { return err(e); } @@ -11008,7 +11476,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; @@ -11063,7 +11531,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.`, ); } } @@ -13303,23 +13772,32 @@ export function createServer(opts?: CreateServerOptions): McpServer { const resultAgent = result.agent ? engine.getAgentState(result.agent.agent_id) : null; + // T1b (#488): one observation feeds this reply's health block + // AND its closure, so the two cannot contradict each other. + const observed = resultAgent + ? await observeAgentOnce(resultAgent, topology) + : null; + // P11 Contract B: a lead that BLOCKS on its children gets the + // closure state in the reply it was already waiting for -- the + // completion signal surfaces where the parent actually looks, + // with no new carrier (#414: a carrier without a reader is not + // a carrier). + const harvest = resultAgent + ? engine.assessHarvestability(resultAgent, { + live: observed?.live ?? null, + }) + : null; const health = resultAgent ? await evaluateServerAgentHealth( resultAgent, { ...healthTopologyOverrides(resultAgent, topology), + ...(observed?.screenOverrides ?? {}), + ...(harvest ? { harvestability: harvest } : {}), }, topology, ) : undefined; - // P11 Contract B: a lead that BLOCKS on its children gets the - // closure state in the reply it was already waiting for -- the - // completion signal surfaces where the parent actually looks, - // with no new carrier (#414: a carrier without a reader is not - // a carrier). - const harvest = resultAgent - ? engine.assessHarvestability(resultAgent) - : null; return { ...result, health, @@ -13488,7 +13966,12 @@ export function createServer(opts?: CreateServerOptions): McpServer { if (!state) return err(new Error(`Agent not found: ${args.agent_id}`)); const topology = await collectSurfaceTopology(); - const harvestability = engine.assessHarvestability(state); + // T1b (#488): the screen this response reports on is the screen its + // closure is resolved from. Read once, used by both. + const observed = await observeAgentOnce(state, topology); + const harvestability = engine.assessHarvestability(state, { + live: observed.live, + }); const authorizedBinding = resolveAuthorizedAgentSurfaceBinding( state, topology, @@ -13498,6 +13981,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { state, { ...healthTopologyOverrides(state, topology), + ...observed.screenOverrides, harvestability, }, topology, @@ -13685,7 +14169,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?: { @@ -13717,6 +14203,30 @@ export function createServer(opts?: CreateServerOptions): McpServer { observedSurface && !observedSurface.read_error ? observedSurface : null; + // AIDEV-NOTE (T1b/#488): ONE observation per row. `closure` + // used to re-resolve live state through the discovery cache + // (`cachedScan()`, null past 2000ms) while `state` below used + // THIS call's own scan -- so a cold cache made one row read + // `working` and `artifact_missing` at the same time, and flap + // as the cache aged. Same evidence, resolved once, passed to + // the health block AND to closure. Costs zero extra screen + // reads: the scan already happened at the top of this call. + const rowLiveState = resolveLiveAgentState( + agent, + trustedScreenObservation + ? { + status: trustedScreenObservation.parsed_status, + agent_type: + trustedScreenObservation.cli === "kiro" + ? "unknown" + : trustedScreenObservation.cli, + control_state: trustedScreenObservation.control_state, + } + : null, + ); + const rowHarvestability = engine.assessHarvestability(agent, { + live: rowLiveState, + }); const health = await evaluateServerAgentHealth( agent, { @@ -13734,6 +14244,12 @@ export function createServer(opts?: CreateServerOptions): McpServer { trustedScreenObservation.actions ?? [], } : {}), + // Without this the health block re-derived harvestability + // through the probe (buildAgentHealthInput's + // `deps.assessHarvestability`), putting a THIRD resolution + // in the same row: `closure_without_artifact` could fire + // beside `closure: "pending"`. + harvestability: rowHarvestability, }, topology, ); @@ -13776,11 +14292,19 @@ 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 // boolean made both of those `false`; that was the S3 bug. - closure: engine.assessHarvestability(agent).closure, + closure: rowHarvestability.closure, ...(args.detail === "full" ? { health: { @@ -13791,7 +14315,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { }, detail: { ...toAgentStatePayload(agent), - harvestability: engine.assessHarvestability(agent), + harvestability: rowHarvestability, }, } : {}), @@ -13853,6 +14377,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, @@ -14128,582 +14674,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/agent-engine.test.ts b/tests/agent-engine.test.ts index c2e259c4..f7fc0d46 100644 --- a/tests/agent-engine.test.ts +++ b/tests/agent-engine.test.ts @@ -3247,6 +3247,50 @@ describe("AgentEngine", () => { } }); + // T1b (#488): the sweep is the third emitter of closure -- its assessment + // feeds the health input, this row's `report=`/`health=` text and the done + // notification, beside a state it derives from the screen it reads. It used + // to take closure from the discovery-cache probe, which is cold on this + // path too, so a live-working agent whose record #408 had flipped rendered + // the blocking `closure_without_artifact` here as well. + it("sweep row closure follows the screen it read, at no extra read", async () => { + stateMgr.writeState( + makeRecord({ + agent_id: "worker-sweep-live-closure", + state: "done", + surface_id: "surface:sweep-live-closure", + workspace_id: "workspace:brainlayer", + cli: "codex", + role: "worker", + task_done_detected_at: "2026-05-25T12:00:00.000Z", + }), + ); + liveSurfaces = [ + { + ...makeSurface("surface:sweep-live-closure"), + workspace_ref: "workspace:brainlayer", + }, + ]; + (mockClient.readScreen as ReturnType).mockResolvedValue({ + surface: "surface:sweep-live-closure", + text: "codex\n• Working (12s • esc to interrupt)\ncodex> ", + lines: 80, + scrollback_used: false, + }); + await engine.getRegistry().reconstitute(); + (mockClient.readScreen as ReturnType).mockClear(); + + await engine.runSweep(); + + const row = (mockClient.setStatus as ReturnType).mock + .calls[0]?.[1] as string | undefined; + expect(row, JSON.stringify((mockClient.setStatus as any).mock.calls)).toBeTruthy(); + expect(row).not.toContain("closure_without_artifact"); + // The pre-read shares `sweepCtx` with the health input's own read, so + // resolving closure from the screen costs nothing extra. + expect(mockClient.readScreen).toHaveBeenCalledTimes(1); + }); + it("does not rewrite TASK_DONE candidate metadata while the sweep stamps liveness", async () => { vi.useFakeTimers(); try { @@ -14053,6 +14097,80 @@ Session ID: ${sessionId}`, } }); + it("terminalizes a retryable requeue once its bounded queue lifetime elapses (#467)", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-11T18:00:00.000Z")); + const boundedEngine = new AgentEngine( + stateMgr, + new AgentRegistry(stateMgr, async () => liveSurfaces), + mockClient, + { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + inboxOpts: { baseDir: TEST_DIR }, + deliveryQueueDeadlineMs: 60_000, + }, + ); + try { + stateMgr.writeState( + makeRecord({ + agent_id: "stuck-booting-delivery", + state: "booting", + surface_id: "surface:42", + }), + ); + liveSurfaces = [makeSurface("surface:42")]; + await boundedEngine.getRegistry().reconstitute(); + boundedEngine.setDeliverySubmitter(async () => { + throw new RetryableDeliveryError( + 'Agent "stuck-booting-delivery" is not in an interactive state (current: booting)', + ); + }); + const receipt = boundedEngine.queueDelivery({ + agent_id: "stuck-booting-delivery", + text: "a lead is waiting on this", + press_enter: true, + source_event: "send_to", + }); + + await boundedEngine.drainDeliveryQueue(); + const deferred = boundedEngine.getDeliveryReceipt( + receipt.delivery_id, + )!; + expect(deferred).toMatchObject({ + delivery_state: "queued", + terminal: false, + }); + // The requeue now carries a stated lifetime instead of retrying forever. + expect(deferred.queue_deadline_at).toBe("2026-08-11T18:01:00.000Z"); + + // Retries inside the lifetime stay nonterminal. + vi.setSystemTime(new Date("2026-08-11T18:00:30.000Z")); + await boundedEngine.drainDeliveryQueue(); + expect( + boundedEngine.getDeliveryReceipt(receipt.delivery_id), + ).toMatchObject({ delivery_state: "queued", terminal: false }); + + // Past the lifetime the lead gets an answer, citing the gate reason. + vi.setSystemTime(new Date("2026-08-11T18:01:00.001Z")); + await boundedEngine.drainDeliveryQueue(); + expect( + boundedEngine.getDeliveryReceipt(receipt.delivery_id), + ).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + submit_verified: false, + resolved_at: expect.any(String), + error: expect.stringMatching( + /queue_deadline_elapsed.*not in an interactive state.*booting/s, + ), + }); + } finally { + boundedEngine.dispose(); + vi.useRealTimers(); + } + }); + it("does not replay a queued receipt whose persisted submission had started", () => { const receipt = engine.queueDelivery({ agent_id: "crashed-queued-delivery", diff --git a/tests/coordination-paths.test.ts b/tests/coordination-paths.test.ts index d9735d6c..98562354 100644 --- a/tests/coordination-paths.test.ts +++ b/tests/coordination-paths.test.ts @@ -112,9 +112,11 @@ describe("P11 boot footer (Constraint 1: <=2 short lines, bytes declared)", () = }); describe("P11 closure state (Constraint 3: no bare boolean at default detail)", () => { - // A genuinely finished worker: the contract was issued AND something observed - // the task ending. `doneEvidence:false` is its own case below. + // T1b (#488): `observed` is a done that something actually SAW -- a done + // marker on screen, a finished transcript. `unobserved` is the bare registry + // flip #408 writes on live agents, which must never claim a deadlock. const issued = { contractIssued: true, doneEvidence: true }; + const unobserved = { contractIssued: true, doneEvidence: false }; it("done + verified artifact => verified", () => { expect( @@ -126,7 +128,7 @@ describe("P11 closure state (Constraint 3: no bare boolean at default detail)", ).toBe("verified"); }); - it("S3 SIGNATURE: done + no artifact => artifact_missing, NOT pending", () => { + it("S3 SIGNATURE: done + done evidence + no artifact => artifact_missing, NOT pending", () => { expect( resolveClosureState({ ...issued, @@ -136,6 +138,26 @@ describe("P11 closure state (Constraint 3: no bare boolean at default detail)", ).toBe("artifact_missing"); }); + it("T1b: done with NO done evidence => pending, never artifact_missing", () => { + const closure = resolveClosureState({ + ...unobserved, + state: "done", + closureArtifactVerified: false, + }); + expect(closure).toBe("pending"); + expect(closure).not.toBe("artifact_missing"); + }); + + it("T1b: a verified artifact IS done evidence, so it still reads verified", () => { + expect( + resolveClosureState({ + ...unobserved, + state: "done", + closureArtifactVerified: true, + }), + ).toBe("verified"); + }); + it("still working => pending, and NEVER artifact_missing", () => { for (const state of ["ready", "working", "idle", "error"]) { const closure = resolveClosureState({ @@ -191,6 +213,7 @@ describe("P11 closure state (Constraint 3: no bare boolean at default detail)", expect( resolveClosureState({ contractIssued: false, + doneEvidence: true, state: "done", closureArtifactVerified: null, doneEvidence: true, diff --git a/tests/delivery-truth-t2.test.ts b/tests/delivery-truth-t2.test.ts new file mode 100644 index 00000000..cfc62d92 --- /dev/null +++ b/tests/delivery-truth-t2.test.ts @@ -0,0 +1,598 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import type { ExecFn } from "../src/cmux-client.js"; +import { withTestSurfaceObserver } from "./helpers/test-surface-observer.js"; + +let testDir = ""; + +async function loadServerModule() { + vi.resetModules(); + const serverModule = await import("../src/server.js"); + return { + ...serverModule, + createServerContext: ( + opts: Parameters[0] = {}, + ) => serverModule.createServerContext(withTestSurfaceObserver(opts)), + createServer: ( + opts: Parameters[0] = {}, + ) => + serverModule.createServer( + opts.context ? opts : withTestSurfaceObserver(opts), + ), + }; +} + +function parseToolResult(result: any) { + return result.structuredContent ?? JSON.parse(result.content[0].text); +} + +async function spawnReadyAgent(server: any) { + const spawn = server._registeredTools["spawn_agent"]; + const spawnResult = await spawn.handler( + { + repo: "brainlayer", + model: "sonnet", + cli: "claude", + workspace: "workspace:1", + boot_prompt_timeout_ms: 100, + }, + {} as any, + ); + const agentId = parseToolResult(spawnResult).agent_id; + const engine = server._registeredTools.interact._engine; + const registry = engine.getRegistry(); + registry.set(agentId, { ...registry.get(agentId), state: "ready" }); + return agentId; +} + +function makeLifecycleExec(readScreenText: () => string): ExecFn { + return vi.fn().mockImplementation(async (_cmd, args: string[]) => { + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: "surface:new", + text: readScreenText(), + lines: 20, + scrollback_used: false, + }), + stderr: "", + }; + } + if (args.includes("list-workspaces")) { + return { + stdout: JSON.stringify({ + workspaces: [ + { + ref: "workspace:1", + title: "Main", + index: 0, + selected: true, + pinned: false, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-panes")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:new"], + selected_surface_ref: "surface:new", + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + pane_ref: "pane:1", + surfaces: [ + { + ref: "surface:new", + title: "agent-pane", + type: "terminal", + index: 0, + selected: true, + }, + ], + }), + stderr: "", + }; + } + return { + stdout: JSON.stringify({ + workspace: "workspace:1", + surface: "surface:new", + pane: "pane:1", + title: "", + type: "terminal", + }), + stderr: "", + }; + }); +} + +const mutatedPane = (mockExec: any): boolean => + mockExec.mock.calls.some(([, args]: [string, string[]]) => + args.some((arg: string) => + ["send", "set-buffer", "paste-buffer", "send-key"].includes(arg), + ), + ); + +describe("T2 delivery truth — composer draft safety (#442)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("send_to refuses a composer holding human-typed draft text, before typing anything", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n❯ "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + // A human left a half-written thought in the composer and never submitted. + screenText = "Claude Code\n> so about the release, I think we should\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + const parsed = parseToolResult(result); + expect(parsed).toMatchObject({ + delivered: false, + terminal: false, + delivery_state: "queued", + }); + expect(parsed.WARNING).toMatch(/not delivered yet/i); + expect(parsed.WARNING).toMatch(/composer already holds text/i); + expect(mutatedPane(mockExec)).toBe(false); + context.dispose(); + }, 20_000); + + it("send_to still delivers when the composer is empty", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n❯ "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + screenText = "Claude Code\n> \nCLAUDE_COUNTER:1\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + expect(parseToolResult(result).ok).toBe(true); + expect(mutatedPane(mockExec)).toBe(true); + context.dispose(); + }); +}); + +describe("T2 delivery truth — draft guard must not fire on chrome (B1)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + // Every frame below has an EMPTY composer. The line under it is ordinary + // Claude chrome that `isComposerFooterOrChromeLine` does not happen to + // whitelist -- and a whitelist miss must never cost a ready pane a refusal. + const EMPTY_COMPOSER_FRAMES: Array<[string, string]> = [ + [ + "shortcut hint", + ["Claude Code", "", "\u23fa Compared both approaches.", "", "\u276f", "? for shortcuts"].join( + "\n", + ), + ], + [ + "accept-edits mode", + ["Claude Code", "> ", "\u23f5\u23f5 accept edits on (shift+tab to cycle)"].join("\n"), + ], + [ + "busy spinner", + ["Claude Code", "\u23fa Done.", "> ", "Working (2s \u2022 esc to interrupt)"].join("\n"), + ], + ["interrupt hint", ["Claude Code", "> ", " esc to interrupt"].join("\n")], + ]; + + it.each(EMPTY_COMPOSER_FRAMES)( + "treats an empty composer under %s as deliverable", + async (_label, screen) => { + const { __submitEvidenceTestHooks } = await loadServerModule(); + expect( + __submitEvidenceTestHooks.composerHoldsForeignDraft( + screen, + "fleet message", + ), + ).toBe(false); + }, + ); + + it("still refuses when the prompt line itself carries someone else's text", async () => { + const { __submitEvidenceTestHooks } = await loadServerModule(); + expect( + __submitEvidenceTestHooks.composerHoldsForeignDraft( + ["Claude Code", "> so about the release, I think we", "? for shortcuts"].join( + "\n", + ), + "fleet message", + ), + ).toBe(true); + }); + + it("send_to delivers to a busy Claude pane whose composer is empty", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n\u276f "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + screenText = [ + "Claude Code", + "\u23fa Done.", + "> ", + "Working (2s \u2022 esc to interrupt)", + ].join("\n"); + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + expect(parseToolResult(result).ok).toBe(true); + expect(mutatedPane(mockExec)).toBe(true); + context.dispose(); + }, 20_000); +}); + +describe("T2 delivery truth — a blocked composer is not a terminal verdict (B1a)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("queues instead of terminally failing when the composer holds unflushed text", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n\u276f "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + // This delivery's OWN prior message, still sitting unflushed in the + // composer (the queued_followup shape). The guard cannot tell it from a + // human draft -- and must not, because the right answer for both is + // "wait for the composer to flush", never a terminal failure. + screenText = "Claude Code\n> an earlier message that has not flushed yet\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "second message", press_enter: true }, + {} as any, + ); + + const parsed = parseToolResult(result); + expect(parsed).toMatchObject({ + ok: true, + delivered: false, + delivery_state: "queued", + terminal: false, + }); + expect(parsed.WARNING).toMatch(/not delivered yet/i); + expect(parsed.WARNING).toMatch(/composer already holds text/i); + // Still the property #442 exists for: nothing was typed. + expect(mutatedPane(mockExec)).toBe(false); + context.dispose(); + }, 20_000); +}); + +describe("T2 delivery truth — unmissable non-delivery (#445)", () => { + it("attaches a plain-language WARNING to every nonterminal receipt", async () => { + const { buildPublicDeliveryReceipt } = await loadServerModule(); + for (const state of ["pending_verify", "queued", "queued_followup"] as const) { + const receipt = buildPublicDeliveryReceipt({ + delivery_state: state, + delivery_id: "d-1", + typed: true, + submit_attempted: true, + submit_verified: null, + retry_count: 0, + }); + expect(receipt).toMatchObject({ delivered: false, terminal: false }); + expect(receipt.WARNING).toMatch(/NOT DELIVERED YET/); + expect(receipt.WARNING).toMatch(/do not relay as sent/i); + } + }); + + it("attaches a terminal-failure WARNING to failed and failed_confirmed receipts", async () => { + const { buildPublicDeliveryReceipt } = await loadServerModule(); + for (const state of ["failed", "failed_confirmed"] as const) { + const receipt = buildPublicDeliveryReceipt({ + delivery_state: state, + typed: true, + submit_attempted: true, + submit_verified: false, + retry_count: 0, + }); + expect(receipt).toMatchObject({ delivered: false, terminal: true }); + expect(receipt.WARNING).toMatch(/NOT DELIVERED/); + expect(receipt.WARNING).toMatch(/do not relay as sent/i); + } + }); + + it("leaves a verified submitted receipt unwarned and keeps an explicit WARNING", async () => { + const { buildPublicDeliveryReceipt, pausedTargetWarning } = + await loadServerModule(); + expect( + buildPublicDeliveryReceipt({ + delivery_state: "submitted", + typed: true, + submit_attempted: true, + submit_verified: true, + retry_count: 0, + }).WARNING, + ).toBeUndefined(); + expect( + buildPublicDeliveryReceipt({ + delivery_state: "queued", + typed: false, + submit_attempted: false, + submit_verified: null, + retry_count: 0, + WARNING: pausedTargetWarning("registry"), + }).WARNING, + ).toBe(pausedTargetWarning("registry")); + }); +}); + +describe("T2 delivery truth — CLI-fallback hang guard (#450)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-cli-timeout-")); + }); + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("kills a wedged cmux subprocess instead of awaiting it forever", async () => { + const { CmuxClient, CMUX_CLI_EXEC_TIMEOUT_MS } = await import( + "../src/cmux-client.js" + ); + // The default ceiling matches the socket transport's request budget. + expect(CMUX_CLI_EXEC_TIMEOUT_MS).toBe(10_000); + + // A `cmux` that never exits, whatever arguments it is handed. + const wedged = join(testDir, "wedged-cmux"); + writeFileSync(wedged, "#!/bin/sh\nsleep 600\n", "utf8"); + chmodSync(wedged, 0o755); + + const client = new CmuxClient({ bin: wedged, execTimeoutMs: 150 }); + const startedAt = Date.now(); + + await expect(client.listWorkspaces()).rejects.toThrow(); + + expect(Date.now() - startedAt).toBeLessThan(5_000); + }, 20_000); +}); + +function makeBootSplitExec(postReturnScreen: string): ExecFn { + let promptSent = false; + return vi.fn().mockImplementation(async (_cmd, args: string[]) => { + if (args.includes("new-split")) { + return { + stdout: JSON.stringify({ + workspace: "workspace:1", + surface: "surface:2", + pane: "pane:1", + title: "New", + type: "terminal", + }), + stderr: "", + }; + } + if (args.includes("list-panes")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:2"], + selected_surface_ref: "surface:2", + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + pane_ref: "pane:1", + surfaces: [ + { + ref: "surface:2", + title: "mimirClaude", + type: "terminal", + index: 0, + selected: true, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("send") && !args.includes("send-key")) { + promptSent = true; + return { stdout: "{}", stderr: "" }; + } + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: "surface:2", + text: promptSent + ? postReturnScreen + : "previous shell output: bun install\nClaude Code\n> ", + lines: 80, + scrollback_used: false, + }), + stderr: "", + }; + } + return { stdout: "{}", stderr: "" }; + }); +} + +describe("T2 delivery truth — boot consumption evidence (#427)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-boot-tokens-")); + }); + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + const writePrompt = () => { + const promptPath = join(testDir, "boot.md"); + mkdirSync(testDir, { recursive: true }); + writeFileSync(promptPath, "Read and follow the brief", "utf8"); + return promptPath; + }; + + it("refuses submit_verified:true while the booted CLI reports 0 tokens", async () => { + const { createServer } = await loadServerModule(); + // #427's race: the CLI is still initialising, so the screen already reads + // as a working agent while the boot prompt has been consumed by nothing -- + // 0 tokens, $0.00, 0m. + const mockExec = makeBootSplitExec( + [ + "Claude Code", + "> ", + " 0 tokens", + " Opus 5 | $0.00 | 0m", + "Working (1s - esc to interrupt)", + ].join("\n"), + ); + const server = createServer({ exec: mockExec, skipAgentLifecycle: true }); + + const result = await (server as any)._registeredTools[ + "new_split" + ].handler( + { + direction: "right", + workspace: "workspace:1", + boot_prompt_path: writePrompt(), + boot_prompt_timeout_ms: 50, + }, + {} as any, + ); + const parsed = parseToolResult(result); + + // The spawn reports the truth instead of a fully-verified receipt for a + // prompt the agent never consumed. + expect(parsed.ok).toBe(false); + expect(parsed.submit_verified).not.toBe(true); + expect(parsed.delivered).not.toBe(true); + expect(parsed.boot_prompt_delivered).not.toBe(true); + expect(parsed.error).toMatch(/boot prompt submit evidence/i); + }, 20_000); + + it("still verifies a boot prompt once the CLI has consumed tokens", async () => { + const { createServer } = await loadServerModule(); + const mockExec = makeBootSplitExec( + [ + "Claude Code", + "> ", + " 1.2k tokens", + " Opus 5 | $0.03 | 1m", + "Working (1s - esc to interrupt)", + ].join("\n"), + ); + const server = createServer({ exec: mockExec, skipAgentLifecycle: true }); + + const result = await (server as any)._registeredTools[ + "new_split" + ].handler( + { + direction: "right", + workspace: "workspace:1", + boot_prompt_path: writePrompt(), + boot_prompt_timeout_ms: 50, + }, + {} as any, + ); + const parsed = parseToolResult(result); + + expect(parsed.boot_prompt_receipt.submit_verified).toBe(true); + expect(parsed.boot_prompt_receipt.delivered).toBe(true); + }, 20_000); +}); diff --git a/tests/f1-live-state-truth.test.ts b/tests/f1-live-state-truth.test.ts index b9dc18a9..7ffcd9c3 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, @@ -382,7 +449,7 @@ describe("F1 — live state, not the stale registry record", () => { expect(row.closure).toBe("pending"); }); - it("P11 closure still reports artifact_missing when the screen confirms done", async () => { + it("P11 closure still reports artifact_missing when a done was OBSERVED", async () => { client.screens["surface:idle"] = [ "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer", "codex>", diff --git a/tests/global-setup.ts b/tests/global-setup.ts new file mode 100644 index 00000000..7c33d0d0 --- /dev/null +++ b/tests/global-setup.ts @@ -0,0 +1,20 @@ +import { mkdirSync, rmSync } from "node:fs"; +import { join } from "node:path"; + +/** + * One temp root per suite RUN, removed when the run ends. + * + * See tests/vitest.setup.ts for why. Isolating per run — not per worker — is + * exactly right: within a run vitest never executes one test file twice at once, + * so the fixed fixture names only collide ACROSS runs. + */ +const root = join("/tmp", `cmuxlayer-vitest-${process.pid}`); + +export function setup(): void { + mkdirSync(root, { recursive: true }); + process.env.CMUXLAYER_TEST_TMP_ROOT = root; +} + +export function teardown(): void { + rmSync(root, { recursive: true, force: true }); +} diff --git a/tests/live-topology-restart.test.ts b/tests/live-topology-restart.test.ts index 13b73cdd..a6aceb20 100644 --- a/tests/live-topology-restart.test.ts +++ b/tests/live-topology-restart.test.ts @@ -23,12 +23,16 @@ const SERVERS = new Set<{ sockets: Set; }>(); +// This hook compiles the whole project so the live daemon under test is the +// real build. That is a minute's work on a loaded CI runner and ~3s on a warm +// Mac -- vitest's 10s hook default made the file pass locally and time out in +// CI, which is the same green-only-on-one-machine failure this lane exists for. beforeAll(() => { execFileSync(resolve("node_modules", ".bin", "tsc"), ["-p", "tsconfig.json"], { cwd: process.cwd(), stdio: "pipe", }); -}); +}, 300_000); function isRecord(value: unknown): value is Record { return typeof value === "object" && value !== null; diff --git a/tests/pre-pr-scripts.test.ts b/tests/pre-pr-scripts.test.ts index 25d9b43a..ba7acc4c 100644 --- a/tests/pre-pr-scripts.test.ts +++ b/tests/pre-pr-scripts.test.ts @@ -124,3 +124,22 @@ describe("pre-PR script ladder", () => { expect(script).toContain("exec bun run pre-pr"); }); }); + +describe("release scripts run where CI runs", () => { + // `sed -i ''` is BSD-only. GNU sed reads the '' as the script and the real + // expression as a filename, exits 2, and takes release.sh down with it — the + // reason every Linux run of the release-receipt tests failed while the same + // tests passed on the maintainer's Mac. + it("keeps release scripts free of the BSD-only in-place sed form", () => { + for (const script of ["release.sh", "release-verify.sh"]) { + const code = readFileSync(join(repoRoot, "scripts", script), "utf8") + .split("\n") + .filter((line) => !/^\s*#/.test(line)) + .join("\n"); + + expect(code, `${script} uses BSD-only sed -i ''`).not.toMatch( + /sed\s+-i\s+(''|"")/, + ); + } + }); +}); diff --git a/tests/ram-watchdog-warn-only.test.ts b/tests/ram-watchdog-warn-only.test.ts index 1f0d1e2a..2972524d 100644 --- a/tests/ram-watchdog-warn-only.test.ts +++ b/tests/ram-watchdog-warn-only.test.ts @@ -176,7 +176,9 @@ done writeFileSync(join(root, "fixtures/memsize.fixture"), "1048576\n"); } -describe("cmux RAM watchdog warn-only regression", () => { +// Each case runs a real bash script through spawnSync; vitest's 5s default is +// the wrong budget for that and flakes under full-suite load. +describe("cmux RAM watchdog warn-only regression", { timeout: 30_000 }, () => { it("turns a watchdog memory breach into notification/snapshot work without SIGKILLing cmux", () => { const root = makeRoot("cmux-watchdog-vitest-"); const logDir = join(root, "logs"); diff --git a/tests/release-receipts.test.ts b/tests/release-receipts.test.ts index 89e24709..6784cd03 100644 --- a/tests/release-receipts.test.ts +++ b/tests/release-receipts.test.ts @@ -7,6 +7,7 @@ import { mkdtempSync, readFileSync, rmSync, + statSync, writeFileSync, } from "node:fs"; import { tmpdir } from "node:os"; @@ -74,6 +75,8 @@ function makeReleaseFixture( withBrew?: boolean; withBrewTapClone?: boolean; withNode?: boolean; + /** What the stubbed `gh` reports for the released commit; "" = no gh. */ + ciConclusion?: string; } = {}, ): Fixture { const { @@ -82,6 +85,7 @@ function makeReleaseFixture( withBrew = true, withBrewTapClone = true, withNode = true, + ciConclusion = "success", } = opts; const root = makeRoot("cmuxlayer-release-receipts-"); @@ -208,6 +212,17 @@ exit 0 ); } + writeExecutable( + join(binDir, "gh"), + `#!/usr/bin/env bash +printf 'gh %s\\n' "$*" >>"$STUB_LOG" +# An unusable gh (absent, unauthenticated, offline) must read as "unknown", +# never as "clean" -- so this stub fails the way the real one does. +[ -n "$STUB_CI_CONCLUSION" ] || exit 1 +printf '%s\\n' "$STUB_CI_CONCLUSION" +`, + ); + if (!withNode) { writeExecutable( join(binDir, "node"), @@ -227,6 +242,7 @@ exit 127 STUB_BREW_REPO: brewRepo, STUB_INSTALLED_VERSION: installedVersion ?? "", STUB_CONTRACT_OUTPUT: contractOutput, + STUB_CI_CONCLUSION: ciConclusion, CMUXLAYER_TAP_DIR: tapDir, CMUXLAYER_RELEASE_RECEIPTS_DIR: receiptsDir, CMUXLAYER_RECEIPT_HOST: "test-mac", @@ -409,7 +425,9 @@ describe("release receipt ledger CLI", () => { }); }); -describe("release.sh receipts", () => { +// These run the real release scripts end to end through stubbed binaries, so +// vitest's 5s default is the wrong budget and flakes under full-suite load. +describe("release.sh receipts", { timeout: 30_000 }, () => { it("writes a release receipt with version, sha256, commit and gate results", () => { const fixture = makeReleaseFixture(); const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); @@ -432,6 +450,70 @@ describe("release.sh receipts", () => { ); }); + // #490: publish.yml failed on 105 consecutive runs and cmuxlayer never reached + // npm, because a release's own receipt said nothing about CI. It does now. + it("records the CI verdict for the commit being released", () => { + const fixture = makeReleaseFixture(); + const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); + + expect(result.status).toBe(0); + const receipt = readReceipt(fixture, "0.4.1"); + expect(receipt.gates.ci).toBe("success"); + // The verdict names the commit it is ABOUT. The read happens before the + // version bump, so it is the commit the release was cut from, not the tag's. + expect(receipt.gates.ci_commit).toBe("1".repeat(40)); + // `gh run list --commit` matches nothing on an abbreviated sha, so a short + // one would silently read as "unknown". Keep the full form. + expect(result.log).toContain(`gh run list --commit ${"1".repeat(40)}`); + expect(result.stdout).toContain( + `CI: success (ci.yml on ${"1".repeat(40)} — the commit this release was cut from)`, + ); + }); + + it("bumps package.json without changing its file mode", () => { + const fixture = makeReleaseFixture(); + const manifest = join(fixture.repoDir, "package.json"); + chmodSync(manifest, 0o640); + + const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); + + expect(result.status).toBe(0); + expect(statSync(manifest).mode & 0o777).toBe(0o640); + }); + + it("never lets a release read as clean while its CI is red", () => { + const fixture = makeReleaseFixture({ ciConclusion: "failure" }); + const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); + + expect(result.status).toBe(0); + expect(readReceipt(fixture, "0.4.1").gates.ci).toBe("failure"); + expect(result.stdout).toContain("WARNING"); + expect(result.stdout).toContain("CI: failure"); + }); + + it("records an unusable gh as unknown rather than as a pass", () => { + const fixture = makeReleaseFixture({ ciConclusion: "" }); + const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); + + expect(result.status).toBe(0); + expect(readReceipt(fixture, "0.4.1").gates.ci).toBe("unknown"); + expect(result.stdout).toContain("WARNING"); + }); + + it("refuses to release on a non-green CI under --require-ci", () => { + const fixture = makeReleaseFixture({ ciConclusion: "failure" }); + const result = runScript(fixture, "release.sh", [ + "0.4.1", + "--yes", + "--require-ci", + ]); + + expect(result.status).not.toBe(0); + // The die message, not `unknown flag: --require-ci`. + expect(result.stderr).toContain("release: --require-ci"); + expect(result.log).not.toContain("git push origin main"); + }); + it("preserves the happy-path release commands (receipts stay additive)", () => { const fixture = makeReleaseFixture(); const result = runScript(fixture, "release.sh", ["0.4.1", "--yes"]); @@ -595,7 +677,7 @@ describe("release.sh receipts", () => { }); }); -describe("release-verify.sh", () => { +describe("release-verify.sh", { timeout: 30_000 }, () => { it("verify-only never upgrades and never resets Homebrew's tap clone", () => { const fixture = makeReleaseFixture({ installedVersion: "0.4.1" }); const result = runScript(fixture, "release-verify.sh", [ 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/seat-identity.test.ts b/tests/seat-identity.test.ts index 5a380a1e..64a76db7 100644 --- a/tests/seat-identity.test.ts +++ b/tests/seat-identity.test.ts @@ -1,6 +1,11 @@ +import { existsSync } from "node:fs"; +import { homedir, tmpdir } from "node:os"; +import { join } from "node:path"; import { describe, expect, it } from "vitest"; import { assertSeatIdentity, + defaultSeatRegistryPath, + loadSeatRegistryFromConfig, type SeatRegistry, } from "../src/seat-identity.js"; @@ -104,3 +109,27 @@ describe("seat identity uniqueness", () => { }); }); }); + +describe("seat registry source", () => { + it("lets the caller point the seat registry away from the machine's ~/.golems", () => { + const pinned = join(tmpdir(), "cmuxlayer-seat-registry-fixture.yaml"); + + expect( + defaultSeatRegistryPath({ CMUXLAYER_SEAT_REGISTRY_PATH: pinned }), + ).toBe(pinned); + expect(defaultSeatRegistryPath({})).toBe( + join(homedir(), ".golems", "config.yaml"), + ); + }); + + // The suite once asserted `brainClaude` — a seat that exists only in the + // maintainer's ~/.golems/config.yaml. It was green on that Mac and red on + // every CI runner for days. Tests state their own registry or get none. + it("never resolves the seat registry from the machine running the suite", () => { + const pinned = defaultSeatRegistryPath(); + + expect(pinned).not.toBe(join(homedir(), ".golems", "config.yaml")); + expect(existsSync(pinned)).toBe(false); + expect(loadSeatRegistryFromConfig()).toBeNull(); + }); +}); diff --git a/tests/send-to-v2-background-verify.test.ts b/tests/send-to-v2-background-verify.test.ts index 5be8392f..19f59faf 100644 --- a/tests/send-to-v2-background-verify.test.ts +++ b/tests/send-to-v2-background-verify.test.ts @@ -629,9 +629,283 @@ describe("send_to v2 background verify", () => { }); expect(existsSync(ticketDir)).toBe(true); expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + // T2 #471: the local evidence ticket stays; escalating a deadline the + // ENGINE ran out of to the issue tracker does not. + expect(filed).toHaveLength(0); + }); + + it("does not escalate a verify_deadline_elapsed failure to a GitHub issue (#471)", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryVerifyDeadlineMs: 1_000, + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "engine stopped looking", + press_enter: true, + }), + ); + + await vi.advanceTimersByTimeAsync(1_000); + const engine = server._registeredTools.interact._engine; + await engine.verifyPendingDeliveries(); + + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + expect(receipt).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "verify_deadline_elapsed", + }); + // Local forensics are still written -- the verdict must cite its evidence. + expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + // ...but the tracker does not get an issue for the engine giving up. + expect(filed).toHaveLength(0); + expect(receipt.ticket_escalated).toBe(false); + expect(receipt.ticket_escalation_declined_reason).toMatch( + /no evidence the message was lost/i, + ); + }); + + it("does not escalate a target_gone failure to a GitHub issue (#443)", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "target vanished", + press_enter: true, + }), + ); + + const engine = server._registeredTools.interact._engine; + engine.getRegistry().remove("agent-1"); + for (let miss = 0; miss < DELIVERY_TARGET_GONE_CONFIRM_MISSES; miss += 1) { + await engine.verifyPendingDeliveries(); + } + + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + expect(receipt).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "target_gone", + ticket_escalated: false, + }); + expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + expect(filed).toHaveLength(0); + }); + + it("still escalates a failure the verifier positively observed", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "observed lost", + press_enter: true, + }), + ); + + const engine = server._registeredTools.interact._engine; + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + await engine.verifyPendingDeliveries(); + + expect(engine.getDeliveryReceipt(sent.delivery_id)).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "composer_rejected_input", + ticket_escalated: true, + }); expect(filed).toHaveLength(1); }); + it("bounds a wedged snapshot read so the verifier cannot stall forever (#450)", async () => { + let reads = 0; + const client = new FakeAgentSurfaceClient(); + const stateMgr = new StateManager(TEST_DIR); + const engine = new AgentEngine( + stateMgr, + new AgentRegistry(stateMgr, async () => []), + client as any, + { + deliveryVerifyTimeoutMs: 50, + // Mirrors the production verifier: no snapshot, no evidence. + deliveryVerifier: async (_receipt, snapshot) => + snapshot?.text + ? { outcome: "delivered" as const, submit_verified: true } + : { + outcome: "pending" as const, + reason: "surface_read_unavailable", + }, + // The CLI-fallback surface read has no subprocess timeout of its own; + // model a wedged `cmux` on the first pass. + deliverySnapshotReader: async () => { + reads += 1; + if (reads === 1) { + return new Promise(() => {}) as never; + } + return { text: "Claude Code\n> \n" }; + }, + }, + ); + try { + engine.acceptPendingVerify({ + delivery_id: "wedged-read-1", + agent_id: "agent-1", + text: "snapshot read wedges", + press_enter: true, + source_event: "send_to", + retry_count: 0, + }); + + let settled = false; + const first = engine.verifyPendingDeliveries().then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(50); + await first; + + expect(settled).toBe(true); + expect(engine.getDeliveryReceipt("wedged-read-1")).toMatchObject({ + delivery_state: "pending_verify", + terminal: false, + }); + + // The latch was released, so a later sweep still resolves the receipt. + await vi.advanceTimersByTimeAsync(30_000); + await engine.verifyPendingDeliveries(); + expect(reads).toBe(2); + expect(engine.getDeliveryReceipt("wedged-read-1")).toMatchObject({ + delivery_state: "submitted", + terminal: true, + }); + } finally { + engine.dispose(); + } + }); + + it("does not claim escalation when the ticket was deduped, the filer is absent, or the filer threw (B2)", async () => { + const observedFailure = async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + }); + + const sendAndVerify = async (extras: Record) => { + const client = new FakeAgentSurfaceClient(); + const local = createVerifyServer(client, extras as any); + registerAgent(local); + const sent = parseResult( + await callTool(local, "send_to", { + agent_id: "agent-1", + text: `escalation exit ${JSON.stringify(extras).length}`, + press_enter: true, + }), + ); + const engine = local._registeredTools.interact._engine; + engine.setDeliveryVerifier(observedFailure); + await engine.verifyPendingDeliveries(); + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + await local.close(); + return receipt; + }; + + // (1) no filer configured -- nothing can be escalated. + expect( + await sendAndVerify({ deliveryTicketDir: join(TEST_DIR, "t-nofiler") }), + ).toMatchObject({ ticket_filed: true, ticket_escalated: false }); + + // (2) the filer threw -- the issue was not filed. + expect( + await sendAndVerify({ + deliveryTicketDir: join(TEST_DIR, "t-throws"), + deliveryIssueFiler: async () => { + throw new Error("gh unavailable"); + }, + }), + ).toMatchObject({ ticket_filed: true, ticket_escalated: false }); + }); + + it("does not claim escalation for a deduped signature (B2)", async () => { + const ticketDir = join(TEST_DIR, "tickets-dedupe-escalation"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + const engine = server._registeredTools.interact._engine; + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + + const first = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "first observed loss", + press_enter: true, + }), + ); + await engine.verifyPendingDeliveries(); + client.resetTypedInput(); + const second = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "second observed loss", + press_enter: true, + }), + ); + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + await engine.verifyPendingDeliveries(); + + // Same signature: exactly one escalation reached the tracker, and the + // receipt that did NOT reach it says so, with a reason. + expect(filed).toHaveLength(1); + const deduped = engine.getDeliveryReceipt(second.delivery_id); + expect(deduped).toMatchObject({ + ticket_filed: true, + ticket_escalated: false, + }); + expect(deduped.ticket_escalation_declined_reason).toMatch( + /already filed/i, + ); + }); + it("dedupes evidence tickets by failure signature", async () => { const ticketDir = join(TEST_DIR, "tickets"); const filed: unknown[] = []; @@ -665,7 +939,8 @@ describe("send_to v2 background verify", () => { await server._registeredTools.interact._engine.verifyPendingDeliveries(); expect(readdirSync(ticketDir)).toHaveLength(1); - expect(filed).toHaveLength(1); + // T2 #471: deadline-elapsed verdicts are never escalated, deduped or not. + expect(filed).toHaveLength(0); }); it("returns a nonterminal wait_for delivery receipt with timed_out instead of rejecting", async () => { diff --git a/tests/server-agent-tools.test.ts b/tests/server-agent-tools.test.ts index 471d8deb..80ee1fe7 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); @@ -7537,7 +7543,19 @@ describe("agent lifecycle tool handlers", () => { cli_session_id: "claude-session", task_summary: "(auto-discovered)", }); - const server = await createUuidRouteServer(routeClient, record); + // The repaired id is the SEAT, not the launcher, so this test states the + // seat registry it repairs against. Reading the host's ~/.golems/config.yaml + // instead is what made this assertion green on one Mac and red in CI. + const server = await createUuidRouteServer(routeClient, record, { + seatRegistry: { + brainClaude: { + repo: "brainlayer", + lane: "brainlayer", + role: "lead", + launchers: { claude: "brainlayerClaude" }, + }, + }, + }); const listResult = await registeredTestTool(server, "list_agents").handler( {}, {} as any, diff --git a/tests/server.test.ts b/tests/server.test.ts index 4010e025..e9e58652 100644 --- a/tests/server.test.ts +++ b/tests/server.test.ts @@ -315,7 +315,16 @@ async function runWithFakeTimers( let elapsed = 0; let idleTurns = 0; const maxIdleTurns = 10_000; - while (!settled && elapsed < advanceMs && idleTurns < maxIdleTurns) { + // AIDEV-NOTE: advanceMs is how much SIMULATED time the operation is expected + // to need. It used to be the loop's only stop condition, which made it a + // machine budget rather than a semantic one: a turn that advances the clock + // without the handler progressing still spends it, and a loaded CI runner + // interleaves more of those than a warm laptop. Calls that are green here + // failed in CI at `elapsed=3000, idleTurns=0` — out of simulated budget, not + // stuck. Allow a generous multiple; idleTurns is what actually catches an + // operation that never progresses, and it is unaffected by machine speed. + const fakeTimeBudget = Math.max(advanceMs * 10, 30_000); + while (!settled && elapsed < fakeTimeBudget && idleTurns < maxIdleTurns) { // A handler can cross real I/O/event-loop boundaries before scheduling its // next poll. Do not spend its fake-time budget until that timer exists. await new Promise((resolve) => REAL_SET_IMMEDIATE(resolve)); @@ -327,7 +336,7 @@ async function runWithFakeTimers( idleTurns += 1; continue; } - const step = Math.min(50, advanceMs - elapsed); + const step = Math.min(50, fakeTimeBudget - elapsed); await advanceTimers(step); elapsed += step; idleTurns = 0; diff --git a/tests/t1-registry-truth.test.ts b/tests/t1-registry-truth.test.ts new file mode 100644 index 00000000..6aebc4e2 --- /dev/null +++ b/tests/t1-registry-truth.test.ts @@ -0,0 +1,615 @@ +/** + * 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 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, readFileSync, 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 { setResumeArtifactResolver } from "../src/resume-verification.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(() => { + // Restore the suite-wide stub from tests/vitest.setup.ts. + setResumeArtifactResolver(() => "unverifiable"); + 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. + // + // 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", + 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("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( + 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 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 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 { + 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: this.title, + 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 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(); + 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(); + }); +}); + +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"); + } + }); +}); + +/** + * #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"); + }); +}); diff --git a/tests/t1b-closure-probe-divergence.test.ts b/tests/t1b-closure-probe-divergence.test.ts new file mode 100644 index 00000000..b8f5bd63 --- /dev/null +++ b/tests/t1b-closure-probe-divergence.test.ts @@ -0,0 +1,503 @@ +/** + * Lane T1b (#488): ONE resolution per `list_agents` response. + * + * `closure` used to derive from the discovery-cache probe (null once the cache + * is 2000ms old) while the SAME row's `state` derived from the live scan the + * call had just taken. Cache warm, the two agreed; cache cold, one row said + * `state: working` and `closure: artifact_missing` -- "route a reviewer NOW" + * against an agent mid-turn -- and the field flapped as the cache aged. + * + * These tests drive the divergence directly: the probe is poisoned to its + * COLD shape (registry fallback) while the response's own scan sees the live + * screen. Closure must follow the scan, not the cold probe. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdirSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { createServer } from "../src/server.js"; +import { resolveLiveAgentState } from "../src/live-agent-state.js"; +import type { StateManager } from "../src/state-manager.js"; +import type { AgentRecord } from "../src/agent-types.js"; + +const TEST_DIR = join(tmpdir(), "cmux-t1b-closure-probe-divergence-test"); +const TEST_OBSERVER_OWNER = "cmux:/tmp/cmux-t1b-closure-probe.sock"; + +const READY_CODEX_SCREEN = [ + "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer", + "codex>", +].join("\n"); +const WORKING_CODEX_SCREEN = [ + "gpt-5.5 xhigh · 99% left · ~/Gits/cmuxlayer", + "• Working (12s • esc to interrupt)", + "codex>", +].join("\n"); + +function parseResult(result: any): any { + return result.structuredContent ?? JSON.parse(result.content[0].text); +} + +async function callTool( + server: any, + name: string, + args: Record, +) { + const tool = server._registeredTools[name]; + if (!tool) throw new Error(`Tool not found: ${name}`); + return tool.handler(args, {} as any); +} + +class LiveSurfaceClient { + readonly workspace = "workspace:1"; + readonly pane = "pane:1"; + readonly readySurface = "surface:ready"; + readonly workingSurface = "surface:working"; + readonly screens: Record = { + "surface:ready": READY_CODEX_SCREEN, + "surface:working": WORKING_CODEX_SCREEN, + }; + + 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: this.pane, + index: 0, + focused: true, + surface_count: 2, + surface_refs: [this.readySurface, this.workingSurface], + selected_surface_ref: this.readySurface, + }, + ], + }; + } + + async listPaneSurfaces() { + return { + workspace_ref: this.workspace, + window_ref: "window:1", + pane_ref: this.pane, + surfaces: [ + { + ref: this.readySurface, + title: "cmuxlayerCodex-ready", + type: "terminal", + index: 0, + selected: true, + }, + { + ref: this.workingSurface, + title: "cmuxlayerCodex-working", + type: "terminal", + index: 1, + selected: false, + }, + ], + }; + } + + async send() {} + async sendKey() {} + + readScreenCalls = 0; + + async readScreen(surface: string, opts?: { lines?: number }) { + const text = this.screens[surface]; + if (text == null) throw new Error(`Unknown surface: ${surface}`); + this.readScreenCalls += 1; + return { surface, text, lines: opts?.lines ?? 30, scrollback_used: false }; + } + + async renameTab() {} +} + +function createLiveServer(client: LiveSurfaceClient) { + return createServer({ + client: client as any, + stateDir: TEST_DIR, + disableSpawnPreflight: true, + surfaceObserverOwnerIdProvider: () => TEST_OBSERVER_OWNER, + surfaceObserverEpochProvider: () => `${TEST_OBSERVER_OWNER}@test`, + }); +} + +function makeAgent( + overrides: Partial & + Pick, +): AgentRecord { + const now = "2026-08-19T13:40:00.000Z"; + return { + workspace_id: "workspace:1", + surface_observer_id: TEST_OBSERVER_OWNER, + state: "idle", + repo: "cmuxlayer", + model: "gpt-5.5", + cli: "codex", + cli_session_id: null, + task_summary: "t1b closure divergence", + pid: null, + version: 1, + created_at: now, + updated_at: now, + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "worker", + task_done_candidate_at: null, + task_done_detected_at: null, + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + crash_recover: false, + respawn_attempts: 0, + user_killed: false, + paused: false, + paused_source: null, + ...overrides, + } as AgentRecord; +} + +function testEngine(server: any) { + return server._registeredTools["interact"]._engine; +} + +function registerAgent(server: any, record: AgentRecord): AgentRecord { + const engine = testEngine(server); + const stateMgr = engine["stateMgr"] as StateManager; + stateMgr.writeState(record); + engine.getRegistry().set(record.agent_id, record); + return record; +} + +/** The cold-cache probe: `cachedScan()` returned null, so no screen evidence. */ +function poisonProbeCold(server: any): void { + testEngine(server).setLiveStateResolver((agent: AgentRecord) => + resolveLiveAgentState(agent, null), + ); +} + +function rowFor(parsed: any, agentId: string): any { + return parsed.agents.find((agent: any) => agent.agent_id === agentId); +} + +function disposeServer(server: any) { + const engine = server?._registeredTools?.interact?._engine; + if (engine && typeof engine.dispose === "function") engine.dispose(); +} + +describe("T1b (#488) — closure and state resolve from ONE observation", () => { + let server: any; + let client: LiveSurfaceClient; + + beforeEach(async () => { + rmSync(TEST_DIR, { recursive: true, force: true }); + mkdirSync(TEST_DIR, { recursive: true }); + client = new LiveSurfaceClient(); + server = createLiveServer(client); + }); + + afterEach(() => { + disposeServer(server); + rmSync(TEST_DIR, { recursive: true, force: true }); + }); + + it("working screen + COLD closure probe reads pending, never artifact_missing", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-busy", + surface_id: client.workingSurface, + // #408's lie plus real prior done evidence: even THAT must not outrank + // a screen this same response read as mid-turn. + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "busy.md"), + done_marker: "DONE_T1B_BUSY", + } as Partial as any), + ); + + // First call warms lifecycle start (which installs the real probe). + await callTool(server, "list_agents", {}); + poisonProbeCold(server); + + const parsed = parseResult(await callTool(server, "list_agents", {})); + const row = rowFor(parsed, "cmuxlayerCodex-t1b-busy"); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + expect(row.state.value).toBe("working"); + expect(row.state.source).toBe("screen"); + expect(row.closure).toBe("pending"); + }); + + it("ready screen + stale-done record with NO done evidence reads pending", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-flip", + surface_id: client.readySurface, + // The bare #408 record flip: `done` with nothing that ever observed a + // done. `artifact_missing` from this alone is the false deadlock. + state: "done", + task_done_detected_at: null, + report_path: join(TEST_DIR, "reports", "flip.md"), + done_marker: "DONE_T1B_FLIP", + } as Partial as any), + ); + + const parsed = parseResult(await callTool(server, "list_agents", {})); + const row = rowFor(parsed, "cmuxlayerCodex-t1b-flip"); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + expect(row.closure).toBe("pending"); + expect(row.closure).not.toBe("artifact_missing"); + }); + + it("the deadlock signal SURVIVES: done evidence + missing report is artifact_missing", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-deadlocked", + surface_id: client.readySurface, + state: "done", + // Positive done evidence: the sweep observed the done, then no report. + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "never-written.md"), + done_marker: "DONE_T1B_DEADLOCK", + } as Partial as any), + ); + + const parsed = parseResult(await callTool(server, "list_agents", {})); + const row = rowFor(parsed, "cmuxlayerCodex-t1b-deadlocked"); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + expect(row.closure).toBe("artifact_missing"); + }); + + it("costs ZERO extra screen reads: one scan per call, both fields off it (#425)", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-cost", + surface_id: client.workingSurface, + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "cost.md"), + done_marker: "DONE_T1B_COST", + } as Partial as any), + ); + + await callTool(server, "list_agents", {}); + const before = client.readScreenCalls; + const parsed = parseResult(await callTool(server, "list_agents", {})); + const reads = client.readScreenCalls - before; + + // Two surfaces exist, so one scan is two reads. Threading that scan into + // closure adds none: the alternative -- forcing fresh evidence on the + // closure path -- would have added one read per row per call. + expect(rowFor(parsed, "cmuxlayerCodex-t1b-cost")?.closure).toBe("pending"); + expect(reads).toBe(Object.keys(client.screens).length); + }); + + it("get_agent_state does not render artifact_missing beside a working screen", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-getstate", + surface_id: client.workingSurface, + // The residual shape the reviewer reproduced: a done that WAS once + // observed, on an agent that is demonstrably working again. + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "getstate.md"), + done_marker: "DONE_T1B_GETSTATE", + } as Partial as any), + ); + + await callTool(server, "list_agents", {}); + poisonProbeCold(server); + + const parsed = parseResult( + await callTool(server, "get_agent_state", { + agent_id: "cmuxlayerCodex-t1b-getstate", + }), + ); + expect(parsed.health?.reconciled_state, JSON.stringify(parsed)).toBe( + "working", + ); + expect(parsed.harvestability.closure).toBe("pending"); + }); + + it("wait_for does not render artifact_missing beside a working screen", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-waitfor", + surface_id: client.workingSurface, + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "waitfor.md"), + done_marker: "DONE_T1B_WAITFOR", + } as Partial as any), + ); + + await callTool(server, "list_agents", {}); + poisonProbeCold(server); + + const parsed = parseResult( + await callTool(server, "wait_for", { + ids: ["cmuxlayerCodex-t1b-waitfor"], + target_state: "done", + timeout_ms: 2000, + }), + ); + const result = parsed.results[0]; + expect(result.health?.reconciled_state, JSON.stringify(parsed)).toBe( + "working", + ); + expect(result.closure).toBe("pending"); + }); + + it("the deadlock signal survives on ALL THREE emitters, not just list_agents", async () => { + // The false-negative the reviewer named: narrowing `artifact_missing` must + // not silence the case it exists for. Done evidence, ready prompt, report + // never written -- every path that emits closure must still say so. + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-realdeadlock", + surface_id: client.readySurface, + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "real-deadlock.md"), + done_marker: "DONE_T1B_REALDEADLOCK", + } as Partial as any), + ); + + const listed = parseResult(await callTool(server, "list_agents", {})); + const state = parseResult( + await callTool(server, "get_agent_state", { + agent_id: "cmuxlayerCodex-t1b-realdeadlock", + }), + ); + const waited = parseResult( + await callTool(server, "wait_for", { + ids: ["cmuxlayerCodex-t1b-realdeadlock"], + target_state: "done", + timeout_ms: 2000, + }), + ); + + expect( + rowFor(listed, "cmuxlayerCodex-t1b-realdeadlock")?.closure, + JSON.stringify(listed), + ).toBe("artifact_missing"); + expect(state.harvestability.closure).toBe("artifact_missing"); + expect(waited.results[0].closure).toBe("artifact_missing"); + }); + + it("narrowing artifact_missing silences the CLAIM, not the evidence: health still flags it", async () => { + // The reviewer's named false-negative: a worker that finished without a + // recognized done marker looks identical to a #408 flip. Closure withholds + // the deadlock CLAIM there -- but `closure_artifact_verified:false` and the + // blocking `closure_without_artifact` health issue still render, so the + // population is auditable rather than invisible. This is the post-merge + // signal to watch: records at `done` with `closure:"pending"` and + // `done_source:"none"`. + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-unobserved", + surface_id: client.readySurface, + state: "done", + task_done_detected_at: null, + report_path: join(TEST_DIR, "reports", "unobserved.md"), + done_marker: "DONE_T1B_UNOBSERVED", + } as Partial as any), + ); + + const parsed = parseResult( + await callTool(server, "get_agent_state", { + agent_id: "cmuxlayerCodex-t1b-unobserved", + }), + ); + expect(parsed.harvestability.closure, JSON.stringify(parsed)).toBe( + "pending", + ); + expect(parsed.harvestability.closure_artifact_verified).toBe(false); + expect(parsed.harvestability.evidence_channel.done_source).toBe("none"); + expect(parsed.health.issue_codes).toContain("closure_without_artifact"); + }); + + it("detail:full health carries the SAME resolution, not a third one", async () => { + // The health block re-derived harvestability through the probe, so a cold + // cache could fire the blocking `closure_without_artifact` issue on the + // very row whose `closure` reads `pending` -- the divergence moved one + // field over. + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-health", + surface_id: client.workingSurface, + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "health.md"), + done_marker: "DONE_T1B_HEALTH", + } as Partial as any), + ); + + await callTool(server, "list_agents", {}); + poisonProbeCold(server); + + const parsed = parseResult( + await callTool(server, "list_agents", { detail: "full" }), + ); + const row = rowFor(parsed, "cmuxlayerCodex-t1b-health"); + expect(row, JSON.stringify(parsed)).toBeTruthy(); + expect(row.closure).toBe("pending"); + expect(row.detail.harvestability.closure).toBe("pending"); + expect(row.health.issue_codes).not.toContain("closure_without_artifact"); + }); + + it("three consecutive calls for an unchanged agent do not flap the closure", async () => { + registerAgent( + server, + makeAgent({ + agent_id: "cmuxlayerCodex-t1b-flap", + surface_id: client.workingSurface, + state: "done", + task_done_detected_at: "2026-08-19T13:41:00.000Z", + report_path: join(TEST_DIR, "reports", "flap.md"), + done_marker: "DONE_T1B_FLAP", + } as Partial as any), + ); + + // The reported flap (`db1ff995`, working→done→working in 17s) is the probe + // cache expiring between calls while nothing about the agent changed. + const first = parseResult(await callTool(server, "list_agents", {})); + poisonProbeCold(server); + const second = parseResult(await callTool(server, "list_agents", {})); + const third = parseResult(await callTool(server, "list_agents", {})); + + const closures = [first, second, third].map( + (parsed) => rowFor(parsed, "cmuxlayerCodex-t1b-flap")?.closure, + ); + expect(closures, JSON.stringify(closures)).toEqual([ + "pending", + "pending", + "pending", + ]); + }); +}); diff --git a/tests/t2b-silent-failures.test.ts b/tests/t2b-silent-failures.test.ts new file mode 100644 index 00000000..ef498899 --- /dev/null +++ b/tests/t2b-silent-failures.test.ts @@ -0,0 +1,504 @@ +/** + * T2b — silent failure with a success receipt. + * + * Two live-reproduced defects, both the same disease: a tool returns ok:true + * for an action it did not perform, or performed only half of. + * + * - #484 send_to(mode:"key") returns ok:true with submit_attempted:false and no + * evidence that anything reached the pane. The documented type -> Return + * recovery therefore reports success for a no-op and the message is lost. + * - #485 close_surface(scope:"agent") delegates to stop_agent and returns + * ok:true state:"done" while the pane stays open. + * + * Every test here fails against the pre-fix tree; see the PR body for the + * red run. + */ +import { describe, expect, it, vi, beforeEach, afterEach } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { createServer } from "../src/server.js"; +import type { ExecFn } from "../src/cmux-client.js"; +import type { AgentRecord } from "../src/agent-types.js"; +import { + getEngine, + getTool, + type ToolCallResult, +} from "./helpers/mcp-tool-harness.js"; + +/** + * Both defects are about what a FAILING receipt says, so these tests read the + * payload of ok and errored results alike — `parseToolResult` throws on the + * error results half of this suite exists to inspect. + */ +function payload(result: ToolCallResult): Record { + return (result.structuredContent ?? + JSON.parse(result.content[0]?.text ?? "{}")) as Record; +} + +const SURFACE = "surface:89"; + +const IDLE_CLAUDE_SCREEN = "Claude Code\nWhat can I help you with?\n> "; +const POPULATED_CLAUDE_SCREEN = + "Claude Code\nWhat can I help you with?\n> lead: please pick up lane T4"; +const WORKING_CLAUDE_SCREEN = "Claude Code\n✻ Working (3s · esc to interrupt)"; +// Working, with the composer visibly empty: the submit was taken up. +const WORKING_AND_CLEARED_CLAUDE_SCREEN = + "Claude Code\n✻ Working (3s · esc to interrupt)\n> "; +// The reported scenario, and the fixture the first round of this lane missed: +// the recipient is busy on its previous turn WHILE the relayed message sits +// unsent in its composer. Status and composer disagree, and the composer wins. +const WORKING_AND_POPULATED_CLAUDE_SCREEN = + "Claude Code\n✻ Working (3s · esc to interrupt)\n> lead: please pick up lane T4"; + +/** + * Minimal cmux CLI mock. `screen` is a live box so a test can change what the + * pane shows after the key is dispatched, which is the whole point of a + * submit-verification test. Closing a surface removes it from the topology, so + * a test can assert the pane is really gone rather than only that a command ran. + */ +function makeExec(opts?: { + screen?: () => string; + onSendKey?: (key: string) => void; + closeSurfaceFails?: boolean; + /** Model a cmux that accepts close-surface but keeps listing the pane. */ + closeLeavesSurfaceListed?: boolean; +}): ExecFn & { calls: string[][] } { + const calls: string[][] = []; + let surfaceLive = true; + const exec = vi.fn().mockImplementation(async (_cmd, args: string[]) => { + calls.push(args); + if (args.includes("list-workspaces")) { + return { + stdout: JSON.stringify({ + workspaces: [ + { + ref: "workspace:1", + title: "Fleet", + index: 0, + selected: true, + pinned: false, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-panes")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + workspace: "workspace:1", + focused: true, + surface_count: surfaceLive ? 1 : 0, + surface_refs: surfaceLive ? [SURFACE] : [], + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + pane_ref: "pane:1", + workspace_ref: "workspace:1", + surfaces: surfaceLive + ? [ + { + ref: SURFACE, + pane: "pane:1", + workspace: "workspace:1", + title: "golemsClaude", + type: "terminal", + selected: true, + }, + ] + : [], + }), + stderr: "", + }; + } + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: SURFACE, + text: opts?.screen?.() ?? "$ ", + lines: 30, + scrollback_used: false, + }), + stderr: "", + }; + } + if (args.includes("send-key")) { + opts?.onSendKey?.(String(args[args.length - 1] ?? "")); + return { stdout: "{}", stderr: "" }; + } + if (args.includes("close-surface")) { + if (opts?.closeSurfaceFails) { + throw new Error("cmux close-surface: pane is not closable"); + } + if (!opts?.closeLeavesSurfaceListed) { + surfaceLive = false; + } + return { stdout: "{}", stderr: "" }; + } + return { stdout: "{}", stderr: "" }; + }) as ExecFn & { calls: string[][] }; + exec.calls = calls; + return exec; +} + +let stateDir: string; + +beforeEach(() => { + stateDir = mkdtempSync(join(tmpdir(), "cmux-t2b-")); +}); + +afterEach(() => { + rmSync(stateDir, { recursive: true, force: true }); +}); + +function makeServer(exec: ExecFn) { + return createServer({ + exec, + stateDir, + disableSpawnPreflight: true, + controlHealthIntervalMs: 0, + sessionIdentityResolver: () => null, + }) as unknown; +} + +async function sendKey(server: unknown, key: string): Promise { + return (await getTool(server, "send_to").handler( + { mode: "key", target: SURFACE, key }, + {}, + )) as ToolCallResult; +} + +describe("#484 — send_to(mode:key) must not report success for an unattempted submit", () => { + it.each(["return", "Return", "enter", "Enter", "KPEnter", "ctrl-m"])( + "recognises %j as a submit key and states that it reached the pane", + async (key) => { + const exec = makeExec({ screen: () => WORKING_AND_CLEARED_CLAUDE_SCREEN }); + const result = await sendKey(makeServer(exec), key); + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.submit_attempted).toBe(true); + expect(data.key_dispatched).toBe(true); + }, + ); + + it("dispatches the caller's key verbatim rather than rewriting it for cmux", async () => { + // The receipt is what had to become truthful; the bytes cmux receives are + // not this lane's to change, and "\n" in particular is shift+enter. + const exec = makeExec({ screen: () => WORKING_AND_CLEARED_CLAUDE_SCREEN }); + const result = await sendKey(makeServer(exec), "Enter"); + + expect(result.isError).toBeUndefined(); + expect(payload(result).key).toBe("Enter"); + expect( + exec.calls.some( + (args) => args.includes("send-key") && args.includes("Enter"), + ), + ).toBe(true); + }); + + it("verifies the submit landed when the composer clears", async () => { + let pressed = false; + const exec = makeExec({ + screen: () => + pressed ? WORKING_AND_CLEARED_CLAUDE_SCREEN : POPULATED_CLAUDE_SCREEN, + onSendKey: () => { + pressed = true; + }, + }); + + const result = await sendKey(makeServer(exec), "return"); + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.submit_verified).toBe(true); + expect(data.submit_verification_reason).toBeNull(); + }); + + it("fails instead of returning ok:true when the composer still holds the unsent text", async () => { + // The exact #484 shape: a lead typed a message, it sat unsent, and the + // documented mode:"key" recovery reported success while the pane never moved. + const exec = makeExec({ screen: () => POPULATED_CLAUDE_SCREEN }); + + const result = await sendKey(makeServer(exec), "return"); + + const data = payload(result); + expect(result.isError).toBe(true); + expect(data.ok).toBe(false); + expect(data.submit_verified).toBe(false); + expect(data.submit_verification_reason).toBe("composer_still_populated"); + }); + + it("lets a populated composer veto a working status instead of losing the race to it", async () => { + // The reported target was BUSY: working on its previous turn while the + // relayed message sat unsent. Reading "working" as proof of submit reported + // submit_verified:true for a message still visible on screen — worse than + // the null it replaced, because null admits ignorance and true asserts an + // observation the pane contradicts. + const exec = makeExec({ + screen: () => WORKING_AND_POPULATED_CLAUDE_SCREEN, + }); + + const result = await sendKey(makeServer(exec), "return"); + + const data = payload(result); + expect(result.isError).toBe(true); + expect(data.ok).toBe(false); + expect(data.submit_verified).toBe(false); + expect(data.submit_verification_reason).toBe("composer_still_populated"); + }); + + it("will not treat a working status as proof when the composer cannot be read", async () => { + // A composer that renders boxed reads as unreadable, and the reported + // target was ALREADY working — so status cannot tell "my submit started a + // turn" from "a turn was already running". Unconfirmed says unconfirmed. + const exec = makeExec({ screen: () => WORKING_CLAUDE_SCREEN }); + + const result = await sendKey(makeServer(exec), "return"); + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.key_dispatched).toBe(true); + expect(data.submit_verified).toBeNull(); + expect(data.submit_verification_reason).toBe("submit_evidence_absent"); + }); + + it("leaves non-submit keys unverified but still states that they were dispatched", async () => { + const exec = makeExec({ screen: () => POPULATED_CLAUDE_SCREEN }); + + const result = await sendKey(makeServer(exec), "escape"); + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.submit_attempted).toBe(false); + expect(data.key_dispatched).toBe(true); + expect(data.submit_verified).toBeNull(); + }); +}); + +function seedAgent(server: unknown, overrides: Partial = {}) { + const engine = getEngine(server) as unknown as { + stateMgr: { writeState(record: AgentRecord): unknown }; + getRegistry(): { set(id: string, record: AgentRecord): unknown }; + }; + const record: AgentRecord = { + agent_id: "golemsClaude-bcc283d3", + surface_id: SURFACE, + workspace_id: "workspace:1", + state: "done", + repo: "golems", + model: "claude-opus-5", + cli: "claude", + cli_session_id: null, + cli_session_path: null, + task_summary: "lane worker", + pid: null, + version: 1, + created_at: "2026-08-19T07:00:00.000Z", + updated_at: "2026-08-19T07:00:00.000Z", + error: null, + parent_agent_id: null, + spawn_depth: 0, + role: "worker", + auto_archive_on_done: false, + task_done_candidate_at: null, + task_done_detected_at: "2026-08-19T07:00:00.000Z", + deletion_intent: false, + quality: "unknown", + max_cost_per_agent: null, + crash_recover: false, + respawn_attempts: 0, + user_killed: false, + boot_prompt_pending: false, + goal_file: null, + launch_cwd: null, + mcp_profile: null, + worktree_path: null, + worktree_branch: null, + ...overrides, + }; + engine.stateMgr.writeState(record); + engine.getRegistry().set(record.agent_id, record); + return record; +} + +async function listSurfaceRefs(server: unknown): Promise { + const result = (await getTool(server, "list_surfaces").handler( + {}, + {}, + )) as ToolCallResult; + const text = JSON.stringify(payload(result)); + return text.includes(SURFACE) ? [SURFACE] : []; +} + +describe("#485 — close_surface(scope:agent) must close the surface or say it did not", () => { + it("closes the pane under agent scope — the surface is gone from list_surfaces", async () => { + // The mechanism (confirmed in source, server.ts:9492-9510 pre-fix): + // scope:"agent" delegated to stop_agent and NO path in that branch closed + // the surface, so ok:true/state:"done" was truthful about the agent and + // silent about the pane. Asserted against list_surfaces, not the receipt. + const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + const server = makeServer(exec); + const record = seedAgent(server); + expect(await listSurfaceRefs(server)).toContain(SURFACE); + + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolCallResult; + + expect(result.isError).toBeUndefined(); + expect(await listSurfaceRefs(server)).not.toContain(SURFACE); + expect(payload(result).surface_closed).toBe(true); + }); + + it("never reads like a completed close while the pane is still listed", async () => { + // cmux accepted the command; the pane is still there. The receipt must not + // claim a closure it can see did not happen. + const exec = makeExec({ + screen: () => IDLE_CLAUDE_SCREEN, + closeLeavesSurfaceListed: true, + }); + const server = makeServer(exec); + const record = seedAgent(server); + + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolCallResult; + + const data = payload(result); + expect(data.surface_closed).toBe(false); + expect(String(data.WARNING)).toMatch(/still listed|not.*closed/i); + expect(await listSurfaceRefs(server)).toContain(SURFACE); + }); + + it("stops reporting the agent as working once its close has been acknowledged", async () => { + // golemsClaude's datum, and the one that stands regardless of latency: + // list_agents reported an agent "working" AFTER its close was acknowledged. + // A state claim inside the settle window is still a state claim, and it was + // wrong, and independent of which scope was used. Driven through + // scope:"surface" because that is where the record marking lives; + // scope:"agent" routes into the same code. + const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + const server = makeServer(exec); + const record = seedAgent(server, { state: "working" }); + + const closeRes = (await getTool(server, "close_surface").handler( + { scope: "surface", surface: SURFACE, force: true }, + {}, + )) as ToolCallResult; + expect(payload(closeRes).surface_closed).toBe(true); + + const listed = (await getTool(server, "list_agents").handler( + {}, + {}, + )) as ToolCallResult; + const agents = (payload(listed).agents ?? []) as Array<{ + agent_id: string; + state?: { value?: string }; + }>; + // Whether the closed agent is still listed at all depends on when the + // lifecycle sweep runs, so do not pin that. The claim under test is the + // STATE: nothing may report this agent as working once its close was + // acknowledged. Checked at the record too, so an absent row cannot make + // this pass vacuously. + const closed = agents.find((a) => a.agent_id === record.agent_id); + expect(closed?.state?.value).not.toBe("working"); + const engine = getEngine(server) as unknown as { + getAgentState(id: string): { state?: string } | null; + }; + expect(engine.getAgentState(record.agent_id)?.state).not.toBe("working"); + }); + + it("refuses to report success when the agent stopped but the pane survived", async () => { + const exec = makeExec({ + screen: () => IDLE_CLAUDE_SCREEN, + closeSurfaceFails: true, + }); + const server = makeServer(exec); + const record = seedAgent(server); + + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolCallResult; + + const data = payload(result); + expect(result.isError).toBe(true); + expect(data.ok).toBe(false); + expect(data.agent_stopped).toBe(true); + expect(data.surface_closed).toBe(false); + expect(String(data.error)).toMatch(/surface/i); + expect(await listSurfaceRefs(server)).toContain(SURFACE); + }); + + it("states plainly that there was no surface to close rather than implying one closed", async () => { + const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + const server = makeServer(exec); + const record = seedAgent(server, { + agent_id: "golemsClaude-c7738dab", + surface_id: "", + }); + + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolCallResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.surface_closed).toBe(false); + expect(data.surface_close_skipped).toBe("no_surface_bound"); + }); + + it("does not escalate to a forced close: an unforced call leaves a live agent's pane alone", async () => { + // Reviewer catch: stop_agent has no liveness refusal of its own, so forcing + // the inner close unconditionally would let an UNFORCED close_surface tear + // down a still-live agent's pane through a guard that could never fire for + // this scope. The caller's own force is passed through instead. + const exec = makeExec({ screen: () => WORKING_CLAUDE_SCREEN }); + const server = makeServer(exec); + const record = seedAgent(server, { state: "working" }); + + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolCallResult; + + const data = payload(result); + expect(data.surface_closed).toBe(false); + expect(await listSurfaceRefs(server)).toContain(SURFACE); + }); + + it("cross-checks scope=workspace: the delegate really deletes, and says so", async () => { + const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + const server = makeServer(exec); + + const result = (await getTool(server, "close_surface").handler( + { scope: "workspace", workspace: "workspace:1", force: true }, + {}, + )) as ToolCallResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.workspace_deleted).toBe(true); + expect( + exec.calls.some( + (args) => args.includes("workspace") && args.includes("close"), + ), + ).toBe(true); + }); +}); diff --git a/tests/vitest.setup.ts b/tests/vitest.setup.ts index b72bbac7..bdd878cf 100644 --- a/tests/vitest.setup.ts +++ b/tests/vitest.setup.ts @@ -1,3 +1,46 @@ +import { mkdirSync } from "node:fs"; +import { join } from "node:path"; +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"; + +// AIDEV-NOTE: the seat registry (~/.golems/config.yaml) names the seats THIS +// operator runs. A test that reads it asserts against the host's fleet, so it +// passes on one Mac and fails everywhere else — which is exactly how a +// `brainClaude` assertion stayed green locally while CI was red for six days. +// Pin it at a path that cannot exist: tests state their own registry or get none. +process.env.CMUXLAYER_SEAT_REGISTRY_PATH = join( + __dirname, + "fixtures", + "no-seat-registry-on-this-machine.yaml", +); + +// AIDEV-NOTE: 63 test files build fixtures at a FIXED name under os.tmpdir() +// (`cmux-agents-test-engine`, `cmux-agents-test-registry`, …) and rmSync that +// path in afterEach. Two suite runs on one machine — two worktrees, or a fleet +// worker testing beside the maintainer — then share those directories and tear +// each other's down mid-test: ENOTEMPTY, plus assertions that quietly read +// another run's state. Measured on one file, run twice at once: 91 and 103 +// failures without this, 0 and 0 with it. One temp root per RUN makes every one +// of those fixed names unique per run without touching 63 files. +// +// The root goes under /tmp, not under macOS's `/var/folders/…/T` default: unix +// socket paths cap at ~104 bytes and several suites bind sockets inside a temp +// dir, so a deeper root breaks them. /tmp/cmuxlayer-vitest- is HALF the +// length of the macOS default — this buys socket headroom rather than spending it. +// tests/global-setup.ts creates this root and removes it when the run ends. +const root = + process.env.CMUXLAYER_TEST_TMP_ROOT?.trim() || + join("/tmp", `cmuxlayer-vitest-${process.ppid}`); +mkdirSync(root, { recursive: true }); +process.env.TMPDIR = root; +process.env.TMP = root; +process.env.TEMP = root; +// #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"); diff --git a/tests/workflow-toolchain.test.ts b/tests/workflow-toolchain.test.ts new file mode 100644 index 00000000..f06b73c6 --- /dev/null +++ b/tests/workflow-toolchain.test.ts @@ -0,0 +1,81 @@ +import { readFileSync, readdirSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +const repoRoot = join(__dirname, ".."); +const workflowDir = join(repoRoot, ".github", "workflows"); + +interface Job { + label: string; + source: string; +} + +/** Every job, across every workflow, that runs the cmuxlayer test suite. */ +function suiteJobs(): Job[] { + const jobs: Job[] = []; + + for (const file of readdirSync(workflowDir)) { + if (!file.endsWith(".yml") && !file.endsWith(".yaml")) continue; + const lines = readFileSync(join(workflowDir, file), "utf8").split("\n"); + + let name: string | null = null; + let body: string[] = []; + const flush = () => { + if (name) jobs.push({ label: `${file}:${name}`, source: body.join("\n") }); + name = null; + body = []; + }; + + for (const line of lines) { + const header = line.match(/^ {2}([A-Za-z0-9_-]+):\s*$/); + if (header) { + flush(); + name = header[1]; + continue; + } + if (name) body.push(line); + } + flush(); + } + + // Match the job BODY, not the `run:` line: `run: |` with the invocation on the + // next line is the ordinary Actions idiom, and a filter anchored to `run:` + // walks straight past it — along with composite and reusable workflows. + return jobs.filter(({ source }) => /\b(npm|bun) (run )?test\b/.test(source)); +} + +/** + * AIDEV-NOTE: publish.yml ran the suite on setup-node alone for 105 releases. + * The suite spawns `bun` (tests/fleet-sidebar.test.ts) and release.sh shells out + * to `bun run`, so a bun-less runner fails on missing toolchain rather than on + * anything about the code. Nobody read the log, so cmuxlayer never reached npm. + */ +describe("workflow toolchain matches what the suite spawns", () => { + it("finds the jobs that run the suite", () => { + expect(suiteJobs().map((job) => job.label)).toContain("publish.yml:publish"); + }); + + it("gives every suite-running job the bun the tests spawn", () => { + for (const { label, source } of suiteJobs()) { + expect(source, `${label} runs the suite without installing bun`).toContain( + "oven-sh/setup-bun", + ); + } + }); + + it("never runs the suite on a node older than package.json engines", () => { + const engines: string = JSON.parse( + readFileSync(join(repoRoot, "package.json"), "utf8"), + ).engines.node; + const minimumMajor = Number(engines.replace(/[^0-9.]/g, "").split(".")[0]); + + for (const { label, source } of suiteJobs()) { + for (const [, version] of source.matchAll(/node-version:\s*'?"?(\d+)/g)) { + expect( + Number(version), + `${label} pins node ${version} but engines require ${engines}`, + ).toBeGreaterThanOrEqual(minimumMajor); + } + } + }); +}); diff --git a/vitest.config.ts b/vitest.config.ts index 124d44d7..84103527 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -3,6 +3,7 @@ import { configDefaults, defineConfig } from "vitest/config"; export default defineConfig({ test: { exclude: [...configDefaults.exclude, "**/.worktrees/**"], + globalSetup: ["./tests/global-setup.ts"], setupFiles: ["./tests/vitest.setup.ts"], }, }); From 03307aa346def951377ad0433ac1a53787bb70bf Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Thu, 20 Aug 2026 08:11:46 +0300 Subject: [PATCH 5/5] fix(f1b): delete dead closure helpers and two merge duplicate-keys Review findings on #478, both of which the suite could not see. 1. closureStateOf and hasPositiveDoneEvidence had zero callers after the #494 merge moved closure onto main's effectiveState line (#488). Deleting them leaves the suite byte-identical -- they compiled only because tsconfig has no noUnusedLocals. The 20-line AIDEV-NOTE above closureStateOf still asserted "the one rule a response may use", so the next reader greping for the closure rule found an authoritative comment on unreachable code. Round 3's fix 1 was superseded by #488's doneEvidence gate in the merge. 2. Two both-sides-kept conflict artifacts from the main merge, invisible to `bun run typecheck` because tsconfig excludes tests/ (now #502): - coordination-paths: doneEvidence twice, same value, harmless. - f1-live-state-truth: task_done_detected_at twice with DIFFERENT values; the second silently won, so a merge decision was being made by JS object ordering. Kept the F1b round-3 value and the comment explaining why that worker EARNED its done, which is what the fixture is for. Suite 138 files / 3194 passed / 1 skipped; typecheck exit 0; both TS1117s gone under direct tsc. Co-Authored-By: cmuxlayerCodex-5054eba0 running gpt-5.6-sol Co-Authored-By: cmuxlayerClaude running claude-opus-5 --- src/agent-engine.ts | 37 ------------------------------- tests/coordination-paths.test.ts | 1 - tests/f1-live-state-truth.test.ts | 4 ---- 3 files changed, 42 deletions(-) diff --git a/src/agent-engine.ts b/src/agent-engine.ts index f38cede9..a3145b14 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -1842,43 +1842,6 @@ export class AgentEngine { return live.state; } - /** - * Positive evidence the task ENDED: a done signal seen on the screen or in - * the harness transcript. Deliberately NOT "the record says done" -- that is - * the thing #408 fabricates. - */ - private hasPositiveDoneEvidence(agent: AgentRecord): boolean { - if (agent.task_done_detected_at) return true; - return this.loadGroundTruthSession(agent)?.state.done === true; - } - - /** - * The state CLOSURE reasons about, and the one rule a response may use. - * - * AIDEV-NOTE (F1b round 3): a list_agents row published its `state` from - * `agent-health`'s reconciled state (the raw screen verdict) while its - * `closure` came from `isLiveActive(live) ? live.state : agent.state` -- so - * a fresh agent at a `ready` prompt whose record had flipped to `done` - * rendered `state:"ready"` beside `closure:"artifact_missing"`. One row, two - * state rules, and the alarming one won. Five such specimens were observed - * live, one spawned two minutes earlier. - * - * This is that same rule, in one place, with two carve-outs that are about - * EVIDENCE rather than about which field is being rendered: - * 1. Activity always wins (F1): a screen showing work in progress overturns - * any record. - * 2. A `done` the agent EARNED survives a ready prompt -- a finished worker - * sits at one too, and its deadlock signal has to keep working. What - * does not survive is a `done` with nothing behind it. - */ - private closureStateOf(agent: AgentRecord, live: LiveAgentState): AgentState { - if (isLiveActive(live)) return live.state; - if (agent.state === "done" && this.hasPositiveDoneEvidence(agent)) { - return "done"; - } - return live.screen_state ?? agent.state; - } - /** Live state for one record, or the record's own state when unprobed. */ liveStateOf(agent: AgentRecord): LiveAgentState { const memo = this.freshLiveStates.get(agent.agent_id); diff --git a/tests/coordination-paths.test.ts b/tests/coordination-paths.test.ts index 98562354..cf6326a6 100644 --- a/tests/coordination-paths.test.ts +++ b/tests/coordination-paths.test.ts @@ -213,7 +213,6 @@ describe("P11 closure state (Constraint 3: no bare boolean at default detail)", expect( resolveClosureState({ contractIssued: false, - doneEvidence: true, state: "done", closureArtifactVerified: null, doneEvidence: true, diff --git a/tests/f1-live-state-truth.test.ts b/tests/f1-live-state-truth.test.ts index 9ea25a8f..7ffcd9c3 100644 --- a/tests/f1-live-state-truth.test.ts +++ b/tests/f1-live-state-truth.test.ts @@ -460,10 +460,6 @@ describe("F1 — live state, not the stale registry record", () => { agent_id: "cmuxlayerCodex-finished", surface_id: client.idleSurface, state: "done", - // T1b (#488): the record alone is no longer enough to claim a deadlock - // -- #408 writes `done` on live agents without anything observing one. - // This agent's done WAS observed, so the signal must survive. - task_done_detected_at: "2026-08-18T13:41:00.000Z", report_path: join(TEST_DIR, "reports", "missing.md"), done_marker: "### @cmuxlayerCodex-finished DONE", // F1b round 3: this worker EARNED its done -- a done signal was