fix(guardrails): resolve source-control enablement across user-global scope

[kyle-sexton]

f2 (audit #912). The flag-commit-pr-skill-bypass advisory's source_control_enabled probe read only the consuming project's .claude/settings.json (+ local override), so when source-control is enabled solely at user-global scope (~/.claude/settings.json) — a common install — the probe false-negatived and the gh pr create advisory never fired.

Fix

• Resolve effective enablement across user-global, project, and local scopes in Claude Code's precedence order (user-global base -> project overrides -> local overrides), matching how the platform merges enabledPlugins.
• Extract a small read_sc_enabled helper (dedups the jq read).
• Make the contract test hermetic w.r.t. HOME (a clean HOME by default, so no CI-runner ~/.claude leaks in) and add four user-global cases, including the exact false-negative this fixes.

Verification

• bash plugins/guardrails/hooks/flag-commit-pr-skill-bypass.test.sh -> 21/21 pass
• shellcheck --rcfile=.shellcheckrc clean
• guardrails 0.9.5 -> 0.9.6 + CHANGELOG entry

Related

• audit: guardrails + source-control hardening — bypass gaps, convention-enforcement SSOT, decoupling #912 -- umbrella tracking issue (not closed here)
• Independent of the feat(source-control): convention parse-contract + shared enforcement resolver seam #913 seam (no shared files)

Closes #916

[claude]

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

---

PR Review

• Gather context
• Read changed files
• Review hook logic
• Review test coverage
• Post findings

---

Summary

Clean, well-scoped fix. The three-scope precedence model in source_control_enabled() is correct and the test suite is solid. Two minor observations below — neither is a blocker.

---

flag-commit-pr-skill-bypass.sh — logic

Precedence chain (lines 132–144):

effective="$user_val"
[[ -n "$base_val" ]] && effective="$base_val"
[[ -n "$local_val" ]] && effective="$local_val"

Correct — last explicit value wins, which matches Claude Code's merge semantics (user-global base → project override → local override). An unset key at a given scope correctly passes through to the next layer.

read_sc_enabled() helper (lines 111–119):

read_sc_enabled() {
  local file="$1"
  [[ -f "$file" ]] || return 0
  jq -r '
    (.enabledPlugins // {}) | to_entries[]
    | select(.key | startswith("source-control@"))
    | .value
  ' "$file" 2>/dev/null | head -1
}

• return 0 on absent file is correct for a command-substitution context — only stdout matters, and the caller only checks [[ -n "$result" ]].
• startswith("source-control@") (not an exact match) is appropriate — picks up any source-control marketplace variant, not just melodic-software.
• head -1 is a safe guard for the multi-entry edge case.
• The SC2329 suppression is accurate — shellcheck doesn't trace command-substitution call sites.

HOME guard (line 133):

[[ -n "${HOME:-}" ]] && user_val=$(read_sc_enabled "$user_settings")

Correct — user_settings="${HOME:-}/.claude/settings.json" would expand to "/.claude/settings.json" when HOME is unset, so the guard prevents a spurious root-relative path being probed.

---

flag-commit-pr-skill-bypass.test.sh — test coverage

Hermetic HOME design (lines 60–66):

HERMETIC_HOME (a temp dir with no .claude) as the run_hook default is the right approach. The env ... HOME="$HERMETIC_HOME" "$@" bash ... pattern works because env applies assignments left-to-right and the last value for a given key wins — so passing HOME="$HOME_ENABLED" as a trailing arg correctly overrides the default. Unconventional but valid.

Four user-global cases (lines 110–128): the exact false-negative, the "key absent at project" variant, and both cross-scope override directions are all covered. Good.

Observation 1 — telemetry test does not use HERMETIC_HOME

Lines 141–151 call the hook directly with env HOOK_TELEMETRY_SINK=... CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" but do not set HOME. If a CI runner has ~/.claude/settings.json with source-control explicitly disabled at user-global, and the hook now reads that before reading the project file, user_val would be "false" — but then base_val from ENABLED_PROJECT is "true", so effective ends up "true" and the advisory fires anyway. No functional problem in practice, but the test is not hermetic on this axis. Low-risk, but worth aligning with the rest of the suite.

Fix this →

Observation 2 — make_home() has no "home dir exists, no settings file" variant

All make_home calls write a settings.json (it always has an explicit value). The case of $HOME/.claude/ existing but containing no settings.json is handled implicitly by HERMETIC_HOME, but there's no explicit test asserting that a user-global dir-with-no-file leaves user_val empty and falls through to project scope. Not a gap in coverage for the reported bug, just a future-hardening note.

---

Other files

• plugin.json — version 0.9.5 → 0.9.6 is correct for a bug fix.
• CHANGELOG.md — entry is accurate, clearly describes the root cause, the fix, and the precedence model. Well written.

---

Verdict: approve. The fix is correct, the tests cover the exact failure mode and the key edge cases, and there are no blocking issues. The telemetry hermetic note (observation 1) is worth a follow-up but does not affect the correctness of the reported fix.

[claude]

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

---

Security Review

• Gather context and understand PR scope
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md — only the guardrails hook and its test.

---

Findings

Overall: no significant security vulnerabilities found in this PR.

The core change adds a user-global scope (~/.claude/settings.json) read to the existing settings-resolution chain. All new reads are:

• Read-only — jq opens files for parsing only; no write surface is introduced.
• Advisory-only — the hook exits 0 unconditionally; a wrong result can only make the advisory fire or stay silent, never block or allow a tool call.
• Input is trusted — HOME and CLAUDE_PROJECT_DIR come from the Claude Code runtime environment, not from the Bash command being inspected.

---

SUGGESTION · Confidence: low · flag-commit-pr-skill-bypass.sh

read_sc_enabled, line 114: the jq invocation passes the file path as a bare positional argument without a -- separator.

jq -r '...' "$file" 2>/dev/null | head -1

If $file ever begins with - (e.g. a pathological HOME=-foo), jq would interpret it as an option flag rather than a file name. In practice this cannot happen: user_settings is always ${HOME}/.claude/settings.json, and HOME provided by Claude Code runtime will start with / (POSIX) or a drive letter (Windows). Impact if it did trigger: the advisory silently does not fire (jq exits non-zero, 2>/dev/null suppresses the error, output is empty → effective stays empty → exit 0 with no message). Non-blocking, so security impact is nil. Hardening would be jq -r '...' -- "$file".

---

No further findings. The [[ -n "${HOME:-}" ]] guard before reading user_settings is correct. The scope-precedence chain (user_val → base_val overrides → local_val overrides) correctly matches Claude Code's merge order. The test's hermetic HOME override (HOME="$HERMETIC_HOME" as the default, per-case override via run_hook's trailing "$@") prevents CI runner leakage. No new execution surfaces, no new trusted-data boundaries, no secret handling.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1dfdb909c3

ℹ️ 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 0s —— View job

---

• Gather context and read changed files
• Review hook logic (precedence, helper, HOME guard)
• Review test coverage and hermetic design
• Assess Codex P2 finding (local-override prerequisite semantics)
• Review CLAUDE.md alignment
• Post findings

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

The core fix is correct and the new user-global test cases cover the exact false-negative. Three items worth addressing before merge — one is a genuine correctness gap (the Codex P2 finding), two are minor.

---

Finding 1 — Codex P2 is valid: local override gate lost its prerequisite

Severity: correctness

Before this PR the precedence chain was:

[[ -n "$base_val" ]] || return 1   # required a project key; local was meaningless without it
[[ -n "$local_val" ]] && effective="$local_val"

After this PR (line 138–139):

[[ -n "$base_val" ]] && effective="$base_val"
[[ -n "$local_val" ]] && effective="$local_val"   # applied even when base_val is empty

The README documents the platform's constraint at lines 127–129:

.claude/settings.local.json as an override, only for a key already present in settings.json — CC ignores a local-only key per anthropics/claude-code#27247

So local_val should only take effect when base_val (the project key) is present.

Failing scenario:

• ~/.claude/settings.json → source-control@melodic-software: true
• project settings.json → no key
• settings.local.json → source-control@melodic-software: false

CC effective value: true (local-only key ignored). Hook effective value: false (advisory suppressed). Advisory incorrectly silent.

The inverse also fails: user-global false, no project key, local true → CC ignores local, source-control is off, but hook fires the advisory.

Fix:

effective="$user_val"
if [[ -n "$base_val" ]]; then
  effective="$base_val"
  [[ -n "$local_val" ]] && effective="$local_val"
fi

A test to add alongside the fix (the missing case from the new suite):

# user-global enabled; no project key; local explicitly disables — hook must fire
# because CC ignores local-only keys (no project key → local is a no-op at platform level)
out=$(run_hook "$(command_json 'gh pr create --title x --body y')" "$NO_KEY_PROJECT" \
  HOME="$HOME_ENABLED" \
  __local_override_file="$NO_KEY_PROJECT/.claude/settings.local.json")
# after writing false to that local file:
assert_contains "local false without project key does not suppress user-global true" "$out" "gh pr create"

Fix this →

---

Finding 2 — README describes the old two-scope behavior

Severity: minor

plugins/guardrails/README.md lines 125–132 still say the hook "reads enabledPlugins from the consuming project's own .claude/settings.json". With this PR it now reads three scopes. The sentence also references the local-key prerequisite from #27247 — if Finding 1 is fixed by gating local on base_val, that reference stays accurate; if not, it becomes actively misleading.

Fix this →

---

Finding 3 — Telemetry test still not hermetic on HOME (carry-over from prior review)

Severity: minor

Lines 142–143 of the test do not pass HOME:

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

ENABLED_PROJECT has base_val=true, so a CI runner's user-global false would still be overridden and the test would pass. But it's the only test block that doesn't inherit HERMETIC_HOME, which is inconsistent with the suite's design intent. One-line fix: add HOME="$HERMETIC_HOME" to the env invocation on line 142.

Fix this →

---

What's correct

• Precedence model (user-global → project → local): correctly matches CC's merge order; the false-negative this PR fixes is real and the fix is sound.
• read_sc_enabled() helper: startswith("source-control@"), head -1, return 0 on absent file, and the SC2329 suppression are all correct.
• HOME guard ([[ -n "${HOME:-}" ]] on line 133): prevents a spurious root-relative path probe when HOME is unset.
• Hermetic HOME design: HERMETIC_HOME as the run_hook default and per-case override via trailing "$@" env args is unconventional but valid — env applies left-to-right and the last value wins.
• Four user-global test cases: cover the exact false-negative, the "key absent at project" variant, and both cross-scope override directions.
• Version bump (0.9.5 → 0.9.6) and CHANGELOG entry are correct and well-written.

---

Finding 1 is the blocker — it introduces a correctness regression relative to the documented CC local-key prerequisite. Findings 2 and 3 are clean-up items. Happy to implement the fix for Finding 1 if useful.

[claude]

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

---

Security Review

• Gather context
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md — only the guardrails hook and its test.

---

Findings

No new security vulnerabilities found in this PR.

The core change introduces a read_sc_enabled() helper and extends source_control_enabled() to probe ~/.claude/settings.json. All new operations are:

• Read-only. jq opens files for parsing only; no write surface is introduced.
• Advisory-only. The hook exits 0 unconditionally; a wrong result can only cause the advisory to fire or stay silent, never block or allow a tool call.
• Inputs are trusted. HOME and CLAUDE_PROJECT_DIR come from the Claude Code runtime environment, not from the Bash command being inspected.

---

SUGGESTION · Confidence: low · read_sc_enabled, line 118

The jq invocation at each of the three call sites passes the file path without a -- end-of-options separator:

jq -r '...' "$file" 2>/dev/null | head -1

If $file begins with -, jq treats it as an option flag. In practice this cannot happen — user_settings expands to ${HOME}/.claude/settings.json (POSIX: starts with /; Windows: drive letter), and settings / local_settings derive from hook::repo_root, which yields an absolute path. If it did trigger: jq exits non-zero, 2>/dev/null suppresses the error, output is empty, effective stays empty, hook exits 0 with no advisory — advisory-only impact, not a security regression. Hardening: jq -r '...' -- "$file".

Note: the prior automated security review on this PR already flagged this identical issue.

---

No further findings. The [[ -n "${HOME:-}" ]] guard before constructing user_settings is correct. The scope-precedence chain (user_val → base_val → local_val) cannot be exploited to inject data through the narrow startswith("source-control@") key filter — the only output consumed is the boolean string "true" / "false". The test's hermetic HERMETIC_HOME correctly prevents CI-runner ~/.claude from leaking into the non-telemetry cases. No new execution surfaces, no credential handling, no egress introduced.

[claude]

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

---

Security Review

• Gather context
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md — only the guardrails hook and its test.

---

Findings

No new security vulnerabilities found in this PR.

The core change extends source_control_enabled() to probe ~/.claude/settings.json (user-global) before the project and local scopes. Security properties of all new operations:

• Read-only. jq opens each file for parsing only; no write surface is introduced.
• Advisory-only output. The hook exits 0 unconditionally; a wrong result can only make the advisory fire or stay silent, never block a tool call or alter execution.
• Narrow jq extraction. The filter selects keys matching startswith("source-control@") and extracts only .value — the boolean "true" / "false" string. No other file content reaches the hook's logic or the telemetry sink.
• No eval / no execution. The extracted string is compared with [[ "$effective" == "true" ]] only; it is never passed to eval, a subshell, or any execution primitive.
• Inputs are trusted. HOME and CLAUDE_PROJECT_DIR come from the Claude Code runtime environment, not from the Bash command text being inspected.

---

SUGGESTION · Confidence: low · read_sc_enabled, line 118

The jq invocation passes the file path without a -- end-of-options separator:

jq -r '...' "$file" 2>/dev/null | head -1

If $file ever starts with -, jq treats it as a flag rather than a file name. In practice this cannot happen: user_settings expands to ${HOME}/.claude/settings.json (POSIX: absolute path starting with /), and settings/local_settings derive from hook::repo_root which also yields an absolute path. If it somehow did trigger, the impact is advisory-only: jq exits non-zero, 2>/dev/null suppresses it, output is empty, effective stays empty, and the hook exits 0 without emitting the advisory. No security regression. Hardening fix: jq -r '...' -- "$file".

Note: both prior automated security reviews on this PR flagged this same issue. No new surface has been added in this revision.

---

No further findings. The [[ -n "${HOME:-}" ]] guard on line 133 correctly prevents probing /.claude/settings.json when HOME is unset. The precedence chain (user_val → base_val overrides → local_val conditional on base_val) is correct and cannot be influenced by content outside the source-control@* key. The test's HERMETIC_HOME design correctly isolates most cases from CI-runner ~/.claude leakage. No new execution surfaces, no credential handling, no egress introduced.

[claude]

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

---

• Gather context and read current file state
• Compare against prior review findings
• Post complete review

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

Reviewing at HEAD ab813b7. The prior Finding 1 blocker (local-override prerequisite) has been addressed in this latest push. Two minor items remain.

---

Finding 1 (BLOCKER from prior review) — FIXED ✓

flag-commit-pr-skill-bypass.sh lines 137–143

The prior review flagged that local values were applied even when base_val was empty — violating CC's documented behavior that local-only keys are ignored when no project key exists. The current code correctly gates on both:

effective="$user_val"
[[ -n "$base_val" ]] && effective="$base_val"
[[ -n "$base_val" && -n "$local_val" ]] && effective="$local_val"

The comment on line 139–142 is precise and references the README constraint. Both logical directions (local-false-without-project-key must not suppress user-global; local-true-without-project-key must not override user-global-disabled) are exercised by tests at lines 131–138.

---

Finding 2 — README still describes old two-scope behavior (minor)

plugins/guardrails/README.md line 19 (table summary):

"Only fires when the consuming project's own .claude/settings.json enables the source-control plugin"

Lines 125–132 (prose):

"reads enabledPlugins from the consuming project's own .claude/settings.json"

Both still describe the old two-scope behavior (project + local only). With this PR the hook now reads three scopes (user-global → project → local). Neither line is in the changed files.

Fix this →

---

Finding 3 — Telemetry test not hermetic on HOME (minor, carry-over)

flag-commit-pr-skill-bypass.test.sh lines 152–153:

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

This is the only test block that calls env directly without going through run_hook, so it doesn't get the HOME="$HERMETIC_HOME" default that run_hook injects. ENABLED_PROJECT has base_val=true, so a CI runner with user-global false would still produce effective=true and the test would pass — no functional impact today. But it's inconsistent with the suite's hermetic design intent.

One-line fix: add HOME="$HERMETIC_HOME" to the env invocation on line 152.

Fix this →

---

What's correct

• Precedence chain (lines 137–143): user_val → base_val overrides → local_val conditional on base_val correctly matches CC's merge semantics including the local-key prerequisite constraint.
• read_sc_enabled() helper: startswith("source-control@"), head -1, return 0 on absent file, SC2329 suppression — all correct.
• HOME guard (line 133): prevents probing a root-relative path when HOME is unset.
• HERMETIC_HOME default in run_hook (line 65): correctly prevents CI runner ~/.claude from leaking into any test that doesn't explicitly set HOME.
• Two new local-prerequisite tests (lines 131–138): directly cover the inverse failures the Finding 1 fix closes.
• Four user-global test cases (lines 115–128): cover the exact false-negative and both cross-scope override directions.
• plugin.json version bump (0.9.5 → 0.9.6) and CHANGELOG entry are correct and well written.

---

Verdict: The correctness blocker (Finding 1) is resolved and covered by tests. Findings 2 and 3 are clean-up items (README documentation and a single test line), neither affects the reported fix. Happy to implement either if useful.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ab813b7d65

ℹ️ 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 54s —— View job

---

• Gather context and read current files at HEAD
• Assess prior findings (Finding 2 README, Finding 3 telemetry hermetic)
• Evaluate Codex P2 finding (CLAUDE_CONFIG_DIR)
• Post complete review

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

Reviewing at HEAD 8640d60. The new commit addresses the Codex P2 finding (CLAUDE_CONFIG_DIR). Two minor carry-overs from the prior review remain open.

---

Codex P2 Finding — FIXED ✓

flag-commit-pr-skill-bypass.sh lines 124–127

user_settings="${CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/settings.json}"
[[ -n "$user_settings" ]] || user_settings="${HOME:+$HOME/.claude/settings.json}"

Correct. When CLAUDE_CONFIG_DIR is set, config lives at $CLAUDE_CONFIG_DIR/settings.json (no .claude/ subdir) — the make_config_dir() helper in the test (line 64–69) writes it flat to verify this. The ${VAR:+...} idiom is the right tool: it expands only when the variable is set and non-empty, so an empty CLAUDE_CONFIG_DIR correctly falls through to the HOME branch. The [[ -n "$user_settings" ]] && guard on line 136 then prevents probing an empty path.

Test coverage (lines 63–69, 74–78, 152–155): make_config_dir() creates the flat layout, run_hook now unsets CLAUDE_CONFIG_DIR via env -u CLAUDE_CONFIG_DIR so no CI-runner value leaks in, and the new test case explicitly passes CLAUDE_CONFIG_DIR="$CFG_ENABLED" as a trailing arg (which env applies after the -u, correctly winning). The comment on line 74–76 explains the override mechanic accurately.

---

Finding 2 — README still describes old two-scope behavior (minor, carry-over)

plugins/guardrails/README.md line 19 (table row):

"Only fires when the consuming project's own .claude/settings.json enables the source-control plugin — silent otherwise."

Lines 125–132 (prose):

"flag-commit-pr-skill-bypass reads enabledPlugins from the consuming project's own .claude/settings.json"

Both still describe the old two-scope behavior. The hook now probes three scopes (user-global via CLAUDE_CONFIG_DIR or ~/.claude/ → project → local), but the README hasn't caught up. The CHANGELOG and hook header comment are accurate — only the user-facing README is stale.

Fix this →

---

Finding 3 — Telemetry test not hermetic on HOME or CLAUDE_CONFIG_DIR (minor, carry-over + new axis)

flag-commit-pr-skill-bypass.test.sh lines 169–170:

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

This is the only test block that calls env directly instead of going through run_hook. As a result it misses both hermetic guards that run_hook now provides — HOME="$HERMETIC_HOME" and -u CLAUDE_CONFIG_DIR. Now that CLAUDE_CONFIG_DIR is a live code path, a CI runner with that variable set could direct the probe to an unrelated config dir. Still no functional breakage today: ENABLED_PROJECT has base_val=true, which overrides any user-global false at any scope. But the asymmetry with the rest of the suite is more notable than it was in the prior review.

Fix: update line 169 to match the run_hook pattern:

env -u CLAUDE_CONFIG_DIR HOOK_TELEMETRY_SINK="$SINK" HOME="$HERMETIC_HOME" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

Fix this →

---

What's correct in this commit

• CLAUDE_CONFIG_DIR path resolution (lines 124–127): ${CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/settings.json} → ${HOME:+$HOME/.claude/settings.json} two-step fallback is correct and covers both the relocated-config and standard-install cases.
• make_config_dir() helper (lines 63–69): writes settings.json flat (no .claude/ subdir), matching the CLAUDE_CONFIG_DIR layout.
• -u CLAUDE_CONFIG_DIR in run_hook (line 77): prevents CI-runner leakage for all tests that go through run_hook; the comment on lines 74–76 explains the override mechanic clearly.
• New CLAUDE_CONFIG_DIR test (lines 152–155): covers the exact relocated-config false-negative.
• Precedence chain (lines 140–146): user_val → base_val overrides → local_val conditional on base_val — correct, and the [[ -n "$base_val" && -n "$local_val" ]] gate for local correctly honors the documented platform constraint.
• Version bump (0.9.6) and CHANGELOG are correct. The hook header comment (lines 8–16) and the source_control_enabled() inline comments (lines 131–145) accurately describe the three-scope model.

---

Verdict: The CLAUDE_CONFIG_DIR fix is correct and well-tested. Findings 2 and 3 are clean-up items that don't affect the fix's correctness. Happy to implement either.

[claude]

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

---

Security Review

• Gather context
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md — only the guardrails hook and its test.

---

Findings

No new security vulnerabilities introduced by this PR.

The core change adds CLAUDE_CONFIG_DIR and user-global ~/.claude/settings.json reads to the scope-resolution chain. Security properties of all new operations:

• Read-only. jq opens files for parsing only. No write surface is introduced.
• Advisory-only. The hook exits 0 unconditionally; a wrong result can only make the advisory fire or stay silent, never block a tool call or alter execution.
• No data from inspected commands reaches the new code path. CLAUDE_CONFIG_DIR and HOME are provided by the Claude Code runtime environment, not derived from the Bash command text being inspected.
• No eval / no execution. The extracted value ("true" / "false") is compared with [[ "$effective" == "true" ]] only; it is never passed to eval, a subshell, or any execution primitive.
• No new trusted-data boundaries. The jq filter is a hardcoded string literal — no user-controlled data is interpolated into the filter text.

---

SUGGESTION · Confidence: low · read_sc_enabled, line 118

jq -r '...' "$file" 2>/dev/null | head -1

The jq invocation passes the file path without a -- end-of-options separator. If $file begins with - (e.g., if CLAUDE_CONFIG_DIR were set to -foo), jq would interpret it as an option flag. In practice this cannot happen: user_settings expands from CLAUDE_CONFIG_DIR/settings.json or HOME/.claude/settings.json, both of which are absolute paths under normal Claude Code operation. If it did trigger: jq exits non-zero, 2>/dev/null suppresses the error, effective stays empty, hook exits 0 without emitting the advisory — advisory-only impact, not a security regression. Hardening: jq -r '...' -- "$file".

Note: this identical issue was flagged by both prior automated security reviews on this PR. No new surface has been added in this revision.

---

No further findings. The CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/settings.json expansion (line 126) is a Bash parameter expansion used as a filename only — never passed to eval or any shell execution. The [[ -n "${HOME:-}" ]]-equivalent guard for user settings ([[ -n "$user_settings" ]] on line 136) correctly prevents a probe when both CLAUDE_CONFIG_DIR and HOME are unset. The env -u CLAUDE_CONFIG_DIR addition to run_hook (line 77) is a positive hardening — it prevents a leaked CLAUDE_CONFIG_DIR from the CI runner environment from influencing test fixture paths. No new execution surfaces, no credential handling, no data egress introduced.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8640d602fa

ℹ️ 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".

[kyle-sexton]

Automated babysit-lane classification (safe tier — reply only; not resolving threads/merging). Re: the codex P2 finding ("Honor the project-key prerequisite for local overrides").

#	Finding	Classification	Evidence
1	chatgpt-codex-connector P2 — honor project-key prerequisite for local overrides	Fixed (current head 8640d602)	flag-commit-pr-skill-bypass.sh line 139 now gates the local override on the project settings.json key already being established ([[ -n "$base_val" && -n "$local_val" ]] && effective="$local_val"), matching the reviewer's literal remediation. Verified against anthropics/claude-code#27247's Case 2: that issue's reproductions are user-global-scope only and don't establish the cross-scope project-local case, so this project-only gate is the defensible, verifiable fix — a broader base_val || user_val gate would assert unverified merge semantics against this hook's fail-quiet design. Not broadening further.

claude[bot] Security-Review's one SUGGESTION (jq ---separator hardening) and PR-Review's 2 non-blocking observations are advisory-only; leaving open, no action required to unblock.

[claude]

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

---

Security Review

• Gather context and read PR diff
• Read changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md.

---

Findings

No new security vulnerabilities introduced by this PR.

The refactored source_control_enabled() extends the previous two-scope read to three scopes and adds multi-key support. Security properties of all new operations:

• Read-only. jq opens files for parsing only — sc_keys() and sc_key_value() both open files in read mode. No write surface is introduced.
• Advisory-only. The hook exits 0 unconditionally; a wrong result can only make the advisory fire or stay silent, never block or alter execution.
• No user-command data reaches the new code path. CLAUDE_CONFIG_DIR, HOME, and CLAUDE_PROJECT_DIR come from the Claude Code runtime environment, not from the Bash command text being inspected.
• Key values are safe from injection. The per-key lookup in sc_key_value passes the key via jq --arg k "$key", which treats the value as a literal string — no jq code injection is possible regardless of key content in the settings files.
• No new eval or execution surfaces. The extracted effective value ("true"/"false") is compared via [[ "$effective" == "true" ]] only — never passed to eval, a subshell, or any execution primitive.

---

SUGGESTION · Confidence: low · sc_keys line 112 and sc_key_value line 120

Both jq invocations pass the file path as a positional argument without a -- end-of-options separator:

jq -r '...' "$file" 2>/dev/null        # sc_keys
jq -r --arg k "$key" '...' "$file" 2>/dev/null | head -1  # sc_key_value

If $file began with -, jq would interpret it as an option flag. In practice this cannot happen: user_settings expands from $CLAUDE_CONFIG_DIR/settings.json or $HOME/.claude/settings.json, both absolute paths under normal Claude Code operation. The [[ -f "$file" ]] || return 0 guard in both functions additionally requires the path to name a regular file before jq is invoked, which a dash-prefixed path typically would not satisfy. If it somehow triggered: jq exits non-zero, 2>/dev/null suppresses the error, output is empty, effective stays empty, hook exits 0 without emitting the advisory — advisory-only impact, not a security regression. Hardening: jq -r '...' -- "$file" in both helpers.

This issue was flagged by all three prior automated security reviews on this PR. No new surface has been added in the current revision — the finding applies equally to the new sc_keys helper as it did to the original read_sc_enabled helper.

---

No further findings. The CLAUDE_CONFIG_DIR two-step fallback (lines 133–134) is a bash parameter expansion that produces a file path — it is never passed to eval, source, or any execution primitive. The sort -u pipeline over key names from trusted local files introduces no injection surface. The while IFS= read -r key; do <<<"$keys" loop is safe: newlines in jq-extracted key names would split across loop iterations, but those fragments would find no matching entries in the other scopes and the loop would correctly fall through to return 1 (fail-quiet). The env -u CLAUDE_CONFIG_DIR addition to run_hook in the test is a positive hardening step, not a compensating control for a vulnerability.

[claude]

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

---

• Gather context
• Read changed files at HEAD 78b848a
• Assess latest Codex P2 finding (multi-key scenario)
• Post findings

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

Reviewing at HEAD 78b848a. This commit addresses the third and final Codex P2 finding — per-exact-key resolution across scopes. Two minor carry-overs from earlier rounds remain open.

---

Codex P2 (per-exact-key resolution) — FIXED ✓

flag-commit-pr-skill-bypass.sh lines 105–160

The prior approach used to_entries[] | head -1 per scope, which collapsed all source-control@* keys to a single value — if @old=true (user-global) and @new=false (project) were present, whichever key jq enumerated first would win and the wrong result would follow.

The current approach is correct:

• sc_keys() — collects all source-control@* key names from a file (no value, just names)
• sc_key_value() — reads one exact key's value using --arg k "$key" (no jq expression interpolation, the key is properly parameterised)
• source_control_enabled() — unions all key names across scopes, then resolves each key independently using the same user_val → base_val → local_val (gated on base_val) precedence chain, returning 0 if any key resolves enabled

Tracing the P2 scenario (@old=true at user-global, @new=false at project):

key	uval	bval	lval	effective	result
source-control@new	""	"false"	""	"false"	skip
source-control@old	"true"	""	""	"true"	return 0 ✓

Advisory fires because @old is enabled — correct, the skill is available through the old marketplace key.

The new test case at lines 157–166 covers exactly this scenario.

---

Finding 1 — README still describes old two-scope behavior (minor, carry-over)

plugins/guardrails/README.md line 19 (table row):

"Only fires when the consuming project's own .claude/settings.json enables the source-control plugin"

Lines 125–132 (prose):

"flag-commit-pr-skill-bypass reads enabledPlugins from the consuming project's own .claude/settings.json"

Both still describe the old single-scope behavior. The hook now resolves per-exact-key across user-global (CLAUDE_CONFIG_DIR / ~/.claude/), project, and local scopes — none of that is reflected here.

Fix this →

---

Finding 2 — Telemetry test not hermetic on HOME or CLAUDE_CONFIG_DIR (minor, carry-over)

flag-commit-pr-skill-bypass.test.sh lines 180–181:

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

This is the only test block that bypasses run_hook and calls env directly — so it misses both hermetic guards that run_hook now injects (HOME="$HERMETIC_HOME" and -u CLAUDE_CONFIG_DIR). Now that CLAUDE_CONFIG_DIR is a live code path, a CI runner with that variable set could redirect the probe to an unrelated config. ENABLED_PROJECT has base_val=true so no functional failure today, but the asymmetry is more notable than before.

Fix: update line 180 to match run_hook's pattern:

env -u CLAUDE_CONFIG_DIR HOOK_TELEMETRY_SINK="$SINK" HOME="$HERMETIC_HOME" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

Fix this →

---

What's correct in this commit

• Per-exact-key resolution — sc_keys() + sc_key_value() split the problem cleanly: enumerate names first, then resolve each key's value per-scope. The union via sort -u over all three scopes is correct (a key only in local still gets evaluated — correctly resolving to effective="" since bval is empty and the local gate fails, so user-global uval stays or falls back to empty → silent).
• sc_key_value() parameterisation — --arg k "$key" is the right tool: the key is passed as a jq variable, never interpolated into the filter expression. No injection surface even if a key name were adversarially constructed.
• Precedence chain per key — uval → bval overrides → lval gated on bval remains correct and the [[ -n "$bval" && -n "$lval" ]] gate for local correctly honors the documented platform constraint from #27247.
• Multi-key test case (lines 157–166) — covers the exact P2 failure mode and verifies the @old=true result is not suppressed by @new=false.
• All prior fixes confirmed present — CLAUDE_CONFIG_DIR resolution (lines 133–134), run_hook's -u CLAUDE_CONFIG_DIR + HOME="$HERMETIC_HOME" guards (line 77), the CLAUDE_CONFIG_DIR test case (lines 152–155), and the local-prerequisite gate — all intact.

---

Verdict: All three Codex P2 findings are resolved and covered by tests. Findings 1 and 2 above are clean-up items — the README is stale, and the telemetry test block is the sole non-hermetic outlier in the suite. Neither affects the correctness of the fix. Ready to merge once the carry-overs are addressed or deferred consciously.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 78b848a543

ℹ️ 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 45s —— View job

---

• Gather context and read changed files at HEAD 65c9edb
• Compare against prior review findings
• Assess new Codex P2 finding (false value preservation in jq)
• Post complete review

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

Reviewing at HEAD 65c9edb. This commit addresses the fourth and final Codex P2 finding — preserving boolean false in the jq extraction. Two minor carry-overs remain open.

---

Codex P2 (preserve boolean false values) — FIXED ✓

flag-commit-pr-skill-bypass.sh lines 119–122

# `has($k)` so a boolean `false` is read as "false", not collapsed to empty by
# jq's `//` (which treats false as absent).
jq -r --arg k "$key" '(.enabledPlugins // {}) | if has($k) then (.[$k] | tostring) else empty end' "$file" 2>/dev/null | head -1

The fix is correct. has($k) returns true even when the value is false; tostring converts it to the string "false". The prior approach with // would silently collapse an explicit false to empty — meaning a project source-control@…: false would look like an absent key, fail to override a user-global true, and fire the advisory despite the skill being disabled. That is exactly what the Codex P2 finding described.

The fix also interacts correctly with the precedence chain:

effective="$uval"
[[ -n "$bval" ]] && effective="$bval"
[[ -n "$bval" && -n "$lval" ]] && effective="$lval"
[[ "$effective" == "true" ]] && return 0

"false" is non-empty — so [[ -n "$bval" ]] fires and effective="$bval" overrides the user-global value, as intended. An absent key still produces an empty string (else empty end), so the absent-key pass-through behavior is unchanged. Both directions are now correct:

uval	bval	lval	effective (before fix)	effective (after fix)
"true"	"false"	""	"true" (wrong)	"false" ✓
"false"	"true"	""	"false" (wrong)	"true" ✓

The inline comment on lines 120–121 is precise and explains the why correctly.

sc_keys() is also correct (line 112): keys[] enumerates all keys in enabledPlugins regardless of value, so a source-control@…: false key is still included in the union — and then resolved correctly per-scope in source_control_enabled().

---

Finding 1 — README still describes old single-scope behavior (minor, carry-over)

plugins/guardrails/README.md line 19 (table row):

"Only fires when the consuming project's own .claude/settings.json enables the source-control plugin — silent otherwise."

Lines 125–132 (prose):

"flag-commit-pr-skill-bypass reads enabledPlugins from the consuming project's own .claude/settings.json"

Both still describe the old project-only scope. The hook now resolves per-exact-key across user-global (CLAUDE_CONFIG_DIR / ~/.claude/), project, and local scopes. The CHANGELOG and hook header comment are accurate; only the user-facing README is stale.

Fix this →

---

Finding 2 — Telemetry test not hermetic on HOME or CLAUDE_CONFIG_DIR (minor, carry-over)

flag-commit-pr-skill-bypass.test.sh lines 180–181:

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

Still the only block that calls env directly instead of going through run_hook, missing both hermetic guards (HOME="$HERMETIC_HOME" and -u CLAUDE_CONFIG_DIR). ENABLED_PROJECT has base_val=true, so neither a CI runner's user-global false nor a leaked CLAUDE_CONFIG_DIR would cause a functional failure today — but the asymmetry with the rest of the suite is notable now that CLAUDE_CONFIG_DIR is a live code path.

Fix: update line 180 to match the run_hook pattern:

env -u CLAUDE_CONFIG_DIR HOOK_TELEMETRY_SINK="$SINK" HOME="$HERMETIC_HOME" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

Fix this →

---

What's correct in this commit

• if has($k) then (.[$k] | tostring) else empty end — correct pattern for boolean-preserving extraction; // alternative would swallow false.
• Inline comment (lines 120–121) — accurately explains why has() is preferred over //.
• sc_keys() enumerates all keys regardless of value — correct; the per-key resolution in source_control_enabled() handles false values explicitly.
• Precedence chain (uval → bval → lval gated on bval) — correct and unchanged from prior review; works correctly now that "false" is non-empty.
• All prior fixes confirmed present — CLAUDE_CONFIG_DIR fallback, run_hook's -u CLAUDE_CONFIG_DIR + HERMETIC_HOME guards, local-key prerequisite gate, and per-exact-key resolution all intact.

---

Verdict: All four Codex P2 findings are now resolved and covered by tests. Findings 1 and 2 above are clean-up items that don't affect the correctness of the reported fix — the README is stale and the telemetry block is the sole non-hermetic outlier in an otherwise hermetic test suite. Happy to implement either if useful.

[claude]

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

---

• Read PR diff and changed files
• Analyze hook script for security vulnerabilities
• Analyze test file for security issues
• Assess plugin.json and CHANGELOG for supply-chain / config risk
• Post findings

---

Security Review — fix(guardrails): resolve source-control enablement across user-global scope

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md. No .github/workflows files changed; no supply-chain changes.

---

No new security vulnerabilities introduced by this PR.

The refactored source_control_enabled() extends scope resolution to user-global settings and adds per-exact-key evaluation across scopes. Security properties of all new operations:

• Read-only. Both sc_keys() and sc_key_value() pass file paths to jq in read mode only. No write surface.
• Advisory-only. The hook exits 0 unconditionally. A wrong result can only make the advisory fire or stay silent, never block or alter execution.
• Key names are not interpolated into jq filter expressions. sc_key_value() passes the key via --arg k "$key", so any key string extracted from a settings file — including adversarially constructed ones with special characters — is treated as a literal by jq, not as code. No jq injection surface.
• No eval / no execution. The effective value ("true" / "false") flows only into [[ "$effective" == "true" ]]. It is never passed to eval, a subshell, or any execution primitive.
• CLAUDE_CONFIG_DIR is the same trust tier as HOME. Both are runtime-provided environment variables; neither originates from the Bash command text being inspected. An adversarial value in either variable can only redirect the probe to a different settings file — advisory-only consequence.
• sort -u over key names: Key names from jq are piped through sort -u; sort treats them as data, not instructions. No injection surface.
• No GitHub Actions changes; no dependency pin changes. No supply-chain risk.

---

SUGGESTION · Confidence: low · sc_keys line 112 and sc_key_value line 122

jq -r '...' "$file" 2>/dev/null                      # sc_keys
jq -r --arg k "$key" '...' "$file" 2>/dev/null | head -1  # sc_key_value

Neither invocation uses a -- end-of-options separator before $file. If $file started with -, jq would interpret it as an option flag. In practice this cannot happen: user_settings is always constructed from $CLAUDE_CONFIG_DIR/settings.json or $HOME/.claude/settings.json (both absolute under normal Claude Code operation), and settings/local_settings derive from hook::repo_root (also absolute). The [[ -f "$file" ]] guard in both helpers further requires the path to name a regular file before jq is invoked, which a dash-prefixed string very rarely satisfies. If it somehow triggered: jq exits non-zero, 2>/dev/null suppresses the error, output is empty, effective stays empty, hook exits 0 without emitting the advisory — advisory-only impact, not a security regression. Hardening: jq -r '...' -- "$file" in both helpers.

This finding was raised by all four prior automated security reviews on this PR. The current revision adds sc_keys() alongside the pre-existing sc_key_value(), so the same observation applies to the new helper. No new surface has been introduced beyond what was already reviewed.

---

No further findings. The ${CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/settings.json} expansion (line 135) produces a file path that is never passed to eval or sourced. The [[ -n "$user_settings" ]] guard (line 136) correctly prevents probing when both CLAUDE_CONFIG_DIR and HOME are unset. Per-key --arg parameterisation in sc_key_value() eliminates any injection risk from adversarial key names in settings files. The env -u CLAUDE_CONFIG_DIR addition to run_hook in the test file is a positive hardening step.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65c9edbfa7

ℹ️ 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 1m 54s —— View job

---

• Gather context and read changed files at HEAD 1dd68b5
• Assess Finding 1 — README three-scope documentation (latest commit docs(guardrails): sync skill-gating README with user-global enablement)
• Assess Finding 2 — Telemetry test hermetic on HOME/CLAUDE_CONFIG_DIR
• Post complete review

---

PR Review — fix(guardrails): resolve source-control enablement across user-global scope

Reviewing at HEAD 1dd68b5. The latest docs(guardrails) commit addressed the README prose section (Finding 1 carry-over). One gap remains in the table row; the telemetry hermetic issue (Finding 2) is still open.

---

Finding 1 (README prose) — FIXED ✓

plugins/guardrails/README.md lines 125–136

The prose section is now accurate and complete:

flag-commit-pr-skill-bypass resolves enabledPlugins the way Claude Code merges it across scopes — user-global ($CLAUDE_CONFIG_DIR/settings.json, else ~/.claude/settings.json) as the base, the project's .claude/settings.json overriding it, and .claude/settings.local.json overriding that (a local override counts only for a key the project already declares — CC ignores a local-only key per anthropics/claude-code#27247). Each exact source-control@… key is resolved independently; if ANY resolves enabled the advisory fires.

Covers CLAUDE_CONFIG_DIR, three-scope precedence, the local-key prerequisite, and per-exact-key semantics. Well written.

---

Finding 1b — Table row still describes old single-scope behavior (minor)

plugins/guardrails/README.md line 19

Only fires when the consuming project's own `.claude/settings.json` enables
the `source-control` plugin — silent otherwise.

The table row wasn't updated alongside the prose. It still implies the hook requires a project-level key, which is now incorrect — a user-global-only install fires the advisory without any project settings.json. One-line fix: tighten the cell to something like Fires when any scope (user-global, project, or local) enables a source-control@… plugin key.

Fix this →

---

Finding 2 — Telemetry test not hermetic on HOME or CLAUDE_CONFIG_DIR (minor, carry-over)

flag-commit-pr-skill-bypass.test.sh lines 180–181

env HOOK_TELEMETRY_SINK="$SINK" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

This block still calls env directly instead of through run_hook, so it doesn't receive run_hook's -u CLAUDE_CONFIG_DIR or HOME="$HERMETIC_HOME" guards. Since ENABLED_PROJECT has base_val=true, a leaked CLAUDE_CONFIG_DIR pointing to a config dir with source-control: false would set uval="false" but bval="true" overrides it — no functional failure today. The asymmetry is more notable now that CLAUDE_CONFIG_DIR is a live code path.

Fix:

env -u CLAUDE_CONFIG_DIR HOOK_TELEMETRY_SINK="$SINK" HOME="$HERMETIC_HOME" CLAUDE_PROJECT_DIR="$ENABLED_PROJECT" \
  bash "$HOOK" <<<"$(command_json 'gh pr create --title x --body y')" >/dev/null 2>&1 || true

Fix this →

---

What's correct

• Hook logic — sc_keys() + sc_key_value() + source_control_enabled() are correct and complete. CLAUDE_CONFIG_DIR fallback (lines 135–136), per-exact-key loop (lines 149–160), has($k) | tostring boolean preservation (line 122), local-key prerequisite gate ([[ -n "$bval" && -n "$lval" ]] on line 158) — all intact and sound.
• README prose (lines 125–136) — fully updated; accurately describes CLAUDE_CONFIG_DIR, three-scope precedence, the #27247 local-key constraint, per-exact-key semantics, and fail-quiet posture.
• Test suite — all seven new test cases (four user-global, one CLAUDE_CONFIG_DIR, one multi-key, two local-key-prerequisite) are present and correctly exercise the failure modes being fixed. run_hook's HERMETIC_HOME + -u CLAUDE_CONFIG_DIR guards are in place for 20 of the 21 cases.
• CHANGELOG and version bump (0.9.5 → 0.9.6) are correct and well written.

---

Verdict: All four Codex P2 blockers are resolved. Finding 1b (table row) and Finding 2 (telemetry hermetic) are clean-up items — neither affects the correctness of the fix. Ready to merge once these are addressed or deferred consciously. Happy to implement either.

[claude]

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

---

Security Review

• Gather context
• Read PR diff and changed files
• Analyze for security vulnerabilities
• Post findings

---

Scope: flag-commit-pr-skill-bypass.sh, flag-commit-pr-skill-bypass.test.sh, plugin.json (version bump), CHANGELOG.md, README.md. No .github/workflows/ files changed; no dependency pins altered.

---

No new security vulnerabilities introduced by this PR.

The refactored source_control_enabled() splits the old single-helper design into sc_keys() + sc_key_value() and adds user-global scope resolution. Security properties of all new surfaces:

• Read-only. Both sc_keys() and sc_key_value() pass file paths to jq in read mode only — no write surface is introduced.
• Advisory-only. The hook exits 0 unconditionally; a wrong result can only make the advisory fire or stay silent, never block a tool call or alter execution.
• Key names are not interpolated into jq filter expressions. sc_key_value() passes the key via --arg k "$key", so any key string extracted from a settings file — including one containing shell metacharacters or jq-special characters — is treated as a literal string value by jq, never as filter code. No jq injection surface.
• No eval / no execution. Key names flow into sort -u (data operation), then into IFS= read -r key (safe, no word-splitting), then into the --arg call above and [[ -n "$key" ]]/[[ "$effective" == "true" ]] comparisons. The extracted effective value ("true"/"false") is never passed to eval, a subshell, or any execution primitive.
• jq's boolean-false preservation. The has($k) then (.[$k] | tostring) else empty pattern correctly preserves an explicit false as the string "false" (non-empty), so a project-level disable is honored. No correctness gap that could silently enable a disabled path.
• CLAUDE_CONFIG_DIR and HOME are in the same trust tier. Both are runtime-provided by the Claude Code process, not derived from the Bash command text being inspected. An adversarially set CLAUDE_CONFIG_DIR can only redirect the probe to a different config file — advisory-only consequence.
• env -u CLAUDE_CONFIG_DIR in run_hook. The test's env -u CLAUDE_CONFIG_DIR unset guard (line 77) is a positive hardening step that correctly prevents CI-runner environment leakage into the test suite.
• No GitHub Actions changes; no supply-chain surface. No workflow files or action/dependency pins were modified.

---

SUGGESTION · Confidence: low · sc_keys line 112 and sc_key_value line 122

jq -r '...' "$file" 2>/dev/null                            # sc_keys
jq -r --arg k "$key" '...' "$file" 2>/dev/null | head -1   # sc_key_value

Neither invocation uses a -- end-of-options separator before $file. If $file began with -, jq would interpret it as an option flag. In practice this cannot happen: user_settings expands from $CLAUDE_CONFIG_DIR/settings.json or $HOME/.claude/settings.json (both absolute under normal Claude Code operation), and settings/local_settings derive from hook::repo_root which also yields an absolute path. The [[ -f "$file" ]] guard in both helpers further requires the path to name an existing regular file before jq is invoked, which a dash-prefixed path very rarely satisfies. If somehow triggered: jq exits non-zero, 2>/dev/null suppresses the error, output is empty, effective stays empty or at the prior scope's value, and the hook exits 0 without emitting the advisory — advisory-only impact, not a security regression. Hardening: jq -r '...' -- "$file" in both helpers.

This finding was raised in all five prior automated security reviews on this PR. The current revision introduces sc_keys() alongside sc_key_value(), so the observation applies equally to both. No new surface has been introduced beyond what was already reviewed.

---

No further findings. The ${CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/settings.json} expansion (line 135) produces a file path used only with [[ -f ... ]] and as a jq argument — never passed to eval, source, or any execution primitive. The [[ -n "$user_settings" ]] guard (line 136/142/152) correctly prevents probing when both CLAUDE_CONFIG_DIR and HOME are unset. The sort -u pipeline over key names from trusted local files is a pure data operation with no injection surface. No new execution surfaces, no credential handling, no egress introduced.