fix(factory): enforce the prompt refuse-lists with --disallowedTools - #619
Open
JakubAnderwald wants to merge 1 commit into
Open
JakubAnderwald wants to merge 1 commit into
JakubAnderwald wants to merge 1 commit into
Conversation
Every factory prompt carries a prose "Refuse:" list, and nothing enforced any of it. All five Claude invocations run --dangerously-skip-permissions with no tool flags at all, so the "allow-list" was markdown honoured by the model's compliance alone — a drifting run could merge a PR, dispatch a release, or reset its own retry budget with nothing in the way. An allow-list does not work here. Measured against the installed CLI: --allowedTools is ignored entirely under --dangerously-skip-permissions, and merely additive without it; Bash ran in both cases. --disallowedTools is the only lever that restricts, and it composes with the bypass flag. Four further probes shaped the design: multi-word prefixes match (Bash(git log:*) denies git log, not git status); compound commands are matched per clause (cd x && ls trips Bash(ls:*)); Task subagents INHERIT the deny set, so it cannot be bypassed by delegation; but `:*` does not span a colon-suffixed token, so Bash(pnpm release:*) is inert against `pnpm release:beta` — the chokepoint has to be fastlane/xcodebuild/gradlew, which are invoked name-first. And a malformed pattern is silently ignored, which is why each stage logs its deny set and the tests validate syntax. Deny sets are layered: a shared core (release verbs, host control, shell escapes, WebFetch/WebSearch, and each dangerous scripts/lib CLI in both relative and absolute form), plus history-rewriting git verbs on the coding stages, plus wholesale git/pnpm/node on the read-only stages. The two core patterns that matter most stop an agent resetting its own retry budget (state-cli) or writing its own board Status (factory-project) — the loop guard the pipeline depends on. Plain git push, pnpm, gh pr create/view/edit and the screenshot curl carve-out stay allowed on the coding stages; a test pins those as UNSAFE-to-deny so a future tightening cannot brick the pipeline overnight. Since git checkout and git reset are now blocked, git restore / git restore --staged were added to the implement and watch allow-lists, and gh pr edit was added to the implement tools list — the revision-run instructions already required it. This is defence-in-depth against drift, NOT a sandbox: gh api can mutate with no method flag, and argument-value and cardinality constraints are inexpressible. ADR-0034 says so explicitly. See ADR-0034. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lw3K4txt5h2GQmWvAAw4Ju
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This branch was successfully deployed
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
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.
Why
Every factory prompt carries a prose "Refuse:" list — no
gh pr merge, nogh workflow run, no fastlane, noclaude/node scripts/...subprocess. Nothing enforced any of it. All five Claude invocations run--dangerously-skip-permissionswith no tool flags at all, so the "allow-list" was markdown honoured by model compliance alone. A drifting run could merge a PR, dispatch a release, or reset its own retry budget with nothing in the way.The intuitive fix doesn't work
Measured against the installed CLI, not assumed:
--dangerously-skip-permissions --allowedTools "Read"--allowedTools "Read"(no bypass flag)--dangerously-skip-permissions --disallowedTools "Bash"--dangerously-skip-permissions --disallowedTools "Bash(ls:*)"lsdenied,echoranSo enforcement is a deny-list, and it composes with the bypass flag. Four more probes shaped the design:
Bash(git log:*)deniesgit log,git statusstill runs.cd /tmp && lstripsBash(ls:*).Tasksubagents inherit the deny set — not bypassable by delegation, which matters most on the twoultracodestages that fan out.:*does not span a colon-suffixed token —Bash(pnpm release:*)is inert againstpnpm release:beta. The chokepoint has to befastlane/xcodebuild/gradlew, invoked name-first.Bash(ls:*(unclosed) letlsrun with no error.What changed
Deny sets built as plain comma-separated bash strings next to the effort block (never arrays — an empty one under
set -uthrows on the mini's bash 3.2), layered:bash -c),WebFetch/WebSearch, and each dangerousscripts/lib/*.mjsCLI in both relative and$REPO_ROOT-absolute form.--implement/--watch) — plus history-rewriting git verbs. Plaingit pushandpnpmstay allowed; they're how work lands and gets verified.--plan/replan, In Test) — plusgit,pnpm,nodewholesale, andEdit. In Test also deniesWrite("you post one comment; that is your entire write surface"); plan keeps it for/tmp/factory-replan-body.md.The two highest-value patterns stop an agent running
state-clito reset its own retry budget, andfactory-projectto write its own board Status — the loop guard the pipeline depends on.Because malformed patterns fail silently, each stage logs the set it passed and the tests validate every pattern's syntax.
Guardrails against over-tightening
The realistic failure mode is denying something a stage needs and finding out at 03:00.
factory-deny-grounding.test.mjscarries an explicitUNSAFEtable per stage —Bash(git push:*),Bash(gh pr:*),Bash(pnpm:*),Bash(curl:*),Task,Write— and fails if any appears in that stage's set.Since
git checkout/git resetare now blocked,git restore/git restore --stagedwere added to the implement and watch allow-lists.gh pr editwas added to the implement tools list — the revision-run instructions at line 179 already required it but the list omitted it, a pre-existing inconsistency.Not a sandbox
Stated in the ADR because the headline invites the wrong reading.
gh apican mutate with no method flag (gh api repos/x/y/issues -f title=z,gh api graphql -f query='mutation{…}'); argument-value and cardinality constraints are inexpressible;env/xargsindirection isn't covered. The controls that hold are architectural and predate this: bash validates the summary line, owns every board write and the merge.Testing
845 tests pass (from 828 — 17 new), lint 0 errors, typecheck 4/4, format clean,
bash -nclean.Verified end-to-end with the real
FACTORY_DENY_PLANstring, not a synthetic one:One existing test needed updating:
factory-agent-effort.test.mjspinned--dangerously-skip-permissions --effortas adjacent strings, and the deny flag now sits between them. Rewritten to match per invocation line rather than by adjacency.See ADR-0034.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Lw3K4txt5h2GQmWvAAw4Ju