fix(guardrails): stop the here-string deadlock that voided two blocking guards by kyle-sexton · Pull Request #2123 · melodic-software/claude-code-plugins

kyle-sexton · 2026-08-10T01:24:41Z

No linked issue

Summary

Two BLOCKING PreToolUse guards — secret-pattern-detection and hardcoded-path-check — returned
no verdict at all for a Write/Edit payload of 65536–65663 bytes inclusive. Not slow:
deadlocked. A live-shape AWS access-key id inside such a payload produced nothing; the same token in
a small payload exits 2. Both hooks are registered at timeout: 60, so the harness cancels the guard
and the verdict is lost — a fail-open reachable by any agent that controls the size of what it writes.

Same class as #1587, which fixed hook-utils.sh's JSON path and stopped at that one call site. This
PR sweeps the class instead of patching only the two reported files.

The defect

Bash delivers a here-string by filling a pipe itself, before the reader is exec'd, and it
appends a newline. A payload in that band puts the write 1–128 bytes past the 65536-byte pipe
capacity and bash blocks forever. At ≥129 bytes over, bash spills to a temp file and it works again —
so the window is closed on both sides, which is exactly why no ordinary size ever caught it.

Reproduced standalone, outside the plugin (bash 5.3.15(1), MINGW64):

C=$(head -c 65600 /dev/zero | tr '\0' b)
timeout 15 bash -c 'grep -qE "Users" <<<"$1"' _ "$C"; echo $?   # 124 (hung)
# 65535 -> 1   65536 -> 124   65600 -> 124   65663 -> 124   65664 -> 1

The while … done <<<"$var" shape hangs identically (rc 124 at 65536 / 65600 / 65663), which is what
pulled the command-scanning guards into scope.

Boundary measurements, through the real hooks

Payload piped to the hook on stdin — never <<<, which would hang the measurement itself. rc 124 =
killed at the bound, i.e. never answered. All numbers from this Windows host (Git Bash + Defender),
under concurrent agent load.

secret-pattern-detection, exact content bytes, AWS access-key id at the tail:

content bytes	case	BEFORE rc	BEFORE secs	AFTER rc	AFTER secs
65535	clean	0	10	0	19
65535	AWS key	2	43	2	63
65536	clean	124	202 (bound)	0	14
65536	AWS key	124	204 (bound)	2	51
65600	clean	124	151 (bound)	0	12
65600	AWS key	124	151 (bound)	2	67
65663	clean	124	156 (bound)	0	24
65663	AWS key	124	155 (bound)	2	41
65664	clean	0	20	0	19
65664	AWS key	2	44	2	59

The BEFORE hangs were taken at a 200-second bound first, then 150 — well past the legitimate slow
path (41–67 s) — so these are deadlocks, not slowness.

hardcoded-path-check, clean payloads. The pre-filter gate runs on every write, so the clean
column is the stronger claim: nothing in the window got a verdict, violating or not.

content bytes	BEFORE rc	BEFORE secs
65535	0	34
65536	124	152 (bound)
65600	124	151 (bound)
65663	124	207 (bound)
65664	0	44

Why not `printf … | grep -q` — the pipefail inversion is real

