fix(skills): replace shell antipatterns blocked by permission check (#711)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: tmchow

plugins/compound-engineering/AGENTS.md

@@ -225,7 +225,34 @@ Why: shell-heavy exploration causes avoidable permission prompts in sub-agent wo
 - [ ] Never instruct agents to use `find`, `ls`, `cat`, `head`, `tail`, `grep`, `rg`, `wc`, or `tree` through a shell for routine file discovery, content search, or file reading
 - [ ] Describe tools by capability class with platform hints — e.g., "Use the native file-search/glob tool (e.g., Glob in Claude Code)" — not by Claude Code-specific tool names alone
 - [ ] When shell is the only option (e.g., `ast-grep`, `bundle show`, git commands), instruct one simple command at a time — no action chaining (`cmd1 && cmd2`, `cmd1 ; cmd2`) and no error suppression (`2>/dev/null`, `|| true`). Two narrow exceptions: boolean conditions within if/while guards (`[ -n "$X" ] || [ -n "$Y" ]`) are fine — that is normal conditional logic, not action chaining. **Value-producing preparatory commands** (`VAR=$(cmd1) && cmd2 "$VAR"`) are also fine when `cmd2` strictly consumes `cmd1`'s output and splitting would require manually threading the value through model context across bash calls (e.g., `BODY_FILE=$(mktemp -u) && cat > "$BODY_FILE" <<EOF ... EOF`). Simple pipes (e.g., `| jq .field`) and output redirection (e.g., `> file`) are acceptable when they don't obscure failures
-- [ ] **Pre-resolution exception:** `!` backtick pre-resolution commands run at skill load time, not at agent runtime. They may use chaining (`&&`, `||`), error suppression (`2>/dev/null`), and fallback sentinels (e.g., `|| echo '__NO_CONFIG__'`) to produce a clean, parseable value for the model. This is the preferred pattern for environment probes (CLI availability, config file reads) that would otherwise require runtime shell calls with chaining. Example: `` !`command -v codex >/dev/null 2>&1 && echo "AVAILABLE" || echo "NOT_FOUND"` ``
+- [ ] **Pre-resolution exception:** `!` backtick pre-resolution commands run at skill load time, not at agent runtime. They may use chaining (`&&`, `||`), error suppression (`2>/dev/null`), and fallback sentinels (e.g., `|| echo '__NO_CONFIG__'`) to produce a clean, parseable value for the model. This is the preferred pattern for environment probes (CLI availability, config file reads) that would otherwise require runtime shell calls with chaining. Three shapes are rejected by Claude Code's safety check and must be avoided in `!` backticks:
+  - **`case ... esac`** is rejected as `Contains case_statement`. Use `&&` chaining or pipe-to-sed, or extract to a script.
+  - **`[A] && B || C`** (mixing `&&` and `||` at the same lexical depth) is rejected as `ambiguous syntax with command separators` (issue #710). Wrap the `&&` chain in a subshell so only `||` remains at top level — `(A && B) || C` — or emit the raw value and let the agent's prose decide. Example of the safe shape: `` !`(top=$(...); [ -n "$top" ] && cat "$top/file") || echo '__SENTINEL__'` ``
+  - **`$(...)` containing a double-quoted string** (e.g., `basename "$(dirname "$common")"`) is rejected as `Unhandled node type: string` (issue #709). Replace nested `$()` with parameter expansion (`${common%/.git}`), pipe to sed (`| sed -E 's|/\.git/?$||; s|.*/||'`), or extract to a script invoked as `` !`bash "${CLAUDE_SKILL_DIR}/scripts/<name>.sh"` ``.
+
+  When the logic is non-trivial, prefer extracting to a script under the skill's `scripts/` directory; the safety check then sees only `bash <quoted-path>`, which sidesteps both current and future safety-check tightenings. Tests in `tests/skill-shell-safety.test.ts` enforce all three patterns.
+
+  **Permission gate on extracted scripts — invoke from the skill body, not from `!` pre-resolution.** A pre-resolution `bash "${CLAUDE_SKILL_DIR}/scripts/<name>.sh"` form passes the safety check but trips Claude Code's permission check at skill-load time, which does *not* honor `defaultMode: bypassPermissions`. Allow-listing via `allowed-tools` frontmatter is unreliable at *load time*: empirically, broad `Bash(bash *)` patterns appear to load with bypass on but narrow filename-pinned patterns like `Bash(bash *upstream-version.sh)` fail with bypass off. Move the script invocation into the skill body so it runs via the runtime Bash tool instead. Two pieces are required for it to actually work:
+
+  1. **Use `${CLAUDE_SKILL_DIR}` for the script path**, not bare relative paths. The runtime Bash tool runs from the user's project CWD, not the skill directory — `bash scripts/<name>.sh` fails with "No such file or directory" empirically. The `${CLAUDE_SKILL_DIR}` env var resolves correctly across `claude --plugin-dir` and standard marketplace-cached installs.
+  2. **Declare narrow `allowed-tools` patterns** pinned to each script filename. At runtime, `allowed-tools` granting is documented to apply, so users without `bypassPermissions` skip the approval prompt. Pin per filename rather than using broad `Bash(bash *)`.
+
+  ```yaml
+  allowed-tools: Bash(bash *upstream-version.sh), Bash(bash *currently-loaded-version.sh)
+  ```
+
+  ````markdown
+  ## Step 1: Probe X
+
+  Run via the Bash tool, in parallel:
+
+  ```bash
+  bash "${CLAUDE_SKILL_DIR}/scripts/upstream-version.sh"
+  bash "${CLAUDE_SKILL_DIR}/scripts/currently-loaded-version.sh"
+  ```
+  ````
+
+  Use this whenever a `!` pre-resolution would invoke `bash <path>`. Reserve pre-resolution for commands whose first token already matches common user allow rules (`git status`, `gh api`, `cat <path>`, `command -v <name>`).
 - [ ] Do not encode shell recipes for routine exploration when native tools can do the job; encode intent and preferred tool classes instead
 - [ ] For shell-only workflows (e.g., `gh`, `git`, `bundle show`, project CLIs), explicit command examples are acceptable when they are simple, task-scoped, and not chained together
 
@@ -245,9 +272,9 @@ Plugin config lives at `.compound-engineering/config.local.yaml` in the repo roo
 
 2. **Worktrees:** Gitignored files are per-worktree. A config file created in the main checkout does not exist in worktrees. When reading config, fall back to the main repo root if the file is missing in the current worktree:
    ```
-   !`(top=$(git rev-parse --show-toplevel 2>/dev/null); [ -n "$top" ] && cat "$top/.compound-engineering/config.local.yaml" 2>/dev/null) || (common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null); [ -n "$common" ] && cat "$(dirname "$common")/.compound-engineering/config.local.yaml" 2>/dev/null) || echo '__NO_CONFIG__'`
+   !`(top=$(git rev-parse --show-toplevel 2>/dev/null); [ -n "$top" ] && cat "$top/.compound-engineering/config.local.yaml" 2>/dev/null) || (common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null); main="${common%/.git}"; [ -n "$main" ] && cat "$main/.compound-engineering/config.local.yaml" 2>/dev/null) || echo '__NO_CONFIG__'`
    ```
-   The first subshell tries the current worktree root. The second derives the main repo root from `git-common-dir` as a fallback. The `[ -n "$top" ]` and `[ -n "$common" ]` guards matter: outside a git repo, both `git rev-parse` invocations emit empty, and an unguarded `cat "$(dirname "")/.compound-engineering/config.local.yaml"` would resolve to a CWD-relative `./...config.local.yaml` and could succeed against a stray file in the user's working directory. Guarded, both branches simply fail and the `__NO_CONFIG__` sentinel takes over. In a regular (non-worktree) checkout, both repo paths are identical.
+   The first subshell tries the current worktree root. The second derives the main repo root from `git-common-dir` (stripping the trailing `/.git` with `${common%/.git}`) as a fallback. Use parameter expansion rather than `dirname` because Claude Code's safety check rejects nested `$(dirname "$common")` inside double-quoted strings as "Unhandled node type: string" (see issue #709). The `[ -n "$top" ]` and `[ -n "$main" ]` guards matter: outside a git repo, both `git rev-parse` invocations emit empty, and an unguarded `cat "/.compound-engineering/config.local.yaml"` would attempt to read from `/`. Guarded, both branches simply fail and the `__NO_CONFIG__` sentinel takes over. In a regular (non-worktree) checkout, both repo paths are identical.
 
 If neither path has the file, fall through to defaults — never fail or block on missing config.
 

plugins/compound-engineering/skills/ce-compound/SKILL.md

@@ -22,7 +22,7 @@ Captures problem solutions while context is fresh, creating structured documenta
 
 ## Pre-resolved context
 
-**Repo name (pre-resolved):** !`common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null) && [ -n "$common" ] && basename "$(dirname "$common")"`
+**Repo name (pre-resolved):** !`git rev-parse --path-format=absolute --git-common-dir 2>/dev/null | sed -E 's|/\.git/?$||; s|.*/||'`
 
 **Git branch (pre-resolved):** !`git rev-parse --abbrev-ref HEAD 2>/dev/null`
 

plugins/compound-engineering/skills/ce-sessions/SKILL.md

@@ -16,7 +16,7 @@ Search your session history.
 
 ## Pre-resolved context
 
-**Repo name (pre-resolved):** !`common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null) && [ -n "$common" ] && basename "$(dirname "$common")"`
+**Repo name (pre-resolved):** !`git rev-parse --path-format=absolute --git-common-dir 2>/dev/null | sed -E 's|/\.git/?$||; s|.*/||'`
 
 **Git branch (pre-resolved):** !`git rev-parse --abbrev-ref HEAD 2>/dev/null`
 

plugins/compound-engineering/skills/ce-setup/SKILL.md

@@ -42,9 +42,9 @@ Display the script's output to the user.
 
 ### Step 3: Evaluate Results
 
-**Platform detection (pre-resolved):** !`[ -n "${CLAUDE_PLUGIN_ROOT}" ] && echo "CLAUDE_CODE" || echo "OTHER"`
+**Plugin root (pre-resolved):** !`echo "${CLAUDE_PLUGIN_ROOT}"`
 
-If the line above resolved to `CLAUDE_CODE`, this is a Claude Code session and `/ce-update` is available. Otherwise, omit any `/ce-update` references from output.
+If the line above resolved to an absolute path (starts with `/` and contains no `${`), this is a Claude Code session and `/ce-update` is available. Anything else — empty, the literal `${CLAUDE_PLUGIN_ROOT}` token, or an unresolved command string like `echo "${CLAUDE_PLUGIN_ROOT}"` left in place by a non-Claude harness that doesn't process `!` pre-resolution — means this is not Claude Code; omit any `/ce-update` references from output.
 
 After the diagnostic report, check whether:
 
@@ -66,7 +66,7 @@ If everything is installed, no repo-local cleanup is needed, and `.compound-engi
     Run /ce-setup anytime to re-check.
 ```
 
-If this is a Claude Code session, append to the message: "Run /ce-update to grab the latest plugin version."
+If this is a Claude Code session (the **Plugin root** above resolved to a non-empty path), append to the message: "Run /ce-update to grab the latest plugin version."
 
 Stop here.
 

plugins/compound-engineering/skills/ce-update/SKILL.md

@@ -9,6 +9,7 @@ description: |
   Code — it relies on the plugin harness cache layout.
 disable-model-invocation: true
 ce_platforms: [claude]
+allowed-tools: Bash(bash *upstream-version.sh), Bash(bash *currently-loaded-version.sh), Bash(bash *marketplace-name.sh)
 ---
 
 # Check Plugin Version
@@ -17,75 +18,72 @@ Verify the installed compound-engineering plugin version matches the upstream
 `plugin.json` on `main`, and recommend the update command if it doesn't.
 Claude Code only.
 
-## Pre-resolved context
-
-Only the **Skill directory** determines whether this session is Claude Code —
-if empty or unresolved, the skill requires Claude Code. The other sections may
-contain error sentinels even in valid Claude Code sessions; the decision logic
-below handles those cases.
-
-`${CLAUDE_SKILL_DIR}` is a Claude Code-documented substitution that resolves
-at skill-load time. For a marketplace-cached install it looks like
-`~/.claude/plugins/cache/<marketplace>/compound-engineering/<version>/skills/ce-update`,
-so the currently-loaded version is the basename two `dirname` levels up.
-
 The upstream version comes from `plugins/compound-engineering/.claude-plugin/plugin.json`
 on `main` rather than the latest GitHub release tag, because the marketplace
 installs plugin contents from `main` HEAD. Comparing against release tags
 false-positives whenever `main` is ahead of the last tag (the normal state
 between releases).
 
-**Skill directory:**
-!`echo "${CLAUDE_SKILL_DIR}"`
-
-**Latest upstream version:**
-!`version=$(gh api repos/EveryInc/compound-engineering-plugin/contents/plugins/compound-engineering/.claude-plugin/plugin.json --jq '.content | @base64d | fromjson | .version' 2>/dev/null) && [ -n "$version" ] && echo "$version" || echo '__CE_UPDATE_VERSION_FAILED__'`
+## Step 1: Probe versions
 
-**Currently loaded version:**
-!`echo "${CLAUDE_SKILL_DIR}" | grep -q "/plugins/cache/.*/compound-engineering/.*/skills/ce-update$" && basename "$(dirname "$(dirname "${CLAUDE_SKILL_DIR}")")" || echo '__CE_UPDATE_NOT_MARKETPLACE__'`
+Run these three scripts in parallel via the Bash tool. Each prints a single
+line of output; capture the values for the decision logic below. Use
+`${CLAUDE_SKILL_DIR}` so the path resolves correctly in both `claude --plugin-dir`
+local-development sessions and standard marketplace-cached installs.
 
-**Marketplace name:**
-!`echo "${CLAUDE_SKILL_DIR}" | grep -q "/plugins/cache/.*/compound-engineering/.*/skills/ce-update$" && basename "$(dirname "$(dirname "$(dirname "$(dirname "${CLAUDE_SKILL_DIR}")")")")" || echo '__CE_UPDATE_NOT_MARKETPLACE__'`
+```bash
+bash "${CLAUDE_SKILL_DIR}/scripts/upstream-version.sh"
+bash "${CLAUDE_SKILL_DIR}/scripts/currently-loaded-version.sh"
+bash "${CLAUDE_SKILL_DIR}/scripts/marketplace-name.sh"
+```
 
-## Decision logic
+`scripts/upstream-version.sh` reads `plugin.json` on `main` via `gh api`. It
+prints the version string, or the sentinel `__CE_UPDATE_VERSION_FAILED__` if
+`gh` is unavailable or rate-limited.
 
-### 1. Platform gate
+`scripts/currently-loaded-version.sh` and `scripts/marketplace-name.sh` parse
+`${CLAUDE_SKILL_DIR}` against the marketplace-cache layout
+`~/.claude/plugins/cache/<marketplace>/compound-engineering/<version>/skills/ce-update`.
+They print the version segment / marketplace segment, or the sentinel
+`__CE_UPDATE_NOT_MARKETPLACE__` if the path doesn't match (typical for
+`claude --plugin-dir` local development).
 
