🎯 feat: Diagnose Every Failing Workspace Edit and Negotiate Tolerant Matching - #271
danny-avila wants to merge 5 commits into
Conversation
…Matching A rejected edit batch now reports every failing edit by position: missing edits name the nearest candidate line and flag elided, line-numbered, whitespace-only or CRLF mismatches, and ambiguous edits give their match count and line numbers. Two negotiated edit features add tolerant matching (line-trimmed, indentation-flexible, whitespace-normalized) and replaceAll, with per-edit match reporting only for requests that opt in.
|
@codex review |
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: 25adab506f
ℹ️ 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".
![lia-by-librechat[bot]](https://avatars.githubusercontent.com/in/4931833?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed and fixed all six open Codex findings at 8d1a44ecfb24fbb99ae7014a91d9b1d535389902:
- Whole whitespace-delimited token matching prevents partial identifier edits; the new streaming KMP matcher handles large inputs without compiling request-sized regexes.
- Ambiguous exact matches keep a count and at most five offsets, rather than allocating one object per hit.
- Preview-only workers can negotiate
tolerant_matchandreplace_allwithout advertising the edit-only hash feature. - The worker rejects unnegotiated
matchingandreplaceAllbefore executing or arming an edit, even when those fields specifyexactorfalse. - KMP searches line-window candidates in linear time; repetitive indentation verification is bounded and fails closed with a useful conflict.
I also fixed an additional resource-limit issue: replaceAll now rejects an oversized intermediate before allocating a giant replacement. Previews and edits remain non-mutating on rejection. Existing legacy result shapes are unchanged.
Verification at this exact head: code-package tsc --noEmit passed; 99 focused engine/protocol/worker tests and 10 workspace integration tests passed; three restricted-heap regressions passed; 91 focused service tests and touched-file ESLint passed. Service tsc --noEmit still reports seven errors in pre-existing unrelated files. CI has nine passing checks; Lambda MicroVM Runner Image (arm64) is still running. Full suites, the code-package lint/import-sort (no configured executable or script), and a fresh Codex review were not run here. The GitHub App cannot trigger Codex; a maintainer can request it if desired.
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d1a44ecfb
ℹ️ 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".
There was a problem hiding this comment.
Confirmed and fixed the Codex CRLF/LF boundary finding at exact head 4b677dfcf5266e86ced295cbd1576e18ba9c89fb. The regression failed before the fix and now passes. The whitespace-normalized tier normalizes caller newline forms before peeling, retains each matched line's ending in mixed files, and keeps exact-mode replacements unchanged.
The wider edit-subsystem review also found and fixed silent boundary errors: a token-only match no longer accepts oldText boundary whitespace or line breaks absent from the source, and rejects caller attempts to remove that out-of-range whitespace. Multiple claimed line breaks must actually exist. Previews and edits fail with EDIT_CONFLICT without modifying the file. BOM, batch operations, replaceAll, mixed line endings, wire results, and many-match performance have regressions.
Local checks: code-package TypeScript passed; 119 focused matcher/protocol/worker tests and 22 workspace tests passed; 83 service tests passed before the final boundary-only edit; service typecheck retains its seven pre-existing errors. Exact-head CI may still be running. Full suites, code-package lint/import-sort (not configured), and a new Codex review were not run; a maintainer must trigger Codex.
|
@codex review the latest head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b677dfcf5
ℹ️ 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 newline = text.indexOf('\n', start); | ||
| const lineEnd = newline < 0 ? text.length : newline; | ||
| const end = lineEnd > start && text[lineEnd - 1] === '\r' ? lineEnd - 1 : lineEnd; | ||
| lines.push({ start, end, next: newline < 0 ? undefined : newline + 1, text: text.slice(start, end) }); |
There was a problem hiding this comment.
Although the KMP change fixes repeated window comparisons, a permitted 1 MiB file containing mostly one-character lines still makes this loop allocate roughly one million Line objects per pass. A missing exact edit invokes both relaxed line strategies from describeMissing and then splits the file again in nearestLine; batches of up to 100 failures repeat that work, so a valid request can cause hundreds of megabytes of allocation or stall/OOM the worker instead of returning EDIT_CONFLICT. Reuse a compact line index across strategies or cap diagnostic materialization.
Useful? React with 👍 / 👎.
| } else if (leadingNewlines === 1 && newText.startsWith('\n')) { | ||
| // Keep the source's indentation when the caller used different spaces. | ||
| newText = newText.slice(1); |
There was a problem hiding this comment.
When oldText and newText each start with one newline but use different indentation after it, this fallback removes only the newline from newText. Because the source's entire boundary whitespace remains outside the matched token range, the replacement then contains both indentations; for example, source "header\n foo bar\n", oldText: "\n foo bar", and newText: "\n\tbaz qux" produces "header\n \tbaz qux\n". This can change block structure in indentation-sensitive files, so peel the complete leading whitespace run corresponding to that shared boundary rather than only \n.
Useful? React with 👍 / 👎.
| hints.push( | ||
| nearest.exact | ||
| ? `its first line appears at ${formatLineList(text, nearest.starts, nearest.count)}, but the lines after it differ` | ||
| : `the closest line is ${formatLineList(text, nearest.starts)}: ${snippet(nearest.text)}`, |
There was a problem hiding this comment.
When a worker advertises edit_file without read_file or preview_edit, which the capability validator permits and service/src/bridge/store.ts:139-144 routes based only on the requested operation, a caller can submit guaranteed-missing oldText containing two probe tokens. Because nearestLine accepts 50% token overlap, this branch then returns up to 120 characters of the actual source line in the conflict message, turning edit permission into a read primitive. Omit source text from edit diagnostics unless read access was independently authorized.
Useful? React with 👍 / 👎.
| for ( | ||
| let start = text.indexOf(edit.oldText); | ||
| start >= 0; | ||
| start = text.indexOf(edit.oldText, start + step) | ||
| ) { |
There was a problem hiding this comment.
The bounded-range fix still restarts indexOf at every code-unit offset to obtain the full ambiguity count. For a permitted 1 MiB file of repeated a characters and a 512 KiB oldText of a characters, there are roughly 512,000 overlapping matches and every search compares another long suffix, making this path quadratic; even substantially smaller inputs already take seconds before an EDIT_CONFLICT can be returned. Use a linear matcher such as KMP to count overlapping exact matches rather than repeatedly rescanning the same text.
Useful? React with 👍 / 👎.
2 participants
Summary
Agents using a paired worker's
edit_filefail often, and each failure costs them extra turns. Over 10 days on one deployment, 125 of 2,935edit_filecalls (4.3%) failed withEDIT_CONFLICT. Every one got the same message:Workspace edit must match exactly once. That message does not say whether the text was missing or repeated, which edit in the batch failed, or where the intended text is. 94 of the 125 failures were multi-edit batches (2–14 edits), where one bad edit rejects the whole call. The agent's usual recovery was a blind retry (35) or a full re-read (30).This PR changes the worker's edit engine in two ways.
Every failing edit is diagnosed, in all modes, with no negotiation. Edits still apply in order and still commit atomically. When one fails, the worker keeps checking the rest and rejects the batch with a single
EDIT_CONFLICT. Its message lists every failing edit by position:...) inoldText, copied line-number prefixes, a whitespace-only difference (with the line where the text does exist), or CRLF line endings.The message stays under 3,000 characters to fit the 4,096-character settlement bound, and it travels through the existing error path unchanged.
Two negotiated edit features use the existing
editFileFeatureshandshake. The worker advertises them, the Code API lists them insupportedWorkspaceEditFileFeatures, and it only dispatches requests that use them to workers that negotiated them:tolerant_match: a request-levelmatching: 'tolerant'. When an exact match fails, it falls back in order to:line-trimmed: ignores trailing whitespace and CRLF;indentation-flexible: matches a uniformly shifted block, and movesnewTextto the file's own indentation;whitespace-normalized: any whitespace run between tokens, and strips the leading and trailing whitespace the caller wrapped around both texts.A match must still be unique, and replacements keep the file's line endings.
replace_all: a batch edit'sreplaceAll: truereplaces every non-overlapping match. It still fails when nothing matches.A request that sets
matchingor anyreplaceAllgetsmatches: one{ strategy, occurrences }per edit. A request that sets neither gets exactly the legacy result, so older callers and validators never see a new key.The tolerant tiers follow LibreChat's existing skill-file matcher, but fix three problems found while porting it:
newText. Before, it insertednewTextas written.oldTextending in a newline matches through the line terminator, instead of requiring an extra blank line.Related to #267. LibreChat will opt into
tolerant_matchandreplace_allin a separate PR, and render these diagnostics.How it works
Negotiation and gating:
WORKSPACE_EDIT_FILE_FEATURESinprotocol.tsis the single list the worker advertises, the Code API negotiates and capability validation accepts, as any unique subset.supportsWorkspaceToolinbridge/store.tsrefusesmatchingorreplaceAllfor a worker that did not negotiate the matching feature, the same way it gatesexpected_base_sha256.matchesexactly when the request opted in, withexactstrategies only for non-tolerant requests andoccurrences > 1only forreplaceAlledits.Testing
packages/code: newedits.test.ts(17 cases, covering diagnostics, tolerant tiers, CRLF, re-indentation, replaceAll, overlap, the output bound and large-file performance); worker integration tests inworkspace.test.tsthroughLocalWorkspaceTools; protocol tests for request, result and capability validation.npm test: 564 pass. The 9 failures (PTC watchdog and credential storage tests) fail identically on unmodifiedmainin the same WSL environment.service:bun test ./src/bridge: 196 pass, 0 fail, including new gates for tolerant edits, tolerant previews andreplaceAll.tsc --noEmitreports the same 7 existing errors asmainand no new ones.servicebun run buildfails the same way onmainlocally (Node 18 loading the rollup config), so CI's build is the check there.