Add ADR for allowed and disallowed tools for agents#293
Conversation
Site previewPreview: https://1dc74fd6-site.fullsend-ai.workers.dev Commit: |
|
@maruiz93; Do you know about |
| - Parameterized restrictions (e.g. `Bash(git push *)`) become enforced by | ||
| Claude Code's permission system instead of prompt-only, closing the | ||
| prompt-injection bypass path. | ||
| - The `permissions.allow` list must be maintained — if a legitimate tool pattern |
There was a problem hiding this comment.
Not actionable; merely a comment.
For this to work seriously as a security mechanism, the allowlist must be quite fine-grained, on the order of, in the case of my personal use, over 100 distinct rules...
It's not a small job to maintain this.
There was a problem hiding this comment.
hmm, I was thinking that we might want for each agent a small amount of tools to be used
There was a problem hiding this comment.
That's ideal, but in practice it's difficult.
There was a problem hiding this comment.
So, this is based on my understanding (which could be incorrect), and is based on my personal interactive use, but I think the reasoning still applies...
On the deny side, the following are necessary to prevent Claude Code from doing bulk adds and not thinking about how to split things up into atomic commits (among other things):
"Bash(git add --all:*)",
"Bash(git add --force:*)",
"Bash(git add -A:*)",
"Bash(git add -f:*)"
tl;dr - blocking "Bash(git add :*)" is insufficient (again, IIUC)
On the allow side, it's arguably more difficult, because you don't want to "open the door" too far, so you end up with even more granularity, e.g.
"Bash(gh api graphql --raw-field:*)",
"Bash(gh issue view:*)",
"Bash(gh pr checkout:*)",
"Bash(gh pr checks:*)",
"Bash(gh pr diff:*)",
"Bash(gh pr view:*)",
"Bash(gh repo view:*)",
"Bash(git apply:*)",
"Bash(git checkout:*)",
"Bash(git cherry-pick:*)",
"Bash(git commit:*)",
"Bash(git fetch:*)",
"Bash(git log:*)",
"Bash(git rebase:*)",
"Bash(git rev-parse:*)",
"Bash(git show:*)",
"Bash(git stash:*)",
"Bash(git status:*)"
There was a problem hiding this comment.
you are right, I did more experimentation and came to the conclussion that the best thing would be relying on the sandbox for security, and using the permissions.deny eventually on the agents for steering, as a way to redirect the agent when doing something we don't want it to do as soon as possible, saving context and tokens. PR is ready for review again. This is the related experiment I did: fullsend-ai/experiments#3
3b38f05 to
b320c1e
Compare
|
@ben-alkov I think I've made some assumptions that I'm not sure about and I don't have enough data in the docs, I'll run some experiments along this PR. Moving to draft |
b320c1e to
0b3612f
Compare
0b3612f to
03a3798
Compare
03a3798 to
01aab48
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
01aab48 to
90de6d1
Compare
|
Can I get some reviews @ben-alkov ? |
rh-hemartin
left a comment
There was a problem hiding this comment.
OK, so sandbox as security and the other stuff as steering, that sounds good. In practice we may never use the steering, as steering also comes from skills and that may be enough.
hmm, right the skills frontmatter include I think everything should work together and eliminating from the agent the posibility to run a specific thing, even if this doesn't forbid to write its own tool, it's a greate way of steering. Also, note that the fullsend agents that are provided by the fullsend agent are compatible with skills in the target repo, but the agent permissions.allow/permissions.deny has more priority than the skills To sum up, I believe that fullsend agents should include |
0024 was already taken on main by the harness-definitions ADR. Renumber to 0026 (next available) and update all cross-references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Review: #293Head SHA: f85900a SummaryClean documentation PR that adds ADR 0027 deciding on FindingsNo findings. FooterOutcome: approve Previous runReview: #293Head SHA: f08648d SummaryThe ADR is well-reasoned and the experimental evidence is valuable, but the PR has two issues that need resolution before merging. First, there is an ADR number collision: ADR 0026 already exists on FindingsCritical
High
Info
FooterOutcome: request-changes |
0026 was merged to upstream main (stage-based-dispatch ADR) since the last renumber. Bump to 0027 and update all cross-references. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
toolsanddisallowedToolsfrontmatter have no effect in--agentsessions (only work for subagents);permissions.denyis enforced even with--dangerously-skip-permissions;bypassPermissionsis a partial bypass--dangerously-skip-permissions+permissions.denyfor steering, with sandbox as sole security layerTest plan
🤖 Generated with Claude Code