fix(edit): keep the indentation a fuzzy match spans - #780
Ishaan Gangwani (ishaan1124) merged 2 commits into
Conversation
|
ANIRUDDHA ADAK (@aniruddhaadak80) is attempting to deploy a commit to the InkVell Team on Vercel. A member of the Team first needs to authorize it. |
Ishaan Gangwani (ishaan1124)
left a comment
There was a problem hiding this comment.
Thanks. The single-line case is real and fixed, but candidate.trimStart() in backend/cli/src/tool/edit.ts is only right when the model quoted the line with no indentation at all. Two cases still come out wrong (main is also wrong here, so it's incomplete rather than a regression):
- A multi-line block quoted at column 0 against a file indented 8 spaces: the first line lands at 8 spaces and the next at 4, which is an
IndentationErrorin Python. - A line quoted at 4 spaces in a file indented 12 ends up at 16.
Strip only the extra padding the fuzzy match added, and add it to every line of the replacement. Slice instead of trimStart(), which can also eat a leading newline:
const pad = indent(candidate).slice(0, Math.max(0, indent(candidate).length - quoted))
const search = candidate.slice(pad.length)
const replacement = pad ? newString.replaceAll("\n", `\n${pad}`) : newStringPlease add tests for the multi-line and partly-indented cases.
A looser replacer widens the match from the quoted substring to the whole line, and the replacement was written for the text that was quoted, not the padding around it, so an indented statement was replaced at column 0. Match the text itself and let the file keep the indentation it had, compared against the quoted oldString so a deliberate outdent still applies.
Stripping the candidate's leading whitespace dropped the padding from the first line of a multi-line block only, so a block quoted at column 0 landed its first line at the file's depth and the rest at the quoted one, which is an IndentationError in Python, and a line quoted at 4 and matched at 12 stacked its own 4 on top of the 12 the file already had. Slice off exactly the padding the matcher added and add it back after every newline of the replacement, which also keeps a leading newline from being trimmed away.
e8487cf to
df7526b
Compare
|
Done in df7526b.
One note on the second case: a line quoted at 4 against a 12-space line is already matched by the exact replacer, because |
|
All checks are green on |
The requested changes are in and verified; CI is green.
a94dc3b
into
synthetic-sciences:main
What does this PR do, and why?
edithas a chain of progressively looser replacers so a model that re-quoted aline with slightly different spacing still lands its edit. One of them widens the
match from the quoted substring to the whole line:
replacethen substitutesnewStringacross that whole region:newStringwas written for the text the model quoted, not for the padding itnever saw. So one divergence in internal spacing turns this:
into this, reported as
Edit applied successfully.:That is an
IndentationErrorin a file the tool claims to have fixed, and in anauto-approved run nobody reads the diff.
LineTrimmedReplacer,BlockAnchorReplacer,IndentationFlexibleReplacerandContextAwareReplacerall widen the same way, so the fix belongs where the candidate is accepted rather
than in each replacer.
The fix keeps the file's padding and matches the text itself:
so the edit lands where the model looked, at the indentation the file already
had. On a CRLF file this also stops the widened match from swallowing the
\randturning one line LF-terminated.
The comparison is against the quoted text, not the replacement
That detail is load-bearing. Comparing against
newStringinstead would refusea deliberate outdent (
return x→return x), which is a valid edit.Comparing against the quoted
oldStringmeans only a widened match is trimmed,so an outdent the model asked for still applies. There is a test for exactly
that.
Linked issue
Self-identified; no upstream issue covers it. I did not open one because this
turn's issue budget went to the two
apply_patchdefects, which were the moreurgent "reported as applied but was not" cases. Happy to file one if you would
rather track it separately.
How did you verify it?
Four cases in
backend/cli/test/tool/edit-replace.test.ts, all against the purereplacefunction:a fuzzy line match keeps the indentation it matched— the repro; on3e94875cthe indentation is gone and the line sits at column 0an explicit de-indent is still applied— the guard against over-rejectingan indented line quoted with its indentation is replaced exactly— the normalpath is untouched
$-literalness tests, unchangedCommands run:
bun test --timeout 60000 ./test/tool/edit-replace.test.ts→ 5 pass, 0 failbun test --timeout 120000 ./test/tool/payload-integrity.test.ts ./test/tool/write-safety.test.ts→ 9 pass, 0 fail (the other two suites that touch
EditTool/replace)bun run --cwd backend/cli typecheck→ exit 0workspace-file-tools.test.tsalso drivesEditTool. It reported a failure onmy first run at exactly 120 s — the per-test timeout — and passes in 27.8 s when
re-run alone, so that was load on this machine, not a behaviour change. Its edit
is at indent 0, which the guard cannot affect. I am flagging the timing because I
would rather you trust the numbers than find the timeout yourself.
Checklist
bun run checkis green (format, typecheck, backend + frontend/ui + SDK tests) — blocked on this Windows checkout by the CRLF and symlink artifacts in my other pull requests; backend typecheck is clean and all fourEditTool/replacesuites are greenbun run --cwd frontend/workspace buildsucceeds if I touchedfrontend/workspaceorfrontend/ui— not touched./tooling/repo/generate.tswas run and thetooling/sdkoutput committed if I changedbackend/cli/src/server— not touchedfrontend/docs/src/content/openscience/is updated if behavior changed — no doc change needed: the looser replacers are an internal convenience and their documented contract is unchangedpackage.jsonversions and tags are written by the release workflow)installandfrontend/landing/public/installare still byte-identical if I touched either — not touched