diff --git a/cmd/entire/cli/agent/claudecode/review_config.go b/cmd/entire/cli/agent/claudecode/review_config.go new file mode 100644 index 0000000000..bb189b4d3d --- /dev/null +++ b/cmd/entire/cli/agent/claudecode/review_config.go @@ -0,0 +1,129 @@ +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, run.ForbiddenRoots); 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) + } + } + 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) + } + } + 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 +} diff --git a/cmd/entire/cli/agent/claudecode/review_config_test.go b/cmd/entire/cli/agent/claudecode/review_config_test.go new file mode 100644 index 0000000000..9ccad860dd --- /dev/null +++ b/cmd/entire/cli/agent/claudecode/review_config_test.go @@ -0,0 +1,132 @@ +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 hook that runs code from the checkout is refused at run time. +func TestPrepareReviewAgentConfigRefusesCheckoutCommand(t *testing.T) { + newReviewConfigRepo(t, nil) + cfg := reviewtypes.RunConfig{AgentConfig: &reviewtypes.AgentConfig{ + Settings: json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"./hook.sh"}]}]}}`), + }} + if _, _, err := prepareReviewAgentConfig(t.Context(), cfg); err == nil || !strings.Contains(err.Error(), "relative path") { + t.Fatalf("err = %v, want a relative-path refusal", err) + } +} diff --git a/cmd/entire/cli/agent/claudecode/reviewer.go b/cmd/entire/cli/agent/claudecode/reviewer.go index c36c6f4d83..65157c54bb 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer.go +++ b/cmd/entire/cli/agent/claudecode/reviewer.go @@ -31,6 +31,7 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { AgentName: "claude-code", BuildCmd: buildReviewCmd, Parser: parseClaudeOutput, + Prepare: prepareReviewAgentConfig, } } @@ -39,6 +40,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) diff --git a/cmd/entire/cli/agent/codex/review_config.go b/cmd/entire/cli/agent/codex/review_config.go new file mode 100644 index 0000000000..9662453a0c --- /dev/null +++ b/cmd/entire/cli/agent/codex/review_config.go @@ -0,0 +1,109 @@ +package codex + +import ( + "context" + "encoding/json" + "fmt" + "slices" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/review" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" +) + +// prepareCodexReviewConfig applies a review profile's agent config: the +// reviewed checkout, and the main repository a linked worktree shares trust +// with, are marked untrusted for this run, which drops their project config, +// hooks and rules; the profile's MCP servers are passed with -c. +func prepareCodexReviewConfig(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 + } + // Nothing is written for Codex; the run only resolves paths. + defer run.Cleanup() + if err := review.ValidateAgentConfig("codex", cfg.AgentConfig, run.ForbiddenRoots); err != nil { + return cfg, nil, fmt.Errorf("review profile config: %w", err) + } + mainRoot, err := run.MainRepoRoot(ctx) + if err != nil { + return cfg, nil, err //nolint:wrapcheck // already names the step + } + roots := []string{run.CheckoutRoot} + if mainRoot != run.CheckoutRoot { + roots = append(roots, mainRoot) + } + cfg.ExtraArgs = append(cfg.ExtraArgs, "-c", untrustedProjectOverride(roots)) + servers, err := codexMCPOverrides(cfg.AgentConfig.MCPServers) + if err != nil { + return cfg, nil, err + } + cfg.ExtraArgs = append(cfg.ExtraArgs, servers...) + cfg.WorkDir = run.CheckoutRoot + return cfg, nil, nil +} + +// untrustedProjectOverride marks roots untrusted for one codex run. It is one +// override: each -c projects=... replaces the whole table, so a second would +// drop the first. +func untrustedProjectOverride(roots []string) string { + entries := make([]string, 0, len(roots)) + for _, root := range roots { + quoted, err := json.Marshal(root) + if err != nil { + quoted = []byte(`""`) + } + entries = append(entries, string(quoted)+`={trust_level="untrusted"}`) + } + return "projects={" + strings.Join(entries, ",") + "}" +} + +// codexMCPOverrides turns profile MCP servers into -c overrides. Values are +// JSON-encoded, which is valid TOML. Literal env values are refused: they +// would be visible in the process list. +func codexMCPOverrides(servers map[string]json.RawMessage) ([]string, error) { + names := make([]string, 0, len(servers)) + for name := range servers { + names = append(names, name) + } + slices.Sort(names) + var args []string + for _, name := range names { + var server struct { + Command string `json:"command"` + Args []string `json:"args"` + URL string `json:"url"` + Env map[string]string `json:"env"` + } + if err := json.Unmarshal(servers[name], &server); err != nil { + return nil, fmt.Errorf("MCP server %q: %w", name, err) + } + if len(server.Env) > 0 { + return nil, fmt.Errorf("MCP server %q: env values aren't supported for Codex in a review profile yet", name) + } + set := func(key string, value any) error { + encoded, err := json.Marshal(value) + if err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + args = append(args, "-c", "mcp_servers."+name+"."+key+"="+string(encoded)) + return nil + } + var err error + if server.Command != "" { + err = set("command", server.Command) + if err == nil && len(server.Args) > 0 { + err = set("args", server.Args) + } + } else { + err = set("url", server.URL) + } + if err != nil { + return nil, err + } + } + return args, nil +} diff --git a/cmd/entire/cli/agent/codex/review_config_test.go b/cmd/entire/cli/agent/codex/review_config_test.go new file mode 100644 index 0000000000..43dc43a0ce --- /dev/null +++ b/cmd/entire/cli/agent/codex/review_config_test.go @@ -0,0 +1,71 @@ +package codex + +import ( + "encoding/json" + "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" +) + +// In a linked worktree Codex shares trust with the main checkout, so both +// are marked untrusted; profile MCP servers go through -c. +func TestPrepareCodexReviewConfigLinkedWorktree(t *testing.T) { + main := t.TempDir() + testutil.InitRepo(t, main) + testutil.WriteFile(t, main, "README.md", "x") + testutil.GitAdd(t, main, "README.md") + testutil.GitCommit(t, main, "init") + wt := filepath.Join(t.TempDir(), "wt") + testutil.RunGit(t, main, "worktree", "add", "-q", "-b", "review", wt) + t.Chdir(wt) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + + cfg := reviewtypes.RunConfig{AgentConfig: &reviewtypes.AgentConfig{ + MCPServers: map[string]json.RawMessage{"docs": json.RawMessage(`{"command":"/opt/mcp/docs","args":["--stdio"]}`)}, + }} + got, _, err := prepareCodexReviewConfig(t.Context(), cfg) + if err != nil { + t.Fatal(err) + } + args := strings.Join(got.ExtraArgs, "\n") + mainResolved, _ := filepath.EvalSymlinks(main) //nolint:errcheck // a temp dir always resolves + wtResolved, _ := filepath.EvalSymlinks(wt) //nolint:errcheck // a temp dir always resolves + if want := untrustedProjectOverride([]string{wtResolved, mainResolved}); strings.Count(args, "projects=") != 1 || !slices.Contains(got.ExtraArgs, want) { + t.Errorf("ExtraArgs want one override %s:\n%s", want, args) + } + for _, want := range []string{`mcp_servers.docs.command="/opt/mcp/docs"`, `mcp_servers.docs.args=["--stdio"]`} { + if !slices.Contains(got.ExtraArgs, want) { + t.Errorf("ExtraArgs missing %s:\n%s", want, args) + } + } + if got.WorkDir != wtResolved { + t.Errorf("WorkDir = %q, want %q", got.WorkDir, wtResolved) + } +} + +func TestPrepareCodexReviewConfigRefusesUnsupported(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + testutil.WriteFile(t, dir, "README.md", "x") + testutil.GitAdd(t, dir, "README.md") + testutil.GitCommit(t, dir, "init") + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + + for name, cfg := range map[string]reviewtypes.AgentConfig{ + "hooks via settings": {Settings: json.RawMessage(`{"hooks":{}}`)}, + "env values": {MCPServers: map[string]json.RawMessage{"docs": json.RawMessage(`{"command":"/opt/x","env":{"TOKEN":"s"}}`)}}, + } { + c := cfg + if _, _, err := prepareCodexReviewConfig(t.Context(), reviewtypes.RunConfig{AgentConfig: &c}); err == nil { + t.Errorf("%s: want an error", name) + } + } +} diff --git a/cmd/entire/cli/agent/codex/reviewer.go b/cmd/entire/cli/agent/codex/reviewer.go index 469aecb9ab..0b21e83811 100644 --- a/cmd/entire/cli/agent/codex/reviewer.go +++ b/cmd/entire/cli/agent/codex/reviewer.go @@ -9,6 +9,7 @@ import ( "log/slog" "os" "os/exec" + "path/filepath" "strings" "sync" "sync/atomic" @@ -30,6 +31,7 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { AgentName: "codex", BuildCmd: buildCodexReviewCmd, Parser: parseCodexOutput, + Prepare: prepareCodexReviewConfig, } } @@ -54,10 +56,18 @@ func buildCodexReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.C promptCfg := cfg promptCfg.Skills = codexNativeSkillInvocations(cfg.Skills) args := []string{codexExecCommand, "--skip-git-repo-check", "--json", "-c", review.CodexGuardrailConfig()} + args = append(args, cfg.ExtraArgs...) args = review.AppendModelFlag(args, cfg.Model) args = append(args, "-") prompt := review.ComposeReviewPrompt(promptCfg) cmd := exec.CommandContext(ctx, "codex", args...) + if cfg.AgentConfig != nil { + // Codex keys project trust on its working directory, so run from + // exactly the directory the untrusted override names. + if root, err := filepath.EvalSymlinks(cfg.WorkDir); err == nil && cfg.WorkDir != "" { + cmd.Dir = root + } + } cmd.Stdin = strings.NewReader(prompt) cmd.Env = review.AppendReviewEnv(os.Environ(), "codex", cfg, prompt) return cmd diff --git a/cmd/entire/cli/agent/pi/review_config.go b/cmd/entire/cli/agent/pi/review_config.go new file mode 100644 index 0000000000..9877d4bdd7 --- /dev/null +++ b/cmd/entire/cli/agent/pi/review_config.go @@ -0,0 +1,38 @@ +package pi + +import ( + "context" + "fmt" + + "github.com/entireio/cli/cmd/entire/cli/review" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" +) + +// preparePiReviewConfig applies a review profile's agent config: the +// checkout's extensions are not discovered (--no-extensions); Entire's own +// extension, written to the run dir so the session is still tracked, and the +// profile's extensions are loaded explicitly. The checkout's skills, prompt +// templates and .pi/settings.json still load; the trust gate lists them. +func preparePiReviewConfig(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 + } + if err := review.ValidateAgentConfig("pi", cfg.AgentConfig, run.ForbiddenRoots); err != nil { + run.Cleanup() + return cfg, nil, fmt.Errorf("review profile config: %w", err) + } + entireExt, err := run.WriteFile("pi/entire/index.ts", []byte(renderExtension())) + if err != nil { + run.Cleanup() + return cfg, nil, err //nolint:wrapcheck // already names the file + } + cfg.ExtraArgs = append(cfg.ExtraArgs, "--no-extensions", "--extension", entireExt) + for _, ext := range cfg.AgentConfig.Extensions { + cfg.ExtraArgs = append(cfg.ExtraArgs, "--extension", ext) + } + return cfg, run.Cleanup, nil +} diff --git a/cmd/entire/cli/agent/pi/review_config_test.go b/cmd/entire/cli/agent/pi/review_config_test.go new file mode 100644 index 0000000000..c7a3aa2558 --- /dev/null +++ b/cmd/entire/cli/agent/pi/review_config_test.go @@ -0,0 +1,40 @@ +package pi + +import ( + "os" + "slices" + "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" +) + +// The checkout's extensions are dropped; Entire's own and the profile's load. +func TestPreparePiReviewConfig(t *testing.T) { + dir := t.TempDir() + testutil.InitRepo(t, dir) + testutil.WriteFile(t, dir, "README.md", "x") + testutil.GitAdd(t, dir, "README.md") + testutil.GitCommit(t, dir, "init") + t.Chdir(dir) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + + cfg := reviewtypes.RunConfig{AgentConfig: &reviewtypes.AgentConfig{Extensions: []string{"/opt/pi/ext.ts"}}} + got, cleanup, err := preparePiReviewConfig(t.Context(), cfg) + if err != nil { + t.Fatal(err) + } + defer cleanup() + if got.ExtraArgs[0] != "--no-extensions" || got.ExtraArgs[1] != "--extension" { + t.Fatalf("ExtraArgs = %v", got.ExtraArgs) + } + entireExt, err := os.ReadFile(got.ExtraArgs[2]) + if err != nil || !IsEntireExtension(entireExt) { + t.Fatalf("Entire extension not written (%v)", err) + } + if !slices.Contains(got.ExtraArgs, "/opt/pi/ext.ts") { + t.Errorf("profile extension missing: %v", got.ExtraArgs) + } +} diff --git a/cmd/entire/cli/agent/pi/reviewer.go b/cmd/entire/cli/agent/pi/reviewer.go index a5efd3a771..de96665736 100644 --- a/cmd/entire/cli/agent/pi/reviewer.go +++ b/cmd/entire/cli/agent/pi/reviewer.go @@ -27,6 +27,7 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { AgentName: string(agent.AgentNamePi), BuildCmd: buildPiReviewCmd, Parser: parsePiReviewOutput, + Prepare: preparePiReviewConfig, } } @@ -36,6 +37,7 @@ func buildPiReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd if cfg.Model != "" { args = append(args, "--model", cfg.Model) } + args = append(args, cfg.ExtraArgs...) args = append(args, prompt) cmd := exec.CommandContext(ctx, "pi", args...) cmd.Env = review.AppendReviewEnv(os.Environ(), string(agent.AgentNamePi), cfg, prompt) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go new file mode 100644 index 0000000000..fe8c555bf5 --- /dev/null +++ b/cmd/entire/cli/review/agent_config.go @@ -0,0 +1,294 @@ +package review + +import ( + "encoding/json" + "errors" + "fmt" + "path/filepath" + "regexp" + "runtime" + "slices" + "strings" + + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// Agents and the review-profile config fields each one can apply. +var agentConfigFields = map[string][]string{ + "claude-code": {"settings", "mcp_servers"}, + "codex": {"mcp_servers"}, + "pi": {"extensions"}, +} + +// agentConfigCommandKeys are Claude Code settings whose value is a command. +var agentConfigCommandKeys = []string{"apiKeyHelper", "awsAuthRefresh", "awsCredentialExport", "otelHeadersHelper"} + +// projectLaunchers resolve the tool they run from the project before the +// user's install (local node_modules, project venvs), so the reviewed branch +// would choose the code. +var projectLaunchers = []string{"npx", "pnpx", "bunx", "uvx", "yarn", "pnpm", "bun", "deno", "uv", "poetry", "pipx"} + +// isProjectLauncher matches a launcher by its program name, so an absolute +// path to one (/usr/local/bin/npx) is caught too: where it is installed +// doesn't change that it resolves tools from the project. +func isProjectLauncher(word string) bool { + name := strings.ToLower(filepath.Base(strings.ReplaceAll(word, `\`, "/"))) + for _, ext := range []string{".exe", ".cmd", ".bat", ".ps1"} { + name = strings.TrimSuffix(name, ext) + } + return slices.Contains(projectLaunchers, name) +} + +var mcpServerNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) + +// toAgentConfig converts a profile's config for a run; nil stays nil. +func toAgentConfig(cfg *settings.ReviewAgentConfig) *reviewtypes.AgentConfig { + if cfg == nil { + return nil + } + return &reviewtypes.AgentConfig{Settings: cfg.Settings, MCPServers: cfg.MCPServers, Extensions: cfg.Extensions} +} + +// ValidateAgentConfig checks a reviewer's profile config before it is saved or +// used. Every command it names must be an absolute path or a bare tool name, +// and none may resolve inside forbiddenRoots (the reviewed checkout and the +// user's own), or the reviewed branch would supply the code that runs. +func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig, forbiddenRoots []string) error { + if cfg == nil { + return nil + } + allowed, known := agentConfigFields[agentName] + if !known { + return fmt.Errorf("%s does not support a review profile config", agentName) + } + check := func(field string, set bool) error { + if set && !slices.Contains(allowed, field) { + return fmt.Errorf("%s does not support %q in a review profile config", agentName, field) + } + return nil + } + if err := errors.Join( + check("settings", len(cfg.Settings) > 0), + check("mcp_servers", len(cfg.MCPServers) > 0), + check("extensions", len(cfg.Extensions) > 0), + ); err != nil { + return err + } + for _, name := range sortedStringKeys(cfg.MCPServers) { + if !mcpServerNamePattern.MatchString(name) { + return fmt.Errorf("MCP server name %q: use letters, digits, '-' and '_'", name) + } + if err := validateMCPServer(name, cfg.MCPServers[name], forbiddenRoots); err != nil { + return err + } + } + if len(cfg.Settings) > 0 { + if err := validateClaudeSettings(cfg.Settings, forbiddenRoots); err != nil { + return err + } + } + for _, ext := range cfg.Extensions { + if !filepath.IsAbs(ext) { + return fmt.Errorf("extension %q: use an absolute path", ext) + } + if under(ext, forbiddenRoots) { + return fmt.Errorf("extension %q is inside a checkout; keep reviewer extensions outside the repository", ext) + } + } + return nil +} + +func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string) error { + var server struct { + Command string `json:"command"` + Args []string `json:"args"` + URL string `json:"url"` + Env map[string]string `json:"env"` + } + if err := json.Unmarshal(raw, &server); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + if err := validateEnv(server.Env, forbiddenRoots); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + switch { + case server.Command != "": + // The command is one program path, not a shell line: check it whole. + if strings.Contains(server.Command, "CLAUDE_PROJECT_DIR") || isProjectLauncher(server.Command) { + return fmt.Errorf("MCP server %q: %q resolves from the reviewed project (a launcher such as npx does even by absolute path); point at the installed tool itself", name, server.Command) + } + if err := validateCommandWord(server.Command, forbiddenRoots); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + for _, arg := range server.Args { + if strings.Contains(arg, "CLAUDE_PROJECT_DIR") { + return fmt.Errorf("MCP server %q: argument %q refers to the project directory, which is the reviewed checkout", name, arg) + } + if isProjectLauncher(arg) { + return fmt.Errorf("MCP server %q: argument %q is a launcher that resolves tools from the reviewed project; point at the installed tool itself", name, arg) + } + if err := validateCommandWord(arg, forbiddenRoots); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + } + case server.URL == "": + return fmt.Errorf("MCP server %q needs a command or a url", name) + } + return nil +} + +func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error { + var settingsObj map[string]json.RawMessage + if err := json.Unmarshal(raw, &settingsObj); err != nil { + return fmt.Errorf("settings: %w", err) + } + var env map[string]string + if rawEnv, ok := settingsObj["env"]; ok { + if err := json.Unmarshal(rawEnv, &env); err != nil { + return fmt.Errorf("settings env: %w", err) + } + } + if err := validateEnv(env, forbiddenRoots); err != nil { + return fmt.Errorf("settings %w", err) + } + for _, key := range agentConfigCommandKeys { + var command string + if json.Unmarshal(settingsObj[key], &command) == nil && command != "" { + if err := validateCommand(command, forbiddenRoots); err != nil { + return fmt.Errorf("settings %s: %w", key, err) + } + } + } + var events map[string][]struct { + Hooks []struct { + Command string `json:"command"` + } `json:"hooks"` + } + if hooks, ok := settingsObj["hooks"]; ok { + if err := json.Unmarshal(hooks, &events); err != nil { + return fmt.Errorf("settings hooks: %w", err) + } + } + for event, groups := range events { + for _, group := range groups { + for _, hook := range group.Hooks { + if hook.Command == "" { + continue + } + if err := validateCommand(hook.Command, forbiddenRoots); err != nil { + return fmt.Errorf("settings hook %s: %w", event, err) + } + } + } + } + return nil +} + +// validateEnv keeps environment values from pointing a program back into the +// checkout: no absolute path inside it, and search-path variables (PATH, +// NODE_PATH, ...) list only absolute directories or inherited variables. +func validateEnv(env map[string]string, forbiddenRoots []string) error { + for _, key := range sortedStringKeys(env) { + value := env[key] + if strings.Contains(value, "CLAUDE_PROJECT_DIR") { + return fmt.Errorf("env %s refers to the project directory, which is the reviewed checkout", key) + } + searchPath := strings.HasSuffix(strings.ToUpper(key), "PATH") + for _, part := range filepath.SplitList(value) { + switch { + case filepath.IsAbs(part): + if under(part, forbiddenRoots) { + return fmt.Errorf("env %s: %q is inside a checkout", key, part) + } + case searchPath && part != "" && !strings.HasPrefix(part, "$"): + return fmt.Errorf("env %s: %q is a relative directory, which resolves inside the reviewed checkout; use an absolute path", key, part) + } + } + } + return nil +} + +// shellSyntax are characters that would make a command a shell program +// rather than a plain program and arguments. +const shellSyntax = ";&|<>()$`'\"\\\n" + +// validateCommand accepts only a plain command: an absolute program (or a bare +// tool name) and plain arguments, with no shell syntax. That keeps splitting +// on whitespace exact, so every word can be checked: paths must be absolute +// and outside the forbidden roots, and project launchers are refused. Hooks +// that need shell features belong in a script at an absolute path. +func validateCommand(command string, forbiddenRoots []string) error { + if strings.Contains(command, "CLAUDE_PROJECT_DIR") { + return fmt.Errorf("%q refers to the project directory, which is the reviewed checkout", command) + } + if strings.ContainsAny(command, shellSyntax) { + return fmt.Errorf("%q uses shell syntax; use an absolute program path and plain arguments, or put the logic in a script at an absolute path", command) + } + words := strings.Fields(command) + if len(words) == 0 { + return errors.New("empty command") + } + for _, word := range words { + if isProjectLauncher(word) { + return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project even by absolute path; point at the installed tool itself", command, word) + } + if err := validateCommandWord(word, forbiddenRoots); err != nil { + return fmt.Errorf("%q: %w", command, err) + } + } + return nil +} + +func validateCommandWord(word string, forbiddenRoots []string) error { + v := word + if _, value, ok := strings.Cut(word, "="); ok && strings.HasPrefix(word, "-") { + v = value // --flag=value: the value is what can be a path + } + switch { + case strings.HasPrefix(v, "$"): + return fmt.Errorf("%q depends on a variable; use an absolute path", v) + case strings.Contains(v, "://"): + // A URL, not a path. + case filepath.IsAbs(v): + if under(v, forbiddenRoots) { + return fmt.Errorf("%q is inside a checkout", v) + } + case strings.ContainsAny(v, `/\`): + return fmt.Errorf("%q is a relative path, which resolves inside the reviewed checkout; use an absolute path", v) + } + return nil +} + +// under reports whether path, or what it resolves to through symlinks, is +// inside one of roots. Windows paths compare case-insensitively. +func under(path string, roots []string) bool { + candidates := []string{filepath.Clean(path)} + if resolved, err := filepath.EvalSymlinks(path); err == nil { + candidates = append(candidates, resolved) + } + for _, candidate := range candidates { + for _, root := range roots { + if root != "" && pathWithin(candidate, filepath.Clean(root)) { + return true + } + } + } + return false +} + +func pathWithin(path, root string) bool { + if runtime.GOOS == "windows" { + path, root = strings.ToLower(path), strings.ToLower(root) + } + return path == root || strings.HasPrefix(path, strings.TrimSuffix(root, string(filepath.Separator))+string(filepath.Separator)) +} + +func sortedStringKeys[V any](m map[string]V) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} diff --git a/cmd/entire/cli/review/agent_config_run.go b/cmd/entire/cli/review/agent_config_run.go new file mode 100644 index 0000000000..f2e5309be4 --- /dev/null +++ b/cmd/entire/cli/review/agent_config_run.go @@ -0,0 +1,166 @@ +package review + +import ( + "context" + "crypto/rand" + "encoding/hex" + "fmt" + "os" + "path" + "path/filepath" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/gitexec" + "github.com/entireio/cli/cmd/entire/cli/gitrepo" + "github.com/entireio/cli/cmd/entire/cli/osroot" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/internal/entireclient/userdirs" +) + +// AgentConfigRun is a private per-run directory under the user's cache for +// the files a reviewer's profile config needs (settings, MCP config, a skills +// plugin). It is removed when the reviewer exits. +type AgentConfigRun struct { + root *os.Root + name string + dir string + // CheckoutRoot is the reviewed checkout, canonicalized. + CheckoutRoot string + // ForbiddenRoots are the checkouts no profile command may run from. + ForbiddenRoots []string +} + +// NewAgentConfigRun creates the run directory and resolves the checkout. +func NewAgentConfigRun(ctx context.Context) (*AgentConfigRun, error) { + checkout, err := paths.WorktreeRoot(ctx) + if err != nil { + return nil, fmt.Errorf("resolve the review checkout: %w", err) + } + canonical, err := filepath.EvalSymlinks(checkout) + if err != nil { + return nil, fmt.Errorf("canonicalize %s: %w", checkout, err) + } + forbidden := []string{checkout, canonical} + if caller := strings.TrimSpace(os.Getenv(envReviewFindingsWorktree)); caller != "" { + forbidden = append(forbidden, caller) + if resolved, err := filepath.EvalSymlinks(caller); err == nil { + forbidden = append(forbidden, resolved) + } + } + + root, err := userdirs.CacheRoot() + if err != nil { + return nil, fmt.Errorf("resolve cache dir: %w", err) + } + cacheDir, err := userdirs.CacheDirChecked() + if err != nil { + return nil, fmt.Errorf("resolve cache dir: %w", err) + } + var suffix [8]byte + if _, err := rand.Read(suffix[:]); err != nil { + return nil, fmt.Errorf("name review run dir: %w", err) + } + name := path.Join("review-runs", hex.EncodeToString(suffix[:])) + if err := osroot.MkdirAllNoSymlink(root, name, 0o700); err != nil { + return nil, fmt.Errorf("create review run dir: %w", err) + } + return &AgentConfigRun{ + root: root, + name: name, + dir: filepath.Join(cacheDir, filepath.FromSlash(name)), + CheckoutRoot: canonical, + ForbiddenRoots: forbidden, + }, nil +} + +// WriteFile writes rel (slash-separated) inside the run dir with mode 0600 and +// returns its absolute path. +func (r *AgentConfigRun) WriteFile(rel string, data []byte) (string, error) { + name := path.Join(r.name, rel) + if dir := path.Dir(name); dir != r.name { + if err := osroot.MkdirAllNoSymlink(r.root, dir, 0o700); err != nil { + return "", fmt.Errorf("create %s: %w", rel, err) + } + } + if err := osroot.WriteFile(r.root, name, data, 0o600); err != nil { + return "", fmt.Errorf("write %s: %w", rel, err) + } + return filepath.Join(r.dir, filepath.FromSlash(rel)), nil +} + +// Cleanup removes the run dir. +func (r *AgentConfigRun) Cleanup() { + _ = osroot.RemoveAllNoSymlinks(r.root, r.name) //nolint:errcheck // best effort; a leftover dir in the cache is harmless +} + +// CopyCheckoutTree copies the regular files under each dir of the checkout's +// HEAD commit into the run dir at dest/. It reads the +// committed tree, so the copy matches what the trust gate inspected, and it +// refuses symlinks and submodules rather than following them. It reports how +// many files it copied. +func (r *AgentConfigRun) CopyCheckoutTree(ctx context.Context, dirs map[string]string) (int, error) { + copied := 0 + for src, dest := range dirs { + out, err := gitexec.Run(ctx, r.CheckoutRoot, "ls-tree", "-r", "-z", "--full-tree", "--end-of-options", "HEAD", "--", src) + if err != nil { + return copied, fmt.Errorf("list %s: %w", src, err) + } + for _, record := range strings.Split(out, "\x00") { + if record == "" { + continue + } + meta, name, found := strings.Cut(record, "\t") + fields := strings.Fields(meta) + if !found || len(fields) != 3 { + return copied, fmt.Errorf("unexpected git ls-tree output %q", record) + } + if fields[0] != "100644" && fields[0] != "100755" { + return copied, fmt.Errorf("%s is a symlink or submodule; the review will not load it", name) + } + rel := strings.TrimPrefix(name, src+"/") + if rel == name || strings.Contains(rel, "..") { + return copied, fmt.Errorf("unexpected path %q under %s", name, src) + } + blob, err := gitexec.Run(ctx, r.CheckoutRoot, "cat-file", "blob", fields[2]) + if err != nil { + return copied, fmt.Errorf("read %s: %w", name, err) + } + if _, err := r.WriteFile(path.Join(dest, rel), []byte(blob)); err != nil { + return copied, err + } + copied++ + } + } + return copied, nil +} + +// MainRepoRoot returns the main checkout of the reviewed repository (the +// parent of the git common dir), canonicalized; for a linked worktree it +// differs from CheckoutRoot. +func (r *AgentConfigRun) MainRepoRoot(_ context.Context) (string, error) { + meta, err := gitrepo.ResolveWorktreeMetadata(r.CheckoutRoot) + if err != nil { + return "", fmt.Errorf("resolve git common dir: %w", err) + } + root := filepath.Dir(meta.CommonDir) + if resolved, err := filepath.EvalSymlinks(root); err == nil { + root = resolved + } + return root, nil +} + +// TreeEntryNames lists the names directly under dir in the checkout's HEAD +// tree, or nil when it has none. +func (r *AgentConfigRun) TreeEntryNames(ctx context.Context, dir string) []string { + out, err := gitexec.Run(ctx, r.CheckoutRoot, "ls-tree", "-z", "--name-only", "--full-tree", "--end-of-options", "HEAD:"+dir) + if err != nil { + return nil + } + var names []string + for _, name := range strings.Split(out, "\x00") { + if name != "" { + names = append(names, name) + } + } + return names +} diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go new file mode 100644 index 0000000000..ce0f6aaa00 --- /dev/null +++ b/cmd/entire/cli/review/agent_config_save.go @@ -0,0 +1,224 @@ +package review + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// Reviewer config choices offered by --edit. +const ( + agentConfigKeep = "keep" + agentConfigCheckout = "checkout" + agentConfigIsolated = "isolated" + agentConfigFile = "file" +) + +// loadAgentConfigFile reads a reviewer config from a JSON file in the +// ReviewAgentConfig shape ({"settings": ..., "mcp_servers": ..., "extensions": [...]}). +func loadAgentConfigFile(path string) (*settings.ReviewAgentConfig, error) { + if !filepath.IsAbs(path) { + abs, err := filepath.Abs(path) + if err != nil { + return nil, fmt.Errorf("resolve %s: %w", path, err) + } + path = abs + } + data, err := os.ReadFile(path) //nolint:gosec // an explicit path the user typed for their own config + if err != nil { + return nil, fmt.Errorf("read %s: %w", path, err) + } + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + var cfg settings.ReviewAgentConfig + if err := dec.Decode(&cfg); err != nil { + return nil, fmt.Errorf("parse %s: %w", path, err) + } + return &cfg, nil +} + +// validateAgentConfigForSave checks a config against the user's own checkout; +// at run time it is checked again against the reviewed checkout. +func validateAgentConfigForSave(ctx context.Context, agentName string, cfg *settings.ReviewAgentConfig) error { + var roots []string + if root, err := paths.WorktreeRoot(ctx); err == nil { + roots = append(roots, root) + if resolved, err := filepath.EvalSymlinks(root); err == nil { + roots = append(roots, resolved) + } + } + return ValidateAgentConfig(agentName, toAgentConfig(cfg), roots) +} + +// stripAgentConfigs removes reviewer configs from a profile before it is +// written to the committed settings file, where they would not be honored and +// could carry secrets. +func stripAgentConfigs(profile settings.ReviewProfileConfig) settings.ReviewProfileConfig { + if len(profile.Agents) == 0 { + return profile + } + agents := make(map[string]settings.ReviewConfig, len(profile.Agents)) + for worker, cfg := range profile.Agents { + cfg.Config = nil + agents[worker] = cfg + } + profile.Agents = agents + return profile +} + +// saveReviewAgentConfigs stores reviewer configs on profileName in a +// developer-owned layer: .entire/settings.local.json when it already defines +// the profile, otherwise clone-local preferences, which keep only the configs +// so the rest of the profile still comes from its own layer. A nil config +// removes it. It returns the file written. +func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map[string]*settings.ReviewAgentConfig) (string, error) { + s, err := settings.Load(reviewSettingsContext(ctx)) + if err != nil { + return "", fmt.Errorf("load settings: %w", err) + } + if s == nil { + s = &settings.EntireSettings{} + } + profile, ok := s.ReviewProfiles[profileName] + if !ok { + return "", fmt.Errorf("review profile %q not found", profileName) + } + agents := make(map[string]settings.ReviewConfig, len(profile.Agents)) + for worker, cfg := range profile.Agents { + agents[worker] = cfg + } + for worker, cfg := range configs { + existing, ok := agents[worker] + if !ok { + return "", fmt.Errorf("review profile %q has no reviewer %q", profileName, worker) + } + existing.Config = cfg + agents[worker] = existing + } + profile.Agents = agents + + _, localRaw, err := loadReviewSettingsRaw(ctx, reviewScopeLocal) + if err != nil { + return "", err + } + localProfiles, err := decodeRawReviewProfiles(localRaw) + if err != nil { + return "", err + } + if _, localOwns := localProfiles[profileName]; localOwns { + if err := saveReviewProfile(ctx, profileName, profile, false, reviewScopeLocal); err != nil { + return "", err + } + return reviewScopeLocal.file(), nil + } + err = settings.ModifyClonePreferences(ctx, func(p *settings.ClonePreferences) error { + workers := p.ReviewAgentConfigs[profileName] + if workers == nil { + workers = map[string]*settings.ReviewAgentConfig{} + } + for worker, cfg := range configs { + if cfg == nil { + delete(workers, worker) + } else { + workers[worker] = cfg + } + } + if p.ReviewAgentConfigs == nil { + p.ReviewAgentConfigs = map[string]map[string]*settings.ReviewAgentConfig{} + } + if len(workers) == 0 { + delete(p.ReviewAgentConfigs, profileName) + } else { + p.ReviewAgentConfigs[profileName] = workers + } + return nil + }) + if err != nil { + return "", fmt.Errorf("save clone-local review preferences: %w", err) + } + return "clone-local review preferences", nil +} + +// parseSetConfig parses --set-config worker=. +func parseSetConfig(value string) (worker, source string, err error) { + worker, source, ok := strings.Cut(value, "=") + worker, source = strings.TrimSpace(worker), strings.TrimSpace(source) + if !ok || worker == "" || source == "" { + return "", "", fmt.Errorf("--set-config %q: use reviewer=", value) + } + return worker, source, nil +} + +// agentConfigFromSource turns a --set-config source into a config: "none" +// removes it (removed is true), "isolated" isolates with nothing extra, +// anything else is a file. +func agentConfigFromSource(source string) (cfg *settings.ReviewAgentConfig, removed bool, err error) { + switch source { + case "none": + return nil, true, nil + case agentConfigIsolated: + return &settings.ReviewAgentConfig{}, false, nil + default: + cfg, err := loadAgentConfigFile(source) + return cfg, false, err + } +} + +func (o reviewConfigureOptions) withoutConfigs() reviewConfigureOptions { + o.Configs = nil + return o +} + +// configureAgentConfigs applies --set-config values to an existing profile. +func configureAgentConfigs(ctx context.Context, cmd *cobra.Command, profileName string, values []string, silentErr func(error) error) error { + fail := func(err error) error { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } + s, err := settings.Load(reviewSettingsContext(ctx)) + if err != nil { + return fail(fmt.Errorf("load settings: %w", err)) + } + if s == nil { + s = &settings.EntireSettings{} + } + profile, ok := s.ReviewProfiles[profileName] + if !ok { + return fail(fmt.Errorf("review profile %q not found; create it with --configure --set-agents first", profileName)) + } + configs := map[string]*settings.ReviewAgentConfig{} + for _, value := range values { + worker, source, err := parseSetConfig(value) + if err != nil { + return fail(err) + } + workerCfg, ok := profile.Agents[worker] + if !ok { + return fail(fmt.Errorf("review profile %q has no reviewer %q", profileName, worker)) + } + cfg, _, err := agentConfigFromSource(source) + if err != nil { + return fail(err) + } + if err := validateAgentConfigForSave(ctx, reviewAgentName(worker, workerCfg), cfg); err != nil { + return fail(fmt.Errorf("--set-config %s: %w", worker, err)) + } + configs[worker] = cfg + } + where, err := saveReviewAgentConfigs(ctx, profileName, configs) + if err != nil { + return fail(err) + } + fmt.Fprintf(cmd.OutOrStdout(), "Saved reviewer agent config for %s to %s.\n", strings.Join(sortedStringKeys(configs), ", "), where) + return nil +} diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go new file mode 100644 index 0000000000..a7f665eae6 --- /dev/null +++ b/cmd/entire/cli/review/agent_config_test.go @@ -0,0 +1,160 @@ +package review + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +func TestValidateAgentConfig(t *testing.T) { + t.Parallel() + + roots := []string{"/repo", "/repo/.entire/worktrees/review-x"} + hook := func(command string) json.RawMessage { + return json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"type":"command","command":` + quote(command) + `}]}]}}`) + } + mcp := func(def string) map[string]json.RawMessage { + return map[string]json.RawMessage{"docs": json.RawMessage(def)} + } + tests := []struct { + name string + agent string + cfg reviewtypes.AgentConfig + wantErr string + }{ + {"absolute hook outside the checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/local/bin/notify --quiet")}, ""}, + {"bare tool name", "claude-code", reviewtypes.AgentConfig{Settings: hook("jq .")}, ""}, + {"relative hook path", "claude-code", reviewtypes.AgentConfig{Settings: hook("./scripts/hook.sh")}, "relative path"}, + {"hook inside the checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/repo/scripts/hook.sh")}, "inside a checkout"}, + {"project dir variable", "claude-code", reviewtypes.AgentConfig{Settings: hook(`$CLAUDE_PROJECT_DIR/x.sh`)}, "project directory"}, + {"project launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook("npx some-tool")}, "resolves tools from the reviewed project"}, + {"argument into checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/node ./tools/x.js")}, "relative path"}, + {"apiKeyHelper relative", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"apiKeyHelper":"bin/key"}`)}, "relative path"}, + {"chained command", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true; node_modules/.bin/x")}, "shell syntax"}, + {"and-chained launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true && npx x")}, "shell syntax"}, + {"pipe", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/cat x | /repo/bin/run")}, "shell syntax"}, + {"quoted path", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/repo/my tools/run" --x`)}, "shell syntax"}, + {"command substitution", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/sh -c $(cat x)")}, "shell syntax"}, + {"backticks", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/echo `id`")}, "shell syntax"}, + {"variable", "claude-code", reviewtypes.AgentConfig{Settings: hook("$HOME/bin/x")}, "shell syntax"}, + {"redirect", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/tool > /tmp/x")}, "shell syntax"}, + {"launcher as an argument", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/env npx tool")}, "resolves tools from the reviewed project"}, + {"flag with relative value", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/tool --config=./x.json")}, "relative path"}, + {"url argument", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/curl https://example.com/x")}, ""}, + {"absolute launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/local/bin/npx tool")}, "resolves tools from the reviewed project"}, + {"absolute launcher as MCP command", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/homebrew/bin/pnpm","args":["dlx","server"]}`)}, "resolves from the reviewed project"}, + {"windows launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook(`C:/node/npx.cmd tool`)}, "resolves tools from the reviewed project"}, + {"flag with absolute value", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/tool --config=/opt/tool/config.json")}, ""}, + {"assignment with path", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/env PATH=/opt/bin:/repo/bin tool")}, "relative path"}, + {"assignment without path", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/env MODE=fast /opt/tool")}, ""}, + {"flag with value in checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/tool --config=/repo/x.json")}, "inside a checkout"}, + {"MCP absolute", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp/bin/docs","args":["--stdio"]}`)}, ""}, + {"MCP url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example"}`)}, ""}, + {"MCP inside checkout", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/repo/node_modules/.bin/mcp"}`)}, "inside a checkout"}, + {"MCP command with spaces", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/my tools/server"}`)}, ""}, + {"MCP relative command", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"bin/server"}`)}, "relative path"}, + {"MCP relative arg", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/usr/bin/node","args":["./server.js"]}`)}, "relative path"}, + {"MCP launcher arg", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/usr/bin/env","args":["npx","server"]}`)}, "resolves tools"}, + {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, + {"codex settings unsupported", "codex", reviewtypes.AgentConfig{Settings: json.RawMessage(`{}`)}, `does not support "settings"`}, + {"pi MCP unsupported", "pi", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"x"}`)}, `does not support "mcp_servers"`}, + {"pi extension relative", "pi", reviewtypes.AgentConfig{Extensions: []string{"ext.ts"}}, "absolute path"}, + {"pi extension in checkout", "pi", reviewtypes.AgentConfig{Extensions: []string{"/repo/.pi/x.ts"}}, "inside a checkout"}, + {"MCP env PATH into checkout", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"tool","env":{"PATH":"/repo/bin:/usr/bin"}}`)}, "inside a checkout"}, + {"MCP env relative PATH", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"tool","env":{"NODE_PATH":"node_modules"}}`)}, "relative directory"}, + {"MCP env token and inherited PATH", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"tool","env":{"TOKEN":"a/b+c","PATH":"$PATH:/opt/bin"}}`)}, ""}, + {"settings env into checkout", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"env":{"TOOL_HOME":"/repo/.tool"}}`)}, "inside a checkout"}, + {"unknown agent", "cursor", reviewtypes.AgentConfig{}, "does not support a review profile config"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := tt.cfg + err := ValidateAgentConfig(tt.agent, &cfg, roots) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("ValidateAgentConfig() = %v, want nil", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("ValidateAgentConfig() = %v, want error containing %q", err, tt.wantErr) + } + }) + } +} + +// A path outside the checkout that links into it is inside it. +func TestValidateAgentConfigFollowsSymlinks(t *testing.T) { + t.Parallel() + + checkout := t.TempDir() + testutil.WriteFile(t, checkout, ".pi/ext.ts", "x") + link := filepath.Join(t.TempDir(), "ext.ts") + if err := os.Symlink(filepath.Join(checkout, ".pi", "ext.ts"), link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + resolved, err := filepath.EvalSymlinks(checkout) + if err != nil { + t.Fatal(err) + } + err = ValidateAgentConfig("pi", &reviewtypes.AgentConfig{Extensions: []string{link}}, []string{resolved}) + if err == nil || !strings.Contains(err.Error(), "inside a checkout") { + t.Fatalf("ValidateAgentConfig() = %v, want inside a checkout", err) + } +} + +// An agent counts as isolated only when every worker on it has a profile +// config, so a mixed profile is never under-reported by the gate. +func TestProfileTrustAgents(t *testing.T) { + t.Parallel() + + profile := settings.ReviewProfileConfig{Agents: map[string]settings.ReviewConfig{ + "claude-a": {Agent: "claude-code", Config: &settings.ReviewAgentConfig{}}, + "claude-b": {Agent: "claude-code"}, + "pi": {Config: &settings.ReviewAgentConfig{}}, + }} + got := profileTrustAgents(profile, "") + want := map[string]bool{"claude-code": false, "pi": true} + if len(got) != len(want) { + t.Fatalf("profileTrustAgents() = %+v", got) + } + for _, a := range got { + if want[a.Name] != a.Isolated { + t.Errorf("%s isolated = %v, want %v", a.Name, a.Isolated, want[a.Name]) + } + } + if only := profileTrustAgents(profile, "claude-a"); len(only) != 1 || !only[0].Isolated { + t.Errorf("--agent claude-a = %+v, want one isolated agent", only) + } +} + +func TestTrustConfirmTextForProfileConfig(t *testing.T) { + t.Parallel() + + subject := foreignSubject() + inv := TrustInventory{Isolated: true, Entries: []TrustEntry{ + {Agent: "claude-code", Kind: TrustKindSkill, Name: "review", Command: ".claude/skills/review", Source: ".claude/skills"}, + }} + title, body := trustConfirmText(subject, inv, "x") + if title != "Load this branch's skills during the review?" || !strings.Contains(body, "uses your profile's config") { + t.Fatalf("title/body = %q / %q", title, body) + } + if title, body := trustConfirmText(subject, TrustInventory{Isolated: true}, "x"); title != "Review this branch?" || !strings.Contains(body, "uses your profile's config") { + t.Fatalf("empty isolated title/body = %q / %q", title, body) + } +} + +func quote(s string) string { + b, err := json.Marshal(s) + if err != nil { + panic(err) + } + return string(b) +} diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 4b24fda7f3..658b8d84d0 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -83,7 +83,7 @@ type Deps struct { CheckoutTarget func(ctx context.Context, out, errOut io.Writer, target ResolvedTarget, untrusted bool) (TargetWorktree, error) // InspectTrust lists what a checkout would run for the named agents. - InspectTrust func(ctx context.Context, source TrustSource, agents []string) (TrustInventory, error) + InspectTrust func(ctx context.Context, source TrustSource, agents []TrustAgent) (TrustInventory, error) // RemoveTarget removes a worktree created specifically for this review. // Reused worktrees are never passed to it. @@ -114,6 +114,10 @@ Flags: --set-model with --configure: per-reviewer model as agent=model (repeatable) --set-slot with --configure: a reviewer slot as agent[=model] (repeatable; the same agent/model may repeat to run it multiple times) + --set-config with --configure: a reviewer's own agent config as + reviewer= (repeatable). It replaces + the reviewed checkout's hooks, MCP servers and extensions; the + checkout's skills still load. Saved to clone-local preferences. --edit re-open the advanced profile skill picker --findings browse local findings; pass a handle to print one saved run --agent NAME run only one reviewer from the selected profile @@ -173,6 +177,7 @@ func NewCommand(deps Deps) *cobra.Command { var setTask string var setModels []string var setSlots []string + var setConfigs []string var target string var cleanupWorktree bool var trustTarget string @@ -262,13 +267,14 @@ func NewCommand(deps Deps) *cobra.Command { } if configure { return runReviewConfigure(ctx, cmd, profileName, reviewConfigureOptions{ - Agents: setAgents, - Judge: setJudge, - Output: setOutput, - Local: setLocal, - Task: setTask, - Models: setModels, - Slots: setSlots, + Agents: setAgents, + Judge: setJudge, + Output: setOutput, + Local: setLocal, + Task: setTask, + Models: setModels, + Slots: setSlots, + Configs: setConfigs, }, deps) } if edit { @@ -303,6 +309,7 @@ func NewCommand(deps Deps) *cobra.Command { cmd.Flags().StringVar(&setTask, "set-task", "", "with --configure: the profile's canonical task text") cmd.Flags().StringArrayVar(&setModels, "set-model", nil, "with --configure: per-reviewer model as agent=model (repeatable)") cmd.Flags().StringArrayVar(&setSlots, "set-slot", nil, "with --configure: a reviewer slot as agent[=model] (repeatable; same agent/model may repeat)") + cmd.Flags().StringArrayVar(&setConfigs, "set-config", nil, "with --configure: a reviewer's own agent config as reviewer= (repeatable; saved to clone-local preferences)") cmd.Flags().BoolVar(&edit, "edit", false, "re-open the advanced review profile skill picker") cmd.Flags().BoolVar(&findings, "findings", false, "browse local review findings; pass a handle to print one saved run") cmd.Flags().BoolVar(&listAgents, "agents", false, "list the reviewer agents you can pass to --agent for the selected profile") @@ -333,6 +340,8 @@ type reviewConfigureOptions struct { Task string // profile task text (--set-task) Models []string // per-reviewer "agent=model" entries (--set-model) Slots []string // reviewer slots as "agent[=model]" entries (--set-slot) + // Configs are reviewer agent configs as "reviewer=" (--set-config). + Configs []string } // reviewCommandIsInteractive requires the exact stdin consumed by huh and @@ -374,7 +383,7 @@ func (o reviewConfigureOptions) scripted() bool { // Local selects the destination only; by itself it must not force the // non-interactive/scripted path. `entire review --configure --local` should // still run the guided picker and preselect the local settings file. - return len(o.Agents) > 0 || o.Judge != "" || o.Output != "" || o.Task != "" || len(o.Models) > 0 || len(o.Slots) > 0 + return len(o.Agents) > 0 || o.Judge != "" || o.Output != "" || o.Task != "" || len(o.Models) > 0 || len(o.Slots) > 0 || len(o.Configs) > 0 } func runReviewConfigure(ctx context.Context, cmd *cobra.Command, profileOverride string, opts reviewConfigureOptions, deps Deps) error { @@ -406,6 +415,9 @@ func runReviewConfigure(ctx context.Context, cmd *cobra.Command, profileOverride // Scripted path: build + save the profile from --set-* flags, no TUI. The // destination is the --local flag (default: project settings). + if len(opts.Configs) > 0 && !opts.withoutConfigs().scripted() { + return configureAgentConfigs(ctx, cmd, profileName, opts.Configs, silentErr) + } if opts.scripted() { profile, buildErr := buildConfiguredProfile(ctx, profileName, opts, s, deps) if buildErr != nil { @@ -421,6 +433,11 @@ func runReviewConfigure(ctx context.Context, cmd *cobra.Command, profileOverride return err } fmt.Fprintf(out, "Review profile %q saved to %s with %s.\n", profileName, scope.file(), strings.Join(sortedMapKeys(profile.Agents), ", ")) + if len(opts.Configs) > 0 { + if err := configureAgentConfigs(ctx, cmd, profileName, opts.Configs, silentErr); err != nil { + return err + } + } fmt.Fprintf(out, "Run `entire review %s` to start.\n", profileName) return nil } @@ -545,7 +562,11 @@ func runReviewListProfiles(ctx context.Context, cmd *cobra.Command, deps Deps) e if model == "" { model = "default" } - reviewers = append(reviewers, reviewAgentName(w, cfg)+" · "+model) + label := reviewAgentName(w, cfg) + " · " + model + if cfg.Config != nil { + label += " · own agent config" + } + reviewers = append(reviewers, label) } fmt.Fprintf(out, " reviewers: %s\n", strings.Join(reviewers, ", ")) @@ -956,6 +977,17 @@ func resolveReviewProfile(ctx context.Context, cmd *cobra.Command, profileOverri return reviewProfileSelection{}, silentErr(err) } notifyDroppedReviewPrompts(cmd.ErrOrStderr(), s, profileName) + // A reviewer config the user set locally but that can't be honored must + // stop the review: running it with the checkout's config instead would + // silently undo the isolation they asked for. + for _, rej := range s.AgentPromptRejections() { + if strings.HasPrefix(rej.Field, "review_profiles."+profileName+".") && settings.AgentConfigRejectionUnverified(rej) { + cmd.SilenceUsage = true + err := fmt.Errorf("%s can't be applied: .entire/settings.local.json could not be verified as untracked; move the profile to clone-local preferences (entire review --configure --set-config)", rej.Field) + fmt.Fprintln(cmd.ErrOrStderr(), "Not run: "+err.Error()) + return reviewProfileSelection{}, silentErr(err) + } + } profile.Task = profileTask(profileName, profile) profile.Agents = nonZeroAgentConfigs(profile.Agents) return reviewProfileSelection{name: profileName, profile: profile, installed: installed}, nil @@ -979,12 +1011,17 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver profileName, profile, installed := selection.name, selection.profile, selection.installed // Gate before the reviewers load the checkout's configuration. - if err := gatePlainReview(ctx, cmd, gateOpts, profileAgentNames(profile, agentOverride), deps); err != nil { + if err := gatePlainReview(ctx, cmd, gateOpts, profileTrustAgents(profile, agentOverride), deps); err != nil { if errors.Is(err, errTrustCancelled) { return nil } return err } + for _, worker := range sortedMapKeys(nonZeroAgentConfigs(profile.Agents)) { + if profile.Agents[worker].Config != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "%s: using your profile's agent config (this checkout's hooks, MCP servers and extensions are not loaded).\n", worker) + } + } outputMode := profileOutput(profile) @@ -1780,6 +1817,7 @@ func applyReviewConfig(runCfg *reviewtypes.RunConfig, cfg settings.ReviewConfig) runCfg.Model = strings.TrimSpace(cfg.Model) runCfg.Skills = cfg.Skills runCfg.AlwaysPrompt = cfg.Prompt + runCfg.AgentConfig = toAgentConfig(cfg.Config) } // findTUISink returns the first *TUISink in the slice (if any). Used by the diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go index 630bd1b99d..2abec12e3e 100644 --- a/cmd/entire/cli/review/configure_test.go +++ b/cmd/entire/cli/review/configure_test.go @@ -786,3 +786,33 @@ func TestReviewConfigureOptionsScripted_LocalOnlyDoesNotSkipInteractive(t *testi t.Fatal("--local with --set-* flags should still use scripted configure") } } + +// Re-saving a local profile from --edit keeps its reviewer configs; only the +// shared file drops them. +func TestSaveReviewProfileConfigKeepsLocalAgentConfig(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + ctx := context.Background() + + agents := map[string]settings.ReviewConfig{ + tAgentClaude: {Agent: tAgentClaude, Config: &settings.ReviewAgentConfig{Extensions: []string{"/opt/x"}}}, + } + for _, scope := range []reviewSettingsScope{reviewScopeLocal, reviewScopeProject} { + if err := saveReviewProfileConfig(ctx, "security", agents, "", scope); err != nil { + t.Fatalf("saveReviewProfileConfig %s: %v", scope.file(), err) + } + _, raw, err := loadReviewSettingsRaw(ctx, scope) + if err != nil { + t.Fatal(err) + } + profiles, err := decodeRawReviewProfiles(raw) + if err != nil { + t.Fatal(err) + } + kept := profiles["security"].Agents[tAgentClaude].Config != nil + if want := scope == reviewScopeLocal; kept != want { + t.Errorf("%s: config kept = %v, want %v", scope.file(), kept, want) + } + } +} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index e7751ad045..9cd61e4281 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -828,6 +828,7 @@ func RunReviewProfileConfigPicker(ctx context.Context, out io.Writer, getInstall fmt.Fprintln(out) selected := map[string]settings.ReviewConfig{} + configs := map[string]*settings.ReviewAgentConfig{} for i, c := range configurable { curated := skilldiscovery.CuratedBuiltinsFor(string(c.name)) @@ -892,6 +893,14 @@ func RunReviewProfileConfigPicker(ctx context.Context, out io.Writer, getInstall Skills: dedupeStrings(append(builtinPicks, discoveredPicks...)), Prompt: strings.TrimSpace(prompt), } + agentCfg, changed, err := promptReviewAgentConfig(ctx, string(c.name), existing[string(c.name)].Config) + if err != nil { + return err + } + if changed { + configs[string(c.name)] = agentCfg + } + cfg.Config = agentCfg if !cfg.IsZero() { selected[string(c.name)] = cfg } @@ -923,6 +932,13 @@ func RunReviewProfileConfigPicker(ctx context.Context, out io.Writer, getInstall return err } fmt.Fprintf(out, "Saved review profile %q to %s. Edit later with `entire review --edit --profile %s`.\n", profileName, scope.file(), profileName) + if len(configs) > 0 { + where, err := saveReviewAgentConfigs(ctx, profileName, configs) + if err != nil { + return err + } + fmt.Fprintf(out, "Saved reviewer agent config to %s (agent config is never written to the shared settings file).\n", where) + } return nil } @@ -965,6 +981,10 @@ func saveReviewProfileConfig(ctx context.Context, profileName string, agents map // clobbered with built-in defaults. profile := profiles[profileName] profile.Agents = agents + // The shared file never holds reviewer configs; the local file keeps them. + if scope == reviewScopeProject { + profile = stripAgentConfigs(profile) + } if strings.TrimSpace(judgeAgent) != "" { profile.Judge = &settings.ReviewConfig{Agent: strings.TrimSpace(judgeAgent)} } else { @@ -1345,3 +1365,53 @@ func dedupeStrings(xs []string) []string { } return out } + +// promptReviewAgentConfig asks which agent config a reviewer runs with. It +// reports whether the choice changed anything. +func promptReviewAgentConfig(ctx context.Context, agentName string, current *settings.ReviewAgentConfig) (*settings.ReviewAgentConfig, bool, error) { + if _, supported := agentConfigFields[agentName]; !supported { + return current, false, nil + } + choice := agentConfigCheckout + options := []huh.Option[string]{ + huh.NewOption("Use the reviewed checkout's config (hooks, MCP servers, settings)", agentConfigCheckout), + huh.NewOption("Use my own config, with nothing extra", agentConfigIsolated), + huh.NewOption("Use my own config, loaded from a file…", agentConfigFile), + } + if current != nil { + choice = agentConfigKeep + options = append([]huh.Option[string]{huh.NewOption("Keep my current config", agentConfigKeep)}, options...) + } + form := newAccessibleForm(huh.NewGroup(huh.NewSelect[string](). + Title("Agent config for " + agentName). + Description("Your own config replaces the checkout's hooks, MCP servers and extensions; the checkout's skills still load."). + Options(options...). + Value(&choice))) + if err := form.RunWithContext(ctx); err != nil { + return nil, false, fmt.Errorf("agent config for %s: %w", agentName, err) + } + switch choice { + case agentConfigKeep: + return current, false, nil + case agentConfigCheckout: + return nil, current != nil, nil + case agentConfigIsolated: + return &settings.ReviewAgentConfig{}, true, nil + } + var path string + input := newAccessibleForm(huh.NewGroup(huh.NewInput(). + Title("Config file for " + agentName). + Description(`JSON: {"settings": {...}, "mcp_servers": {...}, "extensions": [...]}`). + Value(&path))) + if err := input.RunWithContext(ctx); err != nil { + return nil, false, fmt.Errorf("agent config file for %s: %w", agentName, err) + } + cfg, err := loadAgentConfigFile(strings.TrimSpace(path)) + if err != nil { + return nil, false, err + } + if err := validateAgentConfigForSave(ctx, agentName, cfg); err != nil { + return nil, false, fmt.Errorf("agent config for %s: %w", agentName, err) + } + return cfg, true, nil +} diff --git a/cmd/entire/cli/review/profile.go b/cmd/entire/cli/review/profile.go index 2ab73e4d1f..d08c923cb5 100644 --- a/cmd/entire/cli/review/profile.go +++ b/cmd/entire/cli/review/profile.go @@ -503,6 +503,9 @@ func saveReviewProfile(ctx context.Context, profileName string, profile settings return err } hadProfiles := len(profiles) > 0 + if scope == reviewScopeProject { + profile = stripAgentConfigs(profile) + } profiles[profileName] = profile defaultName := decodeRawReviewDefault(raw) switch { diff --git a/cmd/entire/cli/review/target.go b/cmd/entire/cli/review/target.go index c091919cb2..7f2ed2aeae 100644 --- a/cmd/entire/cli/review/target.go +++ b/cmd/entire/cli/review/target.go @@ -74,7 +74,7 @@ func runTargetReview(ctx context.Context, cmd *cobra.Command, req targetReviewRe if len(req.Positional) == 1 { profileName = req.Positional[0] } - var agents []string + var agents []TrustAgent forwardProfile := "" if req.ShowConfig { agents = showConfigAgents(ctx, profileName, req.Gate.AgentOverride) @@ -85,7 +85,7 @@ func runTargetReview(ctx context.Context, cmd *cobra.Command, req targetReviewRe if selErr != nil || selection.done { return selErr } - agents = profileAgentNames(selection.profile, req.Gate.AgentOverride) + agents = profileTrustAgents(selection.profile, req.Gate.AgentOverride) if profileName == "" { forwardProfile = selection.name } diff --git a/cmd/entire/cli/review/trust.go b/cmd/entire/cli/review/trust.go index 6e22ef30a4..a1901c7710 100644 --- a/cmd/entire/cli/review/trust.go +++ b/cmd/entire/cli/review/trust.go @@ -56,9 +56,21 @@ type TrustEntry struct { Entire bool `json:"entire"` } +// TrustAgent is a reviewer agent whose checkout config the gate inspects. +// Isolated agents run with their review profile's config, so only the +// checkout content they still load (skills, commands, Pi settings) counts. +type TrustAgent struct { + Name string + Isolated bool +} + // TrustInventory lists what a checkout would run during a review. type TrustInventory struct { - Entries []TrustEntry + // Isolated is set when every inspected agent uses its profile's config. + Isolated bool + // IsolatedAgents lists the agents that use their profile's config. + IsolatedAgents []string + Entries []TrustEntry // Instructions are files like CLAUDE.md the reviewer reads; informational. Instructions []string } @@ -373,7 +385,7 @@ func (g trustGate) run(ctx context.Context, errOut io.Writer) error { // Agents can hold a PTY, so they never get the confirm. No terminal gets the // same text, so an undetected agent isn't invited to approve itself. if g.AgentCaller != "" || !g.Interactive || g.Confirm == nil { - printTrustRefusal(errOut, what, g.Command, g.Subject.HeadSHA) + printTrustRefusal(errOut, what, g.Inventory.Isolated, g.Command, g.Subject.HeadSHA) return errTrustRefused } title, description := trustConfirmText(g.Subject, g.Inventory, g.Command) @@ -388,11 +400,14 @@ func (g trustGate) run(ctx context.Context, errOut io.Writer) error { return nil } -func printTrustRefusal(errOut io.Writer, what, command, head string) { +func printTrustRefusal(errOut io.Writer, what string, isolated bool, command, head string) { fmt.Fprintln(errOut, "Not run: this review needs the user's approval.") - if what == trustWhatNothing { + switch { + case isolated && what != trustWhatNothing: + fmt.Fprintf(errOut, "The code is by someone else; the review agent uses the profile's config but would still load the branch's skills and commands, and Pi's settings (%s --show-config lists them).\n", command) + case what == trustWhatNothing: fmt.Fprintf(errOut, "The code is by someone else (%s --show-config shows what the review reads).\n", command) - } else { + default: fmt.Fprintf(errOut, "The code is by someone else, and the review agent would load its hooks, MCP servers and settings, running %s on this machine (%s --show-config lists them).\n", what, command) } fmt.Fprintln(errOut, "Stop and show the user this message. Do not approve on their behalf.") @@ -416,10 +431,17 @@ func trustConfirmText(subject TrustSubject, inv TrustInventory, command string) entries := inv.orderedEntries() var title string switch { + case len(entries) == 0 && inv.Isolated: + title = "Review this branch?" + b.WriteString("The review agent uses your profile's config and reads this branch's code and instructions; nothing from it runs on your machine.") + return title, b.String() case len(entries) == 0: title = "Review this branch?" b.WriteString("The review agent reads this branch's code and instructions; nothing from it runs on your machine.") return title, b.String() + case inv.Isolated: + title = "Load this branch's skills during the review?" + b.WriteString("The review agent uses your profile's config, but still loads this branch's skills and commands (and Pi settings), which can run commands:\n") case inv.onlyEntireHooks(): title = "Run this branch's hooks during the review?" b.WriteString("The review agent loads this branch's hooks. These run on your machine:\n") @@ -533,6 +555,9 @@ type trustConfigJSON struct { Yours bool `json:"yours"` Entries []TrustEntry `json:"entries"` Instructions []string `json:"instructions"` + // IsolatedAgents use their review profile's config instead of the + // checkout's hooks, MCP servers and extensions. + IsolatedAgents []string `json:"isolated_agents"` } // printTrustConfig lists everything a review would run, without truncation. @@ -540,12 +565,16 @@ func printTrustConfig(out io.Writer, subject TrustSubject, inv TrustInventory, a entries := inv.orderedEntries() if asJSON { payload := trustConfigJSON{ - Target: subject.Label, - Head: subject.HeadSHA, - Commits: subject.Commits, - Yours: subject.Yours, - Entries: entries, - Instructions: inv.Instructions, + Target: subject.Label, + Head: subject.HeadSHA, + Commits: subject.Commits, + Yours: subject.Yours, + Entries: entries, + Instructions: inv.Instructions, + IsolatedAgents: inv.IsolatedAgents, + } + if payload.IsolatedAgents == nil { + payload.IsolatedAgents = []string{} } if payload.Entries == nil { payload.Entries = []TrustEntry{} @@ -571,6 +600,9 @@ func printTrustConfig(out io.Writer, subject TrustSubject, inv TrustInventory, a } fmt.Fprintf(out, "%s @ %s by %s (%s)\n", sanitizeDisplay(label), shortSHA(subject.HeadSHA), by, pluralCount(subject.Commits, "commit", "commits")) + if len(inv.IsolatedAgents) > 0 { + fmt.Fprintf(out, "Using your profile's agent config (the checkout's hooks, MCP servers and extensions are not loaded): %s\n", strings.Join(inv.IsolatedAgents, ", ")) + } if agents := inv.entireAgents(); len(agents) > 0 { fmt.Fprintf(out, "Entire session tracking: %s\n", strings.Join(agents, ", ")) } diff --git a/cmd/entire/cli/review/trust_cmd_test.go b/cmd/entire/cli/review/trust_cmd_test.go index 69be52d9a7..90f5447d73 100644 --- a/cmd/entire/cli/review/trust_cmd_test.go +++ b/cmd/entire/cli/review/trust_cmd_test.go @@ -5,6 +5,7 @@ import ( "context" "os" "os/exec" + "path/filepath" "strings" "testing" @@ -57,8 +58,8 @@ func setupForeignBranchRepo(t *testing.T) (reviewer *captureRunConfigReviewer, d } return nil }, - InspectTrust: func(_ context.Context, source review.TrustSource, agents []string) (review.TrustInventory, error) { - if source.WorktreeRoot == "" || len(agents) != 1 || agents[0] != "claude-code" { + InspectTrust: func(_ context.Context, source review.TrustSource, agents []review.TrustAgent) (review.TrustInventory, error) { + if source.WorktreeRoot == "" || len(agents) != 1 || agents[0].Name != "claude-code" { t.Errorf("InspectTrust(%+v, %v): want the current worktree and the profile's agent", source, agents) } return review.TrustInventory{Entries: []review.TrustEntry{ @@ -143,3 +144,62 @@ func TestRunReview_GateFlagsValidatedForEveryMode(t *testing.T) { } } } + +// --set-config validates the file and saves to clone-local preferences, +// never the committed settings file. +func TestConfigure_SetConfigSavesToClonePreferences(t *testing.T) { + _, deps, _ := setupForeignBranchRepo(t) + cfgFile := filepath.Join(t.TempDir(), "review-config.json") + if err := os.WriteFile(cfgFile, []byte(`{"settings":{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"/usr/bin/true"}]}]}}}`), 0o600); err != nil { + t.Fatal(err) + } + run := func(args ...string) error { + cmd := review.NewCommand(deps) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs(args) + return cmd.Execute() + } + if err := run("--configure", "general", "--set-config", "claude-code="+cfgFile); err != nil { + t.Fatalf("--set-config: %v", err) + } + s, err := settings.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + cfg := s.ReviewProfiles["general"].Agents["claude-code"].Config + if cfg == nil || !strings.Contains(string(cfg.Settings), "/usr/bin/true") { + t.Fatalf("config not applied: %+v", cfg) + } + if data, err := os.ReadFile(filepath.Join(".entire", "settings.json")); err == nil && strings.Contains(string(data), `"config"`) { + t.Fatalf("config written to the committed settings file:\n%s", data) + } + // Only the config is stored clone-locally, so a later change to the + // profile in its own layer still takes effect. + if err := settings.ModifyClonePreferences(context.Background(), func(p *settings.ClonePreferences) error { + worker := p.ReviewProfiles["general"].Agents["claude-code"] + if worker.Config != nil { + t.Error("the config was copied into the stored profile") + } + worker.Model = "sonnet" + p.ReviewProfiles["general"].Agents["claude-code"] = worker + return nil + }); err != nil { + t.Fatal(err) + } + s, err = settings.Load(context.Background()) + if err != nil { + t.Fatal(err) + } + if got := s.ReviewProfiles["general"].Agents["claude-code"]; got.Model != "sonnet" || got.Config == nil { + t.Fatalf("after editing the profile: model %q, config %v; want sonnet with the config kept", got.Model, got.Config) + } + + bad := filepath.Join(t.TempDir(), "bad.json") + if err := os.WriteFile(bad, []byte(`{"settings":{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"./hook.sh"}]}]}}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := run("--configure", "general", "--set-config", "claude-code="+bad); err == nil { + t.Fatal("a hook running from the checkout was accepted") + } +} diff --git a/cmd/entire/cli/review/trust_run.go b/cmd/entire/cli/review/trust_run.go index 32dcc310e9..c413dd2b5d 100644 --- a/cmd/entire/cli/review/trust_run.go +++ b/cmd/entire/cli/review/trust_run.go @@ -62,42 +62,50 @@ func reviewSettingsContext(ctx context.Context) context.Context { // knownReviewAgents are the agents a review can launch. var knownReviewAgents = []string{"claude-code", "codex", "pi"} -// profileAgentNames lists the reviewer agents a profile launches (or just -// --agent). The judge runs from a temp directory, so it is not included. -func profileAgentNames(profile settings.ReviewProfileConfig, agentOverride string) []string { +// profileTrustAgents lists the reviewer agents a profile launches (or just +// --agent). The judge runs from a temp directory, so it is not included. An +// agent counts as isolated only if every worker on it has a profile config. +func profileTrustAgents(profile settings.ReviewProfileConfig, agentOverride string) []TrustAgent { + workers := nonZeroAgentConfigs(profile.Agents) if agentOverride != "" { if worker, cfg, err := selectProfileWorker(profile, agentOverride); err == nil { - return []string{reviewAgentName(worker, cfg)} + workers = map[string]settings.ReviewConfig{worker: cfg} } } - var names []string - for worker, cfg := range nonZeroAgentConfigs(profile.Agents) { + isolated := map[string]bool{} + for worker, cfg := range workers { name := reviewAgentName(worker, cfg) - if !slices.Contains(names, name) { - names = append(names, name) - } + prev, seen := isolated[name] + isolated[name] = cfg.Config != nil && (!seen || prev) + } + agents := make([]TrustAgent, 0, len(isolated)) + for _, name := range sortedStringKeys(isolated) { + agents = append(agents, TrustAgent{Name: name, Isolated: isolated[name]}) } - slices.Sort(names) - return names + return agents } // showConfigAgents uses the named profile's agents, or all launchable ones. -func showConfigAgents(ctx context.Context, profileName, agentOverride string) []string { +func showConfigAgents(ctx context.Context, profileName, agentOverride string) []TrustAgent { if s, err := settings.Load(reviewSettingsContext(ctx)); err == nil && s != nil { applyLegacyReviewProfileFallback(s) if strings.TrimSpace(profileName) != "" { if _, profile, selErr := selectReviewProfile(s, profileName); selErr == nil { - if names := profileAgentNames(profile, agentOverride); len(names) > 0 { - return names + if agents := profileTrustAgents(profile, agentOverride); len(agents) > 0 { + return agents } } } } - return slices.Clone(knownReviewAgents) + agents := make([]TrustAgent, 0, len(knownReviewAgents)) + for _, name := range knownReviewAgents { + agents = append(agents, TrustAgent{Name: name}) + } + return agents } // gatePlainReview applies the trust gate to a review of the current checkout. -func gatePlainReview(ctx context.Context, cmd *cobra.Command, opts reviewGateOptions, agents []string, deps Deps) error { +func gatePlainReview(ctx context.Context, cmd *cobra.Command, opts reviewGateOptions, agents []TrustAgent, deps Deps) error { worktreeRoot, err := paths.WorktreeRoot(ctx) if err != nil { return fmt.Errorf("resolve worktree root: %w", err) @@ -123,7 +131,7 @@ func gatePlainReview(ctx context.Context, cmd *cobra.Command, opts reviewGateOpt } // inspectReview gathers authorship and, when needed, what source would run. -func inspectReview(ctx context.Context, repoRoot, head string, source TrustSource, agents []string, alwaysInventory bool, deps Deps) (TrustSubject, TrustInventory, error) { +func inspectReview(ctx context.Context, repoRoot, head string, source TrustSource, agents []TrustAgent, alwaysInventory bool, deps Deps) (TrustSubject, TrustInventory, error) { subject, err := commitAuthorship(ctx, repoRoot, head) if err != nil { return TrustSubject{}, TrustInventory{}, err diff --git a/cmd/entire/cli/review/types/reviewer.go b/cmd/entire/cli/review/types/reviewer.go index 09edfd256c..2eb87441ca 100644 --- a/cmd/entire/cli/review/types/reviewer.go +++ b/cmd/entire/cli/review/types/reviewer.go @@ -20,6 +20,7 @@ package types import ( "context" + "encoding/json" "time" ) @@ -80,6 +81,17 @@ type Process interface { // invocations (e.g., "/pr-review-toolkit:review-pr") the configured agent // should run. type RunConfig struct { + // AgentConfig, when set, replaces the reviewed checkout's agent config + // (hooks, MCP servers, extensions) with the review profile's. + AgentConfig *AgentConfig + + // ExtraArgs are agent arguments a reviewer's Prepare produced (files it + // wrote for AgentConfig); BuildCmd places them. + ExtraArgs []string + + // WorkDir, when set by Prepare, is the directory the agent runs in. + WorkDir string + // PromptOverride, when non-empty, is the exact prompt sent to the agent. // It preserves settings.ReviewConfig.Prompt's existing verbatim-override // contract: configured skills are still recorded as structured metadata, @@ -236,3 +248,11 @@ type RunError struct { } func (RunError) isEvent() {} + +// AgentConfig is a reviewer's own agent config from a review profile, in each +// agent's native shape. It mirrors settings.ReviewAgentConfig. +type AgentConfig struct { + Settings json.RawMessage + MCPServers map[string]json.RawMessage + Extensions []string +} diff --git a/cmd/entire/cli/review/types/template.go b/cmd/entire/cli/review/types/template.go index 96c9a98d85..8cbf78f2f1 100644 --- a/cmd/entire/cli/review/types/template.go +++ b/cmd/entire/cli/review/types/template.go @@ -41,6 +41,12 @@ type ReviewerTemplate struct { // must emit Started first, Finished{Success: ...} or RunError last, // and check scanner.Err() before emitting Finished{Success: true}. Parser func(stdout io.Reader) <-chan Event + + // Prepare, when set, runs before BuildCmd. It may return an updated + // RunConfig (ExtraArgs naming files it wrote) and a cleanup that runs + // after the process exits. An error aborts the run before anything is + // spawned. + Prepare func(ctx context.Context, cfg RunConfig) (RunConfig, func(), error) } // Compile-time check. @@ -67,6 +73,23 @@ func (t *ReviewerTemplate) Start(ctx context.Context, cfg RunConfig) (Process, e if t.Parser == nil { return nil, fmt.Errorf("ReviewerTemplate.Start: %w (nil Parser for agent %q)", ErrTemplateMisconfigured, t.AgentName) } + cleanup := func() {} + if t.Prepare != nil { + prepared, done, err := t.Prepare(ctx, cfg) + if err != nil { + return nil, fmt.Errorf("%s: %w", t.AgentName, err) + } + cfg = prepared + if done != nil { + cleanup = done + } + } + started := false + defer func() { + if !started { + cleanup() + } + }() cmd := t.BuildCmd(ctx, cfg) if cmd == nil { return nil, fmt.Errorf("ReviewerTemplate.Start: %w (BuildCmd returned nil for agent %q)", ErrTemplateMisconfigured, t.AgentName) @@ -86,7 +109,9 @@ func (t *ReviewerTemplate) Start(ctx context.Context, cfg RunConfig) (Process, e if err := cmd.Start(); err != nil { return nil, fmt.Errorf("%s: start: %w", t.AgentName, err) } + started = true p := &templateProcess{ + cleanup: cleanup, ctx: ctx, agentName: t.AgentName, cmd: cmd, @@ -108,6 +133,7 @@ var ErrTemplateMisconfigured = errors.New("ReviewerTemplate misconfigured") // templateProcess is the shared Process implementation for ReviewerTemplate. type templateProcess struct { + cleanup func() ctx context.Context agentName string cmd *exec.Cmd @@ -130,6 +156,10 @@ func (p *templateProcess) Wait() error { <-p.stderrDone } err := p.cmd.Wait() + if p.cleanup != nil { + p.cleanup() + p.cleanup = nil + } if err != nil && p.ctx.Err() != nil { return p.ctx.Err() //nolint:wrapcheck // preserve Process cancellation contract } diff --git a/cmd/entire/cli/review_trust_inventory.go b/cmd/entire/cli/review_trust_inventory.go index 63eef3f9d5..be9ae00188 100644 --- a/cmd/entire/cli/review_trust_inventory.go +++ b/cmd/entire/cli/review_trust_inventory.go @@ -76,7 +76,7 @@ var errTrustTooLarge = errors.New("too large to inspect") var trustRoots = []string{".claude", ".codex", ".pi", ".agents", ".mcp.json", trustClaudeMD, "CLAUDE.local.md", trustAgentsMD, "AGENTS.override.md"} // inspectReviewTrust implements review.Deps.InspectTrust. -func inspectReviewTrust(ctx context.Context, source cliReview.TrustSource, agents []string) (cliReview.TrustInventory, error) { +func inspectReviewTrust(ctx context.Context, source cliReview.TrustSource, agents []cliReview.TrustAgent) (cliReview.TrustInventory, error) { var files trustFiles if source.Commit != "" { tree, err := loadGitTrustTree(ctx, source.RepoRoot, source.Commit) @@ -94,20 +94,40 @@ func inspectReviewTrust(ctx context.Context, source cliReview.TrustSource, agent return buildTrustInventory(files, agents) } -func buildTrustInventory(files trustFiles, agents []string) (cliReview.TrustInventory, error) { +func buildTrustInventory(files trustFiles, agents []cliReview.TrustAgent) (cliReview.TrustInventory, error) { var inv cliReview.TrustInventory - for _, name := range agents { + inv.Isolated = len(agents) > 0 + for _, a := range agents { var ( entries []cliReview.TrustEntry err error ) - switch name { - case string(agent.AgentNameClaudeCode): + inv.Isolated = inv.Isolated && a.Isolated + if a.Isolated { + inv.IsolatedAgents = append(inv.IsolatedAgents, a.Name) + } + switch { + case a.Name == string(agent.AgentNameClaudeCode) && a.Isolated: + // The profile's config replaces the checkout's settings and MCP + // servers; its skills and commands still load as a plugin. + entries = instructionDirEntries(files, a.Name, []string{".claude/skills", ".claude/commands"}) + case a.Name == string(agent.AgentNameClaudeCode): entries, err = claudeTrustEntries(files) - case string(agent.AgentNameCodex): + case a.Name == string(agent.AgentNameCodex) && a.Isolated: + // The checkout is untrusted for the run, which drops its config + // layer; skills may still be discovered. + entries = instructionDirEntries(files, a.Name, codexInstructionDirs) + case a.Name == string(agent.AgentNameCodex): entries, err = codexTrustEntries(files) - case string(agent.AgentNamePi): + case a.Name == string(agent.AgentNamePi): entries, err = piTrustEntries(files) + if a.Isolated { + // --no-extensions drops the checkout's extensions; its + // settings, skills and prompt templates still load. + entries = slices.DeleteFunc(entries, func(e cliReview.TrustEntry) bool { + return e.Kind == cliReview.TrustKindExtension + }) + } default: // Other agents have no reviewer runner and run nothing. continue diff --git a/cmd/entire/cli/review_trust_inventory_test.go b/cmd/entire/cli/review_trust_inventory_test.go index 6cd93ea1bf..11d3284321 100644 --- a/cmd/entire/cli/review_trust_inventory_test.go +++ b/cmd/entire/cli/review_trust_inventory_test.go @@ -23,11 +23,11 @@ import ( func trustInventoryBoth(t *testing.T, dir string, agents ...string) cliReview.TrustInventory { t.Helper() head := gitOutputInDir(t, dir, "rev-parse", "HEAD") - fromTree, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, agents) + fromTree, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, trustAgents(agents...)) if err != nil { t.Fatalf("inspect tree: %v", err) } - fromDisk, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{WorktreeRoot: dir}, agents) + fromDisk, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{WorktreeRoot: dir}, trustAgents(agents...)) if err != nil { t.Fatalf("inspect disk: %v", err) } @@ -274,7 +274,7 @@ func TestTrustInventory_InstalledHooksAreEntire(t *testing.T) { t.Fatalf("InstallHooks(%s): %v", name, err) } } - inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{WorktreeRoot: dir}, []string{"claude-code", "codex", "pi"}) + inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{WorktreeRoot: dir}, trustAgents("claude-code", "codex", "pi")) if err != nil { t.Fatal(err) } @@ -324,7 +324,7 @@ func TestTrustInventory_MixedCaseConfigInTree(t *testing.T) { ".MCP.json": `{"mcpServers":{"evil":{"command":"evil-mcp"}}}`, }) head := gitOutputInDir(t, dir, "rev-parse", "HEAD") - inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, []string{"claude-code"}) + inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, trustAgents("claude-code")) if err != nil { t.Fatal(err) } @@ -495,3 +495,47 @@ func TestTrustInventory_UnnamedSecretsAreRedactedByContent(t *testing.T) { t.Errorf("mcp gh = %q, want %q", got["mcp gh"], want) } } + +func trustAgents(names ...string) []cliReview.TrustAgent { + agents := make([]cliReview.TrustAgent, 0, len(names)) + for _, name := range names { + agents = append(agents, cliReview.TrustAgent{Name: name}) + } + return agents +} + +// With a profile config, only what still loads from the checkout is listed: +// Claude's skills and commands, Pi's settings and skills (not extensions). +func TestTrustInventory_IsolatedAgentsListOnlyWhatStillLoads(t *testing.T) { + t.Parallel() + dir := newTrustInventoryRepo(t, map[string]string{ + ".claude/settings.json": `{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"evil-hook"}]}]}}`, + ".mcp.json": `{"mcpServers":{"evil":{"command":"evil-mcp"}}}`, + ".claude/skills/review/SKILL.md": "x", + ".pi/extensions/evil/index.ts": "x", + ".pi/settings.json": `{"packages":["npm:evil"]}`, + }) + head := gitOutputInDir(t, dir, "rev-parse", "HEAD") + agents := []cliReview.TrustAgent{{Name: "claude-code", Isolated: true}, {Name: "pi", Isolated: true}} + inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, agents) + if err != nil { + t.Fatal(err) + } + if !inv.Isolated || !slices.Equal(inv.IsolatedAgents, []string{"claude-code", "pi"}) { + t.Fatalf("Isolated = %v, IsolatedAgents = %v", inv.Isolated, inv.IsolatedAgents) + } + got := map[string]bool{} + for _, e := range inv.Entries { + got[e.Command] = true + } + for _, gone := range []string{"evil-hook", "evil-mcp", ".pi/extensions/evil/index.ts"} { + if got[gone] { + t.Errorf("%q listed although the profile config replaces it", gone) + } + } + for _, want := range []string{".claude/skills/review", `["npm:evil"]`} { + if !got[want] { + t.Errorf("%q not listed although it still loads; got %v", want, got) + } + } +} diff --git a/cmd/entire/cli/settings/agent_config_trust_test.go b/cmd/entire/cli/settings/agent_config_trust_test.go new file mode 100644 index 0000000000..c76afc3887 --- /dev/null +++ b/cmd/entire/cli/settings/agent_config_trust_test.go @@ -0,0 +1,78 @@ +package settings + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const configWithHook = `{"settings":{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"/usr/bin/true"}]}]}}}` + +// A reviewer config names commands Entire runs, so one from the committed +// project file is dropped, and the rejection carries no value (it can hold +// secrets). +func TestAgentConfigTrust_ProjectConfigIsDropped(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true,"review_profiles":{"general":{"agents":{"claude-code":{"config":`+configWithHook+`}}}}}`) + + s := loadedForPromptTrust(t, project, "", local) + worker, ok := s.ReviewProfiles["general"].Agents["claude-code"] + require.True(t, ok, "a worker whose only field was a dropped config must stay present") + assert.Nil(t, worker.Config) + rejections := s.AgentPromptRejections() + require.Len(t, rejections, 1) + assert.Equal(t, "review_profiles.general.agents.claude-code.config", rejections[0].Field) + assert.Empty(t, rejections[0].Value) + assert.False(t, AgentConfigRejectionUnverified(rejections[0])) +} + +// Clone-local configs overlay a profile defined in the project file, which +// keeps supplying the profile's other fields. +func TestAgentConfigTrust_ClonePreferencesConfigIsHonored(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true,"review_profiles":{"general":{"agents":{"claude-code":{"model":"sonnet"}}}}}`) + prefs := writePreferences(t, `{"review_agent_configs":{"general":{"claude-code":`+configWithHook+`}}}`) + + s := loadedForPromptTrust(t, project, prefs, local) + worker := s.ReviewProfiles["general"].Agents["claude-code"] + require.NotNil(t, worker.Config) + assert.Contains(t, string(worker.Config.Settings), "/usr/bin/true") + assert.Equal(t, "sonnet", worker.Model) + assert.Empty(t, s.AgentPromptRejections()) +} + +func TestAgentConfigTrust_UntrackedLocalConfigIsHonored(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true}`) + writeSettingsFile(t, local, `{"review_profiles":{"general":{"agents":{"pi":{"config":{"extensions":["/opt/ext.ts"]}}}}}}`) + + s := loadedForPromptTrust(t, project, "", local) + cfg := s.ReviewProfiles["general"].Agents["pi"].Config + require.NotNil(t, cfg) + assert.Equal(t, []string{"/opt/ext.ts"}, cfg.Extensions) +} + +// The judge runs from a temp dir and the legacy map predates profiles; a +// config in either is reported, never silently used. +func TestAgentConfigTrust_JudgeAndLegacyConfigAreRejected(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true}`) + prefs := writePreferences(t, `{"review_profiles":{"general":{"agents":{"codex":{"model":"x"}},`+ + `"judge":{"agent":"claude-code","config":{}}}},"review":{"pi":{"config":{}}}}`) + + s := loadedForPromptTrust(t, project, prefs, local) + require.NotNil(t, s.ReviewProfiles["general"].Judge) + assert.Nil(t, s.ReviewProfiles["general"].Judge.Config) + assert.Nil(t, s.Review["pi"].Config) + fields := map[string]bool{} + for _, rej := range s.AgentPromptRejections() { + fields[rej.Field] = true + } + assert.True(t, fields["review_profiles.general.judge.config"]) + assert.True(t, fields["review.pi.config"]) +} diff --git a/cmd/entire/cli/settings/agent_prompt_trust.go b/cmd/entire/cli/settings/agent_prompt_trust.go index f62747713d..f10af2f8a6 100644 --- a/cmd/entire/cli/settings/agent_prompt_trust.go +++ b/cmd/entire/cli/settings/agent_prompt_trust.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "sort" + "strings" ) // AgentPromptRejection reports one agent instruction field Load dropped as @@ -23,8 +24,19 @@ type AgentPromptRejection struct { const ( agentPromptRejectionNotLocal = "it did not come from .entire/settings.local.json or clone-local preferences" agentPromptRejectionUnverified = "the local settings file could not be verified as untracked" + // agentConfigRejectionUnsupported marks a reviewer config where none is supported + // (the legacy review map, the judge). + agentConfigRejectionUnsupported = "agent config is only supported for review profile reviewers" ) +// AgentConfigRejectionUnverified reports whether rej dropped a reviewer +// config from a local settings file that could not be verified as untracked. +// The user expects that reviewer to be isolated, so the review must fail +// rather than run with the checkout's config. +func AgentConfigRejectionUnverified(rej AgentPromptRejection) bool { + return strings.HasSuffix(rej.Field, ".config") && rej.Reason == agentPromptRejectionUnverified +} + // AgentPromptRejections reports the agent instruction fields Load dropped as // untrusted. Consumers that would have applied a dropped field (review) should // surface these on stderr, because it is the only signal that an instruction @@ -145,6 +157,31 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin } } + // decideConfig gates a reviewer config like decide gates text. The + // rejection carries no value: the config can hold secrets. + decideConfig := func(field string, cfg *ReviewAgentConfig, setLocally, prefsOwned bool) *ReviewAgentConfig { + if cfg == nil { + return nil + } + if decide(field, "set", setLocally, prefsOwned) == "" { + for i := range s.agentPromptRejections { + if s.agentPromptRejections[i].Field == field { + s.agentPromptRejections[i].Value = "" + } + } + return nil + } + return cfg + } + // rejectConfig reports a config where none is supported; the caller + // clears it. + rejectConfig := func(field string, cfg *ReviewAgentConfig) { + if cfg != nil { + s.agentPromptRejections = append(s.agentPromptRejections, + AgentPromptRejection{Field: field, Reason: agentConfigRejectionUnsupported}) + } + } + // Legacy review map: every layer replaces it wholesale, so its owner is // the last layer that set the key at all. _, localSetsReview := localRaw["review"] @@ -154,7 +191,10 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin hadPrompt := cfg.Prompt != "" cfg.Prompt = decide("review."+worker+".prompt", cfg.Prompt, rawHasKey(localRaw, "review", worker, "prompt"), prefsOwnReview) - keepWorkerPresent(&cfg, worker, hadPrompt) + hadConfig := cfg.Config != nil + rejectConfig("review."+worker+".config", cfg.Config) + cfg.Config = nil + keepWorkerPresent(&cfg, worker, hadPrompt || hadConfig) s.Review[worker] = cfg } @@ -177,7 +217,11 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin hadPrompt := cfg.Prompt != "" cfg.Prompt = decide("review_profiles."+name+".agents."+worker+".prompt", cfg.Prompt, rawHasKey(localRaw, "review_profiles", name, "agents", worker, "prompt"), prefsOwnProfile) - keepWorkerPresent(&cfg, worker, hadPrompt) + hadConfig := cfg.Config != nil + cfg.Config = decideConfig("review_profiles."+name+".agents."+worker+".config", cfg.Config, + rawHasKey(localRaw, "review_profiles", name, "agents", worker, "config"), + prefsOwnProfile || (!localSetsProfile && prefs != nil && prefs.ReviewAgentConfigs[name][worker] != nil)) + keepWorkerPresent(&cfg, worker, hadPrompt || hadConfig) profile.Agents[worker] = cfg } if profile.Judge != nil { @@ -188,6 +232,8 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin // auto-select-a-judge fallback, which is the sane degradation. profile.Judge.Prompt = decide("review_profiles."+name+".judge.prompt", profile.Judge.Prompt, rawHasKey(localRaw, "review_profiles", name, "judge", "prompt"), prefsOwnProfile) + rejectConfig("review_profiles."+name+".judge.config", profile.Judge.Config) + profile.Judge.Config = nil } s.ReviewProfiles[name] = profile } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 19621200e7..9298dec7b4 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -247,6 +247,11 @@ type ClonePreferences struct { ReviewProfiles map[string]ReviewProfileConfig `json:"review_profiles,omitempty"` ReviewDefaultProfile string `json:"review_default_profile,omitempty"` + // ReviewAgentConfigs holds reviewer agent configs by profile, then + // reviewer. They overlay the effective profile at load, so the profile's + // other fields keep coming from whichever layer defines them. + ReviewAgentConfigs map[string]map[string]*ReviewAgentConfig `json:"review_agent_configs,omitempty"` + // Deprecated: legacy pre-profile review settings. Kept so old preference // files parse. New review setup writes ReviewProfiles instead, while // `entire review` may read Review as a fallback when profiles are absent. @@ -542,11 +547,31 @@ type ReviewConfig struct { // settings by any route other than Load() (LoadFromFile, LoadFromBytes) // get the ungated value and must not hand it to an agent. Prompt string `json:"prompt,omitempty"` + + // Config, when set, replaces the reviewed checkout's agent config for this + // reviewer: the checkout's hooks, MCP servers and extensions are not + // loaded, and these are used instead. Presence (even {}) means isolated. + // + // It names commands Entire runs, so Load() honors it only from a + // developer-owned layer, like Prompt; see enforceAgentPromptTrust. + Config *ReviewAgentConfig `json:"config,omitempty"` +} + +// ReviewAgentConfig is a reviewer's own agent config, in each agent's native +// shape. Which fields an agent accepts is checked when the review runs. +type ReviewAgentConfig struct { + // Settings is a Claude Code settings object (hooks, permissions, env, ...). + Settings json.RawMessage `json:"settings,omitempty"` + // MCPServers maps a server name to its definition ({command, args, env} + // or {url}), for Claude Code and Codex. + MCPServers map[string]json.RawMessage `json:"mcp_servers,omitempty"` + // Extensions are absolute paths of Pi extension files. + Extensions []string `json:"extensions,omitempty"` } // IsZero reports whether the config is effectively unset. func (c ReviewConfig) IsZero() bool { - return c.Agent == "" && c.Model == "" && len(c.Skills) == 0 && c.Prompt == "" + return c.Agent == "" && c.Model == "" && len(c.Skills) == 0 && c.Prompt == "" && c.Config == nil } // LocalLayerRejection reports why .entire/settings.local.json was ignored, or @@ -1283,6 +1308,21 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { if prefs.ReviewProfiles != nil { settings.ReviewProfiles = mergeReviewProfiles(settings.ReviewProfiles, prefs.ReviewProfiles) } + for name, workers := range prefs.ReviewAgentConfigs { + profile, ok := settings.ReviewProfiles[name] + if !ok { + continue + } + agents := make(map[string]ReviewConfig, len(profile.Agents)) + for worker, cfg := range profile.Agents { + if agentCfg, ok := workers[worker]; ok { + cfg.Config = agentCfg + } + agents[worker] = cfg + } + profile.Agents = agents + settings.ReviewProfiles[name] = profile + } if prefs.ReviewDefaultProfile != "" { settings.ReviewDefaultProfile = prefs.ReviewDefaultProfile } diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 2b3d74148b..05f561ee8a 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -147,6 +147,60 @@ approval first. prompt for agents without a runner). The judge runs from a temp directory and does not load the checkout's configuration. +## Reviewer agent config in profiles + +A review profile can give each reviewer its own agent config, which replaces +the reviewed checkout's hooks, MCP servers and extensions for that reviewer. +Set it with `entire review --edit` (per reviewer: keep, use the checkout's, +own config with nothing extra, or load a JSON file) or +`entire review --configure --set-config =`. + +```json +{"settings": {"hooks": {...}, "permissions": {...}, "env": {...}}, + "mcp_servers": {"docs": {"command": "/opt/mcp/docs", "args": ["--stdio"]}}, + "extensions": ["/opt/pi/review.ts"]} +``` + +| Agent | Checkout config dropped | Profile config applied | Still loaded from the checkout | +|---|---|---|---| +| Claude Code | `--setting-sources user`, `--strict-mcp-config` | `settings` (with Entire's tracking hooks added) via `--settings`; `mcp_servers` via `--mcp-config` | CLAUDE.md; `.claude/skills` and `.claude/commands`, copied from the committed tree as the `project` plugin (`/review` becomes `/project:review`) | +| Codex | checkout and main repo marked untrusted for the run (project config, hooks, rules) | `mcp_servers` via `-c` (no literal `env` values yet) | AGENTS.md; skills | +| Pi | `--no-extensions` | `extensions` via `--extension`, plus Entire's own | AGENTS.md/CLAUDE.md, skills, prompt templates, `.pi/settings.json` | + +Rules: + +- **Developer-owned only.** A reviewer config is honored only from clone-local + preferences or an untracked `.entire/settings.local.json`, never from the + committed settings file (dropped with a note), the judge, or the legacy + `review` map. A local file that can't be verified as untracked fails the + review instead of running it with the checkout's config. Saves go to the + local file when it defines the profile, else clone-local preferences + (`review_agent_configs`, by profile and reviewer), which overlay only the + config so the rest of the profile still comes from its own layer. +- **Commands stay outside the checkout.** Every hook, MCP and helper command + must be a plain command: an absolute program outside the reviewed and the + user's checkout (or a bare tool name) with plain arguments. Shell syntax + (`;`, `&`, `|`, redirects, `$`, backticks, quotes) is refused, so splitting + on whitespace is exact and every word is checked; hooks that need a shell go + in a script at an absolute path. Relative paths, `$CLAUDE_PROJECT_DIR`, and + launchers that resolve tools from the project (`npx`, `uvx`, `bunx`, …, even + by absolute path) are refused, at save time and again before each run. `env` + values (MCP servers and Claude settings) may not name a path inside a + checkout, and `*PATH` variables list only absolute directories or inherited + variables. Containment follows symlinks and ignores case on Windows. +- **Fail, don't fall back.** An agent that can't apply a field (Codex + `settings`, Pi `mcp_servers`, …) fails the review with an explanation. +- **Gate.** Skills and commands can run their own commands, so reviewing + someone else's code still needs approval; for reviewers with their own + config the warning lists only what still loads from the branch, and + `--show-config` names them (`isolated_agents` in `--json`). Each run prints + which reviewers use their own config. +- Files the run needs (settings, MCP config, the skills plugin) are written + 0600 to a per-run directory under the user cache and removed when the + reviewer exits. An older `entire` binary rejects settings files containing + `config`. +- Team-shared reviewer config and Codex hooks are follow-ups. + ## Flow 1. With `--target`, `entire review` selects the profile in the caller's checkout, resolves the branch directly or through its trail, pins its head, runs the trust gate, prepares a worktree, and re-runs the command there without `--target`.