fix: tighten recursive delete policy matching by Peldan · Pull Request #3251 · microsoft/agent-governance-toolkit

Arvid Peldan (Peldan) · 2026-07-03T13:32:37Z

Description

The recursive-delete deny rule depended too much on narrow command regexes such as rm -rf. Before this change, equivalent recursive deletes like rm -fr important-data, rm -r -f important-data, rm --recursive important-data, Remove-Item -Recurse important-data, and rd /s /q important-data could miss the deny rule and fall through to the normal shell-tool review path instead.

One root cause was that several old rules matched text shape rather than command semantics. In the Claude Code, OpenCode, and Copilot CLI bundles the Bash rule used patterns like \brm\b[\s\S]*\b-rf\b; in JavaScript regex semantics \b-rf\b does not match rm -rf target (the space and - are both non-word characters), so the intended deny never fired for the canonical form. Antigravity's old pattern matched compact rm -rf/rm -fr but missed split/reordered forms (rm -r -f, rm --force --recursive, rm target -rf).

This PR reduces the regex to a broad delete-command trigger and delegates the deny decision to a hasRecursiveDelete() parser that inspects the actual flags, so equivalent destructive forms are denied consistently while safe cleanup targets are not.

Deny semantics: a delete is denied when it is recursive, regardless of whether a force flag is present, unless every target is a known safe-cleanup directory (node_modules, dist, build, …). Force is still parsed but is not required to deny. Non-recursive deletes (rm -f, rm -i, rm --force) are not hard-denied by this rule.

Notable cases covered:

rm -rf, rm -fr, rm -rfv, reordered rm important-data -rf
split/long forms: rm -r -f, rm --recursive, rm --recursive --force (either order)
recursive-without-force: rm -r, rm --recursive, Remove-Item -Recurse
PowerShell: Remove-Item -Recurse -Force, abbreviations Remove-Item -r -fo, ri -r -fo
Windows: rd /s /q, del … /s /q
delimiter-wrapped invocations: `rm -rf /`, {rm -rf /;}, $(rm -rf /)
short-option clusters with unanticipated letters: rm -rfx foo
not denied: safe targets (rm -rf node_modules, Remove-Item -Recurse -Force build), and non-recursive deletes (rm -f, rm -i)

Type of Change

Bug fix (non-breaking change that fixes an issue)
Security fix

Package(s) Affected

agent-governance

Checklist

I have added tests that prove my fix/feature works

Attribution & Prior Art

This contribution does not contain code copied or derived from other projects without attribution
Any external projects that inspired this design are credited in code comments or documentation
If this PR implements functionality similar to an existing open-source project, I have listed it below

AI Assistance

I can explain every meaningful change in this PR: what it does, why, and what tradeoffs were considered
I have run tests and verification appropriate for this change
No part of this PR was autonomously submitted by an AI agent without my review
I have not used AI to generate review comments on others' PRs

If AI tools materially shaped this change, briefly note what was used:
Codex helped add regression tests, find the same parser issue in sibling packages, and carry the reviewed fix across those packages. I reviewed the implementation and test coverage.

IP, Patents, and Licensing

This contribution does not implement patent-pending or patent-encumbered techniques
This contribution does not require an NDA or licensing agreement to understand or use
Any AI tools used have terms compatible with the MIT License

Signed-off-by: arvidpeldan <peldans@gmail.com>

github-actions · 2026-07-03T13:33:19Z

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

github-actions · 2026-07-03T13:33:26Z

🤖 AI Agent: contributor-guide — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Welcome, and thank you for your contribution! 🎉

Your detailed explanation of the problem and solution is excellent, and the added test coverage is thorough.

Before merging, please ensure:

The updated regex patterns are tested against edge cases to avoid unintended matches or false positives.
Verify that the new helper functions (e.g., getRmCommandDetails) are covered by unit tests for all expected scenarios.

For guidance, please refer to CONTRIBUTING.md.

Arvid Peldan (Peldan) · 2026-07-03T13:35:37Z

The branch is split by surface so each package change is easy to review on its own. The first commits apply the recursive-delete fix and matching tests to OpenCode, Claude Code, Copilot CLI, and Antigravity CLI separately. There’s also a small examples commit to keep the Copilot CLI example policies in sync with the package configs.

