Let review profiles carry each reviewer's own agent config

cmd/entire/cli/agent/claudecode/review_config.go

@@ -0,0 +1,143 @@
+package claudecode
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"path/filepath"
+	"strings"
+
+	"github.com/entireio/cli/cmd/entire/cli/jsonutil"
+	"github.com/entireio/cli/cmd/entire/cli/review"
+	reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types"
+)
+
+// reviewPluginName namespaces the checkout's skills and commands, which are
+// loaded as a plugin when a profile config replaces the checkout's settings.
+const reviewPluginName = "project"
+
+// prepareReviewAgentConfig applies a review profile's agent config: the
+// checkout's settings and MCP servers are not loaded (--setting-sources user,
+// --strict-mcp-config); the profile's settings, with Entire's own hooks added
+// so the session is still tracked, and MCP servers are used instead. The
+// checkout's skills and commands are loaded as the "project" plugin, copied
+// from its committed tree.
+func prepareReviewAgentConfig(ctx context.Context, cfg reviewtypes.RunConfig) (reviewtypes.RunConfig, func(), error) {
+	if cfg.AgentConfig == nil {
+		return cfg, nil, nil
+	}
+	run, err := review.NewAgentConfigRun(ctx)
+	if err != nil {
+		return cfg, nil, err //nolint:wrapcheck // already names the step
+	}
+	fail := func(err error) (reviewtypes.RunConfig, func(), error) {
+		run.Cleanup()
+		return cfg, nil, err
+	}
+	if err := review.ValidateAgentConfig("claude-code", cfg.AgentConfig); err != nil {
+		return fail(fmt.Errorf("review profile config: %w", err))
+	}
+	settingsJSON, err := reviewProfileSettings(cfg.AgentConfig.Settings)
+	if err != nil {
+		return fail(err)
+	}
+	settingsPath, err := run.WriteFile("claude/settings.json", settingsJSON)
+	if err != nil {
+		return fail(err)
+	}
+	servers := cfg.AgentConfig.MCPServers
+	if servers == nil {
+		servers = map[string]json.RawMessage{}
+	}
+	mcpJSON, err := jsonutil.MarshalWithNoHTMLEscape(map[string]any{"mcpServers": servers})
+	if err != nil {
+		return fail(fmt.Errorf("encode MCP config: %w", err))
+	}
+	mcpPath, err := run.WriteFile("claude/mcp.json", mcpJSON)
+	if err != nil {
+		return fail(err)
+	}
+	cfg.ExtraArgs = append(cfg.ExtraArgs, flagSettingSources, "user", "--settings", settingsPath, "--strict-mcp-config", "--mcp-config", mcpPath)
+
+	copied, err := run.CopyCheckoutTree(ctx, map[string]string{
+		".claude/skills":   "claude/plugin/skills",
+		".claude/commands": "claude/plugin/commands",
+	})
+	if err != nil {
+		return fail(fmt.Errorf("load the checkout's skills: %w", err))
+	}
+	if copied > 0 {
+		manifest := fmt.Sprintf(`{"name":%q,"version":"0.0.0","description":"Skills and commands from the reviewed checkout"}`, reviewPluginName)
+		manifestPath, err := run.WriteFile("claude/plugin/.claude-plugin/plugin.json", []byte(manifest))
+		if err != nil {
+			return fail(err)
+		}
+		cfg.ExtraArgs = append(cfg.ExtraArgs, "--plugin-dir", filepath.Dir(filepath.Dir(manifestPath)))
+		cfg.Skills = namespaceProjectSkills(ctx, run, cfg.Skills)
+	}
+	return cfg, run.Cleanup, nil
+}
+
+// reviewProfileSettings returns the profile's settings object with Entire's
+// own hooks added, so the review session is tracked even though the
+// checkout's settings (where Entire's hooks normally live) are not loaded.
+func reviewProfileSettings(profile json.RawMessage) ([]byte, error) {
+	obj := map[string]json.RawMessage{}
+	if len(profile) > 0 {
+		if err := json.Unmarshal(profile, &obj); err != nil {
+			return nil, fmt.Errorf("review profile settings: %w", err)
+		}
+	}
+	if obj == nil { // "null"
+		obj = map[string]json.RawMessage{}
+	}
+	rawHooks := map[string]json.RawMessage{}
+	if existing, ok := obj["hooks"]; ok {
+		if err := json.Unmarshal(existing, &rawHooks); err != nil {
+			return nil, fmt.Errorf("review profile settings hooks: %w", err)
+		}
+		if rawHooks == nil {
+			rawHooks = map[string]json.RawMessage{}
+		}
+	}
+	// Entire's hooks are merged by event; an event that isn't a list of
+	// matchers would be dropped without a word.
+	for event, raw := range rawHooks {
+		var matchers []ClaudeHookMatcher
+		if err := json.Unmarshal(raw, &matchers); err != nil || matchers == nil {
+			return nil, fmt.Errorf("review profile settings hooks %s: must be a list of matcher objects", event)
+		}
+	}
+	installHookEntries(rawHooks, false)
+	hooks, err := jsonutil.MarshalWithNoHTMLEscape(rawHooks)
+	if err != nil {
+		return nil, fmt.Errorf("encode hooks: %w", err)
+	}
+	obj["hooks"] = hooks
+	out, err := jsonutil.MarshalWithNoHTMLEscape(obj)
+	if err != nil {
+		return nil, fmt.Errorf("encode settings: %w", err)
+	}
+	return out, nil
+}
+
+// namespaceProjectSkills rewrites "/name" to "/project:name" for skills and
+// commands that came from the checkout, since plugin skills are invoked with
+// the plugin's prefix.
+func namespaceProjectSkills(ctx context.Context, run *review.AgentConfigRun, skills []string) []string {
+	names := map[string]bool{}
+	for _, dir := range []string{".claude/skills", ".claude/commands"} {
+		for _, name := range run.TreeEntryNames(ctx, dir) {
+			names[strings.TrimSuffix(name, ".md")] = true
+		}
+	}
+	out := make([]string, 0, len(skills))
+	for _, skill := range skills {
+		if rest, ok := strings.CutPrefix(skill, "/"); ok && names[rest] {
+			out = append(out, "/"+reviewPluginName+":"+rest)
+			continue
+		}
+		out = append(out, skill)
+	}
+	return out
+}

cmd/entire/cli/agent/claudecode/review_config_test.go

@@ -0,0 +1,145 @@
+package claudecode
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"slices"
+	"strings"
+	"testing"
+
+	"github.com/entireio/cli/cmd/entire/cli/paths"
+	reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types"
+	"github.com/entireio/cli/cmd/entire/cli/testutil"
+)
+
+func newReviewConfigRepo(t *testing.T, files map[string]string) string {
+	t.Helper()
+	dir := t.TempDir()
+	testutil.InitRepo(t, dir)
+	names := []string{}
+	for name, content := range files {
+		testutil.WriteFile(t, dir, name, content)
+		names = append(names, name)
+	}
+	testutil.WriteFile(t, dir, "README.md", "x")
+	testutil.GitAdd(t, dir, append(names, "README.md")...)
+	testutil.GitCommit(t, dir, "init")
+	t.Chdir(dir)
+	paths.ClearWorktreeRootCache()
+	t.Cleanup(paths.ClearWorktreeRootCache)
+	return dir
+}
+
+func argAfter(args []string, flag string) string {
+	if i := slices.Index(args, flag); i >= 0 && i+1 < len(args) {
+		return args[i+1]
+	}
+	return ""
+}
+
+// A profile config replaces the checkout's settings and MCP servers, keeps
+// Entire's tracking hooks, and loads the checkout's skills as a plugin.
+func TestPrepareReviewAgentConfig(t *testing.T) {
+	newReviewConfigRepo(t, map[string]string{
+		".claude/skills/review/SKILL.md": "---\nname: review\ndescription: d\n---\nReview.",
+		".claude/commands/check.md":      "Check.",
+	})
+	cfg := reviewtypes.RunConfig{
+		Skills: []string{"/review", "/other"},
+		AgentConfig: &reviewtypes.AgentConfig{
+			Settings:   json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"/usr/bin/true"}]}]}}`),
+			MCPServers: map[string]json.RawMessage{"docs": json.RawMessage(`{"url":"https://mcp.example"}`)},
+		},
+	}
+	got, cleanup, err := prepareReviewAgentConfig(t.Context(), cfg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, flag := range []string{"--strict-mcp-config", flagSettingSources} {
+		if !slices.Contains(got.ExtraArgs, flag) {
+			t.Errorf("ExtraArgs %v missing %s", got.ExtraArgs, flag)
+		}
+	}
+	if argAfter(got.ExtraArgs, flagSettingSources) != "user" {
+		t.Errorf("--setting-sources = %q, want user", argAfter(got.ExtraArgs, flagSettingSources))
+	}
+	settingsData, err := os.ReadFile(argAfter(got.ExtraArgs, "--settings"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, want := range []string{"/usr/bin/true", "entire hooks claude-code stop"} {
+		if !strings.Contains(string(settingsData), want) {
+			t.Errorf("settings missing %q: %s", want, settingsData)
+		}
+	}
+	mcpData, err := os.ReadFile(argAfter(got.ExtraArgs, "--mcp-config"))
+	if err != nil || !strings.Contains(string(mcpData), "mcp.example") {
+		t.Errorf("mcp config = %s (%v)", mcpData, err)
+	}
+	pluginDir := argAfter(got.ExtraArgs, "--plugin-dir")
+	for _, rel := range []string{".claude-plugin/plugin.json", "skills/review/SKILL.md", "commands/check.md"} {
+		if _, err := os.Stat(filepath.Join(pluginDir, rel)); err != nil {
+			t.Errorf("plugin missing %s: %v", rel, err)
+		}
+	}
+	if !slices.Equal(got.Skills, []string{"/project:review", "/other"}) {
+		t.Errorf("Skills = %v, want the checkout's skill namespaced", got.Skills)
+	}
+	cleanup()
+	if _, err := os.Stat(pluginDir); !os.IsNotExist(err) {
+		t.Errorf("cleanup left %s behind", pluginDir)
+	}
+}
+
+// Without a profile config nothing changes.
+func TestPrepareReviewAgentConfigNoConfig(t *testing.T) {
+	t.Parallel()
+	cfg := reviewtypes.RunConfig{Skills: []string{"/review"}}
+	got, cleanup, err := prepareReviewAgentConfig(t.Context(), cfg)
+	if err != nil || cleanup != nil || len(got.ExtraArgs) != 0 {
+		t.Fatalf("got %+v, cleanup %v, err %v", got, cleanup != nil, err)
+	}
+}
+
+// A symlinked skill is refused rather than followed.
+func TestPrepareReviewAgentConfigRefusesSymlinkedSkill(t *testing.T) {
+	dir := newReviewConfigRepo(t, map[string]string{"elsewhere.md": "x"})
+	if err := os.MkdirAll(filepath.Join(dir, ".claude", "skills"), 0o750); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink("../../elsewhere.md", filepath.Join(dir, ".claude", "skills", "link.md")); err != nil {
+		t.Fatal(err)
+	}
+	testutil.GitAdd(t, dir, ".claude/skills/link.md")
+	testutil.GitCommit(t, dir, "symlink")
+
+	_, _, err := prepareReviewAgentConfig(t.Context(), reviewtypes.RunConfig{AgentConfig: &reviewtypes.AgentConfig{}})
+	if err == nil || !strings.Contains(err.Error(), "symlink") {
+		t.Fatalf("err = %v, want a symlink refusal", err)
+	}
+}
+
+// A null settings or hooks value still gets Entire's hooks rather than
+// crashing the review.
+func TestReviewProfileSettingsNull(t *testing.T) {
+	t.Parallel()
+	for _, profile := range []string{`null`, `{"hooks":null}`} {
+		out, err := reviewProfileSettings(json.RawMessage(profile))
+		if err != nil {
+			t.Fatalf("%s: %v", profile, err)
+		}
+		if !strings.Contains(string(out), "entire hooks claude-code") {
+			t.Errorf("%s: Entire's hooks missing: %s", profile, out)
+		}
+	}
+}
+
+func TestReviewProfileSettingsRefusesMalformedHookEvent(t *testing.T) {
+	t.Parallel()
+	for _, profile := range []string{`{"hooks":{"Stop":{}}}`, `{"hooks":{"Stop":null}}`, `{"hooks":{"Stop":"x"}}`} {
+		if _, err := reviewProfileSettings(json.RawMessage(profile)); err == nil {
+			t.Errorf("%s: accepted", profile)
+		}
+	}
+}

cmd/entire/cli/agent/claudecode/reviewer.go

@@ -8,6 +8,7 @@ import (
 	"io"
 	"os"
 	"os/exec"
+	"slices"
 
 	"github.com/entireio/cli/cmd/entire/cli/review"
 	reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types"
@@ -31,6 +32,7 @@ func NewReviewer() *reviewtypes.ReviewerTemplate {
 		AgentName: "claude-code",
 		BuildCmd:  buildReviewCmd,
 		Parser:    parseClaudeOutput,
+		Prepare:   prepareReviewAgentConfig,
 	}
 }
 
@@ -39,6 +41,7 @@ func NewReviewer() *reviewtypes.ReviewerTemplate {
 func buildReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd {
 	prompt := review.ComposeReviewPrompt(cfg)
 	args := []string{"-p", prompt, flagOutputFormat, "stream-json", "--verbose", "--append-system-prompt", review.ReviewerGuardrail}
+	args = append(args, cfg.ExtraArgs...)
 	args = review.AppendModelFlag(args, cfg.Model)
 	cmd := exec.CommandContext(ctx, "claude", args...)
 	cmd.Env = review.AppendReviewEnv(os.Environ(), "claude-code", cfg, prompt)
@@ -153,6 +156,9 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event {
 				sawResult = true
 				resultErr = env.IsError
 				resultUsage = env.Usage
+				if names := deniedToolNames(env.PermissionDenials); len(names) > 0 {
+					out <- reviewtypes.ToolsBlocked{Names: names, Allow: claudeAllowHint}
+				}
 			}
 		}
 		if err := scanner.Err(); err != nil {
@@ -189,6 +195,25 @@ type claudeEnvelope struct {
 	// Claude API usage shape (transcript parsing + stream-json review parser)
 	// can't drift apart.
 	Usage messageUsage `json:"usage"`
+	// PermissionDenials lists tool calls the permission settings refused.
+	PermissionDenials []permissionDenial `json:"permission_denials"`
+}
+
+type permissionDenial struct {
+	ToolName string `json:"tool_name"`
+}
+
+const claudeAllowHint = "add them to permissions.allow in the reviewer's settings (its review profile config, or ~/.claude/settings.json)"
+
+// deniedToolNames returns the distinct denied tool names, in order.
+func deniedToolNames(denials []permissionDenial) []string {
+	var names []string
+	for _, d := range denials {
+		if d.ToolName != "" && !slices.Contains(names, d.ToolName) {
+			names = append(names, d.ToolName)
+		}
+	}
+	return names
 }
 
 type claudeMessage struct {

cmd/entire/cli/agent/claudecode/reviewer_test.go

@@ -526,3 +526,19 @@ func TestParseClaudeOutput_IgnoresNonObjectMessageOnOtherEvents(t *testing.T) {
 		t.Fatalf("Finished = %+v, want success", finished)
 	}
 }
+
+// Tool calls the permission settings denied are reported once per tool.
+func TestParseClaudeOutput_ReportsPermissionDenials(t *testing.T) {
+	t.Parallel()
+	stream := `{"type":"result","subtype":"success","is_error":false,"permission_denials":[` +
+		`{"tool_name":"mcp__docs__search","tool_use_id":"a"},{"tool_name":"mcp__docs__search","tool_use_id":"b"},{"tool_name":"WebFetch","tool_use_id":"c"}]}` + "\n"
+	var blocked []string
+	for _, ev := range collectEvents(parseClaudeOutput(strings.NewReader(stream))) {
+		if e, ok := ev.(reviewtypes.ToolsBlocked); ok {
+			blocked = append(blocked, e.Names...)
+		}
+	}
+	if strings.Join(blocked, ",") != "mcp__docs__search,WebFetch" {
+		t.Fatalf("blocked = %v", blocked)
+	}
+}