fix(source-control): make worktree_root handoff shell-safe for unset and special-char values

[kyle-sexton]

This was generated by AI during work-loop execution.

Summary

• ${user_config.worktree_root} substitution into skill content is raw text substitution, not
  shell-escaped (confirmed this session against the official
  plugins-reference § User configuration
  docs), so neither quote style around the inline --root '${user_config.worktree_root}' literal in
  context/create.md / SKILL.md was fully safe: unset broke double-quoting (original finding), and a
  configured root containing ', $, or a backtick broke the interim single-quoted fix (feat(source-control): route /worktree create through a shared worktree-creation helper (#399 Phase A) #898).
• worktree-create.sh gains an additive --root-file <path> flag that reads the root from a file's
  first line instead of a --root process argument. Both render sites now write the substituted value
  to a temp file via a quoted heredoc (<<'WT_ROOT_EOF' — fully literal, no expansion or
  quote-processing) and pass --root-file, never inlining the value in a --root shell literal.
• The existing unset/unexpanded-token refuse (exit 3, guidance on stderr, no EnterWorktree(name:)
  fallback) is reused unchanged and reached through the file path: an unset key still leaves the
  literal ${user_config.worktree_root} token, the heredoc writes it verbatim, and the guard still
  fires.
• --root is untouched and stays available for a caller that already holds the value as a real
  process argument (a hook, or direct CLI use) — no shell-literal risk on that path.

Test plan

• Extended plugins/source-control/scripts/worktree-create.test.sh with 8 new cases covering the
  --root-file flag: mutual exclusivity with --root (exit 2), a missing file (exit 2), a root
  containing ', $, and a backtick materializing at the exact computed path (exit 0), empty file
  content refusing (exit 3, reuses the unset guard), and the literal unexpanded
  ${user_config.worktree_root} token via the file refusing (exit 3).
  • Ran the full suite locally: 68/68 pass, exit 0.
• Added a create-action eval (plugins/source-control/skills/worktree/evals/evals.json, id 9)
  asserting the skill renders the safe out-of-band handoff (no single-quoted
  ${user_config.worktree_root} shell literal) and preserves the exit-3 stop-and-surface contract.
