From 26dfb3cb48d431e3620721a1d6465745894ac742 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 7 Oct 2026 16:19:48 -0700 Subject: [PATCH 01/22] Let review profiles carry each reviewer's own agent config A reviewer in a review profile can now have its own agent config, set with `entire review --edit` or `--configure --set-config reviewer=`. It replaces the reviewed checkout's hooks, MCP servers and extensions for that reviewer: - Claude Code: --setting-sources user and --strict-mcp-config drop the checkout's settings and MCP servers; the profile's settings (with Entire's tracking hooks added) and MCP servers are passed as 0600 files. The checkout's skills and commands load as the "project" plugin, copied from the committed tree. - Codex: the checkout and the main repo are marked untrusted for the run; profile MCP servers go through -c. Codex hooks and literal MCP env values are refused. - Pi: --no-extensions, plus Entire's extension and the profile's. Configs are honored only from developer-owned layers and saved there; a local file that can't be verified fails the review. Every command a config names must be absolute and outside the checkouts, or a bare tool name. Reviewing someone else's code still needs approval, since the branch's skills load; the warning and --show-config say which reviewers use their own config and list only what still loads. Co-Authored-By: Claude Opus 5.5 --- .../cli/agent/claudecode/review_config.go | 129 +++++++++++ .../agent/claudecode/review_config_test.go | 132 +++++++++++ cmd/entire/cli/agent/claudecode/reviewer.go | 2 + cmd/entire/cli/agent/codex/review_config.go | 104 +++++++++ .../cli/agent/codex/review_config_test.go | 73 ++++++ cmd/entire/cli/agent/codex/reviewer.go | 10 + cmd/entire/cli/agent/pi/review_config.go | 38 ++++ cmd/entire/cli/agent/pi/review_config_test.go | 40 ++++ cmd/entire/cli/agent/pi/reviewer.go | 2 + cmd/entire/cli/review/agent_config.go | 205 +++++++++++++++++ cmd/entire/cli/review/agent_config_run.go | 166 ++++++++++++++ cmd/entire/cli/review/agent_config_save.go | 209 ++++++++++++++++++ cmd/entire/cli/review/agent_config_test.go | 111 ++++++++++ cmd/entire/cli/review/cmd.go | 60 ++++- cmd/entire/cli/review/picker.go | 68 ++++++ cmd/entire/cli/review/profile.go | 3 + cmd/entire/cli/review/target.go | 4 +- cmd/entire/cli/review/trust.go | 54 ++++- cmd/entire/cli/review/trust_cmd_test.go | 44 +++- cmd/entire/cli/review/trust_run.go | 42 ++-- cmd/entire/cli/review/types/reviewer.go | 20 ++ cmd/entire/cli/review/types/template.go | 30 +++ cmd/entire/cli/review_trust_inventory.go | 34 ++- cmd/entire/cli/review_trust_inventory_test.go | 52 ++++- .../cli/settings/agent_config_trust_test.go | 75 +++++++ cmd/entire/cli/settings/agent_prompt_trust.go | 49 +++- cmd/entire/cli/settings/settings.go | 22 +- docs/architecture/review-command.md | 47 ++++ 28 files changed, 1768 insertions(+), 57 deletions(-) create mode 100644 cmd/entire/cli/agent/claudecode/review_config.go create mode 100644 cmd/entire/cli/agent/claudecode/review_config_test.go create mode 100644 cmd/entire/cli/agent/codex/review_config.go create mode 100644 cmd/entire/cli/agent/codex/review_config_test.go create mode 100644 cmd/entire/cli/agent/pi/review_config.go create mode 100644 cmd/entire/cli/agent/pi/review_config_test.go create mode 100644 cmd/entire/cli/review/agent_config.go create mode 100644 cmd/entire/cli/review/agent_config_run.go create mode 100644 cmd/entire/cli/review/agent_config_save.go create mode 100644 cmd/entire/cli/review/agent_config_test.go create mode 100644 cmd/entire/cli/settings/agent_config_trust_test.go 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..48ea23fb0d --- /dev/null +++ b/cmd/entire/cli/agent/codex/review_config.go @@ -0,0 +1,104 @@ +package codex + +import ( + "context" + "encoding/json" + "fmt" + "slices" + + "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) + } + for _, root := range roots { + cfg.ExtraArgs = append(cfg.ExtraArgs, "-c", untrustedProjectOverride(root)) + } + 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 root untrusted for one codex run. +func untrustedProjectOverride(root string) string { + quoted, err := json.Marshal(root) + if err != nil { + quoted = []byte(`""`) + } + return "projects={" + string(quoted) + `={trust_level="untrusted"}}` +} + +// 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..fcbba972e4 --- /dev/null +++ b/cmd/entire/cli/agent/codex/review_config_test.go @@ -0,0 +1,73 @@ +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 + for _, root := range []string{wtResolved, mainResolved} { + if !strings.Contains(args, untrustedProjectOverride(root)) { + t.Errorf("ExtraArgs missing untrusted override for %s:\n%s", root, 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..3b378aac00 --- /dev/null +++ b/cmd/entire/cli/review/agent_config.go @@ -0,0 +1,205 @@ +package review + +import ( + "encoding/json" + "errors" + "fmt" + "path/filepath" + "regexp" + "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"} + +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"` + } + if err := json.Unmarshal(raw, &server); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + switch { + case server.Command != "": + if err := validateCommand(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") || (filepath.IsAbs(arg) && under(arg, forbiddenRoots)) { + return fmt.Errorf("MCP server %q: argument %q points into a checkout", name, arg) + } + } + 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) + } + 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 +} + +// validateCommand accepts a command line whose program is an absolute path +// outside every forbidden root, or a bare tool name found on PATH, and which +// never refers to the project directory. +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) + } + fields := strings.Fields(command) + if len(fields) == 0 { + return errors.New("empty command") + } + program := strings.Trim(fields[0], `"'`) + switch { + case filepath.IsAbs(program): + if under(program, forbiddenRoots) { + return fmt.Errorf("%q runs a program inside a checkout", command) + } + case strings.ContainsAny(program, `/\`): + return fmt.Errorf("%q uses a relative path, which resolves inside the reviewed checkout; use an absolute path", command) + case slices.Contains(projectLaunchers, program): + return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project; use an absolute path to the tool", command, program) + } + for _, field := range fields[1:] { + arg := strings.Trim(field, `"'`) + if strings.HasPrefix(arg, "./") || strings.HasPrefix(arg, "../") || (filepath.IsAbs(arg) && under(arg, forbiddenRoots)) { + return fmt.Errorf("%q passes %q, which points into a checkout", command, arg) + } + } + return nil +} + +func under(path string, roots []string) bool { + clean := filepath.Clean(path) + for _, root := range roots { + if root == "" { + continue + } + root = filepath.Clean(root) + if clean == root || strings.HasPrefix(clean, root+string(filepath.Separator)) { + return true + } + } + return false +} + +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..72364b6f4c --- /dev/null +++ b/cmd/entire/cli/review/agent_config_save.go @@ -0,0 +1,209 @@ +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. Profiles merge whole per +// layer, so the effective profile is copied there with the configs applied. +// 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 { + if p.ReviewProfiles == nil { + p.ReviewProfiles = map[string]settings.ReviewProfileConfig{} + } + p.ReviewProfiles[profileName] = profile + 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..6f55aa59c5 --- /dev/null +++ b/cmd/entire/cli/review/agent_config_test.go @@ -0,0 +1,111 @@ +package review + +import ( + "encoding/json" + "strings" + "testing" + + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +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")}, "points into a checkout"}, + {"apiKeyHelper relative", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"apiKeyHelper":"bin/key"}`)}, "relative path"}, + {"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 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"}, + {"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) + } + }) + } +} + +// 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/picker.go b/cmd/entire/cli/review/picker.go index e7751ad045..655a1eec29 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,8 @@ func saveReviewProfileConfig(ctx context.Context, profileName string, agents map // clobbered with built-in defaults. profile := profiles[profileName] profile.Agents = agents + // Reviewer configs are saved separately, to a developer-owned layer. + profile = stripAgentConfigs(profile) if strings.TrimSpace(judgeAgent) != "" { profile.Judge = &settings.ReviewConfig{Agent: strings.TrimSpace(judgeAgent)} } else { @@ -1345,3 +1363,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..306f98e42e 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 (%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..4e11dc08ea 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,42 @@ 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) + } + + 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..efb949429e --- /dev/null +++ b/cmd/entire/cli/settings/agent_config_trust_test.go @@ -0,0 +1,75 @@ +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])) +} + +func TestAgentConfigTrust_ClonePreferencesConfigIsHonored(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true}`) + prefs := writePreferences(t, `{"review_profiles":{"general":{"agents":{"claude-code":{"config":`+configWithHook+`}}}}}`) + + s := loadedForPromptTrust(t, project, prefs, local) + cfg := s.ReviewProfiles["general"].Agents["claude-code"].Config + require.NotNil(t, cfg) + assert.Contains(t, string(cfg.Settings), "/usr/bin/true") + 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..9209c3783a 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,10 @@ 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) + keepWorkerPresent(&cfg, worker, hadPrompt || hadConfig) profile.Agents[worker] = cfg } if profile.Judge != nil { @@ -188,6 +231,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..898593734e 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -542,11 +542,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 diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 2b3d74148b..6db444e8e6 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -147,6 +147,53 @@ 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 + layer that owns the profile, else clone-local preferences, which then holds a + copy of the whole profile. +- **Commands stay outside the checkout.** Every hook, MCP and helper command + must be an absolute path outside the reviewed and the user's checkout, or a + bare tool name; relative paths, `$CLAUDE_PROJECT_DIR`, and launchers that + resolve tools from the project (`npx`, `uvx`, `bunx`, …) are refused, at save + time and again before each run. +- **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`. From 17aadee9a63a463ce19ba6697756c1975f866f1b Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 8 Oct 2026 10:21:13 -0700 Subject: [PATCH 02/22] Check every word of a reviewer config command Validation looked only at the first word of a hook command, split on whitespace without quoting, so "/usr/bin/true; node_modules/.bin/x" or a quoted path with spaces inside the checkout got through, and MCP arguments were checked only when absolute. Commands are now split like a shell would (quotes, escapes, ; && | as separators) and every word is checked: paths must be absolute and outside the checkouts, and $VAR paths, command substitution and project launchers are refused. MCP commands are checked as one program path, and their arguments word by word. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4E8H349JGBMPP75KSV3X25A --- cmd/entire/cli/review/agent_config.go | 132 +++++++++++++++++---- cmd/entire/cli/review/agent_config_test.go | 17 ++- 2 files changed, 127 insertions(+), 22 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 3b378aac00..31019c2792 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -98,12 +98,22 @@ func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string } switch { case server.Command != "": - if err := validateCommand(server.Command, forbiddenRoots); err != nil { + // The command is one program path, not a shell line: check it whole. + if strings.Contains(server.Command, "CLAUDE_PROJECT_DIR") || slices.Contains(projectLaunchers, server.Command) { + return fmt.Errorf("MCP server %q: %q resolves from the reviewed project; use an absolute path to the tool", 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") || (filepath.IsAbs(arg) && under(arg, forbiddenRoots)) { - return fmt.Errorf("MCP server %q: argument %q points into a checkout", name, arg) + 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 slices.Contains(projectLaunchers, arg) { + return fmt.Errorf("MCP server %q: argument %q resolves tools from the reviewed project", name, arg) + } + if err := validateCommandWord(arg, forbiddenRoots); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) } } case server.URL == "": @@ -150,37 +160,117 @@ func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error return nil } -// validateCommand accepts a command line whose program is an absolute path -// outside every forbidden root, or a bare tool name found on PATH, and which -// never refers to the project directory. +// validateCommand accepts a shell command only if none of its words can make +// the reviewed checkout supply code: every word is checked, including those of +// commands chained with ;, && or |. A word that is a path must be absolute and +// outside the forbidden roots; relative paths, $VAR paths, command +// substitution, the project directory and project launchers are refused. 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) } - fields := strings.Fields(command) - if len(fields) == 0 { + words, err := splitShellWords(command) + if err != nil { + return fmt.Errorf("%q: %w", command, err) + } + if len(words) == 0 { return errors.New("empty command") } - program := strings.Trim(fields[0], `"'`) - switch { - case filepath.IsAbs(program): - if under(program, forbiddenRoots) { - return fmt.Errorf("%q runs a program inside a checkout", command) + for _, word := range words { + if slices.Contains(projectLaunchers, word) { + return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project; use an absolute path to the tool", command, word) + } + if err := validateCommandWord(word, forbiddenRoots); err != nil { + return fmt.Errorf("%q: %w", command, err) } - case strings.ContainsAny(program, `/\`): - return fmt.Errorf("%q uses a relative path, which resolves inside the reviewed checkout; use an absolute path", command) - case slices.Contains(projectLaunchers, program): - return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project; use an absolute path to the tool", command, program) } - for _, field := range fields[1:] { - arg := strings.Trim(field, `"'`) - if strings.HasPrefix(arg, "./") || strings.HasPrefix(arg, "../") || (filepath.IsAbs(arg) && under(arg, forbiddenRoots)) { - return fmt.Errorf("%q passes %q, which points into a checkout", command, arg) + return nil +} + +func validateCommandWord(word string, forbiddenRoots []string) error { + values := []string{word} + if _, value, ok := strings.Cut(word, "="); ok && strings.HasPrefix(word, "-") { + values = append(values, value) // --flag=value + } + for _, v := range values { + 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 } +// splitShellWords splits a command line into words the way a POSIX shell +// would for these checks: quotes and backslashes group, and control operators +// (; & | < > ( )) separate commands. Command substitution is refused, since +// its result can't be checked. +func splitShellWords(s string) ([]string, error) { + var words []string + var cur strings.Builder + inWord := false + flush := func() { + if inWord { + words = append(words, cur.String()) + cur.Reset() + inWord = false + } + } + for i := 0; i < len(s); i++ { + c := s[i] + switch { + case c == '`' || (c == '$' && i+1 < len(s) && s[i+1] == '('): + return nil, errors.New("command substitution isn't allowed") + case c == ' ' || c == '\t' || c == '\n': + flush() + case strings.IndexByte(";&|<>()", c) >= 0: + flush() + case c == '\\': + if i+1 < len(s) { + i++ + cur.WriteByte(s[i]) + inWord = true + } + case c == '\'': + end := strings.IndexByte(s[i+1:], '\'') + if end < 0 { + return nil, errors.New("unterminated single quote") + } + cur.WriteString(s[i+1 : i+1+end]) + i += end + 1 + inWord = true + case c == '"': + i++ + for ; i < len(s) && s[i] != '"'; i++ { + if s[i] == '`' || (s[i] == '$' && i+1 < len(s) && s[i+1] == '(') { + return nil, errors.New("command substitution isn't allowed") + } + if s[i] == '\\' && i+1 < len(s) && strings.IndexByte(`"\$`+"`", s[i+1]) >= 0 { + i++ + } + cur.WriteByte(s[i]) + } + if i >= len(s) { + return nil, errors.New("unterminated double quote") + } + inWord = true + default: + cur.WriteByte(c) + inWord = true + } + } + flush() + return words, nil +} + func under(path string, roots []string) bool { clean := filepath.Clean(path) for _, root := range roots { diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 6f55aa59c5..b79c83d4d0 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -31,11 +31,26 @@ func TestValidateAgentConfig(t *testing.T) { {"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")}, "points into a checkout"}, + {"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 relative command", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true; node_modules/.bin/x")}, "relative path"}, + {"chained launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true && npx x")}, "resolves tools from the reviewed project"}, + {"piped into checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/cat x | /repo/bin/run")}, "inside a checkout"}, + {"quoted path with spaces inside checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/repo/my tools/run" --x`)}, "inside a checkout"}, + {"quoted path with spaces outside", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/opt/my tools/run" --x`)}, ""}, + {"command substitution", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/sh -c $(cat x)")}, "command substitution"}, + {"backticks in quotes", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/echo \"`id`\"")}, "command substitution"}, + {"variable path", "claude-code", reviewtypes.AgentConfig{Settings: hook("$HOME/bin/x")}, "depends on a variable"}, + {"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")}, ""}, + {"unterminated quote", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/opt/x`)}, "unterminated"}, {"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"`}, From 998bb140ef3c0f4de45c1951c37697875f4d284d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 8 Oct 2026 10:51:19 -0700 Subject: [PATCH 03/22] Accept only plain commands in reviewer configs Parsing shell to decide whether a command is safe added complexity to a security check. Commands are now refused outright if they use shell syntax (; & | < > ( ) $ ` quotes), so splitting on whitespace is exact and every word is checked; hooks that need a shell go in a script at an absolute path. The shell-word parser is removed. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4EA872H4EW8BVS8TRD1ZGY7 --- cmd/entire/cli/review/agent_config.go | 82 ++++------------------ cmd/entire/cli/review/agent_config_test.go | 20 +++--- docs/architecture/review-command.md | 11 +-- 3 files changed, 29 insertions(+), 84 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 31019c2792..b41e6eb7a7 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -160,19 +160,23 @@ func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error return nil } -// validateCommand accepts a shell command only if none of its words can make -// the reviewed checkout supply code: every word is checked, including those of -// commands chained with ;, && or |. A word that is a path must be absolute and -// outside the forbidden roots; relative paths, $VAR paths, command -// substitution, the project directory and project launchers are refused. +// 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) } - words, err := splitShellWords(command) - if err != nil { - return fmt.Errorf("%q: %w", command, err) + 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") } @@ -209,68 +213,6 @@ func validateCommandWord(word string, forbiddenRoots []string) error { return nil } -// splitShellWords splits a command line into words the way a POSIX shell -// would for these checks: quotes and backslashes group, and control operators -// (; & | < > ( )) separate commands. Command substitution is refused, since -// its result can't be checked. -func splitShellWords(s string) ([]string, error) { - var words []string - var cur strings.Builder - inWord := false - flush := func() { - if inWord { - words = append(words, cur.String()) - cur.Reset() - inWord = false - } - } - for i := 0; i < len(s); i++ { - c := s[i] - switch { - case c == '`' || (c == '$' && i+1 < len(s) && s[i+1] == '('): - return nil, errors.New("command substitution isn't allowed") - case c == ' ' || c == '\t' || c == '\n': - flush() - case strings.IndexByte(";&|<>()", c) >= 0: - flush() - case c == '\\': - if i+1 < len(s) { - i++ - cur.WriteByte(s[i]) - inWord = true - } - case c == '\'': - end := strings.IndexByte(s[i+1:], '\'') - if end < 0 { - return nil, errors.New("unterminated single quote") - } - cur.WriteString(s[i+1 : i+1+end]) - i += end + 1 - inWord = true - case c == '"': - i++ - for ; i < len(s) && s[i] != '"'; i++ { - if s[i] == '`' || (s[i] == '$' && i+1 < len(s) && s[i+1] == '(') { - return nil, errors.New("command substitution isn't allowed") - } - if s[i] == '\\' && i+1 < len(s) && strings.IndexByte(`"\$`+"`", s[i+1]) >= 0 { - i++ - } - cur.WriteByte(s[i]) - } - if i >= len(s) { - return nil, errors.New("unterminated double quote") - } - inWord = true - default: - cur.WriteByte(c) - inWord = true - } - } - flush() - return words, nil -} - func under(path string, roots []string) bool { clean := filepath.Clean(path) for _, root := range roots { diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index b79c83d4d0..0c280a2b3b 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -29,21 +29,21 @@ func TestValidateAgentConfig(t *testing.T) { {"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 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 relative command", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true; node_modules/.bin/x")}, "relative path"}, - {"chained launcher", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/true && npx x")}, "resolves tools from the reviewed project"}, - {"piped into checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/cat x | /repo/bin/run")}, "inside a checkout"}, - {"quoted path with spaces inside checkout", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/repo/my tools/run" --x`)}, "inside a checkout"}, - {"quoted path with spaces outside", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/opt/my tools/run" --x`)}, ""}, - {"command substitution", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/sh -c $(cat x)")}, "command substitution"}, - {"backticks in quotes", "claude-code", reviewtypes.AgentConfig{Settings: hook("/usr/bin/echo \"`id`\"")}, "command substitution"}, - {"variable path", "claude-code", reviewtypes.AgentConfig{Settings: hook("$HOME/bin/x")}, "depends on a variable"}, + {"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")}, ""}, - {"unterminated quote", "claude-code", reviewtypes.AgentConfig{Settings: hook(`"/opt/x`)}, "unterminated"}, {"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"}, diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 6db444e8e6..ee37c41157 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -177,10 +177,13 @@ Rules: layer that owns the profile, else clone-local preferences, which then holds a copy of the whole profile. - **Commands stay outside the checkout.** Every hook, MCP and helper command - must be an absolute path outside the reviewed and the user's checkout, or a - bare tool name; relative paths, `$CLAUDE_PROJECT_DIR`, and launchers that - resolve tools from the project (`npx`, `uvx`, `bunx`, …) are refused, at save - time and again before each run. + 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`, …) are + refused, at save time and again before each run. - **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 From 15f9eff5eb3009375d1741d017ffe177b0f2a886 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 8 Oct 2026 11:05:38 -0700 Subject: [PATCH 04/22] Catch project launchers by name and check flag values as paths A project launcher given by absolute path (/usr/local/bin/npx) was accepted, though it still resolves tools from the reviewed project; launchers now match by program name, including Windows extensions. A --flag=value word was checked whole, so --config=/opt/x.json read as a relative path; only the value is checked now. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4EB2DN44AAJS65D8680PKYA --- cmd/entire/cli/review/agent_config.go | 47 +++++++++++++--------- cmd/entire/cli/review/agent_config_test.go | 5 +++ 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index b41e6eb7a7..7a77ec0ca2 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -28,6 +28,17 @@ var agentConfigCommandKeys = []string{"apiKeyHelper", "awsAuthRefresh", "awsCred // 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. @@ -99,7 +110,7 @@ func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string 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") || slices.Contains(projectLaunchers, server.Command) { + if strings.Contains(server.Command, "CLAUDE_PROJECT_DIR") || isProjectLauncher(server.Command) { return fmt.Errorf("MCP server %q: %q resolves from the reviewed project; use an absolute path to the tool", name, server.Command) } if err := validateCommandWord(server.Command, forbiddenRoots); err != nil { @@ -109,7 +120,7 @@ func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string 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 slices.Contains(projectLaunchers, arg) { + if isProjectLauncher(arg) { return fmt.Errorf("MCP server %q: argument %q resolves tools from the reviewed project", name, arg) } if err := validateCommandWord(arg, forbiddenRoots); err != nil { @@ -181,7 +192,7 @@ func validateCommand(command string, forbiddenRoots []string) error { return errors.New("empty command") } for _, word := range words { - if slices.Contains(projectLaunchers, word) { + if isProjectLauncher(word) { return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project; use an absolute path to the tool", command, word) } if err := validateCommandWord(word, forbiddenRoots); err != nil { @@ -192,23 +203,21 @@ func validateCommand(command string, forbiddenRoots []string) error { } func validateCommandWord(word string, forbiddenRoots []string) error { - values := []string{word} + v := word if _, value, ok := strings.Cut(word, "="); ok && strings.HasPrefix(word, "-") { - values = append(values, value) // --flag=value - } - for _, v := range values { - 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) - } + 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 } diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 0c280a2b3b..8bd8343afc 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -44,6 +44,11 @@ func TestValidateAgentConfig(t *testing.T) { {"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")}, ""}, + {"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"}, From 6981bc4d0bdc338a093ce680c217db90b3755c4d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 8 Oct 2026 11:11:17 -0700 Subject: [PATCH 05/22] Stop telling people to use an absolute path to a launcher A launcher such as npx is refused even by absolute path, so the old advice led to a second refusal. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4EBCRMS9W6QPT24XB0QRE41 --- cmd/entire/cli/review/agent_config.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 7a77ec0ca2..fee2e3559e 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -111,7 +111,7 @@ func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string 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; use an absolute path to the tool", name, 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) @@ -121,7 +121,7 @@ func validateMCPServer(name string, raw json.RawMessage, forbiddenRoots []string 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 resolves tools from the reviewed project", name, 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) @@ -193,7 +193,7 @@ func validateCommand(command string, forbiddenRoots []string) error { } for _, word := range words { if isProjectLauncher(word) { - return fmt.Errorf("%q runs %s, which resolves tools from the reviewed project; use an absolute path to the tool", command, 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) From 0954f2a670a52afc0fd9b8d4ce98f6cdcce7d8f0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 9 Oct 2026 18:39:52 -0700 Subject: [PATCH 06/22] Address review comments on reviewer agent config - Codex: mark the checkout and main repo untrusted in one -c override; a second projects= assignment replaced the first. - Clone-local preferences keep only the reviewer configs, by profile and reviewer, and overlay them at load, so later profile edits still apply. - --edit keeps reviewer configs when re-saving a local profile; only the shared file drops them. - env values in MCP servers and Claude settings may not point into a checkout, and *PATH variables list only absolute directories. - Checkout containment follows symlinks and ignores case on Windows. - The no-terminal refusal says Pi's settings still load for an isolated Pi. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4HQEWEBDX2ZT1ZRTA2FZ88J --- cmd/entire/cli/agent/codex/review_config.go | 23 +++--- .../cli/agent/codex/review_config_test.go | 6 +- cmd/entire/cli/review/agent_config.go | 70 ++++++++++++++++--- cmd/entire/cli/review/agent_config_save.go | 27 +++++-- cmd/entire/cli/review/agent_config_test.go | 27 +++++++ cmd/entire/cli/review/configure_test.go | 30 ++++++++ cmd/entire/cli/review/picker.go | 6 +- cmd/entire/cli/review/trust.go | 2 +- cmd/entire/cli/review/trust_cmd_test.go | 20 ++++++ .../cli/settings/agent_config_trust_test.go | 13 ++-- cmd/entire/cli/settings/agent_prompt_trust.go | 3 +- cmd/entire/cli/settings/settings.go | 20 ++++++ docs/architecture/review-command.md | 12 ++-- 13 files changed, 216 insertions(+), 43 deletions(-) diff --git a/cmd/entire/cli/agent/codex/review_config.go b/cmd/entire/cli/agent/codex/review_config.go index 48ea23fb0d..9662453a0c 100644 --- a/cmd/entire/cli/agent/codex/review_config.go +++ b/cmd/entire/cli/agent/codex/review_config.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "slices" + "strings" "github.com/entireio/cli/cmd/entire/cli/review" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -35,9 +36,7 @@ func prepareCodexReviewConfig(ctx context.Context, cfg reviewtypes.RunConfig) (r if mainRoot != run.CheckoutRoot { roots = append(roots, mainRoot) } - for _, root := range roots { - cfg.ExtraArgs = append(cfg.ExtraArgs, "-c", untrustedProjectOverride(root)) - } + cfg.ExtraArgs = append(cfg.ExtraArgs, "-c", untrustedProjectOverride(roots)) servers, err := codexMCPOverrides(cfg.AgentConfig.MCPServers) if err != nil { return cfg, nil, err @@ -47,13 +46,19 @@ func prepareCodexReviewConfig(ctx context.Context, cfg reviewtypes.RunConfig) (r return cfg, nil, nil } -// untrustedProjectOverride marks root untrusted for one codex run. -func untrustedProjectOverride(root string) string { - quoted, err := json.Marshal(root) - if err != nil { - quoted = []byte(`""`) +// 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={" + string(quoted) + `={trust_level="untrusted"}}` + return "projects={" + strings.Join(entries, ",") + "}" } // codexMCPOverrides turns profile MCP servers into -c overrides. Values are diff --git a/cmd/entire/cli/agent/codex/review_config_test.go b/cmd/entire/cli/agent/codex/review_config_test.go index fcbba972e4..43dc43a0ce 100644 --- a/cmd/entire/cli/agent/codex/review_config_test.go +++ b/cmd/entire/cli/agent/codex/review_config_test.go @@ -36,10 +36,8 @@ func TestPrepareCodexReviewConfigLinkedWorktree(t *testing.T) { 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 - for _, root := range []string{wtResolved, mainResolved} { - if !strings.Contains(args, untrustedProjectOverride(root)) { - t.Errorf("ExtraArgs missing untrusted override for %s:\n%s", root, args) - } + 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) { diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index fee2e3559e..fe8c555bf5 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -6,6 +6,7 @@ import ( "fmt" "path/filepath" "regexp" + "runtime" "slices" "strings" @@ -100,13 +101,17 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig, forbidd 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"` + 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. @@ -138,6 +143,15 @@ func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error 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 != "" { @@ -171,6 +185,30 @@ func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error 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" @@ -222,20 +260,30 @@ func validateCommandWord(word string, forbiddenRoots []string) error { 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 { - clean := filepath.Clean(path) - for _, root := range roots { - if root == "" { - continue - } - root = filepath.Clean(root) - if clean == root || strings.HasPrefix(clean, root+string(filepath.Separator)) { - return true + 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 { diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go index 72364b6f4c..ce0f6aaa00 100644 --- a/cmd/entire/cli/review/agent_config_save.go +++ b/cmd/entire/cli/review/agent_config_save.go @@ -77,9 +77,9 @@ func stripAgentConfigs(profile settings.ReviewProfileConfig) settings.ReviewProf // 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. Profiles merge whole per -// layer, so the effective profile is copied there with the configs applied. -// A nil config removes it. It returns the file written. +// 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 { @@ -121,10 +121,25 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map return reviewScopeLocal.file(), nil } err = settings.ModifyClonePreferences(ctx, func(p *settings.ClonePreferences) error { - if p.ReviewProfiles == nil { - p.ReviewProfiles = map[string]settings.ReviewProfileConfig{} + 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 } - p.ReviewProfiles[profileName] = profile return nil }) if err != nil { diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 8bd8343afc..6a6057992c 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -2,11 +2,14 @@ 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) { @@ -61,6 +64,10 @@ func TestValidateAgentConfig(t *testing.T) { {"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 { @@ -81,6 +88,26 @@ func TestValidateAgentConfig(t *testing.T) { } } +// 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) { 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 655a1eec29..9cd61e4281 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -981,8 +981,10 @@ func saveReviewProfileConfig(ctx context.Context, profileName string, agents map // clobbered with built-in defaults. profile := profiles[profileName] profile.Agents = agents - // Reviewer configs are saved separately, to a developer-owned layer. - profile = stripAgentConfigs(profile) + // 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 { diff --git a/cmd/entire/cli/review/trust.go b/cmd/entire/cli/review/trust.go index 306f98e42e..a1901c7710 100644 --- a/cmd/entire/cli/review/trust.go +++ b/cmd/entire/cli/review/trust.go @@ -404,7 +404,7 @@ func printTrustRefusal(errOut io.Writer, what string, isolated bool, command, he fmt.Fprintln(errOut, "Not run: this review needs the user's approval.") 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 (%s --show-config lists them).\n", command) + 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) default: diff --git a/cmd/entire/cli/review/trust_cmd_test.go b/cmd/entire/cli/review/trust_cmd_test.go index 4e11dc08ea..90f5447d73 100644 --- a/cmd/entire/cli/review/trust_cmd_test.go +++ b/cmd/entire/cli/review/trust_cmd_test.go @@ -174,6 +174,26 @@ func TestConfigure_SetConfigSavesToClonePreferences(t *testing.T) { 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 { diff --git a/cmd/entire/cli/settings/agent_config_trust_test.go b/cmd/entire/cli/settings/agent_config_trust_test.go index efb949429e..c76afc3887 100644 --- a/cmd/entire/cli/settings/agent_config_trust_test.go +++ b/cmd/entire/cli/settings/agent_config_trust_test.go @@ -28,16 +28,19 @@ func TestAgentConfigTrust_ProjectConfigIsDropped(t *testing.T) { 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}`) - prefs := writePreferences(t, `{"review_profiles":{"general":{"agents":{"claude-code":{"config":`+configWithHook+`}}}}}`) + 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) - cfg := s.ReviewProfiles["general"].Agents["claude-code"].Config - require.NotNil(t, cfg) - assert.Contains(t, string(cfg.Settings), "/usr/bin/true") + 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()) } diff --git a/cmd/entire/cli/settings/agent_prompt_trust.go b/cmd/entire/cli/settings/agent_prompt_trust.go index 9209c3783a..f10af2f8a6 100644 --- a/cmd/entire/cli/settings/agent_prompt_trust.go +++ b/cmd/entire/cli/settings/agent_prompt_trust.go @@ -219,7 +219,8 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin rawHasKey(localRaw, "review_profiles", name, "agents", worker, "prompt"), prefsOwnProfile) hadConfig := cfg.Config != nil cfg.Config = decideConfig("review_profiles."+name+".agents."+worker+".config", cfg.Config, - rawHasKey(localRaw, "review_profiles", name, "agents", worker, "config"), prefsOwnProfile) + 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 } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 898593734e..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. @@ -1303,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 ee37c41157..05f561ee8a 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -174,16 +174,20 @@ Rules: 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 - layer that owns the profile, else clone-local preferences, which then holds a - copy of the whole profile. + 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`, …) are - refused, at save time and again before each run. + 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 From b54c39d9591c4e51e7a3c1f2d7354dfb01c3f96d Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 9 Oct 2026 18:49:44 -0700 Subject: [PATCH 07/22] Cover KEY=value words in reviewer commands Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4HR0XXR145GNZ0V01YWR5HZ --- cmd/entire/cli/review/agent_config_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 6a6057992c..a7f665eae6 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -51,6 +51,8 @@ func TestValidateAgentConfig(t *testing.T) { {"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"}`)}, ""}, From 99b61a1536a44e91cefe888ef6c89b29c0a793ff Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 11:35:38 -0700 Subject: [PATCH 08/22] Check the command in every Claude settings object statusLine and fileSuggestion run {"command": ...} too; any top-level settings object with a command is now validated. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4V8RZ7Q7XDV6RX39F1GYHTW --- cmd/entire/cli/review/agent_config.go | 12 ++++++++++++ cmd/entire/cli/review/agent_config_test.go | 2 ++ 2 files changed, 14 insertions(+) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index fe8c555bf5..d3b146e060 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -160,6 +160,18 @@ func validateClaudeSettings(raw json.RawMessage, forbiddenRoots []string) error } } } + // Settings such as statusLine and fileSuggestion run {"command": ...}; + // any top-level object with a command is checked, so new ones are too. + for _, key := range sortedStringKeys(settingsObj) { + var obj struct { + Command string `json:"command"` + } + if json.Unmarshal(settingsObj[key], &obj) == nil && obj.Command != "" { + if err := validateCommand(obj.Command, forbiddenRoots); err != nil { + return fmt.Errorf("settings %s: %w", key, err) + } + } + } var events map[string][]struct { Hooks []struct { Command string `json:"command"` diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index a7f665eae6..309367ed25 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -35,6 +35,8 @@ func TestValidateAgentConfig(t *testing.T) { {"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"}, + {"statusLine into checkout", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"statusLine":{"type":"command","command":"/repo/bin/status"}}`)}, "inside a checkout"}, + {"fileSuggestion relative", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"fileSuggestion":{"type":"command","command":"./suggest.sh"}}`)}, "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"}, From eba878fff6879d4258e5f430bc28ea7a8e520228 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 11:53:43 -0700 Subject: [PATCH 09/22] Count only agents that run toward the isolated verdict Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4V9T20YEPEMVCZWM0WACZTV --- cmd/entire/cli/review_trust_inventory.go | 15 ++++++++++----- cmd/entire/cli/review_trust_inventory_test.go | 3 ++- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/cmd/entire/cli/review_trust_inventory.go b/cmd/entire/cli/review_trust_inventory.go index be9ae00188..0eb090931d 100644 --- a/cmd/entire/cli/review_trust_inventory.go +++ b/cmd/entire/cli/review_trust_inventory.go @@ -96,16 +96,15 @@ func inspectReviewTrust(ctx context.Context, source cliReview.TrustSource, agent func buildTrustInventory(files trustFiles, agents []cliReview.TrustAgent) (cliReview.TrustInventory, error) { var inv cliReview.TrustInventory - inv.Isolated = len(agents) > 0 + // Isolated holds when every agent that runs is isolated; agents with no + // reviewer runner don't count either way. + inv.Isolated = true + ran := false for _, a := range agents { var ( entries []cliReview.TrustEntry err error ) - 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 @@ -135,6 +134,11 @@ func buildTrustInventory(files trustFiles, agents []cliReview.TrustAgent) (cliRe if err != nil { return cliReview.TrustInventory{}, err } + ran = true + inv.Isolated = inv.Isolated && a.Isolated + if a.Isolated { + inv.IsolatedAgents = append(inv.IsolatedAgents, a.Name) + } // Name-based hiding misses secrets in positional arguments, hook // commands, and innocuously named keys, so everything displayed also // goes through content-based redaction, which replaces only the @@ -145,6 +149,7 @@ func buildTrustInventory(files trustFiles, agents []cliReview.TrustAgent) (cliRe } inv.Entries = append(inv.Entries, entries...) } + inv.Isolated = inv.Isolated && ran for _, rel := range []string{trustClaudeMD, "CLAUDE.local.md", ".claude/" + trustClaudeMD, trustAgentsMD, "AGENTS.override.md"} { k, err := files.kind(rel) if err != nil { diff --git a/cmd/entire/cli/review_trust_inventory_test.go b/cmd/entire/cli/review_trust_inventory_test.go index 11d3284321..06a86db22b 100644 --- a/cmd/entire/cli/review_trust_inventory_test.go +++ b/cmd/entire/cli/review_trust_inventory_test.go @@ -516,7 +516,8 @@ func TestTrustInventory_IsolatedAgentsListOnlyWhatStillLoads(t *testing.T) { ".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}} + // cursor has no reviewer runner, so it doesn't count against isolation. + agents := []cliReview.TrustAgent{{Name: "claude-code", Isolated: true}, {Name: "pi", Isolated: true}, {Name: "cursor"}} inv, err := inspectReviewTrust(t.Context(), cliReview.TrustSource{RepoRoot: dir, Commit: head}, agents) if err != nil { t.Fatal(err) From 9663c4149400dc5e8d08913204851bb30a2bea04 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 12:05:48 -0700 Subject: [PATCH 10/22] Enforce where reviewer config comes from, not what its commands do The word-level command checks (refused launchers, shell syntax, PATH rules, per-argument paths) could not keep a reviewer's own commands from running branch code: a hook at an absolute path can still run npm scripts. They are removed. The profile's hooks and MCP servers are the user's code, like their own agent settings. Kept: config only from developer-owned files, refusal of fields an agent can't apply, and failing rather than falling back. A value naming a path inside the checkout or $CLAUDE_PROJECT_DIR now gets a warning. Prompts and docs say what is enforced: the branch's hooks, MCP servers and extensions are not loaded, and the profile's own run as configured. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VAG6QWZZ3JR7QY9A66BF32 --- .../cli/agent/claudecode/review_config.go | 2 +- .../agent/claudecode/review_config_test.go | 11 - cmd/entire/cli/agent/codex/review_config.go | 2 +- cmd/entire/cli/agent/pi/review_config.go | 2 +- cmd/entire/cli/review/agent_config.go | 275 +++++------------- cmd/entire/cli/review/agent_config_run.go | 21 +- cmd/entire/cli/review/agent_config_save.go | 22 +- cmd/entire/cli/review/agent_config_test.go | 98 +++---- cmd/entire/cli/review/cmd.go | 3 +- cmd/entire/cli/review/picker.go | 6 +- cmd/entire/cli/review/trust.go | 9 +- cmd/entire/cli/review/trust_cmd_test.go | 26 +- docs/architecture/review-command.md | 20 +- 13 files changed, 187 insertions(+), 310 deletions(-) diff --git a/cmd/entire/cli/agent/claudecode/review_config.go b/cmd/entire/cli/agent/claudecode/review_config.go index bb189b4d3d..8716653971 100644 --- a/cmd/entire/cli/agent/claudecode/review_config.go +++ b/cmd/entire/cli/agent/claudecode/review_config.go @@ -34,7 +34,7 @@ func prepareReviewAgentConfig(ctx context.Context, cfg reviewtypes.RunConfig) (r run.Cleanup() return cfg, nil, err } - if err := review.ValidateAgentConfig("claude-code", cfg.AgentConfig, run.ForbiddenRoots); err != nil { + if err := review.ValidateAgentConfig("claude-code", cfg.AgentConfig); err != nil { return fail(fmt.Errorf("review profile config: %w", err)) } settingsJSON, err := reviewProfileSettings(cfg.AgentConfig.Settings) diff --git a/cmd/entire/cli/agent/claudecode/review_config_test.go b/cmd/entire/cli/agent/claudecode/review_config_test.go index 9ccad860dd..5f0979bc9b 100644 --- a/cmd/entire/cli/agent/claudecode/review_config_test.go +++ b/cmd/entire/cli/agent/claudecode/review_config_test.go @@ -119,14 +119,3 @@ func TestPrepareReviewAgentConfigRefusesSymlinkedSkill(t *testing.T) { 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/codex/review_config.go b/cmd/entire/cli/agent/codex/review_config.go index 9662453a0c..5a23e33857 100644 --- a/cmd/entire/cli/agent/codex/review_config.go +++ b/cmd/entire/cli/agent/codex/review_config.go @@ -25,7 +25,7 @@ func prepareCodexReviewConfig(ctx context.Context, cfg reviewtypes.RunConfig) (r } // Nothing is written for Codex; the run only resolves paths. defer run.Cleanup() - if err := review.ValidateAgentConfig("codex", cfg.AgentConfig, run.ForbiddenRoots); err != nil { + if err := review.ValidateAgentConfig("codex", cfg.AgentConfig); err != nil { return cfg, nil, fmt.Errorf("review profile config: %w", err) } mainRoot, err := run.MainRepoRoot(ctx) diff --git a/cmd/entire/cli/agent/pi/review_config.go b/cmd/entire/cli/agent/pi/review_config.go index 9877d4bdd7..30baaf4c2d 100644 --- a/cmd/entire/cli/agent/pi/review_config.go +++ b/cmd/entire/cli/agent/pi/review_config.go @@ -21,7 +21,7 @@ func preparePiReviewConfig(ctx context.Context, cfg reviewtypes.RunConfig) (revi if err != nil { return cfg, nil, err //nolint:wrapcheck // already names the step } - if err := review.ValidateAgentConfig("pi", cfg.AgentConfig, run.ForbiddenRoots); err != nil { + if err := review.ValidateAgentConfig("pi", cfg.AgentConfig); err != nil { run.Cleanup() return cfg, nil, fmt.Errorf("review profile config: %w", err) } diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index d3b146e060..223402356a 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -21,25 +21,6 @@ var agentConfigFields = map[string][]string{ "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. @@ -50,11 +31,11 @@ func toAgentConfig(cfg *settings.ReviewAgentConfig) *reviewtypes.AgentConfig { 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 { +// ValidateAgentConfig checks that a reviewer's profile config is one the agent +// can apply. It does not vet the commands the config names: like the hooks in +// the user's own agent settings, they are the user's code and run in the +// checkout as configured. +func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { if cfg == nil { return nil } @@ -75,210 +56,95 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig, forbidd ); 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 + var obj map[string]json.RawMessage + if err := json.Unmarshal(cfg.Settings, &obj); err != nil { + return fmt.Errorf("settings: %w", 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) + for _, name := range sortedStringKeys(cfg.MCPServers) { + if !mcpServerNamePattern.MatchString(name) { + return fmt.Errorf("MCP server name %q: use letters, digits, '-' and '_'", name) } - } - 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) + var server struct { + Command string `json:"command"` + URL string `json:"url"` } - if err := validateCommandWord(server.Command, forbiddenRoots); err != nil { + if err := json.Unmarshal(cfg.MCPServers[name], &server); 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) - } + if server.Command == "" && server.URL == "" { + return fmt.Errorf("MCP server %q needs a command or a url", name) } - 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) - } - } - } - // Settings such as statusLine and fileSuggestion run {"command": ...}; - // any top-level object with a command is checked, so new ones are too. - for _, key := range sortedStringKeys(settingsObj) { - var obj struct { - Command string `json:"command"` - } - if json.Unmarshal(settingsObj[key], &obj) == nil && obj.Command != "" { - if err := validateCommand(obj.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"` +// AgentConfigWarnings lists values in cfg that name a path inside one of +// roots or the project directory variable: an easy copy-paste mistake, since +// those would run the reviewed branch's files. They are warnings, not +// refusals; the commands are the user's own. +func AgentConfigWarnings(cfg *reviewtypes.AgentConfig, roots []string) []string { + if cfg == nil { + return nil } - if hooks, ok := settingsObj["hooks"]; ok { - if err := json.Unmarshal(hooks, &events); err != nil { - return fmt.Errorf("settings hooks: %w", err) - } + var values []string + collectStrings(cfg.Settings, &values) + for _, name := range sortedStringKeys(cfg.MCPServers) { + collectStrings(cfg.MCPServers[name], &values) } - 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) - } - } + values = append(values, cfg.Extensions...) + var warnings []string + for _, value := range values { + if namesCheckout(value, roots) { + warnings = append(warnings, fmt.Sprintf("%q names a path inside the checkout; it would run the reviewed branch's files", value)) } } - return nil + return warnings } -// 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) +func collectStrings(raw json.RawMessage, out *[]string) { + var v any + if len(raw) == 0 || json.Unmarshal(raw, &v) != nil { + return + } + var walk func(any) + walk = func(v any) { + switch t := v.(type) { + case string: + *out = append(*out, t) + case []any: + for _, e := range t { + walk(e) + } + case map[string]any: + for _, k := range sortedStringKeys(t) { + walk(t[k]) } } } - return nil + walk(v) } -// 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") +func namesCheckout(value string, roots []string) bool { + if strings.Contains(value, "CLAUDE_PROJECT_DIR") { + return true } + words := strings.FieldsFunc(value, func(r rune) bool { + return r == ' ' || r == '\t' || r == '=' || r == '"' || r == '\'' || r == filepath.ListSeparator + }) 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) + if filepath.IsAbs(word) && under(word, roots) { + return true } - 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 + return false } // 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) - } + candidates := []string{filepath.Clean(path), resolveExisting(path)} for _, candidate := range candidates { for _, root := range roots { if root != "" && pathWithin(candidate, filepath.Clean(root)) { @@ -289,6 +155,23 @@ func under(path string, roots []string) bool { return false } +// resolveExisting resolves symlinks in the longest existing prefix of path, +// so a file that doesn't exist yet still resolves through a linked parent. +func resolveExisting(path string) string { + dir, rest := filepath.Clean(path), "" + for { + if resolved, err := filepath.EvalSymlinks(dir); err == nil { + return filepath.Join(resolved, rest) + } + parent := filepath.Dir(dir) + if parent == dir { + return filepath.Clean(path) + } + rest = filepath.Join(filepath.Base(dir), rest) + dir = parent + } +} + func pathWithin(path, root string) bool { if runtime.GOOS == "windows" { path, root = strings.ToLower(path), strings.ToLower(root) diff --git a/cmd/entire/cli/review/agent_config_run.go b/cmd/entire/cli/review/agent_config_run.go index f2e5309be4..74a214ae8b 100644 --- a/cmd/entire/cli/review/agent_config_run.go +++ b/cmd/entire/cli/review/agent_config_run.go @@ -8,6 +8,7 @@ import ( "os" "path" "path/filepath" + "slices" "strings" "github.com/entireio/cli/cmd/entire/cli/gitexec" @@ -26,8 +27,6 @@ type AgentConfigRun struct { 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. @@ -40,13 +39,6 @@ func NewAgentConfigRun(ctx context.Context) (*AgentConfigRun, error) { 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 { @@ -65,11 +57,10 @@ func NewAgentConfigRun(ctx context.Context) (*AgentConfigRun, error) { 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, + root: root, + name: name, + dir: filepath.Join(cacheDir, filepath.FromSlash(name)), + CheckoutRoot: canonical, }, nil } @@ -118,7 +109,7 @@ func (r *AgentConfigRun) CopyCheckoutTree(ctx context.Context, dirs map[string]s 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, "..") { + if rel == name || slices.Contains(strings.Split(rel, "/"), "..") { return copied, fmt.Errorf("unexpected path %q under %s", name, src) } blob, err := gitexec.Run(ctx, r.CheckoutRoot, "cat-file", "blob", fields[2]) diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go index ce0f6aaa00..d3f246fdb3 100644 --- a/cmd/entire/cli/review/agent_config_save.go +++ b/cmd/entire/cli/review/agent_config_save.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "path/filepath" "strings" @@ -46,9 +47,18 @@ func loadAgentConfigFile(path string) (*settings.ReviewAgentConfig, error) { 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 { +// validateAgentConfigForSave checks that agentName can apply cfg, and warns on +// errOut about values that name a path inside this checkout. +func validateAgentConfigForSave(ctx context.Context, errOut io.Writer, agentName string, cfg *settings.ReviewAgentConfig) error { + if err := ValidateAgentConfig(agentName, toAgentConfig(cfg)); err != nil { + return err + } + warnAgentConfig(ctx, errOut, agentName, cfg) + return nil +} + +// warnAgentConfig prints AgentConfigWarnings for cfg against this checkout. +func warnAgentConfig(ctx context.Context, errOut io.Writer, worker string, cfg *settings.ReviewAgentConfig) { var roots []string if root, err := paths.WorktreeRoot(ctx); err == nil { roots = append(roots, root) @@ -56,7 +66,9 @@ func validateAgentConfigForSave(ctx context.Context, agentName string, cfg *sett roots = append(roots, resolved) } } - return ValidateAgentConfig(agentName, toAgentConfig(cfg), roots) + for _, w := range AgentConfigWarnings(toAgentConfig(cfg), roots) { + fmt.Fprintf(errOut, "Warning: %s config: %s.\n", worker, w) + } } // stripAgentConfigs removes reviewer configs from a profile before it is @@ -210,7 +222,7 @@ func configureAgentConfigs(ctx context.Context, cmd *cobra.Command, profileName if err != nil { return fail(err) } - if err := validateAgentConfigForSave(ctx, reviewAgentName(worker, workerCfg), cfg); err != nil { + if err := validateAgentConfigForSave(ctx, cmd.ErrOrStderr(), reviewAgentName(worker, workerCfg), cfg); err != nil { return fail(fmt.Errorf("--set-config %s: %w", worker, err)) } configs[worker] = cfg diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 309367ed25..8567af41d5 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -15,10 +15,6 @@ import ( 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)} } @@ -28,57 +24,23 @@ func TestValidateAgentConfig(t *testing.T) { 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"}, - {"statusLine into checkout", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"statusLine":{"type":"command","command":"/repo/bin/status"}}`)}, "inside a checkout"}, - {"fileSuggestion relative", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"fileSuggestion":{"type":"command","command":"./suggest.sh"}}`)}, "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"]}`)}, ""}, + // Commands are the user's own and are not vetted, launchers included. + {"claude hook with a launcher", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"yarn lint"}]}]}}`)}, ""}, + {"MCP command", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"npx","args":["server"]}`)}, ""}, {"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 without command or url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"args":["x"]}`)}, "needs a command or a url"}, {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, + {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, {"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"}, + {"pi extension", "pi", reviewtypes.AgentConfig{Extensions: []string{"/opt/ext.ts"}}, ""}, {"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) + err := ValidateAgentConfig(tt.agent, &cfg) if tt.wantErr == "" { if err != nil { t.Fatalf("ValidateAgentConfig() = %v, want nil", err) @@ -92,8 +54,37 @@ func TestValidateAgentConfig(t *testing.T) { } } +// Values naming a path inside the checkout are warned about, not refused. +func TestAgentConfigWarnings(t *testing.T) { + t.Parallel() + + roots := []string{"/repo"} + tests := []struct { + name string + cfg reviewtypes.AgentConfig + want int + }{ + {"hook in checkout", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"command":"/repo/bin/hook --x"}]}]}}`)}, 1}, + {"project dir variable", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"statusLine":{"command":"$CLAUDE_PROJECT_DIR/s.sh"}}`)}, 1}, + {"MCP env PATH into checkout", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"x": json.RawMessage(`{"command":"tool","env":{"PATH":"/usr/bin:/repo/bin"}}`)}}, 1}, + {"flag value in checkout", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"apiKeyHelper":"/opt/key --config=/repo/k.json"}`)}, 1}, + {"extension in checkout", reviewtypes.AgentConfig{Extensions: []string{"/repo/.pi/x.ts"}}, 1}, + {"similar prefix", reviewtypes.AgentConfig{Extensions: []string{"/repository/x.ts"}}, 0}, + {"outside", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"command":"yarn lint"}]}]}}`)}, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + cfg := tt.cfg + if got := AgentConfigWarnings(&cfg, roots); len(got) != tt.want { + t.Fatalf("AgentConfigWarnings() = %q, want %d", got, tt.want) + } + }) + } +} + // A path outside the checkout that links into it is inside it. -func TestValidateAgentConfigFollowsSymlinks(t *testing.T) { +func TestAgentConfigWarningsFollowSymlinks(t *testing.T) { t.Parallel() checkout := t.TempDir() @@ -106,9 +97,8 @@ func TestValidateAgentConfigFollowsSymlinks(t *testing.T) { 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) + if got := AgentConfigWarnings(&reviewtypes.AgentConfig{Extensions: []string{link}}, []string{resolved}); len(got) != 1 { + t.Fatalf("AgentConfigWarnings() = %q, want one warning", got) } } @@ -148,15 +138,7 @@ func TestTrustConfirmTextForProfileConfig(t *testing.T) { 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") { + if title, body := trustConfirmText(subject, TrustInventory{Isolated: true}, "x"); title != "Review this branch?" || !strings.Contains(body, "uses your profile's config") || strings.Contains(body, "nothing from it runs") { 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 658b8d84d0..0806397b8e 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -1019,7 +1019,8 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver } 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) + fmt.Fprintf(cmd.ErrOrStderr(), "%s: using your profile's agent config; this checkout's hooks, MCP servers and extensions are not loaded.\n", worker) + warnAgentConfig(ctx, cmd.ErrOrStderr(), worker, profile.Agents[worker].Config) } } diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index 9cd61e4281..06240cfdb4 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -893,7 +893,7 @@ 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) + agentCfg, changed, err := promptReviewAgentConfig(ctx, out, string(c.name), existing[string(c.name)].Config) if err != nil { return err } @@ -1368,7 +1368,7 @@ func dedupeStrings(xs []string) []string { // 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) { +func promptReviewAgentConfig(ctx context.Context, out io.Writer, agentName string, current *settings.ReviewAgentConfig) (*settings.ReviewAgentConfig, bool, error) { if _, supported := agentConfigFields[agentName]; !supported { return current, false, nil } @@ -1410,7 +1410,7 @@ func promptReviewAgentConfig(ctx context.Context, agentName string, current *set if err != nil { return nil, false, err } - if err := validateAgentConfigForSave(ctx, agentName, cfg); err != nil { + if err := validateAgentConfigForSave(ctx, out, 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/trust.go b/cmd/entire/cli/review/trust.go index a1901c7710..d41459f7eb 100644 --- a/cmd/entire/cli/review/trust.go +++ b/cmd/entire/cli/review/trust.go @@ -433,7 +433,7 @@ func trustConfirmText(subject TrustSubject, inv TrustInventory, command 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.") + b.WriteString("The review agent uses your profile's config, so this branch's hooks, MCP servers and extensions are not loaded. It reads this branch's code and instructions. Your own hooks and MCP servers run in this checkout as you configured them.") return title, b.String() case len(entries) == 0: title = "Review this branch?" @@ -441,7 +441,7 @@ func trustConfirmText(subject TrustSubject, inv TrustInventory, command string) 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") + b.WriteString("The review agent uses your profile's config, so this branch's hooks, MCP servers and extensions are not loaded. It 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") @@ -606,7 +606,10 @@ func printTrustConfig(out io.Writer, subject TrustSubject, inv TrustInventory, a if agents := inv.entireAgents(); len(agents) > 0 { fmt.Fprintf(out, "Entire session tracking: %s\n", strings.Join(agents, ", ")) } - if len(entries) == 0 { + switch { + case len(entries) == 0 && len(inv.IsolatedAgents) > 0: + fmt.Fprintln(out, "Nothing from this checkout is loaded; your profile's own hooks and MCP servers run in it as you configured them.") + case len(entries) == 0: fmt.Fprintln(out, "Nothing from this checkout runs during the review.") } for _, e := range entries { diff --git a/cmd/entire/cli/review/trust_cmd_test.go b/cmd/entire/cli/review/trust_cmd_test.go index 90f5447d73..f5b10723c0 100644 --- a/cmd/entire/cli/review/trust_cmd_test.go +++ b/cmd/entire/cli/review/trust_cmd_test.go @@ -3,6 +3,7 @@ package review_test import ( "bytes" "context" + "encoding/json" "os" "os/exec" "path/filepath" @@ -195,11 +196,28 @@ func TestConfigure_SetConfigSavesToClonePreferences(t *testing.T) { 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 { + // A command naming a file in this checkout is saved with a warning. + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + inCheckout := filepath.Join(t.TempDir(), "in-checkout.json") + hook, err := json.Marshal(filepath.Join(wd, "hook.sh")) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(inCheckout, []byte(`{"settings":{"hooks":{"Stop":[{"hooks":[{"type":"command","command":`+string(hook)+`}]}]}}}`), 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") + var stderr bytes.Buffer + cmd := review.NewCommand(deps) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&stderr) + cmd.SetArgs([]string{"--configure", "general", "--set-config", "claude-code=" + inCheckout}) + if err := cmd.Execute(); err != nil { + t.Fatalf("--set-config with a checkout path: %v", err) + } + if !strings.Contains(stderr.String(), "inside the checkout") { + t.Errorf("no warning for a checkout path; stderr:\n%s", stderr.String()) } } diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 05f561ee8a..12b5029944 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -177,17 +177,15 @@ Rules: 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. +- **Where config comes from is enforced; what commands do is not.** The + branch's hooks, MCP servers and extensions are not loaded. The profile's own + hooks, MCP servers and helpers are the user's code, like the hooks in their + agent settings: they run in the reviewed checkout as configured, and Entire + doesn't vet them (a command at an absolute path can still run the branch's + `package.json` scripts). A value naming a path inside the checkout or + `$CLAUDE_PROJECT_DIR` gets a warning at save time and before each run, since + it is usually a copy-paste mistake. Prompts never say nothing runs when the + profile configures hooks or MCP servers. - **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 From 0179c012d7b1ecf761bfaecfc59d6201ff8ca1f0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 12:15:41 -0700 Subject: [PATCH 11/22] Stop only for reviewers that run, and refuse Codex MCP env at save time - An unverified local reviewer config stops the review only when it belongs to a reviewer of this exact profile that will run (--agent included). - The message says how to fix it: untrack .entire/settings.local.json or remove the profile from it, then set the config again. - Codex MCP servers with env values are refused when the config is saved, not only on every run. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VB29WKPY2J8JSCPNA72DFJ --- cmd/entire/cli/review/agent_config.go | 8 +++-- cmd/entire/cli/review/agent_config_test.go | 21 +++++++++++++ cmd/entire/cli/review/cmd.go | 36 +++++++++++++++++----- cmd/entire/cli/review/target.go | 2 +- 4 files changed, 57 insertions(+), 10 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 223402356a..296229b88f 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -67,8 +67,9 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return fmt.Errorf("MCP server name %q: use letters, digits, '-' and '_'", name) } var server struct { - Command string `json:"command"` - URL string `json:"url"` + Command string `json:"command"` + URL string `json:"url"` + Env map[string]string `json:"env"` } if err := json.Unmarshal(cfg.MCPServers[name], &server); err != nil { return fmt.Errorf("MCP server %q: %w", name, err) @@ -76,6 +77,9 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { if server.Command == "" && server.URL == "" { return fmt.Errorf("MCP server %q needs a command or a url", name) } + if agentName == "codex" && len(server.Env) > 0 { + return fmt.Errorf("MCP server %q: env values aren't supported for Codex in a review profile yet", name) + } } return nil } diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 8567af41d5..7fb68f079f 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -29,6 +29,8 @@ func TestValidateAgentConfig(t *testing.T) { {"MCP command", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"npx","args":["server"]}`)}, ""}, {"MCP url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example"}`)}, ""}, {"MCP without command or url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"args":["x"]}`)}, "needs a command or a url"}, + {"codex MCP env", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, "env values aren't supported for Codex"}, + {"claude MCP env", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, ""}, {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, {"codex settings unsupported", "codex", reviewtypes.AgentConfig{Settings: json.RawMessage(`{}`)}, `does not support "settings"`}, @@ -142,3 +144,22 @@ func TestTrustConfirmTextForProfileConfig(t *testing.T) { t.Fatalf("empty isolated title/body = %q / %q", title, body) } } + +// Only a reviewer that will run, in exactly this profile, stops the review. +func TestUnverifiedReviewerConfig(t *testing.T) { + t.Parallel() + + profile := settings.ReviewProfileConfig{Agents: map[string]settings.ReviewConfig{ + "claude-code": {Agent: "claude-code"}, "codex": {Agent: "codex"}, + }} + unverified := []string{"review_profiles.a.b.agents.claude-code.config", "review_profiles.a.agents.claude-code.config"} + if field, ok := unverifiedReviewerConfig(unverified, "a", profile, ""); !ok || field != unverified[1] { + t.Errorf("all reviewers: got %q, %v", field, ok) + } + if _, ok := unverifiedReviewerConfig(unverified, "a", profile, "codex"); ok { + t.Error("--agent codex was stopped by claude-code's config") + } + if _, ok := unverifiedReviewerConfig(unverified[:1], "a", profile, ""); ok { + t.Error("profile a was stopped by profile a.b's config") + } +} diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 0806397b8e..e04dbae916 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -853,7 +853,7 @@ type reviewProfileSelection struct { } // resolveReviewProfile selects the profile, running setup or the chooser if needed. -func resolveReviewProfile(ctx context.Context, cmd *cobra.Command, profileOverride string, deps Deps) (reviewProfileSelection, error) { +func resolveReviewProfile(ctx context.Context, cmd *cobra.Command, profileOverride, agentOverride string, deps Deps) (reviewProfileSelection, error) { out := cmd.OutOrStdout() silentErr := deps.NewSilentError @@ -980,19 +980,41 @@ func resolveReviewProfile(ctx context.Context, cmd *cobra.Command, profileOverri // 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. + var unverified []string 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) + if settings.AgentConfigRejectionUnverified(rej) { + unverified = append(unverified, rej.Field) } } + if field, ok := unverifiedReviewerConfig(unverified, profileName, profile, agentOverride); ok { + cmd.SilenceUsage = true + err := fmt.Errorf("%s can't be applied: .entire/settings.local.json could not be verified as untracked; untrack it (git rm --cached .entire/settings.local.json) or remove the profile from it, then set the config again with entire review --configure %s --set-config", field, profileName) + 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 } +// unverifiedReviewerConfig returns the first of the unverified (dropped) +// config fields that belongs to a reviewer of profileName that will run. +func unverifiedReviewerConfig(unverified []string, profileName string, profile settings.ReviewProfileConfig, agentOverride string) (string, bool) { + workers := sortedStringKeys(profile.Agents) + if agentOverride != "" { + if worker, _, err := selectProfileWorker(profile, agentOverride); err == nil { + workers = []string{worker} + } + } + for _, worker := range workers { + field := "review_profiles." + profileName + ".agents." + worker + ".config" + if slices.Contains(unverified, field) { + return field, true + } + } + return "", false +} + // runReview executes the main review flow. func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOverride, baseOverride, profileOverride, perRunPrompt string, timeout time.Duration, gateOpts reviewGateOptions, deps Deps) error { out := cmd.OutOrStdout() @@ -1004,7 +1026,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver if gateOpts.ShowConfig { return runReviewShowConfig(ctx, cmd, profileOverride, gateOpts, deps) } - selection, err := resolveReviewProfile(ctx, cmd, profileOverride, deps) + selection, err := resolveReviewProfile(ctx, cmd, profileOverride, agentOverride, deps) if err != nil || selection.done { return err } diff --git a/cmd/entire/cli/review/target.go b/cmd/entire/cli/review/target.go index 7f2ed2aeae..60d4d224ce 100644 --- a/cmd/entire/cli/review/target.go +++ b/cmd/entire/cli/review/target.go @@ -81,7 +81,7 @@ func runTargetReview(ctx context.Context, cmd *cobra.Command, req targetReviewRe } else { // Resolve the profile in the user's checkout, never the branch's worktree; // the gate also needs its agents. - selection, selErr := resolveReviewProfile(ctx, cmd, profileName, deps) + selection, selErr := resolveReviewProfile(ctx, cmd, profileName, req.Gate.AgentOverride, deps) if selErr != nil || selection.done { return selErr } From 355911234cb43010dea75b294e888f9be7056203 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 12:23:10 -0700 Subject: [PATCH 12/22] Mention Pi's settings only when a Pi reviewer runs isolated Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VBG05WE4XZ4GVRFCCRKHTM --- cmd/entire/cli/review/agent_config_test.go | 6 +++++- cmd/entire/cli/review/trust.go | 18 ++++++++++++++---- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 7fb68f079f..6dedff4ecb 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -137,9 +137,13 @@ func TestTrustConfirmTextForProfileConfig(t *testing.T) { {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") { + if title != "Load this branch's skills during the review?" || !strings.Contains(body, "uses your profile's config") || strings.Contains(body, "Pi") { t.Fatalf("title/body = %q / %q", title, body) } + inv.IsolatedAgents = []string{"claude-code", "pi"} + if _, body := trustConfirmText(subject, inv, "x"); !strings.Contains(body, "Pi settings") { + t.Errorf("isolated Pi reviewer, body doesn't mention Pi settings: %q", body) + } if title, body := trustConfirmText(subject, TrustInventory{Isolated: true}, "x"); title != "Review this branch?" || !strings.Contains(body, "uses your profile's config") || strings.Contains(body, "nothing from it runs") { t.Fatalf("empty isolated title/body = %q / %q", title, body) } diff --git a/cmd/entire/cli/review/trust.go b/cmd/entire/cli/review/trust.go index d41459f7eb..b0c044bc4f 100644 --- a/cmd/entire/cli/review/trust.go +++ b/cmd/entire/cli/review/trust.go @@ -385,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.Inventory.Isolated, g.Command, g.Subject.HeadSHA) + printTrustRefusal(errOut, what, g.Inventory, g.Command, g.Subject.HeadSHA) return errTrustRefused } title, description := trustConfirmText(g.Subject, g.Inventory, g.Command) @@ -400,11 +400,12 @@ func (g trustGate) run(ctx context.Context, errOut io.Writer) error { return nil } -func printTrustRefusal(errOut io.Writer, what string, isolated bool, command, head string) { +func printTrustRefusal(errOut io.Writer, what string, inv TrustInventory, command, head string) { + isolated := inv.Isolated fmt.Fprintln(errOut, "Not run: this review needs the user's approval.") 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) + 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%s (%s --show-config lists them).\n", inv.piSettingsNote(", and Pi's settings"), command) case what == trustWhatNothing: fmt.Fprintf(errOut, "The code is by someone else (%s --show-config shows what the review reads).\n", command) default: @@ -441,7 +442,7 @@ func trustConfirmText(subject TrustSubject, inv TrustInventory, command string) 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, so this branch's hooks, MCP servers and extensions are not loaded. It still loads this branch's skills and commands (and Pi settings), which can run commands:\n") + b.WriteString("The review agent uses your profile's config, so this branch's hooks, MCP servers and extensions are not loaded. It still loads this branch's skills and commands" + inv.piSettingsNote(" (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") @@ -629,3 +630,12 @@ func printTrustConfig(out io.Writer, subject TrustSubject, inv TrustInventory, a } return nil } + +// piSettingsNote returns note when an isolated Pi reviewer runs: Pi still +// loads the branch's .pi/settings.json, the other agents load no settings. +func (inv TrustInventory) piSettingsNote(note string) string { + if slices.Contains(inv.IsolatedAgents, "pi") { + return note + } + return "" +} From 426f23c6f6520c5ebeca6e172b99572c56597e08 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 12:41:52 -0700 Subject: [PATCH 13/22] Require absolute Pi extensions; announce only reviewers that run A relative extension would load from the reviewed checkout. The per-run config line and warnings now respect --agent. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VCJ81MAPPYV8NVZY3DHHGP --- cmd/entire/cli/review/agent_config.go | 7 +++++++ cmd/entire/cli/review/agent_config_test.go | 1 + cmd/entire/cli/review/cmd.go | 12 +++++++++--- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 296229b88f..ca01b7ee19 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -81,6 +81,13 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return fmt.Errorf("MCP server %q: env values aren't supported for Codex in a review profile yet", name) } } + // A relative extension would load from the reviewed checkout, the one + // thing a profile config promises not to do. + for _, ext := range cfg.Extensions { + if !filepath.IsAbs(ext) { + return fmt.Errorf("extension %q: use an absolute path", ext) + } + } return nil } diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 6dedff4ecb..4bc6a737fe 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -35,6 +35,7 @@ func TestValidateAgentConfig(t *testing.T) { {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, {"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{".pi/ext.ts"}}, "absolute path"}, {"pi extension", "pi", reviewtypes.AgentConfig{Extensions: []string{"/opt/ext.ts"}}, ""}, {"unknown agent", "cursor", reviewtypes.AgentConfig{}, "does not support a review profile config"}, } diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index e04dbae916..579c811856 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -1039,10 +1039,16 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver } return err } - for _, worker := range sortedMapKeys(nonZeroAgentConfigs(profile.Agents)) { - if profile.Agents[worker].Config != nil { + running := nonZeroAgentConfigs(profile.Agents) + if agentOverride != "" { + if worker, cfg, err := selectProfileWorker(profile, agentOverride); err == nil { + running = map[string]settings.ReviewConfig{worker: cfg} + } + } + for _, worker := range sortedMapKeys(running) { + if running[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) - warnAgentConfig(ctx, cmd.ErrOrStderr(), worker, profile.Agents[worker].Config) + warnAgentConfig(ctx, cmd.ErrOrStderr(), worker, running[worker].Config) } } From 5088d58e52b644b9ee06b1604306d10878660d2b Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 14:08:43 -0700 Subject: [PATCH 14/22] Report blocked reviewer tools, and track Codex reviews with a profile config - Claude's permission_denials and Codex's "requires approval" MCP failures become a ToolsBlocked event; after the run Entire names the blocked tools and where to allow them. - Codex: the untrusted checkout's hooks.json, Entire's own included, isn't loaded, so Entire's hooks are passed as session-flag -c overrides with --dangerously-bypass-hook-trust. The checkout's hooks still don't run. - Codex MCP servers can set default_tools_approval_mode, since exec mode can't ask for approval. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VHH8792A685HP9GNJEPWN4 --- cmd/entire/cli/agent/claudecode/reviewer.go | 23 +++++++++++++ .../cli/agent/claudecode/reviewer_test.go | 16 +++++++++ cmd/entire/cli/agent/codex/review_config.go | 33 ++++++++++++++++--- .../cli/agent/codex/review_config_test.go | 23 +++++++++++++ cmd/entire/cli/agent/codex/reviewer.go | 21 ++++++++++-- cmd/entire/cli/agent/codex/reviewer_test.go | 16 +++++++++ cmd/entire/cli/review/agent_config_test.go | 14 ++++++++ cmd/entire/cli/review/cmd.go | 26 +++++++++++++++ cmd/entire/cli/review/types/reviewer.go | 11 +++++++ docs/architecture/review-command.md | 11 ++++++- 10 files changed, 186 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/agent/claudecode/reviewer.go b/cmd/entire/cli/agent/claudecode/reviewer.go index 857810fb39..79b0fa5bcf 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer.go +++ b/cmd/entire/cli/agent/claudecode/reviewer.go @@ -8,6 +8,7 @@ import ( "io" "os" "os/exec" + "slices" "github.com/entireio/cli/cmd/entire/cli/review" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -155,6 +156,9 @@ func parseClaudeOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event { sawResult = true resultErr = env.IsError resultUsage = env.Usage + if names := deniedToolNames(env.PermissionDenials); len(names) > 0 { + out <- reviewtypes.ToolsBlocked{Names: names, Allow: claudeAllowHint} + } } } if err := scanner.Err(); err != nil { @@ -191,6 +195,25 @@ type claudeEnvelope struct { // Claude API usage shape (transcript parsing + stream-json review parser) // can't drift apart. Usage messageUsage `json:"usage"` + // PermissionDenials lists tool calls the permission settings refused. + PermissionDenials []permissionDenial `json:"permission_denials"` +} + +type permissionDenial struct { + ToolName string `json:"tool_name"` +} + +const claudeAllowHint = "add them to permissions.allow in the reviewer's settings (its review profile config, or ~/.claude/settings.json)" + +// deniedToolNames returns the distinct denied tool names, in order. +func deniedToolNames(denials []permissionDenial) []string { + var names []string + for _, d := range denials { + if d.ToolName != "" && !slices.Contains(names, d.ToolName) { + names = append(names, d.ToolName) + } + } + return names } type claudeMessage struct { diff --git a/cmd/entire/cli/agent/claudecode/reviewer_test.go b/cmd/entire/cli/agent/claudecode/reviewer_test.go index fb1a2f02d7..12012bbe99 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer_test.go +++ b/cmd/entire/cli/agent/claudecode/reviewer_test.go @@ -526,3 +526,19 @@ func TestParseClaudeOutput_IgnoresNonObjectMessageOnOtherEvents(t *testing.T) { t.Fatalf("Finished = %+v, want success", finished) } } + +// Tool calls the permission settings denied are reported once per tool. +func TestParseClaudeOutput_ReportsPermissionDenials(t *testing.T) { + t.Parallel() + stream := `{"type":"result","subtype":"success","is_error":false,"permission_denials":[` + + `{"tool_name":"mcp__docs__search","tool_use_id":"a"},{"tool_name":"mcp__docs__search","tool_use_id":"b"},{"tool_name":"WebFetch","tool_use_id":"c"}]}` + "\n" + var blocked []string + for _, ev := range collectEvents(parseClaudeOutput(strings.NewReader(stream))) { + if e, ok := ev.(reviewtypes.ToolsBlocked); ok { + blocked = append(blocked, e.Names...) + } + } + if strings.Join(blocked, ",") != "mcp__docs__search,WebFetch" { + t.Fatalf("blocked = %v", blocked) + } +} diff --git a/cmd/entire/cli/agent/codex/review_config.go b/cmd/entire/cli/agent/codex/review_config.go index 5a23e33857..919c93e12c 100644 --- a/cmd/entire/cli/agent/codex/review_config.go +++ b/cmd/entire/cli/agent/codex/review_config.go @@ -7,6 +7,7 @@ import ( "slices" "strings" + "github.com/entireio/cli/cmd/entire/cli/agent" "github.com/entireio/cli/cmd/entire/cli/review" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) @@ -42,6 +43,12 @@ func prepareCodexReviewConfig(ctx context.Context, cfg reviewtypes.RunConfig) (r return cfg, nil, err } cfg.ExtraArgs = append(cfg.ExtraArgs, servers...) + // The untrusted checkout's hooks.json, Entire's own hooks included, isn't + // loaded, so Entire's hooks are passed as session flags. Codex runs + // session-flag hooks only with persisted trust or this bypass; it does + // not load the checkout's hooks, which stay untrusted. + cfg.ExtraArgs = append(cfg.ExtraArgs, entireHookOverrides(agent.UseWindowsProductionHooks(ctx))...) + cfg.ExtraArgs = append(cfg.ExtraArgs, "--dangerously-bypass-hook-trust") cfg.WorkDir = run.CheckoutRoot return cfg, nil, nil } @@ -73,10 +80,11 @@ func codexMCPOverrides(servers map[string]json.RawMessage) ([]string, error) { 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"` + Command string `json:"command"` + Args []string `json:"args"` + URL string `json:"url"` + Env map[string]string `json:"env"` + ApprovalMode string `json:"default_tools_approval_mode"` } if err := json.Unmarshal(servers[name], &server); err != nil { return nil, fmt.Errorf("MCP server %q: %w", name, err) @@ -101,9 +109,26 @@ func codexMCPOverrides(servers map[string]json.RawMessage) ([]string, error) { } else { err = set("url", server.URL) } + if err == nil && server.ApprovalMode != "" { + err = set("default_tools_approval_mode", server.ApprovalMode) + } if err != nil { return nil, err } } return args, nil } + +// entireHookOverrides returns -c overrides that register Entire's managed +// Codex hooks, the same commands `entire enable` writes to hooks.json. +func entireHookOverrides(windows bool) []string { + var args []string + for _, h := range managedHooks { + command, err := json.Marshal(h.wrap("entire hooks codex "+h.verb, windows)) + if err != nil { + continue + } + args = append(args, "-c", fmt.Sprintf(`hooks.%s=[{hooks=[{type="command",command=%s,timeout=%d}]}]`, h.event, command, h.timeout)) + } + return args +} diff --git a/cmd/entire/cli/agent/codex/review_config_test.go b/cmd/entire/cli/agent/codex/review_config_test.go index 43dc43a0ce..de96f7998a 100644 --- a/cmd/entire/cli/agent/codex/review_config_test.go +++ b/cmd/entire/cli/agent/codex/review_config_test.go @@ -69,3 +69,26 @@ func TestPrepareCodexReviewConfigRefusesUnsupported(t *testing.T) { } } } + +// Entire's hooks are passed as session flags, since the untrusted checkout's +// hooks.json isn't loaded, and run without persisted trust. +func TestEntireHookOverrides(t *testing.T) { + t.Parallel() + args := strings.Join(entireHookOverrides(false), "\n") + for _, want := range []string{`hooks.SessionStart=[{hooks=[{type="command",command=`, "entire hooks codex session-start", "hooks.Stop="} { + if !strings.Contains(args, want) { + t.Errorf("overrides missing %q:\n%s", want, args) + } + } +} + +func TestCodexMCPOverridesPassApprovalMode(t *testing.T) { + t.Parallel() + args, err := codexMCPOverrides(map[string]json.RawMessage{"docs": json.RawMessage(`{"command":"/opt/docs","default_tools_approval_mode":"approve"}`)}) + if err != nil { + t.Fatal(err) + } + if !slices.Contains(args, `mcp_servers.docs.default_tools_approval_mode="approve"`) { + t.Fatalf("args = %v", args) + } +} diff --git a/cmd/entire/cli/agent/codex/reviewer.go b/cmd/entire/cli/agent/codex/reviewer.go index 0b21e83811..22e37c13c0 100644 --- a/cmd/entire/cli/agent/codex/reviewer.go +++ b/cmd/entire/cli/agent/codex/reviewer.go @@ -207,6 +207,9 @@ func parseCodexOutputBuf(r io.Reader, maxBuf int) <-chan reviewtypes.Event { if env.Item.Type == itemTypeAgentMessage && env.Item.Text != "" { out <- reviewtypes.AssistantText{Text: env.Item.Text} } + if blockedForApproval(env.Item) { + out <- reviewtypes.ToolsBlocked{Names: []string{env.Item.Server + "." + env.Item.Tool}, Allow: codexAllowHint} + } // command_execution completion is intentionally swallowed — // item.started already announced it. aggregated_output is the // tool's stdout, not the model's narrative. @@ -313,9 +316,21 @@ func firstNonEmptyString(values ...string) string { } type codexItem struct { - Type string `json:"type"` - Command string `json:"command"` - Text string `json:"text"` + Type string `json:"type"` + Command string `json:"command"` + Text string `json:"text"` + Server string `json:"server"` + Tool string `json:"tool"` + Status string `json:"status"` + Error codexErrorField `json:"error"` +} + +const codexAllowHint = `set default_tools_approval_mode = "approve" on the MCP server (in its review profile config, or ~/.codex/config.toml)` + +// blockedForApproval reports an MCP tool call Codex refused because exec mode +// can't ask for approval. +func blockedForApproval(item codexItem) bool { + return item.Type == "mcp_tool_call" && item.Status == "failed" && strings.Contains(item.Error.Message, "requires approval") } type codexUsage struct { diff --git a/cmd/entire/cli/agent/codex/reviewer_test.go b/cmd/entire/cli/agent/codex/reviewer_test.go index f703dd3cfb..e7194fddee 100644 --- a/cmd/entire/cli/agent/codex/reviewer_test.go +++ b/cmd/entire/cli/agent/codex/reviewer_test.go @@ -566,3 +566,19 @@ func TestBuildCodexReviewCmd_PromptOverrideVerbatim(t *testing.T) { t.Errorf("PromptOverride modified: %q", got) } } + +// An MCP call exec mode refused for approval is reported as blocked. +func TestParseCodexOutput_ReportsToolBlockedForApproval(t *testing.T) { + t.Parallel() + stream := `{"type":"item.completed","item":{"type":"mcp_tool_call","server":"docs","tool":"search","status":"failed","error":{"message":"MCP tool call requires approval, but approval policy is never"}}}` + "\n" + + `{"type":"turn.completed","usage":{"input_tokens":1,"output_tokens":1}}` + "\n" + var blocked []string + for ev := range parseCodexOutput(strings.NewReader(stream)) { + if e, ok := ev.(reviewtypes.ToolsBlocked); ok { + blocked = append(blocked, e.Names...) + } + } + if strings.Join(blocked, ",") != "docs.search" { + t.Fatalf("blocked = %v", blocked) + } +} diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 4bc6a737fe..6aabfb08fd 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -168,3 +168,17 @@ func TestUnverifiedReviewerConfig(t *testing.T) { t.Error("profile a was stopped by profile a.b's config") } } + +func TestPrintBlockedTools(t *testing.T) { + t.Parallel() + + var b strings.Builder + printBlockedTools(&b, "general", reviewtypes.RunSummary{AgentRuns: []reviewtypes.AgentRun{ + {Name: "claude-code", Buffer: []reviewtypes.Event{reviewtypes.ToolsBlocked{Names: []string{"mcp__docs__search"}}}}, + {Name: "codex"}, + }}) + out := b.String() + if !strings.Contains(out, "claude-code: its permission settings blocked mcp__docs__search") || !strings.Contains(out, "--configure general --set-config") || strings.Contains(out, "codex") { + t.Fatalf("output:\n%s", out) + } +} diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 579c811856..03568c74a6 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -997,6 +997,30 @@ func resolveReviewProfile(ctx context.Context, cmd *cobra.Command, profileOverri return reviewProfileSelection{name: profileName, profile: profile, installed: installed}, nil } +// printBlockedTools reports tools a reviewer tried to call but its permission +// settings denied, and where to allow them. The review itself completed. +func printBlockedTools(w io.Writer, profileName string, summary reviewtypes.RunSummary) { + for _, run := range summary.AgentRuns { + var names []string + allow := "" + for _, ev := range run.Buffer { + if blocked, ok := ev.(reviewtypes.ToolsBlocked); ok { + for _, name := range blocked.Names { + if !slices.Contains(names, name) { + names = append(names, name) + } + } + allow = blocked.Allow + } + } + if len(names) == 0 { + continue + } + fmt.Fprintf(w, "%s: its permission settings blocked %s.\n", run.Name, strings.Join(names, ", ")) + fmt.Fprintf(w, " To allow them, %s; update the profile config with entire review --configure %s --set-config =.\n", allow, profileName) + } +} + // unverifiedReviewerConfig returns the first of the unverified (dropped) // config fields that belongs to a reviewer of profileName that will run. func unverifiedReviewerConfig(unverified []string, profileName string, profile settings.ReviewProfileConfig, agentOverride string) (string, bool) { @@ -1280,6 +1304,7 @@ func runSingleAgentPath( } summary, waitErr := Run(runCtx, reviewer, runCfg, sinks) + printBlockedTools(cmd.ErrOrStderr(), profileName, summary) writePostReviewManifest(ctx, out, worktreeRoot, headSHA, summary, "") maybePostReviewToTrail(ctx, out, deps, outputMode, profileName, summary, "") if waitErr != nil && runCtx.Err() == nil && ctx.Err() == nil { @@ -1465,6 +1490,7 @@ func runMultiAgentPath( runMultiCfg.EnrichAgentRun = reviewAgentRunTokenEnricherForRuns(worktreeRoot, headSHA, plannedAgentRunsForReviewers(reviewers, runMultiCfg)) runMultiCfg.EnrichSummary = reviewSummaryTokenEnricher(worktreeRoot, headSHA) summary, waitErr := RunMulti(runCtx, reviewers, runMultiCfg, sinks) + printBlockedTools(cmd.ErrOrStderr(), profileName, summary) if shouldAbortMultiReview(summary, waitErr) && runCtx.Err() == nil && ctx.Err() == nil { // Operational failure, not a usage error — don't dump the command help. cmd.SilenceUsage = true diff --git a/cmd/entire/cli/review/types/reviewer.go b/cmd/entire/cli/review/types/reviewer.go index 2eb87441ca..073d51741f 100644 --- a/cmd/entire/cli/review/types/reviewer.go +++ b/cmd/entire/cli/review/types/reviewer.go @@ -249,6 +249,17 @@ type RunError struct { func (RunError) isEvent() {} +// ToolsBlocked reports tools the agent tried to call but its permission +// settings denied. The review still completes; the run reports them so the +// user can allow them. +type ToolsBlocked struct { + Names []string + // Allow says how to allow them in this agent's settings. + Allow string +} + +func (ToolsBlocked) 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 { diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 12b5029944..fb7d938641 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -164,7 +164,7 @@ own config with nothing extra, or load a JSON file) or | 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 | +| Codex | checkout and main repo marked untrusted for the run (project config, hooks, rules) | `mcp_servers` via `-c` (no literal `env` values yet; `default_tools_approval_mode` passes through); Entire's tracking hooks via `-c` with `--dangerously-bypass-hook-trust`, which also runs the user's own untrusted hooks | AGENTS.md; skills | | Pi | `--no-extensions` | `extensions` via `--extension`, plus Entire's own | AGENTS.md/CLAUDE.md, skills, prompt templates, `.pi/settings.json` | Rules: @@ -186,6 +186,15 @@ Rules: `$CLAUDE_PROJECT_DIR` gets a warning at save time and before each run, since it is usually a copy-paste mistake. Prompts never say nothing runs when the profile configures hooks or MCP servers. +- **Permissions come from the loaded settings.** Reviewers run headless, so a + tool call not allowed by the settings the reviewer loads is denied. With a + profile config those are the user's own settings plus the profile's + `settings`; the project's `.claude/settings.json` allows no longer apply. + Allow a profile MCP tool in the profile's `settings.permissions.allow` + (e.g. `mcp__docs__search`) for Claude, or with + `"default_tools_approval_mode": "approve"` on the server for Codex, whose + exec mode can't ask. Both report blocked calls, and the run prints them + after the reviewer finishes with where to allow them. - **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 From a8404e4a25635076723a7b82f72f0961a19113b0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 14:27:15 -0700 Subject: [PATCH 15/22] Keep clone-local reviewer configs when the local file defines the profile The local file replaces a profile it defines, which dropped the config saved in clone-local preferences and silently ran the checkout's config. The clone-local configs now apply after the local file, unless it sets its own. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VJK6AGTKN3GRCP68YRP1KM --- .../cli/settings/agent_config_trust_test.go | 17 ++++++++++++++ cmd/entire/cli/settings/agent_prompt_trust.go | 2 +- cmd/entire/cli/settings/settings.go | 23 +++++++++++++++---- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/settings/agent_config_trust_test.go b/cmd/entire/cli/settings/agent_config_trust_test.go index c76afc3887..1cd9224b7c 100644 --- a/cmd/entire/cli/settings/agent_config_trust_test.go +++ b/cmd/entire/cli/settings/agent_config_trust_test.go @@ -76,3 +76,20 @@ func TestAgentConfigTrust_JudgeAndLegacyConfigAreRejected(t *testing.T) { assert.True(t, fields["review_profiles.general.judge.config"]) assert.True(t, fields["review.pi.config"]) } + +// A local file that defines the profile replaces it whole; the clone-local +// config still applies on top unless the local file sets its own. +func TestAgentConfigTrust_ClonePreferencesConfigSurvivesLocalProfile(t *testing.T) { + t.Parallel() + _, project, local := newOPFRepo(t) + writeSettingsFile(t, project, `{"enabled":true}`) + writeSettingsFile(t, local, `{"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()) +} diff --git a/cmd/entire/cli/settings/agent_prompt_trust.go b/cmd/entire/cli/settings/agent_prompt_trust.go index f10af2f8a6..39bb4983c3 100644 --- a/cmd/entire/cli/settings/agent_prompt_trust.go +++ b/cmd/entire/cli/settings/agent_prompt_trust.go @@ -220,7 +220,7 @@ func enforceAgentPromptTrust(ctx context.Context, s *EntireSettings, localSettin 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)) + prefsOwnProfile || (prefs != nil && prefs.ReviewAgentConfigs[name][worker] != nil)) keepWorkerPresent(&cfg, worker, hadPrompt || hadConfig) profile.Agents[worker] = cfg } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 10b928d1f5..9f53eeafbf 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -789,6 +789,7 @@ func loadMergedSettings(ctx context.Context, settingsFileAbs, preferencesFileAbs // and the deprecated `investigate` key left in settings is not read. enforceOPFCommandTrust(ctx, settings, localSettingsFileAbs, localData) enforceExternalAgentsTrust(ctx, settings, localSettingsFileAbs, localData) + applyClonePreferenceAgentConfigs(settings, preferences, localData) enforceAgentPromptTrust(ctx, settings, localSettingsFileAbs, localData, preferences) // allow_symlinked_agent_dirs decides where Entire writes an agent's hook // config, so it gets the same gate, and then installs the surviving set as @@ -1284,12 +1285,17 @@ func clonePreferencesRoot(filePath string) (*os.Root, string, error) { return root, name, nil } -func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { +// applyClonePreferenceAgentConfigs overlays clone-local reviewer configs on +// the effective profiles. It runs after the local file is merged, since that +// file replaces a profile it defines; a config the local file sets itself +// wins. +func applyClonePreferenceAgentConfigs(settings *EntireSettings, prefs *ClonePreferences, localData []byte) { if prefs == nil { return } - if prefs.ReviewProfiles != nil { - settings.ReviewProfiles = mergeReviewProfiles(settings.ReviewProfiles, prefs.ReviewProfiles) + var localRaw map[string]json.RawMessage + if len(localData) > 0 && json.Unmarshal(localData, &localRaw) != nil { + localRaw = nil } for name, workers := range prefs.ReviewAgentConfigs { profile, ok := settings.ReviewProfiles[name] @@ -1298,7 +1304,7 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { } agents := make(map[string]ReviewConfig, len(profile.Agents)) for worker, cfg := range profile.Agents { - if agentCfg, ok := workers[worker]; ok { + if agentCfg, ok := workers[worker]; ok && !rawHasKey(localRaw, "review_profiles", name, "agents", worker, "config") { cfg.Config = agentCfg } agents[worker] = cfg @@ -1306,6 +1312,15 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { profile.Agents = agents settings.ReviewProfiles[name] = profile } +} + +func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { + if prefs == nil { + return + } + if prefs.ReviewProfiles != nil { + settings.ReviewProfiles = mergeReviewProfiles(settings.ReviewProfiles, prefs.ReviewProfiles) + } if prefs.ReviewDefaultProfile != "" { settings.ReviewDefaultProfile = prefs.ReviewDefaultProfile } From 90115fe9fe24529dcbb250d445bb431fdd93336f Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 15:25:33 -0700 Subject: [PATCH 16/22] Test reviewer config end to end and its save paths An integration test drives entire review against fake claude and codex launchers and checks what each receives: the profile's settings and MCP servers, Entire's tracking hooks, the Codex trust overrides and hook-trust bypass, and the blocked-tool notice, with the branch's config left out. Unit tests cover where a config is saved and --set-config none. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VNXYWF41WYSNJ67ZJFQAMY --- .../review_profile_config_test.go | 150 ++++++++++++++++++ cmd/entire/cli/review/configure_test.go | 52 ++++++ cmd/entire/cli/review/trust_cmd_test.go | 11 ++ 3 files changed, 213 insertions(+) create mode 100644 cmd/entire/cli/integration_test/review_profile_config_test.go diff --git a/cmd/entire/cli/integration_test/review_profile_config_test.go b/cmd/entire/cli/integration_test/review_profile_config_test.go new file mode 100644 index 0000000000..120fcfbeaa --- /dev/null +++ b/cmd/entire/cli/integration_test/review_profile_config_test.go @@ -0,0 +1,150 @@ +//go:build integration && !windows + +package integration + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/execx" +) + +// A review profile's agent config, driven through the real binary against +// fake claude and codex launchers that record what they were started with. +// The branch commits its own hooks and MCP servers; the reviewers must get the +// profile's instead, still carry Entire's tracking hooks, and the run must +// report the tool each fake says was blocked. No real agent runs. + +// fakeReviewer records each argument, and the contents of the files passed to +// --settings and --mcp-config (removed when the reviewer exits), to $ARGV_LOG, +// then prints output. +func fakeReviewer(t *testing.T, dir, name, output string) { + t.Helper() + script := `#!/bin/sh +prev="" +for arg in "$@"; do + printf 'ARG %s\n' "$arg" >> "$ARGV_LOG" + case "$prev" in --settings|--mcp-config) cat "$arg" >> "$ARGV_LOG"; printf '\n' >> "$ARGV_LOG" ;; esac + prev="$arg" +done +cat > /dev/null +cat <<'EOF' +` + output + ` +EOF +` + if err := os.WriteFile(filepath.Join(dir, name), []byte(script), 0o755); err != nil { + t.Fatalf("write fake %s: %v", name, err) + } +} + +func TestReviewProfileConfig_ReviewersGetProfileConfig(t *testing.T) { + t.Parallel() + + env := NewFeatureBranchEnv(t) + enableReviewAgent(t, env, agentClaudeCode) + enableReviewAgent(t, env, "codex") + // The branch's own config, which the reviewers must not be handed. + env.WriteFile(".mcp.json", `{"mcpServers":{"branchmcp":{"command":"/branch/mcp"}}}`) + env.WriteSettings(map[string]any{ + "enabled": true, + "review_default_profile": "general", + "review_profiles": map[string]any{ + "general": map[string]any{ + "task": "Review.", + "agents": map[string]any{ + agentClaudeCode: map[string]any{"skills": []string{"/review"}}, + "codex": map[string]any{"skills": []string{"/review"}}, + }, + }, + }, + }) + + cfgDir := t.TempDir() + writeConfig := func(name string, cfg any) string { + data, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + path := filepath.Join(cfgDir, name) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + return path + } + claudeCfg := writeConfig("claude.json", map[string]any{ + "settings": map[string]any{"hooks": map[string]any{"Stop": []any{map[string]any{"hooks": []any{map[string]any{"type": "command", "command": "/opt/profile/stop-hook"}}}}}}, + "mcp_servers": map[string]any{"docs": map[string]any{"command": "/opt/profile/docs"}}, + }) + codexCfg := writeConfig("codex.json", map[string]any{ + "mcp_servers": map[string]any{"docs": map[string]any{"command": "/opt/profile/docs", "default_tools_approval_mode": "approve"}}, + }) + for _, set := range []string{agentClaudeCode + "=" + claudeCfg, "codex=" + codexCfg} { + env.RunCLI("review", "--configure", "general", "--set-config", set) + } + + fakeBin := t.TempDir() + fakeReviewer(t, fakeBin, "claude", + `{"type":"result","subtype":"success","is_error":false,"permission_denials":[{"tool_name":"mcp__docs__search"}]}`) + fakeReviewer(t, fakeBin, "codex", + `{"type":"item.completed","item":{"type":"mcp_tool_call","server":"docs","tool":"search","status":"failed","error":{"message":"MCP tool call requires approval, but approval policy is never"}}} +{"type":"turn.completed","usage":{"input_tokens":1,"output_tokens":1}}`) + + review := func(agentName string) (log, output string) { + argvLog := filepath.Join(t.TempDir(), "argv.log") + cmd := execx.NonInteractive(context.Background(), getTestBinary(), "review", "general", "--agent", agentName) + cmd.Dir = env.RepoDir + cmd.Env = envWithOverrides(env.cliEnv(), + "PATH="+fakeBin+string(os.PathListSeparator)+os.Getenv("PATH"), + "ARGV_LOG="+argvLog, + ) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("entire review --agent %s: %v\n%s", agentName, err, out) + } + data, err := os.ReadFile(argvLog) + if err != nil { + t.Fatalf("%s was not started: %v\n%s", agentName, err, out) + } + return string(data), string(out) + } + expect := func(agentName, text string, want, notWant []string) { + t.Helper() + for _, w := range want { + if !strings.Contains(text, w) { + t.Errorf("%s: missing %q in:\n%s", agentName, w, text) + } + } + for _, w := range notWant { + if strings.Contains(text, w) { + t.Errorf("%s: unexpected %q in:\n%s", agentName, w, text) + } + } + } + + log, out := review(agentClaudeCode) + expect(agentClaudeCode, log, + []string{ + "ARG --setting-sources\nARG user", "ARG --strict-mcp-config", + "/opt/profile/stop-hook", // the profile's hook, in the --settings file + "entire hooks claude-code", // Entire's tracking hooks, added to it + `"docs"`, // the profile's MCP server, in the --mcp-config file + }, + []string{"branchmcp"}) + expect(agentClaudeCode, out, []string{"using your profile's agent config", "blocked mcp__docs__search", "permissions.allow"}, nil) + + log, out = review("codex") + expect("codex", log, + []string{ + `trust_level="untrusted"`, + `ARG mcp_servers.docs.command="/opt/profile/docs"`, + `ARG mcp_servers.docs.default_tools_approval_mode="approve"`, + "ARG hooks.SessionStart=", "entire hooks codex", + "ARG --dangerously-bypass-hook-trust", + }, + []string{"branchmcp"}) + expect("codex", out, []string{"using your profile's agent config", "blocked docs.search", "default_tools_approval_mode"}, nil) +} diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go index 2abec12e3e..155b2e837f 100644 --- a/cmd/entire/cli/review/configure_test.go +++ b/cmd/entire/cli/review/configure_test.go @@ -816,3 +816,55 @@ func TestSaveReviewProfileConfigKeepsLocalAgentConfig(t *testing.T) { } } } + +// A reviewer config goes to the local file when it defines the profile, +// otherwise to clone-local preferences, and a nil config removes it there. +func TestSaveReviewAgentConfigs(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + ctx := context.Background() + + profile := settings.ReviewProfileConfig{Agents: map[string]settings.ReviewConfig{tAgentClaude: {Agent: tAgentClaude}}} + if err := saveReviewProfile(ctx, "shared", profile, true, reviewScopeProject); err != nil { + t.Fatal(err) + } + if err := saveReviewProfile(ctx, "mine", profile, false, reviewScopeLocal); err != nil { + t.Fatal(err) + } + cfg := &settings.ReviewAgentConfig{Extensions: []string{"/opt/x.ts"}} + + where, err := saveReviewAgentConfigs(ctx, "shared", map[string]*settings.ReviewAgentConfig{tAgentClaude: cfg}) + if err != nil || where != "clone-local review preferences" { + t.Fatalf("shared profile: where=%q err=%v", where, err) + } + if where, err := saveReviewAgentConfigs(ctx, "mine", map[string]*settings.ReviewAgentConfig{tAgentClaude: cfg}); err != nil || where != reviewScopeLocal.file() { + t.Fatalf("local profile: where=%q err=%v", where, err) + } + s, err := settings.Load(ctx) + if err != nil { + t.Fatal(err) + } + for _, name := range []string{"shared", "mine"} { + if s.ReviewProfiles[name].Agents[tAgentClaude].Config == nil { + t.Errorf("%s: config not applied", name) + } + } + if _, raw, err := loadReviewSettingsRaw(ctx, reviewScopeProject); err != nil || strings.Contains(string(raw["review_profiles"]), "config") { + t.Fatalf("config reached the shared settings file: %s (err %v)", raw["review_profiles"], err) + } + + if _, err := saveReviewAgentConfigs(ctx, "shared", map[string]*settings.ReviewAgentConfig{tAgentClaude: nil}); err != nil { + t.Fatal(err) + } + prefs, err := settings.LoadClonePreferences(ctx) + if err != nil { + t.Fatal(err) + } + if _, ok := prefs.ReviewAgentConfigs["shared"]; ok { + t.Errorf("removed config left behind: %+v", prefs.ReviewAgentConfigs) + } + if _, err := saveReviewAgentConfigs(ctx, "shared", map[string]*settings.ReviewAgentConfig{"codex": cfg}); err == nil { + t.Error("config for a reviewer the profile doesn't have was saved") + } +} diff --git a/cmd/entire/cli/review/trust_cmd_test.go b/cmd/entire/cli/review/trust_cmd_test.go index f5b10723c0..0a032c6b06 100644 --- a/cmd/entire/cli/review/trust_cmd_test.go +++ b/cmd/entire/cli/review/trust_cmd_test.go @@ -220,4 +220,15 @@ func TestConfigure_SetConfigSavesToClonePreferences(t *testing.T) { if !strings.Contains(stderr.String(), "inside the checkout") { t.Errorf("no warning for a checkout path; stderr:\n%s", stderr.String()) } + + // none removes the reviewer's config, so it uses the checkout's again. + if err := run("--configure", "general", "--set-config", "claude-code=none"); err != nil { + t.Fatalf("--set-config none: %v", err) + } + if s, err = settings.Load(context.Background()); err != nil { + t.Fatal(err) + } + if cfg := s.ReviewProfiles["general"].Agents["claude-code"].Config; cfg != nil { + t.Fatalf("config still set after none: %+v", cfg) + } } From 5c4a78ac35672ad25edacc4ca135424305fd150c Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 13 Oct 2026 16:35:49 -0700 Subject: [PATCH 17/22] Edit a local profile as stored when saving a reviewer config --set-config wrote back the loaded profile. When the local file can't be verified as untracked, Load has dropped its prompts and configs, so saving one reviewer's config erased the others from the file. It now edits the stored profile and changes only the targeted reviewers. --edit, which would pre-fill from the same view, refuses such a profile and says how to fix it. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M4VSYMMH98SEG9RR2V5PAJPJ --- cmd/entire/cli/review/agent_config_save.go | 29 ++++++----- cmd/entire/cli/review/cmd.go | 17 +++++++ cmd/entire/cli/review/configure_test.go | 49 +++++++++++++++++++ cmd/entire/cli/review/picker.go | 40 ++++++++++----- cmd/entire/cli/settings/agent_prompt_trust.go | 7 +++ 5 files changed, 119 insertions(+), 23 deletions(-) diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go index d3f246fdb3..3644ddcdc6 100644 --- a/cmd/entire/cli/review/agent_config_save.go +++ b/cmd/entire/cli/review/agent_config_save.go @@ -104,19 +104,11 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map 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 { + for worker := range configs { + if _, ok := profile.Agents[worker]; !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 { @@ -126,8 +118,21 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map if err != nil { return "", err } - if _, localOwns := localProfiles[profileName]; localOwns { - if err := saveReviewProfile(ctx, profileName, profile, false, reviewScopeLocal); err != nil { + if localProfile, localOwns := localProfiles[profileName]; localOwns { + // Edit the profile as stored, not the loaded view: Load drops fields + // it can't trust (an unverified local file's prompts and configs), + // and writing that view back would erase them. + agents := make(map[string]settings.ReviewConfig, len(localProfile.Agents)) + for worker, cfg := range localProfile.Agents { + agents[worker] = cfg + } + for worker, cfg := range configs { + existing := agents[worker] + existing.Config = cfg + agents[worker] = existing + } + localProfile.Agents = agents + if err := saveReviewProfile(ctx, profileName, localProfile, false, reviewScopeLocal); err != nil { return "", err } return reviewScopeLocal.file(), nil diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 03568c74a6..7e02f78182 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -1021,6 +1021,23 @@ func printBlockedTools(w io.Writer, profileName string, summary reviewtypes.RunS } } +// profileHasUnverifiedFields reports whether Load dropped any of profileName's +// prompts or reviewer configs because the local file holding them could not be +// verified. +func profileHasUnverifiedFields(s *settings.EntireSettings, profileName string, profile settings.ReviewProfileConfig) bool { + prefix := "review_profiles." + profileName + "." + fields := []string{prefix + "task", prefix + "judge.prompt"} + for worker := range profile.Agents { + fields = append(fields, prefix+"agents."+worker+".prompt", prefix+"agents."+worker+".config") + } + for _, rej := range s.AgentPromptRejections() { + if settings.RejectionUnverified(rej) && slices.Contains(fields, rej.Field) { + return true + } + } + return false +} + // unverifiedReviewerConfig returns the first of the unverified (dropped) // config fields that belongs to a reviewer of profileName that will run. func unverifiedReviewerConfig(unverified []string, profileName string, profile settings.ReviewProfileConfig, agentOverride string) (string, bool) { diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go index 155b2e837f..6a8cb95c7d 100644 --- a/cmd/entire/cli/review/configure_test.go +++ b/cmd/entire/cli/review/configure_test.go @@ -3,6 +3,7 @@ package review import ( "bytes" "context" + "io" "strings" "testing" @@ -868,3 +869,51 @@ func TestSaveReviewAgentConfigs(t *testing.T) { t.Error("config for a reviewer the profile doesn't have was saved") } } + +// Saving one reviewer's config into the local file edits that file as stored. +// The loaded view drops an unverified file's prompts and configs (here the +// file is still in HEAD), and writing it back would erase them. +func TestSaveReviewAgentConfigsKeepsUnverifiedLocalFields(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + ctx := context.Background() + + profile := settings.ReviewProfileConfig{Agents: map[string]settings.ReviewConfig{ + tAgentClaude: {Agent: tAgentClaude, Prompt: "Focus on auth.", Config: &settings.ReviewAgentConfig{Extensions: []string{"/opt/x.ts"}}}, + tAgentCodex: {Agent: tAgentCodex}, + }} + if err := saveReviewProfile(ctx, "mine", profile, true, reviewScopeLocal); err != nil { + t.Fatal(err) + } + testutil.RunGit(t, tmp, "add", "-f", ".entire/settings.local.json") + testutil.RunGit(t, tmp, "commit", "-q", "-m", "local settings") + testutil.RunGit(t, tmp, "rm", "-q", "--cached", ".entire/settings.local.json") + settings.ClearVersionedPathCache() + t.Cleanup(settings.ClearVersionedPathCache) + + if _, err := saveReviewAgentConfigs(ctx, "mine", map[string]*settings.ReviewAgentConfig{tAgentCodex: {}}); err != nil { + t.Fatal(err) + } + _, raw, err := loadReviewSettingsRaw(ctx, reviewScopeLocal) + if err != nil { + t.Fatal(err) + } + stored, err := decodeRawReviewProfiles(raw) + if err != nil { + t.Fatal(err) + } + claude := stored["mine"].Agents[tAgentClaude] + if claude.Prompt != "Focus on auth." || claude.Config == nil { + t.Fatalf("claude-code reviewer lost its stored fields: %+v", claude) + } + if stored["mine"].Agents[tAgentCodex].Config == nil { + t.Fatal("codex config not saved") + } + + // --edit would pre-fill from the same loaded view, so it refuses. + installed := func(context.Context) []agenttypes.AgentName { return []agenttypes.AgentName{tAgentClaude, tAgentCodex} } + if err := RunReviewProfileConfigPicker(ctx, io.Discard, installed, "mine"); err == nil || !strings.Contains(err.Error(), "git rm --cached") { + t.Fatalf("--edit on an unverified local profile: err = %v", err) + } +} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index 06240cfdb4..f5ebefb542 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -803,17 +803,9 @@ func RunReviewProfileConfigPicker(ctx context.Context, out io.Writer, getInstall // log at Warn so users debugging "my saved skills aren't pre-checked" can // see why, but keep going with an empty prefill — runReview already // surfaces the same error distinctly when it's the first load. - existing := map[string]settings.ReviewConfig{} - existingJudge := "" - if s, err := settings.Load(ctx); err != nil { - logging.Warn(ctx, "settings.Load failed when pre-filling picker", slog.String("error", err.Error())) - } else if s != nil { - if profile, ok := s.ReviewProfiles[profileName]; ok { - existing = profile.Agents - if j, jok := profileJudge(profile); jok { - existingJudge = j.agent - } - } + existing, existingJudge, err := pickerPrefill(ctx, profileName) + if err != nil { + return err } // Up-front header: make the order and count obvious so users can spot @@ -1415,3 +1407,29 @@ func promptReviewAgentConfig(ctx context.Context, out io.Writer, agentName strin } return cfg, true, nil } + +// pickerPrefill returns the saved reviewers and judge for profileName. It +// refuses a profile whose prompts or configs Load dropped as unverified: the +// picker would show them blank and saving would erase them from the file. +func pickerPrefill(ctx context.Context, profileName string) (map[string]settings.ReviewConfig, string, error) { + s, err := settings.Load(ctx) + if err != nil { + logging.Warn(ctx, "settings.Load failed when pre-filling picker", slog.String("error", err.Error())) + return map[string]settings.ReviewConfig{}, "", nil + } + if s == nil { + return map[string]settings.ReviewConfig{}, "", nil + } + profile, ok := s.ReviewProfiles[profileName] + if !ok { + return map[string]settings.ReviewConfig{}, "", nil + } + if profileHasUnverifiedFields(s, profileName, profile) { + return nil, "", fmt.Errorf("review profile %q is in .entire/settings.local.json, which could not be verified as untracked, so its prompts and reviewer configs aren't loaded and saving would erase them; untrack it (git rm --cached .entire/settings.local.json) and run --edit again", profileName) + } + judge := "" + if j, jok := profileJudge(profile); jok { + judge = j.agent + } + return profile.Agents, judge, nil +} diff --git a/cmd/entire/cli/settings/agent_prompt_trust.go b/cmd/entire/cli/settings/agent_prompt_trust.go index 39bb4983c3..75530752f5 100644 --- a/cmd/entire/cli/settings/agent_prompt_trust.go +++ b/cmd/entire/cli/settings/agent_prompt_trust.go @@ -37,6 +37,13 @@ func AgentConfigRejectionUnverified(rej AgentPromptRejection) bool { return strings.HasSuffix(rej.Field, ".config") && rej.Reason == agentPromptRejectionUnverified } +// RejectionUnverified reports whether rej dropped a field because the local +// settings file holding it could not be verified as untracked. The field is +// still in the file, so writing back the loaded view would erase it. +func RejectionUnverified(rej AgentPromptRejection) bool { + return 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 From 0afabaea8299bcf5b9b402c8da5f1fa1d72c9830 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 15 Oct 2026 19:30:36 -0700 Subject: [PATCH 18/22] Refuse Codex MCP fields it can't pass on, and clear a dropped reviewer's config A Codex MCP server's headers, cwd and other fields were silently left out, so a server could run without its auth header; they are now refused when saved and before the run. Switching a config-only reviewer to the checkout's config in the picker drops it from the profile; clearing its stored config no longer fails because the reviewer is gone. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M518R3297MMFEE22VJX8AHGJ --- cmd/entire/cli/review/agent_config.go | 28 ++++++++++++++++++++-- cmd/entire/cli/review/agent_config_save.go | 11 ++++++--- cmd/entire/cli/review/agent_config_test.go | 2 ++ cmd/entire/cli/review/configure_test.go | 23 ++++++++++++++++++ docs/architecture/review-command.md | 2 +- 5 files changed, 60 insertions(+), 6 deletions(-) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index ca01b7ee19..3e574365b4 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -23,6 +23,10 @@ var agentConfigFields = map[string][]string{ var mcpServerNamePattern = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) +// codexMCPFields are the MCP server fields a Codex reviewer passes through. +// "type" is accepted and dropped: Codex infers it from command or url. +var codexMCPFields = []string{"command", "args", "url", "env", "default_tools_approval_mode", "type"} + // toAgentConfig converts a profile's config for a run; nil stays nil. func toAgentConfig(cfg *settings.ReviewAgentConfig) *reviewtypes.AgentConfig { if cfg == nil { @@ -77,8 +81,13 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { if server.Command == "" && server.URL == "" { return fmt.Errorf("MCP server %q needs a command or a url", name) } - if agentName == "codex" && len(server.Env) > 0 { - return fmt.Errorf("MCP server %q: env values aren't supported for Codex in a review profile yet", name) + if agentName == "codex" { + if err := checkCodexMCPFields(name, cfg.MCPServers[name]); err != nil { + return err + } + if len(server.Env) > 0 { + return fmt.Errorf("MCP server %q: env values aren't supported for Codex in a review profile yet", name) + } } } // A relative extension would load from the reviewed checkout, the one @@ -91,6 +100,21 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return nil } +// checkCodexMCPFields refuses fields a Codex reviewer would not pass on, so a +// server doesn't run without, say, its auth header. +func checkCodexMCPFields(name string, raw json.RawMessage) error { + var fields map[string]json.RawMessage + if err := json.Unmarshal(raw, &fields); err != nil { + return fmt.Errorf("MCP server %q: %w", name, err) + } + for _, field := range sortedStringKeys(fields) { + if !slices.Contains(codexMCPFields, field) { + return fmt.Errorf("MCP server %q: %q isn't supported for Codex in a review profile yet", name, field) + } + } + return nil +} + // AgentConfigWarnings lists values in cfg that name a path inside one of // roots or the project directory variable: an easy copy-paste mistake, since // those would run the reviewed branch's files. They are warnings, not diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go index 3644ddcdc6..6748ad560e 100644 --- a/cmd/entire/cli/review/agent_config_save.go +++ b/cmd/entire/cli/review/agent_config_save.go @@ -104,8 +104,10 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map if !ok { return "", fmt.Errorf("review profile %q not found", profileName) } - for worker := range configs { - if _, ok := profile.Agents[worker]; !ok { + // A removal may name a reviewer the profile just dropped (its config was + // all it had); clearing that config is still wanted. + for worker, cfg := range configs { + if _, ok := profile.Agents[worker]; !ok && cfg != nil { return "", fmt.Errorf("review profile %q has no reviewer %q", profileName, worker) } } @@ -127,7 +129,10 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map agents[worker] = cfg } for worker, cfg := range configs { - existing := agents[worker] + existing, ok := agents[worker] + if !ok && cfg == nil { + continue + } existing.Config = cfg agents[worker] = existing } diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 6aabfb08fd..6c1c563939 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -30,6 +30,8 @@ func TestValidateAgentConfig(t *testing.T) { {"MCP url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example"}`)}, ""}, {"MCP without command or url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"args":["x"]}`)}, "needs a command or a url"}, {"codex MCP env", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, "env values aren't supported for Codex"}, + {"codex MCP header dropped", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example","headers":{"Authorization":"x"}}`)}, `"headers" isn't supported for Codex`}, + {"codex MCP type", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"type":"stdio","command":"/opt/mcp","default_tools_approval_mode":"approve"}`)}, ""}, {"claude MCP env", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, ""}, {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go index 6a8cb95c7d..6ab402c66c 100644 --- a/cmd/entire/cli/review/configure_test.go +++ b/cmd/entire/cli/review/configure_test.go @@ -868,6 +868,29 @@ func TestSaveReviewAgentConfigs(t *testing.T) { if _, err := saveReviewAgentConfigs(ctx, "shared", map[string]*settings.ReviewAgentConfig{"codex": cfg}); err == nil { t.Error("config for a reviewer the profile doesn't have was saved") } + + // The picker drops a reviewer whose config was all it had when it is + // switched to the checkout's config; its stored config is still cleared. + if _, err := saveReviewAgentConfigs(ctx, "shared", map[string]*settings.ReviewAgentConfig{"codex": cfg, tAgentClaude: cfg}); err == nil { + t.Fatal("config for a reviewer the profile doesn't have was saved") + } + if err := settings.ModifyClonePreferences(ctx, func(p *settings.ClonePreferences) error { + p.ReviewAgentConfigs = map[string]map[string]*settings.ReviewAgentConfig{"shared": {"codex": cfg}} + return nil + }); err != nil { + t.Fatal(err) + } + for _, name := range []string{"shared", "mine"} { + if _, err := saveReviewAgentConfigs(ctx, name, map[string]*settings.ReviewAgentConfig{"codex": nil}); err != nil { + t.Fatalf("%s: removing a dropped reviewer's config: %v", name, err) + } + } + if prefs, err := settings.LoadClonePreferences(ctx); err != nil || len(prefs.ReviewAgentConfigs) != 0 { + t.Errorf("dropped reviewer's config left behind: %+v (err %v)", prefs.ReviewAgentConfigs, err) + } + if _, raw, err := loadReviewSettingsRaw(ctx, reviewScopeLocal); err != nil || strings.Contains(string(raw["review_profiles"]), "codex") { + t.Errorf("removal added the dropped reviewer to the local profile: %s (err %v)", raw["review_profiles"], err) + } } // Saving one reviewer's config into the local file edits that file as stored. diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index cf87c3c50b..b18777d4f1 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -169,7 +169,7 @@ own config with nothing extra, or load a JSON file) or | 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; `default_tools_approval_mode` passes through); Entire's tracking hooks via `-c` with `--dangerously-bypass-hook-trust`, which also runs the user's own untrusted hooks | AGENTS.md; skills | +| Codex | checkout and main repo marked untrusted for the run (project config, hooks, rules) | `mcp_servers` via `-c` (`command`, `args`, `url` and `default_tools_approval_mode`; other fields and literal `env` values are refused); Entire's tracking hooks via `-c` with `--dangerously-bypass-hook-trust`, which also runs the user's own untrusted hooks | AGENTS.md; skills | | Pi | `--no-extensions` | `extensions` via `--extension`, plus Entire's own | AGENTS.md/CLAUDE.md, skills, prompt templates, `.pi/settings.json` | Rules: From a80bd12a757d306127ab394b4876b31f84864bed Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 15 Oct 2026 19:48:18 -0700 Subject: [PATCH 19/22] Refuse an MCP server that sets both a command and a url Codex passed only the command, so the reviewer ran a local server instead of the remote one it was configured with. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M519RGC56AFVA2F77ABNHGBX --- cmd/entire/cli/review/agent_config.go | 3 +++ cmd/entire/cli/review/agent_config_test.go | 1 + 2 files changed, 4 insertions(+) diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 3e574365b4..0c8befced5 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -81,6 +81,9 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { if server.Command == "" && server.URL == "" { return fmt.Errorf("MCP server %q needs a command or a url", name) } + if server.Command != "" && server.URL != "" { + return fmt.Errorf("MCP server %q: set a command or a url, not both", name) + } if agentName == "codex" { if err := checkCodexMCPFields(name, cfg.MCPServers[name]); err != nil { return err diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 6c1c563939..ca854a53a0 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -28,6 +28,7 @@ func TestValidateAgentConfig(t *testing.T) { {"claude hook with a launcher", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":[{"hooks":[{"type":"command","command":"yarn lint"}]}]}}`)}, ""}, {"MCP command", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"npx","args":["server"]}`)}, ""}, {"MCP url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example"}`)}, ""}, + {"MCP command and url", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","url":"https://mcp.example"}`)}, "not both"}, {"MCP without command or url", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"args":["x"]}`)}, "needs a command or a url"}, {"codex MCP env", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, "env values aren't supported for Codex"}, {"codex MCP header dropped", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"url":"https://mcp.example","headers":{"Authorization":"x"}}`)}, `"headers" isn't supported for Codex`}, From 891c9019631faef9960cb42e777a74c292cf45e0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 15 Oct 2026 20:07:46 -0700 Subject: [PATCH 20/22] Refuse null Claude settings and hooks in a reviewer config A null settings or hooks value passed validation and then crashed the review while Entire's hooks were added. It is now refused when saved and before the run, and the merge treats it as empty. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M51AW525Z9P1GJFE1P1NC1FE --- .../cli/agent/claudecode/review_config.go | 6 +++++ .../agent/claudecode/review_config_test.go | 15 +++++++++++++ cmd/entire/cli/review/agent_config.go | 22 +++++++++++++++++-- cmd/entire/cli/review/agent_config_test.go | 2 ++ 4 files changed, 43 insertions(+), 2 deletions(-) diff --git a/cmd/entire/cli/agent/claudecode/review_config.go b/cmd/entire/cli/agent/claudecode/review_config.go index 8716653971..8cf1126f34 100644 --- a/cmd/entire/cli/agent/claudecode/review_config.go +++ b/cmd/entire/cli/agent/claudecode/review_config.go @@ -88,11 +88,17 @@ func reviewProfileSettings(profile json.RawMessage) ([]byte, error) { return nil, fmt.Errorf("review profile settings: %w", err) } } + if obj == nil { // "null" + obj = map[string]json.RawMessage{} + } rawHooks := map[string]json.RawMessage{} if existing, ok := obj["hooks"]; ok { if err := json.Unmarshal(existing, &rawHooks); err != nil { return nil, fmt.Errorf("review profile settings hooks: %w", err) } + if rawHooks == nil { + rawHooks = map[string]json.RawMessage{} + } } installHookEntries(rawHooks, false) hooks, err := jsonutil.MarshalWithNoHTMLEscape(rawHooks) diff --git a/cmd/entire/cli/agent/claudecode/review_config_test.go b/cmd/entire/cli/agent/claudecode/review_config_test.go index 5f0979bc9b..7654cd289b 100644 --- a/cmd/entire/cli/agent/claudecode/review_config_test.go +++ b/cmd/entire/cli/agent/claudecode/review_config_test.go @@ -119,3 +119,18 @@ func TestPrepareReviewAgentConfigRefusesSymlinkedSkill(t *testing.T) { t.Fatalf("err = %v, want a symlink refusal", err) } } + +// A null settings or hooks value still gets Entire's hooks rather than +// crashing the review. +func TestReviewProfileSettingsNull(t *testing.T) { + t.Parallel() + for _, profile := range []string{`null`, `{"hooks":null}`} { + out, err := reviewProfileSettings(json.RawMessage(profile)) + if err != nil { + t.Fatalf("%s: %v", profile, err) + } + if !strings.Contains(string(out), "entire hooks claude-code") { + t.Errorf("%s: Entire's hooks missing: %s", profile, out) + } + } +} diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 0c8befced5..0eadf31274 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -61,10 +61,15 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return err } if len(cfg.Settings) > 0 { - var obj map[string]json.RawMessage - if err := json.Unmarshal(cfg.Settings, &obj); err != nil { + obj, err := jsonObject(cfg.Settings) + if err != nil { return fmt.Errorf("settings: %w", err) } + if hooks, ok := obj["hooks"]; ok { + if _, err := jsonObject(hooks); err != nil { + return fmt.Errorf("settings hooks: %w", err) + } + } } for _, name := range sortedStringKeys(cfg.MCPServers) { if !mcpServerNamePattern.MatchString(name) { @@ -103,6 +108,19 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return nil } +// jsonObject decodes raw as a JSON object; null is refused, since the agent +// would treat it as missing. +func jsonObject(raw json.RawMessage) (map[string]json.RawMessage, error) { + var obj map[string]json.RawMessage + if err := json.Unmarshal(raw, &obj); err != nil { + return nil, err //nolint:wrapcheck // callers name the field + } + if obj == nil { + return nil, errors.New("must be an object, not null") + } + return obj, nil +} + // checkCodexMCPFields refuses fields a Codex reviewer would not pass on, so a // server doesn't run without, say, its auth header. func checkCodexMCPFields(name string, raw json.RawMessage) error { diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index ca854a53a0..55ef07f69e 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -35,6 +35,8 @@ func TestValidateAgentConfig(t *testing.T) { {"codex MCP type", "codex", reviewtypes.AgentConfig{MCPServers: mcp(`{"type":"stdio","command":"/opt/mcp","default_tools_approval_mode":"approve"}`)}, ""}, {"claude MCP env", "claude-code", reviewtypes.AgentConfig{MCPServers: mcp(`{"command":"/opt/mcp","env":{"TOKEN":"x"}}`)}, ""}, {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, + {"settings null", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`null`)}, "not null"}, + {"settings hooks null", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":null}`)}, "settings hooks"}, {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, {"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"`}, From 7d6492b0c4e7657dac9256c2d44976851f6c7436 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 15 Oct 2026 20:26:07 -0700 Subject: [PATCH 21/22] Refuse a Claude hook event that isn't a list of matchers An event such as "Stop": {} passed validation and was then dropped while Entire's hooks were merged in, so the reviewer ran without that hook. Co-Authored-By: Claude Opus 5.5 Entire-Checkpoint: 01M51BXQZZG48Y96J19EM44SYV --- cmd/entire/cli/agent/claudecode/review_config.go | 8 ++++++++ cmd/entire/cli/agent/claudecode/review_config_test.go | 9 +++++++++ cmd/entire/cli/review/agent_config.go | 9 ++++++++- cmd/entire/cli/review/agent_config_test.go | 2 ++ 4 files changed, 27 insertions(+), 1 deletion(-) diff --git a/cmd/entire/cli/agent/claudecode/review_config.go b/cmd/entire/cli/agent/claudecode/review_config.go index 8cf1126f34..f256954c5d 100644 --- a/cmd/entire/cli/agent/claudecode/review_config.go +++ b/cmd/entire/cli/agent/claudecode/review_config.go @@ -100,6 +100,14 @@ func reviewProfileSettings(profile json.RawMessage) ([]byte, error) { rawHooks = map[string]json.RawMessage{} } } + // Entire's hooks are merged by event; an event that isn't a list of + // matchers would be dropped without a word. + for event, raw := range rawHooks { + var matchers []ClaudeHookMatcher + if err := json.Unmarshal(raw, &matchers); err != nil || matchers == nil { + return nil, fmt.Errorf("review profile settings hooks %s: must be a list of matcher objects", event) + } + } installHookEntries(rawHooks, false) hooks, err := jsonutil.MarshalWithNoHTMLEscape(rawHooks) if err != nil { diff --git a/cmd/entire/cli/agent/claudecode/review_config_test.go b/cmd/entire/cli/agent/claudecode/review_config_test.go index 7654cd289b..f5434bf78c 100644 --- a/cmd/entire/cli/agent/claudecode/review_config_test.go +++ b/cmd/entire/cli/agent/claudecode/review_config_test.go @@ -134,3 +134,12 @@ func TestReviewProfileSettingsNull(t *testing.T) { } } } + +func TestReviewProfileSettingsRefusesMalformedHookEvent(t *testing.T) { + t.Parallel() + for _, profile := range []string{`{"hooks":{"Stop":{}}}`, `{"hooks":{"Stop":null}}`, `{"hooks":{"Stop":"x"}}`} { + if _, err := reviewProfileSettings(json.RawMessage(profile)); err == nil { + t.Errorf("%s: accepted", profile) + } + } +} diff --git a/cmd/entire/cli/review/agent_config.go b/cmd/entire/cli/review/agent_config.go index 0eadf31274..8e77f939cb 100644 --- a/cmd/entire/cli/review/agent_config.go +++ b/cmd/entire/cli/review/agent_config.go @@ -66,9 +66,16 @@ func ValidateAgentConfig(agentName string, cfg *reviewtypes.AgentConfig) error { return fmt.Errorf("settings: %w", err) } if hooks, ok := obj["hooks"]; ok { - if _, err := jsonObject(hooks); err != nil { + events, err := jsonObject(hooks) + if err != nil { return fmt.Errorf("settings hooks: %w", err) } + for _, event := range sortedStringKeys(events) { + var matchers []map[string]json.RawMessage + if err := json.Unmarshal(events[event], &matchers); err != nil || matchers == nil || slices.ContainsFunc(matchers, func(m map[string]json.RawMessage) bool { return m == nil }) { + return fmt.Errorf("settings hooks %s: must be a list of matcher objects", event) + } + } } } for _, name := range sortedStringKeys(cfg.MCPServers) { diff --git a/cmd/entire/cli/review/agent_config_test.go b/cmd/entire/cli/review/agent_config_test.go index 55ef07f69e..0c6f22bbee 100644 --- a/cmd/entire/cli/review/agent_config_test.go +++ b/cmd/entire/cli/review/agent_config_test.go @@ -37,6 +37,8 @@ func TestValidateAgentConfig(t *testing.T) { {"MCP bad name", "codex", reviewtypes.AgentConfig{MCPServers: map[string]json.RawMessage{"a.b": json.RawMessage(`{"url":"x"}`)}}, "letters, digits"}, {"settings null", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`null`)}, "not null"}, {"settings hooks null", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":null}`)}, "settings hooks"}, + {"settings hook event not a list", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":{}}}`)}, "hooks Stop"}, + {"settings hook event null", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`{"hooks":{"Stop":null}}`)}, "hooks Stop"}, {"settings not an object", "claude-code", reviewtypes.AgentConfig{Settings: json.RawMessage(`[]`)}, "settings"}, {"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"`}, From f6aac2fa91a504588463b3855a25154f190b2001 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 16 Oct 2026 14:18:31 -0700 Subject: [PATCH 22/22] Drop a stale preferences config when the local file owns the profile Removing a reviewer's config from a profile in settings.local.json left any clone-local preferences entry for it, which applied again since the local file no longer set one. Saves to the local file now clear those entries. Co-Authored-By: Claude Opus 5.5 --- cmd/entire/cli/review/agent_config_save.go | 22 ++++++++++++++++++---- cmd/entire/cli/review/configure_test.go | 19 +++++++++++++++++++ 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/cmd/entire/cli/review/agent_config_save.go b/cmd/entire/cli/review/agent_config_save.go index 6748ad560e..6cf9365cec 100644 --- a/cmd/entire/cli/review/agent_config_save.go +++ b/cmd/entire/cli/review/agent_config_save.go @@ -140,15 +140,29 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map if err := saveReviewProfile(ctx, profileName, localProfile, false, reviewScopeLocal); err != nil { return "", err } + // A preferences entry for these reviewers would apply again once the + // local file stops setting one, so drop it. + if err := saveClonePreferenceAgentConfigs(ctx, profileName, configs, true); err != nil { + return "", err + } return reviewScopeLocal.file(), nil } - err = settings.ModifyClonePreferences(ctx, func(p *settings.ClonePreferences) error { + if err := saveClonePreferenceAgentConfigs(ctx, profileName, configs, false); err != nil { + return "", err + } + return "clone-local review preferences", nil +} + +// saveClonePreferenceAgentConfigs stores configs on profileName in clone-local +// preferences; a nil config, or every config when removeAll is set, is removed. +func saveClonePreferenceAgentConfigs(ctx context.Context, profileName string, configs map[string]*settings.ReviewAgentConfig, removeAll bool) error { + 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 { + if cfg == nil || removeAll { delete(workers, worker) } else { workers[worker] = cfg @@ -165,9 +179,9 @@ func saveReviewAgentConfigs(ctx context.Context, profileName string, configs map return nil }) if err != nil { - return "", fmt.Errorf("save clone-local review preferences: %w", err) + return fmt.Errorf("save clone-local review preferences: %w", err) } - return "clone-local review preferences", nil + return nil } // parseSetConfig parses --set-config worker=. diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go index 6ab402c66c..14ebf5e91b 100644 --- a/cmd/entire/cli/review/configure_test.go +++ b/cmd/entire/cli/review/configure_test.go @@ -891,6 +891,25 @@ func TestSaveReviewAgentConfigs(t *testing.T) { if _, raw, err := loadReviewSettingsRaw(ctx, reviewScopeLocal); err != nil || strings.Contains(string(raw["review_profiles"]), "codex") { t.Errorf("removal added the dropped reviewer to the local profile: %s (err %v)", raw["review_profiles"], err) } + + // Removing a config from a local-owned profile also drops a stale + // preferences entry, which would otherwise apply again. + if err := settings.ModifyClonePreferences(ctx, func(p *settings.ClonePreferences) error { + p.ReviewAgentConfigs = map[string]map[string]*settings.ReviewAgentConfig{"mine": {tAgentClaude: cfg}} + return nil + }); err != nil { + t.Fatal(err) + } + if _, err := saveReviewAgentConfigs(ctx, "mine", map[string]*settings.ReviewAgentConfig{tAgentClaude: nil}); err != nil { + t.Fatal(err) + } + s, err = settings.Load(ctx) + if err != nil { + t.Fatal(err) + } + if got := s.ReviewProfiles["mine"].Agents[tAgentClaude].Config; got != nil { + t.Errorf("removed config still applies: %+v", got) + } } // Saving one reviewer's config into the local file edits that file as stored.