feat(source-control): harden /commit — exec-bit backstop, trailer SSOT, composition resilience

[kyle-sexton]

Closes #1579
Closes #1581
Closes #1583
Closes #1584

Summary

Six findings from a post-use behavioral audit of source-control:commit (audited at 0.25.1; the
skill had zero content changes between then and 0.32.1, so all six were still live). Every
finding was verified before implementing — two claims are refuted, one tier is deferred with
its own issue, and one previously-unknown cross-platform hazard was found while implementing.

Fix

Exec-bit backstop — #1579

The ordered exec-bit procedure was advisory prose with no tier under it. Two tiers now sit below it:

• a pre-computed probe at the top of the skill, inside the documented 5,000-token compaction
  re-attach window, reporting staged newly-added shebang files still at 100644;
• skills/commit/scripts/exec-bit-check.sh (--list / --probe / --fix) with a 30-case
  .test.sh, making the per-commit step a command with an exit code instead of a paragraph to recall.

Both, because the probe is only a snapshot at invocation and cannot see files staged later in the
flow — the skill says so explicitly rather than implying the probe is the check.

New hazard found and pinned while implementing: under core.filemode=false — the default on
Windows/NTFS, and the setting in this repo — git ignores worktree permission bits entirely and
stages every file 100644. On such a repo chmod +x alone never reaches the index; only
git update-index --chmod=+x produces a 100755 entry. So on the most common platform here, the
half of the old prose procedure that "looks like it worked" did nothing. The script always performs
both writes, and the test pins the case with core.filemode set explicitly so it tests the same
thing everywhere.

Trailer resolution — #1581

The (<context>) clause is now optional. A census of this repo found compliance not merely low
but collapsing: 41.6% of trailers carry the clause over the last 150 commits, 12.1% over the
last 40. A mandate nobody follows is worse than no mandate.

The ladder gains the rung it never had. Harness-injected commit guidance is neither a config layer
nor a project convention, so a session receiving both it and this skill had no stated tiebreak. It is
now rung 3, with an explicit rule: adopt its shape, never its literal text. Observed
first-hand this session — that guidance can carry a hardcoded model name that does not match the
running session (a Fable 5 trailer injected into an Opus 5 session). Copying it verbatim writes a
false provenance claim into durable git history, which is exactly the harm the skill's own text says
the template exists to prevent.

Composition, structure, observability — #1583

• Composition is now two named forms — re-invoke /commit, or run the per-commit checklist
  yourself as commands — and a composing skill must say which. "Remembered convention" is neither.
  The policy also names what decays: not the message shape (reinforced visibly every commit) but
  the ordered per-commit checks, which produce no signal when skipped.
• Progressive-disclosure split into four reference/ spokes (format-check, exec-bit,
  pathspec-commits, staging-preconditions) with a load-when index, so the re-attach window is
  spent on the per-commit path rather than on ~130 lines of edge machinery. No rule was dropped —
  the staging preconditions keep their detection command and action inline as a table; only the
  per-condition rationale moved.
• A per-commit checklist at the top as the cheap re-anchor.
• Config-layer probes for all three layers. The tracked-team probe tests tracked-ness via
  git ls-files --error-unmatch, not file existence, and reports an untracked file at that path as
  present but UNTRACKED — not a config layer — preserving the rule 0.25.1 established rather than
  reintroducing it as a drafting-surface bug.

Recorded deviation — #1584

disable-model-invocation: false is now declared explicitly (same effective behavior as the
omitted default, but visible). A /commit-shaped skill is the canonical archetype for true, and
every other skill in this plugin declares the field. The body records the deviation, its reason
(composition requires model reachability), and the compensating controls. Prose, not an invented
frontmatter key.

Refuted / deferred — deliberate non-changes

• REFUTED: the "62 of 74 trailers" figure. It does not reproduce on any window of this branch —
  at the window where the total is 74, the non-compliant count is 49. The figures are wrong; the
  direction is right and the trend is worse than claimed. Corrected in the issue and changelog rather
  than repeated.
• REFUTED: "have setup write an explicit trailer_policy." Already implemented. trailer_policy
  is a documented key in reference/config-resolution.md, and /source-control:setup already
  interviews for it and writes it. No change made.
