Skip to content

feat(guardrails): declare node requirement and warn at session start when it is missing - #5547

Merged
kyle-sexton merged 32 commits into
mainfrom
fix/3708-node-declare-shell-form-detector
Sep 30, 2026
Merged

kyle-sexton merged 32 commits into
mainfrom
fix/3708-node-declare-shell-form-detector

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Refs: #3708

Summary

Hook rows launch through node, and Claude Code's native binary does not ship Node. A host without node therefore ran guards that enforced nothing, with no signal. This implements the owner decision (2026-09-29) on #3708: Q1(A) declare the requirement, Q1(B) add a shell-form notice, Q3(a) move the detector row to shell form. Q2 (whether a missing node should block) stays with the owner; the evidence is posted on the issue, which stays open with needs-human.

Fix

  • Q1(A): testing gets a Requirements section and a node probe in its setup check; source-control's existing Node.js line moves into a Requirements section. claude-ops and instruction-placement already declare it on main (instruction-placement from fix(instruction-placement): align /memory claims, consolidate cutover records, neutralize owner-decision records #5285, which this PR does not touch); claude-ops changes only to name the hook-failure-audit exception.
  • Q1(B): guardrails and disk-hygiene get a shell-form SessionStart row that prints a system message and model context when node is not on PATH.
  • Q3(a): the claude-ops hook-failure-audit Stop row runs in shell form, so the detector works when node is the missing piece.
  • docs/plugin-philosophy.md Hooks row and docs/conventions/hook-budget/README.md name the three shell-form exceptions; scripts/check-killswitch-hoist.sh documents the inline rows as not scanned.
  • Version bumps and changelog entries, each above origin/main: guardrails 0.44.0, disk-hygiene 0.37.0, claude-ops 0.77.1, source-control 0.67.2, testing 0.11.9.

Verification

Run on the merged head 365c8f9, which merges origin/main at 681789d:

  • git merge-tree --write-tree HEAD origin/main: no conflicts.
  • scripts/check-changelog-parity.sh --check, --check-order, --check-bump origin/main, --check-preserved origin/main: pass; scripts/check-stale-base-overlap.sh --check origin/main: up to date.
  • scripts/validate-plugins.sh: all manifests and the catalog validated.
  • bash plugins/guardrails/hooks/exec-bash.test.sh: pass, including the notice row with and without node.
  • bash plugins/disk-hygiene/hooks/run-python-hook.test.sh: pass.
  • bash plugins/claude-ops/hooks/hook-failure-audit.test.sh: pass (126 checks), including a pin that the Stop row stays shell form.
  • bash scripts/check-killswitch-hoist.sh, check-hook-exec-form.sh, check-hook-slow-shapes.sh, check-hook-userconfig-argv.sh, check-hooks-description.sh: clean.
  • Not run: hook-census.test.sh (needs strace, unavailable here).
  • Node-absent spawn failure reproduced on Claude Code 2.1.285; evidence in the issue comment.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 8 commits September 29, 2026 22:15
Refs #3708

Add a README Requirements entry for node in instruction-placement, testing and
source-control, and a setup check row that probes `command -v node` and reports
a missing node as FAIL in instruction-placement and testing. No version floor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…sing

Refs #3708

Add a shell-form SessionStart row to guardrails and disk-hygiene that runs
`command -v node` and, when node is absent, prints a JSON notice: systemMessage
for the user and additionalContext for the model, both saying the plugin's
guards cannot launch and enforce nothing. It needs no node, exits 0, and is
silent when node is present. Cover both rows in the plugin tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #3708

Record the shell-form SessionStart row in each README with the hooks-page
basis for why it prints JSON: systemMessage reaches the user and
additionalContext reaches the model, while plain stdout reaches only the model
and exit-2 stderr only the user.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #3708

Register the Stop row as `"shell": "bash"` with the script run through bash and
no args, so the detector that reports failed hook launches does not itself need
node. Update the README and the setup skill to say every other row still starts
through node.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Refs #3708

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

…-shell-form-detector

Renumber claude-ops to 0.71.2 and source-control to 0.64.1 above main's 0.71.1 and 0.64.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 3 commits September 30, 2026 00:04
…rsions

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 2 commits September 30, 2026 00:47
…ows and pin the claude-ops detector row

The hook-budget "What shipped" and "Prerequisite" bullets, the guardrails setup
node check and the disk-hygiene setup node check still said every hook row runs
node, which the shell-form SessionStart notice rows and the hook-failure-audit
Stop row contradict. The hook-budget Scope bullet said no gate enforces the
exceptions; the claude-ops row now has a test like the other two.

