From 05484c7a41e4cf24cf358578ccfbbfdfbfe5197b Mon Sep 17 00:00:00 2001 From: Joao Carreiro Date: Fri, 19 Jun 2026 14:52:38 +0100 Subject: [PATCH] Enforce repair contract at every checkpoint --- src/repair/execute-fix-artifact.ts | 92 +++++++++++++++++-- .../execute-fix-artifact-source.test.ts | 58 ++++++++++++ 2 files changed, 144 insertions(+), 6 deletions(-) diff --git a/src/repair/execute-fix-artifact.ts b/src/repair/execute-fix-artifact.ts index fd2651d335..52468125de 100644 --- a/src/repair/execute-fix-artifact.ts +++ b/src/repair/execute-fix-artifact.ts @@ -2145,9 +2145,11 @@ function editValidatePrepareMerge({ }); } - const firstCheckpoint = commitCheckpointIfNeeded({ + const firstCheckpoint = commitRepairCheckpointIfNeeded({ + fixArtifact, targetDir, message: fixArtifact.pr_title, + phase: "initial", trailers: mode === "replacement" ? coAuthorTrailers(contributorCredits) : [], }); if (firstCheckpoint) { @@ -2176,9 +2178,11 @@ function editValidatePrepareMerge({ baseBranch, sourceHead: repairDeltaBaseHead, onReviewFix: (reviewAttempt: JsonValue) => { - const checkpoint = commitCheckpointIfNeeded({ + const checkpoint = commitRepairCheckpointIfNeeded({ + fixArtifact, targetDir, message: `fix(clawsweeper): address review for ${result.cluster_id} (${reviewAttempt})`, + phase: `review-fix-${reviewAttempt}`, trailers: mode === "replacement" ? coAuthorTrailers(contributorCredits) : [], }); if (checkpoint) { @@ -2207,9 +2211,11 @@ function editValidatePrepareMerge({ headSha: currentHead(targetDir), }); if (sync.status === "already-current") break; - const checkpoint = commitCheckpointIfNeeded({ + const checkpoint = commitRepairCheckpointIfNeeded({ + fixArtifact, targetDir, message: `fix(clawsweeper): reconcile ${result.cluster_id} with main (${attempt})`, + phase: `base-sync-${attempt}`, trailers: mode === "replacement" ? coAuthorTrailers(contributorCredits) : [], }); if (checkpoint) { @@ -2227,9 +2233,11 @@ function editValidatePrepareMerge({ break; } } - const finalCheckpoint = commitCheckpointIfNeeded({ + const finalCheckpoint = commitRepairCheckpointIfNeeded({ + fixArtifact, targetDir, message: `fix(clawsweeper): finalize ${result.cluster_id}`, + phase: "finalize", trailers: mode === "replacement" ? coAuthorTrailers(contributorCredits) : [], }); if (finalCheckpoint) { @@ -3300,8 +3308,16 @@ function checkoutRecoverableReplacementBranch({ return { resumed: false, branch }; } -function commitCheckpointIfNeeded({ targetDir, message, trailers = [] }: LooseRecord) { - if (!run("git", ["status", "--porcelain"], { cwd: targetDir }).trim()) return ""; +function commitRepairCheckpointIfNeeded({ + fixArtifact, + targetDir, + message, + phase, + trailers = [], +}: LooseRecord) { + const status = run("git", ["status", "--porcelain"], { cwd: targetDir }).trim(); + if (!status) return ""; + enforceRepairCheckpointContract({ fixArtifact, phase, status }); run("git", ["add", "--all"], { cwd: targetDir }); const args = ["commit", "-m", message]; for (const trailer of uniqueStrings(trailers)) args.push("-m", trailer); @@ -3309,6 +3325,70 @@ function commitCheckpointIfNeeded({ targetDir, message, trailers = [] }: LooseRe return run("git", ["rev-parse", "HEAD"], { cwd: targetDir }).trim(); } +function enforceRepairCheckpointContract({ fixArtifact, phase, status }: LooseRecord) { + const mustTouch = repairCheckpointMustTouchFiles(fixArtifact); + if (mustTouch.length === 0) return; + + const changedFiles = changedFilesFromPorcelainStatus(String(status)); + if ( + changedFiles.some((file) => + mustTouch.some((expected) => changedFileMatchesContract(file, expected)), + ) + ) { + return; + } + + throw new Error( + [ + `repair checkpoint contract rejected ${String(phase || "checkpoint")}: no must_touch file changed`, + `must_touch=${mustTouch.join(", ")}`, + `changed_files=${changedFiles.join(", ") || "none"}`, + ].join("; "), + ); +} + +function repairCheckpointMustTouchFiles(fixArtifact: LooseRecord): string[] { + const candidates = [ + ...jsonStringArray(fixArtifact.must_touch), + ...jsonStringArray(fixArtifact.must_touch_files), + ]; + return uniqueStrings(candidates.map(normalizeRepairContractPath).filter(Boolean)); +} + +function jsonStringArray(value: JsonValue): string[] { + return Array.isArray(value) ? value.map((entry) => String(entry)) : []; +} + +function changedFilesFromPorcelainStatus(status: string): string[] { + return uniqueStrings( + status + .split(/\r?\n/) + .flatMap((line) => porcelainChangedPaths(line)) + .map(normalizeRepairContractPath) + .filter(Boolean), + ); +} + +function porcelainChangedPaths(line: string): string[] { + if (line.length < 4) return []; + const body = line.slice(3).trim(); + if (!body) return []; + const renamed = body.split(" -> "); + return renamed.length === 2 ? [renamed[1] ?? ""] : [body]; +} + +function changedFileMatchesContract(changedFile: string, expected: string) { + return changedFile === expected || changedFile.startsWith(`${expected.replace(/\/$/, "")}/`); +} + +function normalizeRepairContractPath(value: JsonValue): string { + const pathValue = String(value ?? "").trim(); + if (!pathValue || pathValue.startsWith("/") || pathValue.includes("\0")) return ""; + if (/[`$;&|<>()[\]{}*?~]/.test(pathValue)) return ""; + if (pathValue.split(/[\\/]/).includes("..")) return ""; + return pathValue.replace(/^\.\//, ""); +} + function pushRecoverableBranch({ targetDir, branch }: LooseRecord) { assertIssueImplementationNotPaused(); const remoteSha = remoteBranchSha({ targetDir, branch }); diff --git a/test/repair/execute-fix-artifact-source.test.ts b/test/repair/execute-fix-artifact-source.test.ts index d82ee42e94..2d0f353d5e 100644 --- a/test/repair/execute-fix-artifact-source.test.ts +++ b/test/repair/execute-fix-artifact-source.test.ts @@ -148,3 +148,61 @@ test("issue implementation rechecks opt-out labels immediately before branch pus assert.match(source.slice(helperStart, helperEnd), /repairPauseLabel\(issue\.labels\)/); assert.match(source.slice(helperStart, helperEnd), /refusing to push or open a PR/); }); + +test("repair contract checkpoints use one helper for every checkpoint path", () => { + const source = fs.readFileSync( + path.join(process.cwd(), "src/repair/execute-fix-artifact.ts"), + "utf8", + ); + assert.equal( + [...source.matchAll(/commitCheckpointIfNeeded\(/g)].length, + 0, + "legacy unguarded checkpoint helper must not remain", + ); + assert.equal( + [...source.matchAll(/commitRepairCheckpointIfNeeded\(/g)].length, + 5, + "four checkpoint call sites plus the helper definition should use the guarded helper", + ); + assert.match(source, /phase: "initial"/); + assert.match(source, /phase: `review-fix-\$\{reviewAttempt\}`/); + assert.match(source, /phase: `base-sync-\$\{attempt\}`/); + assert.match(source, /phase: "finalize"/); +}); + +test("repair checkpoint contract is explicit must_touch and bypasses incomplete likely_files", () => { + const source = fs.readFileSync( + path.join(process.cwd(), "src/repair/execute-fix-artifact.ts"), + "utf8", + ); + const helperStart = source.indexOf("function repairCheckpointMustTouchFiles("); + const helperEnd = source.indexOf("function changedFilesFromPorcelainStatus(", helperStart); + assert.notEqual(helperStart, -1); + assert.notEqual(helperEnd, -1); + const helper = source.slice(helperStart, helperEnd); + assert.match(helper, /fixArtifact\.must_touch/); + assert.match(helper, /fixArtifact\.must_touch_files/); + assert.doesNotMatch(helper, /likely_files/); + + const enforceStart = source.indexOf("function enforceRepairCheckpointContract("); + const enforceEnd = source.indexOf("function repairCheckpointMustTouchFiles(", enforceStart); + assert.notEqual(enforceStart, -1); + const enforce = source.slice(enforceStart, enforceEnd); + assert.match(enforce, /if \(mustTouch\.length === 0\) return;/); + assert.match(enforce, /repair checkpoint contract rejected/); +}); + +test("repair contract parser preserves git porcelain path offsets", () => { + const source = fs.readFileSync( + path.join(process.cwd(), "src/repair/execute-fix-artifact.ts"), + "utf8", + ); + const parserStart = source.indexOf("function porcelainChangedPaths("); + const parserEnd = source.indexOf("function changedFileMatchesContract(", parserStart); + assert.notEqual(parserStart, -1); + assert.notEqual(parserEnd, -1); + const parser = source.slice(parserStart, parserEnd); + assert.match(parser, /line\.slice\(3\)/); + assert.doesNotMatch(parser, /status\.slice\(3\)|text\.slice\(3\)/); + assert.match(parser, /body\.split\(" -> "\)/); +});