🎯 feat: Diagnose Every Failing Workspace Edit and Negotiate Tolerant Matching - #271
danny-avila wants to merge 8 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".
There was a problem hiding this comment.
Reviewed and addressed the four fresh Codex findings at 5ac2f45227413ae505890bbe8dd7e5082f65fa0a. They applied to the current head, not stale code.
- Exact matches now use UTF-16 KMP to count overlapping locations in linear time;
replaceAllstill consumes nonoverlapping ranges. A long repetitive input previously exceeded the bounded regression runtime and now completes quickly. - Line windows and nearest-line hints share a compact newline index per source revision, invalidated after every successful edit. This replaces repeated eager allocation of hundreds of thousands of Line objects; a dense valid file with 80 failing edits now completes under a 96 MiB Node heap while keeping batch diagnostics.
- Whitespace-normalized matching peels the whole equivalent leading or trailing newline-and-indentation wrapper rather than only the newline, so source indentation cannot be doubled. Previews and committed edits are covered.
- Worker settlement suppresses edit error text when neither
read_filenorpreview_editis advertised for that specific workspace. It retains the typed error code; read-authorized workspaces still get detailed diagnostics. An edit-only error is not a source-read grant.preview_editalready returns file contents, so it is treated as read-capable.
These regressions failed before the changes. Code-package tsc --noEmit, 129 focused matcher/protocol/worker tests, 23 preview/edit workspace tests, and two 96 MiB stress tests passed. Focused service routing tests: 91 passed. Service tsc --noEmit still has its seven baseline errors. Exact-head CI is pending. Not run: full suites, code-package lint/import-sort (not configured) and a fresh Codex review (maintainer trigger required). The wider review checked operation-scoped authorization, preview-to-edit fencing, atomic mutation failures, Unicode offsets, newline boundaries and memory/time bounds.
|
@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: 5ac2f45227
ℹ️ 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.
Rechecked PR #271 at a79d5db81576dec0c2ebe5c6b5d1544da0120d77. The new Codex P2 was fresh: a valid 1 MiB x -> x replaceAll exhausted a 64 MiB Node heap because it retained about one million range objects. That test failed before this commit and passes now. Match counting retains only the first range; replaceAll streams output in chunks (flushing at a 16 KiB threshold), skips identical pieces, and preserves the early 1 MiB output bound.
I checked all three matching tiers, overlapping versus non-overlapping matches, Unicode, ordered edits, error codes, BOM/preview/atomic commit, result matches.occurrences and negotiated feature boundaries. Four thousand deterministic exact-replacement cases matched a reference implementation. A 100-edit, 1 MiB no-op batch completed under a 64 MiB heap without changing the source.
Local checks passed: code-package npx tsc --noEmit, 55 matcher tests, 24 focused preview/edit integration tests, 21 edit/protocol tests, 27 workspace-worker tests and 83 focused service routing tests. The service typecheck retains seven previously observed errors. Exact-head CI is running. Full suites, code-package lint/import-sort (not configured) and a fresh Codex review (requires a maintainer trigger) were not run locally.
|
@codex review, final review. no further reviews will be requested, last chance |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a79d5db815
ℹ️ 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.
Reproduced and fixed the fresh Codex P2 at 5f927f80ca662bdf214e7068c42ccf988aa11861. When oldText and newText used different boundary indentation, the whitespace-normalized tier rejected an intentionally added leading blank line. A regression failed before the fix. The matcher now peels only the newline-and-indentation wrapper corresponding to oldText, keeping additional requested blank lines. The same rule applies to the trailing wrapper. It still rejects attempts to remove source whitespace outside the matched tokens.
Reviewed the full affected path: exact versus tolerant strategy order, source boundary presence, CRLF/LF conversion, per-match replaceAll, batch and BOM preview-to-edit fencing, result validation, edit authorization, and bounded-memory matching. Focused checks passed: code-package tsc --noEmit, 61 matcher tests, 25 workspace preview/edit tests, 77 protocol/worker tests, five cases under a 64 MiB heap, and 91 service routing tests. The service typecheck retains its seven pre-existing diagnostics. PR #272 is already merged into main; the branch is mergeable against that main and the new head's CI is running.
Not run locally: full suites, code-package lint/import-sort (no configured script), and a new Codex review (requires a maintainer trigger). This note does not claim CI completion or approve a merge.
|
@codex review the latest head, final review. no further reviews will be requested, last chance |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f927f80ca
ℹ️ 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 (matching === 'exact') { | ||
| let tolerant: MatchOutcome | undefined; | ||
| for (const find of RELAXED_STRATEGIES) { | ||
| tolerant = find(text, { oldText, newText: '' }, lines); |
There was a problem hiding this comment.
When an exact edit fails and oldText has leading or trailing whitespace, this diagnostic probe substitutes newText: ''; findWhitespaceNormalized then rejects the candidate because the empty replacement cannot peel the requested boundary wrapper. For example, oldText: "foo bar\n" against "foo bar\n" is only reported as a generic nearest-line mismatch rather than the intended whitespace-only match. Probe with a replacement that preserves oldText's boundary whitespace, or separate match detection from replacement validation.
Useful? React with 👍 / 👎.
| if (side === 'leading' && text.startsWith(boundary)) return text.slice(boundary.length); | ||
| if (side === 'trailing' && text.endsWith(boundary)) return text.slice(0, -boundary.length); |
There was a problem hiding this comment.
When the old boundary is exactly a newline but newText also has indentation after that newline, this shortcut removes only the newline and bypasses the code below that consumes the complete wrapper. Fresh evidence beyond the earlier indentation fix is source: "header\n foo bar", oldText: "\nfoo bar", and newText: "\n\tbaz qux", which silently produces "header\n \tbaz qux"; the symmetric trailing shortcut similarly combines replacement and source whitespace. Only take the shortcut when the complete leading/trailing whitespace run equals boundary, otherwise let the wrapper logic peel the run.
Useful? React with 👍 / 👎.
| let windowIndent = ''; | ||
| let valid = end !== undefined; | ||
| if (valid && strategy === 'indentation-flexible') { | ||
| if (verifications + needle.length > MAX_LINE_WINDOW_VERIFICATIONS) return { status: 'limit' }; |
There was a problem hiding this comment.
The verification budget rejects a single candidate whenever needle.length exceeds 100,000, even though validating that one candidate is linear and the request/file remain within their 1 MiB bounds. For example, a uniquely anchored 100,001-line block whose source is uniformly indented and whose oldText is not is rejected as having “too many repetitive” candidates, while the otherwise identical 100,000-line edit succeeds. Apply the cap to repeated candidate work while permitting at least one complete candidate verification.
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.