diff --git a/schema/repair/codex-result.schema.json b/schema/repair/codex-result.schema.json index 97da5be01f..e90477e0f5 100644 --- a/schema/repair/codex-result.schema.json +++ b/schema/repair/codex-result.schema.json @@ -285,7 +285,8 @@ "source_prs", "repair_strategy", "allow_no_pr", - "branch_update_blockers" + "branch_update_blockers", + "repair_contract" ], "additionalProperties": false, "properties": { @@ -356,6 +357,31 @@ "items": { "type": "string" } + }, + "repair_contract": { + "anyOf": [ + { + "type": "null" + }, + { + "type": "object", + "required": ["must_touch", "match"], + "additionalProperties": false, + "properties": { + "must_touch": { + "type": "array", + "minItems": 1, + "items": { + "type": "string" + } + }, + "match": { + "type": "string", + "enum": ["any", "all"] + } + } + } + ] } } } diff --git a/src/repair/commit-finding-intake.ts b/src/repair/commit-finding-intake.ts index 790758a5bd..1ad642c100 100644 --- a/src/repair/commit-finding-intake.ts +++ b/src/repair/commit-finding-intake.ts @@ -342,6 +342,7 @@ function writeSyntheticRun(context: LooseRecord) { repair_strategy: "new_fix_pr", allow_no_pr: true, branch_update_blockers: [], + repair_contract: null, }, }; fs.writeFileSync( diff --git a/src/repair/deterministic-automerge-result.ts b/src/repair/deterministic-automerge-result.ts index cae6272865..d1abc8891a 100644 --- a/src/repair/deterministic-automerge-result.ts +++ b/src/repair/deterministic-automerge-result.ts @@ -78,6 +78,7 @@ export function deterministicAutomergeResult({ repair_strategy: "repair_contributor_branch", allow_no_pr: false, branch_update_blockers: [], + repair_contract: null, }; return { diff --git a/src/repair/execute-fix-artifact.ts b/src/repair/execute-fix-artifact.ts index c0f944fffc..0c7adb0ccf 100644 --- a/src/repair/execute-fix-artifact.ts +++ b/src/repair/execute-fix-artifact.ts @@ -114,6 +114,11 @@ import { type TargetValidationOptions, } from "./target-validation.js"; import { uniqueStrings } from "./validation-command-utils.js"; +import { + changedFilesFromNameOnlyZ, + enforceRepairContract, + repairContract, +} from "./repair-contract.js"; import { rebaseConflictEditDecision, unresolvedRebaseConflictReason, @@ -1996,6 +2001,8 @@ function editValidatePrepareMerge({ let producedChanges = allowExistingChanges; let previousSummary = ""; const checkpointCommits: JsonValue[] = []; + const hasRepairContract = repairContract(fixArtifact) !== null; + const pushIntermediateCheckpoint = hasRepairContract ? null : pushCheckpoint; if ( !producedChanges && canTreatRebaseAsCompleteRepair({ @@ -2210,7 +2217,7 @@ function editValidatePrepareMerge({ }); if (firstCheckpoint) { checkpointCommits.push(firstCheckpoint); - pushCheckpoint?.(); + pushIntermediateCheckpoint?.(); } let codexReview = null; @@ -2241,7 +2248,7 @@ function editValidatePrepareMerge({ }); if (checkpoint) { checkpointCommits.push(checkpoint); - pushCheckpoint?.(); + pushIntermediateCheckpoint?.(); } }, }); @@ -2272,7 +2279,7 @@ function editValidatePrepareMerge({ }); if (checkpoint) { checkpointCommits.push(checkpoint); - pushCheckpoint?.(); + pushIntermediateCheckpoint?.(); } if (attempt === maxFinalBaseSyncAttempts) { codexReview.final_base_sync = { @@ -2292,7 +2299,7 @@ function editValidatePrepareMerge({ }); if (finalCheckpoint) { checkpointCommits.push(finalCheckpoint); - pushCheckpoint?.(); + pushIntermediateCheckpoint?.(); } const historyCompaction = mode === "replacement" @@ -2304,7 +2311,8 @@ function editValidatePrepareMerge({ checkpointCommits, }) : null; - if (historyCompaction?.status === "compacted") { + enforceFinalRepairContract({ fixArtifact, targetDir, baseBranch }); + if (hasRepairContract || historyCompaction?.status === "compacted") { pushCheckpoint?.(); } const commit = run("git", ["rev-parse", "HEAD"], { cwd: targetDir }).trim(); @@ -3367,6 +3375,15 @@ function commitCheckpointIfNeeded({ targetDir, message, trailers = [] }: LooseRe return run("git", ["rev-parse", "HEAD"], { cwd: targetDir }).trim(); } +function enforceFinalRepairContract({ fixArtifact, targetDir, baseBranch }: LooseRecord) { + if (!repairContract(fixArtifact)) return; + const baseRef = `origin/${baseBranch}`; + const changedFiles = changedFilesFromNameOnlyZ( + run("git", ["diff", "--name-only", "-z", `${baseRef}..HEAD`], { cwd: targetDir }), + ); + enforceRepairContract({ fixArtifact, changedFiles }); +} + function pushRecoverableBranch({ targetDir, branch }: LooseRecord) { assertIssueImplementationNotPaused(); const remoteSha = remoteBranchSha({ targetDir, branch }); diff --git a/src/repair/execute-fix-validation.ts b/src/repair/execute-fix-validation.ts index 80796b8683..20101d3a15 100644 --- a/src/repair/execute-fix-validation.ts +++ b/src/repair/execute-fix-validation.ts @@ -2,6 +2,7 @@ import fs from "node:fs"; import path from "node:path"; import type { JsonValue, LooseRecord } from "./json-types.js"; +import { validateRepairContractShape } from "./repair-contract.js"; import { GITHUB_PR_TITLE_MAX_LENGTH } from "./pr-title.js"; import { slug } from "./text-utils.js"; @@ -51,6 +52,8 @@ export function validateFixArtifact(fixArtifact: LooseRecord): LooseRecord { if (typeof fixArtifact.changelog_required !== "boolean") { throw new Error("fix_artifact.changelog_required must be boolean"); } + const contractErrors = validateRepairContractShape(fixArtifact); + if (contractErrors.length > 0) throw new Error(contractErrors.join("; ")); if (!REPAIR_STRATEGIES.has(fixArtifact.repair_strategy)) { throw new Error("fix_artifact.repair_strategy is not executable"); } diff --git a/src/repair/lib.ts b/src/repair/lib.ts index b2b95245f8..d77ea9e92d 100755 --- a/src/repair/lib.ts +++ b/src/repair/lib.ts @@ -290,6 +290,7 @@ export function renderPrompt( parts.push( "## Required final output", "Return JSON matching `schema/repair/codex-result.schema.json` and nothing else.", + "If the fix has explicit files that must differ from the latest base in the final repaired tree, set `fix_artifact.repair_contract` to an object with `must_touch` and `match` (`any` or `all`). The executor checks this once against the final branch delta after review fixes and base sync. Set `repair_contract` to null when the expected edit surface is uncertain, only represented by incomplete `likely_files`, or the work is a pure deterministic rebase.", ); return parts.join("\n\n"); diff --git a/src/repair/repair-contract.ts b/src/repair/repair-contract.ts new file mode 100644 index 0000000000..b3b12819bb --- /dev/null +++ b/src/repair/repair-contract.ts @@ -0,0 +1,110 @@ +import type { JsonValue, LooseRecord } from "./json-types.js"; + +export interface RepairContract { + mustTouch: string[]; + match: "any" | "all"; +} + +export function repairContract(fixArtifact: LooseRecord): RepairContract | null { + const raw = fixArtifact.repair_contract; + if (!raw || typeof raw !== "object" || Array.isArray(raw)) return null; + if (validateRepairContractShape(fixArtifact).length > 0) return null; + const mustTouch = uniqueStrings(raw.must_touch.map(normalizeRepairContractPath)); + return { + mustTouch, + match: raw.match, + }; +} + +export function enforceRepairContract({ + fixArtifact, + changedFiles: rawChangedFiles, +}: { + fixArtifact: LooseRecord; + changedFiles: readonly string[]; +}): void { + const contract = repairContract(fixArtifact); + if (!contract) return; + + const changedFiles = uniqueStrings( + rawChangedFiles.map(normalizeRepairContractPath).filter(Boolean), + ); + const matched = contract.mustTouch.filter((expected) => + changedFiles.some((file) => changedFileMatchesContract(file, expected)), + ); + const ok = + contract.match === "all" ? matched.length === contract.mustTouch.length : matched.length > 0; + if (ok) return; + + const missing = contract.mustTouch.filter((expected) => !matched.includes(expected)); + throw new Error( + [ + "repair contract rejected final repair tree: required paths are missing from the final branch delta", + `match=${contract.match}`, + `must_touch=${contract.mustTouch.join(", ")}`, + `matched=${matched.join(", ") || "none"}`, + `missing=${missing.join(", ") || "none"}`, + `changed_files=${changedFiles.join(", ") || "none"}`, + ].join("; "), + ); +} + +export function changedFilesFromNameOnlyZ(diff: string): string[] { + return uniqueStrings(diff.split("\0").map(normalizeRepairContractPath).filter(Boolean)); +} + +export function validateRepairContractShape(fixArtifact: LooseRecord): string[] { + const raw = fixArtifact.repair_contract; + if (raw === undefined || raw === null) return []; + if (typeof raw !== "object" || Array.isArray(raw)) { + return ["fix_artifact.repair_contract must be an object when present"]; + } + + const errors: string[] = []; + const allowedKeys = new Set(["must_touch", "match"]); + for (const key of Object.keys(raw)) { + if (!allowedKeys.has(key)) { + errors.push(`fix_artifact.repair_contract.${key} is not allowed`); + } + } + if (!Array.isArray(raw.must_touch) || raw.must_touch.length === 0) { + errors.push("fix_artifact.repair_contract.must_touch must be a non-empty list"); + } + for (const value of Array.isArray(raw.must_touch) ? raw.must_touch : []) { + if (typeof value !== "string") { + errors.push("fix_artifact.repair_contract.must_touch entries must be strings"); + continue; + } + if (!normalizeRepairContractPath(value)) { + errors.push( + `fix_artifact.repair_contract.must_touch contains an unsafe path: ${String(value)}`, + ); + } + } + if (raw.match !== "any" && raw.match !== "all") { + errors.push("fix_artifact.repair_contract.match must be any or all"); + } + if (fixArtifact.deterministic_rebase_only === true) { + errors.push( + "fix_artifact.repair_contract is incompatible with deterministic_rebase_only because a pure base sync has no repair delta", + ); + } + return errors; +} + +function changedFileMatchesContract(changedFile: string, expected: string): boolean { + const prefix = expected.replace(/\/$/, ""); + return changedFile === expected || changedFile.startsWith(`${prefix}/`); +} + +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 uniqueStrings(values: readonly string[]): string[] { + return [...new Set(values)]; +} diff --git a/src/repair/review-results.ts b/src/repair/review-results.ts index f2fbe62792..041178943d 100644 --- a/src/repair/review-results.ts +++ b/src/repair/review-results.ts @@ -1,5 +1,6 @@ #!/usr/bin/env node import type { JsonValue, LooseRecord } from "./json-types.js"; +import { validateRepairContractShape } from "./repair-contract.js"; import fs from "node:fs"; import path from "node:path"; import { parseArgs, parseJob, repoRoot } from "./lib.js"; @@ -526,6 +527,7 @@ function validateFixArtifact(fixArtifact: LooseRecord, failures: LooseRecord[]) if (typeof fixArtifact.changelog_required !== "boolean") { failures.push("fix_artifact.changelog_required must be boolean"); } + failures.push(...validateRepairContractShape(fixArtifact)); if (!FIX_REPAIR_STRATEGIES.has(fixArtifact.repair_strategy)) { failures.push("fix_artifact.repair_strategy is required"); } diff --git a/test/repair/deterministic-automerge-result.test.ts b/test/repair/deterministic-automerge-result.test.ts index 9b6ae0b453..1d967e897f 100644 --- a/test/repair/deterministic-automerge-result.test.ts +++ b/test/repair/deterministic-automerge-result.test.ts @@ -88,6 +88,7 @@ test("deterministic automerge result emits generic direct-Codex repair artifact" ]); assert.deepEqual(result?.fix_artifact.affected_surfaces, ["extensions/memory-core"]); assert.equal(result?.fix_artifact.changelog_required, false); + assert.equal(result?.fix_artifact.repair_contract, null); assert.deepEqual(result?.fix_artifact.source_prs, [ "https://github.com/openclaw/openclaw/pull/71898", ]); diff --git a/test/repair/execute-fix-artifact-source.test.ts b/test/repair/execute-fix-artifact-source.test.ts index 042551edb3..89da65dc8e 100644 --- a/test/repair/execute-fix-artifact-source.test.ts +++ b/test/repair/execute-fix-artifact-source.test.ts @@ -176,3 +176,38 @@ 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 gates the final cumulative tree, not individual checkpoints", () => { + const source = readText(path.join(process.cwd(), "src/repair/execute-fix-artifact.ts")); + assert.equal( + [...source.matchAll(/commitCheckpointIfNeeded\(/g)].length, + 5, + "four checkpoint call sites plus the ordinary commit helper should remain", + ); + assert.doesNotMatch(source, /commitRepairCheckpointIfNeeded|checkpointBaseHead/); + assert.match(source, /enforceFinalRepairContract\(\{ fixArtifact, targetDir, baseBranch \}\)/); + assert.equal( + [...source.matchAll(/pushIntermediateCheckpoint\?\.\(\)/g)].length, + 4, + "contract jobs must defer all four recovery pushes until final validation", + ); + assert.match(source, /if \(hasRepairContract \|\| historyCompaction\?\.status === "compacted"\)/); + + const compact = source.indexOf("const historyCompaction ="); + const enforce = source.indexOf("enforceFinalRepairContract(", compact); + const publish = source.indexOf("if (hasRepairContract", enforce); + const commit = source.indexOf('const commit = run("git", ["rev-parse", "HEAD"]', publish); + assert.ok(compact < enforce && enforce < publish && publish < commit); +}); + +test("final repair contract compares the repaired tree with the latest base", () => { + const source = readText(path.join(process.cwd(), "src/repair/execute-fix-artifact.ts")); + const start = source.indexOf("function enforceFinalRepairContract("); + const end = source.indexOf("function pushRecoverableBranch(", start); + const helper = source.slice(start, end); + assert.match(source, /\.\/repair-contract\.js/); + assert.match(helper, /const baseRef = `origin\/\$\{baseBranch\}`/); + assert.match(helper, /"diff", "--name-only", "-z", `\$\{baseRef\}\.\.HEAD`/); + assert.match(helper, /enforceRepairContract\(\{ fixArtifact, changedFiles \}\)/); + assert.doesNotMatch(helper, /--porcelain=v1|phase|checkpoint/); +}); diff --git a/test/repair/repair-contract.test.ts b/test/repair/repair-contract.test.ts new file mode 100644 index 0000000000..6e3baa7a96 --- /dev/null +++ b/test/repair/repair-contract.test.ts @@ -0,0 +1,177 @@ +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; + +import { + changedFilesFromNameOnlyZ, + enforceRepairContract, + repairContract, + validateRepairContractShape, +} from "../../dist/repair/repair-contract.js"; + +test("repair contract is explicit and ignores incomplete likely_files", () => { + assert.equal(repairContract({ likely_files: ["src/a.ts"] }), null); + assert.equal(repairContract({ repair_contract: null }), null); + assert.deepEqual( + repairContract({ + likely_files: ["src/a.ts"], + repair_contract: { must_touch: ["src/a.ts"], match: "any" }, + }), + { mustTouch: ["src/a.ts"], match: "any" }, + ); +}); + +test("repair contract validates schema-level shape", () => { + assert.deepEqual(validateRepairContractShape({}), []); + assert.deepEqual(validateRepairContractShape({ repair_contract: null }), []); + assert.deepEqual( + validateRepairContractShape({ + repair_contract: { must_touch: ["src/a.ts"], match: "any" }, + }), + [], + ); + assert.match( + validateRepairContractShape({ + repair_contract: { must_touch: ["../secret"], match: "one", scope: "every_checkpoint" }, + }).join("\n"), + /unsafe path|match must be any or all|scope is not allowed/, + ); + assert.match( + validateRepairContractShape({ + repair_contract: { must_touch: ["src/a.ts"] }, + }).join("\n"), + /match must be any or all/, + ); + assert.match( + validateRepairContractShape({ + repair_contract: { must_touch: ["src/a.ts", 7], match: "any", extra: true }, + }).join("\n"), + /entries must be strings|extra is not allowed/, + ); + assert.match( + validateRepairContractShape({ + deterministic_rebase_only: true, + repair_contract: { must_touch: ["src/a.ts"], match: "any" }, + }).join("\n"), + /incompatible with deterministic_rebase_only/, + ); +}); + +test("name-only z parser preserves spaces", () => { + assert.deepEqual(changedFilesFromNameOnlyZ("src/file with space.ts\0docs/guide.md\0"), [ + "src/file with space.ts", + "docs/guide.md", + ]); +}); + +test("final repair contract supports any, all, and directory semantics", () => { + assert.doesNotThrow(() => + enforceRepairContract({ + changedFiles: ["src/a.ts", "docs/review.md"], + fixArtifact: { + repair_contract: { must_touch: ["src/a.ts", "src/c.ts"], match: "any" }, + }, + }), + ); + assert.doesNotThrow(() => + enforceRepairContract({ + changedFiles: ["src/a.ts", "src/c.ts", "test/review-fix.test.ts"], + fixArtifact: { + repair_contract: { must_touch: ["src/a.ts", "src/c.ts"], match: "all" }, + }, + }), + ); + assert.doesNotThrow(() => + enforceRepairContract({ + changedFiles: ["src/repair/a.ts"], + fixArtifact: { repair_contract: { must_touch: ["src/repair"], match: "all" } }, + }), + ); + assert.throws( + () => + enforceRepairContract({ + changedFiles: ["docs/review.md"], + fixArtifact: { + repair_contract: { must_touch: ["src/a.ts", "src/c.ts"], match: "any" }, + }, + }), + /repair contract rejected final repair tree.*missing=src\/a\.ts, src\/c\.ts/, + ); +}); + +test("later checkpoint files do not invalidate a cumulative repair", () => { + assert.doesNotThrow(() => + enforceRepairContract({ + changedFiles: ["src/a.ts", "test/a.test.ts", "docs/base-sync.md"], + fixArtifact: { repair_contract: { must_touch: ["src/a.ts"], match: "all" } }, + }), + ); +}); + +test("git final-tree integration ignores paths changed only by the latest base", () => { + const root = mkdtempSync(join(tmpdir(), "clawsweeper-repair-contract-")); + const git = (...args: string[]) => + execFileSync("git", args, { cwd: root, encoding: "utf8" }).trim(); + try { + git("init", "-q"); + git("config", "user.name", "ClawSweeper Test"); + git("config", "user.email", "clawsweeper@example.invalid"); + mkdirSync(join(root, "src")); + writeFileSync(join(root, "src/a.ts"), "export const value = 1;\n"); + git("add", "."); + git("commit", "-qm", "base"); + const initial = git("rev-parse", "HEAD"); + + git("checkout", "-qb", "latest-base"); + writeFileSync(join(root, "src/a.ts"), "export const value = 2;\n"); + git("add", "."); + git("commit", "-qm", "upstream change"); + const latestBase = git("rev-parse", "HEAD"); + + git("checkout", "-qb", "repaired"); + writeFileSync(join(root, "src/a.ts"), "export const value = 3;\n"); + mkdirSync(join(root, "test")); + writeFileSync(join(root, "test/review-fix.test.ts"), "// later review checkpoint\n"); + git("add", "."); + git("commit", "-qm", "repair plus review fix"); + + const changedFiles = changedFilesFromNameOnlyZ( + execFileSync("git", ["diff", "--name-only", "-z", `${latestBase}..HEAD`], { + cwd: root, + encoding: "utf8", + }), + ); + assert.doesNotThrow(() => + enforceRepairContract({ + changedFiles, + fixArtifact: { repair_contract: { must_touch: ["src/a.ts"], match: "all" } }, + }), + ); + git("checkout", "-qb", "unrelated", initial); + mkdirSync(join(root, "docs")); + writeFileSync(join(root, "docs/review.md"), "unrelated repair\n"); + git("add", "."); + git("commit", "-qm", "unrelated repair"); + git("rebase", "-q", "latest-base"); + const unrelatedFiles = changedFilesFromNameOnlyZ( + execFileSync("git", ["diff", "--name-only", "-z", `${latestBase}..HEAD`], { + cwd: root, + encoding: "utf8", + }), + ); + assert.deepEqual(unrelatedFiles, ["docs/review.md"]); + assert.throws( + () => + enforceRepairContract({ + changedFiles: unrelatedFiles, + fixArtifact: { repair_contract: { must_touch: ["src/a.ts"], match: "all" } }, + }), + /repair contract rejected final repair tree/, + ); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); diff --git a/test/repair/run-worker.test.ts b/test/repair/run-worker.test.ts index db5e8207ff..2d74b00f73 100644 --- a/test/repair/run-worker.test.ts +++ b/test/repair/run-worker.test.ts @@ -7,6 +7,31 @@ import test from "node:test"; const repoRoot = process.cwd(); +test("repair output schema keeps every strict object property required", () => { + const schema = JSON.parse( + fs.readFileSync(path.join(repoRoot, "schema/repair/codex-result.schema.json"), "utf8"), + ); + + const visit = (value: unknown, location: string): void => { + if (!value || typeof value !== "object" || Array.isArray(value)) return; + const node = value as Record; + if (node.type === "object" && node.additionalProperties === false) { + const properties = Object.keys((node.properties ?? {}) as Record).sort(); + const required = Array.isArray(node.required) ? node.required.map(String).sort() : []; + assert.deepEqual(required, properties, `${location} must require every declared property`); + } + for (const [key, child] of Object.entries(node)) { + if (Array.isArray(child)) { + child.forEach((entry, index) => visit(entry, `${location}.${key}[${index}]`)); + } else { + visit(child, `${location}.${key}`); + } + } + }; + + visit(schema, "schema"); +}); + test("run-worker starts Codex in the target checkout when one is available", () => { const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "clawsweeper-run-worker-")); const fakeBin = path.join(tmp, "bin");