Leave a patch that closed its fence without a REPLACE marker as written - #6200
jurgenwerk wants to merge 4 commits into
Conversation
The fence widening that keeps a patch with fenced markdown inside in one block looked for the next REPLACE marker anywhere after the opener. A patch that closed its fence without writing the marker was treated as still streaming: its opener was widened past its own closing fence, so the block ran to the end of the message with the fence and the prose after it rendered inside, or it took the next patch's REPLACE marker as its own and the two blocks became one, writing the second file's content into the first. A block whose own closing fence comes before any REPLACE marker is now left alone. Inner fences are skipped in pairs, so a streaming patch whose inner block has closed is still widened, and the plan-with-layouts case keeps its one block. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The PR body links it by commit, so it stays visible there. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34a5cd482c
ℹ️ 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".
| if (bare && innerFences % 2 === 0) { | ||
| return j; |
There was a problem hiding this comment.
When a valid patch writes Markdown whose nested code block starts with a bare ``` rather than an info-bearing fence such as ```text, innerFences is still even here, so this returns the nested opener as the patch's own closing fence. Because the actual REPLACE marker comes later, the caller skips widening entirely; Markdown rendering then splits the patch at that inner opener, causing the otherwise complete patch to be reported as malformed and not applied.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Code 🤖] Fixed in b7f0411: a bare fence met while no inner block is open is taken as the patch's own close only when nothing fenced follows it or another patch opens before the next REPLACE marker; otherwise it opens an inner block. The case in this thread is now covered by a unit test.
habdelra
left a comment
habdelra
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] Reviewed the fence-widening change against the inputs it has to keep working — the two failure modes it fixes, the streaming case it preserves, and the plan-with-inner-fences case that motivated the widening in the first place — by running widenFencesAroundCodePatches from both main and this branch over each body. Not covered: the applier downstream of the parser, and the Matrix/room plumbing.
One blocking regression: a patch whose markdown content opens an inner fence with a bare ``` (no info string) is no longer widened, so the early-close bug the widening exists to prevent returns for it. Both new failure modes are genuinely fixed and the two tests covering them do fail without the change — I confirmed they discriminate main from this branch. Every existing inner-fence fixture uses ```text, which is why CI stays green on the regression.
Codex's P1 on indexOfPatchClosingFence is the same finding; it reproduces, so it is not a false positive.
indexOfPatchClosingFencereturns an unlabelled inner opener as the patch's own closer — reproducer and a discriminator that handles all five cases, in the thread onpackages/runtime-common/marked-sync.ts.- The new streaming test duplicates the existing one and passes on
main; the bare-inner-fence body is the case that would guard this change — thread onpackages/host/tests/unit/marked-sync-test.ts.
CI was still running at review time; nothing red to act on.
Generated by Claude Code
| let bare = m[3].trim() === '' && m[2].length >= fenceLength; | ||
| if (bare && innerFences % 2 === 0) { |
There was a problem hiding this comment.
[Claude Code 🤖] A complete patch whose markdown content opens an inner fence without an info string loses its widening, so the early-close bug this widening exists to fix comes back for that patch.
bare is true for an inner opener that carries no info string, and innerFences is still 0 there, so this returns the inner opener as the patch's own closing fence. replaceIndex is later, the caller takes the leave-as-written branch, and the patch is emitted unwidened — marked then closes it at that inner fence, which is exactly the failure the header comment above FILE_URL_LINE_PATTERN describes.
Verified by running both revisions of widenFencesAroundCodePatches over this body (markers elided):
```md
https://example.com/realm/plan.md (new)
<SEARCH>
<SEPARATOR>
# Plan
box
more
<REPLACE>
`main` widens the opener to ` ````md `; this branch leaves the body unchanged. Every inner-fence fixture in `marked-sync-test.ts` opens with ` ```text `, so nothing in the suite catches it.
**The fix.** Parity alone can't separate an unlabelled inner opener from the patch's own closer — what separates them is what follows the candidate. A candidate is the patch's own close when no REPLACE marker follows it, *or* when a patch opener (fence + url line + SEARCH marker) sits between it and the next REPLACE marker; otherwise that REPLACE marker belongs to this patch and it should still be widened:
```ts
let ownClose = indexOfPatchClosingFence(lines, i + 1, fenceLength);
if (
ownClose !== -1 &&
(replaceIndex === -1 ||
(replaceIndex > ownClose &&
hasPatchOpener(lines, ownClose + 1, replaceIndex)))
) {
i = ownClose + 1;
continue;
}
with hasPatchOpener reusing the FILE_URL_LINE_PATTERN / SEARCH_MARKER_PATTERN pair already used for isPatch. I ran this against five bodies — the two new tests, the preserved streaming case, PLAN_PATCH_WITH_INNER_FENCES, and the bare-inner-fence body above — and it gives the intended answer on all five, where the current branch gets the last one wrong.
Regression, introduced here. Blocking.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Confirmed and fixed in b7f0411. Parity alone could not tell a bare inner opener from the patch's own close, as you showed. The closer lookup now decides by what follows a bare fence met while no inner block is open: it is the patch's own close when nothing fenced follows it at all, or when another patch opens before the next REPLACE marker; otherwise it opens an inner block and the next bare fence closes it. An opener with an info string still counts as inner unconditionally.
Ran both revisions over the five bodies you named plus the streaming form of the bare-inner one: the two unterminated messages stay unchanged, the other four are widened, matching main on those. The bare-inner case is now the third test, complete and streaming.
| test('widenFencesAroundCodePatches still widens a streaming patch whose inner fence has closed', function (assert) { | ||
| let streaming = `\`\`\`md | ||
| https://example.com/realm/plan.md (new) | ||
| ${SEARCH_MARKER} | ||
| ${SEPARATOR_MARKER} | ||
| # Plan | ||
|
|
||
| \`\`\`text | ||
| box | ||
| \`\`\` | ||
|
|
||
| more plan te`; | ||
| assert.true( | ||
| widenFencesAroundCodePatches(streaming).startsWith('````md\n'), | ||
| 'an inner opener and closer pair does not read as the patch closing', | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Claude Code 🤖] This fixture is the same body as widenFencesAroundCodePatches widens the opener of a patch that is still streaming above, with a weaker assertion, so it pins nothing that test doesn't already pin — both pass on main and on this branch.
The case that would actually guard the new branch is the one it gets wrong: a patch whose inner fence is bare rather than ```text (see the comment on indexOfPatchClosingFence). Swapping this test for that one gives the change a test that fails without it.
Non-blocking on its own; it becomes the coverage for the blocking finding.
Generated by Claude Code
There was a problem hiding this comment.
[Claude Code 🤖] Swapped in b7f0411. The duplicate streaming test is gone; in its place is "a patch whose inner fence has no info string is still widened", which asserts the widened opener and closer, one code block with the inner fences kept as file content, and the streaming prefix of the same body still widened. It fails on the previous commit of this branch and passes now.
…lows it A bare fence inside a patch's markdown content opened an inner block just as a fence with an info string does, but was read as the patch closing early, so a complete patch with such content lost its widening and the early-close bug returned for it. A bare fence met while no inner block is open is the patch's own close only when nothing fenced follows it, or when another patch opens before the next REPLACE marker; otherwise it opens an inner block and the next bare fence closes it. The duplicate streaming test is replaced by the bare-inner-fence case, complete and streaming. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
2 participants
(Written by Claude on Matic's behalf.)
A model wrote a
(new)file block, left out the closing╚═══ REPLACE ═══╝, closed the fence and went on with prose. The host rendered the fence and the prose inside the code view and reported the block as malformed. In another room the same omission on the first of two blocks made the host apply one patch spanning both: the second file's content, its url and its markers were written into the first file.Both come from the fence widening that keeps a patch containing fenced markdown in one block. It looks for the next REPLACE marker anywhere after the opener and treats a block with none as still streaming, widening the opener past the block's own closing fence. With a later block in the message, that later block's marker is taken as this one's and the two merge.
Now a block whose own closing fence comes before any REPLACE marker is left as written. The parser still reports it as malformed, but bounded by its fence, and the patch after it stays its own block. Inner fences are skipped in pairs when looking for that closing fence, so a streaming patch whose inner block has already closed is still widened, and the plan-with-layouts case keeps its one block.
Two tests cover the unterminated block alone and followed by a second patch, one keeps the streaming case widened. Verified against the two real room messages: the widening now leaves both unchanged.
🤖 Generated with Claude Code