diff --git a/agents/claude.ts b/agents/claude.ts index af8baba..4db59e2 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -805,14 +805,11 @@ export const claude = agent({ // the run). the reflection prompt fires once after gates go clean, as a // dedicated turn that nudges the agent to persist learnings. return runPostRunRetryLoop({ + ctx, initialResult: result, initialUsage: result.usage, - stopScript: ctx.stopScript, - summaryFilePath: ctx.summaryFilePath, - summarySeed: ctx.summarySeed, - getUnsubmittedReview: ctx.getUnsubmittedReview, - reflectionPrompt: ctx.learningsFilePath - ? buildLearningsReflectionPrompt(ctx.learningsFilePath) + reflectionPrompt: ctx.toolState.learningsFilePath + ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath) : undefined, canResume: (r) => Boolean(r.sessionId), resume: async (c) => { diff --git a/agents/opencode.ts b/agents/opencode.ts index e0bd2e2..e0da910 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -1145,14 +1145,11 @@ export const opencode = agent({ // the reflection prompt fires once after gates go clean, as a dedicated // turn that nudges the agent to persist learnings. return runPostRunRetryLoop({ + ctx, initialResult: result, initialUsage: result.usage, - stopScript: ctx.stopScript, - summaryFilePath: ctx.summaryFilePath, - summarySeed: ctx.summarySeed, - getUnsubmittedReview: ctx.getUnsubmittedReview, - reflectionPrompt: ctx.learningsFilePath - ? buildLearningsReflectionPrompt(ctx.learningsFilePath) + reflectionPrompt: ctx.toolState.learningsFilePath + ? buildLearningsReflectionPrompt(ctx.toolState.learningsFilePath) : undefined, resume: async (c) => runOpenCode({ diff --git a/agents/postRun.test.ts b/agents/postRun.test.ts index e63dbf4..1f271d1 100644 --- a/agents/postRun.test.ts +++ b/agents/postRun.test.ts @@ -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(); +function makeToolState(overrides: Partial = {}): 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(); - 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(); +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>() - .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>() - .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>() - .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>() - .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>() - .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 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>() - .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("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>() - .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>() - .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>() - .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>() - .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>() - .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>() - .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); }); }); diff --git a/agents/postRun.ts b/agents/postRun.ts index 099715e..2373e6a 100644 --- a/agents/postRun.ts +++ b/agents/postRun.ts @@ -1,5 +1,6 @@ import { readFile } from "node:fs/promises"; import { LIFECYCLE_HOOK_TIMEOUT_MS } from "../lifecycle.ts"; +import type { ToolState } from "../toolState.ts"; import { log } from "../utils/cli.ts"; import { SPAWN_ACTIVITY_TIMEOUT_CODE, @@ -9,6 +10,7 @@ import { } from "../utils/subprocess.ts"; import { type AgentResult, + type AgentRunContext, type AgentUsage, buildCommitPrompt, getGitStatus, @@ -19,6 +21,23 @@ import { type StopHookFailure, } from "./shared.ts"; +/** + * derive "agent picked a review mode but never produced visible output" from + * the literal facts on `toolState`. returns the selected mode when the gate + * should fire, `null` otherwise — pure read, no side effects, safe to invoke + * after every agent attempt. + * + * the gate is anchored to `hadProgressComment` so silent runs (non-issue + * events, dispatcher skipped seeding) don't fire a nudge there's no UI for. + */ +export function getUnsubmittedReview(toolState: ToolState): "Review" | "IncrementalReview" | null { + const mode = toolState.selectedMode; + if (mode !== "Review" && mode !== "IncrementalReview") return null; + if (toolState.review || toolState.finalSummaryWritten) return null; + if (!toolState.hadProgressComment) return null; + return mode; +} + /** * hook output can flow into two size-sensitive places: the LLM resume prompt * (context window) and AgentResult.error (surfaced in GitHub comments capped @@ -144,37 +163,34 @@ export function buildUnsubmittedReviewPrompt(mode: "Review" | "IncrementalReview /** * check the post-run gates: did the stop hook pass, is the working tree * clean, and (when applicable) did the agent touch the rolling PR summary - * snapshot? returns everything that still needs nudging so the caller can - * render a single combined resume prompt. + * snapshot or produce review output? returns everything that still needs + * nudging so the caller can render a single combined resume prompt. * - * the summary-stale check is skipped when `summaryFilePath` / `summarySeed` - * are not provided; this is the common case (non-PR runs, runs where the - * dispatcher didn't request snapshot generation, runs where the seed step - * failed). loop callers also pass these as undefined after the agent has - * already been nudged once, to avoid burning the retry budget on a soft - * non-blocking gate. + * reads run state directly off `ctx.toolState` so each invocation sees the + * latest mutations from MCP tool calls. `skipSummaryStale` lets the loop + * suppress the summary-stale check after the one-shot nudge has been + * delivered (re-firing it would burn the retry budget on a soft gate the + * agent has already decided not to act on). */ -export async function collectPostRunIssues(params: { - stopScript: string | null | undefined; - summaryFilePath?: string | undefined; - summarySeed?: string | undefined; - getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; -}): Promise { +export async function collectPostRunIssues( + ctx: AgentRunContext, + options: { skipSummaryStale?: boolean } = {} +): Promise { const issues: PostRunIssues = {}; - if (params.stopScript) { - const failure = await executeStopHook(params.stopScript); + if (ctx.stopScript) { + const failure = await executeStopHook(ctx.stopScript); if (failure) issues.stopHook = failure; } const status = getGitStatus(); if (status) issues.dirtyTree = status; - if (params.summaryFilePath && params.summarySeed !== undefined) { - const stale = await isSummaryUnchanged(params.summaryFilePath, params.summarySeed); - if (stale) issues.summaryStale = { filePath: params.summaryFilePath }; - } - if (params.getUnsubmittedReview) { - const mode = params.getUnsubmittedReview(); - if (mode) issues.unsubmittedReview = mode; + const summaryFilePath = ctx.toolState.summaryFilePath; + const summarySeed = ctx.toolState.summarySeed; + if (!options.skipSummaryStale && summaryFilePath && summarySeed !== undefined) { + const stale = await isSummaryUnchanged(summaryFilePath, summarySeed); + if (stale) issues.summaryStale = { filePath: summaryFilePath }; } + const unsubmittedMode = getUnsubmittedReview(ctx.toolState); + if (unsubmittedMode) issues.unsubmittedReview = unsubmittedMode; return issues; } @@ -242,19 +258,9 @@ export function buildLearningsReflectionPrompt(filePath: string): string { * behavior: they're logged but don't fail the run. */ export async function runPostRunRetryLoop(params: { + ctx: AgentRunContext; initialResult: R; initialUsage: AgentUsage | undefined; - stopScript: string | null | undefined; - /** absolute path to the seeded PR summary file. when set together with - * `summarySeed`, the loop checks after each agent attempt whether the - * file has been edited; if not, it nudges the agent ONCE via a resume - * turn (subsequent iterations skip the check so we don't keep burning - * retries on a soft gate when the agent has decided no edit is warranted). */ - summaryFilePath?: string | undefined; - /** exact bytes of the seeded summary file used for the unchanged-check. */ - summarySeed?: string | undefined; - /** see {@link AgentRunContext.getUnsubmittedReview}. */ - getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; resume: (context: { prompt: string; previousResult: R }) => Promise; canResume?: ((result: R) => boolean) | undefined; reflectionPrompt?: string | undefined; @@ -264,20 +270,16 @@ export async function runPostRunRetryLoop(params: { let finalIssues: PostRunIssues = {}; let gateResumeCount = 0; let pendingReflection = params.reflectionPrompt; - // nudge for an untouched summary file fires AT MOST ONCE per run. after - // we've delivered the prompt, subsequent gate checks pass undefined so - // the loop doesn't keep flagging the same condition — the agent may have - // legitimately decided no edit is warranted, and re-prompting would - // burn the retry budget without adding signal. + // nudge for an untouched summary file fires AT MOST ONCE per run. once + // delivered, subsequent collectPostRunIssues calls skip the check — the + // agent may have legitimately decided no edit is warranted, and + // re-prompting would burn the retry budget without adding signal. let summaryStaleNudged = false; while (gateResumeCount < MAX_POST_RUN_RETRIES) { if (!result.success) break; - const issues = await collectPostRunIssues({ - stopScript: params.stopScript, - summaryFilePath: summaryStaleNudged ? undefined : params.summaryFilePath, - summarySeed: summaryStaleNudged ? undefined : params.summarySeed, - getUnsubmittedReview: params.getUnsubmittedReview, + const issues = await collectPostRunIssues(params.ctx, { + skipSummaryStale: summaryStaleNudged, }); if (issues.summaryStale) summaryStaleNudged = true; finalIssues = issues; @@ -367,10 +369,7 @@ export async function runPostRunRetryLoop(params: { // NOT re-checked here: we already delivered the one-shot nudge, and // a still-unchanged file at this point is the agent's deliberate // choice. - finalIssues = await collectPostRunIssues({ - stopScript: params.stopScript, - getUnsubmittedReview: params.getUnsubmittedReview, - }); + finalIssues = await collectPostRunIssues(params.ctx, { skipSummaryStale: true }); } if (result.success && finalIssues.stopHook) { diff --git a/agents/shared.ts b/agents/shared.ts index 8435ace..4d13165 100644 --- a/agents/shared.ts +++ b/agents/shared.ts @@ -1,5 +1,6 @@ import { execFileSync } from "node:child_process"; import type { AgentId } from "../external.ts"; +import type { ToolState } from "../toolState.ts"; import { log } from "../utils/cli.ts"; import type { ResolvedInstructions } from "../utils/instructions.ts"; import type { ResolvedPayload } from "../utils/payload.ts"; @@ -59,9 +60,10 @@ export interface PostRunIssues { * over toolState shows neither a `create_pull_request_review` submission * nor a final `report_progress` write happened. derived inline from * `toolState.selectedMode` + `toolState.review` + `toolState.finalSummaryWritten` - * — no parallel toolState flag is stored. carries the mode name so the - * resume prompt can reference it. handled like `stopHook`: nudge via - * resume, hard-fail if still unsatisfied after `MAX_POST_RUN_RETRIES`. + * via {@link getUnsubmittedReview} — no parallel toolState flag is stored. + * carries the mode name so the resume prompt can reference it. handled like + * `stopHook`: nudge via resume, hard-fail if still unsatisfied after + * `MAX_POST_RUN_RETRIES`. */ unsubmittedReview?: "Review" | "IncrementalReview"; } @@ -115,7 +117,14 @@ export interface AgentResult { } /** - * Minimal context passed to agent.run() + * Context passed to agent.run() and threaded through the post-run loop. + * + * design rule: this is the single object that flows through the harness and + * downstream utilities by reference. derived predicates (e.g. + * `getUnsubmittedReview`), tmpfile paths, and seed bytes live on + * `toolState` — read them at the call site, do not duplicate them onto this + * interface. utilities that need run state should accept `ctx` whole, not + * destructure a narrow subset. */ export interface AgentRunContext { payload: ResolvedPayload; @@ -131,26 +140,13 @@ export interface AgentRunContext { */ stopScript?: string | null | undefined; /** - * absolute path to the rolling PR summary tmpfile, when one was seeded - * for this run (Review / IncrementalReview / pr-summary Task). enables - * a post-run sanity nudge that prompts the agent if the file is still - * byte-identical to its seed. + * mutable per-run state shared with the MCP server (by reference). post-run + * gates read fresh values from it after each agent attempt — `summaryFilePath`, + * `summarySeed`, `selectedMode`, `review`, `finalSummaryWritten`, + * `hadProgressComment` are all consulted by `collectPostRunIssues`. see + * `action/toolState.ts` for the literal-state design rule. */ - summaryFilePath?: string | undefined; - /** - * exact bytes of the seeded summary file. compared against the current - * file content after each agent attempt to detect "agent forgot to edit - * the summary" — particularly common with smaller models that lose - * track of multi-step instructions. - */ - summarySeed?: string | undefined; - /** - * absolute path to the rolling repo-level learnings tmpfile. seeded for - * every run from `Repo.learnings`. used by the post-run reflection turn - * so the prompt can point the agent at a concrete path to edit; the - * file's content is read back and persisted by main.ts after the run. - */ - learningsFilePath?: string | undefined; + toolState: ToolState; /** * called synchronously when the agent subprocess is killed for inner * activity timeout. lets main.ts tear down shared resources (MCP HTTP @@ -158,15 +154,6 @@ export interface AgentRunContext { */ onActivityTimeout?: (() => void) | undefined; onToolUse?: ((event: AgentToolUseEvent) => void) | undefined; - /** - * post-run check derived from toolState: returns the selected mode when - * the agent picked Review / IncrementalReview but neither submitted a - * review nor wrote a final progress comment, otherwise `null`. main.ts - * supplies the closure so the agent harness has no direct toolState - * dependency; the closure fires synchronously after each agent attempt - * so it sees the latest mutations from any MCP tool calls. - */ - getUnsubmittedReview?: (() => "Review" | "IncrementalReview" | null) | undefined; } export interface Agent { diff --git a/main.ts b/main.ts index ab2f341..e913314 100644 --- a/main.ts +++ b/main.ts @@ -6,13 +6,9 @@ import { join } from "node:path"; import * as core from "@actions/core"; import { deleteProgressComment, reportProgress } from "./mcp/comment.ts"; import { startInstallation } from "./mcp/dependencies.ts"; -import { - initToolState, - startMcpHttpServer, - type ToolContext, - type ToolState, -} from "./mcp/server.ts"; +import { startMcpHttpServer, type ToolContext } from "./mcp/server.ts"; import { computeModes } from "./modes.ts"; +import { initToolState, type ToolState } from "./toolState.ts"; import { type ActivityTimeout, createProcessOutputActivityTimeout, @@ -929,22 +925,7 @@ export async function main(): Promise { instructions, todoTracker, stopScript: runContext.repoSettings.stopScript, - summaryFilePath: toolState.summaryFilePath, - summarySeed: toolState.summarySeed, - learningsFilePath: toolState.learningsFilePath, - // post-run gate: derive "review mode finished without producing - // anything visible" inline from toolState. no parallel toolState flag — - // the absence of `review` and `finalSummaryWritten` is the signal. - // skipped when there's no progress comment to anchor the failure to - // (e.g. silent runs / non-issue events) so the gate doesn't fire - // on runs where there's nothing to display anyway. - getUnsubmittedReview: () => { - const mode = toolState.selectedMode; - if (mode !== "Review" && mode !== "IncrementalReview") return null; - if (toolState.review || toolState.finalSummaryWritten) return null; - if (!toolState.hadProgressComment) return null; - return mode; - }, + toolState, onActivityTimeout: onInnerActivityTimeout, onToolUse: (event) => { const wasTracked = recordDiffReadFromToolUse({ diff --git a/mcp/git.ts b/mcp/git.ts index 891ec54..69af32f 100644 --- a/mcp/git.ts +++ b/mcp/git.ts @@ -1,10 +1,11 @@ import { regex } from "arkregex"; import { type } from "arktype"; +import type { StoredPushDest } from "../toolState.ts"; import { log } from "../utils/cli.ts"; import { $git } from "../utils/gitAuth.ts"; import { executeLifecycleHook } from "../utils/lifecycle.ts"; import { $ } from "../utils/shell.ts"; -import type { StoredPushDest, ToolContext } from "./server.ts"; +import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; type PushDestination = { diff --git a/mcp/review.ts b/mcp/review.ts index 7ff56b1..b426bf0 100644 --- a/mcp/review.ts +++ b/mcp/review.ts @@ -1,6 +1,7 @@ import type { RestEndpointMethodTypes } from "@octokit/rest"; import { type } from "arktype"; import { formatMcpToolRef } from "../external.ts"; +import type { CommentableLines } from "../toolState.ts"; import { getApiUrl } from "../utils/apiUrl.ts"; import { buildPullfrogFooter } from "../utils/buildPullfrogFooter.ts"; import { log } from "../utils/cli.ts"; @@ -16,6 +17,8 @@ import { deleteProgressComment } from "./comment.ts"; import type { ToolContext } from "./server.ts"; import { execute, tool } from "./shared.ts"; +export type { CommentableLines }; + function getHttpStatus(err: unknown): number | undefined { if (typeof err !== "object" || err === null) return undefined; const status = (err as Record).status; @@ -46,7 +49,6 @@ export function isTransientReviewError(err: unknown): boolean { export const TRANSIENT_REVIEW_RETRY_DELAYS_MS = [1_000, 3_000]; type PullFile = RestEndpointMethodTypes["pulls"]["listFiles"]["response"]["data"][number]; -export type CommentableLines = { RIGHT: Set; LEFT: Set }; /** * parse a PR file's patch to determine which line numbers on each side are diff --git a/mcp/server.ts b/mcp/server.ts index bc75cf0..26705d9 100644 --- a/mcp/server.ts +++ b/mcp/server.ts @@ -3,23 +3,14 @@ import "./arkConfig.ts"; import { createServer } from "node:net"; import { setTimeout as sleep } from "node:timers/promises"; import { FastMCP, type Tool } from "fastmcp"; -import type { AgentUsage } from "../agents/index.ts"; import { type AgentId, pullfrogMcpName } from "../external.ts"; import type { Mode } from "../modes.ts"; -import type { PrepResult } from "../prep/index.ts"; +import type { ToolState } from "../toolState.ts"; import { closeBrowserDaemon } from "../utils/browser.ts"; -import { log } from "../utils/cli.ts"; -import type { DiffCoverageState } from "../utils/diffCoverage.ts"; import type { OctokitWithPlugins } from "../utils/github.ts"; import type { ResolvedPayload } from "../utils/payload.ts"; -import { - type ProgressComment, - type ProgressCommentType, - parseProgressComment, -} from "../utils/progressComment.ts"; import type { AccountPlan } from "../utils/runContext.ts"; import type { RunContextData } from "../utils/runContextData.ts"; -import type { TodoTracker } from "../utils/todoTracking.ts"; import { CheckoutPrTool } from "./checkout.ts"; import { GetCheckSuiteLogsTool } from "./checkSuite.ts"; import { @@ -42,7 +33,6 @@ import { AddLabelsTool } from "./labels.ts"; import { SetOutputTool } from "./output.ts"; import { CreatePullRequestTool, UpdatePullRequestBodyTool } from "./pr.ts"; import { PullRequestInfoTool } from "./prInfo.ts"; -import type { CommentableLines } from "./review.ts"; import { CreatePullRequestReviewTool } from "./review.ts"; import { GetReviewCommentsTool, @@ -54,166 +44,6 @@ import { addTools } from "./shared.ts"; import { KillBackgroundTool, ShellTool } from "./shell.ts"; import { UploadFileTool } from "./upload.ts"; -export type BackgroundProcess = { - pid: number; - outputPath: string; - pidPath: string; -}; - -export type BrowserDaemon = { binDir: string; error?: never } | { binDir?: never; error: string }; - -export type StoredPushDest = { - remoteName: string; - remoteBranch: string; - localBranch: string; -}; - -/** - * mutable per-run record of facts that occurred during execution. shared - * between the action process and the MCP server (one process — toolState is - * just a JS object passed by reference into both surfaces). - * - * design rule: ToolState is LITERAL. each field records a thing that - * happened — `review` is set when `create_pull_request_review` succeeded, - * `finalSummaryWritten` flips when `report_progress` wrote a non-plan body, - * `selectedMode` is set when `select_mode` was called. fields should never - * encode the absence of an event ("unsubmittedReview", "missingArtifact"), - * speculative state, or values derived from other fields. - * - * any predicate the rest of the code needs ("the agent picked review mode but - * never produced a review or progress write") is computed inline at the call - * site, not stored. derived state in this struct invariably drifts from the - * literal fields under refactors and is the wrong layer for the check. - * - * write narrowly: prefer adding state inside the tool that mutates it (e.g. - * `create_pull_request_review` populates `toolState.review`) and reading - * narrowly elsewhere. don't introduce flags from main.ts that mirror what an - * MCP tool already records. - */ -export interface ToolState { - // where we're allowed to push - base repo initially, fork URL for fork PRs - // set by setupGit, updated by checkout_pr. always set before push validation. - pushUrl?: string; - // push destination set by checkout_pr - used as primary source in push_branch - // because git config reads can fail in certain environments - pushDest?: StoredPushDest; - // issue or PR number (same number space in GitHub) - issueNumber?: number; - // PR HEAD sha at checkout time — used to detect new commits pushed during a review - checkoutSha?: string; - // commentable lines per file at checkoutSha — captured during checkout_pr so - // review-time inline-comment validation matches the diff GitHub will anchor - // to (commit_id=checkoutSha). without this, a PR update between checkout and - // review would make listFiles (latest HEAD) disagree with the anchor, - // silently dropping valid comments or letting invalid ones through. - // - // commentableLinesPullNumber records WHICH PR this snapshot belongs to. if - // the agent checks out PR B and then reviews PR A in the same session, the - // cached snapshot for B would silently mis-validate A's comments — keying - // by PR number forces a re-fetch when the target changes. - // - // commentableLinesCheckoutSha pins the snapshot to the SHA it was built - // against. if a second checkout_pr for the SAME PR bumps checkoutSha but - // fails before repopulating the cache (e.g., listFiles rate-limits), the - // stale snapshot would silently mis-validate comments against the new SHA. - // comparing both fields forces a re-fetch when either moves. - commentableLinesByFile?: Map; - commentableLinesPullNumber?: number; - commentableLinesCheckoutSha?: string | undefined; - // SHA to diff incrementally against — set from event payload on first checkout, - // then from checkoutSha when review.ts detects new commits mid-review - beforeSha?: string; - selectedMode?: string; - backgroundProcesses: Map; - browserDaemon?: BrowserDaemon | undefined; - review?: { - id: number; - nodeId: string; - reviewedSha: string | undefined; - }; - // dedupe key: parent review comment_id → most-recent reply written this - // session by reply_to_review_comment. used by duplicateReplyDecision to - // skip identical-body re-emissions of the same call (PR #610 root cause). - // body-keyed (not just id-keyed) so legitimate follow-up replies with - // different content still go through. - reviewReplies?: Map< - number, - { commentId: number; url: string | undefined; bodyWithFooter: string } - >; - dependencyInstallation?: { - status: "not_started" | "in_progress" | "completed" | "failed"; - promise: Promise | undefined; - results: PrepResult[] | undefined; - }; - // undefined = no comment yet, object = active comment, null = deliberately deleted - progressComment: ProgressComment | null | undefined; - // immutable snapshot: true if a progress comment was pre-created at init time. - // survives deleteProgressComment so handleAgentResult can still detect "expected but never reported". - hadProgressComment: boolean; - lastProgressBody?: string; - wasUpdated?: boolean; - // set after a non-plan report_progress successfully writes the final summary. - // decoupled from todoTracker.enabled so cleanup detection survives API failures. - finalSummaryWritten?: boolean; - // set by select_mode when Plan + issue_number and plan-comment API returns existing plan (for report_progress target_plan_comment) - existingPlanCommentId?: number; - previousPlanBody?: string; - // absolute path to the PR summary markdown file the agent edits in place. - // seeded by main.ts before the agent starts when payload.generateSummary is set; - // read back at end-of-run to persist to DB. - summaryFilePath?: string; - // exact bytes of the seeded snapshot file at run start. compared against - // the file content at end-of-run to detect "agent never touched it" — in - // that case persistSummary skips the DB write (saving the seed verbatim - // would either re-write what the DB already has, on incremental runs, or - // serialize the placeholder scaffold, on first runs). - summarySeed?: string; - // set to true after persistSummary completes once. prevents the error-path - // call (which exists so a successful agent edit before a crash still gets - // persisted) from redundantly re-running the DB PATCH on the - // success-then-late-throw path. - summaryPersistAttempted?: boolean; - // absolute path to the rolling repo-level learnings markdown file the - // agent reads at startup and may edit at end-of-run. seeded by main.ts - // for every run from `Repo.learnings` (empty file when no learnings - // exist yet); read back at end-of-run to persist any edits. - learningsFilePath?: string; - // exact bytes of the seeded learnings file at run start. compared - // against the file content at end-of-run to detect "agent never touched - // it" — in that case persistLearnings skips the DB PATCH (saving the - // identical content would be a no-op write that wastes a LearningsRevision - // row and the API round-trip). - learningsSeed?: string; - // mirror of `summaryPersistAttempted` for the learnings tmpfile — guards - // the error-path / exit-signal callers from a redundant second PATCH - // after the success path already persisted. - learningsPersistAttempted?: boolean; - output?: string; - usageEntries: AgentUsage[]; - model?: string | undefined; - todoTracker?: TodoTracker | undefined; - diffCoverage?: DiffCoverageState | undefined; -} - -interface InitToolStateParams { - progressComment: { id: string; type: ProgressCommentType } | undefined; -} - -export function initToolState(params: InitToolStateParams): ToolState { - const resolved = parseProgressComment(params.progressComment); - - if (resolved) { - log.info(`» using pre-created progress comment: ${resolved.id} (${resolved.type})`); - } - - return { - progressComment: resolved, - hadProgressComment: !!resolved, - backgroundProcesses: new Map(), - usageEntries: [], - }; -} - export interface ToolContext { agentId: AgentId; repo: RunContextData["repo"]; diff --git a/toolState.ts b/toolState.ts new file mode 100644 index 0000000..083c6cb --- /dev/null +++ b/toolState.ts @@ -0,0 +1,179 @@ +import type { AgentUsage } from "./agents/shared.ts"; +import type { PrepResult } from "./prep/types.ts"; +import { log } from "./utils/cli.ts"; +import type { DiffCoverageState } from "./utils/diffCoverage.ts"; +import { + type ProgressComment, + type ProgressCommentType, + parseProgressComment, +} from "./utils/progressComment.ts"; +import type { TodoTracker } from "./utils/todoTracking.ts"; + +export type BackgroundProcess = { + pid: number; + outputPath: string; + pidPath: string; +}; + +export type BrowserDaemon = { binDir: string; error?: never } | { binDir?: never; error: string }; + +export type StoredPushDest = { + remoteName: string; + remoteBranch: string; + localBranch: string; +}; + +/** + * Valid inline-comment anchor lines per side at a particular checkout SHA. + * Lives here (not in `mcp/review.ts`) so `ToolState` — which caches + * `Map` per checkout — does not pull the MCP server + * graph into every consumer of run state (the action's main loop, agent + * harnesses, cf-worker indexing). + */ +export type CommentableLines = { RIGHT: Set; LEFT: Set }; + +/** + * mutable per-run record of facts that occurred during execution. shared + * between the action process and the MCP server (one process — toolState is + * just a JS object passed by reference into both surfaces). + * + * design rule: ToolState is LITERAL. each field records a thing that + * happened — `review` is set when `create_pull_request_review` succeeded, + * `finalSummaryWritten` flips when `report_progress` wrote a non-plan body, + * `selectedMode` is set when `select_mode` was called. fields should never + * encode the absence of an event ("unsubmittedReview", "missingArtifact"), + * speculative state, or values derived from other fields. + * + * any predicate the rest of the code needs ("the agent picked review mode but + * never produced a review or progress write") is computed inline at the call + * site, not stored. derived state in this struct invariably drifts from the + * literal fields under refactors and is the wrong layer for the check. + * + * write narrowly: prefer adding state inside the tool that mutates it (e.g. + * `create_pull_request_review` populates `toolState.review`) and reading + * narrowly elsewhere. don't introduce flags from main.ts that mirror what an + * MCP tool already records. + */ +export interface ToolState { + // where we're allowed to push - base repo initially, fork URL for fork PRs + // set by setupGit, updated by checkout_pr. always set before push validation. + pushUrl?: string; + // push destination set by checkout_pr - used as primary source in push_branch + // because git config reads can fail in certain environments + pushDest?: StoredPushDest; + // issue or PR number (same number space in GitHub) + issueNumber?: number; + // PR HEAD sha at checkout time — used to detect new commits pushed during a review + checkoutSha?: string; + // commentable lines per file at checkoutSha — captured during checkout_pr so + // review-time inline-comment validation matches the diff GitHub will anchor + // to (commit_id=checkoutSha). without this, a PR update between checkout and + // review would make listFiles (latest HEAD) disagree with the anchor, + // silently dropping valid comments or letting invalid ones through. + // + // commentableLinesPullNumber records WHICH PR this snapshot belongs to. if + // the agent checks out PR B and then reviews PR A in the same session, the + // cached snapshot for B would silently mis-validate A's comments — keying + // by PR number forces a re-fetch when the target changes. + // + // commentableLinesCheckoutSha pins the snapshot to the SHA it was built + // against. if a second checkout_pr for the SAME PR bumps checkoutSha but + // fails before repopulating the cache (e.g., listFiles rate-limits), the + // stale snapshot would silently mis-validate comments against the new SHA. + // comparing both fields forces a re-fetch when either moves. + commentableLinesByFile?: Map; + commentableLinesPullNumber?: number; + commentableLinesCheckoutSha?: string | undefined; + // SHA to diff incrementally against — set from event payload on first checkout, + // then from checkoutSha when review.ts detects new commits mid-review + beforeSha?: string; + selectedMode?: string; + backgroundProcesses: Map; + browserDaemon?: BrowserDaemon | undefined; + review?: { + id: number; + nodeId: string; + reviewedSha: string | undefined; + }; + // dedupe key: parent review comment_id → most-recent reply written this + // session by reply_to_review_comment. used by duplicateReplyDecision to + // skip identical-body re-emissions of the same call (PR #610 root cause). + // body-keyed (not just id-keyed) so legitimate follow-up replies with + // different content still go through. + reviewReplies?: Map< + number, + { commentId: number; url: string | undefined; bodyWithFooter: string } + >; + dependencyInstallation?: { + status: "not_started" | "in_progress" | "completed" | "failed"; + promise: Promise | undefined; + results: PrepResult[] | undefined; + }; + // undefined = no comment yet, object = active comment, null = deliberately deleted + progressComment: ProgressComment | null | undefined; + // immutable snapshot: true if a progress comment was pre-created at init time. + // survives deleteProgressComment so handleAgentResult can still detect "expected but never reported". + hadProgressComment: boolean; + lastProgressBody?: string; + wasUpdated?: boolean; + // set after a non-plan report_progress successfully writes the final summary. + // decoupled from todoTracker.enabled so cleanup detection survives API failures. + finalSummaryWritten?: boolean; + // set by select_mode when Plan + issue_number and plan-comment API returns existing plan (for report_progress target_plan_comment) + existingPlanCommentId?: number; + previousPlanBody?: string; + // absolute path to the PR summary markdown file the agent edits in place. + // seeded by main.ts before the agent starts when payload.generateSummary is set; + // read back at end-of-run to persist to DB. + summaryFilePath?: string; + // exact bytes of the seeded snapshot file at run start. compared against + // the file content at end-of-run to detect "agent never touched it" — in + // that case persistSummary skips the DB write (saving the seed verbatim + // would either re-write what the DB already has, on incremental runs, or + // serialize the placeholder scaffold, on first runs). + summarySeed?: string; + // set to true after persistSummary completes once. prevents the error-path + // call (which exists so a successful agent edit before a crash still gets + // persisted) from redundantly re-running the DB PATCH on the + // success-then-late-throw path. + summaryPersistAttempted?: boolean; + // absolute path to the rolling repo-level learnings markdown file the + // agent reads at startup and may edit at end-of-run. seeded by main.ts + // for every run from `Repo.learnings` (empty file when no learnings + // exist yet); read back at end-of-run to persist any edits. + learningsFilePath?: string; + // exact bytes of the seeded learnings file at run start. compared + // against the file content at end-of-run to detect "agent never touched + // it" — in that case persistLearnings skips the DB PATCH (saving the + // identical content would be a no-op write that wastes a LearningsRevision + // row and the API round-trip). + learningsSeed?: string; + // mirror of `summaryPersistAttempted` for the learnings tmpfile — guards + // the error-path / exit-signal callers from a redundant second PATCH + // after the success path already persisted. + learningsPersistAttempted?: boolean; + output?: string; + usageEntries: AgentUsage[]; + model?: string | undefined; + todoTracker?: TodoTracker | undefined; + diffCoverage?: DiffCoverageState | undefined; +} + +interface InitToolStateParams { + progressComment: { id: string; type: ProgressCommentType } | undefined; +} + +export function initToolState(params: InitToolStateParams): ToolState { + const resolved = parseProgressComment(params.progressComment); + + if (resolved) { + log.info(`» using pre-created progress comment: ${resolved.id} (${resolved.type})`); + } + + return { + progressComment: resolved, + hadProgressComment: !!resolved, + backgroundProcesses: new Map(), + usageEntries: [], + }; +} diff --git a/utils/browser.ts b/utils/browser.ts index 6b4a680..cc5002f 100644 --- a/utils/browser.ts +++ b/utils/browser.ts @@ -1,7 +1,7 @@ import { execFileSync, spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { dirname } from "node:path"; -import type { ToolState } from "../mcp/server.ts"; +import type { ToolState } from "../toolState.ts"; import { log } from "./cli.ts"; import { filterEnv } from "./secrets.ts"; import { getDevDependencyVersion } from "./version.ts"; diff --git a/utils/errorReport.ts b/utils/errorReport.ts index d952669..b86c198 100644 --- a/utils/errorReport.ts +++ b/utils/errorReport.ts @@ -1,4 +1,4 @@ -import type { ToolState } from "../mcp/server.ts"; +import type { ToolState } from "../toolState.ts"; import { getApiUrl } from "./apiUrl.ts"; import { buildPullfrogFooter } from "./buildPullfrogFooter.ts"; import { createOctokit, parseRepoContext } from "./github.ts"; diff --git a/utils/run.ts b/utils/run.ts index 6397cbb..132a5ef 100644 --- a/utils/run.ts +++ b/utils/run.ts @@ -1,6 +1,6 @@ import type { AgentResult } from "../agents/shared.ts"; import type { MainResult } from "../main.ts"; -import type { ToolState } from "../mcp/server.ts"; +import type { ToolState } from "../toolState.ts"; import { log } from "./cli.ts"; import { reportErrorToComment } from "./errorReport.ts"; diff --git a/utils/setup.ts b/utils/setup.ts index 76c7598..9599958 100644 --- a/utils/setup.ts +++ b/utils/setup.ts @@ -3,7 +3,7 @@ import { mkdtempSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { ShellPermission } from "../external.ts"; -import type { ToolState } from "../mcp/server.ts"; +import type { ToolState } from "../toolState.ts"; import { log } from "./cli.ts"; import type { OctokitWithPlugins } from "./github.ts"; import { isInsideDocker } from "./globals.ts";