Skip to content

fix(filesystem): preserve literal \$ in edit_file newText (closes #4157)#4224

Open
adityachilka1 wants to merge 1 commit into
modelcontextprotocol:mainfrom
adityachilka1:fix/filesystem-replace-pattern
Open

fix(filesystem): preserve literal \$ in edit_file newText (closes #4157)#4224
adityachilka1 wants to merge 1 commit into
modelcontextprotocol:mainfrom
adityachilka1:fix/filesystem-replace-pattern

Conversation

@adityachilka1
Copy link
Copy Markdown

Fixes #4157.

This is the exact fix the issue reporter proposed (and is consistent with the
bug root-cause in the issue body).

The bug

src/filesystem/lib.ts passes normalizedNew as String.prototype.replace's
replacement argument. When that argument is a string, JS interprets these
tokens in it:

Token Interpreted as
$$ a literal $
$& the matched substring
`$`` the portion before match
$' the portion after match
$n nth capture (regex only)

So when a caller does edit_file with newText: "$100 USD", the $1 token
gets eaten (replaced with the content of capture group 1, which is undefined
for a plain-string search, so it's dropped).

The fix

Use the callback form (() => normalizedNew). MDN explicitly documents
this as the way to disable replacement-pattern interpretation:

If replacement is a function, the matched substring is replaced with the
return value of the function.

One-line diff:

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

Tests

Two regression tests added to src/filesystem/__tests__/lib.test.ts:

  1. $$ preservation (literal $)
  2. $&, $``, $'` preservation (replacement-pattern tokens)

Both tests assert that the exact input string is written verbatim. Without
the fix, both tests fail.

Verification

cd src/filesystem && npm test

-> 148 / 148 tests pass across 7 test files (the 2 new tests are in there).

No behavioral regression

The callback always returns a constant ({n normalizedNew {n unbound}), so for
inputs that don't contain $, the behavior is identical to the string-arg form.
The only change is that $ characters now survive the replace.

Closes #4157.

…lcontextprotocol#4157)

The prior code passed `normalizedNew` as String.prototype.replace's replacement
argument. JS interprets `$$`, `$&`, `$'`, `$``, and `$n` as replacement
patterns in that form, so any actual `$` characters in the user's
newText - price tags, regex backreferences in docs, shell examples -
get expanded or dropped.

The callback form (`replace(..., () => string)`) disables pattern
interpretation, so the string is written verbatim.

Two regression tests pin the callback form.

Closes modelcontextprotocol#4157.
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)

2 participants