The final commit is a review follow-up across the affected parsers. It tightens the flag handling for longer Unix clusters like rm -rfv, preserves PowerShell abbreviations like -r -fo, and avoids false positives for ordinary parameters such as -Confirm and -Filter.

Arvid Peldan (Peldan) · 2026-07-04T08:40:00Z

@microsoft-github-policy-service agree company="Regent"

Imran Siddique (imran-siddique)

Reviewed across all four bundles (claude-code, opencode, copilot-cli, antigravity) plus the examples/copilot-cli-agt config mirror. The design is sound and fail-safe: the regex is reduced to a broad delete-command trigger (^|[\s;&|()])(?:rm|remove-item|...)\b and the actual deny decision is delegated to the new hasRecursiveForceDelete() parser. shouldBypassBlockedCommandRule now denies only when the command is genuinely recursive and force AND is not a safe-cleanup target.

I worked through the notable edge cases in getRmCommandDetails:

Split / reordered / clustered short options (rm -r -f, rm -fr, rm -rfv, rm target -rf), long options (--recursive --force in either order), PowerShell -Recurse/-Force and their unambiguous abbreviations (-r, -fo), and Windows rd /s /q all resolve to recursive+force → deny.
-f alone is correctly gated out as force (the PowerShell force check requires length >= 2), and node_modules/build/dist, rm -f, rm --recursive alone, -Confirm/-Filter correctly fall through as not-denied.

Importantly this does not introduce a fail-open: a destructive form the parser fails to classify reverts to the normal shell-tool review path rather than being auto-allowed, and the leading (^|[\s;&|()]) anchor prevents substring false-matches (charm, warm). The PR also correctly identifies the pre-existing \b-rf\b word-boundary bug that meant the canonical rm -rf target never fired in several bundles — this genuinely fixes that gap.

Tests are comprehensive (positive deny + negative allow lists) and mirrored per package; CI green. Careful, well-scoped security hardening. LGTM.

MohammadHaroonAbuomar

Regressions vs the previous denylist:

