diff --git a/backend/internal/adapters/agent/claudecode/claudecode.go b/backend/internal/adapters/agent/claudecode/claudecode.go index 57b248c6a6..6e2b633627 100644 --- a/backend/internal/adapters/agent/claudecode/claudecode.go +++ b/backend/internal/adapters/agent/claudecode/claudecode.go @@ -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, diff --git a/backend/internal/adapters/agent/claudecode/claudecode_test.go b/backend/internal/adapters/agent/claudecode/claudecode_test.go index bec96a677b..97a1d175d3 100644 --- a/backend/internal/adapters/agent/claudecode/claudecode_test.go +++ b/backend/internal/adapters/agent/claudecode/claudecode_test.go @@ -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 { diff --git a/backend/internal/adapters/reviewer/claudecode/claudecode.go b/backend/internal/adapters/reviewer/claudecode/claudecode.go index 8f595e58a2..e9c567905a 100644 --- a/backend/internal/adapters/reviewer/claudecode/claudecode.go +++ b/backend/internal/adapters/reviewer/claudecode/claudecode.go @@ -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 diff --git a/backend/internal/adapters/reviewer/claudecode/claudecode_test.go b/backend/internal/adapters/reviewer/claudecode/claudecode_test.go new file mode 100644 index 0000000000..53b1bb5f47 --- /dev/null +++ b/backend/internal/adapters/reviewer/claudecode/claudecode_test.go @@ -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 +} diff --git a/backend/internal/ports/agent.go b/backend/internal/ports/agent.go index f2c11f052d..9b5d6a9e28 100644 --- a/backend/internal/ports/agent.go +++ b/backend/internal/ports/agent.go @@ -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