From 97833f7adc3bce50cb6fb3c859d45daf98ce2c31 Mon Sep 17 00:00:00 2001 From: Gaurav Jadhav Date: Thu, 22 Oct 2026 15:16:17 +0530 Subject: [PATCH 1/2] fix(proposal): drop only a still-ambiguous edit instead of the whole proposal A hunk's find text is unique in the file as read, but an earlier edit can repeat it once applied. The resulting 'must be unique' violation survived every re-prompt and discarded every other measured edit with it. Name each occurrence by line and say an earlier change repeats it, so the re-prompt can act on it. When the re-prompts are spent, drop only the edit that is still ambiguous, warn and record why in the proposal notes, and keep the rest. Other gate violations still fail the run loudly. Stage each edit's effect on the running text so a dropped edit leaves nothing half applied. Closes #200 Signed-off-by: Gaurav Jadhav --- README.md | 5 ++- src/proposal.js | 61 +++++++++++++++++++++++++++++++----- src/synthesize.js | 12 +++++++- test/proposal.test.js | 68 +++++++++++++++++++++++++++++++++++++++++ test/synthesize.test.js | 60 ++++++++++++++++++++++++++++++++++++ 5 files changed, 196 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index d7732089..99779217 100644 --- a/README.md +++ b/README.md @@ -516,7 +516,10 @@ keeps the merged change rather than guessing. A malformed answer or gate violation triggers a re-prompt naming the exact breach (at most two). If those also fail, backpass **fails loudly** and preserves the latest parseable -rejected proposal, if one was produced. It never silently truncates. A harness that writes +rejected proposal, if one was produced. It never silently truncates. The one edit it will +drop is a change whose anchor text still occurs more than once after the re-prompts +(an earlier edit repeats it); that edit alone is dropped, named in a warning and in the +proposal's notes, and the rest of the proposal goes to review. A harness that writes past the staging copy into the repo is an error, never an apply. Not every annotate turn is an answer, and the three cases are kept apart because they call diff --git a/src/proposal.js b/src/proposal.js index 947d26dc..7b7b2641 100644 --- a/src/proposal.js +++ b/src/proposal.js @@ -211,10 +211,37 @@ function occurrences(haystack, needle) { return count; } +/** `Error.code` of a `find` that matches more than once, so callers can tell it from a stale one. */ +export const AMBIGUOUS_FIND = "AMBIGUOUS_FIND"; + +/** 1-based line where each occurrence of `needle` starts (overlapping ones included), capped for display. */ +function occurrenceLines(haystack, needle, cap = 5) { + const lines = []; + let index = haystack.indexOf(needle); + while (index !== -1 && lines.length < cap) { + // A tail `find` carries the newline before its first line (see `span` in diff.js). + const start = index + (needle.startsWith("\n") ? 1 : 0); + lines.push(haystack.slice(0, start).split("\n").length); + index = haystack.indexOf(needle, index + 1); + } + return lines; +} + +function ambiguousFind(text, find, found, label, file) { + const lines = occurrenceLines(text, find); + const more = found > lines.length ? `, and ${found - lines.length} more` : ""; + return Object.assign( + new Error( + `${label}: "find" text appears ${found} times in ${file} (lines ${lines.join(", ")}${more}); must be unique`, + ), + { code: AMBIGUOUS_FIND }, + ); +} + function replaceOnce(text, find, replace, label, file) { const found = occurrences(text, find); if (found === 0) throw new Error(`${label}: "find" text does not appear in ${file}`); - if (found > 1) throw new Error(`${label}: "find" text appears ${found} times in ${file}; must be unique`); + if (found > 1) throw ambiguousFind(text, find, found, label, file); return text.replace(find, () => replace); } @@ -380,6 +407,7 @@ export function buildProposal(rawResult, context) { scope = null, target = { kind: "surface" }, routing = null, + dropAmbiguous = false, } = context; const violations = []; @@ -754,35 +782,52 @@ export function buildProposal(rawResult, context) { // description line, or the description line(s) a created skill adds. Bodies stay // free-until-triggered and never enter it. const running = new Map(); - for (const edit of accepted) { + /** @type {{ id: string, title: string, reason: string }[]} */ + const dropped = []; + for (const edit of [...accepted]) { const targets = filesOfEdit(edit); let memoryDelta = 0; let otherDelta = 0; let descriptionDelta = edit.kind === "extract" ? createdDescriptionCost(edit) : 0; + // Staged until the whole edit applies, so a dropped edit leaves no half-applied file behind. + const next = new Map(); try { for (const target of targets) { const slice = sliceEditForFile(edit, target); const before = running.get(target) ?? (target === memoryFile.path ? memoryFile.text : (measured.originals?.get(target) ?? "")); - const next = applyEdit(before, slice); - running.set(target, next); - const delta = estimateTokens(next) - estimateTokens(before); + const after = applyEdit(before, slice); + next.set(target, after); + const delta = estimateTokens(after) - estimateTokens(before); if (target === memoryFile.path) memoryDelta += delta; else { otherDelta += delta; if (isSkillFilePath(target, config.skillDirs || config.skillsDir)) { - descriptionDelta += descriptionLineDelta(before, next) * loadedCopies(repo.root, skillFiles, target); + descriptionDelta += descriptionLineDelta(before, after) * loadedCopies(repo.root, skillFiles, target); } } } } catch (err) { - violations.push(err.message); + const ambiguous = err.code === AMBIGUOUS_FIND; + if (ambiguous && dropAmbiguous) { + // Only the still-ambiguous edit goes; every other edit already cleared its gates. + accepted.splice(accepted.indexOf(edit), 1); + dropped.push({ id: edit.id, title: edit.title, reason: err.message }); + notes.push(`dropped edit ${edit.id} (${edit.title}): ${err.message}`); + continue; + } + violations.push( + ambiguous + ? `${err.message}; an earlier change in this proposal repeats it (line numbers count the file as the changes before it leave it) - reword or remove the repeated copy, or merge the two changes into one edit` + : err.message, + ); edit.applicable = false; edit.deltaTokens = 0; edit.descriptionDelta = 0; continue; } + for (const [target, text] of next) running.set(target, text); edit.applicable = true; edit.deltaTokens = edit.targetsMemoryFile ? memoryDelta : otherDelta; edit.descriptionDelta = descriptionDelta; @@ -901,7 +946,7 @@ export function buildProposal(rawResult, context) { notes, }; - return { proposal, violations }; + return { proposal, violations, dropped }; } // Display counter for the apply surface's funnel: the instructions that drew a negative diff --git a/src/synthesize.js b/src/synthesize.js index 0cf5f236..68310047 100644 --- a/src/synthesize.js +++ b/src/synthesize.js @@ -493,7 +493,17 @@ async function annotateLoop({ continue; } - const { proposal, violations } = buildProposal(parsed, { ...context, measured }); + let built = buildProposal(parsed, { ...context, measured }); + // The re-prompts are spent: an edit whose `find` is still ambiguous goes alone, named in + // the notes, instead of discarding every other edit that cleared its gates (#200). + if (built.violations.length && attempts >= ANNOTATE_TURNS) { + const retried = buildProposal(parsed, { ...context, measured, dropAmbiguous: true }); + if (retried.dropped.length) { + built = retried; + for (const { id, title, reason } of retried.dropped) warn(`dropping edit ${id} (${title}): ${reason}`); + } + } + const { proposal, violations } = built; proposal.notes = [...proposal.notes, ...notes]; proposal.usage = usage; proposal.overflowTarget = overflow; diff --git a/test/proposal.test.js b/test/proposal.test.js index db244984..ada67c2a 100644 --- a/test/proposal.test.js +++ b/test/proposal.test.js @@ -7,6 +7,7 @@ import vm from "node:vm"; import { applyEdit, + applyEdits, buildProposal, DEFAULT_MAX_EDITS, effectiveMaxEdits, @@ -170,6 +171,73 @@ test("an edit whose find text is missing or ambiguous is refused, never guessed ); }); +test("an ambiguous find names every occurrence by line so the re-prompt can say where", () => { + const doubled = `${MEMORY_TEXT}\n- Whenever a PR is mentioned, include its URL.\n`; + assert.throws( + () => + applyEdit(doubled, { + id: "e1", + file: "AGENTS.md", + find: "- Whenever a PR is mentioned, include its URL.", + replace: "x", + }), + /appears 2 times in AGENTS\.md \(lines 5, 9\); must be unique/, + ); +}); + +// An earlier edit copies a bullet; a later edit rewrites the original. Each `find` is unique +// in the file as read, but not once the earlier edit has landed. +const COPY_TEXT = [ + ...Array.from({ length: 6 }, (_, i) => `- Rule number ${i + 1} is unique.`), + "- Keep transactions short.", + "- Rule number 7 is unique.", + "- Rule number 8 is unique.", + "", +].join("\n"); +const copyAndRewrite = memoryEdit((t) => + t + .replace("- Rule number 1 is unique.\n", "- Rule number 1 is unique.\n- Keep transactions short.\n") + .replace("- Keep transactions short.\n- Rule number 7", "- Keep every transaction short.\n- Rule number 7") + .replace("- Rule number 8 is unique.", "- Rule number 8 is unique, and checked."), +); +const copyAnnotation = { + edits: [claim(["H1"], { title: "copy" }), claim(["H2"], { title: "rewrite" }), claim(["H3"], { title: "tail" })], +}; + +test("a non-unique find after an earlier edit lands is a violation that says where the text repeats", () => { + const { violations, proposal } = gate({ text: COPY_TEXT, edit: copyAndRewrite, annotation: copyAnnotation }); + assert.equal(violations.length, 1, violations.join("\n")); + assert.match(violations[0], /appears 2 times in AGENTS\.md \(lines 2, 8\); must be unique/); + assert.match(violations[0], /an earlier change in this proposal repeats it/); + assert.equal(proposal.edits.find((e) => e.title === "rewrite").applicable, false); +}); + +test("with dropAmbiguous only the edit whose find is still ambiguous is dropped, named, and the rest is kept", () => { + const { violations, proposal, dropped, memoryFile } = gate({ + text: COPY_TEXT, + edit: copyAndRewrite, + annotation: copyAnnotation, + context: { dropAmbiguous: true }, + }); + assert.deepEqual(violations, []); + assert.deepEqual( + proposal.edits.map((e) => e.title), + ["copy", "tail"], + ); + assert.equal(dropped.length, 1); + assert.equal(dropped[0].title, "rewrite"); + assert.match(dropped[0].reason, /appears 2 times in AGENTS\.md \(lines 2, 8\)/); + assert.ok( + proposal.notes.some((n) => /dropped edit .* \(rewrite\)/.test(n)), + proposal.notes.join("\n"), + ); + const applied = applyEdits( + memoryFile.text, + proposal.edits.map((e) => e), + ); + assert.ok(applied.includes("and checked.") && !applied.includes("every transaction")); +}); + test("a measured edit applies each of its hunks, and refuses once the file has drifted", () => { const { proposal, violations } = gate({ edit: memoryEdit((t) => t.replace("include its URL.", "include its full URL.").replace("Node 18", "Node 22")), diff --git a/test/synthesize.test.js b/test/synthesize.test.js index 7807e2ae..04ec75ca 100644 --- a/test/synthesize.test.js +++ b/test/synthesize.test.js @@ -412,6 +412,66 @@ test("the budget gate is measured on the staged file: growth on an over-budget f assert.ok(proposal.usage.length >= 3, "the re-prompt turn is accounted"); }); +// An edit copies a bullet and another rewrites the original: each `find` is unique in the +// file as read, but the second is ambiguous once the first has landed (#200). +const COPY_AGENTS = [ + "# Memory", + "", + ...Array.from({ length: 6 }, (_, i) => `- Rule number ${i + 1} is unique.`), + "- Keep transactions short.", + "- Rule number 7 is unique.", + "- Rule number 8 is unique.", + "", +].join("\n"); +const copyScript = (annotation) => ({ + edit: { + "AGENTS.md": { + replace: [ + ["- Rule number 1 is unique.\n", "- Rule number 1 is unique.\n- Keep transactions short.\n"], + ["- Keep transactions short.\n- Rule number 7", "- Keep every transaction short.\n- Rule number 7"], + ["- Rule number 8 is unique.", "- Rule number 8 is unique, and checked."], + ], + }, + }, + annotations: [{ reply: annotation }], +}); +const titled = (changes, title, extra = {}) => ({ ...tighten(changes), title, ...extra }); + +test("one still-ambiguous edit is dropped and named after the re-prompts, and the other edits survive", async () => { + const { repo, run } = setup( + copyScript({ edits: [titled(["H1"], "copy"), titled(["H2"], "rewrite"), titled(["H3"], "tail")] }), + { text: COPY_AGENTS }, + ); + const { proposal, violations } = await run(); + assert.deepEqual(violations, []); + assert.deepEqual( + proposal.edits.map((e) => e.title), + ["copy", "tail"], + ); + assert.ok( + proposal.notes.some((n) => /^dropped edit .* \(rewrite\): .*appears 2 times in AGENTS\.md \(lines 4, 10\)/.test(n)), + proposal.notes.join("\n"), + ); + const reprompt = fs.readFileSync(path.join(repo.root, ".backpass", "prompts", "synthesis-annotate-2.md"), "utf8"); + assert.match(reprompt, /appears 2 times in AGENTS\.md \(lines 4, 10\)/); + assert.match(reprompt, /an earlier change in this proposal repeats it/); +}); + +test("dropping an ambiguous edit never hides another gate violation", async () => { + const { run } = setup( + copyScript({ + edits: [titled(["H1"], "copy"), titled(["H2"], "rewrite"), titled(["H3"], "tail", { evidence: [] })], + }), + { text: COPY_AGENTS }, + ); + await assert.rejects(run(), (err) => { + assert.ok(err instanceof ProposalViolation); + assert.ok(err.violations.some((v) => /carries no verbatim evidence quote/.test(v))); + assert.ok(!err.violations.some((v) => /must be unique/.test(v)), "the dropped edit is no longer a violation"); + return true; + }); +}); + test("when every re-prompt fails the gates, synthesis fails loudly and keeps the rejected proposal", async () => { const { config, run } = setup({ edit: { "AGENTS.md": { replace: [[TWO_ITEMS, ""]] } }, From 1e46faf7de0264c08f719337fe34a8efc97f06c8 Mon Sep 17 00:00:00 2001 From: Gaurav Jadhav Date: Fri, 23 Oct 2026 10:14:11 +0530 Subject: [PATCH 2/2] fix(nested): never reuse a live edit id when merging nested proposals Dropping a middle edit (e2 of e1..e3) leaves [e1, e3]; the first nested edit then took e${edits.length + 1}, a second e3. Review and apply key decisions by edit.id, so a rejected change could be applied. Allocate nested ids that no edit already holds. Also pin that a dropped edit spanning several files leaves none of them changed and its token cost and created skill out of the proposal. Closes #200 Signed-off-by: Gaurav Jadhav --- src/nested.js | 13 +++++- test/nested.test.js | 97 +++++++++++++++++++++++++++++++++++++++++++ test/proposal.test.js | 70 +++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 2 deletions(-) diff --git a/src/nested.js b/src/nested.js index 4f88753e..6e586c34 100644 --- a/src/nested.js +++ b/src/nested.js @@ -380,7 +380,7 @@ export function owningFile(sessionIds, weights, attribution) { /** * One proposal over every trained file, reviewed in one apply: the root proposal, then - * each nested file's edits renumbered after it and labeled with their file. A nested + * each nested file's edits numbered after it, never reusing an id, and labeled with their file. A nested * file's image joins `targetFiles`, so apply refuses it if it changed since, the same * freshness contract every non-root target has; `nested` carries each file's own budget, * which apply re-checks against the accepted subset. With `sampledSessions` and @@ -391,11 +391,20 @@ export function mergeNestedProposals(root, passes, { sampledSessions = null, min const targetFiles = [...(root.targetFiles || [])]; const usage = [...(root.usage || [])]; const notes = [...(root.notes || [])]; + // Review and apply key decisions by id, so an id is never reused. A root proposal can have + // a gap (a dropped ambiguous edit), which makes "one past the count" a live neighbour's id. + const taken = new Set(edits.map((edit) => edit.id)); + const nextId = () => { + let serial = edits.length + 1; + while (taken.has(`e${serial}`)) serial += 1; + taken.add(`e${serial}`); + return `e${serial}`; + }; const nested = passes.map((pass) => { const { weight, proposal } = pass; const ids = []; for (const edit of proposal?.edits || []) { - const id = `e${edits.length + 1}`; + const id = nextId(); edits.push({ ...edit, id, targetsMemoryFile: false, nestedMemoryFile: weight.path }); ids.push(id); } diff --git a/test/nested.test.js b/test/nested.test.js index d1a2316b..b6a755cc 100644 --- a/test/nested.test.js +++ b/test/nested.test.js @@ -812,6 +812,103 @@ test("one proposal covers every file: nested edits are renumbered, labeled by fi ); }); +test("a root edit dropped from the middle never leaves its id to a nested edit", () => { + // The model's second edit rewrites a line the first one copied, so its `find` is ambiguous + // once the copy lands and the last run of synthesis drops it: the root is left with [e1, e3]. + const rootText = [ + ...Array.from({ length: 6 }, (_, i) => `- Rule number ${i + 1} is unique.`), + "- Keep transactions short.", + "- Rule number 7 is unique.", + "- Rule number 8 is unique.", + "", + ].join("\n"); + const repo = makeRepo({ "AGENTS.md": rootText, [API.path]: "# API\n\n- Handlers live in src/routes.\n" }); + const config = { budgetTokens: 5000, maxEditsPerRun: 5, minGapEvidence: 2, skillsDir: ".agents/skills" }; + const evidence = [ + { polarity: "negative", text: "it hit this twice", source: "a" }, + { polarity: "negative", text: "and again", source: "b" }, + ]; + const edit = (change, title) => ({ changes: [change], kind: "rewrite", title, evidence }); + + const rootStaged = stageAndMeasure({ + repo, + edit: (dir) => + writeIn(dir, "AGENTS.md", (text) => + text + .replace("- Rule number 1 is unique.\n", "- Rule number 1 is unique.\n- Keep transactions short.\n") + .replace("- Keep transactions short.\n- Rule number 7", "- Keep every transaction short.\n- Rule number 7") + .replace("- Rule number 8 is unique.", "- Rule number 8 is unique, and checked."), + ), + }); + const root = buildProposal( + { edits: [edit("H1", "copy"), edit("H2", "rewrite"), edit("H3", "tail")] }, + { + memoryFile: rootStaged.memoryFile, + config, + repo, + summary: null, + measured: rootStaged.measured, + dropAmbiguous: true, + }, + ); + assert.deepEqual(root.violations, []); + assert.deepEqual( + root.proposal.edits.map((e) => [e.id, e.title]), + [ + ["e1", "copy"], + ["e3", "tail"], + ], + ); + + const apiStaged = stageAndMeasure({ + repo, + memoryPath: API.path, + edit: (dir) => writeIn(dir, API.path, (text) => `${text}- Run the api contract tests before editing a handler.\n`), + }); + const api = buildProposal( + { edits: [edit("H1", "contract tests")] }, + { + memoryFile: apiStaged.memoryFile, + config, + repo, + summary: null, + measured: apiStaged.measured, + target: { kind: "memory", path: API.path, nested: true }, + }, + ); + const proposal = mergeNestedProposals(root.proposal, [ + { + weight: { ...API, file: apiStaged.memoryFile }, + transcripts: [{}, {}], + proposal: api.proposal, + summary: null, + cap: 5000, + skipped: null, + }, + ]); + + const ids = proposal.edits.map((e) => e.id); + assert.equal(new Set(ids).size, ids.length, `edit ids must be unique: ${ids.join(", ")}`); + assert.deepEqual(ids, ["e1", "e3", "e4"]); + assert.deepEqual(proposal.nested[0].edits, ["e4"]); + + // Rejecting the root's e3 must not take the nested edit with it. + const result = applyDecisions({ + proposal, + decisions: { e1: "accepted", e3: "rejected", e4: "accepted" }, + repo, + state: rootStaged.state, + config: { ...config, nestedBudgetTokens: 5000, nestedMemoryFiles: [API.path] }, + }); + assert.deepEqual(result.failed, []); + assert.equal(result.accepted, 2); + assert.equal(result.rejected, 1); + const rootAfter = fs.readFileSync(path.join(repo.root, "AGENTS.md"), "utf8"); + assert.match(rootAfter, /- Rule number 1 is unique\.\n- Keep transactions short\./); + assert.doesNotMatch(rootAfter, /and checked/); + assert.match(fs.readFileSync(path.join(repo.root, API.path), "utf8"), /Run the api contract tests/); +}); + test("apply holds each nested file to its own budget, and writes nothing when the accepted subset breaks it", () => { // Made under a cap its one edit breaks: the writer is the gate of last resort even // for a proposal `cmdApply` would refuse for its violations. diff --git a/test/proposal.test.js b/test/proposal.test.js index ada67c2a..6ed86bde 100644 --- a/test/proposal.test.js +++ b/test/proposal.test.js @@ -1316,6 +1316,76 @@ const noHarmRows = () => ({ })), }); +test("a dropped extract that spans several files leaves none of them changed and costs nothing", () => { + // e1 copies a bullet to the end of node-setup; e2 extracts into node-setup (anchored on that + // bullet, so ambiguous once e1 lands) and a new skill, after rewriting AGENTS.md. Its first + // target applies and a later one does not: the whole extract must go, not half of it. + const extracted = "- Use Node 18 via nvm before running any script."; + const bullet = "- Prefer nvm over a system node."; + const existing = skillFile( + "node-setup", + [bullet, ...Array.from({ length: 6 }, (_, i) => `- Setup rule ${i + 1} is unique.`), ""].join("\n"), + ); + const staged = gate({ + files: { ".agents/skills/node-setup/SKILL.md": existing }, + edit: (root) => { + writeIn(root, "AGENTS.md", (t) => t.replace(extracted, "- See the setup skills.")); + writeIn( + root, + ".agents/skills/node-setup/SKILL.md", + (t) => `${t.replace(`${bullet}\n`, `${bullet}\n${extracted}\n`)}${bullet}\n`, + ); + writeIn(root, ".agents/skills/release-signing/SKILL.md", skillFile("release-signing")); + }, + annotation: { + edits: [ + claim(["H3"], { title: "copy" }), + claim(["H1", "H2", "H4"], { kind: "extract", title: "extend setup and add signing" }), + ], + }, + context: { summary: noHarmRows(), dropAmbiguous: true }, + }); + + assert.deepEqual(staged.violations, [], staged.violations.join("\n")); + assert.deepEqual( + staged.dropped.map((d) => [d.id, d.title]), + [["e2", "extend setup and add signing"]], + ); + assert.match(staged.dropped[0].reason, /appears 2 times in \.agents\/skills\/node-setup\/SKILL\.md/); + assert.deepEqual( + staged.proposal.edits.map((e) => e.title), + ["copy"], + ); + + // Nothing of the dropped extract is charged: the memory file keeps its line and tokens, + // no skill description is added, and no created skill or extension is counted or listed. + assert.equal(staged.proposal.budget.delta, 0); + assert.equal(staged.proposal.budget.projected, staged.proposal.budget.current); + assert.equal(staged.proposal.stats.skillExtractions, 0); + assert.deepEqual( + staged.proposal.edits.flatMap((e) => e.skills || []), + [], + ); + assert.deepEqual( + staged.proposal.targetFiles.map((t) => t.file), + [".agents/skills/node-setup/SKILL.md"], + ); + + const applied = applyDecisions({ + proposal: staged.proposal, + decisions: { e1: "accepted" }, + repo: staged.repo, + state: staged.state, + config: { budgetTokens: 5000 }, + }); + assert.equal(applied.failed.length, 0, JSON.stringify(applied.failed)); + assert.ok(fs.readFileSync(path.join(staged.repo.root, "AGENTS.md"), "utf8").includes(extracted)); + const skill = fs.readFileSync(path.join(staged.repo.root, ".agents/skills/node-setup/SKILL.md"), "utf8"); + assert.ok(!skill.includes(extracted), "the extension half of the dropped extract did not land"); + assert.equal(skill.split(bullet).length - 1, 2, "only the copy landed"); + assert.equal(fs.existsSync(path.join(staged.repo.root, ".agents/skills/release-signing/SKILL.md")), false); +}); + test("an extract may extend an existing SKILL.md when the staged file keeps prior lines and carries the removal", () => { const existing = skillFile("node-setup", "- Prefer nvm over a system node.\n"); const extracted = "- Use Node 18 via nvm before running any script.";