diff --git a/src/agent-engine.ts b/src/agent-engine.ts index 1c74f4f..a3145b1 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, @@ -720,6 +737,21 @@ 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; +/** + * 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; @@ -1476,6 +1508,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: ( @@ -1744,8 +1782,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) ); @@ -2401,11 +2510,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) || @@ -2418,6 +2534,7 @@ export class AgentEngine { agent: AgentRecord, targetState: AgentState, waitForReadyPatternMatches: Map, + effectiveState: AgentState = agent.state, ): Promise<{ agent: AgentRecord; source?: RefreshedTargetStateEvidenceSource; @@ -2427,6 +2544,7 @@ export class AgentEngine { agent, targetState, waitForReadyPatternMatches, + effectiveState, ); } if (!this.requiresOutputDoneEvidence(targetState)) return { agent }; @@ -2434,19 +2552,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 }; } @@ -7527,29 +7666,125 @@ 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, + if (!agent) { + return { + exists: false, + state: null, + source, + detail: `no registry or state record for ${agentId}`, + }; + } + + 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. + // + // 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, + 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}`, + }; + } + 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") { + const live = resolveLiveAgentState(agent, { + status: parsed.status, + agent_type: parsed.agent_type, + control_state: parsed.control_state, }); - const parsed = parseScreen(cleanScreenText(screen.text)); + // 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: - parsed.agent_type !== "unknown" && - parsed.control_state !== "dead" && - parsed.control_state !== "stale_surface", - state: parsed.status === "frozen" ? "error" : parsed.status, + exists: true, + state: live.state, source, + detail: "registry hit, screen unparseable", }; - } catch { - return { exists: false, state: null, source }; } + return { + exists: true, + state: parsed.status === "frozen" ? "error" : parsed.status, + source, + }; }; private async sweepWatchesBestEffort(): Promise { @@ -8303,15 +8538,38 @@ 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. + // + // 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, + initialState, ); if (initialEvidence) { return { matched: true, - state: initial.state, + state: initialState, elapsed: Date.now() - start, source: initialEvidence === "state" ? "immediate" : initialEvidence, agent: toPublicAgent(initial), @@ -8319,10 +8577,10 @@ export class AgentEngine { } // Already in terminal error state and target isn't error? - if (initial.state === "error" && targetState !== "error") { + if (initialState === "error" && targetState !== "error") { return { matched: false, - state: initial.state, + state: initialState, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8331,10 +8589,10 @@ export class AgentEngine { } // Already in terminal done state and target isn't done? - if (initial.state === "done" && targetState !== "done") { + if (initialState === "done" && targetState !== "done") { return { matched: false, - state: initial.state, + state: initialState, elapsed: Date.now() - start, source: "immediate", agent: toPublicAgent(initial), @@ -8343,6 +8601,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) => { @@ -8356,9 +8617,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?.state ?? "error", + state: + current && timeoutLive + ? this.terminationStateOf(current, timeoutLive) + : "error", elapsed, source: "timeout", agent: current ? toPublicAgent(current) : null, @@ -8385,22 +8657,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. 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, + liveState, ); if (evidenceSource) { clearInterval(checkInterval); finish({ matched: true, - state: current.state, + state: liveState, elapsed, source: refreshed.source ?? @@ -8411,19 +8704,16 @@ export class AgentEngine { } // Fail-fast on terminal error - if ( - TERMINAL_STATES.has(current.state) && - current.state !== targetState - ) { + if (TERMINAL_STATES.has(liveState) && liveState !== targetState) { clearInterval(checkInterval); finish({ matched: false, - state: current.state, + state: liveState, elapsed, source: "sweep", agent: toPublicAgent(current), error: - current.error ?? `Agent entered terminal state: ${current.state}`, + current.error ?? `Agent entered terminal state: ${liveState}`, }); } }, WAIT_FOR_SWEEP_INTERVAL_MS); diff --git a/src/server.ts b/src/server.ts index 89a60f6..1d7bade 100644 --- a/src/server.ts +++ b/src/server.ts @@ -10801,34 +10801,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, @@ -10836,8 +10845,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; @@ -11170,6 +11217,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/src/watch-spec.ts b/src/watch-spec.ts index 506bfe8..3ca4085 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/coordination-paths.test.ts b/tests/coordination-paths.test.ts index e7e35ae..cf6326a 100644 --- a/tests/coordination-paths.test.ts +++ b/tests/coordination-paths.test.ts @@ -185,13 +185,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, - doneEvidence: true, 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 9236d03..7ffcd9c 100644 --- a/tests/f1-live-state-truth.test.ts +++ b/tests/f1-live-state-truth.test.ts @@ -460,12 +460,13 @@ 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 + // 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), ); @@ -479,4 +480,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 new file mode 100644 index 0000000..c2657c0 --- /dev/null +++ b/tests/f1b-wait-for-watch-live-state.test.ts @@ -0,0 +1,812 @@ +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 type { LiveAgentState } 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"; + +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(), + // 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(), + 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); + }); +}); + + +/** + * 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. 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); + 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"); + }); +}); + + + +/** 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"); + }); +}); diff --git a/tests/painpoint-e2e.test.ts b/tests/painpoint-e2e.test.ts index 1ef3684..e32e636 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 6eb58f1..80ee1fe 100644 --- a/tests/server-agent-tools.test.ts +++ b/tests/server-agent-tools.test.ts @@ -13000,7 +13000,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 = @@ -13008,7 +13008,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);