From c6a757424cb930250dd01dda9cd754f5a98da255 Mon Sep 17 00:00:00 2001 From: David Blass Date: Mon, 4 May 2026 19:09:42 +0000 Subject: [PATCH] Stop hook + learnings reflection via post-run loop (#515) (#548) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add stop hook + learnings reflection to post-run loop (#515) stop hook (#515): repo-configured script that runs after the agent finishes. non-zero exit resumes the agent with the hook output as guidance; persistent failure (3 attempts) marks the run failed. the dirty-tree and stop-hook gates share a single retry loop so a fix + push happen in one turn. learnings reflection: per Colin, the learnings step baked into mode checklists rarely fires — the agent stays focused on the task and the meta-ask falls through. the post-run loop now delivers a dedicated one-shot --continue turn asking the agent to call update_learnings if relevant, nothing else competing for attention. reflection doesn't consume the gate-retry budget; if it dirties the tree, the next loop iteration catches it via the dirty-tree gate. plumbing: Repo.stopScript column + migration, zod schema, run-context api, AgentSettings UI. RepoSettings.stopScript threads through to AgentRunContext and into each agent harness. subprocess-dependent logic lives in action/agents/postRun.ts to keep action/agents/shared.ts lean — shared.ts is reachable from pullfrog/internal, and pulling node:child_process through it leaks into root tsc (which uses bundler resolution, not NodeNext). * fix: preserve successful run when reflection turn fails The post-run reflection turn (update_learnings nudge) is a best-effort one-shot; its failure must not flip a successful run to failed. Prior code overwrote `result` with the reflection's return value, so a model API error during reflection caused the whole run to be reported as failed even though the gated work had already completed cleanly. Now: save the pre-reflection result, and if reflection returns `success: false`, log a warning, restore the prior success, and exit without re-invoking the gates (re-running a freshly-green stop hook risks a flaky false-positive failure). Adds action/agents/postRun.test.ts covering the reflection path — previously uncovered. * fix: surface both stop-hook stdout and stderr to the agent The `(stderr || stdout)` heuristic in executeStopHook dropped stdout entirely whenever stderr had any content. Scripts that emit a benign warning to stderr and the actionable error to stdout (common for wrapper scripts) starved the agent of the information it needed to fix the issue. Now concatenate both streams (stderr first, stdout second, skipping empty ones) before truncation. This keeps stdout's tail — usually where summaries and totals live — intact under the 4096-char cap. * test: lock in the core post-run retry + reflection invariants PR #548's test plan ships four manual verification scenarios. Convert three to vitest coverage, catching regressions on the hottest code paths: - persistent stop hook failure exhausts MAX_POST_RUN_RETRIES and surfaces as AgentResult.error with both the retry count and the verbatim hook output (so the GitHub-comment rendering stays actionable). - every gate retry is fed the hook output as the resume prompt. - usage aggregates across the initial run plus every retry (billing relies on this). - reflection turn still fires when no stop hook is configured and the tree is clean. Manual item remaining is the full UI round-trip of the settings form, which is out of scope for unit tests. * test: cover executeStopHook soft-fail and truncation invariants Three paths the PR documents but previously had no regression gates: - timeout (SPAWN_TIMEOUT_CODE) and activity-timeout (SPAWN_ACTIVITY_TIMEOUT_CODE) must return null, not a failure. a hook that times out is an infra problem; retrying with an agent turn risks an infinite loop. - spawn errors (ENOENT from a typoed binary, etc.) take the same soft-fail path for the same reason. - oversize hook output is truncated to the last 4096 chars with a "truncated" marker, keeping the tail (where summaries live) and protecting the 65535-char GitHub-comment budget downstream. Regression targets — a refactor that accidentally surfaces an infra failure as a gate failure, or blows the comment budget, will now fail loudly in CI. * test: cover soft-fail, no-resume, and short-circuit invariants Three more documented behaviors that previously had no regression gates: - dirty-tree-only is a soft-fail: persistent uncommitted changes log and warn but DO NOT flip the run to failed. a regression that started surfacing this as AgentResult.error would break every run that leaves a test fixture untracked. - canResume=false + stop hook failure still surfaces the hook failure as AgentResult.error. the retry budget is zero so "N retry attempts" is correctly omitted from the message, but the run still reports WHY it failed rather than silently reporting success. - initial result with success=false short-circuits the loop: no gate checks, no reflection, no resume calls. the original agent error flows through verbatim for clean triage. Also reset mockedSpawn in beforeEach so test state doesn't leak between cases. * test: lock in the reflection-dirties-tree → dirty-tree-gate path The PR description claims: "if the reflection turn dirties the tree, the loop picks that up on the next iteration via the normal dirty-tree gate." There was no regression gate on this invariant. Without it, a refactor that moved the reflection out of the retry loop (e.g., into a one-shot post-loop call) would silently bypass the commit-before-you-finish contract whenever the agent misbehaves during reflection — uncommitted changes would ship as part of the run's "success" state. The test sequences three getGitStatus returns (clean → dirty → clean) and asserts two resume calls: REFLECTION first, then UNCOMMITTED CHANGES with the dirtying file in the prompt. * fix: preserve pre-reflection task output when reflection succeeds the reflection turn's reply ("done" or "updated learnings with N bullets") is a meta-ask, not a task summary. before this fix, result = reflectionResult clobbered the original task's output on the returned AgentResult, so downstream consumers (handleAgentResult's fallback path when toolState is empty, programmatic callers of main()) saw the reflection's trivial reply instead of the real summary. spread reflectionResult to inherit fields subsequent gate retries need (e.g. the new sessionId claude emits per --resume invocation), but keep the pre-reflection output verbatim. Co-Authored-By: Claude Opus 4.7 * fix: fall back to reflection's output when pre-reflection output is empty the prior fix used `??` which only fell through on null/undefined. runs that communicate exclusively through MCP tools (e.g. report_progress) and emit no plain text leave result.output = "", which `??` preserved as-is — dropping the reflection's reply and leaving handleAgentResult's fallback path with nothing to show. switch to `||` so empty-string pre-reflection output yields the reflection's output instead of ""; non-empty task output still wins as intended. Co-Authored-By: Claude Opus 4.7 * test: drop reflection-failure-skips-hook test (over-specified control flow) the test pinned the literal `break` in the post-reflection failure branch with stopScript=null, asserting only that getGitStatus was called once. that's not a behavior contract — a reasonable refactor (e.g. `continue` to re-check gates with explicit flake guards) would fail this test even though the new behavior would be fine. the "does not flip a successful run to failed" test already covers the only thing callers depend on. * test: drop low-value mock-driven tests from postRun - "fires the reflection turn when no stop hook is configured" — fully subsumed by the output-preservation test (asserts task output survives, which is only possible if reflection fired). - "uses stdout alone" / "uses stderr alone" — pin format trivia (`filter(Boolean).join`) that LLMs ignore. - "returns empty output (not undefined) when both streams are empty" — guards a TS-impossible case; every consumer uses `output || "(no output)"`. - "returns null on activity-timeout" — duplicate of the timeout test; same `return null` branch with a different constant. --------- Co-authored-by: Claude Opus 4.7 Co-authored-by: Colin McDonnell --- agents/claude.ts | 53 +++-- agents/opencode.ts | 42 ++-- agents/postRun.test.ts | 429 +++++++++++++++++++++++++++++++++++++++++ agents/postRun.ts | 262 +++++++++++++++++++++++++ agents/shared.ts | 32 ++- main.ts | 1 + test/changed-agents.sh | 4 +- utils/runContext.ts | 3 + 8 files changed, 766 insertions(+), 60 deletions(-) create mode 100644 agents/postRun.test.ts create mode 100644 agents/postRun.ts 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,