• shellcheck clean on both the helper and its test file (after moving one # shellcheck disable=SC2016 to the line it actually needed to cover).
• markdownlint-cli2 clean on the two render-site markdown files after fixing an
  MD031 (blanks-around-fences) violation introduced by an early draft.
• Validated plugin.json and the extended evals.json against their respective JSON Schemas
  (check-jsonschema, both ok).
• Ran the repo's local quality gates against origin/main for this diff: docs-only detector (not
  docs-only, correctly runs full suite), check-changed-skills.sh (worktree skill: PASS, 0 errors),
  check-changelog-parity.sh --check and --check-bump (both pass — version bump 0.26.2 → 0.26.3
  with a matching ## [0.26.3] entry), check-skill-portability.sh (no unexcused coupling tokens).
• Debugged and fixed one test-authoring pitfall along the way: the special-char fixture root
  initially inherited $TEST_TMPDIR's bare POSIX /tmp/... form, which defeated MSYS's
  POSIX→Windows argv auto-conversion for git.exe when the path also carried $/backtick bytes,
  silently landing the worktree at the wrong physical location even though the printed path and exit
  code looked correct. Fixed by anchoring that one fixture to a cygpath -m-normalized
  (TEST_TMPDIR_NATIVE) drive-letter path instead — matching how a real Windows directory-type
  userConfig value actually resolves.

Related

Closes #965. Origin: #898 (interim single-quote fix) and the Codex review that surfaced the
special-char gap; consolidated triage/ratification trail is on the issue itself.

[claude]

Claude finished @kyle-sexton's task in 2m 8s —— View job

---

PR Review: fix(source-control): make worktree_root handoff shell-safe

• Gather context and understand the PR
• Read changed files
• Review shell script changes (worktree-create.sh)
• Review skill/context markdown changes
• Review test coverage (worktree-create.test.sh)
• Review evals and plugin metadata
• Post final review

---

Summary

This is a clean, well-scoped fix for a genuine shell-injection class of bug. The core approach — moving the raw-substituted value out-of-band via a quoted heredoc to a temp file — is the correct solution. The implementation is sound, the documentation is accurate, and the test suite is thorough. I have a few observations, one of which is worth considering before merge.

---

Correctness

worktree-create.sh L110–130

The mutual exclusivity guard, the file-existence check, and the IFS= read -r root < "$root_file" idiom are all correct:

• IFS= prevents field splitting on embedded whitespace.
• -r prevents backslash interpretation.
• Redirected < "$root_file" reads only the first line; the trailing newline is stripped. Every other byte (including ', $, backtick, NUL-adjacent characters) passes through unchanged.
• An empty file produces root="", which hits the existing unset guard (exit 3) correctly, matching the documented contract.

Unset-when-neither-flag-is-provided behavior — Providing neither --root nor --root-file still exits 3 (root unconfigured) rather than 2 (usage error). This is a pre-existing behavior for the --root path; the PR preserves it correctly and does not regress it.

context/create.md heredoc pattern — The rendered snippet is correct:

cat > "$root_file" <<'WT_ROOT_EOF'
${user_config.worktree_root}
WT_ROOT_EOF

When Claude runs this, the quoted heredoc delimiter means the ${user_config.worktree_root} body — already substituted by the plugin runtime before Claude sees it — is written verbatim with no further shell expansion. If the key is unset, the literal token is written, the read picks it up, and the existing guard fires. ✅

---

One thing worth discussing

Temp file not cleaned up on interruption — context/create.md L77 has rm -f "$root_file" as an unconditional step after the helper call. This works for normal exit and helper failure, but a SIGINT or unexpected model interruption between mktemp and rm -f would orphan a temp file containing the user's worktree_root path. The path value itself is not a secret, but a trap 'rm -f "$root_file"' EXIT INT TERM before the heredoc write would make this strictly cleaner:

root_file="$(mktemp)"
trap 'rm -f "$root_file"' EXIT INT TERM
cat > "$root_file" <<'WT_ROOT_EOF'
${user_config.worktree_root}
WT_ROOT_EOF
bash "${CLAUDE_PLUGIN_ROOT}/scripts/worktree-create.sh" \
  --name "<validated-name>" --root-file "$root_file"

Since worktree_root is just a directory path, the security impact is low. And given the guidance is rendered into Claude's working context (not a persistent shell script), the cleanup gap is narrow in practice. Acceptable to defer, worth calling out. Fix this →

---

Test coverage

The 8 new test cases are well-chosen:

Case	Flag combo	Expected exit
Both --root + --root-file	mutual exclusivity	2
--root-file missing file	file existence	2
Special-char root (', $, backtick)	happy path	0
Empty file content	unset guard reuse	3
Literal ${user_config.worktree_root} token	unset guard reuse	3

The TEST_TMPDIR_NATIVE / cygpath -m logic for the special-char fixture is thoughtful — the MSYS POSIX→Windows auto-conversion heuristic is documented to fail when the path carries metacharacter bytes, and anchoring to a drive-letter form is the right fix. The inline comment explains it clearly.

One untested edge: a file with no trailing newline (e.g. printf '%s' '/some/path' > "$root_file"). IFS= read -r root < "$root_file" exits with status 1 (EOF without newline) but still populates root correctly; the script does not check read's exit status, so this works. Not a bug, but worth a quick note in the comment or a test case for future readers.

---

Documentation and evals

SKILL.md — The updated headline rule precisely captures the constraint ("never inline it in a quoted --root argument — a value containing ', $, or a backtick would break the shell literal") and cross-references the render. ✅

Eval ID 9 — The expectations are concrete and testable: no inlined --root literal, heredoc present, exit-3 contract preserved. ✅

CHANGELOG.md — Entry is clear, cites both the original and interim issues (#898, #965), explains why both quote styles failed, and describes the contract-preserving behavior for the unset case. ✅

---

Overall

Approve with the trap observation noted. The fix is correct, the guard reuse for the unset case is elegant, the Windows fixture handling is careful, and the test suite covers all the meaningful branches. The temp file cleanup gap is minor and can be addressed in a follow-up if desired.

[claude]

Claude finished @kyle-sexton's task in 2m 59s —— View job

---

Security Review — PR #1334

Tasks

• Load tools
• Read PR diff
• Review changed files
• Post findings

---

Overview

This PR replaces an unsafe inline shell-literal pattern (embedding a raw-substituted ${user_config.worktree_root} in a --root '...' argument) with a properly isolated out-of-band handoff via a quoted heredoc and --root-file. The security direction of the change is correct and the core mechanics are sound. I found no critical or important vulnerabilities.

---

Findings

SUGGESTION · Low severity · CONFIRMED

Heredoc delimiter WT_ROOT_EOF can collide with a user-configured root value

context/create.md line 72–74

cat > "$root_file" <<'WT_ROOT_EOF'
${user_config.worktree_root}
WT_ROOT_EOF

After plugin substitution, if worktree_root is exactly the string WT_ROOT_EOF (a relative path the user could plausibly configure), the rendered heredoc body begins immediately with the delimiter string, causing bash to treat the heredoc as empty. root_file ends up holding no bytes; IFS= read -r root < "$root_file" sets root to the empty string; and the existing unset guard fires correctly — exit 3, no worktree created, no fallback. The security contract holds.

The consequence is a confusing failure rather than a security bypass, but it is a real code edge case. Using a higher-entropy delimiter such as WT_ROOT_EOF_7f3a9e2b makes an accidental collision essentially impossible at no other cost.

---

SUGGESTION · Low severity · CONFIRMED

Temp file persists when Claude halts on a non-zero helper exit before reaching rm -f

context/create.md line 71–78

root_file="$(mktemp)"
cat > "$root_file" <<'WT_ROOT_EOF'
${user_config.worktree_root}
WT_ROOT_EOF
bash "${CLAUDE_PLUGIN_ROOT}/scripts/worktree-create.sh" \
  --name "<validated-name>" --root-file "$root_file"
rm -f "$root_file"           # ← not reached if Claude stops on non-zero exit above

The skill correctly instructs Claude to STOP on any non-zero helper exit, but the cleanup line is after the invocation. When Claude stops early the temp file survives. mktemp creates it with mode 0600 (owner-only), and its sole content is a filesystem path — not a secret — so this is a resource leak rather than a credential leak. Adding trap 'rm -f "${root_file:-}"' EXIT before the cat line would guarantee cleanup on any exit path.

---

Positive security notes

• IFS= read -r root < "$root_file" correctly handles all special-character bytes — single quotes, $, backticks, backslashes — passing them through unchanged.
• [[ ! -f "$root_file" ]] (double brackets) safely handles special characters in the path itself.
• The mutual-exclusivity guard (--root and --root-file together → exit 2) and the existing unset/unexpanded-token guard (exit 3) are both reused unchanged; neither can be bypassed via the --root-file path.
• The containment check, normalize_path, and git -C "$toplevel" worktree add all apply identically regardless of which flag populated root, so the in-repo placement refusal is not weakened.
• The printf '%s\n' "$root" pattern in the test file (worktree-create.test.sh line 332) avoids format-string injection correctly.

---

No critical or important security issues found. The two suggestions above are hygiene-level items; the core fix is mechanically correct and an unambiguous security improvement over the prior single-quoted inline pattern.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9e6189243a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e77dcb242

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[github-actions]

Warning

Automated review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30179205597
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":4791,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

[github-actions]

Warning

Automated security review did not complete — this is an infrastructure failure, not a review verdict.

Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."

• Run: https://github.com/melodic-software/claude-code-plugins/actions/runs/30179205573
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":1965,"total_cost_usd":0}

Re-running the job, or pushing a new commit, will retry the review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa2c566892

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9890a31260

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude encountered an error after 5s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.