From be3a664e5962caf6ede35588a3fa9af6ae246a72 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 00:41:30 -0700 Subject: [PATCH 01/13] fix(opencode): recover empty final responses --- README.md | 2 +- src/core/agents/opencode.test.ts | 74 ++++++++++++++++++++++++++++++- src/core/agents/opencode.ts | 76 +++++++++++++++++++++++++++----- 3 files changed, 140 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 7b061e82..988aa125 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an OpenCode turn completes without a final answer, gnhf nudges the same session once to continue before recording a failure, and records the nudge in the run log. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/opencode.test.ts b/src/core/agents/opencode.test.ts index 6209ce4b..35b51a99 100644 --- a/src/core/agents/opencode.test.ts +++ b/src/core/agents/opencode.test.ts @@ -22,6 +22,7 @@ vi.mock("../debug-log.js", () => ({ })); import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; import { OpenCodeAgent } from "./opencode.js"; import { buildAgentOutputSchema } from "./types.js"; @@ -34,6 +35,7 @@ const STOP_AGENT_OUTPUT_SCHEMA = buildAgentOutputSchema({ }); const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -884,7 +886,53 @@ describe("OpenCodeAgent", () => { }); }); - it("rejects with 'OpenCode produced no final answer' when the stream ends with no structured output and no final_answer text", async () => { + it("continues the same session once when the first turn has no final answer", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ healthy: true, version: "1.3.13" })) + .mockResolvedValueOnce(jsonResponse({ id: "session-123" })) + .mockResolvedValueOnce( + sseResponse([ + 'data: {"directory":"/repo","payload":{"type":"message.updated","properties":{"sessionID":"session-123","info":{"id":"msg-empty-1","role":"assistant","tokens":{"input":1,"output":1,"cache":{"read":0,"write":0}}}}}}\n\n', + 'data: {"directory":"/repo","payload":{"type":"session.idle","properties":{"sessionID":"session-123"}}}\n\n', + ]), + ) + .mockResolvedValueOnce(promptAsyncResponse()) + .mockResolvedValueOnce( + sseResponse( + finalAnswerEvents("recovered", { + input: 2, + output: 3, + read: 0, + write: 0, + }), + ), + ) + .mockResolvedValueOnce(promptAsyncResponse()) + .mockResolvedValueOnce(jsonResponse(true)); + + await expect(agent.run("test", "/repo")).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + usage: { inputTokens: 3, outputTokens: 4 }, + }); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "opencode:output:continuation", + expect.objectContaining({ sessionId: "session-123", attempt: 1 }), + ); + + const promptRequests = fetchMock.mock.calls.filter( + ([url, init]) => + String(url).includes("/prompt_async") && init?.method === "POST", + ); + expect(promptRequests).toHaveLength(2); + expect(promptRequests[1]?.[1]?.body).toContain( + "You did not produce a final answer", + ); + }); + + it("rejects with 'OpenCode produced no final answer' after one continuation is also empty", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); @@ -905,11 +953,23 @@ describe("OpenCodeAgent", () => { parts: [{ type: "step-start" }], }), ) + .mockResolvedValueOnce( + sseResponse([ + 'data: {"directory":"/repo","payload":{"type":"message.part.updated","properties":{"sessionID":"session-123","part":{"id":"finish-2","type":"step-finish","tokens":{"input":2,"output":1,"cache":{"read":0,"write":0}}}}}}\n\n', + 'data: {"directory":"/repo","payload":{"type":"session.idle","properties":{"sessionID":"session-123"}}}\n\n', + ]), + ) + .mockResolvedValueOnce(promptAsyncResponse()) .mockResolvedValueOnce(jsonResponse(true)); await expect(agent.run("test", "/repo")).rejects.toThrow( "OpenCode produced no final answer", ); + expect( + fetchMock.mock.calls.filter(([url]) => + String(url).includes("/prompt_async"), + ), + ).toHaveLength(2); }); it("does not fall back to reasoning-phase text when no final_answer text was emitted", async () => { @@ -926,6 +986,12 @@ describe("OpenCodeAgent", () => { ]), ) .mockResolvedValueOnce(promptAsyncResponse()) + .mockResolvedValueOnce( + sseResponse( + 'data: {"directory":"/repo","payload":{"type":"session.idle","properties":{"sessionID":"session-123"}}}\n\n', + ), + ) + .mockResolvedValueOnce(promptAsyncResponse()) .mockResolvedValueOnce(jsonResponse(true)); await expect(agent.run("test", "/repo")).rejects.toThrow( @@ -950,6 +1016,12 @@ describe("OpenCodeAgent", () => { ]), ) .mockResolvedValueOnce(promptAsyncResponse()) + .mockResolvedValueOnce( + sseResponse( + 'data: {"directory":"/repo","payload":{"type":"session.idle","properties":{"sessionID":"session-123"}}}\n\n', + ), + ) + .mockResolvedValueOnce(promptAsyncResponse()) .mockResolvedValueOnce(jsonResponse(true)); await expect(agent.run("test", "/repo")).rejects.toThrow( diff --git a/src/core/agents/opencode.ts b/src/core/agents/opencode.ts index c797a640..fc19d4a2 100644 --- a/src/core/agents/opencode.ts +++ b/src/core/agents/opencode.ts @@ -142,6 +142,34 @@ interface OpenCodeDeps { spawn?: typeof spawn; } +const EMPTY_RESPONSE_CONTINUATION_PROMPT = + "You did not produce a final answer. Continue and provide your final summary now."; + +class OpenCodeEmptyResponseError extends Error { + constructor() { + super("OpenCode produced no final answer"); + this.name = "OpenCodeEmptyResponseError"; + } +} + +function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { + return { + inputTokens: left.inputTokens + right.inputTokens, + outputTokens: left.outputTokens + right.outputTokens, + cacheReadTokens: left.cacheReadTokens + right.cacheReadTokens, + cacheCreationTokens: left.cacheCreationTokens + right.cacheCreationTokens, + }; +} + +function emptyTokenUsage(): TokenUsage { + return { + inputTokens: 0, + outputTokens: 0, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }; +} + interface OpenCodeServer { baseUrl: string; child: ChildProcessWithoutNullStreams; @@ -371,15 +399,43 @@ export class OpenCodeAgent implements Agent { try { const server = await this.ensureServer(cwd, runController.signal); sessionId = await this.createSession(server, cwd, runController.signal); - const result = await this.streamMessage( - server, - sessionId, - buildPrompt(prompt, this.schema), - runController.signal, - logStream, - onUsage, - onMessage, - ); + let firstTurnUsage = emptyTokenUsage(); + let result: AgentResult; + try { + result = await this.streamMessage( + server, + sessionId, + buildPrompt(prompt, this.schema), + runController.signal, + logStream, + (usage) => { + firstTurnUsage = usage; + onUsage?.(usage); + }, + onMessage, + ); + } catch (error) { + if (!(error instanceof OpenCodeEmptyResponseError)) throw error; + + appendDebugLog("opencode:output:continuation", { + sessionId, + attempt: 1, + prompt: EMPTY_RESPONSE_CONTINUATION_PROMPT, + }); + const retryResult = await this.streamMessage( + server, + sessionId, + EMPTY_RESPONSE_CONTINUATION_PROMPT, + runController.signal, + logStream, + (usage) => onUsage?.(addTokenUsage(firstTurnUsage, usage)), + onMessage, + ); + result = { + output: retryResult.output, + usage: addTokenUsage(firstTurnUsage, retryResult.usage), + }; + } appendDebugLog("opencode:run:end", { sessionId, elapsedMs: Date.now() - runStartedAt, @@ -1083,7 +1139,7 @@ export class OpenCodeAgent implements Agent { sessionId, hasStructuredOutput: structuredOutputFromSSE !== null, }); - throw new Error("OpenCode produced no final answer"); + throw new OpenCodeEmptyResponseError(); } try { From d965707121bd9b25d185ac3fcf67fc38ffef37d3 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 02:25:41 -0700 Subject: [PATCH 02/13] no-mistakes(review): gate empty-response nudge on turn completion across session agents --- README.md | 2 +- src/core/agents/acp.test.ts | 79 ++++++++ src/core/agents/acp.ts | 290 +++++++++++++++++------------- src/core/agents/empty-response.ts | 95 ++++++++++ src/core/agents/opencode.test.ts | 29 +++ src/core/agents/opencode.ts | 91 +++------- src/core/agents/rovodev.test.ts | 133 ++++++++++++++ src/core/agents/rovodev.ts | 60 +++++-- 8 files changed, 577 insertions(+), 202 deletions(-) create mode 100644 src/core/agents/empty-response.ts diff --git a/README.md b/README.md index 988aa125..f694fd7e 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an OpenCode turn completes without a final answer, gnhf nudges the same session once to continue before recording a failure, and records the nudge in the run log. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When a session-based agent (OpenCode, Rovo Dev, or an ACP target) completes a turn without a final answer, gnhf nudges the same session once to continue before recording a failure, and records the nudge in the run log. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/acp.test.ts b/src/core/agents/acp.test.ts index 25947efe..cba01efd 100644 --- a/src/core/agents/acp.test.ts +++ b/src/core/agents/acp.test.ts @@ -516,6 +516,85 @@ describe("AcpAgent", () => { expect(result.output).toEqual(VALID_OUTPUT); }); + it("nudges the same session once when a completed turn produced no output text", async () => { + const { runtime, calls } = createFakeRuntime([ + { events: [], result: { status: "completed" } }, + { + events: [textDelta(JSON.stringify(VALID_OUTPUT))], + result: { status: "completed" }, + }, + ]); + const agent = makeAgent(runtime); + const onUsage = vi.fn(); + + const result = await agent.run("p", "/w", { onUsage }); + + expect(result.output).toEqual(VALID_OUTPUT); + expect(calls.startTurnInputs).toHaveLength(2); + expect(calls.ensureSessionInputs).toHaveLength(1); + expect(calls.startTurnInputs[1]?.text).toContain( + "You did not produce a final answer", + ); + expect(calls.startTurnInputs[1]?.text).not.toContain( + "gnhf final output contract", + ); + expect(onUsage).toHaveBeenLastCalledWith(result.usage); + }); + + it("reports usage across both the empty turn and its continuation", async () => { + // The first turn streams only reasoning, so it completes with no output + // text while still burning output tokens that must survive into the total. + const firstTurnText = "thinking hard"; + const secondTurnText = JSON.stringify(VALID_OUTPUT); + const { runtime } = createFakeRuntime([ + { + events: [textDelta(firstTurnText, "thought")], + result: { status: "completed" }, + }, + { + events: [textDelta(secondTurnText)], + result: { status: "completed" }, + }, + ]); + const agent = makeAgent(runtime); + + const result = await agent.run("p", "/w"); + + expect(result.usage.outputTokens).toBe( + Math.ceil(firstTurnText.length / 4) + + Math.ceil(secondTurnText.length / 4), + ); + }); + + it("rejects after the ACP continuation is also empty", async () => { + const { runtime, calls } = createFakeRuntime([ + { events: [], result: { status: "completed" } }, + { events: [], result: { status: "completed" } }, + ]); + const agent = makeAgent(runtime); + + await expect(agent.run("p", "/w")).rejects.toThrow( + "ACP agent returned no output text", + ); + expect(calls.startTurnInputs).toHaveLength(2); + }); + + it("does not nudge when the empty turn failed rather than completed", async () => { + const { runtime, calls } = createFakeRuntime([ + { + events: [], + result: { + status: "failed", + error: { message: "transient", retryable: true }, + }, + }, + ]); + const agent = makeAgent(runtime); + + await expect(agent.run("p", "/w")).rejects.toThrow("transient"); + expect(calls.startTurnInputs).toHaveLength(1); + }); + it("throws PermanentAgentError when the runtime reports a non-retryable failure", async () => { const { runtime } = createFakeRuntime([ { diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index 4737aac9..dc921dd4 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -9,8 +9,13 @@ import { type AcpRuntimeTurnResult, type AcpxRuntime, } from "acpx/runtime"; +import type { WriteStream } from "node:fs"; import { appendDebugLog, serializeError } from "../debug-log.js"; import { redactAcpTargetForLogs } from "../config.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { parseAgentJson } from "./json-extract.js"; import { PermanentAgentError, @@ -19,6 +24,8 @@ import { type AgentOutputSchema, type AgentResult, type AgentRunOptions, + type OnMessage, + type OnUsage, type TokenUsage, } from "./types.js"; @@ -207,6 +214,49 @@ export class AcpAgent implements Agent { } this.handle = handle; + const logStream = logPath ? createWriteStream(logPath) : null; + try { + // The ACP session is persistent, so a continuation turn still sees the + // first turn's reasoning, tool calls, and the output contract that + // buildAcpPrompt already delivered - the nudge stays bare. + return await runTurnWithEmptyResponseRetry({ + logEvent: "acp:turn:continuation", + logFields: { + target: redactAcpTargetForLogs(this.target), + sessionKey: this.runId, + }, + onUsage, + initialText: buildAcpPrompt(prompt, this.schema), + runTurn: (text, onTurnUsage) => + this.runTurn({ + runtime, + handle, + text, + cwd, + signal, + onMessage, + onUsage: onTurnUsage, + logStream, + }), + }); + } finally { + logStream?.end(); + } + } + + private async runTurn(params: { + runtime: AcpxRuntimeLike; + handle: AcpRuntimeHandle; + text: string; + cwd: string; + signal?: AbortSignal; + onMessage?: OnMessage; + onUsage?: OnUsage; + logStream: WriteStream | null; + }): Promise { + const { runtime, handle, text: acpPrompt, cwd, signal, logStream } = params; + const { onMessage, onUsage } = params; + const requestId = randomUUID(); appendDebugLog("acp:turn:start", { target: redactAcpTargetForLogs(this.target), @@ -215,7 +265,6 @@ export class AcpAgent implements Agent { cwd, }); - const acpPrompt = buildAcpPrompt(prompt, this.schema); const promptTokenEstimate = estimateTokens(acpPrompt.length); const startedAt = Date.now(); @@ -266,7 +315,6 @@ export class AcpAgent implements Agent { // streams the entire response as one continuous message without any // tool_call to break it up). let outputBuf = ""; - const logStream = logPath ? createWriteStream(logPath) : null; const computeUsage = (): TokenUsage => { const usedDelta = Math.max(0, latestUsed - iterationStartUsed); @@ -300,148 +348,146 @@ export class AcpAgent implements Agent { pendingStream = null; }; - try { - // Surface an initial input-token estimate immediately so the renderer - // shows non-zero numbers as soon as the iteration starts. - onUsage?.(computeUsage()); - - try { - for await (const event of turn.events) { - logStream?.write(`${JSON.stringify(event)}\n`); - - if (event.type === "text_delta") { - const stream = event.stream ?? "output"; - const text = event.text; - if (!text) continue; - if (pendingStream !== null && pendingStream !== stream) { - flushPendingMessage(); - } - pendingStream = stream; - pendingMessage += text; - // Count both output and thought streams toward output tokens - - // reasoning is real generated text that consumes tokens. Without - // this, agents that stream reasoning before answering (Gemini, - // GPT-5, etc.) leave the renderer at 0 output tokens for the - // entire thinking phase. outputBuf stays output-only because it - // is used for JSON parsing and reasoning text would corrupt it. - if (stream === "output") { - outputBuf += text; - } - agentOutputChars += text.length; - onUsage?.(computeUsage()); - continue; - } + // Surface an initial input-token estimate immediately so the renderer + // shows non-zero numbers as soon as the iteration starts. + onUsage?.(computeUsage()); - if (event.type === "tool_call") { - // A tool_call ends the in-flight assistant message - flush - // whatever prose the assistant streamed so far, but don't surface - // the tool_call text itself. Tool descriptions like - // "tool call (completed)" are noisy and not useful in the TUI; - // the user wants to see assistant prose, not mechanics. + try { + for await (const event of turn.events) { + logStream?.write(`${JSON.stringify(event)}\n`); + + if (event.type === "text_delta") { + const stream = event.stream ?? "output"; + const text = event.text; + if (!text) continue; + if (pendingStream !== null && pendingStream !== stream) { flushPendingMessage(); - // Each tool call (not its many tool_call_update follow-ups) bumps - // the input-cost heuristic so the fallback estimate scales with - // actual work. Adapters tag the initial event "tool_call" and - // later updates "tool_call_update" - count only the former. - if (event.tag === "tool_call") { - toolCallCount += 1; - if (!usageUpdateReceived) onUsage?.(computeUsage()); - } - continue; } + pendingStream = stream; + pendingMessage += text; + // Count both output and thought streams toward output tokens - + // reasoning is real generated text that consumes tokens. Without + // this, agents that stream reasoning before answering (Gemini, + // GPT-5, etc.) leave the renderer at 0 output tokens for the + // entire thinking phase. outputBuf stays output-only because it + // is used for JSON parsing and reasoning text would corrupt it. + if (stream === "output") { + outputBuf += text; + } + agentOutputChars += text.length; + onUsage?.(computeUsage()); + continue; + } - if (event.type === "status") { - // Status events are metadata (usage_update, mode change, etc.) - // and fire frequently mid-stream. Don't surface their text via - // onMessage - it would flicker over the actual assistant message - // the user is reading. - if (typeof event.used === "number" && event.used !== latestUsed) { - latestUsed = event.used; - this.lastReportedUsed = latestUsed; - usageUpdateReceived = true; - onUsage?.(computeUsage()); - } - continue; + if (event.type === "tool_call") { + // A tool_call ends the in-flight assistant message - flush + // whatever prose the assistant streamed so far, but don't surface + // the tool_call text itself. Tool descriptions like + // "tool call (completed)" are noisy and not useful in the TUI; + // the user wants to see assistant prose, not mechanics. + flushPendingMessage(); + // Each tool call (not its many tool_call_update follow-ups) bumps + // the input-cost heuristic so the fallback estimate scales with + // actual work. Adapters tag the initial event "tool_call" and + // later updates "tool_call_update" - count only the former. + if (event.tag === "tool_call") { + toolCallCount += 1; + if (!usageUpdateReceived) onUsage?.(computeUsage()); } + continue; } - flushPendingMessage(); - } catch (error) { - if (signal?.aborted || isAbortError(error)) { - await turn.cancel({ reason: "gnhf-aborted" }).catch(() => undefined); - appendDebugLog("acp:turn:aborted", { - target: redactAcpTargetForLogs(this.target), - requestId, - elapsedMs: Date.now() - startedAt, - }); - throw createAbortError(); + + if (event.type === "status") { + // Status events are metadata (usage_update, mode change, etc.) + // and fire frequently mid-stream. Don't surface their text via + // onMessage - it would flicker over the actual assistant message + // the user is reading. + if (typeof event.used === "number" && event.used !== latestUsed) { + latestUsed = event.used; + this.lastReportedUsed = latestUsed; + usageUpdateReceived = true; + onUsage?.(computeUsage()); + } + continue; } - appendDebugLog("acp:turn:stream-error", { + } + flushPendingMessage(); + } catch (error) { + if (signal?.aborted || isAbortError(error)) { + await turn.cancel({ reason: "gnhf-aborted" }).catch(() => undefined); + appendDebugLog("acp:turn:aborted", { target: redactAcpTargetForLogs(this.target), requestId, elapsedMs: Date.now() - startedAt, - error: serializeAcpErrorForLog(error, this.target), }); - throw redactAcpErrorForThrow(error, this.target); + throw createAbortError(); } - - const result: AcpRuntimeTurnResult = await turn.result; - appendDebugLog("acp:turn:result", { + appendDebugLog("acp:turn:stream-error", { target: redactAcpTargetForLogs(this.target), requestId, - status: result.status, - stopReason: - result.status === "completed" || result.status === "cancelled" - ? result.stopReason - : undefined, - errorCode: result.status === "failed" ? result.error.code : undefined, - retryable: - result.status === "failed" ? result.error.retryable : undefined, elapsedMs: Date.now() - startedAt, - outputLength: outputBuf.length, + error: serializeAcpErrorForLog(error, this.target), }); + throw redactAcpErrorForThrow(error, this.target); + } - if (result.status === "cancelled") { - throw createAbortError(); - } - if (result.status === "failed") { - const message = redactRawAcpTargetInString( - result.error.message || "ACP turn failed", - this.target, - ); - if (result.error.retryable === false) { - throw new PermanentAgentError( - message, - result.error.code ?? "ACP_TURN_FAILED", - ); - } - throw new Error(message); - } - - if (lastOutputMessage.length === 0 && outputBuf.length === 0) { - throw new Error("ACP agent returned no output text"); - } + const result: AcpRuntimeTurnResult = await turn.result; + appendDebugLog("acp:turn:result", { + target: redactAcpTargetForLogs(this.target), + requestId, + status: result.status, + stopReason: + result.status === "completed" || result.status === "cancelled" + ? result.stopReason + : undefined, + errorCode: result.status === "failed" ? result.error.code : undefined, + retryable: + result.status === "failed" ? result.error.retryable : undefined, + elapsedMs: Date.now() - startedAt, + outputLength: outputBuf.length, + }); - // Try the most recent assistant message first - that's where the - // structured answer is supposed to live. Fall back to extracting a - // JSON object out of the full output stream if the last message - // alone doesn't parse (e.g. the agent streamed prose and JSON in - // one uninterrupted message, so we have to dig the JSON out). - let parsed = parseAgentJson(lastOutputMessage); - if (parsed === null && outputBuf !== lastOutputMessage) { - parsed = parseAgentJson(outputBuf); - } - if (parsed === null) { - const preview = (lastOutputMessage || outputBuf).slice(0, 200); - throw new Error( - `Failed to parse ACP agent output as JSON. Last assistant message started with: ${JSON.stringify(preview)}`, + if (result.status === "cancelled") { + throw createAbortError(); + } + if (result.status === "failed") { + const message = redactRawAcpTargetInString( + result.error.message || "ACP turn failed", + this.target, + ); + if (result.error.retryable === false) { + throw new PermanentAgentError( + message, + result.error.code ?? "ACP_TURN_FAILED", ); } + throw new Error(message); + } - const output = validateAgentOutput(parsed, this.schema); - return { output, usage: computeUsage() }; - } finally { - logStream?.end(); + if (lastOutputMessage.length === 0 && outputBuf.length === 0) { + throw new EmptyAgentResponseError("ACP agent returned no output text", { + turnCompleted: result.status === "completed", + }); + } + + // Try the most recent assistant message first - that's where the + // structured answer is supposed to live. Fall back to extracting a + // JSON object out of the full output stream if the last message + // alone doesn't parse (e.g. the agent streamed prose and JSON in + // one uninterrupted message, so we have to dig the JSON out). + let parsed = parseAgentJson(lastOutputMessage); + if (parsed === null && outputBuf !== lastOutputMessage) { + parsed = parseAgentJson(outputBuf); + } + if (parsed === null) { + const preview = (lastOutputMessage || outputBuf).slice(0, 200); + throw new Error( + `Failed to parse ACP agent output as JSON. Last assistant message started with: ${JSON.stringify(preview)}`, + ); } + + const output = validateAgentOutput(parsed, this.schema); + return { output, usage: computeUsage() }; } async close(): Promise { diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts new file mode 100644 index 00000000..b7168b7a --- /dev/null +++ b/src/core/agents/empty-response.ts @@ -0,0 +1,95 @@ +import { appendDebugLog } from "../debug-log.js"; +import type { AgentResult, OnUsage, TokenUsage } from "./types.js"; + +export const EMPTY_RESPONSE_CONTINUATION_PROMPT = + "You did not produce a final answer. Continue and provide your final summary now."; + +/** + * Thrown by an adapter when a turn produced no final message. `turnCompleted` + * separates "the agent finished its turn and simply said nothing" - which one + * continuation nudge can recover - from "the transport died before the turn + * ended", where nudging would post into a session that is still working (or + * gone) and would replace a clear diagnostic with a transport error. + */ +export class EmptyAgentResponseError extends Error { + readonly turnCompleted: boolean; + + constructor(message: string, options: { turnCompleted: boolean }) { + super(message); + this.name = "EmptyAgentResponseError"; + this.turnCompleted = options.turnCompleted; + } +} + +export function emptyTokenUsage(): TokenUsage { + return { + inputTokens: 0, + outputTokens: 0, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }; +} + +export function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { + const total: TokenUsage = { + inputTokens: left.inputTokens + right.inputTokens, + outputTokens: left.outputTokens + right.outputTokens, + cacheReadTokens: left.cacheReadTokens + right.cacheReadTokens, + cacheCreationTokens: left.cacheCreationTokens + right.cacheCreationTokens, + }; + if (left.estimated || right.estimated) { + total.estimated = true; + } + return total; +} + +export interface EmptyResponseRetryOptions { + /** Debug-log event name recorded when the nudge is sent. */ + logEvent: string; + logFields?: Record; + onUsage?: OnUsage; + /** Prompt for the first turn. The continuation turn is always the bare nudge. */ + initialText: string; + runTurn: (text: string, onTurnUsage: OnUsage) => Promise; +} + +/** + * Runs one turn and, if it completed without a final message, runs exactly one + * bare continuation turn in the same session. Usage reported to `onUsage` stays + * cumulative across both turns; any other failure propagates untouched. + */ +export async function runTurnWithEmptyResponseRetry({ + logEvent, + logFields, + onUsage, + initialText, + runTurn, +}: EmptyResponseRetryOptions): Promise { + let firstTurnUsage = emptyTokenUsage(); + + try { + return await runTurn(initialText, (usage) => { + firstTurnUsage = { ...usage }; + onUsage?.(usage); + }); + } catch (error) { + if (!(error instanceof EmptyAgentResponseError) || !error.turnCompleted) { + throw error; + } + + appendDebugLog(logEvent, { + ...logFields, + attempt: 1, + prompt: EMPTY_RESPONSE_CONTINUATION_PROMPT, + }); + + const retry = await runTurn(EMPTY_RESPONSE_CONTINUATION_PROMPT, (usage) => { + onUsage?.(addTokenUsage(firstTurnUsage, usage)); + }); + + return { + output: retry.output, + usage: addTokenUsage(firstTurnUsage, retry.usage), + }; + } +} diff --git a/src/core/agents/opencode.test.ts b/src/core/agents/opencode.test.ts index 35b51a99..7c2b6d75 100644 --- a/src/core/agents/opencode.test.ts +++ b/src/core/agents/opencode.test.ts @@ -932,6 +932,35 @@ describe("OpenCodeAgent", () => { ); }); + it("does not continue when the event stream ends before session.idle", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ healthy: true, version: "1.3.13" })) + .mockResolvedValueOnce(jsonResponse({ id: "session-123" })) + .mockResolvedValueOnce( + sseResponse([ + 'data: {"directory":"/repo","payload":{"type":"message.updated","properties":{"sessionID":"session-123","info":{"id":"msg-empty-1","role":"assistant","tokens":{"input":1,"output":1,"cache":{"read":0,"write":0}}}}}}\n\n', + ]), + ) + .mockResolvedValueOnce(promptAsyncResponse()) + .mockResolvedValueOnce(jsonResponse(true)); + + await expect(agent.run("test", "/repo")).rejects.toThrow( + "OpenCode produced no final answer", + ); + expect( + fetchMock.mock.calls.filter(([url]) => + String(url).includes("/prompt_async"), + ), + ).toHaveLength(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "opencode:output:continuation", + expect.anything(), + ); + }); + it("rejects with 'OpenCode produced no final answer' after one continuation is also empty", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/opencode.ts b/src/core/agents/opencode.ts index fc19d4a2..db9ed2c9 100644 --- a/src/core/agents/opencode.ts +++ b/src/core/agents/opencode.ts @@ -16,6 +16,10 @@ import { type TokenUsage, } from "./types.js"; import { appendDebugLog, serializeError } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { shutdownChildProcess } from "./managed-process.js"; interface OpenCodeMessagePart { @@ -142,34 +146,6 @@ interface OpenCodeDeps { spawn?: typeof spawn; } -const EMPTY_RESPONSE_CONTINUATION_PROMPT = - "You did not produce a final answer. Continue and provide your final summary now."; - -class OpenCodeEmptyResponseError extends Error { - constructor() { - super("OpenCode produced no final answer"); - this.name = "OpenCodeEmptyResponseError"; - } -} - -function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { - return { - inputTokens: left.inputTokens + right.inputTokens, - outputTokens: left.outputTokens + right.outputTokens, - cacheReadTokens: left.cacheReadTokens + right.cacheReadTokens, - cacheCreationTokens: left.cacheCreationTokens + right.cacheCreationTokens, - }; -} - -function emptyTokenUsage(): TokenUsage { - return { - inputTokens: 0, - outputTokens: 0, - cacheReadTokens: 0, - cacheCreationTokens: 0, - }; -} - interface OpenCodeServer { baseUrl: string; child: ChildProcessWithoutNullStreams; @@ -399,43 +375,23 @@ export class OpenCodeAgent implements Agent { try { const server = await this.ensureServer(cwd, runController.signal); sessionId = await this.createSession(server, cwd, runController.signal); - let firstTurnUsage = emptyTokenUsage(); - let result: AgentResult; - try { - result = await this.streamMessage( - server, - sessionId, - buildPrompt(prompt, this.schema), - runController.signal, - logStream, - (usage) => { - firstTurnUsage = usage; - onUsage?.(usage); - }, - onMessage, - ); - } catch (error) { - if (!(error instanceof OpenCodeEmptyResponseError)) throw error; - - appendDebugLog("opencode:output:continuation", { - sessionId, - attempt: 1, - prompt: EMPTY_RESPONSE_CONTINUATION_PROMPT, - }); - const retryResult = await this.streamMessage( - server, - sessionId, - EMPTY_RESPONSE_CONTINUATION_PROMPT, - runController.signal, - logStream, - (usage) => onUsage?.(addTokenUsage(firstTurnUsage, usage)), - onMessage, - ); - result = { - output: retryResult.output, - usage: addTokenUsage(firstTurnUsage, retryResult.usage), - }; - } + const activeSessionId = sessionId; + const result = await runTurnWithEmptyResponseRetry({ + logEvent: "opencode:output:continuation", + logFields: { sessionId: activeSessionId }, + onUsage, + initialText: buildPrompt(prompt, this.schema), + runTurn: (text, onTurnUsage) => + this.streamMessage( + server, + activeSessionId, + text, + runController.signal, + logStream, + onTurnUsage, + onMessage, + ), + }); appendDebugLog("opencode:run:end", { sessionId, elapsedMs: Date.now() - runStartedAt, @@ -1138,8 +1094,11 @@ export class OpenCodeAgent implements Agent { appendDebugLog("opencode:output:missing", { sessionId, hasStructuredOutput: structuredOutputFromSSE !== null, + sawSessionIdle, + }); + throw new EmptyAgentResponseError("OpenCode produced no final answer", { + turnCompleted: sawSessionIdle, }); - throw new OpenCodeEmptyResponseError(); } try { diff --git a/src/core/agents/rovodev.test.ts b/src/core/agents/rovodev.test.ts index e2986d99..8e48ce92 100644 --- a/src/core/agents/rovodev.test.ts +++ b/src/core/agents/rovodev.test.ts @@ -23,8 +23,10 @@ vi.mock("../debug-log.js", () => ({ })); import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -411,6 +413,137 @@ describe("RovoDevAgent", () => { expect(getPort).toHaveBeenCalledTimes(1); }); + it("continues the same session once when a closed turn produced no text", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ status: "healthy" })) + .mockResolvedValueOnce( + jsonResponse({ session_id: "session-123", title: "gnhf" }), + ) + .mockResolvedValueOnce(jsonResponse({ message: "ok", prompt_set: true })) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce( + textResponse( + [ + "event: request-usage", + 'data: {"input_tokens":10,"cache_write_tokens":0,"cache_read_tokens":0,"output_tokens":4}', + "", + "event: close", + "data: ", + "", + ].join("\n"), + ), + ) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce( + textResponse( + [ + "event: part_start", + 'data: {"index":0,"part":{"content":"{\\"success\\":true,\\"summary\\":\\"recovered\\",\\"key_changes_made\\":[],\\"key_learnings\\":[]}","part_kind":"text"},"event_kind":"part_start"}', + "", + "event: request-usage", + 'data: {"input_tokens":5,"cache_write_tokens":0,"cache_read_tokens":0,"output_tokens":3}', + "", + "event: close", + "data: ", + "", + ].join("\n"), + ), + ) + .mockResolvedValueOnce(jsonResponse({ message: "deleted" })); + + const onUsage = vi.fn(); + const result = await agent.run("test", "/repo", { onUsage }); + + expect(result.output).toEqual({ + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], + }); + expect(result.usage).toMatchObject({ inputTokens: 15, outputTokens: 7 }); + expect(onUsage).toHaveBeenLastCalledWith(result.usage); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "rovodev:output:continuation", + expect.objectContaining({ sessionId: "session-123", attempt: 1 }), + ); + + const chatMessages = fetchMock.mock.calls.filter(([url]) => + String(url).includes("/v3/set_chat_message"), + ); + expect(chatMessages).toHaveLength(2); + expect(chatMessages[1]?.[1]?.body).toContain( + "You did not produce a final answer", + ); + }); + + it("rejects after the rovodev continuation is also empty", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const emptyClosedTurn = () => + textResponse(["event: close", "data: ", ""].join("\n")); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ status: "healthy" })) + .mockResolvedValueOnce( + jsonResponse({ session_id: "session-123", title: "gnhf" }), + ) + .mockResolvedValueOnce(jsonResponse({ message: "ok", prompt_set: true })) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce(emptyClosedTurn()) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce(emptyClosedTurn()) + .mockResolvedValueOnce(jsonResponse({ message: "deleted" })); + + await expect(agent.run("test", "/repo")).rejects.toThrow( + "rovodev returned no text output", + ); + expect( + fetchMock.mock.calls.filter(([url]) => + String(url).includes("/v3/set_chat_message"), + ), + ).toHaveLength(2); + }); + + it("does not continue when the stream ends without a close event", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ status: "healthy" })) + .mockResolvedValueOnce( + jsonResponse({ session_id: "session-123", title: "gnhf" }), + ) + .mockResolvedValueOnce(jsonResponse({ message: "ok", prompt_set: true })) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce( + textResponse( + [ + "event: request-usage", + 'data: {"input_tokens":10,"cache_write_tokens":0,"cache_read_tokens":0,"output_tokens":4}', + "", + ].join("\n"), + ), + ) + .mockResolvedValueOnce(jsonResponse({ message: "deleted" })); + + await expect(agent.run("test", "/repo")).rejects.toThrow( + "rovodev returned no text output", + ); + expect( + fetchMock.mock.calls.filter(([url]) => + String(url).includes("/v3/set_chat_message"), + ), + ).toHaveLength(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "rovodev:output:continuation", + expect.anything(), + ); + }); + it("rejects when the final text is not valid JSON", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/rovodev.ts b/src/core/agents/rovodev.ts index 5fc571ba..0b44b836 100644 --- a/src/core/agents/rovodev.ts +++ b/src/core/agents/rovodev.ts @@ -14,6 +14,10 @@ import type { } from "./types.js"; import { validateAgentOutput } from "./types.js"; import { appendDebugLog, serializeError } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { parseAgentJson } from "./json-extract.js"; import { shutdownChildProcess } from "./managed-process.js"; @@ -227,22 +231,35 @@ export class RovoDevAgent implements Agent { try { const server = await this.ensureServer(cwd, runController.signal); sessionId = await this.createSession(server, runController.signal); - await this.setInlineSystemPrompt(server, sessionId, runController.signal); - await this.setChatMessage( + const activeSessionId = sessionId; + await this.setInlineSystemPrompt( server, - sessionId, - prompt, + activeSessionId, runController.signal, ); - const result = await this.streamChat( - server, - sessionId, - runController.signal, - logStream, + const result = await runTurnWithEmptyResponseRetry({ + logEvent: "rovodev:output:continuation", + logFields: { sessionId: activeSessionId }, onUsage, - onMessage, - ); + initialText: prompt, + runTurn: async (text, onTurnUsage) => { + await this.setChatMessage( + server, + activeSessionId, + text, + runController.signal, + ); + return this.streamChat( + server, + activeSessionId, + runController.signal, + logStream, + onTurnUsage, + onMessage, + ); + }, + }); appendDebugLog("rovodev:run:end", { sessionId, elapsedMs: Date.now() - runStartedAt, @@ -555,6 +572,10 @@ export class RovoDevAgent implements Agent { cacheCreationTokens: 0, }; let latestTextSegment = ""; + // Rovo Dev terminates a turn with an `event: close` frame. Seeing it is the + // only way to tell a finished-but-silent turn from a stream that was cut + // short, so it gates the empty-response continuation. + let sawClose = false; let currentTextParts: string[] = []; let currentTextIndexes = new Map(); const decoder = new TextDecoder(); @@ -598,6 +619,11 @@ export class RovoDevAgent implements Agent { } } + if (eventName === "close") { + sawClose = true; + return; + } + const rawData = dataLines.join("\n"); if (rawData.length === 0) return; @@ -614,6 +640,11 @@ export class RovoDevAgent implements Agent { ? ((payload as Record).event_kind as string) : ""); + if (kind === "close") { + sawClose = true; + return; + } + if (kind === "request-usage") { handleUsage(payload as RovoDevRequestUsageEvent); return; @@ -735,12 +766,15 @@ export class RovoDevAgent implements Agent { sessionId, elapsedMs: Date.now() - streamStartedAt, bytesRead, + sawClose, }); const finalText = latestTextSegment.trim(); if (!finalText) { - appendDebugLog("rovodev:output:missing", { sessionId }); - throw new Error("rovodev returned no text output"); + appendDebugLog("rovodev:output:missing", { sessionId, sawClose }); + throw new EmptyAgentResponseError("rovodev returned no text output", { + turnCompleted: sawClose, + }); } const schema = JSON.parse( From 24531e7fae3cd902ae13e8795ae49070622e7fd8 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 06:48:18 -0700 Subject: [PATCH 03/13] no-mistakes(review): extend empty-response nudge to all agents; fix idle gate and usage --- README.md | 2 +- src/core/agents/acp.ts | 1 + src/core/agents/claude.test.ts | 121 +++++++++++++++++++++++++-- src/core/agents/claude.ts | 95 +++++++++++++++++++-- src/core/agents/codex.test.ts | 102 ++++++++++++++++++++++ src/core/agents/codex.ts | 75 ++++++++++++++--- src/core/agents/copilot.test.ts | 76 ++++++++++++++++- src/core/agents/copilot.ts | 59 +++++++++++-- src/core/agents/empty-response.ts | 25 +++++- src/core/agents/opencode.ts | 23 +++-- src/core/agents/pi.test.ts | 90 +++++++++++++++++++- src/core/agents/pi.ts | 58 +++++++++++-- src/core/agents/rovodev.ts | 1 + src/core/agents/stream-utils.test.ts | 20 +---- src/core/agents/stream-utils.ts | 12 +-- src/core/config.test.ts | 13 +++ src/core/config.ts | 5 +- 17 files changed, 701 insertions(+), 77 deletions(-) diff --git a/README.md b/README.md index f694fd7e..91741440 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When a session-based agent (OpenCode, Rovo Dev, or an ACP target) completes a turn without a final answer, gnhf nudges the same session once to continue before recording a failure, and records the nudge in the run log. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once to continue before recording a failure, and records the nudge in the run log; session-based agents (OpenCode, Rovo Dev, ACP targets) and `claude` are nudged inside the same session, and the other CLI agents are re-asked with their usual output contract. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index dc921dd4..987e997d 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -467,6 +467,7 @@ export class AcpAgent implements Agent { if (lastOutputMessage.length === 0 && outputBuf.length === 0) { throw new EmptyAgentResponseError("ACP agent returned no output text", { turnCompleted: result.status === "completed", + usage: computeUsage(), }); } diff --git a/src/core/agents/claude.test.ts b/src/core/agents/claude.test.ts index 556876cf..383cfe57 100644 --- a/src/core/agents/claude.test.ts +++ b/src/core/agents/claude.test.ts @@ -6,11 +6,19 @@ vi.mock("node:child_process", () => ({ spawn: vi.fn(), })); +vi.mock("../debug-log.js", () => ({ + appendDebugLog: vi.fn(), + initDebugLog: vi.fn(), + serializeError: vi.fn(), +})); + import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; import { ClaudeAgent } from "./claude.js"; import { PermanentAgentError, buildAgentOutputSchema } from "./types.js"; const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); const STOP_SCHEMA = buildAgentOutputSchema({ includeStopField: true, @@ -1171,16 +1179,87 @@ describe("ClaudeAgent", () => { await expect(promise).rejects.toThrow("claude reported error"); }); - it("rejects when structured_output is null", async () => { - const proc = createMockProcess(); - mockSpawn.mockReturnValue(proc); + it("resumes the same session once with the bare nudge when structured_output is null", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); const promise = agent.run("prompt", "/cwd"); - emitLine(proc, { + emitLine(first, { + type: "system", + subtype: "init", + session_id: "session-abc", + }); + emitLine(first, { + type: "result", + subtype: "success", + is_error: false, + session_id: "session-abc", + usage: { + input_tokens: 10, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 5, + }, + structured_output: null, + }); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + + emitLine(second, { type: "result", subtype: "success", is_error: false, + session_id: "session-abc", + usage: { + input_tokens: 3, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 2, + }, + structured_output: { + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], + }, + }); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + usage: { inputTokens: 13, outputTokens: 7 }, + }); + + const continuationArgs = mockSpawn.mock.calls[1]![1] as string[]; + expect(continuationArgs).toContain("--resume"); + expect(continuationArgs[continuationArgs.indexOf("--resume") + 1]).toBe( + "session-abc", + ); + expect(continuationArgs[continuationArgs.indexOf("-p") + 1]).toBe( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + expect(continuationArgs).toContain("--json-schema"); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "claude:output:continuation", + expect.objectContaining({ attempt: 1 }), + ); + }); + + it("rejects after exactly one continuation when structured_output is still null", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + + const promise = agent.run("prompt", "/cwd"); + + const emptyResult = { + type: "result", + subtype: "success", + is_error: false, + session_id: "session-abc", usage: { input_tokens: 0, cache_read_input_tokens: 0, @@ -1188,13 +1267,43 @@ describe("ClaudeAgent", () => { output_tokens: 0, }, structured_output: null, - }); + }; - proc.emit("close", 0); + emitLine(first, emptyResult); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitLine(second, emptyResult); + second.emit("close", 0); await expect(promise).rejects.toThrow( "claude returned no structured_output", ); + expect(mockSpawn).toHaveBeenCalledTimes(2); + }); + + it("does not continue when claude reported an error result", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "result", + subtype: "error_during_execution", + is_error: true, + usage: { + input_tokens: 0, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 0, + }, + structured_output: null, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow("claude reported error"); + expect(mockSpawn).toHaveBeenCalledTimes(1); }); it("picks up structured_output from a later result event when the first had none", async () => { diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 1d3efe86..b494d1ca 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -1,5 +1,5 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream } from "node:fs"; +import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, type Agent, @@ -7,9 +7,16 @@ import { type AgentOutputSchema, type AgentResult, type AgentRunOptions, + type OnMessage, + type OnUsage, type TokenUsage, PermanentAgentError, } from "./types.js"; +import { appendDebugLog } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { shutdownChildProcess } from "./managed-process.js"; import { parseJSONLStream, setupAbortHandler } from "./stream-utils.js"; @@ -50,7 +57,10 @@ interface ClaudeResultEvent { structured_output: AgentOutput | null; } -type ClaudeEvent = ClaudeAssistantEvent | ClaudeResultEvent | { type: string }; +type ClaudeEvent = + | ClaudeAssistantEvent + | ClaudeResultEvent + | { type: string; session_id?: string }; interface ClaudeAgentDeps { bin?: string; @@ -138,10 +148,15 @@ function isFinalStructuredResult(event: ClaudeResultEvent): boolean { ); } +function userSpecifiedSessionContinuation(userArgs: string[]): boolean { + return userArgs.some((arg) => arg === "-c" || arg === "--continue"); +} + function buildClaudeArgs( prompt: string, schema: AgentOutputSchema, extraArgs?: string[], + resumeSessionId?: string | null, ): string[] { const userArgs = extraArgs ?? []; const userSpecifiedPermissionMode = userArgs.some( @@ -162,6 +177,9 @@ function buildClaudeArgs( "stream-json", "--json-schema", JSON.stringify(schema), + ...(resumeSessionId && !userSpecifiedSessionContinuation(userArgs) + ? ["--resume", resumeSessionId] + : []), ...(userSpecifiedPermissionMode ? [] : ["--dangerously-skip-permissions"]), ]; } @@ -321,19 +339,60 @@ export class ClaudeAgent implements Agent { deps.schema ?? buildAgentOutputSchema({ includeStopField: false }); } - run( + async run( prompt: string, cwd: string, options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; + const logStream = logPath ? createWriteStream(logPath) : null; + // Populated from the first turn's stream so a continuation resumes that + // exact conversation and still sees its own reasoning and tool calls. + let sessionId: string | null = null; - return new Promise((resolve, reject) => { - const logStream = logPath ? createWriteStream(logPath) : null; + try { + // `--json-schema` is a spawn flag, so the continuation turn keeps the + // output contract without any extra prompt scaffolding. + return await runTurnWithEmptyResponseRetry({ + logEvent: "claude:output:continuation", + onUsage, + initialText: prompt, + runTurn: (text, onTurnUsage) => + this.runTurn(text, cwd, { + onUsage: onTurnUsage, + onMessage, + signal, + logStream, + resumeSessionId: sessionId, + onSessionId: (id) => { + sessionId = id; + }, + }), + }); + } finally { + logStream?.end(); + } + } + + private runTurn( + prompt: string, + cwd: string, + options: { + onUsage?: OnUsage; + onMessage?: OnMessage; + signal?: AbortSignal; + logStream: WriteStream | null; + resumeSessionId: string | null; + onSessionId: (sessionId: string) => void; + }, + ): Promise { + const { onUsage, onMessage, signal, logStream } = options; + const { resumeSessionId, onSessionId } = options; + return new Promise((resolve, reject) => { const child = spawn( this.bin, - buildClaudeArgs(prompt, this.schema, this.extraArgs), + buildClaudeArgs(prompt, this.schema, this.extraArgs, resumeSessionId), { cwd, detached: this.platform !== "win32", @@ -383,6 +442,11 @@ export class ClaudeAgent implements Agent { }); parseJSONLStream(child.stdout!, logStream, (event) => { + const eventSessionId = (event as { session_id?: unknown }).session_id; + if (typeof eventSessionId === "string" && eventSessionId) { + onSessionId(eventSessionId); + } + if (event.type === "assistant") { const msg = (event as ClaudeAssistantEvent).message; const nextUsage = toTokenUsage(msg.usage); @@ -499,7 +563,6 @@ export class ClaudeAgent implements Agent { if (finalResultCleanupTimer) { clearTimeout(finalResultCleanupTimer); } - logStream?.end(); if (code !== 0 && !closedAfterFinalCleanup) { const failure = describeExitFailure(code, stdoutTail, stderr); reject( @@ -533,7 +596,23 @@ export class ClaudeAgent implements Agent { } if (!terminalResultEvent.structured_output) { - reject(new Error("claude returned no structured_output")); + appendDebugLog("claude:output:missing", { + subtype: terminalResultEvent.subtype, + resumed: resumeSessionId !== null, + }); + reject( + new EmptyAgentResponseError( + "claude returned no structured_output", + { + // A non-error `result` event with subtype "success" is claude's + // own end-of-turn signal; it just carried no final answer. + turnCompleted: true, + usage: toTokenUsage( + latestResultUsage ?? terminalResultEvent.usage, + ), + }, + ), + ); return; } diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 86369a8d..3e1a9e8f 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -6,10 +6,18 @@ vi.mock("node:child_process", () => ({ spawn: vi.fn(), })); +vi.mock("../debug-log.js", () => ({ + appendDebugLog: vi.fn(), + initDebugLog: vi.fn(), + serializeError: vi.fn(), +})); + import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; import { CodexAgent } from "./codex.js"; const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -21,6 +29,35 @@ function createMockProcess() { return proc as typeof proc & ReturnType; } +function emitJson(proc: ReturnType, event: unknown) { + proc.stdout.emit("data", Buffer.from(`${JSON.stringify(event)}\n`)); +} + +function agentMessage(text: string) { + return { + type: "item.completed", + item: { type: "agent_message", text }, + }; +} + +function turnCompleted(inputTokens: number, outputTokens: number) { + return { + type: "turn.completed", + usage: { + input_tokens: inputTokens, + cached_input_tokens: 0, + output_tokens: outputTokens, + }, + }; +} + +const FINAL_OUTPUT = JSON.stringify({ + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], +}); + describe("CodexAgent", () => { beforeEach(() => { vi.clearAllMocks(); @@ -203,4 +240,69 @@ describe("CodexAgent", () => { ); expect(proc.kill).not.toHaveBeenCalled(); }); + + it("re-asks once with the bare nudge when a completed turn had no agent message", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, turnCompleted(10, 5)); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, agentMessage(FINAL_OUTPUT)); + emitJson(second, turnCompleted(3, 2)); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + usage: { inputTokens: 13, outputTokens: 7 }, + }); + + const continuationArgs = mockSpawn.mock.calls[1]![1] as string[]; + expect(continuationArgs).toContain( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + expect(continuationArgs).toContain("--output-schema"); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "codex:output:continuation", + expect.objectContaining({ attempt: 1 }), + ); + }); + + it("does not re-ask when the turn never completed", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow("codex returned no agent message"); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "codex:output:continuation", + expect.anything(), + ); + }); + + it("fails after exactly one re-ask when the continuation is also empty", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, turnCompleted(10, 5)); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, turnCompleted(1, 1)); + second.emit("close", 0); + + await expect(promise).rejects.toThrow("codex returned no agent message"); + expect(mockSpawn).toHaveBeenCalledTimes(2); + }); }); diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index f1b25a19..bf5e25b7 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -1,12 +1,19 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream } from "node:fs"; +import { createWriteStream, type WriteStream } from "node:fs"; import type { Agent, AgentResult, AgentOutput, + OnMessage, + OnUsage, TokenUsage, AgentRunOptions, } from "./types.js"; +import { appendDebugLog } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { parseJSONLStream, setupAbortHandler, @@ -133,16 +140,47 @@ export class CodexAgent implements Agent { this.schemaPath = schemaPath; } - run( + async run( prompt: string, cwd: string, options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; + const logStream = logPath ? createWriteStream(logPath) : null; - return new Promise((resolve, reject) => { - const logStream = logPath ? createWriteStream(logPath) : null; + try { + // `--output-schema` is a spawn flag, so the continuation turn carries the + // same output contract without any extra prompt scaffolding. + return await runTurnWithEmptyResponseRetry({ + logEvent: "codex:output:continuation", + onUsage, + initialText: prompt, + runTurn: (text, onTurnUsage) => + this.runTurn(text, cwd, { + onUsage: onTurnUsage, + onMessage, + signal, + logStream, + }), + }); + } finally { + logStream?.end(); + } + } + private runTurn( + prompt: string, + cwd: string, + options: { + onUsage?: OnUsage; + onMessage?: OnMessage; + signal?: AbortSignal; + logStream: WriteStream | null; + }, + ): Promise { + const { onUsage, onMessage, signal, logStream } = options; + + return new Promise((resolve, reject) => { const child = spawn( this.bin, buildCodexArgs(prompt, this.schemaPath, this.extraArgs), @@ -163,6 +201,10 @@ export class CodexAgent implements Agent { } let lastAgentMessage: string | null = null; + // `turn.completed` is codex's own end-of-turn signal, so it - not a + // clean process exit - is what separates a finished-but-silent turn + // from a turn that never got to answer. + let sawTurnCompleted = false; const cumulative: TokenUsage = { inputTokens: 0, outputTokens: 0, @@ -180,18 +222,27 @@ export class CodexAgent implements Agent { onMessage?.(lastAgentMessage); } - if (event.type === "turn.completed" && "usage" in event) { - const u = (event as CodexTurnCompleted).usage; - cumulative.inputTokens += u.input_tokens ?? 0; - cumulative.outputTokens += u.output_tokens ?? 0; - cumulative.cacheReadTokens += u.cached_input_tokens ?? 0; - onUsage?.({ ...cumulative }); + if (event.type === "turn.completed") { + sawTurnCompleted = true; + if ("usage" in event) { + const u = (event as CodexTurnCompleted).usage; + cumulative.inputTokens += u.input_tokens ?? 0; + cumulative.outputTokens += u.output_tokens ?? 0; + cumulative.cacheReadTokens += u.cached_input_tokens ?? 0; + onUsage?.({ ...cumulative }); + } } }); - setupChildProcessHandlers(child, "codex", logStream, reject, () => { + setupChildProcessHandlers(child, "codex", reject, () => { if (!lastAgentMessage) { - reject(new Error("codex returned no agent message")); + appendDebugLog("codex:output:missing", { sawTurnCompleted }); + reject( + new EmptyAgentResponseError("codex returned no agent message", { + turnCompleted: sawTurnCompleted, + usage: cumulative, + }), + ); return; } diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index e5e326c0..62c8a66a 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -6,11 +6,19 @@ vi.mock("node:child_process", () => ({ spawn: vi.fn(), })); +vi.mock("../debug-log.js", () => ({ + appendDebugLog: vi.fn(), + initDebugLog: vi.fn(), + serializeError: vi.fn(), +})); + import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; import { CopilotAgent } from "./copilot.js"; import { buildAgentOutputSchema } from "./types.js"; const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -264,15 +272,77 @@ describe("CopilotAgent", () => { expect(args[1]).toContain("should_fully_stop"); }); - it("rejects when copilot returns no assistant message", async () => { + it("re-asks once with the bare nudge inside the usual output contract when the turn had no assistant message", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CopilotAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, { + type: "assistant.message", + data: { outputTokens: 5 }, + }); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, { + type: "assistant.message", + data: { + content: JSON.stringify({ + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], + }), + outputTokens: 3, + }, + }); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + usage: { outputTokens: 8 }, + }); + + const continuationPrompt = (mockSpawn.mock.calls[1]![1] as string[])[1]!; + expect(continuationPrompt).toContain( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + expect(continuationPrompt).toContain("gnhf final output contract"); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "copilot:output:continuation", + expect.objectContaining({ attempt: 1 }), + ); + }); + + it("rejects after exactly one re-ask when copilot still returns no assistant message", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CopilotAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + second.emit("close", 0); + + await expect(promise).rejects.toThrow("copilot returned no agent message"); + expect(mockSpawn).toHaveBeenCalledTimes(2); + }); + + it("does not re-ask when copilot exits non-zero", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); const agent = new CopilotAgent(); const promise = agent.run("test prompt", "/work/dir"); - proc.emit("close", 0); + proc.stderr.emit("data", Buffer.from("boom")); + proc.emit("close", 1); - await expect(promise).rejects.toThrow("copilot returned no agent message"); + await expect(promise).rejects.toThrow("copilot exited with code 1"); + expect(mockSpawn).toHaveBeenCalledTimes(1); }); it("rejects when the final assistant message is not valid JSON", async () => { diff --git a/src/core/agents/copilot.ts b/src/core/agents/copilot.ts index e7ae1506..73c413e1 100644 --- a/src/core/agents/copilot.ts +++ b/src/core/agents/copilot.ts @@ -1,5 +1,5 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream } from "node:fs"; +import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, parseAgentOutput, @@ -7,8 +7,15 @@ import { type AgentOutputSchema, type AgentResult, type AgentRunOptions, + type OnMessage, + type OnUsage, type TokenUsage, } from "./types.js"; +import { appendDebugLog } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { parseJSONLStream, setupAbortHandler, @@ -207,16 +214,48 @@ export class CopilotAgent implements Agent { deps.schema ?? buildAgentOutputSchema({ includeStopField: false }); } - run( + async run( prompt: string, cwd: string, options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; + const logStream = logPath ? createWriteStream(logPath) : null; - return new Promise((resolve, reject) => { - const logStream = logPath ? createWriteStream(logPath) : null; + try { + // Copilot carries its output contract in the prompt text, so the bare + // nudge goes through the same buildCopilotPrompt scaffolding the first + // turn used - no continuation-specific schema machinery. + return await runTurnWithEmptyResponseRetry({ + logEvent: "copilot:output:continuation", + onUsage, + initialText: prompt, + runTurn: (text, onTurnUsage) => + this.runTurn(text, cwd, { + onUsage: onTurnUsage, + onMessage, + signal, + logStream, + }), + }); + } finally { + logStream?.end(); + } + } + private runTurn( + prompt: string, + cwd: string, + options: { + onUsage?: OnUsage; + onMessage?: OnMessage; + signal?: AbortSignal; + logStream: WriteStream | null; + }, + ): Promise { + const { onUsage, onMessage, signal, logStream } = options; + + return new Promise((resolve, reject) => { const child = spawn( this.bin, buildCopilotArgs(prompt, this.schema, this.extraArgs), @@ -272,9 +311,17 @@ export class CopilotAgent implements Agent { } }); - setupChildProcessHandlers(child, "copilot", logStream, reject, () => { + setupChildProcessHandlers(child, "copilot", reject, () => { if (!lastAgentMessage) { - reject(new Error("copilot returned no agent message")); + appendDebugLog("copilot:output:missing", {}); + reject( + new EmptyAgentResponseError("copilot returned no agent message", { + // copilot exposes no end-of-turn event, so a clean exit is the + // only completion signal it gives. + turnCompleted: true, + usage: cumulative, + }), + ); return; } diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts index b7168b7a..8f25b6b4 100644 --- a/src/core/agents/empty-response.ts +++ b/src/core/agents/empty-response.ts @@ -10,14 +10,24 @@ export const EMPTY_RESPONSE_CONTINUATION_PROMPT = * continuation nudge can recover - from "the transport died before the turn * ended", where nudging would post into a session that is still working (or * gone) and would replace a clear diagnostic with a transport error. + * + * `usage` is what the empty turn actually cost. It is required so the tokens + * burned by a turn that never returned an `AgentResult` stay part of the + * iteration total instead of depending on whichever `onUsage` callback the + * adapter happened to fire last. */ export class EmptyAgentResponseError extends Error { readonly turnCompleted: boolean; + readonly usage: TokenUsage; - constructor(message: string, options: { turnCompleted: boolean }) { + constructor( + message: string, + options: { turnCompleted: boolean; usage: TokenUsage }, + ) { super(message); this.name = "EmptyAgentResponseError"; this.turnCompleted = options.turnCompleted; + this.usage = { ...options.usage }; } } @@ -48,15 +58,19 @@ export interface EmptyResponseRetryOptions { logEvent: string; logFields?: Record; onUsage?: OnUsage; - /** Prompt for the first turn. The continuation turn is always the bare nudge. */ + /** + * Text for the first turn. The continuation turn is always the bare nudge - + * adapters that cannot parse a bare turn apply their own existing prompt + * scaffolding inside `runTurn` so both turns are wrapped identically. + */ initialText: string; runTurn: (text: string, onTurnUsage: OnUsage) => Promise; } /** * Runs one turn and, if it completed without a final message, runs exactly one - * bare continuation turn in the same session. Usage reported to `onUsage` stays - * cumulative across both turns; any other failure propagates untouched. + * bare continuation turn. Usage reported to `onUsage` stays cumulative across + * both turns; any other failure propagates untouched. */ export async function runTurnWithEmptyResponseRetry({ logEvent, @@ -77,6 +91,9 @@ export async function runTurnWithEmptyResponseRetry({ throw error; } + firstTurnUsage = error.usage; + onUsage?.({ ...firstTurnUsage }); + appendDebugLog(logEvent, { ...logFields, attempt: 1, diff --git a/src/core/agents/opencode.ts b/src/core/agents/opencode.ts index db9ed2c9..7ad6aa16 100644 --- a/src/core/agents/opencode.ts +++ b/src/core/agents/opencode.ts @@ -859,6 +859,13 @@ export class OpenCodeAgent implements Agent { onMessage?.(trimmed); }; + // `sawSessionIdle` means the session really reported idle, so it is the + // only safe gate for an empty-response continuation. `streamTerminated` + // is the broader "stop reading" signal and is also set by provider + // errors, which must never qualify for a nudge. + let sawSessionIdle = false; + let streamTerminated = false; + const handleEvent = (event: OpenCodeStreamEvent) => { const errorInfo = extractStreamError(event, sessionId); if (errorInfo) { @@ -919,13 +926,17 @@ export class OpenCodeAgent implements Agent { return false; } - return payload?.type === "session.idle"; + if (payload?.type === "session.idle") { + sawSessionIdle = true; + return true; + } + + return false; }; const decoder = new TextDecoder(); const reader = eventResponse.body.getReader(); let buffer = ""; - let sawSessionIdle = false; const processRawEvent = (rawEvent: string) => { if (!rawEvent.trim()) return; @@ -940,7 +951,7 @@ export class OpenCodeAgent implements Agent { const event = JSON.parse(dataLines.join("\n")) as OpenCodeStreamEvent; noteEvent(event.payload?.type); if (handleEvent(event)) { - sawSessionIdle = true; + streamTerminated = true; } } catch { // Ignore malformed SSE events. @@ -968,7 +979,7 @@ export class OpenCodeAgent implements Agent { processRawEvent(buffer.slice(0, boundary)); buffer = buffer.slice(boundary + separatorLen); - if (sawSessionIdle) return; + if (streamTerminated) return; } if (flushRemainder && buffer.trim()) { @@ -979,7 +990,7 @@ export class OpenCodeAgent implements Agent { let bytesRead = 0; try { - while (!sawSessionIdle) { + while (!streamTerminated) { let readResult: ReadableStreamReadResult; try { readResult = await reader.read(); @@ -1045,6 +1056,7 @@ export class OpenCodeAgent implements Agent { elapsedMs: Date.now() - streamStartedAt, bytesRead, sawSessionIdle, + streamTerminated, telemetry: buildTelemetry(), }); @@ -1098,6 +1110,7 @@ export class OpenCodeAgent implements Agent { }); throw new EmptyAgentResponseError("OpenCode produced no final answer", { turnCompleted: sawSessionIdle, + usage: { ...usage }, }); } diff --git a/src/core/agents/pi.test.ts b/src/core/agents/pi.test.ts index 3ce1a367..6298bfb2 100644 --- a/src/core/agents/pi.test.ts +++ b/src/core/agents/pi.test.ts @@ -6,11 +6,19 @@ vi.mock("node:child_process", () => ({ spawn: vi.fn(), })); +vi.mock("../debug-log.js", () => ({ + appendDebugLog: vi.fn(), + initDebugLog: vi.fn(), + serializeError: vi.fn(), +})); + import { execFileSync, spawn } from "node:child_process"; +import { appendDebugLog } from "../debug-log.js"; import { PiAgent } from "./pi.js"; import { buildAgentOutputSchema } from "./types.js"; const mockSpawn = vi.mocked(spawn); +const mockAppendDebugLog = vi.mocked(appendDebugLog); function createMockProcess() { const proc = Object.assign(new EventEmitter(), { @@ -359,7 +367,77 @@ describe("PiAgent", () => { await expect(promise).rejects.toThrow("Invalid pi output"); }); - it("rejects empty final text", async () => { + it("re-asks once with the bare nudge inside the usual output contract when the final text is empty", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new PiAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, { + type: "message_end", + message: { + role: "assistant", + id: "msg-1", + content: " ", + usage: { input: 10, output: 5 }, + }, + }); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, { + type: "message_end", + message: { + role: "assistant", + id: "msg-2", + content: finalOutput({ summary: "recovered" }), + usage: { input: 3, output: 2 }, + }, + }); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + usage: { inputTokens: 13, outputTokens: 7 }, + }); + + const continuationPrompt = second.stdin.write.mock.calls[0]![0] as string; + expect(continuationPrompt).toContain( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + expect(continuationPrompt).toContain("gnhf final output contract"); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "pi:output:continuation", + expect.objectContaining({ attempt: 1 }), + ); + }); + + it("rejects after exactly one re-ask when the continuation is also empty", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new PiAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, { + type: "message_end", + message: { role: "assistant", content: " " }, + }); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, { + type: "message_end", + message: { role: "assistant", content: " " }, + }); + second.emit("close", 0); + + await expect(promise).rejects.toThrow("pi returned no text output"); + expect(mockSpawn).toHaveBeenCalledTimes(2); + }); + + it("does not re-ask when pi reports an error stop reason", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); const agent = new PiAgent(); @@ -367,11 +445,17 @@ describe("PiAgent", () => { const promise = agent.run("test prompt", "/work/dir"); emitJson(proc, { type: "message_end", - message: { role: "assistant", content: " " }, + message: { + role: "assistant", + content: "", + stopReason: "error", + errorMessage: "provider exploded", + }, }); proc.emit("close", 0); - await expect(promise).rejects.toThrow("pi returned no text output"); + await expect(promise).rejects.toThrow("pi reported error: provider explod"); + expect(mockSpawn).toHaveBeenCalledTimes(1); }); it("rejects malformed JSON", async () => { diff --git a/src/core/agents/pi.ts b/src/core/agents/pi.ts index 6581e713..16da7b30 100644 --- a/src/core/agents/pi.ts +++ b/src/core/agents/pi.ts @@ -1,5 +1,5 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream } from "node:fs"; +import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, parseAgentOutput, @@ -8,8 +8,15 @@ import { type AgentOutputSchema, type AgentResult, type AgentRunOptions, + type OnMessage, + type OnUsage, type TokenUsage, } from "./types.js"; +import { appendDebugLog } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; import { parseJSONLStream, setupAbortHandler, @@ -213,15 +220,48 @@ export class PiAgent implements Agent { deps.schema ?? buildAgentOutputSchema({ includeStopField: false }); } - run( + async run( prompt: string, cwd: string, options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; + const logStream = logPath ? createWriteStream(logPath) : null; + + try { + // Pi runs ephemerally (`--no-session`) and carries its output contract + // in the prompt, so the bare nudge is delivered through the same + // buildPiPrompt scaffolding rather than new schema machinery. + return await runTurnWithEmptyResponseRetry({ + logEvent: "pi:output:continuation", + onUsage, + initialText: prompt, + runTurn: (text, onTurnUsage) => + this.runTurn(text, cwd, { + onUsage: onTurnUsage, + onMessage, + signal, + logStream, + }), + }); + } finally { + logStream?.end(); + } + } + + private runTurn( + prompt: string, + cwd: string, + options: { + onUsage?: OnUsage; + onMessage?: OnMessage; + signal?: AbortSignal; + logStream: WriteStream | null; + }, + ): Promise { + const { onUsage, onMessage, signal, logStream } = options; return new Promise((resolve, reject) => { - const logStream = logPath ? createWriteStream(logPath) : null; const child = spawn(this.bin, buildPiArgs(this.extraArgs), { cwd, detached: this.platform !== "win32", @@ -358,7 +398,7 @@ export class PiAgent implements Agent { } }); - setupChildProcessHandlers(child, "pi", logStream, reject, () => { + setupChildProcessHandlers(child, "pi", reject, () => { if (latestAssistantMessage) { const stopReason = latestAssistantMessage.stopReason; if (stopReason === "error" || stopReason === "aborted") { @@ -379,7 +419,15 @@ export class PiAgent implements Agent { textByIndexToString(streamTextByIndex).trim(); if (!finalText) { - reject(new Error("pi returned no text output")); + appendDebugLog("pi:output:missing", {}); + reject( + new EmptyAgentResponseError("pi returned no text output", { + // pi's error and aborted stop reasons are already rejected + // above, so reaching here means the turn ended normally. + turnCompleted: true, + usage: lastEmittedUsage, + }), + ); return; } diff --git a/src/core/agents/rovodev.ts b/src/core/agents/rovodev.ts index 0b44b836..f5f85fda 100644 --- a/src/core/agents/rovodev.ts +++ b/src/core/agents/rovodev.ts @@ -774,6 +774,7 @@ export class RovoDevAgent implements Agent { appendDebugLog("rovodev:output:missing", { sessionId, sawClose }); throw new EmptyAgentResponseError("rovodev returned no text output", { turnCompleted: sawClose, + usage: { ...usage }, }); } diff --git a/src/core/agents/stream-utils.test.ts b/src/core/agents/stream-utils.test.ts index 81a00491..67745839 100644 --- a/src/core/agents/stream-utils.test.ts +++ b/src/core/agents/stream-utils.test.ts @@ -73,20 +73,12 @@ describe("setupChildProcessHandlers", () => { const child = createMockChild(); const reject = vi.fn(); const onSuccess = vi.fn(); - const logStream = { end: vi.fn() }; - setupChildProcessHandlers( - child as never, - "codex", - logStream as never, - reject, - onSuccess, - ); + setupChildProcessHandlers(child as never, "codex", reject, onSuccess); child.stderr.emit("data", Buffer.from("boom")); child.emit("close", 2); - expect(logStream.end).toHaveBeenCalledTimes(1); expect(onSuccess).not.toHaveBeenCalled(); expect(reject).toHaveBeenCalledWith( new Error("codex exited with code 2: boom"), @@ -97,15 +89,8 @@ describe("setupChildProcessHandlers", () => { const child = createMockChild(); const reject = vi.fn(); const onSuccess = vi.fn(); - const logStream = { end: vi.fn() }; - setupChildProcessHandlers( - child as never, - "rovodev", - logStream as never, - reject, - onSuccess, - ); + setupChildProcessHandlers(child as never, "rovodev", reject, onSuccess); child.emit("error", new Error("ENOENT")); expect(reject).toHaveBeenCalledWith( @@ -113,7 +98,6 @@ describe("setupChildProcessHandlers", () => { ); child.emit("close", 0); - expect(logStream.end).toHaveBeenCalledTimes(1); expect(onSuccess).toHaveBeenCalledTimes(1); }); }); diff --git a/src/core/agents/stream-utils.ts b/src/core/agents/stream-utils.ts index 22899469..81ad506c 100644 --- a/src/core/agents/stream-utils.ts +++ b/src/core/agents/stream-utils.ts @@ -3,14 +3,17 @@ import type { Readable } from "node:stream"; import type { WriteStream } from "node:fs"; /** - * Wire stderr collection, spawn-error handling, and the common close-handler - * prefix (logStream.end + non-zero exit code rejection) for a child process. - * Calls `onSuccess` only when the process exits with code 0. + * Wire stderr collection, spawn-error handling, and non-zero exit rejection + * for a child process. Calls `onSuccess` only when the process exits with + * code 0. + * + * The log stream is deliberately not owned here: an agent run can span more + * than one spawn (an empty-response continuation reuses the same log file), + * so the caller closes it once the whole run is done. */ export function setupChildProcessHandlers( child: ChildProcess, agentName: string, - logStream: WriteStream | null, reject: (err: Error) => void, onSuccess: () => void, ): void { @@ -25,7 +28,6 @@ export function setupChildProcessHandlers( }); child.on("close", (code) => { - logStream?.end(); if (code !== 0) { reject(new Error(`${agentName} exited with code ${code}: ${stderr}`)); return; diff --git a/src/core/config.test.ts b/src/core/config.test.ts index fe188bb9..e1828ab8 100644 --- a/src/core/config.test.ts +++ b/src/core/config.test.ts @@ -430,6 +430,19 @@ describe("loadConfig", () => { ); }); + it.each(["-r", "--resume", "--resume=abc123"])( + "throws when agentArgsOverride.claude contains reserved flag %s", + (flag) => { + mockReadFileSync.mockReturnValue( + `agentArgsOverride:\n claude:\n - ${flag}\n`, + ); + + expect(() => loadConfig()).toThrow( + /agentArgsOverride\.claude\[0\].*managed by gnhf/, + ); + }, + ); + it("reads acpRegistryOverrides from config", () => { mockReadFileSync.mockReturnValue( [ diff --git a/src/core/config.ts b/src/core/config.ts index 5613639f..099e28ec 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -118,7 +118,10 @@ function isReservedAgentArg(agent: AgentName, arg: string): boolean { arg === "--output-format" || arg.startsWith("--output-format=") || arg === "--json-schema" || - arg.startsWith("--json-schema=") + arg.startsWith("--json-schema=") || + arg === "-r" || + arg === "--resume" || + arg.startsWith("--resume=") ); case "codex": return ( From 3f1ebd105b71c61bf4e8b45a72cce6d8b79f3645 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 07:12:18 -0700 Subject: [PATCH 04/13] no-mistakes(review): resume codex/copilot sessions, exclude pi, fix log stream lifetime --- README.md | 2 +- src/core/agents/claude.ts | 20 ++-- src/core/agents/codex.test.ts | 86 ++++++++++++++- src/core/agents/codex.ts | 158 +++++++++++++++++++++++---- src/core/agents/copilot.test.ts | 16 ++- src/core/agents/copilot.ts | 57 +++++++--- src/core/agents/empty-response.ts | 18 +-- src/core/agents/pi.test.ts | 70 ++---------- src/core/agents/pi.ts | 56 ++++------ src/core/agents/stream-utils.test.ts | 86 ++++++++++++++- src/core/agents/stream-utils.ts | 71 +++++++++++- src/core/config.ts | 6 +- 12 files changed, 477 insertions(+), 169 deletions(-) diff --git a/README.md b/README.md index 91741440..200c713a 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once to continue before recording a failure, and records the nudge in the run log; session-based agents (OpenCode, Rovo Dev, ACP targets) and `claude` are nudged inside the same session, and the other CLI agents are re-asked with their usual output contract. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once inside the same session to continue before recording a failure, and records the nudge in the run log; OpenCode, Rovo Dev, and ACP targets reuse their live session, and `claude`, `codex`, and `copilot` resume theirs (`--resume`, `codex exec resume`, `--continue`). Known limitation: `pi` runs with `--no-session`, so there is no session to continue - an empty `pi` response is recorded as a failure that names this reason rather than retried, because a context-free retry could only invent a summary. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index b494d1ca..7d6a07d8 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -1,5 +1,4 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, type Agent, @@ -18,7 +17,11 @@ import { runTurnWithEmptyResponseRetry, } from "./empty-response.js"; import { shutdownChildProcess } from "./managed-process.js"; -import { parseJSONLStream, setupAbortHandler } from "./stream-utils.js"; +import { + AgentLogFile, + parseJSONLStream, + setupAbortHandler, +} from "./stream-utils.js"; const DEFAULT_FINAL_RESULT_EXIT_GRACE_MS = 15_000; /** Upper bound on the stdout tail kept for non-zero-exit error reporting. */ @@ -345,7 +348,7 @@ export class ClaudeAgent implements Agent { options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; - const logStream = logPath ? createWriteStream(logPath) : null; + const logFile = new AgentLogFile(logPath); // Populated from the first turn's stream so a continuation resumes that // exact conversation and still sees its own reasoning and tool calls. let sessionId: string | null = null; @@ -362,7 +365,7 @@ export class ClaudeAgent implements Agent { onUsage: onTurnUsage, onMessage, signal, - logStream, + logFile, resumeSessionId: sessionId, onSessionId: (id) => { sessionId = id; @@ -370,7 +373,7 @@ export class ClaudeAgent implements Agent { }), }); } finally { - logStream?.end(); + logFile.finish(); } } @@ -381,12 +384,12 @@ export class ClaudeAgent implements Agent { onUsage?: OnUsage; onMessage?: OnMessage; signal?: AbortSignal; - logStream: WriteStream | null; + logFile: AgentLogFile; resumeSessionId: string | null; onSessionId: (sessionId: string) => void; }, ): Promise { - const { onUsage, onMessage, signal, logStream } = options; + const { onUsage, onMessage, signal, logFile } = options; const { resumeSessionId, onSessionId } = options; return new Promise((resolve, reject) => { @@ -401,6 +404,7 @@ export class ClaudeAgent implements Agent { env: process.env, }, ); + logFile.track(child); if ( setupAbortHandler(signal, child, reject, () => @@ -441,7 +445,7 @@ export class ClaudeAgent implements Agent { reject(new Error(`Failed to spawn claude: ${err.message}`)); }); - parseJSONLStream(child.stdout!, logStream, (event) => { + parseJSONLStream(child.stdout!, logFile, (event) => { const eventSessionId = (event as { session_id?: unknown }).session_id; if (typeof eventSessionId === "string" && eventSessionId) { onSessionId(eventSessionId); diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 3e1a9e8f..60e59a9b 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -33,6 +33,10 @@ function emitJson(proc: ReturnType, event: unknown) { proc.stdout.emit("data", Buffer.from(`${JSON.stringify(event)}\n`)); } +function threadStarted(threadId: string) { + return { type: "thread.started", thread_id: threadId }; +} + function agentMessage(text: string) { return { type: "item.completed", @@ -241,13 +245,14 @@ describe("CodexAgent", () => { expect(proc.kill).not.toHaveBeenCalled(); }); - it("re-asks once with the bare nudge when a completed turn had no agent message", async () => { + it("resumes the recorded thread once with the bare nudge when a completed turn had no agent message", async () => { const first = createMockProcess(); const second = createMockProcess(); mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); const agent = new CodexAgent("/tmp/schema.json"); const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, threadStarted("thread-abc")); emitJson(first, turnCompleted(10, 5)); first.emit("close", 0); @@ -261,11 +266,16 @@ describe("CodexAgent", () => { usage: { inputTokens: 13, outputTokens: 7 }, }); - const continuationArgs = mockSpawn.mock.calls[1]![1] as string[]; - expect(continuationArgs).toContain( + expect(mockSpawn.mock.calls[1]![1]).toEqual([ + "exec", + "resume", + "thread-abc", "You did not produce a final answer. Continue and provide your final summary now.", - ); - expect(continuationArgs).toContain("--output-schema"); + "--json", + "--output-schema", + "/tmp/schema.json", + "--dangerously-bypass-approvals-and-sandbox", + ]); expect(mockAppendDebugLog).toHaveBeenCalledWith( "codex:output:continuation", expect.objectContaining({ attempt: 1 }), @@ -278,6 +288,7 @@ describe("CodexAgent", () => { const agent = new CodexAgent("/tmp/schema.json"); const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, threadStarted("thread-abc")); proc.emit("close", 0); await expect(promise).rejects.toThrow("codex returned no agent message"); @@ -288,6 +299,70 @@ describe("CodexAgent", () => { ); }); + it("does not re-ask when codex reported no thread id to resume", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, turnCompleted(10, 5)); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/no thread id/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + }); + + it("does not re-ask when configured codex args are unsupported by resume", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json", { + extraArgs: ["--full-auto"], + }); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, threadStarted("thread-abc")); + emitJson(proc, turnCompleted(10, 5)); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/--full-auto.*codex exec resume/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + }); + + it("forwards resume-compatible user args to the continuation", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CodexAgent("/tmp/schema.json", { + extraArgs: ["--model", "gpt-5.5"], + }); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, threadStarted("thread-abc")); + emitJson(first, turnCompleted(1, 1)); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, agentMessage(FINAL_OUTPUT)); + emitJson(second, turnCompleted(1, 1)); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { summary: "recovered" }, + }); + expect(mockSpawn.mock.calls[1]![1]).toEqual([ + "exec", + "resume", + "--model", + "gpt-5.5", + "thread-abc", + "You did not produce a final answer. Continue and provide your final summary now.", + "--json", + "--output-schema", + "/tmp/schema.json", + "--dangerously-bypass-approvals-and-sandbox", + ]); + }); + it("fails after exactly one re-ask when the continuation is also empty", async () => { const first = createMockProcess(); const second = createMockProcess(); @@ -295,6 +370,7 @@ describe("CodexAgent", () => { const agent = new CodexAgent("/tmp/schema.json"); const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, threadStarted("thread-abc")); emitJson(first, turnCompleted(10, 5)); first.emit("close", 0); diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index bf5e25b7..97aa23cf 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -1,5 +1,4 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream, type WriteStream } from "node:fs"; import type { Agent, AgentResult, @@ -15,6 +14,7 @@ import { runTurnWithEmptyResponseRetry, } from "./empty-response.js"; import { + AgentLogFile, parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, @@ -34,7 +34,52 @@ interface CodexTurnCompleted { }; } -type CodexEvent = CodexItemCompleted | CodexTurnCompleted | { type: string }; +interface CodexThreadStarted { + type: "thread.started"; + thread_id?: string; + threadId?: string; + id?: string; +} + +type CodexEvent = + | CodexItemCompleted + | CodexTurnCompleted + | CodexThreadStarted + | { type: string }; + +function threadIdOf(event: CodexThreadStarted): string | null { + const candidate = event.thread_id ?? event.threadId ?? event.id; + return typeof candidate === "string" && candidate ? candidate : null; +} + +// `codex exec resume` is a narrower subcommand than `codex exec`: it rejects +// the execution-mode flags below outright. Rather than silently downgrading a +// user's sandbox choice or shelling out a command codex will refuse, gnhf +// skips the empty-response continuation for these configurations. +const CODEX_RESUME_UNSUPPORTED_ARGS = [ + "--full-auto", + "-s", + "--sandbox", + "-a", + "--ask-for-approval", + "--approve-for-me", + "--oss", + "--local-provider", + "-p", + "--profile", + "--color", +]; + +function codexResumeUnsupportedArg(extraArgs?: string[]): string | null { + return ( + (extraArgs ?? []).find((arg) => + CODEX_RESUME_UNSUPPORTED_ARGS.some( + (unsupported) => + arg === unsupported || arg.startsWith(`${unsupported}=`), + ), + ) ?? null + ); +} interface CodexAgentDeps { bin?: string; @@ -91,13 +136,8 @@ function terminateCodexProcess( child.kill("SIGTERM"); } -function buildCodexArgs( - prompt: string, - schemaPath: string, - extraArgs?: string[], -): string[] { - const userArgs = extraArgs ?? []; - const userSpecifiedExecutionMode = userArgs.some( +function userSpecifiedExecutionMode(userArgs: string[]): boolean { + return userArgs.some( (arg) => arg === "--full-auto" || arg === "--dangerously-bypass-approvals-and-sandbox" || @@ -108,6 +148,14 @@ function buildCodexArgs( arg.startsWith("--ask-for-approval=") || arg === "-a", ); +} + +function buildCodexArgs( + prompt: string, + schemaPath: string, + extraArgs?: string[], +): string[] { + const userArgs = extraArgs ?? []; return [ "exec", @@ -116,7 +164,7 @@ function buildCodexArgs( "--json", "--output-schema", schemaPath, - ...(userSpecifiedExecutionMode + ...(userSpecifiedExecutionMode(userArgs) ? [] : ["--dangerously-bypass-approvals-and-sandbox"]), "--color", @@ -124,6 +172,32 @@ function buildCodexArgs( ]; } +// `codex exec resume ` replays the recorded session, so the +// continuation turn keeps the first turn's reasoning and tool calls. It does +// not accept `--color`, so that flag is dropped here rather than forwarded. +function buildCodexResumeArgs( + prompt: string, + schemaPath: string, + threadId: string, + extraArgs?: string[], +): string[] { + const userArgs = extraArgs ?? []; + + return [ + "exec", + "resume", + ...userArgs, + threadId, + prompt, + "--json", + "--output-schema", + schemaPath, + ...(userSpecifiedExecutionMode(userArgs) + ? [] + : ["--dangerously-bypass-approvals-and-sandbox"]), + ]; +} + export class CodexAgent implements Agent { name = "codex"; @@ -146,7 +220,8 @@ export class CodexAgent implements Agent { options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; - const logStream = logPath ? createWriteStream(logPath) : null; + const logFile = new AgentLogFile(logPath); + let threadId: string | null = null; try { // `--output-schema` is a spawn flag, so the continuation turn carries the @@ -160,11 +235,15 @@ export class CodexAgent implements Agent { onUsage: onTurnUsage, onMessage, signal, - logStream, + logFile, + resumeThreadId: threadId, + onThreadId: (id) => { + threadId = id; + }, }), }); } finally { - logStream?.end(); + logFile.finish(); } } @@ -175,15 +254,25 @@ export class CodexAgent implements Agent { onUsage?: OnUsage; onMessage?: OnMessage; signal?: AbortSignal; - logStream: WriteStream | null; + logFile: AgentLogFile; + resumeThreadId: string | null; + onThreadId: (threadId: string) => void; }, ): Promise { - const { onUsage, onMessage, signal, logStream } = options; + const { onUsage, onMessage, signal, logFile } = options; + const { resumeThreadId, onThreadId } = options; return new Promise((resolve, reject) => { const child = spawn( this.bin, - buildCodexArgs(prompt, this.schemaPath, this.extraArgs), + resumeThreadId + ? buildCodexResumeArgs( + prompt, + this.schemaPath, + resumeThreadId, + this.extraArgs, + ) + : buildCodexArgs(prompt, this.schemaPath, this.extraArgs), { cwd, shell: shouldUseWindowsShell(this.bin, this.platform), @@ -191,6 +280,7 @@ export class CodexAgent implements Agent { env: process.env, }, ); + logFile.track(child); if ( setupAbortHandler(signal, child, reject, () => @@ -205,6 +295,7 @@ export class CodexAgent implements Agent { // clean process exit - is what separates a finished-but-silent turn // from a turn that never got to answer. let sawTurnCompleted = false; + let turnThreadId: string | null = resumeThreadId; const cumulative: TokenUsage = { inputTokens: 0, outputTokens: 0, @@ -212,7 +303,15 @@ export class CodexAgent implements Agent { cacheCreationTokens: 0, }; - parseJSONLStream(child.stdout!, logStream, (event) => { + parseJSONLStream(child.stdout!, logFile, (event) => { + if (event.type === "thread.started") { + const id = threadIdOf(event as CodexThreadStarted); + if (id) { + turnThreadId = id; + onThreadId(id); + } + } + if ( event.type === "item.completed" && "item" in event && @@ -236,12 +335,27 @@ export class CodexAgent implements Agent { setupChildProcessHandlers(child, "codex", reject, () => { if (!lastAgentMessage) { - appendDebugLog("codex:output:missing", { sawTurnCompleted }); + const unsupportedArg = codexResumeUnsupportedArg(this.extraArgs); + const resumeBlockedReason = !turnThreadId + ? "codex reported no thread id, so the turn cannot be resumed" + : unsupportedArg + ? `configured codex arg "${unsupportedArg}" is not supported by \`codex exec resume\`, so the turn cannot be resumed` + : null; + appendDebugLog("codex:output:missing", { + sawTurnCompleted, + hasThreadId: turnThreadId !== null, + resumeBlockedReason, + }); reject( - new EmptyAgentResponseError("codex returned no agent message", { - turnCompleted: sawTurnCompleted, - usage: cumulative, - }), + new EmptyAgentResponseError( + resumeBlockedReason + ? `codex returned no agent message (${resumeBlockedReason})` + : "codex returned no agent message", + { + turnCompleted: sawTurnCompleted && resumeBlockedReason === null, + usage: cumulative, + }, + ), ); return; } diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index 62c8a66a..32d5cb15 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -272,7 +272,7 @@ describe("CopilotAgent", () => { expect(args[1]).toContain("should_fully_stop"); }); - it("re-asks once with the bare nudge inside the usual output contract when the turn had no assistant message", async () => { + it("continues the previous session once with the bare nudge when the turn had no assistant message", async () => { const first = createMockProcess(); const second = createMockProcess(); mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); @@ -305,11 +305,17 @@ describe("CopilotAgent", () => { usage: { outputTokens: 8 }, }); - const continuationPrompt = (mockSpawn.mock.calls[1]![1] as string[])[1]!; - expect(continuationPrompt).toContain( + expect(mockSpawn.mock.calls[1]![1]).toEqual([ + "--continue", + "-p", "You did not produce a final answer. Continue and provide your final summary now.", - ); - expect(continuationPrompt).toContain("gnhf final output contract"); + "--output-format", + "json", + "--stream", + "off", + "--no-color", + "--allow-all", + ]); expect(mockAppendDebugLog).toHaveBeenCalledWith( "copilot:output:continuation", expect.objectContaining({ attempt: 1 }), diff --git a/src/core/agents/copilot.ts b/src/core/agents/copilot.ts index 73c413e1..e5d02a88 100644 --- a/src/core/agents/copilot.ts +++ b/src/core/agents/copilot.ts @@ -1,5 +1,4 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, parseAgentOutput, @@ -17,6 +16,7 @@ import { runTurnWithEmptyResponseRetry, } from "./empty-response.js"; import { + AgentLogFile, parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, @@ -151,6 +151,29 @@ function buildCopilotArgs( ]; } +// `--continue` resumes the most recently closed local session, so the +// continuation turn still sees the first turn's reasoning, tool calls, and +// the output contract it was already given - the nudge itself stays bare. +function buildCopilotContinueArgs( + prompt: string, + extraArgs?: string[], +): string[] { + const userArgs = extraArgs ?? []; + + return [ + ...userArgs, + "--continue", + "-p", + prompt, + "--output-format", + "json", + "--stream", + "off", + "--no-color", + ...(userSpecifiedPermissionMode(userArgs) ? [] : ["--allow-all"]), + ]; +} + function numberField( usage: Record, names: string[], @@ -220,26 +243,28 @@ export class CopilotAgent implements Agent { options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; - const logStream = logPath ? createWriteStream(logPath) : null; + const logFile = new AgentLogFile(logPath); + let turnsStarted = 0; try { - // Copilot carries its output contract in the prompt text, so the bare - // nudge goes through the same buildCopilotPrompt scaffolding the first - // turn used - no continuation-specific schema machinery. return await runTurnWithEmptyResponseRetry({ logEvent: "copilot:output:continuation", onUsage, initialText: prompt, - runTurn: (text, onTurnUsage) => - this.runTurn(text, cwd, { + runTurn: (text, onTurnUsage) => { + const continuation = turnsStarted > 0; + turnsStarted += 1; + return this.runTurn(text, cwd, { onUsage: onTurnUsage, onMessage, signal, - logStream, - }), + logFile, + continuation, + }); + }, }); } finally { - logStream?.end(); + logFile.finish(); } } @@ -250,15 +275,18 @@ export class CopilotAgent implements Agent { onUsage?: OnUsage; onMessage?: OnMessage; signal?: AbortSignal; - logStream: WriteStream | null; + logFile: AgentLogFile; + continuation: boolean; }, ): Promise { - const { onUsage, onMessage, signal, logStream } = options; + const { onUsage, onMessage, signal, logFile, continuation } = options; return new Promise((resolve, reject) => { const child = spawn( this.bin, - buildCopilotArgs(prompt, this.schema, this.extraArgs), + continuation + ? buildCopilotContinueArgs(prompt, this.extraArgs) + : buildCopilotArgs(prompt, this.schema, this.extraArgs), { cwd, shell: shouldUseWindowsShell(this.bin, this.platform), @@ -266,6 +294,7 @@ export class CopilotAgent implements Agent { env: process.env, }, ); + logFile.track(child); if ( setupAbortHandler(signal, child, reject, () => @@ -283,7 +312,7 @@ export class CopilotAgent implements Agent { cacheCreationTokens: 0, }; - parseJSONLStream(child.stdout!, logStream, (event) => { + parseJSONLStream(child.stdout!, logFile, (event) => { if (event.type === "assistant.message") { const data = (event as CopilotAssistantMessageEvent).data; if (typeof data.content === "string") { diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts index 8f25b6b4..04aee393 100644 --- a/src/core/agents/empty-response.ts +++ b/src/core/agents/empty-response.ts @@ -31,15 +31,6 @@ export class EmptyAgentResponseError extends Error { } } -export function emptyTokenUsage(): TokenUsage { - return { - inputTokens: 0, - outputTokens: 0, - cacheReadTokens: 0, - cacheCreationTokens: 0, - }; -} - export function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { const total: TokenUsage = { inputTokens: left.inputTokens + right.inputTokens, @@ -79,19 +70,14 @@ export async function runTurnWithEmptyResponseRetry({ initialText, runTurn, }: EmptyResponseRetryOptions): Promise { - let firstTurnUsage = emptyTokenUsage(); - try { - return await runTurn(initialText, (usage) => { - firstTurnUsage = { ...usage }; - onUsage?.(usage); - }); + return await runTurn(initialText, (usage) => onUsage?.(usage)); } catch (error) { if (!(error instanceof EmptyAgentResponseError) || !error.turnCompleted) { throw error; } - firstTurnUsage = error.usage; + const firstTurnUsage = error.usage; onUsage?.({ ...firstTurnUsage }); appendDebugLog(logEvent, { diff --git a/src/core/agents/pi.test.ts b/src/core/agents/pi.test.ts index 6298bfb2..15e87560 100644 --- a/src/core/agents/pi.test.ts +++ b/src/core/agents/pi.test.ts @@ -367,76 +367,28 @@ describe("PiAgent", () => { await expect(promise).rejects.toThrow("Invalid pi output"); }); - it("re-asks once with the bare nudge inside the usual output contract when the final text is empty", async () => { - const first = createMockProcess(); - const second = createMockProcess(); - mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + it("fails an empty response without retrying and names --no-session as the reason", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); const agent = new PiAgent(); const promise = agent.run("test prompt", "/work/dir"); - emitJson(first, { - type: "message_end", - message: { - role: "assistant", - id: "msg-1", - content: " ", - usage: { input: 10, output: 5 }, - }, - }); - first.emit("close", 0); - - await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); - emitJson(second, { + emitJson(proc, { type: "message_end", - message: { - role: "assistant", - id: "msg-2", - content: finalOutput({ summary: "recovered" }), - usage: { input: 3, output: 2 }, - }, - }); - second.emit("close", 0); - - await expect(promise).resolves.toMatchObject({ - output: { success: true, summary: "recovered" }, - usage: { inputTokens: 13, outputTokens: 7 }, + message: { role: "assistant", content: " " }, }); + proc.emit("close", 0); - const continuationPrompt = second.stdin.write.mock.calls[0]![0] as string; - expect(continuationPrompt).toContain( - "You did not produce a final answer. Continue and provide your final summary now.", + await expect(promise).rejects.toThrow( + /pi returned no text output.*--no-session/, ); - expect(continuationPrompt).toContain("gnhf final output contract"); - expect(mockAppendDebugLog).toHaveBeenCalledWith( + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( "pi:output:continuation", - expect.objectContaining({ attempt: 1 }), + expect.anything(), ); }); - it("rejects after exactly one re-ask when the continuation is also empty", async () => { - const first = createMockProcess(); - const second = createMockProcess(); - mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); - const agent = new PiAgent(); - - const promise = agent.run("test prompt", "/work/dir"); - emitJson(first, { - type: "message_end", - message: { role: "assistant", content: " " }, - }); - first.emit("close", 0); - - await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); - emitJson(second, { - type: "message_end", - message: { role: "assistant", content: " " }, - }); - second.emit("close", 0); - - await expect(promise).rejects.toThrow("pi returned no text output"); - expect(mockSpawn).toHaveBeenCalledTimes(2); - }); - it("does not re-ask when pi reports an error stop reason", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/pi.ts b/src/core/agents/pi.ts index 16da7b30..6a2b27b7 100644 --- a/src/core/agents/pi.ts +++ b/src/core/agents/pi.ts @@ -1,5 +1,4 @@ import { execFileSync, spawn } from "node:child_process"; -import { createWriteStream, type WriteStream } from "node:fs"; import { buildAgentOutputSchema, parseAgentOutput, @@ -14,15 +13,20 @@ import { } from "./types.js"; import { appendDebugLog } from "../debug-log.js"; import { - EmptyAgentResponseError, - runTurnWithEmptyResponseRetry, -} from "./empty-response.js"; -import { + AgentLogFile, parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, } from "./stream-utils.js"; +// gnhf runs pi with `--no-session`, so a turn leaves nothing behind to +// continue. A second spawn would be a fresh agent with no knowledge of the +// task, the workspace edits, or the notes - it could only invent a final +// summary - so pi is deliberately excluded from empty-response recovery and +// reports the reason instead. +const PI_EMPTY_RESPONSE_MESSAGE = + "pi returned no text output (gnhf runs pi with --no-session, so there is no session to continue and the empty response cannot be recovered)"; + interface PiAgentDeps { bin?: string; extraArgs?: string[]; @@ -226,26 +230,17 @@ export class PiAgent implements Agent { options?: AgentRunOptions, ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; - const logStream = logPath ? createWriteStream(logPath) : null; + const logFile = new AgentLogFile(logPath); try { - // Pi runs ephemerally (`--no-session`) and carries its output contract - // in the prompt, so the bare nudge is delivered through the same - // buildPiPrompt scaffolding rather than new schema machinery. - return await runTurnWithEmptyResponseRetry({ - logEvent: "pi:output:continuation", + return await this.runTurn(prompt, cwd, { onUsage, - initialText: prompt, - runTurn: (text, onTurnUsage) => - this.runTurn(text, cwd, { - onUsage: onTurnUsage, - onMessage, - signal, - logStream, - }), + onMessage, + signal, + logFile, }); } finally { - logStream?.end(); + logFile.finish(); } } @@ -256,10 +251,10 @@ export class PiAgent implements Agent { onUsage?: OnUsage; onMessage?: OnMessage; signal?: AbortSignal; - logStream: WriteStream | null; + logFile: AgentLogFile; }, ): Promise { - const { onUsage, onMessage, signal, logStream } = options; + const { onUsage, onMessage, signal, logFile } = options; return new Promise((resolve, reject) => { const child = spawn(this.bin, buildPiArgs(this.extraArgs), { @@ -269,6 +264,7 @@ export class PiAgent implements Agent { stdio: ["pipe", "pipe", "pipe"], env: process.env, }); + logFile.track(child); child.stdin?.write(buildPiPrompt(prompt, this.schema)); child.stdin?.end(); @@ -339,7 +335,7 @@ export class PiAgent implements Agent { updateUsage(message, streaming); }; - parseJSONLStream(child.stdout!, logStream, (event) => { + parseJSONLStream(child.stdout!, logFile, (event) => { if (!isRecord(event)) return; if (event.type === "message_update") { @@ -419,15 +415,11 @@ export class PiAgent implements Agent { textByIndexToString(streamTextByIndex).trim(); if (!finalText) { - appendDebugLog("pi:output:missing", {}); - reject( - new EmptyAgentResponseError("pi returned no text output", { - // pi's error and aborted stop reasons are already rejected - // above, so reaching here means the turn ended normally. - turnCompleted: true, - usage: lastEmittedUsage, - }), - ); + appendDebugLog("pi:output:missing", { + recoverable: false, + reason: "pi runs with --no-session", + }); + reject(new Error(PI_EMPTY_RESPONSE_MESSAGE)); return; } diff --git a/src/core/agents/stream-utils.test.ts b/src/core/agents/stream-utils.test.ts index 67745839..b4369eb7 100644 --- a/src/core/agents/stream-utils.test.ts +++ b/src/core/agents/stream-utils.test.ts @@ -1,6 +1,10 @@ import { EventEmitter } from "node:events"; -import { describe, it, expect, vi } from "vitest"; +import { mkdtempSync, readFileSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, it, expect, vi } from "vitest"; import { + AgentLogFile, parseJSONLStream, setupAbortHandler, setupChildProcessHandlers, @@ -20,6 +24,86 @@ function createMockReadable() { return new EventEmitter(); } +describe("AgentLogFile", () => { + const tempDirs: string[] = []; + + function tempLogPath() { + const dir = mkdtempSync(join(tmpdir(), "gnhf-log-")); + tempDirs.push(dir); + return join(dir, "iteration.jsonl"); + } + + afterEach(() => { + while (tempDirs.length > 0) { + rmSync(tempDirs.pop()!, { recursive: true, force: true }); + } + }); + + function flush(): Promise { + return new Promise((resolve) => setTimeout(resolve, 20)); + } + + it("keeps recording stdout that arrives after an aborted run finished", async () => { + const logPath = tempLogPath(); + const logFile = new AgentLogFile(logPath); + const child = createMockChild(); + const stdout = createMockReadable(); + logFile.track(child as never); + parseJSONLStream(stdout as never, logFile, () => {}); + + stdout.emit("data", Buffer.from('{"type":"before-abort"}\n')); + // The abort path rejects the run while the child is still streaming. + logFile.finish(); + stdout.emit("data", Buffer.from('{"type":"after-abort"}\n')); + child.emit("close", 143); + await flush(); + + const written = readFileSync(logPath, "utf8"); + expect(written).toContain("before-abort"); + expect(written).toContain("after-abort"); + }); + + it("stays open across a second spawn and closes once the last child exits", async () => { + const logPath = tempLogPath(); + const logFile = new AgentLogFile(logPath); + const first = createMockChild(); + logFile.track(first as never); + logFile.write('{"turn":1}\n'); + first.emit("close", 0); + + const second = createMockChild(); + logFile.track(second as never); + logFile.write('{"turn":2}\n'); + logFile.finish(); + second.emit("close", 0); + await flush(); + + const written = readFileSync(logPath, "utf8"); + expect(written).toContain('{"turn":1}'); + expect(written).toContain('{"turn":2}'); + + // A late stdout chunk after the file closed must not raise + // ERR_STREAM_WRITE_AFTER_END. + expect(() => logFile.write('{"turn":"late"}\n')).not.toThrow(); + await flush(); + expect(readFileSync(logPath, "utf8")).not.toContain("late"); + }); + + it("closes a run whose child failed to spawn", async () => { + const logPath = tempLogPath(); + const logFile = new AgentLogFile(logPath); + const child = createMockChild(); + logFile.track(child as never); + logFile.write('{"spawn":"failed"}\n'); + + child.emit("error", new Error("ENOENT")); + logFile.finish(); + await flush(); + + expect(readFileSync(logPath, "utf8")).toContain('{"spawn":"failed"}'); + }); +}); + describe("parseJSONLStream", () => { it("parses complete JSONL events across chunk boundaries and writes chunks to the log", () => { const stream = createMockReadable(); diff --git a/src/core/agents/stream-utils.ts b/src/core/agents/stream-utils.ts index 81ad506c..4bbdce98 100644 --- a/src/core/agents/stream-utils.ts +++ b/src/core/agents/stream-utils.ts @@ -1,15 +1,76 @@ import type { ChildProcess } from "node:child_process"; import type { Readable } from "node:stream"; -import type { WriteStream } from "node:fs"; +import { createWriteStream } from "node:fs"; +import { appendDebugLog, serializeError } from "../debug-log.js"; + +/** Sink for raw agent stdout. */ +export interface AgentLogSink { + write(chunk: string | Buffer): void; +} + +/** + * Per-run log file for adapters that spawn a CLI once per turn. + * + * An agent run can span more than one spawn (an empty-response continuation + * reuses the same log file), and a turn can be rejected - by an abort, say - + * while its child is still streaming stdout. Ending the file on either event + * alone would truncate a later turn or write after end, so the file is closed + * only once the run has finished *and* every child it spawned has exited. + */ +export class AgentLogFile implements AgentLogSink { + private readonly stream: ReturnType | null; + private openChildren = 0; + private runFinished = false; + private ended = false; + + constructor(logPath?: string) { + this.stream = logPath ? createWriteStream(logPath) : null; + // Log writes are best effort; a failed write must not take down the run. + this.stream?.on("error", (error) => { + appendDebugLog("agent:log:write-failed", { + error: serializeError(error), + }); + }); + } + + write(chunk: string | Buffer): void { + if (this.ended) return; + this.stream?.write(chunk); + } + + /** Keep the file open until `child` has exited. */ + track(child: ChildProcess): void { + this.openChildren += 1; + let settled = false; + const onChildGone = () => { + if (settled) return; + settled = true; + this.openChildren -= 1; + this.endIfIdle(); + }; + child.on("close", onChildGone); + child.on("error", onChildGone); + } + + /** The run is done; close as soon as no tracked child is still running. */ + finish(): void { + this.runFinished = true; + this.endIfIdle(); + } + + private endIfIdle(): void { + if (this.ended || !this.runFinished || this.openChildren > 0) return; + this.ended = true; + this.stream?.end(); + } +} /** * Wire stderr collection, spawn-error handling, and non-zero exit rejection * for a child process. Calls `onSuccess` only when the process exits with * code 0. * - * The log stream is deliberately not owned here: an agent run can span more - * than one spawn (an empty-response continuation reuses the same log file), - * so the caller closes it once the whole run is done. + * The log file is deliberately not owned here - see `AgentLogFile`. */ export function setupChildProcessHandlers( child: ChildProcess, @@ -42,7 +103,7 @@ export function setupChildProcessHandlers( */ export function parseJSONLStream( stream: Readable, - logStream: WriteStream | null, + logStream: AgentLogSink | null, callback: (event: T) => void, ): void { let buffer = ""; diff --git a/src/core/config.ts b/src/core/config.ts index 099e28ec..167bcc30 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -126,6 +126,7 @@ function isReservedAgentArg(agent: AgentName, arg: string): boolean { case "codex": return ( arg === "exec" || + arg === "resume" || arg === "--json" || arg === "--output-schema" || arg.startsWith("--output-schema=") || @@ -164,7 +165,10 @@ function isReservedAgentArg(agent: AgentName, arg: string): boolean { arg === "--no-color" || arg === "--share" || arg.startsWith("--share=") || - arg === "--share-gist" + arg === "--share-gist" || + arg === "--continue" || + arg === "--resume" || + arg.startsWith("--resume=") ); case "pi": return ( From c670575b2f5825b8af228a1a950a7b7da2d4006b Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 07:29:58 -0700 Subject: [PATCH 05/13] no-mistakes(review): require session identity for copilot/claude nudge; complete codex denylist --- README.md | 2 +- src/core/agents/claude.test.ts | 56 ++++++++++++++++++++ src/core/agents/claude.ts | 22 +++++++- src/core/agents/codex.test.ts | 44 +++++++++------ src/core/agents/codex.ts | 22 +++++--- src/core/agents/copilot.test.ts | 27 +++++++++- src/core/agents/copilot.ts | 94 ++++++++++++++++++++++++--------- 7 files changed, 215 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index 200c713a..e2930986 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once inside the same session to continue before recording a failure, and records the nudge in the run log; OpenCode, Rovo Dev, and ACP targets reuse their live session, and `claude`, `codex`, and `copilot` resume theirs (`--resume`, `codex exec resume`, `--continue`). Known limitation: `pi` runs with `--no-session`, so there is no session to continue - an empty `pi` response is recorded as a failure that names this reason rather than retried, because a context-free retry could only invent a summary. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once inside the same session to continue before recording a failure, and records the nudge in the run log. OpenCode, Rovo Dev, and ACP targets reuse their live session; `claude`, `codex`, and `copilot` resume the exact session the empty turn used, and skip the nudge when that session cannot be identified or resumed (no session id reported, or agent args such as `--no-session-persistence` or ones `codex exec resume` rejects); `pi` is excluded because gnhf runs it with `--no-session`. Whenever the nudge is skipped the failure names the reason, because a retry that cannot reach the original session could only invent a summary. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/claude.test.ts b/src/core/agents/claude.test.ts index 383cfe57..1362af48 100644 --- a/src/core/agents/claude.test.ts +++ b/src/core/agents/claude.test.ts @@ -1282,6 +1282,62 @@ describe("ClaudeAgent", () => { expect(mockSpawn).toHaveBeenCalledTimes(2); }); + it("does not continue when --no-session-persistence makes the session unresumable", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const noPersistenceAgent = new ClaudeAgent({ + extraArgs: ["--no-session-persistence"], + }); + + const promise = noPersistenceAgent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "result", + subtype: "success", + is_error: false, + session_id: "session-abc", + usage: { + input_tokens: 0, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 0, + }, + structured_output: null, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/--no-session-persistence/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "claude:output:continuation", + expect.anything(), + ); + }); + + it("does not continue when claude reported no session id", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + const promise = agent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "result", + subtype: "success", + is_error: false, + usage: { + input_tokens: 0, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 0, + }, + structured_output: null, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/no session id/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + }); + it("does not continue when claude reported an error result", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 7d6a07d8..4b3ba0be 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -155,6 +155,12 @@ function userSpecifiedSessionContinuation(userArgs: string[]): boolean { return userArgs.some((arg) => arg === "-c" || arg === "--continue"); } +// `--no-session-persistence` tells claude not to write the session to disk, so +// there is nothing for `--resume` to reopen. +function sessionPersistenceDisabled(userArgs: string[]): boolean { + return userArgs.some((arg) => arg === "--no-session-persistence"); +} + function buildClaudeArgs( prompt: string, schema: AgentOutputSchema, @@ -415,6 +421,7 @@ export class ClaudeAgent implements Agent { } let resultEvent: ClaudeResultEvent | null = null; + let turnSessionId: string | null = resumeSessionId; let finalStructuredResultEvent: ClaudeResultEvent | null = null; let latestResultUsage: ClaudeResultEvent["usage"] | null = null; let finalResultCleanupTimer: ReturnType | null = null; @@ -448,6 +455,7 @@ export class ClaudeAgent implements Agent { parseJSONLStream(child.stdout!, logFile, (event) => { const eventSessionId = (event as { session_id?: unknown }).session_id; if (typeof eventSessionId === "string" && eventSessionId) { + turnSessionId = eventSessionId; onSessionId(eventSessionId); } @@ -600,17 +608,27 @@ export class ClaudeAgent implements Agent { } if (!terminalResultEvent.structured_output) { + const userArgs = this.extraArgs ?? []; + const resumeBlockedReason = sessionPersistenceDisabled(userArgs) + ? "--no-session-persistence disables session resume, so the turn cannot be continued" + : !turnSessionId && !userSpecifiedSessionContinuation(userArgs) + ? "claude reported no session id, so the turn cannot be resumed" + : null; appendDebugLog("claude:output:missing", { subtype: terminalResultEvent.subtype, resumed: resumeSessionId !== null, + hasSessionId: turnSessionId !== null, + resumeBlockedReason, }); reject( new EmptyAgentResponseError( - "claude returned no structured_output", + resumeBlockedReason + ? `claude returned no structured_output (${resumeBlockedReason})` + : "claude returned no structured_output", { // A non-error `result` event with subtype "success" is claude's // own end-of-turn signal; it just carried no final answer. - turnCompleted: true, + turnCompleted: resumeBlockedReason === null, usage: toTokenUsage( latestResultUsage ?? terminalResultEvent.usage, ), diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 60e59a9b..347a3b2b 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -312,21 +312,35 @@ describe("CodexAgent", () => { expect(mockSpawn).toHaveBeenCalledTimes(1); }); - it("does not re-ask when configured codex args are unsupported by resume", async () => { - const proc = createMockProcess(); - mockSpawn.mockReturnValue(proc); - const agent = new CodexAgent("/tmp/schema.json", { - extraArgs: ["--full-auto"], - }); - - const promise = agent.run("test prompt", "/work/dir"); - emitJson(proc, threadStarted("thread-abc")); - emitJson(proc, turnCompleted(10, 5)); - proc.emit("close", 0); - - await expect(promise).rejects.toThrow(/--full-auto.*codex exec resume/); - expect(mockSpawn).toHaveBeenCalledTimes(1); - }); + // Each of these is accepted by `codex exec` but rejected by + // `codex exec resume`, so forwarding it would replace the accurate + // empty-response diagnostic with a codex CLI usage error. + it.each([ + ["--add-dir", "/shared"], + ["-C", "/shared"], + ["--cd", "/shared"], + ["--sandbox", "workspace-write"], + ["--full-auto"], + ["--oss"], + ["--profile", "work"], + ])( + "does not re-ask when configured codex args include %s", + async (...extraArgs) => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json", { extraArgs }); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, threadStarted("thread-abc")); + emitJson(proc, turnCompleted(10, 5)); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow( + new RegExp(`${extraArgs[0]!.replace("-", "\\-")}.*codex exec resume`), + ); + expect(mockSpawn).toHaveBeenCalledTimes(1); + }, + ); it("forwards resume-compatible user args to the continuation", async () => { const first = createMockProcess(); diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index 97aa23cf..17a6933d 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -52,22 +52,28 @@ function threadIdOf(event: CodexThreadStarted): string | null { return typeof candidate === "string" && candidate ? candidate : null; } -// `codex exec resume` is a narrower subcommand than `codex exec`: it rejects -// the execution-mode flags below outright. Rather than silently downgrading a -// user's sandbox choice or shelling out a command codex will refuse, gnhf -// skips the empty-response continuation for these configurations. +// `codex exec resume` is a narrower subcommand than `codex exec` and clap +// rejects anything it does not declare. Rather than silently downgrading a +// user's sandbox choice or shelling out a command codex will refuse - which +// would replace the accurate empty-response diagnostic with a CLI usage error +// - gnhf skips the empty-response continuation for these configurations. +// Verified against codex-cli 0.147.0 by diffing `codex exec --help` against +// `codex exec resume --help`; the approval flags are kept because older codex +// releases still accept them on `codex exec` and resume rejects them too. const CODEX_RESUME_UNSUPPORTED_ARGS = [ - "--full-auto", + "--add-dir", + "-C", + "--cd", "-s", "--sandbox", - "-a", - "--ask-for-approval", "--approve-for-me", "--oss", "--local-provider", "-p", "--profile", - "--color", + "--full-auto", + "-a", + "--ask-for-approval", ]; function codexResumeUnsupportedArg(extraArgs?: string[]): string | null { diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index 32d5cb15..1dfe3c53 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -272,13 +272,17 @@ describe("CopilotAgent", () => { expect(args[1]).toContain("should_fully_stop"); }); - it("continues the previous session once with the bare nudge when the turn had no assistant message", async () => { + it("resumes the reported session once with the bare nudge when the turn had no assistant message", async () => { const first = createMockProcess(); const second = createMockProcess(); mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); const agent = new CopilotAgent(); const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, { + type: "session.started", + data: { session_id: "session-abc" }, + }); emitJson(first, { type: "assistant.message", data: { outputTokens: 5 }, @@ -306,7 +310,8 @@ describe("CopilotAgent", () => { }); expect(mockSpawn.mock.calls[1]![1]).toEqual([ - "--continue", + "--resume", + "session-abc", "-p", "You did not produce a final answer. Continue and provide your final summary now.", "--output-format", @@ -322,6 +327,23 @@ describe("CopilotAgent", () => { ); }); + it("does not re-ask when copilot reported no session id to resume", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CopilotAgent(); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, { type: "assistant.message", data: { outputTokens: 5 } }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/no session id/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "copilot:output:continuation", + expect.anything(), + ); + }); + it("rejects after exactly one re-ask when copilot still returns no assistant message", async () => { const first = createMockProcess(); const second = createMockProcess(); @@ -329,6 +351,7 @@ describe("CopilotAgent", () => { const agent = new CopilotAgent(); const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, { type: "session.started", session_id: "session-abc" }); first.emit("close", 0); await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); diff --git a/src/core/agents/copilot.ts b/src/core/agents/copilot.ts index e5d02a88..213b7b10 100644 --- a/src/core/agents/copilot.ts +++ b/src/core/agents/copilot.ts @@ -38,6 +38,30 @@ type CopilotEvent = | CopilotAssistantMessageEvent | (CopilotUsageEvent & { type: string }); +const COPILOT_SESSION_ID_KEYS = ["session_id", "sessionId"]; + +// Copilot's `--continue` picks a session by recency, which is not the same +// thing as picking the turn that just went silent. An empty turn is exactly +// the case where copilot may not have persisted a session at all, so the +// continuation is only safe when copilot itself named a session. Nothing in +// the JSONL contract guarantees that field, so when it is absent gnhf skips +// recovery rather than nudging a session it cannot identify. +function copilotSessionIdOf(event: unknown): string | null { + if (typeof event !== "object" || event === null) return null; + const record = event as Record; + const containers: unknown[] = [record, record.data, record.session]; + + for (const container of containers) { + if (typeof container !== "object" || container === null) continue; + for (const key of COPILOT_SESSION_ID_KEYS) { + const value = (container as Record)[key]; + if (typeof value === "string" && value) return value; + } + } + + return null; +} + interface CopilotAgentDeps { bin?: string; extraArgs?: string[]; @@ -151,18 +175,20 @@ function buildCopilotArgs( ]; } -// `--continue` resumes the most recently closed local session, so the -// continuation turn still sees the first turn's reasoning, tool calls, and -// the output contract it was already given - the nudge itself stays bare. -function buildCopilotContinueArgs( +// `--resume ` reopens that exact session, so the continuation turn +// still sees the first turn's reasoning, tool calls, and the output contract it +// was already given - the nudge itself stays bare. +function buildCopilotResumeArgs( prompt: string, + sessionId: string, extraArgs?: string[], ): string[] { const userArgs = extraArgs ?? []; return [ ...userArgs, - "--continue", + "--resume", + sessionId, "-p", prompt, "--output-format", @@ -244,24 +270,24 @@ export class CopilotAgent implements Agent { ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; const logFile = new AgentLogFile(logPath); - let turnsStarted = 0; + let sessionId: string | null = null; try { return await runTurnWithEmptyResponseRetry({ logEvent: "copilot:output:continuation", onUsage, initialText: prompt, - runTurn: (text, onTurnUsage) => { - const continuation = turnsStarted > 0; - turnsStarted += 1; - return this.runTurn(text, cwd, { + runTurn: (text, onTurnUsage) => + this.runTurn(text, cwd, { onUsage: onTurnUsage, onMessage, signal, logFile, - continuation, - }); - }, + resumeSessionId: sessionId, + onSessionId: (id) => { + sessionId = id; + }, + }), }); } finally { logFile.finish(); @@ -276,16 +302,18 @@ export class CopilotAgent implements Agent { onMessage?: OnMessage; signal?: AbortSignal; logFile: AgentLogFile; - continuation: boolean; + resumeSessionId: string | null; + onSessionId: (sessionId: string) => void; }, ): Promise { - const { onUsage, onMessage, signal, logFile, continuation } = options; + const { onUsage, onMessage, signal, logFile } = options; + const { resumeSessionId, onSessionId } = options; return new Promise((resolve, reject) => { const child = spawn( this.bin, - continuation - ? buildCopilotContinueArgs(prompt, this.extraArgs) + resumeSessionId + ? buildCopilotResumeArgs(prompt, resumeSessionId, this.extraArgs) : buildCopilotArgs(prompt, this.schema, this.extraArgs), { cwd, @@ -305,6 +333,7 @@ export class CopilotAgent implements Agent { } let lastAgentMessage: string | null = null; + let turnSessionId: string | null = resumeSessionId; const cumulative: TokenUsage = { inputTokens: 0, outputTokens: 0, @@ -313,6 +342,12 @@ export class CopilotAgent implements Agent { }; parseJSONLStream(child.stdout!, logFile, (event) => { + const eventSessionId = copilotSessionIdOf(event); + if (eventSessionId) { + turnSessionId = eventSessionId; + onSessionId(eventSessionId); + } + if (event.type === "assistant.message") { const data = (event as CopilotAssistantMessageEvent).data; if (typeof data.content === "string") { @@ -342,14 +377,25 @@ export class CopilotAgent implements Agent { setupChildProcessHandlers(child, "copilot", reject, () => { if (!lastAgentMessage) { - appendDebugLog("copilot:output:missing", {}); + const resumeBlockedReason = turnSessionId + ? null + : "copilot reported no session id, so the turn cannot be resumed"; + appendDebugLog("copilot:output:missing", { + hasSessionId: turnSessionId !== null, + resumeBlockedReason, + }); reject( - new EmptyAgentResponseError("copilot returned no agent message", { - // copilot exposes no end-of-turn event, so a clean exit is the - // only completion signal it gives. - turnCompleted: true, - usage: cumulative, - }), + new EmptyAgentResponseError( + resumeBlockedReason + ? `copilot returned no agent message (${resumeBlockedReason})` + : "copilot returned no agent message", + { + // copilot exposes no end-of-turn event, so a clean exit is the + // only completion signal it gives. + turnCompleted: resumeBlockedReason === null, + usage: cumulative, + }, + ), ); return; } From cfa8867dc6382aa7b9ba3f2b9378b13ab694dcb2 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Sun, 9 Aug 2026 09:16:10 -0700 Subject: [PATCH 06/13] no-mistakes(review): exclude copilot recovery; guard codex --ephemeral and failed continuations --- README.md | 2 +- src/core/agents/codex.test.ts | 52 ++++++++++++++ src/core/agents/codex.ts | 15 +++- src/core/agents/copilot.test.ts | 80 ++------------------- src/core/agents/copilot.ts | 115 +++++------------------------- src/core/agents/empty-response.ts | 52 ++++++++++++-- src/core/config.ts | 5 +- 7 files changed, 136 insertions(+), 185 deletions(-) diff --git a/README.md b/README.md index e2930986..5d40953b 100644 --- a/README.md +++ b/README.md @@ -151,7 +151,7 @@ After installing from npm, the skill is available under the installed package di ``` - **Incremental commits** - each successful iteration is a separate unsigned git commit, so you can cherry-pick or revert individual changes without GPG or SSH signing prompts blocking the run; if `git commit` fails, gnhf preserves the uncommitted work and asks the next agent iteration to repair it -- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once inside the same session to continue before recording a failure, and records the nudge in the run log. OpenCode, Rovo Dev, and ACP targets reuse their live session; `claude`, `codex`, and `copilot` resume the exact session the empty turn used, and skip the nudge when that session cannot be identified or resumed (no session id reported, or agent args such as `--no-session-persistence` or ones `codex exec resume` rejects); `pi` is excluded because gnhf runs it with `--no-session`. Whenever the nudge is skipped the failure names the reason, because a retry that cannot reach the original session could only invent a summary. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. +- **Failure handling** - failed iterations are rolled back with `git reset --hard` except commit failures, which preserve uncommitted work for repair; agent-reported failures proceed to the next iteration immediately, retryable hard agent errors use exponential backoff, and permanent agent errors such as Claude low credit balance abort immediately and print the run log path. When an agent completes a turn without a final answer, gnhf nudges it once inside the same session to continue before recording a failure, and records the nudge in the run log. Covered: OpenCode, Rovo Dev, and ACP targets reuse their live session, and `claude` and `codex` resume the exact session the empty turn used - they skip the nudge when that session cannot be identified or resumed (no session id reported, or agent args such as `--no-session-persistence` or `--ephemeral`). Not covered: `pi`, because gnhf runs it with `--no-session`, and `copilot`, because it has no verified exact-session resume contract ([#193](https://github.com/kunchenguid/gnhf/issues/193)). Whenever the nudge is skipped the failure names the reason, because a retry that cannot reach the original session could only invent a summary. Complete no-op iterations are reported as failures and count toward the consecutive-failure abort limit. If the run exits with a pending commit failure, the exit summary warns that uncommitted changes were left for repair. - **Runtime caps** - `--max-iterations` stops before the next iteration begins, `--max-tokens` can abort mid-iteration once reported usage reaches the cap, and `--stop-when` ends the loop after an iteration whose agent output reports the natural-language condition is met unless a commit failure needs repair first; resumed runs reuse the saved stop condition unless you pass a new value, or `--stop-when ""` to clear it; pending commit-failure repair work is preserved and other uncommitted work is rolled back, and in the interactive TUI the final state remains visible until you press Ctrl+C to exit - **Iteration finalization** - agents are expected to finish validation, stop any background processes they started, and only then emit the final JSON result for the iteration - **Graceful interrupts** - in the interactive TUI, the first Ctrl+C requests a graceful stop and lets the current iteration finish (or ends backoff early), the second Ctrl+C force-stops immediately, and `SIGTERM` also force-stops immediately diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index 347a3b2b..a1ea98bb 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -342,6 +342,58 @@ describe("CodexAgent", () => { }, ); + it("does not re-ask when --ephemeral leaves no rollout to resume", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const agent = new CodexAgent("/tmp/schema.json", { + extraArgs: ["--ephemeral"], + }); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(proc, threadStarted("thread-abc")); + emitJson(proc, turnCompleted(10, 5)); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/--ephemeral records no rollout/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "codex:output:continuation", + expect.anything(), + ); + }); + + it("keeps the empty-response diagnostic when the continuation spawn itself fails", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, threadStarted("thread-abc")); + emitJson(first, turnCompleted(10, 5)); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + second.stderr.emit( + "data", + Buffer.from("error: unexpected argument '--add-dir' found"), + ); + second.emit("close", 2); + + const error = await promise.then( + () => null, + (err: Error) => err, + ); + expect(error?.message).toBe("codex returned no agent message"); + expect((error?.cause as Error).message).toContain( + "codex exited with code 2", + ); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "codex:output:continuation", + expect.objectContaining({ continuationFailed: true }), + ); + }); + it("forwards resume-compatible user args to the continuation", async () => { const first = createMockProcess(); const second = createMockProcess(); diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index 17a6933d..38c3aa95 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -76,6 +76,13 @@ const CODEX_RESUME_UNSUPPORTED_ARGS = [ "--ask-for-approval", ]; +// `--ephemeral` is accepted by both `codex exec` and `codex exec resume`, so +// the denylist above cannot catch it: the block is semantic. An ephemeral run +// records no rollout, so resuming its thread id fails with "no rollout found". +function codexRecordsNoRollout(extraArgs?: string[]): boolean { + return (extraArgs ?? []).includes("--ephemeral"); +} + function codexResumeUnsupportedArg(extraArgs?: string[]): string | null { return ( (extraArgs ?? []).find((arg) => @@ -344,9 +351,11 @@ export class CodexAgent implements Agent { const unsupportedArg = codexResumeUnsupportedArg(this.extraArgs); const resumeBlockedReason = !turnThreadId ? "codex reported no thread id, so the turn cannot be resumed" - : unsupportedArg - ? `configured codex arg "${unsupportedArg}" is not supported by \`codex exec resume\`, so the turn cannot be resumed` - : null; + : codexRecordsNoRollout(this.extraArgs) + ? "--ephemeral records no rollout, so the thread cannot be resumed" + : unsupportedArg + ? `configured codex arg "${unsupportedArg}" is not supported by \`codex exec resume\`, so the turn cannot be resumed` + : null; appendDebugLog("codex:output:missing", { sawTurnCompleted, hasThreadId: turnThreadId !== null, diff --git a/src/core/agents/copilot.test.ts b/src/core/agents/copilot.test.ts index 1dfe3c53..487da894 100644 --- a/src/core/agents/copilot.test.ts +++ b/src/core/agents/copilot.test.ts @@ -272,71 +272,22 @@ describe("CopilotAgent", () => { expect(args[1]).toContain("should_fully_stop"); }); - it("resumes the reported session once with the bare nudge when the turn had no assistant message", async () => { - const first = createMockProcess(); - const second = createMockProcess(); - mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + it("fails an empty response without retrying and names the missing resume contract", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); const agent = new CopilotAgent(); const promise = agent.run("test prompt", "/work/dir"); - emitJson(first, { + emitJson(proc, { type: "session.started", data: { session_id: "session-abc" }, }); - emitJson(first, { - type: "assistant.message", - data: { outputTokens: 5 }, - }); - first.emit("close", 0); - - await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); - emitJson(second, { - type: "assistant.message", - data: { - content: JSON.stringify({ - success: true, - summary: "recovered", - key_changes_made: [], - key_learnings: [], - }), - outputTokens: 3, - }, - }); - second.emit("close", 0); - - await expect(promise).resolves.toMatchObject({ - output: { success: true, summary: "recovered" }, - usage: { outputTokens: 8 }, - }); - - expect(mockSpawn.mock.calls[1]![1]).toEqual([ - "--resume", - "session-abc", - "-p", - "You did not produce a final answer. Continue and provide your final summary now.", - "--output-format", - "json", - "--stream", - "off", - "--no-color", - "--allow-all", - ]); - expect(mockAppendDebugLog).toHaveBeenCalledWith( - "copilot:output:continuation", - expect.objectContaining({ attempt: 1 }), - ); - }); - - it("does not re-ask when copilot reported no session id to resume", async () => { - const proc = createMockProcess(); - mockSpawn.mockReturnValue(proc); - const agent = new CopilotAgent(); - - const promise = agent.run("test prompt", "/work/dir"); emitJson(proc, { type: "assistant.message", data: { outputTokens: 5 } }); proc.emit("close", 0); - await expect(promise).rejects.toThrow(/no session id/); + await expect(promise).rejects.toThrow( + /copilot returned no agent message.*resume contract/, + ); expect(mockSpawn).toHaveBeenCalledTimes(1); expect(mockAppendDebugLog).not.toHaveBeenCalledWith( "copilot:output:continuation", @@ -344,23 +295,6 @@ describe("CopilotAgent", () => { ); }); - it("rejects after exactly one re-ask when copilot still returns no assistant message", async () => { - const first = createMockProcess(); - const second = createMockProcess(); - mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); - const agent = new CopilotAgent(); - - const promise = agent.run("test prompt", "/work/dir"); - emitJson(first, { type: "session.started", session_id: "session-abc" }); - first.emit("close", 0); - - await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); - second.emit("close", 0); - - await expect(promise).rejects.toThrow("copilot returned no agent message"); - expect(mockSpawn).toHaveBeenCalledTimes(2); - }); - it("does not re-ask when copilot exits non-zero", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/copilot.ts b/src/core/agents/copilot.ts index 213b7b10..597a9218 100644 --- a/src/core/agents/copilot.ts +++ b/src/core/agents/copilot.ts @@ -11,10 +11,6 @@ import { type TokenUsage, } from "./types.js"; import { appendDebugLog } from "../debug-log.js"; -import { - EmptyAgentResponseError, - runTurnWithEmptyResponseRetry, -} from "./empty-response.js"; import { AgentLogFile, parseJSONLStream, @@ -38,29 +34,15 @@ type CopilotEvent = | CopilotAssistantMessageEvent | (CopilotUsageEvent & { type: string }); -const COPILOT_SESSION_ID_KEYS = ["session_id", "sessionId"]; - -// Copilot's `--continue` picks a session by recency, which is not the same -// thing as picking the turn that just went silent. An empty turn is exactly -// the case where copilot may not have persisted a session at all, so the -// continuation is only safe when copilot itself named a session. Nothing in -// the JSONL contract guarantees that field, so when it is absent gnhf skips -// recovery rather than nudging a session it cannot identify. -function copilotSessionIdOf(event: unknown): string | null { - if (typeof event !== "object" || event === null) return null; - const record = event as Record; - const containers: unknown[] = [record, record.data, record.session]; - - for (const container of containers) { - if (typeof container !== "object" || container === null) continue; - for (const key of COPILOT_SESSION_ID_KEYS) { - const value = (container as Record)[key]; - if (typeof value === "string" && value) return value; - } - } - - return null; -} +// Copilot has no verified exact-session resume contract: `--continue` selects +// by recency rather than identity, and nothing in its JSONL output is +// documented to carry a session id that `--resume` would accept. Continuing +// the wrong session would let the agent describe an earlier iteration's work +// as if it were this turn's, so copilot is excluded from empty-response +// recovery and reports why instead. See +// https://github.com/kunchenguid/gnhf/issues/193. +const COPILOT_EMPTY_RESPONSE_MESSAGE = + "copilot returned no agent message (copilot has no verified exact-session resume contract, so the empty response cannot be safely recovered)"; interface CopilotAgentDeps { bin?: string; @@ -175,31 +157,6 @@ function buildCopilotArgs( ]; } -// `--resume ` reopens that exact session, so the continuation turn -// still sees the first turn's reasoning, tool calls, and the output contract it -// was already given - the nudge itself stays bare. -function buildCopilotResumeArgs( - prompt: string, - sessionId: string, - extraArgs?: string[], -): string[] { - const userArgs = extraArgs ?? []; - - return [ - ...userArgs, - "--resume", - sessionId, - "-p", - prompt, - "--output-format", - "json", - "--stream", - "off", - "--no-color", - ...(userSpecifiedPermissionMode(userArgs) ? [] : ["--allow-all"]), - ]; -} - function numberField( usage: Record, names: string[], @@ -270,24 +227,13 @@ export class CopilotAgent implements Agent { ): Promise { const { onUsage, onMessage, signal, logPath } = options ?? {}; const logFile = new AgentLogFile(logPath); - let sessionId: string | null = null; try { - return await runTurnWithEmptyResponseRetry({ - logEvent: "copilot:output:continuation", + return await this.runTurn(prompt, cwd, { onUsage, - initialText: prompt, - runTurn: (text, onTurnUsage) => - this.runTurn(text, cwd, { - onUsage: onTurnUsage, - onMessage, - signal, - logFile, - resumeSessionId: sessionId, - onSessionId: (id) => { - sessionId = id; - }, - }), + onMessage, + signal, + logFile, }); } finally { logFile.finish(); @@ -302,19 +248,14 @@ export class CopilotAgent implements Agent { onMessage?: OnMessage; signal?: AbortSignal; logFile: AgentLogFile; - resumeSessionId: string | null; - onSessionId: (sessionId: string) => void; }, ): Promise { const { onUsage, onMessage, signal, logFile } = options; - const { resumeSessionId, onSessionId } = options; return new Promise((resolve, reject) => { const child = spawn( this.bin, - resumeSessionId - ? buildCopilotResumeArgs(prompt, resumeSessionId, this.extraArgs) - : buildCopilotArgs(prompt, this.schema, this.extraArgs), + buildCopilotArgs(prompt, this.schema, this.extraArgs), { cwd, shell: shouldUseWindowsShell(this.bin, this.platform), @@ -333,7 +274,6 @@ export class CopilotAgent implements Agent { } let lastAgentMessage: string | null = null; - let turnSessionId: string | null = resumeSessionId; const cumulative: TokenUsage = { inputTokens: 0, outputTokens: 0, @@ -342,12 +282,6 @@ export class CopilotAgent implements Agent { }; parseJSONLStream(child.stdout!, logFile, (event) => { - const eventSessionId = copilotSessionIdOf(event); - if (eventSessionId) { - turnSessionId = eventSessionId; - onSessionId(eventSessionId); - } - if (event.type === "assistant.message") { const data = (event as CopilotAssistantMessageEvent).data; if (typeof data.content === "string") { @@ -377,26 +311,11 @@ export class CopilotAgent implements Agent { setupChildProcessHandlers(child, "copilot", reject, () => { if (!lastAgentMessage) { - const resumeBlockedReason = turnSessionId - ? null - : "copilot reported no session id, so the turn cannot be resumed"; appendDebugLog("copilot:output:missing", { - hasSessionId: turnSessionId !== null, - resumeBlockedReason, + recoverable: false, + reason: "copilot has no verified exact-session resume contract", }); - reject( - new EmptyAgentResponseError( - resumeBlockedReason - ? `copilot returned no agent message (${resumeBlockedReason})` - : "copilot returned no agent message", - { - // copilot exposes no end-of-turn event, so a clean exit is the - // only completion signal it gives. - turnCompleted: resumeBlockedReason === null, - usage: cumulative, - }, - ), - ); + reject(new Error(COPILOT_EMPTY_RESPONSE_MESSAGE)); return; } diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts index 04aee393..37df4aeb 100644 --- a/src/core/agents/empty-response.ts +++ b/src/core/agents/empty-response.ts @@ -1,4 +1,5 @@ -import { appendDebugLog } from "../debug-log.js"; +import { appendDebugLog, serializeError } from "../debug-log.js"; +import { PermanentAgentError } from "./types.js"; import type { AgentResult, OnUsage, TokenUsage } from "./types.js"; export const EMPTY_RESPONSE_CONTINUATION_PROMPT = @@ -22,15 +23,22 @@ export class EmptyAgentResponseError extends Error { constructor( message: string, - options: { turnCompleted: boolean; usage: TokenUsage }, + options: { turnCompleted: boolean; usage: TokenUsage; cause?: unknown }, ) { - super(message); + super(message, { cause: options.cause }); this.name = "EmptyAgentResponseError"; this.turnCompleted = options.turnCompleted; this.usage = { ...options.usage }; } } +function isAbortError(error: unknown): boolean { + return ( + error instanceof Error && + (error.name === "AbortError" || error.message === "Agent was aborted") + ); +} + export function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { const total: TokenUsage = { inputTokens: left.inputTokens + right.inputTokens, @@ -62,6 +70,14 @@ export interface EmptyResponseRetryOptions { * Runs one turn and, if it completed without a final message, runs exactly one * bare continuation turn. Usage reported to `onUsage` stays cumulative across * both turns; any other failure propagates untouched. + * + * If the continuation itself fails for a reason unrelated to the empty response + * - a CLI that refuses the resume command, say - the original empty-response + * diagnostic is what the user gets, with the continuation failure attached as + * `cause` and recorded in the run log. Recovery is best effort, so a broken + * continuation must never overwrite the accurate description of what went + * wrong. Aborts and permanent errors still propagate so `--max-tokens`, Ctrl+C, + * and abort-worthy provider failures behave exactly as before. */ export async function runTurnWithEmptyResponseRetry({ logEvent, @@ -86,9 +102,33 @@ export async function runTurnWithEmptyResponseRetry({ prompt: EMPTY_RESPONSE_CONTINUATION_PROMPT, }); - const retry = await runTurn(EMPTY_RESPONSE_CONTINUATION_PROMPT, (usage) => { - onUsage?.(addTokenUsage(firstTurnUsage, usage)); - }); + let retry: AgentResult; + try { + retry = await runTurn(EMPTY_RESPONSE_CONTINUATION_PROMPT, (usage) => { + onUsage?.(addTokenUsage(firstTurnUsage, usage)); + }); + } catch (continuationError) { + if ( + continuationError instanceof EmptyAgentResponseError || + continuationError instanceof PermanentAgentError || + isAbortError(continuationError) + ) { + throw continuationError; + } + + appendDebugLog(logEvent, { + ...logFields, + attempt: 1, + continuationFailed: true, + error: serializeError(continuationError), + }); + + throw new EmptyAgentResponseError(error.message, { + turnCompleted: error.turnCompleted, + usage: firstTurnUsage, + cause: continuationError, + }); + } return { output: retry.output, diff --git a/src/core/config.ts b/src/core/config.ts index 167bcc30..b287cd4e 100644 --- a/src/core/config.ts +++ b/src/core/config.ts @@ -165,10 +165,7 @@ function isReservedAgentArg(agent: AgentName, arg: string): boolean { arg === "--no-color" || arg === "--share" || arg.startsWith("--share=") || - arg === "--share-gist" || - arg === "--continue" || - arg === "--resume" || - arg.startsWith("--resume=") + arg === "--share-gist" ); case "pi": return ( From 0481ca077f6dd18617a8479b30e486c49fce7882 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Tue, 11 Aug 2026 12:27:57 -0700 Subject: [PATCH 07/13] no-mistakes(review): fix empty-response abort and cumulative usage --- src/core/agents/acp.ts | 1 + src/core/agents/claude.ts | 1 + src/core/agents/codex.ts | 1 + src/core/agents/empty-response.test.ts | 121 +++++++++++++++++++++++++ src/core/agents/empty-response.ts | 26 +++++- src/core/agents/opencode.ts | 1 + src/core/agents/rovodev.ts | 1 + 7 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 src/core/agents/empty-response.test.ts diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index 987e997d..5ed29848 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -226,6 +226,7 @@ export class AcpAgent implements Agent { sessionKey: this.runId, }, onUsage, + signal, initialText: buildAcpPrompt(prompt, this.schema), runTurn: (text, onTurnUsage) => this.runTurn({ diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 4b3ba0be..381961b0 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -365,6 +365,7 @@ export class ClaudeAgent implements Agent { return await runTurnWithEmptyResponseRetry({ logEvent: "claude:output:continuation", onUsage, + signal, initialText: prompt, runTurn: (text, onTurnUsage) => this.runTurn(text, cwd, { diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index 38c3aa95..456671f2 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -242,6 +242,7 @@ export class CodexAgent implements Agent { return await runTurnWithEmptyResponseRetry({ logEvent: "codex:output:continuation", onUsage, + signal, initialText: prompt, runTurn: (text, onTurnUsage) => this.runTurn(text, cwd, { diff --git a/src/core/agents/empty-response.test.ts b/src/core/agents/empty-response.test.ts new file mode 100644 index 00000000..a85e9996 --- /dev/null +++ b/src/core/agents/empty-response.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, it, vi } from "vitest"; + +vi.mock("../debug-log.js", () => ({ + appendDebugLog: vi.fn(), + serializeError: vi.fn(), +})); + +import { appendDebugLog } from "../debug-log.js"; +import { + EmptyAgentResponseError, + runTurnWithEmptyResponseRetry, +} from "./empty-response.js"; +import type { TokenUsage } from "./types.js"; + +function usage(inputTokens: number, outputTokens: number): TokenUsage { + return { + inputTokens, + outputTokens, + cacheReadTokens: 0, + cacheCreationTokens: 0, + }; +} + +describe("runTurnWithEmptyResponseRetry", () => { + it("does not start or log a continuation after usage aborts the run", async () => { + const controller = new AbortController(); + const runTurn = vi.fn().mockRejectedValue( + new EmptyAgentResponseError("empty", { + turnCompleted: true, + usage: usage(10, 5), + }), + ); + + await expect( + runTurnWithEmptyResponseRetry({ + logEvent: "agent:continuation", + onUsage: () => controller.abort(), + signal: controller.signal, + initialText: "prompt", + runTurn, + }), + ).rejects.toThrow("Agent was aborted"); + + expect(runTurn).toHaveBeenCalledTimes(1); + expect(appendDebugLog).not.toHaveBeenCalled(); + }); + + it("preserves cumulative usage when the continuation is also empty", async () => { + const firstUsage = usage(10, 5); + const continuationUsage = usage(3, 2); + const onUsage = vi.fn(); + const runTurn = vi + .fn() + .mockRejectedValueOnce( + new EmptyAgentResponseError("first empty", { + turnCompleted: true, + usage: firstUsage, + }), + ) + .mockRejectedValueOnce( + new EmptyAgentResponseError("continuation empty", { + turnCompleted: true, + usage: continuationUsage, + }), + ); + + const error = await runTurnWithEmptyResponseRetry({ + logEvent: "agent:continuation", + onUsage, + initialText: "prompt", + runTurn, + }).catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(EmptyAgentResponseError); + expect(error).toMatchObject({ + message: "continuation empty", + usage: usage(13, 7), + }); + expect(onUsage).toHaveBeenLastCalledWith(usage(13, 7)); + expect(runTurn).toHaveBeenCalledTimes(2); + }); + + it("surfaces a token abort after publishing cumulative empty usage", async () => { + const controller = new AbortController(); + const onUsage = vi.fn((reported: TokenUsage) => { + if (reported.inputTokens + reported.outputTokens >= 20) { + controller.abort(); + } + }); + const runTurn = vi + .fn() + .mockRejectedValueOnce( + new EmptyAgentResponseError("first empty", { + turnCompleted: true, + usage: usage(10, 5), + }), + ) + .mockRejectedValueOnce( + new EmptyAgentResponseError("continuation empty", { + turnCompleted: true, + usage: usage(3, 2), + }), + ); + + const error = await runTurnWithEmptyResponseRetry({ + logEvent: "agent:continuation", + onUsage, + signal: controller.signal, + initialText: "prompt", + runTurn, + }).catch((caught: unknown) => caught); + + expect(error).toBeInstanceOf(EmptyAgentResponseError); + expect(error).toMatchObject({ + message: "Agent was aborted", + usage: usage(13, 7), + }); + expect(onUsage).toHaveBeenLastCalledWith(usage(13, 7)); + expect(runTurn).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts index 37df4aeb..4a94572a 100644 --- a/src/core/agents/empty-response.ts +++ b/src/core/agents/empty-response.ts @@ -39,6 +39,10 @@ function isAbortError(error: unknown): boolean { ); } +function createAbortError(): Error { + return new Error("Agent was aborted"); +} + export function addTokenUsage(left: TokenUsage, right: TokenUsage): TokenUsage { const total: TokenUsage = { inputTokens: left.inputTokens + right.inputTokens, @@ -57,6 +61,7 @@ export interface EmptyResponseRetryOptions { logEvent: string; logFields?: Record; onUsage?: OnUsage; + signal?: AbortSignal; /** * Text for the first turn. The continuation turn is always the bare nudge - * adapters that cannot parse a bare turn apply their own existing prompt @@ -83,6 +88,7 @@ export async function runTurnWithEmptyResponseRetry({ logEvent, logFields, onUsage, + signal, initialText, runTurn, }: EmptyResponseRetryOptions): Promise { @@ -95,6 +101,9 @@ export async function runTurnWithEmptyResponseRetry({ const firstTurnUsage = error.usage; onUsage?.({ ...firstTurnUsage }); + if (signal?.aborted) { + throw createAbortError(); + } appendDebugLog(logEvent, { ...logFields, @@ -108,8 +117,23 @@ export async function runTurnWithEmptyResponseRetry({ onUsage?.(addTokenUsage(firstTurnUsage, usage)); }); } catch (continuationError) { + if (continuationError instanceof EmptyAgentResponseError) { + const cumulativeUsage = addTokenUsage( + firstTurnUsage, + continuationError.usage, + ); + onUsage?.({ ...cumulativeUsage }); + throw new EmptyAgentResponseError( + signal?.aborted ? "Agent was aborted" : continuationError.message, + { + turnCompleted: continuationError.turnCompleted, + usage: cumulativeUsage, + cause: continuationError, + }, + ); + } + if ( - continuationError instanceof EmptyAgentResponseError || continuationError instanceof PermanentAgentError || isAbortError(continuationError) ) { diff --git a/src/core/agents/opencode.ts b/src/core/agents/opencode.ts index 7ad6aa16..20be2cf6 100644 --- a/src/core/agents/opencode.ts +++ b/src/core/agents/opencode.ts @@ -380,6 +380,7 @@ export class OpenCodeAgent implements Agent { logEvent: "opencode:output:continuation", logFields: { sessionId: activeSessionId }, onUsage, + signal: runController.signal, initialText: buildPrompt(prompt, this.schema), runTurn: (text, onTurnUsage) => this.streamMessage( diff --git a/src/core/agents/rovodev.ts b/src/core/agents/rovodev.ts index f5f85fda..636c8c04 100644 --- a/src/core/agents/rovodev.ts +++ b/src/core/agents/rovodev.ts @@ -242,6 +242,7 @@ export class RovoDevAgent implements Agent { logEvent: "rovodev:output:continuation", logFields: { sessionId: activeSessionId }, onUsage, + signal: runController.signal, initialText: prompt, runTurn: async (text, onTurnUsage) => { await this.setChatMessage( From fa0c968bf70038416843dd28d7d6f7800cc70761 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Tue, 11 Aug 2026 12:43:47 -0700 Subject: [PATCH 08/13] no-mistakes(review): stabilize log tests with close synchronization --- src/core/agents/stream-utils.test.ts | 16 +++++----------- src/core/agents/stream-utils.ts | 12 +++++++++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/core/agents/stream-utils.test.ts b/src/core/agents/stream-utils.test.ts index b4369eb7..b1238e59 100644 --- a/src/core/agents/stream-utils.test.ts +++ b/src/core/agents/stream-utils.test.ts @@ -39,10 +39,6 @@ describe("AgentLogFile", () => { } }); - function flush(): Promise { - return new Promise((resolve) => setTimeout(resolve, 20)); - } - it("keeps recording stdout that arrives after an aborted run finished", async () => { const logPath = tempLogPath(); const logFile = new AgentLogFile(logPath); @@ -53,10 +49,10 @@ describe("AgentLogFile", () => { stdout.emit("data", Buffer.from('{"type":"before-abort"}\n')); // The abort path rejects the run while the child is still streaming. - logFile.finish(); + const closed = logFile.finish(); stdout.emit("data", Buffer.from('{"type":"after-abort"}\n')); child.emit("close", 143); - await flush(); + await closed; const written = readFileSync(logPath, "utf8"); expect(written).toContain("before-abort"); @@ -74,9 +70,9 @@ describe("AgentLogFile", () => { const second = createMockChild(); logFile.track(second as never); logFile.write('{"turn":2}\n'); - logFile.finish(); + const closed = logFile.finish(); second.emit("close", 0); - await flush(); + await closed; const written = readFileSync(logPath, "utf8"); expect(written).toContain('{"turn":1}'); @@ -85,7 +81,6 @@ describe("AgentLogFile", () => { // A late stdout chunk after the file closed must not raise // ERR_STREAM_WRITE_AFTER_END. expect(() => logFile.write('{"turn":"late"}\n')).not.toThrow(); - await flush(); expect(readFileSync(logPath, "utf8")).not.toContain("late"); }); @@ -97,8 +92,7 @@ describe("AgentLogFile", () => { logFile.write('{"spawn":"failed"}\n'); child.emit("error", new Error("ENOENT")); - logFile.finish(); - await flush(); + await logFile.finish(); expect(readFileSync(logPath, "utf8")).toContain('{"spawn":"failed"}'); }); diff --git a/src/core/agents/stream-utils.ts b/src/core/agents/stream-utils.ts index 4bbdce98..6ea7f5f3 100644 --- a/src/core/agents/stream-utils.ts +++ b/src/core/agents/stream-utils.ts @@ -19,14 +19,19 @@ export interface AgentLogSink { */ export class AgentLogFile implements AgentLogSink { private readonly stream: ReturnType | null; + private readonly closePromise: Promise; private openChildren = 0; private runFinished = false; private ended = false; constructor(logPath?: string) { - this.stream = logPath ? createWriteStream(logPath) : null; + const stream = logPath ? createWriteStream(logPath) : null; + this.stream = stream; + this.closePromise = stream + ? new Promise((resolve) => stream.once("close", () => resolve())) + : Promise.resolve(); // Log writes are best effort; a failed write must not take down the run. - this.stream?.on("error", (error) => { + stream?.on("error", (error) => { appendDebugLog("agent:log:write-failed", { error: serializeError(error), }); @@ -53,9 +58,10 @@ export class AgentLogFile implements AgentLogSink { } /** The run is done; close as soon as no tracked child is still running. */ - finish(): void { + finish(): Promise { this.runFinished = true; this.endIfIdle(); + return this.closePromise; } private endIfIdle(): void { From f407c3b85eecf6d9f4068dcd17568832dadfc1cf Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Tue, 11 Aug 2026 15:09:13 -0700 Subject: [PATCH 09/13] no-mistakes(review): fix retry abort, usage, and post-tool recovery --- src/core/agents/acp.test.ts | 62 ++++++++++++++++++++++++++ src/core/agents/acp.ts | 39 ++++++++++++---- src/core/agents/empty-response.test.ts | 45 +++++++++++++++++++ src/core/agents/empty-response.ts | 12 +++-- src/core/agents/rovodev.test.ts | 54 ++++++++++++++++++++++ src/core/agents/rovodev.ts | 1 + 6 files changed, 202 insertions(+), 11 deletions(-) diff --git a/src/core/agents/acp.test.ts b/src/core/agents/acp.test.ts index cba01efd..c5622e17 100644 --- a/src/core/agents/acp.test.ts +++ b/src/core/agents/acp.test.ts @@ -566,6 +566,68 @@ describe("AcpAgent", () => { ); }); + it("does not add fallback input to a cumulative continuation update", async () => { + const { runtime } = createFakeRuntime([ + { + events: [ + { + type: "status", + text: "u", + tag: "usage_update", + used: 100, + size: 1000, + }, + textDelta(JSON.stringify(VALID_OUTPUT)), + ], + result: { status: "completed" }, + }, + { events: [], result: { status: "completed" } }, + { + events: [ + { + type: "status", + text: "u", + tag: "usage_update", + used: 160, + size: 1000, + }, + textDelta(JSON.stringify(VALID_OUTPUT)), + ], + result: { status: "completed" }, + }, + ]); + const agent = makeAgent(runtime); + + await agent.run("warmup", "/w"); + const onUsage = vi.fn(); + const result = await agent.run("recover", "/w", { onUsage }); + + expect(result.usage.inputTokens).toBe(60); + expect(onUsage).toHaveBeenLastCalledWith(result.usage); + }); + + it("continues when tool work leaves no final output message", async () => { + const { runtime, calls } = createFakeRuntime([ + { + events: [ + textDelta("I will inspect the file."), + { type: "tool_call", text: "Read file", toolCallId: "1" }, + ], + result: { status: "completed" }, + }, + { + events: [textDelta(JSON.stringify(VALID_OUTPUT))], + result: { status: "completed" }, + }, + ]); + const agent = makeAgent(runtime); + + const result = await agent.run("p", "/w"); + + expect(result.output).toEqual(VALID_OUTPUT); + expect(calls.startTurnInputs).toHaveLength(2); + }); + it("rejects after the ACP continuation is also empty", async () => { const { runtime, calls } = createFakeRuntime([ { events: [], result: { status: "completed" } }, diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index 5ed29848..a494a0e3 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -13,6 +13,7 @@ import type { WriteStream } from "node:fs"; import { appendDebugLog, serializeError } from "../debug-log.js"; import { redactAcpTargetForLogs } from "../config.js"; import { + addTokenUsage, EmptyAgentResponseError, runTurnWithEmptyResponseRetry, } from "./empty-response.js"; @@ -215,6 +216,7 @@ export class AcpAgent implements Agent { this.handle = handle; const logStream = logPath ? createWriteStream(logPath) : null; + const turnUsageUpdates: boolean[] = []; try { // The ACP session is persistent, so a continuation turn still sees the // first turn's reasoning, tool calls, and the output contract that @@ -227,9 +229,24 @@ export class AcpAgent implements Agent { }, onUsage, signal, + combineUsage: (firstTurnUsage, continuationUsage) => { + const combined = addTokenUsage( + firstTurnUsage, + continuationUsage, + ); + if (!turnUsageUpdates[0] && turnUsageUpdates[1]) { + combined.inputTokens = continuationUsage.inputTokens; + if (!continuationUsage.estimated) { + delete combined.estimated; + } + } + return combined; + }, initialText: buildAcpPrompt(prompt, this.schema), - runTurn: (text, onTurnUsage) => - this.runTurn({ + runTurn: (text, onTurnUsage) => { + const turnIndex = turnUsageUpdates.length; + turnUsageUpdates.push(false); + return this.runTurn({ runtime, handle, text, @@ -237,8 +254,12 @@ export class AcpAgent implements Agent { signal, onMessage, onUsage: onTurnUsage, + onUsageUpdate: () => { + turnUsageUpdates[turnIndex] = true; + }, logStream, - }), + }); + }, }); } finally { logStream?.end(); @@ -253,10 +274,11 @@ export class AcpAgent implements Agent { signal?: AbortSignal; onMessage?: OnMessage; onUsage?: OnUsage; + onUsageUpdate?: () => void; logStream: WriteStream | null; }): Promise { const { runtime, handle, text: acpPrompt, cwd, signal, logStream } = params; - const { onMessage, onUsage } = params; + const { onMessage, onUsage, onUsageUpdate } = params; const requestId = randomUUID(); appendDebugLog("acp:turn:start", { @@ -311,10 +333,8 @@ export class AcpAgent implements Agent { // the turn, so this is the primary candidate to JSON.parse - separating // it from intermediate prose like "Let me examine the code...". let lastOutputMessage = ""; - // Concatenation of every output-stream chunk in the turn, used as a - // fallback when `lastOutputMessage` doesn't parse (e.g. when the agent - // streams the entire response as one continuous message without any - // tool_call to break it up). + // Concatenation of output-stream chunks since the most recent tool-call + // boundary, used as a fallback when `lastOutputMessage` doesn't parse. let outputBuf = ""; const computeUsage = (): TokenUsage => { @@ -387,6 +407,8 @@ export class AcpAgent implements Agent { // "tool call (completed)" are noisy and not useful in the TUI; // the user wants to see assistant prose, not mechanics. flushPendingMessage(); + lastOutputMessage = ""; + outputBuf = ""; // Each tool call (not its many tool_call_update follow-ups) bumps // the input-cost heuristic so the fallback estimate scales with // actual work. Adapters tag the initial event "tool_call" and @@ -407,6 +429,7 @@ export class AcpAgent implements Agent { latestUsed = event.used; this.lastReportedUsed = latestUsed; usageUpdateReceived = true; + onUsageUpdate?.(); onUsage?.(computeUsage()); } continue; diff --git a/src/core/agents/empty-response.test.ts b/src/core/agents/empty-response.test.ts index a85e9996..70302ffd 100644 --- a/src/core/agents/empty-response.test.ts +++ b/src/core/agents/empty-response.test.ts @@ -118,4 +118,49 @@ describe("runTurnWithEmptyResponseRetry", () => { expect(onUsage).toHaveBeenLastCalledWith(usage(13, 7)); expect(runTurn).toHaveBeenCalledTimes(2); }); + + it("rejects a successful continuation after cumulative usage aborts", async () => { + const controller = new AbortController(); + const onUsage = vi.fn((reported: TokenUsage) => { + if (reported.inputTokens + reported.outputTokens >= 20) { + controller.abort(); + } + }); + const continuationUsage = usage(3, 2); + const runTurn = vi + .fn() + .mockRejectedValueOnce( + new EmptyAgentResponseError("first empty", { + turnCompleted: true, + usage: usage(10, 5), + }), + ) + .mockImplementationOnce( + async (_text: string, onTurnUsage: (usage: TokenUsage) => void) => { + onTurnUsage(continuationUsage); + return { + output: { + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], + }, + usage: continuationUsage, + }; + }, + ); + + await expect( + runTurnWithEmptyResponseRetry({ + logEvent: "agent:continuation", + onUsage, + signal: controller.signal, + initialText: "prompt", + runTurn, + }), + ).rejects.toThrow("Agent was aborted"); + + expect(onUsage).toHaveBeenLastCalledWith(usage(13, 7)); + expect(runTurn).toHaveBeenCalledTimes(2); + }); }); diff --git a/src/core/agents/empty-response.ts b/src/core/agents/empty-response.ts index 4a94572a..67868c46 100644 --- a/src/core/agents/empty-response.ts +++ b/src/core/agents/empty-response.ts @@ -62,6 +62,7 @@ export interface EmptyResponseRetryOptions { logFields?: Record; onUsage?: OnUsage; signal?: AbortSignal; + combineUsage?: (left: TokenUsage, right: TokenUsage) => TokenUsage; /** * Text for the first turn. The continuation turn is always the bare nudge - * adapters that cannot parse a bare turn apply their own existing prompt @@ -89,6 +90,7 @@ export async function runTurnWithEmptyResponseRetry({ logFields, onUsage, signal, + combineUsage = addTokenUsage, initialText, runTurn, }: EmptyResponseRetryOptions): Promise { @@ -114,11 +116,11 @@ export async function runTurnWithEmptyResponseRetry({ let retry: AgentResult; try { retry = await runTurn(EMPTY_RESPONSE_CONTINUATION_PROMPT, (usage) => { - onUsage?.(addTokenUsage(firstTurnUsage, usage)); + onUsage?.(combineUsage(firstTurnUsage, usage)); }); } catch (continuationError) { if (continuationError instanceof EmptyAgentResponseError) { - const cumulativeUsage = addTokenUsage( + const cumulativeUsage = combineUsage( firstTurnUsage, continuationError.usage, ); @@ -154,9 +156,13 @@ export async function runTurnWithEmptyResponseRetry({ }); } + if (signal?.aborted) { + throw createAbortError(); + } + return { output: retry.output, - usage: addTokenUsage(firstTurnUsage, retry.usage), + usage: combineUsage(firstTurnUsage, retry.usage), }; } } diff --git a/src/core/agents/rovodev.test.ts b/src/core/agents/rovodev.test.ts index 8e48ce92..837fea87 100644 --- a/src/core/agents/rovodev.test.ts +++ b/src/core/agents/rovodev.test.ts @@ -479,6 +479,60 @@ describe("RovoDevAgent", () => { ); }); + it("continues when tool work leaves no final text segment", async () => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + + fetchMock + .mockResolvedValueOnce(jsonResponse({ status: "healthy" })) + .mockResolvedValueOnce( + jsonResponse({ session_id: "session-123", title: "gnhf" }), + ) + .mockResolvedValueOnce(jsonResponse({ message: "ok", prompt_set: true })) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce( + textResponse( + [ + "event: part_start", + 'data: {"index":0,"part":{"content":"I will inspect the file.","part_kind":"text"},"event_kind":"part_start"}', + "", + "event: on_call_tools_start", + 'data: {"parts":[{"tool_name":"open_files","args":"{}","tool_call_id":"tool-1","part_kind":"tool-call"}]}', + "", + "event: tool-return", + 'data: {"tool_name":"open_files","content":"ok","tool_call_id":"tool-1","part_kind":"tool-return"}', + "", + "event: close", + "data: ", + "", + ].join("\n"), + ), + ) + .mockResolvedValueOnce(jsonResponse({ response: "Chat message set" })) + .mockResolvedValueOnce( + textResponse( + [ + "event: part_start", + 'data: {"index":0,"part":{"content":"{\\"success\\":true,\\"summary\\":\\"recovered\\",\\"key_changes_made\\":[],\\"key_learnings\\":[]}","part_kind":"text"},"event_kind":"part_start"}', + "", + "event: close", + "data: ", + "", + ].join("\n"), + ), + ) + .mockResolvedValueOnce(jsonResponse({ message: "deleted" })); + + const result = await agent.run("test", "/repo"); + + expect(result.output.summary).toBe("recovered"); + expect( + fetchMock.mock.calls.filter(([url]) => + String(url).includes("/v3/set_chat_message"), + ), + ).toHaveLength(2); + }); + it("rejects after the rovodev continuation is also empty", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/rovodev.ts b/src/core/agents/rovodev.ts index 636c8c04..65b3db1b 100644 --- a/src/core/agents/rovodev.ts +++ b/src/core/agents/rovodev.ts @@ -592,6 +592,7 @@ export class RovoDevAgent implements Agent { }; const resetCurrentMessage = () => { + latestTextSegment = ""; currentTextParts = []; currentTextIndexes = new Map(); }; From 24119871f03eb5430b7b6e35b96b9700334ecdc0 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Tue, 11 Aug 2026 15:27:33 -0700 Subject: [PATCH 10/13] no-mistakes(document): Format ACP empty-response recovery code --- src/core/agents/acp.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index a494a0e3..935a1271 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -230,10 +230,7 @@ export class AcpAgent implements Agent { onUsage, signal, combineUsage: (firstTurnUsage, continuationUsage) => { - const combined = addTokenUsage( - firstTurnUsage, - continuationUsage, - ); + const combined = addTokenUsage(firstTurnUsage, continuationUsage); if (!turnUsageUpdates[0] && turnUsageUpdates[1]) { combined.inputTokens = continuationUsage.inputTokens; if (!continuationUsage.estimated) { From f6820f100e291a9d3a55be41d1795715f259c324 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Wed, 12 Aug 2026 01:07:09 -0700 Subject: [PATCH 11/13] no-mistakes(review): fix exact-session and whitespace empty-response recovery --- src/core/agents/acp.test.ts | 22 ++++++++ src/core/agents/acp.ts | 2 +- src/core/agents/claude.test.ts | 98 ++++++++++++++++++++++++++++++++++ src/core/agents/claude.ts | 17 +++--- src/core/agents/codex.test.ts | 26 +++++++++ src/core/agents/codex.ts | 5 +- 6 files changed, 159 insertions(+), 11 deletions(-) diff --git a/src/core/agents/acp.test.ts b/src/core/agents/acp.test.ts index c5622e17..134c147d 100644 --- a/src/core/agents/acp.test.ts +++ b/src/core/agents/acp.test.ts @@ -541,6 +541,28 @@ describe("AcpAgent", () => { expect(onUsage).toHaveBeenLastCalledWith(result.usage); }); + it("nudges when a completed turn produced only whitespace output", async () => { + const { runtime, calls } = createFakeRuntime([ + { + events: [textDelta(" \n\t")], + result: { status: "completed" }, + }, + { + events: [textDelta(JSON.stringify(VALID_OUTPUT))], + result: { status: "completed" }, + }, + ]); + const agent = makeAgent(runtime); + + const result = await agent.run("p", "/w"); + + expect(result.output).toEqual(VALID_OUTPUT); + expect(calls.startTurnInputs).toHaveLength(2); + expect(calls.startTurnInputs[1]?.text).toContain( + "You did not produce a final answer", + ); + }); + it("reports usage across both the empty turn and its continuation", async () => { // The first turn streams only reasoning, so it completes with no output // text while still burning output tokens that must survive into the total. diff --git a/src/core/agents/acp.ts b/src/core/agents/acp.ts index 935a1271..a8f50d9f 100644 --- a/src/core/agents/acp.ts +++ b/src/core/agents/acp.ts @@ -485,7 +485,7 @@ export class AcpAgent implements Agent { throw new Error(message); } - if (lastOutputMessage.length === 0 && outputBuf.length === 0) { + if (!lastOutputMessage.trim() && !outputBuf.trim()) { throw new EmptyAgentResponseError("ACP agent returned no output text", { turnCompleted: result.status === "completed", usage: computeUsage(), diff --git a/src/core/agents/claude.test.ts b/src/core/agents/claude.test.ts index 1362af48..ed2fb85d 100644 --- a/src/core/agents/claude.test.ts +++ b/src/core/agents/claude.test.ts @@ -1248,6 +1248,70 @@ describe("ClaudeAgent", () => { ); }); + it.each(["-c", "--continue"])( + "replaces configured %s with the captured session id for recovery", + async (continuationArg) => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const continuingAgent = new ClaudeAgent({ + extraArgs: [continuationArg], + }); + + const promise = continuingAgent.run("prompt", "/cwd"); + + emitLine(first, { + type: "result", + subtype: "success", + is_error: false, + session_id: "session-continued", + usage: { + input_tokens: 1, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 1, + }, + structured_output: null, + }); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + + const initialArgs = mockSpawn.mock.calls[0]![1] as string[]; + const continuationArgs = mockSpawn.mock.calls[1]![1] as string[]; + expect(initialArgs).toContain(continuationArg); + expect(continuationArgs).not.toContain(continuationArg); + expect(continuationArgs).toContain("--resume"); + expect(continuationArgs[continuationArgs.indexOf("--resume") + 1]).toBe( + "session-continued", + ); + + emitLine(second, { + type: "result", + subtype: "success", + is_error: false, + session_id: "session-continued", + usage: { + input_tokens: 1, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 1, + }, + structured_output: { + success: true, + summary: "recovered", + key_changes_made: [], + key_learnings: [], + }, + }); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { summary: "recovered" }, + }); + }, + ); + it("rejects after exactly one continuation when structured_output is still null", async () => { const first = createMockProcess(); const second = createMockProcess(); @@ -1338,6 +1402,40 @@ describe("ClaudeAgent", () => { expect(mockSpawn).toHaveBeenCalledTimes(1); }); + it.each(["-c", "--continue"])( + "does not recover configured %s without a captured session id", + async (continuationArg) => { + const proc = createMockProcess(); + mockSpawn.mockReturnValue(proc); + const continuingAgent = new ClaudeAgent({ + extraArgs: [continuationArg], + }); + + const promise = continuingAgent.run("prompt", "/cwd"); + + emitLine(proc, { + type: "result", + subtype: "success", + is_error: false, + usage: { + input_tokens: 0, + cache_read_input_tokens: 0, + cache_creation_input_tokens: 0, + output_tokens: 0, + }, + structured_output: null, + }); + proc.emit("close", 0); + + await expect(promise).rejects.toThrow(/no session id/); + expect(mockSpawn).toHaveBeenCalledTimes(1); + expect(mockAppendDebugLog).not.toHaveBeenCalledWith( + "claude:output:continuation", + expect.anything(), + ); + }, + ); + it("does not continue when claude reported an error result", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/claude.ts b/src/core/agents/claude.ts index 381961b0..92bdea80 100644 --- a/src/core/agents/claude.ts +++ b/src/core/agents/claude.ts @@ -151,8 +151,8 @@ function isFinalStructuredResult(event: ClaudeResultEvent): boolean { ); } -function userSpecifiedSessionContinuation(userArgs: string[]): boolean { - return userArgs.some((arg) => arg === "-c" || arg === "--continue"); +function isSessionContinuationArg(arg: string): boolean { + return arg === "-c" || arg === "--continue"; } // `--no-session-persistence` tells claude not to write the session to disk, so @@ -168,7 +168,10 @@ function buildClaudeArgs( resumeSessionId?: string | null, ): string[] { const userArgs = extraArgs ?? []; - const userSpecifiedPermissionMode = userArgs.some( + const turnArgs = resumeSessionId + ? userArgs.filter((arg) => !isSessionContinuationArg(arg)) + : userArgs; + const userSpecifiedPermissionMode = turnArgs.some( (arg) => arg === "--dangerously-skip-permissions" || arg === "--permission-mode" || @@ -178,7 +181,7 @@ function buildClaudeArgs( ); return [ - ...userArgs, + ...turnArgs, "-p", prompt, "--verbose", @@ -186,9 +189,7 @@ function buildClaudeArgs( "stream-json", "--json-schema", JSON.stringify(schema), - ...(resumeSessionId && !userSpecifiedSessionContinuation(userArgs) - ? ["--resume", resumeSessionId] - : []), + ...(resumeSessionId ? ["--resume", resumeSessionId] : []), ...(userSpecifiedPermissionMode ? [] : ["--dangerously-skip-permissions"]), ]; } @@ -612,7 +613,7 @@ export class ClaudeAgent implements Agent { const userArgs = this.extraArgs ?? []; const resumeBlockedReason = sessionPersistenceDisabled(userArgs) ? "--no-session-persistence disables session resume, so the turn cannot be continued" - : !turnSessionId && !userSpecifiedSessionContinuation(userArgs) + : !turnSessionId ? "claude reported no session id, so the turn cannot be resumed" : null; appendDebugLog("claude:output:missing", { diff --git a/src/core/agents/codex.test.ts b/src/core/agents/codex.test.ts index a1ea98bb..021daf0e 100644 --- a/src/core/agents/codex.test.ts +++ b/src/core/agents/codex.test.ts @@ -282,6 +282,32 @@ describe("CodexAgent", () => { ); }); + it("recovers a completed turn whose agent message is only whitespace", async () => { + const first = createMockProcess(); + const second = createMockProcess(); + mockSpawn.mockReturnValueOnce(first).mockReturnValueOnce(second); + const agent = new CodexAgent("/tmp/schema.json"); + + const promise = agent.run("test prompt", "/work/dir"); + emitJson(first, threadStarted("thread-abc")); + emitJson(first, agentMessage(" \n\t")); + emitJson(first, turnCompleted(10, 5)); + first.emit("close", 0); + + await vi.waitFor(() => expect(mockSpawn).toHaveBeenCalledTimes(2)); + emitJson(second, agentMessage(FINAL_OUTPUT)); + emitJson(second, turnCompleted(3, 2)); + second.emit("close", 0); + + await expect(promise).resolves.toMatchObject({ + output: { success: true, summary: "recovered" }, + }); + expect(mockAppendDebugLog).toHaveBeenCalledWith( + "codex:output:continuation", + expect.objectContaining({ attempt: 1 }), + ); + }); + it("does not re-ask when the turn never completed", async () => { const proc = createMockProcess(); mockSpawn.mockReturnValue(proc); diff --git a/src/core/agents/codex.ts b/src/core/agents/codex.ts index 456671f2..06715268 100644 --- a/src/core/agents/codex.ts +++ b/src/core/agents/codex.ts @@ -348,7 +348,8 @@ export class CodexAgent implements Agent { }); setupChildProcessHandlers(child, "codex", reject, () => { - if (!lastAgentMessage) { + const finalAgentMessage = lastAgentMessage?.trim(); + if (!finalAgentMessage) { const unsupportedArg = codexResumeUnsupportedArg(this.extraArgs); const resumeBlockedReason = !turnThreadId ? "codex reported no thread id, so the turn cannot be resumed" @@ -377,7 +378,7 @@ export class CodexAgent implements Agent { } try { - const output = JSON.parse(lastAgentMessage) as AgentOutput; + const output = JSON.parse(finalAgentMessage) as AgentOutput; resolve({ output, usage: cumulative }); } catch (err) { reject( From f02400414948caf9333441e293f7077868509f45 Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Wed, 12 Aug 2026 01:38:04 -0700 Subject: [PATCH 12/13] no-mistakes(document): Confirm empty-response docs and lint cleanliness --- e2e/e2e.test.ts | 112 ++++++++++++++++++++++++++ e2e/fixtures/mock-opencode-server.mjs | 40 +++++++++ 2 files changed, 152 insertions(+) diff --git a/e2e/e2e.test.ts b/e2e/e2e.test.ts index 434f5d22..25bb476b 100644 --- a/e2e/e2e.test.ts +++ b/e2e/e2e.test.ts @@ -234,6 +234,118 @@ describe("gnhf e2e", () => { expect(debugEvents).toContain("run:complete"); }, 30_000); + it("recovers one completed empty turn in the same session and records the nudge", async () => { + const cwd = createRepo(); + tempDirs.push(cwd); + const logDir = mkdtempSync(join(tmpdir(), "gnhf-e2e-logs-")); + tempDirs.push(logDir); + const mockLogPath = join(logDir, "mock-opencode.jsonl"); + + const result = await runCli( + cwd, + [ + "recover the empty response", + "--agent", + "opencode", + "--max-iterations", + "1", + "--prevent-sleep", + "off", + ], + { + env: { + ...createTestEnv(mockLogPath, tempDirs), + GNHF_MOCK_OPENCODE_EMPTY_ONCE: "1", + }, + }, + ); + + expect(result.code).toBe(0); + expect(git(["rev-list", "--count", "HEAD"], cwd)).toBe("2"); + + const messages = readJsonLines(mockLogPath).filter( + (entry) => entry.event === "message:start", + ); + expect(messages).toHaveLength(2); + expect(messages[1]?.sessionId).toBe(messages[0]?.sessionId); + expect(messages[1]?.prompt).toBe( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + + const debugLogPath = findRunLogPath(cwd); + const continuationEvents = readJsonLines(debugLogPath).filter( + (entry) => entry.event === "opencode:output:continuation", + ); + expect(continuationEvents).toEqual([ + expect.objectContaining({ + attempt: 1, + sessionId: messages[0]?.sessionId, + prompt: + "You did not produce a final answer. Continue and provide your final summary now.", + }), + ]); + expect( + readFileSync(join(dirname(debugLogPath), "notes.md"), "utf-8"), + ).toContain("**Summary:** mocked objective complete"); + }, 30_000); + + it("fails after one continuation when both completed turns are empty", async () => { + const cwd = createRepo(); + tempDirs.push(cwd); + const logDir = mkdtempSync(join(tmpdir(), "gnhf-e2e-logs-")); + tempDirs.push(logDir); + const mockLogPath = join(logDir, "mock-opencode.jsonl"); + + const result = await runCli( + cwd, + [ + "stop after one empty-response retry", + "--agent", + "opencode", + "--max-iterations", + "1", + "--prevent-sleep", + "off", + ], + { + env: { + ...createTestEnv(mockLogPath, tempDirs), + GNHF_MOCK_OPENCODE_ALWAYS_EMPTY: "1", + }, + }, + ); + + expect(result.code).toBe(0); + expect(git(["rev-list", "--count", "HEAD"], cwd)).toBe("1"); + + const messages = readJsonLines(mockLogPath).filter( + (entry) => entry.event === "message:start", + ); + expect(messages).toHaveLength(2); + expect(messages[1]?.sessionId).toBe(messages[0]?.sessionId); + expect(messages[1]?.prompt).toBe( + "You did not produce a final answer. Continue and provide your final summary now.", + ); + + const debugLogPath = findRunLogPath(cwd); + const debugEntries = readJsonLines(debugLogPath); + expect( + debugEntries.filter( + (entry) => entry.event === "opencode:output:continuation", + ), + ).toHaveLength(1); + const iterationEnd = debugEntries.find( + (entry) => entry.event === "iteration:end", + ); + expect(iterationEnd).toMatchObject({ + success: false, + summary: "OpenCode produced no final answer", + }); + expect( + readFileSync(join(dirname(debugLogPath), "notes.md"), "utf-8"), + ).toContain("[ERROR] OpenCode produced no final answer"); + }, 30_000); + it("runs on the current branch and pushes each successful iteration", async () => { const cwd = createRepo(); tempDirs.push(cwd); diff --git a/e2e/fixtures/mock-opencode-server.mjs b/e2e/fixtures/mock-opencode-server.mjs index 8abf5d30..de05b2b5 100755 --- a/e2e/fixtures/mock-opencode-server.mjs +++ b/e2e/fixtures/mock-opencode-server.mjs @@ -157,6 +157,34 @@ function emitCompletedEvents(sessionId, summary) { return output; } +function emitEmptyCompletedEvents(sessionId) { + broadcast({ + directory: "/repo", + payload: { + type: "message.updated", + properties: { + sessionID: sessionId, + info: { + id: "msg-empty-1", + role: "assistant", + tokens: { + input: 3, + output: 0, + cache: { read: 0, write: 0 }, + }, + }, + }, + }, + }); + broadcast({ + directory: "/repo", + payload: { + type: "session.idle", + properties: { sessionID: sessionId }, + }, + }); +} + function applyWorkspaceChange(sessionId) { const session = sessions.get(sessionId); if (!session?.directory) return; @@ -282,6 +310,18 @@ const server = createServer(async (req, res) => { return; } + const shouldEmitEmpty = + process.env.GNHF_MOCK_OPENCODE_ALWAYS_EMPTY === "1" || + (process.env.GNHF_MOCK_OPENCODE_EMPTY_ONCE === "1" && + !session?.emittedEmptyTurn); + if (shouldEmitEmpty && session) { + session.emittedEmptyTurn = true; + emitEmptyCompletedEvents(sessionId); + res.writeHead(204); + res.end(); + return; + } + applyWorkspaceChange(sessionId); emitCompletedEvents(sessionId, "mocked objective complete"); res.writeHead(204); From 74cdae979005a5043a247bac8818b8e1a154fd0d Mon Sep 17 00:00:00 2001 From: Jason Williams Date: Wed, 12 Aug 2026 01:57:36 -0700 Subject: [PATCH 13/13] no-mistakes: apply CI fixes --- .github/workflows/no-mistakes-required.yml | 20 +++- src/no-mistakes-required.test.ts | 118 +++++++++++++++++++++ 2 files changed, 133 insertions(+), 5 deletions(-) create mode 100644 src/no-mistakes-required.test.ts diff --git a/.github/workflows/no-mistakes-required.yml b/.github/workflows/no-mistakes-required.yml index 65c5b2d1..609fde8d 100644 --- a/.github/workflows/no-mistakes-required.yml +++ b/.github/workflows/no-mistakes-required.yml @@ -16,6 +16,7 @@ on: permissions: contents: read + pull-requests: read # GitHub concurrency groups retain at most one pending run, replacing older # pending runs even when cancel-in-progress is false. Give body-bearing events @@ -40,16 +41,25 @@ jobs: steps: - name: Verify no-mistakes signature in PR body env: - PR_BODY: ${{ github.event.pull_request.body }} + GH_TOKEN: ${{ github.token }} PR_AUTHOR: ${{ github.event.pull_request.user.login }} PR_NUMBER: ${{ github.event.pull_request.number }} run: | set -eu marker='Updates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)' - if printf '%s' "${PR_BODY:-}" | grep -qF -- "$marker"; then - echo "Found no-mistakes signature in PR #${PR_NUMBER} body." - exit 0 - fi + max_attempts=5 + attempt=1 + while [ "$attempt" -le "$max_attempts" ]; do + pr_body="$(gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR_NUMBER}" --jq '.body // ""')" + if printf '%s' "$pr_body" | grep -qF -- "$marker"; then + echo "Found no-mistakes signature in PR #${PR_NUMBER} body." + exit 0 + fi + if [ "$attempt" -lt "$max_attempts" ]; then + sleep 2 + fi + attempt=$((attempt + 1)) + done { echo "::error::This PR was not raised through no-mistakes." echo diff --git a/src/no-mistakes-required.test.ts b/src/no-mistakes-required.test.ts new file mode 100644 index 00000000..6704228c --- /dev/null +++ b/src/no-mistakes-required.test.ts @@ -0,0 +1,118 @@ +import { spawnSync } from "node:child_process"; +import { + chmodSync, + mkdtempSync, + mkdirSync, + readFileSync, + rmSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { delimiter, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, describe, expect, it } from "vitest"; +import yaml from "js-yaml"; + +const root = fileURLToPath(new URL("..", import.meta.url)); +const temporaryDirectories: string[] = []; + +function verificationScript(): string { + const workflow = yaml.load( + readFileSync( + join(root, ".github", "workflows", "no-mistakes-required.yml"), + "utf8", + ), + ) as { + jobs: { check: { steps: Array<{ name?: string; run?: string }> } }; + }; + const step = workflow.jobs.check.steps.find( + ({ name }) => name === "Verify no-mistakes signature in PR body", + ); + if (!step?.run) throw new Error("no-mistakes verification step is missing"); + return step.run; +} + +function runVerification(signedAfter: number) { + const temp = mkdtempSync(join(tmpdir(), "gnhf-no-mistakes-check-")); + temporaryDirectories.push(temp); + const bin = join(temp, "bin"); + const countFile = join(temp, "gh-count"); + mkdirSync(bin); + + const gh = join(bin, "gh"); + writeFileSync( + gh, + `#!/bin/sh +set -eu +count=0 +if [ -f "$MOCK_GH_COUNT_FILE" ]; then + count="$(cat "$MOCK_GH_COUNT_FILE")" +fi +count=$((count + 1)) +printf '%s' "$count" > "$MOCK_GH_COUNT_FILE" +if [ "$count" -ge "$MOCK_SIGNED_AFTER" ]; then + printf '%s\n' "$MOCK_SIGNED_BODY" +else + printf '%s\n' "$MOCK_UNSIGNED_BODY" +fi +`, + ); + chmodSync(gh, 0o755); + + const sleep = join(bin, "sleep"); + writeFileSync(sleep, "#!/bin/sh\nexit 0\n"); + chmodSync(sleep, 0o755); + + const result = spawnSync("bash", ["-c", verificationScript()], { + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}${delimiter}${process.env.PATH ?? ""}`, + GH_TOKEN: "test-token", + GITHUB_REPOSITORY: "kunchenguid/gnhf", + PR_AUTHOR: "contributor", + PR_NUMBER: "198", + MOCK_GH_COUNT_FILE: countFile, + MOCK_SIGNED_AFTER: String(signedAfter), + MOCK_UNSIGNED_BODY: "## What Changed\n\nPending pipeline summary.", + MOCK_SIGNED_BODY: + "## Pipeline\n\nUpdates from [git push no-mistakes](https://github.com/kunchenguid/no-mistakes)", + }, + }); + + return { + ...result, + calls: Number(readFileSync(countFile, "utf8")), + }; +} + +afterEach(() => { + for (const directory of temporaryDirectories.splice(0)) { + rmSync(directory, { recursive: true, force: true }); + } +}); + +describe.skipIf(process.platform === "win32")( + "no-mistakes PR signature workflow", + () => { + it("accepts a signature written shortly after the synchronize event", () => { + const result = runVerification(2); + + expect(result.status).toBe(0); + expect(result.calls).toBe(2); + expect(result.stdout).toContain( + "Found no-mistakes signature in PR #198 body.", + ); + }); + + it("still rejects a PR whose live body remains unsigned", () => { + const result = runVerification(6); + + expect(result.status).toBe(1); + expect(result.calls).toBe(5); + expect(result.stderr).toContain( + "This PR was not raised through no-mistakes.", + ); + }); + }, +);