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.
51 lines
1.6 KiB
TypeScript
51 lines
1.6 KiB
TypeScript
import { describe, expect, it } from "vitest";
|
|
import type { ToolState } from "../toolState.ts";
|
|
import { getUnsubmittedReview } from "./postRun.ts";
|
|
|
|
function makeToolState(overrides: Partial<ToolState> = {}): ToolState {
|
|
return {
|
|
progressComment: undefined,
|
|
hadProgressComment: true,
|
|
backgroundProcesses: new Map(),
|
|
usageEntries: [],
|
|
...overrides,
|
|
};
|
|
}
|
|
|
|
describe("getUnsubmittedReview", () => {
|
|
it("returns null when mode is not a review mode", () => {
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Build" }))).toBeNull();
|
|
expect(getUnsubmittedReview(makeToolState())).toBeNull();
|
|
});
|
|
|
|
it("returns null when a review was already submitted", () => {
|
|
expect(
|
|
getUnsubmittedReview(
|
|
makeToolState({
|
|
selectedMode: "Review",
|
|
review: { id: 1, nodeId: "n", reviewedSha: undefined },
|
|
})
|
|
)
|
|
).toBeNull();
|
|
});
|
|
|
|
it("returns null when report_progress wrote a final summary", () => {
|
|
expect(
|
|
getUnsubmittedReview(makeToolState({ selectedMode: "Review", finalSummaryWritten: true }))
|
|
).toBeNull();
|
|
});
|
|
|
|
it("returns null when there is no progress comment to anchor the failure to", () => {
|
|
expect(
|
|
getUnsubmittedReview(makeToolState({ selectedMode: "Review", hadProgressComment: false }))
|
|
).toBeNull();
|
|
});
|
|
|
|
it("returns the selected mode when the gate should fire", () => {
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "Review" }))).toBe("Review");
|
|
expect(getUnsubmittedReview(makeToolState({ selectedMode: "IncrementalReview" }))).toBe(
|
|
"IncrementalReview"
|
|
);
|
|
});
|
|
});
|