From 048478e71bb389475829646f39e62bbfce19d0b3 Mon Sep 17 00:00:00 2001 From: tigers1997 Date: Wed, 26 Aug 2026 10:15:36 -0400 Subject: [PATCH] fix(hooks): escape cmd_* answers for the quoting context they land in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A check command containing a double quote was interpolated raw into the generated Stop hook's "label|" bash array element. The quote closed the element early and the whole CHECKS=(...) array stopped parsing, so the hook died at load with a syntax error, emitted no report, and left a check loop that looks installed. Reported from a downstream project with: cmd_lint = python -c "import ast,io,glob; [ast.parse(...)]" Silent failure is the worst outcome for a check loop, and the quieter half of the same bug is worse still: a command with an embedded quote but no parentheses parses without error and splits into several array elements, so the check runs truncated and nothing complains. The .ps1 variant had the bug mirrored — a single quote breaks the single-quoted PowerShell hashtable value — but nobody had hit it yet. substitute_placeholders() now takes an escaper chosen per target extension: backslash, ", $ and backtick for the double-quoted bash literal in stop-run-checks.sh; ' doubled for the single-quoted PowerShell literal in stop-run-checks.ps1. Prose targets (CLAUDE.md) keep the raw answer, so the escaping cannot leak into documentation. Escaping applies at interpolation only: eval "$run_cmd" still expands $VAR and $(...) in the command at run time, as it always did. test-command-quoting.sh scaffolds with a command carrying ", ', $, backtick, backslash, parens and a semicolon, then asserts both hooks parse and that all three commands round-trip byte-exact out of the rendered literal — escaping that mangles the command is no better than escaping that breaks the parse. Reverting the fix reproduces the reported syntax error verbatim. The PowerShell probe skips cleanly where no PowerShell is on PATH. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Ke9nVhgkkvmXF55iv5nopf --- CHANGELOG.md | 2 + configure.py | 40 ++++- test/stop-run-checks/test-command-quoting.sh | 146 +++++++++++++++++++ 3 files changed, 184 insertions(+), 4 deletions(-) create mode 100644 test/stop-run-checks/test-command-quoting.sh diff --git a/CHANGELOG.md b/CHANGELOG.md index 1153f84..cfbbc40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ All notable changes to this project. Format: [Keep a Changelog](https://keepacha ## Unreleased +- **fix(hooks): escape `cmd_*` answers for the quoting context they land in.** A check command containing a double quote — `python -c "import ast,io,glob; [...]"` is the report that surfaced this — was interpolated raw into the generated Stop hook's `"label|"` bash array element. The quote closed the element early and the whole `CHECKS=(...)` array stopped parsing, so the hook died at load with a syntax error, emitted no report, and left a check loop that *looks* installed. Silent failure is the worst outcome for a check loop, and the quieter half of the same bug is worse still: a command with an embedded quote but no parentheses parses without error and splits into several array elements, so the check runs truncated and nothing complains. Values are now escaped per target extension — backslash, `"`, `$` and backtick for the double-quoted bash literal in `stop-run-checks.sh`; `'` doubled for the single-quoted PowerShell literal in `stop-run-checks.ps1` — while prose targets (`CLAUDE.md`) keep the raw answer. The escaping applies at interpolation only: `eval` still expands `$VAR` and `$(...)` in the command at run time, as it always did. A regression test asserts both hooks parse and that all three commands round-trip byte-exact out of the rendered literal. If you are already scaffolded: the retrofit path is non-destructive, so a re-run stages the corrected `stop-run-checks.sh` to `.claude-retrofit/incoming/` and leaves the broken file live — promote it, or re-run with `--on-collision=overwrite`, to actually pick up the fix. + - **chore(ci): own-CI reviewer repinned `claude-sonnet-4-6` → `claude-sonnet-5`.** Same tier and list price, so the v2.7.0 cost decision stands unchanged; this is a currency bump, not a re-opening of that call. Landed on its own because `anthropics/claude-code-action@v1` refuses to run when the PR's `review.yml` differs from the copy on the default branch ("Workflow validation failed") — carrying a repin inside a feature branch therefore *silently disables the AI review on that PR and every PR stacked on it*, with no red check to notice. A `review.yml` change has to be its own PR, where verdict-gate's self-bootstrap escape hatch is the designed handling. - **feat: generate `templates/INDEX.md`, scaffold `.claude/workflows/`, and document teams / channels / routines.** Three gaps the currency audit left open. **(1)** `templates/INDEX.md` was hand-maintained and had gone stale enough to mislead — it still referenced a `configurator.html` that no longer exists and was missing half the modules. It is now **generated** from `MODULES` by `python3 configure.py --write-index`, and `--check` fails when the committed copy and the generator disagree, so it cannot drift again. **(2)** Dynamic workflows have been a first-class Claude Code surface since 2.1.154 and the configurator scaffolded nothing for them. The `multi-agent` module now ships `.claude/workflows/spec-fanout.js` (runs as `/spec-fanout`), which generates N variants of one spec into disjoint slots and then **screens each variant against the spec** before reporting. It is the workflow-native successor to the `/infinite` skill in the same module — same job, but the runtime holds the loop and the intermediate results, the run is resumable, and the screening pass is a real gate rather than a suggestion. Project workflows under `.claude/workflows/` are shared with everyone who clones the repo. `--check` gained a rule validating that every shipped workflow declares a usable `meta` block and uses no `import()` (the runtime rejects both). `workflowSizeGuideline` is stubbed in `settings.local.json.example`. **(3)** `docs/04` gains a table comparing the five ways to run work in parallel (subagent / skill / agent team / workflow / worktree session) by *who holds the plan*, and states plainly why the configurator ships no templates for agent teams, channels or routines: teams are spawned in conversation and live for a session (only `teammateMode` is worth setting, and it's a per-machine terminal preference — stubbed in settings.local); the channel gate keys `channelsEnabled` and `allowedChannelPlugins` are **managed-settings only**, so a project cannot enable them; and routines are scheduled cloud agents that run against a repo rather than from your checkout, where a `Stop` or `SessionStart` hook is the project-scoped equivalent. diff --git a/configure.py b/configure.py index 31e659b..2a24a80 100755 --- a/configure.py +++ b/configure.py @@ -489,12 +489,40 @@ def compute_placeholders(form_values: dict, selected: set, module_flags: dict = return v -def substitute_placeholders(text: str, values: dict) -> str: +def escape_bash_dq(s: str) -> str: + r"""Escape a value being interpolated into a double-quoted bash string. + + Generated hooks embed cmd_* answers as `"label|"` array elements. + A raw `"` in the command closes the element early and the whole CHECKS + array fails to parse — the hook is then dead on arrival and fails + silently, which looks identical to "installed and passing". `$` and + backtick would expand (or execute) at parse time; `\` would eat the next + character. Backslash goes first so the escapes we add aren't re-escaped. + """ + for ch in ("\\", '"', "$", "`"): + s = s.replace(ch, "\\" + ch) + return s + + +def escape_pwsh_sq(s: str) -> str: + """Escape a value being interpolated into a single-quoted PowerShell + string. Doubling is the only escape a single-quoted string recognizes — + everything else, backtick included, is already literal.""" + return s.replace("'", "''") + + +# Quoting context of the template being rendered, keyed by file extension. +# Templates with no entry (Markdown, JSON, …) substitute values verbatim. +SHELL_ESCAPERS = {".sh": escape_bash_dq, ".ps1": escape_pwsh_sq} + + +def substitute_placeholders(text: str, values: dict, escape=None) -> str: import re def repl(m): k = m.group(1) if k in values and values[k] is not None: - return str(values[k]) + v = str(values[k]) + return escape(v) if escape else v return m.group(0) return re.sub(r"\{\{(\w+)\}\}", repl, text) @@ -2451,7 +2479,9 @@ def collect_files(form_values: dict, selected: set, module_flags: dict = None) - src = TEMPLATE_DIR / rel content = src.read_text(encoding="utf-8") if "{{" in content: - content = substitute_placeholders(content, placeholders) + content = substitute_placeholders( + content, placeholders, + SHELL_ESCAPERS.get(os.path.splitext(rel)[1])) files.append({ "target": tgt, "content": content, @@ -2472,7 +2502,9 @@ def collect_files(form_values: dict, selected: set, module_flags: dict = None) - src = TEMPLATE_DIR / rel content = src.read_text(encoding="utf-8") if "{{" in content: - content = substitute_placeholders(content, placeholders) + content = substitute_placeholders( + content, placeholders, + SHELL_ESCAPERS.get(os.path.splitext(rel)[1])) files.append({ "target": tgt, "content": content, diff --git a/test/stop-run-checks/test-command-quoting.sh b/test/stop-run-checks/test-command-quoting.sh new file mode 100644 index 0000000..f374c7a --- /dev/null +++ b/test/stop-run-checks/test-command-quoting.sh @@ -0,0 +1,146 @@ +#!/usr/bin/env bash +# cmd_* answers are interpolated into a quoted string literal in the generated +# Stop hook — a double-quoted bash array element, a single-quoted PowerShell +# hashtable value. Anything the host shell treats as syntax inside those quotes +# has to be escaped on the way in, or the literal ends early. +# +# The real-world break: a lint command with an embedded double quote, e.g. +# python -c "import ast,io,glob; [ast.parse(io.open(f).read()) for f in glob.glob('*.py')]" +# rendered raw into `"lint|"`, and the whole CHECKS array stopped parsing. +# The hook then failed at load with a syntax error and produced no report — +# the worst failure mode for a check loop, because it still looks installed. +# The quieter half of the same bug: a command with no parens splits into +# several array elements instead of erroring, and the check runs truncated. +# +# Both shells are asserted the same way: the file must parse, and each command +# must round-trip byte-for-byte out of the rendered literal. +# +# PowerShell probes skip cleanly when no PowerShell is on PATH (pwsh ships on +# all three GitHub runner images; a developer machine may not have it). +set -euo pipefail + +PS="" +for candidate in pwsh powershell.exe powershell; do + if command -v "$candidate" >/dev/null 2>&1; then PS="$candidate"; break; fi +done + +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT + +# Every metacharacter that means something inside the target quoting context: +# double quote and single quote (the literal terminators), `$` and backtick +# (expansion/substitution in bash), backslash (escape in bash), and parens + +# semicolon (what turns a broken literal into a hard parse error). +LINT_CMD='python -c "import ast,io,glob; [ast.parse(io.open(f).read()) for f in glob.glob('"'"'*.py'"'"')]"' +TEST_CMD='sh -c "echo $HOME `id -u` \ done"' +TYPECHECK_CMD="mypy --config 'a b.ini' ." + +python3 configure.py --persona solo-experienced --yes --save-config-only "$tmp/cfg.json" >/dev/null +LINT_CMD="$LINT_CMD" TEST_CMD="$TEST_CMD" TYPECHECK_CMD="$TYPECHECK_CMD" \ +python3 - "$tmp/cfg.json" <<'PY' +import json, os, sys +path = sys.argv[1] +cfg = json.load(open(path, encoding="utf-8")) +cfg["formValues"]["cmd_lint"] = os.environ["LINT_CMD"] +cfg["formValues"]["cmd_test"] = os.environ["TEST_CMD"] +cfg["formValues"]["cmd_typecheck"] = os.environ["TYPECHECK_CMD"] +json.dump(cfg, open(path, "w", encoding="utf-8"), indent=1) +PY + +# --- bash ------------------------------------------------------------------ +bashdir="$tmp/bash" +mkdir -p "$bashdir" +python3 configure.py --config "$tmp/cfg.json" --yes --dir "$bashdir" >/dev/null + +hook="$bashdir/.claude/hooks/stop-run-checks.sh" +[ -f "$hook" ] || { echo "FAIL: bash hook not scaffolded at $hook"; exit 1; } + +if ! bash -n "$hook" 2>"$tmp/bash-syntax.err"; then + echo "FAIL: rendered stop-run-checks.sh does not parse:" + cat "$tmp/bash-syntax.err" + exit 1 +fi + +# Source just the CHECKS array and print it back. Byte-exact round-trip is the +# assertion — escaping that mangles the command is no better than escaping that +# breaks the parse. +sed -n '/^CHECKS=(/,/^)/p' "$hook" > "$tmp/checks.sh" +bash -c 'set -u; . "$1"; printf "%s\n" "${CHECKS[@]}"' _ "$tmp/checks.sh" > "$tmp/checks.out" + +{ + printf 'typecheck|%s\n' "$TYPECHECK_CMD" + printf 'lint|%s\n' "$LINT_CMD" + printf 'test|%s\n' "$TEST_CMD" +} > "$tmp/checks.expected" + +if ! diff -u "$tmp/checks.expected" "$tmp/checks.out"; then + echo "FAIL: CHECKS entries didn't round-trip (expected left, got right)" + exit 1 +fi +echo " bash OK: CHECKS parses and all 3 commands round-trip byte-exact" + +# The escaping must not leak into prose targets — CLAUDE.md renders the same +# answers inside Markdown backticks, where a bash backslash would be visible. +grep -qF -- "- Lint: \`$LINT_CMD\`" "$bashdir/CLAUDE.md" \ + || { echo "FAIL: CLAUDE.md got shell-escaped cmd_lint; escaping leaked out of .sh"; exit 1; } +echo " bash OK: CLAUDE.md kept the raw command (escaping is per-extension)" + +# --- powershell ------------------------------------------------------------ +psdir="$tmp/ps" +mkdir -p "$psdir" +python3 configure.py --config "$tmp/cfg.json" --yes --hook-shell powershell --dir "$psdir" >/dev/null + +pshook="$psdir/.claude/hooks/stop-run-checks.ps1" +[ -f "$pshook" ] || { echo "FAIL: powershell hook not scaffolded at $pshook"; exit 1; } + +# A raw single quote survives only as ''. Cheap check that runs without pwsh. +if grep -q "config 'a b.ini'" "$pshook"; then + echo "FAIL: single quotes in cmd_typecheck reached the .ps1 undoubled" + exit 1 +fi + +if [ -z "$PS" ]; then + echo " SKIP: no PowerShell on PATH; .ps1 parse/round-trip probes not run" +else + # The probe lives in a file rather than -Command: it needs single-quoted + # regexes (PowerShell interpolates `$` inside double quotes), which don't + # survive nesting inside the single-quoted bash argument. + cat > "$tmp/probe.ps1" <<'PROBE' +$ErrorActionPreference = 'Stop' +$f = $args[0] +$errs = $null +$null = [System.Management.Automation.Language.Parser]::ParseFile($f, [ref]$null, [ref]$errs) +if ($errs.Count) { + $errs | ForEach-Object { $_.Message } | Write-Host + throw 'rendered stop-run-checks.ps1 does not parse' +} +# Re-evaluate the $checks literal in isolation and emit label|command. +$m = [regex]::Match((Get-Content $f -Raw), '(?s)\$checks = @\(.*?\n\)') +if (-not $m.Success) { throw 'could not locate the $checks block' } +$c = & ([scriptblock]::Create($m.Value + "`n`$checks")) +$c | ForEach-Object { '{0}|{1}' -f $_.label, $_.command } +PROBE + + # Git Bash hands out MSYS paths; PowerShell needs the Windows form. + if command -v cygpath >/dev/null 2>&1; then + probe_arg=$(cygpath -w "$tmp/probe.ps1"); hook_arg=$(cygpath -w "$pshook") + else + probe_arg="$tmp/probe.ps1"; hook_arg="$pshook" + fi + + if ! "$PS" -NoProfile -ExecutionPolicy Bypass -File "$probe_arg" "$hook_arg" > "$tmp/ps.out" 2>&1; then + echo "FAIL: powershell probe failed" + cat "$tmp/ps.out" + exit 1 + fi + + # PowerShell writes CRLF; --strip-trailing-cr keeps that from reading + # as a round-trip failure (the expected file is LF). + if ! diff -u --strip-trailing-cr "$tmp/checks.expected" "$tmp/ps.out"; then + echo "FAIL: \$checks entries didn't round-trip (expected left, got right)" + exit 1 + fi + echo " powershell OK ($PS): \$checks parses and all 3 commands round-trip byte-exact" +fi + +echo "PASS: cmd_* answers survive interpolation into both generated Stop hooks"