Colin McDonnell
8e36f76cfa
* 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.
61 lines
1.9 KiB
TypeScript
61 lines
1.9 KiB
TypeScript
import type { AgentResult } from "../agents/shared.ts";
|
|
import type { MainResult } from "../main.ts";
|
|
import type { ToolState } from "../toolState.ts";
|
|
import { log } from "./cli.ts";
|
|
import { reportErrorToComment } from "./errorReport.ts";
|
|
|
|
export interface HandleAgentResultParams {
|
|
result: AgentResult;
|
|
toolState: ToolState;
|
|
silent: boolean | undefined;
|
|
}
|
|
|
|
export async function handleAgentResult(ctx: HandleAgentResultParams): Promise<MainResult> {
|
|
if (!ctx.result.success) {
|
|
return {
|
|
success: false,
|
|
error: ctx.result.error || "Agent execution failed",
|
|
output: ctx.result.output!,
|
|
};
|
|
}
|
|
|
|
// IncrementalReview's non-substantive path exits cleanly without
|
|
// submitting any review, so no MCP write tool flips wasUpdated and the
|
|
// strict completion check below would otherwise fail the run. The
|
|
// isReviewMode skip is load-bearing for that path: the agent's exit
|
|
// code is the completion signal, not a progress-comment write.
|
|
// (Review mode that submits a real review now flips wasUpdated via
|
|
// create_pull_request_review, so the skip is redundant for the
|
|
// substantive-review path but kept for symmetry with IncrementalReview.)
|
|
// See plans/review_progress_comment_cleanup_b0120f6c.plan.md.
|
|
const mode = ctx.toolState.selectedMode;
|
|
const isReviewMode = mode === "Review" || mode === "IncrementalReview";
|
|
if (
|
|
!isReviewMode &&
|
|
!ctx.toolState.wasUpdated &&
|
|
ctx.toolState.hadProgressComment &&
|
|
!ctx.silent
|
|
) {
|
|
const error = ctx.result.error || "agent completed without reporting progress";
|
|
try {
|
|
await reportErrorToComment({
|
|
toolState: ctx.toolState,
|
|
error,
|
|
title: "Error",
|
|
});
|
|
} catch {}
|
|
return {
|
|
success: false,
|
|
error,
|
|
output: ctx.result.output || "",
|
|
};
|
|
}
|
|
|
|
log.success("Task complete.");
|
|
|
|
return {
|
|
success: true,
|
|
output: ctx.result.output || "",
|
|
};
|
|
}
|