fix(guardrails): stop blocking a PowerShell call of a constant target

[kyle-sexton]

Closes #1973

Summary

ps::might_invoke_git's call-target branch matched any quote character after & or ., so & "C:\tools\publish.ps1" — the ordinary PowerShell script-invocation idiom, carrying no git token and a compile-time constant path — routed to the fail-closed sink and was refused by a git guard. Both quote styles and the dot-source form were affected, and because the predicate is shared, the same command false-blocked twice: once from block-dangerous-git, once from block-no-verify.

The branch now matches only a genuinely computed target: a bare variable or subexpression (& $tool, & (…)), or a double-quoted string that interpolates (& "$tool", & "C:\tools\$ver\x.exe").

Per PowerShell about_Quoting_Rules, a $-free double-quoted string and any single-quoted string are compile-time constants, so such a target is statically decidable as non-git.

No fail-open

This is the risk that mattered, so it is covered in both directions:

• The literal-git probe runs quote-intact, so & 'git' …, & "git" …, and & "C:\Git\cmd\git.exe" … are still caught by name.
• Every interpolated form still blocks — a blanket removal of the quote class would have failed OPEN on & "$tool" commit.

Regression cases for both directions are checked in on both guards' suites (13 new cases).

Also in this change

• Block messages name the real trigger. They previously listed backtick / --% / subexpression / script-block / here-string regardless of which of the four sink triggers actually fired, so "remove the unparsable construct" was unactionable when the trigger was a launcher or a computed target. ps::classify_git_command now records the trigger in PS_SINK_TRIGGER and each message prints remediation specific to it.
• Both fail-closed messages name their kill switch, which the sibling too-long and alias-cap messages already did.
• Telemetry carries the trigger instead of collapsing all four shapes into one powershell-unparsable token that hid the false-positive rate.
• The git and python-write lanes share one call-target predicate instead of two drifted regexes — the git lane's blanket quote match was the drift. The python lane's behavior is unchanged; it takes the interpolating-string half only, as before.

One implementation note: the shared operator prefix is spelled out in each predicate rather than concatenated in from a variable. Mixing an unquoted variable with adjacent literal regex text in a [[ =~ ]] pattern is version-sensitive, and a predicate that quietly stops matching fails OPEN. The two named functions are the seam that prevents drift.

Verification

858 tests pass across the five affected suites, shellcheck clean:

Suite	Result
block-dangerous-git	329 pass
block-hook-bypass	211 pass
block-noncanonical-commit	172 pass
block-no-verify	115 pass
block-convention-violation	31 pass

Before the fix, all five constant-target shapes exited 2 on a live repro battery; after, all exit 0 while every must-block shape still exits 2.

guardrails 0.19.0 → 0.19.1.

Related

• Reported via the plugin handoff inbox item 20260807-182002-guardrails-block-dangerous-git-ps-call-op-false-positive, produced by a plugin-quality:audit run on a consuming project.
• One correction to that brief, recorded on the item: & $tool reset --hard (bare variable, no other construct) is allowed and is the documented A2b residual, not part of this false-positive class — nothing routes it to the sink.

[cursor]

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]

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

---

Claude finished the code review

• Gather context (diff, changed files, CLAUDE.md/REVIEW.md guidelines)
• Understand the request (code-review lane)
• Review ps-command.sh predicate changes for correctness
• Review hook changes (block-dangerous-git.sh, block-no-verify.sh)
• Review test coverage
• Post review findings

Summary

Solid, well-documented fix. I traced the new predicates (ps::call_target_is_bare_computed, ps::call_target_is_interpolating_string) against every existing and new test case by hand — the regexes correctly narrow the git-lane sink to genuinely computed targets while leaving the literal-git probe quote-intact, so the fail-open guard rail (& "$tool" …, & "git" …) holds. The python-write lane's behavior is provably unchanged (identical regex, just extracted into the shared function). Message/telemetry changes (PS_SINK_TRIGGER) are correctly reset per call and only surfaced on the exit-2 path, so they can't leak stale state across segments. Kill-switch names in both block messages match the actual config keys (block_dangerous_git_enabled, block_no_verify_enabled). Version bump and CHANGELOG accurately describe the change.

