Repository navigation
fix: insert paths and task input verbatim at placeholder substitutions - #6352
Conversation
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 <file>
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.
probepark
left a comment
There was a problem hiding this comment.
Review (head a0cab54, gajae-reviewer on behalf of probepark)
CI: base-caused failures only (1 shard). test:@gajae-code/coding-agent:shard-1-of-8 failed 11 tests in sdk-broker.test.ts (session.delete / transcript authority) and task-autorouting-preflight.test.ts (managed artifact owner). All 11 are a subset of the 12 that fail on the dev base bb00f4c itself: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37263181896/job/111626046408 (#6349 managed-owner area; this diff touches none of it). evidence producer and the Affected path validation aggregate fail only on "required affected shards did not succeed". check:@gajae-code/coding-agent passed, and all 4 targeted suites this PR touches passed (substitute-plugin-root, expand-command, coordinator-mcp-policy, launch-worktree).
Scope: +71 / -6, 10 files. 5 source lines in packages/coding-agent (cli/auth-broker-cli, coordinator-mcp/policy, discovery/substitute-plugin-root, gjc-runtime/launch-worktree, task/commands), plus 4 test files and 1 changelog fragment.
Conventions: changelog fragment packages/coding-agent/changelog.d/replacement-pattern-paths.md present; no generated files; no released CHANGELOG sections touched; no labels.
Notable:
src/task/commands.ts:L79,src/discovery/substitute-plugin-root.ts:L50,src/gjc-runtime/launch-worktree.ts:L64,src/coordinator-mcp/policy.ts:L34-35,src/cli/auth-broker-cli.ts:L470: each string replacement becomes() => value. A function replacer's return value is inserted literally, and the regex/search pattern on each line is unchanged (/\$@/g,/^~/,/^~(?=\/|$)/, literal{repo}and${…_PLUGIN_ROOT}), so the matches are the same as before. No other behavior changes.- Tests assert against literal expected strings, not a
replaceAlloracle:expand-command.test.ts:L171-173,launch-worktree.test.ts:L116-121,substitute-plugin-root.test.ts:L140-144,coordinator-mcp-policy.test.ts:L94-98. The$&,$$,$`,$'and$1inputs would each fail under the old code.auth-broker-cli.tshas no unit test, which the PR body acknowledges; it is the same one-token change.
Blocking: none
Body verdict line count=0, not updated. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:6c414e716a2f86ef53694b609287d759ba03bf552a49f7c8adc68032a7096d7c reviewer:human reviewer-id:probepark evidence:function-replacers-5-sites;regression-tests-literal-expected;shard1-fails-match-dev-base-bb00f4c;check-green
Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:6c414e716a2f86ef53694b609287d759ba03bf552a49f7c8adc68032a7096d7c reviewer:human reviewer-id:probepark evidence:function-replacers-5-sites;regression-tests-literal-expected;shard1-fails-match-dev-base-bb00f4c;check-green
|
Merged into dev as
— |
What
Replace string replacements with function replacers at five placeholder substitutions, so the substituted path or input is inserted verbatim.
Why
String.prototype.replaceandreplaceAlltreat a string replacement as a template.$&,$$,$`and$'inside it are expanded, not copied. Each site below passes a runtime value — an install path, a directory name, a home directory, or user text — as that template.Measured on
dev(bb00f4c):substitutePluginRoot/plugins/a$&b/plugins/a${CLAUDE_PLUGIN_ROOT}b/bin/runsubstitutePluginRoot…/x$y/p`…/xy/p/servers/run.shresolveWorktreeBucketForPath/src/q$'z/src/q/.worktreesz/.worktreesresolveWorktreeBucketForPath/src/x$$y/src/x$y/.worktreesexpandCommandsed s/a/$&/gsed s/a/$@/gexpandCommandregex /^$`/regex /^Review this change:\n/auth-broker import ~/…/home/a$&b/home/a~b/…$is a legal filename character on every platform these paths come from. The input cases are ordinary text: a shell snippet, a regex, or a price.Call sites
discovery/substitute-plugin-root.tscommand,args,env,cwdgjc-runtime/launch-worktree.ts{repo})coordinator-mcp/policy.ts{repo},~)cli/auth-broker-cli.tsgjc auth-broker import ~/…task/commands.ts$@in workflow command instructions; exported on the public SDK surfaceThe plugin case corrupts the path the MCP server is spawned from. The worktree cases send launch worktrees and the coordinator's allowlist to the wrong directory. In the coordinator case, a legitimate managed worktree is then rejected as outside the allowed roots.
Fix
Each replacement becomes
() => value. A function replacer's return value is inserted literally. No other behavior changes.Testing
Regression tests were added to the existing suites for the plugin root, the launch worktree bucket and the coordinator policy, plus a new
test/task/expand-command.test.ts. Every new test fails on unmodifieddev(7 failures) and passes with the change.The tests compare against a
split/joinoracle, not againstreplaceAll. My first harness built its expected values with the samereplaceAllit was testing, so the expected values were corrupted the same way and every case passed. I'm noting it because a test written that way will hide this whole defect class.Across 10,028 inputs, including 5,000 randomized strings built from
$,&,`,', digits and/, the fixed functions match the oracle exactly.The
auth-broker-clisite has no unit-test entry point. It was reproduced by overridingos.homedir, and it gets the same one-token change.Other
replace/replaceAllcalls with a variable replacement were checked and left alone. Their replacement is a compile-time constant, or a fixed hash or version value with no$.Approval
Merges to
devrequire one approving GitHub review from a write-access maintainer on the current head.devbun checkpasses (package-scopedcheckgreen)packages/coding-agent/changelog.d/