Repository navigation
fix(proposal): drop only a still-ambiguous edit instead of the whole proposal - #225
jadhavgaurav wants to merge 2 commits into
Conversation
…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 kunchenguid#200 Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Medium impact] The PR appears safe to merge; no blocking issues remain. Reviews (2) · Last reviewed commit: "fix(nested): never reuse a live edit id ..." · Reviewed by Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97833f7adc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| 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); |
There was a problem hiding this comment.
Keep surviving IDs unique across nested merges
When a root run drops a non-final edit (for example e2 from [e1, e2, e3]) and also has a nested-memory proposal, this splice leaves the root IDs as [e1, e3]; mergeNestedProposals then assigns its first nested edit e${edits.length + 1} at src/nested.js:398, producing a second e3. The review/apply paths key decisions by edit.id (src/apply/writer.js:305-306), so those two edits cannot be accepted or rejected independently and a file change the reviewer rejected can be applied; renumber survivors or allocate nested IDs from an actually unused ID.
Useful? React with 👍 / 👎.
|
Speaking as Kun's firstmate: thanks @jadhavgaurav for picking up #200. Putting line numbers in the re-prompt is a clear win, and staging each edit's targets in This is waiting on you first. Two review findings on
CI and the no-mistakes check also need to be green. Heads-up so nothing surprises you later: once those are fixed, this goes to Kun for a decision before it merges. On main, the README says that when the re-prompts are spent, backpass fails loudly and keeps the rejected proposal. This PR replaces that documented default with a partial proposal (one edit dropped and named) that goes on to review. That may well be the right call, but it changes the default path, so it's Kun's decision. contract-class: new-default. The unconfigured run now hands a partial proposal to review where the specified path failed loudly. A bugfix motive doesn't make a replaced default into a restore. (Our earlier issue triage called this restore; on reading the tip against the README, I'm correcting that.) VISION per-rule (main
|
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 kunchenguid#200
Signed-off-by: Gaurav Jadhav <gaurav.vjadhav01@gmail.com>
|
Thanks. Both findings are addressed in 1e46faf.
|
|
Speaking as Kun's firstmate: thanks @jadhavgaurav, both findings are handled well in This is waiting on you for one last thing: live evidence. Everything so far ran against the fake acpx. Could you run a real After that, it goes to Kun for a decision, not back to you. As noted before, the README says that once the re-prompts are spent, backpass fails loudly and keeps the rejected proposal, and this PR replaces that documented default with a partial proposal. contract-class: new-default. The unconfigured run now produces a partial proposal where the documented default fails loudly. VISION per-rule (main
|
|
Live runs against a real harness, on head Setup
What I tried to provoke The ambiguity needs a hunk whose
Run 1 (duplicate bullet in two sections) (trimmed: edits e2 and e3 were two more REWRITEs, notes omitted.) The one violation was not an ambiguous anchor. The re-prompt text saved in So the model tried to merge the two copies, hit the harm-evidence gate, and rewrote one copy instead. Each copy is widened with its surrounding context, so no anchor stayed ambiguous. Run 2 (duplicate twice at the bottom) Again the violation was the harm-evidence gate on a dedupe attempt ( Run 3 (no duplicate, rules filed last) No warnings. The model did make the move I was hoping for, but A single unique line moved upward, or a contiguous block moved intact, would be the shape that trips it, and the model did not choose that shape in any of the three runs. What this does and does not show
|
|
Speaking as Kun's firstmate: thanks @jadhavgaurav, three real synthesis runs on This is now with Kun for a decision, not waiting on you. As flagged before, the README says that once the re-prompts are spent backpass fails loudly and keeps the rejected proposal, and this PR replaces that documented default with a partial proposal (the ambiguous edit dropped and named) that goes on to review. CI is green on ubuntu, macOS, the generated-files guard, and no-mistakes, and it merges cleanly onto main contract-class: new-default. The unconfigured run now produces a partial proposal where the documented default fails loudly. VISION per-rule (main
|
Intent
Fix issue #200: one non-unique hunk anchor ("find" text appears N times; must be unique) discards the whole synthesis proposal after the re-prompts are spent. Tell the re-prompt where each occurrence is and what repeats it, and when the re-prompts are exhausted drop only the still-ambiguous edit, naming why in a warning and the proposal notes, keeping every other edit that cleared its gates. Other violations still fail the run loudly. Closes
#200.What Changed
src/proposal.js:replaceOncenow raises anAMBIGUOUS_FIND-coded error that reports the 1-based line of each occurrence of a non-uniquefindanchor (capped, with a "+N more" tail), so a re-prompt can point at exactly what repeats.buildProposalgains adropAmbiguousoption: when set, an edit whose anchor is still ambiguous is spliced out ofacceptedand recorded in a newdroppedlist (with id, title, and reason) instead of failing the whole proposal; its target files are staged separately so a dropped edit leaves no partially-applied file behind. Other violations are unaffected and still fail the run.src/synthesize.js: inannotateLoop, once the re-prompt budget (ANNOTATE_TURNS) is exhausted and violations remain, the loop rebuilds the proposal withdropAmbiguous: true; if that drops anything, the retried proposal is used and a warning is emitted per dropped edit viawarn(...).README.md: documents that an edit whose anchor is still ambiguous after re-prompts is dropped alone, named in a warning and in the proposal notes, while the rest of the proposal proceeds to review.test/proposal.test.jsandtest/synthesize.test.jsfor the new drop-only-the-ambiguous-edit behavior.mergeNestedProposals(src/nested.js) gives each nested edit an id no edit already holds. A dropped middle edit leaves ids like[e1, e3], and numbering by count gave the first nested edit a seconde3; review and apply key decisions byedit.id, so a rejected change could have been applied.test/nested.test.jsruns a drop together with a nested merge and applies decisions by id;test/proposal.test.jsadds a multi-target extract where the first target applies and a later one is ambiguous.Risk Assessment
✅ Low: The change is well-bounded to the stated fix (issue #200), gated precisely to fire only after re-prompts are exhausted, correctly excludes dropped edits from all downstream accounting (edits list, budget, token deltas), leaves non-ambiguous violations fully blocking, and is backed by behavioral tests with an independent oracle (hand-computed line numbers) that verify both the proposal-level and synthesis-loop-level behavior; it also incidentally fixes a pre-existing partial-apply leak into the
runningmap for multi-target edits.Testing
pnpm run check(eslint, prettier, tsc, 958 tests) passes on 1e46faf.test/nested.test.js: a root with a dropped middle edit (e1,e3) merged with a nested edit gets unique ids (e1,e3,e4), and acceptinge1ande4while rejectinge3writes the nested file and not the rejected root edit. With onlysrc/nested.jsreverted (saved patch,git apply -R) it fails withedit ids must be unique: e1, e3, e3.test/proposal.test.js: a dropped extract that spansAGENTS.md, an existing skill and a created skill leaves the surviving proposal with the budget delta at 0, no skill extraction counted, no created skill, and apply changes none of the three files for it. With the per-editnextstaging changed to writerunningafter each target (a temporary edit tosrc/proposal.js, reverted), this test fails and nothing else does.Pipeline
Updates from git push no-mistakes
✅ **Intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
node --test test/nested.test.js test/proposal.test.js test/synthesize.test.js (163 pass, 0 fail)git status --porcelain (clean worktree, no transient artifacts)✅ **Document** - passed
✅ No issues found.
package.json- Lint/format/typecheck could not be run: node_modules is not installed in this worktree and no eslint/prettier binaries are on PATH or available locally; installing dependencies is outside this phase's scope.✅ **Push** - passed
✅ No issues found.
Issues
Closes #200