fix(artifacts): make edits more robust (lenient id linking + typography-tolerant matching)#2379
Conversation
When the model renames or drifts an artifact identifier on edit (e.g. green-button -> blue-button), the update orphaned into a dead 'This edit couldn't be linked to an artifact' card. collectArtifacts now falls back to the most-recently-created artifact when an update id doesn't match (and an artifact exists), attaching the edit as a proper new version. Also hardens the artifacts system prompt: keep the identifier byte-identical across versions, copy old_str verbatim, and don't call tools mid-artifact.
![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: c067d5875d
ℹ️ 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".
| // most-recently-created one (the Map preserves insertion order). Only leave a | ||
| // disabled card when there's genuinely nothing to link to. | ||
| if (!artifact && artifacts.size > 0) { | ||
| artifact = [...artifacts.values()].at(-1); |
There was a problem hiding this comment.
When there is more than one artifact, a case/whitespace-only drift for a non-latest artifact is now attached to the most recently created artifact instead. For example, after creating app and then chart, an update with identifier="App" will go through this fallback and be applied to chart, usually producing a failed/no-op version there while leaving app unchanged. Since the new behavior is explicitly meant to handle case/whitespace drift, it should first look for a normalized identifier match before using the last-artifact fallback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed. The fallback now first looks for a normalized (trim+lowercase) identifier match, so a drifted id like App links to app even when a later artifact (chart) exists; it only falls back to the most-recent artifact when there's no unambiguous normalized match. Added a multi-artifact drift test (commit 349e59f).
…hing findMatch now normalizes curly quotes and en/em dashes to ASCII (1:1, length-preserving, so offsets still index raw content) before matching, so a model that emits or omits smart quotes/dashes doesn't fail the edit. Unicode spaces are already covered by the existing whitespace-tolerant regex. Inspired by pi-mono's edit tool; the apply-against-original/reverse-order and hard-error ideas were deliberately not ported (we re-search per pair so there's no stale-offset bug, and have no tool-call retry loop).
gary149
changed the title
…st-artifact fallback Addresses review: with multiple artifacts, a case/whitespace-drifted id (e.g. 'App' for 'app') was attached to the most-recently-created artifact instead of the one it meant. Now try a normalized (trim+lowercase) id match first; fall back to the latest only when there's no unambiguous normalized match.
1 participant
Editing an existing artifact often failed. Repro (create a "green button", then "edit to a blue button, don't change all code") produced a dead card "This edit couldn't be linked to an artifact" plus "Edited, 1 change didn't apply". This PR addresses both the linking and the matching side.
1. Lenient update linking (the dead card)
collectArtifactslinked an update to an existing artifact only by exactartifacts.get(op.identifier), with no fallback. Models routinely rename the identifier on edit (green-buttontoblue-button) or drift its case/whitespace, orphaning the update into aversion: -1dead card.Now, when an update op's identifier doesn't match any artifact and at least one artifact exists, it links to the most-recently-created artifact (the
Mappreserves insertion order) and attaches the edit as a proper new version (using the resolved identifier). The disabled card only remains when there is genuinely nothing to link to.2. Typography-tolerant matching (the "didn't apply")
findMatchonly handled whitespace runs and doubled brackets, so a<old_str>that uses or omits curly "smart" quotes or en/em dashes failed to match. It now normalizes those to ASCII (1:1, length-preserving, so offsets still index the raw content) on both sides before matching. Unicode spaces (NBSP etc.) were already covered by the existing whitespace-tolerant regex (JS\smatches them).Inspired by
pi-mono's edit tool. Two of its ideas were deliberately not ported: "match all pairs against the original + reverse-order apply" (we re-search each pair in the mutated content, so we have no stale-offset bug, and it would break our intentional sequential-pairs behavior) and hard errors on non-unique/overlap (we have no tool-call retry loop, so we stay tolerant and surface failures via the existing "N didn't apply" + "ask to fix" affordances).Out of scope (follow-up)
The duplicate-card / "Called 1 tool" case comes from a tools-vs-artifact interleave in
runMcpFlow(a tool call mid-reply splits/duplicates an in-progress artifact); that's an architectural rework for a separate PR. Lenient linking already turns its orphan into a proper version.Tests
artifacts.spec.tsadds: renamed-id / case-drifted / multi-artifact linking; smart-quote (both directions), em-dash, and NBSP matching. The existing sequential-pairs, whitespace-tolerant, doubled-bracket, and zero-artifact "couldn't be linked" tests stay green.npm run check,npm run lint, and the artifact suite (46 tests) pass.