Skip to content

[Bug]: edit with replace_all corrupts literal dollar sequences in replacement text #155

Closed

Description

@Ashwinhegde19

Product or interface

Source build or repository tooling — native edit tool used by the CLI.

Version

Source version 0.4.12, commit 17b8ded9c16da54224c2a05e54185cbee4f15021.
Node.js 24.21.0; pnpm 9.12.0.

Platform

Linux

OS version and architecture

Ubuntu 26.04.1 LTS, x86_64.

Issue area

File / workspace / path / encoding

Steps to reproduce

Bulk edits interpret JavaScript replacement-string sequences inside new_string instead of inserting the requested text literally.

The following self-contained reproduction runs from the source checkout with Node.js 24.21.0. It imports the repository's actual buildReplaceAllEdit implementation, using Node's TypeScript transformation to avoid an additional loader. It only creates and removes a synthetic temporary file; no account or model is needed.

node --input-type=module <<'NODE'
import { readFile, writeFile, mkdtemp, rm } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import { stripTypeScriptTypes } from 'node:module';

const source = await readFile(
  'packages/agent-tools/src/shared/replace-all-edit.ts', 'utf8',
);
const js = stripTypeScriptTypes(source, { mode: 'transform' });
const { buildReplaceAllEdit } = await import(
  'data:text/javascript;base64,' + Buffer.from(js).toString('base64')
);
const dir = await mkdtemp(join(tmpdir(), 'mcode-replace-all-'));
try {
  const file = join(dir, 'example.txt');
  await writeFile(file, 'before TOKEN after\nTOKEN\n');
  const result = await buildReplaceAllEdit(file, 'TOKEN', '$$');
  console.log(JSON.stringify({
    expected: 'before $$ after\n$$\n',
    actual: result.edits[0].newText,
    replacedCount: result.replacedCount,
  }, null, 2));
} finally {
  await rm(dir, { recursive: true, force: true });
}
NODE

I also reproduced the final on-disk corruption through the actual LocalEditTool.execute implementation in a temporary Vitest test using the repository's Vitest aliases:

await writeFile(file, 'before TOKEN after\nTOKEN\n');
const result = await new LocalEditTool(directory).execute({} as never, {
  file_path: file,
  old_string: 'TOKEN',
  new_string: '$$',
  replace_all: true,
});
expect(await readFile(file, 'utf8')).toBe('before $$ after\n$$\n');

That expected-behavior assertion fails, and the tool reports success. A single-occurrence control with replace_all: false correctly preserves $$.

Provider type / permission mode: not applicable to these direct tool reproductions. No model, permission service, real user files, or credentials are involved.

Expected and actual behavior

Expected: new_string is inserted literally. The resulting content should be before $$ after\n$$\n.

Actual: The resulting content is before $ after\n$\n, with a successful edit result.

Additional cases confirmed through LocalEditTool:

  • $& produces a false “No changes made” error because the matched text is reinserted.
  • $' inserts the text following each match.
  • Dollar-backtick inserts the text preceding each match.

The latter two can duplicate unrelated surrounding file contents while reporting success. This affects legitimate replacement text in shell scripts, Makefiles, and JavaScript replacement expressions.

Redacted error summary

Helper output:
{
  "expected": "before $$ after\n$$\n",
  "actual": "before $ after\n$\n",
  "replacedCount": 2
}

Actual edit-tool result:
Successfully replaced 1 block(s) in <temporary-directory>/example.txt.

Root cause and proposed fix

rawStringReplaceAll passes a replacement string directly to String.prototype.replaceAll, enabling substitution sequences. Its split().join() fallback already has literal semantics. The first-party LocalEditTool wrapper passes this transformed content to the actual file-edit pipeline.

A focused fix would use haystack.replaceAll(needle, () => replacement) and add regression tests for all four sequences, including final file-content assertions through the actual tool. This proposal has not yet been committed or submitted as a PR.

Validation: four expected-behavior assertions failed for the four bulk-replacement sequences; the single-edit dollar-sequence control passed. The self-contained helper reproduction above was also executed successfully and produced the shown mismatch. These are source-level, offline tests, not live-model or published-package acceptance. No full pnpm verify, native Windows/macOS validation, or published npm-package validation was performed for this report.

Contribution proposal

I would like to contribute the focused fix and regression tests. I read CONTRIBUTING.md and understand that code PRs are currently accepted only from repository collaborators. I am opening this issue first as requested. Would the maintainers accept a PR from me for this fix, and is collaborator access or another contribution arrangement required? I will follow the test registration, public-source inventory, and pnpm verify requirements for an accepted PR.

Screenshots / Desktop log upload ID

Not applicable; deterministic source-level reproduction provided above.

Before submitting

  • I have searched existing issues, including replace_all and edit-related reports, and found no matching report.
  • I have included my version and removed sensitive information. All files and inputs above are synthetic.

Activity

  1. dvd233 commented on Sep 18, 2026

    @dvd233

    The deterministic replace_all reproduction looks actionable, and I verified the current CONTRIBUTING.md says code/documentation contributions are currently limited to repository collaborators. Before preparing a branch, could a maintainer confirm whether an external contributor dvd233 may submit a focused fix for #155, or whether collaborator access is required? I will wait for that policy decision before creating a patch or PR.

  2. hetaoBackend commented on Sep 18, 2026

    @hetaoBackend
    Collaborator

    Thanks for the precise reproduction and proposed fix. The fix is implemented in #165: the shared helper now uses a replacement callback so new_string is inserted literally.

    Validation on commit 9a35236fdc2ca1944c8e3b6eddbd7a50fee8ac22:

    • All 12 regression cases pass, covering $$, $&, dollar-backtick, and dollar-apostrophe through both the helper and actual file edits, including multiple matches and single-edit controls.
    • The full local pnpm verify passed all 14 gates on macOS arm64 / Node 26.4.0.
    • I also launched the built CLI against a local synthetic provider and executed four real runtime edit calls, each replacing two matches. All tool results succeeded and the exact on-disk contents preserved the requested dollar sequences. This is offline acceptance, not a live-model or published-package test.
    • Linux, macOS and Windows CI, the aggregate verification check, and source-history/artifact checks all passed.

    The merge attempt is currently blocked by the repository's required approving review. The PR has not merged and this fix has not been released; keeping this issue open until the merge completes.

  3. hetaoBackend commented on Sep 18, 2026

    @hetaoBackend
    Collaborator

    Update: #165 has now merged into main as f3160cd, and this issue is closed as completed. The review blocker mentioned above no longer applies.

    The fix and regression tests are included in the merged source. Validation results are recorded above; this confirms the source merge, not a published-package release. Thanks again for the detailed reproduction and proposed fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions