Guard whitespace-only old strings in str_replace against empty-match file corruption - #1291
Open
nordicnode wants to merge 1 commit into
Open
Guard whitespace-only old strings in str_replace against empty-match file corruption#1291nordicnode wants to merge 1 commit into
nordicnode wants to merge 1 commit into
Conversation
…file corruption
A whitespace-only oldString absent from the file collapsed to an empty
search string in the whitespace-insensitive fallback, where indexOf('')
vacuously succeeded at position 0. The empty match made replaceAll
insert the new string between every character of the file while the
tool reported success. Strip-to-empty old strings now fall through to
the not-found error, leaving the file untouched.
Regression tests cover the space/tab/newline/mixed-unicode collapse
classes, whitespace-only files, exact-match and allowMultiple
whitespace replacements, error accumulation across replacement pairs,
and preservation of the non-empty whitespace-insensitive fallback.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem & Context
str_replacesilently corrupted entire files when given a whitespace-onlyoldStringthat does not appear in the file. In the last-resort whitespace-insensitive match insidetryMatchOldStr, the search string is stripped of all whitespace:oldStr.replace(/\s+/g, ''). For a whitespace-onlyoldString(a single space, tabs, newlines, U+00A0, …) the stripped result is'', andindexOf('')vacuously succeeds at position 0. The position-mapping loops then produce an empty match, which the caller applies withcontent.replaceAll('', newStr)— insertingnewStringbetween every character of the file — and the tool returned the success shape, so the corrupted file was written to disk with no error surfaced to the model.Reproduction (pre-fix, against the real module):
Changes Made
packages/agent-runtime/src/process-str-replace.ts: guard the whitespace-insensitive fallback so anoldStringthat strips to''is reported as not-found (falls through to the existing not-found error return) instead of producing a vacuous empty match. +8/−1 lines, no behavior change on any other path.packages/agent-runtime/src/__tests__/process-str-replace.test.ts: six regression/behavior-preservation tests (details below), importing the real productionprocessStrReplace.Architecture & Conventions Conformance
common/src/types/contracts/, no module monkey patching)terminalCommandBroker(no directspawnor TUI-process bypass) — no process execution touchedgetCliEnv()for CLI,getSdkEnv()for SDK, no forbiddengetProcessEnv()imports) — no env access addedIS_FREEBUFFpreserved, no paid features introduced) — product-agnostic bug fiximport typeused for types) — no import changesScope Verification
packages/agent-runtime/web/,freebuff/web/,packages/internal/,packages/billing/,packages/bigquery/, orpackages/build-tools/Testing & Verification
bun run build:sdk(passed cleanly)bun run build:freebuff(passed cleanly)bun cli/scripts/smoke-binary.ts cli/bin/freebuff(passed cleanly — with the CI env var set and a writableHOME; the sandboxed checkout has a read-only home directory)Tests were proven to catch the bug: with the fix stashed, the new tests fail 6/6 against the unfixed source (
"XaX\tXbX\nXcX\tXdX\nX"corruption reproduced); with the fix applied, all 30 tests in the file pass. Fix and tests were additionally verified by a differential property fuzz (20,640 cases over whitespace-only old strings, Unicode whitespace classes, CRLF files,allowMultiple, multi-pair sequences, and patch round-trips viaapplyPatch): 572 violations pre-fix, 0 post-fix.Verification Output / Log Snippet
New tests (all against the production module, no local reimplementations):
does not corrupt the file for a space / tabs / newlines / mixed whitespace— the four collapse-to-empty classes, each must return the not-found error shapereports the not-found error even when the file is entirely whitespacestill replaces whitespace-only old strings that exist in the file(exact-match branch preserved)still replaces all whitespace occurrences with allowMultiple: truelets later replacements proceed after a failed whitespace-only one(error-message accumulation intact)keeps whitespace-insensitive matching working for non-empty anchors(guard does not over-correct the fallback)