Skip to content

fix(filesystem): preserve literal $ in edit_file newText (#4157)#4172

Open
dhruba-datta wants to merge 1 commit into
modelcontextprotocol:mainfrom
dhruba-datta:fix/edit-file-dollar-corruption
Open

fix(filesystem): preserve literal $ in edit_file newText (#4157)#4172
dhruba-datta wants to merge 1 commit into
modelcontextprotocol:mainfrom
dhruba-datta:fix/edit-file-dollar-corruption

Conversation

@dhruba-datta
Copy link
Copy Markdown

Fixes #4157.

Root cause

applyFileEdits (in src/filesystem/lib.ts) invoked
String.prototype.replace(searchString, replacementString) where the
second argument is interpreted as a replacement-string pattern,
not a literal string. This silently corrupted content containing
literal $ characters:

Pattern in newText Replaced by
$$ a single $
$& the matched substring
$` the substring preceding the match
$' the substring following the match
$<name> (regex named-group syntax)

Particularly hit JS/TS code with $variables, currency amounts, regex
source, and shell scripts. The fallback line-by-line path (via splice
/ join) was not affected.

Fix

Pass the replacement via the callback form:

modifiedContent = modifiedContent.replace(normalizedOld, () => normalizedNew);

The function's return value is used verbatim, which bypasses the
pattern interpreter while preserving "replace first occurrence only"
semantics. One-line change to the production code.

Test coverage

Added a regression test in __tests__/lib.test.ts covering all five
pattern cases ($$, $&, $`, $', $<name>). The new test runs
alongside the existing applyFileEdits test block and matches the
file's existing style.

Full vitest suite: 147 passed, 0 failed locally after the change.

Impact

@modelcontextprotocol/server-filesystem has ~297k weekly downloads
per the issue, and the corruption is silent — edit_file returns
success and a clean-looking diff while the on-disk content differs
from intent.

Fixes modelcontextprotocol#4157.

`applyFileEdits` (in src/filesystem/lib.ts) invoked
`String.prototype.replace(searchString, replacementString)` where the
second argument is interpreted as a replacement-string pattern, not a
literal string. This silently corrupted content containing literal
$ characters:

  | Pattern in newText | Replaced by                       |
  | ------------------ | --------------------------------- |
  | $$               | a single $                       |
  | $&                | the matched substring             |
  | $`               | the substring preceding the match |
  | $'                | the substring following the match |
  | $<name>           | (regex named-group syntax)        |

Particularly hit JS/TS code with $variables, currency, regex source,
and shell scripts.

Fix: pass the replacement via the callback form
`replace(needle, () => normalizedNew)`. The function's return value
is used verbatim, bypassing the pattern interpreter, while preserving
"replace first occurrence only" semantics. The fallback line-by-line
path was not affected.

Added regression test in lib.test.ts covering all five pattern cases.
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.

filesystem: edit_file newText interpreted as String.prototype.replace replacement-pattern (literal $ corrupted)

1 participant