-If **Skill directory** is empty or unresolved: tell the user this skill
-requires Claude Code and stop. No further action.
+## Step 2: Apply decision logic
 
-### 2. Handle failure cases
+### Handle failure cases
 
-If **Latest upstream version** contains `__CE_UPDATE_VERSION_FAILED__`: tell
+If `scripts/upstream-version.sh` printed `__CE_UPDATE_VERSION_FAILED__`: tell
 the user the upstream version could not be fetched (gh may be unavailable or
 rate-limited) and stop.
 
-If **Currently loaded version** contains `__CE_UPDATE_NOT_MARKETPLACE__`: this
-session loaded the skill from outside the standard marketplace cache (typical
-when using `claude --plugin-dir` for local development, or for a non-standard
-install). Tell the user (substituting the actual path):
+If `scripts/currently-loaded-version.sh` printed
+`__CE_UPDATE_NOT_MARKETPLACE__`: the skill is loaded from outside the
+standard marketplace cache. Two cases collapse to the same handling: a
+`claude --plugin-dir` local-development session, or a non-Claude-Code
+platform (this skill is Claude Code-only because it relies on the plugin
+harness cache layout). Tell the user:
 
-> "Skill is loaded from `{skill-directory}` — not the standard marketplace
-> cache at `~/.claude/plugins/cache/`. This is normal when using
+> "Skill is loaded from outside the marketplace cache at
+> `~/.claude/plugins/cache/`. This is normal when using
 > `claude --plugin-dir` for local development. No action for this session.
 > Your marketplace install (if any) is unaffected — run `/ce-update` in a
 > regular Claude Code session (no `--plugin-dir`) to check that cache."
 
 Then stop.
 
