Skip to content

fix(patch): a malformed patch is rejected instead of reported as applied - #826

Merged
Ishaan Gangwani (ishaan1124) merged 4 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/apply-patch-malformed-rejected
Sep 29, 2026
Merged

Ishaan Gangwani (ishaan1124) merged 4 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/apply-patch-malformed-rejected

Conversation

@aniruddhaadak80

@aniruddhaadak80 ANIRUDDHA ADAK (aniruddhaadak80) commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

apply_patch accepted two shapes of a malformed patch and reported both to the model as applied. An *** Update File: section with no @@ header left the file byte-for-byte unchanged while printing Success. Updated the following files:, and a hunk line that lost its leading space or sign was silently dropped, shrinking the context used to place the edit. Both shapes are now parse errors.

Root cause

Both defects are in parseUpdateFileChunks in backend/cli/src/patch/index.ts.

A change line with no @@ header above it was skipped. The section loop only starts a chunk on a line beginning @@ and incremented past everything else:

if (lines[i].startsWith("@@")) { /* ...make a chunk... */ } else { i++ }

So an update section written as

*** Begin Patch
*** Update File: a.py
-    old
+    new
*** End Patch

parsed to chunks: []. deriveNewContentsFromChunks then computed replacements = [], so the "new" content was the file's own bytes. The hunks.length === 0 guard in the apply path could not catch it, because the *** Update File: header had already produced one hunk - the tool saw a hunk with nothing in it and reported success with an empty diff.

A hunk line with no prefix was dropped from both sides. The change-line loop handled " ", "-" and "+" and had no else, so a line with none of those prefixes fell out of both old_lines and new_lines. That line is typically the only thing distinguishing one block from the next, and seekSequence then retried with progressively looser passes (rstrip, both-end trim, punctuation folding) to find somewhere to apply the shortened hunk. What the model asked for in one function can land in another.

Changes

  • backend/cli/src/patch/index.ts
    • The change-line loop now has an else that throws when a line carries no " ", "-" or "+" prefix, naming the file and the 1-based line number. The check runs before i++, so the line is never silently consumed.
    • The outer section loop now throws when a - or + line appears before any @@ header. Lines that are neither a change line nor a header (a blank line between two hunks) still advance normally, so that shape keeps working.
    • parseUpdateFileChunks takes the file path from its caller so both errors can name the file.
    • Both errors are raised from parsePatch, which the existing maybeParseApplyPatch wrapper already turns into MaybeApplyPatch.PatchParseError, so they surface through the normal tool-error path.
  • backend/cli/test/tool/apply_patch.test.ts
    • rejects an update section that has no hunk header: the first defect. Uses the exact patch from the report and also asserts the file on disk is unchanged.
    • rejects a hunk line that lost its prefix: the second defect. The patch is one that succeeds and applies wrongly today, so it is a real regression rather than a shape that merely reads as malformed; it also asserts the file is unchanged.
  • CHANGELOG.md
    • A bullet under Unreleased -> Fixed, per AGENTS.md ("User-visible behaviour -> a line under Unreleased").

Deliberately not changed

Two shapes stay as they are, matching the scope in the report:

  • A line that is indented is still read as a context line whose first space is the prefix. No parser can tell x from a lost-prefix x, so this one is left alone on purpose.
  • A blank line between two hunks still separates them. Only a -/+ line outside a hunk is rejected, so a section with @@ ... @@ and a blank between them parses exactly as before.

Testing

Both new tests fail on f1bdf252 with the same assertion, because the tool returns a successful result:

$ bun test --timeout 15000 ./test/tool/apply_patch.test.ts -t "rejects an update section that has no hunk header"
error:
Expected promise that rejects
Received promise that resolved: Promise { <resolved> }
(fail) tool.apply_patch freeform > rejects an update section that has no hunk header
 0 pass
 1 fail

With the fix, the whole file is green:

$ bun test --timeout 15000 ./test/tool/apply_patch.test.ts
 42 pass
 0 fail
 100 expect() calls
Ran 42 tests across 1 file. [15.54s]

This includes the pre-existing rejects missing second chunk context, which contains a blank line inside a hunk and still rejects (now with the clearer message), and the two insert-only hunks tests, which exercise hunk shapes this change touches.

Typecheck:

$ bun run typecheck
@synsci/openscience:typecheck: $ tsgo --noEmit      # passes
Tasks:    4 successful, 7 total
Failed:   @synsci/workspace#typecheck

The one failure is frontend/workspace/src/custom-elements.d.ts(1,1), which is pre-existing on main and unrelated to this change - the committed file is 33 bytes containing the bare string ../../ui/src/custom-elements.d.ts, which is not a valid declaration file. git status shows it unmodified against HEAD, so the breakage is in the commit, not in this working tree. The backend package this PR changes typechecks clean.

The repo's gitleaks pre-commit hook ran on the commit and reported no leaks found.

Fixes #775

Two shapes of a malformed patch were accepted and reported to the model as
applied.

An `*** Update File:` section with no `@@` header parsed to zero chunks, because
the section loop only starts a chunk on a line beginning `@@` and skipped every
other line. `deriveNewContentsFromChunks` then computed zero replacements, so the
"new" content was the file's own bytes and the tool printed
`Success. Updated the following files:` with an empty diff. The
`hunks.length === 0` guard could not see it, because the `*** Update File:`
header had already produced one hunk.

A hunk line with no leading space or sign was dropped from both `old_lines` and
`new_lines`, because the change-line loop handled `" "`, `"-"` and `"+"` with no
`else`. That line is usually the only thing distinguishing one block from
another, and `seekSequence` then retried with progressively looser trimming, so
an edit asked for in one function could land in another.

Both are now parse errors, which the existing `MaybeApplyPatch.PatchParseError`
path already reports as a tool error. Two shapes are deliberately left alone: a
line that is merely indented is still read as a context line, since no parser
can tell `   x` from a lost-prefix `    x`, and a blank line between two hunks
still separates them.

Fixes synthetic-sciences#775
@vercel

vercel Bot commented Sep 29, 2026

Copy link
Copy Markdown

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.

The malformed-hunk guard rejects a change line that appears before any \@@\
header, and a \Move to\ with no hunk below it has no change line, so it
passes through with an empty chunk list. That path is valid input per the
tool description and has to keep renaming the file.
…n empty update

The malformed-line check also fired on an empty line, so a blank line
before `*** End Patch` or between file sections failed a patch that main
applies. An empty line inside a hunk is blank context, as in the Codex
format; trailing blank lines at the end of a hunk are dropped as
separators. An `*** Update File:` with no hunk and no `*** Move to:`
still reported success while changing nothing, so it is now rejected;
rename-only updates keep working.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ishaan1124

Copy link
Copy Markdown
Member

I pushed one commit (d9a7f49) on top of yours. The no-prefix check also fired on an empty line, so a blank line before *** End Patch or between file sections failed patches that main applies. A bare empty hunk line is now blank context, trailing blank lines are dropped as separators, and an *** Update File: with no hunk and no *** Move to: is rejected. Rename-only updates still work, and the tests cover all of these cases. CI is green.

@ishaan1124
Ishaan Gangwani (ishaan1124) merged commit afcb78c into synthetic-sciences:main Sep 29, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A malformed patch is reported as applied when nothing was changed

2 participants