fix(cursor,responses): convert structured edits and keep replay across empty deltas (#1017) - #1144
Conversation
…_patch calls (#1017) Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
…ic structured tools Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
Gate the native-mutation refusal hint on whether the synthetic structured edit tools are actually advertised (not every apply_patch request widens). Reject trailing-newline-only and identical old/new replacements as a silent no-op instead of emitting an empty hunk that apply_patch would drop. Give addReplacement a single StructuredEditTranslation return type so the patch field does not overload two different meanings. Document the line-based matching limitations (single-location, edits matched against ORIGINAL content, no final-newline-only edits) in the tool descriptions. Test hardening: remove optional-chaining after expect(), use toEqual for full-shape assertions, add no-op rejection cases and a stateful non-identity wire-name multi_edit translation case. Verification: 21 focused tests pass, typecheck/lint/privacy clean. Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
…name (#1017) Builds on Agent59353's structured-edit work in the commits below. The conversion itself is unchanged and is the hard part: validated JSON, several argument spellings, line-based hunks, no-op and final-newline rejection, and a drop-with-explanation instead of a best-effort patch. The gap was at the other end. `translateStructuredEditCall` decided a call was ours from the tool NAME alone, and both call sites pass a name that came off the wire. `cursorStructuredEditTools` already refuses to shadow a client tool called `edit_file` or `multi_edit` — so the collision was understood at injection — but that knowledge never reached the translation. A user running an MCP server that exposes `edit_file` would have their call silently re-emitted as `apply_patch`, or dropped with an error naming a conversion they never requested. This threads the answer through instead: live-transport records the bare names we actually advertised on this request, derived from `cursorStructuredEditTools` rather than from the name, and the event state carries them. Both call sites convert only when the name is in that set. The stateless fallback now passes through rather than converting. It has no request state, so it cannot know whether we advertised anything, and the safe direction is obvious: an unconverted structured call is a visible, recoverable failure; a wrongly converted one edits a file. Live traffic always carries state, so real conversions are unaffected. Its test previously pinned the old contract ("stateless native-exec path converts edit_file the same way"), so it now pins the new one and says why. Ablation on the new collision test — restoring the name-only gate: (fail) a client tool named edit_file is not hijacked when we advertised nothing (#1036 review) 21 pass, 1 fail Exactly one test goes red, and the twenty-one conversion tests stay green, which is what shows the gate narrows behavior without breaking the feature. tests/cursor-structured-edit.test.ts 22 pass / 0 fail; cursor-tool-budget and cursor-protobuf-events 33 pass / 0 fail; typecheck clean. Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
Adapt PR #1036 for issue #1017 by tagging injected edit tools with internal provenance and deriving conversion eligibility only after the request-builder catalog has been filtered and budgeted. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com> Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
Adapt PR #1126 by preserving reasoning replay candidates across empty text_delta and thinking_delta events in streaming and batch builders. Keep the cache memory-only; omit disk persistence, exit hooks, counters, config plumbing, and openai-chat diagnostics. Co-authored-by: Agent59353 <agent59353@taskmarket.dev> Co-authored-by: NexusCore <22769595+ZachDreamZ@users.noreply.github.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 3cb5b7b8f7
ℹ️ 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 removed = oldLines.map(line => `-${line}`); | ||
| const added = newLines.map(line => `+${line}`); | ||
| return { hunk: ["@@", ...removed, ...added].join("\n") }; |
There was a problem hiding this comment.
When old_string is duplicated or differs only in whitespace, this context-free @@ hunk does not preserve the advertised exact-match semantics: Codex apply_patch accepts ambiguous input by editing the first occurrence and can fall back to whitespace-stripped matching, potentially changing the wrong block. In translateStructuredEditCall, resolve old_string against the target file first and reject zero or multiple exact matches before constructing the patch.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| const editResult = addReplacement(edit as Record<string, unknown>); | ||
| if (editResult.error !== undefined) return editResult; | ||
| hunks.push(editResult.patch); |
There was a problem hiding this comment.
When a valid multi_edit lists unique replacements in reverse file order, appending hunks in request order makes Codex apply_patch advance past the later match and then fail to find the earlier one, so the whole edit is rejected even though every replacement exists in the original file. Resolve each replacement's original offset and sort the emitted hunks by source position while preserving the documented original-file matching semantics.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| const structuredEditNames = tools | ||
| ?.filter(tool => !tool.namespace && isCursorStructuredEditToolName(tool.name)) | ||
| .map(tool => tool.name) ?? []; |
There was a problem hiding this comment.
When the client already exposes a bare tool named edit_file or multi_edit, cursorStructuredEditTools deliberately leaves that tool client-owned, but this name-only filter still tells the model that OpenCodex converts it and that it accepts the synthetic exact-replacement arguments. If the client tool has a different schema, the model can issue invalid calls even though the return-path provenance gate correctly refuses conversion. Include cursorStructuredEdit in this helper's input type and derive the guidance names with isCursorSyntheticStructuredEditTool.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| const lines = text.split("\n"); | ||
| if (lines.length > 0 && lines[lines.length - 1] === "") lines.pop(); | ||
| return lines; |
There was a problem hiding this comment.
When the target uses CRLF line endings, splitting only on \n leaves \r in the generated removal and addition lines, and Codex apply_patch subsequently rewrites the file with LF endings; even a one-block structured edit can therefore produce a whole-file line-ending diff on Windows repositories. Detect the target's line-ending style and preserve it through conversion, or reject this structured path rather than silently normalizing the file.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
3cb5b7b to
80e3b9a
Compare
The independent audit returned FAIL on two points, both recorded rather than smoothed over: - #1144 credited NexusCore in prose while git showed only Agent59353, the identity on #1126's head. A PR that claims credit git does not record fails the contract this campaign exists to uphold. All seven commits now carry a Co-authored-by trailer for NexusCore; tree byte-identical, suites still 32/0. - #1115 is closed - by its author Simon-Opopeee, verified from the timeline, not by any campaign action. Also recorded what the audit confirmed: dev untouched, all withheld-work claims true by diff, the Anthropic narrowing genuinely gated, and no PR body claiming green over a failing code check.
1 participant
Summary
Two fixes: Cursor stops emitting invalid
apply_patchpayloads, and reasoning replay survives empty deltas.Cursor structured edits. Codex exposes
apply_patchas a single-string freeform tool, and the Cursor adapter emitted normalized arguments with no structured-edit conversion — so Cursor consistently produced payloads Codex could not apply. Cursor-compatibleedit_file/multi_edittools are now injected only when Codex actually exposedapply_patch, exact-match replacements are converted into a valid freeform patch envelope, the converted call is emitted asapply_patchpreserving the original call id, and malformed or ambiguous replacements raise an explicit bridge error instead of forwarding invalid patch text.Reasoning replay. Empty
text_delta/thinking_deltaevents dropped replay candidates in both the streaming and batch builders, so a restart could not reconstruct the turn.Closes #1017.
Attribution
Both are @ZachDreamZ's (NexusCore) work, from #1036 and #1126 — five of the seven commits are cherry-picked with their authorship intact. Reported by @Vincent-HD (#1017).
Correction on #1036. It computed tool visibility from
cursorVisibleToolsbut derived structured-edit availability and names from the earlierrequest.tools. Once filtering or budgeting drops a tool the two disagree, and a synthetic edit identified by wire name alone can collide with a real client tool. The set is now derived from the final filtered catalog, injected tools carry internal provenance rather than being recognized by name, and a post-filter regression test pins it.What I withheld from #1126, and why. Its empty-delta fix was bundled with optional on-disk persistence of chain-of-thought, exit hooks, and global counters. Writing model reasoning to disk is a privacy-surface change —
reasoning-replay-cache.tsdocuments a memory-only contract deliberately — and that decision deserves its own PR rather than riding along with a defect repair.src/config.ts,src/lib/config-dir.tsandsrc/adapters/openai-chat.tshave zero diff here, verified, and a subprocess test proves that setting the persistence environment variables produces no file or directory. If you want persistent replay, it is a reasonable feature — it just needs the privacy question answered first.#1036 and #1126 are left open for the author. Planning unit:
devlog/_plan/260806_stacked_bug_campaign/(phases 110, 120).Stack 10 of the 260806 attribution campaign, stacked on #1142.
Verification
bun test tests/cursor-structured-edit.test.ts tests/bridge-reasoning-replay-batch.test.ts tests/reasoning-replay-robustness.test.ts— 32 pass, 0 fail (re-run after rebase onto stack 9)bun run typecheck— exit 0bun run privacy:scan— passedgit diff --exit-code -- src/config.ts src/lib/config-dir.ts src/adapters/openai-chat.ts— exit 0, no outputChecklist