fix(guardrails): PowerShell read-only git grouping + ps-computed-positional (#4235) - #4817
Closed
kyle-sexton wants to merge 13 commits into
Closed
kyle-sexton wants to merge 13 commits into
kyle-sexton wants to merge 13 commits into
Conversation
This was referenced Sep 28, 2026
cursor Bot
pushed a commit
that referenced
this pull request
Sep 28, 2026
#4236) (#4848) <!-- CURSOR_AGENT_PR_BODY_BEGIN --> Closes #4236 ## Summary Document that the dispatcher keeps running after a deny, name the PowerShell over-blocks with the rewrite that already passes, and pin that a dual-blocked PowerShell sink prints both denials. ## Fix - README states that after-block continuation is deliberate: a dual-blocked PowerShell sink prints both denials. The over-length short-circuit (#4528) stays the one exception. - Scope notes: unroll `foreach { git … }` (and other untokenizable `{}` / `()` grouping) into flat `git -C <path> …;` statements; rewrite `& $var script arg1 arg2` as a quoted literal path or a flag-first call (`& $sh -File …`). Binding-to-literal relief remains #4234; grouping interrogation relief remains #4235 / #4817 (open; this PR does not claim that relief). - `run-guards.test.sh` pins `Invoke-Command -ScriptBlock { git reset --hard }` through `block-no-verify` + `block-dangerous-git` and asserts both `block_no_verify_enabled` and `block_dangerous_git_enabled` on stderr. ## Verification ``` cd plugins/guardrails/hooks bash run-guards.test.sh ``` PASS=311 FAIL=0, including `PS dual sink: block-no-verify reason is on stderr` and `PS dual sink: block-dangerous-git reason is on stderr too`. ## Decision - **Claim:** KEEP the full guard chain after a block. Do not stop at the first exit 2. - **Basis:** The dual-sink PowerShell path this issue recorded (`Invoke-Command { git reset --hard }`) is refused by both `block-no-verify` and `block-dangerous-git`. Ending the chain at the first deny would hide the second lever. The over-length ceiling (#4528) remains the documented short-circuit. - **As of:** 2026-09-28 - **Recheck:** if a later dispatcher change makes after-block continuation the dominant cost on the PowerShell allow path, or if #4235 lands and the dual-sink shape no longer hits both guards. ## needs-human - **Claim:** Windows `RUN_GUARDS_PROFILE=1` cost of the kept full chain on Git Bash PowerShell allow is unmeasured here. - **Basis:** This environment is Linux CI. The issue's Windows profile numbers cannot be reproduced on this host. - **As of:** 2026-09-28 - **Recheck:** run `RUN_GUARDS_PROFILE=1` on Windows Git Bash against the PowerShell allow path and compare to the README budget table. ## Related - #4235 / #4817 (read-only git grouping, separate; overlap checked — this PR documents the rewrite that already works on main). - #4234 (computed-call literal binding). - #4528 (over-length first-block short-circuit). guardrails 0.39.3 (serialized above origin/main 0.38.13 and in-flight 0.39.2 on #4251). <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-ab53da24-b89d-4314-a060-0da474e837e9?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-ab53da24-b89d-4314-a060-0da474e837e9&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
cursor
Bot
changed the title
Contributor
|
PR body contract — issue linkage This PR body conforms to the issue-linkage contract. Nothing to do. |
cursor
Bot
changed the title
…ow denial text (#4235) Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…4235) The last #4235 acceptance item: a narrow lever for the PowerShell & $var two-positional arm. ps::write_bypass tags that arm so the guard can name the rewrite and grant ps-computed-positional without opening Set-Content, splats, or Bash writes. guardrails 0.39.3. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/4235-ps-readonly-git-grouping-37e9
branch
from
September 28, 2026 12:39
b3a6e8f to
f33074d
Compare
Regenerate the README options table for block_windows_drive_tmp_enabled and block_root_delete_target_enabled, whose descriptions drifted from the manifest. Not pushed: this branch's own HEAD commit f33074d dropped ~150 lines of the #4683 PowerShell here-string/bare-CR guard (ps::has_bare_cr, ps::payload_has_bare_cr, ps::line_confirms_herestring_opener, PS_REDUCTION_UNTRUSTED*) that origin/main carries. See report. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
The #4235 tip rewrote ps-command.sh from a tree older than #4683 and dropped ps::has_bare_cr, ps::payload_has_bare_cr, ps::line_confirms_herestring_opener and the PS_REDUCTION_UNTRUSTED state, plus the callers' reason attribution. Restore them and keep the #4235 additions (interrogation-ok scope, block_no_verify_allow, block_hook_bypass_allow, commit-gated denial text). A granted ps-computed-positional returned before the cmdlet scans, so `& $sh a b; Set-Content f x` or `; iex $p` passed. The grant now re-runs ps::write_bypass with that arm skipped. README content from main that the branch had dropped is restored. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
An inherited PS_WRITE_BYPASS_SKIP_POSITIONAL=1 would have skipped the computed-positional arm with no allow token. ps::write_bypass now takes `skip-positional` as its second argument. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
With the ps-unparsable-* tokens set, block-no-verify blanked the whole
opaque region, so `& { git commit --no-verify -m x }`, an iex string, or
a here-string piped to `git commit -F - --no-verify` passed. The grant
now waives only the can't-parse denial: every git call in the raw text
runs through check_segment before the region is blanked.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
Git spellings kept slipping past the raw-text scan (`git.exe`, `& $g`, `& 'git'`, a splat, a concatenated iex string). A ps-unparsable-* grant now waives the parse block only when the whole command has no no-verify/noverify text, no -n-style flag beside commit, and no dynamically built command (+, -join, -f, a call on a variable or quoted name, a splat, iex on anything but one single-quoted literal). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
…$( ) git expands an unambiguous long-option prefix (`--no-verif`, `--no-v`), GIT_CONFIG_* env vars can set core.hooksPath, and `$(...)` can assemble the flag at run time. Any of those in the command's raw text now keeps the parse block under a ps-unparsable-* grant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
…riters PowerShell reads NBSP as whitespace (`git commit<NBSP>-n`), and [Environment]::SetEnvironmentVariable or `si env:` can disable a hook manager. Any non-ASCII byte, `env:`, `[environment]` or `setenvironmentvariable` in the raw command now keeps the parse block under a ps-unparsable-* grant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie
kyle-sexton
marked this pull request as ready for review
September 28, 2026 19:30
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eff46cf67
ℹ️ 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".
| [[ "$seg" =~ $word_commit && "$seg" =~ $short ]] && return 0 | ||
| done | ||
| [[ "$lc" == *+* ]] && return 0 | ||
| local joinf='(^|[[:space:])])-(join|f)([[:space:](]|$)' |
There was a problem hiding this comment.
When ps-unparsable-launcher is allowed, the extra ] in this regex prevents both -join and -f from ever matching their intended boundary. For example, pwsh -Command (('git com','mit --no-','verify') -join '') is accepted with that token even though it constructs and executes git commit --no-verify; the other raw checks do not see the split commit or flag. Correct the boundary so dynamically assembled launcher commands cannot use the new allow-list to bypass hooks.
Useful? React with 👍 / 👎.
Comment on lines
+297
to
+299
| start=$((off + ${#pre} + ${#BASH_REMATCH[1]})) | ||
| hook::bash_parse_segments "${raw:start}" check_segment | ||
| off=$((start + 3)) |
There was a problem hiding this comment.
When the launcher sink is allowlisted, starting the secondary parse exactly at git discards any hook-manager assignment earlier in the launched command. For example, cmd /c "set LEFTHOOK=0 && git commit -m x" exits 0 with block_no_verify_allow=ps-unparsable-launcher: the whole-command prefilter does not reject configured manager names, and check_segment receives only git commit -m x. Inspect the launcher prefix or reject configured hook-manager disables before granting the sink.
Useful? React with 👍 / 👎.
| # `prune` for the tests below. A mutating form elsewhere in the same command | ||
| # (`git remote -v; git remote add x y`, `git stash list; git stash drop`) still | ||
| # leaves its stem behind and still blocks. | ||
| while [[ "$lc" =~ (^|[^[:alnum:]_.-])(remote([[:space:]]+(-v|--verbose))?[[:space:]]*([\;\|\}\)]|$)|remote[[:space:]]+(show|get-url)|stash[[:space:]]+(list|show))([^[:alnum:]_.-]|$) ]]; do |
There was a problem hiding this comment.
When the advertised read-only loop is formatted conventionally with one command per line, a newline after git remote -v is not recognized as an end-of-statement marker here, so remote remains classified as mutating. The new interrogation tokenizer likewise folds newlines into ordinary whitespace rather than its ; sentinel, causing both block-no-verify and block-dangerous-git to reject a multiline foreach { git remote -v; git status } equivalent even though the same semicolon-separated text passes. Preserve newlines as statement separators in both classifiers.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
Author
|
Closing by operator decision (2026-09-28): it adds |
kyle-sexton
added a commit
that referenced
this pull request
Sep 30, 2026
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
…5340) No related issue: audit follow-up for the guardrails plugin. Every issue it touches is already closed and several are being reopened by hand, so no closing keyword is used. ## Summary An audit of the 499 PRs an unattended agent opened between 2026-09-27 and 2026-09-29 found guardrails defects in six groups: guard gaps that let a destructive command through, tests that no longer test what they claim, a non-atomic cache, an undeclared Node requirement, README and CHANGELOG statements that were wrong, and an unratified performance claim in PLAN.md. This PR fixes what does not need the owner, and leaves the owner-reserved questions alone (see Related). Guardrails goes 0.41.9 to 0.42.0. ## Fix - `block-windows-drive-tmp`: a drive-root `\tmp` is refused on a usertemp host; `curl`/`wget` short-flag clusters, `--output-dir` and an option after a bare `-O` are judged; the usertemp mount fallback needs `usertemp` on the `/tmp` mount's own line. The suite feeds commands on stdin, so the 9 `/usr/bin` writer cases that were skipped now run. - `block-root-delete-target` (PowerShell): a delete after LF, CRLF or a bare CR, or inside a scriptblock, grouping or `$( )`, is judged; `$env:NAME\subpath` no longer refused as a bare variable (`$env:TEMP`, `$env:TEMP\`, `$env:TEMP\*` still are). - PowerShell classifier: one untrusted-reduction flag instead of two, CR scan skipped when the command has no CR, sink-budget tests rebuilt so they exercise budget exhaustion. - `stale-path-verify` and `skill-reference-verify`: a partly written cache is a miss and is rebuilt; no extra process on the hook path. - `wsl` regression rows added to the `block-hook-bypass`, `block-noncanonical-commit` and `block-convention-violation` suites. - Node on PATH is declared in the README, checked by `/guardrails:setup check`, and listed in `prerequisites.json`; setup's apply text follows the reconfiguration convention. - README: `wsl` since-version, six re-parsing guards, PowerShell no-token over-blocks, plugin-scoped GitHub MCP matcher, contributor to-do and tracker asides removed. PLAN.md: the "no further process can be removed" floor is scoped to the PostToolUse cold path and the goal is marked a draft. - CHANGELOG: five released entries corrected in place (0.41.7, 0.40.0, 0.38.11, 0.38.1, 0.37.3), no heading removed. They are named in the new 0.42.0 entry. - From other groups' requests (extending the 0.42.0 entry, no second bump): - README: the exec-form dispatcher rows are dated to 0.41.3 (they said 0.41.0), and each fire is stated as two processes, node and then the bash it spawns, with the candidate order left to the header of `hooks/exec-bash.mjs` (hook-launcher). - `/guardrails:setup check` probes the bash `hooks/exec-bash.mjs` resolves, not the Bash tool's own bash (hook-launcher). - PLAN.md says its census rows and floor line were measured under shell form and not re-measured under exec form, and that no hook or skill reads it (hook-launcher). - `exec-bash.test.sh` accepts the first bash on `PATH` (a Homebrew bash) as well as `/bin/bash` and `/usr/bin/bash`, and the plugin's resolver test gains the PATH-first cases from `lib/exec-bash.resolver.test.mjs` (hook-launcher). ## Verification - `git merge origin/main`: two conflicts, the plugin version and the CHANGELOG head, resolved by keeping 0.42.0 above the 0.41.9 entry that #5309 added on main. - `scripts/check-changelog-parity.sh --check --check-order`: pass. `--check-preserved origin/main`: all 225 headings preserved. - `scripts/validate-plugins.sh`: all manifests and the catalog validate. - `scripts/affected-tests.sh --run --jobs 8`: every guardrails suite passes (block-windows-drive-tmp, block-root-delete-target, block-dangerous-git, block-hook-bypass, block-noncanonical-commit, run-guards, stale-path-verify, skill-reference-verify, setup and the rest). Three suites fail on this host for missing tools, unrelated to the change: `scripts/check-html-assets.test.sh` and `scripts/check-script-contract.test.sh` (htmlhint not installed), `scripts/hook-census.test.sh` (strace not installed). 25 Node and Python suites are selected but belong to other lanes. - `scripts/check-guardrails-ps-differential.sh origin/main`: 613 commands, 0 cells where main refuses and this branch does not. The scratch under-block differentials for block-windows-drive-tmp (6648 cells, 0 lost) and block-root-delete-target (15823 payloads; the 31 cells that differ are `$env:NAME\subpath` spellings Bash also allows, plus one backtick-before-CR-CR-LF read as PowerShell reads it) are recorded in the task notes. - Baseline differential runs at the two earlier commits and at main: 0 cells lost in all three. Substitution-cap cost on WSL2 Linux: 2000 unquoted `$(:)` took a median 6038 ms; the same 2000 inside single quotes or a heredoc body took 140 ms, level with a 137 ms control with no substitution text. No Windows measurement was taken. - No Windows host and no strace here: Windows behavior is proven by the test-windows lane after the ready flip, and the spawn ceilings by CI's ratchet step. - Cross-group pass, on head `c3111b8d6` after merging origin/main `0589e13f4` (clean): - `exec-bash.test.sh` and `exec-bash.resolver.test.sh` pass. With a bash symlink first on `PATH` outside `/bin` and `/usr/bin`, the old pin fails (`FAIL: bash was not a real path`) and the new one passes. - `check-changelog-parity.sh --check`, `--check-order`, `--check-preserved origin/main` (226 headings) and `--check-bump origin/main` pass. - `validate-plugins.sh`, `sync-exec-bash.sh --check` (21 copies) and `check-skill.sh` on the setup skill (PASS, 0 errors) pass. So do `coverage-manifest.test.sh` (17 checks), `check-skill-portability.sh` and `check-shell-portability.sh`. markdownlint, typos and shellcheck are clean on the changed files. - `scripts/affected-tests.sh --run --jobs 8` against origin/main: 287 selected suites pass. The same three as before fail on this host for missing tools: `check-html-assets.test.sh` and `check-script-contract.test.sh` (htmlhint), and `hook-census.test.sh` (strace). - PR #4715's disclosed "`git reset --hard` + nested here-string" bypass: 12 PowerShell nested here-string payloads carrying `git reset --hard` exit 2 from `block-dangerous-git.sh` on this branch and on main, so no issue was filed. One Bash probe exits 0 on both: a `-c` operand built from a command substitution, which is the README's declared `$(…)` residual. - Ready pass, on head `600ec5b3c` after merging origin/main `a82943beb` (clean; main's 7 new commits touch no guardrails path): - Security review of the PR diff: no findings. Every guard change adds refusals or matches the Bash lane. Probes still refuse `$env:TEMP\..`, `${env:TEMP}\.`, a stray `)` or `}` before a delete, a delete after a keyword block, unclosed levels, and a delete after a JSON `\r` or `\u000d`. A raw control byte in the payload is invalid JSON, so the CR re-read keeps the command as first read. - `check-changelog-parity.sh` (`--check`, `--check-order`, `--check-preserved origin/main`, `--check-bump origin/main`), `validate-plugins.sh`, `sync-exec-bash.sh --check`, shellcheck, markdownlint and typos on the changed files: pass. `exec-bash.test.sh` and the node resolver test pass. - `scripts/affected-tests.sh --run --jobs 8 origin/main`: the same three suites fail for missing tools (htmlhint, strace). `audit-coverage.test.sh` and `cant-fail-scan.test.sh` also failed in the 8-job run; both pass run alone on this head and on a snapshot of `a82943beb`, and this PR does not touch their paths. ## Related Audit: `.work/audit/REPORT.md` (local, not committed). Refs #3683 #3686 #3951 #4118 #4236 #4242 #4247 #4251 #4261 #4390 #4516 #4527 #4528 #4651 #4678 #4679 #4681 #4682 #4683 #4685. Left for the owner, not implemented here: - #4390: reopened with a decision packet on the cache design, plus an addendum on PLAN.md's Done-when (ratify the ceilings, k × S, or re-measure on Windows first) and `async` keep-or-reverse. - #4684: reopened with a decision packet on the substitution cap against the hook-precision rule. - #5341: a new decision issue for block-root-delete-target scope (launcher grammar, second PowerShell parser). - #4118, #4681 and #4683: reopened with ratify-or-reverse packets, because #4766, #4755 and #5036 took decisions reserved for the owner and were merged by `app/cursor`. #3683 gets the same packet for #4845's host-skip call. - #4679: a packet on whether hook-observability condition 3 admits a once-per-(session, agent) latch, or the admission is recorded as an owner-approved exception. - #3951: a packet on whether to build the heredoc-aware fix now in its own PR, since the owner's 2026-09-28 comment closing #4811 said more guard logic is not justified now. Operator-only steps stay open on #3683, #4236, #4261, #4527, #4678, #4679 and #4684 (Windows-host measurements and runs, and one interactive render check). Found and declared, not fixed: `env -f x rm -rf /` exits 0; PowerShell `$r = Remove-Item ...`, comma-list and here-string forms remain gaps in block-root-delete-target. An unmeasured Windows `RUN_GUARDS_PROFILE` cost stays on #4236. Cross-group requests: - claude-ops: `audit_skill_visibility.test.sh` skips its whole `--installed` contract on any Git Bash host (apply `host_path` to the `--installed` argument or narrow the probe; correct the CHANGELOG guess). - claude-config: `unhobble/SKILL.md` (~L382) and `unhobble/evals/evals.json` restate the block-hook-bypass exit-code claim with no verification record. - planning: `planning/skills/setup/SKILL.md` (~L158) has an unwrapped line in the apply bullet. - conventions: `docs/conventions/hook-observability/README.md` condition 3 needs to say whether a once-per-(session, agent) latch counts as a state transition. - scripts: add the shapes from the PowerShell differential findings to `scripts/guardrails-ps-differential-corpus.jsonl`. - hook-launcher: #5309 (0.41.9) landed the PATH lookup and the visible notice for an unresolvable bash in `exec-bash.mjs`. Still open: a missing node needs a launcher design that does not depend on node (#3708). The README wording and the PLAN.md census note are now in this PR. - scripts: the 12 nested here-string `git reset --hard` payloads above are candidates for the same corpus. Cross-group requests received, checked against origin/main `0589e13f4`: - hook-launcher: - The README exec-form version and two-process shape, the PLAN.md annotation, the relaxed launcher test pin, the PATH-first resolver cases and the setup bash probe: applied (see Fix). - Node declaration: already done here (README Requirements, setup check item 3, `prerequisites.json`). - Five owner decisions: #4390 already had its packet. #3683 was already reopened and now has a ratify-or-reverse packet. #4118, #4681 and #4683 are now reopened with packets. #3686 belongs to hook-launcher. - ci: - #4527 already carries the per-suite questions. - The `\tmp\x` downgrade follow-up is not filed, because bca5f57 in this PR restores the block assertion: the helper no longer downgrades a backslash-led `\tmp`, and the quoted spellings expect 2. - #3683: #5316's windows-2025 run shows `cloud-bootstrap-plugins` PASS=79 with no probe skip, so its probe needs no rework. The results, and #4845's causes marked as hypotheses, are on #3683. - tracker: - #3951 not implemented: owner decision (packet above). - #4235 not delivered by this PR; it needs its own dispatch. The owner's comment closing #4817 asks for "a design scoped to #4235", which belongs in its own PR: its `ps-command.sh` changes overlap this PR's classifier edits and need the token-subset differential. The tracker posts the scope comments. - #4678: reopened with Windows operator steps (not run here). - #4679's render confirmation: already an open item with the exact steps. - The #4715 bypass: probed, not reproducible (see Verification). Its payloads go to the scripts group rather than this PR, because the corpus lives in `scripts/`. - conventions: - The setup apply bullet already carries main's scope-matching caveat 2. - #4679: the condition-3 packet is posted. The wording goes to conventions after the owner answers. - source-control: commented on #4247 with #5317's widened matcher. - performance: PLAN.md is already relabelled and #4390 already carries both labels. The Done-when and `async` questions are posted as an addendum. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01EugXnFddtpHcY5gTuyEirB --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com>
kyle-sexton
added a commit
that referenced
this pull request
Sep 30, 2026
Refs: #4235 ## Summary A PowerShell command with `{}`/`()` grouping was refused by `block-dangerous-git` even when every git call in it was read-only, so `foreach ($d in ...) { git -C $d status; git -C $d log }` failed. Guardrails goes to 0.44.1. The owner decided on #4235 (2026-09-30) how the environment-write guard in that relief must work, Option A: "Parse, do not substring-match: block any `Env:` write (Set-Item/New-Item/`${env:...}`/`[Environment]::SetEnvironmentVariable`) whose target name is not a literal string, fail closed. Add regression cases for a concatenated and a computed `Env:` path." This PR implements that decision. The Claude security lane's CRITICAL thread on the earlier substring guard (`PRRT_kwDOTCGFQM6nlUph`) is still open, and a newer one on the location-change arm (`PRRT_kwDOTCGFQM6nqcu1`) is addressed by `2b63cb907`; both stay for the lane to re-judge. The PR is a draft until then. Issue #4235 has two acceptance criteria beyond the relief: - The `block-no-verify` narrow-token criterion is met on main by #4833 (`block-no-verify.sh` honors `ps-unparsable-special-construct` from `block_dangerous_git_allow`). - The `block-hook-bypass` `ps-computed-positional` criterion is not implemented. The owner excluded it in the PR #4817 close comment, so the PR uses `Refs`, not `Closes`. The owner decides whether to strike that criterion and close the issue. ## Fix - New `interrogation-ok` sink scope in `ps::classify_git_command`. It relieves only the special-construct trigger, and only when every git invocation is on the `ps::git_command_is_interrogation_only` allowlist (`status`, `log`, `show`, `diff`, `rev-parse`, `ls-files`, `remote -v`, `stash list` and similar), optionally behind `-C <path>`. `-c`, `--exec-path`, computed or obscured subcommands, aliases, `fetch`, `grep` and mutating verbs still block. Dynamic-invocation, launcher and here-string triggers are unchanged. - `ps::has_unprovable_env_write` replaces the seven substring checks for the environment-write guard. It reads syntax and refuses whatever it cannot prove has a plain literal target: a provider cmdlet (`Set-`/`New-`/`Add-`/`Copy-`/`Move-`/`Rename-` `Item`, `Content`, `ItemProperty`, their aliases, `mkdir`) whose operands are not plain words or that has no operand; an `Env:`/`Function:`/`Alias:` drive path; a `$env:NAME` assignment, alone, in a target list or as a foreach variable; a static `::` call; any method call; `ForEach-Object -MemberName`; a computed call target; a word spliced from quoted parts (`Set'-'Item`, `E'nv':X`); a function, filter or alias definition; `Add-Type`, `New-PSDrive`, `Invoke-Command`; a `$( )` inside an expandable string; and a write beside a `Set-Location`/`cd` whose target is computed or piped in. - `ps::git_command_is_readonly` judges `remote` and `stash` by arguments instead of listing them as mutating stems. - The sink denial offers the commit form only for a commit and names the PowerShell rewrite first, including the fallback line for an unrecognized trigger. - README note and CHANGELOG entry updated; version 0.44.1 (main is at 0.44.0). Trade-off the owner may want to weigh: the scan refuses ordinary idioms that contain a method call, a static call or `"$($x.Name)"` inside a group with git, because it cannot prove them harmless. An operator unrolls the loop into flat statements, computes the value into a variable first, or sets `ps-unparsable-special-construct`. It does not read code in a file, or a script block held in a variable and run by a cmdlet that accepts one. The broader alternative (relieve only commands whose every command word is on an allowlist) stays available and is the owner's call. ## Verification - The four commands from the verifier now exit 2 (each returned 0 on the earlier head): `$n = "GI"+"T_PAGER"; New-Item -Path ("E"+"nv:") -Name $n ...; & { git log }`, `$p = "E"+"nv:\GI"+"T_PAGER"; Set-Item $p ...; & { git log }`, `Set-Content -Path ("E"+"nv:" + $e) ...; & { git log }`, and the `Type.InvokeMember` form. - I ran 64 environment-write variants in real PowerShell 7.6 and confirmed each one sets the variable; the guard refuses all 64 beside `& { git log }`. They include splat, pipeline and variable-held paths, `ForEach-Object -MemberName`, reflection, `Set-Alias`, script blocks, and `Set-Location -Path $computed` followed by a relative write. - `block-dangerous-git.test.sh`: 815 cases pass, including new rows for a split `Env:` token, a path held in a variable, `InvokeMember`, and the variants above, plus predicate pins that test `ps::has_unprovable_env_write` on its own. All 25 guardrails suites pass; `shellcheck` is clean on the changed `.sh` files. - `scripts/check-changelog-parity.sh --check`, `--check-order`, `--check-preserved origin/main`, `--check-bump origin/main` and `scripts/validate-plugins.sh` pass at this head. - `bash scripts/affected-tests.sh --run --jobs 8 origin/main`: 430 shell suites run, and the only failures are three suites that need tools this host lacks (`check-html-assets.test.sh` and `check-script-contract.test.sh` need `htmlhint`; `hook-census.test.sh` needs `strace`). - `scripts/check-guardrails-ps-differential.sh origin/main` over 837 commands: the only cells that move are 19 unique read-only git commands (for example `& { git status --porcelain }`, `(git diff) > out.txt`) that main refused and the branch allows. The `block-no-verify`, convention, noncanonical, hook-bypass and delete rows show 0 moved cells, and no command is refused by the branch that main allows. The script reports the loosened cells by design. ## Related - #4235, #4236, #4833, #4817 - #5340 also edits `ps-command.sh`; main was merged into this branch twice since it opened. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4235
Summary
Lets read-only PowerShell git inside
{}/()grouping (aforeachover repos runningstatus,log,remote -v,stash list) throughblock-no-verifyandblock-dangerous-git, fits the sink denial text to the command, and adds two narrow levers:block_no_verify_allow(theps-unparsable-*sink tokens) andblock_hook_bypass_allow=ps-computed-positional. guardrails 0.42.0.Fix
ps::git_command_is_readonlyreadsremoteandstashby their argument:remote,remote -v,remote show,remote get-url,stash list,stash showare read-only;remote add,stash pop,stash dropstill block.block-dangerous-gituses a newinterrogation-oksink scope backed byps::git_command_is_interrogation_only, an allowlist of built-in interrogators read off the argv. Assembled subcommands, aliases,-c,$env:GIT_*, a quoted'git'target, andiexstill block.block-no-verifytakesblock_no_verify_allow. A granted token waives only the can't-parse denial, and only when the whole command's raw text has nono-verify/noverify/--no-v*text, no non-ASCII character, noenv:,[Environment],SetEnvironmentVariable,GIT_orhooksPathtext, no-n-style short flag besidecommit, and no dynamically built command (+,-join,-f,$(,${,& $x,& 'git', a splat,iexon anything but one literal). A commit message mentioning no-verify blocks under a grant (accepted).block-hook-bypasstakesblock_hook_bypass_allow=ps-computed-positionalfor the& $vartwo-positional arm. A grant re-runs the write scan with only that arm skipped, soSet-Contentoriexin the same command still blocks.commit, and lead with the in-PowerShell rewrite before the Bash-tool suggestion.ps::has_bare_cr,ps::payload_has_bare_cr,ps::line_confirms_herestring_opener,PS_REDUCTION_UNTRUSTED*) and main's README content are kept intact; an earlier tip of this branch had dropped them.Verification
All guardrails suites plus
lib/hook-utils.test.shpass, except three failures that reproduce identically on a cleanorigin/maincheckout on the same machine:block-hook-bypass"two blocking guards dispatched" and tworun-guardspost-verify jq-count cases.New tests: the read-only grouping, remote/stash, allow-token, and denial-text cases; plus
Set-Contentandiexbeside a granted& $shcall still block (both fail without the rescan), and an inheritedPS_WRITE_BYPASS_SKIP_POSITIONAL=1grants nothing. Twenty-two no-verify bypass shapes (NBSP,[Environment]::SetEnvironmentVariable,si env:, inside grouping, script blocks, here-strings, iex,git.exe,& $g,& 'git', splats, concatenation,--no-v*prefixes,GIT_CONFIG_*env,$(subexpressions) block with everyps-unparsable-*token set on both guards (each fails without its gate);& { git status; git commit -m x },& { git remote -v }andiex 'git status'stay allowed under the grant.scripts/check-guardrails-ps-differential.sh origin/mainover the 613-command corpus. Every loosened cell is one of 19 read-only interrogator commands inside grouping (& { git status --porcelain },(git diff) > out.txt,git status; Get-Process | Where-Object { ... }), onblock-dangerous-gitand the dispatcher row only. None carries a here-string or CR shape.shellcheck, shell-portability, killswitch-hoist, hook-exec-form, plugin-options-docs, changelog-parity (
--check,--check-order,--check-bump,--check-preserved), stale-base-overlap, typos, markdownlint, and the PowerShell differential self-test pass locally.Related
& $var script record dirblocked as a file write; block reason names the wrong cause #4234 (the& $varpositional false positive)🤖 Generated with Claude Code
https://claude.ai/code/session_01PT4esxbdC7eQiwQxy35Mie