hardcoded-path-patterns.sh:73-75 carried a comment justifying the here-string, and the
justification was half right. Measured under set -o pipefail, with the match on line 1 so grep -q
can exit before the writer finishes (a single-line payload never reproduces this — grep must read it
all, so printf never gets SIGPIPE'd):

shape	`set +o pipefail`	`set -o pipefail`
`grep -qE pat <<<"$C"`	0	0 — but deadlocks in the window
`printf … \| grep -qE pat`	0	141 ← inversion
`printf … \| grep -E pat >/dev/null`	0	0
`grep -qE pat < <(printf …)`	0	0 ← chosen

Both hooks run under set -uo pipefail (set -e is off), so the inversion is live — and it is worse
than a wrong status, because both gate sites are written if ! grep -q …:

grep -q matches → exits 0 → SIGPIPEs printf → pipefail reports 141 → if ! 141 is
true → the gate early-returns clean. A fail-open on the very payload that contained the
secret.

Chosen idiom: process substitution. It keeps the writer outside the pipeline, so pipefail can
never see its SIGPIPE, while preserving the -q early exit the gate exists for — and it never
blocks. Verified at all six sizes in both the match and no-match directions.

Two shapes, chosen by whether the reader drains its input. The pattern lib previously
contradicted hook-utils.sh inside the same plugin — it told readers to PREFER a here-string
over printf | grep, while hook-utils.sh told them a whole payload must never go through <<<.
The lib now states the same rule and cites it:

reader drains (jq, grep without -q) → printf … | reader
reader may exit early (grep -q) → reader < <(printf …)

For while … done loops the substitution is < <(printf '%s\n' …). The \n is mandatory and makes
it byte-identical to the here-string it replaces (<<< appends a newline unconditionally), so no
loop can drop its final line.

Repo-wide sweep of `<<<` — every site, with a verdict

876 occurrences total; 329 outside *.test.sh. Fix criterion: the string can reach 65536–65663
bytes from an agent- or attacker-controlled source, AND a hang loses a security verdict.

Fixed (18 sites)

site	input	why
`guardrails/lib/path-detection/hardcoded-path-patterns.sh:76,83`	whole Write/Edit payload	reported; blocking
`guardrails/hooks/secret-pattern-detection.sh:158,207`	whole Write/Edit payload	reported; blocking
`guardrails/hooks/hardcoded-path-check.sh:219`	`$VIOLATIONS`	not in the original report. `$VIOLATIONS` embeds each MATCHED LINE verbatim, and the lib's `head -3` bounds the line COUNT, not bytes — so one 65KB minified line carrying a hardcoded path makes it payload-sized. It deadlocks on the blocked path, after the stderr message but before `exit 2`. Measured separately below
`guardrails/hooks/block-convention-violation.sh:132,158`	`$cmd` (Bash/PowerShell command)	blocking guard; loop shape hangs identically
`guardrails/hooks/block-hook-bypass.sh:248,497,566`	`$cmd`, `$NORMALIZED_SEGMENTS` (derived from `$COMMAND`)	blocking guard
`guardrails/hooks/flag-commit-pr-skill-bypass.sh:229`	`$cmd`	same command stripper
`guardrails/lib/powershell/ps-command.sh:143,671`	`$cmd`, `$norm`	shared lib behind the blocking PowerShell guards
`guardrails/hooks/workflow-resilience-check.sh:72,79`	`$SCRIPT` (inline Workflow script, or a `scriptPath` file read)	advisory, so no verdict is lost — but a hang wedges the Workflow call until the harness cancels
`source-control/hooks/pr-body-linkage-gate.sh:194`	`$text` (PR body)	blocking gate. GitHub caps a PR body at exactly 65536 characters — the documented maximum lands inside the hang window
`source-control/hooks/pr-linkage-validator.sh:76,113`	`$body`	same input, same cap

source-control is deliberately in scope rather than left as a half-fix; it costs the second plugin
bump in this PR.

Judged safe — no fix, with reason

site	reason
`guardrails/lib/verification/verify-cli-flag.sh:159,161` (`$HELP_OUTPUT`)	local CLI `--help` output; not attacker-influenced. A latent hang, not a security hole — noted, not fixed
`guardrails/hooks/cli-flag-verify.sh:188,337` (`read -ra` on `$seg`/`$chainstr`)	advisory PostToolUse; a single ≥64KB fragment is possible in principle, but no verdict is at stake. Latent hang, noted
`guardrails/hooks/flag-commit-pr-skill-bypass.sh:196` (`$keys`), `skill-reference-verify.sh:160` (`$declared`)	jq-derived plugin/settings key names; bounded by manifest size, not by any payload
`guardrails/hooks/block-no-verify.sh:102`	a `userConfig` option value (administrator-provided scalar)
`biome-format:211,237,251`, `ruff-format:250,275` (`$OUTPUT`)	formatter/linter output. Same mechanism, different consequence class — no security verdict at stake, only a wedged formatter. Recommended follow-up, kept out to keep this PR reviewable
`claude-ops-paths.sh:19,68`, `worktree-create.sh:373,412`, `babysit-readiness-gate.sh:286,293`, `check-plugin-manifest-presence.sh:75`	`IFS=… read -ra` splits of a path or a short CSV; cannot approach 64KB
`source-control/skills/pull-request/scripts/fetch-annotations.sh:177` (`$FILTERED`)	jq-filtered CI annotation records in a skill script, not a hook gate; no blocking verdict
`claude-config/skills/audit/scripts/`, `claude-ops/skills/lanes/scripts/`, `work-items` adapters	skill-invoked scripts over jq-bounded JSON, not a hook payload; no blocking verdict
~547 sites in `*.test.sh`	fixed small fixtures authored in-repo; reported as one class

lib/hook-utils.sh itself was already clean (#1587), and the stdin→CONTENT path in both hooks is
printf '%s' "$INPUT" | jq -r throughout — verified, because otherwise the payload would have hung
upstream and this fix would have changed nothing.

The `$VIOLATIONS` site, measured

A BEFORE run cannot reach line 219 through the hook (the gate deadlocks first), so it is measured
directly, and its reachability is confirmed on the patched hook:

$VIOLATIONS as hpp::scan_text builds it: the label, then "<lineno>:<line>" with the
matched line embedded VERBATIM, then the block terminator.  bytes = 65628

PRE-FIX   grep -E 'detected:$' <<<"$VIOLATIONS"    -> rc=124  (deadlock, 60s bound)
POST-FIX  grep -E 'detected:$' < <(printf '%s' …)  -> rc=0    (4s under load)

Tests

Neither suite had a single payload-size case before this (grep -n '65536\|head -c' returned
nothing). Added to both secret-pattern-detection.test.sh and hardcoded-path-check.test.sh:

clean payloads at 65535, 65536, 65600, 65663, 65664 — the window and both shoulders;
a real detectable secret / hardcoded path inside the window (65536 and 65600) that must exit 2;
the empty-content case, pinning that printf '%s' "" (zero bytes) matches the old <<<"" (one
empty line) in outcome;
a stderr assertion that the grep -q early-exit leaks no Broken pipe noise onto the hook's
user-facing channel.

Every case is bounded by timeout 150 so a regression fails loudly instead of hanging CI, and
asserts the exact expected code — 124 is reported as its own named failure. A "non-zero means
blocked" assertion would have accepted the hang and would not have caught this defect. The payload is
piped, never fed to the hook with <<<, which would hang the test itself at exactly these sizes.

In the path suite the detect payload separates the filler from the home path with a space: the
slash-rooted macOS/Linux bodies require a left boundary, so a path glued straight onto filler bytes
legitimately does not match — and the "must block" case would have passed for the wrong reason. That
was caught by a measurement script that omitted the space and returned 0 where 2 was expected.

Known follow-up (not fixed here)

On this Windows host the patched detect path measured 41–67 s against the timeout: 60
registration in hooks.json. So on Git Bash under Defender a large payload can still lose its verdict
to the harness — now by slowness rather than deadlock. The cost is process spawns, not matching: on a
hit, itemization runs 12 patterns × 5 processes. That is a separate defect with a separate fix
(batch the itemization), deliberately out of scope here, and stated rather than left for the next
auditor to "discover" as a half-fix.

Verification

shellcheck -x clean on all 17 changed files.
shfmt -d clean on every changed file. The pre-existing drift in the two .test.sh files is
unchanged by this PR — confirmed byte-identical at origin/main — and does not touch the added block.
scripts/check-shell-portability.sh --paths … — no unexcused GNU-only constructs in 27 shell files.
scripts/check-changelog-parity.sh --check-bump origin/main — passes.
scripts/sync-hook-utils.sh --check and --check-bump origin/main — pass.

lib/hook-utils.sh is left byte-identical to main on purpose. Its guidance was already correct
(only its upper bound was imprecise), and the sync gate requires a version bump plus a changelog entry
for all fourteen other plugins carrying the shared lib in exchange for a comment-only edit — churn
that would bury a security fix. The contradiction is resolved on the guardrails side, which is where
the wrong advice lived.

markdownlint-cli2 — 0 issues.
secret-pattern-detection.test.sh — PASS=52 FAIL=0
hardcoded-path-check.test.sh — PASS=94 FAIL=0

Version bumps are patch, deliberately. A reviewer may reach for minor on "payloads that were
allowed are now blocked". Nothing legitimate becomes refused that the guard did not already intend to
refuse — the fix restores the documented contract rather than widening it, which matches this repo's
practice (source-control 0.49.3 shipped a behavior-changing exec-bit-check fix at patch level;
the guardrails 0.21.0 minor was called out specifically for an acceptance change that could
refuse previously-allowed legitimate work).

Rider

The README hook table listed all six guards registered under the Bash|PowerShell matcher as
PreToolUse · Bash; no row named PowerShell at all. Verified row-by-row against hooks.json
(6 rows, 6 hooks, exact match) and corrected.

Refs fix(hook-utils): read hook stdin in chunks so a large payload is not blocked #1587 — fixed this same deadlock class in hook-utils.sh's JSON path and stopped at that one
call site; this PR sweeps the rest.
Refs fix(guardrails): stop blocking read-only python open() and cat > /dev/null #2007 — prior guardrails fix in the same review stream.

…wo blocking guards `secret-pattern-detection` and `hardcoded-path-check` returned NO VERDICT AT ALL — not slowly, ever — for a Write/Edit payload of 65536-65663 bytes. Bash delivers a here-string by filling a pipe ITSELF before the reader is exec'd, and appends a newline, so a payload in that band lands 1-128 bytes past the 65536-byte pipe capacity and blocks forever; at >=129 bytes over it spills to a temp file, which is why 65535 and 65664 always passed and only the band between them hung. Measured on Git Bash against the pre-fix hooks: a 65536-byte payload carrying a live-shape AWS access-key id returned nothing at a 200s bound, where the same token in a small payload exits 2 immediately. Both are registered at `timeout: 60`, so the harness cancels the guard and the verdict is lost. Every whole-payload `<<<` now feeds its reader through process substitution. `printf | grep -q` is NOT the alternative and the comment that justified the here-string was half right about why: `grep -q` early-exits and SIGPIPEs printf, so under the `set -uo pipefail` these hooks run with, the pipeline reports 141 and `if ! grep -q` reads that as "no match" — inverting a real detection into a fail-open. Process substitution keeps the writer out of the pipeline while preserving the early exit. Verified at every boundary size, both directions. Swept the same class beyond the four reported sites: the telemetry-label grep in hardcoded-path-check.sh (`$VIOLATIONS` embeds matched lines verbatim, and `head -3` bounds lines, not bytes), the command-scanning loops in block-convention-violation, block-hook-bypass, flag-commit-pr-skill-bypass and the PowerShell command lib, workflow-resilience-check's script scan, and source-control's PR-body linkage gate and validator — where GitHub's 65536-char body cap sits inside the hang window. Adds boundary regression cases at 65535/65536/65600/65663/65664 to both suites, including detectable secrets and paths inside the window that must exit 2; every case is bounded by `timeout` and asserts the exact code, with 124 as its own loud failure. Reconciles the contradictory here-string comments in hook-utils.sh and the pattern lib, and corrects the README's six `PreToolUse · Bash` rows whose registered matcher is `Bash|PowerShell`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…not a literal The eleven `done < <(printf '%s\n' …)` sites landed with a LITERAL newline inside the format string instead of the `\n` escape. `printf` emits the same bytes either way, so behavior and the passing suites were unaffected, but the source read as a two-line statement. Also records why these bumps are patch rather than minor: nothing legitimate becomes refused that the guards did not already intend to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…ring-deadlock # Conflicts: # plugins/guardrails/.claude-plugin/plugin.json # plugins/guardrails/CHANGELOG.md # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

cursor · 2026-08-10T01:24:46Z

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

claude · 2026-08-10T01:25:06Z

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

Review complete

Todo list

Gather context: read PR body, diff, and repo guidelines (CLAUDE.md / AGENTS.md)
Review lib/hook-utils.sh template change (mirrored to 17 plugin copies)
Review guardrails hook fixes (secret-pattern-detection, hardcoded-path-check, hardcoded-path-patterns, block-convention-violation, block-hook-bypass, flag-commit-pr-skill-bypass, ps-command, workflow-resilience-check)
Review source-control hook fixes (pr-body-linkage-gate, pr-linkage-validator)
Review test coverage additions
Review CHANGELOG/README/version bump correctness
Post inline comments for findings, then finalize summary

Summary

This is a well-executed, well-documented fix. I independently verified the core mechanics rather than taking the PR's narrative at face value:

hook::json_complete template: confirmed lib/hook-utils.sh is the documented single source of truth (SINGLE SOURCE OF TRUTH... Edit the source and run scripts/sync-hook-utils.sh; CI rejects drifted copies), and all 18 copies land on the identical post-fix SHA (79b56080) — the sync was done correctly, not hand-duplicated with drift.
README hook-table claim ("all six Bash|PowerShell-matcher guards were mislabeled PreToolUse · Bash"): I read plugins/guardrails/hooks/hooks.json directly and confirmed the matcher is exactly "Bash|PowerShell" for block-no-verify, block-dangerous-git, block-hook-bypass, flag-commit-pr-skill-bypass, block-noncanonical-commit, block-convention-violation — 6 hooks, 6 rows updated, no mismatch.
Sweep completeness: I grepped remaining <<< sites across plugins/guardrails/hooks, plugins/guardrails/lib, and plugins/source-control/hooks outside *.test.sh. Every remaining site (block-no-verify.sh:102, skill-reference-verify.sh:160, cli-flag-verify.sh:188,337, flag-commit-pr-skill-bypass.sh:196, verify-cli-flag.sh:159,161) matches the PR's own "judged safe" table exactly — no unfixed site was missed and no fixed site was unnecessary.
Deadlock-avoidance mechanism: process substitution forks the writer (printf) as a genuinely separate process that runs concurrently with the reader, so the single-process self-deadlock that afflicts <<< (bash fills the pipe itself, before the reader is exec'd) can't occur — this is architecturally sound, not just empirically patched.
while … done <<<"$x" → while … done < <(printf '%s\n' "$x"): verified this is behavior-preserving for the empty-input case too (<<<"" yields one empty line; printf '%s\n' "" also yields exactly one empty line via read), and process substitution avoids the same subshell-variable-loss pitfall the here-string already avoided (no regression there).
Test additions: the new boundary-case blocks in both .test.sh files call the shared guardrails-test-helpers.sh assertion functions (assert_exit, assert_contains, assert_absent) with correct argument order, and all fixture variables they depend on (LINUX_HOME, AWS_TOKEN, FIXTURE, TEST_TMPDIR) are defined earlier in each file. The exact-exit-code assertion (rejecting 124 explicitly rather than accepting "any non-zero") is the right design for a deadlock regression test.

I did not independently re-run the byte-boundary timing measurements (65535–65664, the 41–67s "known follow-up" figures, shellcheck -x/shfmt -d/check-shell-portability.sh/check-changelog-parity.sh results, or the PASS/FAIL test counts) — those are author-claimed and platform-specific (Windows/Git Bash/Defender); treat them as unverified by me.

Two minor, non-blocking findings posted inline:

secret-pattern-detection.sh:161-163 — the comment claims printf … | grep "is not the fix either" for check_pattern's grep -nE call, but that reader has no -q and always drains, so it can't trigger the SIGPIPE/pipefail inversion the comment describes (and this call's exit status isn't even inspected — only the captured value is). Not a bug (process substitution is still safe here), just a comment that overstates the risk versus the two-shapes rule the PR itself documents in hook-utils.sh. Same pattern recurs at hardcoded-path-check.sh:226.
pr-linkage-validator.sh:113 — this is the one converted <<< site in the whole PR missing the sibling # not <<<: a >=64KiB here-string deadlocks … inline comment that every other conversion (including the one 37 lines above it in the same file) carries.

No correctness bugs, security-lane-scoped items (left to the security lane per REVIEW.md), or missing test coverage found. Version bumps (both patch) and CHANGELOG entries are consistent with the diff and with the repo's stated patch-vs-minor precedent.
· Branch

claude · 2026-08-10T01:25:10Z

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

Security Review — PR #2123

Todo list

Gather context: gh pr diff for PR 2123
Read changed hook files (guardrails, source-control, hook-utils.sh copies)
Analyze for injection, auth/access-control gaps, secret handling, logic flaws in the here-string→process-substitution fix
Check plugin.json / CHANGELOG / README changes for anything security-relevant
Post findings as PR review

Scope

Reviewed all 34 changed files: the 15 duplicated hook-utils.sh copies (comment-only expansion, no code change — printf '%s' "$1" | jq -e . >/dev/null 2>&1 is byte-identical before/after), the 9 guardrails hook/lib files converting whole-payload <<< here-strings to < <(printf …) process substitution, the 2 source-control PR-linkage gate files with the same conversion, plus plugin.json/CHANGELOG.md/README.md metadata. No .github/workflows/* files are touched by this PR.

Analysis

This is a mechanical, well-reasoned fix for a real fail-open (blocking guards losing their verdict to a bash here-string deadlock in the 65536–65663 byte band). I traced the security-relevant properties of the fix rather than trusting the PR description:

< <(printf '%s' "$var") vs printf '%s' "$var" | reader — for every grep -q gate site (hardcoded-path-patterns.sh:76,83, secret-pattern-detection.sh:158,207, hardcoded-path-check.sh:219, workflow-resilience-check.sh:72,79), the writer (printf) is kept outside the pipeline via process substitution. Confirmed this is the correct fix for the pipefail inversion described in the PR: with printf | grep -q, grep -q's early exit SIGPIPEs printf, and under set -o pipefail the pipeline status becomes 141, which every affected gate reads via if ! grep -q … — turning a real match into a false "clean" exit. With process substitution, $? after grep -q < <(printf …) is grep's own exit status only; the substituted writer's SIGPIPE cannot reach it. Verified this by reading the actual if !-guarded call sites, not just the changelog claim.
hardcoded-path-check.sh:219 (labels_json=$(grep -E 'detected:$' < <(printf '%s' "$VIOLATIONS") …) || labels_json='[]') sits between the stderr block message and exit 2. Confirmed exit 2 is unconditional regardless of whether this grep succeeds — the || fallback only affects telemetry payload shape, not the blocking verdict, so there's no new fail-open surface here even in a partial-failure case.
No command/format-string injection introduced. Every new printf '%s\n' "$var" / printf '%s' "$var" uses a literal format string with the variable passed positionally — safe even if $var contains % or other format specifiers.
No subshell-scoping regression. The while … done <<<"$cmd" → while … done < <(printf …) conversions (in block-convention-violation.sh, block-hook-bypass.sh, flag-commit-pr-skill-bypass.sh, ps-command.sh, pr-body-linkage-gate.sh, pr-linkage-validator.sh) both use input redirection, not a cmd | while pipe — the loop body still runs in the current shell, so variable mutations inside the loop (result+=, out+=, array appends) are preserved exactly as before. This matters because a pipe-based rewrite here would have silently broken these detectors.
Coverage claim spot-checked. I grepped the current (post-fix) versions of every touched file for remaining <<< usage. The only survivor is flag-commit-pr-skill-bypass.sh:196 (done <<<"$keys"), which matches the PR's own "judged safe, not fixed" list (jq-derived plugin/settings key names, bounded by manifest size, not attacker-controlled payload) — the fixed/excluded-site accounting in the PR description is accurate against the actual diff, not just asserted.
Version/manifest changes (plugins/guardrails/.claude-plugin/plugin.json 0.22.0→0.22.1, plugins/source-control/.claude-plugin/plugin.json 0.51.0→0.51.1) are patch bumps with no permission, userConfig, or trust-boundary changes — plain version strings.
No secrets, tokens, or credentials are introduced or logged; the only "secret-shaped" strings are pre-existing test fixtures (e.g. an AWS-key-shaped token) used to assert the detector still fires inside the hang window, consistent with the existing test suite's pattern.

Findings

No security issues found. The fix correctly closes the fail-open described in the PR body without introducing injection, authorization, or logic-inversion regressions in the process-substitution rewrite. This PR touches no GitHub Actions workflows, so trigger/permission/supply-chain concerns are out of scope for this review (and are zizmor's lane per the review brief).

github-actions · 2026-08-10T01:31:30Z

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f3db590050

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

The comment-precision edit to lib/hook-utils.sh tripped the hook-utils-sync gate, which requires EVERY plugin carrying the shared lib to bump its version so consumers receive the change: fourteen more plugin bumps plus fourteen changelog entries, in exchange for a comment-only edit that would bury a security fix in churn. Its guidance was already correct (`printf | jq`, never `jq <<<`) — only its upper bound was imprecise. The contradiction this PR was asked to reconcile lived on the guardrails side, where the pattern lib told readers to PREFER a here-string; that lib now states the same rule as hook-utils.sh and cites it. `scripts/sync-hook-utils.sh --check` and `--check-bump origin/main` both pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude · 2026-08-10T01:42:10Z

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

Security Review — PR #2123 (re-run at HEAD `78c3fd0`)

Todo list

Gather context: gh pr diff for PR 2123 at HEAD 78c3fd0 (17 files, matches PR description)
Read changed hook/lib files (guardrails, source-control)
Analyze for injection, auth/access-control gaps, secret handling, logic flaws in the here-string→process-substitution fix
Check plugin.json / CHANGELOG / README changes for anything security-relevant
Post findings

Scope

Reviewed the full diff at the current HEAD (78c3fd0): 9 guardrails hook/lib files converting whole-payload <<< here-strings to < <(printf …) process substitution, 2 source-control PR-linkage files with the same conversion, plus plugin.json/CHANGELOG.md/README.md metadata and two .test.sh files. No .github/workflows/* files are touched. lib/hook-utils.sh and its 14 sibling copies are not part of this diff — the latest commit (78c3fd0) reverted an earlier comment-only edit to keep that shared file byte-identical to main, which also resolves the Codex bot's P1 finding (the sync-gate would otherwise have required 14 more version bumps).

Analysis

Independently traced the security-relevant properties rather than trusting the PR narrative:

< <(printf '%s' "$var") vs printf '%s' "$var" | reader — for every grep -q gate site (hardcoded-path-patterns.sh:76,83, secret-pattern-detection.sh:158,207, hardcoded-path-check.sh:219, workflow-resilience-check.sh:72,79), the writer (printf) is kept outside the pipeline. This is the correct fix for the described pipefail inversion: with printf | grep -q, grep -q's early exit SIGPIPEs printf, and under set -o pipefail the pipeline status becomes 141, which every affected gate reads via if ! grep -q … — turning a real match into a false "clean" exit. Process substitution's $? after grep -q < <(printf …) reflects only grep's own status; the substituted writer's SIGPIPE can't reach it.
No subshell-scoping regression in the while … done <<<"$x" → while … done < <(printf …) conversions (block-convention-violation.sh, block-hook-bypass.sh, flag-commit-pr-skill-bypass.sh, ps-command.sh, pr-body-linkage-gate.sh, pr-linkage-validator.sh): process substitution only forks the writer (printf) into a subshell — the while loop itself keeps running in the current shell, since this is input redirection (<), not a pipe (|). Variable/array mutations inside every loop body (result+=, out+=, lines+=) survive exactly as before. A pipe-based rewrite here would have silently broken these detectors by subshelling the loop; that mistake was not made.
No format-string or injection surface introduced. Every new call is printf '%s' "$var" / printf '%s\n' "$var" — a literal format string with the variable passed positionally, safe even if $var contains %, $(...), backticks, or other shell metacharacters (nothing here is eval'd or re-parsed as code).
hardcoded-path-check.sh:219-224 — the process-substitution grep for labels_json sits between the stderr block message and exit 2; exit 2 is unconditional (|| labels_json='[]' only affects telemetry payload shape), so there's no new fail-open on this path.
pipefail doesn't apply to process substitution — only to | pipelines — so this conversion doesn't reintroduce the exact defect it's fixing; correctly the design rationale given in the PR.
Coverage spot-check: grepped the post-fix versions of every touched file for remaining <<<; the only survivor (flag-commit-pr-skill-bypass.sh:196, done <<<"$keys") matches the PR's own "judged safe" list (jq-derived plugin/settings key names bounded by manifest size, not attacker-controlled payload).
Version/manifest changes (guardrails 0.22.0→0.22.1, source-control 0.51.0→0.51.1) are plain patch bumps — no permission, userConfig, or trust-boundary changes.
No secrets/tokens/credentials introduced or logged; the only "secret-shaped" strings are pre-existing AWS-key-shaped test fixtures used to assert the detector still fires inside the hang window.

Findings

No security issues found. The here-string→process-substitution rewrite correctly closes the described fail-open (blocking guards silently losing their verdict on a 65536–65663-byte payload) without introducing injection, authorization, or logic-inversion regressions, and the previously-flagged CI-breaking side effect on the shared hook-utils.sh sync gate has been resolved in the current HEAD. This PR touches no GitHub Actions workflows, so trigger/permission/supply-chain concerns remain out of scope for this lane (zizmor's advisory lane).
· Branch

claude · 2026-08-10T01:42:23Z

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

Review complete

Todo list

Gather context: gh pr diff, focus on the latest commit (78c3fd0) since the last review round
Review the hook-utils.sh revert (Codex P1 concern about the sync/version-bump gate)
Re-check the rest of the diff for correctness/architecture/error-handling/test-coverage/maintainability
Post inline comments for findings, then finalize summary

Summary

This synchronize event added one commit (78c3fd0) on top of the state the earlier review rounds saw. I verified it directly rather than trusting the commit message:

Codex's P1 finding is resolved. 78c3fd0 reverts the comment-only edit to lib/hook-utils.sh and all 17 plugin copies back to their exact origin/main content — I confirmed this with git diff origin/main..HEAD -- '**/hook-utils.sh', which returns empty, and gh pr diff now shows exactly the 17 files in the PR's own file list (no hook-utils.sh copies, no unrelated session-flow/hook-utils.test.sh drift from main's continued advancement). Since the shared lib is now byte-identical to main, scripts/sync-hook-utils.sh --check-bump has nothing to flag, and the fourteen otherwise-required carrier-plugin version bumps are correctly avoided.

Reviewing the current full diff for correctness/maintainability, I found one new issue (not previously flagged) and confirmed two previously-flagged nits are still open:

New finding — filed as inline comments:

plugins/guardrails/CHANGELOG.md:49-50 and plugins/source-control/CHANGELOG.md:17-18 both embed a literal raw newline inside an inline-code span (`< <(printf '%s\n ` on one line, `' …)` on the next) instead of the escaped \n text. This is the exact same defect commit 48d2e52 fixed in the shell scripts themselves ("the loop substitution's newline as an escape, not a literal") — it just wasn't caught in the CHANGELOG prose, and none of the later commits touched these lines. Per CommonMark, a raw line break inside a code span renders as a space, so both spans will visibly render as printf '%s ' … — silently dropping the \n from the sentence that is specifically explaining why the \n is required for byte-identical here-string behavior.

Previously-flagged, still unaddressed (not re-filed, to avoid duplicate threads):

secret-pattern-detection.sh:163 and the analogous hardcoded-path-check.sh:226 — the "printf … | grep is not the fix either" comment still overstates the risk for these non--q, fully-draining grep calls.
pr-linkage-validator.sh:113 still lacks the sibling # not <<<: … comment that every other conversion site in the PR carries.

Everything else re-checked against this HEAD (process-substitution correctness, patch-vs-minor version bump reasoning, README table accuracy, <<< sweep completeness) still holds as verified in the prior review round — no regressions from 78c3fd0. I did not independently re-run the author-claimed timing/shellcheck/shfmt/test-count figures in the PR body; those remain unverified-by-me.
· Branch

kyle-sexton · 2026-08-10T01:56:29Z

Suite evidence

CI is authoritative and covers every affected suite. plugin-gate runs
scripts/run-plugin-tests.sh, which discovers every plugins/**/*.test.sh plus
.claude/hooks/*.test.sh — so all eight suites covering files this branch touches ran there, on
Linux, including the new boundary cases.

All 33 checks pass, including the three required ones:

check	result
`pr-title`	pass
`do-not-merge`	pass
`ci-status`	pass
`plugin-gate` (every `plugins/*/.test.sh`)	pass, 5m19s
`hook-utils-sync`	pass
`hook-utils-windows`	pass
`changelog-parity-gate`	pass
`pr-issue-linkage`	pass
`shell-portability-lint`, `portability-lint`, `silent-skip-gate`, `cross-plugin-source-drift`	pass

hook-utils-sync failed on the first push and drove a deliberate scope decision: the gate requires
every plugin carrying the shared lib to bump when it changes, so a comment-only edit to
lib/hook-utils.sh would have cost fourteen extra plugin bumps and fourteen changelog entries. The
lib is now byte-identical to main and the reconciliation lives on the guardrails side, where the
incorrect advice actually was. See the third commit.

Local runs (Git Bash on Windows — the host where the deadlock reproduces)

Recorded because CI runs Linux, and the defect was found on Windows:

suite	result
`secret-pattern-detection.test.sh`	PASS=52 FAIL=0 (10 new boundary assertions)
`hardcoded-path-check.test.sh`	PASS=94 FAIL=0 (10 new boundary assertions)
`block-convention-violation.test.sh`	PASS=31 FAIL=0
`block-hook-bypass.test.sh`	50 assertions, 0 failures at the point the local run was stopped as redundant with `plugin-gate`

All 20 new boundary assertions pass on both hosts:

ok: boundary: clean 65535-byte payload → exit 0
ok: boundary: clean 65536-byte payload → exit 0
ok: boundary: clean 65600-byte payload → exit 0
ok: boundary: clean 65663-byte payload → exit 0
ok: boundary: clean 65664-byte payload → exit 0
ok: boundary: AWS key in 65536-byte payload → exit 2      (path suite: linux home path)
ok: boundary: AWS key in 65600-byte payload → exit 2      (path suite: linux home path)
ok: boundary: in-window block still reports the label
ok: boundary: no SIGPIPE noise on stderr
ok: boundary: empty content → exit 0

Every one of these is bounded by timeout 150 and asserts an exact exit code, with 124 reported as
its own named failure — so a reintroduced deadlock fails loudly rather than hanging the job.

… comment Review findings on #2123, all four verified against the source first. Both CHANGELOGs embedded a LITERAL newline inside an inline-code span rather than the two-character `\n`. CommonMark converts a line ending inside a code span to a space, so `< <(printf '%s\n' …)` rendered as `< <(printf '%s ' …)` — dropping the escape from view in the very sentence explaining why it is what makes the substitution byte-identical to the here-string. Same defect class as the shell one this PR already fixed, from the same authoring mistake. pr-linkage-validator.sh:113 was the only converted site missing the trailing `# not <<<` note, in exactly the file where a future edit might revert to `<<<"$body"` for readability. secret-pattern-detection.sh's comment claimed `printf … | grep` "is not the fix either" at a site whose reader takes no `-q`, drains its input, and has its pipeline status discarded — where a plain pipe WOULD have been correct. Measured at 65600 bytes: that same `grep -nE` hangs on a here-string exactly as `grep -q` does, so the deadlock is a property of `<<<` alone and the conversion was required regardless; only the CHOICE of replacement depends on the reader. The comment now says that, and names uniformity with the `-q` gates as the reason process substitution is used here. No behavior change: three comment/prose edits and one comment line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

claude · 2026-08-10T02:38:25Z

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

Code Review — PR #2123 (re-run at HEAD `2895890`)

Todo list

Gather context: diffed the fixup commit 2895890 against its parent and against the PR's file list
Verify the latest commit (2895890) actually fixes the two CHANGELOG newline findings and the overstated comment finding
Re-check rest of diff for correctness/architecture/error-handling/test-coverage/maintainability
Post inline comments for any new findings, then finalize summary

Summary

This synchronize event added one commit, 2895890, which is a direct, verified response to four review findings raised in earlier rounds on this PR (two from the code-review lane, two from Codex). I read the current file state rather than trusting the commit message:

plugins/guardrails/CHANGELOG.md:49 and plugins/source-control/CHANGELOG.md:17 — both previously had a literal newline embedded inside an inline-code span (`< <(printf '%s\n ` ending one line, `' …)` ` starting the next), which CommonMark renders as a space, silently dropping the \n from view. Both are now single lines reading `< <(printf '%s\n' …)` / `< <(printf '%s\n' "$body")`. Confirmed by reading the rendered file content directly — guardrails/CHANGELOG.md#L49, source-control/CHANGELOG.md#L17.
plugins/source-control/hooks/pr-linkage-validator.sh:113 — was the one converted <<< → process-substitution site missing the sibling # not <<<: a >=64KiB here-string deadlocks (see hardcoded-path-patterns.sh) comment that every other conversion site carries. Now present, matching the pattern used elsewhere in the same file (strip_html_comments, 37 lines up) and across the PR.
plugins/guardrails/hooks/secret-pattern-detection.sh:156-173 — the comment previously overstated the risk, claiming printf … | grep "is not the fix either" at a call site (check_pattern's grep -nE, no -q) that actually drains its input and never inspects the pipeline's exit status, so a plain pipe would in fact have been safe there. The rewritten comment now separates the two claims correctly: the here-string deadlock is unconditional regardless of reader shape (and is empirically confirmed at 65600 bytes for this exact non--q grep), while process substitution here is a deliberate uniformity choice with the plugin's -q gates, not a correctness requirement. This is accurate and resolves the finding without overclaiming in the other direction.

I also re-verified the diff is scoped to exactly the 17 files listed for this PR (via gh pr diff 2123, matched file-for-file against the PR's changed-files list) and re-read the full text of both CHANGELOGs — no other broken code spans or unclosed backtick spans survive in the added content. The fixup commit is comment/prose-only (confirmed via git show 2895890 — only #-prefixed lines and one added comment line change in the two .sh files; the one existing code line in secret-pattern-detection.sh is untouched), so there's no new logic to verify and no risk of a shellcheck/shfmt regression from this commit.

No new findings. All four previously-flagged items are correctly resolved, and I found nothing else to flag in this round.
· Branch

claude · 2026-08-10T02:38:26Z

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

Security Review — PR #2123 (re-run at HEAD `2895890`)

Todo list

Gather context: gh pr diff at current HEAD (2895890), 17 files, matches PR description
Read changed hook/lib files (guardrails, source-control)
Analyze for injection, auth/access-control gaps, secret handling, logic flaws in the here-string→process-substitution fix
Check plugin.json / CHANGELOG / README changes for anything security-relevant
Post findings

Scope

Diffed origin/main...HEAD via gh pr diff (shallow clone locally has no merge-base with origin/main, so gh was used instead of git diff). The diff at 2895890 is unchanged in substance from the prior two security-review passes on this PR (f3db590, 78c3fd0): 9 guardrails hook/lib files converting whole-payload <<< here-strings to < <(printf …) process substitution, 2 source-control PR-linkage files with the same conversion, plus plugin.json/CHANGELOG.md/README.md metadata and 2 .test.sh files. lib/hook-utils.sh and its plugin copies remain outside the diff (byte-identical to main, as established in the prior round). No .github/workflows/* files are touched.

The one new commit since the last security-review pass (2895890) is doc/comment-only — confirmed via git show --stat 2895890: 2 CHANGELOG files (fixing a literal-newline-in-code-span rendering defect) and 2 shell files (secret-pattern-detection.sh comment correction, pr-linkage-validator.sh adding a missing # not <<< comment). No executable line changed; the process-substitution logic itself is identical to what was already reviewed.

Analysis

Re-verified the security-relevant properties directly against source rather than trusting the commit message:

< <(printf '%s' "$var") vs printf '%s' "$var" | reader — for every grep -q gate site (hardcoded-path-patterns.sh:494,523, secret-pattern-detection.sh:351, hardcoded-path-check.sh:199, workflow-resilience-check.sh:471,479), the writer (printf) is kept outside the pipeline. This is the correct fix for the pipefail inversion described in the PR: with printf | grep -q, grep -q's early exit SIGPIPEs printf, and under set -o pipefail the pipeline status becomes 141, which every affected gate reads via if ! grep -q … — turning a real match into a false "clean" exit. Process substitution's $? after grep -q < <(printf …) reflects only grep's own status; the substituted writer's SIGPIPE can't reach it, and pipefail doesn't apply to process substitution in the first place (only to | pipelines).
No subshell-scoping regression in the while … done <<<"$x" → while … done < <(printf …) conversions (block-convention-violation.sh, block-hook-bypass.sh, flag-commit-pr-skill-bypass.sh, ps-command.sh, pr-body-linkage-gate.sh, pr-linkage-validator.sh): this is input redirection (<), not a pipe (|), so the loop body still runs in the current shell and variable/array mutations (result+=, out+=, lines+=) survive exactly as before. A pipe-based rewrite would have silently broken these detectors by subshelling the loop; that mistake wasn't made.
No injection surface introduced. Every new call is printf '%s' "$var" / printf '%s\n' "$var" — a literal format string with the variable passed positionally, safe even if $var contains %, $(...), backticks, or other shell metacharacters (nothing is eval'd or re-parsed as code).
hardcoded-path-check.sh:199-201 — the process-substitution grep for labels_json sits between the stderr block message and exit 2; exit 2 is unconditional (|| labels_json='[]' only affects telemetry payload shape), so no new fail-open on this path.
Coverage spot-check: grepped the post-fix files for remaining <<<; the only survivor (flag-commit-pr-skill-bypass.sh:196, done <<<"$keys") matches the PR's own "judged safe" list (jq-derived plugin/settings key names bounded by manifest size, not attacker-controlled payload).
Version/manifest changes (guardrails 0.22.0→0.22.1, source-control 0.51.0→0.51.1) are plain patch bumps — no permissions, userConfig, or trust-boundary changes.
The final commit's CHANGELOG rewording and comment corrections are prose-only; they don't touch any conditional, pattern, or exit-code logic, so they carry no security implication of their own.
No secrets/tokens/credentials introduced or logged; the only "secret-shaped" strings are pre-existing AWS-key-shaped test fixtures used to assert the detector still fires inside the hang window.

Findings

No security issues found. This PR closes a real fail-open (blocking guards silently losing their verdict on a 65536–65663-byte payload) via a here-string→process-substitution rewrite that is architecturally sound: it preserves early-exit semantics for grep -q gates without exposing pipefail to the writer's SIGPIPE, and preserves current-shell variable scoping for the while loops. No injection, authorization, or logic-inversion regressions were introduced, and the doc-only commit since the last review pass doesn't change any of that analysis. This PR touches no GitHub Actions workflows, so trigger/permission/supply-chain concerns remain out of scope for this lane (zizmor's advisory lane).
· Branch

…ing guards (#2135) Closes #2122 ## Update — `main` moved under this PR, and the disposition changed with it **#2120 merged (`fd075c27`), and it fixed the same function with the opposite value disposition: it STRIPS every NUL out of a value where this branch TRUNCATED at the first one.** The PR went `DIRTY`. Resolved by merging `origin/main` into the branch — never a rebase, since force-push is blocked here twice over. **The resolution keeps `main`'s strip and this branch's flag plus fail-closed guards.** That is additive over `main` rather than a choice between the two sides, and it is what this body already argued for in its own words: the disposition is *immaterial for this PR's own two callers*, which refuse on the flag before reading a value, while `main` now carries the ten scanner-class callers #2120 converted, none of which consults the flag. Truncating would have hidden a credential placed after a NUL from `secret-pattern-detection` and `hardcoded-path-check`. **Everything below that says "truncate" describes the pre-merge branch; the shipped behaviour is strip + flag.** ### The textual merge git produced was silently fatal, and was not taken git auto-merged the function body into a hybrid carrying BOTH `main`'s per-filter `split("\u0000") | join("")` **and** this branch's array-level `explode | .[0:(index(0) // length)] | implode`. Strip runs first, so `index(0)` looked at a value with no NUL left in it and **the flag read `0` on every payload** — the guards would never have fired, with no conflict marker and no test of the pre-merge branch able to see it. The flag is now computed from the untouched values with the strip applied after, and both the library and the guard comments say the ordering is load-bearing, because it is exactly what the next textual merge will get wrong again. ### Why the flag and the guards are still needed after #2120 #2120 closed the fail-open for the CONTENT guards. It did not close the COMMAND guards: stripping SPLICES the bytes either side of the NUL into a token the payload never carried contiguously, and the guards then match against that token. Re-measured at the hook boundary, `origin/main` at `fd075c27` versus this tree, same script, same host, on fixtures whose NUL is a real byte — verified by decoding each fixture and counting the byte (`jq -j .tool_input.command | tr -dc '\u0000' | wc -c` = 1) rather than trusting that the escape survived construction: | payload | `main` | this change | | --- | --- | --- | | `git commit --no-verify<NUL>x` | **0 ALLOWED** | **2 blocked** | | `git push --force<NUL>x` | **0 ALLOWED** | **2 blocked** | | a lone NUL | **0 ALLOWED** | **2 blocked** | | a trailing NUL | **0 ALLOWED** | **2 blocked** | | `git commit --no-veri<NUL>fy` | 2 blocked | 2 blocked | | clean `--no-verify` | 2 | 2 | | clean `--force` | 2 | 2 | | harmless (`git status`) | 0 | 0 | Identical on both guards. **The fifth row is stated, not counted:** the splice happens to reassemble a real `--no-verify` there, so `main` already blocks it and it evidences nothing about this change. The live rows are the first four, and the first two are the ones that matter — a real `--no-verify` and a real `--force` that `main` waves through. No clean command changed verdict in either direction. ### Tests re-pointed rather than deleted Every assertion this branch wrote against truncation was measuring a value the helper no longer produces, so each was rewritten for strip and two new cases were added: the splice (`--no-verify<NUL>x` -> the single token `--no-verifyx`), and an ALL-NUL value, which strips to empty — that case, and not a leading NUL, is the real reason both guards consult the flag ahead of their empty-command skip. The guard suites keep every NUL row at exit 2; the verdict never depended on the disposition, only its justification did, and one mislabelled row was corrected accordingly. ### Conflicts and versions - `lib/hook-utils.sh` — header comment and jq program, resolved by hand. - The 16 vendored copies were **regenerated with `scripts/sync-hook-utils.sh`**, not hand-resolved; `--check` reports 16/16 byte-identical. - 16 CHANGELOGs where both sides claimed the same version: this branch's entry moves up one patch above `main`'s and is rewritten for the resolved design. - **All 16 `plugin.json` files had auto-merged to `main`'s number, leaving no bump at all** — no conflict, only `--check-bump` catches it, exactly the trap flagged below. Re-bumped: `guardrails 0.23.1 -> 0.23.2`, `markdown-format 0.11.2 -> 0.11.3`, `source-control 0.51.2 -> 0.51.3`, patch bumps for the other 13. - **Coordination with #2130:** it also bumps `markdown-format` to `0.11.3`. Whichever merges second must re-bump. ### `main` moved twice more: three merges, and one of them was silently lossy `main` landed #2147, then #2140 and #2149, while this PR sat. Three merge passes, no rebase at any point. Second pass: #2147 took `guardrails` to `0.24.0` and edited `block-dangerous-git.sh`, which this branch also edits — resolved by keeping main's three-field `hook::jq_fields "$INPUT" '.tool_input.command' '.cwd' '.tool_name'` call verbatim and appending this branch's NUL block after it. Third pass: one changelog conflict on `source-control`. Every plugin manifest had auto-merged to main's number with no bump on **both** passes. **The second pass exposed a defect this branch had introduced, and it is worth reading even if you skip the rest.** An earlier commit here accidentally wrote a **real NUL byte** into `plugins/guardrails/CHANGELOG.md` — a `\u0000` that was meant to be literal text in a prose description of the fixtures. git classifies any file containing a NUL as **binary**, so the textual three-way merge never ran on that changelog: it kept ours wholesale and **silently discarded main's entire `0.24.0` section**, with no conflict marker and nothing in `git status` to distinguish it from a file that merged cleanly. It was caught by counting NUL bytes across the touched files, not by reading the diff. The byte is gone, the section is restored, and the changelog's `0.24.1` entry now sits above main's `0.24.0`. That is a mistake this PR made, not a pre-existing one, and it is reported rather than quietly fixed because the failure mode generalises: **a NUL in a tracked text file turns every future merge of that file into a silent take-ours.** In a repository whose CHANGELOGs are the merge-conflict surface for every shared-library change, that is worth knowing independently of this fix. ### Incidental, and relevant to the "what I could NOT verify" list below While posting a review reply, the **harness itself refused a tool call** whose `command` field carried a stray control character, with `command contains control characters that would be hidden in the approval dialog`. That is a live observation of the validation the list below names as unverified — it fires, and it fires on the `command` field. It is **not** the discriminating probe: it says nothing about whether that validation runs before or after PreToolUse hooks, and nothing about whether the rejected class includes NUL specifically rather than the control characters it does cover. Recorded as an observation, not as evidence that the guards are unreachable. Nothing in this change leans on it in either direction. ### Gates re-run after the merge `sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump origin/main` - `check-changelog-parity.sh --check` / `--check-bump origin/main` / `--check-order` - `shellcheck -x` with **no severity floor** on `lib/hook-utils.sh`, the `bash-format` vendored copy, both guards and all three test files (rc 0 — this is what the two open review threads reported failing; the jq-variable spelling they flagged is gone from the current program text) - `shfmt -d -i 2` (rc 0). Suite results after the merge are in the thread below. ## The defect `hook::jq_fields` frames its fields with a NUL delimiter drawn from the same byte space as the values it separates. A JSON NUL escape inside a value splits that value in two, the cardinality check `((${#values[@]} == $#)) || return 1` fires, and both real callers spell that `|| exit 0` — a PreToolUse **ALLOW**, emitted with no diagnostic of any kind. One correction to the issue's mechanism, because it moves where the fix belongs. The collision is **reliably detected**, not intermittently: every NUL adds exactly one record, so the count is always `N + k` for `k >= 1` and the check never misses. The defect therefore never lived in the library's return value. It lives in **one exit path serving two conditions with opposite correct responses** — "jq is absent or cannot parse this" (where allowing is the documented, deliberate behaviour) and "this payload carries a NUL" (where allowing is wrong). Separating those two is the fix. ## Design **jq truncates each value at its first NUL and reports the fact; the caller owns the verdict.** - `lib/hook-utils.sh` — each filter becomes `... | explode | .[0:(index(0) // length)] | implode`. The separator then cannot occur inside a value, so the record count no longer depends on what a parseable payload holds. - A leading record carries the NUL flag, computed from the untruncated values and emitted by the **same** jq program, so reporting it costs no second spawn. It surfaces as `HOOK_JQ_FIELDS_NUL`, assigned in the same unconditional block that resets `HOOK_JQ_FIELDS` — above all three return paths, so no early return can leak a stale `1`, which in a guard would mean blocking a clean payload on the strength of an earlier one. - `block-no-verify.sh` and `block-dangerous-git.sh` fail **CLOSED** on that flag, **before** their empty-command skip, because the helper truncates at the first NUL and a leading one therefore leaves an empty value that would otherwise be waved through as "no command". ### Why fail CLOSED, and why that argument does not depend on the executor **No executor-fidelity claim is made here, in either direction.** Two behaviours were measured and they disagree, and which of them a hook payload actually reaches has **not been traced by anyone**: | measured | result | | --- | --- | | bash parsing a command it reads (stdin, script file) | **discards** the NUL — `echo ha<NUL>rd` prints `hard`, and `--no-verify<NUL>x` becomes `--no-verifyx` | | a NUL inside an argv word handed to `execve` | the string simply ends there | | Node v24.18.0 `child_process` — argv, `shell: true`, and `execSync` | **refuses** outright, `ERR_INVALID_ARG_VALUE: must be a string without null bytes`, while the same calls with a clean string run normally | An earlier draft of this PR argued that truncation was right *because the executor truncates*. That was wrong — it generalised the argv case to a path that is not known to be the one in use. **The correct argument is that the design does not need it:** failing closed on the flag is correct under deletion, under truncation, and under refusal alike, so it cannot be invalidated by tracing the path later. That is the whole case for it. Matching the value would need the trace; refusing does not. ### Truncate rather than delete, on grounds that appeal to no shell Truncation never fabricates a token the payload did not carry contiguously, and when a caller forgets the flag it is the *content* class that degrades rather than the command class — a matcher sees a prefix rather than a joined token that matches nothing. **For this PR's own two callers the choice is immaterial: they refuse on the flag before reading a value at all.** It is the conservative default, not the accurate one, and the flag is the load-bearing part. ### Why the library does not block on its own It is sourced by 15 other plugins, formatters among them, for which exiting 2 would be wrong; and a sourced library calling `exit` on its caller's behalf is hidden control flow. Policy stays with the caller and the library only reports the fact. ### Rejected alternatives | Alternative | Why not | | --- | --- | | Delete the NUL (`map(select(. != 0))`) | Fabricates contiguity the payload did not have, and inverts which caller class degrades unsafely when a hook forgets the flag; see above. Not rejected on executor grounds. | | `gsub` / `split`+`join` on a NUL | Both work on jq 1.8.2 here, but each puts a NUL inside the jq **program** text — a regex pattern and a string literal. A construct whose behaviour varied across jq builds would fail EVERY payload: a universal fail-open, strictly worse than the payload-dependent one. `explode`/`implode` use integer comparison only, with no NUL anywhere in the program. This is a reason, not a measurement — see the unverified list. | | Length-prefixed framing | Needs `read -N` (bash 4.1+); this lib supports 3.2+. | | An explicit emitted count | Redundant once the separator is absent from the value space. | | Per-field `@base64` | Needs a `base64` binary; only `jq` is a documented prerequisite. | | `@sh` + `eval` | Puts payload-derived text through `eval`. | | Fail closed inside the library | Impossible without the library exiting on its caller's behalf, which is wrong for the 15 other plugins. | ## Scope **This is a shared-library change, and the repo's own gate makes it 55 files.** `plugins/guardrails/hooks/hook-utils.sh` is a **vendored copy**; `lib/hook-utils.sh` is the source of truth. CI enforces `scripts/sync-hook-utils.sh --check` (all 16 copies byte-identical) and `--check-bump` (every carrying plugin bumped when the lib changes), so editing only the guardrails copy would fail CI. Precedent: 9b90e35, 50 files. Hence 16 vendored copies, 16 `plugin.json` bumps and 16 changelog entries, plus the lib, its test, the two guards, their two test files and the guardrails README. **`hook::jq_field` — SINGULAR — is untouched.** It is a separate two-line function; there is no shared internal the two route through. `grep -rn "hook::jq_field " --include=*.sh plugins/`, with the vendored copies excluded, finds **22 call sites across 12 files** in `claude-ops`, `context-guard` and `source-control`. None of them are touched. `git diff origin/main -- lib/hook-utils.sh` mentions `hook::jq_field` on exactly two lines, both of them the same doc-comment cross-reference inside the *plural* function's header ("Values are CR-stripped, as in `hook::jq_field`"); the singular function's own body appears nowhere in the diff. **Blast radius is exactly the two guards.** **No other plugin is affected by the truncation.** `grep -rn "hook::jq_fields" --include=*.sh .`, excluding the 16 vendored copies and `lib/hook-utils.*`, returns exactly two call sites — both in this PR. Every other hit across the 16 plugins is the doc comment in the vendored library. Nothing round-trips a value into a file, and nothing compares a length or hash against one. **Versions**, taken against `origin/main` at the time of the last rebase: `guardrails 0.23.0 -> 0.23.1`, `markdown-format 0.11.1 -> 0.11.2`, `source-control 0.51.1 -> 0.51.2`, and plain patch bumps for the other 13. Worth flagging for anyone rebasing a sibling branch: when a plugin's version moved on `main` mid-flight, `git` **auto-merged the manifest to main's number**, silently leaving no bump at all — no conflict, and only `sync-hook-utils.sh --check-bump` catches it. That happened three times here. #2120 is still open against the same guardrails files and owes a re-bump. ## Two caller classes want opposite dispositions — which is why there is a flag This is the strongest argument for the design, and it is demonstrated rather than theoretical. #2120 has independently fixed the same function with the **opposite** disposition: at its head `9fb8383d`, `hook::jq_fields` does `... | tostring | split("<NUL>") | join("")` — it **strips**. Neither disposition is simply right, because the two caller classes disagree: | payload | under strip | under truncate | | --- | --- | --- | | `content: harmless<NUL>aws_secret=AKIA…` (a scanner) | secret is joined and **scanned** | secret is cut off and **invisible** | | `command: --no-verify<NUL>x` (a guard) | joins to `--no-verifyx`, matches nothing, **allowed** | leaves `--no-verify`, **blocked** | (Which of those two readings the executor would agree with is untraced, and is not the argument — see above. The point is only that a caller ignoring the flag degrades unsafely in one class or the other, depending which disposition the helper picks.) Both halves measured. The command half is the boundary table below. The content half I measured by driving the helper directly, since no shipped hook reads `.tool_input.content` through it on `main`: ``` payload: .tool_input.content = "harmless preamble<NUL>aws_secret=AKIA…" this branch (truncate) rc=0 flag=1 value=[harmless preamble] credential NOT visible 468bb2d (base) rc=1 flag=- value=[<none>] credential NOT visible ``` **So yes — truncation loses post-NUL content for a scanning caller.** Stated plainly because it is a real consequence of this design. It is not a regression (the base loses it too, and additionally allows), and truncation is still the chosen default: it keeps the *command* class safe when a caller ignores the flag, where strip keeps the *content* class safe instead. Strip inverts which class fails unsafely; it does not remove the failure. Neither is chosen on executor grounds. **A single disposition cannot serve both callers. The flag is what resolves it** — the helper reports, and each caller decides: a command guard refuses outright, a content scanner refuses the write rather than scanning a value it knows is incomplete. Either way the credential never lands. ### The count, measured on `9fb8383d` **Every one of the ten hooks #2120 converts calls `hook::jq_fields`. Zero of them consult any NUL signal. Six own an `exit 2` verdict:** | hook | `jq_fields` calls | flag checks | `exit 2` paths | | --- | --- | --- | --- | | `secret-pattern-detection` | 2 | **0** | 2 | | `hardcoded-path-check` | 2 | **0** | 2 | | `block-convention-violation` | 2 | **0** | 3 | | `block-hook-bypass` | 2 | **0** | 2 | | `block-noncanonical-commit` | 2 | **0** | 5 | | `cli-flag-verify` | 2 | **0** | 1 | | `skill-reference-verify` | 3 | **0** | 0 | | `stale-path-verify` | 3 | **0** | 0 | | `flag-commit-pr-skill-bypass` | 2 | **0** | 0 | | `workflow-resilience-check` | 2 | **0** | 0 | Zero flag checks is expected — the flag does not exist on their branch. The point is what it implies for whichever of us merges second: **merge order does not rescue it.** This PR first, then their rebase, and the scanning hooks receive truncated values with no flag check. Theirs first, then this one, and the same is true the moment strip becomes truncate. **A reader must not conclude that this PR makes that conversion safe. It does not.** Adding the flag checks to those ten hooks is a prerequisite for the conversion, not a follow-up — and it is theirs to do, since those hooks exist in converted form only on their branch. This PR deliberately does not touch them. `hardcoded-path-check.sh` is a **third** caller class worth calling out: it reads `.tool_input.content`, `.new_string` and `.new_source` **and** owns two `exit 2` paths, so it is both scanner and guard. Per-field reachability was checked separately and holds: at their head, both `secret-pattern-detection.sh` and `hardcoded-path-check.sh` reach `exit 2` through `.content` and through `.new_string`. (`hardcoded-path-check.sh` returns early unless `CLAUDE_PROJECT_DIR` is set, so a probe without it exits 0 on every payload and looks exactly like "not reachable".) #2123 needs nothing — its diff introduces zero `hook::jq_fields` call sites. **Merge coordination:** #2120 now also edits `lib/hook-utils.sh`, so this is a direct conflict on the same function rather than only on the manifest and changelog. Whoever merges second must **keep both correctness properties** — the flag and the fail-closed guards from here, and the scanning-caller requirement from there — rather than resolving by taking one side of the hunk. ## Evidence ### Hook boundary, before and after Real hooks, payload piped on stdin, exit code read. BEFORE is a `git archive` of `origin/main` at `468bb2d9` — re-measured after #2123 merged, because #2123 changed `plugins/guardrails/lib/powershell/ps-command.sh`, which both guards source. AFTER is this branch. Same script, same host. | case | before | after | | --- | --- | --- | | clean `git push --no-verify` / `git reset --hard` | 2 | 2 | | clean harmless (`echo hi` / `git status`) | 0 | 0 | | trailing NUL | **0** | **2** | | NUL splitting the flag (`--no-veri<NUL>fy`) | **0** | **2** | | NUL then junk (`--no-verify<NUL>x`) | **0** | **2** | | leading NUL | **0** | **2** | | NUL in an otherwise harmless command | **0** | **2** | Identical for both guards. No row where a clean command changed verdict. The `<NUL>x` row is the one that matters most: it is the payload that executes as the dangerous command. ### The leading-NUL row blocks for the right reason Identical truncated content, opposite verdicts, so the flag decides rather than incidental matching: | payload | exit | | --- | --- | | `"command": ""` (empty, no NUL) | 0 | | `command` field absent entirely | 0 | | leading NUL, truncates to empty | **2** | | a lone NUL and nothing else | **2** | Same on both guards. ### Test suites, same host, baseline vs branch **Both arms ran in full**, serially, on an uncontended host: every `*.test.sh` under `plugins/guardrails/hooks/` plus `lib/hook-utils.test.sh` — 14 suites, every one of them listed below. BASELINE is the same `468bb2d9` tree used for the boundary table; BRANCH is this tip. | suite | baseline | branch | delta | | --- | --- | --- | --- | | `lib/hook-utils.test.sh` | 156 / 0 | **162 / 0** | +6 new cases | | `block-dangerous-git.test.sh` | 341 / 0 | **346 / 0** | +5 new cases | | `block-no-verify.test.sh` | 120 / 0 | **127 / 0** | +7 new cases | | `block-convention-violation.test.sh` | 31 / 0 | 31 / 0 | — | | `block-hook-bypass.test.sh` | 260 / 0 | 260 / 0 | — | | `block-noncanonical-commit.test.sh` | 202 / 0 | 202 / 0 | — | | `cli-flag-verify.test.sh` | 52 / 0 | 52 / 0 | — | | `flag-commit-pr-skill-bypass.test.sh` | 29 / 0 | 29 / 0 | — | | `hardcoded-path-check.test.sh` | 94 / 0 | 94 / 0 | — | | `require-jq-notice-isolation.test.sh` | 2 / 0 | 2 / 0 | — | | `secret-pattern-detection.test.sh` | 52 / 0 | 52 / 0 | — | | `skill-reference-verify.test.sh` | 96 / 0 | 96 / 0 | — | | `stale-path-verify.test.sh` | 87 / 0 | 87 / 0 | — | | `workflow-resilience-check.test.sh` | 16 / 0 | 16 / 0 | — | | **total** | **1538 / 0** | **1556 / 0** | **+18, 0 failures either side** | Every suite that does not exercise the new path is byte-identical across the two arms, so the +18 is entirely the new cases. No pre-existing failure to disambiguate. Two of the new library tests look redundant and are not: `HOOK_JQ_FIELDS_NUL` is checked both after a clean payload and after an **early return**, each running a NUL payload first, because a single-call test cannot observe a stale flag however it is written, and two of the three return paths fire before any NUL could be seen. ### Other gates, all re-run after the rebase `sync-hook-utils.sh --check` (16/16) - `sync-hook-utils.sh --check-bump origin/main` - `check-changelog-parity.sh --check` / `--check-bump origin/main` / `--check-order` - `check-silent-skips.sh` - `check-contract-clause-coverage.py` - `check-cross-plugin-source-drift.sh --check` - `check-hook-userconfig-argv.sh` - `check-plugin-manifest-presence.sh` - `sync-parse-concern-value.sh --check` - `sync-resolve-convention-pattern.sh --check` - `sync-standards-contract.sh --check` - `check-skill-leaf-names.sh --check` - `check-shell-portability.sh --paths` - `shellcheck -x -S warning` (rc 0) - `shfmt -d -i 2` (rc 0) - `markdownlint-cli2` (0 issues) - `check-manifest-duplicate-keys.py`. ## What this PR does NOT fix, stated rather than implied **A payload jq cannot parse still returns 1 and is still allowed.** Malformed JSON, a wrongly typed field or an empty buffer all reach the same `|| exit 0`, exactly as before this change. Process substitution also means jq's own exit status is never observed. That path is untouched here and out of scope, and the header comment now says so instead of claiming — as an earlier draft of this very fix did — that nothing a payload contains can reach it. That claim is the same reasoning shape that produced #2122, and it should not ship inside its fix. ## What I could NOT verify - **How a command actually travels from hook payload to execution.** Nobody traced it. Two shell behaviours were measured and they disagree, and Node refuses NUL-bearing strings on every shape tried, so the command may never reach a shell parser at all. The design is built so this does not matter: fail-closed is right under deletion, truncation, and refusal alike. An earlier draft of this PR did lean on it, in one direction and then the other; both are gone, from the body and from the code comments, the README and the changelog. - **Whether the harness's control-character validation runs before or after PreToolUse hooks**, and **whether the class it rejects includes NUL specifically.** The discriminating probe is bypass-shaped and was deliberately not run. The guard that exists is worded *"contains control characters that would be hidden in the approval dialog"* — approval-surface anti-spoofing, covering `command` / `script` / `url` only, with no equivalent on `content` / `new_string` / `file_text`. It is an implementation detail, not a documented guarantee, and nothing here leans on it in either direction. - **Behaviour on jq builds other than 1.8.2, and on bash other than 5.3.9 (Cygwin).** The chosen construct uses only `explode`, `implode`, `index`, array slicing and `any` — core since jq 1.5 — precisely to keep that risk low, but it was not executed against an older jq. The repo's `hook-utils-windows` job exercises Git Bash on windows-2025 in CI. - **Any performance claim.** The spawn count is unchanged at one, which is structural. Measured per-field cost of the sanitiser was below spawn noise on this host — the no-op control benchmarked *slower* than all three candidates — so no number is claimed. - **Whether a NUL payload can reach a PreToolUse hook through the harness's own serialization.** Being settled separately. This fix does not depend on the answer: failing open on a parse failure is wrong regardless of how the parse came to fail. ## Related - Closes #2122 — the reproduction and the shipped-hook measurements this PR fixes. - Refs #2120 (`perf/guardrails-jq-fields`) — converts ten further guardrails hooks to `hook::jq_fields` and, at `9fb8383d`, independently fixes the same function by **stripping** NULs. Direct conflict on `lib/hook-utils.sh`; whoever merges second must keep both correctness properties rather than taking one side of the hunk, and those ten hooks need `HOOK_JQ_FIELDS_NUL` checks as a prerequisite. Not closed by this PR. - Refs #2123 — merged mid-work; changed `plugins/guardrails/lib/powershell/ps-command.sh`, which both guards source, so the boundary table was re-measured against it. Introduces no `hook::jq_fields` call site, so it needs nothing from this change. - Refs 9b90e35 (#1979) — the precedent for a shared-lib change costing a version bump and a changelog entry in every carrying plugin. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

kyle-sexton and others added 3 commits August 9, 2026 20:12

Merge remote-tracking branch 'origin/main' into fix/guardrails-herest…

f3db590

…ring-deadlock # Conflicts: # plugins/guardrails/.claude-plugin/plugin.json # plugins/guardrails/CHANGELOG.md # plugins/source-control/.claude-plugin/plugin.json # plugins/source-control/CHANGELOG.md

claude Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread plugins/guardrails/hooks/secret-pattern-detection.sh Outdated

claude Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread plugins/source-control/hooks/pr-linkage-validator.sh

chatgpt-codex-connector Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread lib/hook-utils.sh Outdated

claude Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread plugins/guardrails/CHANGELOG.md Outdated

claude Bot reviewed Aug 10, 2026

View reviewed changes

Comment thread plugins/source-control/CHANGELOG.md Outdated

kyle-sexton mentioned this pull request Aug 10, 2026

guardrails: REPLACE_ALL conflates a jq failure with a legitimate false, silently #2126

Closed

kyle-sexton merged commit 468bb2d into main Aug 10, 2026
33 checks passed

kyle-sexton deleted the fix/guardrails-herestring-deadlock branch August 10, 2026 02:50

kyle-sexton mentioned this pull request Aug 10, 2026

fix(hook-utils): stop a NUL in a payload value from voiding two blocking guards #2135

Merged

kyle-sexton mentioned this pull request Aug 11, 2026

A required check can run, pass, and not attach to the head SHA — merge refused while every surface reads green melodic-software/ci-workflows#399

Closed

Navigation Menu

Uh oh!

fix(guardrails): stop the here-string deadlock that voided two blocking guards - #2123

fix(guardrails): stop the here-string deadlock that voided two blocking guards#2123
kyle-sexton merged 5 commits into
mainfrom
fix/guardrails-herestring-deadlock

kyle-sexton commented Aug 10, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Aug 10, 2026

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

kyle-sexton commented Aug 10, 2026

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

claude Bot commented Aug 10, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

The defect

Boundary measurements, through the real hooks

Why not printf … | grep -q — the pipefail inversion is real

Repo-wide sweep of <<< — every site, with a verdict

Fixed (18 sites)

Judged safe — no fix, with reason

The $VIOLATIONS site, measured

Tests

Known follow-up (not fixed here)

Verification

Rider

Related

Uh oh!

cursor Bot commented Aug 10, 2026

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Summary

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #2123

Scope

Analysis

Findings

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #2123 (re-run at HEAD 78c3fd0)

Scope

Analysis

Findings

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Summary

Uh oh!

Uh oh!

Uh oh!

kyle-sexton commented Aug 10, 2026

Suite evidence

Local runs (Git Bash on Windows — the host where the deadlock reproduces)

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review — PR #2123 (re-run at HEAD 2895890)

Summary

Uh oh!

claude Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Security Review — PR #2123 (re-run at HEAD 2895890)

Scope

Analysis

Findings

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kyle-sexton commented Aug 10, 2026 •

edited

Loading

Why not `printf … | grep -q` — the pipefail inversion is real

Repo-wide sweep of `<<<` — every site, with a verdict

The `$VIOLATIONS` site, measured

claude Bot commented Aug 10, 2026 •

edited

Loading

claude Bot commented Aug 10, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading

claude Bot commented Aug 10, 2026 •

edited

Loading

Security Review — PR #2123 (re-run at HEAD `78c3fd0`)

claude Bot commented Aug 10, 2026 •

edited

Loading

claude Bot commented Aug 10, 2026 •

edited

Loading

Code Review — PR #2123 (re-run at HEAD `2895890`)

claude Bot commented Aug 10, 2026 •

edited

Loading

Security Review — PR #2123 (re-run at HEAD `2895890`)