From 6db4a6d02e737c7104cedd23efeb767defa07d61 Mon Sep 17 00:00:00 2001 From: "pullfrog[bot]" <226033991+pullfrog[bot]@users.noreply.github.com> Date: Wed, 6 May 2026 20:54:34 +0000 Subject: [PATCH] fix(mcp): preserve coveragePreflightRan across checkout_pr refreshes (#576) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit checkout_pr unconditionally rebuilds ctx.toolState.diffCoverage via createDiffCoverageState, which initialised coveragePreflightRan to false. a second checkout_pr therefore reset the "one-time nudge per review session" guarantee in runDiffCoveragePreflight, and the next create_pull_request_review threw the diff-coverage pre-flight error again — even after the agent had already gone through the read-and-resubmit dance once. createDiffCoverageState now accepts an optional previous state and carries forward coveragePreflightRan. coveredRanges are intentionally not carried because their line numbers are tied to the previous diff's content (especially under incremental diffs). closes #566 Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> Co-authored-by: David Blass --- mcp/checkout.ts | 1 + utils/diffCoverage.test.ts | 18 ++++++++++++++++++ utils/diffCoverage.ts | 6 +++++- 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/mcp/checkout.ts b/mcp/checkout.ts index 0d0f9b4..1b3eb48 100644 --- a/mcp/checkout.ts +++ b/mcp/checkout.ts @@ -545,6 +545,7 @@ export function CheckoutPrTool(ctx: ToolContext) { diffPath, totalLines: countLines({ content: formatResult.content }), toc: formatResult.toc, + previous: ctx.toolState.diffCoverage, }); log.debug( `» diff coverage initialized: diffPath=${diffPath}, totalLines=${ctx.toolState.diffCoverage.totalLines}, tocEntries=${ctx.toolState.diffCoverage.tocEntries.length}` diff --git a/utils/diffCoverage.test.ts b/utils/diffCoverage.test.ts index dfa96db..c4877f0 100644 --- a/utils/diffCoverage.test.ts +++ b/utils/diffCoverage.test.ts @@ -98,6 +98,24 @@ describe("diff coverage line checker", () => { ]); }); + it("carries forward coveragePreflightRan from a previous state across checkout refreshes", () => { + const previous = createDiffCoverageState({ diffPath, totalLines: 30, toc }); + previous.coveragePreflightRan = true; + previous.coveredRanges = [{ startLine: 5, endLine: 10 }]; + + const next = createDiffCoverageState({ diffPath, totalLines: 50, toc, previous }); + + expect(next.coveragePreflightRan).toBe(true); + // coveredRanges are tied to the previous diff content and must not leak forward + expect(next.coveredRanges).toEqual([]); + expect(next.totalLines).toBe(50); + }); + + it("defaults coveragePreflightRan to false when no previous state is provided", () => { + const state = createDiffCoverageState({ diffPath, totalLines: 30, toc }); + expect(state.coveragePreflightRan).toBe(false); + }); + it("computes per-file unread ranges from tracked reads", () => { const state = createDiffCoverageState({ diffPath, diff --git a/utils/diffCoverage.ts b/utils/diffCoverage.ts index 125360d..2ced6d9 100644 --- a/utils/diffCoverage.ts +++ b/utils/diffCoverage.ts @@ -78,13 +78,17 @@ export function createDiffCoverageState(params: { diffPath: string; totalLines: number; toc: string; + previous?: DiffCoverageState | undefined; }): DiffCoverageState { return { diffPath: params.diffPath, totalLines: params.totalLines, tocEntries: parseDiffTocEntries({ toc: params.toc }), coveredRanges: [], - coveragePreflightRan: false, + // carry forward across checkout_pr refreshes so the nudge stays "once per + // review session". coveredRanges are intentionally not carried because + // line numbers are tied to the previous diff's content. + coveragePreflightRan: params.previous?.coveragePreflightRan ?? false, }; }