Skip to content

feat(guardrails): allow name-prefix entries in block_root_delete_target_allowed_roots - #6624

Merged
kyle-sexton merged 4 commits into
mainfrom
fix/6542-allowed-roots-prefix
Oct 8, 2026
Merged

kyle-sexton merged 4 commits into
mainfrom
fix/6542-allowed-roots-prefix

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

Closes #6542

Summary

block-root-delete-target refused agents' own throwaway test directories (D:/worktrees/.tmp-<n>), and block_root_delete_target_allowed_roots could not express a narrow allowance: a glob entry granted nothing, and listing D:/worktrees would allow deleting every worktree.

Fix

An allowed_roots entry ending in one * after a literal name (for example D:/worktrees/.tmp-*) is now a name-prefix entry. A target passes only when its real path is a direct child of the entry's directory, its name starts with the prefix and has at least one more character, and it is not itself a symlink. Still refused: the directory, the bare prefix, a sibling, anything below a matching child, a glob of the prefix, .. escapes, and a matching symlink with or without a trailing slash. The entry's directory follows the existing entry rules (absolute, no .., no other glob character, never a filesystem root or HOME), and the key is still read only from the hook's own environment.

Why the * form: an entry holding a glob character granted nothing before (README Option details), so no entry that already allowed something changes meaning. A plain trailing - form would have reinterpreted an existing directory entry whose name ends in -.

README option docs, the plugin.json userConfig description, the generated options table and a minor changelog fragment are updated.

Verification

  • plugins/guardrails/hooks/block-root-delete-target.test.sh on Windows Git Bash: the new prefix cases (Bash, PowerShell, symlink, drive-form entry with MSYS-form operand) pass.
  • Repro-first: against the origin/main hook the 12 new allow assertions fail (expected exit 0, got 2), including the .tmp-6527 child with the prefix listed.
  • 4 failures pre-exist identically on origin/main on this host (a relative root grants nothing, the root '/' does not allow a top-level path); they are unrelated to this change.
  • shellcheck clean; changelog parity (--check, --check-order, --check-bump, --check-preserved), check-changelog-fragments.sh --check-required and sync-shared-copies.sh --check pass.

Related

🤖 Generated with Claude Code

https://claude.ai/code/session_01EC7nAKxQg52rCug6r6tYfe

kyle-sexton and others added 2 commits October 7, 2026 19:55
…et_allowed_roots

An allowed_roots entry ending in one `*` after a literal name, such as
`D:/worktrees/.tmp-*`, now lets a recursive delete through for a direct
child of that directory whose name extends the prefix by at least one
character, compared by real path. The directory, the bare prefix, a
sibling, anything below a matching child, a glob, a `..` escape and a
matching symlink stay refused. Entries holding a glob character granted
nothing before, so no effective entry changes meaning.

Closes #6542

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EC7nAKxQg52rCug6r6tYfe
…ing-slash prefix targets

Security review of the name-prefix entry: Win32 trims a trailing dot or
space, so a name like `.tmp-.` reached the bare prefix and `.tmp-n.`
reached a symlink past the -L test. A trailing-slash operand whose child
does not exist at hook time could name a link the same command creates,
which rm then follows. Both now stay refused for name-prefix entries.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EC7nAKxQg52rCug6r6tYfe
@kyle-sexton
kyle-sexton marked this pull request as ready for review October 8, 2026 01:24
@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Security review in progress

  • Gather context (diff, changed files)
  • Check skip gate (not eligible to skip — this is a security guardrail hook change)
  • Review guardrails hook logic for security-relevant flaws
  • Check for instruction-surface deletions against protected classes (this PR only adds a narrower allowance mechanism; no existing guardrail rule is removed or softened)
  • Adversarial validation of candidate findings
  • Post findings / summary

View job run

@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 10m 26s —— View job


Code review in progress

  • Gather context (diff, changed files, CLAUDE.md/REVIEW.md)
  • Deep-dive the core rdt_allowed/pdir matching logic in block-root-delete-target.sh
  • Hunter agents reviewing hook logic edge cases and test/docs consistency
  • Adversarial verification of any candidate findings
  • Post findings

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-08T01:29:41.383653Z 526260e Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

claude-review has reviewed this pull request through 9f73b6a; a later push is reviewed from there.

…er budget

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EC7nAKxQg52rCug6r6tYfe
@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 10m 48s —— View job


Security review in progress

  • Gather context (diff, changed files)
  • Check skip gate (not eligible to skip — this is a security guardrail hook change)
  • Deep-dive rdt_allowed/pdir matching logic, parsing, symlink/trailing-dot/trailing-slash handling in block-root-delete-target.sh
  • Check instruction-surface deletions against protected classes (this PR only adds a narrower, opt-in allowance; no existing guardrail rule is removed, narrowed, or softened — not a register match)
  • Hunter agent double-checks the new logic for anything missed
  • Adversarial validation of any candidate findings
  • Post findings / summary

