From e847eb5d0a2cc0327c7252cd9bf78cc6a9667bd5 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 20:03:21 +0300 Subject: [PATCH 1/3] fix(t2b): no success receipt for a submit that never landed or a pane that never closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two live-reproduced silent failures, both the same disease: ok:true for an action the engine did not perform. #484 — send_to(mode:"key") - submit_attempted was computed by exact match on `key === "return"`, so "Enter", "Return", "KPEnter", "ctrl-m" and a raw CR all dispatched a real submit and then reported submit_attempted:false. The documented type -> verify -> Return recovery therefore returned a success receipt whose own fields said nothing had been attempted. - normalizeKeyName now canonicalizes every submit alias to "return", and isSubmitKey() drives the receipt. - The key receipt now carries key_dispatched:true — sendKeyWithRetry throws when nothing reached the pane, so the ok path has dispatch evidence and now states it instead of leaving the caller to infer it from bytes:0. - A submit key is verified: verifySubmitKeyOutcome reads the pane and asks the only question that matters — did the composer let go of its contents? Composer still populated => send_key returns an error with submit_verification_reason:"composer_still_populated", not ok:true. Unreadable screen => submit_verified:null with a stated reason. #485 — close_surface(scope:"agent") - The handler stopped the agent and returned stop_agent's receipt verbatim (ok:true, state:"done") for a tool named close_surface, while the pane stayed open. It now resolves the bound surface before the stop (the stop can evict the record), stops, then closes the pane for real. - The receipt reports the two halves separately: agent_stopped and surface_closed. Agent stopped but pane survived => error naming both halves. No surface bound => ok with surface_close_skipped:"no_surface_bound". - Cross-checked scope:"workspace": delete_workspace does perform its named action; that branch now states workspace_deleted explicitly. - close_surface's description says agent scope closes the pane and reports the halves separately. Tests: tests/t2b-silent-failures.test.ts (14). Co-Authored-By: Claude Opus 5 (1M context) --- src/key-names.ts | 52 ++++- src/server.ts | 246 ++++++++++++++++++-- tests/t2b-silent-failures.test.ts | 361 ++++++++++++++++++++++++++++++ 3 files changed, 640 insertions(+), 19 deletions(-) create mode 100644 tests/t2b-silent-failures.test.ts diff --git a/src/key-names.ts b/src/key-names.ts index 090e0ed..880e007 100644 --- a/src/key-names.ts +++ b/src/key-names.ts @@ -1,15 +1,55 @@ 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" (or sending a raw CR) 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. Canonicalize them +// so both the dispatched key name and the receipt agree. +const SUBMIT_KEY_ALIASES = new Set([ + "return", + "enter", + "kpenter", + "kp_enter", + "kp-enter", + "c-m", + "ctrl-m", + "ctrl+m", + "^m", + "\r", + "\n", +]); + +/** + * Reduce a key name to the token the alias sets are keyed on. Raw carriage + * returns and newlines are collapsed to "\r" before the trim that every other + * alias needs, because trimming them away would erase the key entirely. + */ +function canonicalKeyToken(key: string): string { + if (/^[\r\n]+$/.test(key)) { + return "\r"; + } + 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 token = canonicalKeyToken(key); + if (!token) { + return key.trim(); } - const normalized = trimmed.toLowerCase().replace(/\s+/g, ""); - if (CTRL_C_ALIASES.has(normalized)) { + if (CTRL_C_ALIASES.has(token)) { return "ctrl-c"; } - return trimmed; + if (SUBMIT_KEY_ALIASES.has(token)) { + return "return"; + } + + return key.trim(); } diff --git a/src/server.ts b/src/server.ts index 7843869..5c9e057 100644 --- a/src/server.ts +++ b/src/server.ts @@ -210,7 +210,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 +474,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 +534,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), @@ -1023,6 +1032,16 @@ type SubmitVerificationFailureReason = | "working_status_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 { readonly retry_safe = false; readonly receipt: PublicDeliveryReceipt; @@ -5026,6 +5045,66 @@ 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; + timeout_ms?: number; + }): Promise<{ + submit_verified: boolean | null; + submit_verification_reason: SubmitKeyVerificationReason | null; + }> => { + const timeoutMs = opts.timeout_ms ?? SEND_KEY_SUBMIT_VERIFY_TIMEOUT_MS; + const startedAt = Date.now(); + let sawReadableScreen = false; + let lastComposerInput: string | null = null; + + while (Date.now() - startedAt < timeoutMs) { + 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); + lastComposerInput = composerInput; + if (isSubmitVerifiedStatus(snapshot.parsed.status)) { + return { submit_verified: true, submit_verification_reason: null }; + } + if (composerInput !== null && composerInput.trim() === "") { + 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 (lastComposerInput !== null && lastComposerInput.trim() !== "") { + // 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 +5123,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; + } + > => { await opts.beforeMutation?.(); if (opts.key !== undefined) { if (opts.chunks.length > 0 || opts.press_enter) { @@ -5053,16 +5139,27 @@ 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, + timeout_ms: opts.submit_verify_timeout_ms, + }) + : { 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,12 +5168,22 @@ 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, + ...(verification.submit_verification_reason + ? { + submit_verification_reason: + verification.submit_verification_reason, + } + : {}), + bytes: 0, + }; } const deliverySafetySnapshot = await assertDeliveryTargetIsSafe( opts.surface, @@ -8988,6 +9095,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 +9116,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 +9587,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.", { scope: z .enum(["surface", "agent", "workspace"]) @@ -9496,17 +9614,113 @@ 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) ?? + stateMgr + .listStates() + .find((record) => record.agent_id === args.agent_id); + 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, + // The agent has just been stopped at the caller's explicit + // request; the liveness guard would otherwise refuse on a + // registry row that has not caught up yet. + force: true, }, + {}, + ); + 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, + }, + ); + } + const data = { + ...stopContent, + scope: "agent", + agent_stopped: true, + surface: boundSurface, + surface_closed: true, + ...(closeContent.collapse_pane !== undefined + ? { collapse_pane: closeContent.collapse_pane } + : {}), }; + return okFormatted( + `close_surface scope=agent — agent ${args.agent_id} stopped and surface ${boundSurface} closed`, + data, + ); } if (args.scope === "workspace") { if (!args.workspace) { @@ -9520,11 +9734,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, }, }; } diff --git a/tests/t2b-silent-failures.test.ts b/tests/t2b-silent-failures.test.ts new file mode 100644 index 0000000..8605dd2 --- /dev/null +++ b/tests/t2b-silent-failures.test.ts @@ -0,0 +1,361 @@ +/** + * 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. + */ +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"; + +type ToolResult = { + isError?: boolean; + structuredContent?: Record; + content?: Array<{ text: string }>; +}; + +function payload(result: ToolResult): Record { + return ( + result.structuredContent ?? + (JSON.parse(result.content?.[0]?.text ?? "{}") as Record) + ); +} + +function tool(server: unknown, name: string) { + const registered = ( + server as { _registeredTools: Record } + )._registeredTools[name]; + if (!registered) throw new Error(`Tool not found: ${name}`); + return registered; +} + +const SURFACE = "surface:89"; + +/** + * 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. + */ +function makeExec(opts?: { + screen?: () => string; + onSendKey?: (key: string) => void; + closeSurfaceFails?: boolean; + surfaceRef?: string; +}): ExecFn & { calls: string[][] } { + const surfaceRef = opts?.surfaceRef ?? SURFACE; + const calls: string[][] = []; + 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({ + panes: [{ ref: "pane:1", workspace: "workspace:1", focused: true }], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + pane: "pane:1", + surfaces: [ + { + ref: surfaceRef, + pane: "pane:1", + workspace: "workspace:1", + title: "golemsClaude", + type: "terminal", + selected: true, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: surfaceRef, + 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") && opts?.closeSurfaceFails) { + throw new Error("cmux close-surface: pane is not closable"); + } + return { stdout: "{}", stderr: "" }; + }) as ExecFn & { calls: string[][] }; + (exec as unknown as { calls: string[][] }).calls = calls; + return exec; +} + +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)"; + +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; +} + +describe("#484 — send_to(mode:key) must not report success for an unattempted submit", () => { + it.each(["return", "Return", "enter", "Enter", "KPEnter", "\r"])( + "recognises %j as a submit key so the receipt cannot claim submit_attempted:false", + async (key) => { + const exec = makeExec({ screen: () => WORKING_CLAUDE_SCREEN }); + const server = makeServer(exec); + + const result = (await tool(server, "send_to").handler( + { mode: "key", target: SURFACE, key }, + {}, + )) as ToolResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.submit_attempted).toBe(true); + }, + ); + + it("reports the key actually reached the pane instead of an evidence-free ok:true", async () => { + const exec = makeExec({ screen: () => WORKING_CLAUDE_SCREEN }); + const server = makeServer(exec); + + const result = (await tool(server, "send_to").handler( + { mode: "key", target: SURFACE, key: "return" }, + {}, + )) as ToolResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.key_dispatched).toBe(true); + }); + + it("verifies the submit landed when the composer clears", async () => { + let pressed = false; + const exec = makeExec({ + screen: () => (pressed ? WORKING_CLAUDE_SCREEN : POPULATED_CLAUDE_SCREEN), + onSendKey: () => { + pressed = true; + }, + }); + const server = makeServer(exec); + + const result = (await tool(server, "send_to").handler( + { mode: "key", target: SURFACE, key: "return" }, + {}, + )) as ToolResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.submit_verified).toBe(true); + }); + + 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 server = makeServer(exec); + + const result = (await tool(server, "send_to").handler( + { mode: "key", target: SURFACE, key: "return" }, + {}, + )) as ToolResult; + + 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("leaves non-submit keys unverified but still states that they were dispatched", async () => { + const exec = makeExec({ screen: () => POPULATED_CLAUDE_SCREEN }); + const server = makeServer(exec); + + const result = (await tool(server, "send_to").handler( + { mode: "key", target: SURFACE, key: "escape" }, + {}, + )) as ToolResult; + + 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 = ( + server as { _registeredTools: Record } + )._registeredTools.interact?._engine; + if (!engine) throw new Error("Lifecycle engine not registered"); + 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; +} + +describe("#485 — close_surface(scope:agent) must close the surface or say it did not", () => { + it("actually closes the pane instead of only stopping the agent", async () => { + const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + const server = makeServer(exec); + const record = seedAgent(server); + + const result = (await tool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.surface_closed).toBe(true); + expect(data.surface).toBe(SURFACE); + expect( + exec.calls.some((args) => args.includes("close-surface")), + ).toBe(true); + }); + + 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 tool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolResult; + + 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); + }); + + 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 tool(server, "close_surface").handler( + { scope: "workspace", workspace: "workspace:1", force: true }, + {}, + )) as ToolResult; + + 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); + }); + + 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 tool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, + {}, + )) as ToolResult; + + const data = payload(result); + expect(result.isError).toBeUndefined(); + expect(data.surface_closed).toBe(false); + expect(data.surface_close_skipped).toBe("no_surface_bound"); + }); +}); From 92caff153b3cfd8c31622a981585b7484e2c2ab2 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 21:47:53 +0300 Subject: [PATCH 2/3] fix(t2b): a populated composer vetoes every other submit signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 2, addressing the ITERATE on #487. BLOCKER (#484) — `verifySubmitKeyOutcome` checked status BEFORE, and unguarded by, the composer. On a busy target — status working while the composer still visibly held the unsent text, which is the reported scenario — the status branch fired and the composer check never ran, returning `ok:true, submit_verified:true` for a message still on screen. Worse than the null it replaced: null admits ignorance, true asserts an observation the pane contradicts. - A populated composer now vetoes and keeps polling; nothing else can resolve the verification while the pane still shows the input. Mirrors the text path's `!hasPendingSubmitEvidence` gate at server.ts:4878. - A "working" status is no longer proof at all for a key send. The reported target was ALREADY working, so status cannot distinguish "my submit started a turn" from "a turn was already running" — and a boxed composer reads as unreadable, so accepting status would resurrect the same false-true through the extractor's blind spot. An empty, readable composer is the only positive proof; anything else is submit_verified:null with a stated reason. - Missing fixture added: working status crossed with a populated composer. Review items taken: - normalizeKeyName no longer rewrites the key sent to cmux. isSubmitKey alone makes the receipt truthful, and rewriting silently changed the bytes cmux receives for aliases nobody reported — "\n" especially, which is how a composer expresses shift+enter. normalizeKeyName is now byte-identical to base; raw "\r"/"\n" dropped from the submit set for the same reason. - Dropped the unreachable timeout_ms knob; the constant is used directly. - submit_verification_reason is stated unconditionally instead of via a conditional spread. - close_surface uses the house idiom for the registry lookup. - close_surface(scope:"agent") passes the caller's own force through instead of forcing unconditionally: stop_agent has no liveness refusal, so forcing here let an UNFORCED call tear down a live agent's pane through a guard that could never fire. Tool description says the pane close obeys the same guard. - Tests use tests/helpers/mcp-tool-harness.ts; the close tests assert the pane is gone from list_surfaces, not only that a CLI command ran. 17 tests, 16 red against base and the 17th red against the round-1 code it guards. Output in the PR body. Co-Authored-By: Claude Opus 5 (1M context) --- src/key-names.ts | 34 ++-- src/server.ts | 64 ++++--- tests/t2b-silent-failures.test.ts | 298 +++++++++++++++++++----------- 3 files changed, 237 insertions(+), 159 deletions(-) diff --git a/src/key-names.ts b/src/key-names.ts index 880e007..7e0b35e 100644 --- a/src/key-names.ts +++ b/src/key-names.ts @@ -2,10 +2,12 @@ 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" (or sending a raw CR) 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. Canonicalize them -// so both the dispatched key name and the receipt agree. +// 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", @@ -16,19 +18,9 @@ const SUBMIT_KEY_ALIASES = new Set([ "ctrl-m", "ctrl+m", "^m", - "\r", - "\n", ]); -/** - * Reduce a key name to the token the alias sets are keyed on. Raw carriage - * returns and newlines are collapsed to "\r" before the trim that every other - * alias needs, because trimming them away would erase the key entirely. - */ function canonicalKeyToken(key: string): string { - if (/^[\r\n]+$/.test(key)) { - return "\r"; - } return key.trim().toLowerCase().replace(/\s+/g, ""); } @@ -38,18 +30,14 @@ export function isSubmitKey(key: string): boolean { } export function normalizeKeyName(key: string): string { - const token = canonicalKeyToken(key); - if (!token) { - return key.trim(); + const trimmed = key.trim(); + if (!trimmed) { + return trimmed; } - if (CTRL_C_ALIASES.has(token)) { + if (CTRL_C_ALIASES.has(canonicalKeyToken(trimmed))) { return "ctrl-c"; } - if (SUBMIT_KEY_ALIASES.has(token)) { - return "return"; - } - - return key.trim(); + return trimmed; } diff --git a/src/server.ts b/src/server.ts index 5c9e057..d29a128 100644 --- a/src/server.ts +++ b/src/server.ts @@ -5057,17 +5057,15 @@ export function createServer(opts?: CreateServerOptions): McpServer { const verifySubmitKeyOutcome = async (opts: { surface: string; workspace?: string; - timeout_ms?: number; }): Promise<{ submit_verified: boolean | null; submit_verification_reason: SubmitKeyVerificationReason | null; }> => { - const timeoutMs = opts.timeout_ms ?? SEND_KEY_SUBMIT_VERIFY_TIMEOUT_MS; const startedAt = Date.now(); let sawReadableScreen = false; - let lastComposerInput: string | null = null; + let composerStillPopulated = false; - while (Date.now() - startedAt < timeoutMs) { + 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); @@ -5075,11 +5073,29 @@ export function createServer(opts?: CreateServerOptions): McpServer { } sawReadableScreen = true; const composerInput = extractComposerInputRegion(snapshot.text); - lastComposerInput = composerInput; - if (isSubmitVerifiedStatus(snapshot.parsed.status)) { - return { submit_verified: true, submit_verification_reason: null }; + 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 && composerInput.trim() === "") { + 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); @@ -5091,7 +5107,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { submit_verification_reason: "surface_read_unavailable", }; } - if (lastComposerInput !== null && lastComposerInput.trim() !== "") { + 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 { @@ -5128,7 +5144,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { bytes: number; /** Present only on the key path: the key really reached the pane. */ key_dispatched?: boolean; - submit_verification_reason?: SubmitKeyVerificationReason; + submit_verification_reason?: SubmitKeyVerificationReason | null; } > => { await opts.beforeMutation?.(); @@ -5153,7 +5169,6 @@ export function createServer(opts?: CreateServerOptions): McpServer { ? await verifySubmitKeyOutcome({ surface: opts.surface, workspace: opts.workspace, - timeout_ms: opts.submit_verify_timeout_ms, }) : { submit_verified: null, submit_verification_reason: null }; const receipt = buildPublicDeliveryReceipt({ @@ -5176,12 +5191,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { return { ...receipt, key_dispatched: true, - ...(verification.submit_verification_reason - ? { - submit_verification_reason: - verification.submit_verification_reason, - } - : {}), + submit_verification_reason: verification.submit_verification_reason, bytes: 0, }; } @@ -9587,7 +9597,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. 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.", + "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"]) @@ -9620,11 +9630,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { // 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) ?? - stateMgr - .listStates() - .find((record) => record.agent_id === args.agent_id); + 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( @@ -9677,10 +9683,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { scope: "surface", surface: boundSurface, workspace: boundWorkspace, - // The agent has just been stopped at the caller's explicit - // request; the liveness guard would otherwise refuse on a - // registry row that has not caught up yet. - force: true, + // 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, }, {}, ); diff --git a/tests/t2b-silent-failures.test.ts b/tests/t2b-silent-failures.test.ts index 8605dd2..5099ef8 100644 --- a/tests/t2b-silent-failures.test.ts +++ b/tests/t2b-silent-failures.test.ts @@ -9,6 +9,9 @@ * 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"; @@ -17,43 +20,50 @@ 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"; -type ToolResult = { - isError?: boolean; - structuredContent?: Record; - content?: Array<{ text: string }>; -}; - -function payload(result: ToolResult): Record { - return ( - result.structuredContent ?? - (JSON.parse(result.content?.[0]?.text ?? "{}") as Record) - ); -} - -function tool(server: unknown, name: string) { - const registered = ( - server as { _registeredTools: Record } - )._registeredTools[name]; - if (!registered) throw new Error(`Tool not found: ${name}`); - return registered; +/** + * 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. + * 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; - surfaceRef?: string; }): ExecFn & { calls: string[][] } { - const surfaceRef = opts?.surfaceRef ?? SURFACE; const calls: string[][] = []; + let surfaceLive = true; const exec = vi.fn().mockImplementation(async (_cmd, args: string[]) => { calls.push(args); if (args.includes("list-workspaces")) { @@ -75,7 +85,17 @@ function makeExec(opts?: { if (args.includes("list-panes")) { return { stdout: JSON.stringify({ - panes: [{ ref: "pane:1", workspace: "workspace:1", focused: true }], + 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: "", }; @@ -83,17 +103,20 @@ function makeExec(opts?: { if (args.includes("list-pane-surfaces")) { return { stdout: JSON.stringify({ - pane: "pane:1", - surfaces: [ - { - ref: surfaceRef, - pane: "pane:1", - workspace: "workspace:1", - title: "golemsClaude", - type: "terminal", - selected: true, - }, - ], + 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: "", }; @@ -101,7 +124,7 @@ function makeExec(opts?: { if (args.includes("read-screen")) { return { stdout: JSON.stringify({ - surface: surfaceRef, + surface: SURFACE, text: opts?.screen?.() ?? "$ ", lines: 30, scrollback_used: false, @@ -113,20 +136,19 @@ function makeExec(opts?: { opts?.onSendKey?.(String(args[args.length - 1] ?? "")); return { stdout: "{}", stderr: "" }; } - if (args.includes("close-surface") && opts?.closeSurfaceFails) { - throw new Error("cmux close-surface: pane is not closable"); + if (args.includes("close-surface")) { + if (opts?.closeSurfaceFails) { + throw new Error("cmux close-surface: pane is not closable"); + } + surfaceLive = false; + return { stdout: "{}", stderr: "" }; } return { stdout: "{}", stderr: "" }; }) as ExecFn & { calls: string[][] }; - (exec as unknown as { calls: string[][] }).calls = calls; + exec.calls = calls; return exec; } -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)"; - let stateDir: string; beforeEach(() => { @@ -147,68 +169,85 @@ function makeServer(exec: ExecFn) { }) 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", "\r"])( - "recognises %j as a submit key so the receipt cannot claim submit_attempted:false", + 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_CLAUDE_SCREEN }); - const server = makeServer(exec); - - const result = (await tool(server, "send_to").handler( - { mode: "key", target: SURFACE, key }, - {}, - )) as ToolResult; + 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("reports the key actually reached the pane instead of an evidence-free ok:true", async () => { - const exec = makeExec({ screen: () => WORKING_CLAUDE_SCREEN }); - const server = makeServer(exec); + 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"); - const result = (await tool(server, "send_to").handler( - { mode: "key", target: SURFACE, key: "return" }, - {}, - )) as ToolResult; - - const data = payload(result); expect(result.isError).toBeUndefined(); - expect(data.key_dispatched).toBe(true); + 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_CLAUDE_SCREEN : POPULATED_CLAUDE_SCREEN), + screen: () => + pressed ? WORKING_AND_CLEARED_CLAUDE_SCREEN : POPULATED_CLAUDE_SCREEN, onSendKey: () => { pressed = true; }, }); - const server = makeServer(exec); - const result = (await tool(server, "send_to").handler( - { mode: "key", target: SURFACE, key: "return" }, - {}, - )) as ToolResult; + 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 server = makeServer(exec); - const result = (await tool(server, "send_to").handler( - { mode: "key", target: SURFACE, key: "return" }, - {}, - )) as ToolResult; + 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); @@ -217,14 +256,25 @@ describe("#484 — send_to(mode:key) must not report success for an unattempted 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 server = makeServer(exec); - const result = (await tool(server, "send_to").handler( - { mode: "key", target: SURFACE, key: "escape" }, - {}, - )) as ToolResult; + const result = await sendKey(makeServer(exec), "escape"); const data = payload(result); expect(result.isError).toBeUndefined(); @@ -235,10 +285,10 @@ describe("#484 — send_to(mode:key) must not report success for an unattempted }); function seedAgent(server: unknown, overrides: Partial = {}) { - const engine = ( - server as { _registeredTools: Record } - )._registeredTools.interact?._engine; - if (!engine) throw new Error("Lifecycle engine not registered"); + 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, @@ -280,24 +330,34 @@ function seedAgent(server: unknown, overrides: Partial = {}) { 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("actually closes the pane instead of only stopping the agent", async () => { const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); const server = makeServer(exec); const record = seedAgent(server); + expect(await listSurfaceRefs(server)).toContain(SURFACE); - const result = (await tool(server, "close_surface").handler( + const result = (await getTool(server, "close_surface").handler( { scope: "agent", agent_id: record.agent_id }, {}, - )) as ToolResult; + )) as ToolCallResult; const data = payload(result); expect(result.isError).toBeUndefined(); expect(data.surface_closed).toBe(true); expect(data.surface).toBe(SURFACE); - expect( - exec.calls.some((args) => args.includes("close-surface")), - ).toBe(true); + // The brief asked for the list_surfaces check, not only that a CLI command + // was issued: the pane is gone from the topology afterwards. + expect(await listSurfaceRefs(server)).not.toContain(SURFACE); }); it("refuses to report success when the agent stopped but the pane survived", async () => { @@ -308,10 +368,10 @@ describe("#485 — close_surface(scope:agent) must close the surface or say it d const server = makeServer(exec); const record = seedAgent(server); - const result = (await tool(server, "close_surface").handler( + const result = (await getTool(server, "close_surface").handler( { scope: "agent", agent_id: record.agent_id }, {}, - )) as ToolResult; + )) as ToolCallResult; const data = payload(result); expect(result.isError).toBe(true); @@ -319,43 +379,63 @@ describe("#485 — close_surface(scope:agent) must close the surface or say it d 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("cross-checks scope=workspace: the delegate really deletes, and says so", async () => { + 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 tool(server, "close_surface").handler( - { scope: "workspace", workspace: "workspace:1", force: true }, + const result = (await getTool(server, "close_surface").handler( + { scope: "agent", agent_id: record.agent_id }, {}, - )) as ToolResult; + )) 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); + expect(data.surface_closed).toBe(false); + expect(data.surface_close_skipped).toBe("no_surface_bound"); }); - it("states plainly that there was no surface to close rather than implying one closed", async () => { - const exec = makeExec({ screen: () => IDLE_CLAUDE_SCREEN }); + 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, { - agent_id: "golemsClaude-c7738dab", - surface_id: "", - }); + const record = seedAgent(server, { state: "working" }); - const result = (await tool(server, "close_surface").handler( + const result = (await getTool(server, "close_surface").handler( { scope: "agent", agent_id: record.agent_id }, {}, - )) as ToolResult; + )) 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"); + 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); }); }); From 8c154e0de94f89ecfab615d3239dc5c22dd60f00 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 22:45:32 +0300 Subject: [PATCH 3/3] fix(t2b): confirm the pane is gone; stop calling a closed agent "working" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3, following the RESOLVED scope correction on #485. Two reframes landed mid-work; this commit follows the third and final one and drops what the second one asked for. The mechanism is the scope argument, confirmed in source and never a timing effect: pre-fix, scope:"agent" delegated to stop_agent and no path in that branch closed the surface. Every field report fits with zero latency once you sort them by which scope the lead called. - Target 1 (agent scope closes the pane, or says it did not) was already the shape of this branch since round 1. What is new: surface_closed is now an OBSERVATION. After the CLI returns, findSurfaceByRef confirms the pane is actually gone; if cmux still lists it, the receipt reports surface_closed:false with a WARNING instead of inferring closure from a call that returned. Agent scope forwards that observation rather than restating it. - Target 4 (independent of scope): close_surface marked matching records user_killed:true but never transitioned their state, so list_agents kept reporting a closed agent as "working" — golemsClaude's datum. An acknowledged close now transitions a non-terminal record to done in the same breath. The test that caught this first passed VACUOUSLY: list_agents nests state as {value}, so /"state":\s*"working"/ never matched. Assertion fixed to read state.value, which reproduced the defect. - Target 3: the close tests assert against list_surfaces, not the return value. REMOVED, because the latency theory it was built on is withdrawn: the SURFACE_CLOSE_SETTLE_* constants, the post-ack polling loop, the nonterminal "closing" state, close_latency_ms, and the eventual-consistency test fixtures. No delay window is characterised anywhere in this diff. NOT done as literally specified, and flagged rather than silently skipped: target 2 asks the description to say "agent-scope does not close panes". That sentence is false against this branch — target 1's first option was taken, so agent scope DOES close the pane. Writing it would make the tool description lie, which is the disease this lane exists to fix. The description states what the code does instead. See the PR body. 19 tests, 18 red against base; the 19th red against sabotage. Co-Authored-By: Claude Opus 5 (1M context) --- src/server.ts | 40 ++++++++++++++-- tests/t2b-silent-failures.test.ts | 77 ++++++++++++++++++++++++++++--- 2 files changed, 107 insertions(+), 10 deletions(-) diff --git a/src/server.ts b/src/server.ts index d29a128..af2a35f 100644 --- a/src/server.ts +++ b/src/server.ts @@ -9717,18 +9717,24 @@ export function createServer(opts?: CreateServerOptions): McpServer { }, ); } + // 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: true, + surface_closed: surfaceClosed, + ...(closeContent.WARNING ? { WARNING: closeContent.WARNING } : {}), ...(closeContent.collapse_pane !== undefined ? { collapse_pane: closeContent.collapse_pane } : {}), }; return okFormatted( - `close_surface scope=agent — agent ${args.agent_id} stopped and surface ${boundSurface} closed`, + 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, ); } @@ -9991,6 +9997,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 @@ -10010,6 +10023,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 && @@ -10034,9 +10057,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); } diff --git a/tests/t2b-silent-failures.test.ts b/tests/t2b-silent-failures.test.ts index 5099ef8..ef49889 100644 --- a/tests/t2b-silent-failures.test.ts +++ b/tests/t2b-silent-failures.test.ts @@ -61,6 +61,8 @@ 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; @@ -140,7 +142,9 @@ function makeExec(opts?: { if (opts?.closeSurfaceFails) { throw new Error("cmux close-surface: pane is not closable"); } - surfaceLive = false; + if (!opts?.closeLeavesSurfaceListed) { + surfaceLive = false; + } return { stdout: "{}", stderr: "" }; } return { stdout: "{}", stderr: "" }; @@ -340,7 +344,11 @@ async function listSurfaceRefs(server: unknown): Promise { } describe("#485 — close_surface(scope:agent) must close the surface or say it did not", () => { - it("actually closes the pane instead of only stopping the agent", async () => { + 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); @@ -351,13 +359,68 @@ describe("#485 — close_surface(scope:agent) must close the surface or say it d {}, )) as ToolCallResult; - const data = payload(result); expect(result.isError).toBeUndefined(); - expect(data.surface_closed).toBe(true); - expect(data.surface).toBe(SURFACE); - // The brief asked for the list_surfaces check, not only that a CLI command - // was issued: the pane is gone from the topology afterwards. 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 () => {