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.
180 lines
8.3 KiB
TypeScript
180 lines
8.3 KiB
TypeScript
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<path, CommentableLines>` 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<number>; LEFT: Set<number> };
|
|
|
|
/**
|
|
* 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<string, CommentableLines>;
|
|
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<string, BackgroundProcess>;
|
|
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<PrepResult[]> | 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: [],
|
|
};
|
|
}
|