@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

Scope: incremental — this lane last reviewed 526260e. Per the task's instructions, only the changes since then were reviewed: plugins/guardrails/.claude-plugin/plugin.json and plugins/guardrails/README.md, both touched only to shorten the block_root_delete_target_allowed_roots option description (commit f039948, "fit the allowed_roots description in its 300-character budget"). The hook script and its test file are unchanged since 526260e and were covered by that earlier review.

Finding

The shortened description still doesn't fit the budget it's meant to fit. Posted as inline comments on both occurrences:

The new description text is 309 characters, 9 over the DESCRIPTION_BUDGET = 300 that scripts/validate-plugin-contracts.mjs (lines 1282, 1313-1315) enforces on every userConfig option description. I verified the count directly (printf '%s' "<text>" | wc -m → 309); this isn't restating the PR's own claimed verification, which doesn't mention this check at all.

Both files carry the identical text, so both need trimming together to stay in sync.

Other checks

  • No markdown links, backticks, **, *emphasis*, or em dashes in the new text — the description otherwise clears the other optionDescriptionProblems checks in the same validator.
  • The plugin.json description and the README table row remain textually identical to each other, consistent with how this repo keeps those two in sync.
  • No behavior change in this diff — docs/description text only.

Comment thread plugins/guardrails/.claude-plugin/plugin.json Outdated
Comment thread plugins/guardrails/README.md Outdated
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review has reviewed this pull request through 9f73b6a; a later push is reviewed from there.

@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 35s —— View job


Security review complete

Scope: incremental — this lane last reviewed f039948. Per the task's instructions, only the changes since then were reviewed, read from .claude-lane/incremental.diff: plugins/guardrails/.claude-plugin/plugin.json and plugins/guardrails/README.md. Both files were touched only to reword the block_root_delete_target_allowed_roots option description (shortening it further after the prior claude-review pass flagged it as over the 300-character budget). The hook script (block-root-delete-target.sh) and its test file are unchanged since f039948 and were covered by the earlier security review of this PR.

Skip gate

This increment qualifies for the skip gate: no security-relevant surface.

  • It is a pure text edit to a userConfig description string and the matching README table cell — no executable code, no hook logic, no test changed.
  • It is not an instruction-surface deletion/narrowing/softening: the description's meaning is unchanged — "block-root-delete-target allows a recursive delete whose real path is strictly under one" became "a recursive delete passes when its real path is strictly under one." Same restriction, same scope, same name-prefix carve-out sentence carried over verbatim. Nothing the hook enforces is described more permissively, and the enforcement itself (block-root-delete-target.sh) is untouched by this increment.
  • Compared against the instruction exception register classes: this isn't a guardrail rule file, hook block/allow list, or standing-instruction document being edited — it's descriptive metadata about an existing, unchanged option. I did not need the register fallback since the file is a documentation/description change, not a rule deletion.

Finding

None. No security issues in this increment.

@claude

claude Bot commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

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


Code review complete

Scope: incremental — this lane last reviewed f039948. Per the task's instructions, only the changes since then were reviewed: plugins/guardrails/.claude-plugin/plugin.json and plugins/guardrails/README.md, both touched only in commit 9f73b6a ("trim the allowed_roots description to 287 characters"). The hook script and its test file are unchanged since 526260e and were covered by that earlier review.

  • Gather context (incremental diff, changed files)
  • Review the trimmed block_root_delete_target_allowed_roots description against the 300-character budget flagged by the prior review
  • Check the description text against the other optionDescriptionProblems rules (backtick, **, *emphasis*, markdown link, em dash)
  • Confirm the plugin.json description and the README table row stay textually identical
  • Post findings

Findings

None. This commit fixes the issue raised by the prior review round: the description is now 287 characters (verified directly: printf '%s' "<text>" | wc -m → 287), under the DESCRIPTION_BUDGET = 300 enforced by scripts/validate-plugin-contracts.mjs (lines 1282, 1313–1315).

  • The lone * in D:/worktrees/.tmp-* is a single asterisk with no closing partner, so it can't trip the SINGLE_ASTERISK_EMPHASIS regex (scripts/validate-plugin-contracts.mjs:1288), which requires a matching opening/closing pair.
  • No backtick, **, markdown link, or em dash in the new text.
  • plugins/guardrails/.claude-plugin/plugin.json:102 and the plugins/guardrails/README.md:1547 table row carry identical text, consistent with how this repo keeps those two in sync.
  • No behavior change in this diff — description text only.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Oct 8, 2026
Merged via the queue into main with commit 4d3442a Oct 8, 2026
26 checks passed
@kyle-sexton
kyle-sexton deleted the fix/6542-allowed-roots-prefix branch October 8, 2026 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(guardrails): block-root-delete-target refuses agent test temp dirs like D:/worktrees/.tmp-*

1 participant