-### 3. Compare versions
+### Compare versions
 
-**Up to date** — `{currently loaded} == {latest upstream}`:
+**Up to date** — `currently_loaded == upstream`:
 
 > "compound-engineering **v{version}** is installed and up to date."
 
-**Out of date** — `{currently loaded} != {latest upstream}`:
+**Out of date** — `currently_loaded != upstream`:
 
-> "compound-engineering is on **v{currently loaded}** but **v{latest upstream}** is available.
+> "compound-engineering is on **v{currently_loaded}** but **v{upstream}** is available.
 >
 > Update with:
 > ```
-> claude plugin update compound-engineering@{marketplace-name}
+> claude plugin update compound-engineering@{marketplace_name}
 > ```
 > Then restart Claude Code to apply."
 

plugins/compound-engineering/skills/ce-update/scripts/currently-loaded-version.sh

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+# Print the version segment of the skill's own location when it matches the
+# marketplace cache layout `~/.claude/plugins/cache/<marketplace>/compound-engineering/<version>/skills/ce-update`,
+# or the literal sentinel `__CE_UPDATE_NOT_MARKETPLACE__` otherwise.
+#
+# Derives skill_dir from BASH_SOURCE rather than $CLAUDE_SKILL_DIR because
+# CLAUDE_SKILL_DIR is documented as a SKILL.md content substitution and is
+# not a guaranteed environment variable for Bash tool subprocesses. The
+# script is invoked as `bash "${CLAUDE_SKILL_DIR}/scripts/<name>.sh"`, so
+# BASH_SOURCE[0] is the absolute script path either way and self-locating
+# is reliable.
+
+set -u
+
+script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+skill_dir="$(dirname "$script_dir")"
+
+# Match `.../plugins/cache/*/compound-engineering/<version>/skills/ce-update[/]?`
+# Capture group 1 is the version segment.
+version=$(printf '%s\n' "$skill_dir" | sed -nE 's|.*/plugins/cache/[^/]+/compound-engineering/([^/]+)/skills/ce-update/?$|\1|p')
+
+if [ -n "$version" ]; then
+  echo "$version"
+else
+  echo '__CE_UPDATE_NOT_MARKETPLACE__'
+fi

