Skip to content

Guard whitespace-only old strings in str_replace against empty-match file corruption - #1291

Open
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:fix/str-replace-whitespace-empty
Open

Guard whitespace-only old strings in str_replace against empty-match file corruption#1291
nordicnode wants to merge 1 commit into
CodebuffAI:mainfrom
nordicnode:fix/str-replace-whitespace-empty

Conversation

@nordicnode

Copy link
Copy Markdown

Problem & Context

str_replace silently corrupted entire files when given a whitespace-only oldString that does not appear in the file. In the last-resort whitespace-insensitive match inside tryMatchOldStr, the search string is stripped of all whitespace: oldStr.replace(/\s+/g, ''). For a whitespace-only oldString (a single space, tabs, newlines, U+00A0, …) the stripped result is '', and indexOf('') vacuously succeeds at position 0. The position-mapping loops then produce an empty match, which the caller applies with content.replaceAll('', newStr) — inserting newString between 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):

initialContent: "a\tb\nc\td\n"
replacement:    { oldString: " ", newString: "X", allowMultiple: false }
result:         success, content = "XaX\tXbX\nXcX\tXdX\nX"
expected:       error, file untouched

Changes Made

  • packages/agent-runtime/src/process-str-replace.ts: guard the whitespace-insensitive fallback so an oldString that 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 production processStrReplace.

Architecture & Conventions Conformance

  • Adheres to Dependency Injection (contracts defined in common/src/types/contracts/, no module monkey patching)
  • Terminal commands use terminalCommandBroker (no direct spawn or TUI-process bypass) — no process execution touched
  • Environment hygiene respected (getCliEnv() for CLI, getSdkEnv() for SDK, no forbidden getProcessEnv() imports) — no env access added
  • Freebuff mode compatibility (IS_FREEBUFF preserved, no paid features introduced) — product-agnostic bug fix
  • Imports ordered and explicit (import type used for types) — no import changes

Scope Verification

  • All modified files are within allowed public directories: packages/agent-runtime/
  • NO modifications to web/, freebuff/web/, packages/internal/, packages/billing/, packages/bigquery/, or packages/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 writable HOME; the sandboxed checkout has a read-only home directory)
  • Unit / integration tests added or updated in affected package
  • Anti-flake principles observed (no sleeps, no ports, no filesystem, no timers — pure string-processing tests)

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 via applyPatch): 572 violations pre-fix, 0 post-fix.

Verification Output / Log Snippet

$ bun test packages/agent-runtime/src/__tests__/process-str-replace.test.ts
 30 pass
 0 fail
 93 expect() calls
Ran 30 tests across 1 file. [31.00ms]

$ bun run --cwd packages/agent-runtime test   # remaining failures pre-exist on main (MCP schema = #1259, find-files WIP)
 601 pass / 5 fail (identical to baseline without this change)

$ bun run --cwd packages/agent-runtime typecheck
 0 new errors (2 agents-graveyard = #1202; 1 from uncommitted find-files WIP, not this branch's files)

$ bun cli/scripts/smoke-binary.ts cli/bin/freebuff
 smoke-binary: tree-sitter init OK.
 smoke-binary: OK (matched /\x1b\[\?1049h/, exit code null, 9176 bytes captured, attempt 1/3).

New tests (all against the production module, no local reimplementations):

  1. 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 shape
  2. reports the not-found error even when the file is entirely whitespace
  3. still replaces whitespace-only old strings that exist in the file (exact-match branch preserved)
  4. still replaces all whitespace occurrences with allowMultiple: true
  5. lets later replacements proceed after a failed whitespace-only one (error-message accumulation intact)
  6. keeps whitespace-insensitive matching working for non-empty anchors (guard does not over-correct the fallback)

…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant