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

[salazaralan73-eng]

Summary

edit_file in @modelcontextprotocol/server-filesystem silently corrupts content containing literal $ characters. The root cause is that applyFileEdits (in src/filesystem/lib.ts) invokes JavaScript's String.prototype.replace(searchString, replacementString) where the second argument is interpreted as a replacement string pattern, not as a literal string.

Pattern	Replaced by
$$	a single $
$&	the matched substring
$`	the substring preceding the match
$'	the substring following the match

Affected file & line

src/filesystem/lib.ts, inside applyFileEdits():

if (modifiedContent.includes(normalizedOld)) {
  modifiedContent = modifiedContent.replace(normalizedOld, normalizedNew); // ← bug
  continue;
}

The fallback line-by-line path (using splice / join) is not affected.

Minimal reproduction

mkdir /tmp/mcp-repro && cd /tmp/mcp-repro
printf 'price: PLACEHOLDER\n' > sample.txt
# from any MCP client, invoke edit_file:
# oldText: "PLACEHOLDER"
# newText: "$$100 USD"
# expected: price: $$100 USD
# actual:   price: $100 USD   ← $$ collapsed to single $

Same corruption with $&, $`, $'.

Proposed fix (one line)

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

The callback form treats the return value as literal, disabling replacement-pattern interpretation while preserving "replace first occurrence only" semantics.

Impact

• Affects ~297k weekly downloads of @modelcontextprotocol/server-filesystem
• Silent corruption: tool reports success and returns a diff, but on-disk content differs from intent
• Particularly affects JS/TS/PHP code ($ variables), currency, regex source, shell scripts

Workaround (until fix lands)

Escape $ in newText by doubling it ($ → $$).

Related

#2033 — different manifestation of $ handling in newText (that issue reports a parse error; this one reports silent content corruption on successful edits).

Willing to open a PR with the one-line fix + parametric tests if maintainers approve direction.

[Sean-Kenneth-Doherty]

I checked this against current origin/main (97ba6b3) and can confirm the exact replacement-pattern corruption with a focused regression test.

Test case added locally:

it('treats dollar signs in replacement text literally', async () => {
  const edits = [
    { oldText: 'line2', newText: '$$100 $& value' }
  ];

  mockFs.rename.mockResolvedValueOnce(undefined);

  await applyFileEdits('/test/file.txt', edits, false);

  expect(mockFs.writeFile).toHaveBeenCalledWith(
    expect.stringMatching(/\/test\/file\.txt\.[a-f0-9]+\.tmp$/),
    'line1\n$$100 $& value\nline3\n',
    'utf-8'
  );
});

On unmodified main, the focused test fails as:

expected: $$100 $& value
received: $100 line2 value

So $$ is collapsed and $& is expanded to the matched oldText, as described.

The callback replacement fix from the issue body:

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

then passes locally:

npm exec -w src/filesystem -- vitest run __tests__/lib.test.ts --testNamePattern "dollar signs"
# 1 passed, 45 skipped

npm exec -w src/filesystem -- vitest run __tests__/lib.test.ts
# 46 passed

npm run build -w src/filesystem
# passed

git diff --check
# passed

I am not opening a PR right this second because I already have two MCP PRs in review and do not want to add review noise, but the one-line fix plus the regression test above is ready if maintainers want it split into a PR.

[minglong51]

@Sean-Kenneth-Doherty thanks for the diagnosis and the regression test — you've done the load-bearing work here. Since you flagged PR capacity is tight at the moment, happy to package the callback-form fix (modifiedContent.replace(normalizedOld, () => normalizedNew)) together with your test into a PR while you wrap up the others. Will credit your investigation in the description and link back to this thread.