Skip to content

feat(guardrails): deny a blocking hook row whose dispatcher exits 0 without a verdict - #10120

Merged
kyle-sexton merged 7 commits into
mainfrom
work-resolve-issue/issue-7681-40232573194-1
Oct 27, 2026
Merged

kyle-sexton merged 7 commits into
mainfrom
work-resolve-issue/issue-7681-40232573194-1

Conversation

@melodic-automation-lanes

Copy link
Copy Markdown
Contributor

Closes #7681

Summary

Opened by the work-resolve-issue lane. The run's own summary:

The shared launcher lib/exec-bash.mjs gains a per-row opt-in --require-verdict flag. The launcher puts a marker path in the child's EXEC_BASH_VERDICT_FILE, inside a fresh mkdtemp directory it removes on exit. If the child exits 0 without writing the marker, the launcher exits 2 with one stderr line. This catches the Windows Git Bash crash that reads as exit 0 with empty stdout. guardrails run-guards.sh writes the marker as its last action at each deliberate exit, never from a --report process, and the five --fail-closed rows in hooks.json now also pass --require-verdict. Rows without the flag behave as before, except that an inherited marker variable is dropped. The 24 vendored copies were regenerated, and every carrier got a minor fragment.

Not verified

  • plugins/guardrails/hooks/exec-bash.test.sh, run-guards.test.sh, require-jq-posture.test.sh, block-cross-domain.test.sh, abort-boundary.test.sh and lib/exec-bash.*.test.mjs were not run here. This lane runs no interpreter, and check-affected skips test suites ('runs the checked tree's own test code')
  • The shared-copy sync check (scripts/sync-shared-copies.sh --check) and scripts/check-changelog-fragments.sh --check as CI runs them on the PR are left to the PR's CI. The copies were regenerated with the sync wrapper, and the lane's check-changelog-fragments passed.
  • Fragments were written by hand, not with scripts/new-changelog-fragment.sh --carriers-of lib/exec-bash.mjs minor. That script refuses on a detached HEAD ('HEAD is not on a branch'). The files follow its naming and front-matter format, and check-changelog-fragments validated all 24.
  • The issue's step 3 says to commit with a runner commit tool. This run's instructions say to leave every change uncommitted for the runner to turn into a patch, and none was given, so nothing was committed.
  • Not checked on Windows: Git Bash writing the marker through the forward-slash C:/... path the launcher passes, and the real crash repro. #8992 owns repeating the melo-lap-001 crash repro and confirming a deny.
  • Behavior change for reviewers: the guardrails dispatcher's own-code abort used to be fail-open with a systemMessage notice (#3528). Under --require-verdict it now denies the call with a fail-closed line. A guard's own crash still settles fail-open and the chain finishes with a verdict. Also left as
  • The issue's owner decision says the change is structural, so a human builds or merges it. This PR needs a human merge.

Fix

  • lib/exec-bash.mjs: new --require-verdict switch, parsed with --fail-closed through a shared SWITCHES table. Adds a VERDICT_ENV export and a mkdtemp marker directory cleaned up in an exit handler; the path is passed with forward slashes on win32. A child exit 0 with no marker exits 2 with a 'no-verdi
  • plugins/*/hooks/exec-bash.mjs (24 copies, guardrails included): regenerated from lib/exec-bash.mjs with scripts/sync-shared-copies.sh.
  • plugins/guardrails/hooks/run-guards.sh: new run_guards::verdict writes the marker (a builtin redirection, no fork) right before each deliberate exit: no guards, empty stdin, cut-short stdin, the substitution-cap block, and run_guards::finish. It skips --report processes and the forked rest-of-chain
  • plugins/guardrails/hooks/hooks.json: the five dispatcher rows (Write|Edit, the three GitHub MCP rows, Bash|PowerShell) pass --require-verdict after --fail-closed.
  • plugins/guardrails/hooks/exec-bash.test.sh: new --require-verdict cases. Covered: exit 0 with no marker denies with one stderr line naming the script; exit 0 with the marker allows, passes stdout through, and removes the marker directory; exit 2 still blocks; the --fail-closed SIGSEGV deny is kept;
  • plugins/guardrails/hooks/run-guards.test.sh: new verdict-marker section. The marker is written on allow, block, empty stdin, cut-short stdin, a guard's hard error, a deep exit whose rest forks, and the substitution cap. None is written for a dispatcher killed mid-chain, an abort in the dispatcher's
  • plugins/guardrails/hooks/require-jq-posture.test.sh: the Bash-row args are sliced after the run-guards.sh element instead of args[2:]. The old slice had already been passing the literal run-guards.sh path as a guard since --fail-closed was added.
  • plugins/guardrails/hooks/block-cross-domain.test.sh: same slice fix (was args[3:], which the new flag would have shifted).
  • plugins/guardrails/README.md: new Known-limits bullet on the verdict marker: what --fail-closed and --require-verdict each deny, and that a dispatcher abort is now denied.
  • .changes/<carrier>/feat-7681-require-verdict-<hex>.md (24 new files, one per carrier of lib/exec-bash.mjs): bump minor; guardrails' also records the Fixed behavior.

Verification

The runner ran each check from the base copy against 8297fd6:

  • scripts/check-changelog-fragments.sh --check: passed

The run reports:

  • .lane/checks/bin/check-changelog-fragments: all 59 fragments under .changes/ valid (run after the final edit).
  • .lane/checks/bin/check-affected: 10 passed, 6 skipped, 0 failed over 56 changed files (run after the final edit). An earlier run failed shellcheck SC2016 on a single-quoted printf in exec-bash.test.sh; fixed with a quoted heredoc.
  • .lane/verify/bin/sync-shared-copies: regenerated all 24 copies, exit 0 (re-run after the last lib/exec-bash.mjs edit).
  • .lane/verify/bin/cant-fail-scan --file plugins/guardrails/hooks/exec-bash.test.sh --check: PASS, no findings.
  • .lane/verify/bin/cant-fail-scan --file plugins/guardrails/hooks/run-guards.test.sh --check: PASS, no gating findings. One report-only source-text-read finding is at existing line 898, not in this change.
  • Fresh-context review subagent over the whole diff: no confirmed bugs. Two low-severity findings were fixed: the dispatcher's abort line said 'not checked' on a call the launcher now denies, and an inherited EXEC_BASH_VERDICT_FILE could reach a child of a launch without the flag.
  • The lane run: https://github.com/melodic-software/claude-code-plugins/actions/runs/40232573194

Left out: the items under Not verified.

Related

#7681

Lane-Activity: work-resolve-issue/resolve-issue
@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

claude-review needs no new review of dc00cde841fccdde92ed97d9097ebf8d8ef91b6c: no file of this pull request changed since the review of 37fd378.

Incremental review of plugins/disk-hygiene/skills/clean/scripts/test_owner_registry.py since 28fd36c. No findings.

The change raises the pinned deletion/spawn count for hooks/exec-bash.mjs in LAUNCHER_ACTIONS from 1 to 3 and updates the comment to match.

I checked the new count against the file. The test's ACTIONS regex matches child_process (line 94, the import), rmSync (line 95, the import) and rmSync (line 567, the verdict-marker directory cleanup). That is 3 matches, so the pin is exact. The test still fails if a fourth deletion or spawn verb is added to the launcher. The run-python-hook.sh pin of 2 also still matches what the regex finds in that file. I did not run the test suite, so this is a static check by reading the files.

No closing issue criteria, manifest changes or trusted-path files are involved in this increment. This change spends no billable resources, so there is no hold.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review needs no new review of dc00cde841fccdde92ed97d9097ebf8d8ef91b6c: no file of this pull request changed since the review of 37fd378.

No security findings in the incremental change.

The change since 28fd36c is in plugins/disk-hygiene/skills/clean/scripts/test_owner_registry.py. It raises the LAUNCHER_ACTIONS allowance for hooks/exec-bash.mjs from 1 to 3 and updates the comment. This is a test-only allowlist for a "no unexpected deletion calls in shipped code" check.

The bump matches the launcher's code. exec-bash.mjs has three lines matching the destructive-call regex: the child_process import, the rmSync import, and one rmSync call. That call removes only the per-launch mkdtempSync(tmpdir()/exec-bash-verdict-*) marker directory, on process exit. The path is not caller-controlled, so it can't delete anything else. The test guard still allows only the three named launcher files. No new file is allowlisted and the pattern is unchanged.

critical-findings: 0

Manifest items: none touched in this increment. No trusted-path file changed since the last review.

@melodic-automation-lanes

melodic-automation-lanes Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Unattended merge: native auto-merge leaves this pull request unarmed at head 37fd3780bcf98f7a46b21a93358e4d959f9171fb. The lanes clear this reason on their own; the next scheduled run judges it again.

pr-merge's gate refuses its verifier verdict (verify:missing)

kyle-sexton and others added 2 commits October 26, 2026 01:04
…owner registry

lib/exec-bash.mjs now removes the mkdtemp directory it makes for the
--require-verdict marker, which adds two rmSync matches to the vendored
disk-hygiene/hooks/exec-bash.mjs. Raise that file's pinned action count
from 1 to exactly 3, the registry's own route for widening a launcher.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kyle-sexton and others added 2 commits October 26, 2026 09:02
The --require-verdict launcher change imports rmSync and removes its
per-launch marker directory on exit, so the shipped exec-bash.mjs now names
child_process once and rmSync twice. Raise its pinned launcher-action count
from 1 to 3 so the owner-registry scan accepts that known cleanup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Both sides raised the exec-bash.mjs launcher-action count to 3; keep the
pushed comment wording.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@melodic-automation-lanes melodic-automation-lanes Bot added the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 26, 2026
@melodic-automation-lanes

Copy link
Copy Markdown
Contributor Author

The pr-unblock lane stopped on this pull request and needs a person: the lane could not fix every failing check; its run log names each one and why.

  • Reason: ci-fix-escalated
  • Resume key: 37fd3780bcf98f7a46b21a93358e4d959f9171fb (the head the lane stopped on)

To resume, deal with the reason above, remove the needs-human label (and do-not-merge, if set), then dispatch the pr-unblock workflow with pr-number=10120 and activity=fix-ci:

gh workflow run pr-unblock.yml -R melodic-software/claude-code-plugins -f pr-number=10120 -f activity=fix-ci

…sue-7681-40232573194-1

Brings in #10280's guardrails fix for the inherited test-windows-secrets
failure.

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

@kyle-sexton kyle-sexton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Owner decision 2026-10-26 ~22:3xZ in the hub session: merge the PRs currently held for the owner, security-flagged or otherwise. Approving this head (dc00cde) under that decision; the needs-human label is removed next.

@kyle-sexton kyle-sexton removed the needs-human Human-in-the-loop required; autonomous sessions must not resolve items carrying this. label Oct 27, 2026
@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 27, 2026
Merged via the queue into main with commit b73efa3 Oct 27, 2026
59 of 60 checks passed
@kyle-sexton
kyle-sexton deleted the work-resolve-issue/issue-7681-40232573194-1 branch October 27, 2026 02:35
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.

fix(guardrails): blocking hook rows fail open when the dispatcher process crashes

1 participant