The disk-hygiene run-python-hook test's later EXIT trap also dropped NOPY_DIR,
leaking that temp directory.

Refs: #3708

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… eval

The setup eval's expected output still said every hook row starts through
node, which the setup skill body no longer says.

Refs: #3708

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 2 commits September 30, 2026 11:23
Take origin/main's instruction-placement files: the Requirements section and the node probe in
setup already landed there, so this branch no longer changes that plugin. Renumber claude-ops,
disk-hygiene, source-control and testing above main's current versions and keep each changelog
entry on top of main's.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…-shell-form-detector

# Conflicts:
#	plugins/disk-hygiene/.claude-plugin/plugin.json
#	plugins/disk-hygiene/CHANGELOG.md
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 30, 2026 15:44
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T15:52:36.699516Z e254d8a Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

No security issues found.

Scope reviewed: the diff against origin/main (29 files, +178/-41), focused on the security-relevant surface — the three new/changed hook rows and their test coverage.

What changed, from a security lens:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json each add a new SessionStart row in shell form ("shell": "bash") that runs command -v node >/dev/null 2>&1 || printf '%s\n' '{...json...}'. The JSON payload is a static literal string with no interpolation of user input, argv, environment content, or file contents — nothing an attacker controls flows into the command. guardrails/hooks.json#L1-L13, disk-hygiene/hooks.json#L1-L13
  • plugins/claude-ops/hooks/hooks.json's hook-failure-audit Stop row moves from node exec form to shell form: "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh\"". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own plugin-install path, not attacker-influenced, and the value is double-quoted in the shell string, so this isn't a new injection surface. claude-ops/hooks.json#L249-L256
  • None of the existing PreToolUse guard rows (secrets, destructive-delete, recursive-delete, git-bypass, etc.) were converted to shell form or otherwise weakened — only new SessionStart notice rows were added, and the Stop-row change is a detector, not a blocking guard.
  • Test coverage (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh) pins the shell-form rows to stay silent/non-blocking when node is present and to emit the notice without crashing when it isn't, on both the model-context and user-message channels.

Instruction-surface deletion lens: docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow the prior "every hook row is exec form" / "no shell-form hook row remains" statements to name three explicit shell-form exceptions. I checked this against the instruction exception register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority). The exec-form-only rule is a house-style/architecture convention, not itself a guardrail preventing one of those consequence classes — the actual security-relevant guard rows (secrets, destructive delete, etc.) are untouched by this PR. The three named exceptions are each pinned by their own plugin's hook test (regression back to node fails the test), so this isn't an unenforced softening. No finding here.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Run skip-gate checks (open, not draft, non-trivial, no prior review on this head)
  • Review diff for correctness, error handling, test coverage, maintainability
  • Adversarially verify candidate findings
  • Post inline comments for anchored findings
  • Post final summary

No blocking findings. I reviewed the full diff (gh pr diff 5547) against origin/main across all 29 changed files, with particular attention to the new shell-form SessionStart/Stop hook rows in guardrails, disk-hygiene, and claude-ops, since those are the functional core of this PR (everything else is documentation/changelog/version).

