From a0cab54e48c348752f0e2e05038d046952d0fdea Mon Sep 17 00:00:00 2001 From: 10kH Date: Mon, 5 Oct 2026 05:47:06 +0000 Subject: [PATCH] fix: insert paths and task input verbatim at placeholder substitutions Five call sites pass a runtime value as the replacement argument of String.prototype.replace/replaceAll. A string replacement is a template, so JavaScript expands `$&`, `$$`, `` $` `` and `$'` inside the value instead of copying it: plugin root /plugins/a$&b -> a${CLAUDE_PLUGIN_ROOT}b/bin/run repo name /src/q$'z -> /src/q/.worktreesz/.worktrees repo name /src/x$$y -> /src/x$y/.worktrees task input sed s/a/$&/g -> sed s/a/$@/g task input regex /^$`/ -> regex /^Review this change:\n/ The fix replaces each string replacement with a function replacer, which inserts its return value literally. discovery/substitute-plugin-root.ts plugin MCP command/args/env/cwd gjc-runtime/launch-worktree.ts launch worktree bucket {repo} coordinator-mcp/policy.ts managed worktree {repo} and ~ cli/auth-broker-cli.ts ~ expansion for import task/commands.ts $@ in workflow command instructions Verified against a split/join oracle on 10,028 inputs, including 5,000 randomized strings built from `$`, `&`, `` ` ``, `'` and digits. Each new test fails on the previous code. --- .../changelog.d/replacement-pattern-paths.md | 3 +++ .../coding-agent/src/cli/auth-broker-cli.ts | 2 +- .../src/coordinator-mcp/policy.ts | 5 ++-- .../src/discovery/substitute-plugin-root.ts | 4 +++- .../src/gjc-runtime/launch-worktree.ts | 3 ++- packages/coding-agent/src/task/commands.ts | 4 +++- .../test/coordinator-mcp-policy.test.ts | 10 ++++++++ .../test/gjc-runtime/launch-worktree.test.ts | 13 +++++++++++ .../substitute-plugin-root.test.ts | 10 ++++++++ .../test/task/expand-command.test.ts | 23 +++++++++++++++++++ 10 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 packages/coding-agent/changelog.d/replacement-pattern-paths.md create mode 100644 packages/coding-agent/test/task/expand-command.test.ts diff --git a/packages/coding-agent/changelog.d/replacement-pattern-paths.md b/packages/coding-agent/changelog.d/replacement-pattern-paths.md new file mode 100644 index 00000000000..417f9d48752 --- /dev/null +++ b/packages/coding-agent/changelog.d/replacement-pattern-paths.md @@ -0,0 +1,3 @@ +### Fixed + +- Paths and task input are now inserted verbatim where GJC substitutes a placeholder. Five call sites passed a variable as the replacement argument of `String.prototype.replace`/`replaceAll`, so JavaScript expanded `$&`, `$$`, `` $` `` and `$'` inside the value instead of copying it: a plugin installed under `a$&b` launched its MCP server from `a${CLAUDE_PLUGIN_ROOT}b/...`, a repository named `q$'z` had its launch worktrees resolved to `q/.worktreesz/.worktrees`, and a workflow command run with `sed s/a/$&/g` received `sed s/a/$@/g`. Affected: plugin MCP `command`/`args`/`env`/`cwd` root substitution, the launch-worktree bucket and coordinator managed-worktree `{repo}` templates, `~` expansion for `gjc auth-broker import`, and `$@` in workflow command instructions. diff --git a/packages/coding-agent/src/cli/auth-broker-cli.ts b/packages/coding-agent/src/cli/auth-broker-cli.ts index c8f9661d2f8..529907edba2 100644 --- a/packages/coding-agent/src/cli/auth-broker-cli.ts +++ b/packages/coding-agent/src/cli/auth-broker-cli.ts @@ -467,7 +467,7 @@ async function runImport(flags: AuthBrokerCommandArgs["flags"]): Promise { if (!target) { throw new Error("Usage: gjc auth-broker import [--provider=] [--include-disabled] [--dry-run]"); } - const resolvedTarget = path.resolve(target.startsWith("~") ? target.replace(/^~/, os.homedir()) : target); + const resolvedTarget = path.resolve(target.startsWith("~") ? target.replace(/^~/, () => os.homedir()) : target); const { entries, skipped } = await loadImportPlan(resolvedTarget, flags.provider, flags.includeDisabled === true); if (flags.json) { diff --git a/packages/coding-agent/src/coordinator-mcp/policy.ts b/packages/coding-agent/src/coordinator-mcp/policy.ts index cf68a415493..5d6e59dcb60 100644 --- a/packages/coding-agent/src/coordinator-mcp/policy.ts +++ b/packages/coding-agent/src/coordinator-mcp/policy.ts @@ -122,8 +122,9 @@ function parseRootList(value: string | undefined): string[] { } function resolveManagedWorktreeRoot(root: string, configured: string | undefined): string { - const template = (configured?.trim() || "{repo}/.worktrees").replace(/^~(?=\/|$)/, os.homedir()); - const resolved = template.replaceAll("{repo}", path.basename(root)); + // Function replacers so a `$` in the home or repository path is kept verbatim. + const template = (configured?.trim() || "{repo}/.worktrees").replace(/^~(?=\/|$)/, () => os.homedir()); + const resolved = template.replaceAll("{repo}", () => path.basename(root)); return path.resolve(path.isAbsolute(resolved) ? resolved : path.join(path.dirname(root), resolved)); } diff --git a/packages/coding-agent/src/discovery/substitute-plugin-root.ts b/packages/coding-agent/src/discovery/substitute-plugin-root.ts index c005de8fa0c..16c44c03e14 100644 --- a/packages/coding-agent/src/discovery/substitute-plugin-root.ts +++ b/packages/coding-agent/src/discovery/substitute-plugin-root.ts @@ -8,7 +8,9 @@ const GJC_VAR = "$" + "{GJC_PLUGIN_ROOT}"; export function substitutePluginRoot(value: T, rootPath: string): T { if (typeof value === "string") { - return value.replaceAll(CLAUDE_VAR, rootPath).replaceAll(GJC_VAR, rootPath) as T; + // Function replacers: a string replacement would expand `$&`, `$$`, `` $` `` + // and `$'` inside the install path instead of inserting it verbatim. + return value.replaceAll(CLAUDE_VAR, () => rootPath).replaceAll(GJC_VAR, () => rootPath) as T; } if (Array.isArray(value)) { return value.map(v => substitutePluginRoot(v, rootPath)) as T; diff --git a/packages/coding-agent/src/gjc-runtime/launch-worktree.ts b/packages/coding-agent/src/gjc-runtime/launch-worktree.ts index d652fdd129c..ad957c7af9d 100644 --- a/packages/coding-agent/src/gjc-runtime/launch-worktree.ts +++ b/packages/coding-agent/src/gjc-runtime/launch-worktree.ts @@ -205,7 +205,8 @@ export function resolveWorktreeBucketForPath( const template = expandHomePrefix(configured || DEFAULT_WORKTREE_BUCKET, home, pathApi); return pathApi.resolve( pathApi.dirname(repoRoot), - template.replaceAll(REPO_NAME_PLACEHOLDER, pathApi.basename(repoRoot)), + // Function replacer so a `$` in the repository directory name is kept verbatim. + template.replaceAll(REPO_NAME_PLACEHOLDER, () => pathApi.basename(repoRoot)), ); } diff --git a/packages/coding-agent/src/task/commands.ts b/packages/coding-agent/src/task/commands.ts index 4415e1a55fa..6442b00187d 100644 --- a/packages/coding-agent/src/task/commands.ts +++ b/packages/coding-agent/src/task/commands.ts @@ -120,7 +120,9 @@ export function getCommand(commands: WorkflowCommand[], name: string): WorkflowC * Replaces $@ with the provided input. */ export function expandCommand(command: WorkflowCommand, input: string): string { - return command.instructions.replace(/\$@/g, input); + // Function replacer: `input` is user text and must be inserted verbatim, not + // parsed for `$&`, `$$`, `` $` `` or `$'` replacement patterns. + return command.instructions.replace(/\$@/g, () => input); } /** diff --git a/packages/coding-agent/test/coordinator-mcp-policy.test.ts b/packages/coding-agent/test/coordinator-mcp-policy.test.ts index 82e182c49d5..dad00d812f8 100644 --- a/packages/coding-agent/test/coordinator-mcp-policy.test.ts +++ b/packages/coding-agent/test/coordinator-mcp-policy.test.ts @@ -152,6 +152,16 @@ describe("Hermes MCP safety policy", () => { await expect(assertCoordinatorWorkdir(config, worktree)).resolves.toBe(worktree); }); + it("derives the managed worktree root from a repository name containing replacement-pattern characters", () => { + // `{repo}` used a string replacement, so `$&` in the directory name expanded to + // the placeholder itself and `$'` spliced the template remainder into the path. + for (const name of ["a$&b", "x$$y", "q$'z"]) { + const root = path.join(os.tmpdir(), name); + const config = buildCoordinatorMcpConfig({ GJC_COORDINATOR_MCP_WORKDIR_ROOTS: root }); + expect(config.managedWorktreeRoots).toEqual([path.join(root, ".worktrees")]); + } + }); + it("rejects artifact symlink escapes and enforces byte caps", async () => { const root = await tempRoot(); const outside = await tempRoot(); diff --git a/packages/coding-agent/test/gjc-runtime/launch-worktree.test.ts b/packages/coding-agent/test/gjc-runtime/launch-worktree.test.ts index 51a10fae9a0..2d350f370a6 100644 --- a/packages/coding-agent/test/gjc-runtime/launch-worktree.test.ts +++ b/packages/coding-agent/test/gjc-runtime/launch-worktree.test.ts @@ -773,6 +773,19 @@ describe("GJC_WORKTREE_DIR path red-team", () => { }); }); +describe("resolveWorktreeBucketForPath repository names with replacement-pattern characters", () => { + // `{repo}` was substituted with a string replacement, so `$&`, `$$`, `` $` `` and + // `$'` in the directory name were expanded: `a$&b` resolved to `a{repo}b`, and + // `q$'z` spliced the template remainder into the path. + for (const name of ["a$&b", "x$$y", "x$`y", "q$'z", "$1"]) { + it(`keeps ${JSON.stringify(name)} verbatim`, () => { + const repo = `/src/${name}`; + expect(resolveWorktreeBucketForPath(repo, undefined, "/home/u", path.posix)).toBe(`/src/${name}/.worktrees`); + expect(resolveWorktreeBucketForPath(repo, "~/wt/{repo}", "/home/u", path.posix)).toBe(`/home/u/wt/${name}`); + }); + } +}); + describe("resolveWorktreeBucketForPath Windows semantics", () => { const home = "C:\\Users\\kim"; const repo = "C:\\repos\\app"; diff --git a/packages/coding-agent/test/marketplace/substitute-plugin-root.test.ts b/packages/coding-agent/test/marketplace/substitute-plugin-root.test.ts index 381300c2429..95389bcac68 100644 --- a/packages/coding-agent/test/marketplace/substitute-plugin-root.test.ts +++ b/packages/coding-agent/test/marketplace/substitute-plugin-root.test.ts @@ -56,4 +56,14 @@ describe("substitutePluginRoot", () => { it("returns string unchanged when no variables present", () => { expect(substitutePluginRoot("no-vars-here", ROOT)).toBe("no-vars-here"); }); + it("inserts an install path containing replacement-pattern characters verbatim", () => { + // A string replacement would expand `$&`, `$$`, `` $` `` and `$'` inside the + // path: `a$&b` became `a${CLAUDE_PLUGIN_ROOT}b`, `x$`y` lost `$`y`, and `q$'z` + // spliced the remainder of the template into the middle of the path. + for (const root of ["/plugins/a$&b", "/plugins/x$$y", "/plugins/x$`y", "/plugins/q$'z", "/plugins/$1"]) { + expect(substitutePluginRoot(`${CLAUDE_VAR}/bin/server`, root)).toBe(`${root}/bin/server`); + expect(substitutePluginRoot(`${GJC_VAR}/bin/server`, root)).toBe(`${root}/bin/server`); + expect(substitutePluginRoot([`--config=${CLAUDE_VAR}/c.json`], root)).toEqual([`--config=${root}/c.json`]); + } + }); }); diff --git a/packages/coding-agent/test/task/expand-command.test.ts b/packages/coding-agent/test/task/expand-command.test.ts new file mode 100644 index 00000000000..9ea945cb2f9 --- /dev/null +++ b/packages/coding-agent/test/task/expand-command.test.ts @@ -0,0 +1,23 @@ +import { describe, expect, it } from "bun:test"; +import { expandCommand, type WorkflowCommand } from "../../src/task/commands"; + +function command(instructions: string): WorkflowCommand { + return { name: "review", description: "", instructions, source: "user", filePath: "/tmp/review.md" }; +} + +describe("expandCommand", () => { + it("substitutes every $@ with the task input", () => { + expect(expandCommand(command("Review:\n$@\nThen re-check $@."), "the login fix")).toBe( + "Review:\nthe login fix\nThen re-check the login fix.", + ); + }); + + it("inserts input containing replacement-pattern characters verbatim", () => { + // The input was passed as a string replacement, so `$&` became `$@`, `$$` + // collapsed to `$`, and `` $` `` / `$'` spliced the instructions before or + // after the placeholder into the user's text. + for (const input of ["sed s/a/$&/g", "costs $$5", "regex /^$`/", "the $' suffix", "awk '{print $1}'"]) { + expect(expandCommand(command("Task: $@ (done)"), input)).toBe(`Task: ${input} (done)`); + } + }); +});