plugins/compound-engineering/skills/ce-update/scripts/marketplace-name.sh

@@ -0,0 +1,22 @@
+#!/usr/bin/env bash
+# Print the marketplace-name segment of the skill's own location when it
+# matches the marketplace cache layout
+# `~/.claude/plugins/cache/<marketplace>/compound-engineering/<version>/skills/ce-update`,
+# or the literal sentinel `__CE_UPDATE_NOT_MARKETPLACE__` otherwise.
+#
+# Derives skill_dir from BASH_SOURCE rather than $CLAUDE_SKILL_DIR — see
+# currently-loaded-version.sh for the rationale.
+
+set -u
+
+script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+skill_dir="$(dirname "$script_dir")"
+
+# Capture group 1 is the marketplace segment.
+marketplace=$(printf '%s\n' "$skill_dir" | sed -nE 's|.*/plugins/cache/([^/]+)/compound-engineering/[^/]+/skills/ce-update/?$|\1|p')
+
+if [ -n "$marketplace" ]; then
+  echo "$marketplace"
+else
+  echo '__CE_UPDATE_NOT_MARKETPLACE__'
+fi

plugins/compound-engineering/skills/ce-update/scripts/upstream-version.sh

@@ -0,0 +1,17 @@
+#!/usr/bin/env bash
+# Print the upstream `version` field from plugins/compound-engineering/.claude-plugin/plugin.json
+# on main, or the literal sentinel `__CE_UPDATE_VERSION_FAILED__` if the lookup fails.
+#
+# Compared to release tags, this reads the current main HEAD because the marketplace
+# installs plugin contents from main HEAD; comparing against tags false-positives
+# whenever main is ahead of the last tag.
+
+set -u
+
+version=$(gh api repos/EveryInc/compound-engineering-plugin/contents/plugins/compound-engineering/.claude-plugin/plugin.json --jq '.content | @base64d | fromjson | .version' 2>/dev/null)
+
+if [ -n "$version" ]; then
+  echo "$version"
+else
+  echo '__CE_UPDATE_VERSION_FAILED__'
+fi

plugins/compound-engineering/skills/ce-work-beta/SKILL.md

@@ -43,7 +43,7 @@ After extracting tokens from arguments, resolve the delegation state using this
 3. **Hard default** -- `false` (delegation off)
 
 **Config (pre-resolved):**
-!`(top=$(git rev-parse --show-toplevel 2>/dev/null); [ -n "$top" ] && cat "$top/.compound-engineering/config.local.yaml" 2>/dev/null) || (common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null); [ -n "$common" ] && cat "$(dirname "$common")/.compound-engineering/config.local.yaml" 2>/dev/null) || echo '__NO_CONFIG__'`
+!`(top=$(git rev-parse --show-toplevel 2>/dev/null); [ -n "$top" ] && cat "$top/.compound-engineering/config.local.yaml" 2>/dev/null) || (common=$(git rev-parse --path-format=absolute --git-common-dir 2>/dev/null); main="${common%/.git}"; [ -n "$main" ] && cat "$main/.compound-engineering/config.local.yaml" 2>/dev/null) || echo '__NO_CONFIG__'`
 
 If the block above contains YAML key-value pairs, extract values for the keys listed below.
 If it shows `__NO_CONFIG__`, the file does not exist — all settings fall through to defaults.