feat(review): enforce reviewer read-only via tool allowlist (#2194)

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>

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,

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 {

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

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
+}

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