diff --git a/cmd/entire/cli/agent/claudecode/models.go b/cmd/entire/cli/agent/claudecode/models.go new file mode 100644 index 000000000..666900254 --- /dev/null +++ b/cmd/entire/cli/agent/claudecode/models.go @@ -0,0 +1,20 @@ +package claudecode + +import ( + "context" + + "github.com/entireio/cli/cmd/entire/cli/agent" +) + +var _ agent.ModelLister = (*ClaudeCodeAgent)(nil) + +// ListModels returns common Claude model aliases for `entire review --model`. +// Claude Code's CLI accepts these aliases (per `claude --help`) as well as full +// model identifiers; the list is advisory and intentionally non-exhaustive. +func (c *ClaudeCodeAgent) ListModels(_ context.Context) ([]agent.ModelInfo, error) { + return []agent.ModelInfo{ + {ID: "opus", Note: "alias — latest Claude Opus"}, + {ID: "sonnet", Note: "alias — latest Claude Sonnet"}, + {ID: "haiku", Note: "alias — latest Claude Haiku (fast)"}, + }, nil +} diff --git a/cmd/entire/cli/agent/claudecode/reviewer.go b/cmd/entire/cli/agent/claudecode/reviewer.go index a30e01049..492d617d1 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer.go +++ b/cmd/entire/cli/agent/claudecode/reviewer.go @@ -37,7 +37,11 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { // Exposed at package level for test inspection of argv and env. func buildReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd { prompt := review.ComposeReviewPrompt(cfg) - cmd := exec.CommandContext(ctx, "claude", "-p", prompt, "--output-format", "stream-json", "--verbose") + args := []string{"-p", prompt, "--output-format", "stream-json", "--verbose"} + if cfg.Model != "" { + args = append(args, "--model", cfg.Model) + } + cmd := exec.CommandContext(ctx, "claude", args...) cmd.Env = review.AppendReviewEnv(os.Environ(), "claude-code", cfg, prompt) return cmd } diff --git a/cmd/entire/cli/agent/codex/models.go b/cmd/entire/cli/agent/codex/models.go new file mode 100644 index 000000000..9e67f19f7 --- /dev/null +++ b/cmd/entire/cli/agent/codex/models.go @@ -0,0 +1,20 @@ +package codex + +import ( + "context" + + "github.com/entireio/cli/cmd/entire/cli/agent" +) + +var _ agent.ModelLister = (*CodexAgent)(nil) + +// ListModels returns example Codex model identifiers for `entire review +// --model`. Codex has no model-enumeration command, so these are advisory +// examples — `--model` forwards any value the codex CLI accepts. +func (c *CodexAgent) ListModels(_ context.Context) ([]agent.ModelInfo, error) { + return []agent.ModelInfo{ + {ID: "gpt-5-codex", Note: "example — Codex-tuned"}, + {ID: "gpt-5", Note: "example"}, + {ID: "o3", Note: "example"}, + }, nil +} diff --git a/cmd/entire/cli/agent/codex/reviewer.go b/cmd/entire/cli/agent/codex/reviewer.go index 0567056ef..4f3827d25 100644 --- a/cmd/entire/cli/agent/codex/reviewer.go +++ b/cmd/entire/cli/agent/codex/reviewer.go @@ -35,7 +35,11 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { func buildCodexReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd { promptCfg := cfg promptCfg.Skills = expandCodexBuiltinReview(cfg.Skills) - args := []string{codexExecCommand, "--skip-git-repo-check", "--json", "-"} + args := []string{codexExecCommand, "--skip-git-repo-check", "--json"} + if cfg.Model != "" { + args = append(args, "--model", cfg.Model) + } + args = append(args, "-") prompt := review.ComposeReviewPrompt(promptCfg) cmd := exec.CommandContext(ctx, "codex", args...) cmd.Stdin = strings.NewReader(prompt) diff --git a/cmd/entire/cli/agent/geminicli/models.go b/cmd/entire/cli/agent/geminicli/models.go new file mode 100644 index 000000000..bd72a0b4a --- /dev/null +++ b/cmd/entire/cli/agent/geminicli/models.go @@ -0,0 +1,19 @@ +package geminicli + +import ( + "context" + + "github.com/entireio/cli/cmd/entire/cli/agent" +) + +var _ agent.ModelLister = (*GeminiCLIAgent)(nil) + +// ListModels returns example Gemini model identifiers for `entire review +// --model`. The Gemini CLI has no model-enumeration command, so these are +// advisory examples — `--model` forwards any value the gemini CLI accepts. +func (g *GeminiCLIAgent) ListModels(_ context.Context) ([]agent.ModelInfo, error) { + return []agent.ModelInfo{ + {ID: "gemini-2.5-pro", Note: "example"}, + {ID: "gemini-2.5-flash", Note: "example — faster"}, + }, nil +} diff --git a/cmd/entire/cli/agent/geminicli/reviewer.go b/cmd/entire/cli/agent/geminicli/reviewer.go index e23b308c4..1c45bb833 100644 --- a/cmd/entire/cli/agent/geminicli/reviewer.go +++ b/cmd/entire/cli/agent/geminicli/reviewer.go @@ -35,7 +35,11 @@ func buildGeminiReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec. // Per the existing GenerateText implementation: pass "-p " " " as the // argv placeholder to trigger headless (non-interactive) mode, and pipe // the actual prompt via stdin to avoid argv size limits. - cmd := exec.CommandContext(ctx, "gemini", "-p", " ") + args := []string{"-p", " "} + if cfg.Model != "" { + args = append(args, "--model", cfg.Model) + } + cmd := exec.CommandContext(ctx, "gemini", args...) cmd.Stdin = strings.NewReader(prompt) // Agent name must equal string(ag.Name()) — adoptReviewEnv compares // ENTIRE_REVIEW_AGENT against it; any drift silently skips adoption. diff --git a/cmd/entire/cli/agent/model_lister.go b/cmd/entire/cli/agent/model_lister.go new file mode 100644 index 000000000..b07f63787 --- /dev/null +++ b/cmd/entire/cli/agent/model_lister.go @@ -0,0 +1,41 @@ +package agent + +import "context" + +// ModelInfo describes one model an agent can run via `--model`. +type ModelInfo struct { + // ID is the value passed to the agent CLI's --model flag (an exact model + // identifier or a provider alias such as "sonnet"). + ID string + // Note is an optional short human hint (e.g. "alias", "faster", + // "example") shown alongside the ID. It carries no behavior. + Note string +} + +// ModelLister is an optional capability for agents that can advertise the +// models usable with `entire review --model`. +// +// Built-in agents whose CLI has no model-enumeration command (claude-code, +// codex, gemini) return a curated, intentionally non-exhaustive list of common +// models/aliases — `--model` ultimately accepts anything the agent CLI does. +// Agents whose CLI can enumerate models live (e.g. Pi's `pi --list-models`) +// may shell out instead. +type ModelLister interface { + Agent + + // ListModels returns the advertised models for this agent. The list is + // advisory; callers must still allow arbitrary `--model` values. + ListModels(ctx context.Context) ([]ModelInfo, error) +} + +// AsModelLister returns the agent as a ModelLister if it implements the +// capability. Unlike AsTextGenerator this does not consult CapabilityDeclarer: +// the model list is advisory only, so a plain type assertion is sufficient and +// keeps the external-agent capability protocol unchanged. +func AsModelLister(ag Agent) (ModelLister, bool) { + if ag == nil { + return nil, false + } + ml, ok := ag.(ModelLister) + return ml, ok +} diff --git a/cmd/entire/cli/integration_test/review_test.go b/cmd/entire/cli/integration_test/review_test.go index 1a7f92bd7..1b310bb5b 100644 --- a/cmd/entire/cli/integration_test/review_test.go +++ b/cmd/entire/cli/integration_test/review_test.go @@ -112,10 +112,17 @@ func TestReviewCommand_PassesReviewEnvToSpawnedAgentHook(t *testing.T) { env := NewFeatureBranchEnv(t) enableReviewAgent(t, env, "claude-code") env.WriteSettings(map[string]any{ - "enabled": true, - "review": map[string]any{ - "claude-code": map[string]any{ - "skills": []string{"/review"}, + "enabled": true, + "review_default_profile": "general", + "review_profiles": map[string]any{ + "general": map[string]any{ + "task": "Test review task.", + "agents": map[string]any{ + "claude-code": map[string]any{ + "skills": []string{"/review"}, + }, + }, + "master": "claude-code", }, }, }) @@ -229,9 +236,16 @@ func TestReview_MissingSkillAtSpawn_ErrorsCleanly(t *testing.T) { enableReviewAgent(t, env, "claude-code") env.WriteSettings(map[string]any{ - "review": map[string]any{ - "claude-code": map[string]any{ - "skills": []string{"/nonexistent:skill-xyz"}, + "review_default_profile": "general", + "review_profiles": map[string]any{ + "general": map[string]any{ + "task": "Test review task.", + "agents": map[string]any{ + "claude-code": map[string]any{ + "skills": []string{"/nonexistent:skill-xyz"}, + }, + }, + "master": "claude-code", }, }, }) diff --git a/cmd/entire/cli/labs.go b/cmd/entire/cli/labs.go index d4f86cd67..386c5a024 100644 --- a/cmd/entire/cli/labs.go +++ b/cmd/entire/cli/labs.go @@ -17,7 +17,7 @@ var experimentalCommands = []experimentalCommandInfo{ { Name: "review", Invocation: "entire review", - Summary: "Run configured review skills against the current branch", + Summary: "Run a review profile against the current branch", }, { Name: "investigate", diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index f8852c176..3a0c12f7c 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -2,9 +2,9 @@ // // cmd.go provides NewCommand(), the cobra entry point for `entire review`. // It routes through the new AgentReviewer / Sink / Run architecture for -// launchable agents (claude-code, codex, gemini) and falls back to -// RunMarkerFallback for non-launchable agents (cursor, opencode, -// factoryai-droid, copilot-cli). +// agents with review-runner adapters (claude-code, codex, gemini) and falls +// back to RunMarkerFallback for agents that are not yet wired into that review +// runner contract. package review import ( @@ -14,6 +14,8 @@ import ( "io" "log/slog" "os" + "sort" + "strings" "charm.land/huh/v2" "github.com/spf13/cobra" @@ -43,14 +45,6 @@ type Deps struct { // NewSilentError wraps an error so the cobra root does not double-print it. NewSilentError func(err error) error - // PromptForAgentFn overrides the interactive agent picker. Nil means - // PromptForAgent is used (the real huh form). Tests inject a stub. - PromptForAgentFn func(ctx context.Context, eligible []AgentChoice) (string, error) - - // MultiPickerFn overrides PickAgents for the multi-agent picker. Nil - // means PickAgents is used (the real huh form). Tests inject a stub. - MultiPickerFn func(ctx context.Context, eligible []AgentChoice) (PickedAgents, error) - // HeadHasReviewCheckpoint checks whether HEAD's checkpoint metadata // includes a review session. Returns (true, infoString) if HasReview is set. // Injected to avoid an import cycle: review → checkpoint → codex → review. @@ -62,8 +56,8 @@ type Deps struct { ReviewCheckpointContext func(ctx context.Context, worktreeRoot string, scopeBaseRef string) string // ReviewerFor maps an agent registry name to its AgentReviewer - // implementation. Returns nil for non-launchable agents (cursor, opencode, - // factoryai-droid, copilot-cli). Injected to break the import cycle: + // implementation. Returns nil for agents that do not yet have a review-runner + // adapter. Injected to break the import cycle: // per-agent reviewer packages import review (for ComposeReviewPrompt / // AppendReviewEnv), so review/cmd.go cannot import them back. ReviewerFor func(agentName string) reviewtypes.AgentReviewer @@ -72,36 +66,26 @@ type Deps struct { // subcommand. Callers in the cli package pass newReviewAttachCmd() here; // tests pass nil to skip the subcommand. AttachCmd *cobra.Command - - // SynthesisProvider, when non-nil, enables the synthesis sink in TTY mode. - // Production wiring resolves the same provider entire explain uses. - // When nil, the synthesis sink is not appended and synthesis is unavailable. - SynthesisProvider SynthesisProvider - - // PromptYN overrides the y/N confirmation form used by SynthesisSink. - // Nil means the real huh form is used (realPromptYN in synthesis_sink.go). - // Tests inject a stub to avoid TTY interactions. - PromptYN func(ctx context.Context, question string, def bool) (bool, error) -} - -// runReviewDeps carries the subset of Deps that runReview itself reads -// directly (vs. NewCommand's wiring). Kept unexported so tests construct a -// Deps value at the package boundary; runReview unpacks the relevant fields. -type runReviewDeps struct { - promptForAgentFn func(ctx context.Context, eligible []AgentChoice) (string, error) - multiPickerFn func(ctx context.Context, eligible []AgentChoice) (PickedAgents, error) } // NewCommand returns the `entire review` cobra command wired with the // provided deps. Callers in the cli package pass a fully-populated Deps; // tests pass a Deps with stub fields. func NewCommand(deps Deps) *cobra.Command { + var configure bool var edit bool var agentOverride string + var modelOverride string var baseOverride string + var profileOverride string + var perRunPrompt string var findings bool - var fix bool - var all bool + var listModels bool + var listAgents bool + var setAgents []string + var setMaster string + var setTask string + var setModels []string cmd := &cobra.Command{ Use: "review", @@ -109,10 +93,11 @@ func NewCommand(deps Deps) *cobra.Command { // users who know about it can still run `entire review` / `entire // review --help` and the command works normally. Hidden: true, - Short: "Run configured review skills against the current branch", - Long: `Run configured review skills against the current branch. Review -preferences are loaded from Entire settings and clone-local preferences. On -first run, an interactive picker writes clone-local preferences. + Short: "Run a review profile against the current branch", + Long: `Run a named review profile against the current branch. Review +profiles are loaded from Entire settings and clone-local preferences. On +first run, simple guided setup writes clone-local preferences and asks before +starting agents. Labs entry: review is experimental. We are actively refining it based on user feedback. @@ -121,12 +106,21 @@ The review session is recorded as part of the next checkpoint, so the review metadata is permanently attached to the commit it covers. Flags: - --edit re-open the review config picker + --configure set up a review profile (shows available agents + profiles). + With --set-* flags it writes the profile non-interactively; + otherwise it opens the wizard (interactive) without starting agents. + --set-agents with --configure: comma-separated worker agents for the profile + --set-master with --configure: master agent that writes the final report + --set-task with --configure: the profile's canonical task text + --set-model with --configure: per-worker model as agent=model (repeatable) + --edit re-open the advanced review profile skill picker --findings browse local review findings - --fix apply review findings in a normal agent session - --all with --fix, apply all sources/findings without selectors - --agent NAME select a specific configured agent when more than one is - configured (default: alphabetically first) + --agent NAME run only one worker from the selected profile + --agents list the worker agents you can pass to --agent for the profile + --model NAME override the model for the --agent worker (requires --agent) + --models list the models each review agent advertises (optionally --agent NAME) + --profile NAME select a review profile (also accepted as positional arg) + --prompt TEXT add one-off per-run instructions for this invocation --base REF scope the review against REF instead of mainline. Useful for stacked PRs where the review base is the parent feature branch, not main. Default: first existing of origin/HEAD, @@ -137,10 +131,10 @@ Subcommands: 'entire attach --review ')`, Args: func(_ *cobra.Command, args []string) error { if len(args) > 1 { - return fmt.Errorf("accepts at most one review session id, received %d", len(args)) + return fmt.Errorf("accepts at most one argument, received %d", len(args)) } - if len(args) == 1 && !fix { - return errors.New("review session id is only valid with --fix") + if len(args) == 1 && profileOverride != "" { + return errors.New("pass profile either positionally or with --profile, not both") } return nil }, @@ -151,60 +145,64 @@ Subcommands: // and agent.Get can't see them. external.DiscoverAndRegister(ctx) - if all && !fix { - return errors.New("--all requires --fix") + if listModels { + return runReviewListModels(ctx, cmd, agentOverride, deps) + } + if listAgents { + listProfile := profileOverride + if len(args) == 1 { + listProfile = args[0] + } + return runReviewListAgents(ctx, cmd, listProfile, deps) } + modes := 0 - for _, enabled := range []bool{edit, findings, fix} { + for _, enabled := range []bool{configure, edit, findings} { if enabled { modes++ } } if modes > 1 { - return errors.New("--edit, --findings, and --fix are mutually exclusive") + return errors.New("--configure, --edit, and --findings are mutually exclusive") } - // The migration prompt is only relevant for flows that write or - // read picker config (--edit and the default review run). - // --findings (read-only browsing) and --fix (uses - // ReviewFixAgent only) don't interact with the picker, so - // prompting in those paths interrupts the user for no reason. - if !findings && !fix { - if err := maybePromptReviewSettingsMigration( - ctx, - cmd.OutOrStdout(), - cmd.ErrOrStderr(), - interactive.IsTerminalWriter(cmd.OutOrStdout()) && interactive.CanPromptInteractively(), - deps.PromptYN, - ); err != nil { - return err - } + if modelOverride != "" && agentOverride == "" { + return errors.New("--model requires --agent (the model applies to a single worker)") + } + profileName := profileOverride + if len(args) == 1 { + profileName = args[0] + } + if configure { + return runReviewConfigure(ctx, cmd, profileName, reviewConfigureOptions{ + Agents: setAgents, + Master: setMaster, + Task: setTask, + Models: setModels, + }, deps) } if edit { - _, err := RunReviewConfigPicker(ctx, cmd.OutOrStdout(), deps.GetAgentsWithHooksInstalled) + _, err := RunReviewProfileConfigPicker(ctx, cmd.OutOrStdout(), deps.GetAgentsWithHooksInstalled, profileName) return err } if findings { return runReviewFindings(ctx, cmd, deps.NewSilentError) } - if fix { - target := "" - if len(args) == 1 { - target = args[0] - } - return runReviewFix(ctx, cmd, target, all, agentOverride, deps.NewSilentError) - } - innerDeps := runReviewDeps{ - promptForAgentFn: deps.PromptForAgentFn, - multiPickerFn: deps.MultiPickerFn, - } - return runReview(ctx, cmd, agentOverride, baseOverride, deps, innerDeps) + return runReview(ctx, cmd, agentOverride, modelOverride, baseOverride, profileName, perRunPrompt, deps) }, } - cmd.Flags().BoolVar(&edit, "edit", false, "re-open the review config picker") + cmd.Flags().BoolVar(&configure, "configure", false, "set up a review profile; shows available agents and accepts --set-* flags for non-interactive config") + cmd.Flags().StringSliceVar(&setAgents, "set-agents", nil, "with --configure: worker agents for the profile (comma-separated)") + cmd.Flags().StringVar(&setMaster, "set-master", "", "with --configure: master agent that writes the final report") + cmd.Flags().StringVar(&setTask, "set-task", "", "with --configure: the profile's canonical task text") + cmd.Flags().StringArrayVar(&setModels, "set-model", nil, "with --configure: per-worker model as agent=model (repeatable)") + cmd.Flags().BoolVar(&edit, "edit", false, "re-open the advanced review profile skill picker") cmd.Flags().BoolVar(&findings, "findings", false, "browse local review findings") - cmd.Flags().BoolVar(&fix, "fix", false, "apply review findings in a normal agent session") - cmd.Flags().BoolVar(&all, "all", false, "with --fix, apply all sources/findings without selectors") - cmd.Flags().StringVar(&agentOverride, "agent", "", "select a specific configured agent (default: alphabetically first)") + cmd.Flags().BoolVar(&listAgents, "agents", false, "list the worker agents you can pass to --agent for the selected profile") + cmd.Flags().BoolVar(&listModels, "models", false, "list the models each review agent advertises (optionally filtered by --agent)") + cmd.Flags().StringVar(&agentOverride, "agent", "", "run one configured worker from the selected profile") + cmd.Flags().StringVar(&modelOverride, "model", "", "override the model for the --agent worker (requires --agent)") + cmd.Flags().StringVar(&profileOverride, "profile", "", "review profile to run (default: review_default_profile or general)") + cmd.Flags().StringVar(&perRunPrompt, "prompt", "", "one-off instructions appended to this review run") cmd.Flags().StringVar(&baseOverride, "base", "", "git ref to scope the review against (default: origin/HEAD → origin/main → origin/master → main → master)") if deps.AttachCmd != nil { cmd.AddCommand(deps.AttachCmd) @@ -212,8 +210,348 @@ Subcommands: return cmd } +// reviewConfigureOptions carries the non-interactive `--configure` inputs. +type reviewConfigureOptions struct { + Agents []string // worker agent names (--set-agents) + Master string // master agent (--set-master) + Task string // profile task text (--set-task) + Models []string // per-worker "agent=model" entries (--set-model) +} + +func (o reviewConfigureOptions) scripted() bool { + return len(o.Agents) > 0 || o.Master != "" || o.Task != "" || len(o.Models) > 0 +} + +func runReviewConfigure(ctx context.Context, cmd *cobra.Command, profileOverride string, opts reviewConfigureOptions, deps Deps) error { + out := cmd.OutOrStdout() + silentErr := deps.NewSilentError + if _, err := paths.WorktreeRoot(ctx); err != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "Not a git repository. Run `entire enable` first.") + return silentErr(errors.New("not a git repository")) + } + s, err := settings.Load(ctx) + if err != nil { + cmd.SilenceUsage = true + fmt.Fprintf(cmd.ErrOrStderr(), "Failed to load settings: %v\n", err) + return silentErr(err) + } + if s == nil { + s = &settings.EntireSettings{} + } + profileName := strings.TrimSpace(profileOverride) + if profileName == "" { + profileName = strings.TrimSpace(s.ReviewDefaultProfile) + } + if profileName == "" { + profileName = DefaultProfileName + } + installed := deps.GetAgentsWithHooksInstalled(ctx) + + // Scripted path: build + save the profile from --set-* flags, no TUI. + if opts.scripted() { + profile, buildErr := buildConfiguredProfile(ctx, profileName, opts, s, deps) + if buildErr != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), buildErr.Error()) + return silentErr(buildErr) + } + if err := saveReviewProfile(ctx, profileName, profile, true); err != nil { + return err + } + fmt.Fprintf(out, "Review profile %q saved with %s.\n", profileName, strings.Join(sortedProfileAgentNames(profile), ", ")) + fmt.Fprintf(out, "Run `entire review %s` to start.\n", profileName) + return nil + } + + // Interactive path: the guided wizard already lists the agents, so don't + // duplicate the catalog here. + if interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively() { + name, profile, setupErr := RunReviewGuidedSetup(ctx, out, installed, deps.ReviewerFor, profileName, false) + if setupErr != nil { + return handlePickerError(cmd, silentErr, setupErr) + } + if err := saveReviewProfile(ctx, name, profile, true); err != nil { + return err + } + fmt.Fprintf(out, "Review profile %q saved. Run `entire review`, or `entire review %s`, to start.\n", name, name) + return nil + } + + // Non-interactive with no --set-* flags: this is the discovery view — show + // the available agents, current profiles, and how to configure. + catalog := availableReviewAgents(installed, deps.ReviewerFor) + printReviewConfigCatalog(out, profileName, catalog, s) + return nil +} + +// runReviewListModels prints the models each review-runner agent advertises +// (claude-code, codex, gemini, ...). It needs no git repo or profile: model +// lists are advisory metadata. With agentFilter set, only that agent is shown. +func runReviewListModels(ctx context.Context, cmd *cobra.Command, agentFilter string, deps Deps) error { + out := cmd.OutOrStdout() + installed := deps.GetAgentsWithHooksInstalled(ctx) + catalog := availableReviewAgents(installed, deps.ReviewerFor) + + if agentFilter != "" { + filtered := make([]reviewAgentCatalogEntry, 0, 1) + for _, e := range catalog { + if e.Name == agentFilter { + filtered = append(filtered, e) + } + } + if len(filtered) == 0 { + cmd.SilenceUsage = true + err := fmt.Errorf("agent %q has no review runner adapter; available: %s", agentFilter, strings.Join(reviewAgentNames(deps), ", ")) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return deps.NewSilentError(err) + } + catalog = filtered + } + + for _, e := range catalog { + fmt.Fprintf(out, "%s:\n", e.Name) + ag, getErr := agent.Get(types.AgentName(e.Name)) + if getErr != nil { + fmt.Fprintln(out, " (agent unavailable)") + continue + } + lister, ok := agent.AsModelLister(ag) + if !ok { + fmt.Fprintln(out, " (no advertised models; pass any value your CLI accepts via --model)") + continue + } + models, listErr := lister.ListModels(ctx) + if listErr != nil || len(models) == 0 { + fmt.Fprintln(out, " (model list unavailable)") + continue + } + for _, m := range models { + if m.Note != "" { + fmt.Fprintf(out, " %-18s %s\n", m.ID, m.Note) + } else { + fmt.Fprintf(out, " %s\n", m.ID) + } + } + } + + fmt.Fprintln(out) + fmt.Fprintln(out, "These are common models/aliases, not an exhaustive list. Use one with:") + fmt.Fprintln(out, " entire review --agent --model ") + return nil +} + +const reviewHooksInstalledStatus = "hooks installed" + +// runReviewListAgents lists the worker agents valid for `--agent` in the +// resolved profile (with hook-install status and the master marked). With no +// usable profile it falls back to the available review-agent catalog. +func runReviewListAgents(ctx context.Context, cmd *cobra.Command, profileOverride string, deps Deps) error { + out := cmd.OutOrStdout() + installed := deps.GetAgentsWithHooksInstalled(ctx) + installedSet := make(map[string]struct{}, len(installed)) + for _, n := range installed { + installedSet[string(n)] = struct{}{} + } + + s, err := settings.Load(ctx) + if err == nil && s != nil { + if name, profile, selErr := selectReviewProfile(s, profileOverride); selErr == nil { + profile.Agents = nonZeroAgentConfigs(profile.Agents) + fmt.Fprintf(out, "Workers in review profile %q (pass one to --agent):\n", name) + for _, worker := range sortedProfileAgentNames(profile) { + cfg := profile.Agents[worker] + status := reviewHooksInstalledStatus + if _, ok := installedSet[reviewAgentName(worker, cfg)]; !ok { + status = "hooks NOT installed — run `entire configure --agent " + reviewAgentName(worker, cfg) + "`" + } + marker := "" + if worker == profile.Master { + marker = " [master]" + } + fmt.Fprintf(out, " %s — %s%s\n", reviewWorkerLabel(worker, cfg), status, marker) + } + fmt.Fprintln(out) + fmt.Fprintln(out, "See all available agents and profiles with `entire review --configure`.") + return nil + } + } + + // No usable profile: show the catalog of available review agents instead. + catalog := availableReviewAgents(installed, deps.ReviewerFor) + fmt.Fprintln(out, "No review profile configured yet. Available review agents:") + for _, e := range catalog { + status := "not installed — run `entire configure --agent " + e.Name + "`" + if e.Installed { + status = reviewHooksInstalledStatus + } + fmt.Fprintf(out, " %-14s %s\n", e.Name, status) + } + fmt.Fprintln(out) + fmt.Fprintln(out, "Configure a profile with `entire review --configure`.") + return nil +} + +// reviewAgentCatalogEntry is one row in the `--configure` discovery listing. +type reviewAgentCatalogEntry struct { + Name string + Installed bool +} + +// availableReviewAgents lists every registered agent that has a review-runner +// adapter (claude-code, codex, gemini, pi, ...), marking which have hooks +// installed in this repo. Derived from the registry + deps.ReviewerFor so it +// never drifts from the set of agents `entire review` can actually launch. +func availableReviewAgents(installed []types.AgentName, reviewerFor func(string) reviewtypes.AgentReviewer) []reviewAgentCatalogEntry { + installedSet := make(map[string]struct{}, len(installed)) + for _, n := range installed { + installedSet[string(n)] = struct{}{} + } + var out []reviewAgentCatalogEntry + for _, name := range agent.List() { + ns := string(name) + if reviewerFor(ns) == nil { + continue + } + _, ok := installedSet[ns] + out = append(out, reviewAgentCatalogEntry{Name: ns, Installed: ok}) + } + return out +} + +func printReviewConfigCatalog(out io.Writer, profileName string, catalog []reviewAgentCatalogEntry, s *settings.EntireSettings) { + fmt.Fprintln(out, "Available review agents:") + if len(catalog) == 0 { + fmt.Fprintln(out, " (none — install one with `entire configure --agent claude-code`)") + } + for _, e := range catalog { + status := "not installed — run `entire configure --agent " + e.Name + "`" + if e.Installed { + status = reviewHooksInstalledStatus + } + fmt.Fprintf(out, " %-14s %s\n", e.Name, status) + } + + fmt.Fprintln(out) + profiles := nonZeroProfiles(s.ReviewProfiles) + if len(profiles) == 0 { + fmt.Fprintln(out, "Configured profiles: (none yet)") + } else { + fmt.Fprintln(out, "Configured profiles:") + for _, name := range sortedProfileNames(profiles) { + p := profiles[name] + marker := "" + if name == strings.TrimSpace(s.ReviewDefaultProfile) { + marker = " (default)" + } + line := fmt.Sprintf(" %s%s: %s", name, marker, strings.Join(sortedProfileAgentNames(p), ", ")) + if strings.TrimSpace(p.Master) != "" { + line += " master=" + p.Master + } + fmt.Fprintln(out, line) + } + } + + fmt.Fprintln(out) + fmt.Fprintf(out, "Configure %q non-interactively, e.g.:\n", profileName) + fmt.Fprintf(out, " entire review --configure --profile %s --set-agents %s --set-master \n", + profileName, exampleAgentList(catalog)) +} + +func exampleAgentList(catalog []reviewAgentCatalogEntry) string { + names := make([]string, 0, len(catalog)) + for _, e := range catalog { + if e.Installed { + names = append(names, e.Name) + } + } + if len(names) == 0 { + return "claude-code,codex" + } + if len(names) > 2 { + names = names[:2] + } + return strings.Join(names, ",") +} + +// buildConfiguredProfile produces a ReviewProfileConfig from --set-* flags, +// merging onto any existing profile so unspecified profile-level fields +// (task, master_model) are preserved. +func buildConfiguredProfile(ctx context.Context, profileName string, opts reviewConfigureOptions, s *settings.EntireSettings, deps Deps) (settings.ReviewProfileConfig, error) { + profile := s.ReviewProfiles[profileName] + + if len(opts.Agents) > 0 { + agents := make(map[string]settings.ReviewConfig, len(opts.Agents)) + for _, raw := range opts.Agents { + name := strings.TrimSpace(raw) + if name == "" { + continue + } + if deps.ReviewerFor(name) == nil { + return settings.ReviewProfileConfig{}, fmt.Errorf("agent %q has no review runner adapter; available: %s", name, strings.Join(reviewAgentNames(deps), ", ")) + } + agents[name] = defaultReviewAgentConfig(profileName, name) + } + if len(agents) == 0 { + return settings.ReviewProfileConfig{}, errors.New("--set-agents listed no usable agents") + } + profile.Agents = agents + } + if len(nonZeroAgentConfigs(profile.Agents)) == 0 { + return settings.ReviewProfileConfig{}, errors.New("profile has no agents; pass --set-agents") + } + + for _, raw := range opts.Models { + key, model, ok := strings.Cut(raw, "=") + key = strings.TrimSpace(key) + model = strings.TrimSpace(model) + if !ok || key == "" { + return settings.ReviewProfileConfig{}, fmt.Errorf("invalid --set-model %q; expected agent=model", raw) + } + workerName, _, selErr := selectProfileWorker(profile, key) + if selErr != nil { + return settings.ReviewProfileConfig{}, fmt.Errorf("--set-model %q: %w", raw, selErr) + } + cfg := profile.Agents[workerName] + cfg.Model = model + profile.Agents[workerName] = cfg + } + + if opts.Task != "" { + profile.Task = opts.Task + } + if strings.TrimSpace(profile.Task) == "" { + profile.Task = profileTask(profileName, settings.ReviewProfileConfig{}) + } + + if opts.Master != "" { + profile.Master = opts.Master + } + if len(nonZeroAgentConfigs(profile.Agents)) > 1 { + if strings.TrimSpace(profile.Master) == "" { + profile.Master = defaultReviewMaster(ctx, profile.Agents) + } + if _, _, masterErr := selectProfileWorker(profile, profile.Master); masterErr != nil { + return settings.ReviewProfileConfig{}, fmt.Errorf("master %q is not one of the profile workers (%s)", profile.Master, strings.Join(sortedProfileAgentNames(profile), ", ")) + } + } else { + profile.Master = "" + } + return profile, nil +} + +func reviewAgentNames(deps Deps) []string { + var names []string + for _, name := range agent.List() { + if deps.ReviewerFor(string(name)) != nil { + names = append(names, string(name)) + } + } + return names +} + // runReview executes the main review flow. -func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverride string, deps Deps, innerDeps runReviewDeps) error { +func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOverride, baseOverride, profileOverride, perRunPrompt string, deps Deps) error { out := cmd.OutOrStdout() silentErr := deps.NewSilentError @@ -237,88 +575,177 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverr "Fix your Entire settings or clone-local review preferences and re-run `entire review`.") return silentErr(err) } - if s == nil || len(s.Review) == 0 { - if !ConfirmFirstRunSetup(ctx, out) { - return nil - } - picked, pickErr := RunReviewConfigPicker(ctx, out, deps.GetAgentsWithHooksInstalled) - if pickErr != nil { - return pickErr - } - if s == nil { - s = &settings.EntireSettings{} - } - s.Review = picked - fmt.Fprintln(out) - fmt.Fprintln(out, "Setup complete — running review now.") - } - - // 3. Resolve installed agents and determine the dispatch path. - // - // Three paths: - // - Multi-agent: 2+ launchable eligible agents AND no --agent override → - // show multi-select picker then RunMulti. Steps 3.5, 3.6, and the - // single-agent skill-verify guard are skipped; each reviewer pulls - // its own skills from settings at spawn time via RunConfig. - // - Single-agent (default): 1 or fewer launchable eligible agents, OR - // --agent override set. Falls through to the full agent-selection and - // validation path below (steps 3–3.6). installed := deps.GetAgentsWithHooksInstalled(ctx) - if agentOverride == "" { - launchableEligible := computeLaunchableEligible(s, installed, deps.ReviewerFor) - if len(launchableEligible) >= 2 { - return runMultiAgentPath(ctx, cmd, launchableEligible, baseOverride, s, innerDeps, deps, out) - } - } - - // Single-agent path: pick agent, verify hooks + skills, scope, run. - - // 3a. Base selection on the eligible set (configured AND installed): - // - 0 eligible: fall through; SelectReviewAgent below errors with the - // full configured map (clearer "no installed agent" diagnostic than - // a silent fail). - // - 1 eligible: use it directly. This matters when the alphabetically- - // first configured agent isn't installed but exactly one other is — - // without this, SelectReviewAgent would default to the alphabetical - // first and the verify-hooks check below would error needlessly. - // - 2+ eligible: prompt with single-select (non-launchable agents reach - // this branch since computeLaunchableEligible filtered them out above). - if agentOverride == "" { - eligible := ComputeEligibleConfigured(s, installed) - switch { - case len(eligible) == 1: - agentOverride = eligible[0].Name - case len(eligible) > 1: - fn := innerDeps.promptForAgentFn - if fn == nil { - fn = PromptForAgent + if s == nil { + s = &settings.EntireSettings{} + } + // Trigger first-run setup when no usable profile exists. Counting only + // non-zero profiles means a placeholder/empty entry (e.g. an empty + // `general` profile in a hand-edited preferences file) still routes through + // guided setup / the non-interactive default instead of dead-ending later in + // selectReviewProfile with "every profile is empty". + if len(nonZeroProfiles(s.ReviewProfiles)) == 0 { + profileForSetup := profileOverride + var profile settings.ReviewProfileConfig + guidedSetup := interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively() + if guidedSetup { + var setupErr error + profileForSetup, profile, setupErr = RunReviewGuidedSetup(ctx, out, installed, deps.ReviewerFor, profileForSetup, true) + if setupErr != nil { + return handlePickerError(cmd, silentErr, setupErr) } - picked, pickErr := fn(ctx, eligible) - if pickErr != nil { - cmd.SilenceUsage = true - fmt.Fprintln(cmd.ErrOrStderr(), pickErr.Error()) - return silentErr(pickErr) + } else { + if profileForSetup == "" { + profileForSetup = DefaultProfileName } - if picked == "" { - // Defensive: empty picker return must not fall through to - // alphabetical-first default. + defaultProfile, defaultErr := defaultReviewProfileForInstalledAgents(ctx, profileForSetup, installed, deps.ReviewerFor) + if defaultErr != nil { cmd.SilenceUsage = true - emptyErr := errors.New("agent picker returned empty agent name") - fmt.Fprintln(cmd.ErrOrStderr(), emptyErr.Error()) - return silentErr(emptyErr) + fmt.Fprintln(cmd.ErrOrStderr(), defaultErr.Error()) + return silentErr(defaultErr) + } + profile = defaultProfile + fmt.Fprintf(out, "No review profiles found — using default %q profile with %s.\n", profileForSetup, strings.Join(sortedProfileAgentNames(profile), ", ")) + fmt.Fprintln(out, "Configure later with `entire review --configure`.") + fmt.Fprintln(out) + } + if saveErr := saveDefaultReviewProfile(ctx, profileForSetup, profile); saveErr != nil { + return saveErr + } + s.ReviewProfiles = map[string]settings.ReviewProfileConfig{profileForSetup: profile} + s.ReviewDefaultProfile = profileForSetup + if guidedSetup { + runNow, confirmErr := ConfirmRunReviewNow(ctx, out) + if confirmErr != nil { + return handlePickerError(cmd, silentErr, confirmErr) + } + if !runNow { + return nil } - agentOverride = picked + fmt.Fprintln(out) } } - agentName, cfg, err := SelectReviewAgent(s.Review, agentOverride) + profileName, profile, err := selectReviewProfile(s, profileOverride) if err != nil { cmd.SilenceUsage = true fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) return silentErr(err) } + profile.Task = profileTask(profileName, profile) + profile.Agents = nonZeroAgentConfigs(profile.Agents) + + if agentOverride != "" { + workerName, cfg, selectErr := selectProfileWorker(profile, agentOverride) + if selectErr != nil { + cmd.SilenceUsage = true + err := fmt.Errorf("%w in review profile %q", selectErr, profileName) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } + if modelOverride != "" { + cfg.Model = modelOverride + } + return runSingleAgentPath(ctx, cmd, profileName, workerName, baseOverride, perRunPrompt, profile.Task, cfg, installed, deps, out) + } + + if missing := missingInstalledProfileAgents(profile.Agents, installed); len(missing) > 0 { + cmd.SilenceUsage = true + err := fmt.Errorf("hooks are not installed for review profile %q agent(s): %s; run `entire configure --agent ` first, or edit the profile", profileName, strings.Join(missing, ", ")) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } - return runSingleAgentPath(ctx, cmd, agentName, baseOverride, cfg, installed, deps, out) + eligible := ComputeEligibleConfiguredForProfile(profile, installed) + switch len(eligible) { + case 0: + cmd.SilenceUsage = true + err := fmt.Errorf("review profile %q has no eligible agents", profileName) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + case 1: + cfg := profile.Agents[eligible[0].Name] + return runSingleAgentPath(ctx, cmd, profileName, eligible[0].Name, baseOverride, perRunPrompt, profile.Task, cfg, installed, deps, out) + default: + launchableEligible := computeLaunchableEligibleForProfile(profile, installed, deps.ReviewerFor) + if len(launchableEligible) != len(eligible) { + nonLaunchable := nonLaunchableEligibleNames(profile, eligible, deps.ReviewerFor) + cmd.SilenceUsage = true + err := fmt.Errorf("review profile %q includes agent(s) without review runner adapters in a fan-out run: %s. Use --agent for a single manual fallback, or remove them from the profile", profileName, strings.Join(nonLaunchable, ", ")) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } + if strings.TrimSpace(profile.Master) == "" { + cmd.SilenceUsage = true + err := fmt.Errorf("review profile %q has multiple workers but no master; set review_profiles.%s.master", profileName, profileName) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } + if _, _, masterErr := selectProfileWorker(profile, profile.Master); masterErr != nil { + cmd.SilenceUsage = true + err := fmt.Errorf("review profile %q master is invalid: %w", profileName, masterErr) + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) + } + return runMultiAgentPath(ctx, cmd, profileName, profile, launchableEligible, baseOverride, perRunPrompt, deps, out) + } +} + +func missingInstalledProfileAgents(configured map[string]settings.ReviewConfig, installed []types.AgentName) []string { + installedSet := make(map[string]struct{}, len(installed)) + for _, name := range installed { + installedSet[string(name)] = struct{}{} + } + var missing []string + for name, cfg := range configured { + if cfg.IsZero() { + continue + } + agentName := reviewAgentName(name, cfg) + if _, ok := installedSet[agentName]; !ok { + missing = append(missing, reviewWorkerLabel(name, cfg)) + } + } + sort.Strings(missing) + return missing +} + +func nonLaunchableEligibleNames(profile settings.ReviewProfileConfig, eligible []AgentChoice, reviewerFor func(string) reviewtypes.AgentReviewer) []string { + var out []string + for _, c := range eligible { + cfg := profile.Agents[c.Name] + if reviewerFor(reviewAgentName(c.Name, cfg)) == nil { + out = append(out, reviewWorkerLabel(c.Name, cfg)) + } + } + sort.Strings(out) + return out +} + +// confirmReReviewOrProceed implements the "HEAD already reviewed" guard. +// It returns (proceed, err). When the checkpoint has no prior review it returns +// (true, nil). In a non-interactive context it cannot prompt, so it proceeds +// (the user explicitly invoked `entire review`) after printing a note rather +// than blocking on a confirm form that would error out. +func confirmReReviewOrProceed(ctx context.Context, out io.Writer, deps Deps) (bool, error) { + reviewed, meta := deps.HeadHasReviewCheckpoint(ctx) + if !reviewed { + return true, nil + } + if !interactive.CanPromptInteractively() { + fmt.Fprintf(out, "Note: HEAD was already reviewed (%s); re-running.\n", meta) + return true, nil + } + var proceed bool + form := newAccessibleForm(huh.NewGroup( + huh.NewConfirm(). + Title(fmt.Sprintf("Already reviewed: %s. Proceed anyway?", meta)). + Value(&proceed), + )) + if err := form.RunWithContext(ctx); err != nil { + return false, err //nolint:wrapcheck // propagate huh cancellation + } + return proceed, nil } // runSingleAgentPath completes a single-agent review: verifies hooks + skills, @@ -327,13 +754,15 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverr func runSingleAgentPath( ctx context.Context, cmd *cobra.Command, - agentName, baseOverride string, + profileName, workerName, baseOverride, perRunPrompt, task string, cfg settings.ReviewConfig, installed []types.AgentName, deps Deps, out io.Writer, ) error { silentErr := deps.NewSilentError + agentName := reviewAgentName(workerName, cfg) + displayName := reviewWorkerLabel(workerName, cfg) // 3.5. Verify hooks are installed for the selected agent. found := false @@ -348,7 +777,7 @@ func runSingleAgentPath( fmt.Fprintf(cmd.ErrOrStderr(), "Hooks are not installed for %q. Run `entire configure --agent %s` first, "+ "or remove %q from review settings.\n", - agentName, agentName, agentName) + agentName, agentName, displayName) return silentErr(fmt.Errorf("hooks not installed for %s", agentName)) } @@ -364,21 +793,12 @@ func runSingleAgentPath( } // 4. Re-run guard: check if HEAD's checkpoint already has a review. - if reviewed, meta := deps.HeadHasReviewCheckpoint(ctx); reviewed { - var proceed bool - form := newAccessibleForm(huh.NewGroup( - huh.NewConfirm(). - Title(fmt.Sprintf("Already reviewed: %s. Proceed anyway?", meta)). - Value(&proceed), - )) - if err := form.RunWithContext(ctx); err != nil { - fmt.Fprintln(out, "prompt cancelled") - return err //nolint:wrapcheck // propagate huh cancellation - } - if !proceed { - fmt.Fprintln(out, "Review cancelled.") - return nil - } + if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps); guardErr != nil { + fmt.Fprintln(out, "prompt cancelled") + return guardErr + } else if !proceed { + fmt.Fprintln(out, "Review cancelled.") + return nil } // 5. Resolve HEAD SHA and worktree root. @@ -405,6 +825,9 @@ func runSingleAgentPath( } runCfg := reviewtypes.RunConfig{ + ProfileName: profileName, + Task: task, + PerRunPrompt: perRunPrompt, ScopeBaseRef: scopeBaseRef, CheckpointContext: checkpointContext, StartingSHA: headSHA, @@ -414,9 +837,10 @@ func runSingleAgentPath( // 7. Branch on launchability. reviewer := deps.ReviewerFor(agentName) if reviewer == nil { - // Non-launchable: write marker (with scope-aware prompt) and print guidance. + // No review runner adapter yet: write marker (with scope-aware prompt) and print guidance. return RunMarkerFallback(ctx, agentName, runCfg, worktreeRoot, out) } + reviewer = &perAgentConfiguredReviewer{name: displayName, inner: reviewer, cfg: runCfg} runCtx, cancelRun := context.WithCancel(ctx) defer cancelRun() @@ -427,7 +851,7 @@ func runSingleAgentPath( out: out, isTTY: interactive.IsTerminalWriter(out) && canPrompt, canPrompt: canPrompt, - agentName: agentName, + agentName: displayName, cancelRun: cancelRun, }) if tuiSink, ok := findTUISink(sinks); ok { @@ -481,40 +905,20 @@ func detectScope(ctx context.Context, worktreeRoot, baseOverride string, out io. return stats.BaseRef, nil } -// runMultiAgentPath handles the multi-agent review flow: shows the multi-select -// picker, collects an optional per-run prompt, builds per-agent RunConfigs, -// then runs all selected agents concurrently via RunMulti. -// -// This path skips the single-agent validation steps (3.5 hooks, 3.6 skills, -// re-run guard) for brevity — computeLaunchableEligible has already ensured -// each eligible agent has hooks installed and a Reviewer available. +// runMultiAgentPath handles the profile-native fan-out flow. Every configured +// worker in the selected profile runs concurrently against the same canonical +// task, then the profile's master agent produces the final report. func runMultiAgentPath( ctx context.Context, cmd *cobra.Command, + profileName string, + profile settings.ReviewProfileConfig, launchableEligible []AgentChoice, baseOverride string, - s *settings.EntireSettings, - innerDeps runReviewDeps, + perRunPrompt string, deps Deps, out io.Writer, ) error { - // Note: skill verification is intentionally skipped here. The - // computeLaunchableEligible filter in the dispatch fork already - // guarantees every agent in launchableEligible has hooks installed - // AND a non-nil ReviewerFor mapping, so a per-agent verify pass would - // be redundant. - silentErr := deps.NewSilentError - - // Show multi-select picker (or use injected stub in tests). - pickerFn := innerDeps.multiPickerFn - if pickerFn == nil { - pickerFn = PickAgents - } - picked, pickErr := pickerFn(ctx, launchableEligible) - if pickErr != nil { - return handlePickerError(cmd, silentErr, pickErr) - } - // Resolve worktree root and HEAD SHA for scope detection. worktreeRoot, err := paths.WorktreeRoot(ctx) if err != nil { @@ -527,6 +931,14 @@ func runMultiAgentPath( return fmt.Errorf("resolve HEAD: %w", shaErr) } + if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps); guardErr != nil { + fmt.Fprintln(out, "prompt cancelled") + return guardErr + } else if !proceed { + fmt.Fprintln(out, "Review cancelled.") + return nil + } + scopeBaseRef, scopeErr := detectScope(ctx, worktreeRoot, baseOverride, out) if scopeErr != nil { cmd.SilenceUsage = true @@ -536,28 +948,34 @@ func runMultiAgentPath( if deps.ReviewCheckpointContext != nil { checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) } - - // Build per-agent reviewers with individual RunConfigs (each agent has - // its own skills + always-prompt from s.Review[name]). - reviewers := make([]reviewtypes.AgentReviewer, 0, len(picked.Names)) - for _, name := range picked.Names { - agentCfg := s.Review[name] // zero value is safe (empty skills/prompt) - reviewer := deps.ReviewerFor(name) + reviewers := make([]reviewtypes.AgentReviewer, 0, len(launchableEligible)) + for _, choice := range launchableEligible { + workerName := choice.Name + agentCfg := profile.Agents[workerName] + agentName := reviewAgentName(workerName, agentCfg) + if len(agentCfg.Skills) > 0 { + ag, agErr := agent.Get(types.AgentName(agentName)) + if agErr != nil { + return fmt.Errorf("resolve agent %s: %w", agentName, agErr) + } + if err := VerifyConfiguredSkillsInstalled(ctx, ag, agentCfg); err != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return deps.NewSilentError(err) + } + } + reviewer := deps.ReviewerFor(agentName) if reviewer == nil { - // Shouldn't happen given launchableEligible was filtered for - // ReviewerFor != nil, but be defensive. cmd.SilenceUsage = true - return silentErr(fmt.Errorf("agent %q is not launchable but appeared in eligible list", name)) + return deps.NewSilentError(fmt.Errorf("agent %q has no review runner adapter but appeared in eligible list", agentName)) } - // Wrap the reviewer so it sees the per-agent RunConfig at Start time. - // We cannot pass a different RunConfig per reviewer in RunMulti's - // current API (all reviewers share one RunConfig). Instead, build a - // configuredReviewer adapter that injects per-agent skills into - // RunConfig before forwarding to the underlying reviewer. reviewers = append(reviewers, &perAgentConfiguredReviewer{ + name: reviewWorkerLabel(workerName, agentCfg), inner: reviewer, cfg: runConfigWithReviewConfig(reviewtypes.RunConfig{ - PerRunPrompt: picked.PerRun, + ProfileName: profileName, + Task: profile.Task, + PerRunPrompt: perRunPrompt, ScopeBaseRef: scopeBaseRef, CheckpointContext: checkpointContext, StartingSHA: headSHA, @@ -565,14 +983,6 @@ func runMultiAgentPath( }) } - // Compose sinks based on TTY detection. - // TTY mode: [TUISink, DumpSink] — TUI owns the live dashboard; DumpSink - // renders the post-run narrative after TUI dismisses (RunFinished is called - // on each sink in order, and TUISink.RunFinished blocks until user dismisses). - // Non-TTY mode: [DumpSink] alone. - // - // A derived context is used so the TUI's Ctrl+C handler can cancel the run - // via the same cancelRun function that the orchestrator's context is built on. runCtx, cancelRun := context.WithCancel(ctx) defer cancelRun() @@ -582,12 +992,13 @@ func runMultiAgentPath( } aggregateOutput := "" - // TUI requires both: - // - terminal stdout (otherwise ANSI codes corrupt redirected output) - // - a promptable stdin (otherwise the post-run dismissal loop blocks - // forever — happens when entire review is invoked from inside an - // agent like Claude Code or Gemini CLI, where stdout is a TTY but - // keypresses are never delivered) + masterWorkerName, masterCfg, masterErr := selectProfileWorker(profile, profile.Master) + masterLabel := profile.Master + if masterErr == nil { + masterLabel = reviewWorkerLabel(masterWorkerName, masterCfg) + } + masterAgentName, masterModel := resolveProfileMaster(profile) + masterProvider := AgentSynthesisProvider{AgentName: masterAgentName, Model: masterModel} sinks := composeMultiAgentSinks(multiAgentSinkInputs{ out: out, isTTY: interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively(), @@ -595,9 +1006,12 @@ func runMultiAgentPath( agentNames: agentNames, cancelRun: cancelRun, runContext: runCtx, - synthesisProvider: deps.SynthesisProvider, - promptYN: deps.PromptYN, - perRunPrompt: picked.PerRun, + synthesisProvider: masterProvider, + perRunPrompt: perRunPrompt, + profileName: profileName, + task: profile.Task, + masterName: masterLabel, + autoSynthesis: true, onSynthesisResult: func(result string) { aggregateOutput = result }, @@ -607,11 +1021,6 @@ func runMultiAgentPath( defer tuiSink.Wait() } - // Multi-agent only wires EnrichAgentRun. The per-agent enricher emits a - // synthetic Tokens event as each agent finishes, which the dispatch loop - // overwrites onto st.tokens (run_multi.go:168). That value flows into - // agentRuns[i].Tokens in the final summary, so a summary-level pass would - // redo the same store.List + token hydration once per run. summary, waitErr := RunMulti(runCtx, reviewers, reviewtypes.RunConfig{ EnrichAgentRun: reviewAgentRunTokenEnricher(worktreeRoot, headSHA), }, sinks) @@ -656,6 +1065,10 @@ type multiAgentSinkInputs struct { synthesisProvider SynthesisProvider promptYN func(ctx context.Context, question string, def bool) (bool, error) perRunPrompt string + profileName string + task string + masterName string + autoSynthesis bool onSynthesisResult func(result string) } @@ -669,30 +1082,32 @@ type singleAgentSinkInputs struct { // composeMultiAgentSinks builds the sink slice for a multi-agent run. // -// - Non-TTY: [DumpSink] alone — narrative dump only, no live UI, no prompts. +// - Non-TTY: [DumpSink, SynthesisSink?] — narrative dump plus profile-native +// final report when autoSynthesis is enabled. // - TTY: [TUISink, DumpSink, SynthesisSink?] — TUI owns the live dashboard; -// DumpSink renders the post-run narrative; SynthesisSink (if a provider is -// configured AND stdin can prompt) appends the y/N synthesis offer. +// DumpSink renders the post-run narrative; SynthesisSink renders the final +// report after the TUI exits. // -// The synthesis sink is only appended when canPrompt is true: without a -// promptable stdin, the y/N form would never resolve. SynthesisSink also -// guards on InputTTY internally (defense in depth) but suppressing it here -// avoids constructing a sink that will silently no-op. +// Prompted legacy synthesis is still only appended when canPrompt is true. +// Profile-native auto synthesis does not need stdin, so it is available in +// redirected and CI output too. func composeMultiAgentSinks(in multiAgentSinkInputs) []reviewtypes.Sink { - if !in.isTTY { - return []reviewtypes.Sink{DumpSink{W: in.out}} + sinks := []reviewtypes.Sink{} + if in.isTTY { + sinks = append(sinks, NewTUISink(in.agentNames, in.cancelRun, in.out, os.Stdin)) } - sinks := []reviewtypes.Sink{ - NewTUISink(in.agentNames, in.cancelRun, in.out, os.Stdin), - DumpSink{W: in.out}, - } - if in.synthesisProvider != nil && in.canPrompt { + sinks = append(sinks, DumpSink{W: in.out}) + if in.synthesisProvider != nil && (in.autoSynthesis || in.canPrompt) { sinks = append(sinks, SynthesisSink{ Provider: in.synthesisProvider, Writer: in.out, InputTTY: in.canPrompt, PromptYN: in.promptYN, PerRunPrompt: in.perRunPrompt, + ProfileName: in.profileName, + Task: in.task, + MasterName: in.masterName, + Auto: in.autoSynthesis, RunContext: in.runContext, OnResult: in.onSynthesisResult, }) @@ -752,7 +1167,7 @@ func warnManifestNotWritten(out io.Writer, reason string) { fmt.Fprintln(out) fmt.Fprintln(out, "Note: review skills ran but findings were not persisted.") fmt.Fprintf(out, " Reason: %s\n", reason) - fmt.Fprintln(out, " `entire review --findings` and `entire review --fix` will not see this run.") + fmt.Fprintln(out, " `entire review --findings` will not see this run.") fmt.Fprintln(out, " Re-run with `ENTIRE_LOG_LEVEL=debug` for diagnostic detail.") } @@ -795,11 +1210,8 @@ func runConfigWithReviewConfig(base reviewtypes.RunConfig, cfg settings.ReviewCo } func applyReviewConfig(runCfg *reviewtypes.RunConfig, cfg settings.ReviewConfig) { + runCfg.Model = strings.TrimSpace(cfg.Model) runCfg.Skills = cfg.Skills - if len(cfg.Skills) == 0 { - runCfg.PromptOverride = cfg.Prompt - return - } runCfg.AlwaysPrompt = cfg.Prompt } @@ -819,11 +1231,19 @@ func findTUISink(sinks []reviewtypes.Sink) (*TUISink, bool) { // RunMulti pass a single shared RunConfig at the API boundary while each // agent in a multi-agent run still sees its own skills and always-prompt. type perAgentConfiguredReviewer struct { + name string inner reviewtypes.AgentReviewer cfg reviewtypes.RunConfig } -func (r *perAgentConfiguredReviewer) Name() string { return r.inner.Name() } +func (r *perAgentConfiguredReviewer) Name() string { + if strings.TrimSpace(r.name) != "" { + return strings.TrimSpace(r.name) + } + return r.inner.Name() +} +func (r *perAgentConfiguredReviewer) ActualAgentName() string { return r.inner.Name() } +func (r *perAgentConfiguredReviewer) ModelName() string { return strings.TrimSpace(r.cfg.Model) } func (r *perAgentConfiguredReviewer) Start(ctx context.Context, _ reviewtypes.RunConfig) (reviewtypes.Process, error) { return r.inner.Start(ctx, r.cfg) //nolint:wrapcheck // transparent adapter; callers see inner's error type directly } diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index b9b11d7ea..69697b894 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -43,10 +43,8 @@ func installHooksForCmdTest(t *testing.T, agentName types.AgentName) { } } -// seedReviewConfig persists a review config map into clone-local preferences for -// test setup, preserving any other existing preferences. It replaces the former -// review.SaveReviewConfig, which had no production caller (the picker writes via -// the combined config+fix-agent writer instead). +// seedReviewConfig persists a default review profile into clone-local +// preferences for test setup, preserving any other existing preferences. func seedReviewConfig(ctx context.Context, cfg map[string]settings.ReviewConfig) error { prefs, err := settings.LoadClonePreferences(ctx) if err != nil { @@ -55,10 +53,54 @@ func seedReviewConfig(ctx context.Context, cfg map[string]settings.ReviewConfig) if prefs == nil { prefs = &settings.ClonePreferences{} } - prefs.Review = cfg + prefs.ReviewDefaultProfile = review.DefaultProfileName + prefs.ReviewProfiles = map[string]settings.ReviewProfileConfig{ + review.DefaultProfileName: { + Task: "Test review task.", + Agents: cfg, + Master: defaultTestMaster(cfg), + }, + } return settings.SaveClonePreferences(ctx, prefs) } +func defaultTestMaster(cfg map[string]settings.ReviewConfig) string { + if _, ok := cfg[string(agent.AgentNameClaudeCode)]; ok { + return string(agent.AgentNameClaudeCode) + } + for name := range cfg { + return name + } + return "" +} + +// TestReviewCmd_ListAgents verifies `entire review --agents` lists the +// configured profile workers (the valid --agent values) with the master marked. +func TestReviewCmd_ListAgents(t *testing.T) { + setupCmdTestRepo(t) + ctx := context.Background() + if err := seedReviewConfig(ctx, map[string]settings.ReviewConfig{ + string(agent.AgentNameClaudeCode): {Skills: []string{"/review"}}, + string(agent.AgentNameCodex): {Skills: []string{"/review"}}, + }); err != nil { + t.Fatalf("seedReviewConfig: %v", err) + } + + rootCmd := cli.NewRootCmd() + buf := &bytes.Buffer{} + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"review", "--agents"}) + if err := rootCmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + out := buf.String() + for _, want := range []string{"claude-code", "codex", "[master]", "--agent"} { + if !strings.Contains(out, want) { + t.Errorf("--agents output missing %q:\n%s", want, out) + } + } +} + // TestReviewCmd_Help verifies `entire review --help` contains the expected // flags and subcommands without panicking. func TestReviewCmd_Help(t *testing.T) { @@ -71,7 +113,7 @@ func TestReviewCmd_Help(t *testing.T) { t.Fatalf("execute: %v", err) } out := buf.String() - for _, want := range []string{"review", "--edit", "--findings", "--fix", "--all", "--agent", "attach", "Labs entry"} { + for _, want := range []string{"review", "--configure", "--edit", "--findings", "--agent", "--agents", "--model", "--models", "attach", "Labs entry"} { if !strings.Contains(out, want) { t.Errorf("--help output missing %q: %s", want, out) } @@ -82,6 +124,45 @@ func TestReviewCmd_Help(t *testing.T) { } } +// TestReviewCmd_ListModels verifies `entire review --models` prints the +// advertised models for the built-in review agents without needing a repo. +func TestReviewCmd_ListModels(t *testing.T) { + t.Parallel() + rootCmd := cli.NewRootCmd() + buf := &bytes.Buffer{} + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"review", "--models"}) + if err := rootCmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + out := buf.String() + for _, want := range []string{"claude-code", "opus", "sonnet", "codex", "gpt-5-codex", "gemini", "gemini-2.5-pro"} { + if !strings.Contains(out, want) { + t.Errorf("--models output missing %q:\n%s", want, out) + } + } +} + +// TestReviewCmd_ListModelsFilteredByAgent verifies the --agent filter narrows +// the model listing to a single agent. +func TestReviewCmd_ListModelsFilteredByAgent(t *testing.T) { + t.Parallel() + rootCmd := cli.NewRootCmd() + buf := &bytes.Buffer{} + rootCmd.SetOut(buf) + rootCmd.SetArgs([]string{"review", "--models", "--agent", "codex"}) + if err := rootCmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + out := buf.String() + if !strings.Contains(out, "gpt-5-codex") { + t.Errorf("expected codex models, got:\n%s", out) + } + if strings.Contains(out, "gemini-2.5-pro") { + t.Errorf("--agent codex should not list gemini models:\n%s", out) + } +} + // TestNewReviewCmd_NoHiddenFlags ensures the removed internal flags are gone. func TestNewReviewCmd_NoHiddenFlags(t *testing.T) { t.Parallel() @@ -105,7 +186,6 @@ func TestReview_NotGitRepoReturnsSilentError(t *testing.T) { args []string }{ {"findings", []string{"review", "--findings"}}, - {"fix", []string{"review", "--fix", "review-session"}}, } for _, tt := range tests { @@ -152,8 +232,8 @@ func TestRunReview_MissingHooksAborts(t *testing.T) { if err == nil { t.Fatal("expected error when hooks are not installed") } - if !strings.Contains(errBuf.String(), "Hooks are not installed") { - t.Errorf("expected 'Hooks are not installed' in stderr, got: %s", errBuf.String()) + if !strings.Contains(errBuf.String(), "hooks are not installed") { + t.Errorf("expected 'hooks are not installed' in stderr, got: %s", errBuf.String()) } _, ok, readErr := review.ReadPendingReviewMarker(context.Background()) @@ -351,13 +431,13 @@ func newDispatchTestDeps( for _, name := range launchableAgents { launchableSet[name] = struct{}{} } + _ = promptForAgentFn + _ = multiPickerFn return review.Deps{ GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { return installed }, - NewSilentError: func(err error) error { return err }, - PromptForAgentFn: promptForAgentFn, - MultiPickerFn: multiPickerFn, + NewSilentError: func(err error) error { return err }, HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { return false, "" // no review guard }, @@ -464,8 +544,8 @@ func TestRunReview_ConfigPromptAugmentsSelectedSkills(t *testing.T) { } // TestDispatchFork_TwoLaunchableNoOverride verifies that when 2+ launchable -// agents are configured and --agent is empty, the multi-picker is invoked -// and RunMulti is called (not the single-agent path). +// agents are configured and --agent is empty, the profile fan-out runs without +// invoking the old per-run multi-picker. func TestDispatchFork_TwoLaunchableNoOverride(t *testing.T) { setupCmdTestRepo(t) @@ -498,8 +578,8 @@ func TestDispatchFork_TwoLaunchableNoOverride(t *testing.T) { if err := cmd.Execute(); err != nil { t.Fatalf("unexpected error: %v", err) } - if !multiPickerCalled { - t.Error("expected multi-picker to be invoked for 2 launchable agents with no --agent override") + if multiPickerCalled { + t.Error("multi-picker should not be invoked; profile config is the fan-out contract") } } @@ -521,19 +601,11 @@ func TestDispatchFork_MultiAgentPassesPerAgentConfigs(t *testing.T) { claudeReviewer := &captureRunConfigReviewer{name: "claude-code"} codexReviewer := &captureRunConfigReviewer{name: testCodexAgent} - multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { - return review.PickedAgents{ - Names: []string{"claude-code", testCodexAgent}, - PerRun: "Focus this run on regressions.", - }, nil - } - deps := review.Deps{ GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { return []types.AgentName{"claude-code", testCodexAgent} }, NewSilentError: func(err error) error { return err }, - MultiPickerFn: multiPickerFn, HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { return false, "" }, @@ -552,7 +624,7 @@ func TestDispatchFork_MultiAgentPassesPerAgentConfigs(t *testing.T) { cmd := review.NewCommand(deps) cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) - cmd.SetArgs([]string{}) + cmd.SetArgs([]string{"--prompt", "Focus this run on regressions."}) if err := cmd.Execute(); err != nil { t.Fatalf("unexpected error: %v", err) @@ -705,8 +777,8 @@ func TestDispatchFork_MultiPickerCancellationExitsCleanly(t *testing.T) { } } -// TestDispatchFork_MultiPickerNoSelectionSurfacesError verifies that when the -// multi-picker returns ErrNoAgentsSelected, a clear error is shown to the user. +// TestDispatchFork_MultiPickerNoSelectionNotUsed verifies profile fan-out no +// longer asks a per-run multi-picker, so picker selection errors are irrelevant. func TestDispatchFork_MultiPickerNoSelectionSurfacesError(t *testing.T) { setupCmdTestRepo(t) @@ -717,25 +789,25 @@ func TestDispatchFork_MultiPickerNoSelectionSurfacesError(t *testing.T) { t.Fatal(err) } + multiPickerCalled := false multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { + multiPickerCalled = true return review.PickedAgents{}, review.ErrNoAgentsSelected } installed := []types.AgentName{"agent-a", "agent-b"} deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) - errBuf := &bytes.Buffer{} cmd := review.NewCommand(deps) cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(errBuf) + cmd.SetErr(&bytes.Buffer{}) cmd.SetArgs([]string{}) - err := cmd.Execute() - if err == nil { - t.Fatal("expected non-nil error when no agents are selected") + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) } - if !strings.Contains(errBuf.String(), "no agents selected") { - t.Errorf("stderr should mention 'no agents selected', got: %q", errBuf.String()) + if multiPickerCalled { + t.Error("multi-picker should not be called by profile fan-out") } } @@ -1004,7 +1076,7 @@ func TestDispatchFork_SynthesisSinkNilProviderNoComposition(t *testing.T) { installed := []types.AgentName{"agent-a", "agent-b"} deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) - deps.SynthesisProvider = nil // explicitly nil — synthesis unavailable + // Profile-native review uses the profile master rather than deps-level synthesis. buf := &bytes.Buffer{} cmd := review.NewCommand(deps) @@ -1040,7 +1112,7 @@ func TestDispatchFork_SingleAgentNoSynthesis(t *testing.T) { // cursor is installed but not launchable (ReviewerFor returns nil). installed := []types.AgentName{"cursor"} deps := newDispatchTestDeps(t, installed, nil /* no launchable */, nil, nil) - deps.SynthesisProvider = provider + _ = provider buf := &bytes.Buffer{} cmd := review.NewCommand(deps) diff --git a/cmd/entire/cli/review/configure_test.go b/cmd/entire/cli/review/configure_test.go new file mode 100644 index 000000000..67028b73f --- /dev/null +++ b/cmd/entire/cli/review/configure_test.go @@ -0,0 +1,126 @@ +package review + +import ( + "context" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/agent" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +func TestModelInList(t *testing.T) { + models := []agent.ModelInfo{{ID: "opus"}, {ID: "sonnet"}} + if !modelInList("opus", models) { + t.Error("expected opus to be in list") + } + if modelInList("gpt-5", models) { + t.Error("gpt-5 should not be in list") + } +} + +func configureTestDeps(adapter ...string) Deps { + set := map[string]struct{}{} + for _, a := range adapter { + set[a] = struct{}{} + } + return Deps{ + ReviewerFor: func(name string) reviewtypes.AgentReviewer { + if _, ok := set[name]; ok { + return &stubReviewer{name: name} + } + return nil + }, + } +} + +func TestBuildConfiguredProfile_FromFlags(t *testing.T) { + deps := configureTestDeps("claude-code", "codex") + profile, err := buildConfiguredProfile( + context.Background(), + "general", + reviewConfigureOptions{ + Agents: []string{"claude-code", "codex"}, + Master: "codex", + Models: []string{"claude-code=opus"}, + }, + &settings.EntireSettings{}, + deps, + ) + if err != nil { + t.Fatalf("buildConfiguredProfile: %v", err) + } + if len(profile.Agents) != 2 { + t.Fatalf("agents = %d, want 2: %#v", len(profile.Agents), profile.Agents) + } + if got := profile.Agents["claude-code"].Model; got != "opus" { + t.Errorf("claude-code model = %q, want opus", got) + } + if profile.Master != "codex" { + t.Errorf("master = %q, want codex", profile.Master) + } + if profile.Task == "" { + t.Error("task should default to the built-in general task") + } +} + +func TestBuildConfiguredProfile_RejectsNonAdapterAgent(t *testing.T) { + deps := configureTestDeps("claude-code") + _, err := buildConfiguredProfile( + context.Background(), + "general", + reviewConfigureOptions{Agents: []string{"cursor"}}, + &settings.EntireSettings{}, + deps, + ) + if err == nil { + t.Fatal("expected error for agent without a review-runner adapter") + } +} + +func TestBuildConfiguredProfile_PreservesExistingTaskAndMasterModel(t *testing.T) { + deps := configureTestDeps("claude-code", "codex") + s := &settings.EntireSettings{ + ReviewProfiles: map[string]settings.ReviewProfileConfig{ + "general": { + Task: "Custom task text.", + MasterModel: "opus", + Agents: map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review"}}, + }, + Master: "claude-code", + }, + }, + } + // Only change the worker set; task + master_model must survive. + profile, err := buildConfiguredProfile( + context.Background(), + "general", + reviewConfigureOptions{Agents: []string{"claude-code", "codex"}, Master: "claude-code"}, + s, + deps, + ) + if err != nil { + t.Fatalf("buildConfiguredProfile: %v", err) + } + if profile.Task != "Custom task text." { + t.Errorf("task = %q, want preserved custom task", profile.Task) + } + if profile.MasterModel != "opus" { + t.Errorf("master_model = %q, want preserved opus", profile.MasterModel) + } +} + +func TestBuildConfiguredProfile_InvalidModelSpec(t *testing.T) { + deps := configureTestDeps("claude-code") + _, err := buildConfiguredProfile( + context.Background(), + "general", + reviewConfigureOptions{Agents: []string{"claude-code"}, Models: []string{"no-equals"}}, + &settings.EntireSettings{}, + deps, + ) + if err == nil { + t.Fatal("expected error for malformed --set-model spec") + } +} diff --git a/cmd/entire/cli/review/export_test.go b/cmd/entire/cli/review/export_test.go index 42926a160..cb19a66cb 100644 --- a/cmd/entire/cli/review/export_test.go +++ b/cmd/entire/cli/review/export_test.go @@ -12,7 +12,9 @@ import ( // ExposedComposeSynthesisPrompt exposes composeSynthesisPrompt for // package-external tests (synthesis_prompt_test.go, synthesis_sink_test.go). // Only compiled during `go test`. -var ExposedComposeSynthesisPrompt = composeSynthesisPrompt +func ExposedComposeSynthesisPrompt(summary reviewtypes.RunSummary, perRunPrompt string) string { + return composeSynthesisPrompt(summary, perRunPrompt, "", "") +} // SinkComposeInputs is the test-facing alias for multiAgentSinkInputs. // It lets external tests drive composeMultiAgentSinks with explicit isTTY diff --git a/cmd/entire/cli/review/fix.go b/cmd/entire/cli/review/fix.go index 8ce584e21..d69262319 100644 --- a/cmd/entire/cli/review/fix.go +++ b/cmd/entire/cli/review/fix.go @@ -5,44 +5,18 @@ import ( "errors" "fmt" "io" - "slices" - "strconv" "strings" "charm.land/huh/v2" "github.com/spf13/cobra" - "github.com/entireio/cli/cmd/entire/cli/agent" - agenttypes "github.com/entireio/cli/cmd/entire/cli/agent/types" - "github.com/entireio/cli/cmd/entire/cli/agentlaunch" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/mdrender" "github.com/entireio/cli/cmd/entire/cli/paths" - "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/stringutil" ) -type reviewFixSourceKind string - -const ( - reviewFixSourceAgent reviewFixSourceKind = "agent" - reviewFixSourceAggregate reviewFixSourceKind = "aggregate" - reviewCommandBinary = "entire" -) - -type reviewFixSource struct { - Kind reviewFixSourceKind - Agent string - Label string - Output string - Synthetic bool -} - -type reviewFinding struct { - ID string - Title string - Body string -} +const reviewCommandBinary = "entire" func runReviewFindings(ctx context.Context, cmd *cobra.Command, silentErr func(error) error) error { worktreeRoot, err := paths.WorktreeRoot(ctx) @@ -71,45 +45,6 @@ func runReviewFindings(ctx context.Context, cmd *cobra.Command, silentErr func(e return nil } -func runReviewFix( - ctx context.Context, - cmd *cobra.Command, - target string, - all bool, - agentOverride string, - silentErr func(error) error, -) error { - worktreeRoot, err := paths.WorktreeRoot(ctx) - if err != nil { - cmd.SilenceUsage = true - fmt.Fprintln(cmd.ErrOrStderr(), "Not a git repository. Run `entire enable` first.") - return wrapReviewSilentError(silentErr, errors.New("not a git repository")) - } - - manifest, err := resolveReviewFixManifest(ctx, cmd, worktreeRoot, target) - if err != nil { - return err - } - sources, err := selectReviewFixSources(ctx, cmd, manifest, all) - if err != nil { - return err - } - findings, err := selectReviewFindings(ctx, cmd, sources, all) - if err != nil { - return err - } - - fixAgent, err := resolveReviewFixAgent(ctx, cmd, sources, agentOverride) - if err != nil { - return err - } - prompt := composeReviewFixPrompt(manifest, reviewFixSourcesFromFindings(findings)) - if err := agentlaunch.LaunchFixAgent(ctx, fixAgent, prompt); err != nil { - return fmt.Errorf("launch review fix agent: %w", err) - } - return nil -} - func wrapReviewSilentError(silentErr func(error) error, err error) error { if silentErr == nil { return err @@ -117,29 +52,6 @@ func wrapReviewSilentError(silentErr func(error) error, err error) error { return silentErr(err) } -func resolveReviewFixManifest(ctx context.Context, cmd *cobra.Command, worktreeRoot string, target string) (LocalReviewManifest, error) { - if target != "" { - manifest, _, err := resolveLocalReviewManifestBySessionID(ctx, worktreeRoot, target) - return manifest, err - } - manifests, err := loadLocalReviewManifests(ctx, worktreeRoot) - if err != nil { - return LocalReviewManifest{}, err - } - switch len(manifests) { - case 0: - return LocalReviewManifest{}, errors.New("no local review findings found") - case 1: - return manifests[0], nil - default: - if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() { - printReviewFindingsList(cmd.OutOrStdout(), manifests) - return LocalReviewManifest{}, errors.New("multiple review runs found; pass a session id") - } - return promptForReviewManifest(ctx, manifests) - } -} - func promptForReviewManifest(ctx context.Context, manifests []LocalReviewManifest) (LocalReviewManifest, error) { options := make([]huh.Option[int], len(manifests)) for i, manifest := range manifests { @@ -159,447 +71,22 @@ func promptForReviewManifest(ctx context.Context, manifests []LocalReviewManifes return manifests[picked], nil } -func selectReviewFixSources(ctx context.Context, cmd *cobra.Command, manifest LocalReviewManifest, all bool) ([]reviewFixSource, error) { - sources := reviewFixSourcesForManifest(manifest) - if len(sources) == 0 { - return nil, errors.New("selected review has no output to fix") - } - if all { - return reviewFixSourcesForAll(sources), nil - } - if len(sources) == 1 { - return sources, nil - } - if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() { - return nil, errors.New("multiple review sources found; rerun with --all or use an interactive terminal") - } - - values := make([]string, len(sources)) - options := make([]huh.Option[string], len(sources)) - defaults := defaultReviewFixSourceSelection(sources) - for i, source := range sources { - value := strconv.Itoa(i) - values[i] = value - options[i] = huh.NewOption(source.Label, value) - } - picked := defaults - form := newAccessibleForm(huh.NewGroup( - huh.NewMultiSelect[string](). - Title(reviewFixSourcePickerTitle(manifest)). - Description("ctrl+a select all · space toggle · enter continue"). - Options(options...). - Height(reviewPickerHeight(len(options))). - Value(&picked), - )) - if err := form.RunWithContext(ctx); err != nil { - return nil, fmt.Errorf("review source picker: %w", err) - } - if len(picked) == 0 { - return nil, errors.New("no review sources selected") - } - selected := make([]reviewFixSource, 0, len(picked)) - for _, value := range picked { - idx := slices.Index(values, value) - if idx >= 0 { - selected = append(selected, sources[idx]) - } - } - return selected, nil -} - -func selectReviewFindings(ctx context.Context, cmd *cobra.Command, sources []reviewFixSource, all bool) ([]reviewFinding, error) { - findings := extractReviewFindings(sources) - if all || len(findings) <= 1 { - return findings, nil - } - if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() { - return nil, errors.New("multiple findings found; rerun with --all or use an interactive terminal") - } - options := make([]huh.Option[string], len(findings)) - picked := make([]string, len(findings)) - for i, finding := range findings { - picked[i] = finding.ID - options[i] = huh.NewOption(finding.Title, finding.ID) - } - form := newAccessibleForm(huh.NewGroup( - huh.NewMultiSelect[string](). - Title("Select findings to fix"). - Description("ctrl+a select all · space toggle · enter fix"). - Options(options...). - Height(reviewPickerHeight(len(options))). - Value(&picked), - )) - if err := form.RunWithContext(ctx); err != nil { - return nil, fmt.Errorf("review finding picker: %w", err) - } - if len(picked) == 0 { - return nil, errors.New("no findings selected") - } - selected := make([]reviewFinding, 0, len(picked)) - for _, finding := range findings { - if slices.Contains(picked, finding.ID) { - selected = append(selected, finding) - } - } - return selected, nil -} - -func composeReviewFixPrompt(manifest LocalReviewManifest, sources []reviewFixSource) string { - var b strings.Builder - b.WriteString("Fix only the selected review findings.\n") - b.WriteString("Do not rewrite unrelated code. Run targeted tests where practical, then report what changed and what verification passed.\n") - if manifest.StartingSHA != "" { - fmt.Fprintf(&b, "\nReviewed commit: %s\n", manifest.StartingSHA) - } - if manifest.WorktreePath != "" { - fmt.Fprintf(&b, "Worktree: %s\n", manifest.WorktreePath) - } - for _, source := range sources { - if strings.TrimSpace(source.Output) == "" { - continue - } - fmt.Fprintf(&b, "\n## %s\n\n%s\n", source.Label, strings.TrimSpace(source.Output)) - } - return strings.TrimSpace(b.String()) + "\n" -} - -func reviewFixSourcesForManifest(manifest LocalReviewManifest) []reviewFixSource { - sources := make([]reviewFixSource, 0, len(manifest.Sources)+1) - for _, source := range manifest.Sources { - if strings.TrimSpace(source.Output) == "" { - continue - } - label := source.Label - if label == "" { - label = source.Agent - } - sources = append(sources, reviewFixSource{ - Kind: reviewFixSourceAgent, - Agent: source.Agent, - Label: label + " findings", - Output: source.Output, - }) - } - if strings.TrimSpace(manifest.AggregateOutput) != "" { - sources = append(sources, reviewFixSource{ - Kind: reviewFixSourceAggregate, - Label: "Aggregate summary", - Output: manifest.AggregateOutput, - }) - } else if len(sources) > 1 { - sources = append(sources, reviewFixSource{ - Kind: reviewFixSourceAggregate, - Label: "Aggregate findings", - Output: selectedSourcesOutput(sources), - Synthetic: true, - }) - } - return sources -} - +// reviewPickerHeight reserves the title + description lines huh.MultiSelect +// subtracts from Height before sizing its option viewport. Shared by the +// profile master picker. func reviewPickerHeight(optionCount int) int { - // huh.MultiSelect subtracts the title and description from Height before - // sizing the option viewport, so reserve those two lines explicitly. return min(optionCount+3, 14) } -func reviewFixSourcePickerTitle(manifest LocalReviewManifest) string { - handle := reviewManifestHandle(manifest) - if handle == "" { - return "Choose findings source" - } - return "Choose findings source (" + handle + ")" -} - -func reviewFixSourcesForAll(sources []reviewFixSource) []reviewFixSource { - selected := make([]reviewFixSource, 0, len(sources)) - for _, source := range sources { - if source.Synthetic { - continue - } - selected = append(selected, source) - } - if len(selected) == 0 { - return sources - } - return selected -} - -func defaultReviewFixSourceSelection(sources []reviewFixSource) []string { - var aggregate []string - var agents []string - for i, source := range sources { - value := strconv.Itoa(i) - if source.Kind == reviewFixSourceAggregate { - aggregate = append(aggregate, value) - continue - } - agents = append(agents, value) - } - if len(aggregate) > 0 { - return aggregate - } - return agents -} - -func extractReviewFindings(sources []reviewFixSource) []reviewFinding { - var findings []reviewFinding - for i, source := range sources { - sourceFindings := extractSourceFindings(source, i) - findings = append(findings, sourceFindings...) - } - if len(findings) > 0 { - return findings - } - combined := selectedSourcesOutput(sources) - if combined == "" { - return nil - } - return []reviewFinding{{ - ID: "full-output", - Title: "Full selected review output", - Body: combined, - }} -} - -func extractSourceFindings(source reviewFixSource, sourceIndex int) []reviewFinding { - lines := strings.Split(source.Output, "\n") - var findings []reviewFinding - var current *reviewFinding - for _, line := range lines { - trimmed := strings.TrimSpace(line) - title, ok := reviewFindingTitle(trimmed) - if ok { - if current != nil { - findings = append(findings, *current) - } - current = &reviewFinding{ - ID: fmt.Sprintf("source-%d-%d", sourceIndex, len(findings)+1), - Title: source.Label + ": " + stringutil.TruncateRunes(title, 90, "..."), - Body: title, - } - continue - } - if current != nil { - current.Body = strings.TrimSpace(current.Body + "\n" + line) - } - } - if current != nil { - findings = append(findings, *current) - } - return findings -} - -func reviewFindingTitle(line string) (string, bool) { - line = strings.TrimLeft(line, "#*- \t") - line = strings.TrimSpace(line) - if len(line) < 3 { - return "", false - } - if isSeverityNumberedTitle(line) { - return line, true - } - lower := strings.ToLower(line) - for _, prefix := range []string{"blocker", "critical", "high", "medium", "low"} { - if strings.HasPrefix(lower, prefix+":") || strings.HasPrefix(lower, prefix+" -") || strings.HasPrefix(lower, prefix+".") { - return line, true - } - } - return "", false -} - -func isSeverityNumberedTitle(line string) bool { - if len(line) < 3 { - return false - } - switch line[0] { - case 'H', 'M', 'L': - default: - return false - } - return line[1] >= '0' && line[1] <= '9' && (line[2] == '.' || line[2] == ')') -} - -func reviewFixSourcesFromFindings(findings []reviewFinding) []reviewFixSource { - var b strings.Builder - for _, finding := range findings { - if strings.TrimSpace(finding.Body) == "" { - continue - } - fmt.Fprintf(&b, "## %s\n\n%s\n\n", finding.Title, strings.TrimSpace(finding.Body)) - } - return []reviewFixSource{{ - Kind: reviewFixSourceAgent, - Label: "Selected findings", - Output: strings.TrimSpace(b.String()), - }} -} - -func selectedSourcesOutput(sources []reviewFixSource) string { - var b strings.Builder - for _, source := range sources { - if strings.TrimSpace(source.Output) == "" { - continue - } - fmt.Fprintf(&b, "## %s\n\n%s\n\n", source.Label, strings.TrimSpace(source.Output)) - } - return strings.TrimSpace(b.String()) -} - -func resolveReviewFixAgent(ctx context.Context, cmd *cobra.Command, sources []reviewFixSource, agentOverride string) (string, error) { - if agentOverride != "" { - return agentOverride, nil - } - if agentName, ok := reviewFixAgentFromSelectedSources(sources); ok { - return agentName, nil - } - - s, err := settings.Load(ctx) - if err != nil { - return "", fmt.Errorf("load review fix settings: %w", err) - } - choices := reviewFixAgentChoices(s.Review) - if len(choices) == 0 { - choices = reviewFixAgentChoicesFromSources(sources) - } - switch len(choices) { - case 0: - return "", errors.New("cannot determine fix agent; rerun with --agent") - case 1: - return choices[0].Name, nil - } - if pick, ok := savedReviewFixAgentPick(choices, s.ReviewFixAgent); ok { - return pick, nil - } - - if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() { - return "", errors.New("multiple fix agents configured; rerun with --agent or run `entire review --edit`") - } - - picked, err := promptForReviewFixAgent(ctx, choices, s.ReviewFixAgent) - if err != nil { - return "", err - } - if err := SaveReviewFixAgent(ctx, picked); err != nil { - return "", err - } - return picked, nil -} - -func reviewFixAgentFromSelectedSources(sources []reviewFixSource) (string, bool) { - if len(sources) != 1 { - return "", false - } - source := sources[0] - if source.Kind != reviewFixSourceAgent || source.Agent == "" { - return "", false - } - return source.Agent, true -} - -func reviewFixAgentChoices(configured map[string]settings.ReviewConfig) []AgentChoice { - choices := make([]AgentChoice, 0, len(configured)) - for name, cfg := range configured { - if cfg.IsZero() { - continue - } - choice, ok := reviewFixAgentChoice(name) - if ok { - choices = append(choices, choice) - } - } - slices.SortFunc(choices, func(a, b AgentChoice) int { - return strings.Compare(a.Name, b.Name) - }) - return choices -} - -func reviewFixAgentChoicesFromSources(sources []reviewFixSource) []AgentChoice { - seen := map[string]struct{}{} - var choices []AgentChoice - for _, source := range sources { - if source.Agent == "" { - continue - } - if _, ok := seen[source.Agent]; ok { - continue - } - choice, ok := reviewFixAgentChoice(source.Agent) - if !ok { - continue - } - seen[source.Agent] = struct{}{} - choices = append(choices, choice) - } - slices.SortFunc(choices, func(a, b AgentChoice) int { - return strings.Compare(a.Name, b.Name) - }) - return choices -} - -func reviewFixAgentChoice(name string) (AgentChoice, bool) { - if _, ok := agent.LauncherFor(agenttypes.AgentName(name)); !ok { - return AgentChoice{}, false - } - label := name - if ag, err := agent.Get(agenttypes.AgentName(name)); err == nil { - label = string(ag.Type()) - } - return AgentChoice{Name: name, Label: label}, true -} - -func defaultReviewFixAgentPick(choices []AgentChoice, saved string) string { - if pick, ok := savedReviewFixAgentPick(choices, saved); ok { - return pick - } - if len(choices) == 0 { - return "" - } - return choices[0].Name -} - -func savedReviewFixAgentPick(choices []AgentChoice, saved string) (string, bool) { - for _, choice := range choices { - if choice.Name == saved { - return saved, true - } - } - return "", false -} - -func promptForReviewFixAgent(ctx context.Context, choices []AgentChoice, saved string) (string, error) { - options := make([]huh.Option[string], 0, len(choices)) - for _, choice := range choices { - options = append(options, huh.NewOption(choice.Label, choice.Name)) - } - picked := defaultReviewFixAgentPick(choices, saved) - form := newAccessibleForm(huh.NewGroup( - huh.NewSelect[string](). - Title("Choose fix agent"). - Description("Used for aggregate or multi-agent review findings. Saved for next time."). - Options(options...). - Height(reviewPickerHeight(len(options))). - Value(&picked), - )) - if err := form.RunWithContext(ctx); err != nil { - return "", fmt.Errorf("fix agent picker: %w", err) - } - return picked, nil -} - func writeReviewCompletionFooter(w io.Writer, manifest LocalReviewManifest) { - handle := reviewManifestHandle(manifest) - if handle == "" { - return - } fmt.Fprintln(w) fmt.Fprintln(w, "Review complete.") + if reviewManifestHandle(manifest) == "" { + return + } fmt.Fprintln(w) - fmt.Fprintln(w, "To apply all review findings:") - fmt.Fprintf(w, " %s review --fix %s --all\n", reviewCommandBinary, handle) - fmt.Fprintln(w) - fmt.Fprintln(w, "To choose findings:") - fmt.Fprintf(w, " %s review --fix %s\n", reviewCommandBinary, handle) + fmt.Fprintln(w, "Browse findings:") + fmt.Fprintf(w, " %s review --findings\n", reviewCommandBinary) } func reviewManifestHandle(manifest LocalReviewManifest) string { @@ -614,11 +101,8 @@ func reviewManifestHandle(manifest LocalReviewManifest) string { func printReviewFindingsList(w io.Writer, manifests []LocalReviewManifest) { fmt.Fprintln(w, "Review Findings") fmt.Fprintln(w) - commandName := reviewCommandBinary for _, manifest := range manifests { fmt.Fprintf(w, "%s\n", reviewManifestListLabel(manifest)) - fmt.Fprintf(w, " fix all: %s review --fix %s --all\n", commandName, reviewManifestHandle(manifest)) - fmt.Fprintf(w, " choose: %s review --fix %s\n", commandName, reviewManifestHandle(manifest)) } } @@ -630,7 +114,6 @@ func printReviewManifestDetail(w io.Writer, manifest LocalReviewManifest) { if strings.TrimSpace(manifest.AggregateOutput) != "" { printRenderedReviewSection(w, "Aggregate summary", manifest.AggregateOutput) } - writeReviewCompletionFooter(w, manifest) } func printRenderedReviewSection(w io.Writer, title string, body string) { diff --git a/cmd/entire/cli/review/manifest.go b/cmd/entire/cli/review/manifest.go index e9850b88b..3179f0843 100644 --- a/cmd/entire/cli/review/manifest.go +++ b/cmd/entire/cli/review/manifest.go @@ -70,15 +70,16 @@ func buildLocalReviewManifestFromSummary( } usedSessions := map[string]bool{} for _, run := range summary.AgentRuns { - st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, run.Name, states, usedSessions) + agentName := agentNameForRun(run) + st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, agentName, run.Model, states, usedSessions) if st == nil || st.SessionID == "" { continue } usedSessions[st.SessionID] = true manifest.Sources = append(manifest.Sources, ManifestSource{ SessionID: st.SessionID, - Agent: run.Name, - Label: labelForReviewAgent(run.Name), + Agent: agentName, + Label: labelForReviewRun(run), Status: run.Status.String(), Output: agentRunOutput(run), }) @@ -189,7 +190,8 @@ func explainEmptyManifest( // across store.List orderings and faithfully represents the full set of // mismatched types rather than whichever happened to be iterated last. for _, run := range summary.AgentRuns { - wantType := agentTypeForReviewAgent(run.Name) + agentName := agentNameForRun(run) + wantType := agentTypeForReviewAgent(agentName) if wantType == "" { continue } @@ -209,7 +211,7 @@ func explainEmptyManifest( } if !anyMatch { sort.Strings(observedTypes) - return fmt.Sprintf("found %d tagged review session(s) but AgentType mismatch for agent %q: state=%q, run=%q", len(tagged), run.Name, strings.Join(observedTypes, ", "), wantType), false + return fmt.Sprintf("found %d tagged review session(s) but AgentType mismatch for agent %q: state=%q, run=%q", len(tagged), agentName, strings.Join(observedTypes, ", "), wantType), false } } @@ -277,7 +279,7 @@ func hydrateReviewAgentRunTokensFromStates( states []*session.State, lookup agentTypeLookup, ) reviewtypes.AgentRun { - st := matchReviewSessionState(worktreeRoot, headSHA, run.StartedAt, run.Name, states, map[string]bool{}) + st := matchReviewSessionState(worktreeRoot, headSHA, run.StartedAt, agentNameForRun(run), run.Model, states, map[string]bool{}) if st == nil || st.SessionID == "" { return run } @@ -299,7 +301,7 @@ func hydrateReviewSummaryTokensFromStates( ) reviewtypes.RunSummary { usedSessions := map[string]bool{} for i, run := range summary.AgentRuns { - st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, run.Name, states, usedSessions) + st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, agentNameForRun(run), run.Model, states, usedSessions) if st == nil || st.SessionID == "" { continue } @@ -392,6 +394,7 @@ func matchReviewSessionState( headSHA string, runStartedAt time.Time, agentName string, + modelName string, states []*session.State, used map[string]bool, ) *session.State { @@ -413,6 +416,9 @@ func matchReviewSessionState( if wantAgentType != "" && st.AgentType != "" && st.AgentType != wantAgentType { continue } + if !reviewRunModelMatches(modelName, st.ModelName) { + continue + } if best == nil || st.StartedAt.After(best.StartedAt) { best = st } @@ -420,6 +426,50 @@ func matchReviewSessionState( return best } +func reviewRunModelMatches(want, got string) bool { + want = strings.ToLower(strings.TrimSpace(want)) + got = strings.ToLower(strings.TrimSpace(got)) + if want == "" || got == "" { + return true + } + if want == got { + return true + } + wantCompact := compactReviewModelID(want) + gotCompact := compactReviewModelID(got) + if wantCompact == "" || gotCompact == "" { + return true + } + return strings.Contains(gotCompact, wantCompact) || strings.Contains(wantCompact, gotCompact) +} + +// compactReviewModelID normalizes a model string for fuzzy comparison between a +// configured profile model (e.g. "anthropic/claude-sonnet:high") and the model +// recorded on a session (e.g. "claude-sonnet-4-5"). It drops the provider +// prefix (before the last "/") so the prefix does not skew matching, drops the +// trailing thinking-level suffix (after ":"), and keeps only alphanumerics. +// +// Session model names do not carry the thinking-level suffix, so two workers +// that share a model but differ only by thinking level ("...:high" vs +// "...:low") normalize to the same id. Disambiguating those is left to the +// start-time + used-session fallback in matchReviewSessionState, which still +// links each worker to a distinct session. +func compactReviewModelID(s string) string { + if slash := strings.LastIndexByte(s, '/'); slash >= 0 && slash < len(s)-1 { + s = s[slash+1:] + } + if colon := strings.IndexByte(s, ':'); colon >= 0 { + s = s[:colon] + } + var b strings.Builder + for _, r := range strings.ToLower(s) { + if (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') { + b.WriteRune(r) + } + } + return b.String() +} + func agentTypeForReviewAgent(agentName string) agenttypes.AgentType { ag, err := agent.Get(agenttypes.AgentName(agentName)) if err != nil { @@ -428,6 +478,20 @@ func agentTypeForReviewAgent(agentName string) agenttypes.AgentType { return ag.Type() } +func agentNameForRun(run reviewtypes.AgentRun) string { + if strings.TrimSpace(run.AgentName) != "" { + return strings.TrimSpace(run.AgentName) + } + return run.Name +} + +func labelForReviewRun(run reviewtypes.AgentRun) string { + if strings.TrimSpace(run.Name) != "" && run.Name != agentNameForRun(run) { + return run.Name + } + return labelForReviewAgent(agentNameForRun(run)) +} + func labelForReviewAgent(agentName string) string { if typ := agentTypeForReviewAgent(agentName); typ != "" { return string(typ) diff --git a/cmd/entire/cli/review/manifest_test.go b/cmd/entire/cli/review/manifest_test.go index b8edfadf7..bdab02c19 100644 --- a/cmd/entire/cli/review/manifest_test.go +++ b/cmd/entire/cli/review/manifest_test.go @@ -235,48 +235,7 @@ func TestLocalReviewManifest_PrefixMatchWithinSameManifestDoesNotAmbiguate(t *te } } -func TestComposeReviewFixPrompt_UsesSelectedSources(t *testing.T) { - manifest := LocalReviewManifest{ - WorktreePath: "/repo", - Sources: []ManifestSource{ - { - SessionID: "claude-session", - Agent: "claude-code", - Label: "Claude Code", - Output: "H1. Claude finding", - }, - { - SessionID: "codex-session", - Agent: manifestTestCodexAgent, - Label: "Codex", - Output: "M1. Codex finding", - }, - }, - AggregateOutput: "Aggregate finding", - } - - prompt := composeReviewFixPrompt(manifest, []reviewFixSource{ - {Kind: reviewFixSourceAgent, Label: "Codex", Output: "M1. Codex finding"}, - {Kind: reviewFixSourceAggregate, Label: "Aggregate summary", Output: "Aggregate finding"}, - }) - - for _, want := range []string{ - "Fix only the selected review findings.", - "Codex", - "M1. Codex finding", - "Aggregate summary", - "Aggregate finding", - } { - if !strings.Contains(prompt, want) { - t.Fatalf("prompt missing %q:\n%s", want, prompt) - } - } - if strings.Contains(prompt, "H1. Claude finding") { - t.Fatalf("prompt should not include unselected Claude output:\n%s", prompt) - } -} - -func TestWriteReviewCompletionFooter_PrintsExactFixCommands(t *testing.T) { +func TestWriteReviewCompletionFooter_PointsToFindings(t *testing.T) { manifest := LocalReviewManifest{ Sources: []ManifestSource{{SessionID: "claude-session", Label: "Claude Code"}}, } @@ -285,18 +244,17 @@ func TestWriteReviewCompletionFooter_PrintsExactFixCommands(t *testing.T) { writeReviewCompletionFooter(&b, manifest) got := b.String() - for _, want := range []string{ - "Review complete.", - "entire review --fix claude-session --all", - "entire review --fix claude-session", - } { + for _, want := range []string{"Review complete.", "entire review --findings"} { if !strings.Contains(got, want) { t.Fatalf("footer missing %q:\n%s", want, got) } } + if strings.Contains(got, "--fix") { + t.Fatalf("footer should not reference removed --fix:\n%s", got) + } } -func TestPrintReviewFindingsList_PrintsProductionCommandName(t *testing.T) { +func TestPrintReviewFindingsList_ListsSessionsWithoutLocalPath(t *testing.T) { oldArgs := os.Args t.Cleanup(func() { os.Args = oldArgs }) os.Args = []string{"/tmp/local-build/entire"} @@ -317,45 +275,8 @@ func TestPrintReviewFindingsList_PrintsProductionCommandName(t *testing.T) { if strings.Contains(got, "/tmp/local-build/entire") { t.Fatalf("findings list should not print local binary path:\n%s", got) } - if !strings.Contains(got, "entire review --fix claude-session --all") { - t.Fatalf("findings list missing production command:\n%s", got) - } -} - -func TestReviewFixSourcesForManifest_AddsAggregateFallbackForMultipleAgents(t *testing.T) { - manifest := LocalReviewManifest{ - Sources: []ManifestSource{ - { - SessionID: "claude-session", - Agent: "claude-code", - Label: "Claude Code", - Output: "H1. Claude finding", - }, - { - SessionID: "codex-session", - Agent: manifestTestCodexAgent, - Label: "Codex", - Output: "M1. Codex finding", - }, - }, - } - - sources := reviewFixSourcesForManifest(manifest) - - if len(sources) != 3 { - t.Fatalf("sources = %d, want 3: %#v", len(sources), sources) - } - aggregate := sources[2] - if aggregate.Kind != reviewFixSourceAggregate { - t.Fatalf("aggregate kind = %q, want %q", aggregate.Kind, reviewFixSourceAggregate) - } - if aggregate.Label != "Aggregate findings" { - t.Fatalf("aggregate label = %q", aggregate.Label) - } - for _, want := range []string{"Claude Code findings", "H1. Claude finding", "Codex findings", "M1. Codex finding"} { - if !strings.Contains(aggregate.Output, want) { - t.Fatalf("aggregate output missing %q:\n%s", want, aggregate.Output) - } + if !strings.Contains(got, "claude-session") { + t.Fatalf("findings list missing session handle:\n%s", got) } } @@ -367,67 +288,13 @@ func TestReviewPickerHeight_ShowsAllSmallOptionSets(t *testing.T) { } } -func TestReviewFixSourcePickerTitle_IncludesSessionHandle(t *testing.T) { - manifest := LocalReviewManifest{ - Sources: []ManifestSource{{SessionID: "073be48b-2a68-473e-b783-9fa7b78a85aa"}}, - } - - got := reviewFixSourcePickerTitle(manifest) - - if !strings.Contains(got, "073be48b-2a68-473e-b783-9fa7b78a85aa") { - t.Fatalf("title = %q, want session id", got) - } -} - -func TestReviewFixAgentFromSelectedSources_UsesSingleAgentSource(t *testing.T) { - got, ok := reviewFixAgentFromSelectedSources([]reviewFixSource{ - {Kind: reviewFixSourceAgent, Agent: manifestTestCodexAgent, Label: "Codex findings"}, - }) - - if !ok { - t.Fatal("expected single-source agent inference") - } - if got != manifestTestCodexAgent { - t.Fatalf("agent = %q, want codex", got) - } -} - -func TestReviewFixAgentFromSelectedSources_DoesNotInferForAggregateOrMultiple(t *testing.T) { - tests := []struct { - name string - sources []reviewFixSource - }{ - { - name: "aggregate", - sources: []reviewFixSource{ - {Kind: reviewFixSourceAggregate, Label: "Aggregate summary"}, - }, - }, - { - name: "multiple agents", - sources: []reviewFixSource{ - {Kind: reviewFixSourceAgent, Agent: "claude-code"}, - {Kind: reviewFixSourceAgent, Agent: manifestTestCodexAgent}, - }, - }, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got, ok := reviewFixAgentFromSelectedSources(tc.sources) - if ok { - t.Fatalf("agent = %q, want no inference", got) - } - }) - } -} - -func TestSavedReviewFixAgentPick_UsesSavedWhenAvailable(t *testing.T) { +func TestSavedAgentPick_UsesSavedWhenAvailable(t *testing.T) { choices := []AgentChoice{ {Name: "claude-code", Label: "Claude Code"}, {Name: manifestTestCodexAgent, Label: "Codex"}, } - got, ok := savedReviewFixAgentPick(choices, manifestTestCodexAgent) + got, ok := savedAgentPick(choices, manifestTestCodexAgent) if !ok { t.Fatal("expected saved agent match") @@ -437,28 +304,16 @@ func TestSavedReviewFixAgentPick_UsesSavedWhenAvailable(t *testing.T) { } } -func TestSavedReviewFixAgentPick_RejectsUnknownSavedAgent(t *testing.T) { +func TestSavedAgentPick_RejectsUnknownSavedAgent(t *testing.T) { choices := []AgentChoice{{Name: "claude-code", Label: "Claude Code"}} - got, ok := savedReviewFixAgentPick(choices, manifestTestCodexAgent) + got, ok := savedAgentPick(choices, manifestTestCodexAgent) if ok { t.Fatalf("saved pick = %q, want no match", got) } } -func TestPickReviewFixAgentPreference_PreservesCurrentWhenNoChoices(t *testing.T) { - t.Parallel() - - got, err := pickReviewFixAgentPreference(context.Background(), nil, manifestTestCodexAgent) - if err != nil { - t.Fatalf("pickReviewFixAgentPreference: %v", err) - } - if got != manifestTestCodexAgent { - t.Fatalf("fix agent = %q, want codex", got) - } -} - func TestBuildLocalReviewManifestFromSummary_GroupsAgentSessionsAndAggregate(t *testing.T) { started := time.Date(2026, 5, 7, 10, 0, 0, 0, time.UTC) summary := reviewtypes.RunSummary{ @@ -524,7 +379,7 @@ func TestWarnManifestNotWritten_PrintsReasonAndDiagnosticHints(t *testing.T) { for _, want := range []string{ "Note: review skills ran but findings were not persisted.", "Reason: test reason text", - "`entire review --findings` and `entire review --fix` will not see this run.", + "`entire review --findings` will not see this run.", "`ENTIRE_LOG_LEVEL=debug`", } { if !strings.Contains(got, want) { diff --git a/cmd/entire/cli/review/marker_fallback.go b/cmd/entire/cli/review/marker_fallback.go index 18b48296f..30ebe5c15 100644 --- a/cmd/entire/cli/review/marker_fallback.go +++ b/cmd/entire/cli/review/marker_fallback.go @@ -2,16 +2,15 @@ // // marker_fallback.go provides the PendingReviewMarker type and its // write/read/clear helpers, plus RunMarkerFallback which handles review for -// non-launchable agents (cursor, opencode, factoryai-droid, copilot-cli) — // agents that don't (yet) implement AgentReviewer. // -// For launchable agents (claude-code, codex, gemini) the new -// architecture uses env-var handshake (env.go) + AgentReviewer.Start, and +// For adapter-backed review workers, the new architecture uses env-var +// handshake (env.go) + AgentReviewer.Start, and // the lifecycle hook reads ENTIRE_REVIEW_* env vars off the spawned // process — there is no marker-file adoption code path. // -// For non-launchable agents the marker is purely a record of what the user -// was asked to do: RunMarkerFallback writes it before printing manual-start +// For agents without a review-runner adapter, the marker is purely a record of +// what the user was asked to do: RunMarkerFallback writes it before printing manual-start // guidance, and `entire attach --review ` (and its discovery // shortcut `entire review attach`) reads the marker to tag a manual // session after the fact. ReadPendingReviewMarker / ClearPendingReviewMarker @@ -35,7 +34,7 @@ import ( const pendingReviewMarkerFilename = "review-pending.json" // PendingReviewMarker is written by `entire review` before instructing the -// user to open a non-launchable agent. The marker records which agent and +// user to open an agent manually. The marker records which agent and // skills should run so that `entire review attach` can tag the resulting // session after the fact. // @@ -116,14 +115,14 @@ func ClearPendingReviewMarker(ctx context.Context) error { return nil } -// RunMarkerFallback handles review for non-launchable agents (cursor, -// opencode, factoryai-droid, copilot-cli) by writing the pending-review -// marker file and printing manual-start guidance. The user is told to open -// the agent themselves and run the configured skills. +// RunMarkerFallback handles review for agents that do not yet have an Entire +// review-runner adapter by writing the pending-review marker file and printing +// manual-start guidance. The user is told to open the agent themselves and run +// the configured skills. // // The marker is NOT auto-adopted by anything — the lifecycle hook reads // ENTIRE_REVIEW_* env vars on the spawned process, not the marker file. -// For non-launchable agents the user starts the agent manually, so no env +// For adapterless review agents the user starts the agent manually, so no env // inheritance happens. The marker exists purely so that `entire attach // --review ` (and its `entire review attach` shortcut) has a // record of what the user was asked to review when tagging the session @@ -146,7 +145,7 @@ func RunMarkerFallback(ctx context.Context, agentName string, cfg reviewtypes.Ru return fmt.Errorf("write pending marker: %w", err) } - fmt.Fprintf(out, "%s does not support subprocess launch yet. Marker written.\n", agentName) + fmt.Fprintf(out, "%s does not have an Entire review runner adapter yet. Marker written.\n", agentName) if len(cfg.Skills) > 0 { fmt.Fprintf(out, "Start %s manually and run these skills:\n", agentName) for i, skill := range cfg.Skills { diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go deleted file mode 100644 index ad2fe8a3c..000000000 --- a/cmd/entire/cli/review/migration.go +++ /dev/null @@ -1,285 +0,0 @@ -package review - -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "log/slog" - - "github.com/entireio/cli/cmd/entire/cli/logging" - "github.com/entireio/cli/cmd/entire/cli/settings" -) - -type projectReviewSettings struct { - path string - raw map[string]json.RawMessage - review json.RawMessage - fixAgent json.RawMessage - hasReview bool - hasFixAgent bool -} - -func maybePromptReviewSettingsMigration( - ctx context.Context, - out io.Writer, - errOut io.Writer, - canPrompt bool, - promptYN func(context.Context, string, bool) (bool, error), -) error { - project, ok, err := loadProjectReviewSettings(ctx) - if err != nil { - return err - } - if !ok { - return nil - } - - // Skip the prompt entirely if the user has already declined. Without this, - // teams who intentionally commit review prefs would be re-prompted on - // every invocation of `entire review`. - prefs, prefsErr := settings.LoadClonePreferences(ctx) - if prefsErr != nil { - return fmt.Errorf("load review preferences for migration: %w", prefsErr) - } - if prefs != nil && prefs.ReviewMigrationDismissed { - return nil - } - - // Bail before prompting if .entire/settings.local.json already has review - // keys. settings.local.json overrides clone-local preferences (mergeJSON - // wholesale-replaces the review map), so migrating without cleaning the - // local file first would silently nullify the migration on the very next - // settings.Load — the user clicks "yes", their config moves to clone - // prefs, then the local override hides it. Better to surface the - // precondition up front than to leave the user wondering why their - // migrated config disappeared. - // - // Intentionally does NOT set ReviewMigrationDismissed: this is a fixable - // precondition, not a user-rejected migration; the prompt should fire - // again on the next run after the user cleans settings.local.json. - if localHas, localPath, localErr := localSettingsHasReviewKeys(ctx); localErr != nil { - return fmt.Errorf("inspect local settings for migration: %w", localErr) - } else if localHas { - fmt.Fprintln(errOut, "Cannot migrate review preferences: .entire/settings.local.json also has review keys.") - fmt.Fprintf(errOut, "Those override clone-local preferences and would mask the migration. Remove the\n") - fmt.Fprintf(errOut, "`review` / `review_fix_agent` keys from %s, then re-run `entire review`.\n", localPath) - return nil - } - - if !canPrompt { - // Log at Warn so operators tailing .entire/logs/ catch the pending - // migration on scripted/CI invocations where the stderr hint may - // scroll past unnoticed. - logging.Warn(ctx, "review migration pending: project settings has review keys that may be committed", - slog.String("project_settings_path", project.path), - slog.Bool("has_review", project.hasReview), - slog.Bool("has_fix_agent", project.hasFixAgent)) - fmt.Fprintln(errOut, "Review preferences are stored in project settings (.entire/settings.json).") - fmt.Fprintln(errOut, "These are typically committed and may be visible to teammates.") - fmt.Fprintln(errOut, "Run `entire review --edit` interactively to move them to clone-local preferences.") - return nil - } - - if promptYN == nil { - promptYN = realPromptYN - } - migrate, err := promptYN(ctx, "Review preferences are stored in project settings (.entire/settings.json), which is typically committed. Move them to clone-local preferences so they stay private?", false) - if err != nil { - return fmt.Errorf("review settings migration prompt: %w", err) - } - if !migrate { - if prefs == nil { - prefs = &settings.ClonePreferences{} - } - prefs.ReviewMigrationDismissed = true - if err := settings.SaveClonePreferences(ctx, prefs); err != nil { - return fmt.Errorf("save migration dismissal: %w", err) - } - return nil - } - - moved, err := migrateProjectReviewSettings(ctx, project) - if err != nil { - return err - } - if moved { - fmt.Fprintln(out, "Moved review preferences from project settings to clone-local preferences.") - } else { - fmt.Fprintln(out, "Removed unused review keys from project settings; nothing to move.") - } - return nil -} - -func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, bool, error) { - path, raw, exists, err := settings.LoadProjectRaw(ctx) - if err != nil { - return nil, false, fmt.Errorf("review migration: %w", err) - } - if !exists { - return nil, false, nil - } - - reviewRaw, hasReview := raw["review"] - fixAgentRaw, hasFixAgent := raw["review_fix_agent"] - if !hasReview && !hasFixAgent { - return nil, false, nil - } - return &projectReviewSettings{ - path: path, - raw: raw, - review: reviewRaw, - fixAgent: fixAgentRaw, - hasReview: hasReview, - hasFixAgent: hasFixAgent, - }, true, nil -} - -// migrateProjectReviewSettings copies review keys from the project settings -// file into clone-local preferences and strips them from the project file. -// -// Returns moved=true when any review data was copied into prefs. When the -// project file's review keys are empty/null (or fully conflict with existing -// prefs, which is rejected upstream), moved=false but the project keys are -// still stripped as cleanup. -// -// Write ordering: prefs are saved first (atomic), then the project file is -// rewritten (atomic). Both writes use temp-then-rename so a crash mid-write -// leaves the original file intact rather than truncated. If the project -// rewrite fails after the prefs write succeeded, prefs precedence covers -// the gap until the next run. -func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSettings) (moved bool, err error) { - if project == nil { - return false, nil - } - - prefs, err := settings.LoadClonePreferences(ctx) - if err != nil { - return false, fmt.Errorf("load review preferences for migration: %w", err) - } - if prefs == nil { - prefs = &settings.ClonePreferences{} - } - - preferencesChanged := false - if project.hasReview && !isJSONNull(project.review) { - var projectReview map[string]settings.ReviewConfig - if err := json.Unmarshal(project.review, &projectReview); err != nil { - return false, fmt.Errorf("parsing project review settings: %w", err) - } - if len(projectReview) > 0 { - merged, mergedOK, conflicts := mergeProjectReviewIntoPrefs(prefs.Review, projectReview) - if len(conflicts) > 0 { - return false, fmt.Errorf( - "review settings exist in both %s and clone-local preferences for agent(s) %v; "+ - "reconcile manually by removing the redundant keys from %s, then re-run `entire review`", - project.path, conflicts, project.path, - ) - } - if mergedOK { - prefs.Review = merged - preferencesChanged = true - } - } - } - if project.hasFixAgent && !isJSONNull(project.fixAgent) { - var fixAgent string - if err := json.Unmarshal(project.fixAgent, &fixAgent); err != nil { - return false, fmt.Errorf("parsing project review_fix_agent: %w", err) - } - if fixAgent != "" { - if prefs.ReviewFixAgent != "" && prefs.ReviewFixAgent != fixAgent { - return false, fmt.Errorf( - "review_fix_agent differs between %s (%q) and clone-local preferences (%q); "+ - "reconcile manually by removing review_fix_agent from %s, then re-run `entire review`", - project.path, fixAgent, prefs.ReviewFixAgent, project.path, - ) - } - if prefs.ReviewFixAgent == "" { - prefs.ReviewFixAgent = fixAgent - preferencesChanged = true - } - } - } - - if preferencesChanged { - if err := settings.SaveClonePreferences(ctx, prefs); err != nil { - return false, fmt.Errorf("save review preferences for migration: %w", err) - } - } - - delete(project.raw, "review") - delete(project.raw, "review_fix_agent") - if err := settings.SaveProjectRaw(project.path, project.raw); err != nil { - return false, fmt.Errorf("save project settings after review migration: %w", err) - } - return preferencesChanged, nil -} - -// mergeProjectReviewIntoPrefs merges projectReview into the current prefs map. -// Per-agent conflicts (same key, different value) are surfaced rather than -// silently resolved — the caller can then refuse the migration with a clear -// message. Non-overlapping entries are merged. Returns ok=false when nothing -// would change (prefs already had every project entry verbatim). -func mergeProjectReviewIntoPrefs(prefs, projectReview map[string]settings.ReviewConfig) (merged map[string]settings.ReviewConfig, ok bool, conflicts []string) { - merged = make(map[string]settings.ReviewConfig, len(prefs)+len(projectReview)) - for k, v := range prefs { - merged[k] = v - } - changed := false - for k, projectV := range projectReview { - if existing, present := merged[k]; present { - if !reviewConfigEqual(existing, projectV) { - conflicts = append(conflicts, k) - } - continue - } - merged[k] = projectV - changed = true - } - if len(conflicts) > 0 { - return nil, false, conflicts - } - return merged, changed, nil -} - -func reviewConfigEqual(a, b settings.ReviewConfig) bool { - if a.Prompt != b.Prompt { - return false - } - if len(a.Skills) != len(b.Skills) { - return false - } - for i := range a.Skills { - if a.Skills[i] != b.Skills[i] { - return false - } - } - return true -} - -func isJSONNull(raw json.RawMessage) bool { - return bytes.Equal(bytes.TrimSpace(raw), []byte("null")) -} - -// localSettingsHasReviewKeys reports whether .entire/settings.local.json -// exists and contains either a "review" or "review_fix_agent" key. Both keys -// override clone-local preferences via mergeJSON's wholesale-replace path, -// so the migration must surface their presence rather than silently produce -// a state where the migrated config never takes effect. -// -// Returns the absolute path of the local settings file too, so callers can -// quote the exact location in the warning they show the user. -func localSettingsHasReviewKeys(ctx context.Context) (has bool, path string, err error) { - path, raw, exists, loadErr := settings.LoadLocalRaw(ctx) - if loadErr != nil { - return false, path, fmt.Errorf("local settings review-keys check: %w", loadErr) - } - if !exists { - return false, path, nil - } - _, hasReview := raw["review"] - _, hasFixAgent := raw["review_fix_agent"] - return hasReview || hasFixAgent, path, nil -} diff --git a/cmd/entire/cli/review/migration_test.go b/cmd/entire/cli/review/migration_test.go deleted file mode 100644 index 97d30eec0..000000000 --- a/cmd/entire/cli/review/migration_test.go +++ /dev/null @@ -1,421 +0,0 @@ -package review - -import ( - "bytes" - "context" - "encoding/json" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/entireio/cli/cmd/entire/cli/session" - "github.com/entireio/cli/cmd/entire/cli/settings" - "github.com/entireio/cli/cmd/entire/cli/testutil" -) - -func TestReviewSettingsMigration_MovesProjectReviewToClonePreferences(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectSettings := []byte(`{ - "enabled": true, - "log_level": "debug", - "review": {"claude-code": {"skills": ["/review"], "prompt": "project"}}, - "review_fix_agent": "claude-code" - }`) - projectPath := filepath.Join(entireDir, "settings.json") - if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - - prompted := false - promptQuestion := "" - var out bytes.Buffer - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(_ context.Context, question string, _ bool) (bool, error) { - prompted = true - promptQuestion = question - return true, nil - }); err != nil { - t.Fatalf("migration: %v", err) - } - if !prompted { - t.Fatal("expected migration prompt") - } - for _, want := range []string{"project settings", "clone-local preferences", "typically committed"} { - if !strings.Contains(promptQuestion, want) { - t.Fatalf("migration prompt = %q, want it to mention %q", promptQuestion, want) - } - } - - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if got := prefs.Review["claude-code"].Prompt; got != "project" { - t.Fatalf("migrated prompt = %q, want project", got) - } - if prefs.ReviewFixAgent != "claude-code" { - t.Fatalf("ReviewFixAgent = %q, want claude-code", prefs.ReviewFixAgent) - } - - raw := map[string]json.RawMessage{} - data, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - if err := json.Unmarshal(data, &raw); err != nil { - t.Fatalf("unmarshal project settings: %v", err) - } - if _, ok := raw["review"]; ok { - t.Fatalf("project review key was not removed: %s", data) - } - if _, ok := raw["review_fix_agent"]; ok { - t.Fatalf("project review_fix_agent key was not removed: %s", data) - } - if _, ok := raw["log_level"]; !ok { - t.Fatalf("unrelated project settings were not preserved: %s", data) - } -} - -// TestReviewSettingsMigration_MergesNonOverlappingPrefs verifies that when the -// project file has review keys for an agent NOT present in clone-local prefs, -// the migration merges them in. Previously the migration silently dropped any -// project config when prefs already had any review entry — that was data loss. -func TestReviewSettingsMigration_MergesNonOverlappingPrefs(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - projectSettings := []byte(`{ - "enabled": true, - "review": {"project-agent": {"prompt": "project"}} - }`) - if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - if err := settings.SaveClonePreferences(context.Background(), &settings.ClonePreferences{ - Review: map[string]settings.ReviewConfig{ - "local-agent": {Prompt: "local"}, - }, - }); err != nil { - t.Fatalf("seed preferences: %v", err) - } - - var out bytes.Buffer - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { - return true, nil - }); err != nil { - t.Fatalf("migration: %v", err) - } - - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if got := prefs.Review["local-agent"].Prompt; got != "local" { - t.Fatalf("local prompt = %q, want preserved as %q", got, "local") - } - if got := prefs.Review["project-agent"].Prompt; got != "project" { - t.Fatalf("project prompt = %q, want merged in as %q", got, "project") - } - - data, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - raw := map[string]json.RawMessage{} - if err := json.Unmarshal(data, &raw); err != nil { - t.Fatalf("unmarshal project settings: %v", err) - } - if _, ok := raw["review"]; ok { - t.Fatalf("project review key was not removed: %s", data) - } -} - -// TestReviewSettingsMigration_RefusesConflictingPrefs verifies that when both -// the project file and clone-local prefs have review config for the SAME agent -// with DIFFERENT values, the migration aborts with a clear error rather than -// silently dropping one side. The user must reconcile manually. -func TestReviewSettingsMigration_RefusesConflictingPrefs(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - projectSettings := []byte(`{ - "enabled": true, - "review": {"claude-code": {"prompt": "project"}} - }`) - if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - if err := settings.SaveClonePreferences(context.Background(), &settings.ClonePreferences{ - Review: map[string]settings.ReviewConfig{ - "claude-code": {Prompt: "local"}, - }, - }); err != nil { - t.Fatalf("seed preferences: %v", err) - } - - var out bytes.Buffer - err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { - return true, nil - }) - if err == nil { - t.Fatal("expected migration to refuse conflicting prefs") - } - if !strings.Contains(err.Error(), "claude-code") { - t.Errorf("error = %q, want it to name the conflicting agent (claude-code)", err.Error()) - } - if !strings.Contains(err.Error(), "reconcile manually") { - t.Errorf("error = %q, want it to guide manual reconciliation", err.Error()) - } - - // Project file must NOT have been rewritten on the conflict path. - data, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - if !bytes.Contains(data, []byte("claude-code")) { - t.Fatalf("project file was modified despite conflict abort: %s", data) - } - - // Clone prefs must be unchanged. - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if got := prefs.Review["claude-code"].Prompt; got != "local" { - t.Errorf("local prompt = %q, want unchanged as %q", got, "local") - } -} - -// TestReviewSettingsMigration_NoMoveCleansUpKeys verifies the cleanup-only -// path: project has only `null` values for review keys, so nothing actually -// moves, but the project keys are still stripped and the success message -// reflects that distinction. -func TestReviewSettingsMigration_NoMoveCleansUpKeys(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - if err := os.WriteFile(projectPath, []byte(`{ - "enabled": true, - "review": null, - "review_fix_agent": null - }`), 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - - var out bytes.Buffer - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { - return true, nil - }); err != nil { - t.Fatalf("migration: %v", err) - } - if !strings.Contains(out.String(), "Removed unused review keys") { - t.Errorf("output = %q, want the cleanup-only message", out.String()) - } - - data, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - raw := map[string]json.RawMessage{} - if err := json.Unmarshal(data, &raw); err != nil { - t.Fatalf("unmarshal project settings: %v", err) - } - if _, ok := raw["review"]; ok { - t.Fatalf("project review key was not removed: %s", data) - } -} - -// TestReviewSettingsMigration_DeclinePersistsDismissal verifies that declining -// the prompt records ReviewMigrationDismissed in clone-local prefs, and that a -// subsequent invocation does NOT re-prompt. Without this, teams who -// intentionally commit review prefs would be re-prompted on every command. -func TestReviewSettingsMigration_DeclinePersistsDismissal(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - projectSettings := []byte(`{ - "enabled": true, - "review": {"claude-code": {"prompt": "project"}} - }`) - if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - - // First invocation: user declines. - var out bytes.Buffer - promptCount := 0 - declineThenFail := func(context.Context, string, bool) (bool, error) { - promptCount++ - return false, nil - } - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, declineThenFail); err != nil { - t.Fatalf("first invocation: %v", err) - } - if promptCount != 1 { - t.Errorf("first invocation prompted %d times, want 1", promptCount) - } - - // Dismissal must be persisted. - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if prefs == nil || !prefs.ReviewMigrationDismissed { - t.Fatalf("ReviewMigrationDismissed = false, want true after decline (prefs = %+v)", prefs) - } - - // Project file must be untouched on decline. - data, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - if !bytes.Contains(data, []byte("claude-code")) { - t.Errorf("project file was modified on decline: %s", data) - } - - // Second invocation: must NOT re-prompt. - failIfPrompted := func(context.Context, string, bool) (bool, error) { - t.Fatal("prompt should not be called when dismissal is persisted") - return false, nil - } - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, failIfPrompted); err != nil { - t.Fatalf("second invocation: %v", err) - } -} - -func TestReviewSettingsMigration_SkipsWhenProjectHasNoReviewKeys(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - if err := os.WriteFile(projectPath, []byte(`{"enabled":true,"log_level":"debug"}`), 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - - var out bytes.Buffer - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { - t.Fatal("prompt should not be called") - return false, nil - }); err != nil { - t.Fatalf("migration: %v", err) - } - - preferencesPath, err := settings.ClonePreferencesPath(context.Background()) - if err != nil { - t.Fatalf("preferences path: %v", err) - } - if _, err := os.Stat(preferencesPath); !os.IsNotExist(err) { - t.Fatalf("preferences file exists after no-op migration: %v", err) - } -} - -// TestReviewSettingsMigration_BailsOnLocalSettingsReviewKeys pins the -// precondition: when .entire/settings.local.json has review keys, those -// override clone-local preferences via mergeJSON's wholesale-replace path, -// so the migration must surface the conflict up front rather than silently -// produce a migrated-but-masked state. Bailing also intentionally does NOT -// set ReviewMigrationDismissed — this is a fixable precondition, not a -// rejected migration, and the user should be re-prompted after cleaning -// settings.local.json. -func TestReviewSettingsMigration_BailsOnLocalSettingsReviewKeys(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - session.ClearGitCommonDirCache() - - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatalf("mkdir .entire: %v", err) - } - projectPath := filepath.Join(entireDir, "settings.json") - projectSettings := []byte(`{ - "enabled": true, - "review": {"claude-code": {"prompt": "project"}} - }`) - if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { - t.Fatalf("write project settings: %v", err) - } - localPath := filepath.Join(entireDir, "settings.local.json") - localSettings := []byte(`{"review": {"local-agent": {"prompt": "local"}}}`) - if err := os.WriteFile(localPath, localSettings, 0o600); err != nil { - t.Fatalf("write local settings: %v", err) - } - - var out, errOut bytes.Buffer - if err := maybePromptReviewSettingsMigration(context.Background(), &out, &errOut, true, func(context.Context, string, bool) (bool, error) { - t.Fatal("prompt should not be called when settings.local.json has review keys") - return false, nil - }); err != nil { - t.Fatalf("migration: %v", err) - } - - stderr := errOut.String() - for _, want := range []string{"settings.local.json", "review", "Remove"} { - if !strings.Contains(stderr, want) { - t.Errorf("stderr = %q, want it to mention %q", stderr, want) - } - } - - // Project file must NOT have been rewritten — the bail path leaves - // everything in place so the user can clean settings.local.json and - // re-run. - got, err := os.ReadFile(projectPath) - if err != nil { - t.Fatalf("read project settings: %v", err) - } - if !bytes.Contains(got, []byte(`"claude-code"`)) { - t.Fatalf("project file was modified despite bail; got: %s", got) - } - - // Dismissal must NOT be persisted — the user didn't choose to dismiss, - // they hit a fixable precondition. Next run should re-prompt. - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if prefs != nil && prefs.ReviewMigrationDismissed { - t.Fatalf("ReviewMigrationDismissed = true after bail; should not persist a fixable precondition as dismissal") - } -} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index caf226f4c..28ef6b227 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -50,17 +50,17 @@ func newAccessibleForm(groups ...*huh.Group) *huh.Form { // setup phase explicit, and the trailing "running review now" line in the // caller closes the loop on what comes next. func ConfirmFirstRunSetup(ctx context.Context, out io.Writer) bool { - fmt.Fprintln(out, "No review config found — let's set one up first.") + fmt.Fprintln(out, "No review profiles found — let's set one up first.") fmt.Fprintln(out) - fmt.Fprintln(out, "You'll pick skills for each installed agent. They're saved to") - fmt.Fprintln(out, "local review preferences; edit later with `entire review --edit`.") - fmt.Fprintln(out, "After setup, the review will run with your selection.") + fmt.Fprintln(out, "You'll choose a review type and worker agents. They're saved to") + fmt.Fprintln(out, "local review preferences; configure later with `entire review --configure`.") + fmt.Fprintln(out, "After setup, you can start the review immediately.") fmt.Fprintln(out) proceed := true form := newAccessibleForm(huh.NewGroup( huh.NewConfirm(). - Title("Set up review skills now?"). + Title("Set up review now?"). Affirmative("Yes"). Negative("Cancel"). Value(&proceed), @@ -75,6 +75,73 @@ func ConfirmFirstRunSetup(ctx context.Context, out io.Writer) bool { return proceed } +// RunReviewGuidedSetup is the simple config path for `entire review`. +// It intentionally avoids the per-agent skills picker: users choose the review +// profile and worker agents, then Entire fills in opinionated per-agent +// defaults. Advanced skill-level editing remains available via --edit. +func RunReviewGuidedSetup( + ctx context.Context, + out io.Writer, + installed []types.AgentName, + reviewerFor func(string) reviewtypes.AgentReviewer, + profileName string, + firstRun bool, +) (string, settings.ReviewProfileConfig, error) { + if firstRun { + if !ConfirmFirstRunSetup(ctx, out) { + return "", settings.ReviewProfileConfig{}, ErrPickerCancelled + } + } else { + fmt.Fprintln(out, "Configure a review profile.") + fmt.Fprintln(out, "You'll choose a review type and worker agents. Skill details use opinionated defaults.") + fmt.Fprintln(out) + } + + launchable := launchableInstalledAgentNames(installed, reviewerFor) + if len(launchable) == 0 { + return "", settings.ReviewProfileConfig{}, errors.New("no agents with review runner adapters and hooks installed; run `entire configure --agent claude-code`, `entire configure --agent codex`, or `entire configure --agent gemini`") + } + + profileName = strings.TrimSpace(profileName) + profileWasProvided := profileName != "" + if profileName == "" { + profileName = DefaultProfileName + } + if !profileWasProvided { + pickedProfile, err := promptForSimpleReviewProfile(ctx) + if err != nil { + return "", settings.ReviewProfileConfig{}, err + } + profileName = pickedProfile + } + + selected := launchable + if len(launchable) > 1 { + picked, err := promptForSimpleReviewAgents(ctx, launchable) + if err != nil { + return "", settings.ReviewProfileConfig{}, err + } + selected = picked + } + profile, err := defaultReviewProfileForInstalledAgents(ctx, profileName, agentNamesToTypes(selected), reviewerFor) + if err != nil { + return "", settings.ReviewProfileConfig{}, err + } + if err := promptForSimpleReviewModels(ctx, profileName, &profile, selected); err != nil { + return "", settings.ReviewProfileConfig{}, err + } + if len(profile.Agents) > 1 { + master, err := promptForSimpleReviewMaster(ctx, profile) + if err != nil { + return "", settings.ReviewProfileConfig{}, err + } + profile.Master = master + } + fmt.Fprintf(out, "Saved %q review profile with %s.\n", profileName, strings.Join(sortedProfileAgentNames(profile), ", ")) + fmt.Fprintln(out) + return profileName, profile, nil +} + // RunReviewConfigPicker presents a huh multi-select for each installed agent // that has curated review skills, and saves the selection to // clone-local review preferences. Previously-saved skills are pre-checked via @@ -82,7 +149,284 @@ func ConfirmFirstRunSetup(ctx context.Context, out io.Writer) bool { // selections in its own agent picker. // // getInstalled is injected to avoid an import cycle with the cli package. +func launchableInstalledAgentNames(installed []types.AgentName, reviewerFor func(string) reviewtypes.AgentReviewer) []string { + names := make([]string, 0, len(installed)) + for _, name := range installed { + if reviewerFor != nil && reviewerFor(string(name)) == nil { + continue + } + names = append(names, string(name)) + } + sort.Strings(names) + return names +} + +func agentNamesToTypes(names []string) []types.AgentName { + out := make([]types.AgentName, len(names)) + for i, name := range names { + out[i] = types.AgentName(name) + } + return out +} + +func promptForSimpleReviewProfile(ctx context.Context) (string, error) { + picked := DefaultProfileName + form := newAccessibleForm(huh.NewGroup( + huh.NewSelect[string](). + Title("What kind of review?"). + Options( + huh.NewOption("General — correctness, regressions, tests", DefaultProfileName), + huh.NewOption("Security — auth, injection, secrets", "security"), + huh.NewOption("Accessibility — keyboard, screen readers, contrast", "accessibility"), + ). + Value(&picked), + )) + if err := form.RunWithContext(ctx); err != nil { + return "", fmt.Errorf("review profile picker: %w", err) + } + return picked, nil +} + +func promptForSimpleReviewAgents(ctx context.Context, launchable []string) ([]string, error) { + options := make([]huh.Option[string], 0, len(launchable)) + for _, name := range launchable { + options = append(options, huh.NewOption(labelForSimpleAgent(name), name).Selected(true)) + } + picked := append([]string(nil), launchable...) + form := newAccessibleForm(huh.NewGroup( + huh.NewMultiSelect[string](). + Title("Which agents should review?"). + Description("All are selected by default. Space toggles, enter confirms."). + Options(options...). + Height(len(options) + 2). + Value(&picked), + )) + if err := form.RunWithContext(ctx); err != nil { + return nil, fmt.Errorf("review agent picker: %w", err) + } + if len(picked) == 0 { + return nil, ErrNoAgentsSelected + } + sort.Strings(picked) + return picked, nil +} + +func promptForSimpleReviewModels(ctx context.Context, profileName string, profile *settings.ReviewProfileConfig, selectedAgents []string) error { + customize := false + form := newAccessibleForm(huh.NewGroup( + huh.NewConfirm(). + Title("Choose models?"). + Description("Optional. Leave this off to use each agent's default model."). + Affirmative("Choose models"). + Negative("Use defaults"). + Value(&customize), + )) + if err := form.RunWithContext(ctx); err != nil { + return fmt.Errorf("review model setup: %w", err) + } + if !customize { + return nil + } + + for _, workerName := range sortedProfileAgentNames(*profile) { + cfg := profile.Agents[workerName] + agentName := reviewAgentName(workerName, cfg) + model, err := promptForModelChoice(ctx, labelForSimpleAgent(agentName), agentName, strings.TrimSpace(cfg.Model)) + if err != nil { + return fmt.Errorf("review model picker for %s: %w", workerName, err) + } + cfg.Model = model + profile.Agents[workerName] = cfg + } + + for { + addVariant := false + variantForm := newAccessibleForm(huh.NewGroup( + huh.NewConfirm(). + Title("Add another model variant?"). + Description("Use this to run the same agent more than once with different models."). + Affirmative("Add variant"). + Negative("Done"). + Value(&addVariant), + )) + if err := variantForm.RunWithContext(ctx); err != nil { + return fmt.Errorf("review model variant picker: %w", err) + } + if !addVariant { + return nil + } + agentName := selectedAgents[0] + if len(selectedAgents) > 1 { + options := make([]huh.Option[string], 0, len(selectedAgents)) + for _, name := range selectedAgents { + options = append(options, huh.NewOption(labelForSimpleAgent(name), name)) + } + selectForm := newAccessibleForm(huh.NewGroup( + huh.NewSelect[string](). + Title("Which agent should get another model?"). + Options(options...).Value(&agentName), + )) + if err := selectForm.RunWithContext(ctx); err != nil { + return fmt.Errorf("review model variant agent picker: %w", err) + } + } + model, err := promptForModelChoice(ctx, labelForSimpleAgent(agentName), agentName, "") + if err != nil { + return fmt.Errorf("review model variant value: %w", err) + } + if model == "" { + continue + } + cfg := defaultReviewAgentConfig(profileName, agentName) + cfg.Agent = agentName + cfg.Model = model + workerName := workerIDForAgentModel(agentName, model, profile.Agents) + profile.Agents[workerName] = cfg + } +} + +func labelForSimpleAgent(name string) string { + ag, err := agent.Get(types.AgentName(name)) + if err != nil { + return name + } + return string(ag.Type()) +} + +// reviewModelCustomSentinel is the select value for "type a custom model". +// It cannot collide with a real model id (which never contains spaces). +const reviewModelCustomSentinel = "__custom__" + +// promptForModelChoice asks for a worker's model. When the agent advertises +// models (agent.ModelLister) it shows a select of those plus "Default" and +// "Custom…"; otherwise it falls back to a free-text input. current pre-selects +// the existing value. Returns the chosen model ("" means the agent default). +func promptForModelChoice(ctx context.Context, displayLabel, agentName, current string) (string, error) { + models := listAgentModelOptions(ctx, agentName) + if len(models) == 0 { + model := current + form := newAccessibleForm(huh.NewGroup( + huh.NewInput(). + Title("Model for " + displayLabel). + Description("Optional; any value accepted by the agent CLI."). + Value(&model), + )) + if err := form.RunWithContext(ctx); err != nil { + return "", fmt.Errorf("model input: %w", err) + } + return strings.TrimSpace(model), nil + } + + options := make([]huh.Option[string], 0, len(models)+2) + options = append(options, huh.NewOption("Default (agent's own default model)", "")) + for _, m := range models { + label := m.ID + if m.Note != "" { + label = m.ID + " — " + m.Note + } + options = append(options, huh.NewOption(label, m.ID)) + } + options = append(options, huh.NewOption("Custom… (type any value)", reviewModelCustomSentinel)) + + picked := current + if current != "" && !modelInList(current, models) { + picked = reviewModelCustomSentinel + } + form := newAccessibleForm(huh.NewGroup( + huh.NewSelect[string](). + Title("Model for " + displayLabel). + Description("Pick a model, Default, or Custom… to type any value."). + Options(options...). + Height(reviewPickerHeight(len(options))). + Value(&picked), + )) + if err := form.RunWithContext(ctx); err != nil { + return "", fmt.Errorf("model picker: %w", err) + } + if picked != reviewModelCustomSentinel { + return picked, nil + } + + model := current + customForm := newAccessibleForm(huh.NewGroup( + huh.NewInput(). + Title("Custom model for " + displayLabel). + Description("Any value accepted by the agent CLI."). + Value(&model), + )) + if err := customForm.RunWithContext(ctx); err != nil { + return "", fmt.Errorf("custom model input: %w", err) + } + return strings.TrimSpace(model), nil +} + +func listAgentModelOptions(ctx context.Context, agentName string) []agent.ModelInfo { + ag, err := agent.Get(types.AgentName(agentName)) + if err != nil { + return nil + } + lister, ok := agent.AsModelLister(ag) + if !ok { + return nil + } + models, err := lister.ListModels(ctx) + if err != nil { + return nil + } + return models +} + +func modelInList(id string, models []agent.ModelInfo) bool { + for _, m := range models { + if m.ID == id { + return true + } + } + return false +} + +func promptForSimpleReviewMaster(ctx context.Context, profile settings.ReviewProfileConfig) (string, error) { + choices := reviewMasterAgentChoices(profile.Agents) + if len(choices) == 0 { + return "", errors.New("no selected review agent can write the final report") + } + if len(choices) == 1 { + return choices[0].Name, nil + } + return promptForReviewMasterAgent(ctx, choices, profile.Master) +} + +func ConfirmRunReviewNow(ctx context.Context, out io.Writer) (bool, error) { + runNow := true + form := newAccessibleForm(huh.NewGroup( + huh.NewConfirm(). + Title("Start review now?"). + Affirmative("Start review"). + Negative("Not now"). + Value(&runNow), + )) + if err := form.RunWithContext(ctx); err != nil { + // Aborting the confirm (Ctrl+C / Esc) is a clean "not now", not a + // command error. Surface it as picker-cancelled so the caller maps it + // to a silent exit via handlePickerError. + fmt.Fprintln(out, "Review not started. Run `entire review` when ready.") + return false, ErrPickerCancelled + } + if !runNow { + fmt.Fprintln(out, "Review not started. Run `entire review` when ready.") + } + return runNow, nil +} + func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func(context.Context) []types.AgentName) (map[string]settings.ReviewConfig, error) { + return RunReviewProfileConfigPicker(ctx, out, getInstalled, DefaultProfileName) +} + +func RunReviewProfileConfigPicker(ctx context.Context, out io.Writer, getInstalled func(context.Context) []types.AgentName, profileName string) (map[string]settings.ReviewConfig, error) { + profileName = strings.TrimSpace(profileName) + if profileName == "" { + profileName = DefaultProfileName + } installed := getInstalled(ctx) if len(installed) == 0 { return nil, errors.New( @@ -126,18 +470,20 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func ) } - // Load existing config so we can pre-check saved skills and seed saved - // prompts. A load error here means the settings file is malformed; log - // at Warn so users debugging "my saved skills aren't pre-checked" can + // Load existing profile config so we can pre-check saved skills and seed + // saved prompts. A load error here means the settings file is malformed; + // 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{} - existingFixAgent := "" + existingMaster := "" 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 { - existing = s.Review - existingFixAgent = s.ReviewFixAgent + if profile, ok := s.ReviewProfiles[profileName]; ok { + existing = profile.Agents + existingMaster = profile.Master + } } // Up-front header: make the order and count obvious so users can spot @@ -182,11 +528,16 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func existing[string(c.name)].Skills, curated, discovered, ) prompt := existing[string(c.name)].Prompt + model := existing[string(c.name)].Model fields := BuildReviewPickerFields( string(c.name), curated, discovered, activeHints, prompt, &builtinPicks, &discoveredPicks, &prompt, ) + fields = append(fields, huh.NewInput(). + Title("Model (optional)"). + Description("Leave blank to use the agent default; pass any model string accepted by the agent CLI."). + Value(&model)) // Prepend a non-blocking header Note so the agent being configured // is always clearly visible. @@ -201,6 +552,7 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func } cfg := settings.ReviewConfig{ + Model: strings.TrimSpace(model), Skills: dedupeStrings(append(builtinPicks, discoveredPicks...)), Prompt: strings.TrimSpace(prompt), } @@ -223,14 +575,14 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func return nil, errors.New("no review skills or prompt configured") } - fixAgent, err := pickReviewFixAgentPreference(ctx, merged, existingFixAgent) + masterAgent, err := pickReviewMasterAgentPreference(ctx, merged, existingMaster) if err != nil { return nil, err } - if err := saveReviewConfigAndFixAgent(ctx, merged, fixAgent); err != nil { + if err := saveReviewProfileConfig(ctx, profileName, merged, masterAgent); err != nil { return nil, err } - fmt.Fprintln(out, "Saved review config to local review preferences. Edit later with `entire review --edit`.") + fmt.Fprintf(out, "Saved review profile %q to local review preferences. Edit later with `entire review --edit --profile %s`.\n", profileName, profileName) return merged, nil } @@ -255,7 +607,7 @@ func MergePickerResults(existing map[string]settings.ReviewConfig, offered map[s return merged } -func SaveReviewFixAgent(ctx context.Context, agentName string) error { +func saveReviewProfileConfig(ctx context.Context, profileName string, agents map[string]settings.ReviewConfig, master string) error { prefs, err := settings.LoadClonePreferences(ctx) if err != nil { return fmt.Errorf("load review preferences before save: %w", err) @@ -263,59 +615,123 @@ func SaveReviewFixAgent(ctx context.Context, agentName string) error { if prefs == nil { prefs = &settings.ClonePreferences{} } - prefs.ReviewFixAgent = agentName - if err := settings.SaveClonePreferences(ctx, prefs); err != nil { - return fmt.Errorf("save review preferences: %w", err) + if prefs.ReviewProfiles == nil { + prefs.ReviewProfiles = map[string]settings.ReviewProfileConfig{} } - return nil -} - -func saveReviewConfigAndFixAgent(ctx context.Context, review map[string]settings.ReviewConfig, fixAgent string) error { - prefs, err := settings.LoadClonePreferences(ctx) - if err != nil { - return fmt.Errorf("load review preferences before save: %w", err) + // Merge into any existing profile so the advanced skills picker only + // rewrites what it actually edits (agents + master). Profile-level fields + // the picker never surfaces — custom `task` text and `master_model` — are + // preserved instead of being clobbered with built-in defaults. + profile := prefs.ReviewProfiles[profileName] + profile.Agents = agents + profile.Master = master + if strings.TrimSpace(profile.Task) == "" { + profile.Task = profileTask(profileName, settings.ReviewProfileConfig{}) } - if prefs == nil { - prefs = &settings.ClonePreferences{} + prefs.ReviewProfiles[profileName] = profile + if prefs.ReviewDefaultProfile == "" { + prefs.ReviewDefaultProfile = profileName } - prefs.Review = review - prefs.ReviewFixAgent = fixAgent if err := settings.SaveClonePreferences(ctx, prefs); err != nil { return fmt.Errorf("save review preferences: %w", err) } return nil } -func pickReviewFixAgentPreference(ctx context.Context, review map[string]settings.ReviewConfig, current string) (string, error) { - choices := reviewFixAgentChoices(review) +func pickReviewMasterAgentPreference(ctx context.Context, review map[string]settings.ReviewConfig, current string) (string, error) { + choices := reviewMasterAgentChoices(review) switch len(choices) { case 0: return current, nil case 1: return choices[0].Name, nil default: - return promptForReviewFixAgent(ctx, choices, current) + return promptForReviewMasterAgent(ctx, choices, current) } } -// ComputeEligibleConfigured returns the sorted list of agents that are both -// configured (non-zero ReviewConfig entry) AND have hooks installed. Only -// eligible agents are valid picker targets — spawning a review for an agent -// without hooks would silently drop the review metadata. -func ComputeEligibleConfigured(s *settings.EntireSettings, installed []types.AgentName) []AgentChoice { - if s == nil { - return nil +// defaultAgentPick returns the saved choice if it is still offered, otherwise +// the first choice. Shared by the master picker. +func defaultAgentPick(choices []AgentChoice, saved string) string { + if pick, ok := savedAgentPick(choices, saved); ok { + return pick + } + if len(choices) == 0 { + return "" + } + return choices[0].Name +} + +func savedAgentPick(choices []AgentChoice, saved string) (string, bool) { + for _, choice := range choices { + if choice.Name == saved { + return saved, true + } + } + return "", false +} + +func reviewMasterAgentChoices(configured map[string]settings.ReviewConfig) []AgentChoice { + choices := make([]AgentChoice, 0, len(configured)) + for name, cfg := range configured { + if cfg.IsZero() { + continue + } + agentName := reviewAgentName(name, cfg) + ag, err := agent.Get(types.AgentName(agentName)) + if err != nil { + continue + } + if _, ok := agent.AsTextGenerator(ag); !ok { + continue + } + label := string(ag.Type()) + if name != agentName || strings.TrimSpace(cfg.Model) != "" { + label = reviewWorkerLabel(name, cfg) + } + choices = append(choices, AgentChoice{Name: name, Label: label}) + } + sort.Slice(choices, func(i, j int) bool { return choices[i].Name < choices[j].Name }) + return choices +} + +func promptForReviewMasterAgent(ctx context.Context, choices []AgentChoice, saved string) (string, error) { + options := make([]huh.Option[string], 0, len(choices)) + for _, choice := range choices { + options = append(options, huh.NewOption(choice.Label, choice.Name)) } + picked := defaultAgentPick(choices, saved) + form := newAccessibleForm(huh.NewGroup( + huh.NewSelect[string](). + Title("Choose review master"). + Description("The master critically evaluates worker reports and writes the final report."). + Options(options...). + Height(reviewPickerHeight(len(options))). + Value(&picked), + )) + if err := form.RunWithContext(ctx); err != nil { + return "", fmt.Errorf("review master picker: %w", err) + } + return picked, nil +} + +// ComputeEligibleConfiguredForProfile returns the sorted list of agents in a +// profile that are both configured and have hooks installed. +func ComputeEligibleConfiguredForProfile(profile settings.ReviewProfileConfig, installed []types.AgentName) []AgentChoice { + return eligibleAgentChoices(profile.Agents, installed) +} + +func eligibleAgentChoices(configured map[string]settings.ReviewConfig, installed []types.AgentName) []AgentChoice { installedSet := make(map[types.AgentName]struct{}, len(installed)) for _, name := range installed { installedSet[name] = struct{}{} } - out := make([]AgentChoice, 0, len(s.Review)) - for name, cfg := range s.Review { + out := make([]AgentChoice, 0, len(configured)) + for name, cfg := range configured { if cfg.IsZero() { continue } - if _, ok := installedSet[types.AgentName(name)]; !ok { + if _, ok := installedSet[types.AgentName(reviewAgentName(name, cfg))]; !ok { continue } out = append(out, AgentChoice{Name: name, Label: labelForAgentChoice(name, cfg)}) @@ -326,32 +742,39 @@ func ComputeEligibleConfigured(s *settings.EntireSettings, installed []types.Age // labelForAgentChoice builds the picker-visible label for an agent row. func labelForAgentChoice(name string, cfg settings.ReviewConfig) string { + label := reviewWorkerLabel(name, cfg) switch { case len(cfg.Skills) > 0: - return fmt.Sprintf("%s (%d skills configured)", name, len(cfg.Skills)) + return fmt.Sprintf("%s (%d skills configured)", label, len(cfg.Skills)) case cfg.Prompt != "": - return name + " (prompt-only)" + return label + " (prompt-only)" default: - return name + return label } } -// computeLaunchableEligible returns the subset of ComputeEligibleConfigured -// that also have a non-nil AgentReviewer (i.e., are launchable by the CLI). -// Used by the dispatch fork in cmd.go to decide whether to route to the -// multi-agent path. +// computeLaunchableEligibleForProfile returns the subset of +// ComputeEligibleConfiguredForProfile that also have a non-nil AgentReviewer. +// "Launchable" here is a historical shorthand for "has an Entire review-runner +// adapter"; it is not a claim about whether the agent's own CLI supports +// headless execution. // -// reviewerFor is deps.ReviewerFor injected at the cmd layer; it returns nil -// for non-launchable agents (cursor, opencode, factoryai-droid, copilot-cli). -func computeLaunchableEligible( - s *settings.EntireSettings, +// reviewerFor is deps.ReviewerFor injected at the cmd layer; it returns nil for +// agents that are known to Entire but not yet wired into `entire review`. +func computeLaunchableEligibleForProfile( + profile settings.ReviewProfileConfig, installed []types.AgentName, reviewerFor func(string) reviewtypes.AgentReviewer, ) []AgentChoice { - eligible := ComputeEligibleConfigured(s, installed) + eligible := ComputeEligibleConfiguredForProfile(profile, installed) + return filterLaunchableEligibleForProfile(profile, eligible, reviewerFor) +} + +func filterLaunchableEligibleForProfile(profile settings.ReviewProfileConfig, eligible []AgentChoice, reviewerFor func(string) reviewtypes.AgentReviewer) []AgentChoice { out := make([]AgentChoice, 0, len(eligible)) for _, c := range eligible { - if reviewerFor(c.Name) != nil { + cfg := profile.Agents[c.Name] + if reviewerFor(reviewAgentName(c.Name, cfg)) != nil { out = append(out, c) } } diff --git a/cmd/entire/cli/review/picker_test.go b/cmd/entire/cli/review/picker_test.go index 208b4c0bb..27a49dde0 100644 --- a/cmd/entire/cli/review/picker_test.go +++ b/cmd/entire/cli/review/picker_test.go @@ -1,7 +1,6 @@ package review_test import ( - "context" "reflect" "strings" "testing" @@ -10,7 +9,6 @@ import ( "github.com/entireio/cli/cmd/entire/cli/agent/skilldiscovery" "github.com/entireio/cli/cmd/entire/cli/review" "github.com/entireio/cli/cmd/entire/cli/settings" - "github.com/entireio/cli/cmd/entire/cli/testutil" ) const ( @@ -286,21 +284,3 @@ func TestBuildReviewPickerFields_SingleBuiltinDefaultsSelectedAndRenders(t *test t.Fatalf("single built-in option did not render:\n%s", got) } } - -func TestSaveReviewFixAgent_PersistsSettings(t *testing.T) { - tmp := t.TempDir() - testutil.InitRepo(t, tmp) - t.Chdir(tmp) - - if err := review.SaveReviewFixAgent(context.Background(), testCodexAgent); err != nil { - t.Fatal(err) - } - - prefs, err := settings.LoadClonePreferences(context.Background()) - if err != nil { - t.Fatalf("load preferences: %v", err) - } - if prefs.ReviewFixAgent != testCodexAgent { - t.Fatalf("ReviewFixAgent = %q, want %s", prefs.ReviewFixAgent, testCodexAgent) - } -} diff --git a/cmd/entire/cli/review/profile.go b/cmd/entire/cli/review/profile.go new file mode 100644 index 000000000..0f5ff9e55 --- /dev/null +++ b/cmd/entire/cli/review/profile.go @@ -0,0 +1,360 @@ +package review + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/agent" + "github.com/entireio/cli/cmd/entire/cli/agent/types" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +const DefaultProfileName = "general" + +const ( + defaultGeneralTask = "Review this change for correctness, regressions, API design, missing tests, maintainability, and user-facing behavior changes. Report only actionable findings with concrete evidence." + defaultSecurityTask = "Review this change for security vulnerabilities: authentication and authorization bugs, injection risks, secrets exposure, unsafe dependency or deserialization behavior, privilege-boundary mistakes, insecure defaults, and data leakage. Report only actionable findings with concrete evidence." + defaultAccessibilityTask = "Review this change for accessibility regressions: keyboard navigation, focus management, semantic markup, labels, ARIA correctness, color contrast, reduced-motion behavior, screen-reader behavior, and inclusive error states. Report only actionable findings with concrete evidence." +) + +// profileTask returns the configured task, or a built-in task for conventional +// profile names when the config leaves task empty. +func profileTask(name string, cfg settings.ReviewProfileConfig) string { + if strings.TrimSpace(cfg.Task) != "" { + return strings.TrimSpace(cfg.Task) + } + switch strings.ToLower(name) { + case "", DefaultProfileName: + return defaultGeneralTask + case "security": + return defaultSecurityTask + case "accessibility", "a11y": + return defaultAccessibilityTask + default: + return defaultGeneralTask + } +} + +// selectReviewProfile resolves the profile to run. No legacy fallback is used: +// users must configure review_profiles (the command is experimental, so there +// is intentionally no migration from the old review map). +func selectReviewProfile(s *settings.EntireSettings, override string) (string, settings.ReviewProfileConfig, error) { + if s == nil || len(s.ReviewProfiles) == 0 { + return "", settings.ReviewProfileConfig{}, errors.New("no review profiles configured; run `entire review --configure` or add review_profiles to Entire preferences") + } + profiles := nonZeroProfiles(s.ReviewProfiles) + if len(profiles) == 0 { + return "", settings.ReviewProfileConfig{}, errors.New("no review profiles configured; every profile is empty") + } + + name := strings.TrimSpace(override) + if name == "" { + name = strings.TrimSpace(s.ReviewDefaultProfile) + } + if name == "" { + if _, ok := profiles[DefaultProfileName]; ok { + name = DefaultProfileName + } else if len(profiles) == 1 { + for only := range profiles { + name = only + } + } else { + return "", settings.ReviewProfileConfig{}, fmt.Errorf( + "multiple review profiles configured (%s); pass a profile name or set review_default_profile", + strings.Join(sortedProfileNames(profiles), ", ")) + } + } + + cfg, ok := profiles[name] + if !ok { + return "", settings.ReviewProfileConfig{}, fmt.Errorf( + "review profile %q is not configured; configured profiles: %s", + name, strings.Join(sortedProfileNames(profiles), ", ")) + } + if len(nonZeroAgentConfigs(cfg.Agents)) == 0 { + return "", settings.ReviewProfileConfig{}, fmt.Errorf("review profile %q has no configured agents", name) + } + return name, cfg, nil +} + +func nonZeroProfiles(in map[string]settings.ReviewProfileConfig) map[string]settings.ReviewProfileConfig { + out := make(map[string]settings.ReviewProfileConfig, len(in)) + for name, cfg := range in { + name = strings.TrimSpace(name) + if name == "" || cfg.IsZero() { + continue + } + out[name] = cfg + } + return out +} + +func sortedProfileNames(in map[string]settings.ReviewProfileConfig) []string { + names := make([]string, 0, len(in)) + for name := range in { + names = append(names, name) + } + sort.Strings(names) + return names +} + +func nonZeroAgentConfigs(in map[string]settings.ReviewConfig) map[string]settings.ReviewConfig { + out := make(map[string]settings.ReviewConfig, len(in)) + for name, cfg := range in { + name = strings.TrimSpace(name) + if name == "" || cfg.IsZero() { + continue + } + out[name] = cfg + } + return out +} + +func sortedProfileAgentNames(profile settings.ReviewProfileConfig) []string { + names := make([]string, 0, len(profile.Agents)) + for name := range profile.Agents { + names = append(names, name) + } + sort.Strings(names) + return names +} + +func reviewAgentName(workerName string, cfg settings.ReviewConfig) string { + if strings.TrimSpace(cfg.Agent) != "" { + return strings.TrimSpace(cfg.Agent) + } + return strings.TrimSpace(workerName) +} + +func reviewWorkerLabel(workerName string, cfg settings.ReviewConfig) string { + agentName := reviewAgentName(workerName, cfg) + parts := []string{workerName} + var details []string + if agentName != "" && agentName != workerName { + details = append(details, agentName) + } + if strings.TrimSpace(cfg.Model) != "" { + details = append(details, "model "+strings.TrimSpace(cfg.Model)) + } + if len(details) > 0 { + parts = append(parts, " ("+strings.Join(details, ", ")+")") + } + return strings.Join(parts, "") +} + +func resolveProfileMaster(profile settings.ReviewProfileConfig) (string, string) { + workerName, cfg, err := selectProfileWorker(profile, profile.Master) + if err != nil { + return profile.Master, strings.TrimSpace(profile.MasterModel) + } + model := strings.TrimSpace(profile.MasterModel) + if model == "" { + model = strings.TrimSpace(cfg.Model) + } + return reviewAgentName(workerName, cfg), model +} + +func selectProfileWorker(profile settings.ReviewProfileConfig, selector string) (string, settings.ReviewConfig, error) { + selector = strings.TrimSpace(selector) + if selector == "" { + return "", settings.ReviewConfig{}, errors.New("empty review worker selector") + } + if cfg, ok := profile.Agents[selector]; ok && !cfg.IsZero() { + return selector, cfg, nil + } + var matches []string + for workerName, cfg := range profile.Agents { + if cfg.IsZero() { + continue + } + if reviewAgentName(workerName, cfg) == selector { + matches = append(matches, workerName) + } + } + sort.Strings(matches) + switch len(matches) { + case 1: + return matches[0], profile.Agents[matches[0]], nil + case 0: + configured := sortedProfileAgentNames(profile) + if len(configured) == 0 { + return "", settings.ReviewConfig{}, fmt.Errorf("review worker or agent %q is not configured", selector) + } + return "", settings.ReviewConfig{}, fmt.Errorf("review worker or agent %q is not configured; configured workers: %s", selector, strings.Join(configured, ", ")) + default: + return "", settings.ReviewConfig{}, fmt.Errorf("agent %q has multiple review workers (%s); choose one by worker name", selector, strings.Join(matches, ", ")) + } +} + +func workerIDForAgentModel(agentName, model string, existing map[string]settings.ReviewConfig) string { + base := strings.TrimSpace(agentName) + if strings.TrimSpace(model) != "" { + base += ":" + sanitizeWorkerIDPart(model) + } + if base == "" { + base = "worker" + } + candidate := base + for i := 2; ; i++ { + if _, exists := existing[candidate]; !exists { + return candidate + } + candidate = fmt.Sprintf("%s-%d", base, i) + } +} + +func sanitizeWorkerIDPart(s string) string { + s = strings.ToLower(strings.TrimSpace(s)) + var b strings.Builder + lastDash := false + for _, r := range s { + keep := (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') + if keep { + b.WriteRune(r) + lastDash = false + continue + } + if !lastDash { + b.WriteByte('-') + lastDash = true + } + } + out := strings.Trim(b.String(), "-") + if out == "" { + return "model" + } + return out +} + +func defaultReviewProfileForInstalledAgents( + ctx context.Context, + profileName string, + installed []types.AgentName, + reviewerFor func(string) reviewtypes.AgentReviewer, +) (settings.ReviewProfileConfig, error) { + profileName = strings.TrimSpace(profileName) + if profileName == "" { + profileName = DefaultProfileName + } + installedNames := make([]string, 0, len(installed)) + for _, name := range installed { + installedNames = append(installedNames, string(name)) + } + sort.Strings(installedNames) + + agents := make(map[string]settings.ReviewConfig, len(installedNames)) + for _, name := range installedNames { + if reviewerFor != nil && reviewerFor(name) == nil { + continue + } + cfg := defaultReviewAgentConfig(profileName, name) + if cfg.IsZero() { + continue + } + agents[name] = cfg + } + if len(agents) == 0 { + return settings.ReviewProfileConfig{}, errors.New("no agents with review runner adapters and hooks installed; run `entire configure --agent claude-code`, `entire configure --agent codex`, or `entire configure --agent gemini`") + } + return settings.ReviewProfileConfig{ + Task: profileTask(profileName, settings.ReviewProfileConfig{}), + Agents: agents, + Master: defaultReviewMaster(ctx, agents), + }, nil +} + +func defaultReviewAgentConfig(profileName, agentName string) settings.ReviewConfig { + focus := defaultProfileFocus(profileName) + switch agentName { + case string(agent.AgentNameClaudeCode): + if strings.EqualFold(profileName, "security") { + return settings.ReviewConfig{Skills: []string{"/security-review"}} + } + return settings.ReviewConfig{Skills: []string{"/review"}, Prompt: focus} + case string(agent.AgentNameCodex): + return settings.ReviewConfig{Skills: []string{"/review"}, Prompt: focus} + case string(agent.AgentNameGemini): + prompt := "Review the change according to the profile task." + if focus != "" { + prompt += " " + focus + } + return settings.ReviewConfig{Prompt: prompt} + default: + return settings.ReviewConfig{} + } +} + +func defaultProfileFocus(profileName string) string { + switch strings.ToLower(strings.TrimSpace(profileName)) { + case "security": + return "Focus specifically on security issues." + case "accessibility", "a11y": + return "Focus specifically on accessibility issues." + default: + return "" + } +} + +func defaultReviewMaster(ctx context.Context, configured map[string]settings.ReviewConfig) string { + for _, preferred := range []string{string(agent.AgentNameClaudeCode), string(agent.AgentNameCodex), string(agent.AgentNameGemini)} { + for _, workerName := range sortedReviewConfigKeys(configured) { + cfg := configured[workerName] + if reviewAgentName(workerName, cfg) == preferred && agentSupportsTextGeneration(ctx, preferred) { + return workerName + } + } + } + for _, workerName := range sortedReviewConfigKeys(configured) { + if agentSupportsTextGeneration(ctx, reviewAgentName(workerName, configured[workerName])) { + return workerName + } + } + return "" +} + +func sortedReviewConfigKeys(configured map[string]settings.ReviewConfig) []string { + names := make([]string, 0, len(configured)) + for name := range configured { + names = append(names, name) + } + sort.Strings(names) + return names +} + +func agentSupportsTextGeneration(_ context.Context, name string) bool { + ag, err := agent.Get(types.AgentName(name)) + if err != nil { + return false + } + _, ok := agent.AsTextGenerator(ag) + return ok +} + +func saveDefaultReviewProfile(ctx context.Context, profileName string, profile settings.ReviewProfileConfig) error { + return saveReviewProfile(ctx, profileName, profile, false) +} + +func saveReviewProfile(ctx context.Context, profileName string, profile settings.ReviewProfileConfig, makeDefault bool) error { + prefs, err := settings.LoadClonePreferences(ctx) + if err != nil { + return fmt.Errorf("load review preferences before save: %w", err) + } + if prefs == nil { + prefs = &settings.ClonePreferences{} + } + if prefs.ReviewProfiles == nil { + prefs.ReviewProfiles = map[string]settings.ReviewProfileConfig{} + } + prefs.ReviewProfiles[profileName] = profile + if makeDefault || prefs.ReviewDefaultProfile == "" { + prefs.ReviewDefaultProfile = profileName + } + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save review preferences: %w", err) + } + return nil +} diff --git a/cmd/entire/cli/review/prompt.go b/cmd/entire/cli/review/prompt.go index bca72eecc..b22a588cf 100644 --- a/cmd/entire/cli/review/prompt.go +++ b/cmd/entire/cli/review/prompt.go @@ -15,13 +15,14 @@ import ( reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) -// ComposeReviewPrompt assembles the prompt sent to the agent. It joins -// the configured skill invocations, the always-prompt, the per-run -// prompt, and a scope clause that pins the agent to commits unique to -// the current branch vs cfg.ScopeBaseRef plus any uncommitted changes. +// ComposeReviewPrompt assembles the prompt sent to a worker agent. It joins +// the configured skill invocations, the profile's canonical task, per-agent +// instructions, the per-run prompt, and a scope clause that pins the agent to +// commits unique to the current branch vs cfg.ScopeBaseRef plus any +// uncommitted changes. // -// Empty sections are skipped (no triple-newline gaps). The scope clause -// is only added when cfg.ScopeBaseRef is non-empty. +// Empty sections are skipped (no triple-newline gaps). The scope clause is +// only added when cfg.ScopeBaseRef is non-empty. func ComposeReviewPrompt(cfg reviewtypes.RunConfig) string { if cfg.PromptOverride != "" { return cfg.PromptOverride @@ -29,11 +30,19 @@ func ComposeReviewPrompt(cfg reviewtypes.RunConfig) string { var sections []string - // Skills: one per line, joined as a single section. + // Skills: one per line, joined as a single section. These are agent-specific + // mechanics; the canonical task below keeps multi-agent fan-out coherent. if len(cfg.Skills) > 0 { sections = append(sections, strings.Join(cfg.Skills, "\n")) } + if cfg.ProfileName != "" { + sections = append(sections, "Review profile: "+cfg.ProfileName) + } + if trimmed := strings.TrimRight(cfg.Task, "\n\r "); trimmed != "" { + sections = append(sections, "Task: "+trimmed) + } + // AlwaysPrompt and PerRunPrompt: each is its own section if non-empty after trim. if trimmed := strings.TrimRight(cfg.AlwaysPrompt, "\n\r "); trimmed != "" { sections = append(sections, trimmed) diff --git a/cmd/entire/cli/review/run.go b/cmd/entire/cli/review/run.go index afb800978..4703d634e 100644 --- a/cmd/entire/cli/review/run.go +++ b/cmd/entire/cli/review/run.go @@ -14,6 +14,25 @@ import ( reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) +type reviewerRunMetadata interface { + ActualAgentName() string + ModelName() string +} + +func reviewerActualAgentName(r reviewtypes.AgentReviewer) string { + if meta, ok := r.(reviewerRunMetadata); ok && meta.ActualAgentName() != "" { + return meta.ActualAgentName() + } + return r.Name() +} + +func reviewerModelName(r reviewtypes.AgentReviewer) string { + if meta, ok := r.(reviewerRunMetadata); ok { + return meta.ModelName() + } + return "" +} + // Run executes a single-agent review. Events from the agent are forwarded // to all sinks via AgentEvent as they arrive; on completion, RunFinished // is called on each sink with the populated RunSummary. @@ -29,6 +48,12 @@ func Run( sinks []reviewtypes.Sink, ) (reviewtypes.RunSummary, error) { started := time.Now() + displayName := reviewer.Name() + agentName := reviewerActualAgentName(reviewer) + modelName := reviewerModelName(reviewer) + if modelName == "" { + modelName = cfg.Model + } proc, err := reviewer.Start(ctx, cfg) if err != nil { @@ -41,7 +66,9 @@ func Run( FinishedAt: finished, Cancelled: status == reviewtypes.AgentStatusCancelled, AgentRuns: []reviewtypes.AgentRun{{ - Name: reviewer.Name(), + Name: displayName, + AgentName: agentName, + Model: modelName, Status: status, Err: err, StartedAt: started, @@ -77,7 +104,7 @@ func Run( } } for _, sink := range sinks { - sink.AgentEvent(reviewer.Name(), ev) + sink.AgentEvent(displayName, ev) } } @@ -91,13 +118,13 @@ func Run( firstRunErr = waitErr } for _, sink := range sinks { - sink.AgentEvent(reviewer.Name(), synthEvent) + sink.AgentEvent(displayName, synthEvent) } } status := classifyStatus(ctx, waitErr, eventOutcome{finishedSeen: finishedSeen, finishedOk: finishedOk, sawRunError: sawRunError}) runErr := waitErr if runErr == nil && status == reviewtypes.AgentStatusFailed { - runErr = agentRunFailureError(reviewer.Name(), firstRunErr) + runErr = agentRunFailureError(displayName, firstRunErr) } summary := reviewtypes.RunSummary{ @@ -105,7 +132,9 @@ func Run( FinishedAt: finished, Cancelled: status == reviewtypes.AgentStatusCancelled, AgentRuns: []reviewtypes.AgentRun{{ - Name: reviewer.Name(), + Name: displayName, + AgentName: agentName, + Model: modelName, Status: status, Tokens: tokens, Buffer: buffer, diff --git a/cmd/entire/cli/review/run_multi.go b/cmd/entire/cli/review/run_multi.go index 5d37486a6..9d6479a8d 100644 --- a/cmd/entire/cli/review/run_multi.go +++ b/cmd/entire/cli/review/run_multi.go @@ -55,6 +55,8 @@ import ( // a third write path would require a mutex (or a redesign). type perAgentState struct { name string + agentName string + model string proc reviewtypes.Process startErr error startedAt time.Time @@ -110,6 +112,8 @@ func RunMulti( for i, r := range reviewers { states[i] = &perAgentState{ name: r.Name(), + agentName: reviewerActualAgentName(r), + model: reviewerModelName(r), startedAt: time.Now(), } } @@ -143,6 +147,8 @@ func RunMulti( } emitEnrichedAgentTokens(ctx, cfg, fanIn, idx, reviewtypes.AgentRun{ Name: states[idx].name, + AgentName: states[idx].agentName, + Model: states[idx].model, StartedAt: states[idx].startedAt, Duration: finishedAt.Sub(states[idx].startedAt), Err: waitErr, @@ -204,6 +210,8 @@ func RunMulti( } agentRuns[i] = reviewtypes.AgentRun{ Name: st.name, + AgentName: st.agentName, + Model: st.model, Status: status, Tokens: st.tokens, Buffer: st.buffer, diff --git a/cmd/entire/cli/review/synthesis_prompt.go b/cmd/entire/cli/review/synthesis_prompt.go index 5938614a5..76c1e9d5e 100644 --- a/cmd/entire/cli/review/synthesis_prompt.go +++ b/cmd/entire/cli/review/synthesis_prompt.go @@ -41,7 +41,7 @@ import ( // scoped to agents that produced narrative output. SynthesisSink already // guards on len(usable) >= 2 before calling, so the empty case won't reach // the LLM in production. -func composeSynthesisPrompt(summary reviewtypes.RunSummary, perRunPrompt string) string { +func composeSynthesisPrompt(summary reviewtypes.RunSummary, perRunPrompt string, profileName string, task string) string { usable := usableAgentRuns(summary) if len(usable) == 0 { return "" @@ -49,7 +49,14 @@ func composeSynthesisPrompt(summary reviewtypes.RunSummary, perRunPrompt string) var b strings.Builder - fmt.Fprintf(&b, "You reviewed the same code change with %d agents. Here are their reports:\n", len(usable)) + fmt.Fprintf(&b, "You reviewed the same code change with %d agents. You are the final reviewer; critically adjudicate their reports instead of blindly summarizing.\n", len(usable)) + if profileName != "" { + fmt.Fprintf(&b, "Review profile: %s\n", profileName) + } + if strings.TrimSpace(task) != "" { + fmt.Fprintf(&b, "Canonical task: %s\n", strings.TrimSpace(task)) + } + b.WriteString("\nHere are their reports:\n") for _, run := range usable { narrative := joinAssistantText(run.Buffer) @@ -62,16 +69,27 @@ func composeSynthesisPrompt(summary reviewtypes.RunSummary, perRunPrompt string) } b.WriteString(` -Synthesize a unified verdict with these sections: - - Common findings (issues all agents flagged) - - Unique findings (issues only one agent caught) - - Disagreements (areas where agents reached different conclusions) - - Priority order (top 5 issues to address first) +Critically evaluate the worker reports. Do not blindly summarize. + +Rules: + - Prefer findings backed by concrete evidence (file, function, behavior, test, or diff detail). + - Discard unsupported or speculative claims unless they are clearly labeled as needing verification. + - Identify contradictions between workers and decide which claim is better supported. + - Merge duplicate findings. + - Call out important uncertainty instead of pretending certainty. + +Produce one canonical final report with these sections: + - Executive verdict + - Common findings / high-confidence findings, prioritized + - Unique findings worth keeping + - Needs verification / uncertain findings + - Disagreements or rejected false positives + - Priority order / recommended next actions -Be concise; aim for ~300 words.`) +Be concise but specific; include evidence pointers where available.`) if perRunPrompt != "" { - b.WriteString("\n\n") + b.WriteString("\n\nPer-run user instructions:\n") b.WriteString(perRunPrompt) } diff --git a/cmd/entire/cli/review/synthesis_sink.go b/cmd/entire/cli/review/synthesis_sink.go index 805ad63b6..400dff982 100644 --- a/cmd/entire/cli/review/synthesis_sink.go +++ b/cmd/entire/cli/review/synthesis_sink.go @@ -17,6 +17,8 @@ import ( "log/slog" "time" + "github.com/entireio/cli/cmd/entire/cli/agent" + agenttypes "github.com/entireio/cli/cmd/entire/cli/agent/types" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/mdrender" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -34,15 +36,44 @@ type SynthesisProvider interface { Synthesize(ctx context.Context, prompt string) (string, error) } +// AgentSynthesisProvider asks a named agent's text-generation API to produce +// the final report. This is the profile-native master implementation used by +// `entire review`: workers run as review sessions, while the master is an +// isolated text-generation call so it consolidates reports without creating a +// second review worker session. +type AgentSynthesisProvider struct { + AgentName string + Model string +} + +func (p AgentSynthesisProvider) Synthesize(ctx context.Context, prompt string) (string, error) { + ag, err := agent.Get(agenttypes.AgentName(p.AgentName)) + if err != nil { + return "", fmt.Errorf("resolve master agent %s: %w", p.AgentName, err) + } + tg, ok := agent.AsTextGenerator(ag) + if !ok { + return "", fmt.Errorf("master agent %s does not support text generation", p.AgentName) + } + return tg.GenerateText(ctx, prompt, p.Model) //nolint:wrapcheck // caller owns display +} + // SynthesisSink composes a multi-agent verdict by calling a configured -// summary provider after the run finishes. AgentEvent is a no-op; all -// work happens in RunFinished. +// provider after the run finishes. In normal profile-native `entire review` +// runs this is the profile's master adjudication phase: Auto is true and +// Provider is the profile master, so the master report is produced without a +// y/N prompt. The legacy opt-in path (Auto false) keeps the prompted-synthesis +// behavior. AgentEvent is a no-op; all work happens in RunFinished. type SynthesisSink struct { Provider SynthesisProvider Writer io.Writer InputTTY bool // true if stdin can prompt the user PromptYN func(ctx context.Context, question string, def bool) (bool, error) - PerRunPrompt string // if non-empty, included in the synthesis prompt for context + PerRunPrompt string // if non-empty, included in the synthesis prompt for context + ProfileName string + Task string + MasterName string + Auto bool // when true, run without a y/N prompt (profile-native final report) RunContext context.Context // optional; nil falls back to context.Background() ProviderTimeout time.Duration // optional; zero uses defaultSynthesisProviderTimeout OnResult func(result string) @@ -64,13 +95,11 @@ func (SynthesisSink) AgentEvent(_ string, _ reviewtypes.Event) {} // - fewer than 2 agents produced usable output (status Succeeded or Failed // with non-empty narrative buffer) // -// Otherwise prompt y/N (default N). On y: compose prompt, call provider, -// print response. On provider failure: print "synthesis unavailable: " -// with the underlying error; user can still commit. +// In profile-native mode (Auto=true), the master phase is mandatory and runs +// without a y/N prompt. In legacy sink mode (Auto=false), prompt y/N (default +// N). On provider failure: print "final report unavailable: " with the +// underlying error; user can still commit. func (s SynthesisSink) RunFinished(summary reviewtypes.RunSummary) { - if !s.InputTTY { - return - } if summary.Cancelled { return } @@ -79,31 +108,48 @@ func (s SynthesisSink) RunFinished(summary reviewtypes.RunSummary) { } ctx := s.runContext() - promptFn := s.PromptYN - if promptFn == nil { - promptFn = realPromptYN - } + if !s.Auto { + if !s.InputTTY { + return + } + promptFn := s.PromptYN + if promptFn == nil { + promptFn = realPromptYN + } - yes, err := promptFn(ctx, "Synthesize a unified verdict across all agent reviews?", false) - if err != nil { - // huh form errors (terminal-resize anomalies, stdin EOF, stub - // failures) shouldn't block the user from committing — they get the - // same silent skip as a "no" answer. Logged at debug for diagnostics. - logging.Debug(ctx, "synthesis prompt error", - slog.String("error", err.Error())) - return - } - if !yes { - return + yes, err := promptFn(ctx, "Synthesize a unified verdict across all agent reviews?", false) + if err != nil { + // huh form errors (terminal-resize anomalies, stdin EOF, stub + // failures) shouldn't block the user from committing — they get the + // same silent skip as a "no" answer. Logged at debug for diagnostics. + logging.Debug(ctx, "synthesis prompt error", + slog.String("error", err.Error())) + return + } + if !yes { + return + } } - synthesisPrompt := composeSynthesisPrompt(summary, s.PerRunPrompt) + synthesisPrompt := composeSynthesisPrompt(summary, s.PerRunPrompt, s.ProfileName, s.Task) providerCtx, cancelProvider := s.providerContext() defer cancelProvider() - fmt.Fprintln(s.Writer, "Generating summary...") + if s.Auto { + if s.MasterName != "" { + fmt.Fprintf(s.Writer, "Generating final report with %s...\n", s.MasterName) + } else { + fmt.Fprintln(s.Writer, "Generating final report...") + } + } else { + fmt.Fprintln(s.Writer, "Generating summary...") + } result, provErr := s.Provider.Synthesize(providerCtx, synthesisPrompt) if provErr != nil { - fmt.Fprintf(s.Writer, "synthesis unavailable: %v\n", provErr) + if s.Auto { + fmt.Fprintf(s.Writer, "final report unavailable: %v\n", provErr) + } else { + fmt.Fprintf(s.Writer, "synthesis unavailable: %v\n", provErr) + } return } if s.OnResult != nil { diff --git a/cmd/entire/cli/review/types/reviewer.go b/cmd/entire/cli/review/types/reviewer.go index 97f9a631b..ecd5baa23 100644 --- a/cmd/entire/cli/review/types/reviewer.go +++ b/cmd/entire/cli/review/types/reviewer.go @@ -24,7 +24,7 @@ import "context" type AgentReviewer interface { // Name returns the agent's registry key (e.g., "claude-code", "codex", // "gemini"). Stable identifier; do not change after release without - // updating settings migration. + // updating profile settings. Name() string // Start spawns the agent with the given run configuration. The returned @@ -75,12 +75,26 @@ type RunConfig struct { // but they are not prepended to the prompt text. PromptOverride string + // ProfileName is the named review profile being run (e.g. "general", + // "security", "accessibility"). It is included in the prompt and final + // adjudication context for traceability. + ProfileName string + + // Task is the canonical review task for this profile. Every worker agent in + // a fan-out run receives the same task; per-agent Skills/AlwaysPrompt adapt + // execution mechanics without changing the task identity. + Task string + + // Model is an optional model hint passed to the agent CLI. Empty means use + // the agent's default model. + Model string + // Skills are skill invocation strings passed to the agent verbatim. Skills []string - // AlwaysPrompt is the per-agent always-prompt configured in settings. - // Concatenated with Skills + PerRunPrompt + a scope clause to form the - // composed agent prompt. + // AlwaysPrompt is the per-agent additional instruction configured in the + // selected review profile. Concatenated with Task + Skills + PerRunPrompt + + // a scope clause to form the composed agent prompt. AlwaysPrompt string // PerRunPrompt is optional textarea input from a single invocation. diff --git a/cmd/entire/cli/review/types/sink.go b/cmd/entire/cli/review/types/sink.go index 0f013fa43..16db359b2 100644 --- a/cmd/entire/cli/review/types/sink.go +++ b/cmd/entire/cli/review/types/sink.go @@ -48,9 +48,20 @@ type RunSummary struct { AgentRuns []AgentRun } -// AgentRun is per-agent post-run data. +// AgentRun is per-worker post-run data. type AgentRun struct { - Name string + // Name is the display/worker name shown in review output. It may be an alias + // such as "claude-code:sonnet" when the same underlying agent runs more than + // once with different models. + Name string + + // AgentName is the underlying agent registry key used for lifecycle/session + // matching. Empty means Name is also the agent name. + AgentName string + + // Model is the optional model hint used for this worker. + Model string + Status AgentStatus Tokens Tokens diff --git a/cmd/entire/cli/review_bridge.go b/cmd/entire/cli/review_bridge.go index 6d33edebe..7e0cb9094 100644 --- a/cmd/entire/cli/review_bridge.go +++ b/cmd/entire/cli/review_bridge.go @@ -8,18 +8,12 @@ package cli // review → claudecode/codex/geminicli → review import ( - "bytes" - "context" - "fmt" - "strings" - "github.com/spf13/cobra" "github.com/entireio/cli/cmd/entire/cli/agent" "github.com/entireio/cli/cmd/entire/cli/agent/claudecode" "github.com/entireio/cli/cmd/entire/cli/agent/codex" "github.com/entireio/cli/cmd/entire/cli/agent/geminicli" - "github.com/entireio/cli/cmd/entire/cli/logging" cliReview "github.com/entireio/cli/cmd/entire/cli/review" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) @@ -27,12 +21,6 @@ import ( // buildReviewDeps builds the review.Deps struct used by review.NewCommand. // attachCmd is the cobra.Command for `entire review attach`; pass nil in // tests that don't need the subcommand. -// -// SynthesisProvider is a lazySynthesisProvider that defers resolution of the -// configured summary provider to the first Synthesize call. This avoids -// running resolveCheckpointSummaryProvider during CLI startup (and during -// every `entire review --help` invocation in tests). Note: side effects are -// DEFERRED, not eliminated — see lazySynthesisProvider doc below. func buildReviewDeps(attachCmd *cobra.Command) cliReview.Deps { return cliReview.Deps{ GetAgentsWithHooksInstalled: GetAgentsWithHooksInstalled, @@ -42,80 +30,13 @@ func buildReviewDeps(attachCmd *cobra.Command) cliReview.Deps { HeadHasReviewCheckpoint: headHasReviewCheckpoint, ReviewCheckpointContext: reviewCheckpointContext, ReviewerFor: launchableReviewerFor, - PromptForAgentFn: nil, // use real PromptForAgent AttachCmd: attachCmd, - SynthesisProvider: lazySynthesisProvider{}, - } -} - -// lazySynthesisProvider wraps the summary-provider resolution so it runs -// only when Synthesize is first called (not at CLI startup). -// -// IMPORTANT: side effects are DEFERRED, not eliminated. resolveCheckpoint- -// SummaryProvider auto-selects a default provider AND persists the choice -// to .entire/settings.local.json (via persistSummaryProviderSelection) on -// the FIRST call against an unconfigured repo. The disk write still -// happens — it's just triggered by the user picking "y" on the synthesis -// prompt, not by every `entire review --help`. -// -// If a future caller needs read-only resolution (e.g. CI mode, where -// touching settings would dirty the working tree), introduce a flag on -// resolveCheckpointSummaryProvider for skip-persistence. -type lazySynthesisProvider struct{} - -// Synthesize resolves the configured summary provider on demand and delegates -// the generation call to the underlying TextGenerator. Errors from resolution -// are returned to SynthesisSink, which prints "synthesis unavailable: " -// and lets the user continue without blocking the commit. -// -// resolveCheckpointSummaryProvider's user-facing chatter (auto-select notice, -// "Using " line, external_agents flag-flip note, persistence-failure -// warning) is captured and routed through logging instead of printing inline -// with the synthesis output. The persistence-failure path is also surfaced as -// logging.Warn at the source (explain_summary_provider.go), so real failures -// are not silenced — they live in .entire/logs/. -// -// Note: first call against an unconfigured repo will write -// .entire/settings.local.json — see the lazySynthesisProvider doc above. -func (lazySynthesisProvider) Synthesize(ctx context.Context, prompt string) (string, error) { - var captured bytes.Buffer - provider, err := resolveCheckpointSummaryProvider(ctx, &captured) - logProviderResolutionOutput(ctx, &captured) - if err != nil { - return "", err - } - ag, agErr := getSummaryAgent(provider.Name) - if agErr != nil { - return "", agErr - } - tg, ok := agent.AsTextGenerator(ag) - if !ok { - return "", fmt.Errorf("agent %s does not support text generation", provider.Name) - } - return tg.GenerateText(ctx, prompt, provider.Model) //nolint:wrapcheck // SynthesisSink owns display -} - -// logProviderResolutionOutput routes captured output from resolveCheckpoint- -// SummaryProvider through logging so it ends up in .entire/logs/ rather than -// inline with the synthesis verdict. Lines starting with "Warning:" go to -// Warn; other notices (auto-select reason, "Using X for summary generation", -// external_agents flag-flip note) go to Info. -func logProviderResolutionOutput(ctx context.Context, buf *bytes.Buffer) { - for _, line := range strings.Split(strings.TrimRight(buf.String(), "\n"), "\n") { - if line == "" { - continue - } - if strings.HasPrefix(line, "Warning:") { - logging.Warn(ctx, "synthesis provider resolution", "message", line) - continue - } - logging.Info(ctx, "synthesis provider resolution", "message", line) } } -// launchableReviewerFor returns the AgentReviewer for known launchable agents, -// or nil for non-launchable agents (cursor, opencode, factoryai-droid, -// copilot-cli). This lives in the cli package to avoid the import cycle: +// launchableReviewerFor returns the AgentReviewer for agents with a review-runner +// adapter, or nil for agents that are known to Entire but not yet wired into +// `entire review` fan-out. This lives in the cli package to avoid the import cycle: // // review/cmd.go → claudecode/codex/geminicli → review func launchableReviewerFor(agentName string) reviewtypes.AgentReviewer { diff --git a/cmd/entire/cli/review_context_test.go b/cmd/entire/cli/review_context_test.go index ad85ffb4f..057d9ddf4 100644 --- a/cmd/entire/cli/review_context_test.go +++ b/cmd/entire/cli/review_context_test.go @@ -434,7 +434,7 @@ func writeReviewContextSettings(t *testing.T, repoRoot string) { if err := os.MkdirAll(entireDir, 0o750); err != nil { t.Fatalf("create .entire dir: %v", err) } - settingsJSON := `{"enabled":true,"review":{"claude-code":{"skills":["/review"]}}}` + "\n" + settingsJSON := `{"enabled":true,"review_default_profile":"general","review_profiles":{"general":{"task":"Test review task.","agents":{"claude-code":{"skills":["/review"]}},"master":"claude-code"}}}` + "\n" if err := os.WriteFile(filepath.Join(entireDir, "settings.json"), []byte(settingsJSON), 0o600); err != nil { t.Fatalf("write review settings: %v", err) } diff --git a/cmd/entire/cli/root.go b/cmd/entire/cli/root.go index d7b5d491a..ab75fa3a3 100644 --- a/cmd/entire/cli/root.go +++ b/cmd/entire/cli/root.go @@ -91,7 +91,7 @@ func NewRootCmd() *cobra.Command { cmd.AddCommand(newPluginGroupCmd()) // 'plugin' (managed install/list/remove) // Top-level lifecycle and standalone commands. - cmd.AddCommand(cliReview.NewCommand(buildReviewDeps(newReviewAttachCmd()))) // hidden during maturation; runs configured review skills + cmd.AddCommand(cliReview.NewCommand(buildReviewDeps(newReviewAttachCmd()))) // hidden during maturation; runs review profiles cmd.AddCommand(investigate.NewCommand(buildInvestigateDeps())) // hidden during maturation; runs a multi-agent investigation cmd.AddCommand(newOrgCmd()) // hidden during maturation; control-plane org management cmd.AddCommand(newProjectCmd()) // hidden during maturation; control-plane project management diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index fe2bef0a9..5d19637f0 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -84,12 +84,25 @@ type EntireSettings struct { // Redaction configures PII redaction behavior for transcripts and metadata. Redaction *RedactionSettings `json:"redaction,omitempty"` - // Review maps agent name (e.g. "claude-code") to the review config for - // that agent. When empty, `entire review` triggers the first-run picker. + // ReviewProfiles maps profile names (e.g. "general", "security") to + // named review setups. `entire review` runs one profile: its canonical task + // is fanned out to the configured agents, then an optional master agent + // consolidates the worker reports. + ReviewProfiles map[string]ReviewProfileConfig `json:"review_profiles,omitempty"` + + // ReviewDefaultProfile is the profile used by `entire review` when no + // profile is supplied. If empty, `general` is used when present, otherwise + // the single configured profile is used. + ReviewDefaultProfile string `json:"review_default_profile,omitempty"` + + // Deprecated: legacy pre-profile review settings. Kept so old config files + // still parse, but `entire review` no longer reads this field. Review map[string]ReviewConfig `json:"review,omitempty"` - // ReviewFixAgent is the default agent used when applying aggregate or - // multi-agent review findings with `entire review --fix`. + // ReviewFixAgent is a legacy saved fix-agent preference. The `entire review + // --fix` flow has been removed; this field is retained only so older + // settings/preferences files still parse. It is no longer read by + // `entire review`. ReviewFixAgent string `json:"review_fix_agent,omitempty"` // Investigate holds configuration for `entire investigate`. Empty means @@ -137,14 +150,13 @@ type EntireSettings struct { // same clone see the same preferences. Not committed because the file lives // inside .git/. type ClonePreferences struct { + ReviewProfiles map[string]ReviewProfileConfig `json:"review_profiles,omitempty"` + ReviewDefaultProfile string `json:"review_default_profile,omitempty"` + + // Deprecated: legacy pre-profile review settings. Kept so old preference + // files parse, but new review setup writes ReviewProfiles instead. Review map[string]ReviewConfig `json:"review,omitempty"` ReviewFixAgent string `json:"review_fix_agent,omitempty"` - - // ReviewMigrationDismissed records that the user declined the one-shot - // migration of review keys from project settings to clone-local prefs. - // Once true, `entire review` stops prompting on every invocation; the - // user can re-enable by editing this file or deleting the key. - ReviewMigrationDismissed bool `json:"review_migration_dismissed,omitempty"` } // SummaryGenerationSettings configures provider selection for on-demand @@ -236,32 +248,75 @@ func (s *EntireSettings) SummaryTimeoutValue() time.Duration { return time.Duration(s.SummaryTimeoutSeconds) * time.Second } -// ReviewConfig holds the per-agent review configuration. Both fields are -// optional; together they describe what `entire review` should ask the -// agent to do. +// ReviewProfileConfig is a named review setup. The profile-level Task is the +// canonical task every worker agent is asked to run; per-agent ReviewConfig +// entries adapt that task to agent-specific mechanics such as slash commands +// or additional instructions. Master names the agent that consolidates worker +// outputs into the final report. +// +// Example: +// +// "review_profiles": { +// "security": { +// "task": "Review this change for auth, injection, secrets, and privilege-boundary bugs.", +// "agents": { +// "claude-sonnet": {"agent": "claude-code", "model": "sonnet", "skills": ["/security-review"]}, +// "claude-opus": {"agent": "claude-code", "model": "opus", "skills": ["/security-review"]}, +// "codex": {"model": "gpt-5-codex", "skills": ["/review"], "prompt": "Focus on security."} +// }, +// "master": "claude-sonnet" +// } +// } // -// Precedence when composing the review prompt sent to the agent: -// - If Prompt is non-empty, it is used verbatim. -// - Otherwise, Skills are composed into a default template -// ("Please run these review skills in order: 1. /X 2. /Y"). +// MasterModel is an optional model hint passed to the master agent's text +// generation API. +// ReviewProfileConfig is intentionally small: the review package owns built-in +// default task text for conventional profile names like "general". +type ReviewProfileConfig struct { + Task string `json:"task,omitempty"` + Agents map[string]ReviewConfig `json:"agents,omitempty"` + Master string `json:"master,omitempty"` + MasterModel string `json:"master_model,omitempty"` +} + +// IsZero reports whether the profile is effectively unset. +func (c ReviewProfileConfig) IsZero() bool { + return c.Task == "" && len(c.Agents) == 0 && c.Master == "" && c.MasterModel == "" +} + +// ReviewConfig holds one worker's configuration within a review profile. +// The profile's agents map is keyed by worker id. For simple configs the worker +// id is also the agent registry name (for example "claude-code"). To run the +// same agent more than once with different models, use stable worker ids and set +// Agent to the underlying registry name. // -// Skills are always recorded on the checkpoint metadata regardless of -// which path composed the prompt — they're the structured, queryable -// tag alongside ReviewPrompt (which is the ground truth). +// Skills are agent-specific invocations passed before the task. Prompt is +// additional agent-specific instruction appended after the profile task; it is +// no longer a verbatim replacement for the whole review prompt. type ReviewConfig struct { + // Agent is the underlying agent registry key for this worker. Empty means + // the profile map key is the agent name. Set this when the map key is an + // alias such as "claude-sonnet" or "claude-opus". + Agent string `json:"agent,omitempty"` + + // Model is an optional model hint passed to the agent CLI for this worker. + // Empty means use the agent's own default. + Model string `json:"model,omitempty"` + // Skills is the list of slash-prefixed skill invocations configured - // for this agent. May be empty when Prompt carries the full request. + // for this agent. May be empty for prompt/model-driven workers (e.g. Pi), + // in which case the profile task plus Prompt drive the review. Skills []string `json:"skills,omitempty"` - // Prompt, when non-empty, carries saved review instructions. When - // Skills is non-empty it is appended after the selected skills; when - // Skills is empty it is the full prompt for prompt-only review configs. + // Prompt, when non-empty, carries saved agent-specific instructions. It is + // appended after the profile task (and after any Skills); it is not a + // verbatim replacement for the whole review prompt. Prompt string `json:"prompt,omitempty"` } // IsZero reports whether the config is effectively unset. func (c ReviewConfig) IsZero() bool { - return len(c.Skills) == 0 && c.Prompt == "" + return c.Agent == "" && c.Model == "" && len(c.Skills) == 0 && c.Prompt == "" } // ReviewConfigFor returns the configured review config for the given agent. @@ -432,10 +487,10 @@ func LoadFromFile(filePath string) (*EntireSettings, error) { // - exists: false when the file does not exist (raw is empty); true otherwise. // - err: parse error or read error other than ENOENT. // -// Pair with SaveProjectRaw for read-modify-write flows like the review-key -// migration. Owning the path resolution and raw IO here keeps callers from -// duplicating settings parsing in violation of the "Settings access must go -// through the settings package" rule in CLAUDE.md. +// Pair with SaveProjectRaw for read-modify-write flows that need to preserve +// unrelated keys. Owning the path resolution and raw IO here keeps callers +// from duplicating settings parsing in violation of the "Settings access must +// go through the settings package" rule in CLAUDE.md. func LoadProjectRaw(ctx context.Context) (path string, raw map[string]json.RawMessage, exists bool, err error) { path, err = paths.AbsPath(ctx, EntireSettingsFile) if err != nil { @@ -460,9 +515,8 @@ func LoadProjectRaw(ctx context.Context) (path string, raw map[string]json.RawMe // exists=false (and an empty raw map) when the file does not exist — the // common case for users who haven't created the local override file. // -// Pair with the migration flow: callers can use this to detect when local -// overrides would mask a freshly-migrated setting, then warn the user -// before performing the migration. +// Pair with SaveProjectRaw for read-modify-write flows that need to preserve +// unrelated keys in the per-developer override file. func LoadLocalRaw(ctx context.Context) (path string, raw map[string]json.RawMessage, exists bool, err error) { path, err = paths.AbsPath(ctx, EntireSettingsLocalFile) if err != nil { @@ -620,6 +674,12 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { if prefs == nil { return } + if prefs.ReviewProfiles != nil { + settings.ReviewProfiles = prefs.ReviewProfiles + } + if prefs.ReviewDefaultProfile != "" { + settings.ReviewDefaultProfile = prefs.ReviewDefaultProfile + } if prefs.Review != nil { settings.Review = prefs.Review } @@ -659,6 +719,13 @@ func mergeJSON(settings *EntireSettings, data []byte) error { if err := mergeCommitLinking(settings, raw); err != nil { return err } + if profilesRaw, ok := raw["review_profiles"]; ok { + var profiles map[string]ReviewProfileConfig + if err := json.Unmarshal(profilesRaw, &profiles); err != nil { + return fmt.Errorf("parsing review_profiles field: %w", err) + } + settings.ReviewProfiles = profiles + } if reviewRaw, ok := raw["review"]; ok { var review map[string]ReviewConfig if err := json.Unmarshal(reviewRaw, &review); err != nil { @@ -726,6 +793,9 @@ func mergeScalarFields(settings *EntireSettings, raw map[string]json.RawMessage) if err := mergeRawStringNonEmpty(raw, "log_level", &settings.LogLevel); err != nil { return err } + if err := mergeRawStringNonEmpty(raw, "review_default_profile", &settings.ReviewDefaultProfile); err != nil { + return err + } if err := mergeRawStringNonEmpty(raw, "review_fix_agent", &settings.ReviewFixAgent); err != nil { return err } diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 3fd5988ed..e85d4b092 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -1,45 +1,81 @@ # `entire review` Command -`entire review` runs a set of configured review skills inside an agent session. The review session is an immutable fact attached to a checkpoint — no verdict, no status tracking, no empty commits. On the next `git commit`, the review session is condensed into the checkpoint metadata alongside normal sessions, permanently recording that the code was reviewed and which skills were run. +`entire review` runs a named review profile. A profile defines one canonical task (for example `general`, `security`, or `accessibility`), a set of worker agents that all run that task, and an optional master agent that critically adjudicates worker reports into one final report. Worker review sessions are immutable facts attached to checkpoints; the master report is stored locally in the review manifest for findings/fix workflows. ## Command Surface ``` -entire review # Normal run: load config, run configured agent(s) -entire review --edit # Re-open the skills picker before running -entire review --agent # Force a specific configured agent (skips multi-picker) +entire review # Run the default review profile +entire review security # Run a named profile +entire review --profile accessibility # Same, flag form +entire review --configure # Interactive: guided wizard. Non-interactive: list agents + profiles +entire review --configure --profile general --set-agents claude-code,codex --set-master claude-code + # Configure a profile non-interactively (no TUI) +entire review --configure --profile general --set-model codex=gpt-5-codex --set-task "..." +entire review --edit --profile general # Advanced skill-level config (skill picker) +entire review --agent # Run one worker from the selected profile +entire review --agent --model # Override that worker's model for this run +entire review --agents # List the profile's workers (valid --agent values) +entire review --models # List models each review agent advertises +entire review --models --agent codex # ...filtered to one agent +entire review --prompt "focus on auth" # Add one-off instructions +entire review --findings # Browse local review findings entire review attach # Tag an existing agent session as a review (post-hoc) entire review attach --force # Skip confirmation entire review attach --agent # Agent that created the session entire review attach --skills # Declare which skills were run ``` -When two or more launchable agents are configured and `--agent` is not set, a multi-select picker appears with an optional per-run prompt field (e.g. "focus on security"). Selecting one agent or passing `--agent` runs the single-agent path; selecting two or more runs the N-agent path. +When no profiles are configured, `entire review` uses a simple guided setup: choose review type, choose worker agents, optionally choose models/model variants (a select of each agent's advertised models via `agent.ModelLister`, with a `Custom…` option to type any value), save the profile, then explicitly confirm whether to start agents. `entire review --configure` is the configuration entry point: +- With `--set-agents` / `--set-master` / `--set-task` / `--set-model agent=model`, it writes the profile non-interactively (no TUI). `--set-*` writes preserve profile-level fields the flags don't touch (custom `task`, `master_model`). +- With no `--set-*` flags in an interactive terminal, it opens the guided wizard (which already lists the selectable agents). +- With no `--set-*` flags in a non-interactive context, it prints the discovery view: the **available review agents** (those with review-runner adapters, marking which have hooks installed) and the **currently configured profiles**, plus an example `--set-*` command. In non-interactive output, first run falls back to the default `general` profile automatically. Defaults are intentionally simple: Claude/Codex use `/review`, Gemini uses the profile task directly, and Claude is preferred as master when available. + +When two or more adapter-backed review workers are configured in the selected profile and `--agent` is not set, `entire review` fans out to all configured workers. There is no per-run multi-picker: the profile is the fan-out contract. Profiles with multiple workers must set `master`; the master runs after workers finish and produces the canonical final report. ## Settings Schema -Review skills are configured per-agent in `.entire/settings.json`: +Review profiles are configured in clone-local preferences (or settings) under `review_profiles`: ```json { - "review": { - "claude-code": {"skills": ["/pr-review-toolkit:review-pr"], "prompt": "Be thorough."}, - "codex": {"skills": ["/codex:adversarial-review"]} + "review_default_profile": "general", + "review_profiles": { + "general": { + "task": "Review this change for correctness, regressions, tests, and maintainability.", + "agents": { + "claude-code": {"skills": ["/review"]}, + "codex": {"skills": ["/review"]} + }, + "master": "claude-code" + }, + "security": { + "task": "Review this change for auth, injection, secrets, and privilege-boundary bugs.", + "agents": { + "claude-sonnet": {"agent": "claude-code", "model": "sonnet", "skills": ["/security-review"]}, + "claude-opus": {"agent": "claude-code", "model": "opus", "skills": ["/security-review"]}, + "codex": {"model": "gpt-5-codex", "skills": ["/review"], "prompt": "Focus on security."} + }, + "master": "claude-sonnet" + } } } ``` -The key is the agent name. The value is a `ReviewConfig` with `skills` (skill invocations passed verbatim to the agent) and optional `prompt` (an always-prompt appended to the composed prompt). Settings field: `EntireSettings.Review` in `cmd/entire/cli/settings/settings.go`. +`entire review --models` lists the models each review-runner agent advertises via the optional `agent.ModelLister` capability (`cmd/entire/cli/agent/model_lister.go`). Agents whose CLI has no enumeration command (claude-code, codex, gemini) return a curated, non-exhaustive list of common models/aliases; the `--model` flag still forwards any value the agent CLI accepts. Agents whose CLI can enumerate live (e.g. Pi's `pi --list-models`) may implement `ListModels` by shelling out. + +The profile-level `task` is the shared work item. Each `agents` map entry is a worker id. For simple entries the worker id is also the agent name; to run the same agent more than once, use aliases and set `agent` plus `model`. Per-worker `skills`, `prompt`, and `model` adapt that task to agent-specific mechanics. Settings fields: `EntireSettings.ReviewProfiles` and `EntireSettings.ReviewDefaultProfile` in `cmd/entire/cli/settings/settings.go`. The old top-level `review` map is no longer used by `entire review`. ## How It Works (env-var handshake) -1. `entire review` selects the configured agent (override → alphabetically first → prompt if multiple), composes the review prompt via `review.ComposeReviewPrompt`, and computes scope (mainline base ref via `review.ComputeScopeStats`, overridable with `--base`). -2. **For launchable agents** (claude-code, codex, gemini-cli): the spawned agent process is given env vars `ENTIRE_REVIEW_{SESSION,AGENT,SKILLS,PROMPT,STARTING_SHA}` that the agent's `UserPromptSubmit` lifecycle hook reads to tag the session as `Kind = "agent_review"` with the configured skills/prompt. Each spawned process has its own env, so multiple worktrees and multi-agent runs are correct by construction (no shared marker file, no race). -3. **For non-launchable agents** (cursor, opencode, factoryai-droid): `RunMarkerFallback` writes a `PendingReviewMarker` file and prints guidance — the user opens the agent themselves and runs the skills. Single shared file (`review/marker_fallback.go`); adding new non-launchable agents is a registry entry, not a new file. -4. The agent runs the review skills; the session ends naturally. -5. On the next `git commit`, the PostCommit hook condenses the review session into the checkpoint on `entire/checkpoints/v1`, with `Kind` and `ReviewSkills` recorded in `CommittedMetadata`. -6. The `CheckpointSummary` sets `HasReview = true` for O(1) lookup. `HasReview` is an umbrella "any review happened" flag — future review kinds (e.g. manual review) should also set it. -7. `entire status` and the re-run guard read `HasReview` from the checkpoint metadata (no commit history walking). +1. `entire review` selects a profile (positional/`--profile` → `review_default_profile` → `general` → only configured profile). If no profiles exist, it runs simple guided setup in an interactive terminal and asks before starting agents, or writes an opinionated clone-local default profile in non-interactive mode. It then composes worker prompts via `review.ComposeReviewPrompt` and computes scope (mainline base ref via `review.ComputeScopeStats`, overridable with `--base`). +2. **For agents with review-runner adapters** (claude-code, codex, gemini-cli): the spawned agent process is given env vars `ENTIRE_REVIEW_{SESSION,AGENT,SKILLS,PROMPT,STARTING_SHA}` that the agent's `UserPromptSubmit` lifecycle hook reads to tag the session as `Kind = "agent_review"` with the configured skills/prompt. Each spawned process has its own env, so multiple worktrees and multi-agent runs are correct by construction (no shared marker file, no race). +3. **For agents without review-runner adapters yet**: `RunMarkerFallback` writes a `PendingReviewMarker` file and prints guidance — the user opens the agent themselves and runs the skills. This is an adapter backlog path, not a statement that the agent cannot be launched headlessly. +4. Worker agents run the selected profile's task; each session ends naturally. +5. In multi-worker profiles, the configured master agent receives all worker reports and produces one critical final report. The master prompt asks it to reject unsupported claims, resolve contradictions, merge duplicates, and prioritize evidence-backed findings. +6. On the next `git commit`, the PostCommit hook condenses worker review sessions into the checkpoint on `entire/checkpoints/v1`, with `Kind`, `ReviewSkills`, and `ReviewPrompt` recorded in `CommittedMetadata`. +7. The `CheckpointSummary` sets `HasReview = true` for O(1) lookup. `HasReview` is an umbrella "any review happened" flag — future review kinds (e.g. manual review) should also set it. +8. `entire status` and the re-run guard read `HasReview` from the checkpoint metadata (no commit history walking). ## Checkpoint Metadata @@ -50,9 +86,9 @@ Review metadata is stored at two levels on `entire/checkpoints/v1`: ## Architecture -- **`AgentReviewer` interface** (`cmd/entire/cli/review/types/reviewer.go`): per-agent contract with `Name() string` and `Start(ctx, RunConfig) (Process, error)`. Each launchable agent implements this in its own package. +- **`AgentReviewer` interface** (`cmd/entire/cli/review/types/reviewer.go`): per-agent contract with `Name() string` and `Start(ctx, RunConfig) (Process, error)`. Each adapter-backed review worker implements this in its own package. - **`ReviewerTemplate`** (`cmd/entire/cli/review/types/template.go`): shared scaffolding (Spawn → pipe stdout → run parser → forward events → close). Each agent supplies only its `BuildCmd` (argv/env) and `Parser` (stdout-to-Event stream). -- **`Sink` interface**: consumers of the event stream. Production sinks: `DumpSink` (post-run per-agent narrative), `TUISink` (Bubble Tea live dashboard with Ctrl+O drill-in), `SynthesisSink` (opt-in y/N cross-agent verdict). Sinks are composed by `composeMultiAgentSinks` based on TTY detection. +- **`Sink` interface**: consumers of the event stream. Production sinks: `DumpSink` (post-run per-agent narrative), `TUISink` (Bubble Tea live dashboard with Ctrl+O drill-in), `SynthesisSink` (profile-master final report / legacy prompted synthesis). Sinks are composed by `composeMultiAgentSinks` based on TTY detection. - **`Run(ctx, reviewer, cfg, sinks)`** (`cmd/entire/cli/review/run.go`): single-agent orchestrator. Forwards events to all sinks via `AgentEvent`, calls `RunFinished` once at end with a populated `RunSummary`. Sink dispatch is serialized; sinks need not internally synchronize. - **`RunMulti(ctx, reviewers, cfg, sinks)`** (`cmd/entire/cli/review/run_multi.go`): N-agent orchestrator. Each agent runs concurrently in its own goroutine; events fan into a single dispatch loop so the serial-dispatch contract is preserved. Per-agent skills/prompts are injected via `perAgentConfiguredReviewer` adapter (each reviewer sees its own `RunConfig` despite the shared API surface). - **Env-var contract** (`cmd/entire/cli/review/env.go`): single source of truth for `ENTIRE_REVIEW_*` constants used by spawn-side and lifecycle adoption. @@ -60,10 +96,10 @@ Review metadata is stored at two levels on `entire/checkpoints/v1`: ## Multi-Agent UI -When `RunMulti` is dispatched in a TTY, the sink slice is `[TUISink, DumpSink, SynthesisSink?]`: +When `RunMulti` is dispatched in a TTY, the sink slice is `[TUISink, DumpSink, SynthesisSink]` for profiles with a master: - **`TUISink` / `reviewTUIModel`** (`cmd/entire/cli/review/tui_sink.go`, `tui_model.go`, `tui_detail.go`): live dashboard with one row per agent (name, status, tokens, last assistant preview, duration). `Ctrl+O` enters drill-in mode on the alt screen showing the full event buffer for the selected agent; `Esc` returns to the dashboard. `Ctrl+C` cancels the run via the shared `CancelFunc`. The model uses `tea.WithoutSignalHandler` so the cobra root retains SIGINT routing. After all agents finish, the user dismisses with any key — `RunFinished` blocks on dismissal so `DumpSink` renders below the TUI rather than overlapping it. -- **`SynthesisSink`** (`cmd/entire/cli/review/synthesis_sink.go`): opt-in y/N prompt offered after the dump. On "y", composes a synthesis prompt covering all agent narratives + per-run user prompt, calls the configured summary provider, and prints the unified verdict. Skipped silently when stdin can't prompt, the run was cancelled, or fewer than 2 agents produced usable output. Provider failures degrade gracefully ("synthesis unavailable: ") so the user can still commit. +- **`SynthesisSink`** (`cmd/entire/cli/review/synthesis_sink.go`): in profile-native mode, runs automatically after the dump, composes an adjudication prompt covering all worker narratives + per-run user prompt + profile task, calls the profile master agent, and prints the final report. Skipped when the run was cancelled or fewer than 2 workers produced usable output. Provider failures degrade gracefully ("final report unavailable: ") so the user can still commit. The old prompted y/N mode remains available for tests/legacy callers but `entire review` uses auto mode. - **Sink composition** (`composeMultiAgentSinks` in `cmd/entire/cli/review/cmd.go`): pure helper taking explicit `isTTY`/`canPrompt` so tests don't depend on real TTY detection. `findTUISink` picks the TUI out of the slice for `Start`/`Wait` lifecycle hooks. ## Skill Discovery (Claude Code) @@ -76,7 +112,7 @@ For the plugin cache, `pickLatestVersion` picks ONE version directory per plugin The redesign eliminated several constructs from the prior implementation. None should be reintroduced without explicit design: -- `PendingReviewMarker` for launchable agents (env-var handshake makes it unnecessary) +- `PendingReviewMarker` for adapter-backed review workers (env-var handshake makes it unnecessary) - `WorktreePath` field + worktree-scoping logic (env per process eliminates the multi-tenant problem) - `AgentEntries` map on the marker (each agent has its own env) - Marker overwrite tripwire / refuse-attach guard (the bug classes they defended against don't exist) @@ -89,17 +125,17 @@ The redesign eliminated several constructs from the prior implementation. None s ## Key Files - `cmd/entire/cli/review/cmd.go` — `NewCommand()`, `runReview` dispatch fork, `composeMultiAgentSinks` -- `cmd/entire/cli/review/picker.go` / `multipicker.go` — config-edit picker, first-run setup, single- and multi-agent selection +- `cmd/entire/cli/review/picker.go` / `profile.go` — profile config picker, first-run setup, profile resolution/default tasks - `cmd/entire/cli/review/attach.go` + `cli/review_helpers.go:newReviewAttachCmd` — `entire review attach` subcommand -- `cmd/entire/cli/review/marker_fallback.go` — non-launchable agent flow (single shared file) +- `cmd/entire/cli/review/marker_fallback.go` — manual fallback for agents without review-runner adapters yet (single shared file) - `cmd/entire/cli/review/prompt.go` / `scope.go` / `run.go` / `dump.go` / `run_multi.go` — core machinery (single-agent + N-agent fan-in) - `cmd/entire/cli/review/tui_sink.go` / `tui_model.go` / `tui_detail.go` — Bubble Tea TUI sink -- `cmd/entire/cli/review/synthesis_sink.go` / `synthesis_prompt.go` — opt-in cross-agent verdict +- `cmd/entire/cli/review/synthesis_sink.go` / `synthesis_prompt.go` — profile master adjudication (runs automatically for multi-worker profiles) plus the legacy opt-in synthesis path - `cmd/entire/cli/review/types/{reviewer,sink,template}.go` — interface contracts (CU2 + CU4 + CU5b) - `cmd/entire/cli/review/env.go` — `ENTIRE_REVIEW_*` constants + `EncodeSkills`/`DecodeSkills` + `AppendReviewEnv` - `cmd/entire/cli/agent/{claudecode,codex,geminicli}/reviewer.go` — per-agent `AgentReviewer` implementations (claude-code, codex, gemini-cli) - `cmd/entire/cli/agent/claudecode/discovery.go` — skill discovery + `pickLatestVersion` plugin-cache dedupe - `cmd/entire/cli/lifecycle.go` — `adoptReviewEnv` reads `ENTIRE_REVIEW_*` from process env; replaces marker-file adoption -- `cmd/entire/cli/review_bridge.go` / `review_helpers.go` — bridge code in `cli` package for cycle-bound functions (`headHasReviewCheckpoint`, `launchableReviewerFor`, `newReviewAttachCmd`, `lazySynthesisProvider`) +- `cmd/entire/cli/review_bridge.go` / `review_helpers.go` — bridge code in `cli` package for cycle-bound functions (`headHasReviewCheckpoint`, `launchableReviewerFor`, `newReviewAttachCmd`) - `cmd/entire/cli/checkpoint/checkpoint.go` — `Kind`, `ReviewSkills`, `ReviewPrompt` on `CommittedMetadata`; `HasReview` on `CheckpointSummary` -- `cmd/entire/cli/settings/settings.go` — `EntireSettings.Review` field +- `cmd/entire/cli/settings/settings.go` — `EntireSettings.ReviewProfiles` + `EntireSettings.ReviewDefaultProfile` (the old `EntireSettings.Review` map is parse-tolerated but unused by `entire review`)