• DEFERRED with its own issue (source-control:commit: evaluate a PreToolUse exec-bit tier for memory-following commits (deferred from #1579) #1585): the PreToolUse hook tier. It is the only tier covering
  memory-following commits that never invoke the skill, but source-control ships zero hooks today
  and a Bash-matching hook changes the install footprint for every consumer — it needs its own
  security review. There is also an unsettled design question: guardrails already owns
  PreToolUse git-commit gating, so which plugin should own this check is genuinely open.
• DEFERRED, needs a verified claim: trailer key CASE. The census found a second, unreported drift
  axis — 134 of 137 trailers use Co-authored-by: against 3 using the skill's Co-Authored-By:.
  Git preserves the key's case verbatim (git interpret-trailers does not normalize it). Whether to
  standardize on the dominant lowercase form needs a verified claim about how the forge parses the
  key, which this work did not establish — so it is recorded as an open question in source-control:commit: Co-Authored-By trailer default drifts three ways and the ladder never names the harness #1581, not
  answered by assumption.

Verification

Dogfooded live, and it caught a real defect in this very PR. Both new scripts were chmod +x-ed
and staged — and staged at 100644 anyway, because core.filemode=false. The new probe reported
2 staged shebang file(s) still at mode 100644, --fix corrected both, and they are committed at
100755. This is precisely the failure the finding describes, caught pre-commit instead of by CI
post-push.

Gates run locally, all green:

• scripts/check-changed-skills.sh origin/main → PASS, 0 errors. All 3 base-ref trigger phrases
  preserved; description unchanged (418/1536); SKILL.md 304/500 lines; the new script test is
  auto-detected and passes.
• exec-bit-check.test.sh → 30 cases, 0 failures (detection scope, symlink skip, staged-deletion
  skip, already-tracked skip, worktree+index fix ordering, core.filemode=false pinned, pathspec
  limiting, paths with spaces, all error exits).
• shellcheck --rcfile .shellcheckrc → clean, with the repo's require-double-brackets and
  add-default-case optional checks enabled. shfmt -d → clean.
• scripts/check-shell-portability.sh / check-skill-portability.sh / check-skill-leaf-names.sh /
  check-silent-skips.sh → all clean.
• scripts/check-changelog-parity.sh --check-bump origin/main → pass (0.32.1 → 0.33.0 with a matching
  ## [0.33.0] entry). scripts/validate-plugins.sh → all manifests + catalog valid.
• markdownlint-cli2 over all 44 source-control markdown files → 0 issues.

Four evals added: the exec-bit backstop running as a command, an anti-pattern eval for copying the
harness trailer's hardcoded model verbatim, the untracked-team-config-file case, and the
composition-form case.

Every harness-behavior claim is grounded in a page fetched this session — the 5,000-token re-attach
budget, the 500-line SKILL.md guidance, and the disable-model-invocation semantics from
https://code.claude.com/docs/en/skills; the PreToolUse mechanism cited in #1585 from
https://code.claude.com/docs/en/hooks and https://code.claude.com/docs/en/plugins-reference.

Related

• Refs source-control:commit: evaluate a PreToolUse exec-bit tier for memory-following commits (deferred from #1579) #1585 — the deferred PreToolUse exec-bit tier, with its open ownership question.
• Refs audit: guardrails + source-control hardening — bypass gaps, convention-enforcement SSOT, decoupling #912 — the standing guardrails + source-control hardening umbrella this sits under.
• While filing these issues, guardrails' block-noncanonical-commit.sh false-fired on a
  gh issue create whose body text merely mentioned the guarded command. Noted in source-control:commit: evaluate a PreToolUse exec-bit tier for memory-following commits (deferred from #1579) #1585's
  precision section; it belongs with the existing hook-precision class (guardrails: hooks ship false-positive over-fires with no per-hook fixtures or shared precision convention (6-issue class: diff-scope, quoted-arg, boolean-intent, stdin, path-branch) #547), not this PR.
• Originating handoff item: 20260724-165259-source-control-commit-audit.

[claude]

Claude finished @kyle-sexton's task in 5m 15s —— View job

---

Code Review — feat(source-control): harden /commit

• Gather context and understand the request
• Read all changed files
• Review SKILL.md changes
• Review new reference/ spoke files
• Review exec-bit-check.sh script
• Review exec-bit-check.test.sh test script
• Review evals.json, plugin.json, and CHANGELOG.md
• Synthesize findings

---

This is a well-scoped and carefully documented hardening. The core.filemode=false discovery is a real platform hazard and the double-write fix is correct. Three findings below — one documentation/code mismatch that could mislead a future reader, one scope gap in the detection logic, and one minor test coverage gap — followed by several positive observations.

---

Finding 1 — Header comment overstates the --fix guarantee

File: exec-bit-check.sh lines 34–37

The header says:

Both writes are therefore always attempted: the chmod for the file's own sake and for filemode=true repos, the update-index for what actually gets committed.

But in --fix mode, when chmod +x fails for a disk-present file, continue exits the per-path block before git update-index runs:

# lines 213–227
if [[ -e "$path" ]]; then
  chmod +x -- "$path" 2>/dev/null || {
    …
    failed=1
    continue          # ← index update skipped
  }
fi
git update-index --chmod=+x -- "$path" …

The implementation is defensible — doing the index-only fix would create a worktree/index mismatch that a later git add would silently revert, so aborting the path is arguably safer. But the comment contradicts the code and would mislead someone debugging an edge case where chmod fails (e.g., a read-only mount, FAT32 with strict permissions). The core.filemode=false case is not affected in practice (chmod is a no-op that still exits 0), but the comment should match what the code does.

Suggested wording: replace "always attempted" with "both attempted when the worktree bit can be set; the index update is skipped if chmod fails to keep the two writes consistent." Fix this →

---

Finding 2 — C (copy) entries silently excluded from exec-bit detection

File: exec-bit-check.sh lines 146–149

When git diff --cached --name-status -z reports a copy (C<score> <src> <dst>), the while-loop reads and discards both _src and _dst — the destination is never added to added:

C*)
  IFS= read -r -d '' _src || break
  IFS= read -r -d '' _dst || break
  ;;

A file staged via copy (possible when --find-copies-harder is active or similarity thresholds are met) could need an exec-bit fix but will never be reported. The A (add) case covers the common workflow, so the practical impact is low — but the script's stated contract ("every path staged as a NEW file") doesn't explicitly exclude copies, and a copy destination is structurally a new file in the index.

Either add _dst to added alongside A entries, or explicitly document the exclusion in the header's "Deliberate scope limits" block alongside the symlink and tracked-file limits. Fix this →

---

Finding 3 — Test case 5b doesn't assert the worktree bit under core.filemode=false

File: exec-bit-check.test.sh lines 173–193

Case 5b pins the core.filemode=false regression and asserts the index mode:

bash "$HELPER" --repo-dir "$repo5b" --fix >/dev/null 2>&1
assert_eq "--fix reaches the index under core.filemode=false" \
  "100755" "$(staged_mode "$repo5b" nofilemode.sh)"

But the worktree chmod is also executed for the file's "own sake" (per the header comment and reference/exec-bit.md — worktree mismatch causes git status noise and breaks the pathspec-commit form). Case 5 (without core.filemode=false) does check the worktree bit at line 155, but case 5b doesn't. Adding the same check here — gated by skip_case if the bit isn't observable, as in case 5 — would make the regression test cover both writes rather than only the one that was the regression.

---

Positive observations

core.filemode=false handling. The double-write (worktree first, index second) with explicit git update-index --chmod=+x is correct. The reasoning is clearly explained in the header, in reference/exec-bit.md, and in the CHANGELOG. The live dogfood catch (the PR's own scripts shipped at 100644) is a strong confirmation that the check does what it claims.

NUL-delimited path parsing. The while IFS= read -r -d '' status loop with per-status field reads is correct and handles paths with spaces, quotes, and newlines. The pathspec-to-added mapping for A entries is exact; R and C field consumption prevents byte-stream misalignment.

Progressive-disclosure structure. Moving ~130 lines of edge machinery (pathspec commits, format-check caveats, staging rationale) into reference/ spokes is well-motivated by the 5,000-token re-attach window constraint. The load-when index table at the bottom of SKILL.md is a clean solution — the hub stays within the attach window while full detail is one explicit load away. No content was lost.

Trailer resolution ladder. Adding rung 3 (harness-injected guidance) closes a real gap — a session receiving both the harness instruction and the skill had no stated tiebreak. The "adopt shape, never literal text" rule with the false-provenance motivation is concrete and actionable. The new eval #11 directly tests the anti-pattern.

The exec-bit probe in the pre-computed context block. Combining an early-read snapshot (probe at load time) with a mandatory re-run at step 4 is the right two-tier design; the skill explicitly says the probe is a snapshot and cannot see files staged later. This avoids the false confidence of a probe that claims to substitute for the real check.

Version bump. 0.32.1 → 0.33.0 is correct for a feature release (new backstop, new reference spokes, new evals, behavior changes to the trailer ladder).

disable-model-invocation: false recorded deviation. Making the default explicit with a stated reason (composition requires model reachability) and named compensating controls is exactly the right approach. The deviation section doubles as documentation for any future reviewer wondering why this skill doesn't match the fleet archetype.

---

Three findings total: one documentation/code mismatch (comment overstates "always attempted"), one undocumented scope gap (copy destinations not checked), one test coverage gap (5b doesn't assert the worktree bit). None are blocking — all three are minor/informational and the overall implementation is solid.

[claude]

Claude finished @kyle-sexton's task in 3m 15s —— View job

---

Security Review — PR #1590

• Set up task list
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Result: No security vulnerabilities found

This PR adds one new shell script (exec-bit-check.sh) and its test suite, plus four reference markdown files and updates to SKILL.md and evals.json. Reviewed against injection surfaces, trust-boundary issues, filesystem hazards, and unsafe credential/secret handling.

exec-bit-check.sh

Option injection — all git invocations use -- before path arguments (L158, L163, L175, L214, L222). chmod +x -- "$path" is likewise guarded.

Path special characters — git diff is consumed NUL-delimited (-z) with IFS= read -r -d '' at L140–158, correctly handling spaces, newlines, and quotes in filenames. Rename (R*) and copy (C*) records consume their extra fields explicitly so the byte-stream stays in sync.

Blob SHA extraction — blob_sha is derived from the <mode> <sha> <stage>\t<path> format of git ls-files --stage, fed directly to git cat-file blob "$blob_sha". Because git only emits valid hex SHAs in this position, the string is controlled by git, not by any user-supplied input. The || continue guards every early-exit path.

--repo-dir filesystem traversal — cd -- "$repo_dir" accepts any path reachable by the process, but the effect is bounded: --list and --probe are read-only, and --fix only modifies files already staged as new additions in a git repository. The subsequent git rev-parse --is-inside-work-tree check at L129 gates all operations on a valid git context. No concern at the tool's designed threat surface.

${CLAUDE_PLUGIN_ROOT} in SKILL.md pre-computed context — the invocation bash "${CLAUDE_PLUGIN_ROOT}/skills/commit/scripts/exec-bit-check.sh" --probe double-quotes the expansion; this is set by the trusted plugin runtime, not by user input, so no injection vector.

pathspec-commits.md — hide/restore code sample

The sample at L96–143 correctly:

• arms the trap restore_hidden EXIT before any mv (L103) so the hidden file is restored on every exit path including failures;
• uses -e "$f" -o -L "$f" (L124, L100) rather than -e alone, correctly catching dangling symlinks where -e would return false;
• guards the hide path with both a disk check and git ls-files --error-unmatch (L126) to prevent clobbering a tracked file staged for deletion;
• uses -z NUL-delimited reads for the same path-safety reasons as the script.

exec-bit-check.test.sh

Temporary repos are created under mktemp -d with trap 'rm -rf "$TEST_TMPDIR"' EXIT. GPG signing is disabled via config only in the test-scoped repos. git config core.filemode false is set per-repo (not globally), so the test does not mutate the developer's git config. The source path "$SCRIPT_DIR/../../../scripts/test-helpers.sh" resolves within the plugin directory tree.

No security issues found.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f031174e7f

ℹ️ 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 finished @kyle-sexton's task in 3m 25s —— View job

---

PR Review — feat(source-control): harden /commit

• Gather context and understand the request
• Read exec-bit-check.sh in full
• Read exec-bit-check.test.sh in full
• Read updated SKILL.md
• Read new reference/ spokes
• Verify Codex P1 findings
• Verify previous review findings
• Synthesize and post findings

---

Two Codex P1 findings needed independent verification before this could merge. Both confirmed. Summary: one is a silent correctness failure in the real-world invocation path; one is a narrow but real security hazard. Three prior findings from the earlier Claude review are also confirmed below for completeness.

---

Finding 1 — Silent false-clean when script is invoked from a repo subdirectory (confirmed, high)

File: exec-bit-check.sh lines 174, 179

git diff --cached --name-status outputs paths relative to the repository root (per the git diff manpage: "file names are relative to the repository root if --relative is not specified"). The git ls-files --stage -- "$path" call on line 179 resolves its argument relative to the current working directory. When these two are not the same — i.e., when the script is invoked from a subdirectory — every git ls-files lookup returns an empty string, stage_line is empty, continue fires, and no offender is ever reported or fixed, even if staged shebang files exist.

The test suite uses --repo-dir "$repo" in every case, which cds to the test repo root. So all 30 test cases run from the root and do not exercise this path. In the real invocation from SKILL.md:

Exec-bit backstop: !`bash "${CLAUDE_PLUGIN_ROOT}/skills/commit/scripts/exec-bit-check.sh" --probe …`

— no --repo-dir is passed. If Claude Code's working directory happens to be a project subdirectory, the probe silently returns none and the backstop misses every staged shebang file.

Fix: After the git rev-parse --is-inside-work-tree guard, cd to $(git rev-parse --show-toplevel). Any pathspec arguments passed via -- <path>... must be resolved to repo-root-relative form before the cd. One approach:

toplevel="$(git rev-parse --show-toplevel)"
if [[ "${#paths[@]}" -gt 0 ]] && [[ "$(pwd)" != "$toplevel" ]]; then
  resolved=()
  for p in "${paths[@]}"; do
    resolved+=("$(realpath --relative-to="$toplevel" "$(pwd)/$p" 2>/dev/null || printf '%s' "$p")")
  done
  paths=("${resolved[@]}")
fi
cd -- "$toplevel" || { echo "$PROG: cannot enter repo root" >&2; exit 3; }

Fix this →

---

Finding 2 — chmod +x follows a worktree symlink, potentially reaching outside the repository (confirmed, medium)

File: exec-bit-check.sh lines 229–234

The index check (line 185) correctly skips entries with mode 120000. But that guard covers index symlinks. Once a file has been staged as 100644, the worktree path is free to be replaced by a symlink without re-staging. When that happens, [[ -e "$path" ]] returns true (follows the link to the target), and chmod +x -- "$path" follows the link and sets the execute bit on the symlink's target — which may be a file outside the repository entirely.

The attack surface is narrow (it requires either a race between staging and --fix, or a deliberate worktree manipulation), but the consequence — chmod +x applied to an unrelated file — is the exact hazard the index-mode guard was added to prevent and the prose in reference/exec-bit.md documents.

Fix: add an explicit -L check before the chmod:

if [[ -e "$path" ]]; then
  if [[ -L "$path" ]]; then
    echo "$PROG: warning: '$path' is a symlink in the worktree; setting index entry only" >&2
  else
    chmod +x -- "$path" 2>/dev/null || {
      echo "$PROG: chmod +x failed for '$path'" >&2
      failed=1
      continue
    }
  fi
fi
git update-index --chmod=+x -- "$path" …

Fix this →

---

Findings 3–5 — Confirming the earlier Claude review

These three were already reported; confirming all three from my read.

Finding 3 — Header comment says "Both writes are therefore always attempted" but at line 233, continue after a chmod failure skips git update-index. The comment is inaccurate for the failure path and would mislead a reader debugging a chmod-fails-but-index-should-still-be-set scenario (read-only mount, FAT32 with strict permissions). Minor documentation bug; the implementation choice itself is defensible.

Finding 4 — C* (copy) entries at lines 162–165 consume _src and _dst but never add _dst to added. A copy destination is structurally a new file in the index and could need an exec-bit fix. The scope doc says "every path staged as a NEW file (A)" — copies are implicitly excluded but not stated. Either add _dst to added or add "Copies skipped" to the "Deliberate scope limits" block.

Finding 5 — Test case 5b asserts the index mode (100755) after --fix under core.filemode=false, which is the regression being pinned. But the worktree chmod is also executed (for core.filemode=true repos and for git status cleanliness). Case 5 conditionally checks [[ -x "$repo5/fixme.sh" ]] with a skip_case fallback. Case 5b doesn't. On Linux, core.filemode=false means git ignores the worktree bit — it doesn't prevent chmod from actually setting it — so [[ -x "$repo5b/nofilemode.sh" ]] should pass on Linux and the check could be added with the same skip_case guard as case 5.

---

Overall

The core logic — double-write ordering, shebang-from-blob, NUL-delimited parsing, --fix scope refusal — is correct and well-tested on the paths the test suite covers. The progressive-disclosure structure, the trailer resolution ladder with its new rung 3, and the core.filemode=false discovery and fix are all solid. Finding 1 is the one that needs addressing before merge: the pre-computed probe is the primary consumer of the script and it runs without --repo-dir, so the CWD hazard is live in the exact codepath this PR added.

[claude]

Claude finished @kyle-sexton's task in 5m 0s —— View job

---

Security Review — PR #1590 feat(source-control): harden /commit

• Gather context and understand the request
• Read PR diff and changed shell scripts
• Analyze exec-bit-check.sh for security issues
• Analyze exec-bit-check.test.sh for security issues
• Analyze reference markdown files for embedded code samples
• Synthesize and post findings

---

This PR introduces exec-bit-check.sh (252 lines), its 30-case test suite, and four reference markdown files. The injection prevention posture is strong throughout: NUL-delimited parsing, -- before every path argument to git, blob-based shebang detection that never touches worktree paths during the read phase, and git update-index confined to paths already in the git-controlled index. No command injection, SQL injection, secret handling, or dangerous GitHub Actions patterns are present.

Two logic-level findings — both independently confirming the Codex review's P1 items — are below. No new findings beyond these two.

---

Finding 1 — Symlink traversal via chmod in --fix mode

Severity: SUGGESTION | Confidence: HIGH

Location: exec-bit-check.sh L229–234

if [[ -e "$path" ]]; then
  chmod +x -- "$path" 2>/dev/null || {
    …
    continue
  }
fi

The detection phase correctly filters staged paths whose index mode is 120000 (symlink) before the blob probe — so a symlink that was staged as a symlink is never an offender. The fix phase does not re-check whether the worktree path is currently a symlink. If a file staged as a new regular file (A, 100644) is later replaced in the worktree by a symlink pointing outside the repository, chmod +x -- "$path" follows the link and makes the target executable. On Linux, chmod follows symlinks by default; there is no --no-dereference that works portably.

Exploitability constraint: The attacker must have write access to the worktree (to plant the symlink) and the target file must be owned by the user running --fix. If the target is not user-owned, chmod exits non-zero, the path is skipped, and the index update is also skipped (so no false-"fixed" state is recorded). Practical exposure is therefore limited to files the invoking user already owns.

Suggested mitigation: Add a [[ -L "$path" ]] && { warn and skip; continue; } guard immediately before the chmod block. The index update (git update-index --chmod=+x) is still valid for a path that the index records as a regular file but the worktree has swapped for a symlink — but skipping both writes and surfacing a warning is the safer choice, matching the rationale already applied at the detection phase.

---

Finding 2 — False-negative when invoked from a repository subdirectory

Severity: SUGGESTION | Confidence: HIGH

Location: exec-bit-check.sh L174, L179 and SKILL.md L16

# L174 — git diff emits repo-root-relative paths
done < <(git diff --cached --name-status -z -- … 2>/dev/null)
…
# L179 — git ls-files interprets $path relative to CWD
stage_line="$(git ls-files --stage -- "$path" 2>/dev/null | head -n 1)"

git diff --cached --name-status outputs paths relative to the repository root regardless of CWD. git ls-files --stage -- "$path" interprets its pathspec relative to the current working directory. When the script is invoked from a subdirectory (without --repo-dir pointing to the repo root), the two coordinate systems diverge: a path like src/tool.sh from git diff is looked up as src/src/tool.sh from within src/, returning empty. Every stage_line check fails, every offender is missed, and the script silently exits 0 with no findings.

Security relevance: The script is the described "deterministic tier" under the prose backstop — the thing that catches what a long session's prose recall misses. A systematic false negative on every invocation from a subdirectory means the safety net silently passes regardless of actual exec-bit state, which is precisely the failure mode the script exists to prevent.

SKILL.md pre-computed probe invokes the script without --repo-dir (L16: bash "${CLAUDE_PLUGIN_ROOT}/…/exec-bit-check.sh" --probe). Claude Code's CWD is typically the project root, so this is low-frequency in practice — but any session started from a subdirectory hits the silent false negative for both the snapshot probe and the step-4 --fix run.

Suggested mitigation: Either (a) change all git ls-files and git update-index calls to use --full-name / --work-tree flags or resolve to the repo root via git rev-parse --show-toplevel before processing paths, or (b) document that the script must be invoked with --repo-dir "$(git rev-parse --show-toplevel)" and update the SKILL.md pre-computed context line accordingly.

---

What was checked and found clean

• Option injection — all git calls use -- before path arguments; chmod +x -- "$path" likewise. No unquoted expansions in command position.
• NUL-delimited path parsing — while IFS= read -r -d '' status with per-field reads for R*/C* is correct; paths with spaces, quotes, or newlines are handled intact.
• Blob SHA extraction — derived from git ls-files --stage output, which produces 40-hex-character SHAs only; cannot produce option-flag prefixes. The [[ -n "$blob_sha" ]] || continue guard handles any empty-output edge case.
• Shebang read from staged blob — git cat-file blob "$blob_sha" reads the index content, not the worktree file; eliminates both quoting hazards and TOCTOU races between the shebang check and a worktree modification.
• --repo-dir scope — git rev-parse --is-inside-work-tree gates all operations; a path that is not a valid git repository exits immediately.
• --fix scope guard — unscoped --fix refuses (exit 2) rather than silently mutating another session's staged entries; tested explicitly in case group 8b.
• Test isolation — temp repos under mktemp -d with trap 'rm -rf "$TEST_TMPDIR"' EXIT; core.filemode=false and commit.gpgsign=false set per-repo, not globally.
• pathspec-commits.md code sample — trap armed before the hide loop; -e ... -o -L ... probes handle dangling symlinks; git ls-files --error-unmatch collision check prevents the hide path from clobbering a tracked absent file.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 26e52499ce

ℹ️ 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 finished @kyle-sexton's task in 2m 52s —— View job

---

PR Review — feat(source-control): harden /commit (updated commits)

• Gather context and understand the request
• Read current state of exec-bit-check.sh
• Read current state of exec-bit-check.test.sh
• Read updated SKILL.md
• Verify fixes for P1/P2 findings from prior reviews
• Check for any new issues introduced
• Synthesize findings

---

The three commits since the initial review (26e5249, 5ad60b6) addressed all P1 and P2 findings. Verification of the fixes below, followed by the three minor items from the first review that remain open.

---

P1 fixes — verified

Finding: silent false-clean when invoked from a subdirectory

Fixed at lines 148–175. The script now:

1. captures repo_prefix via git rev-parse --show-prefix (trailing slash included, so prepending is exact)
2. re-anchors any caller-supplied relative pathspecs from paths[] before the directory change
3. cds to git rev-parse --show-toplevel

The re-anchoring logic correctly leaves absolute paths (/*) and magic pathspecs (:*) alone. Case group 10 (lines 290–319 of the test) exercises the exact scenario: --list and --probe from sub/, plus a --fix with a CWD-relative pathspec (nested/deep.sh) that is re-anchored to sub/nested/deep.sh. ✓

Finding: chmod +x could follow a worktree symlink outside the repository

Fixed at lines 290–309. The [[ -L "$path" ]] test is ordered before [[ -e "$path" ]] (load-bearing, as the comment notes — -e follows links). A worktree symlink over a staged regular file fails the entire path with exit 4 and skips both writes. Case group 11 (lines 321–351) confirms exit 4, the error message, and that the symlink target is not made executable. ✓

Finding: Codex P2 — newline in path breaks --list one-record-per-line contract

Addressed two ways: --list now uses %q for any path containing a newline (lines 244–249), making the ambiguity visible rather than silent; --list0 (NUL-delimited) added as the fully unambiguous machine-readable form (lines 252–258). Case group 12 (lines 353–367) counts NUL terminators with tr -dc '\0' | wc -c. ✓

Codex P1: SKILL.md config probes not resolving from repo root

Not present in the current code. Lines 18–19 of SKILL.md already compute R="$(git rev-parse --show-toplevel ...)" and reference $R/.claude/... for both the tracked-team and personal-overlay probes. Lines 29–36 document this explicitly. ✓

---

Three minor items still open (same as prior review — none blocking)

1 — Header comment still overstates "always attempted"

exec-bit-check.sh lines 35–37 says:

Both writes are therefore always attempted: the chmod for the file's own sake…

There are now two early exits before git update-index: the symlink refusal (continue at line 300) and a chmod failure (continue at line 306). Neither reaches the index write. The comment is still explaining the motivation for the double-write, but "always attempted" is no longer accurate in these cases. A follow-up wording fix is fine; not blocking.

2 — C* (copy) destinations silently excluded

Lines 201–204 read and discard _src and _dst for copy entries without adding _dst to added. A copy destination is structurally a new file in the index. The "Deliberate scope limits" header block (lines 14–23) doesn't mention this exclusion. The practical impact is low (copies outside of --find-copies-harder are rare in typical commits), but either adding _dst or documenting "Copies skipped" would close the undocumented gap.

3 — Test case 5b doesn't assert the worktree bit under core.filemode=false

Case 5 at line 155 uses [[ -x "$repo5/fixme.sh" ]] with a skip_case fallback to verify the worktree bit was set. Case 5b at lines 191–193 only asserts the index mode. On Linux, core.filemode=false tells git to ignore the worktree bit — it doesn't prevent chmod from setting it — so the same [[ -x ... ]] + skip_case pattern from case 5 could be added here to fully cover both writes in the regression fixture.

---

Overall

All P1 and P2 blocking findings are resolved. The implementation is solid: the repo-root anchor with pathspec re-anchoring is correct, the symlink guard ordering is correct and well-commented, and the new test groups (10, 11, 12) cover the added code precisely. The SKILL.md probe documentation at lines 29–36 is a clean explicit statement of the invariant. The three remaining items are the same informational notes from the first review — none are blocking.

[claude]

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

---

Security Review — PR #1590 feat(source-control): harden /commit

• Gather context and understand the request
• Read current state of changed files (HEAD SHA 5ad60b6)
• Verify fixes for previously-reported P1 findings (repo-root anchoring, symlink traversal)
• Analyze exec-bit-check.sh for residual or new security issues
• Analyze exec-bit-check.test.sh for security issues
• Analyze reference markdown code samples for embedded security issues
• Synthesize and post findings

---

Both P1 findings reported in prior reviews are fixed at the current HEAD. One residual issue — the manual-fallback code sample in a reference file — carries the same class of defect the script was just hardened against.

---

P1 fixes verified

Repo-root anchoring — FIXED

exec-bit-check.sh lines 160–175

The script now captures --show-prefix before any cd, prepends it to caller pathspecs that are not already absolute or magic-prefixed, then cds to --show-toplevel. The --show-prefix approach is correct and portable — it uses git's own view of the prefix rather than realpath, which requires the file to exist and behaves differently across platforms. Test case group 10 (lines 297–319 of the test file) exercises this: runs the script from a subdirectory without --repo-dir and asserts both root-level and nested offenders are reported, and that a cwd-relative pathspec passed to --fix is re-anchored and fixes the right entry without touching out-of-scope paths. The SKILL.md pre-computed config-layer probes are also fixed: both use R="$(git rev-parse --show-toplevel)" before any file test (SKILL.md lines 18–19).

Symlink traversal via chmod in --fix mode — FIXED

exec-bit-check.sh lines 297–309

[[ -L "$path" ]] is now tested before [[ -e "$path" ]]. On a worktree/index disagreement (path staged as a regular 100644 blob but the worktree entry is a symlink), the script refuses the path loudly, sets failed=1, skips both the chmod and the git update-index, and exits 4. The comment at lines 290–296 explicitly documents the ordering constraint and why it is load-bearing. Test case group 11 pins the regression: it stages a regular file, replaces the worktree entry with a symlink pointing to outside/victim.txt, asserts --fix exits 4 and refuses with the expected message, and asserts the symlink's target was not made executable.

---

Finding — Manual-fallback snippet missing the -L guard

Severity: SUGGESTION | Confidence: HIGH

reference/exec-bit.md lines 113–119

for f in <newly-added paths>; do
  mode=$(git ls-files --stage -- "$f" | cut -d' ' -f1)
  case "$mode" in 100644) ;; *) continue ;; esac  # skip 100755, symlinks (120000), gitlinks
  git cat-file blob "$(git ls-files --stage -- "$f" | cut -d' ' -f2)" | head -c 2 | grep -q '^#!' || continue
  chmod +x -- "$f"
  git update-index --chmod=+x -- "$f"
done

The case "$mode" guard correctly skips paths whose staged mode is 120000, which is the form git uses when the path was originally added as a symlink. It does not guard against the case the script was just hardened against: a path staged as a regular 100644 blob whose worktree entry is subsequently replaced by a symlink. In that state, mode is still 100644, the blob still starts with #!, and chmod +x -- "$f" follows the link — potentially making a file outside the repository executable.

This is documentation guidance rather than auto-executed code, so a user must actively reach for the fallback and a worktree symlink must be present for the path to be live. But the fallback is framed as the equivalent inline form of the script ("exec-bit-check.sh is the mechanic; this file explains why — and what to do when it cannot run") and teaches the same pattern the script was just fixed to prevent. Adding a [[ -L "$f" ]] && { echo "skipping symlink: $f" >&2; continue; } guard before chmod +x would keep the sample consistent with the script.

Fix this →

---

What was checked and found clean

• Option injection — all git invocations use -- before path arguments; chmod +x -- "$path" likewise. No unquoted expansions in command position.
• NUL-delimited path parsing — the while IFS= read -r -d '' status loop with per-field reads for R*/C* is correct throughout; paths with spaces, newlines, and quotes are handled intact.
• Blob SHA from controlled output — blob_sha is derived from git ls-files --stage output, which produces 40-char hex SHAs only. Cannot carry option-flag prefixes. The [[ -n "$blob_sha" ]] || continue guard handles any empty-output edge case.
• Shebang read from staged blob — git cat-file blob "$blob_sha" reads the index content, not the worktree file, eliminating both path-quoting hazards and TOCTOU races between the shebang check and a worktree modification.
• --fix scope refusal — unscoped --fix exits 2 and mutates nothing; test case 8b pins this. The --all explicit opt-in is documented and tested.
• --show-prefix re-anchoring — empty-prefix case is guarded ([[ -n "$repo_prefix" ]]); absolute paths and :*-prefixed magic pathspecs are left untouched.
• Test isolation — temp repos under mktemp -d with trap 'rm -rf "$TEST_TMPDIR"' EXIT; core.filemode=false and commit.gpgsign=false set per-repo, not globally; source path "$SCRIPT_DIR/../../../scripts/test-helpers.sh" resolves within the plugin tree.
• pathspec-commits.md code sample — trap armed before the hide loop; -e ... -o -L ... probes handle dangling symlinks; git ls-files --error-unmatch collision check guards against clobbering a tracked file.
• SKILL.md pre-computed probe — bash "${CLAUDE_PLUGIN_ROOT}/…/exec-bit-check.sh" --probe double-quotes the expansion; CLAUDE_PLUGIN_ROOT is set by the trusted plugin runtime, not user input.

One finding — the manual-fallback snippet missing its -L guard. All other surfaces are clean.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ad60b668c

ℹ️ 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 0s —— 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/30216316926
• Failure class: rate-limit
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":1758,"total_cost_usd":0,"api_error_status":429,"class":"rate-limit"}

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/30216316928
• Failure class: rate-limit
• Last SDK result: {"subtype":"success","is_error":true,"num_turns":1,"duration_ms":352,"total_cost_usd":0,"api_error_status":429,"class":"rate-limit"}

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: 35131b7592

ℹ️ 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]

Include copy destinations in the exec-bit scan

When copy detection is enabled and a tracked source is modified in the same staged change, git diff --cached --name-status can report the new path as C100 <source> <destination> rather than A; git diff -h describes -C as “detect copies.” This branch consumes and discards the destination, so a newly copied shebang file staged as 100644 is never reported or fixed and can be committed non-executable. Treat the C* destination as an added candidate.

Useful? React with 👍 / 👎.

[kyle-sexton]

Only half discharged. Leaving this thread OPEN, tracked at #2118.

PR #2098 widened the candidate set to R*/C* destinations but gated both on the source being 100755. Your ask was that a C* destination be treated as an added candidate; only the exec-source half of that shipped.

A fresh-context reviewer, briefed to refute rather than confirm the discharge, caught this attacking my own work. I reproduced it independently against origin/main before accepting it:

########## NON-exec source (the defect)
  diff.renames=false   raw=:000000 100644 … A    copy.sh          --list -> [copy.sh]
  diff.renames=copies  raw=:100644 100644 … C095 lib.sh copy.sh   --list -> []

########## exec source (control, proves the fixture discriminates)
  diff.renames=false   raw=:000000 100644 … A    copy.sh          --list -> [copy.sh]
  diff.renames=copies  raw=:100755 100644 … C095 lib.sh copy.sh   --list -> [copy.sh]

Identical staged content, two different answers depending on the consumer's diff.renames setting — which is the precise failure mode #2098's own body says the fix exists to remove.

The reason it slipped: R* and C* share one predicate and one justification, and the justification only fits one of them. The script comment reads "a deliberately non-executable tracked file merely being moved… squarely outside this check's newly-added-only scope." True for a rename — same tracked file, new path. False for a copy — the destination did not previously exist, so it is newly added and squarely inside scope.

Worse, #2098 added a test that pins the defect. Case group 21 asserts "a copy whose SOURCE was never executable is NOT reported." I wrote that case in response to a reviewer asking for a copy-branch negative twin of the rename negative, and implemented it without noticing the two branches are not symmetric here. So the gap is now locked in by a passing test — the shape most likely to read as intended behaviour to whoever looks next.

#2118 carries the proposed fix (split the arms: R* keeps the source-mode gate, C* drops it), the required test changes including inverting case group 21, and a new case asserting that the two diff.renames configurations agree — the property this finding is actually about, which nothing currently pins.