What I specifically checked and verified (independently, not just author-claimed):

  • JSON shape of the new notice rows (plugins/guardrails/hooks/hooks.json:4-16, plugins/disk-hygiene/hooks/hooks.json:4-16): the command -v node >/dev/null 2>&1 || printf '...' shell one-liners parse as valid, well-formed JSON strings and valid bash — the embedded single-quoted JSON literal with escaped inner double quotes is syntactically sound, and the || short-circuit means nothing prints (exit 0) when node resolves.
  • Whether systemMessage + hookSpecificOutput.additionalContext actually reach the user/model for a SessionStart hook. This is the crux of Q1(B) working at all. I fetched the live Claude Code hooks docs (https://code.claude.com/docs/en/hooks) rather than trusting the PR's citation, and confirmed both fields are honored for SessionStart — the docs even give systemMessage as the canonical example for a SessionStart warning, and hookSpecificOutput.additionalContext is documented for this event. This is a new pattern for this repo (no prior SessionStart row used this shape), but it checks out.
  • Windows shell-form resolution for "shell": "bash". The exec-form launcher (exec-bash.mjs) exists specifically to avoid the System32 WSL-relay bash.exe trap. I confirmed via the same live docs fetch that Claude Code's native shell-form resolution on Windows also targets Git Bash specifically (falling back to PowerShell only when Git Bash is absent), so the three new inline rows don't reintroduce that historical footgun.
  • scripts/check-killswitch-hoist.sh discovery logic (scripts/check-killswitch-hoist.sh:104-159): traced the token-walk by hand. The claude-ops hook-failure-audit Stop row still embeds the literal hook-failure-audit.sh path inside its command string, so it remains discovered and scanned by rule 2 (contrary to my first-pass suspicion that dropping args would make it invisible). The guardrails/disk-hygiene node-probe rows reference no .sh file at all, so they correctly fall into "NOT SCANNED," matching the updated comment at scripts/check-killswitch-hoist.sh:57-58.
  • check-hook-exec-form.sh: confirmed by reading its header that shell-form rows (no args) are explicitly out of scope ("shell form... is never inspected here"), so the new rows can't trip that gate.
  • Test coverage for all three new/changed rows (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh) exercises both the "node present" (silent) and "node absent" (warns, exit 0) paths, and pins the shell-form shape so a future accidental sweep back to node exec-form fails the test — this matches the doc claims in docs/conventions/hook-budget/README.md.
  • Doc/version/changelog consistency (plugin.json bumps vs. CHANGELOG.md entries, README section placement) reads correctly for all five touched plugins; I did not re-run the author's listed shell scripts myself (no shell-exec permission in this environment), so that verification remains author-claimed.

No inline comments were needed — I found nothing that clears the "a careful reviewer would block or flag" bar. Nice work tracing the two failure branches (missing bash vs. missing node) separately in the disk-hygiene README's fail-open table rather than conflating them.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e254d8a985

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

Comment thread plugins/guardrails/hooks/hooks.json
Bump disk-hygiene to 0.34.0, guardrails to 0.44.0 and source-control to 0.65.5
above main, keep main's changelog entries, and document that the guardrails
SessionStart notice does not read the per-guard toggles.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review per custom instructions and follow it exactly
  • Post findings/summary

Skip gate: this head already has an applicable successful security review.

I ran git diff origin/main...HEAD on the merged head 1d2dbe29c8a2e5fb71aff29ea439bad7431c7913 and confirmed it is byte-for-byte the same 29-file, +180/-41 diff already reviewed and cleared at commit e254d8a (the "No security issues found" review posted above at 2026-09-30T15:45:23Z). The only content added since e254d8a (checked via git diff e254d8a 1d2dbe2 --stat) came from merging origin/main forward — unrelated attribution/disk-hygiene-clean/session-flow/planning work that landed on main in the interim, none of which touches this PR's own files. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, I independently re-checked the PR's own security-relevant surface (scoped to this PR's diff, not the interim main commits):

  • plugins/guardrails/hooks/hooks.json SessionStart row and plugins/disk-hygiene/hooks/hooks.json SessionStart row: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the printf payload is a fixed string literal, so there's no injection surface here.
  • plugins/claude-ops/hooks/hooks.json Stop row: moves from node-exec form to "shell": "bash" with bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions. docs/conventions/instruction-exception-register/README.md is present in this checkout; none of the protected classes there (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: the Codex bot's P2 review comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, so it's out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Review skipped — no new content since last review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Run skip-gate checks

The current head 1d2dbe2 is a merge of origin/main into this branch that only pulls in unrelated upstream changes (from other merged PRs). git diff origin/main...HEAD at this head is identical to the diff at e254d8a — same 29 files, +180/-41 — which this lane already reviewed in full (see the "Code review complete" comment above: no blocking findings).

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review. Nothing new to report.

Keep main's changelog entries, bump guardrails to 0.44.0, disk-hygiene to
0.34.0 and source-control to 0.65.6 above main.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Keep main's disk-hygiene 0.34.3 entry below this PR's 0.35.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (9f60c7946d0d63667c27016a5e9c0cec0834bbe0) is still the same 29 files, +180/-41 already reviewed and cleared seven times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1 — all "No security issues found"). I checked git diff 4f643c1..HEAD --stat directly: it touches 30 files, none of them this PR's core security surface (docs/native-surfaces*, plugins/context-budget/**, plugins/evals/**, plugins/playbooks/**, plugins/prototype/**, plugins/visualization/**, etc.) — all pulled in by merging origin/main forward again. Exactly one file overlaps this PR's own file set, plugins/disk-hygiene/CHANGELOG.md; diffing it directly shows an unrelated 0.34.3 entry (#5520 doc fix) landed below this PR's own entry in the interim — this PR's own SessionStart-notice bullet is byte-identical to what was already reviewed. I also confirmed git diff 4f643c1 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md docs/conventions/hook-budget/README.md is empty — the hook rows and instruction-surface docs are untouched since the last review. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Run skip-gate checks (open, not draft, non-trivial, this head already has an applicable successful review from this lane)

Review skipped — no new content since last review

The current head 9f60c79 is a merge of origin/main into this branch. git log --oneline 4f643c1..9f60c79 shows only one new commit besides the merge itself, fa4142d (feat(plugins): bake native route phrases into the remaining sweep units (#5570)), which touches only context-budget, evals, playbooks, prototype, visualization, and docs/native-surfaces* — none of which overlap this PR's own file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged at 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, and 4f643c1.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

Keep main's disk-hygiene 0.34.4 entry below this PR's 0.35.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Run skip-gate checks (open, not draft, non-trivial, this head already has an applicable successful review from this lane)

Review skipped — no new content since last review

The current head 5f79b94 is a merge of origin/main into this branch. git log --oneline 9f60c79..5f79b94 shows only one new commit besides the merge itself, 0c38144 (fix(disk-hygiene): gate --sizes-only behind the large-scan confirmation and keep no per-path entries, #5587), which does not touch any of this PR's own files.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged at 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, and 9f60c79.

I diffed this PR's own file set between the last-reviewed head (9f60c79) and the current head (5f79b94) directly. The functional files — plugins/{guardrails,disk-hygiene}/hooks/hooks.json, plugins/claude-ops/hooks/hooks.json, all three test files (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), the setup SKILL.mds, the READMEs, scripts/check-killswitch-hoist.sh, and both docs files — are byte-identical to what was already reviewed. The only overlap is plugins/disk-hygiene/CHANGELOG.md, which gained an unrelated upstream [0.34.4] entry (the --sizes-only large-scan-gate fix, #4009/#5587) landing below this PR's own SessionStart-notice entry via the merge — this PR's own entry is untouched.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Security review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (5f79b940e8630e8528c1ec8c41087851027de43d) is still the same 29 files, +180/-41 already reviewed and cleared seven times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79 — all "No security issues found"). git log --oneline 9f60c79..5f79b94 shows only one new commit besides the merge itself, 0c38144 (fix(disk-hygiene): gate --sizes-only behind the large-scan confirmation and keep no per-path entries (#5587)), which touches plugins/disk-hygiene/skills/clean/**, plugins/planning/**, and docs/native-surfaces* — none of which overlap this PR's own file set except plugins/disk-hygiene/CHANGELOG.md and plugins/disk-hygiene/README.md. I diffed those two directly (git diff origin/main...HEAD -- <files>): the only new content is an unrelated 0.34.4 changelog entry (#4009 fix) and README wording about --sizes-only's large-scan gate, both outside this PR's own contribution — this PR's own SessionStart-notice bullet and Requirements-section wording are byte-identical to what was already reviewed. I also confirmed git diff 9f60c79 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md docs/conventions/hook-budget/README.md is empty — the hook rows and instruction-surface docs are untouched since the last review. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

Bump disk-hygiene to 0.36.0 and source-control to 0.67.1 above main and keep
main's changelog entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (f75aa503ab738add5bb551478ca7dcfa4b37412e) is still the same 29 files, +180/-41 already reviewed and cleared eight times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94 — all "No security issues found"). git log --oneline 5f79b94..f75aa50 shows only one new commit besides the merge itself, 2e0be76 (feat(disk-hygiene): handoff-apply, a Linux route for an acknowledged checkout (#5541)), an unrelated disk-hygiene feature.

I diffed this PR's own file set between the last-reviewed head (5f79b94) and the current head (f75aa50) directly. The security-relevant files are byte-identical since the last review:

git diff 5f79b94 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md

→ empty.

Four files did pick up interleaved content from the #5541 merge (plugins/disk-hygiene/{.claude-plugin/plugin.json,CHANGELOG.md,README.md}, plugins/source-control/{.claude-plugin/plugin.json,CHANGELOG.md,README.md}, plugins/claude-ops/{.claude-plugin/plugin.json,CHANGELOG.md}), but comparing this PR's own contribution against the current origin/main (git diff origin/main...HEAD -- <those files>) shows only version-number renumbering (disk-hygiene 0.35.0→0.36.0, source-control 0.67.0→0.67.1, claude-ops 0.75.1→0.75.2) above newly-landed unrelated upstream entries (handoff-apply, worktree_reap_after_hours). This PR's own text — the SessionStart node-probe changelog bullet, the Requirements-section wording, the shell-form hook-failure-audit note — is byte-identical to what was already reviewed. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head f75aa50 is a merge of origin/main into this branch. git log --oneline 5f79b94..f75aa50 shows only one new commit besides the merge itself, 2e0be76 (feat(disk-hygiene): handoff-apply, a Linux route for an acknowledged checkout (#5541)), which touches plugins/disk-hygiene/skills/clean/**, plugins/disk-hygiene/hygiene.py, and related docs — none of which overlap this PR's own file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, and 5f79b94.

I diffed this PR's own functional file set between the last-reviewed head (5f79b94) and the current head (f75aa50) directly — hooks.json in guardrails/disk-hygiene/claude-ops, all three test files, scripts/check-killswitch-hoist.sh, both instruction-surface docs, and every setup/SKILL.md — and it is byte-identical (git diff 5f79b94 HEAD -- <those files> is empty). The only overlap with this PR's changed-file set is renumbering from the merge: disk-hygiene version 0.35.0 → 0.36.0 and source-control 0.66.1 → 0.67.1, because unrelated upstream entries (handoff-apply #5541, worktree reap/audit work) landed in those plugins' CHANGELOGs above this PR's own slot. This PR's own contributed CHANGELOG/README lines (the SessionStart notice bullets, the Requirements-section wording, the hook-failure-audit shell-form line) are unchanged.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

Keep main's disk-hygiene 0.35.1 entry below this PR's 0.36.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head 205f13f is a merge of origin/main into this branch, pulling in one new commit besides the merge itself: 93d58c6 (docs(disk-hygiene): record hook ask behavior under bypassPermissions, #5613). That commit only touches plugins/disk-hygiene/CHANGELOG.md (an unrelated upstream entry, landing separately from this PR's own [0.36.0] entry) and plugins/disk-hygiene/skills/clean/reference/safety-model.md, neither of which is part of this PR's own contribution.

gh pr diff 5547 at this head is still the same 29 files this lane has already reviewed in full at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, and f75aa50. I compared the file list and the disk-hygiene/CHANGELOG.md/README.md hunks directly against the diff at f75aa50: this PR's own contributed content (the SessionStart node-probe entries, the Requirements-section wording, the hooks.json rows, the test files, scripts/check-killswitch-hoist.sh, both instruction-surface docs) is unchanged — the merge only interleaved unrelated upstream changelog/doc entries around it.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (205f13f0337e708064a675e980b1970c42909674) is still the same 29 files, +180/-41 already reviewed and cleared nine times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50 — all "No security issues found"). git log --oneline f75aa50..HEAD shows only one new commit besides the merge itself, 93d58c6 (docs(disk-hygiene): record hook ask behavior under bypassPermissions (#5613)), a docs-only change to safety-model.md that does not touch this PR's own file set.

I diffed this PR's own 29 changed files between the last-reviewed head (f75aa50) and the current head (205f13f) directly. Only one file picked up any content: plugins/disk-hygiene/CHANGELOG.md gained an unrelated [0.35.1] entry (the safety-model.md bypassPermissions record, #5613/#5609) landing above this PR's own [0.35.0] entry via the merge — this PR's own SessionStart-notice bullet is byte-identical to what was already reviewed. I also confirmed directly that the three security-relevant hook-row files and the instruction-surface docs are untouched since the last review:

git diff f75aa50 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md

→ empty. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

Bump claude-ops to 0.75.3, source-control to 0.67.2 and testing to 0.11.9 above
main and keep main's changelog entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (5ccf2d6f1a09dc368eecadc145d32d8006554d0c) is still the same 29 files, +180/-41 already reviewed and cleared ten times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50, 205f13f — all "No security issues found"). git log --oneline 205f13f..HEAD shows only one new commit besides the merge itself, 1b42067 (fix(code-metrics): setup check no longer lists adapter_paths as a collector (#5611)), which touches only plugins/code-metrics/** — outside this PR's own file set.

The merge itself (5ccf2d6) pulled forward a larger batch of unrelated origin/main history than prior syncs — a hook-utils.sh update landing across ~20 formatter/guard plugins, and guardrails/hooks/secret-pattern-detection.sh and block-windows-drive-tmp.sh changes (the new credential-read guard referenced in the author's earlier reply-thread comment). None of that is this PR's own content: I confirmed git diff origin/main...HEAD (the correct three-dot comparison against the current upstream tip) is still exactly 29 files, +180/-41, and diffed the three security-relevant hook-row files directly:

git diff 205f13f HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md

→ empty. The one visible change against the new origin/main tip is that guardrails/hooks/hooks.json's top-level description string picked up a clause naming the new upstream credential-read guard (merge noise from the interim PR, not this PR's edit) — the SessionStart node-probe rows this PR actually adds are byte-identical to what was already reviewed. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head 5ccf2d6 is a merge of origin/main into this branch. git log --oneline 205f13f..5ccf2d6 shows only one new commit besides the merge itself, 1b42067 (fix(code-metrics): setup check no longer lists adapter_paths as a collector (#5611)), which touches only the code-metrics plugin — none of it part of this PR's own file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through ten subsequent heads, most recently 205f13f. I diffed this PR's own functional files directly between the last-reviewed head (205f13f) and the current head (5ccf2d6) — hooks.json in guardrails/disk-hygiene/claude-ops, all three test files (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), scripts/check-killswitch-hoist.sh, and both instruction-surface docs (docs/plugin-philosophy.md, docs/conventions/hook-budget/README.md) — and the diff is empty.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

Keep main's disk-hygiene 0.35.2 entry below this PR's 0.36.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (1e478baae8ca3816ce0fb705bf4ced0d7556c499) is still the same 29 files, +180/-41 already reviewed and cleared eleven times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50, 205f13f, 5ccf2d6 — all "No security issues found"). git log --oneline 5ccf2d6..HEAD shows only one new commit besides the merge itself, 20379dc (fix(disk-hygiene): dedupe the engine gate's marker-free identity probes (#5526)), which touches only plugins/disk-hygiene/hygiene.py/destructive_guard.py and its tests — outside this PR's own 29-file set.

I confirmed directly that this PR's security-relevant surface is untouched since the last review:

git diff 5ccf2d6 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md

→ empty. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head 1e478ba is a merge of origin/main into this branch. git log --oneline 5ccf2d6..1e478ba shows only one new commit besides the merge itself, 20379dc (fix(disk-hygiene): dedupe the engine gate's marker-free identity probes (#5526)), which touches plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py and its tests — none of it part of this PR's own file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through eleven subsequent heads, most recently 5ccf2d6.

I diffed this PR's own functional files directly between the last-reviewed head (5ccf2d6) and the current head (1e478ba) — hooks.json in guardrails/disk-hygiene/claude-ops, all three test files (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), scripts/check-killswitch-hoist.sh, and both instruction-surface docs (docs/plugin-philosophy.md, docs/conventions/hook-budget/README.md) — and the diff is empty. The only overlap with this PR's changed-file set is plugins/disk-hygiene/CHANGELOG.md, which picked up an unrelated [0.35.2] entry (the #5526 fix) landing below this PR's own unchanged [0.36.0] entry via the merge.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

Bump claude-ops to 0.76.1 above main and keep main's 0.76.0 entry.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

gh pr diff 5547 at the current head (0ad5c76783425856fac3d0c2783ba9326480c192) is still the same 29 files already reviewed and cleared twelve times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50, 205f13f, 5ccf2d6 — all "No security issues found"). git log --oneline 1e478ba..HEAD shows only one new commit besides the merge itself, 4eb7a55 (feat(claude-ops): changelog apply hands decisions off instead of implementing them (#5617)), which touches plugins/claude-ops/skills/changelog/**, docs/conventions/**, and a large batch of scripts/sync-* helper files — none of it part of this PR's own 29-file set.

I read the full current diff directly (gh pr diff 5547) and confirmed it is content-identical to the diff already reviewed: the same 29 files, and the three security-relevant hook-row files are byte-for-byte what was cleared before:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head 0ad5c76 (0ad5c76783425856fac3d0c2783ba9326480c192) is a merge of origin/main into this branch. git log --oneline 1e478ba..0ad5c76 shows only one new commit besides the merge itself, 4eb7a55 (feat(claude-ops): changelog apply hands decisions off instead of implementing them (#5617)), which touches the claude-ops changelog skill and its evals — none of it part of this PR's own file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through twelve subsequent heads, most recently 1e478ba.

I diffed this PR's own functional files directly between the last-reviewed head (1e478ba) and the current head (0ad5c76):

git diff 1e478ba HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json plugins/guardrails/hooks/exec-bash.test.sh \
  plugins/disk-hygiene/hooks/run-python-hook.test.sh plugins/claude-ops/hooks/hook-failure-audit.test.sh \
  scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md docs/conventions/hook-budget/README.md \
  plugins/testing/skills/setup/SKILL.md plugins/source-control/README.md plugins/claude-ops/skills/setup/SKILL.md

→ empty.

The only files that picked up any content from the merge overlap this PR's file set in claude-ops only: plugins/claude-ops/.claude-plugin/plugin.json (version renumbered 0.76.0 → 0.76.1 because the unrelated #5617 entry landed at 0.76.0 in the interim), plugins/claude-ops/CHANGELOG.md (this PR's own hook-failure-audit shell-form entry renumbered to [0.76.1], text unchanged), and plugins/claude-ops/README.md (this PR's own Requirements-section wording is byte-identical). None of that is new content from this PR.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head 747725f (747725fe67dc1bf878f7f081963e8046d8302cd3) is a merge of origin/main into this branch. git log --oneline 0ad5c76..747725f shows only one new commit besides the merge itself, 4e49cab (feat(disk-hygiene): add model-invocable read-only audit skill (#5590)), which is itself held with a do-not-merge label and touches only disk-hygiene's own skill/hygiene files — none of them part of this PR's file set.

git diff origin/main...HEAD --stat at this head is still the same 29 files, +180/-41 already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through twelve subsequent heads, most recently 0ad5c76.

I diffed this PR's own file set directly between the last-reviewed head (0ad5c76) and the current head (747725f):

git diff 0ad5c76 747725f -- docs/plugin-philosophy.md docs/conventions/hook-budget/README.md \
  plugins/claude-ops plugins/disk-hygiene/.claude-plugin/plugin.json plugins/disk-hygiene/CHANGELOG.md \
  plugins/disk-hygiene/README.md plugins/disk-hygiene/hooks plugins/disk-hygiene/skills/setup \
  plugins/guardrails plugins/source-control plugins/testing scripts/check-killswitch-hoist.sh

The only overlap is plugins/disk-hygiene/.claude-plugin/plugin.json and CHANGELOG.md: the version renumbered 0.36.0 → 0.37.0 because the unrelated #5590 entry (itself held behind do-not-merge) landed at 0.36.0 in the interim. This PR's own contributed changelog text (the SessionStart node-probe notice bullet) is byte-identical to what was already reviewed, just relabeled under the new [0.37.0] header. All other functional files — hooks.json in guardrails/disk-hygiene/claude-ops, the three test files (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), scripts/check-killswitch-hoist.sh, and both instruction-surface docs — are unchanged (empty diff).

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (747725fe67dc1bf878f7f081963e8046d8302cd3) is still the same 29 files, +180/-41 already reviewed and cleared thirteen times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50, 205f13f, 5ccf2d6, 0ad5c76 — all "No security issues found"). git log --oneline 0ad5c76..747725f shows only one new commit besides the merge itself, 4e49cab (feat(disk-hygiene): add model-invocable read-only audit skill (#5590)), an unrelated disk-hygiene feature (/disk-hygiene:audit) that carries its own do-not-merge hold history on its own PR and is not this PR's content.

I diffed this PR's own 29-file set directly between the last-reviewed head (0ad5c76) and the current head (747725f). Three files picked up content from that merge, all in disk-hygiene:

  • plugins/disk-hygiene/.claude-plugin/plugin.json: version renumbered 0.36.0 → 0.37.0 because the unrelated #5590 entry landed at 0.36.0 in the interim.
  • plugins/disk-hygiene/CHANGELOG.md: this PR's own SessionStart-notice entry renumbered to [0.37.0], text unchanged; the new [0.36.0] entry below it is #5590's own contribution, not this PR's.
  • plugins/disk-hygiene/README.md: gained the unrelated /disk-hygiene:audit usage line and blurb — not this PR's content, and it doesn't touch the SessionStart node-probe section this PR added.

I confirmed directly that this PR's security-relevant surface is byte-identical to what was already reviewed:

git diff 0ad5c76 HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md plugins/guardrails/hooks/exec-bash.test.sh \
  plugins/disk-hygiene/hooks/run-python-hook.test.sh plugins/claude-ops/hooks/hook-failure-audit.test.sh

→ empty. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow it exactly
  • Post findings/summary

Review skipped — no new content since last review

The current head da1db3c (da1db3c848b3aaeb2754fd19f09c5876dd6b9e20) is a merge of origin/main into this branch. git log --oneline 747725f..da1db3c shows only one new commit besides the merge itself, ee58afe (fix(claude-ops): cap unreferenced_versions in the install-state JSON (#5614)), which touches plugins/claude-ops/skills/audit-install-state/** — none of it part of this PR's own file set.

gh pr diff 5547 --name-only at this head is still the same 29 files already reviewed in full by this lane at e254d8a ("Code review complete — No blocking findings," posted 2026-09-30T15:47:17Z) and reconfirmed unchanged through thirteen subsequent heads, most recently 747725f.

I diffed this PR's own functional files directly between the last-reviewed head (747725f) and the current head (da1db3c):

git diff 747725f HEAD -- plugins/claude-ops/hooks/hooks.json plugins/claude-ops/hooks/hook-failure-audit.test.sh \
  plugins/disk-hygiene/hooks/hooks.json plugins/guardrails/hooks/hooks.json plugins/guardrails/hooks/exec-bash.test.sh \
  plugins/disk-hygiene/hooks/run-python-hook.test.sh scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md

→ empty.

The only overlap with this PR's changed-file set is plugins/claude-ops/.claude-plugin/plugin.json and plugins/claude-ops/CHANGELOG.md: the version renumbered 0.76.1 → 0.77.1 because the unrelated #5614 entry landed at 0.77.0 in the interim. This PR's own contributed changelog text (the hook-failure-audit shell-form entry) is byte-identical to what was already reviewed, just relabeled under the new [0.77.1] header.

Per the skip gate in /review:code-review ("this head already has a successful review from this lane that still applies"), I'm not re-running the full review.

Nothing new to report. The one open review thread (Codex's P2 suggestion to suppress the node-missing notice when every guard toggle is off, on plugins/guardrails/hooks/hooks.json:10) already has the author's reasoned decline in-thread, and this lane's prior passes didn't independently flag it as blocking.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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


Tasks

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow it exactly
  • Post findings/summary

Security review

Skip gate: this head already has an applicable successful security review.

git diff origin/main...HEAD at the current head (da1db3c848b3aaeb2754fd19f09c5876dd6b9e20) is still the same 29 files, +180/-41 already reviewed and cleared fourteen times on this PR (e254d8a, 1d2dbe2, be3c213, e2e12a3, 1e3bfb2, 969b1c1, 4f643c1, 9f60c79, 5f79b94, f75aa50, 205f13f, 5ccf2d6, 0ad5c76, 747725f — all "No security issues found"). git log --oneline 747725f..HEAD shows only one new commit besides the merge itself, ee58afe (fix(claude-ops): cap unreferenced_versions in the install-state JSON (#5614)), an unrelated claude-ops fix to install_state.py's report size that does not touch this PR's own file set.

I confirmed directly that this PR's security-relevant surface is byte-identical to what was already reviewed:

git diff 747725f HEAD -- plugins/guardrails/hooks/hooks.json plugins/disk-hygiene/hooks/hooks.json \
  plugins/claude-ops/hooks/hooks.json scripts/check-killswitch-hoist.sh docs/plugin-philosophy.md \
  docs/conventions/hook-budget/README.md plugins/guardrails/hooks/exec-bash.test.sh \
  plugins/disk-hygiene/hooks/run-python-hook.test.sh plugins/claude-ops/hooks/hook-failure-audit.test.sh

→ empty. Per the skip gate, I'm not re-litigating a review that already applies to this exact diff.

For completeness, the PR's own security-relevant surface remains unchanged from the prior reviews:

  • plugins/guardrails/hooks/hooks.json and plugins/disk-hygiene/hooks/hooks.json SessionStart rows: command -v node >/dev/null 2>&1 || printf '%s\n' '{...static JSON...}'. No argv, env, or file content is interpolated into the command — the payload is a fixed string literal, so there's no injection surface.
  • plugins/claude-ops/hooks/hooks.json Stop row: shell form bash "${CLAUDE_PLUGIN_ROOT}/hooks/hook-failure-audit.sh". ${CLAUDE_PLUGIN_ROOT} is Claude Code's own install path, not attacker-influenced, and it's double-quoted, so no new shell-injection surface.
  • No PreToolUse guard row (secrets, destructive-delete, git-bypass, etc.) was weakened, removed, or converted to shell form — only new non-blocking SessionStart notices were added, and the Stop-row change is a detector, not an enforcement guard.
  • Instruction-surface lens: docs/conventions/instruction-exception-register/README.md is present in this checkout. docs/plugin-philosophy.md and docs/conventions/hook-budget/README.md narrow "every hook row is exec form" to name three explicit shell-form exceptions; none of the register's protected classes (irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance, agent-authority) map to this rule — it's a house-style/architecture convention about hook launch mechanics, not a guard against one of those consequence classes, and the actual security-relevant guard rows are untouched. Each of the three named exceptions is pinned by its own plugin's hook test (exec-bash.test.sh, run-python-hook.test.sh, hook-failure-audit.test.sh), so a regression back to node exec form fails CI. No finding.

No CRITICAL, IMPORTANT, or SUGGESTION security findings.

(Note: Codex's earlier P2 comment about the notice firing even when all guards are disabled is a UX/noise concern, not a security issue — it doesn't weaken any guard or open an exploit path, and the author already declined it in-thread with a reasoned explanation. Out of this lane's scope.)

@kyle-sexton
kyle-sexton merged commit 7106c28 into main Sep 30, 2026
34 of 35 checks passed
@kyle-sexton
kyle-sexton deleted the fix/3708-node-declare-shell-form-detector branch September 30, 2026 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant