fix(scripts): validate the precompute gate's base ref in the parent shell

[kyle-sexton]

Summary

scripts/check-skill-precompute-compose.sh <base-ref> treated an invalid base
ref as a clean pass.

Target discovery ran inside mapfile -t targets < <( ... ). The base-ref
branch validated the ref and, on failure, printed an error and ran exit 2 --
but that exit terminated only the process-substitution subshell. mapfile's
own status reflects the read, and a process substitution's exit code is not
propagated, so the parent saw nothing but an empty targets, scanned zero
files, printed No skill SKILL.md files in scope — nothing to gate. and
exited 0.

A typo'd branch or a shallow clone missing the ref therefore read as "nothing
to gate". The error text did reach stderr, but the exit code CI gates on said
success: a silent pass in exactly the situation the validation exists to catch.

Reproduced on main:

$ bash scripts/check-skill-precompute-compose.sh no-such-ref-xyz
No skill SKILL.md files in scope — nothing to gate.
rc=0

Fix

git rev-parse --verify is hoisted into the parent shell, ahead of the
mapfile, so its exit 2 is the script's exit 2. The cd at the top of the
script already put us at the repo root, so nothing about the check moves. The
now-redundant base="$first" indirection goes with it, and the mode dispatch
keeps --all and --paths out of the check.

After:

$ bash scripts/check-skill-precompute-compose.sh no-such-ref-xyz
Error: base ref no-such-ref-xyz is not a valid commit
rc=2

One deliberate behavior change beyond the issue: a bare -- is let through by
the argument parser as first and was never a working paths separator. It
previously printed the same "not a valid commit" error and exited 0; it now
exits 2 with it. That is the same correction, applied to the same wrong exit
code.

Verification

Two new cases in scripts/check-skill-precompute-compose.test.sh:

• an invalid base ref against a git fixture that has a real commit (so the
  failure is unambiguously about the bogus ref, not an empty HEAD), asserting
  rc 2 and naming rc 0 explicitly as the scripts: check-skill-precompute-compose treats an invalid base ref as a pass because exit 2 dies inside a process substitution #3377 shape;
• --all against a fixture with no git repository at all, which pins the mode
  dispatch that the hoisted validation now depends on.

Results:

• With the fix: PASS=13 FAIL=0.
• With check-skill-precompute-compose.sh reverted to HEAD, the base-ref
  case fails with
  FAIL: an invalid base ref passed SILENTLY (rc=0), the #3377 shape
  (PASS=12 FAIL=1). The --all case passes pre-fix by design.
• scripts/affected-tests.sh --run on both changed files selects
  scripts/check-skill-precompute-compose.test.sh and nothing else, and is
  green (All 1 selected suites passed or were skipped.).
• Modes unchanged on the real tree: a valid base ref (HEAD) still exits 0,
  and the existing suite's --paths, --strict --paths, bare base-ref,
  --strict <base-ref>, no-args, --help, -h and --strict-alone cases all
  still pass.
• CI invokes this gate as --all (ci.yml:1493), which never reaches the
  validation, so no CI lane changes behavior.
• shellcheck --rcfile .shellcheckrc and
  scripts/check-shell-portability.sh --paths on both files: clean. (shfmt -d
  reports one pre-existing diff at --strict) STRICT=1; shift ;;, untouched
  here and unrelated to this change; shfmt is not a CI gate.)

Related

Closes #3377

Found by the batch-simplify sweep on claude/code-tidying-batch-simplify-s7ljbi
and deliberately left unfixed there because that sweep was behavior-preserving.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 1m 41s —— View job

---

Claude's Code Review

• Gather context (PR diff, description, repo instructions)
• Understand the request (invoke /review:code-review)
• Review changed files for correctness/maintainability
• Post findings (summary)

Scope: scripts/check-skill-precompute-compose.sh, scripts/check-skill-precompute-compose.test.sh (diff: git diff origin/main...HEAD).

Verdict: No blocking issues found. This is a small, well-scoped fix.

What I checked:

• Traced the control flow: the git rev-parse --verify check is hoisted into the parent shell (lines 101–109), ahead of the mapfile -t targets < <(...) process substitution (lines 111–125). This does fix the described bug: previously exit 2 inside the process substitution only terminated that subshell, and mapfile's status reflects the read rather than the subshell's exit code, so an invalid ref silently produced zero targets and a 0 exit.
• Confirmed the mode dispatch (--all | --paths skip the validation; everything else is checked) is correct and consistent with how --all/--paths are used elsewhere in the file, and that CI's --all invocation (ci.yml:1493) never reaches the new validation, so no behavior change on the actual CI gate.
• Confirmed the redundant base="$first" removal is a clean no-op refactor — "$first" is used directly in the git diff --name-only call where "$base" was before.
• The noted deliberate behavior change (bare -- now exits 2 instead of silently passing) is consistent with the fix and correctly called out in the PR description.
• Reviewed the two new test cases in check-skill-precompute-compose.test.sh (lines 190–227): both correctly target the fixed regression — one asserts rc=2 for a bad ref against a real git fixture (with an explicit branch for the old silent-pass shape), the other pins --all still working against a fixture with no .git at all, which is exactly the case that would break if the hoisted validation accidentally ran unconditionally.

Not independently verified: the Bash tool in this review lane didn't have permission to execute the test suite or shellcheck/shfmt (only gh pr diff/view/comment/review are pre-approved for this lane), so I could not run check-skill-precompute-compose.test.sh myself. The PR description's test/lint output (PASS=13 FAIL=0, shellcheck/shfmt clean) is author-claimed and unverified by me — I confirmed correctness via static trace of the control flow instead.

No inline comments posted — nothing rose to the high-signal bar for this lane.

[claude]

Claude finished @kyle-sexton's task in 55s —— View job

---

Security review

• Load /review:security-review skill instructions
• Gather PR diff and read changed files
• Apply skip gate
• Hunt for security issues (logic/trust-boundary/injection/Actions)
• Adversarially validate any candidate findings
• Post final report

No security issues found.

This PR touches only scripts/check-skill-precompute-compose.sh and its test file. The change hoists an existing git rev-parse --verify check out of a process-substitution subshell (mapfile -t targets < <( ... )) into the parent shell, so the check's exit 2 actually terminates the script instead of only the subshell. That's a correctness fix for a gate that was silently passing (exit 0) on an invalid/typo'd base ref — no new capability, input source, or trust boundary is introduced.

Reviewed for security-relevant surface and found none:

• $first (the base-ref argument) is passed as a literal argument to git rev-parse --verify and git diff --name-only, never interpolated into a shell string or eval'd, so there's no command-injection path from an attacker-controlled ref name.
• The script is invoked only with --all in CI (ci.yml:1493), which the mode-dispatch (case "$first" in --all | --paths) ;;) explicitly routes around the new validation, so CI's actual invocation path is unaffected by this change.
• No secrets, credentials, network calls, permissions, or GitHub Actions trigger/permission changes are involved.
• The test additions only add local git fixtures under a temp dir; no new untrusted input handling.

This falls under the skip-gate condition of "no security-relevant surface after reading the diff" — it's a shell-scripting correctness fix (subshell exit-code propagation), not a vulnerability or a fix for one.

[github-actions]

Last security-reviewed head: 001f66bf9586c5ae2e2b1964730fc3c7012d59b0. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[github-actions]

Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count.