From 49a0b2b6e82c12c6009753afa317071640a2dc57 Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 19:43:20 +0300 Subject: [PATCH 1/2] fix(t2): stop delivery receipts asserting outcomes nobody observed truth-v3 lane T2 (delivery truth), Round 1. Six issues, each with a failing test written first. #442 send_to no longer types into a composer holding text this delivery did not write. A human's half-written draft plus our payload plus Return submits their words; the picker/permission gates never covered it because the screen is an ordinary ready composer that simply is not empty. Cursor is exempt and says why: its composer retains accepted text after submit, so non-empty there is the normal post-send screen, not an unsent draft. #467 a retryable requeue now carries a bounded lifetime. A target stuck booting used to retry behind a 30s-capped backoff forever, leaving a lead a receipt it could wait on indefinitely; it now resolves failed_confirmed citing the gate reason that kept refusing. #471 #443 verify_deadline_elapsed and target_gone stop escalating to GitHub issues. Both are outcomes the engine caused -- it stopped looking, or there was nothing left to look at -- and neither is evidence the message was lost. The local evidence ticket is still written, so the verdict keeps citing evidence. #445 every nonterminal and terminal-failed receipt now carries a plain-language WARNING at the top level. ok:true with delivered:false was routinely read as success; the booleans were correct and still misread. #450 the delivery snapshot read moved inside the verify hang guard, and the CLI-fallback exec got a timeout. A wedged cmux subprocess could hold deliveryVerifyInFlight forever -- the exact stall SF7 exists to prevent. #427 boot-prompt verification refuses submit_verified while the screen reports 0 tokens. A slow boot can render a working-looking status while the prompt sits unsent; a null token count stays inconclusive on purpose. Deferred with reasons written into the issues: #435 (boot mid-turn), #420 (stale inbox replay). #432 was already closed by #441/#449; verified against the existing tests, no change in this diff. Co-Authored-By: Claude Opus 5 (1M context) --- src/agent-engine.ts | 111 ++++- src/cmux-client.ts | 18 + src/server.ts | 160 ++++++- tests/agent-engine.test.ts | 74 ++++ tests/delivery-truth-t2.test.ts | 463 +++++++++++++++++++++ tests/send-to-v2-background-verify.test.ts | 184 +++++++- 6 files changed, 997 insertions(+), 13 deletions(-) create mode 100644 tests/delivery-truth-t2.test.ts diff --git a/src/agent-engine.ts b/src/agent-engine.ts index 59cf8946..ac2bcd43 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -260,7 +260,17 @@ export interface AgentDeliveryReceipt { composer_accepted?: boolean; /** Hard deadline for background verify; ISO timestamp. */ verify_deadline_at?: string | null; + /** + * Hard deadline for a retryable requeue; ISO timestamp. Set on the first + * retryable refusal so a target that never becomes interactive resolves + * instead of leaving the caller an open queue forever (#467). + */ + queue_deadline_at?: string | null; ticket_filed?: boolean; + /** Whether the local evidence ticket was escalated to the issue tracker. */ + ticket_escalated?: boolean; + /** Why escalation was declined, when it was. */ + ticket_escalation_declined_reason?: string | null; /** Consecutive verifier observations that the target agent is missing. */ verify_miss_count?: number; /** Last time background verify actually read the target surface. */ @@ -268,6 +278,7 @@ export interface AgentDeliveryReceipt { } export const DEFAULT_DELIVERY_VERIFY_DEADLINE_MS = 10 * 60 * 1000; +export const DEFAULT_DELIVERY_QUEUE_DEADLINE_MS = 10 * 60 * 1000; export const DELIVERY_TARGET_GONE_CONFIRM_MISSES = 3; const DELIVERY_WAIT_POLL_MS = 100; @@ -664,6 +675,7 @@ export interface AgentEngineOptions { deliveryVerifyTimeoutMs?: number; /** How long a pending_verify delivery may stay nonterminal before failed_confirmed. */ deliveryVerifyDeadlineMs?: number; + deliveryQueueDeadlineMs?: number; /** * Local evidence-ticket directory. Omitted/null disables tickets so bare * construction never writes ~/.cmuxlayer/tickets or calls gh. Production @@ -1546,6 +1558,7 @@ export class AgentEngine { private deliverySubmitTimeoutMs: number; private deliveryVerifyTimeoutMs: number; private deliveryVerifyDeadlineMs: number; + private deliveryQueueDeadlineMs: number; private deliveryTicketDir: string | null; private deliveryIssueFiler: DeliveryIssueFiler | null = null; private autoReviveBackoffBaseMs: number; @@ -1578,6 +1591,10 @@ export class AgentEngine { 1, opts?.deliveryVerifyDeadlineMs ?? DEFAULT_DELIVERY_VERIFY_DEADLINE_MS, ); + this.deliveryQueueDeadlineMs = Math.max( + 1, + opts?.deliveryQueueDeadlineMs ?? DEFAULT_DELIVERY_QUEUE_DEADLINE_MS, + ); this.deliveryTicketDir = opts?.deliveryTicketDir ?? null; this.deliveryIssueFiler = opts?.deliveryIssueFiler ?? null; this.deliveryVerifier = opts?.deliveryVerifier ?? null; @@ -6885,9 +6902,20 @@ export class AgentEngine { let snapshot: DeliveryVerifySnapshot | null | undefined; if (this.deliverySnapshotReader) { if (!snapshots.has(snapshotKey)) { + // AIDEV-NOTE (T2 #450): the snapshot read must be inside the + // hang guard, not before it. SF8 hoisted the surface read out of + // the verifier and awaited it OUTSIDE SF7's race; the CLI + // fallback path has no subprocess timeout, so one wedged `cmux` + // held deliveryVerifyInFlight forever and every later verify + // pass short-circuited -- exactly the stall SF7 exists to + // prevent. A timed-out read yields a null snapshot, which the + // verifier already treats as "no evidence, stay pending". snapshots.set( snapshotKey, - await this.deliverySnapshotReader(receipt), + await this.withDeliveryVerifyTimeout( + this.deliverySnapshotReader(receipt), + "Delivery snapshot read", + ).catch(() => null), ); } snapshot = snapshots.get(snapshotKey) ?? null; @@ -6964,6 +6992,30 @@ export class AgentEngine { } } + /** Bound one delivery-verify side quest to the verify timeout. */ + private withDeliveryVerifyTimeout( + work: Promise, + label: string, + ): Promise { + let timeout: ReturnType | null = null; + return Promise.race([ + work, + new Promise((_resolve, reject) => { + timeout = setTimeout( + () => + reject( + new Error( + `${label} timed out after ${this.deliveryVerifyTimeoutMs}ms`, + ), + ), + this.deliveryVerifyTimeoutMs, + ); + }), + ]).finally(() => { + if (timeout) clearTimeout(timeout); + }); + } + private verifyReadIntervalMs( receipt: AgentDeliveryReceipt, now: number, @@ -6994,6 +7046,35 @@ export class AgentEngine { return since > 0 && since < this.verifyReadIntervalMs(receipt, now); } + /** + * A confirmed-failure verdict is worth an issue only when something was + * actually observed to go wrong with the message. + * + * AIDEV-NOTE (T2 #471/#443): `verify_deadline_elapsed` means the ENGINE + * stopped looking, and `target_gone` means there was nothing left to look + * at. Neither is evidence the message was lost, and auto-filing them + * produced issues #471 and #443 -- tracker noise describing cmuxlayer's own + * timers, not a defect. The local evidence ticket is still written either + * way, so the verdict keeps citing its evidence; only the escalation stops. + */ + private deliveryFailureEscalationDecline( + reason: string, + ): string | null { + if (reason === "verify_deadline_elapsed") { + return ( + "background verify ran out of deadline before observing an outcome; " + + "no evidence the message was lost" + ); + } + if (reason === "target_gone") { + return ( + "the target agent disappeared before an outcome could be observed; " + + "no evidence the message was lost" + ); + } + return null; + } + private async fileConfirmedFailureTicket( receipt: AgentDeliveryReceipt, reason: string, @@ -7028,7 +7109,11 @@ export class AgentEngine { dir: ticketDir, }); receipt.ticket_filed = true; + const declineReason = this.deliveryFailureEscalationDecline(reason); + receipt.ticket_escalated = declineReason === null; + receipt.ticket_escalation_declined_reason = declineReason; this.persistDeliveryReceipts(); + if (declineReason !== null) return; if (!written.created) return; if (!this.deliveryIssueFiler) return; try { @@ -7058,6 +7143,27 @@ export class AgentEngine { if (agent.paused === true) { continue; } + // AIDEV-NOTE (T2 #467): a retryable refusal is nonterminal, but it is + // not unbounded. Without this, a target stuck `booting` retried behind + // a 30s-capped backoff forever and the caller's receipt never resolved + // -- a lead could wait on it indefinitely. The lifetime is stamped on + // the first retryable requeue below; when it elapses the caller gets a + // terminal answer that cites the gate reason that kept refusing. + if ( + receipt.queue_deadline_at && + Date.now() >= Date.parse(receipt.queue_deadline_at) + ) { + const gateReason = receipt.error ?? "no gate reason recorded"; + receipt.delivery_state = "failed_confirmed"; + receipt.terminal = true; + receipt.submit_verified = false; + receipt.resolved_at = new Date().toISOString(); + receipt.next_attempt_at = null; + receipt.error = `queue_deadline_elapsed after ${receipt.retry_count} retryable refusals; last gate reason: ${gateReason}`; + this.persistDeliveryReceipts(); + this.appendDeliveryReceiptEventBestEffort(receipt); + continue; + } if ( receipt.next_attempt_at && Date.parse(receipt.next_attempt_at) > Date.now() @@ -7123,6 +7229,9 @@ export class AgentEngine { if (error instanceof RetryableDeliveryError) { receipt.submission_started_at = null; receipt.retry_count += 1; + receipt.queue_deadline_at ??= new Date( + Date.now() + this.deliveryQueueDeadlineMs, + ).toISOString(); const backoffMs = Math.min( 30_000, 250 * 2 ** Math.min(receipt.retry_count - 1, 16), diff --git a/src/cmux-client.ts b/src/cmux-client.ts index 2e2408bb..50a7b52a 100644 --- a/src/cmux-client.ts +++ b/src/cmux-client.ts @@ -28,6 +28,16 @@ import { parseCmuxStatusFrame } from "./cmux-status-frame.js"; import { isCmuxAccessControlDenied } from "./cmux-access-control.js"; const execFileAsync = promisify(execFile); +/** + * Hard ceiling on one CLI-fallback `cmux` invocation. + * + * AIDEV-NOTE (T2 #450): the socket transport is bounded by its own + * REQUEST_TIMEOUT_MS, but this fallback had none -- a wedged `cmux` + * subprocess held its caller (notably the delivery snapshot read) forever. + * Matches the socket client's 10s budget so neither transport can outlast the + * other. + */ +export const CMUX_CLI_EXEC_TIMEOUT_MS = 10_000; const STANDARD_BUNDLED_CMUX = "/Applications/cmux.app/Contents/Resources/bin/cmux"; @@ -55,6 +65,8 @@ interface CmuxClientOptions { bin?: string; env?: NodeJS.ProcessEnv; existsSync?: (path: string) => boolean; + /** Hard ceiling on one CLI-fallback exec; defaults to CMUX_CLI_EXEC_TIMEOUT_MS. */ + execTimeoutMs?: number; } interface CmuxIdentifyResult { @@ -75,6 +87,7 @@ export class CmuxClient { private bin?: string; private env?: NodeJS.ProcessEnv; private existsSync: (path: string) => boolean; + private execTimeoutMs: number; private observerTransportGeneration = 0; constructor(opts?: CmuxClientOptions) { @@ -82,6 +95,10 @@ export class CmuxClient { this.bin = opts?.bin; this.env = opts?.env; this.existsSync = opts?.existsSync ?? fs.existsSync; + this.execTimeoutMs = Math.max( + 1, + opts?.execTimeoutMs ?? CMUX_CLI_EXEC_TIMEOUT_MS, + ); } setEnv(env: NodeJS.ProcessEnv | undefined): void { @@ -121,6 +138,7 @@ export class CmuxClient { ) : await execFileAsync(bin, cliArgs, { ...(env ? { env } : {}), + timeout: this.execTimeoutMs, }); return stdout; } catch (error) { diff --git a/src/server.ts b/src/server.ts index 78438694..614978e8 100644 --- a/src/server.ts +++ b/src/server.ts @@ -955,6 +955,7 @@ export function buildPublicDeliveryReceipt(input: { evidencedState === "submitted" || evidencedState === "failed" || evidencedState === "failed_confirmed"; + const warning = input.WARNING ?? defaultNonDeliveryWarning(evidencedState); return { delivered: evidencedState === "submitted", terminal, @@ -966,10 +967,42 @@ export function buildPublicDeliveryReceipt(input: { ? { delivery: evidencedState, delivery_state: evidencedState } : {}), ...(input.delivery_id ? { delivery_id: input.delivery_id } : {}), - ...(input.WARNING ? { WARNING: input.WARNING } : {}), + ...(warning ? { WARNING: warning } : {}), }; } +/** + * One plain-language line a caller cannot honestly quote as "sent". + * + * AIDEV-NOTE (T2 #445): `ok:true` with `delivered:false` was routinely read as + * success -- a lead's own words: "I treated the first as evidence of the + * second." The booleans two levels down were correct and still misread, so the + * receipt now says it in words at the top level. Explicit callers keep their + * own WARNING (the paused-target line is more specific than this default). + */ +function defaultNonDeliveryWarning( + state: PublicDeliveryState | undefined, +): string | undefined { + switch (state) { + case "pending_verify": + case "queued": + case "queued_followup": + return ( + `NOT DELIVERED YET — state ${state}: the message has not been observed ` + + "to land. It resolves in the background; do not relay as sent. " + + "Query wait_for({delivery_id}) for the terminal outcome." + ); + case "failed": + case "failed_confirmed": + return ( + `NOT DELIVERED — terminal failure (${state}). The message did not ` + + "land and will not be retried; do not relay as sent." + ); + default: + return undefined; + } +} + export function pausedTargetWarning(source: string): string { return ( `WARNING — target pane is paused (source: ${source}) and cannot act. ` + @@ -1021,6 +1054,7 @@ type SubmitVerificationFailureReason = | "surface_screen_empty" | "input_still_pending" | "working_status_not_observed" + | "consumption_not_observed" | "submit_evidence_absent"; class SubmitVerificationError extends Error { @@ -1065,13 +1099,19 @@ class DeliverySafetyGateError extends Error { constructor( readonly error_code: - "blocked_by_interactive_prompt" | "blocked_by_permission_prompt", + | "blocked_by_interactive_prompt" + | "blocked_by_permission_prompt" + | "blocked_by_composer_draft", readonly screen: ParsedScreenResult, ) { super( error_code === "blocked_by_permission_prompt" ? "delivery blocked by active permission prompt" - : "target surface has an open picker/menu; refused to type (would be consumed as menu keystrokes)", + : error_code === "blocked_by_composer_draft" + ? "target composer already holds text this delivery did not write; " + + "refused to type (typing + Return would submit or mutate someone " + + "else's draft). Clear the composer, or resend once it is empty." + : "target surface has an open picker/menu; refused to type (would be consumed as menu keystrokes)", ); this.name = "DeliverySafetyGateError"; } @@ -2394,6 +2434,44 @@ function screenShowsPendingInput( ); } +/** + * True when the target composer holds text that this delivery did not put + * there -- a human's half-written draft, or another agent's unsent message. + * + * AIDEV-NOTE (T2 #442): the discriminator is "is the visible composer content + * contained in the payload we are about to type". A partially-typed payload + * (chunk 1 landed, chunk 2 pending) IS ours and must stay deliverable, so it + * is compared whitespace-insensitively against the payload rather than + * required to be empty. Anything else is foreign and must not be typed into. + */ +function composerHoldsForeignDraft( + screenText: string, + submittedText: string, +): boolean { + // AIDEV-NOTE (T2 #442): Cursor is deliberately exempt. Its composer RETAINS + // the accepted text after a submit (the "retained composer" state #441/#449 + // built evidence rules around), so a non-empty Cursor composer is the normal + // post-send screen, not an unsent draft -- and nothing on that screen + // distinguishes the two. Guarding it would refuse every legitimate second + // send to a Cursor pane. Claude and Codex clear on submit, so there a + // non-empty composer really does mean somebody's text is sitting unsent. + if (inferComposerCli(screenText) === "cursor") { + return false; + } + const composer = extractComposerInputRegion(screenText, submittedText); + if (composer === null) { + // No recognisable composer region (bare shell, unreadable frame). The + // pre-existing gates own those cases; do not invent a refusal here. + return false; + } + const compactDraft = composer.replace(/\s+/g, ""); + if (!compactDraft) { + return false; + } + const compactPayload = submittedText.replace(/\s+/g, ""); + return !(compactPayload.length > 0 && compactPayload.includes(compactDraft)); +} + function stripCodexQueueGutter(line: string): string { return line.replace(/^\s*[│┃║┆┊]\s?/, "").trimEnd(); } @@ -2756,6 +2834,7 @@ function parseRawSubmitEvidenceMetrics( export const __submitEvidenceTestHooks = { extractComposerInputRegion, screenShowsPendingInput, + composerHoldsForeignDraft, }; function hasRawSubmitEvidenceIncrease( @@ -4678,6 +4757,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { surface: string, workspace?: string, cli?: CliType, + draftGuard?: { submittedText: string }, ): Promise<{ text: string; parsed: ParsedScreenResult } | null> => { const snapshot = await readParsedSurface(surface, workspace, { throwOnSurfaceGone: true, @@ -4700,6 +4780,23 @@ export function createServer(opts?: CreateServerOptions): McpServer { ); } + // AIDEV-NOTE (T2 #442): a composer that already holds text nobody in this + // delivery wrote is a human (or another agent) mid-draft. Typing into it + // concatenates, and the Return that follows SUBMITS their words. The + // picker/permission gates above never covered this: the screen is a + // perfectly ordinary ready composer, it just is not empty. Refuse before + // the first keystroke -- a refused send is recoverable, a submitted draft + // is not. + if ( + draftGuard && + composerHoldsForeignDraft(snapshot.text, draftGuard.submittedText) + ) { + throw new DeliverySafetyGateError( + "blocked_by_composer_draft", + snapshot.parsed, + ); + } + return snapshot; }; @@ -4780,6 +4877,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { let retriedAt: number | null = null; let sawReadableScreen = false; let sawBlankScreen = false; + let lastBootConsumptionRefuted = false; const screenIncludesSubmittedText = (screenText: string): boolean => { const trimmed = opts.text.trim(); if (!trimmed) { @@ -4842,6 +4940,18 @@ export function createServer(opts?: CreateServerOptions): McpServer { delivery: "queued_followup", }; } + // AIDEV-NOTE (T2 #427): `0 tokens` is a definitive negative. An agent + // handed a prompt that has consumed nothing did not receive it, whatever + // the composer looks like -- a slow boot can render a working-looking + // banner while the CLI is still initialising, and that race produced + // `submit_verified: true` receipts for prompts that never left the + // composer. A NULL token count stays inconclusive on purpose: several + // CLIs never report one, and treating unknown as zero would turn this + // guard into a fleet-wide false negative. + const bootConsumptionRefuted = + opts.require_working_status === true && + snapshot.parsed.token_count === 0; + lastBootConsumptionRefuted = bootConsumptionRefuted; const screenCli = inferComposerCli(snapshot.text, snapshot.parsed); const cursorShowsSubmittedResponse = screenCli === "cursor" && @@ -4856,6 +4966,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { const composerInput = extractComposerInputRegion(snapshot.text); if ( !hasPendingSubmitEvidence && + !bootConsumptionRefuted && (isSubmitVerifiedStatus(snapshot.parsed.status) || cursorShowsSubmittedResponse) ) { @@ -4870,6 +4981,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { composerInput !== null && composerInput.trim() === "" && !hasPendingSubmitEvidence && + !bootConsumptionRefuted && screenHasAnyAgentIdentity(snapshot.text, snapshot.parsed); if (hasClearedAgentComposer) { sawClearedComposerEvidence = true; @@ -4993,6 +5105,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { opts.require_working_status || lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput || + lastBootConsumptionRefuted || !sawReadableScreen ? false : noSubmitEvidenceResult; @@ -5005,9 +5118,11 @@ export function createServer(opts?: CreateServerOptions): McpServer { ? sawBlankScreen ? "surface_screen_empty" : "surface_read_unavailable" - : opts.require_working_status - ? "working_status_not_observed" - : "submit_evidence_absent"; + : lastBootConsumptionRefuted + ? "consumption_not_observed" + : opts.require_working_status + ? "working_status_not_observed" + : "submit_evidence_absent"; const allowPendingVerify = opts.source_event === "send_to" || opts.source_event === "dispatch_nudge"; if (allowPendingVerify && submitVerified === false) { @@ -5078,9 +5193,24 @@ export function createServer(opts?: CreateServerOptions): McpServer { } return { ...receipt, bytes: 0 }; } + // AIDEV-NOTE (T2 #442): the draft guard covers the caller-initiated relay + // paths, where refusing is cheap and a foreign draft is a live risk. Boot + // and cwd delivery run against a pane cmuxlayer just launched, where the + // only text on screen is the launcher's own echo -- refusing there would + // break spawn, not protect a human. + const draftGuardedEvent = + opts.source_event === "send_to" || + opts.source_event === "send_to_agent" || + opts.source_event === "send_input" || + opts.source_event === "dispatch_nudge"; + const draftGuardText = opts.chunks.join(""); const deliverySafetySnapshot = await assertDeliveryTargetIsSafe( opts.surface, opts.workspace, + undefined, + draftGuardedEvent && draftGuardText.trim().length > 0 + ? { submittedText: draftGuardText } + : undefined, ); const deliveryBatches = buildInputDeliveryBatches(opts.chunks); const shouldPaste = shouldPasteInputDelivery( @@ -5489,7 +5619,17 @@ export function createServer(opts?: CreateServerOptions): McpServer { }); if (snapshot) { lastText = snapshot.text; - if (isSubmitVerifiedStatus(snapshot.parsed.status)) { + const metrics = parseRawSubmitEvidenceMetrics(snapshot.text); + // AIDEV-NOTE (T2 #427): `0 tokens` is a definitive negative -- an agent + // handed a prompt that has consumed nothing did not receive it. A slow + // boot (MCP servers still connecting, banner mid-render) can present a + // working-looking status while the prompt is still sitting in the + // composer, and accepting that status produced fully-verified receipts + // for workers that sat at `0 tokens` with their entire brief unsent. + // A NULL count stays inconclusive on purpose: several CLIs never + // report one, and reading unknown as zero would break every boot. + const consumptionRefuted = metrics.tokenCount === 0; + if (!consumptionRefuted && isSubmitVerifiedStatus(snapshot.parsed.status)) { return; } @@ -5501,10 +5641,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { if ( composerInput !== null && !hasPendingInput && - hasRawSubmitEvidenceIncrease( - parseRawSubmitEvidenceMetrics(snapshot.text), - opts.baseline_metrics, - ) + hasRawSubmitEvidenceIncrease(metrics, opts.baseline_metrics) ) { return; } @@ -5515,6 +5652,7 @@ export function createServer(opts?: CreateServerOptions): McpServer { !hasPendingInput; if ( composerCleared && + !consumptionRefuted && screenHasAnyAgentIdentity(snapshot.text, snapshot.parsed) ) { if (composerInput === lastClearedComposerInput) { diff --git a/tests/agent-engine.test.ts b/tests/agent-engine.test.ts index c2e259c4..2801ab80 100644 --- a/tests/agent-engine.test.ts +++ b/tests/agent-engine.test.ts @@ -14053,6 +14053,80 @@ Session ID: ${sessionId}`, } }); + it("terminalizes a retryable requeue once its bounded queue lifetime elapses (#467)", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-08-11T18:00:00.000Z")); + const boundedEngine = new AgentEngine( + stateMgr, + new AgentRegistry(stateMgr, async () => liveSurfaces), + mockClient, + { + spawnPreflight: async () => {}, + sessionIdentityResolver: () => null, + inboxOpts: { baseDir: TEST_DIR }, + deliveryQueueDeadlineMs: 60_000, + }, + ); + try { + stateMgr.writeState( + makeRecord({ + agent_id: "stuck-booting-delivery", + state: "booting", + surface_id: "surface:42", + }), + ); + liveSurfaces = [makeSurface("surface:42")]; + await boundedEngine.getRegistry().reconstitute(); + boundedEngine.setDeliverySubmitter(async () => { + throw new RetryableDeliveryError( + 'Agent "stuck-booting-delivery" is not in an interactive state (current: booting)', + ); + }); + const receipt = boundedEngine.queueDelivery({ + agent_id: "stuck-booting-delivery", + text: "a lead is waiting on this", + press_enter: true, + source_event: "send_to", + }); + + await boundedEngine.drainDeliveryQueue(); + const deferred = boundedEngine.getDeliveryReceipt( + receipt.delivery_id, + )!; + expect(deferred).toMatchObject({ + delivery_state: "queued", + terminal: false, + }); + // The requeue now carries a stated lifetime instead of retrying forever. + expect(deferred.queue_deadline_at).toBe("2026-08-11T18:01:00.000Z"); + + // Retries inside the lifetime stay nonterminal. + vi.setSystemTime(new Date("2026-08-11T18:00:30.000Z")); + await boundedEngine.drainDeliveryQueue(); + expect( + boundedEngine.getDeliveryReceipt(receipt.delivery_id), + ).toMatchObject({ delivery_state: "queued", terminal: false }); + + // Past the lifetime the lead gets an answer, citing the gate reason. + vi.setSystemTime(new Date("2026-08-11T18:01:00.001Z")); + await boundedEngine.drainDeliveryQueue(); + expect( + boundedEngine.getDeliveryReceipt(receipt.delivery_id), + ).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + submit_verified: false, + resolved_at: expect.any(String), + error: expect.stringMatching( + /queue_deadline_elapsed.*not in an interactive state.*booting/s, + ), + }); + } finally { + boundedEngine.dispose(); + vi.useRealTimers(); + } + }); + it("does not replay a queued receipt whose persisted submission had started", () => { const receipt = engine.queueDelivery({ agent_id: "crashed-queued-delivery", diff --git a/tests/delivery-truth-t2.test.ts b/tests/delivery-truth-t2.test.ts new file mode 100644 index 00000000..59382338 --- /dev/null +++ b/tests/delivery-truth-t2.test.ts @@ -0,0 +1,463 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { + chmodSync, + mkdirSync, + mkdtempSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import type { ExecFn } from "../src/cmux-client.js"; +import { withTestSurfaceObserver } from "./helpers/test-surface-observer.js"; + +let testDir = ""; + +async function loadServerModule() { + vi.resetModules(); + const serverModule = await import("../src/server.js"); + return { + ...serverModule, + createServerContext: ( + opts: Parameters[0] = {}, + ) => serverModule.createServerContext(withTestSurfaceObserver(opts)), + createServer: ( + opts: Parameters[0] = {}, + ) => + serverModule.createServer( + opts.context ? opts : withTestSurfaceObserver(opts), + ), + }; +} + +function parseToolResult(result: any) { + return result.structuredContent ?? JSON.parse(result.content[0].text); +} + +async function spawnReadyAgent(server: any) { + const spawn = server._registeredTools["spawn_agent"]; + const spawnResult = await spawn.handler( + { + repo: "brainlayer", + model: "sonnet", + cli: "claude", + workspace: "workspace:1", + boot_prompt_timeout_ms: 100, + }, + {} as any, + ); + const agentId = parseToolResult(spawnResult).agent_id; + const engine = server._registeredTools.interact._engine; + const registry = engine.getRegistry(); + registry.set(agentId, { ...registry.get(agentId), state: "ready" }); + return agentId; +} + +function makeLifecycleExec(readScreenText: () => string): ExecFn { + return vi.fn().mockImplementation(async (_cmd, args: string[]) => { + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: "surface:new", + text: readScreenText(), + lines: 20, + scrollback_used: false, + }), + stderr: "", + }; + } + if (args.includes("list-workspaces")) { + return { + stdout: JSON.stringify({ + workspaces: [ + { + ref: "workspace:1", + title: "Main", + index: 0, + selected: true, + pinned: false, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-panes")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:new"], + selected_surface_ref: "surface:new", + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + pane_ref: "pane:1", + surfaces: [ + { + ref: "surface:new", + title: "agent-pane", + type: "terminal", + index: 0, + selected: true, + }, + ], + }), + stderr: "", + }; + } + return { + stdout: JSON.stringify({ + workspace: "workspace:1", + surface: "surface:new", + pane: "pane:1", + title: "", + type: "terminal", + }), + stderr: "", + }; + }); +} + +const mutatedPane = (mockExec: any): boolean => + mockExec.mock.calls.some(([, args]: [string, string[]]) => + args.some((arg: string) => + ["send", "set-buffer", "paste-buffer", "send-key"].includes(arg), + ), + ); + +describe("T2 delivery truth — composer draft safety (#442)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("send_to refuses a composer holding human-typed draft text, before typing anything", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n❯ "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + // A human left a half-written thought in the composer and never submitted. + screenText = "Claude Code\n> so about the release, I think we should\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + const parsed = parseToolResult(result); + expect(result.isError).toBe(true); + expect(parsed).toMatchObject({ + ok: false, + delivered: false, + delivery_state: "failed", + terminal: true, + typed: false, + submit_attempted: false, + error_code: "blocked_by_composer_draft", + }); + expect(mutatedPane(mockExec)).toBe(false); + context.dispose(); + }, 20_000); + + it("send_to still delivers when the composer is empty", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n❯ "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + screenText = "Claude Code\n> \nCLAUDE_COUNTER:1\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + expect(parseToolResult(result).ok).toBe(true); + expect(mutatedPane(mockExec)).toBe(true); + context.dispose(); + }); +}); + +describe("T2 delivery truth — unmissable non-delivery (#445)", () => { + it("attaches a plain-language WARNING to every nonterminal receipt", async () => { + const { buildPublicDeliveryReceipt } = await loadServerModule(); + for (const state of ["pending_verify", "queued", "queued_followup"] as const) { + const receipt = buildPublicDeliveryReceipt({ + delivery_state: state, + delivery_id: "d-1", + typed: true, + submit_attempted: true, + submit_verified: null, + retry_count: 0, + }); + expect(receipt).toMatchObject({ delivered: false, terminal: false }); + expect(receipt.WARNING).toMatch(/NOT DELIVERED YET/); + expect(receipt.WARNING).toMatch(/do not relay as sent/i); + } + }); + + it("attaches a terminal-failure WARNING to failed and failed_confirmed receipts", async () => { + const { buildPublicDeliveryReceipt } = await loadServerModule(); + for (const state of ["failed", "failed_confirmed"] as const) { + const receipt = buildPublicDeliveryReceipt({ + delivery_state: state, + typed: true, + submit_attempted: true, + submit_verified: false, + retry_count: 0, + }); + expect(receipt).toMatchObject({ delivered: false, terminal: true }); + expect(receipt.WARNING).toMatch(/NOT DELIVERED/); + expect(receipt.WARNING).toMatch(/do not relay as sent/i); + } + }); + + it("leaves a verified submitted receipt unwarned and keeps an explicit WARNING", async () => { + const { buildPublicDeliveryReceipt, pausedTargetWarning } = + await loadServerModule(); + expect( + buildPublicDeliveryReceipt({ + delivery_state: "submitted", + typed: true, + submit_attempted: true, + submit_verified: true, + retry_count: 0, + }).WARNING, + ).toBeUndefined(); + expect( + buildPublicDeliveryReceipt({ + delivery_state: "queued", + typed: false, + submit_attempted: false, + submit_verified: null, + retry_count: 0, + WARNING: pausedTargetWarning("registry"), + }).WARNING, + ).toBe(pausedTargetWarning("registry")); + }); +}); + +describe("T2 delivery truth — CLI-fallback hang guard (#450)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-cli-timeout-")); + }); + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("kills a wedged cmux subprocess instead of awaiting it forever", async () => { + const { CmuxClient, CMUX_CLI_EXEC_TIMEOUT_MS } = await import( + "../src/cmux-client.js" + ); + // The default ceiling matches the socket transport's request budget. + expect(CMUX_CLI_EXEC_TIMEOUT_MS).toBe(10_000); + + // A `cmux` that never exits, whatever arguments it is handed. + const wedged = join(testDir, "wedged-cmux"); + writeFileSync(wedged, "#!/bin/sh\nsleep 600\n", "utf8"); + chmodSync(wedged, 0o755); + + const client = new CmuxClient({ bin: wedged, execTimeoutMs: 150 }); + const startedAt = Date.now(); + + await expect(client.listWorkspaces()).rejects.toThrow(); + + expect(Date.now() - startedAt).toBeLessThan(5_000); + }, 20_000); +}); + +function makeBootSplitExec(postReturnScreen: string): ExecFn { + let promptSent = false; + return vi.fn().mockImplementation(async (_cmd, args: string[]) => { + if (args.includes("new-split")) { + return { + stdout: JSON.stringify({ + workspace: "workspace:1", + surface: "surface:2", + pane: "pane:1", + title: "New", + type: "terminal", + }), + stderr: "", + }; + } + if (args.includes("list-panes")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + panes: [ + { + ref: "pane:1", + index: 0, + focused: true, + surface_count: 1, + surface_refs: ["surface:2"], + selected_surface_ref: "surface:2", + }, + ], + }), + stderr: "", + }; + } + if (args.includes("list-pane-surfaces")) { + return { + stdout: JSON.stringify({ + workspace_ref: "workspace:1", + window_ref: "window:1", + pane_ref: "pane:1", + surfaces: [ + { + ref: "surface:2", + title: "mimirClaude", + type: "terminal", + index: 0, + selected: true, + }, + ], + }), + stderr: "", + }; + } + if (args.includes("send") && !args.includes("send-key")) { + promptSent = true; + return { stdout: "{}", stderr: "" }; + } + if (args.includes("read-screen")) { + return { + stdout: JSON.stringify({ + surface: "surface:2", + text: promptSent + ? postReturnScreen + : "previous shell output: bun install\nClaude Code\n> ", + lines: 80, + scrollback_used: false, + }), + stderr: "", + }; + } + return { stdout: "{}", stderr: "" }; + }); +} + +describe("T2 delivery truth — boot consumption evidence (#427)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-boot-tokens-")); + }); + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + const writePrompt = () => { + const promptPath = join(testDir, "boot.md"); + mkdirSync(testDir, { recursive: true }); + writeFileSync(promptPath, "Read and follow the brief", "utf8"); + return promptPath; + }; + + it("refuses submit_verified:true while the booted CLI reports 0 tokens", async () => { + const { createServer } = await loadServerModule(); + // #427's race: the CLI is still initialising, so the screen already reads + // as a working agent while the boot prompt has been consumed by nothing -- + // 0 tokens, $0.00, 0m. + const mockExec = makeBootSplitExec( + [ + "Claude Code", + "> ", + " 0 tokens", + " Opus 5 | $0.00 | 0m", + "Working (1s - esc to interrupt)", + ].join("\n"), + ); + const server = createServer({ exec: mockExec, skipAgentLifecycle: true }); + + const result = await (server as any)._registeredTools[ + "new_split" + ].handler( + { + direction: "right", + workspace: "workspace:1", + boot_prompt_path: writePrompt(), + boot_prompt_timeout_ms: 50, + }, + {} as any, + ); + const parsed = parseToolResult(result); + + // The spawn reports the truth instead of a fully-verified receipt for a + // prompt the agent never consumed. + expect(parsed.ok).toBe(false); + expect(parsed.submit_verified).not.toBe(true); + expect(parsed.delivered).not.toBe(true); + expect(parsed.boot_prompt_delivered).not.toBe(true); + expect(parsed.error).toMatch(/boot prompt submit evidence/i); + }, 20_000); + + it("still verifies a boot prompt once the CLI has consumed tokens", async () => { + const { createServer } = await loadServerModule(); + const mockExec = makeBootSplitExec( + [ + "Claude Code", + "> ", + " 1.2k tokens", + " Opus 5 | $0.03 | 1m", + "Working (1s - esc to interrupt)", + ].join("\n"), + ); + const server = createServer({ exec: mockExec, skipAgentLifecycle: true }); + + const result = await (server as any)._registeredTools[ + "new_split" + ].handler( + { + direction: "right", + workspace: "workspace:1", + boot_prompt_path: writePrompt(), + boot_prompt_timeout_ms: 50, + }, + {} as any, + ); + const parsed = parseToolResult(result); + + expect(parsed.boot_prompt_receipt.submit_verified).toBe(true); + expect(parsed.boot_prompt_receipt.delivered).toBe(true); + }, 20_000); +}); diff --git a/tests/send-to-v2-background-verify.test.ts b/tests/send-to-v2-background-verify.test.ts index 5be8392f..ec007c6f 100644 --- a/tests/send-to-v2-background-verify.test.ts +++ b/tests/send-to-v2-background-verify.test.ts @@ -629,9 +629,190 @@ describe("send_to v2 background verify", () => { }); expect(existsSync(ticketDir)).toBe(true); expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + // T2 #471: the local evidence ticket stays; escalating a deadline the + // ENGINE ran out of to the issue tracker does not. + expect(filed).toHaveLength(0); + }); + + it("does not escalate a verify_deadline_elapsed failure to a GitHub issue (#471)", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryVerifyDeadlineMs: 1_000, + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "engine stopped looking", + press_enter: true, + }), + ); + + await vi.advanceTimersByTimeAsync(1_000); + const engine = server._registeredTools.interact._engine; + await engine.verifyPendingDeliveries(); + + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + expect(receipt).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "verify_deadline_elapsed", + }); + // Local forensics are still written -- the verdict must cite its evidence. + expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + // ...but the tracker does not get an issue for the engine giving up. + expect(filed).toHaveLength(0); + expect(receipt.ticket_escalated).toBe(false); + expect(receipt.ticket_escalation_declined_reason).toMatch( + /no evidence the message was lost/i, + ); + }); + + it("does not escalate a target_gone failure to a GitHub issue (#443)", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "target vanished", + press_enter: true, + }), + ); + + const engine = server._registeredTools.interact._engine; + engine.getRegistry().remove("agent-1"); + for (let miss = 0; miss < DELIVERY_TARGET_GONE_CONFIRM_MISSES; miss += 1) { + await engine.verifyPendingDeliveries(); + } + + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + expect(receipt).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "target_gone", + ticket_escalated: false, + }); + expect(readdirSync(ticketDir).length).toBeGreaterThan(0); + expect(filed).toHaveLength(0); + }); + + it("still escalates a failure the verifier positively observed", async () => { + const ticketDir = join(TEST_DIR, "tickets"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + + const sent = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "observed lost", + press_enter: true, + }), + ); + + const engine = server._registeredTools.interact._engine; + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + await engine.verifyPendingDeliveries(); + + expect(engine.getDeliveryReceipt(sent.delivery_id)).toMatchObject({ + delivery_state: "failed_confirmed", + terminal: true, + error: "composer_rejected_input", + ticket_escalated: true, + }); expect(filed).toHaveLength(1); }); + it("bounds a wedged snapshot read so the verifier cannot stall forever (#450)", async () => { + let reads = 0; + const client = new FakeAgentSurfaceClient(); + const stateMgr = new StateManager(TEST_DIR); + const engine = new AgentEngine( + stateMgr, + new AgentRegistry(stateMgr, async () => []), + client as any, + { + deliveryVerifyTimeoutMs: 50, + // Mirrors the production verifier: no snapshot, no evidence. + deliveryVerifier: async (_receipt, snapshot) => + snapshot?.text + ? { outcome: "delivered" as const, submit_verified: true } + : { + outcome: "pending" as const, + reason: "surface_read_unavailable", + }, + // The CLI-fallback surface read has no subprocess timeout of its own; + // model a wedged `cmux` on the first pass. + deliverySnapshotReader: async () => { + reads += 1; + if (reads === 1) { + return new Promise(() => {}) as never; + } + return { text: "Claude Code\n> \n" }; + }, + }, + ); + try { + engine.acceptPendingVerify({ + delivery_id: "wedged-read-1", + agent_id: "agent-1", + text: "snapshot read wedges", + press_enter: true, + source_event: "send_to", + retry_count: 0, + }); + + let settled = false; + const first = engine.verifyPendingDeliveries().then(() => { + settled = true; + }); + await vi.advanceTimersByTimeAsync(50); + await first; + + expect(settled).toBe(true); + expect(engine.getDeliveryReceipt("wedged-read-1")).toMatchObject({ + delivery_state: "pending_verify", + terminal: false, + }); + + // The latch was released, so a later sweep still resolves the receipt. + await vi.advanceTimersByTimeAsync(30_000); + await engine.verifyPendingDeliveries(); + expect(reads).toBe(2); + expect(engine.getDeliveryReceipt("wedged-read-1")).toMatchObject({ + delivery_state: "submitted", + terminal: true, + }); + } finally { + engine.dispose(); + } + }); + it("dedupes evidence tickets by failure signature", async () => { const ticketDir = join(TEST_DIR, "tickets"); const filed: unknown[] = []; @@ -665,7 +846,8 @@ describe("send_to v2 background verify", () => { await server._registeredTools.interact._engine.verifyPendingDeliveries(); expect(readdirSync(ticketDir)).toHaveLength(1); - expect(filed).toHaveLength(1); + // T2 #471: deadline-elapsed verdicts are never escalated, deduped or not. + expect(filed).toHaveLength(0); }); it("returns a nonterminal wait_for delivery receipt with timed_out instead of rejecting", async () => { From a1ea8a372b38d6df560d53dd14b49980cae9abcd Mon Sep 17 00:00:00 2001 From: Etan Joseph Heyman Date: Wed, 19 Aug 2026 22:37:49 +0300 Subject: [PATCH 2/2] =?UTF-8?q?fix(t2):=20clear=20ITERATE=20=E2=80=94=20dr?= =?UTF-8?q?aft=20guard=20reads=20the=20prompt=20line,=20escalation=20repor?= =?UTF-8?q?ts=20what=20happened?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1 on #483: B1, B1a, B2 blocking; M1, M2, M3, N1, N2 minimality. B1 — the draft guard fired on ordinary Claude panes with an EMPTY composer. extractComposerInputRegion appends every following line until it recognises chrome, so `? for shortcuts`, `accept edits on` and `Working (2s - esc to interrupt)` all read as somebody's draft, and a ready pane got a hard terminal refusal. The guard now reads only the composer's own prompt line: a human draft always begins there, chrome never does. Widening the chrome whitelist is what put the hole there in the first place. B1a — a blocked composer is no longer a terminal verdict. The same screen is produced by this delivery's own unflushed prior message, and the guard cannot tell that from a human draft. Both want "wait for the composer to flush", so the refusal is now a RetryableDeliveryError: send_to returns a nonterminal queued receipt, and #467's bounded lifetime still guarantees a terminal answer. A terminal `failed` there was this PR's own disease. B2 — ticket_escalated was stamped true before three exits that mean no issue was filed (deduped signature, no filer, filer threw). Both fields are now written from one resolved outcome at every exit, and only after the filer actually resolves. M1 use withDeliveryVerifyTimeout at both race sites instead of one helper and one hand-rolled copy of itself. M2 delete the unreachable lastBootConsumptionRefuted disjunct. M3 unpack the five-deep failure-reason ternary into a named if-chain, and give assertDeliveryTargetIsSafe an options object so no caller passes a positional undefined. N1 state why paused targets deliberately do not age out. N2 killSignal SIGKILL so a SIGTERM-ignoring subprocess still cannot hang the promise. Two pre-existing defects found and filed rather than fixed here: #498 (the ticket_filed idempotence guard does not hold, so the ticket path runs twice) and #499 (vitest cross-file interference, reproducible on 49a0b2b). Co-Authored-By: Claude Opus 5 (1M context) --- src/agent-engine.ts | 63 +++++--- src/cmux-client.ts | 3 + src/server.ts | 166 ++++++++++++++------- tests/delivery-truth-t2.test.ts | 149 +++++++++++++++++- tests/send-to-v2-background-verify.test.ts | 93 ++++++++++++ 5 files changed, 390 insertions(+), 84 deletions(-) diff --git a/src/agent-engine.ts b/src/agent-engine.ts index ac2bcd43..330fb878 100644 --- a/src/agent-engine.ts +++ b/src/agent-engine.ts @@ -6920,29 +6920,16 @@ export class AgentEngine { } snapshot = snapshots.get(snapshotKey) ?? null; } - let timeout: ReturnType | null = null; try { - observation = await Promise.race([ + observation = await this.withDeliveryVerifyTimeout( this.deliveryVerifier(receipt, snapshot), - new Promise((_resolve, reject) => { - timeout = setTimeout( - () => - reject( - new Error( - `Delivery verify timed out after ${this.deliveryVerifyTimeoutMs}ms`, - ), - ), - this.deliveryVerifyTimeoutMs, - ); - }), - ]); + "Delivery verify", + ); } catch (error) { observation = { outcome: "pending", reason: error instanceof Error ? error.message : String(error), }; - } finally { - if (timeout) clearTimeout(timeout); } receipt.verify_last_attempt_at = new Date().toISOString(); this.persistDeliveryReceipts(); @@ -7109,17 +7096,39 @@ export class AgentEngine { dir: ticketDir, }); receipt.ticket_filed = true; + + // AIDEV-NOTE (T2 B2): both fields are written from the SAME resolved + // outcome, at every exit, and never before the escalation is known. + // Stamping `escalated: true` up front and then returning early -- deduped + // signature, no filer configured, filer threw -- left receipts asserting + // an escalation that never happened. That is this lane's own disease: a + // receipt reporting something the engine did not observe. + const settleEscalation = (declined: string | null): void => { + receipt.ticket_escalated = declined === null; + receipt.ticket_escalation_declined_reason = declined; + this.persistDeliveryReceipts(); + }; + const declineReason = this.deliveryFailureEscalationDecline(reason); - receipt.ticket_escalated = declineReason === null; - receipt.ticket_escalation_declined_reason = declineReason; - this.persistDeliveryReceipts(); - if (declineReason !== null) return; - if (!written.created) return; - if (!this.deliveryIssueFiler) return; + if (declineReason !== null) return settleEscalation(declineReason); + if (!written.created) { + return settleEscalation( + "an issue for this failure signature was already filed; " + + "this occurrence was appended to the existing ticket", + ); + } + if (!this.deliveryIssueFiler) { + return settleEscalation("no issue filer is configured"); + } try { await this.deliveryIssueFiler(ticket); - } catch { - // Local ticket is authoritative; GitHub is best-effort. + settleEscalation(null); + } catch (error) { + // Local ticket is authoritative; GitHub is best-effort -- but the + // receipt must say the escalation did not land. + settleEscalation( + `issue filer failed: ${error instanceof Error ? error.message : String(error)}`, + ); } } @@ -7140,6 +7149,12 @@ export class AgentEngine { this.appendDeliveryReceiptEventBestEffort(receipt); continue; } + // AIDEV-NOTE (T2 N1): a paused target deliberately does NOT age out. + // #467's bounded lifetime exists for a target that is failing to + // become interactive on its own; pausing is a human's resumable act, + // and expiring queued work under it would discard the message the + // pause was protecting. The receipt stays nonterminal, and the + // paused-target WARNING already tells the caller it is not delivered. if (agent.paused === true) { continue; } diff --git a/src/cmux-client.ts b/src/cmux-client.ts index 50a7b52a..6bae8acc 100644 --- a/src/cmux-client.ts +++ b/src/cmux-client.ts @@ -139,6 +139,9 @@ export class CmuxClient { : await execFileAsync(bin, cliArgs, { ...(env ? { env } : {}), timeout: this.execTimeoutMs, + // N2: a subprocess that ignores SIGTERM would still hang the + // promise the timeout exists to bound. + killSignal: "SIGKILL", }); return stdout; } catch (error) { diff --git a/src/server.ts b/src/server.ts index 614978e8..a7a109f0 100644 --- a/src/server.ts +++ b/src/server.ts @@ -1049,6 +1049,29 @@ class DeliveryError extends Error { } } +/** + * Why a submit could not be verified. This is the sentence a receipt shows the + * fleet when a delivery did not land, so it is spelled out rather than nested: + * the order is "what we saw" before "what we required", most specific first. + */ +function resolveSubmitVerificationFailureReason(observed: { + sawPendingInput: boolean; + sawReadableScreen: boolean; + sawBlankScreen: boolean; + bootConsumptionRefuted: boolean; + requireWorkingStatus: boolean; +}): SubmitVerificationFailureReason { + if (observed.sawPendingInput) return "input_still_pending"; + if (!observed.sawReadableScreen) { + return observed.sawBlankScreen + ? "surface_screen_empty" + : "surface_read_unavailable"; + } + if (observed.bootConsumptionRefuted) return "consumption_not_observed"; + if (observed.requireWorkingStatus) return "working_status_not_observed"; + return "submit_evidence_absent"; +} + type SubmitVerificationFailureReason = | "surface_read_unavailable" | "surface_screen_empty" @@ -1099,19 +1122,13 @@ class DeliverySafetyGateError extends Error { constructor( readonly error_code: - | "blocked_by_interactive_prompt" - | "blocked_by_permission_prompt" - | "blocked_by_composer_draft", + "blocked_by_interactive_prompt" | "blocked_by_permission_prompt", readonly screen: ParsedScreenResult, ) { super( error_code === "blocked_by_permission_prompt" ? "delivery blocked by active permission prompt" - : error_code === "blocked_by_composer_draft" - ? "target composer already holds text this delivery did not write; " + - "refused to type (typing + Return would submit or mutate someone " + - "else's draft). Clear the composer, or resend once it is empty." - : "target surface has an open picker/menu; refused to type (would be consumed as menu keystrokes)", + : "target surface has an open picker/menu; refused to type (would be consumed as menu keystrokes)", ); this.name = "DeliverySafetyGateError"; } @@ -2434,15 +2451,49 @@ function screenShowsPendingInput( ); } +/** + * The text sitting on the composer's OWN input line, or null when no composer + * prompt line is on screen. + * + * AIDEV-NOTE (T2 #442/B1): deliberately NOT `extractComposerInputRegion`. That + * one appends every following line until it recognises a chrome line, so any + * footer missing from `isComposerFooterOrChromeLine` -- `? for shortcuts`, + * `accept edits on`, `Working (2s * esc to interrupt)` -- reads as composer + * content. That is fine for its own callers, which ask "is the composer + * CLEAR", where a false non-empty just withholds submit evidence. It is not + * fine for the draft guard, where a false non-empty REFUSES a ready pane. So + * the guard reads only the prompt line: a human draft always begins there, + * and chrome never does. Widening the chrome whitelist instead is what put + * this hole in the first place. + */ +function composerPromptLineInput(screenText: string): string | null { + const lines = normalizeTerminalText(screenText).split("\n"); + const cli = inferComposerCli(screenText); + const start = currentComposerRegionStart(cli, lines); + let end = lines.length; + while (end > start && isComposerFooterOrChromeLine(lines[end - 1] ?? "")) { + end -= 1; + } + + for (let index = end - 1; index >= start; index -= 1) { + const line = lines[index] ?? ""; + const match = + matchComposerPromptLine(line) ?? matchLegacyClaudePromptLine(cli, line); + if (match) { + return normalizeKnownPlaceholderComposerInput(cli, match.input.trim()); + } + } + return null; +} + /** * True when the target composer holds text that this delivery did not put - * there -- a human's half-written draft, or another agent's unsent message. + * there -- a human's half-written draft, or an earlier message still unflushed. * - * AIDEV-NOTE (T2 #442): the discriminator is "is the visible composer content - * contained in the payload we are about to type". A partially-typed payload - * (chunk 1 landed, chunk 2 pending) IS ours and must stay deliverable, so it - * is compared whitespace-insensitively against the payload rather than - * required to be empty. Anything else is foreign and must not be typed into. + * AIDEV-NOTE (T2 #442): a partially-typed payload (chunk 1 landed, chunk 2 + * pending) IS ours and must stay deliverable, so the line content is compared + * whitespace-insensitively against the payload rather than required to be + * empty. */ function composerHoldsForeignDraft( screenText: string, @@ -2458,18 +2509,21 @@ function composerHoldsForeignDraft( if (inferComposerCli(screenText) === "cursor") { return false; } - const composer = extractComposerInputRegion(screenText, submittedText); - if (composer === null) { - // No recognisable composer region (bare shell, unreadable frame). The - // pre-existing gates own those cases; do not invent a refusal here. + // No recognisable composer prompt line (bare shell, unreadable frame). The + // pre-existing gates own those cases; do not invent a refusal here. + const promptLine = composerPromptLineInput(screenText); + if (promptLine === null) { return false; } - const compactDraft = composer.replace(/\s+/g, ""); + const compactDraft = promptLine.replace(/\s+/g, ""); if (!compactDraft) { return false; } const compactPayload = submittedText.replace(/\s+/g, ""); - return !(compactPayload.length > 0 && compactPayload.includes(compactDraft)); + if (compactPayload.length === 0) { + return true; + } + return !compactPayload.includes(compactDraft); } function stripCodexQueueGutter(line: string): string { @@ -4753,12 +4807,14 @@ export function createServer(opts?: CreateServerOptions): McpServer { } }; - const assertDeliveryTargetIsSafe = async ( - surface: string, - workspace?: string, - cli?: CliType, - draftGuard?: { submittedText: string }, - ): Promise<{ text: string; parsed: ParsedScreenResult } | null> => { + const assertDeliveryTargetIsSafe = async (opts: { + surface: string; + workspace?: string; + cli?: CliType; + /** When set, also refuse a composer already holding someone else's text. */ + draftGuardText?: string; + }): Promise<{ text: string; parsed: ParsedScreenResult } | null> => { + const { surface, workspace, cli } = opts; const snapshot = await readParsedSurface(surface, workspace, { throwOnSurfaceGone: true, }); @@ -4787,13 +4843,22 @@ export function createServer(opts?: CreateServerOptions): McpServer { // perfectly ordinary ready composer, it just is not empty. Refuse before // the first keystroke -- a refused send is recoverable, a submitted draft // is not. + // + // RETRYABLE, not terminal (T2 B1a): the same screen is produced by this + // delivery's OWN unflushed prior message (the queued_followup shape), and + // the guard cannot tell that from a human draft. For both, the right + // answer is "wait for the composer to flush", which the v2 queue already + // expresses -- and #467's bounded queue lifetime guarantees the caller + // still gets a terminal answer if it never does. A terminal `failed` here + // would be this PR's own disease: a verdict the engine did not observe. if ( - draftGuard && - composerHoldsForeignDraft(snapshot.text, draftGuard.submittedText) + opts.draftGuardText !== undefined && + composerHoldsForeignDraft(snapshot.text, opts.draftGuardText) ) { - throw new DeliverySafetyGateError( - "blocked_by_composer_draft", - snapshot.parsed, + throw new RetryableDeliveryError( + "target composer already holds text this delivery did not write; " + + "refused to type (typing + Return would submit or mutate it). " + + "Delivery stays queued until the composer is clear.", ); } @@ -5105,24 +5170,20 @@ export function createServer(opts?: CreateServerOptions): McpServer { opts.require_working_status || lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput || - lastBootConsumptionRefuted || !sawReadableScreen ? false : noSubmitEvidenceResult; const failureReason: SubmitVerificationFailureReason | null = - submitVerified !== false - ? null - : lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput - ? "input_still_pending" - : !sawReadableScreen - ? sawBlankScreen - ? "surface_screen_empty" - : "surface_read_unavailable" - : lastBootConsumptionRefuted - ? "consumption_not_observed" - : opts.require_working_status - ? "working_status_not_observed" - : "submit_evidence_absent"; + submitVerified === false + ? resolveSubmitVerificationFailureReason({ + sawPendingInput: + lastHasPendingSubmitEvidence || lastRetryEligiblePendingInput, + sawReadableScreen, + sawBlankScreen, + bootConsumptionRefuted: lastBootConsumptionRefuted, + requireWorkingStatus: opts.require_working_status === true, + }) + : null; const allowPendingVerify = opts.source_event === "send_to" || opts.source_event === "dispatch_nudge"; if (allowPendingVerify && submitVerified === false) { @@ -5204,14 +5265,13 @@ export function createServer(opts?: CreateServerOptions): McpServer { opts.source_event === "send_input" || opts.source_event === "dispatch_nudge"; const draftGuardText = opts.chunks.join(""); - const deliverySafetySnapshot = await assertDeliveryTargetIsSafe( - opts.surface, - opts.workspace, - undefined, - draftGuardedEvent && draftGuardText.trim().length > 0 - ? { submittedText: draftGuardText } - : undefined, - ); + const deliverySafetySnapshot = await assertDeliveryTargetIsSafe({ + surface: opts.surface, + workspace: opts.workspace, + ...(draftGuardedEvent && draftGuardText.trim().length > 0 + ? { draftGuardText } + : {}), + }); const deliveryBatches = buildInputDeliveryBatches(opts.chunks); const shouldPaste = shouldPasteInputDelivery( opts.chunks, diff --git a/tests/delivery-truth-t2.test.ts b/tests/delivery-truth-t2.test.ts index 59382338..cfc62d92 100644 --- a/tests/delivery-truth-t2.test.ts +++ b/tests/delivery-truth-t2.test.ts @@ -173,16 +173,13 @@ describe("T2 delivery truth — composer draft safety (#442)", () => { ); const parsed = parseToolResult(result); - expect(result.isError).toBe(true); expect(parsed).toMatchObject({ - ok: false, delivered: false, - delivery_state: "failed", - terminal: true, - typed: false, - submit_attempted: false, - error_code: "blocked_by_composer_draft", + terminal: false, + delivery_state: "queued", }); + expect(parsed.WARNING).toMatch(/not delivered yet/i); + expect(parsed.WARNING).toMatch(/composer already holds text/i); expect(mutatedPane(mockExec)).toBe(false); context.dispose(); }, 20_000); @@ -214,6 +211,144 @@ describe("T2 delivery truth — composer draft safety (#442)", () => { }); }); +describe("T2 delivery truth — draft guard must not fire on chrome (B1)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + // Every frame below has an EMPTY composer. The line under it is ordinary + // Claude chrome that `isComposerFooterOrChromeLine` does not happen to + // whitelist -- and a whitelist miss must never cost a ready pane a refusal. + const EMPTY_COMPOSER_FRAMES: Array<[string, string]> = [ + [ + "shortcut hint", + ["Claude Code", "", "\u23fa Compared both approaches.", "", "\u276f", "? for shortcuts"].join( + "\n", + ), + ], + [ + "accept-edits mode", + ["Claude Code", "> ", "\u23f5\u23f5 accept edits on (shift+tab to cycle)"].join("\n"), + ], + [ + "busy spinner", + ["Claude Code", "\u23fa Done.", "> ", "Working (2s \u2022 esc to interrupt)"].join("\n"), + ], + ["interrupt hint", ["Claude Code", "> ", " esc to interrupt"].join("\n")], + ]; + + it.each(EMPTY_COMPOSER_FRAMES)( + "treats an empty composer under %s as deliverable", + async (_label, screen) => { + const { __submitEvidenceTestHooks } = await loadServerModule(); + expect( + __submitEvidenceTestHooks.composerHoldsForeignDraft( + screen, + "fleet message", + ), + ).toBe(false); + }, + ); + + it("still refuses when the prompt line itself carries someone else's text", async () => { + const { __submitEvidenceTestHooks } = await loadServerModule(); + expect( + __submitEvidenceTestHooks.composerHoldsForeignDraft( + ["Claude Code", "> so about the release, I think we", "? for shortcuts"].join( + "\n", + ), + "fleet message", + ), + ).toBe(true); + }); + + it("send_to delivers to a busy Claude pane whose composer is empty", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n\u276f "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + screenText = [ + "Claude Code", + "\u23fa Done.", + "> ", + "Working (2s \u2022 esc to interrupt)", + ].join("\n"); + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "fleet message", press_enter: true }, + {} as any, + ); + + expect(parseToolResult(result).ok).toBe(true); + expect(mutatedPane(mockExec)).toBe(true); + context.dispose(); + }, 20_000); +}); + +describe("T2 delivery truth — a blocked composer is not a terminal verdict (B1a)", () => { + beforeEach(() => { + testDir = mkdtempSync(join(tmpdir(), "cmuxlayer-t2-delivery-truth-")); + }); + + afterEach(() => { + rmSync(testDir, { recursive: true, force: true }); + vi.resetModules(); + }); + + it("queues instead of terminally failing when the composer holds unflushed text", async () => { + const { createServer, createServerContext } = await loadServerModule(); + let screenText = "Claude Code\n\u276f "; + const mockExec = makeLifecycleExec(() => screenText); + const context = createServerContext({ + exec: mockExec, + stateDir: testDir, + disableSpawnPreflight: true, + sessionIdentityResolver: () => null, + }); + const server = createServer({ context }); + const agentId = await spawnReadyAgent(server); + + // This delivery's OWN prior message, still sitting unflushed in the + // composer (the queued_followup shape). The guard cannot tell it from a + // human draft -- and must not, because the right answer for both is + // "wait for the composer to flush", never a terminal failure. + screenText = "Claude Code\n> an earlier message that has not flushed yet\n"; + mockExec.mockClear(); + + const result = await (server as any)._registeredTools["send_to"].handler( + { agent_id: agentId, text: "second message", press_enter: true }, + {} as any, + ); + + const parsed = parseToolResult(result); + expect(parsed).toMatchObject({ + ok: true, + delivered: false, + delivery_state: "queued", + terminal: false, + }); + expect(parsed.WARNING).toMatch(/not delivered yet/i); + expect(parsed.WARNING).toMatch(/composer already holds text/i); + // Still the property #442 exists for: nothing was typed. + expect(mutatedPane(mockExec)).toBe(false); + context.dispose(); + }, 20_000); +}); + describe("T2 delivery truth — unmissable non-delivery (#445)", () => { it("attaches a plain-language WARNING to every nonterminal receipt", async () => { const { buildPublicDeliveryReceipt } = await loadServerModule(); diff --git a/tests/send-to-v2-background-verify.test.ts b/tests/send-to-v2-background-verify.test.ts index ec007c6f..19f59faf 100644 --- a/tests/send-to-v2-background-verify.test.ts +++ b/tests/send-to-v2-background-verify.test.ts @@ -813,6 +813,99 @@ describe("send_to v2 background verify", () => { } }); + it("does not claim escalation when the ticket was deduped, the filer is absent, or the filer threw (B2)", async () => { + const observedFailure = async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + }); + + const sendAndVerify = async (extras: Record) => { + const client = new FakeAgentSurfaceClient(); + const local = createVerifyServer(client, extras as any); + registerAgent(local); + const sent = parseResult( + await callTool(local, "send_to", { + agent_id: "agent-1", + text: `escalation exit ${JSON.stringify(extras).length}`, + press_enter: true, + }), + ); + const engine = local._registeredTools.interact._engine; + engine.setDeliveryVerifier(observedFailure); + await engine.verifyPendingDeliveries(); + const receipt = engine.getDeliveryReceipt(sent.delivery_id); + await local.close(); + return receipt; + }; + + // (1) no filer configured -- nothing can be escalated. + expect( + await sendAndVerify({ deliveryTicketDir: join(TEST_DIR, "t-nofiler") }), + ).toMatchObject({ ticket_filed: true, ticket_escalated: false }); + + // (2) the filer threw -- the issue was not filed. + expect( + await sendAndVerify({ + deliveryTicketDir: join(TEST_DIR, "t-throws"), + deliveryIssueFiler: async () => { + throw new Error("gh unavailable"); + }, + }), + ).toMatchObject({ ticket_filed: true, ticket_escalated: false }); + }); + + it("does not claim escalation for a deduped signature (B2)", async () => { + const ticketDir = join(TEST_DIR, "tickets-dedupe-escalation"); + const filed: unknown[] = []; + const client = new FakeAgentSurfaceClient(); + server = createVerifyServer(client, { + deliveryTicketDir: ticketDir, + deliveryIssueFiler: async (ticket) => { + filed.push(ticket); + }, + }); + registerAgent(server); + const engine = server._registeredTools.interact._engine; + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + + const first = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "first observed loss", + press_enter: true, + }), + ); + await engine.verifyPendingDeliveries(); + client.resetTypedInput(); + const second = parseResult( + await callTool(server, "send_to", { + agent_id: "agent-1", + text: "second observed loss", + press_enter: true, + }), + ); + engine.setDeliveryVerifier(async () => ({ + outcome: "failed_confirmed" as const, + reason: "composer_rejected_input", + })); + await engine.verifyPendingDeliveries(); + + // Same signature: exactly one escalation reached the tracker, and the + // receipt that did NOT reach it says so, with a reason. + expect(filed).toHaveLength(1); + const deduped = engine.getDeliveryReceipt(second.delivery_id); + expect(deduped).toMatchObject({ + ticket_filed: true, + ticket_escalated: false, + }); + expect(deduped.ticket_escalation_declined_reason).toMatch( + /already filed/i, + ); + }); + it("dedupes evidence tickets by failure signature", async () => { const ticketDir = join(TEST_DIR, "tickets"); const filed: unknown[] = [];