Left-anchor: (^|[\s;&|()]) misses `rm -rf /` and {rm -rf /;} which the old \b caught. Add ` and { to the class in all 14 JSONs.
Recursive-without-force is now allowed on antigravity/copilot (a test asserts rm --recursive important-data passes). That is a loosening; restore the deny or own it explicitly in the title/body.
isUnixRmShortOptionCluster fails open on unknown letters (rm -rfI foo slips through). Invert to single-dash-letters-only.
Restore the trailing newline in agent-governance-opencode/lib/policy.mjs.

Follow-up to the recursive-delete hardening, addressing review feedback on PR microsoft#3251. Applied to all four bundles (claude-code, opencode, copilot-cli, antigravity) plus the examples config mirror. - Trigger anchor: add ` and { to the delete-command character class in all 14 policy JSONs so `rm -rf /` and {rm -rf /;} are matched, which the previous class dropped. Also add normalizeCommandNameToken() so the parser un-glues leading `, {, (, $ from the command token; without it a matched trigger still bypassed the deny (commandIndex === -1). - Recursive-without-force: restore the deny. hasRecursiveForceDelete -> hasRecursiveDelete now gates on recursive alone (force is still parsed but no longer required), so `rm --recursive`, `rm -r`, and `Remove-Item -Recurse` are denied again unless the target is a safe-cleanup dir. - Short-option cluster: invert isUnixRmShortOptionCluster from the allow-list /^-[dfiprvw]+$/ to letters-only /^-[a-z]+$/, so a cluster with an unanticipated letter (e.g. `rm -rfx foo`) no longer fails open and drops its r/f flags. - Restore the trailing newline in agent-governance-opencode/lib/policy.mjs. Tests updated per bundle: recursive-only and delimiter-wrapped forms moved to the deny lists, a genuine fail-open cluster (-rfx) added as a regression guard, and the artificial bash -Confirm negative replaced with `rm -i` (PowerShell -Confirm/-Filter negatives kept under their proper surface). All four suites pass (npm run check, exit 0). Signed-off-by: arvidpeldan <peldans@gmail.com>

github-actions · 2026-07-07T08:02:12Z

🤖 AI Agent: test-generator — `agent-governance-antigravity-cli/assets/extensions/agt-global-policy/lib/policy.mjs`

AI-generated review output. Treat it as untrusted analysis and verify before acting.

`agent-governance-antigravity-cli/assets/extensions/agt-global-policy/lib/policy.mjs`

hasRecursiveDelete_invalid_command_syntax -- Test for invalid command syntax that should not trigger a recursive delete detection.
getRmCommandDetails_unexpected_command_format -- Validate handling of unexpected or malformed command formats.
isSafeCleanupCommand_edge_cases -- Test edge cases for isSafeCleanupCommand with unusual but valid command structures.
splitCommandSegments_complex_chains -- Test splitting of command chains with complex combinations of operators and quoted strings.
parseWindowsSlashFlags_unexpected_flags -- Validate parsing of unexpected or malformed Windows-style slash flags.

`agent-governance-antigravity-cli/assets/extensions/agt-global-policy/config/default-policy.json`

commandPatterns_edge_cases -- Test for edge cases in the updated regex patterns for command matching.

`agent-governance-antigravity-cli/assets/extensions/agt-global-policy/config/profiles/advisory.json`

commandPatterns_edge_cases -- Test for edge cases in the updated regex patterns for command matching.

`agent-governance-antigravity-cli/assets/extensions/agt-global-policy/config/profiles/balanced.json`

commandPatterns_edge_cases -- Test for edge cases in the updated regex patterns for command matching.

`agent-governance-antigravity-cli/assets/extensions/agt-global-policy/config/profiles/strict.json`

commandPatterns_edge_cases -- Test for edge cases in the updated regex patterns for command matching.

github-actions · 2026-07-07T08:02:14Z

🤖 AI Agent: code-reviewer — View details

AI-generated review output. Treat it as untrusted analysis and verify before acting.

TL;DR: 0 blockers, 1 warning. The recursive delete policy improvement is solid but could benefit from additional test coverage for edge cases.

#	Sev	Issue	Where
1	Warn	Edge cases for recursive delete detection may lack test coverage (e.g., complex command chaining).	Tests

Action items: Add tests for edge cases like chained commands (rm -rf && rm -r), mixed safe/unsafe targets, and ambiguous flag parsing.

Warnings: fine as follow-up PRs.

github-actions · 2026-07-07T08:02:15Z

🤖 AI Agent: breaking-change-detector — API Compatibility

AI-generated review output. Treat it as untrusted analysis and verify before acting.

API Compatibility

Severity	Change	Impact
High	The function signature of `shouldBypassBlockedCommandRule` was changed from `shouldBypassBlockedCommandRule(rule, commandText)` to `shouldBypassBlockedCommandRule(rule, commandText, toolName)`.	This is a breaking change for any external code or modules that rely on the previous function signature.
High	The function `isSafeCleanupCommand` now requires an additional `toolName` parameter.	This is a breaking change for any external code that calls this function without the new parameter.
High	The function `hasUnsafeRecursiveDelete` was introduced, and it calls `hasRecursiveDelete` and `isSafeCleanupCommand`.	If external code previously relied on the absence of this function or its behavior, this could be a breaking change depending on its usage.
High	The function `hasRecursiveDelete` was introduced and is now used in the deny decision logic.	This could alter the behavior of existing policies, potentially causing previously allowed commands to be denied.
High	The function `getRmCommandDetails` was introduced and replaces parts of the logic in `isSafeCleanupCommand`.	This could change the behavior of existing functionality, leading to potential breaking changes in how recursive delete commands are identified and handled.

github-actions · 2026-07-07T08:02:15Z

🤖 AI Agent: docs-sync-checker — Docs Sync

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Docs Sync

hasRecursiveDelete() in policy.mjs -- missing docstring
README.md -- section on "Command Deny Rules" needs update to reflect changes in recursive delete policy
CHANGELOG.md -- missing entry for changes to recursive delete policy logic

github-actions · 2026-07-07T08:02:16Z

🤖 AI Agent: security-scanner — Security Review

AI-generated review output. Treat it as untrusted analysis and verify before acting.

Security Review

Severity	Finding	Fix
High	The updated regex pattern in `commandPatterns` may overmatch and unintentionally block commands that are not intended to be denied. The new pattern `(^	[\s;&
Medium	The `getRmCommandDetails` function parses command tokens but may not handle all edge cases of command-line input, such as malformed commands or unexpected input formats. This could lead to bypasses or false positives/negatives.	Add comprehensive test cases for edge cases, including malformed commands and unexpected input formats, to ensure robust parsing.
Medium	The `splitCommandSegments` function may not handle all edge cases of command chaining and quoting, potentially allowing malicious commands to bypass detection.	Enhance the parsing logic to handle complex command chaining and quoting scenarios more robustly. Add test cases for various edge cases.
Medium	The `isPowerShellTool` function relies on a regex match for tool names, which may not cover all possible PowerShell aliases or variations. This could lead to incorrect parsing of PowerShell commands.	Expand the regex to include all known PowerShell aliases and variations, or use a more robust method to identify PowerShell tools.
Medium	The `parseWindowsSlashFlags` function assumes that any token starting with `/` and containing only letters is a valid flag. This could lead to misinterpretation of valid file paths as flags.	Add stricter validation to distinguish between valid Windows flags and file paths that start with a slash.
Medium	The `isUnixRmShortOptionCluster` function matches any single-dash short-option cluster with letters, which could lead to false positives or misinterpretation of valid commands.	Implement a more precise validation for Unix short-option clusters, possibly by maintaining an allow-list of known valid flags.

Arvid Peldan (Peldan) · 2026-07-07T08:30:14Z

Regressions vs the previous denylist:

Left-anchor: (^|[\s;&|()]) misses `rm -rf /` and {rm -rf /;} which the old \b caught. Add ` and { to the class in all 14 JSONs.

Recursive-without-force is now allowed on antigravity/copilot (a test asserts rm --recursive important-data passes). That is a loosening; restore the deny or own it explicitly in the title/body.

isUnixRmShortOptionCluster fails open on unknown letters (rm -rfI foo slips through). Invert to single-dash-letters-only.

Restore the trailing newline in agent-governance-opencode/lib/policy.mjs.

Thanks! All four addressed in bb6f6c8:

Left-anchor: Added ` and { to the trigger class in all 14 JSONs. This alone wasn't enough, though: the parser's tokenizer kept `rm/{rm/$(rm as one glued token, so a matched trigger still bypassed the deny (commandIndex === -1). Added normalizeCommandNameToken() to strip leading ` { ( $, so `rm -rf /`, {rm -rf /;}, and $(rm -rf /) are now actually denied.
Recursive-without-force: Restored the deny. hasRecursiveForceDelete → hasRecursiveDelete gates on recursive alone now, so rm --recursive / rm -r / Remove-Item -Recurse are denied unless the target is a safe-cleanup dir. Applied to all four bundles for consistency, and the PR description now states this explicitly.
Cluster fails open: Inverted /^-[dfiprvw]+$/ → /^-[a-z]+$/. Small note: -rfI was actually already caught (i is in the old set), so the real fail-open is a letter outside it! The regression test uses rm -rfx foo, which slipped before and denies now.
Trailing newline: Restored in agent-governance-opencode/lib/policy.mjs.

Signed-off-by: arvidpeldan <peldans@gmail.com>

Imran Siddique (imran-siddique)

The four requested changes are all present at head (backtick/brace anchors, recursive-without-force now denies, letter-only cluster parsing, trailing newline), and the parser-based approach is fail-closed. The blocking review is stale and should be re-reviewed or dismissed.

Copilot

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (5)

agent-governance-copilot-cli/assets/extensions/agt-global-policy/lib/policy.mjs:1171

parseWindowsSlashFlags() currently treats any token made only of / + letters as a Windows switch bundle (e.g. it matches /srv/emptydir). When the delete command is rmdir/rd/del/erase, this can misread Unix absolute paths as flags (the s in /srv sets recursive: true), causing non-recursive deletes like rmdir /srv/emptydir to be hard-denied.