Repository navigation
Make the forbidden-path block depth-agnostic for bare relative globs (#177) - #178
Conversation
…177) The sticky forbidden-path BLOCK matched operator-configured globs via the shared `glob_match`, which only special-cases a leading `**/`. So a bare relative pattern like `secrets/**` or `migrations/**` matched only a top-level dir: a nested `app/secrets/key.pem` slipped past the sticky BLOCK with merely the softer sensitive-area REVIEW_REQUIRED escalation. The built-in defaults dodged this only by shipping explicit `**/...` twins. Add `engines.risk.forbidden_path_match`: a bare relative pattern is treated as depth-agnostic (matched at any directory depth), while a rooted (`/...`) or already-anchored (`**/...`) pattern is honoured as written. It matches a strict superset of `glob_match`, so it can only ever make the block stricter (invariant #1) and leaves default-config behaviour byte-for-byte unchanged. Deliberately not folded into `glob_match`, which also backs `files_outside_scope` (plan-scope drift) where matching more paths would *weaken* that escalation. Wire it into `_forbidden_violations` only. Forbidden-path blocking is orchestrator-only with no Rego mirror, so no gate/PolicyInput/Rego/schema change (invariant #2 doesn't apply). Adds a unit test for the matcher and an end-to-end test that a custom bare `secrets/**` hard-blocks a nested diff. AGENTS.md updated. Closes #177 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SvTTiFQsksv1Pv69CHPw25
JeremySNR
left a comment
There was a problem hiding this comment.
Self-review — what a human reviewer should double-check:
1. Semantic breadth of forbidden_path_match for bare globs. The core decision is that a bare relative pattern is matched at any depth. This is intentional and stricter-is-safe for a forbidden list, but it does broaden matching beyond what the operator literally typed:
secrets/**now also blocksapp/secrets/…,services/x/secrets/…, etc. (the fix's whole point). ✅- A bare filename glob like
Dockerfileor*.pemnow also matches nested (build/Dockerfile). For a forbidden list this is the desired direction, but confirm no one was relying on a bare filename meaning "root only." The escape hatch is documented and tested: a rooted/Dockerfilepins it to the repo root. - I verified
secrets/**does not match sibling-prefixed dirs likesrc/secretsmanager/util.py(segment-boundary, not substring) — test covers it.
2. Scope containment — why glob_match was left alone. glob_match also backs files_outside_scope (plan-scope-drift escalation), where more matching means fewer files flagged outside scope → a weaker escalate-only gate. Making the shared matcher permissive would silently relax that gate (invariant #1 violation). I confirmed forbidden_path_match is used only at the single forbidden enforcement site (_forbidden_violations); the path_required_roles matcher at the same layer still uses glob_match (correct — that path is a different, unrelated rule). Please sanity-check there's no other consumer that should arguably get the depth-agnostic treatment.
3. No gate-contract change. Forbidden-path blocking is orchestrator-only with no Rego mirror, so there is no foundry.rego / PolicyInput / policy_vectors edit and invariant #2's lock-step doesn't apply. Nothing in the policy engine or schemas changed.
4. Default globs untouched. I kept the redundant **/… twins in DEFAULT_FORBIDDEN_GLOBS rather than pruning them — removing entries would churn the policy-comparison / preset-strictness tests for no behavioural gain, and they remain valid belt-and-suspenders.
CI note: locally the full offline suite is green (1612 passed); the OIDC/encryption/PDF modules can't import in my sandbox due to a cryptography Rust-binding panic (a system/pip version conflict in the sandbox, not this diff — those files are untouched). CI runs them in a clean environment, so watch those jobs here.
Generated by Claude Code
Closes #177.
Problem
The forbidden-path block — the sticky, non-retryable
BLOCKEDgate, the strongest action the control plane takes on a diff — silently under-matched an operator-configured bare relative glob against nested paths._forbidden_violationsmatched via the sharedengines.risk.glob_match, which only special-cases a leading**/.fnmatchanchors at the string start, so:app/secrets/key.pemservices/api/migrations/0001.pysecrets/**(bare)migrations/**(bare)**/secrets/**The built-in
DEFAULT_FORBIDDEN_GLOBSdodged this only because they were hand-authored to ship both variants (added for #22). But an operator who overridespolicy.forbidden_globs(or addspolicy.repo_forbidden_globs) with the natural["secrets/**"]got a gate that hard-blocks a top-levelsecrets/change but lets a nestedapp/secrets/key.pemthrough with only the softer sensitive-areaREVIEW_REQUIRED— not the sticky BLOCK they configured.foundry-policy explainshows the glob as present, reinforcing a false sense of coverage.Change
engines.risk.forbidden_path_match— a bare relative pattern is treated as depth-agnostic (secrets/**blocksapp/secrets/key.pem); a rooted (/…) or already-anchored (**/…) pattern is honoured exactly as written. It returns a strict superset ofglob_match._forbidden_violationsonly (the single forbidden-path enforcement site).glob_matchleft unchanged. It also backsfiles_outside_scope/_scope_entry_covers(plan-scope drift), where matching more paths marks more files "in scope" → fewer escalations → a weaker gate. So the fix is deliberately forbidden-path-specific, not a change to the shared matcher.config.py/planner.pyupdated to note the default**/…twins are now belt-and-suspenders;AGENTS.mdmodule-map row updated per the maintenance rule.Why it's safe (invariants)
glob_match→ forbidden matching only ever gets stricter; default-config behaviour is byte-for-byte unchanged (defaults already match at depth).foundry.rego/PolicyInput/policy_vectorschange. No schema change either.Tests
test_forbidden_path_match_is_depth_agnostic_for_bare_globs— bare relative matches at depth; rooted/anchored honoured; superset ofglob_match; genuine non-matches (secretsmanager/) stay non-matches.test_custom_bare_forbidden_glob_blocks_a_nested_path— end-to-end: a customsecrets/**hard-blocks a nestedapp/secrets/…diff, and the block stays sticky.ruffclean.Risks / follow-up
Dockerfile) now also matches at any depth (build/Dockerfile). That is the intended stricter-is-safe direction for a forbidden list; a rooted/Dockerfilestill pins it to the root.cryptographyRust-binding panic (system/pip version conflict); those tests are unrelated to this change and untouched by it.🤖 Generated with Claude Code
https://claude.ai/code/session_01SvTTiFQsksv1Pv69CHPw25
Generated by Claude Code
Note
Low Risk
Stricter-only orchestrator change at a single call site; no Rego or policy contract change. Bare filename globs (e.g.
Dockerfile) now match at any depth, which is intentional for forbidden lists.Overview
Fixes #177: operator bare relative forbidden globs (e.g.
secrets/**,migrations/**) no longer only match repo-root paths. The sticky forbidden-path block now uses a dedicatedforbidden_path_matchmatcher so nested paths likeapp/secrets/key.pemhard-BLOCK instead of slipping through with only sensitive-area escalation.glob_matchis unchanged — it still backs plan-scope drift and path approval roles, where broader matching would weaken gates.Docs/comments in AGENTS.md, config.py, and planner.py note that default
**/…twins are belt-and-suspenders under the new matcher. Unit tests cover the matcher; an orchestrator test asserts customsecrets/**blocks nested paths and stays sticky.Reviewed by Cursor Bugbot for commit 9bc7f7d. Bugbot is set up for automated code reviews on this repo. Configure here.