postrun: thread AgentRunContext through the retry loop instead of repackaging (#652)
* postrun: thread AgentRunContext through the retry loop instead of repackaging
drop the per-gate plumbing in `runPostRunRetryLoop`: the loop now receives
`ctx: AgentRunContext` whole and reads `ctx.stopScript` + `ctx.toolState.*`
directly. `getUnsubmittedReview` becomes a pure utility in postRun.ts
instead of a closure shipped over `AgentRunContext`. `AgentRunContext`
loses 4 fields that duplicated `toolState` (`summaryFilePath`,
`summarySeed`, `learningsFilePath`, `getUnsubmittedReview`) and gains
`toolState: ToolState`. both harness call sites collapse from 11 lines to
7; main.ts deletes the inline closure.
`ToolState` and friends move from `action/mcp/server.ts` to
`action/toolState.ts` so non-MCP code (agents, post-run loop) stops
importing run-state types from the MCP server module.
no behavior change. 503/503 tests green.
* toolState: relocate `CommentableLines` to break dep cycle with mcp/review
`action/toolState.ts` was importing `CommentableLines` from
`mcp/review.ts`, which pulled the entire MCP server compile graph (24
files) into any consumer of `ToolState` — including `cf-worker-indexing`
via the `pullfrog/internal` re-export chain through `utils/log.ts` →
`agents/shared.ts` → `toolState.ts`. that exposed a pre-existing TS
error in `mcp/issueEvents.ts` (octokit types resolve differently under
cf-worker's `moduleResolution: bundler`).
move `CommentableLines` (a small `{ RIGHT: Set<number>; LEFT: Set<number> }`
state-shape type) to `toolState.ts` where it's used; re-export from
`mcp/review.ts` for back-compat with test and call-site imports. cuts
cf-worker's mcp/ compile inclusion from 24 files back to 0.
* postRun: drop mock-heavy retry-loop tests; keep pure gate predicate
`runPostRunRetryLoop` and `executeStopHook` were covered by ~560 lines
of mock-heavy regression-gate tests that stubbed `spawn` / `getGitStatus`
and fabricated `AgentRunContext` to drive orchestration paths. per
AGENTS.md ("prefer no test over a mock-heavy test that only catches the
most obvious form of regression") and the empirical track record — the
one real production failure of this code path (#646) was a missing npm
release, not a logic bug a unit test could catch — the value-to-ceremony
ratio is poor. delete them.
keep only the pure predicate: `getUnsubmittedReview(toolState)` is a
decision function whose four input conditions have user-visible
consequences when wrong. 5 assertions, no mocks, no ctx fabrication.
488 tests still pass.
* toolState: import PrepResult from prep/types.ts, not the barrel
same dep-cycle class as the previous CommentableLines fix. importing
PrepResult from prep/index.ts pulled prep/installNodeDependencies.ts
into the Next.js production build's typecheck graph (via
pullfrog/internal → utils/log.ts → agents/shared.ts → toolState.ts →
prep/index.ts → installNodeDependencies.ts), and Next.js's stricter
NODE_ENV-required ProcessEnv shape rejected an existing
`env: { PATH: ... }` literal.
prep/types.ts is a leaf module with zero imports — re-routing the type
import severs the chain. Vercel preview deploy goes from Error → Ready;
preview-sync stops racing the deploy.
This commit is contained in:
Colin McDonnell
committed by
pullfrog[bot]
pullfrog[bot]
parent
dee13b160f
commit
8e36f76cfa
+35
-546
@@ -1,561 +1,50 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import { SPAWN_TIMEOUT_CODE, SpawnTimeoutError } from "../utils/subprocess.ts";
|
||||
import type { AgentResult } from "./shared.ts";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import type { ToolState } from "../toolState.ts";
|
||||
import { getUnsubmittedReview } from "./postRun.ts";
|
||||
|
||||
vi.mock("./shared.ts", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("./shared.ts")>();
|
||||
function makeToolState(overrides: Partial<ToolState> = {}): ToolState {
|
||||
return {
|
||||
...actual,
|
||||
getGitStatus: vi.fn(() => ""),
|
||||
progressComment: undefined,
|
||||
hadProgressComment: true,
|
||||
backgroundProcesses: new Map(),
|
||||
usageEntries: [],
|
||||
...overrides,
|
||||
};
|
||||
});
|
||||
}
|
||||
|
||||
vi.mock("../utils/subprocess.ts", async (importOriginal) => {
|
||||
const actual = await importOriginal<typeof import("../utils/subprocess.ts")>();
|
||||
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> = {}): AgentResult => ({
|
||||
success: true,
|
||||
output: "ok",
|
||||
...overrides,
|
||||
});
|
||||
|
||||
describe("runPostRunRetryLoop — reflection turn", () => {
|
||||
beforeEach(() => {
|
||||
mockedGetGitStatus.mockReset();
|
||||
mockedGetGitStatus.mockReturnValue("");
|
||||
mockedSpawn.mockReset();
|
||||
describe("getUnsubmittedReview", () => {
|
||||
it("returns null when mode is not a review mode", () => {
|
||||
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Build" }))).toBeNull();
|
||||
expect(getUnsubmittedReview(makeToolState())).toBeNull();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
it("returns null when a review was already submitted", () => {
|
||||
expect(
|
||||
getUnsubmittedReview(
|
||||
makeToolState({
|
||||
selectedMode: "Review",
|
||||
review: { id: 1, nodeId: "n", reviewedSha: undefined },
|
||||
})
|
||||
)
|
||||
).toBeNull();
|
||||
});
|
||||
|
||||
it("does not flip a successful run to failed when reflection returns success:false", async () => {
|
||||
// the reflection turn is a best-effort nudge (edit the learnings
|
||||
// tmpfile). 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<AgentResult>>()
|
||||
.mockResolvedValue({ success: false, error: "model API transient failure" });
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: initial,
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
resume,
|
||||
reflectionPrompt: "REFLECTION: edit learnings file 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("returns null when report_progress wrote a final summary", () => {
|
||||
expect(
|
||||
getUnsubmittedReview(makeToolState({ selectedMode: "Review", finalSummaryWritten: true }))
|
||||
).toBeNull();
|
||||
});
|
||||
|
||||
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<AgentResult>>()
|
||||
.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("returns null when there is no progress comment to anchor the failure to", () => {
|
||||
expect(
|
||||
getUnsubmittedReview(makeToolState({ selectedMode: "Review", hadProgressComment: false }))
|
||||
).toBeNull();
|
||||
});
|
||||
|
||||
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<AgentResult>>()
|
||||
.mockResolvedValue(successResult({ output: "done" }));
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: initial,
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
resume,
|
||||
reflectionPrompt: "REFLECTION: consider editing 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<AgentResult>>()
|
||||
.mockResolvedValue(successResult({ output: "done" }));
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: initial,
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
resume,
|
||||
reflectionPrompt: "REFLECTION: consider editing 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<AgentResult>>()
|
||||
.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<AgentResult>>()
|
||||
.mockResolvedValue(successResult({ output: "resumed" }));
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: initial,
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
resume,
|
||||
reflectionPrompt: "REFLECTION: consider editing 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<AgentResult>>()
|
||||
.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<AgentResult>>()
|
||||
.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<AgentResult>>()
|
||||
.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<AgentResult>>()
|
||||
.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("nudges the agent when Review mode finished without submitting (no report_progress exit offered)", async () => {
|
||||
// simulate the canonical bug: the agent picked Review mode, talked itself
|
||||
// out of acting, and stopped without calling create_pull_request_review.
|
||||
// the gate should fire a single resume that ONLY offers
|
||||
// create_pull_request_review (Review mode's contract forbids
|
||||
// report_progress as an exit). once the resume flips the toolState
|
||||
// (modeled here by the closure returning null on the second call), the
|
||||
// loop should exit cleanly with success=true.
|
||||
let submitted = false;
|
||||
const resume = vi
|
||||
.fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise<AgentResult>>()
|
||||
.mockImplementation(async () => {
|
||||
submitted = true;
|
||||
return successResult({ output: "review submitted" });
|
||||
});
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: successResult({ output: "analysis turn" }),
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
getUnsubmittedReview: () => (submitted ? null : "Review"),
|
||||
resume,
|
||||
});
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(resume).toHaveBeenCalledTimes(1);
|
||||
const prompt = resume.mock.calls[0]?.[0].prompt ?? "";
|
||||
expect(prompt).toContain("MISSING REVIEW OUTPUT");
|
||||
expect(prompt).toContain("Review mode");
|
||||
expect(prompt).toContain("create_pull_request_review");
|
||||
// Review mode contract: nudge must NOT offer report_progress as an exit
|
||||
// (would contradict modes.ts step 5 "ALWAYS submit ... do NOT call
|
||||
// report_progress").
|
||||
expect(prompt).not.toContain("report_progress");
|
||||
});
|
||||
|
||||
it("offers `report_progress` in the IncrementalReview nudge for the trivial-skip path", async () => {
|
||||
// IncrementalReview's mode prompt step 7 has a legitimate "no new
|
||||
// issues, non-substantive changes only → report_progress" exit. the
|
||||
// nudge must offer it so the agent can satisfy the gate without
|
||||
// submitting a vacuous re-review.
|
||||
let satisfied = false;
|
||||
const resume = vi
|
||||
.fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise<AgentResult>>()
|
||||
.mockImplementation(async () => {
|
||||
satisfied = true;
|
||||
return successResult({ output: "no review warranted, reported" });
|
||||
});
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: successResult({ output: "analysis turn" }),
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
getUnsubmittedReview: () => (satisfied ? null : "IncrementalReview"),
|
||||
resume,
|
||||
});
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
const prompt = resume.mock.calls[0]?.[0].prompt ?? "";
|
||||
expect(prompt).toContain("IncrementalReview mode");
|
||||
expect(prompt).toContain("create_pull_request_review");
|
||||
expect(prompt).toContain("report_progress");
|
||||
});
|
||||
|
||||
it("hard-fails IncrementalReview after MAX_POST_RUN_RETRIES with mode-appropriate error copy", async () => {
|
||||
// parallel to the persistent-stop-hook test: if the agent keeps stopping
|
||||
// without producing a review, the run must fail loudly with a clear error
|
||||
// so the user knows something went wrong instead of seeing a deleted
|
||||
// progress comment and silence. IncrementalReview's error string lists
|
||||
// both exits the gate accepts.
|
||||
const resume = vi
|
||||
.fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise<AgentResult>>()
|
||||
.mockResolvedValue(successResult({ output: "still didn't submit" }));
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: successResult({ output: "analysis turn" }),
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
getUnsubmittedReview: () => "IncrementalReview",
|
||||
resume,
|
||||
});
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error).toContain("IncrementalReview");
|
||||
expect(result.error).toContain("create_pull_request_review");
|
||||
expect(result.error).toContain("report_progress");
|
||||
expect(result.error).toContain("3 retry attempts");
|
||||
expect(resume).toHaveBeenCalledTimes(3);
|
||||
});
|
||||
|
||||
it("hard-fails Review with a mode-appropriate error string (no `report_progress`)", async () => {
|
||||
// Review mode's contract is "always submit a review" (modes.ts step 5).
|
||||
// the terminal error must mirror the nudge copy: only
|
||||
// create_pull_request_review is named — listing report_progress would
|
||||
// contradict the mode prompt and confuse triage.
|
||||
const resume = vi
|
||||
.fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise<AgentResult>>()
|
||||
.mockResolvedValue(successResult({ output: "still didn't submit" }));
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: successResult({ output: "analysis turn" }),
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
getUnsubmittedReview: () => "Review",
|
||||
resume,
|
||||
});
|
||||
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error).toContain("Review mode");
|
||||
expect(result.error).toContain("create_pull_request_review");
|
||||
expect(result.error).not.toContain("report_progress");
|
||||
});
|
||||
|
||||
it("does not fire the unsubmitted-review gate when review was submitted", async () => {
|
||||
// negative case: the closure returns null (review or progress was
|
||||
// recorded), the gate must stay silent regardless of mode.
|
||||
const resume = vi
|
||||
.fn<(ctx: { prompt: string; previousResult: AgentResult }) => Promise<AgentResult>>()
|
||||
.mockResolvedValue(successResult());
|
||||
|
||||
const result = await runPostRunRetryLoop({
|
||||
initialResult: successResult({ output: "review submitted inline" }),
|
||||
initialUsage: undefined,
|
||||
stopScript: null,
|
||||
getUnsubmittedReview: () => null,
|
||||
resume,
|
||||
});
|
||||
|
||||
expect(result.success).toBe(true);
|
||||
expect(resume).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<AgentResult>>()
|
||||
.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)
|
||||
it("returns the selected mode when the gate should fire", () => {
|
||||
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Review" }))).toBe("Review");
|
||||
expect(getUnsubmittedReview(makeToolState({ selectedMode: "IncrementalReview" }))).toBe(
|
||||
"IncrementalReview"
|
||||
);
|
||||
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);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user