feat(review): enforce reviewer read-only via tool allowlist
The reviewer's read-only guarantee was enforced only by the prompt. Add AllowedTools/DisallowedTools to ports.LaunchConfig and plumb them through the claude-code agent adapter to --allowedTools/--disallowedTools (each list comma-joined into one value so a rule containing spaces like "Bash(git diff:*)" is not split into separate tool names). Empty lists emit nothing, so worker sessions are unaffected. Launch the reviewer off bypassPermissions (which skips the permission system and ignores allow/deny rules) in the default auto mode, with an allowlist scoped to Read/Grep/Glob and the few Bash commands a reviewer needs (gh, git diff/log/show/status, ao review submit) and an explicit deny list for Edit/Write/NotebookEdit/git push/git commit as defense in depth. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
8bbc4c94fe
commit
cfe46072b9
|
|
@ -150,6 +150,7 @@ func (p *Plugin) GetLaunchCommand(ctx context.Context, cfg ports.LaunchConfig) (
|
|||
permissions = cfg.Config.Permissions
|
||||
}
|
||||
appendPermissionFlags(&cmd, permissions)
|
||||
appendToolFlags(&cmd, cfg.AllowedTools, cfg.DisallowedTools)
|
||||
|
||||
if model := strings.TrimSpace(cfg.Config.Model); model != "" {
|
||||
cmd = append(cmd, "--model", model)
|
||||
|
|
@ -313,6 +314,20 @@ func appendPermissionFlags(cmd *[]string, permissions ports.PermissionMode) {
|
|||
}
|
||||
}
|
||||
|
||||
// appendToolFlags emits --allowedTools / --disallowedTools for a tool-scoped
|
||||
// launch. Each list is joined with commas into one value so rules that contain
|
||||
// spaces (e.g. "Bash(git diff:*)") are not split into separate tool names.
|
||||
// Empty lists emit nothing, so an unrestricted launch is unchanged. These rules
|
||||
// only bite when the launch is off bypassPermissions, which ignores them.
|
||||
func appendToolFlags(cmd *[]string, allowed, disallowed []string) {
|
||||
if len(allowed) > 0 {
|
||||
*cmd = append(*cmd, "--allowedTools", strings.Join(allowed, ","))
|
||||
}
|
||||
if len(disallowed) > 0 {
|
||||
*cmd = append(*cmd, "--disallowedTools", strings.Join(disallowed, ","))
|
||||
}
|
||||
}
|
||||
|
||||
func normalizePermissionMode(mode ports.PermissionMode) ports.PermissionMode {
|
||||
switch mode {
|
||||
case ports.PermissionModeDefault,
|
||||
|
|
|
|||
|
|
@ -580,6 +580,38 @@ func readJSON(t *testing.T, path string) map[string]any {
|
|||
return m
|
||||
}
|
||||
|
||||
func TestGetLaunchCommandEmitsToolAllowlist(t *testing.T) {
|
||||
p := &Plugin{resolvedBinary: "claude"}
|
||||
|
||||
cmd, err := p.GetLaunchCommand(context.Background(), ports.LaunchConfig{
|
||||
AllowedTools: []string{"Read", "Grep", "Bash(git diff:*)"},
|
||||
DisallowedTools: []string{"Edit", "Write", "Bash(git push:*)"},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
|
||||
// Each list is one comma-joined value so a rule with spaces stays intact.
|
||||
if !containsSubsequence(cmd, []string{"--allowedTools", "Read,Grep,Bash(git diff:*)"}) {
|
||||
t.Fatalf("missing joined --allowedTools value; got %#v", cmd)
|
||||
}
|
||||
if !containsSubsequence(cmd, []string{"--disallowedTools", "Edit,Write,Bash(git push:*)"}) {
|
||||
t.Fatalf("missing joined --disallowedTools value; got %#v", cmd)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetLaunchCommandOmitsToolFlagsWhenUnset(t *testing.T) {
|
||||
p := &Plugin{resolvedBinary: "claude"}
|
||||
|
||||
cmd, err := p.GetLaunchCommand(context.Background(), ports.LaunchConfig{Prompt: "do it"})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if contains(cmd, "--allowedTools") || contains(cmd, "--disallowedTools") {
|
||||
t.Fatalf("unrestricted launch should emit no tool flags; got %#v", cmd)
|
||||
}
|
||||
}
|
||||
|
||||
func contains(values []string, needle string) bool {
|
||||
for _, v := range values {
|
||||
if v == needle {
|
||||
|
|
|
|||
|
|
@ -31,6 +31,37 @@ func (r *Reviewer) Harness() domain.ReviewerHarness {
|
|||
|
||||
var _ ports.Reviewer = (*Reviewer)(nil)
|
||||
|
||||
// reviewerAllowedTools is the read-only tool allowlist the reviewer launches
|
||||
// with. The reviewer runs headless (no human to approve prompts) but must stay
|
||||
// read-only, so instead of bypassPermissions — which skips the permission
|
||||
// system entirely and ignores allow/deny rules — it launches in the default
|
||||
// mode where these rules are honored: allow rules auto-approve without
|
||||
// prompting, so the reviewer can read the checkout and run the few commands it
|
||||
// needs (git diff/log/show to inspect the PR, gh to post the review, and
|
||||
// `ao review submit` to record the verdict) without stalling.
|
||||
var reviewerAllowedTools = []string{
|
||||
"Read",
|
||||
"Grep",
|
||||
"Glob",
|
||||
"Bash(gh:*)",
|
||||
"Bash(git diff:*)",
|
||||
"Bash(git log:*)",
|
||||
"Bash(git show:*)",
|
||||
"Bash(git status:*)",
|
||||
"Bash(ao review submit:*)",
|
||||
}
|
||||
|
||||
// reviewerDisallowedTools hard-denies the write paths as defense in depth, so a
|
||||
// misbehaving model cannot edit files or move the branch even if a future
|
||||
// allowlist entry would otherwise admit it.
|
||||
var reviewerDisallowedTools = []string{
|
||||
"Edit",
|
||||
"Write",
|
||||
"NotebookEdit",
|
||||
"Bash(git push:*)",
|
||||
"Bash(git commit:*)",
|
||||
}
|
||||
|
||||
// ReviewCommand builds a claude-code invocation that reviews the worker's
|
||||
// checkout for the PR, with the review prompt baked in.
|
||||
func (r *Reviewer) ReviewCommand(ctx context.Context, inv ports.ReviewInvocation) (ports.ReviewCommandSpec, error) {
|
||||
|
|
@ -39,10 +70,12 @@ func (r *Reviewer) ReviewCommand(ctx context.Context, inv ports.ReviewInvocation
|
|||
WorkspacePath: inv.WorkspacePath,
|
||||
Prompt: inv.Prompt,
|
||||
SystemPrompt: inv.SystemPrompt,
|
||||
// The reviewer runs headless with no human to approve tool prompts; it
|
||||
// is read-only by prompt and must run gh/ao on its own, so bypass the
|
||||
// permission gate rather than stall on the first prompt.
|
||||
Permissions: ports.PermissionModeBypassPermissions,
|
||||
// Launch off bypassPermissions so the allow/deny lists are enforced.
|
||||
// Set an explicit non-bypass mode instead of deferring to the user's
|
||||
// Claude defaultMode, which may itself be bypassPermissions.
|
||||
Permissions: ports.PermissionModeAuto,
|
||||
AllowedTools: reviewerAllowedTools,
|
||||
DisallowedTools: reviewerDisallowedTools,
|
||||
})
|
||||
if err != nil {
|
||||
return ports.ReviewCommandSpec{}, err
|
||||
|
|
|
|||
|
|
@ -0,0 +1,71 @@
|
|||
package claudecode
|
||||
|
||||
import (
|
||||
"context"
|
||||
"testing"
|
||||
|
||||
"github.com/aoagents/agent-orchestrator/backend/internal/ports"
|
||||
)
|
||||
|
||||
// captureAgent is a stub ports.Agent that records the LaunchConfig the reviewer
|
||||
// builds, so the test asserts the reviewer's tool policy without needing the
|
||||
// real claude binary on PATH.
|
||||
type captureAgent struct {
|
||||
got ports.LaunchConfig
|
||||
}
|
||||
|
||||
func (a *captureAgent) GetConfigSpec(context.Context) (ports.ConfigSpec, error) {
|
||||
return ports.ConfigSpec{}, nil
|
||||
}
|
||||
func (a *captureAgent) GetLaunchCommand(_ context.Context, cfg ports.LaunchConfig) ([]string, error) {
|
||||
a.got = cfg
|
||||
return []string{"claude"}, nil
|
||||
}
|
||||
func (a *captureAgent) GetPromptDeliveryStrategy(context.Context, ports.LaunchConfig) (ports.PromptDeliveryStrategy, error) {
|
||||
return ports.PromptDeliveryInCommand, nil
|
||||
}
|
||||
func (a *captureAgent) GetAgentHooks(context.Context, ports.WorkspaceHookConfig) error { return nil }
|
||||
func (a *captureAgent) GetRestoreCommand(context.Context, ports.RestoreConfig) ([]string, bool, error) {
|
||||
return nil, false, nil
|
||||
}
|
||||
func (a *captureAgent) SessionInfo(context.Context, ports.SessionRef) (ports.SessionInfo, bool, error) {
|
||||
return ports.SessionInfo{}, false, nil
|
||||
}
|
||||
|
||||
func TestReviewCommandLaunchesReadOnlyOffBypass(t *testing.T) {
|
||||
agent := &captureAgent{}
|
||||
r := &Reviewer{agent: agent}
|
||||
|
||||
if _, err := r.ReviewCommand(context.Background(), ports.ReviewInvocation{
|
||||
ReviewerID: "review-w1",
|
||||
WorkspacePath: "/ws/w1",
|
||||
Prompt: "review it",
|
||||
SystemPrompt: "you are a reviewer",
|
||||
}); err != nil {
|
||||
t.Fatalf("ReviewCommand: %v", err)
|
||||
}
|
||||
|
||||
// The allowlist is what enforces read-only, so it must launch in an
|
||||
// explicit non-bypass mode: bypassPermissions ignores allow/deny rules
|
||||
// entirely, and an empty mode would defer to a user's defaultMode.
|
||||
if agent.got.Permissions != ports.PermissionModeAuto {
|
||||
t.Fatalf("reviewer must launch in auto permission mode; got %q", agent.got.Permissions)
|
||||
}
|
||||
if !contains(agent.got.AllowedTools, "Read") || !contains(agent.got.AllowedTools, "Bash(ao review submit:*)") {
|
||||
t.Fatalf("allowlist missing read-only review tools: %#v", agent.got.AllowedTools)
|
||||
}
|
||||
for _, denied := range []string{"Edit", "Write", "Bash(git push:*)", "Bash(git commit:*)"} {
|
||||
if !contains(agent.got.DisallowedTools, denied) {
|
||||
t.Fatalf("disallow list missing %q: %#v", denied, agent.got.DisallowedTools)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func contains(values []string, needle string) bool {
|
||||
for _, v := range values {
|
||||
if v == needle {
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
|
@ -100,11 +100,19 @@ const (
|
|||
|
||||
// LaunchConfig carries inputs needed to build a new agent launch command.
|
||||
type LaunchConfig struct {
|
||||
Config AgentConfig
|
||||
IssueID string
|
||||
Permissions PermissionMode
|
||||
Prompt string
|
||||
SessionID string
|
||||
Config AgentConfig
|
||||
IssueID string
|
||||
Permissions PermissionMode
|
||||
Prompt string
|
||||
SessionID string
|
||||
// AllowedTools and DisallowedTools scope the agent to a tool allowlist when
|
||||
// it runs in a non-bypass permission mode (allow rules auto-approve, deny
|
||||
// rules auto-reject). They are the enforced read-only guarantee the reviewer
|
||||
// relies on: bypassPermissions ignores both lists, so a restricted launch
|
||||
// must leave Permissions off bypass. Empty means no restriction, so worker
|
||||
// sessions are unaffected.
|
||||
AllowedTools []string
|
||||
DisallowedTools []string
|
||||
SystemPrompt string
|
||||
SystemPromptFile string
|
||||
WorkspacePath string
|
||||
|
|
|
|||
Loading…
Reference in New Issue