diff --git a/agents/claude.ts b/agents/claude.ts index a2195bd..4d324f1 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -26,17 +26,14 @@ import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/ import { ThinkingTimer } from "../utils/timer.ts"; import type { TodoTracker } from "../utils/todoTracking.ts"; import { getDevDependencyVersion } from "../utils/version.ts"; +import { buildLearningsReflectionPrompt, runPostRunRetryLoop } from "./postRun.ts"; import { type AgentResult, type AgentRunContext, type AgentUsage, agent, - buildCommitPrompt, - getGitStatus, logTokenTable, - MAX_COMMIT_RETRIES, MAX_STDERR_LINES, - mergeAgentUsage, } from "./shared.ts"; async function installClaudeCli(): Promise { @@ -652,35 +649,31 @@ export const claude = agent({ onToolUse: ctx.onToolUse, }; - let result = await runClaude({ + const result = await runClaude({ ...runParams, args: [...baseArgs, "-p", ctx.instructions.full], }); - // usage needs to aggregate across the initial run + every commit retry. - // each runClaude() returns only its own iteration's usage, so without - // merging the caller sees only the final retry's slice and undercounts. - let aggregatedUsage = result.usage; - // post-run: if the working tree is dirty, resume the session and ask the agent to commit - for (let attempt = 0; attempt < MAX_COMMIT_RETRIES; attempt++) { - if (!result.success || !result.sessionId) break; - const status = getGitStatus(); - if (!status) break; - - log.info(`» dirty working tree (attempt ${attempt + 1}/${MAX_COMMIT_RETRIES}):\n${status}`); - result = await runClaude({ - ...runParams, - args: [ - ...baseArgs, - "-p", - buildCommitPrompt("claude", status), - "--resume", - result.sessionId, - ], - }); - aggregatedUsage = mergeAgentUsage(aggregatedUsage, result.usage); - } - - return { ...result, usage: aggregatedUsage }; + // post-run retry loop aggregates usage across the initial run + every + // resume, so the caller sees the whole session — not just the final + // slice. claude needs a sessionId to `--resume`; if it's missing the + // loop bails (checks still ran, so persistent hook failures still fail + // the run). the reflection prompt fires once after gates go clean, as a + // dedicated turn that nudges the agent to persist learnings. + return runPostRunRetryLoop({ + initialResult: result, + initialUsage: result.usage, + stopScript: ctx.stopScript, + reflectionPrompt: buildLearningsReflectionPrompt("claude"), + canResume: (r) => Boolean(r.sessionId), + resume: async (c) => { + const sessionId = c.previousResult.sessionId; + if (!sessionId) throw new Error("unreachable: canResume gated on sessionId"); + return runClaude({ + ...runParams, + args: [...baseArgs, "-p", c.prompt, "--resume", sessionId], + }); + }, + }); }, }); diff --git a/agents/opencode.ts b/agents/opencode.ts index 43391e6..aa95bb5 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -26,17 +26,14 @@ import { SPAWN_ACTIVITY_TIMEOUT_CODE, SpawnTimeoutError, spawn } from "../utils/ import { ThinkingTimer } from "../utils/timer.ts"; import type { TodoTracker } from "../utils/todoTracking.ts"; import { getDevDependencyVersion } from "../utils/version.ts"; +import { buildLearningsReflectionPrompt, runPostRunRetryLoop } from "./postRun.ts"; import { type AgentResult, type AgentRunContext, type AgentUsage, agent, - buildCommitPrompt, - getGitStatus, logTokenTable, - MAX_COMMIT_RETRIES, MAX_STDERR_LINES, - mergeAgentUsage, } from "./shared.ts"; async function installOpencodeCli(): Promise { @@ -701,29 +698,26 @@ export const opencode = agent({ onToolUse: ctx.onToolUse, }; - let result = await runOpenCode({ + const result = await runOpenCode({ ...runParams, args: [...baseArgs, ctx.instructions.full], }); - // usage needs to aggregate across the initial run + every commit retry. - // each runOpenCode() returns only its own iteration's usage, so without - // merging the caller sees only the final retry's slice and undercounts. - let aggregatedUsage = result.usage; - // post-run: if the working tree is dirty, continue the session and ask the agent to commit - for (let attempt = 0; attempt < MAX_COMMIT_RETRIES; attempt++) { - if (!result.success) break; - const status = getGitStatus(); - if (!status) break; - - log.info(`» dirty working tree (attempt ${attempt + 1}/${MAX_COMMIT_RETRIES}):\n${status}`); - result = await runOpenCode({ - ...runParams, - args: [...baseArgs, "--continue", buildCommitPrompt("opencode", status)], - }); - aggregatedUsage = mergeAgentUsage(aggregatedUsage, result.usage); - } - - return { ...result, usage: aggregatedUsage }; + // post-run retry loop aggregates usage across the initial run + every + // resume, so the caller sees the whole session — not just the final + // slice. opencode always accepts `--continue`, so no canResume guard. + // the reflection prompt fires once after gates go clean, as a dedicated + // turn that nudges the agent to persist learnings. + return runPostRunRetryLoop({ + initialResult: result, + initialUsage: result.usage, + stopScript: ctx.stopScript, + reflectionPrompt: buildLearningsReflectionPrompt("opencode"), + resume: async (c) => + runOpenCode({ + ...runParams, + args: [...baseArgs, "--continue", c.prompt], + }), + }); }, }); diff --git a/agents/postRun.test.ts b/agents/postRun.test.ts new file mode 100644 index 0000000..f81df09 --- /dev/null +++ b/agents/postRun.test.ts @@ -0,0 +1,429 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { SPAWN_TIMEOUT_CODE, SpawnTimeoutError } from "../utils/subprocess.ts"; +import type { AgentResult } from "./shared.ts"; + +vi.mock("./shared.ts", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + getGitStatus: vi.fn(() => ""), + }; +}); + +vi.mock("../utils/subprocess.ts", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + spawn: vi.fn(), + }; +}); + +const { runPostRunRetryLoop, executeStopHook } = await import("./postRun.ts"); +const { getGitStatus } = await import("./shared.ts"); +const { spawn } = await import("../utils/subprocess.ts"); +const mockedGetGitStatus = vi.mocked(getGitStatus); +const mockedSpawn = vi.mocked(spawn); + +const successResult = (overrides: Partial = {}): AgentResult => ({ + success: true, + output: "ok", + ...overrides, +}); + +describe("runPostRunRetryLoop — reflection turn", () => { + beforeEach(() => { + mockedGetGitStatus.mockReset(); + mockedGetGitStatus.mockReturnValue(""); + mockedSpawn.mockReset(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("does not flip a successful run to failed when reflection returns success:false", async () => { + // the reflection turn is a best-effort nudge (update_learnings). if it + // fails — e.g. the model API errors mid-turn — the underlying task has + // already completed and been gated cleanly, so the run as a whole must + // still be reported as successful. + const initial = successResult({ output: "task done" }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue({ success: false, error: "model API transient failure" }); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + reflectionPrompt: "REFLECTION: call update_learnings if anything is worth saving", + }); + + expect(result.success).toBe(true); + expect(result.output).toBe("task done"); + expect(result.error).toBeUndefined(); + expect(resume).toHaveBeenCalledTimes(1); + expect(resume.mock.calls[0]?.[0].prompt).toMatch(/REFLECTION/); + }); + + it("still aggregates usage from a failed reflection turn", async () => { + // the reflection consumed tokens even if it didn't produce useful output; + // the run total must reflect that so billing/reporting stays accurate. + const initial = successResult({ + usage: { agent: "claude", inputTokens: 100, outputTokens: 50 }, + }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue({ + success: false, + error: "model API transient failure", + usage: { agent: "claude", inputTokens: 10, outputTokens: 5 }, + }); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: initial.usage, + stopScript: null, + resume, + reflectionPrompt: "reflect", + }); + + expect(result.success).toBe(true); + expect(result.usage?.inputTokens).toBe(110); + expect(result.usage?.outputTokens).toBe(55); + }); + + it("falls back to the reflection's output when the pre-reflection output is empty", async () => { + // the preservation fix must only kick in when the task actually produced + // meaningful output. runs that communicate exclusively through MCP tools + // (e.g. report_progress) leave result.output = "" — using `??` here kept + // the empty string and dropped the reflection's reply, leaving the + // fallback `handleAgentResult` path with nothing to show. prefer the + // reflection's output (even a trivial "done") over no output at all. + const initial = successResult({ output: "" }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "done" })); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + reflectionPrompt: "REFLECTION: consider update_learnings", + }); + + expect(result.success).toBe(true); + expect(result.output).toBe("done"); + }); + + it("preserves the pre-reflection task output when a trivial reflection ('done') succeeds", async () => { + // the reflection turn is a meta-ask — its literal reply ("done" or a + // short "updated learnings with N bullets") is not the task summary the + // caller wants to see. before this fix, `result = reflectionResult` + // clobbered the task's output on the returned AgentResult, so downstream + // consumers (handleAgentResult's fallback path when toolState is empty, + // programmatic callers of main()) saw "done" instead of the real + // summary. assert the task's output survives a successful reflection. + const initial = successResult({ output: "Implemented feature X; tests pass; pushed PR #42" }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "done" })); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + reflectionPrompt: "REFLECTION: consider update_learnings", + }); + + expect(result.success).toBe(true); + expect(result.output).toBe("Implemented feature X; tests pass; pushed PR #42"); + }); + + it("skips reflection entirely when canResume returns false", async () => { + const initial = successResult(); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult()); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + canResume: () => false, + reflectionPrompt: "reflect", + }); + + expect(result.success).toBe(true); + expect(resume).not.toHaveBeenCalled(); + }); + + it("catches a reflection turn that dirties the tree via the dirty-tree gate on the next iteration", async () => { + // PR claims: "if the reflection turn dirties the tree, the loop picks + // that up on the next iteration via the normal dirty-tree gate." lock + // it in — without this invariant the reflection prompt could bypass the + // commit-before-you-finish contract whenever the agent misbehaves. + // + // three getGitStatus calls in sequence: + // 1. clean (triggers reflection) + // 2. reflection left the tree dirty + // 3. retry committed the changes — now clean, loop exits + mockedGetGitStatus + .mockReturnValueOnce("") + .mockReturnValueOnce(" M scratch/notes.md") + .mockReturnValueOnce(""); + + const initial = successResult(); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "resumed" })); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + reflectionPrompt: "REFLECTION: consider update_learnings", + }); + + expect(result.success).toBe(true); + // call 0: reflection; call 1: dirty-tree retry + expect(resume).toHaveBeenCalledTimes(2); + expect(resume.mock.calls[0]?.[0].prompt).toContain("REFLECTION"); + expect(resume.mock.calls[1]?.[0].prompt).toContain("UNCOMMITTED CHANGES"); + expect(resume.mock.calls[1]?.[0].prompt).toContain("scratch/notes.md"); + }); + + it("surfaces a persistent stop hook failure as AgentResult.error after MAX_POST_RUN_RETRIES", async () => { + // PR test plan item #1: "confirm the agent is resumed with the hook + // output and the run fails after 3 attempts if never resolved." + // + // stop the hook from passing on every invocation, have `resume` always + // return success (the agent tried but couldn't fix the issue), and + // verify: (a) the loop exhausts all retries, (b) the final result is + // success=false, (c) the error mentions the retry count and the hook + // output verbatim so the GitHub comment surfaces what actually failed. + const hookFailure = { + stdout: "lint: 3 issues in src/foo.ts", + stderr: "", + exitCode: 7, + durationMs: 5, + }; + mockedSpawn.mockResolvedValue(hookFailure); + + const initial = successResult({ output: "agent done" }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "retry done" })); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: "pnpm lint", + resume, + reflectionPrompt: undefined, + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("stop hook failed"); + expect(result.error).toContain("exit code 7"); + expect(result.error).toContain("3 retry attempts"); + expect(result.error).toContain("lint: 3 issues in src/foo.ts"); + // each retry feeds the hook output back into the agent as the resume prompt + expect(resume).toHaveBeenCalledTimes(3); + for (const call of resume.mock.calls) { + expect(call[0].prompt).toContain("STOP HOOK FAILED"); + expect(call[0].prompt).toContain("lint: 3 issues in src/foo.ts"); + } + }); + + it("treats a persistently dirty tree (no stop hook failure) as a soft-fail", async () => { + // the PR documents: "dirty-tree-only failures preserve prior behavior: + // they're logged but don't fail the run." a regression that started + // surfacing dirty-tree as AgentResult.error would make every run that + // leaves untracked test fixtures around fail spuriously. guard it. + mockedGetGitStatus.mockReturnValue(" M src/foo.ts"); + const initial = successResult(); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult({ output: "tried but tree still dirty" })); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: null, + resume, + }); + + expect(result.success).toBe(true); + expect(result.error).toBeUndefined(); + // retries were attempted (the loop fed the dirty-tree prompt back to the agent) + expect(resume).toHaveBeenCalledTimes(3); + for (const call of resume.mock.calls) { + expect(call[0].prompt).toContain("UNCOMMITTED CHANGES"); + } + }); + + it("surfaces a stop hook failure even when canResume is false (no retry budget, still fails the run)", async () => { + // the retry loop is best-effort. when canResume says no (e.g. claude + // without a sessionId), we still need the failure gate to fire so the + // user sees WHY the run failed instead of an opaque success. covers the + // "checks still ran even if we can't resume" comment in postRun.ts. + mockedSpawn.mockResolvedValue({ + stdout: "typecheck: 2 errors", + stderr: "", + exitCode: 1, + durationMs: 1, + }); + const initial = successResult(); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult()); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: "pnpm typecheck", + resume, + canResume: () => false, + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("stop hook failed"); + expect(result.error).toContain("typecheck: 2 errors"); + // no retries were attempted because canResume said no — error lists no + // retry count (that would be a lie). + expect(result.error).not.toContain("retry attempt"); + expect(resume).not.toHaveBeenCalled(); + }); + + it("short-circuits the loop when the initial result is already failed", async () => { + // if the agent already failed (timeout, model error) there's no point + // running gates or a reflection — the run is toast. preserve the original + // error verbatim so triage is straightforward. + const initial: AgentResult = { + success: false, + error: "agent died mid-turn", + output: "partial", + }; + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue(successResult()); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: undefined, + stopScript: "pnpm lint", + resume, + reflectionPrompt: "reflect", + }); + + expect(result.success).toBe(false); + expect(result.error).toBe("agent died mid-turn"); + expect(resume).not.toHaveBeenCalled(); + expect(mockedSpawn).not.toHaveBeenCalled(); + expect(mockedGetGitStatus).not.toHaveBeenCalled(); + }); + + it("aggregates usage across every gate retry", async () => { + // billing/reporting rely on the usage summary reflecting the full run, + // not just the final retry's slice. regression gate. + mockedSpawn.mockResolvedValue({ + stdout: "fail", + stderr: "", + exitCode: 1, + durationMs: 1, + }); + const initial = successResult({ + usage: { agent: "claude", inputTokens: 100, outputTokens: 50 }, + }); + const resume = vi + .fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise>() + .mockResolvedValue({ + success: true, + output: "retry", + usage: { agent: "claude", inputTokens: 10, outputTokens: 5 }, + }); + + const result = await runPostRunRetryLoop({ + initialResult: initial, + initialUsage: initial.usage, + stopScript: "flaky", + resume, + }); + + // 100 initial + 10 * 3 retries = 130 + expect(result.usage?.inputTokens).toBe(130); + expect(result.usage?.outputTokens).toBe(65); + }); +}); + +describe("executeStopHook — output capture", () => { + beforeEach(() => { + mockedSpawn.mockReset(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("includes both stdout and stderr in the failure output when both are populated", async () => { + // hooks that wrap other tools commonly emit a benign warning to stderr + // (e.g. "config file not found, using defaults") and the actionable error + // to stdout. a `(stderr || stdout)` heuristic drops stdout entirely + // whenever stderr is non-empty, starving the agent of the information it + // needs to fix the issue. + mockedSpawn.mockResolvedValue({ + stdout: "ERROR: lint check failed at path/to/file.ts:42", + stderr: "Warning: config file not found, using defaults", + exitCode: 1, + durationMs: 5, + }); + const failure = await executeStopHook("run-lint"); + expect(failure).not.toBeNull(); + expect(failure?.output).toContain("ERROR: lint check failed at path/to/file.ts:42"); + expect(failure?.output).toContain("Warning: config file not found, using defaults"); + }); + + it("returns null (treated as passed) when spawn throws a timeout", async () => { + // infra-level failures can't be fixed by the agent. surfacing them as a + // hook failure would put the loop into a retry cycle that never + // terminates. soft-fail and let the run succeed. + mockedSpawn.mockRejectedValue( + new SpawnTimeoutError("hook exceeded 10 minutes", SPAWN_TIMEOUT_CODE) + ); + const failure = await executeStopHook("slow-hook"); + expect(failure).toBeNull(); + }); + + it("returns null (treated as passed) on spawn ENOENT (command not found)", async () => { + // if the user misconfigures the hook (wrong binary, typo), the spawn + // itself throws. same rationale as timeouts: soft-fail, don't retry. + mockedSpawn.mockRejectedValue( + Object.assign(new Error("spawn nosuchbin ENOENT"), { code: "ENOENT" }) + ); + const failure = await executeStopHook("nosuchbin"); + expect(failure).toBeNull(); + }); + + it("truncates oversize output, keeping the tail", async () => { + // the error is embedded in AgentResult.error and flows into GitHub + // comments (65535-char cap). the 4096-char truncation is our guardrail; + // lock it in so a well-meaning refactor can't blow the comment budget. + const longTail = "LAST_LINE_IS_ACTIONABLE"; + const longOutput = "x".repeat(10_000) + longTail; + mockedSpawn.mockResolvedValue({ + stdout: longOutput, + stderr: "", + exitCode: 1, + durationMs: 1, + }); + const failure = await executeStopHook("noisy"); + expect(failure?.output).toContain(longTail); + expect(failure?.output).toContain("truncated"); + expect(failure?.output.length).toBeLessThan(longOutput.length); + }); +}); diff --git a/agents/postRun.ts b/agents/postRun.ts new file mode 100644 index 0000000..02f6fba --- /dev/null +++ b/agents/postRun.ts @@ -0,0 +1,262 @@ +import { type AgentId, formatMcpToolRef } from "../external.ts"; +import { LIFECYCLE_HOOK_TIMEOUT_MS } from "../lifecycle.ts"; +import { log } from "../utils/cli.ts"; +import { + SPAWN_ACTIVITY_TIMEOUT_CODE, + SPAWN_TIMEOUT_CODE, + SpawnTimeoutError, + spawn, +} from "../utils/subprocess.ts"; +import { + type AgentResult, + type AgentUsage, + buildCommitPrompt, + getGitStatus, + hasPostRunIssues, + MAX_POST_RUN_RETRIES, + mergeAgentUsage, + type PostRunIssues, + type StopHookFailure, +} from "./shared.ts"; + +/** + * hook output can flow into two size-sensitive places: the LLM resume prompt + * (context window) and AgentResult.error (surfaced in GitHub comments capped + * at 65535 chars). truncate the tail to keep both bounded; the tail is + * usually the most actionable part of a failing script's output. + */ +const MAX_HOOK_OUTPUT_CHARS = 4096; + +function truncateHookOutput(raw: string): string { + if (raw.length <= MAX_HOOK_OUTPUT_CHARS) return raw; + return `...(truncated, showing last ${MAX_HOOK_OUTPUT_CHARS} chars)\n${raw.slice(-MAX_HOOK_OUTPUT_CHARS)}`; +} + +/** + * run the user-configured stop hook. + * + * parallel to `executeLifecycleHook` (which soft-fails with a warning), but + * returns structured output so agent harnesses can feed the failure back into + * the session as a resume prompt. + * + * - non-zero exit → `StopHookFailure`, actionable: the output is fed to the + * agent so it can fix the underlying issue. + * - timeout / spawn error → null, treated as passed: we can't usefully ask the + * agent to fix an infrastructure problem, and retrying would risk infinite + * loops. + */ +export async function executeStopHook(script: string): Promise { + log.info("» executing stop hook..."); + try { + const result = await spawn({ + cmd: "bash", + args: ["-c", script], + env: process.env, + timeout: LIFECYCLE_HOOK_TIMEOUT_MS, + activityTimeout: 0, + onStdout: (chunk) => process.stdout.write(chunk), + onStderr: (chunk) => process.stderr.write(chunk), + }); + if (result.exitCode === 0) { + log.info("» stop hook passed"); + return null; + } + // include both streams — scripts often emit a benign warning to stderr + // and the actionable error to stdout (or vice versa), and picking one + // starves the agent of the diagnostic it needs. stderr-first so stdout + // (typically longer, where truncation is more likely to bite) keeps its + // tail — summaries/totals usually live at the end. + const combined = [result.stderr.trim(), result.stdout.trim()].filter(Boolean).join("\n"); + const output = truncateHookOutput(combined); + log.info(`» stop hook failed with exit code ${result.exitCode}`); + return { exitCode: result.exitCode, output }; + } catch (err) { + const isTimeout = + err instanceof SpawnTimeoutError && + (err.code === SPAWN_TIMEOUT_CODE || err.code === SPAWN_ACTIVITY_TIMEOUT_CODE); + const msg = err instanceof Error ? err.message : String(err); + log.warning( + `stop hook ${isTimeout ? "timed out" : "failed to spawn"}: ${msg} — skipping retry` + ); + return null; + } +} + +export function buildStopHookPrompt(failure: StopHookFailure): string { + return [ + `STOP HOOK FAILED — the repo-configured stop hook exited with code ${failure.exitCode}. your work is not done until the hook exits cleanly. address the issue below and push any resulting changes to a pull request.`, + "", + "```", + failure.output || "(no output)", + "```", + ].join("\n"); +} + +/** + * check the two post-run gates: did the stop hook pass and is the working + * tree clean? returns everything that still needs fixing so the caller can + * render a single combined resume prompt. + */ +export async function collectPostRunIssues(params: { + stopScript: string | null | undefined; +}): Promise { + const issues: PostRunIssues = {}; + if (params.stopScript) { + const failure = await executeStopHook(params.stopScript); + if (failure) issues.stopHook = failure; + } + const status = getGitStatus(); + if (status) issues.dirtyTree = status; + return issues; +} + +export function buildPostRunPrompt(issues: PostRunIssues): string { + const parts: string[] = []; + if (issues.stopHook) parts.push(buildStopHookPrompt(issues.stopHook)); + if (issues.dirtyTree) parts.push(buildCommitPrompt(issues.dirtyTree)); + return parts.join("\n\n---\n\n"); +} + +/** + * prompt for a dedicated post-run reflection turn nudging the agent to call + * `update_learnings` if it discovered anything worth persisting. + * + * this exists because the learnings step baked into mode checklists is + * frequently ignored — the agent stays focused on the task and the meta-ask + * falls through. delivering it as its own resume turn, with nothing competing + * for attention, raises the fire rate substantially. + */ +export function buildLearningsReflectionPrompt(agentId: AgentId): string { + const t = (name: string) => formatMcpToolRef(agentId, name); + return [ + `REFLECTION — before you finish, think back over this task: did you discover anything about this repo's setup, test commands, conventions, or patterns that you are confident is correct and would reliably help future runs?`, + "", + `if so, call \`${t("update_learnings")}\` to persist it.`, + "", + `rules:`, + `- only call \`${t("update_learnings")}\` when the finding is high-confidence and broadly useful. skip if unsure, speculative, or one-off.`, + `- pass the FULL merged list: existing learnings from the original prompt + your new discoveries. one fact per bullet, lines starting with \`- \`.`, + `- deduplicate, and drop bullets that are clearly wrong or no longer relevant to the current codebase.`, + `- if you already called \`${t("update_learnings")}\` earlier in this run, or nothing new is worth capturing, just reply "done" and stop — do not edit the repo for this reflection.`, + ].join("\n"); +} + +/** + * shared post-run retry loop used by every agent harness. + * + * checks the post-run gates (stop hook + dirty tree), and if either is + * failing, invokes `resume` to let the agent fix and push in the same turn. + * bails at `MAX_POST_RUN_RETRIES` attempts. the `canResume` predicate is + * consulted before each retry — harnesses that can't re-enter the session + * (e.g. claude without a sessionId) return false here. + * + * an optional `reflectionPrompt` fires exactly once, after the gates first + * observe a clean state. it's a one-shot nudge (e.g. "update learnings if + * relevant"), not a gate, so it does not consume the gate-retry budget. if + * the reflection turn dirties the tree, the loop picks that up on the next + * iteration via the normal dirty-tree gate. + * + * stop hook must pass for the run to succeed; persistent hook failures are + * surfaced as `AgentResult.error`. dirty-tree-only failures preserve prior + * behavior: they're logged but don't fail the run. + */ +export async function runPostRunRetryLoop(params: { + initialResult: R; + initialUsage: AgentUsage | undefined; + stopScript: string | null | undefined; + resume: (context: { prompt: string; previousResult: R }) => Promise; + canResume?: ((result: R) => boolean) | undefined; + reflectionPrompt?: string | undefined; +}): Promise { + let result = params.initialResult; + let aggregatedUsage = params.initialUsage; + let finalIssues: PostRunIssues = {}; + let gateResumeCount = 0; + let pendingReflection = params.reflectionPrompt; + + while (gateResumeCount < MAX_POST_RUN_RETRIES) { + if (!result.success) break; + const issues = await collectPostRunIssues({ stopScript: params.stopScript }); + finalIssues = issues; + + if (!hasPostRunIssues(issues)) { + // gates are clean. if a reflection prompt is pending, deliver it once + // and loop back to re-check — the reflection may have touched the tree. + if (!pendingReflection) break; + if (params.canResume && !params.canResume(result)) break; + log.info("» post-run reflection: nudging agent to update learnings if relevant"); + const preReflection = result; + const reflectionResult = await params.resume({ + prompt: pendingReflection, + previousResult: result, + }); + aggregatedUsage = mergeAgentUsage(aggregatedUsage, reflectionResult.usage); + pendingReflection = undefined; + if (!reflectionResult.success) { + // reflection is a best-effort nudge. its failure must not flip a + // successful run to failed — the gated work is already done. keep + // the pre-reflection result and exit without re-running the gates + // (which would risk a flaky false-positive hook failure right after + // it just passed). + log.warning( + `» reflection turn failed (${reflectionResult.error ?? "unknown error"}), preserving prior successful result` + ); + result = preReflection; + break; + } + // reflection replies are meta-asks ("done", "updated learnings with N + // bullets") — not a task summary. keep the pre-reflection output so + // the returned AgentResult still reflects what the run accomplished, + // while inheriting reflection-specific fields the harness needs for + // any subsequent gate retry (e.g. the new sessionId claude emits per + // --resume invocation). + // use `||` (not `??`) so an empty pre-reflection output falls through + // to the reflection's reply. runs that only emit MCP tool calls and no + // plain text leave result.output = "" — keeping "" would starve the + // fallback path in handleAgentResult of anything to show. + result = { + ...reflectionResult, + output: preReflection.output || reflectionResult.output, + }; + continue; + } + + // checks still ran even if we can't resume, so the failure gate below + // can still catch a persistent stop-hook failure. + if (params.canResume && !params.canResume(result)) { + log.info("» post-run retry skipped: cannot resume agent session"); + break; + } + + log.info(`» post-run retry (attempt ${gateResumeCount + 1}/${MAX_POST_RUN_RETRIES})`); + const prompt = buildPostRunPrompt(issues); + result = await params.resume({ prompt, previousResult: result }); + aggregatedUsage = mergeAgentUsage(aggregatedUsage, result.usage); + gateResumeCount++; + } + + // we exhausted retries without observing a clean state — finalIssues + // reflects pre-resume state, so re-check to see what the last resume + // actually did. when the subprocess failed we skip: its own error is more + // actionable than a stale "stop hook still failing" message. when the loop + // already observed a clean state we skip: re-running the hook risks flaky + // false-positive failures right after it just passed. + if (gateResumeCount > 0 && result.success && hasPostRunIssues(finalIssues)) { + finalIssues = await collectPostRunIssues({ stopScript: params.stopScript }); + } + + if (result.success && finalIssues.stopHook) { + const retryNote = + gateResumeCount > 0 + ? ` after ${gateResumeCount} retry ${gateResumeCount === 1 ? "attempt" : "attempts"}` + : ""; + return { + ...result, + success: false, + error: `stop hook failed${retryNote} (exit code ${finalIssues.stopHook.exitCode}): ${finalIssues.stopHook.output || "(no output)"}`, + usage: aggregatedUsage, + }; + } + + return { ...result, usage: aggregatedUsage }; +} diff --git a/agents/shared.ts b/agents/shared.ts index 2614366..3fec569 100644 --- a/agents/shared.ts +++ b/agents/shared.ts @@ -8,9 +8,13 @@ import type { TodoTracker } from "../utils/todoTracking.ts"; // maximum number of stderr lines to keep in the rolling buffer during agent execution export const MAX_STDERR_LINES = 20; -// ── post-run commit enforcement ───────────────────────────────────────────────── +// ── post-run retry loop ──────────────────────────────────────────────────────── -export const MAX_COMMIT_RETRIES = 3; +/** + * how many times the post-run loop may resume the agent to fix a dirty tree + * or a failing stop hook before giving up. + */ +export const MAX_POST_RUN_RETRIES = 3; export function getGitStatus(): string { try { @@ -23,7 +27,7 @@ export function getGitStatus(): string { } } -export function buildCommitPrompt(_agentId: AgentId, status: string): string { +export function buildCommitPrompt(status: string): string { return [ `UNCOMMITTED CHANGES — the working tree is dirty. push all changes to a pull request (new or existing). \`git status\` must be clean before you finish.`, "", @@ -33,6 +37,20 @@ export function buildCommitPrompt(_agentId: AgentId, status: string): string { ].join("\n"); } +export interface StopHookFailure { + exitCode: number; + output: string; +} + +export interface PostRunIssues { + stopHook?: StopHookFailure; + dirtyTree?: string; +} + +export function hasPostRunIssues(issues: PostRunIssues): boolean { + return issues.stopHook !== undefined || issues.dirtyTree !== undefined; +} + /** * token/cost usage data from a single agent run. * @@ -82,6 +100,12 @@ export interface AgentRunContext { tmpdir: string; instructions: ResolvedInstructions; todoTracker?: TodoTracker | undefined; + /** + * user-configured stop hook script. runs after the agent finishes each + * attempt; non-zero exit resumes the agent with the hook output as + * guidance. null when the repo has no stop hook configured. + */ + stopScript?: string | null | undefined; /** * called synchronously when the agent subprocess is killed for inner * activity timeout. lets main.ts tear down shared resources (MCP HTTP @@ -116,7 +140,7 @@ export function formatCostUsd(costUsd: number): string { * merge two AgentUsage snapshots into one running total. * * both agent harnesses invoke their runner multiple times per `run()` when the - * post-run dirty-tree loop kicks in (MAX_COMMIT_RETRIES). each invocation + * post-run retry loop kicks in (MAX_POST_RUN_RETRIES). each invocation * produces its own AgentUsage; we sum them so downstream callers (usage * summary, WorkflowRun persistence) see the whole session — not just the * final retry's slice. diff --git a/main.ts b/main.ts index c614653..775e6de 100644 --- a/main.ts +++ b/main.ts @@ -458,6 +458,7 @@ export async function main(): Promise { tmpdir, instructions, todoTracker, + stopScript: runContext.repoSettings.stopScript, onActivityTimeout: onInnerActivityTimeout, onToolUse: (event) => { const wasTracked = recordDiffReadFromToolUse({ diff --git a/test/changed-agents.sh b/test/changed-agents.sh index 28965a6..4cf6537 100755 --- a/test/changed-agents.sh +++ b/test/changed-agents.sh @@ -4,7 +4,7 @@ # outputs a JSON array of agent names to stdout. # # only agents whose harness file changed AND are exported from index.ts are included. -# shared.ts/index.ts and other non-harness action changes fall back to opencode as a canary. +# shared.ts/index.ts/postRun.ts and other non-harness action changes fall back to opencode as a canary. set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" @@ -39,7 +39,7 @@ has_non_agent_change=false while IFS= read -r file; do [[ -z "$file" ]] && continue case "$file" in - action/agents/shared.ts|action/agents/index.ts) + action/agents/shared.ts|action/agents/index.ts|action/agents/postRun.ts) has_non_agent_change=true ;; action/agents/*.ts) diff --git a/utils/runContext.ts b/utils/runContext.ts index 3dad7ff..94c9924 100644 --- a/utils/runContext.ts +++ b/utils/runContext.ts @@ -15,6 +15,7 @@ export interface RepoSettings { setupScript: string | null; postCheckoutScript: string | null; prepushScript: string | null; + stopScript: string | null; push: PushPermission; shell: ShellPermission; prApproveEnabled: boolean; @@ -37,6 +38,7 @@ const defaultSettings: RepoSettings = { setupScript: null, postCheckoutScript: null, prepushScript: null, + stopScript: null, push: "restricted", shell: "restricted", prApproveEnabled: false, @@ -106,6 +108,7 @@ export async function fetchRunContext(params: { setupScript: data.settings?.setupScript ?? null, postCheckoutScript: data.settings?.postCheckoutScript ?? null, prepushScript: data.settings?.prepushScript ?? null, + stopScript: data.settings?.stopScript ?? null, }, apiToken: data.apiToken, oss: data.oss ?? false,