I could not execute the test suites or shellcheck in this sandboxed environment (script execution requires approval that isn't available here) — this review is based on static reading of the diff and manual regex tracing, not a live run. The PR body's reported 858 passing tests / shellcheck-clean claim is not independently verified by me.

Findings

🟡 Suggestion — ps::write_bypass duplicates the bare-computed regex the PR just extracted into a shared predicate
plugins/guardrails/lib/powershell/ps-command.sh#L531

This PR's stated goal is "the git and python-write lanes share one call-target predicate instead of two drifted regexes" (new ps::call_target_is_bare_computed / ps::call_target_is_interpolating_string, doc-commented at L182-205). ps::write_bypass (the block-hook-bypass guard's write-bypass lane) has its own third, inline copy of the identical bare-computed regex:

[[ "$lcq" =~ (^|[[:space:]\;\{\}\(\|\&])[.\&][[:space:]]*[\(\$] ]]

its own comment even says "Mirrors ps::might_invoke_git's treatment of the same shape on the git side" — acknowledging the duplication without consolidating it. Not a functional bug (the regex is correct and equivalent), but it's exactly the drift risk this PR's own commentary warns about for the other two lanes; a future edit to the shared predicate's separator class or operator shape (the file's docblock explicitly worries about exactly this kind of drift) would silently miss this third copy. Consider having ps::write_bypass call ps::call_target_is_bare_computed too, in a follow-up.

🟡 Nit — ps::has_dynamic_invocation still uses the blanket "any quote char" pattern this PR replaced elsewhere
plugins/guardrails/lib/powershell/ps-command.sh#L356

ps::has_dynamic_invocation (pre-existing, untouched by this PR) matches [.\&][[:space:]]*[$q] where q carries both quote characters — the same over-broad shape that produced #1968 in ps::might_invoke_git. It's currently harmless: it only feeds PS_SINK_TRIGGER routing, and the actual block/allow decision is ps::might_invoke_git's narrower predicate, so a constant-quoted target (e.g. & "publish.ps1") still correctly resolves to allowed (exit 0) — I traced this through ps::classify_git_command and confirmed no block message or telemetry with a misleading trigger is ever emitted for that path. Flagging only because it's now the sole remaining instance of the pattern this PR otherwise eliminated, and it labels such commands dynamic-invocation in PS_SINK_TRIGGER for the (currently unreachable) case where that matters. Not blocking.

Test coverage

The new regression cases in both .test.sh files are well-targeted: they cover both quote styles, both &/. operators, the fail-open guard rail (interpolating variants of each still block), and literal-git targets in both quote styles (still blocked by name). block-no-verify.test.sh correctly adds a subset rather than duplicating the full matrix, since ps::might_invoke_git is shared — reasonable given the predicate is exercised exhaustively on the block-dangerous-git side.

[claude]

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

---

Security review complete

• Gather context (diff + changed files)
• Review ps-command.sh predicate changes for fail-open risk
• Review block-dangerous-git.sh / block-no-verify.sh changes
• Review test files and plugin.json/CHANGELOG for anything security-relevant
• Post findings as PR review

No security issues found. This PR narrows a false-positive-prone regex in a defensive guardrail hook (bash), and I traced the control flow closely for a fail-open regression (the class of bug this exact file has been bitten by before — see the #740/#903 references in the file's own comments) since that's the failure mode that would actually matter here.

What I checked:

• The new predicates (ps::call_target_is_bare_computed, ps::call_target_is_interpolating_string) narrow only the pass condition, not the sink-routing condition. ps::has_dynamic_invocation (unchanged, L379) still matches any quoted call target and routes it to the fail-closed sink. Only inside the sink does the narrowed ps::might_invoke_git decide allow-vs-block, and it still catches a literal git token via the quote-intact literal probe that runs first (L247). So & "git" …, & "C:\Git\cmd\git.exe" …, and & 'git' … are still blocked by name — confirmed against the new test cases in both .test.sh files.
• The interpolation test is grounded correctly in PowerShell semantics: a double-quoted string only interpolates on $, and a $-free double-quoted string or any single-quoted string is a compile-time constant per about_Quoting_Rules — matches what the regex actually implements (\"[^\"]*\$, i.e. any $ before the target's contents, not anchored to a full parse).
• No new injection surface. PS_SINK_TRIGGER (new global, threaded into emit_tel's form telemetry field and the block messages) is only ever assigned one of four fixed string literals inside ps::classify_git_command — never attacker-controlled text — and the telemetry emission already goes through jq -n --arg, so there's no format-string or injection concern there.
• Bracket-class quirks in the regexes (e.g. [.\&], [[:space:]\;\{\}\(\|\&] matching a literal backslash via the no-op \ inside POSIX bracket expressions) are pre-existing, unchanged by this diff, and err toward more matching, i.e. the safe (over-block) direction — not a new fail-open.
• The remediation text added to the block messages (e.g. "a constant path in quotes is decidable and is not blocked") discloses no more than the source already does — this is an open-source, locally-inspectable bash guard, not a remote secret-gated boundary, and the file's own header already documents the threat model as "accidental-destruction friction... not a boundary against deliberate evasion." Naming the kill-switch config option in the fail-closed messages also matches the existing sibling-message convention the PR description cites, not a new pattern.
• ps::write_bypass (the separate python/content-write lane's own call-target check, L510 onward) is untouched by this diff and out of scope — it uses its own inline quote/computed check, not the new shared predicate, which is consistent with the PR's stated scope (only ps::might_invoke_git and ps::might_write_via_python3 are unified).

I wasn't able to execute the shell test suites myself in this sandbox (no approval for running the .test.sh scripts), so I verified the fail-open question by static trace of the regex/control-flow instead of by running the 858 asserted-passing tests; the reasoning above is independent of that run.

[github-actions]

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: f0394e9b8f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

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

---

Security review complete

• Gather context (diff, changed files)
• Review ps-command.sh predicate changes for fail-open / injection risk
• Review block-dangerous-git.sh / block-no-verify.sh changes
• Review test files and plugin.json/CHANGELOG for anything security-relevant
• Post findings

No security issues found.

This PR narrows a false-positive-prone regex in a defensive guardrail hook (bash) that fail-closes on PowerShell git commands it cannot parse with confidence. I traced the change specifically for a fail-open regression — a genuinely computed/dangerous call target slipping past the sink — since that's the failure mode that would matter here (the file's own header already frames the threat model as accidental-destruction friction, not a boundary against deliberate evasion, so I focused on whether the narrowing itself is sound rather than on disclosure).

What I checked:

• The narrowing is provably one-directional (removes matches only, adds none). The old combined regex ([\$\($q], q="\"'")matched$, </code>(<code>, </code>"<code>, or </code>'<code> immediately after the call/dot-source operator — i.e. *any* quoted target, regardless of content. The new split (</code>ps::call_target_is_bare_computed<code>: $or(only;ps::call_target_is_interpolating_string: a double-quoted string containing $before its closing quote) is a strict subset of what the old regex matched. Since a single-quoted PowerShell string and a$-free double-quoted string are compile-time constants per [about_Quoting_Rules`](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_quoting_rules), removing those matches cannot cause a genuinely computed/dangerous target to fall through — there is no PowerShell target shape the new predicates fail to catch that the old, broader predicate caught for a real reason.
• The interpolation regex (\"[^\"]*\$) can't be tricked by content after the string closes. [^\"]* cannot cross a ", so a $ appearing in a later, unquoted argument (e.g. & "script.ps1" -Arg $x) does not cause the quoted target itself to be misclassified as computed — confirmed by tracing the character class, not just the test matrix.
• The literal-git probe still runs quote-intact and backtick-recovered, ahead of the narrowed predicates, so & 'git' …, & "git" …, and & "C:\Git\cmd\git.exe" … are still caught by name regardless of the call-target narrowing — confirmed against the new test cases in both .test.sh files.
• PS_SINK_TRIGGER (new global, threaded into emit_tel and both block messages) is only ever assigned one of four fixed string literals inside ps::classify_git_command (herestring-unbalanced / special-construct / dynamic-invocation / launcher) or left empty — never attacker-controlled text — so emit_tel "blocked" "powershell-unparsable-${PS_SINK_TRIGGER:-unknown}" carries no injection surface, and ps::print_sink_trigger_line's case statement only prints fixed strings. It's also reset to "" at the top of ps::classify_git_command, and that function is called once per hook invocation (a fresh process), so there's no cross-segment or cross-invocation state-leak.
• The python-write lane (ps::might_write_via_python3) now calls the same ps::call_target_is_interpolating_string predicate the git lane uses, but its behavior is unchanged — it's the identical regex the lane already had, just extracted; it still takes only the interpolating-string half, not the newly-added bare-computed half, matching the PR's stated scope.
• No workflow, permissions, dependency-pin, or config-schema changes — the diff is confined to plugins/guardrails/lib/powershell/ps-command.sh, the two hook scripts' telemetry/message call sites, their test files, and the plugin manifest version bump / changelog. Nothing here falls under GitHub Actions trigger/permission risk or supply-chain pinning, so there's nothing in this PR's scope for zizmor's lane to cover either.

Two non-security remediation-wording issues were already flagged by Codex's review (the dynamic-invocation message mislabeling an already-literal git target as "computed," and the unbalanced-here-string message naming the wrong terminator style for a @" opener) — those are message-accuracy/UX concerns, not security findings, so I'm not duplicating them here per this review's scope.

[claude]

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

---

Claude finished the code review

• Gather context (diff, changed files, CLAUDE.md/REVIEW.md guidelines)
• Understand the request (code-review lane, re-triggered after new commit + Codex's review comments)
• Review latest commit (324dbb3, "satisfy the shell-portability gate") against the earlier round
• Verify whether Codex's two P2 findings are still valid on HEAD
• Post review findings

Summary

324dbb3 is a small, low-risk follow-up to f0394e9 (already reviewed): it rewords the dynamic-invocation remediation line to drop a Windows-path literal that tripped the shell-portability gate's GNU-construct heuristic, and adds a portability-ok: exemption comment to the pre-existing \> escape in ps::write_bypass. I confirmed against scripts/shell-portability-tokens.txt:59 (\>) that this token is the one the gate actually flags, and the exemption is correctly scoped to the hit line per the gate's own convention (check-shell-portability.sh:32, :266). No logic changed — same regexes, same control flow as the already-reviewed f0394e9.

Codex's two findings — verified against HEAD, both still open

I traced both by hand through the current ps-command.sh rather than taking them at face value, since 324dbb3 touched the exact line one of them concerns.

1. dynamic-invocation trigger message mislabels a literal-git block as "computed" — still true on HEAD.
ps-command.sh#L350-L358 (ps::has_dynamic_invocation) intentionally matches any quoted call target — not just an interpolating one — because it can't tell & 'git' from & 'git commit' by shape alone (see its own docstring, L339-349). For & 'git' reset --hard (test case at block-dangerous-git.test.sh#L632), that means PS_SINK_TRIGGER is set to dynamic-invocation even though ps::might_invoke_git then blocks it via the literal-name probe, not the computed-target branch. The block message shown is ps-command.sh#L452-L454 (edited by 324dbb3):

"...whose program name is not statically decidable. Invoke the target by its literal name — a constant quoted path is decidable and is not blocked; only an interpolating target ... reaches this branch."

The bolded claim is false for this exact, tested case, and the reworded text (from 324dbb3) makes the claim more specific and thus more wrong than the pre-edit version — the portability fix addressed the gate, not this. The advice to "invoke by literal name" is also unactionable: & 'git' already does that. The test suite only asserts exit code 2, not message content, so this wasn't caught. Not a security issue (confirmed by the security lane's pass, and confirmed again here — the block itself is correct), but it's a real remediation-accuracy bug worth fixing before merge, or as an immediate fast-follow. Fix →

2. Here-string close message names the wrong terminator for a @" opener — still true on HEAD.
ps-command.sh#L446-L448 hardcodes '@ regardless of opener style. ps::blank_herestrings (L101-142) already computes closer="${hs_quote}@" per-opener (L109) but only surfaces a boolean PS_HERESTRING_UNBALANCED, discarding which quote style opened. 324dbb3 didn't touch this function or message, so it's untouched by the new commit. Also worth fixing, same low-severity remediation-accuracy class as #1.

Everything else from the first review round stands unchanged

The ps::write_bypass duplicated bare-computed regex (ps-command.sh#L531) and the pre-existing broad ps::has_dynamic_invocation-adjacent pattern in the (unrelated, untouched) trigger-labeling path are still there as previously noted — non-blocking, already flagged, not re-litigated here.

I could not execute the shell test suites or shellcheck in this sandbox (no approval for running .test.sh scripts) — findings above are from static tracing of the regex/control-flow against the actual test cases in the diff, not a live run.

[claude]

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

---

Claude finished the code review

• Gather context (diff, changed files, prior review rounds on this PR)
• Review latest commit (89551ec) against Codex's two open P2 findings from the prior round
• Re-check for regressions in ps-command.sh / test coverage
• Post review findings

Summary

89551ec is a targeted, well-scoped fix for both P2 findings Codex raised (and that my own prior review round confirmed were still open on 324dbb3). I traced both fixes against the actual code paths rather than taking the commit message at face value.

1. Here-string terminator now matches the actual opener. ps-command.sh#L82-85 adds PS_HERESTRING_QUOTE, set in ps::blank_herestrings (L111, L150) to the hanging opener's quote character, and reset unconditionally at the top of every call — so there's no cross-invocation staleness. ps::print_sink_trigger_line's herestring-unbalanced branch (L463-470) now names '@ for a @' opener and "@ for a @" opener, matching PowerShell's actual pairing rule, with a sane "name both" fallback for the (currently unreachable, since classify_git_command always calls blank_herestrings first) case where the quote wasn't recorded.

2. Dynamic-invocation message no longer tells the operator to do what they already did. The old text asserted "a constant quoted path is decidable and is not blocked," which was false for & 'git' reset --hard — that command names its program literally and is still blocked, because (per ps::has_dynamic_invocation, L356-364) it's the call-operator-plus-quoted-string form that routes here, not target decidability. The rewritten line (L472-479) correctly attributes the routing to the form and tells the operator to drop the iex/&/. entirely — actionable for every shape that reaches this branch, including a literal-named target.

Test coverage matches the fix. The new block-no-verify.test.sh cases (L280-311) capture stderr (previously discarded by run_pwsh, which is exactly how both defects survived to review) and pin: @'→'@, @"→"@, the literal-target case getting "drop the operator" advice, an explicit assert_absent that the old "invoke the target by its literal name" phrase is gone, and that iex of a literal gets the same advice. This is a good, minimal regression net — it tests message content, which the exit-code-only assertions in the original PR never did, and that gap is exactly what let both defects through the first three review rounds.

I checked whether block-dangerous-git.test.sh needed the same message assertions — it doesn't, since ps::print_sink_trigger_line is shared code exercised identically by both hooks and the PR's own established convention (per its body) is that block-no-verify.test.sh carries a subset rather than duplicating the full matrix.

No regressions found in the diff. The two non-blocking suggestions from the first review round (ps::write_bypass's duplicated bare-computed regex; ps::has_dynamic_invocation-adjacent broad pattern) are outside this commit's scope and remain as previously noted — not re-litigating them here.

I could not execute the .test.sh suites or shellcheck in this sandbox (script execution isn't approved here) — verification above is from static tracing of the regex/control-flow against the new test cases, not a live run.

[claude]

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

---

Security review complete

• Gather context (diff, changed files)
• Review ps-command.sh predicate changes for fail-open / injection risk
• Review block-dangerous-git.sh / block-no-verify.sh changes
• Review test files and plugin.json/CHANGELOG for anything security-relevant
• Post findings

No security issues found.

This review covers the new commit since the last security pass, 89551ec ("make the PowerShell sink remediation lines true of every command that reaches them"), and re-confirms the full PR diff against origin/main is otherwise unchanged from the prior two security-review passes on this PR.

What changed in 89551ec: it's a message-text-only fix, addressing Codex's two earlier P2 findings (mislabeled dynamic-invocation remediation, wrong here-string terminator named). No block/allow control flow changed.

What I verified:

• The new PS_HERESTRING_QUOTE global carries no injection surface. It's assigned only one of two single characters (' or ") captured from the command's own here-string opener at ps-command.sh#L150, consumed only inside a fixed-string case statement at L465-469 that emits one of three hardcoded strings — never interpolated into a regex, eval, or command execution.
• No stale-state / cross-command leak. PS_HERESTRING_QUOTE is reset to "" at the top of ps::blank_herestrings (L111) on every call, mirroring the pre-existing PS_HERESTRING_UNBALANCED reset, and ps::print_sink_trigger_line only ever reads it after a fresh ps::classify_git_command → ps::blank_herestrings run on the same command — same safe pattern already verified for PS_SINK_TRIGGER in the prior review round.
• The reworded dynamic-invocation message (L472) is advisory text only — it now correctly instructs dropping the invocation operator rather than mischaracterizing a literal-git target as computed. This doesn't touch the actual block predicate (ps::might_invoke_git, unchanged), so it can't affect the allow/block decision, only what the operator is told to do about a block that already fires correctly.
• emit_tel "blocked" "powershell-unparsable-${PS_SINK_TRIGGER:-unknown}" in both hook scripts (unchanged in this commit, previously reviewed) still only ever carries one of four fixed literals — confirmed again against the current ps::classify_git_command, no new value space introduced.
• New test assertions in block-no-verify.test.sh (L281+) now check stderr message content for both fixed cases, closing the gap that let the two message defects ship silently (they only asserted exit codes before) — a coverage improvement, not itself a security control, but it reduces the chance a future remediation-text change regresses without being caught.
• No workflow, permissions, dependency-pin, or config-schema changes in this commit — confined to ps-command.sh, block-no-verify.test.sh, and CHANGELOG.md.

Combined with the two prior security-review passes on this PR (predicate-narrowing fail-open analysis, telemetry/injection review), I have no outstanding security concerns on the current HEAD.
· branch fix/guardrails-ps-call-operator-false-positive