From 2dbd78aa52021656023a8391648e79083b14dbcb Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Thu, 4 Jun 2026 13:26:20 -0400 Subject: [PATCH] feat(review): role-driven UX cutover + TUI (staging, fix picker, muted output) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command-layer redesign of `entire review`, stacked on the codex correctness PR. Drops the legacy in-flow picker; reviewer membership is decided up front by roles (`entire review setup`) or one-off --reviewers/--fixer flags. - Role resolution + invoker-aware non-interactive fallback; recursion guard on ENTIRE_REVIEW_SESSION; `entire review fix` promoted to a real subcommand (legacy --fix hidden). Deletes the spawn-time multi-agent picker. - Pre-launch staging view: scope banner + itemised checkpoints/sessions + optional per-run prompt in one huh form before fan-out. - Inline post-review fix prompt [Y]es/[s]elect/[n]o/[A]lways; unified navigable source⇄findings fix picker (aggregate selectable); FixerOf is the single source of truth for the fix agent. - Live-token display (TUI input-only during streaming) and muted markdown palette for dense review/synthesis output. - Opt-in roles (new agents default Skip) with a ≥1-reviewer guard. - Addresses PR review feedback: codex skill seeding via seedDefaultSkills (name-matched on-disk discovery when curated builtins are empty), codex-only completion footer, inline_prompt doc/test naming. Co-Authored-By: Claude Opus 4.8 (1M context) Entire-Checkpoint: 91311075f208 --- cmd/entire/cli/mdrender/mdrender.go | 55 +- cmd/entire/cli/review/banner.go | 63 +++ cmd/entire/cli/review/banner_test.go | 92 ++++ cmd/entire/cli/review/cmd.go | 509 +++++++++++-------- cmd/entire/cli/review/cmd_test.go | 531 +++++++++++--------- cmd/entire/cli/review/dump.go | 10 +- cmd/entire/cli/review/export_test.go | 11 +- cmd/entire/cli/review/fix.go | 371 ++++++++------ cmd/entire/cli/review/fix_findings_test.go | 69 +++ cmd/entire/cli/review/flags.go | 99 ++++ cmd/entire/cli/review/flags_test.go | 146 ++++++ cmd/entire/cli/review/inline_prompt.go | 67 +++ cmd/entire/cli/review/inline_prompt_test.go | 86 ++++ cmd/entire/cli/review/manifest.go | 30 +- cmd/entire/cli/review/manifest_test.go | 191 +++---- cmd/entire/cli/review/multipicker.go | 106 ---- cmd/entire/cli/review/multipicker_test.go | 96 ---- cmd/entire/cli/review/picker.go | 283 +---------- cmd/entire/cli/review/picker_test.go | 71 +++ cmd/entire/cli/review/post_review.go | 277 ++++++++++ cmd/entire/cli/review/post_review_test.go | 313 ++++++++++++ cmd/entire/cli/review/setup.go | 186 ++++++- cmd/entire/cli/review/setup_invoker_test.go | 46 ++ cmd/entire/cli/review/setup_test.go | 78 ++- cmd/entire/cli/review/synthesis_sink.go | 17 +- cmd/entire/cli/review/tui_model.go | 21 +- cmd/entire/cli/review/tui_model_test.go | 56 +++ cmd/entire/cli/review_bridge.go | 1 - cmd/entire/cli/review_context.go | 103 +++- cmd/entire/cli/review_context_test.go | 27 +- docs/architecture/review-command.md | 120 ++++- 31 files changed, 2876 insertions(+), 1255 deletions(-) create mode 100644 cmd/entire/cli/review/banner.go create mode 100644 cmd/entire/cli/review/banner_test.go create mode 100644 cmd/entire/cli/review/fix_findings_test.go create mode 100644 cmd/entire/cli/review/flags.go create mode 100644 cmd/entire/cli/review/flags_test.go create mode 100644 cmd/entire/cli/review/inline_prompt.go create mode 100644 cmd/entire/cli/review/inline_prompt_test.go delete mode 100644 cmd/entire/cli/review/multipicker.go delete mode 100644 cmd/entire/cli/review/multipicker_test.go create mode 100644 cmd/entire/cli/review/post_review.go create mode 100644 cmd/entire/cli/review/post_review_test.go create mode 100644 cmd/entire/cli/review/setup_invoker_test.go diff --git a/cmd/entire/cli/mdrender/mdrender.go b/cmd/entire/cli/mdrender/mdrender.go index d78fe1c74..ab9fc065b 100644 --- a/cmd/entire/cli/mdrender/mdrender.go +++ b/cmd/entire/cli/mdrender/mdrender.go @@ -36,7 +36,19 @@ const DefaultTerminalWidth = 80 // of which indicate a malformed StyleConfig (programmer error) rather // than a runtime condition. Renderer panics are recovered and returned as // errors so callers can fall back to raw markdown instead of crashing. -func Render(markdown string, width int, darkBackground bool) (rendered string, err error) { +func Render(markdown string, width int, darkBackground bool) (string, error) { + return renderWithStyles(markdown, width, stylesForBackground(darkBackground)) +} + +// RenderMuted is Render with a low-chroma palette: hierarchy is conveyed by +// bold + indentation rather than colour, and inline-code/link highlighting is +// dropped. Use it for dense, markdown-heavy output (e.g. the multi-agent +// review dump) where the full palette reads as noisy and hard to scan. +func RenderMuted(markdown string, width int, darkBackground bool) (string, error) { + return renderWithStyles(markdown, width, mutedStyles(darkBackground)) +} + +func renderWithStyles(markdown string, width int, styles ansi.StyleConfig) (rendered string, err error) { defer func() { if r := recover(); r != nil { rendered = "" @@ -45,7 +57,7 @@ func Render(markdown string, width int, darkBackground bool) (rendered string, e }() renderer, err := glamour.NewTermRenderer( - glamour.WithStyles(stylesForBackground(darkBackground)), + glamour.WithStyles(styles), glamour.WithWordWrap(width), glamour.WithPreservedNewLines(), ) @@ -73,6 +85,15 @@ func RenderForWriter(w io.Writer, markdown string) (string, error) { return Render(markdown, terminalWidth(w), termenv.HasDarkBackground()) } +// RenderMutedForWriter is RenderForWriter using the low-chroma palette (see +// RenderMuted). Non-terminal / NO_COLOR writers still get raw markdown. +func RenderMutedForWriter(w io.Writer, markdown string) (string, error) { + if !shouldRender(w) { + return markdown, nil + } + return RenderMuted(markdown, terminalWidth(w), termenv.HasDarkBackground()) +} + // shouldRender returns true if w is a terminal writer and NO_COLOR is unset. func shouldRender(w io.Writer) bool { if os.Getenv("NO_COLOR") != "" { @@ -168,6 +189,36 @@ func stylesForBackground(darkBackground bool) ansi.StyleConfig { return styles } +// mutedStyles returns a calmer variant of the CLI palette for dense, +// markdown-heavy output (the multi-agent review dump). It KEEPS the coloured, +// bold headings — they're sparse and give the same scannable structure as +// dispatch — and only neutralises the HIGH-FREQUENCY inline elements that +// multiply with dense findings and read as noise: the highlight block behind +// inline code (file paths), coloured list bullets, and coloured links. Bold +// emphasis (e.g. severity labels) is left intact. +func mutedStyles(darkBackground bool) ansi.StyleConfig { + styles := stylesForBackground(darkBackground) + neutral := "252" + if !darkBackground { + neutral = "234" + } + // Inline code: drop the background highlight and the orange foreground — + // file paths appear in nearly every finding, so the block + accent read as + // a sea of colour. Keep it as plain (slightly emphasised by mono) text. + styles.Code.Color = strPtr(neutral) + styles.Code.BackgroundColor = nil + // List bullets / enumeration markers: neutral, not orange/indigo. + styles.Item.Color = strPtr(neutral) + styles.Enumeration.Color = strPtr(neutral) + // Links: keep the underline as the affordance, drop the colour + bold so a + // finding full of [file](path) links isn't multi-coloured. + styles.Link.Color = nil + styles.Link.Underline = boolPtrV(true) + styles.LinkText.Color = nil + styles.LinkText.Bold = boolPtrV(false) + return styles +} + // chromaForBackground returns the syntax-highlighting palette for code // blocks. Dark and light backgrounds use distinct text colors but share // the same accent colors for keywords/functions/literals. diff --git a/cmd/entire/cli/review/banner.go b/cmd/entire/cli/review/banner.go new file mode 100644 index 000000000..7d789864f --- /dev/null +++ b/cmd/entire/cli/review/banner.go @@ -0,0 +1,63 @@ +package review + +import ( + "fmt" + "strings" +) + +// formatContextBanner returns the transparency block printed below the scope +// banner. It itemises the prior checkpoint/session context `entire review` is +// folding into the agent prompt so the user can see exactly what's being +// reviewed — the value over running the underlying skill manually. The block +// is never omitted; the empty variant reassures the user nothing went wrong, +// there simply is no history. +// +// Example: +// +// Checkpoints in scope (2): +// • a3b2c4d5 feat(review): emit honest live tokens +// • b4c3d5e6 feat(review): flag-driven roles +// In-progress sessions (1): +// • ac3d5c6e Claude Code +// +// When counts are present but the itemised slices aren't populated (defensive), +// it falls back to a one-line count summary. +func formatContextBanner(r ContextResult) string { + if r.Checkpoints == 0 && r.Sessions == 0 { + return "No prior session or checkpoint context for this branch yet." + } + var b strings.Builder + switch { + case len(r.CheckpointItems) > 0: + fmt.Fprintf(&b, "Checkpoints in scope (%d):\n", len(r.CheckpointItems)) + for _, c := range r.CheckpointItems { + summary := c.Summary + if summary == "" { + summary = "(no summary)" + } + fmt.Fprintf(&b, " • %s %s\n", c.ID, summary) + } + case r.Checkpoints > 0: + fmt.Fprintf(&b, "%s in scope.\n", pluralizeContextNoun(r.Checkpoints, "checkpoint", "checkpoints")) + } + switch { + case len(r.SessionItems) > 0: + fmt.Fprintf(&b, "In-progress sessions (%d):\n", len(r.SessionItems)) + for _, s := range r.SessionItems { + fmt.Fprintf(&b, " • %s %s\n", s.ID, s.Agent) + } + case r.Sessions > 0: + fmt.Fprintf(&b, "%s in progress.\n", pluralizeContextNoun(r.Sessions, "session", "sessions")) + } + return strings.TrimRight(b.String(), "\n") +} + +// pluralizeContextNoun returns " " when n == 1 and +// " " otherwise. Kept private to banner.go; the review package +// has no other plural cases that would justify a shared utility. +func pluralizeContextNoun(n int, singular, plural string) string { + if n == 1 { + return fmt.Sprintf("%d %s", n, singular) + } + return fmt.Sprintf("%d %s", n, plural) +} diff --git a/cmd/entire/cli/review/banner_test.go b/cmd/entire/cli/review/banner_test.go new file mode 100644 index 000000000..8bd7048e4 --- /dev/null +++ b/cmd/entire/cli/review/banner_test.go @@ -0,0 +1,92 @@ +package review + +import "testing" + +// TestFormatContextBanner pins the itemised scope banner: an empty state, the +// itemised checkpoints+sessions layout, and the count-only fallback used when +// items aren't populated. +func TestFormatContextBanner(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + in ContextResult + want string + }{ + { + name: "neither", + in: ContextResult{}, + want: "No prior session or checkpoint context for this branch yet.", + }, + { + name: "itemised checkpoints and sessions", + in: ContextResult{ + Checkpoints: 2, Sessions: 1, + CheckpointItems: []CheckpointScopeItem{ + {ID: "a3b2c4d5", Summary: "feat(review): emit honest live tokens"}, + {ID: "b4c3d5e6", Summary: "feat(review): flag-driven roles"}, + }, + SessionItems: []SessionScopeItem{ + {ID: "ac3d5c6e", Agent: "Claude Code"}, + }, + }, + want: "Checkpoints in scope (2):\n" + + " • a3b2c4d5 feat(review): emit honest live tokens\n" + + " • b4c3d5e6 feat(review): flag-driven roles\n" + + "In-progress sessions (1):\n" + + " • ac3d5c6e Claude Code", + }, + { + name: "sessions listed by short id and agent", + in: ContextResult{ + Sessions: 2, + SessionItems: []SessionScopeItem{ + {ID: "ac3d5c6e", Agent: "Claude Code"}, + {ID: "3d4c9f88", Agent: "Codex"}, + }, + }, + want: "In-progress sessions (2):\n" + + " • ac3d5c6e Claude Code\n" + + " • 3d4c9f88 Codex", + }, + { + name: "count-only fallback when items absent", + in: ContextResult{Checkpoints: 3, Sessions: 1}, + want: "3 checkpoints in scope.\n1 session in progress.", + }, + { + name: "empty summary renders placeholder", + in: ContextResult{ + Checkpoints: 1, + CheckpointItems: []CheckpointScopeItem{{ID: "a3b2c4d5"}}, + }, + want: "Checkpoints in scope (1):\n • a3b2c4d5 (no summary)", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + if got := formatContextBanner(tc.in); got != tc.want { + t.Errorf("formatContextBanner(%+v) =\n%q\nwant\n%q", tc.in, got, tc.want) + } + }) + } +} + +func TestPluralizeContextNoun(t *testing.T) { + t.Parallel() + + tests := []struct { + n int + want string + }{ + {n: 1, want: "1 checkpoint"}, + {n: 2, want: "2 checkpoints"}, + {n: 0, want: "0 checkpoints"}, + } + for _, tc := range tests { + if got := pluralizeContextNoun(tc.n, "checkpoint", "checkpoints"); got != tc.want { + t.Errorf("pluralizeContextNoun(%d) = %q, want %q", tc.n, got, tc.want) + } + } +} diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index e50a11fe2..13191366c 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -14,6 +14,7 @@ import ( "io" "log/slog" "os" + "strings" "charm.land/huh/v2" "github.com/spf13/cobra" @@ -30,6 +31,42 @@ import ( "github.com/entireio/cli/cmd/entire/cli/settings" ) +// ContextResult bundles the composed checkpoint/session context with +// the counts of checkpoints and in-progress sessions reflected in it. +// Counts power the transparency banner; Prompt is what flows into the +// agent's composed review prompt. +type ContextResult struct { + // Prompt is the composed context text injected into the agent prompt. + // Empty when no checkpoints or sessions contributed. + Prompt string + // Checkpoints is the number of unique committed checkpoints rendered in + // the composed prompt (capped at the renderer's internal limit; the + // truncation tail is not counted). + Checkpoints int + // Sessions is the number of in-progress sessions rendered in the + // composed prompt. + Sessions int + // CheckpointItems lists the committed checkpoints in scope (short id + + // one-line summary) for the human-facing scope banner. Len matches + // Checkpoints. + CheckpointItems []CheckpointScopeItem + // SessionItems lists the in-progress sessions in scope (agent + latest + // prompt) for the scope banner. Len matches Sessions. + SessionItems []SessionScopeItem +} + +// CheckpointScopeItem is one committed checkpoint shown in the scope banner. +type CheckpointScopeItem struct { + ID string // short checkpoint id (first 8 hex chars) + Summary string // one-line commit subject / checkpoint summary +} + +// SessionScopeItem is one in-progress session shown in the scope banner. +type SessionScopeItem struct { + ID string // short session id (first 8 chars) — stable, unlike a long prompt + Agent string // display name of the agent that owns the session +} + // Deps collects the runtime-injectable hooks NewCommand needs from the // parent cli package. Tests stub fields to drive branches that would // otherwise require a real TTY or enabled repo. Production wiring is @@ -43,23 +80,17 @@ 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. HeadHasReviewCheckpoint func(ctx context.Context) (bool, string) // ReviewCheckpointContext returns best-effort checkpoint context for the - // branch review scope. Injected from the cli package because checkpoint - // readers cannot be imported here without cycling through agent reviewers. - ReviewCheckpointContext func(ctx context.Context, worktreeRoot string, scopeBaseRef string) string + // branch review scope along with the counts of checkpoints and in-progress + // sessions reflected in the composed prompt. Counts power the transparency + // banner. Injected from the cli package because checkpoint readers cannot + // be imported here without cycling through agent reviewers. + ReviewCheckpointContext func(ctx context.Context, worktreeRoot string, scopeBaseRef string) ContextResult // ReviewerFor maps an agent registry name to its AgentReviewer // implementation. Returns nil for non-launchable agents (cursor, opencode, @@ -84,14 +115,6 @@ type Deps struct { 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. @@ -102,6 +125,9 @@ func NewCommand(deps Deps) *cobra.Command { var findings bool var fix bool var all bool + var perRunPrompt string + var reviewersFlag []string + var fixerFlag string cmd := &cobra.Command{ Use: "review", @@ -111,8 +137,7 @@ func NewCommand(deps Deps) *cobra.Command { 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. +preferences are loaded from Entire settings. Labs entry: review is experimental. We are actively refining it based on user feedback. @@ -121,20 +146,20 @@ 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 - --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) - --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, - origin/main, origin/master, main, master. + --edit re-open the review config picker (alias for setup) + --findings browse local review findings + --fix DEPRECATED: use 'entire review fix' instead + --all with --fix, apply all sources/findings without selectors + --agent NAME select a specific configured agent (default: alphabetically first) + --base REF scope the review against REF instead of mainline. + --prompt TEXT per-run extra context (bypasses the inline ask) + --reviewers LIST one-off override: agents to use as reviewers (comma-separated) + --fixer NAME one-off override: agent to use as fixer Subcommands: - attach tag an existing session as a review (equivalent to - 'entire attach --review ')`, + setup configure reviewers, fixer, and per-agent skills + fix [session-id] apply review findings via the configured Fixer + attach tag an existing session as a 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)) @@ -163,61 +188,79 @@ Subcommands: if modes > 1 { return errors.New("--edit, --findings, and --fix 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 edit { - _, err := RunReviewConfigPicker(ctx, cmd.OutOrStdout(), deps.GetAgentsWithHooksInstalled) - return err - } if findings { return runReviewFindings(ctx, cmd, deps.NewSilentError) } if fix { + fmt.Fprintln(cmd.ErrOrStderr(), + "Hint: `entire review --fix` is deprecated; use `entire review fix` instead.") target := "" if len(args) == 1 { target = args[0] } - return runReviewFix(ctx, cmd, target, all, agentOverride, deps.NewSilentError) + return runReviewFix(ctx, cmd, target, all, agentOverride, perRunPrompt, deps.NewSilentError) } - innerDeps := runReviewDeps{ - promptForAgentFn: deps.PromptForAgentFn, - multiPickerFn: deps.MultiPickerFn, + // The migration prompt is only relevant for flows that write or + // read picker config. + if err := maybePromptReviewSettingsMigration( + ctx, + cmd.OutOrStdout(), + cmd.ErrOrStderr(), + interactive.IsTerminalWriter(cmd.OutOrStdout()) && interactive.CanPromptInteractively(), + deps.PromptYN, + ); err != nil { + return err + } + if edit { + external.DiscoverAndRegister(ctx) + _, err := RunSetup(ctx, cmd.OutOrStdout(), + deps.GetAgentsWithHooksInstalled, SetupForms{}) + return err } - return runReview(ctx, cmd, agentOverride, baseOverride, deps, innerDeps) + return runReview(ctx, cmd, agentOverride, baseOverride, perRunPrompt, reviewersFlag, fixerFlag, deps) }, } - cmd.Flags().BoolVar(&edit, "edit", false, "re-open the review config picker") + cmd.Flags().BoolVar(&edit, "edit", false, "re-open the review config picker (alias for `entire review setup`)") 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(&fix, "fix", false, "DEPRECATED: use `entire review fix` instead") 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().StringVar(&baseOverride, "base", "", "git ref to scope the review against (default: origin/HEAD → origin/main → origin/master → main → master)") + cmd.Flags().StringVar(&perRunPrompt, "prompt", "", "per-run extra context appended to reviewer instructions; bypasses the inline ask") + cmd.Flags().StringSliceVar(&reviewersFlag, "reviewers", nil, "one-off override: agents to use as reviewers (comma-separated or repeatable; e.g. --reviewers claude-code,codex)") + cmd.Flags().StringVar(&fixerFlag, "fixer", "", "one-off override: agent to use as fixer (e.g. --fixer codex)") + // Hide the deprecated --fix flag from help but keep it functional for one + // release; the RunE prints a hint redirecting users to `entire review fix`. + if err := cmd.Flags().MarkHidden("fix"); err != nil { + // Should not happen for a flag we just declared. + logging.Debug(context.Background(), "mark --fix hidden failed", slog.String("error", err.Error())) + } if deps.AttachCmd != nil { cmd.AddCommand(deps.AttachCmd) } cmd.AddCommand(newReviewSetupCmd(deps)) + cmd.AddCommand(newReviewFixCmd(deps)) return cmd } -// runReview executes the main review flow. -func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverride string, deps Deps, innerDeps runReviewDeps) error { +// runReview executes the main review flow. perRunPrompt is the value of +// the --prompt flag (empty when not set; the inline ask runs when empty +// and stdin is promptable). reviewersFlag / fixerFlag carry the one-off +// role overrides; when either is non-empty, saved config is replaced +// for this invocation only and is NOT persisted. +func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverride, perRunPrompt string, reviewersFlag []string, fixerFlag string, deps Deps) error { out := cmd.OutOrStdout() silentErr := deps.NewSilentError + // 0. Recursion guard. Prevents fan-out loops when a reviewer agent's + // prompt accidentally tries to re-invoke `entire review`. + if os.Getenv(EnvSession) != "" { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "Already in a review session — refusing to start a nested review.") + fmt.Fprintln(cmd.ErrOrStderr(), "If you reached this from a reviewer agent's prompt, this is likely a loop and you should exit the inner agent.") + return silentErr(errors.New("nested review session refused")) + } + // 1. Pre-flight: must be in a git repo. if _, err := paths.WorktreeRoot(ctx); err != nil { cmd.SilenceUsage = true @@ -227,99 +270,141 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverr // 2. Load config. A load error means the settings file exists but is // malformed (Load returns a default-filled object when the file is - // missing). Surface the error instead of silently opening the picker, - // which would cause the config writer to write over the user's other - // settings with an empty EntireSettings{}. + // missing). Surface the error instead of silently overwriting. s, err := settings.Load(ctx) if err != nil { cmd.SilenceUsage = true fmt.Fprintf(cmd.ErrOrStderr(), "Failed to load settings: %v\n", err) fmt.Fprintln(cmd.ErrOrStderr(), - "Fix your Entire settings or clone-local review preferences and re-run `entire review`.") + "Fix your Entire settings and re-run `entire review`.") return silentErr(err) } - if s == nil || len(s.Review) == 0 { - if !ConfirmFirstRunSetup(ctx, out) { - return nil + + installed := deps.GetAgentsWithHooksInstalled(ctx) + + // 3. Flag-based one-off overrides take precedence over saved config. + override, overrideErr := resolveRolesFromFlags(reviewersFlag, fixerFlag, installed) + if overrideErr != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), overrideErr.Error()) + return silentErr(overrideErr) + } + if override != nil { + // `entire review` needs at least one Reviewer (or Both). Flag-based + // override that only sets a Fixer is rejected here, not in + // resolveRolesFromFlags, because `entire review fix` legitimately + // resolves a fixer-only override (no reviewer needed). + hasReviewer := false + for _, cfg := range override { + if cfg.Role.IsReviewer() { + hasReviewer = true + break + } } - picked, pickErr := RunReviewConfigPicker(ctx, out, deps.GetAgentsWithHooksInstalled) - if pickErr != nil { - return pickErr + if !hasReviewer { + cmd.SilenceUsage = true + msg := errors.New( + "--fixer alone defines a fixer with no reviewer; entire review needs at least one " + + "(pass --reviewers to fix this, or run `entire review fix` for fix-only operations)", + ) + fmt.Fprintln(cmd.ErrOrStderr(), msg.Error()) + return silentErr(msg) } if s == nil { s = &settings.EntireSettings{} } - s.Review = picked - fmt.Fprintln(out) - fmt.Fprintln(out, "Setup complete — running review now.") + s.Review = mergeFlagOverrideWithSavedSkills(ctx, override, s.Review) } - // 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) - } - } + // userExplicitlyOmittedFixer = `--reviewers` passed but `--fixer` empty. + // Drives the post-review footer (offers `--fixer ` hint instead + // of the setup nag when no Fixer is configured). + userExplicitlyOmittedFixer := len(reviewersFlag) > 0 && strings.TrimSpace(fixerFlag) == "" - // 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 - } - picked, pickErr := fn(ctx, eligible) - if pickErr != nil { - cmd.SilenceUsage = true - fmt.Fprintln(cmd.ErrOrStderr(), pickErr.Error()) - return silentErr(pickErr) - } - if picked == "" { - // Defensive: empty picker return must not fall through to - // alphabetical-first default. - cmd.SilenceUsage = true - emptyErr := errors.New("agent picker returned empty agent name") - fmt.Fprintln(cmd.ErrOrStderr(), emptyErr.Error()) - return silentErr(emptyErr) - } - agentOverride = picked + // 4. Replace the legacy auto-picker with the invoker-aware fallback. + if s == nil || len(s.Review) == 0 { + if interactive.CanPromptInteractively() { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "Not configured yet.") + fmt.Fprintln(cmd.ErrOrStderr()) + fmt.Fprintln(cmd.ErrOrStderr(), "Run: entire review setup") + return silentErr(errors.New("review not configured")) + } + // Non-interactive: try invoker-only fallback. + invoker := DetectInvokingAgent() + if invoker == "" { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), + "Cannot run review: no config, no --reviewers/--fixer flags, and no invoking agent detected (CI / piped).") + fmt.Fprintln(cmd.ErrOrStderr(), "Run: entire review setup (from an interactive shell)") + return silentErr(errors.New("review not configured and no invoker")) + } + cfg, bErr := invokerOnlyReviewConfig(ctx, invoker, installed) + if bErr != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "Cannot run review:", bErr) + fmt.Fprintln(cmd.ErrOrStderr(), "Hint: from an interactive shell, run `entire review setup`.") + return silentErr(bErr) + } + if s == nil { + s = &settings.EntireSettings{} + } + s.Review = cfg + // Intentionally do NOT persist — this is a one-off fallback like + // --reviewers/--fixer. Persisting would create hidden state: a user + // who runs first from Claude Code, later from Codex, would silently + // get the saved Claude-Both config instead of switching to Codex-Both. + fmt.Fprintln(out, formatInvokerOnlyNote(invoker)) + } + + // 5. Resolve reviewers from roles. Roles answer "who reviews" up front; + // the spawn-time multi-agent picker is gone. + reviewers := ReviewersOf(s) + if len(reviewers) == 0 { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "No agents are configured as Reviewers.") + fmt.Fprintln(cmd.ErrOrStderr(), "Run: entire review setup") + return silentErr(errors.New("no reviewers configured")) + } + + // 6. The optional inline per-run prompt ask happens inside the dispatch + // paths, AFTER the scope + checkpoint/session banner is printed — so the + // user sees everything that's about to be reviewed (the staging summary) + // before deciding what extra context to add, and the banner isn't + // immediately obscured by the live TUI. + + // 7. Dispatch: + // - With --agent override: single-agent path against the named agent. + // - Otherwise: all configured reviewers (1 = single, 2+ = multi). + if agentOverride != "" { + cfg, ok := s.Review[agentOverride] + if !ok || cfg.IsZero() { + err := fmt.Errorf("agent %q is not configured as a reviewer", agentOverride) + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return silentErr(err) } + return runSingleAgentPath(ctx, cmd, agentOverride, baseOverride, perRunPrompt, cfg, installed, deps, out, s, userExplicitlyOmittedFixer) } - agentName, cfg, err := SelectReviewAgent(s.Review, agentOverride) - if err != nil { - cmd.SilenceUsage = true - fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) - return silentErr(err) + if len(reviewers) == 1 { + name := reviewers[0] + cfg := s.Review[name] + return runSingleAgentPath(ctx, cmd, name, baseOverride, perRunPrompt, cfg, installed, deps, out, s, userExplicitlyOmittedFixer) } - return runSingleAgentPath(ctx, cmd, agentName, baseOverride, cfg, installed, deps, out) + choices := agentChoicesFrom(reviewers, s.Review) + return runMultiAgentPath(ctx, cmd, choices, baseOverride, perRunPrompt, s, deps, out, userExplicitlyOmittedFixer) +} + +// agentChoicesFrom converts a sorted slice of agent names into the +// AgentChoice slice consumed by runMultiAgentPath. +func agentChoicesFrom(names []string, m map[string]settings.ReviewConfig) []AgentChoice { + out := make([]AgentChoice, len(names)) + for i, n := range names { + out[i] = AgentChoice{Name: n, Label: labelForAgentChoice(n, m[n])} + } + return out } // runSingleAgentPath completes a single-agent review: verifies hooks + skills, @@ -328,11 +413,13 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverr func runSingleAgentPath( ctx context.Context, cmd *cobra.Command, - agentName, baseOverride string, + agentName, baseOverride, perRunPrompt string, cfg settings.ReviewConfig, installed []types.AgentName, deps Deps, out io.Writer, + s *settings.EntireSettings, + userExplicitlyOmittedFixer bool, ) error { silentErr := deps.NewSilentError @@ -395,19 +482,23 @@ func runSingleAgentPath( cmd.SilenceUsage = true return fmt.Errorf("resolve HEAD: %w", shaErr) } - scopeBaseRef, scopeErr := detectScope(ctx, worktreeRoot, baseOverride, out) + scopeBaseRef, scopeBanner, scopeErr := detectScope(ctx, worktreeRoot, baseOverride) if scopeErr != nil { cmd.SilenceUsage = true return scopeErr } - checkpointContext := "" + var ctxResult ContextResult if deps.ReviewCheckpointContext != nil { - checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) + ctxResult = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) } + // Staging step: present the scope + checkpoints/sessions and collect the + // optional per-run prompt in one styled view, before fan-out. + perRunPrompt = stagePerRunContext(ctx, out, scopeBanner, ctxResult, perRunPrompt) runCfg := reviewtypes.RunConfig{ + PerRunPrompt: perRunPrompt, ScopeBaseRef: scopeBaseRef, - CheckpointContext: checkpointContext, + CheckpointContext: ctxResult.Prompt, StartingSHA: headSHA, } applyReviewConfig(&runCfg, cfg) @@ -437,11 +528,16 @@ func runSingleAgentPath( } summary, waitErr := Run(runCtx, reviewer, runCfg, sinks) - writePostReviewManifest(ctx, out, worktreeRoot, headSHA, summary, "") + manifest := writePostReviewManifestAndReturn(ctx, out, worktreeRoot, headSHA, summary, "") if waitErr != nil && runCtx.Err() == nil && ctx.Err() == nil { // Non-cancellation error: surface to caller. return fmt.Errorf("review run: %w", waitErr) } + if manifest != nil { + if err := RunPostReviewFixPrompt(ctx, cmd, s, *manifest, perRunPrompt, silentErr, userExplicitlyOmittedFixer); err != nil { + return err + } + } return nil } @@ -453,18 +549,19 @@ func runSingleAgentPath( // returns ("", err) so the caller can fail-loudly before spawning agents. // Otherwise (auto-detection failed): returns "" and the caller proceeds in // degraded mode without a scope banner. -func detectScope(ctx context.Context, worktreeRoot, baseOverride string, out io.Writer) (string, error) { +// detectScope resolves the scope base ref and returns the human-facing scope +// banner string (e.g. "Reviewing feat/X vs main: 3 commits, …") for the caller +// to render — printing is left to the staging step so the banner can be folded +// into the styled per-run prompt. Returns ("", "", nil) in degraded mode (no +// override, auto-detection failed); a bad explicit --base aborts loudly. +func detectScope(ctx context.Context, worktreeRoot, baseOverride string) (baseRef, banner string, err error) { repo, openErr := gitrepo.OpenPath(worktreeRoot) if openErr != nil { logging.Debug(ctx, "review repo open failed", slog.String("error", openErr.Error())) - // Fail-loud when the user explicitly asked for a base. Without this - // branch an explicit --base flag would be silently dropped on - // PlainOpen failure, inconsistent with the ComputeScopeStats error - // path below that aborts on bad overrides. if baseOverride != "" { - return "", fmt.Errorf("--base %q given but cannot open repository at %q: %w", baseOverride, worktreeRoot, openErr) + return "", "", fmt.Errorf("--base %q given but cannot open repository at %q: %w", baseOverride, worktreeRoot, openErr) } - return "", nil + return "", "", nil } defer repo.Close() stats, statsErr := ComputeScopeStats(ctx, repo, baseOverride) @@ -473,49 +570,36 @@ func detectScope(ctx context.Context, worktreeRoot, baseOverride string, out io. // A bad ref must abort before agents spawn so the user learns about // the typo immediately, not after a long review run. if baseOverride != "" { - return "", statsErr + return "", "", statsErr } logging.Debug(ctx, "review scope detection failed", slog.String("error", statsErr.Error())) - return "", nil + return "", "", nil } - fmt.Fprintln(out, formatScopeBanner(stats)) - return stats.BaseRef, nil + return stats.BaseRef, formatScopeBanner(stats), 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. +// runMultiAgentPath handles the multi-agent review flow: builds per-agent +// RunConfigs and runs all selected agents concurrently via RunMulti. +// +// Roles answer "who reviews" up front (via `entire review setup` or the +// --reviewers flag), so this path no longer presents a spawn-time +// multi-select picker. // // 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. +// re-run guard) for brevity — the caller has already ensured each agent in +// `reviewers` has been chosen via the role model. func runMultiAgentPath( ctx context.Context, cmd *cobra.Command, - launchableEligible []AgentChoice, - baseOverride string, + choices []AgentChoice, + baseOverride, perRunPrompt string, s *settings.EntireSettings, - innerDeps runReviewDeps, deps Deps, out io.Writer, + userExplicitlyOmittedFixer bool, ) 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 { @@ -528,27 +612,33 @@ func runMultiAgentPath( return fmt.Errorf("resolve HEAD: %w", shaErr) } - scopeBaseRef, scopeErr := detectScope(ctx, worktreeRoot, baseOverride, out) + scopeBaseRef, scopeBanner, scopeErr := detectScope(ctx, worktreeRoot, baseOverride) if scopeErr != nil { cmd.SilenceUsage = true return scopeErr } - checkpointContext := "" + var ctxResult ContextResult if deps.ReviewCheckpointContext != nil { - checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) + ctxResult = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) } + // Staging step: present the scope + checkpoints/sessions and collect the + // optional per-run prompt in one styled view, before fan-out. + perRunPrompt = stagePerRunContext(ctx, out, scopeBanner, ctxResult, perRunPrompt) // 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 { + reviewers := make([]reviewtypes.AgentReviewer, 0, len(choices)) + for _, choice := range choices { + name := choice.Name agentCfg := s.Review[name] // zero value is safe (empty skills/prompt) reviewer := deps.ReviewerFor(name) if reviewer == nil { - // Shouldn't happen given launchableEligible was filtered for - // ReviewerFor != nil, but be defensive. + // Skip non-launchable agents — roles have already decided + // the set, but a non-launchable reviewer just falls through + // to marker fallback and the multi-agent dispatch path can't + // represent that. Surface a clear error instead. cmd.SilenceUsage = true - return silentErr(fmt.Errorf("agent %q is not launchable but appeared in eligible list", name)) + return silentErr(fmt.Errorf("agent %q is configured as a reviewer but is not launchable", name)) } // Wrap the reviewer so it sees the per-agent RunConfig at Start time. // We cannot pass a different RunConfig per reviewer in RunMulti's @@ -558,9 +648,9 @@ func runMultiAgentPath( reviewers = append(reviewers, &perAgentConfiguredReviewer{ inner: reviewer, cfg: runConfigWithReviewConfig(reviewtypes.RunConfig{ - PerRunPrompt: picked.PerRun, + PerRunPrompt: perRunPrompt, ScopeBaseRef: scopeBaseRef, - CheckpointContext: checkpointContext, + CheckpointContext: ctxResult.Prompt, StartingSHA: headSHA, }, agentCfg), }) @@ -598,7 +688,7 @@ func runMultiAgentPath( runContext: runCtx, synthesisProvider: deps.SynthesisProvider, promptYN: deps.PromptYN, - perRunPrompt: picked.PerRun, + perRunPrompt: perRunPrompt, onSynthesisResult: func(result string) { aggregateOutput = result }, @@ -616,25 +706,16 @@ func runMultiAgentPath( summary, waitErr := RunMulti(runCtx, reviewers, reviewtypes.RunConfig{ EnrichAgentRun: reviewAgentRunTokenEnricher(worktreeRoot, headSHA), }, sinks) - writePostReviewManifest(ctx, out, worktreeRoot, headSHA, summary, aggregateOutput) + manifest := writePostReviewManifestAndReturn(ctx, out, worktreeRoot, headSHA, summary, aggregateOutput) if waitErr != nil && runCtx.Err() == nil && ctx.Err() == nil { return fmt.Errorf("review run: %w", waitErr) } - return nil -} - -// handlePickerError maps multi-picker error sentinels to the appropriate -// command-layer response. -// - ErrPickerCancelled → return nil (user cancelled; no error shown) -// - ErrNoAgentsSelected → surface error to user -// - other errors → surface to user -func handlePickerError(cmd *cobra.Command, silentErr func(error) error, pickErr error) error { - if errors.Is(pickErr, ErrPickerCancelled) { - return nil + if manifest != nil { + if err := RunPostReviewFixPrompt(ctx, cmd, s, *manifest, perRunPrompt, silentErr, userExplicitlyOmittedFixer); err != nil { + return err + } } - cmd.SilenceUsage = true - fmt.Fprintln(cmd.ErrOrStderr(), pickErr.Error()) - return silentErr(pickErr) + return nil } // multiAgentSinkInputs collects the parameters composeMultiAgentSinks needs. @@ -701,30 +782,31 @@ func composeMultiAgentSinks(in multiAgentSinkInputs) []reviewtypes.Sink { return sinks } -func writePostReviewManifest( +// writePostReviewManifestAndReturn persists the post-review manifest to +// disk and returns it so the caller can thread it into the post-review +// fix prompt. Returns nil when the manifest was not written +// (cancellation, no findings, write failure, or no matching review +// session state). +func writePostReviewManifestAndReturn( ctx context.Context, out io.Writer, worktreeRoot string, headSHA string, summary reviewtypes.RunSummary, aggregateOutput string, -) { +) *LocalReviewManifest { if summary.Cancelled || len(summary.AgentRuns) == 0 { - return + return nil } manifest, states, err := localReviewManifestFromCurrentState(ctx, worktreeRoot, headSHA, summary, aggregateOutput) if err != nil { logging.Debug(ctx, "review manifest not written", slog.String("error", err.Error())) warnManifestNotWritten(out, "could not load session state: "+err.Error()) - return + return nil } if len(manifest.Sources) == 0 { reason, sentinel := explainEmptyManifest(worktreeRoot, headSHA, summary, states) if sentinel { - // Matcher and explainer have drifted — the matcher rejected - // every tagged session for a reason none of the explainer's - // filters cover. Surface at Warn so this gets noticed without - // requiring debug logging. logging.Warn(ctx, "review manifest matcher/explainer drift detected", slog.String("reason", reason), slog.Int("tagged_state_count", len(states)), @@ -734,14 +816,15 @@ func writePostReviewManifest( slog.String("reason", reason)) } warnManifestNotWritten(out, reason) - return + return nil } if err := writeLocalReviewManifest(ctx, manifest); err != nil { logging.Debug(ctx, "review manifest write failed", slog.String("error", err.Error())) warnManifestNotWritten(out, "write to disk failed: "+err.Error()) - return + return nil } writeReviewCompletionFooter(out, manifest) + return &manifest } // warnManifestNotWritten prints a user-visible note explaining that the @@ -796,6 +879,10 @@ func runConfigWithReviewConfig(base reviewtypes.RunConfig, cfg settings.ReviewCo } func applyReviewConfig(runCfg *reviewtypes.RunConfig, cfg settings.ReviewConfig) { + // Per-spawn tuning applies regardless of the skills-vs-prompt branch + // below; reviewers that don't support these knobs ignore them. + runCfg.Model = cfg.Model + runCfg.ReasoningEffort = cfg.ReasoningEffort runCfg.Skills = cfg.Skills if len(cfg.Skills) == 0 { runCfg.PromptOverride = cfg.Prompt diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index b9b11d7ea..51d976320 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -43,22 +43,6 @@ 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). -func seedReviewConfig(ctx context.Context, cfg map[string]settings.ReviewConfig) error { - prefs, err := settings.LoadClonePreferences(ctx) - if err != nil { - return err - } - if prefs == nil { - prefs = &settings.ClonePreferences{} - } - prefs.Review = cfg - return settings.SaveClonePreferences(ctx, prefs) -} - // TestReviewCmd_Help verifies `entire review --help` contains the expected // flags and subcommands without panicking. func TestReviewCmd_Help(t *testing.T) { @@ -97,38 +81,45 @@ func TestNewReviewCmd_NoHiddenFlags(t *testing.T) { } } -// TestReview_NotGitRepoReturnsSilentError checks that review outside a git repo -// returns a SilentError and prints the message once, for any mode flag. -func TestReview_NotGitRepoReturnsSilentError(t *testing.T) { - tests := []struct { - name string - args []string - }{ - {"findings", []string{"review", "--findings"}}, - {"fix", []string{"review", "--fix", "review-session"}}, +func TestReviewFindings_NotGitRepoReturnsSilentError(t *testing.T) { + t.Chdir(t.TempDir()) + + rootCmd := cli.NewRootCmd() + errBuf := &bytes.Buffer{} + rootCmd.SetErr(errBuf) + rootCmd.SetArgs([]string{"review", "--findings"}) + + err := rootCmd.Execute() + if err == nil { + t.Fatal("expected error outside a git repo") + } + var silentErr *cli.SilentError + if !errors.As(err, &silentErr) { + t.Fatalf("expected SilentError, got %T: %v", err, err) } + if got := strings.Count(errBuf.String(), "Not a git repository"); got != 1 { + t.Fatalf("not-git message count = %d, want 1; stderr:\n%s", got, errBuf.String()) + } +} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Chdir(t.TempDir()) +func TestReviewFix_NotGitRepoReturnsSilentError(t *testing.T) { + t.Chdir(t.TempDir()) - rootCmd := cli.NewRootCmd() - errBuf := &bytes.Buffer{} - rootCmd.SetErr(errBuf) - rootCmd.SetArgs(tt.args) + rootCmd := cli.NewRootCmd() + errBuf := &bytes.Buffer{} + rootCmd.SetErr(errBuf) + rootCmd.SetArgs([]string{"review", "--fix", "review-session"}) - err := rootCmd.Execute() - if err == nil { - t.Fatal("expected error outside a git repo") - } - var silentErr *cli.SilentError - if !errors.As(err, &silentErr) { - t.Fatalf("expected SilentError, got %T: %v", err, err) - } - if got := strings.Count(errBuf.String(), "Not a git repository"); got != 1 { - t.Fatalf("not-git message count = %d, want 1; stderr:\n%s", got, errBuf.String()) - } - }) + err := rootCmd.Execute() + if err == nil { + t.Fatal("expected error outside a git repo") + } + var silentErr *cli.SilentError + if !errors.As(err, &silentErr) { + t.Fatalf("expected SilentError, got %T: %v", err, err) + } + if got := strings.Count(errBuf.String(), "Not a git repository"); got != 1 { + t.Fatalf("not-git message count = %d, want 1; stderr:\n%s", got, errBuf.String()) } } @@ -138,7 +129,7 @@ func TestRunReview_MissingHooksAborts(t *testing.T) { setupCmdTestRepo(t) // Save config but don't install hooks. - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "claude-code": {Skills: []string{testReviewSkill}}, }); err != nil { t.Fatal(err) @@ -182,7 +173,7 @@ func TestRunReview_NonLaunchableAgentPreservesMarker(t *testing.T) { // Use prompt-only config: cursor has no curated built-ins, so a Skills // value would trip the installed-skill guard before reaching this path. - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ nonLaunchableAgent: {Prompt: "review the diff"}, }); err != nil { t.Fatal(err) @@ -219,7 +210,7 @@ func TestRunReview_MissingConfiguredSkillAbortsBeforeMarker(t *testing.T) { setupCmdTestRepo(t) installHooksForCmdTest(t, "claude-code") - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "claude-code": {Skills: []string{"/bogus:skill-does-not-exist"}}, }); err != nil { t.Fatal(err) @@ -251,7 +242,7 @@ func TestRunReview_PromptOnlyConfigSkipsVerification(t *testing.T) { setupCmdTestRepo(t) installHooksForCmdTest(t, "cursor") - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "cursor": {Prompt: "review the diff"}, }); err != nil { t.Fatal(err) @@ -280,7 +271,7 @@ func TestRunReview_FlagOverrideSkipsPicker(t *testing.T) { installHooksForCmdTest(t, "cursor") installHooksForCmdTest(t, "opencode") - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "cursor": {Prompt: "review the diff"}, "opencode": {Prompt: "review the diff"}, }); err != nil { @@ -311,7 +302,7 @@ func TestRunReview_FlagOverrideMustBeEligibleAgent(t *testing.T) { installHooksForCmdTest(t, "cursor") // opencode has no hooks installed - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "cursor": {Prompt: "review the diff"}, "opencode": {Prompt: "review the diff"}, }); err != nil { @@ -343,8 +334,6 @@ func newDispatchTestDeps( t *testing.T, installed []types.AgentName, launchableAgents []string, - multiPickerFn func(ctx context.Context, eligible []review.AgentChoice) (review.PickedAgents, error), - promptForAgentFn func(ctx context.Context, eligible []review.AgentChoice) (string, error), ) review.Deps { t.Helper() launchableSet := make(map[string]struct{}, len(launchableAgents)) @@ -355,9 +344,7 @@ func newDispatchTestDeps( 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 }, @@ -414,7 +401,7 @@ func (r *captureRunConfigReviewer) Start(_ context.Context, cfg reviewtypes.RunC func TestRunReview_ConfigPromptAugmentsSelectedSkills(t *testing.T) { setupCmdTestRepo(t) - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "claude-code": { Skills: []string{"/review"}, Prompt: "Focus on auth regressions.", @@ -463,55 +450,128 @@ 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). -func TestDispatchFork_TwoLaunchableNoOverride(t *testing.T) { - setupCmdTestRepo(t) - - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "agent-a": {Prompt: "review"}, - "agent-b": {Prompt: "review"}, - }); err != nil { - t.Fatal(err) - } - - multiPickerCalled := false - multiPickerFn := func(_ context.Context, eligible []review.AgentChoice) (review.PickedAgents, error) { - multiPickerCalled = true - names := make([]string, 0, len(eligible)) - for _, e := range eligible { - names = append(names, e.Name) - } - return review.PickedAgents{Names: names, PerRun: ""}, nil - } - - installed := []types.AgentName{"agent-a", "agent-b"} - deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) - - buf := &bytes.Buffer{} - cmd := review.NewCommand(deps) - cmd.SetOut(buf) - cmd.SetErr(&bytes.Buffer{}) - cmd.SetArgs([]string{}) - - if err := cmd.Execute(); err != nil { - t.Fatalf("unexpected error: %v", err) +// TestRunReview_IncludingContextBanner_SingleAgent verifies the +// transparency banner: the single-agent path always prints a line +// describing the session/checkpoint-context state directly below the scope +// banner. The line includes ACTUAL counts (number of checkpoints + number +// of sessions) when context is present; the empty variant is preserved for +// no-history branches. The line is never omitted — it surfaces what +// `entire review` does for the user vs. running a review skill manually. +func TestRunReview_IncludingContextBanner_SingleAgent(t *testing.T) { + const emptyLine = "No prior session or checkpoint context for this branch yet." + tests := []struct { + name string + result review.ContextResult + wantLine string + }{ + { + name: "itemised checkpoint bullet", + result: review.ContextResult{ + Prompt: "ctx", Checkpoints: 2, Sessions: 1, + CheckpointItems: []review.CheckpointScopeItem{ + {ID: "a1b2c3d4", Summary: "feat: add thing"}, + {ID: "b2c3d4e5", Summary: "fix: the bug"}, + }, + SessionItems: []review.SessionScopeItem{{ID: "ac3d5c6e", Agent: "Claude Code"}}, + }, + wantLine: " • a1b2c3d4 feat: add thing", + }, + { + name: "checkpoint header shows count", + result: review.ContextResult{ + Prompt: "ctx", Checkpoints: 2, + CheckpointItems: []review.CheckpointScopeItem{ + {ID: "a1b2c3d4", Summary: "feat: add thing"}, + {ID: "b2c3d4e5", Summary: "fix: the bug"}, + }, + }, + wantLine: "Checkpoints in scope (2):", + }, + { + name: "session header and bullet", + result: review.ContextResult{ + Prompt: "ctx", Sessions: 1, + SessionItems: []review.SessionScopeItem{{ID: "3d4c9f88", Agent: "Codex"}}, + }, + wantLine: "In-progress sessions (1):", + }, + { + name: "empty context advertised as absent", + result: review.ContextResult{}, + wantLine: emptyLine, + }, } - if !multiPickerCalled { - t.Error("expected multi-picker to be invoked for 2 launchable agents with no --agent override") + for _, tc := range tests { + // Cannot t.Parallel — t.Chdir in setupCmdTestRepo. + t.Run(tc.name, func(t *testing.T) { + setupCmdTestRepo(t) + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + testAgentName: {Skills: []string{"/review"}}, + }); err != nil { + t.Fatal(err) + } + reviewer := &captureRunConfigReviewer{name: testAgentName} + deps := review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { + return []types.AgentName{testAgentName} + }, + NewSilentError: func(err error) error { return err }, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { return false, "" }, + ReviewCheckpointContext: func(_ context.Context, _ string, _ string) review.ContextResult { + return tc.result + }, + ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { + if agentName == testAgentName { + return reviewer + } + return nil + }, + } + out := &bytes.Buffer{} + cmd := review.NewCommand(deps) + cmd.SetOut(out) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{}) + if err := cmd.Execute(); err != nil { + t.Fatalf("execute: %v", err) + } + if !strings.Contains(out.String(), tc.wantLine) { + t.Fatalf("expected output to contain %q; output:\n%s", tc.wantLine, out.String()) + } + // The line must appear AFTER the scope banner and BEFORE any + // agent output. We don't have a strict agent-table marker on + // the non-TTY path, but we can at least require the scope + // banner to come first. + bannerIdx := strings.Index(out.String(), "Reviewing ") + ctxIdx := strings.Index(out.String(), tc.wantLine) + if bannerIdx == -1 || ctxIdx == -1 || ctxIdx < bannerIdx { + t.Errorf("transparency line must follow the scope banner; output:\n%s", out.String()) + } + // Sanity: the RunConfig threaded to the reviewer carries the + // composed prompt — the banner change only affects printing, + // it must not change the data plumbing. + if reviewer.got.CheckpointContext != tc.result.Prompt { + t.Errorf("CheckpointContext on RunConfig = %q, want %q", + reviewer.got.CheckpointContext, tc.result.Prompt) + } + }) } } -func TestDispatchFork_MultiAgentPassesPerAgentConfigs(t *testing.T) { +// TestRunReview_RolesDispatchToMultiAgent verifies that when 2+ reviewers are +// configured (post-Chunk 2 role model), the multi-agent path is taken +// directly — no spawn-time picker is shown. +func TestRunReview_RolesDispatchToMultiAgent(t *testing.T) { setupCmdTestRepo(t) - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "claude-code": { + Role: settings.RoleReviewer, Skills: []string{"/review"}, Prompt: "Claude saved prompt.", }, testCodexAgent: { + Role: settings.RoleBoth, Skills: []string{"/review"}, Prompt: "Codex saved prompt.", }, @@ -521,19 +581,12 @@ 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, "" }, @@ -575,167 +628,161 @@ func TestDispatchFork_MultiAgentPassesPerAgentConfigs(t *testing.T) { if tc.reviewer.got.AlwaysPrompt != tc.wantPrompt { t.Fatalf("%s AlwaysPrompt = %q, want %q", tc.name, tc.reviewer.got.AlwaysPrompt, tc.wantPrompt) } - if tc.reviewer.got.PerRunPrompt != "Focus this run on regressions." { - t.Fatalf("%s PerRunPrompt = %q", tc.name, tc.reviewer.got.PerRunPrompt) - } if tc.reviewer.got.StartingSHA == "" { t.Fatalf("%s StartingSHA is empty", tc.name) } } } -// TestDispatchFork_OneLaunchableOneNonLaunchableNoOverride verifies that when -// only 1 agent is launchable (the other is non-launchable), the single-agent -// path is taken (no multi-picker). Uses cursor (real non-launchable agent with -// hooks) + agent-a (fake launchable stub). -func TestDispatchFork_OneLaunchableOneNonLaunchableNoOverride(t *testing.T) { +// TestRunReview_RecursionGuard verifies that ENTIRE_REVIEW_SESSION in env +// refuses a nested review (prevents fan-out loops). +func TestRunReview_RecursionGuard(t *testing.T) { setupCmdTestRepo(t) - installHooksForCmdTest(t, "cursor") - - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "cursor": {Prompt: "review"}, - "agent-a": {Prompt: "review"}, - }); err != nil { - t.Fatal(err) - } + t.Setenv("ENTIRE_REVIEW_SESSION", "outer-session-id") - multiPickerCalled := false - multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { - multiPickerCalled = true - return review.PickedAgents{}, errors.New("should not be called") - } - // Stub single-select picker to avoid TTY: always picks cursor. - singlePickerFn := func(_ context.Context, _ []review.AgentChoice) (string, error) { - return "cursor", nil - } - - installed := []types.AgentName{"cursor", "agent-a"} - // Only agent-a is launchable. With 1 launchable agent, computeLaunchableEligible - // returns 1 entry, so multi-path is skipped. The single-select picker picks cursor. - // ReviewerFor("cursor") returns nil → marker fallback path (writes marker file). - deps := newDispatchTestDeps(t, installed, []string{"agent-a"}, multiPickerFn, singlePickerFn) + installed := []types.AgentName{"claude-code"} + deps := newDispatchTestDeps(t, installed, []string{"claude-code"}) + errBuf := &bytes.Buffer{} cmd := review.NewCommand(deps) cmd.SetOut(&bytes.Buffer{}) - cmd.SetErr(&bytes.Buffer{}) + cmd.SetErr(errBuf) cmd.SetArgs([]string{}) - executeErr := cmd.Execute() // may error (agent-a not a real agent); we only care about picker routing - _ = executeErr // intentionally ignored: this test only asserts picker routing - if multiPickerCalled { - t.Error("multi-picker should NOT be invoked when only 1 launchable agent is configured") + if err := cmd.Execute(); err == nil { + t.Fatal("expected refusal, got nil") + } + if !strings.Contains(errBuf.String(), "Already in a review session") { + t.Errorf("expected recursion-guard message, got:\n%s", errBuf.String()) } } -// TestDispatchFork_TwoLaunchableWithAgentOverride verifies that --agent flag -// bypasses the multi-picker even when 2+ launchable agents are configured. -// The test uses cursor (non-launchable, real agent) + agent-a (fake launchable) -// with --agent cursor so the single-agent path runs to completion via marker -// fallback (cursor is non-launchable in reviewerFor, so nil → marker fallback). -func TestDispatchFork_TwoLaunchableWithAgentOverride(t *testing.T) { +// TestRunReview_ReviewersFlagSingleAgent verifies that --reviewers +// runs review with that agent only and ignores saved config. +func TestRunReview_ReviewersFlagSingleAgent(t *testing.T) { setupCmdTestRepo(t) - installHooksForCmdTest(t, "cursor") // cursor needs real hooks - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "cursor": {Prompt: "review"}, - "agent-a": {Prompt: "review"}, + // Save a different agent in config — should be ignored. + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + "codex": {Role: settings.RoleReviewer, Skills: []string{"/review"}}, }); err != nil { t.Fatal(err) } - multiPickerCalled := false - multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { - multiPickerCalled = true - return review.PickedAgents{}, errors.New("should not be called") + claudeReviewer := &captureRunConfigReviewer{name: testAgentName} + deps := review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { + return []types.AgentName{testAgentName, "codex"} + }, + NewSilentError: func(err error) error { return err }, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { return false, "" }, + ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { + if agentName == testAgentName { + return claudeReviewer + } + return nil + }, } - - // cursor + agent-a both installed; agent-a is launchable but cursor is not. - // With 1 launchable agent (agent-a) among the 2 eligible agents, the - // multi-agent path would NOT fire (needs 2+ launchable). But when we - // additionally pass --agent cursor, the multi-picker is bypassed by the - // agentOverride check at the top of step 3. - installed := []types.AgentName{"cursor", "agent-a"} - deps := newDispatchTestDeps(t, installed, []string{"agent-a"}, multiPickerFn, nil) - - buf := &bytes.Buffer{} cmd := review.NewCommand(deps) - cmd.SetOut(buf) + cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) - cmd.SetArgs([]string{"--agent", "cursor"}) - - // cursor is not launchable in our stub (reviewerFor returns nil), so it - // falls through to RunMarkerFallback. That's fine — we only care that - // multiPickerCalled is false. - executeErr := cmd.Execute() - _ = executeErr // intentionally ignored: this test only asserts picker routing - if multiPickerCalled { - t.Error("multi-picker should NOT be invoked when --agent override is set") + cmd.SetArgs([]string{"--reviewers", "claude-code"}) + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !claudeReviewer.called { + t.Fatal("claude-code reviewer should have been started") } } -// TestDispatchFork_MultiPickerCancellationExitsCleanly verifies that when -// the multi-picker is cancelled (ErrPickerCancelled), the command exits with -// nil error (no user-facing error). -func TestDispatchFork_MultiPickerCancellationExitsCleanly(t *testing.T) { +// TestRunReview_ReviewersFlagUnknownAgentErrors verifies that --reviewers +// with an uninstalled agent name produces a clear error. +func TestRunReview_ReviewersFlagUnknownAgentErrors(t *testing.T) { setupCmdTestRepo(t) - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "agent-a": {Prompt: "review"}, - "agent-b": {Prompt: "review"}, - }); err != nil { - t.Fatal(err) - } + deps := newDispatchTestDeps(t, []types.AgentName{"claude-code"}, []string{"claude-code"}) - multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { - return review.PickedAgents{}, review.ErrPickerCancelled + errBuf := &bytes.Buffer{} + cmd := review.NewCommand(deps) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(errBuf) + cmd.SetArgs([]string{"--reviewers", "no-such-agent"}) + + if err := cmd.Execute(); err == nil { + t.Fatal("expected error for unknown agent") } + if !strings.Contains(errBuf.String(), "no-such-agent") { + t.Errorf("error should name the unknown agent, got: %q", errBuf.String()) + } +} - installed := []types.AgentName{"agent-a", "agent-b"} - deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) +// TestRunReview_FixerOnlyFlagErrorsAtReviewLayer verifies that +// `entire review --fixer X` (without --reviewers) errors at the +// review-command layer with a helpful message. The check lives in +// runReview rather than resolveRolesFromFlags so that +// `entire review fix --fixer X` (which only needs a Fixer) is not +// rejected by the shared flag-resolution path. +func TestRunReview_FixerOnlyFlagErrorsAtReviewLayer(t *testing.T) { + setupCmdTestRepo(t) + + deps := newDispatchTestDeps(t, + []types.AgentName{"claude-code", "codex"}, + []string{"claude-code", "codex"}, + ) errBuf := &bytes.Buffer{} cmd := review.NewCommand(deps) cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(errBuf) - cmd.SetArgs([]string{}) + cmd.SetArgs([]string{"--fixer", "codex"}) err := cmd.Execute() - if err != nil { - t.Errorf("ErrPickerCancelled should produce nil command error, got: %v", err) + if err == nil { + t.Fatal("expected error when --fixer is set without --reviewers, got nil") + } + stderr := errBuf.String() + if !strings.Contains(stderr, "needs at least one") { + t.Errorf("expected error to mention needing at least one reviewer, got: %s", stderr) + } + if !strings.Contains(stderr, "--reviewers") { + t.Errorf("expected error to mention --reviewers as the fix, got: %s", stderr) + } + if !strings.Contains(stderr, "entire review fix") { + t.Errorf("expected error to mention `entire review fix` as the alternative, got: %s", stderr) } } -// TestDispatchFork_MultiPickerNoSelectionSurfacesError verifies that when the -// multi-picker returns ErrNoAgentsSelected, a clear error is shown to the user. -func TestDispatchFork_MultiPickerNoSelectionSurfacesError(t *testing.T) { +// TestReviewFixCmd_FixerOnlyFlagAccepted verifies that +// `entire review fix --fixer codex` is NOT rejected by the shared +// flag-resolution path (the scope-bug fix). It may still fail later for +// unrelated reasons in this test repo (no findings, no review session), +// but the "needs at least one reviewer" check must NOT fire. +func TestReviewFixCmd_FixerOnlyFlagAccepted(t *testing.T) { setupCmdTestRepo(t) - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "agent-a": {Prompt: "review"}, - "agent-b": {Prompt: "review"}, - }); err != nil { - t.Fatal(err) - } - - multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { - return review.PickedAgents{}, review.ErrNoAgentsSelected - } - - installed := []types.AgentName{"agent-a", "agent-b"} - deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) + deps := newDispatchTestDeps(t, + []types.AgentName{"claude-code", "codex"}, + []string{"claude-code", "codex"}, + ) errBuf := &bytes.Buffer{} cmd := review.NewCommand(deps) cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(errBuf) - cmd.SetArgs([]string{}) + cmd.SetArgs([]string{"fix", "--fixer", "codex"}) - err := cmd.Execute() - if err == nil { - t.Fatal("expected non-nil error when no agents are selected") + // We don't care whether the overall command succeeds — only that the + // failure isn't the "needs at least one reviewer" check from + // resolveRolesFromFlags. + if err := cmd.Execute(); err != nil { + t.Logf("entire review fix exited with error (expected — test repo has no findings): %v", err) + } + stderr := errBuf.String() + if strings.Contains(stderr, "needs at least one") { + t.Errorf("entire review fix --fixer X should not be rejected by the reviewer-required check; got: %s", stderr) } - if !strings.Contains(errBuf.String(), "no agents selected") { - t.Errorf("stderr should mention 'no agents selected', got: %q", errBuf.String()) + if strings.Contains(stderr, "Reviewer or Both") { + t.Errorf("entire review fix --fixer X should not be rejected by the Reviewer-or-Both check; got: %s", stderr) } } @@ -987,23 +1034,15 @@ func TestFindTUISink_NoTUIInSlice(t *testing.T) { func TestDispatchFork_SynthesisSinkNilProviderNoComposition(t *testing.T) { setupCmdTestRepo(t) - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "agent-a": {Prompt: "review"}, - "agent-b": {Prompt: "review"}, + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + "agent-a": {Role: settings.RoleReviewer, Prompt: "review"}, + "agent-b": {Role: settings.RoleReviewer, Prompt: "review"}, }); err != nil { t.Fatal(err) } - multiPickerFn := func(_ context.Context, eligible []review.AgentChoice) (review.PickedAgents, error) { - names := make([]string, 0, len(eligible)) - for _, e := range eligible { - names = append(names, e.Name) - } - return review.PickedAgents{Names: names, PerRun: ""}, nil - } - installed := []types.AgentName{"agent-a", "agent-b"} - deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}, multiPickerFn, nil) + deps := newDispatchTestDeps(t, installed, []string{"agent-a", "agent-b"}) deps.SynthesisProvider = nil // explicitly nil — synthesis unavailable buf := &bytes.Buffer{} @@ -1029,7 +1068,7 @@ func TestDispatchFork_SingleAgentNoSynthesis(t *testing.T) { setupCmdTestRepo(t) installHooksForCmdTest(t, "cursor") - if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ "cursor": {Prompt: "review"}, }); err != nil { t.Fatal(err) @@ -1039,7 +1078,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 := newDispatchTestDeps(t, installed, nil /* no launchable */) deps.SynthesisProvider = provider buf := &bytes.Buffer{} @@ -1055,3 +1094,47 @@ func TestDispatchFork_SingleAgentNoSynthesis(t *testing.T) { t.Error("synthesis provider should NOT be called on single-agent path") } } + +func TestRunConfigWithReviewConfig_MapsModelAndReasoningEffort(t *testing.T) { + t.Parallel() + + // Skills branch: per-spawn knobs map through alongside Skills/AlwaysPrompt. + got := review.ExposedRunConfigWithReviewConfig(reviewtypes.RunConfig{}, settings.ReviewConfig{ + Role: settings.RoleReviewer, + Skills: []string{"/review"}, + Prompt: "Be thorough.", + Model: "gpt-5-mini", + ReasoningEffort: "low", + }) + if got.Model != "gpt-5-mini" { + t.Errorf("Model = %q, want %q", got.Model, "gpt-5-mini") + } + if got.ReasoningEffort != "low" { + t.Errorf("ReasoningEffort = %q, want %q", got.ReasoningEffort, "low") + } + if got.AlwaysPrompt != "Be thorough." { + t.Errorf("AlwaysPrompt = %q, want %q", got.AlwaysPrompt, "Be thorough.") + } + + // Prompt-only branch (no skills): knobs still map; Prompt becomes the + // verbatim override. + got = review.ExposedRunConfigWithReviewConfig(reviewtypes.RunConfig{}, settings.ReviewConfig{ + Role: settings.RoleReviewer, + Prompt: "Just look at the diff.", + ReasoningEffort: "medium", + }) + if got.ReasoningEffort != "medium" { + t.Errorf("prompt-only: ReasoningEffort = %q, want %q", got.ReasoningEffort, "medium") + } + if got.PromptOverride != "Just look at the diff." { + t.Errorf("prompt-only: PromptOverride = %q, want %q", got.PromptOverride, "Just look at the diff.") + } + + // Empty knobs stay empty (no accidental defaulting). + got = review.ExposedRunConfigWithReviewConfig(reviewtypes.RunConfig{}, settings.ReviewConfig{ + Skills: []string{"/review"}, + }) + if got.Model != "" || got.ReasoningEffort != "" { + t.Errorf("empty knobs leaked: Model=%q ReasoningEffort=%q", got.Model, got.ReasoningEffort) + } +} diff --git a/cmd/entire/cli/review/dump.go b/cmd/entire/cli/review/dump.go index 49e26cdd2..bfe039e56 100644 --- a/cmd/entire/cli/review/dump.go +++ b/cmd/entire/cli/review/dump.go @@ -89,10 +89,12 @@ func (s DumpSink) dumpAgent(run reviewtypes.AgentRun) { } } - // RenderForWriter is TTY-aware: returns raw markdown for non-TTY writers, - // glamour-styled output otherwise. Errors are best-effort — fall back to - // raw markdown so the user always gets the content. - rendered, err := mdrender.RenderForWriter(s.W, b.String()) + // RenderMutedForWriter is TTY-aware: raw markdown for non-TTY writers, + // low-chroma glamour output otherwise. The muted palette keeps review + // findings (markdown-dense: headings, bold severities, code paths, links) + // readable instead of a wall of colour. Errors are best-effort — fall back + // to raw markdown so the user always gets the content. + rendered, err := mdrender.RenderMutedForWriter(s.W, b.String()) if err != nil { rendered = b.String() } diff --git a/cmd/entire/cli/review/export_test.go b/cmd/entire/cli/review/export_test.go index 42926a160..b5b0ed2e7 100644 --- a/cmd/entire/cli/review/export_test.go +++ b/cmd/entire/cli/review/export_test.go @@ -4,8 +4,6 @@ import ( "context" "io" - "charm.land/huh/v2" - reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) @@ -14,6 +12,11 @@ import ( // Only compiled during `go test`. var ExposedComposeSynthesisPrompt = composeSynthesisPrompt +// ExposedRunConfigWithReviewConfig exposes runConfigWithReviewConfig so +// external tests can assert the settings.ReviewConfig → RunConfig mapping +// (skills/prompt branch plus the Model/ReasoningEffort per-spawn knobs). +var ExposedRunConfigWithReviewConfig = runConfigWithReviewConfig + // SinkComposeInputs is the test-facing alias for multiAgentSinkInputs. // It lets external tests drive composeMultiAgentSinks with explicit isTTY // and canPrompt values without depending on real TTY detection. @@ -61,10 +64,6 @@ func ExposedComposeSingleAgentSinks(in SingleAgentSinkComposeInputs) []reviewtyp }) } -func ExposedBuildAgentMultiSelect(options []huh.Option[string], picked *[]string) *huh.MultiSelect[string] { - return buildAgentMultiSelect(options, picked) -} - // ExposedFindTUISink exposes findTUISink for tests. func ExposedFindTUISink(sinks []reviewtypes.Sink) (*TUISink, bool) { return findTUISink(sinks) diff --git a/cmd/entire/cli/review/fix.go b/cmd/entire/cli/review/fix.go index 8ce584e21..1e5a513fa 100644 --- a/cmd/entire/cli/review/fix.go +++ b/cmd/entire/cli/review/fix.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "os" "slices" "strconv" "strings" @@ -12,8 +13,7 @@ import ( "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/agent/external" "github.com/entireio/cli/cmd/entire/cli/agentlaunch" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/mdrender" @@ -77,8 +77,16 @@ func runReviewFix( target string, all bool, agentOverride string, + perRunPrompt string, silentErr func(error) error, ) error { + if os.Getenv(EnvSession) != "" { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), "Already in a review session — refusing to start a nested review.") + fmt.Fprintln(cmd.ErrOrStderr(), "If you reached this from a reviewer agent's prompt, this is likely a loop and you should exit the inner agent.") + return wrapReviewSilentError(silentErr, errors.New("nested review session refused")) + } + worktreeRoot, err := paths.WorktreeRoot(ctx) if err != nil { cmd.SilenceUsage = true @@ -90,11 +98,18 @@ func runReviewFix( if err != nil { return err } - sources, err := selectReviewFixSources(ctx, cmd, manifest, all) - if err != nil { - return err + // Interactive multi-source: one navigable form (source step ⇄ findings + // step) so the user can go back and change the source — e.g. to the + // aggregate — without restarting. Other cases keep the linear path. + var sources []reviewFixSource + var findings []reviewFinding + if !all && shouldUseInteractiveFixPicker(cmd, manifest) { + sources, findings, err = selectReviewFixInteractive(ctx, manifest) + } else { + if sources, err = selectReviewFixSources(ctx, cmd, manifest, all); err == nil { + findings, err = selectReviewFindings(ctx, cmd, sources, all) + } } - findings, err := selectReviewFindings(ctx, cmd, sources, all) if err != nil { return err } @@ -104,6 +119,9 @@ func runReviewFix( return err } prompt := composeReviewFixPrompt(manifest, reviewFixSourcesFromFindings(findings)) + if perRunPrompt != "" { + prompt += "\n\nAdditional context for this run:\n" + perRunPrompt + } if err := agentlaunch.LaunchFixAgent(ctx, fixAgent, prompt); err != nil { return fmt.Errorf("launch review fix agent: %w", err) } @@ -244,6 +262,101 @@ func selectReviewFindings(ctx context.Context, cmd *cobra.Command, sources []rev return selected, nil } +// shouldUseInteractiveFixPicker reports whether the unified, navigable +// source+findings picker applies: an interactive terminal with more than one +// fix source (the only case where the back-and-forth between the source and +// findings steps is useful). +func shouldUseInteractiveFixPicker(cmd *cobra.Command, manifest LocalReviewManifest) bool { + if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() { + return false + } + return len(reviewFixSourcesForManifest(manifest)) > 1 +} + +// selectReviewFixInteractive presents source selection and findings selection +// as a single huh form with two groups, so the user can move back to the +// source step and change their mind — e.g. switch to the Aggregate source — +// without restarting the command (mirrors the config wizard's navigation). +// +// The findings group's options are recomputed (OptionsFunc) from whichever +// sources are currently selected, bound to the source selection so going back +// and changing it refreshes the findings list. Returns the selected sources +// (for fix-agent resolution) and the selected findings. +func selectReviewFixInteractive(ctx context.Context, manifest LocalReviewManifest) ([]reviewFixSource, []reviewFinding, error) { + sources := reviewFixSourcesForManifest(manifest) + sourceValues := make([]string, len(sources)) + sourceOptions := make([]huh.Option[string], len(sources)) + for i, source := range sources { + value := strconv.Itoa(i) + sourceValues[i] = value + sourceOptions[i] = huh.NewOption(source.Label, value) + } + pickedSources := defaultReviewFixSourceSelection(sources) + var pickedFindings []string + + selectedSources := func() []reviewFixSource { + out := make([]reviewFixSource, 0, len(pickedSources)) + for _, value := range pickedSources { + if idx := slices.Index(sourceValues, value); idx >= 0 { + out = append(out, sources[idx]) + } + } + return out + } + findingsOptions := func() []huh.Option[string] { + findings := extractReviewFindings(selectedSources()) + opts := make([]huh.Option[string], len(findings)) + for i, finding := range findings { + opts[i] = huh.NewOption(finding.Title, finding.ID) + } + return opts + } + + form := newAccessibleForm( + huh.NewGroup( + huh.NewMultiSelect[string](). + Title(reviewFixSourcePickerTitle(manifest)). + Description("space toggle · ctrl+a all · enter next"). + Options(sourceOptions...). + Height(reviewPickerHeight(len(sourceOptions))). + Value(&pickedSources). + Validate(func(value []string) error { + if len(value) == 0 { + return errors.New("select at least one source") + } + return nil + }), + ), + huh.NewGroup( + huh.NewMultiSelect[string](). + Title("Select findings to fix"). + Description("space toggle · ctrl+a all · shift+tab back to sources · enter fix"). + OptionsFunc(findingsOptions, &pickedSources). + Height(reviewPickerHeight(len(extractReviewFindings(sources)))). + Value(&pickedFindings). + Validate(func(value []string) error { + if len(value) == 0 { + return errors.New("select at least one finding (or shift+tab to change sources)") + } + return nil + }), + ), + ) + if err := form.RunWithContext(ctx); err != nil { + return nil, nil, fmt.Errorf("review fix picker: %w", err) + } + + chosenSources := selectedSources() + allFindings := extractReviewFindings(chosenSources) + selected := make([]reviewFinding, 0, len(pickedFindings)) + for _, finding := range allFindings { + if slices.Contains(pickedFindings, finding.ID) { + selected = append(selected, finding) + } + } + return chosenSources, selected, nil +} + func composeReviewFixPrompt(manifest LocalReviewManifest, sources []reviewFixSource) string { var b strings.Builder b.WriteString("Fix only the selected review findings.\n") @@ -400,14 +513,26 @@ func reviewFindingTitle(line string) (string, bool) { 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+".") { + for _, sev := range []string{"blocker", "critical", "high", "medium", "low"} { + if !strings.HasPrefix(lower, sev) { + continue + } + // Require a non-letter boundary after the severity word so we match the + // real formats agents emit — "Medium:", "High -", "Critical.", and + // codex's bolded "**Medium** [file]: …" (which arrives here as + // "Medium** [file]…" after the leading markdown is trimmed) — without + // matching ordinary words like "lower" or "highlight". + if rest := lower[len(sev):]; rest == "" || !isASCIILetter(rest[0]) { return line, true } } return "", false } +func isASCIILetter(b byte) bool { + return (b >= 'a' && b <= 'z') || (b >= 'A' && b <= 'Z') +} + func isSeverityNumberedTitle(line string) bool { if len(line) < 3 { return false @@ -446,160 +571,40 @@ func selectedSourcesOutput(sources []reviewFixSource) string { return strings.TrimSpace(b.String()) } -func resolveReviewFixAgent(ctx context.Context, cmd *cobra.Command, sources []reviewFixSource, agentOverride string) (string, error) { +func resolveReviewFixAgent(ctx context.Context, _ *cobra.Command, _ []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 + fixer := FixerOf(s) + if fixer == "" { + return "", errors.New("no Fixer configured — run: entire review setup") } - 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 + return fixer, nil } func writeReviewCompletionFooter(w io.Writer, manifest LocalReviewManifest) { - handle := reviewManifestHandle(manifest) - if handle == "" { + // Skip only when the manifest is truly empty (review ran but nothing was + // persisted). A codex-only run has sources with output but no SessionID + // (codex exec fires no hook to tag a session) — it must still get the + // footer. `entire review fix` with no handle resolves to the most-recent + // manifest, so the handle is optional. + if len(manifest.Sources) == 0 && strings.TrimSpace(manifest.AggregateOutput) == "" { return } + handle := reviewManifestHandle(manifest) + arg := "" + if handle != "" { + arg = " " + handle + } fmt.Fprintln(w) fmt.Fprintln(w, "Review complete.") 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.Fprintf(w, "Run: %s review fix%s --all apply all findings\n", reviewCommandBinary, arg) + fmt.Fprintf(w, " %s review fix%s choose findings interactively\n", reviewCommandBinary, arg) } func reviewManifestHandle(manifest LocalReviewManifest) string { @@ -617,8 +622,8 @@ func printReviewFindingsList(w io.Writer, manifests []LocalReviewManifest) { 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)) + 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)) } } @@ -666,6 +671,76 @@ func reviewManifestListLabel(manifest LocalReviewManifest) string { return fmt.Sprintf("%s · local · %s", handle, strings.Join(agents, ", ")) } +// newReviewFixCmd returns the `entire review fix` cobra subcommand. +// +// It is the canonical entry point for applying review findings; the +// legacy `entire review --fix` flag still works but emits a deprecation +// hint via the parent `review` command's RunE. +func newReviewFixCmd(deps Deps) *cobra.Command { + var ( + all bool + agentOverride string + perRunPrompt string + reviewersFlag []string + fixerFlag string + ) + cmd := &cobra.Command{ + Use: "fix [session-id]", + Short: "Apply review findings via the configured Fixer agent", + Long: `Apply review findings via the configured Fixer agent. + +With no positional argument, the most recent review run is selected +(or the user is prompted to pick when multiple exist). + +Flags: + --all apply all findings/sources without selectors + --agent NAME override the configured Fixer for this run + --prompt TEXT per-run extra context appended to the fix prompt + --reviewers LIST one-off override: agents to use as reviewers + --fixer NAME one-off override: agent to use as fixer`, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + external.DiscoverAndRegister(ctx) + target := "" + if len(args) == 1 { + target = args[0] + } + // One-off flag overrides apply by writing to settings in-memory + // before runReviewFix loads them via settings.Load — we capture + // the override here and re-resolve fix agent. + installed := deps.GetAgentsWithHooksInstalled(ctx) + override, err := resolveRolesFromFlags(reviewersFlag, fixerFlag, installed) + if err != nil { + cmd.SilenceUsage = true + fmt.Fprintln(cmd.ErrOrStderr(), err.Error()) + return wrapReviewSilentError(deps.NewSilentError, err) + } + if override != nil { + // --fixer wins for the fix subcommand. Prefer the + // explicit --fixer name; otherwise use FixerOf on the + // override. + fix := strings.TrimSpace(fixerFlag) + if fix != "" { + agentOverride = fix + } else { + tmp := &settings.EntireSettings{Review: override} + if f := FixerOf(tmp); f != "" { + agentOverride = f + } + } + } + return runReviewFix(ctx, cmd, target, all, agentOverride, perRunPrompt, deps.NewSilentError) + }, + } + cmd.Flags().BoolVar(&all, "all", false, "apply all findings/sources without selectors") + cmd.Flags().StringVar(&agentOverride, "agent", "", "override the configured Fixer for this run") + cmd.Flags().StringVar(&perRunPrompt, "prompt", "", "per-run extra context appended to the fix prompt") + cmd.Flags().StringSliceVar(&reviewersFlag, "reviewers", nil, "one-off override: agents to use as reviewers (comma-separated or repeatable)") + cmd.Flags().StringVar(&fixerFlag, "fixer", "", "one-off override: agent to use as fixer") + return cmd +} + func reviewManifestPreview(manifest LocalReviewManifest) string { for _, source := range manifest.Sources { if text := strings.TrimSpace(source.Output); text != "" { diff --git a/cmd/entire/cli/review/fix_findings_test.go b/cmd/entire/cli/review/fix_findings_test.go new file mode 100644 index 000000000..d96d5e6ca --- /dev/null +++ b/cmd/entire/cli/review/fix_findings_test.go @@ -0,0 +1,69 @@ +package review + +import ( + "strings" + "testing" +) + +func TestReviewFindingTitle_RecognisesRealSeverityFormats(t *testing.T) { + t.Parallel() + + matches := []string{ + "- **Medium** [cmd.go](/x/cmd.go:1): desc", // codex bolded-severity bullet + "**Low** [f](/x): x", + "Medium: something is wrong", + "High - something", + "Critical. something", + "Blocker: data loss", + "H1. severity-numbered title", + } + for _, line := range matches { + if _, ok := reviewFindingTitle(line); !ok { + t.Errorf("expected a finding title for %q", line) + } + } + + nonMatches := []string{ + "Lower the timeout to 5s", // "low" is a prefix of a real word + "highlight the regression", + "**Findings**", + "No tests run; review only.", + "- just a bullet of prose with no severity", + "", + } + for _, line := range nonMatches { + if _, ok := reviewFindingTitle(line); ok { + t.Errorf("did not expect a finding title for %q", line) + } + } +} + +// TestExtractSourceFindings_CodexBulletFormat pins parsing against a real codex +// `$code-reviewer` output (bolded-severity bullets) — the format that the +// previous matcher silently collapsed to a single full-output finding, breaking +// the [s] select-findings picker. +func TestExtractSourceFindings_CodexBulletFormat(t *testing.T) { + t.Parallel() + + output := strings.Join([]string{ + "**Findings**", + "- **Medium** [cmd.go](/x/cmd.go:571): runMultiAgentPath skips validation.", + "", + "- **Low** [post_review.go](/x/post_review.go:119): footer points to a wrong command.", + "", + "- **Low** [t1.txt](/x/t1.txt:1): stray test artifact, remove it.", + "", + "No tests run; review only.", + }, "\n") + + findings := extractSourceFindings(reviewFixSource{Kind: reviewFixSourceAgent, Label: "codex", Output: output}, 0) + if len(findings) != 3 { + t.Fatalf("findings = %d, want 3 (one per severity bullet):\n%+v", len(findings), findings) + } + // Each finding carries its own body so a [s] selection scopes the fix prompt. + for i, f := range findings { + if strings.TrimSpace(f.Body) == "" { + t.Errorf("finding %d has empty body", i) + } + } +} diff --git a/cmd/entire/cli/review/flags.go b/cmd/entire/cli/review/flags.go new file mode 100644 index 000000000..1e4401e7a --- /dev/null +++ b/cmd/entire/cli/review/flags.go @@ -0,0 +1,99 @@ +// Package review — see env.go for package-level rationale. +// +// flags.go implements the `--reviewers` / `--fixer` one-off override +// resolution used by `entire review` and `entire review fix`. These +// flags are how a calling agent translates user intent ("you and Claude +// review, codex fixes") into a concrete CLI invocation without editing +// settings. +package review + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/entireio/cli/cmd/entire/cli/agent/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// resolveRolesFromFlags builds an EntireSettings.Review map from the +// --reviewers and --fixer flags. Returns (nil, nil) if no flags are +// set (caller should fall through to saved config / fallback). +// +// Validates that every named agent is installed; returns a clear +// error otherwise. +// +// An agent appearing in BOTH lists gets Role Both. Agents in only +// --reviewers get Role Reviewer. The --fixer agent (if not also a +// reviewer) gets Role Fixer. +// +//nolint:nilnil // (nil, nil) is the documented "no flags set" sentinel; caller checks override != nil. +func resolveRolesFromFlags(reviewers []string, fixer string, installed []types.AgentName) (map[string]settings.ReviewConfig, error) { + if len(reviewers) == 0 && fixer == "" { + return nil, nil + } + installedSet := make(map[string]struct{}, len(installed)) + for _, a := range installed { + installedSet[string(a)] = struct{}{} + } + + out := make(map[string]settings.ReviewConfig) + reviewerSet := make(map[string]struct{}, len(reviewers)) + for _, name := range reviewers { + name = strings.TrimSpace(name) + if name == "" { + continue + } + if _, ok := installedSet[name]; !ok { + return nil, fmt.Errorf("--reviewers: agent not installed: %s", name) + } + reviewerSet[name] = struct{}{} + out[name] = settings.ReviewConfig{Role: settings.RoleReviewer} + } + fixer = strings.TrimSpace(fixer) + if fixer != "" { + if _, ok := installedSet[fixer]; !ok { + return nil, fmt.Errorf("--fixer: agent not installed: %s", fixer) + } + if _, alsoReviews := reviewerSet[fixer]; alsoReviews { + out[fixer] = settings.ReviewConfig{Role: settings.RoleBoth} + } else { + out[fixer] = settings.ReviewConfig{Role: settings.RoleFixer} + } + } + if len(out) == 0 { + return nil, errors.New("--reviewers / --fixer: at least one agent required") + } + return out, nil +} + +// mergeFlagOverrideWithSavedSkills layers per-agent Skills/Prompt from +// `saved` onto the role-only entries in `override`. Agents named in the +// flags but not present in saved config fall back to seedDefaultSkills +// (curated built-ins, or name-matched on-disk discovery for agents like +// codex that ship no curated built-ins) so the run still has a review +// skill to invoke rather than a generic scope-only prompt. +// +// Note: the resulting map only contains agents named in the flags; it +// is a one-off override and never mixes in agents that were not +// explicitly requested. +func mergeFlagOverrideWithSavedSkills(ctx context.Context, override, saved map[string]settings.ReviewConfig) map[string]settings.ReviewConfig { + out := make(map[string]settings.ReviewConfig, len(override)) + for name, cfg := range override { + merged := cfg + if prev, ok := saved[name]; ok { + if len(prev.Skills) > 0 { + merged.Skills = append([]string(nil), prev.Skills...) + } + if prev.Prompt != "" { + merged.Prompt = prev.Prompt + } + } + if len(merged.Skills) == 0 && merged.Prompt == "" { + merged.Skills = seedDefaultSkills(ctx, name) + } + out[name] = merged + } + return out +} diff --git a/cmd/entire/cli/review/flags_test.go b/cmd/entire/cli/review/flags_test.go new file mode 100644 index 000000000..f165f7693 --- /dev/null +++ b/cmd/entire/cli/review/flags_test.go @@ -0,0 +1,146 @@ +package review + +import ( + "context" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/agent/types" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +func TestResolveRolesFromFlags_NoFlagsReturnsNil(t *testing.T) { + t.Parallel() + got, err := resolveRolesFromFlags(nil, "", []types.AgentName{"claude-code"}) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != nil { + t.Errorf("expected nil when no flags set, got %v", got) + } +} + +func TestResolveRolesFromFlags_SingleReviewer(t *testing.T) { + t.Parallel() + got, err := resolveRolesFromFlags( + []string{"claude-code"}, "", + []types.AgentName{"claude-code", "codex"}, + ) + if err != nil { + t.Fatalf("err: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 entry, got %d", len(got)) + } + if got["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q, want %q", got["claude-code"].Role, settings.RoleReviewer) + } +} + +func TestResolveRolesFromFlags_MultipleReviewers(t *testing.T) { + t.Parallel() + got, err := resolveRolesFromFlags( + []string{"claude-code", "codex"}, "", + []types.AgentName{"claude-code", "codex"}, + ) + if err != nil { + t.Fatalf("err: %v", err) + } + if len(got) != 2 { + t.Fatalf("expected 2 entries, got %d: %+v", len(got), got) + } + if got["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q", got["claude-code"].Role) + } + if got["codex"].Role != settings.RoleReviewer { + t.Errorf("codex role = %q", got["codex"].Role) + } +} + +func TestResolveRolesFromFlags_AgentInBothListsGetsRoleBoth(t *testing.T) { + t.Parallel() + got, err := resolveRolesFromFlags( + []string{"codex"}, "codex", + []types.AgentName{"codex"}, + ) + if err != nil { + t.Fatalf("err: %v", err) + } + if got["codex"].Role != settings.RoleBoth { + t.Errorf("codex role = %q, want %q", got["codex"].Role, settings.RoleBoth) + } +} + +// TestResolveRolesFromFlags_FixerOnlyAssignsFixer documents that +// `--fixer X` without `--reviewers` is a legitimate combination at the +// flag-resolution layer: `entire review fix --fixer X` uses it to apply +// existing findings via a one-off fixer override. The "needs at least +// one reviewer" check is enforced higher up in `runReview` (cmd.go), +// not here, so this function can be shared with `entire review fix`. +func TestResolveRolesFromFlags_FixerOnlyAssignsFixer(t *testing.T) { + t.Parallel() + installed := []types.AgentName{"claude-code", "codex"} + got, err := resolveRolesFromFlags(nil, "codex", installed) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(got) != 1 { + t.Fatalf("expected 1 entry, got %d: %+v", len(got), got) + } + if got["codex"].Role != settings.RoleFixer { + t.Errorf("codex role = %q, want %q", got["codex"].Role, settings.RoleFixer) + } +} + +func TestResolveRolesFromFlags_UnknownReviewerErrors(t *testing.T) { + t.Parallel() + _, err := resolveRolesFromFlags( + []string{"no-such-agent"}, "", + []types.AgentName{"claude-code"}, + ) + if err == nil { + t.Fatal("expected error for unknown reviewer") + } + if !strings.Contains(err.Error(), "no-such-agent") { + t.Errorf("error should name the agent, got: %v", err) + } +} + +func TestResolveRolesFromFlags_UnknownFixerErrors(t *testing.T) { + t.Parallel() + _, err := resolveRolesFromFlags( + nil, "no-such-agent", + []types.AgentName{"claude-code"}, + ) + if err == nil { + t.Fatal("expected error for unknown fixer") + } + if !strings.Contains(err.Error(), "no-such-agent") { + t.Errorf("error should name the agent, got: %v", err) + } +} + +func TestMergeFlagOverrideWithSavedSkills_CopiesSavedSkills(t *testing.T) { + t.Parallel() + override := map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + } + saved := map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review", "/test-auditor"}, Prompt: "Focus on auth."}, + "codex": {Skills: []string{"/codex-review"}}, // not in override; should be dropped + } + got := mergeFlagOverrideWithSavedSkills(context.Background(), override, saved) + if len(got) != 1 { + t.Fatalf("expected 1 entry, got %d", len(got)) + } + cfg := got["claude-code"] + if cfg.Role != settings.RoleReviewer { + t.Errorf("role = %q", cfg.Role) + } + if len(cfg.Skills) != 2 { + t.Errorf("skills = %v, want 2 entries", cfg.Skills) + } + if cfg.Prompt != "Focus on auth." { + t.Errorf("prompt = %q", cfg.Prompt) + } +} diff --git a/cmd/entire/cli/review/inline_prompt.go b/cmd/entire/cli/review/inline_prompt.go new file mode 100644 index 000000000..97a378f38 --- /dev/null +++ b/cmd/entire/cli/review/inline_prompt.go @@ -0,0 +1,67 @@ +// Package review — see env.go for package-level rationale. +// +// inline_prompt.go provides the ReadSingleKey helper used by the +// post-review fix prompt ([Y]es / [s]elect / [n]o / [A]lways). It is a +// minimal single-rune reader so the inline prompt does not require a +// full huh.Form for a one-character answer. +package review + +import ( + "bufio" + "errors" + "fmt" + "io" + "unicode" +) + +// KeyChoice describes the menu offered by ReadSingleKey. +// +// Default is the rune returned on EOF or on plain Enter. (ReadSingleKey +// does not detect TTY-ness itself; callers gate on CanPromptInteractively +// before reading, so a non-TTY simply hits EOF and falls back to Default.) +// Allowed is a documentation+matching string: each rune in Allowed is one +// of the legal answers. Matching is case-insensitive, but the returned +// rune is in the case it appears in Allowed (so the caller can decide +// whether a key is the "default-shown" form). +type KeyChoice struct { + Default rune + Allowed string +} + +// ReadSingleKey reads runes from r until one matches an entry in +// choice.Allowed (case-insensitive). Behavior: +// +// - EOF, newline, or carriage-return returns choice.Default. +// - Whitespace (other than newline / carriage-return) is skipped. +// - Unknown runes are swallowed; the loop continues. +// - Returns choice.Default if r is nil. +// +// The returned rune is in the case present in choice.Allowed, not the +// case the user typed. +func ReadSingleKey(r io.Reader, choice KeyChoice) (rune, error) { + if r == nil { + return choice.Default, nil + } + br := bufio.NewReader(r) + for { + b, _, err := br.ReadRune() + if errors.Is(err, io.EOF) { + return choice.Default, nil + } + if err != nil { + return 0, fmt.Errorf("read key: %w", err) + } + if b == '\n' || b == '\r' { + return choice.Default, nil + } + if unicode.IsSpace(b) { + continue + } + for _, a := range choice.Allowed { + if unicode.ToLower(a) == unicode.ToLower(b) { + return a, nil + } + } + // Unknown — try next rune. + } +} diff --git a/cmd/entire/cli/review/inline_prompt_test.go b/cmd/entire/cli/review/inline_prompt_test.go new file mode 100644 index 000000000..8467d0649 --- /dev/null +++ b/cmd/entire/cli/review/inline_prompt_test.go @@ -0,0 +1,86 @@ +package review_test + +import ( + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/review" +) + +func TestReadSingleKey_EmptyReaderReturnsDefault(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(strings.NewReader(""), review.KeyChoice{ + Default: 'Y', Allowed: "YsnA", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 'Y' { + t.Errorf("got %q, want Y (default on EOF)", got) + } +} + +func TestReadSingleKey_NilReaderReturnsDefault(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(nil, review.KeyChoice{ + Default: 'N', Allowed: "YN", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 'N' { + t.Errorf("got %q, want N", got) + } +} + +func TestReadSingleKey_UnknownInputRePrompts(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(strings.NewReader("xY"), review.KeyChoice{ + Default: 'N', Allowed: "YN", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 'Y' { + t.Errorf("got %q, want Y", got) + } +} + +func TestReadSingleKey_NormalizesCase(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(strings.NewReader("a"), review.KeyChoice{ + Default: 'N', Allowed: "YnA", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 'A' { + t.Errorf("got %q, want A (case-normalized from 'a')", got) + } +} + +func TestReadSingleKey_PlainEnterReturnsDefault(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(strings.NewReader("\n"), review.KeyChoice{ + Default: 'Y', Allowed: "YsnA", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 'Y' { + t.Errorf("got %q, want Y", got) + } +} + +func TestReadSingleKey_SkipsLeadingWhitespace(t *testing.T) { + t.Parallel() + got, err := review.ReadSingleKey(strings.NewReader("\t s"), review.KeyChoice{ + Default: 'Y', Allowed: "YsnA", + }) + if err != nil { + t.Fatalf("err: %v", err) + } + if got != 's' { + t.Errorf("got %q, want s", got) + } +} diff --git a/cmd/entire/cli/review/manifest.go b/cmd/entire/cli/review/manifest.go index e9850b88b..23e806720 100644 --- a/cmd/entire/cli/review/manifest.go +++ b/cmd/entire/cli/review/manifest.go @@ -70,17 +70,32 @@ func buildLocalReviewManifestFromSummary( } usedSessions := map[string]bool{} for _, run := range summary.AgentRuns { + output := agentRunOutput(run) st := matchReviewSessionState(worktreeRoot, headSHA, summary.StartedAt, run.Name, states, usedSessions) - if st == nil || st.SessionID == "" { + sessionID := "" + if st != nil { + sessionID = st.SessionID + } + // Include the agent when it produced review output OR matched a tagged + // review session. Agents whose lifecycle hooks don't fire during the + // run — notably codex, which fires no hooks during `codex exec` — never + // get a tagged review session state, but their review narrative is + // captured live in run.Buffer (agentRunOutput). Gating on session state + // alone silently dropped those agents' findings from the fix manifest, + // so the [s] picker showed only the hook-firing agents (e.g. claude) + // plus the aggregate. + if output == "" && sessionID == "" { continue } - usedSessions[st.SessionID] = true + if sessionID != "" { + usedSessions[sessionID] = true + } manifest.Sources = append(manifest.Sources, ManifestSource{ - SessionID: st.SessionID, + SessionID: sessionID, Agent: run.Name, Label: labelForReviewAgent(run.Name), Status: run.Status.String(), - Output: agentRunOutput(run), + Output: output, }) } return manifest @@ -562,8 +577,11 @@ func localReviewManifestDir(ctx context.Context) (string, error) { func localReviewManifestFilename(manifest LocalReviewManifest) string { name := manifest.CreatedAt.UTC().Format("20060102T150405") - if len(manifest.Sources) > 0 && manifest.Sources[0].SessionID != "" { - name += "-" + safeManifestFilenamePart(manifest.Sources[0].SessionID) + // Use the manifest handle (first source WITH a session id) rather than + // Sources[0] — a session-less source (e.g. codex, whose exec fires no + // hooks) can be first, and we still want a stable, identifiable filename. + if handle := reviewManifestHandle(manifest); handle != "" { + name += "-" + safeManifestFilenamePart(handle) } return name + ".json" } diff --git a/cmd/entire/cli/review/manifest_test.go b/cmd/entire/cli/review/manifest_test.go index b8edfadf7..0ef4d24a9 100644 --- a/cmd/entire/cli/review/manifest_test.go +++ b/cmd/entire/cli/review/manifest_test.go @@ -17,7 +17,10 @@ import ( "github.com/entireio/cli/cmd/entire/cli/testutil" ) -const manifestTestCodexAgent = "codex" +const ( + manifestTestCodexAgent = "codex" + manifestTestClaudeSession = "claude-session" +) const manifestTokenTestAgentName agenttypes.AgentName = "review-token-test" const manifestTokenTestAgentType agenttypes.AgentType = "Review Token Test" @@ -287,8 +290,8 @@ func TestWriteReviewCompletionFooter_PrintsExactFixCommands(t *testing.T) { got := b.String() for _, want := range []string{ "Review complete.", - "entire review --fix claude-session --all", - "entire review --fix claude-session", + "entire review fix claude-session --all", + "entire review fix claude-session", } { if !strings.Contains(got, want) { t.Fatalf("footer missing %q:\n%s", want, got) @@ -296,6 +299,34 @@ func TestWriteReviewCompletionFooter_PrintsExactFixCommands(t *testing.T) { } } +// A codex-only run has a source with output but no SessionID (codex exec +// fires no hook to tag a session). The footer must still print, with the +// handle omitted — `entire review fix --all` resolves to the latest run. +func TestWriteReviewCompletionFooter_PrintsForSessionlessCodexManifest(t *testing.T) { + manifest := LocalReviewManifest{ + Sources: []ManifestSource{{SessionID: "", Label: "Codex", Output: "H1. finding"}}, + } + var b strings.Builder + + writeReviewCompletionFooter(&b, manifest) + + got := b.String() + if !strings.Contains(got, "Review complete.") { + t.Fatalf("codex-only footer should print:\n%s", got) + } + if !strings.Contains(got, "entire review fix --all") { + t.Fatalf("codex-only footer should offer handleless fix command:\n%s", got) + } +} + +func TestWriteReviewCompletionFooter_SkipsEmptyManifest(t *testing.T) { + var b strings.Builder + writeReviewCompletionFooter(&b, LocalReviewManifest{}) + if got := b.String(); strings.Contains(got, "Review complete.") { + t.Fatalf("empty manifest must not print a footer:\n%s", got) + } +} + func TestPrintReviewFindingsList_PrintsProductionCommandName(t *testing.T) { oldArgs := os.Args t.Cleanup(func() { os.Args = oldArgs }) @@ -317,7 +348,7 @@ 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") { + if !strings.Contains(got, "entire review fix claude-session --all") { t.Fatalf("findings list missing production command:\n%s", got) } } @@ -379,86 +410,6 @@ func TestReviewFixSourcePickerTitle_IncludesSessionHandle(t *testing.T) { } } -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) { - choices := []AgentChoice{ - {Name: "claude-code", Label: "Claude Code"}, - {Name: manifestTestCodexAgent, Label: "Codex"}, - } - - got, ok := savedReviewFixAgentPick(choices, manifestTestCodexAgent) - - if !ok { - t.Fatal("expected saved agent match") - } - if got != manifestTestCodexAgent { - t.Fatalf("saved pick = %q, want codex", got) - } -} - -func TestSavedReviewFixAgentPick_RejectsUnknownSavedAgent(t *testing.T) { - choices := []AgentChoice{{Name: "claude-code", Label: "Claude Code"}} - - got, ok := savedReviewFixAgentPick(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{ @@ -515,6 +466,65 @@ func TestBuildLocalReviewManifestFromSummary_GroupsAgentSessionsAndAggregate(t * } } +// TestBuildLocalReviewManifestFromSummary_IncludesAgentWithOutputButNoSession +// locks the codex case: codex fires no lifecycle hooks during `codex exec`, so +// it never gets a tagged review session state — but its review narrative is +// captured live in run.Buffer and must still become a fix source (otherwise +// the [s] picker shows only claude + the aggregate, never codex). +func TestBuildLocalReviewManifestFromSummary_IncludesAgentWithOutputButNoSession(t *testing.T) { + started := time.Date(2026, 5, 7, 10, 0, 0, 0, time.UTC) + summary := reviewtypes.RunSummary{ + StartedAt: started, + AgentRuns: []reviewtypes.AgentRun{ + { + Name: "claude-code", + Status: reviewtypes.AgentStatusSucceeded, + Buffer: []reviewtypes.Event{reviewtypes.AssistantText{Text: "Claude finding"}}, + }, + { + Name: manifestTestCodexAgent, + Status: reviewtypes.AgentStatusSucceeded, + Buffer: []reviewtypes.Event{reviewtypes.AssistantText{Text: "Codex finding"}}, + }, + }, + } + // Only claude has a tagged review session; codex has none (no exec hooks). + states := []*session.State{ + { + SessionID: manifestTestClaudeSession, + Kind: session.KindAgentReview, + WorktreePath: "/repo", + BaseCommit: "abc123", + StartedAt: started.Add(time.Second), + AgentType: agenttypes.AgentType("Claude Code"), + }, + } + + manifest := buildLocalReviewManifestFromSummary("/repo", "abc123", summary, states, "") + if len(manifest.Sources) != 2 { + t.Fatalf("sources = %d, want 2 (codex included despite no session): %#v", len(manifest.Sources), manifest.Sources) + } + var codex *ManifestSource + for i := range manifest.Sources { + if manifest.Sources[i].Agent == manifestTestCodexAgent { + codex = &manifest.Sources[i] + } + } + if codex == nil { + t.Fatalf("codex source missing: %#v", manifest.Sources) + } + if codex.Output != "Codex finding" { + t.Errorf("codex Output = %q, want %q", codex.Output, "Codex finding") + } + if codex.SessionID != "" { + t.Errorf("codex SessionID = %q, want empty (no tagged session)", codex.SessionID) + } + // The handle still resolves via claude's real session id. + if got := reviewManifestHandle(manifest); got != manifestTestClaudeSession { + t.Errorf("handle = %q, want %q", got, manifestTestClaudeSession) + } +} + func TestWarnManifestNotWritten_PrintsReasonAndDiagnosticHints(t *testing.T) { var b strings.Builder @@ -547,7 +557,7 @@ func TestWritePostReviewManifest_WarnsWhenNoMatchingSessions(t *testing.T) { } // SHA is irrelevant: matcher never runs since no session states exist. - writePostReviewManifest(context.Background(), &out, repoRoot, "abc123", summary, "") + _ = writePostReviewManifestAndReturn(context.Background(), &out, repoRoot, "abc123", summary, "") got := out.String() if !strings.Contains(got, "Note: review skills ran but findings were not persisted.") { @@ -813,11 +823,12 @@ func TestExplainEmptyManifest_MultiAgentNamesFailingAgent(t *testing.T) { // TestBuildLocalReviewManifestFromSummary_PartialMatch_NoWarning pins the // behavior that a partial-success run (one agent matched, another didn't) -// produces a non-empty manifest. writePostReviewManifest only fires the -// "findings were not persisted" warning when len(manifest.Sources) == 0, -// so partial success silently succeeds — intentional behavior that this -// test makes explicit. A future refactor that changes this would have to -// update the test, forcing the change to be deliberate. +// produces a non-empty manifest. writePostReviewManifestAndReturn only +// fires the "findings were not persisted" warning when +// len(manifest.Sources) == 0, so partial success silently succeeds — +// intentional behavior that this test makes explicit. A future refactor +// that changes this would have to update the test, forcing the change +// to be deliberate. func TestBuildLocalReviewManifestFromSummary_PartialMatch_NoWarning(t *testing.T) { t.Parallel() started := time.Now() diff --git a/cmd/entire/cli/review/multipicker.go b/cmd/entire/cli/review/multipicker.go deleted file mode 100644 index e86d5429b..000000000 --- a/cmd/entire/cli/review/multipicker.go +++ /dev/null @@ -1,106 +0,0 @@ -// Package review — see env.go for package-level rationale. -// -// multipicker.go provides spawn-time agent multi-selection and per-run -// prompt collection for multi-agent review runs. When 2+ launchable agents -// are configured AND the user has not passed --agent, the dispatch logic -// in cmd.go calls PickAgents to let the user choose a subset and optionally -// add a one-off prompt without editing settings. -package review - -import ( - "context" - "errors" - "fmt" - "sort" - - "charm.land/huh/v2" -) - -// PickedAgents is the result of PickAgents: the agents the user selected -// for this run, plus an optional per-run prompt to append to the composed -// review prompt for each agent. -type PickedAgents struct { - // Names contains the agent registry keys selected by the user, - // e.g. ["claude-code", "codex"]. Sorted alphabetically. - Names []string - - // PerRun is optional textarea content; "" when the user skipped or cleared it. - PerRun string -} - -// ErrPickerCancelled is returned when the user aborts the multi-select. -var ErrPickerCancelled = errors.New("agent picker cancelled") - -// ErrNoAgentsSelected is returned when the user unchecks all agents. -// Caller should surface a clear error rather than running with zero agents. -var ErrNoAgentsSelected = errors.New("no agents selected for review") - -// PickAgents shows a multi-select form populated from eligible (the agents -// that are both configured AND have an AgentReviewer), pre-checks all of -// them, and returns the user's selection plus an optional per-run prompt. -// -// Returns ErrPickerCancelled if the user aborts. An empty selection (user -// unchecked all boxes) returns ErrNoAgentsSelected. -// -// Requires len(eligible) >= 2; returns an error if the caller passes fewer -// than 2 choices — this function is for multi-agent flows only. -func PickAgents(ctx context.Context, eligible []AgentChoice) (PickedAgents, error) { - if len(eligible) < 2 { - return PickedAgents{}, fmt.Errorf("PickAgents requires at least 2 eligible agents, got %d", len(eligible)) - } - if ctx.Err() != nil { - return PickedAgents{}, ErrPickerCancelled - } - - // Sort alphabetically for stable display order regardless of how the - // caller populated the slice. - sorted := make([]AgentChoice, len(eligible)) - copy(sorted, eligible) - sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) - - // Build options pre-selected (all agents checked by default — mirrors - // PR #1018 behaviour so the user can just press Enter to run all). - options := make([]huh.Option[string], 0, len(sorted)) - for _, c := range sorted { - options = append(options, huh.NewOption(c.Label, c.Name).Selected(true)) - } - - var picked []string - multiForm := newAccessibleForm(huh.NewGroup( - buildAgentMultiSelect(options, &picked), - )) - if err := multiForm.RunWithContext(ctx); err != nil { - return PickedAgents{}, ErrPickerCancelled - } - - if len(picked) == 0 { - return PickedAgents{}, ErrNoAgentsSelected - } - - // Sort the selection alphabetically so the caller receives a stable slice. - sort.Strings(picked) - - // Per-run prompt: optional textarea presented after agent selection. - var perRun string - promptForm := newAccessibleForm(huh.NewGroup( - huh.NewText(). - Title("Optional per-run prompt"). - Description("e.g. 'focus on auth' — appended to the review prompt for this run only. Leave blank to skip."). - Value(&perRun), - )) - if err := promptForm.RunWithContext(ctx); err != nil { - // Cancellation on the prompt step (Ctrl+C) propagates as picker - // cancelled — we don't want an empty prompt here; user can retry. - return PickedAgents{}, ErrPickerCancelled - } - - return PickedAgents{Names: picked, PerRun: perRun}, nil -} - -func buildAgentMultiSelect(options []huh.Option[string], picked *[]string) *huh.MultiSelect[string] { - return huh.NewMultiSelect[string](). - Title("Which agents should run this review?"). - Options(options...). - Height(len(options) + 1). - Value(picked) -} diff --git a/cmd/entire/cli/review/multipicker_test.go b/cmd/entire/cli/review/multipicker_test.go deleted file mode 100644 index dd8cd99fd..000000000 --- a/cmd/entire/cli/review/multipicker_test.go +++ /dev/null @@ -1,96 +0,0 @@ -package review_test - -import ( - "context" - "errors" - "strings" - "testing" - - "charm.land/huh/v2" - - "github.com/entireio/cli/cmd/entire/cli/review" -) - -// TestPickAgents_TooFewEligibleReturnsError verifies that calling PickAgents -// with fewer than 2 choices returns an error — it is the caller's -// responsibility to route single-agent flows through the single-agent path. -func TestPickAgents_TooFewEligibleReturnsError(t *testing.T) { - t.Parallel() - _, err := review.PickAgents(context.Background(), []review.AgentChoice{ - {Name: "claude-code", Label: "claude-code (1 skill configured)"}, - }) - if err == nil { - t.Fatal("expected error for single-element eligible list") - } - // Must NOT be ErrPickerCancelled or ErrNoAgentsSelected — it's a caller - // contract violation, not a user action. - if errors.Is(err, review.ErrPickerCancelled) { - t.Errorf("should not return ErrPickerCancelled for too-few-eligible") - } - if errors.Is(err, review.ErrNoAgentsSelected) { - t.Errorf("should not return ErrNoAgentsSelected for too-few-eligible") - } -} - -// TestPickAgents_EmptyEligibleReturnsError covers the zero-length case. -func TestPickAgents_EmptyEligibleReturnsError(t *testing.T) { - t.Parallel() - _, err := review.PickAgents(context.Background(), nil) - if err == nil { - t.Fatal("expected error for empty eligible list") - } - if errors.Is(err, review.ErrPickerCancelled) || errors.Is(err, review.ErrNoAgentsSelected) { - t.Errorf("wrong error sentinel for empty eligible: %v", err) - } -} - -// TestPickAgents_CancelledContextReturnsPickerCancelled verifies that a -// pre-cancelled context causes PickAgents to return ErrPickerCancelled -// (not a raw context.Canceled). The huh RunWithContext method returns an -// error for a cancelled context, which PickAgents maps to ErrPickerCancelled. -func TestPickAgents_CancelledContextReturnsPickerCancelled(t *testing.T) { - t.Parallel() - ctx, cancel := context.WithCancel(context.Background()) - cancel() // cancel before calling PickAgents - - _, err := review.PickAgents(ctx, []review.AgentChoice{ - {Name: "claude-code", Label: "claude-code (1 skill configured)"}, - {Name: "codex", Label: "codex (2 skills configured)"}, - }) - if err == nil { - t.Fatal("expected error from cancelled context") - } - if !errors.Is(err, review.ErrPickerCancelled) { - t.Errorf("expected ErrPickerCancelled, got: %v", err) - } -} - -// TestPickedAgentsSentinels verifies the exported error sentinels are distinct -// values so callers can distinguish them cleanly. -func TestPickedAgentsSentinels(t *testing.T) { - t.Parallel() - if errors.Is(review.ErrPickerCancelled, review.ErrNoAgentsSelected) { - t.Error("ErrPickerCancelled and ErrNoAgentsSelected must be distinct") - } - if errors.Is(review.ErrNoAgentsSelected, review.ErrPickerCancelled) { - t.Error("ErrNoAgentsSelected and ErrPickerCancelled must be distinct") - } -} - -func TestAgentMultiSelectRendersAllEligibleAgents(t *testing.T) { - t.Parallel() - - var picked []string - field := review.ExposedBuildAgentMultiSelect([]huh.Option[string]{ - huh.NewOption("claude-code (3 skills configured)", "claude-code").Selected(true), - huh.NewOption("codex (1 skills configured)", "codex").Selected(true), - }, &picked).WithWidth(80) - field.Focus() - - view := field.View() - for _, want := range []string{"claude-code", "codex"} { - if !strings.Contains(view, want) { - t.Fatalf("agent picker did not render %q:\n%s", want, view) - } - } -} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index e82fcba25..bae3428a2 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -9,7 +9,6 @@ import ( "context" "errors" "fmt" - "io" "log/slog" "sort" "strings" @@ -20,7 +19,6 @@ import ( "github.com/entireio/cli/cmd/entire/cli/agent/skilldiscovery" "github.com/entireio/cli/cmd/entire/cli/agent/types" "github.com/entireio/cli/cmd/entire/cli/logging" - reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" "github.com/entireio/cli/cmd/entire/cli/settings" "github.com/entireio/cli/cmd/entire/cli/uiform" ) @@ -40,200 +38,6 @@ func newAccessibleForm(groups ...*huh.Group) *huh.Form { return uiform.New(groups...) } -// ConfirmFirstRunSetup prints a banner framing the picker as first-run -// setup (rather than the review itself) and waits for the user to confirm. -// Returns false if the user cancels; caller should bail gracefully. -// -// Signposting matters here because `entire review` with no config silently -// drops into the picker — users running the command to start a review can -// mistake the picker for the review. The banner + confirmation makes the -// 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) - 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) - - proceed := true - form := newAccessibleForm(huh.NewGroup( - huh.NewConfirm(). - Title("Set up review skills now?"). - Affirmative("Yes"). - Negative("Cancel"). - Value(&proceed), - )) - if err := form.RunWithContext(ctx); err != nil { - fmt.Fprintln(out, "Setup cancelled.") - return false - } - if !proceed { - fmt.Fprintln(out, "Setup cancelled.") - } - return proceed -} - -// 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 -// huh.Option.Selected(true), mirroring how `entire enable` preserves prior -// selections in its own agent picker. -// -// getInstalled is injected to avoid an import cycle with the cli package. -func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func(context.Context) []types.AgentName) (map[string]settings.ReviewConfig, error) { - installed := getInstalled(ctx) - if len(installed) == 0 { - return nil, errors.New( - "no agents with hooks installed; " + - "run 'entire configure --agent ' to install hooks for one, " + - "or 'entire enable' to set up the repo", - ) - } - - // Narrow to agents that have a curated skills list; others need manual - // editing of clone-local preferences under review.. - type configurableAgent struct { - name types.AgentName - ag agent.Agent - } - var configurable []configurableAgent - for _, name := range installed { - if !skilldiscovery.IsEligible(string(name)) { - continue - } - ag, err := agent.Get(name) - if err != nil { - continue - } - configurable = append(configurable, configurableAgent{name: name, ag: ag}) - } - if len(configurable) == 0 { - prefsPath, pathErr := settings.ClonePreferencesPath(ctx) - if pathErr != nil { - return nil, errors.New( - "no installed agents have curated review skills; " + - "install an eligible agent and run `entire review --edit`, " + - "or edit clone-local review preferences under review.", - ) - } - return nil, fmt.Errorf( - "no installed agents have curated review skills; "+ - "install an eligible agent and run `entire review --edit`, "+ - "or edit clone-local review preferences (%s) under review.", - prefsPath, - ) - } - - // 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 - // 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 := "" - 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 - } - - // Up-front header: make the order and count obvious so users can spot - // when an agent they expected isn't being offered (e.g., hooks not - // installed for it yet). - labels := make([]string, 0, len(configurable)) - for _, c := range configurable { - labels = append(labels, string(c.ag.Type())) - } - fmt.Fprintf(out, "Configuring review for %d agent(s): %s\n", len(configurable), strings.Join(labels, ", ")) - fmt.Fprintln(out, "(Previously-saved skills are pre-checked. Space to toggle, enter to confirm.)") - fmt.Fprintln(out) - - selected := map[string]settings.ReviewConfig{} - for i, c := range configurable { - curated := skilldiscovery.CuratedBuiltinsFor(string(c.name)) - - // Discover + dedupe + filter hints. - var discovered []agent.DiscoveredSkill - if d, ok := c.ag.(agent.SkillDiscoverer); ok { - if ds, dErr := d.DiscoverReviewSkills(ctx); dErr == nil { - discovered = ds - } else { - logging.Debug(ctx, "review discovery failed", - slog.String("agent", string(c.name)), slog.String("error", dErr.Error())) - } - } - builtinNames := builtinNameSet(curated) - discovered = filterOutBuiltinCollisions(discovered, builtinNames) - - discoveredSet := make(map[string]struct{}, len(discovered)) - for _, d := range discovered { - discoveredSet[d.Name] = struct{}{} - } - activeHints := skilldiscovery.ActiveInstallHintsFor(string(c.name), discoveredSet) - - // Pre-populate pick slices from saved config so the picker preselects - // them. The header promises "previously-saved skills are pre-checked"; - // without this split + Option.Selected(true) in BuildReviewPickerFields, - // --edit with accept-defaults silently wipes the agent's saved skills. - builtinPicks, discoveredPicks := SplitSavedPicks( - existing[string(c.name)].Skills, curated, discovered, - ) - prompt := existing[string(c.name)].Prompt - - fields := BuildReviewPickerFields( - string(c.name), curated, discovered, activeHints, prompt, - &builtinPicks, &discoveredPicks, &prompt, - ) - - // Prepend a non-blocking header Note so the agent being configured - // is always clearly visible. - header := huh.NewNote(). - Title(string(c.ag.Type())). - Description(fmt.Sprintf("Agent %d of %d · pick review skills and optional instructions", i+1, len(configurable))) - fields = append([]huh.Field{header}, fields...) - - form := newAccessibleForm(huh.NewGroup(fields...)) - if err := form.RunWithContext(ctx); err != nil { - return nil, fmt.Errorf("picker for %s: %w", c.name, err) - } - - cfg := settings.ReviewConfig{ - Skills: dedupeStrings(append(builtinPicks, discoveredPicks...)), - Prompt: strings.TrimSpace(prompt), - } - if !cfg.IsZero() { - selected[string(c.name)] = cfg - } - } - // Merge the picker's output with existing entries the picker could not - // surface. Without the merge, save would replace s.Review wholesale and - // silently drop entries the user had configured for external agents, - // uncurated agents, or agents whose hooks are temporarily uninstalled. - offered := make(map[string]struct{}, len(configurable)) - for _, c := range configurable { - offered[string(c.name)] = struct{}{} - } - merged := MergePickerResults(existing, offered, selected) - - // The emptiness check runs on `merged`, not `selected`. - if len(merged) == 0 { - return nil, errors.New("no review skills or prompt configured") - } - - fixAgent, err := pickReviewFixAgentPreference(ctx, merged, existingFixAgent) - if err != nil { - return nil, err - } - if err := saveReviewConfigAndFixAgent(ctx, merged, fixAgent); err != nil { - return nil, err - } - fmt.Fprintln(out, "Saved review config to local review preferences. Edit later with `entire review --edit`.") - return merged, nil -} - // MergePickerResults combines the picker's output with existing review // config entries that the picker did not surface. Agents in `offered` are // fully controlled by the picker: if they appear in `selected` with a @@ -290,34 +94,6 @@ func SaveReviewFixAgent(ctx context.Context, agentName string) error { 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) - } - if prefs == nil { - prefs = &settings.ClonePreferences{} - } - 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) - switch len(choices) { - case 0: - return current, nil - case 1: - return choices[0].Name, nil - default: - return promptForReviewFixAgent(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 @@ -356,55 +132,6 @@ func labelForAgentChoice(name string, cfg settings.ReviewConfig) string { } } -// 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. -// -// 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, - installed []types.AgentName, - reviewerFor func(string) reviewtypes.AgentReviewer, -) []AgentChoice { - eligible := ComputeEligibleConfigured(s, installed) - out := make([]AgentChoice, 0, len(eligible)) - for _, c := range eligible { - if reviewerFor(c.Name) != nil { - out = append(out, c) - } - } - return out -} - -// PromptForAgent renders the single-select agent picker shown when more than -// one eligible agent is configured. Returns the chosen agent name. Respects -// accessibility mode via newAccessibleForm. -func PromptForAgent(ctx context.Context, eligible []AgentChoice) (string, error) { - if err := ctx.Err(); err != nil { - return "", fmt.Errorf("agent picker: %w", err) - } - if len(eligible) == 0 { - return "", errors.New("no eligible agents to prompt for") - } - options := make([]huh.Option[string], 0, len(eligible)) - for _, c := range eligible { - options = append(options, huh.NewOption(c.Label, c.Name)) - } - picked := eligible[0].Name - form := newAccessibleForm(huh.NewGroup( - huh.NewSelect[string](). - Title("Which agent should run this review?"). - Options(options...). - Value(&picked), - )) - if err := form.RunWithContext(ctx); err != nil { - return "", fmt.Errorf("agent picker: %w", err) - } - return picked, nil -} - // SelectReviewAgent picks an agent from the configured review map. // // If override is non-empty, returns the config for that agent or an error @@ -472,6 +199,16 @@ func VerifyConfiguredSkillsInstalled(ctx context.Context, ag agent.Agent, cfg se if len(missing) == 0 { return nil } + // Codex resolves skills by loose description match against the catalog it + // injects into every session — not exact slash commands — and legacy saved + // configs may still carry pre-$form invocations like "/review". On-disk + // presence is therefore not authoritative for codex: log a hint but don't + // block the spawn, since the named skill still loose-matches at runtime. + if string(ag.Name()) == string(agent.AgentNameCodex) { + logging.Debug(ctx, "codex review skill(s) not found on disk; relying on codex loose match", + slog.String("skills", strings.Join(missing, ", "))) + return nil + } return fmt.Errorf( "configured review skill(s) not installed: %s\n"+ "run `entire review --edit` to reconfigure, or install the plugin and retry", diff --git a/cmd/entire/cli/review/picker_test.go b/cmd/entire/cli/review/picker_test.go index 208b4c0bb..745acff6b 100644 --- a/cmd/entire/cli/review/picker_test.go +++ b/cmd/entire/cli/review/picker_test.go @@ -2,6 +2,8 @@ package review_test import ( "context" + "os" + "path/filepath" "reflect" "strings" "testing" @@ -304,3 +306,72 @@ func TestSaveReviewFixAgent_PersistsSettings(t *testing.T) { t.Fatalf("ReviewFixAgent = %q, want %s", prefs.ReviewFixAgent, testCodexAgent) } } + +// TestSaveReviewConfig_ReturnsErrorOnMalformedSettings ensures SaveReviewConfig +// does not overwrite existing preferences when preferences.json is malformed. +func TestSaveReviewConfig_ReturnsErrorOnMalformedSettings(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + + preferencesPath, err := settings.ClonePreferencesPath(context.Background()) + if err != nil { + t.Fatalf("preferences path: %v", err) + } + if err := os.MkdirAll(filepath.Dir(preferencesPath), 0o750); err != nil { + t.Fatal(err) + } + malformed := []byte(`{"review": {`) + if err := os.WriteFile(preferencesPath, malformed, 0o600); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(preferencesPath) + if err != nil { + t.Fatal(err) + } + + err = review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + testAgentName: {Skills: []string{testReviewSkill}}, + }) + if err == nil { + t.Fatal("expected SaveReviewConfig to error on malformed preferences") + } + + after, err := os.ReadFile(preferencesPath) + if err != nil { + t.Fatal(err) + } + if string(before) != string(after) { + t.Errorf("preferences.json was overwritten on load error:\nbefore=%q\nafter=%q", before, after) + } +} + +// TestVerifyConfiguredSkillsInstalled_CodexIsAdvisory pins that codex skill +// verification never hard-fails on a non-discovered skill — codex resolves +// skills by loose description match, and legacy saved configs carry pre-$form +// invocations like "/review" that must keep working. Claude Code stays strict. +func TestVerifyConfiguredSkillsInstalled_CodexIsAdvisory(t *testing.T) { + // Cannot t.Parallel — t.Setenv isolates ~/.codex / ~/.claude so discovery + // finds nothing and every configured skill is "missing". + t.Setenv("HOME", t.TempDir()) + + codexAg, err := agent.Get("codex") + if err != nil { + t.Fatalf("agent.Get(codex): %v", err) + } + cfg := settings.ReviewConfig{Skills: []string{"/review", "$not-installed"}} + if err := review.VerifyConfiguredSkillsInstalled(context.Background(), codexAg, cfg); err != nil { + t.Errorf("codex verification should be advisory (nil), got: %v", err) + } + + // Control: Claude Code still hard-fails on a missing skill. + claudeAg, err := agent.Get("claude-code") + if err != nil { + t.Fatalf("agent.Get(claude-code): %v", err) + } + if err := review.VerifyConfiguredSkillsInstalled( + context.Background(), claudeAg, settings.ReviewConfig{Skills: []string{"/bogus:nope"}}, + ); err == nil { + t.Error("claude-code verification should still hard-fail on a missing skill") + } +} diff --git a/cmd/entire/cli/review/post_review.go b/cmd/entire/cli/review/post_review.go new file mode 100644 index 000000000..5d5603304 --- /dev/null +++ b/cmd/entire/cli/review/post_review.go @@ -0,0 +1,277 @@ +// Package review — see env.go for package-level rationale. +// +// post_review.go owns the end-of-run UX: the inline `--prompt` ask, the +// post-review fix prompt ([Y]es / [s]elect / [n]o / [A]lways), and the +// findings-footer printed when the user declines or stdin is not +// promptable. +package review + +import ( + "context" + "fmt" + "io" + "os" + "strings" + + "charm.land/huh/v2" + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/agentlaunch" + "github.com/entireio/cli/cmd/entire/cli/interactive" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// stagePerRunContext renders the pre-launch staging view — the scope banner +// plus the checkpoints/sessions in scope — and collects the optional per-run +// prompt, all before any agent is spawned. +// +// Interactive (and no --prompt): a single styled huh form — the optional +// context input on top, the "what's being reviewed" summary as a Note +// underneath (scope line as the note title, the checkpoints/sessions block as +// its body). The user sees everything in scope while deciding what context to +// add, and the form holds the screen until they answer rather than flashing by +// before the live TUI. +// +// --prompt supplied, or non-interactive (CI / agent host): the same summary is +// printed plainly so it's still visible, and no prompt is asked. +// +// Returns the per-run prompt (the supplied --prompt, the typed value, or ""). +func stagePerRunContext(ctx context.Context, out io.Writer, scopeBanner string, ctxResult ContextResult, perRunPrompt string) string { + contextBanner := formatContextBanner(ctxResult) + + if perRunPrompt != "" || !interactive.CanPromptInteractively() { + if scopeBanner != "" { + fmt.Fprintln(out, scopeBanner) + } + fmt.Fprintln(out, contextBanner) + if perRunPrompt != "" { + fmt.Fprintln(out, "Context: "+perRunPrompt) + } + return perRunPrompt + } + + noteTitle := "In scope" + if scopeBanner != "" { + noteTitle = scopeBanner + } + var value string + form := newAccessibleForm(huh.NewGroup( + huh.NewInput(). + Title("Add context for this run?"). + Description("Optional — press ↩ to skip"). + Value(&value), + huh.NewNote(). + Title(noteTitle). + Description(sanitizeForHuhNote(contextBanner)), + )) + if err := form.RunWithContext(ctx); err != nil { + // Cancellation = no per-run prompt; review proceeds. + return "" + } + return strings.TrimSpace(value) +} + +// sanitizeForHuhNote neutralises markdown that huh's Note renderer mangles in +// the terminal — notably backticks, which appear in checkpoint commit subjects +// like "add `entire review setup`". Display-only; the agent-facing checkpoint +// context (ContextResult.Prompt) is untouched. +func sanitizeForHuhNote(s string) string { + return strings.ReplaceAll(s, "`", "'") +} + +// findingsCount sums findings across all sources in a manifest. Used by +// the fix prompt header. +func findingsCount(m LocalReviewManifest) int { + total := 0 + for _, src := range m.Sources { + total += len(extractSourceFindings(reviewFixSource{ + Kind: reviewFixSourceAgent, + Agent: src.Agent, + Label: src.Label, + Output: src.Output, + }, 0)) + } + // If no structured findings parsed but at least one source has + // non-empty output, treat the manifest as having one finding so + // the prompt offers to apply something. Mirrors the + // extractReviewFindings fallback used by `entire review fix`. + if total == 0 { + for _, src := range m.Sources { + if strings.TrimSpace(src.Output) != "" { + return 1 + } + } + if strings.TrimSpace(m.AggregateOutput) != "" { + return 1 + } + } + return total +} + +// postReviewFixLauncher abstracts the fix dispatch so RunPostReviewFixPrompt +// can be exercised in tests without spawning real agent subprocesses. +type postReviewFixLauncher func(ctx context.Context, cmd *cobra.Command, manifest LocalReviewManifest, fixer, perRunPrompt string, all bool, silentErr func(error) error) error + +// launchFixFromManifest composes the manifest's prompt + the per-run +// prompt and dispatches the fix. The all=false branch defers to +// runReviewFix (which opens the selector); the all=true branch composes +// the prompt directly and calls agentlaunch.LaunchFixAgent. +func launchFixFromManifest( + ctx context.Context, + cmd *cobra.Command, + manifest LocalReviewManifest, + fixer, perRunPrompt string, + all bool, + silentErr func(error) error, +) error { + if !all { + // [s] / interactive picker — delegate to runReviewFix with the + // manifest's handle as the target. + return runReviewFix(ctx, cmd, reviewManifestHandle(manifest), false, fixer, perRunPrompt, silentErr) + } + sources, err := selectReviewFixSources(ctx, cmd, manifest, true /* all */) + if err != nil { + return err + } + prompt := composeReviewFixPrompt(manifest, sources) + if perRunPrompt != "" { + prompt += "\n\nAdditional context for this run:\n" + perRunPrompt + } + if err := agentlaunch.LaunchFixAgent(ctx, fixer, prompt); err != nil { + return fmt.Errorf("launch review fix agent: %w", err) + } + out := cmd.OutOrStdout() + fmt.Fprintln(out) + fmt.Fprintln(out, `Run: git commit -am "review: apply fixes"`) + return nil +} + +// printFindingsFooter prints the consolidated Run: block shown when the +// user picks [n] or when stdin isn't promptable. +func printFindingsFooter(w io.Writer, _ LocalReviewManifest) { + fmt.Fprintln(w) + fmt.Fprintln(w, "Skipped. Findings preserved on disk.") + fmt.Fprintln(w) + fmt.Fprintln(w, "Run: entire review fix apply when ready") + fmt.Fprintln(w, " entire review fix --all apply all, skip picker") + fmt.Fprintln(w, " entire review findings just browse the report") +} + +// RunPostReviewFixPrompt is the end-of-review UX entrypoint. Called by +// runReview / runMultiAgentPath after the manifest is written. Always +// returns nil on the "skip" or "no findings" paths; only returns an +// error if the fix launch itself errored. +// +// userExplicitlyOmittedFixer is true when the invocation came via +// --reviewers WITHOUT --fixer (the user signaled "just review, +// don't fix yet"). In that case the no-fixer footer is the inline +// `--fixer ` hint, not the "Run: entire review setup" nag. +func RunPostReviewFixPrompt( + ctx context.Context, + cmd *cobra.Command, + s *settings.EntireSettings, + manifest LocalReviewManifest, + perRunPrompt string, + silentErr func(error) error, + userExplicitlyOmittedFixer bool, +) error { + return runPostReviewFixPromptWithDeps(ctx, cmd, s, manifest, perRunPrompt, silentErr, userExplicitlyOmittedFixer, launchFixFromManifest, os.Stdin, interactive.CanPromptInteractively()) +} + +// runPostReviewFixPromptWithDeps is the test-injectable form of +// RunPostReviewFixPrompt. Production code threads launchFixFromManifest, +// os.Stdin, and interactive.CanPromptInteractively(); tests inject +// capture stubs and a canPrompt flag to exercise the interactive switch +// branches without a real TTY. +func runPostReviewFixPromptWithDeps( + ctx context.Context, + cmd *cobra.Command, + s *settings.EntireSettings, + manifest LocalReviewManifest, + perRunPrompt string, + silentErr func(error) error, + userExplicitlyOmittedFixer bool, + launch postReviewFixLauncher, + stdin io.Reader, + canPrompt bool, +) error { + out := cmd.OutOrStdout() + if findingsCount(manifest) == 0 { + return nil + } + + fixer := FixerOf(s) + if fixer == "" { + // Two sub-cases: + // (a) saved config never set a fixer — point at setup. + // (b) --reviewers/--fixer was used and explicitly omitted + // --fixer — user signaled "just review, don't fix yet"; + // offer a one-off `--fixer` hint instead of nagging + // about setup. + fmt.Fprintln(out) + if userExplicitlyOmittedFixer { + fmt.Fprintln(out, "Findings ready. To apply: re-run with `--fixer `, or browse: `entire review findings`.") + } else { + fmt.Fprintln(out, "Found findings, but no Fixer is configured.") + fmt.Fprintln(out, "Run: entire review setup") + } + return nil + } + + if s != nil && s.FixAfterReview == settings.FixAfterReviewAlways { + return launch(ctx, cmd, manifest, fixer, perRunPrompt, true /* all */, silentErr) + } + + if !canPrompt { + printFindingsFooter(out, manifest) + return nil + } + + fmt.Fprintf(out, "\nApply %d fixes now with %s?\n", findingsCount(manifest), displayLabelFor(fixer)) + fmt.Fprintln(out, " [Y]es · [s]elect fixes · [n]o · [A]lways") + choice, err := ReadSingleKey(stdin, KeyChoice{Default: 'Y', Allowed: "YsnA"}) + if err != nil { + return fmt.Errorf("read fix-prompt key: %w", err) + } + switch choice { + case 'Y': + return launch(ctx, cmd, manifest, fixer, perRunPrompt, true /* all */, silentErr) + case 's': + return launch(ctx, cmd, manifest, fixer, perRunPrompt, false /* all */, silentErr) + case 'n': + printFindingsFooter(out, manifest) + return nil + case 'A': + if s == nil { + s = &settings.EntireSettings{} + } + s.FixAfterReview = settings.FixAfterReviewAlways + // Persist to clone-local preferences (gitignored), not project + // settings.json. The legacy migration nudge fires when review + // keys appear in the committable file; writing them there from + // here would trip that prompt on every subsequent invocation. + if err := saveFixAfterReviewPref(ctx, settings.FixAfterReviewAlways); err != nil { + fmt.Fprintln(cmd.ErrOrStderr(), "warning: could not persist 'always' preference:", err) + } + return launch(ctx, cmd, manifest, fixer, perRunPrompt, true /* all */, silentErr) + } + return nil +} + +// saveFixAfterReviewPref persists the FixAfterReview mode into clone-local +// preferences (gitignored), where review-related settings belong. Mirrors +// the pattern in SaveReviewConfig / SaveReviewFixAgent (picker.go). +func saveFixAfterReviewPref(ctx context.Context, mode settings.FixAfterReviewMode) error { + prefs, err := settings.LoadClonePreferences(ctx) + if err != nil { + return fmt.Errorf("load clone preferences: %w", err) + } + if prefs == nil { + prefs = &settings.ClonePreferences{} + } + prefs.FixAfterReview = mode + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save clone preferences: %w", err) + } + return nil +} diff --git a/cmd/entire/cli/review/post_review_test.go b/cmd/entire/cli/review/post_review_test.go new file mode 100644 index 000000000..d9311697e --- /dev/null +++ b/cmd/entire/cli/review/post_review_test.go @@ -0,0 +1,313 @@ +package review + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +func newPostReviewTestCmd(out, errOut *bytes.Buffer) *cobra.Command { + cmd := &cobra.Command{Use: "review"} + cmd.SetOut(out) + cmd.SetErr(errOut) + return cmd +} + +func nonEmptyManifest() LocalReviewManifest { + return LocalReviewManifest{ + Sources: []ManifestSource{{ + SessionID: "s1", + Agent: "claude-code", + Label: "Claude Code", + Output: "H1. Some finding\nDetails about the finding.", + }}, + } +} + +func TestFindingsCount_CountsHeadingsAcrossSources(t *testing.T) { + t.Parallel() + m := LocalReviewManifest{ + Sources: []ManifestSource{ + {Output: "H1. one\nbody\nH2. two\nmore"}, + {Output: "M1. three"}, + }, + } + if got := findingsCount(m); got != 3 { + t.Errorf("got %d, want 3", got) + } +} + +func TestFindingsCount_EmptyManifestZero(t *testing.T) { + t.Parallel() + if got := findingsCount(LocalReviewManifest{}); got != 0 { + t.Errorf("got %d, want 0", got) + } +} + +func TestRunPostReviewFixPrompt_NoFindingsReturnsNilWithoutPrinting(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + called := false + launch := func(_ context.Context, _ *cobra.Command, _ LocalReviewManifest, _, _ string, _ bool, _ func(error) error) error { + called = true + return nil + } + if err := runPostReviewFixPromptWithDeps(context.Background(), cmd, s, LocalReviewManifest{}, "", nil, false, launch, strings.NewReader(""), false); err != nil { + t.Fatalf("err: %v", err) + } + if called { + t.Error("launch should not be called when there are no findings") + } + if out.Len() != 0 { + t.Errorf("expected no output for empty manifest, got: %q", out.String()) + } +} + +func TestRunPostReviewFixPrompt_NoFixerPrintsSetupHint(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + // No agent has the Fixer/Both role. + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + }} + if err := runPostReviewFixPromptWithDeps(context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, nil, strings.NewReader(""), false); err != nil { + t.Fatalf("err: %v", err) + } + if !strings.Contains(out.String(), "no Fixer is configured") { + t.Errorf("expected setup hint, got: %q", out.String()) + } + if !strings.Contains(out.String(), "entire review setup") { + t.Errorf("expected setup pointer, got: %q", out.String()) + } +} + +func TestRunPostReviewFixPrompt_NoFixerExplicitOmittedPrintsHintNotSetup(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + }} + if err := runPostReviewFixPromptWithDeps(context.Background(), cmd, s, nonEmptyManifest(), "", nil, true /* userExplicitlyOmittedFixer */, nil, strings.NewReader(""), false); err != nil { + t.Fatalf("err: %v", err) + } + if !strings.Contains(out.String(), "--fixer ") { + t.Errorf("expected --fixer hint, got: %q", out.String()) + } + if strings.Contains(out.String(), "entire review setup") { + t.Errorf("setup nag should NOT appear when user explicitly omitted --fixer, got: %q", out.String()) + } +} + +func TestRunPostReviewFixPrompt_AlwaysModeSkipsPromptAndLaunches(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{ + FixAfterReview: settings.FixAfterReviewAlways, + Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }, + } + captured := false + var capturedAll bool + launch := func(_ context.Context, _ *cobra.Command, _ LocalReviewManifest, _, _ string, all bool, _ func(error) error) error { + captured = true + capturedAll = all + return nil + } + if err := runPostReviewFixPromptWithDeps(context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, launch, strings.NewReader(""), false); err != nil { + t.Fatalf("err: %v", err) + } + if !captured { + t.Fatal("launch should have been called in always mode") + } + if !capturedAll { + t.Errorf("always mode should launch with all=true") + } + if strings.Contains(out.String(), "Apply") { + t.Errorf("always mode should not show the prompt, got: %q", out.String()) + } +} + +func TestRunPostReviewFixPrompt_NonTTYPrintsFooter(t *testing.T) { + t.Parallel() + // canPrompt=false simulates the non-interactive call site (CI, piped + // stdin). The helper should print the footer instead of reading keys. + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + launch := func(_ context.Context, _ *cobra.Command, _ LocalReviewManifest, _, _ string, _ bool, _ func(error) error) error { + return nil + } + if err := runPostReviewFixPromptWithDeps(context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, launch, strings.NewReader(""), false); err != nil { + t.Fatalf("err: %v", err) + } + if !strings.Contains(out.String(), "Findings preserved on disk") { + t.Errorf("expected footer, got: %q", out.String()) + } + if !strings.Contains(out.String(), "Run: entire review fix") { + t.Errorf("expected Run: pointer, got: %q", out.String()) + } +} + +// captureLaunch returns a postReviewFixLauncher that records whether it +// was called and the `all` argument it received. It's shared across the +// interactive-branch tests below. +type captureLaunch struct { + called bool + all bool +} + +func (c *captureLaunch) fn(_ context.Context, _ *cobra.Command, _ LocalReviewManifest, _, _ string, all bool, _ func(error) error) error { + c.called = true + c.all = all + return nil +} + +func TestRunPostReviewFixPrompt_InteractiveY_LaunchesAll(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + c := &captureLaunch{} + if err := runPostReviewFixPromptWithDeps( + context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, + c.fn, strings.NewReader("Y"), true, + ); err != nil { + t.Fatalf("err: %v", err) + } + if !c.called { + t.Fatal("expected launcher to be called on [Y]") + } + if !c.all { + t.Errorf("[Y] should launch with all=true, got all=%v", c.all) + } +} + +func TestRunPostReviewFixPrompt_InteractiveS_LaunchesSelect(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + c := &captureLaunch{} + if err := runPostReviewFixPromptWithDeps( + context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, + c.fn, strings.NewReader("s"), true, + ); err != nil { + t.Fatalf("err: %v", err) + } + if !c.called { + t.Fatal("expected launcher to be called on [s]") + } + if c.all { + t.Errorf("[s] should launch with all=false (delegates to selector), got all=%v", c.all) + } +} + +func TestRunPostReviewFixPrompt_InteractiveN_PrintsFooterNoLaunch(t *testing.T) { + t.Parallel() + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + c := &captureLaunch{} + if err := runPostReviewFixPromptWithDeps( + context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, + c.fn, strings.NewReader("n"), true, + ); err != nil { + t.Fatalf("err: %v", err) + } + if c.called { + t.Errorf("[n] should not call the launcher, but it was called (all=%v)", c.all) + } + if !strings.Contains(out.String(), "Findings preserved on disk") { + t.Errorf("[n] should print footer; got: %q", out.String()) + } +} + +func TestRunPostReviewFixPrompt_InteractiveA_PersistsAlwaysAndLaunchesAll(t *testing.T) { + // Cannot use t.Parallel() because we use t.Chdir(). The [A] branch + // persists to clone-local preferences (via SaveClonePreferences), + // which needs a real git common dir — hence testutil.InitRepo. + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + + var out, errOut bytes.Buffer + cmd := newPostReviewTestCmd(&out, &errOut) + s := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleBoth}, + }} + c := &captureLaunch{} + if err := runPostReviewFixPromptWithDeps( + context.Background(), cmd, s, nonEmptyManifest(), "", nil, false, + c.fn, strings.NewReader("A"), true, + ); err != nil { + t.Fatalf("err: %v", err) + } + if !c.called { + t.Fatal("expected launcher to be called on [A]") + } + if !c.all { + t.Errorf("[A] should launch with all=true, got all=%v", c.all) + } + if s.FixAfterReview != settings.FixAfterReviewAlways { + t.Errorf("[A] should set FixAfterReview = Always, got %q", s.FixAfterReview) + } + if strings.Contains(errOut.String(), "could not persist") { + t.Errorf("unexpected persistence warning: %q", errOut.String()) + } + // FixAfterReview must land in clone-local prefs (gitignored), NOT + // .entire/settings.json. Writing to the committable file would trip + // maybePromptReviewSettingsMigration on the next `entire review`. + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("LoadClonePreferences: %v", err) + } + if prefs == nil || prefs.FixAfterReview != settings.FixAfterReviewAlways { + t.Errorf("clone-local prefs should hold FixAfterReview=Always, got: %+v", prefs) + } + if _, projectRaw, exists, err := settings.LoadProjectRaw(context.Background()); err != nil { + t.Fatalf("LoadProjectRaw: %v", err) + } else if exists { + if _, has := projectRaw["fix_after_review"]; has { + t.Errorf("project settings.json contains fix_after_review key; would trigger legacy migration nudge") + } + } +} + +func TestPrintFindingsFooter_Contents(t *testing.T) { + t.Parallel() + var out bytes.Buffer + printFindingsFooter(&out, LocalReviewManifest{}) + got := out.String() + for _, want := range []string{ + "Skipped", + "Run: entire review fix", + "--all", + "entire review findings", + } { + if !strings.Contains(got, want) { + t.Errorf("footer missing %q:\n%s", want, got) + } + } +} diff --git a/cmd/entire/cli/review/setup.go b/cmd/entire/cli/review/setup.go index ae8ba4d04..2329a387b 100644 --- a/cmd/entire/cli/review/setup.go +++ b/cmd/entire/cli/review/setup.go @@ -15,6 +15,7 @@ import ( "fmt" "io" "log/slog" + "os" "sort" "strings" @@ -29,6 +30,108 @@ import ( "github.com/entireio/cli/cmd/entire/cli/settings" ) +// Agent registry names used by DetectInvokingAgent. Declared as +// constants so goconst doesn't flag the literals as duplicates of +// strings used elsewhere in the package. +const ( + agentNameClaudeCode = "claude-code" + agentNameCodex = "codex" + agentNameGemini = "gemini-cli" + agentNameCopilot = "copilot-cli" + agentNamePi = "pi" +) + +// DetectInvokingAgent reads the same env sentinels that +// interactive.CanPromptInteractively() consults to detect when entire +// is running inside an agent CLI. Returns the registry name of the +// caller (e.g. "claude-code", "codex") or "" if none detected. +// +// IMPORTANT: keep this list in sync with the sentinels in the +// interactive package — drift between the two would mean we'd detect +// non-interactive correctly but fail to identify the caller. +func DetectInvokingAgent() string { + switch { + case os.Getenv("CLAUDE_CODE") != "": + return agentNameClaudeCode + case os.Getenv("CODEX") != "": + return agentNameCodex + case os.Getenv("GEMINI_CLI") != "": + return agentNameGemini + case os.Getenv("COPILOT_CLI") != "": + return agentNameCopilot + case os.Getenv("PI_CODING_AGENT") != "": + return agentNamePi + } + return "" +} + +// seedDefaultSkills returns the skill invocation tokens to use for an agent +// that has no saved skills and no explicit per-run skill flags. It prefers +// the agent's curated built-ins; when an agent ships none (e.g. codex, whose +// review skills are discovered on disk rather than bundled in the binary) it +// falls back to on-disk discovered skills whose name signals a review skill +// (contains "review"). Without this, codex would fall back to a generic +// scope-only prompt instead of invoking e.g. $code-reviewer. +// +// Best-effort: returns nil (the run still works — the agent reviews against +// scope) if the agent can't be resolved or nothing matches. +func seedDefaultSkills(ctx context.Context, agentName string) []string { + if curated := skilldiscovery.CuratedBuiltinsFor(agentName); len(curated) > 0 { + out := make([]string, 0, len(curated)) + for _, b := range curated { + out = append(out, b.Name) + } + return out + } + ag, err := agent.Get(types.AgentName(agentName)) + if err != nil { + return nil + } + d, ok := ag.(agent.SkillDiscoverer) + if !ok { + return nil + } + discovered, err := d.DiscoverReviewSkills(ctx) + if err != nil { + return nil + } + var out []string + for _, sk := range discovered { + if strings.Contains(strings.ToLower(sk.Name), "review") { + out = append(out, sk.Name) + } + } + return out +} + +// invokerOnlyReviewConfig builds a default review map from a single +// invoking agent. The agent gets Role Both (reviews AND fixes). +// Returns an error if the agent isn't installed or has no launcher. +func invokerOnlyReviewConfig(ctx context.Context, agentName string, installed []types.AgentName) (map[string]settings.ReviewConfig, error) { + installedSet := make(map[string]struct{}, len(installed)) + for _, a := range installed { + installedSet[string(a)] = struct{}{} + } + if _, ok := installedSet[agentName]; !ok { + return nil, fmt.Errorf("invoking agent %q is not installed in this repo", agentName) + } + if _, ok := agent.LauncherFor(types.AgentName(agentName)); !ok { + return nil, fmt.Errorf("invoking agent %q has no launcher (cannot subprocess-spawn)", agentName) + } + cfg := settings.ReviewConfig{Role: settings.RoleBoth, Skills: seedDefaultSkills(ctx, agentName)} + return map[string]settings.ReviewConfig{agentName: cfg}, nil +} + +// formatInvokerOnlyNote returns the one-line user-visible explanation +// shown when an unconfigured review falls back to the invoking agent. +func formatInvokerOnlyNote(agentName string) string { + return fmt.Sprintf( + "Note: %s as reviewer + fixer (no config). Customize with `entire review setup`, "+ + "or tell your agent who should review/fix.", + agentName, + ) +} + // SetupForms collects the form constructors RunSetup uses. Production // passes a zero value (uses the real huh forms); tests inject stubs. type SetupForms struct { @@ -43,6 +146,8 @@ type SetupForms struct { // seeded from existing settings when available. Step 2: for every agent that // landed on a Reviewer-side role, present a skills + instructions picker. // After both steps, NormalizeRoles enforces the at-most-one-fixer invariant. +// +//nolint:unparam // map return is consumed by setup tests; production callers ignore it intentionally. func RunSetup( ctx context.Context, out io.Writer, @@ -63,10 +168,14 @@ func RunSetup( } sort.Strings(agentNames) - // Pre-seed current roles from saved settings; default to Reviewer when - // the agent has no prior entry. A load error is non-fatal here — we - // proceed with the default seeds and warn so users debugging "my saved - // roles aren't pre-selected" can find the reason. + // Pre-seed current roles from saved settings; default to Skip when the + // agent has no prior entry. Defaulting to Skip makes reviewing opt-in: + // every agent with Entire hooks installed is listed, but the user must + // explicitly choose Reviewer/Both for the ones they want — otherwise a + // machine with several agents enabled silently turns them all into + // reviewers (the "why is gemini/opencode reviewing?" surprise). The + // "at least one reviewer" guard below forces an explicit pick. A load + // error is non-fatal — proceed with Skip seeds and warn. current := make(map[string]settings.Role, len(agentNames)) saved, loadErr := settings.Load(ctx) if loadErr != nil { @@ -80,7 +189,7 @@ func RunSetup( continue } } - current[name] = settings.RoleReviewer + current[name] = settings.RoleSkip } pickRoles := forms.PickRoles @@ -108,6 +217,27 @@ func RunSetup( } normalized := NormalizeRoles(configMap) + // Guard against saving a non-functional config (zero reviewers). + // Without at least one agent in Role Reviewer or Both, `entire review` + // has nothing to run — refusing here surfaces the problem at setup + // time instead of after the user tries to invoke review. Checked + // against the normalized map so that pick-time edge cases (e.g. + // duplicate-fixer demotion creating a reviewer) are accounted for; + // only configs that are truly non-functional after normalization fail. + hasReviewer := false + for _, cfg := range normalized { + if cfg.Role.IsReviewer() { + hasReviewer = true + break + } + } + if !hasReviewer { + return nil, errors.New( + "no agents have role Reviewer or Both; entire review needs at least one " + + "(re-run `entire review setup` and pick Reviewer or Both for at least one agent)", + ) + } + pickSkills := forms.PickSkills if pickSkills == nil { pickSkills = realPickSkills @@ -207,15 +337,15 @@ func BuildPickRolesFields(agents []string, ptrs map[string]*settings.Role) []huh } // realPickRoles is the production role picker. It allocates one pointer per -// agent (seeded from current, defaulting to Reviewer when unset), passes the -// pointer map to BuildPickRolesFields, runs the form, then copies values -// back into current. +// agent (seeded from current, defaulting to Skip when unset so reviewing is +// opt-in), passes the pointer map to BuildPickRolesFields, runs the form, then +// copies values back into current. func realPickRoles(ctx context.Context, agents []string, current map[string]settings.Role) (map[string]settings.Role, error) { ptrs := make(map[string]*settings.Role, len(agents)) for _, name := range agents { v := current[name] if v == "" { - v = settings.RoleReviewer + v = settings.RoleSkip } ptrs[name] = &v } @@ -249,6 +379,15 @@ func BuildSetupSkillsFields( ) []huh.Field { var fields []huh.Field + // Header identifying which agent these skills are being configured for. + // The per-agent setup loop reuses this form for each agent in turn, so + // without it the skill list is ambiguous (e.g. claude vs codex). + // Note: huh renders Note text as markdown, so backticks get mangled in the + // terminal — keep these strings backtick-free. + fields = append(fields, huh.NewNote(). + Title("Review skills — "+displayLabelFor(agentName)). + Description("Skills this agent runs during entire review.")) + if builtinPicksOut != nil && len(*builtinPicksOut) == 0 && len(builtins) == 1 && strings.TrimSpace(previousPrompt) == "" { *builtinPicksOut = []string{builtins[0].Name} @@ -333,8 +472,8 @@ func BuildSetupSkillsFields( } // realPickSkills is the production skills picker for a single agent. It -// mirrors the per-agent loop inside RunReviewConfigPicker but uses -// BuildSetupSkillsFields (Input not Text for instructions). +// uses BuildSetupSkillsFields (Input not Text for instructions) so a +// single-line entry behaves like the rest of the setup wizard. func realPickSkills(ctx context.Context, agentName string, prefill settings.ReviewConfig) (settings.ReviewConfig, error) { ag, err := agent.Get(types.AgentName(agentName)) if err != nil { @@ -385,7 +524,7 @@ func PrintSetupBanner(out io.Writer, review map[string]settings.ReviewConfig) { var fixer string for name, cfg := range review { if cfg.Role.IsReviewer() { - reviewers = append(reviewers, displayLabelFor(name)) + reviewers = append(reviewers, displayLabelFor(name)+" "+reviewerSkillSuffix(cfg)) } if cfg.Role.IsFixer() { fixer = displayLabelFor(name) @@ -395,7 +534,10 @@ func PrintSetupBanner(out io.Writer, review map[string]settings.ReviewConfig) { fmt.Fprintln(out) fmt.Fprintln(out, "Review configured.") if len(reviewers) > 0 { - fmt.Fprintf(out, " Reviewers: %s\n", strings.Join(reviewers, ", ")) + fmt.Fprintln(out, " Reviewers:") + for _, r := range reviewers { + fmt.Fprintf(out, " %s\n", r) + } } else { fmt.Fprintln(out, " Reviewers: (none)") } @@ -409,6 +551,24 @@ func PrintSetupBanner(out io.Writer, review map[string]settings.ReviewConfig) { fmt.Fprintln(out, "Run: entire review") } +// reviewerSkillSuffix annotates a reviewer with what will drive its review. +// A configured prompt counts as guidance even with zero skills — that's how +// skill-less agents (notably gemini, which has no skill system here) get a +// focused review. Only an agent with neither skills nor a prompt does a truly +// generic, scope-only pass, which is what the last branch flags. +func reviewerSkillSuffix(cfg settings.ReviewConfig) string { + switch n := len(cfg.Skills); { + case n == 1: + return "(1 skill)" + case n > 1: + return fmt.Sprintf("(%d skills)", n) + case strings.TrimSpace(cfg.Prompt) != "": + return "(custom prompt)" + default: + return "(no skills — generic review)" + } +} + // displayLabelFor resolves an agent's human-readable name (Type) from the // registry, falling back to the registry key if Get fails. Used by the // roles picker and the post-setup banner so users see "Claude Code" diff --git a/cmd/entire/cli/review/setup_invoker_test.go b/cmd/entire/cli/review/setup_invoker_test.go new file mode 100644 index 000000000..073905f68 --- /dev/null +++ b/cmd/entire/cli/review/setup_invoker_test.go @@ -0,0 +1,46 @@ +package review_test + +import ( + "testing" + + "github.com/entireio/cli/cmd/entire/cli/review" +) + +func TestDetectInvokingAgent_ReadsEnvSentinels(t *testing.T) { + // Cannot use t.Parallel() because we use t.Setenv to drive env-var + // behavior; Go's testing package panics on Setenv + Parallel. + + cases := []struct { + envKey string + want string + }{ + {"CLAUDE_CODE", "claude-code"}, + {"CODEX", "codex"}, + {"GEMINI_CLI", "gemini-cli"}, + {"COPILOT_CLI", "copilot-cli"}, + {"PI_CODING_AGENT", "pi"}, + } + for _, c := range cases { + t.Run(c.envKey, func(t *testing.T) { + // Clear other sentinels so the test doesn't see drift from + // the parent shell. + for _, k := range []string{"CLAUDE_CODE", "CODEX", "GEMINI_CLI", "COPILOT_CLI", "PI_CODING_AGENT"} { + t.Setenv(k, "") + } + t.Setenv(c.envKey, "1") + got := review.DetectInvokingAgent() + if got != c.want { + t.Errorf("env %s=1 → DetectInvokingAgent() = %q, want %q", c.envKey, got, c.want) + } + }) + } +} + +func TestDetectInvokingAgent_NoSentinelReturnsEmpty(t *testing.T) { + for _, k := range []string{"CLAUDE_CODE", "CODEX", "GEMINI_CLI", "COPILOT_CLI", "PI_CODING_AGENT"} { + t.Setenv(k, "") + } + if got := review.DetectInvokingAgent(); got != "" { + t.Errorf("with no sentinels, got %q, want \"\"", got) + } +} diff --git a/cmd/entire/cli/review/setup_test.go b/cmd/entire/cli/review/setup_test.go index 16059afd7..890078569 100644 --- a/cmd/entire/cli/review/setup_test.go +++ b/cmd/entire/cli/review/setup_test.go @@ -18,13 +18,14 @@ import ( "github.com/entireio/cli/cmd/entire/cli/testutil" ) -// SetExistingConfigForTest writes a minimal .entire/settings.json into the -// current working directory so RunSetup's preselect-from-saved branch has -// something to read. +// SetExistingConfigForTest seeds clone-local review preferences (the same +// destination RunSetup writes to) so RunSetup's preselect-from-saved branch +// has something to read. Requires the test to have called testutil.InitRepo +// + t.Chdir so the git common dir resolves. func SetExistingConfigForTest(t *testing.T, reviewMap map[string]settings.ReviewConfig) { t.Helper() - if err := settings.Save(context.Background(), &settings.EntireSettings{Review: reviewMap}); err != nil { - t.Fatalf("SetExistingConfigForTest: settings.Save: %v", err) + if err := review.SaveReviewConfig(context.Background(), reviewMap); err != nil { + t.Fatalf("SetExistingConfigForTest: SaveReviewConfig: %v", err) } } @@ -85,11 +86,13 @@ func TestRunSetup_DefaultsRolesFromExistingConfig(t *testing.T) { } forms := review.SetupForms{ PickRoles: func(_ context.Context, agents []string, current map[string]settings.Role) (map[string]settings.Role, error) { + // claude-code is pre-seeded from saved config (Reviewer); codex has + // no saved entry so it defaults to Skip (reviewing is opt-in). if current["claude-code"] != settings.RoleReviewer { t.Errorf("pre-seed claude-code = %q, want reviewer", current["claude-code"]) } - if current["codex"] != settings.RoleReviewer { - t.Errorf("default codex = %q, want reviewer", current["codex"]) + if current["codex"] != settings.RoleSkip { + t.Errorf("default codex = %q, want skip (opt-in)", current["codex"]) } _ = agents return map[string]settings.Role{ @@ -111,6 +114,56 @@ func TestRunSetup_DefaultsRolesFromExistingConfig(t *testing.T) { if out["claude-code"].Role != settings.RoleReviewer { t.Errorf("claude-code role = %q, want reviewer", out["claude-code"].Role) } + + // Persistence target check: review config must land in clone-local + // preferences (gitignored), NOT .entire/settings.json. Writing to the + // committable file would re-trigger maybePromptReviewSettingsMigration + // on the next `entire review`. + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("LoadClonePreferences: %v", err) + } + if prefs == nil || prefs.Review["codex"].Role != settings.RoleFixer { + t.Errorf("expected clone-local prefs to hold codex=Fixer, got: %+v", prefs) + } + // Project settings.json must NOT have a review key after setup. + if _, projectRaw, exists, err := settings.LoadProjectRaw(context.Background()); err != nil { + t.Fatalf("LoadProjectRaw: %v", err) + } else if exists { + if _, has := projectRaw["review"]; has { + t.Errorf("project settings.json contains review key after setup; would trigger legacy migration nudge") + } + } +} + +func TestRunSetup_NoReviewersErrors(t *testing.T) { + // Uses t.Chdir; cannot t.Parallel. + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + forms := review.SetupForms{ + PickRoles: func(_ context.Context, _ []string, _ map[string]settings.Role) (map[string]settings.Role, error) { + return map[string]settings.Role{ + "claude-code": settings.RoleFixer, + "codex": settings.RoleSkip, + }, nil + }, + // PickSkills should NOT be called. + PickSkills: func(_ context.Context, name string, _ settings.ReviewConfig) (settings.ReviewConfig, error) { + t.Errorf("PickSkills should not be called when no reviewers configured; got call for %q", name) + return settings.ReviewConfig{}, nil + }, + } + _, err := review.RunSetup(context.Background(), io.Discard, + func(context.Context) []types.AgentName { + return []types.AgentName{"claude-code", "codex"} + }, forms) + if err == nil { + t.Fatal("expected error when no reviewers selected, got nil") + } + if !strings.Contains(err.Error(), "Reviewer or Both") { + t.Errorf("expected error to mention 'Reviewer or Both', got: %v", err) + } } func TestRunSetup_EnforcesAtMostOneFixerAfterPick(t *testing.T) { @@ -215,13 +268,18 @@ func TestPrintSetupBanner_MultipleReviewersWithDisplayLabels(t *testing.T) { t.Parallel() var buf bytes.Buffer review.PrintSetupBanner(&buf, map[string]settings.ReviewConfig{ - "claude-code": {Role: settings.RoleReviewer}, + "claude-code": {Role: settings.RoleReviewer, Skills: []string{"/review", "/security-review"}}, "gemini": {Role: settings.RoleReviewer}, "codex": {Role: settings.RoleFixer}, }) got := buf.String() - if !strings.Contains(got, "Reviewers: Claude Code, Gemini CLI") { - t.Errorf("expected display-label list, got:\n%s", got) + // Reviewers are annotated with their configured skill count; zero skills + // renders as a generic-review note rather than "(0 skills)". + if !strings.Contains(got, "Claude Code (2 skills)") { + t.Errorf("expected skill-count annotation, got:\n%s", got) + } + if !strings.Contains(got, "Gemini CLI (no skills — generic review)") { + t.Errorf("expected generic-review annotation for skill-less reviewer, got:\n%s", got) } if !strings.Contains(got, "Fixer: Codex") { t.Errorf("expected fixer line, got:\n%s", got) diff --git a/cmd/entire/cli/review/synthesis_sink.go b/cmd/entire/cli/review/synthesis_sink.go index 805ad63b6..6824d44b5 100644 --- a/cmd/entire/cli/review/synthesis_sink.go +++ b/cmd/entire/cli/review/synthesis_sink.go @@ -109,15 +109,16 @@ func (s SynthesisSink) RunFinished(summary reviewtypes.RunSummary) { if s.OnResult != nil { s.OnResult(result) } - // The synthesis verdict is markdown — render it through the same palette - // dispatch / DumpSink use, so multi-agent reviews finish with a visually - // consistent block. Non-TTY writers receive raw markdown unchanged. + // The synthesis verdict is markdown — render it with the same muted palette + // as the per-agent DumpSink so the multi-agent review finishes calm and + // consistent rather than as another wall of colour. Non-TTY writers receive + // raw markdown unchanged. // - // Use Fprint (not Fprintln): mdrender.Render returns glamour output that - // already ends with a newline, and the raw-markdown fallback path has its - // own terminal newline from the LLM response. Adding Fprintln would double - // the trailing blank line. - rendered, err := mdrender.RenderForWriter(s.Writer, result) + // Use Fprint (not Fprintln): mdrender returns glamour output that already + // ends with a newline, and the raw-markdown fallback path has its own + // terminal newline from the LLM response. Adding Fprintln would double the + // trailing blank line. + rendered, err := mdrender.RenderMutedForWriter(s.Writer, result) if err != nil { rendered = result } diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 8edba338a..a555df792 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -524,9 +524,13 @@ func (m reviewTUIModel) currentTerminalSize() (int, int) { return width, height } +// reviewHeaderStyle dims the column header so it reads as a header without +// adding colour — the dashboard stays calm and scannable (no per-cell colour). +var reviewHeaderStyle = lipgloss.NewStyle().Faint(true) + // headerLine returns the column header row. func (m reviewTUIModel) headerLine() string { - return m.renderTableLine("AGENT", "STATUS", "DURATION", "TOKENS", "PREVIEW") + return reviewHeaderStyle.Render(m.renderTableLine("AGENT", "STATUS", "DURATION", "TOKENS", "PREVIEW")) } // renderRow renders one agent row. @@ -564,9 +568,22 @@ func (m reviewTUIModel) renderRow(row agentRow) string { } } + // Token display: claude's live stream-json carries input tokens + // throughout the run but only surfaces output_tokens on the final + // `result` envelope. Codex carries usage only at turn.completed. So + // during a run the parser may emit Tokens{In: N, Out: 0} (input known, + // output not yet). Render input-only as just "" (no "/0" slash) so + // the column isn't misleading; render the finalized pair as "/". tokStr := "" - if row.tokens.In > 0 || row.tokens.Out > 0 { + switch { + case row.tokens.In > 0 && row.tokens.Out > 0: tokStr = fmt.Sprintf("%s/%s", formatCompact(row.tokens.In), formatCompact(row.tokens.Out)) + case row.tokens.In > 0: + tokStr = formatCompact(row.tokens.In) + case row.tokens.Out > 0: + // Out > 0 with In == 0 is unlikely in practice but keep the symmetry + // so we never silently drop a real token count. + tokStr = "0/" + formatCompact(row.tokens.Out) } preview := row.preview diff --git a/cmd/entire/cli/review/tui_model_test.go b/cmd/entire/cli/review/tui_model_test.go index 5197fd6a2..271ec6a6d 100644 --- a/cmd/entire/cli/review/tui_model_test.go +++ b/cmd/entire/cli/review/tui_model_test.go @@ -78,6 +78,62 @@ func TestTUIModel_AgentEvent_TokensOverwrite(t *testing.T) { } } +// TestTUIModel_DashboardTokenDisplay locks the three display branches for +// the tokens column. Claude's live stream-json carries input tokens +// throughout the run but only surfaces output_tokens on the final result +// envelope, so the parser may emit Tokens{In: N, Out: 0} during the run. +// Rendering that as "N/0" was misleading; the input-only branch renders +// just "N", and the finalized pair renders as "/". +func TestTUIModel_DashboardTokenDisplay(t *testing.T) { + t.Parallel() + + // Values chosen so the rendered string fits inside the 8-char tokens + // column, otherwise the renderer's display-width truncation hides the + // suffix and the substring assertion measures the truncated view. + tests := []struct { + name string + tokens reviewtypes.Tokens + wantSubstr string + denySubstr string + }{ + { + name: "input only renders without slash", + tokens: reviewtypes.Tokens{In: 54000, Out: 0}, + wantSubstr: "54.0k", + denySubstr: "54.0k/", + }, + { + name: "finalized pair renders with slash", + tokens: reviewtypes.Tokens{In: 5400, Out: 696}, + wantSubstr: "5.4k/696", + }, + { + name: "output-only is exposed not silently dropped", + tokens: reviewtypes.Tokens{In: 0, Out: 696}, + wantSubstr: "0/696", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + m := newTestModel([]string{"agent-a"}, func() {}) + m.termWidth = 200 + updated, _ := m.Update(agentEventMsg{agent: "agent-a", ev: tt.tokens}) + m = mustModel(t, updated) + + out := m.dashboardView() + if !strings.Contains(out, tt.wantSubstr) { + t.Errorf("dashboard missing %q, got:\n%s", tt.wantSubstr, out) + } + if tt.denySubstr != "" && strings.Contains(out, tt.denySubstr) { + t.Errorf("dashboard must not contain %q (avoid misleading In/Out split when Out unknown), got:\n%s", + tt.denySubstr, out) + } + }) + } +} + func TestTUIModel_AgentEvent_FinishedSuccess(t *testing.T) { t.Parallel() m := newTestModel([]string{"agent-a"}, func() {}) diff --git a/cmd/entire/cli/review_bridge.go b/cmd/entire/cli/review_bridge.go index 6d33edebe..b07c9e438 100644 --- a/cmd/entire/cli/review_bridge.go +++ b/cmd/entire/cli/review_bridge.go @@ -42,7 +42,6 @@ func buildReviewDeps(attachCmd *cobra.Command) cliReview.Deps { HeadHasReviewCheckpoint: headHasReviewCheckpoint, ReviewCheckpointContext: reviewCheckpointContext, ReviewerFor: launchableReviewerFor, - PromptForAgentFn: nil, // use real PromptForAgent AttachCmd: attachCmd, SynthesisProvider: lazySynthesisProvider{}, } diff --git a/cmd/entire/cli/review_context.go b/cmd/entire/cli/review_context.go index 3a0f1d982..e3e3d7e52 100644 --- a/cmd/entire/cli/review_context.go +++ b/cmd/entire/cli/review_context.go @@ -16,6 +16,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/gitrepo" "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" + cliReview "github.com/entireio/cli/cmd/entire/cli/review" "github.com/entireio/cli/cmd/entire/cli/session" "github.com/entireio/cli/cmd/entire/cli/stringutil" "github.com/entireio/cli/cmd/entire/cli/trailers" @@ -36,10 +37,21 @@ type reviewContextSessionMetadataPromptsReader interface { ReadSessionMetadataAndPrompts(ctx context.Context, checkpointID checkpointid.CheckpointID, sessionIndex int) (*checkpoint.SessionContent, error) } -func reviewCheckpointContext(ctx context.Context, worktreeRoot string, scopeBaseRef string) string { - committed := reviewCommittedCheckpointContext(ctx, worktreeRoot, scopeBaseRef) - inProgress := reviewSessionContextForCurrentHead(ctx, worktreeRoot) - return joinReviewContextSections(committed, inProgress) +// reviewCheckpointContext builds the composed checkpoint/session context for +// the branch review scope and reports how many checkpoints and in-progress +// sessions contributed to it. The cli package owns this helper (checkpoint +// readers can't be imported into the review subpackage without a cycle), and +// the result is plumbed into review.Deps.ReviewCheckpointContext. +func reviewCheckpointContext(ctx context.Context, worktreeRoot string, scopeBaseRef string) cliReview.ContextResult { + committed, ckCount, ckItems := reviewCommittedCheckpointContext(ctx, worktreeRoot, scopeBaseRef) + inProgress, sessionCount, sessItems := reviewSessionContextForCurrentHead(ctx, worktreeRoot) + return cliReview.ContextResult{ + Prompt: joinReviewContextSections(committed, inProgress), + Checkpoints: ckCount, + Sessions: sessionCount, + CheckpointItems: ckItems, + SessionItems: sessItems, + } } // joinReviewContextSections concatenates non-empty review-context sections @@ -60,17 +72,17 @@ func joinReviewContextSections(sections ...string) string { // reviewSessionContext. Kept separate from reviewCheckpointContext so that // in-progress session context is surfaced even when there are no committed // checkpoints in scope (the common case: branch with only uncommitted work). -func reviewSessionContextForCurrentHead(ctx context.Context, worktreeRoot string) string { +func reviewSessionContextForCurrentHead(ctx context.Context, worktreeRoot string) (string, int, []cliReview.SessionScopeItem) { repo, err := gitrepo.OpenPath(worktreeRoot) if err != nil { logging.Debug(ctx, "review session context: open repo", slog.String("error", err.Error())) - return "" + return "", 0, nil } defer repo.Close() head, err := repo.Head() if err != nil { logging.Debug(ctx, "review session context: resolve HEAD", slog.String("error", err.Error())) - return "" + return "", 0, nil } return reviewSessionContext(ctx, worktreeRoot, head.Hash().String()) } @@ -78,10 +90,11 @@ func reviewSessionContextForCurrentHead(ctx context.Context, worktreeRoot string // reviewCommittedCheckpointContext renders the "Checkpoint context from // commits in scope:" section. Previously the body of reviewCheckpointContext; // extracted so the parent can compose it with the in-progress session -// section. -func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, scopeBaseRef string) string { +// section. Returns the rendered block plus the number of unique checkpoints +// rendered (excluding the truncation tail). +func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, scopeBaseRef string) (string, int, []cliReview.CheckpointScopeItem) { if scopeBaseRef == "" { - return "" + return "", 0, nil } messages, commitsTruncated, err := reviewContextCommitMessages(ctx, worktreeRoot, scopeBaseRef, reviewContextMaxCommitScans) @@ -89,21 +102,24 @@ func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, if err != nil { logging.Debug(ctx, "review checkpoint context: list commit messages", slog.String("error", err.Error())) } - return "" + return "", 0, nil } repo, err := gitrepo.OpenPath(worktreeRoot) if err != nil { logging.Debug(ctx, "review checkpoint context: open repo", slog.String("error", err.Error())) - return "" + return "", 0, nil } defer repo.Close() store := checkpoint.NewCommittedReadStore(ctx, repo) var lines []string + var items []cliReview.CheckpointScopeItem seen := map[checkpointid.CheckpointID]bool{} omittedCheckpoints := 0 + renderedCheckpoints := 0 for _, message := range messages { + subject := reviewContextCommitSubject(message) for _, cpID := range trailers.ParseAllCheckpoints(message) { if seen[cpID] { continue @@ -118,6 +134,8 @@ func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, summary, err := checkpoint.ReadCommittedCheckpoint(ctx, store, cpID) if err != nil { lines = append(lines, fmt.Sprintf("- %s: checkpoint metadata unavailable", cpID)) + items = append(items, cliReview.CheckpointScopeItem{ID: shortCheckpointID(cpID), Summary: subject}) + renderedCheckpoints++ continue } detail := reviewCheckpointDetail(ctx, store, cpID, summary) @@ -125,10 +143,12 @@ func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, detail = "no summary or prompt recorded" } lines = append(lines, fmt.Sprintf("- %s: %s", cpID, detail)) + items = append(items, cliReview.CheckpointScopeItem{ID: shortCheckpointID(cpID), Summary: subject}) + renderedCheckpoints++ } } if len(lines) == 0 { - return "" + return "", 0, nil } if omittedCheckpoints > 0 { lines = append(lines, fmt.Sprintf("- ... %d more %s omitted", omittedCheckpoints, reviewContextCheckpointNoun(omittedCheckpoints))) @@ -139,7 +159,7 @@ func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, return "Checkpoint context from commits in scope:\n" + strings.Join(lines, "\n") + - "\n\nUse `entire explain ` for full checkpoint context, or `entire explain --raw-transcript` for raw transcripts." + "\n\nUse `entire explain ` for full checkpoint context, or `entire explain --raw-transcript` for raw transcripts.", renderedCheckpoints, items } // reviewSessionContext returns a "In-progress session context (uncommitted):" @@ -168,19 +188,19 @@ func reviewCommittedCheckpointContext(ctx context.Context, worktreeRoot string, // disk at lifecycle.go:294-310 on every turn for mid-turn commit availability, // before SaveStep copies them onto the shadow branch. Filesystem is canonical // for in-progress reads; the shadow-branch copy is only canonical post-condensation. -func reviewSessionContext(ctx context.Context, worktreeRoot, headSHA string) string { +func reviewSessionContext(ctx context.Context, worktreeRoot, headSHA string) (string, int, []cliReview.SessionScopeItem) { if worktreeRoot == "" || headSHA == "" { - return "" + return "", 0, nil } store, err := session.NewStateStore(ctx) if err != nil { logging.Debug(ctx, "review session context: open state store", slog.String("error", err.Error())) - return "" + return "", 0, nil } states, err := store.List(ctx) if err != nil { logging.Debug(ctx, "review session context: list session states", slog.String("error", err.Error())) - return "" + return "", 0, nil } // Canonicalise the current worktree path once. State files written by @@ -191,6 +211,7 @@ func reviewSessionContext(ctx context.Context, worktreeRoot, headSHA string) str worktreeCanon := canonicalisePath(worktreeRoot) var lines []string + var items []cliReview.SessionScopeItem for _, st := range states { if st == nil { continue @@ -207,16 +228,17 @@ func reviewSessionContext(ctx context.Context, worktreeRoot, headSHA string) str if st.Kind == session.KindAgentReview { continue } - line := formatReviewSessionLine(worktreeRoot, st) + line, item := formatReviewSessionLine(worktreeRoot, st) if line == "" { continue } lines = append(lines, line) + items = append(items, item) } if len(lines) == 0 { - return "" + return "", 0, nil } - return "In-progress session context (uncommitted):\n" + strings.Join(lines, "\n") + return "In-progress session context (uncommitted):\n" + strings.Join(lines, "\n"), len(lines), items } // canonicalisePath returns the symlink-resolved absolute form of p. Falls @@ -232,17 +254,18 @@ func canonicalisePath(p string) string { return p } -// formatReviewSessionLine renders one entry of the in-progress section. -// Returns "" when the session has no prompt content to report. -func formatReviewSessionLine(worktreeRoot string, st *session.State) string { +// formatReviewSessionLine renders one entry of the in-progress section and the +// structured banner item for the same session. Returns ("", zero) when the +// session has no prompt content to report. +func formatReviewSessionLine(worktreeRoot string, st *session.State) (string, cliReview.SessionScopeItem) { promptPath := filepath.Join(worktreeRoot, paths.SessionMetadataDirFromSessionID(st.SessionID), paths.PromptFileName) raw, err := os.ReadFile(promptPath) //nolint:gosec // path constructed from validated session ID + fixed constants if err != nil { - return "" + return "", cliReview.SessionScopeItem{} } promptText := reviewPromptText(string(raw)) if promptText == "" { - return "" + return "", cliReview.SessionScopeItem{} } short := st.SessionID @@ -263,7 +286,7 @@ func formatReviewSessionLine(worktreeRoot string, st *session.State) string { parts = append(parts, fmt.Sprintf("(touched: %d %s)", n, fileWord)) } parts = append(parts, "prompt: "+promptText) - return strings.Join(parts, " ") + return strings.Join(parts, " "), cliReview.SessionScopeItem{ID: short, Agent: agentName} } func reviewCheckpointDetail( @@ -399,6 +422,32 @@ func reviewContextCheckpointNoun(count int) string { return "checkpoints" } +// shortCheckpointID returns the first 8 hex chars of a checkpoint ID for the +// scope banner (full IDs are 12 chars; 8 is enough to recognise and to feed +// `entire explain`). +func shortCheckpointID(cpID checkpointid.CheckpointID) string { + s := string(cpID) + if len(s) > 8 { + return s[:8] + } + return s +} + +// reviewContextCommitSubject extracts the trimmed first line of a commit +// message, capped for one-line display in the scope banner. +func reviewContextCommitSubject(message string) string { + subject := message + if i := strings.IndexByte(subject, '\n'); i >= 0 { + subject = subject[:i] + } + subject = strings.TrimSpace(subject) + const maxSubjectLen = 72 + if runes := []rune(subject); len(runes) > maxSubjectLen { + return strings.TrimSpace(string(runes[:maxSubjectLen-1])) + "…" + } + return subject +} + func reviewContextCommitMessages(ctx context.Context, repoRoot string, scopeBaseRef string, maxCommits int) ([]string, bool, error) { if maxCommits <= 0 { return nil, false, nil diff --git a/cmd/entire/cli/review_context_test.go b/cmd/entire/cli/review_context_test.go index 470375fde..d95dd688d 100644 --- a/cmd/entire/cli/review_context_test.go +++ b/cmd/entire/cli/review_context_test.go @@ -52,7 +52,8 @@ func TestReviewCheckpointContext_IncludesSummaryAndPromptFallback(t *testing.T) }) commitReviewContextChange(t, repoRoot, "prompt.go", "prompt\n", "prompt change", "Entire-Checkpoint: "+promptCheckpointID) - got := reviewCheckpointContext(context.Background(), repoRoot, "master") + result := reviewCheckpointContext(context.Background(), repoRoot, "master") + got := result.Prompt for _, want := range []string{ "Checkpoint context from commits in scope:", summaryCheckpointID, @@ -75,6 +76,9 @@ func TestReviewCheckpointContext_IncludesSummaryAndPromptFallback(t *testing.T) t.Fatalf("review checkpoint context contains %q:\n%s", unwanted, got) } } + if result.Checkpoints != 2 { + t.Errorf("Checkpoints = %d, want 2", result.Checkpoints) + } } // Review committed-context reads resolve against the v1 custom ref when the @@ -94,13 +98,13 @@ func TestReviewCheckpointContext_ReadsV1CustomRefWhenEnabled(t *testing.T) { const wantDetail = "summary: custom-ref-only checkpoint; read via custom ref" // Mirror disabled: reads hit the v1 branch, which no longer holds the checkpoint. - if got := reviewCheckpointContext(context.Background(), repoRoot, "master"); strings.Contains(got, wantDetail) { + if got := reviewCheckpointContext(context.Background(), repoRoot, "master").Prompt; strings.Contains(got, wantDetail) { t.Fatalf("checkpoint detail leaked with mirror disabled:\n%s", got) } // Mirror enabled: reads hit the custom ref. enableV1CustomRefMirror(t, repoRoot) - if got := reviewCheckpointContext(context.Background(), repoRoot, "master"); !strings.Contains(got, wantDetail) { + if got := reviewCheckpointContext(context.Background(), repoRoot, "master").Prompt; !strings.Contains(got, wantDetail) { t.Fatalf("checkpoint detail missing with mirror enabled:\n%s", got) } } @@ -133,7 +137,8 @@ func TestReviewCheckpointContext_CapsCheckpointLines(t *testing.T) { ) } - got := reviewCheckpointContext(context.Background(), repoRoot, "master") + result := reviewCheckpointContext(context.Background(), repoRoot, "master") + got := result.Prompt if count := strings.Count(got, "summary: checkpoint summary"); count != reviewContextMaxCheckpoints { t.Fatalf("checkpoint context summary count = %d, want %d:\n%s", count, reviewContextMaxCheckpoints, got) } @@ -143,6 +148,10 @@ func TestReviewCheckpointContext_CapsCheckpointLines(t *testing.T) { if !strings.Contains(got, "1 more checkpoint omitted") { t.Fatalf("checkpoint context missing truncation notice:\n%s", got) } + if result.Checkpoints != reviewContextMaxCheckpoints { + t.Errorf("Checkpoints = %d, want %d (capped, not counting omitted tail)", + result.Checkpoints, reviewContextMaxCheckpoints) + } } func TestReviewCheckpointDetail_ReadsSessionMetadataOnceForPromptFallback(t *testing.T) { @@ -588,7 +597,7 @@ func TestReviewSessionContext_IncludesActiveSessionWithLatestPrompt(t *testing.T writeReviewContextSessionPrompt(t, repoRoot, sessionID, "Implement the auth feature.\n\n---\n\nAlso handle the edge case for refresh tokens.") - got := reviewSessionContext(context.Background(), repoRoot, headSHA) + got, count, _ := reviewSessionContext(context.Background(), repoRoot, headSHA) for _, want := range []string{ "In-progress session context (uncommitted):", sessionID[:8], @@ -604,6 +613,9 @@ func TestReviewSessionContext_IncludesActiveSessionWithLatestPrompt(t *testing.T t.Errorf("expected section to contain %q, got:\n%s", want, got) } } + if count != 1 { + t.Errorf("session count = %d, want 1", count) + } } // TestReviewSessionContext_SkipsSessionsOutsideScope verifies the four @@ -635,10 +647,13 @@ func TestReviewSessionContext_SkipsSessionsOutsideScope(t *testing.T) { writeReviewContextSessionPrompt(t, repoRoot, c.state.SessionID, "LEAKED-"+c.name) } - got := reviewSessionContext(context.Background(), repoRoot, headSHA) + got, count, _ := reviewSessionContext(context.Background(), repoRoot, headSHA) if got != "" { t.Fatalf("expected empty section when no in-scope sessions match; got:\n%s", got) } + if count != 0 { + t.Errorf("session count = %d, want 0", count) + } for _, c := range cases { if strings.Contains(got, "LEAKED-"+c.name) { t.Errorf("%s session leaked into output", c.name) diff --git a/docs/architecture/review-command.md b/docs/architecture/review-command.md index 3fd5988ed..7061ce815 100644 --- a/docs/architecture/review-command.md +++ b/docs/architecture/review-command.md @@ -5,35 +5,75 @@ ## 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 # Normal run: dispatch to configured Reviewers +entire review setup # Two-step role + skills picker (canonical configuration entrypoint) +entire review --edit # Alias for `entire review setup` +entire review --prompt "" # Per-run extra context (bypasses inline ask) +entire review --reviewers # One-off override: agents to run as Reviewers (comma-separated) +entire review --fixer # One-off override: agent to use as Fixer +entire review --agent # Force a specific configured Reviewer +entire review --base # Scope against instead of mainline +entire review fix [session-id] # Apply review findings via the configured Fixer +entire review fix --all # Apply all findings without selectors +entire review fix --prompt "" # Per-run extra context for the fix run +entire review --findings # Browse local review findings (read-only) 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. +`entire review` no longer auto-opens a picker. Resolution is role-driven: + +1. `--reviewers` / `--fixer` flags → one-off override (NOT persisted). +2. Saved roles (`ReviewersOf(s)` / `FixerOf(s)`) → use them. +3. Interactive, no saved roles → print `Run: entire review setup`; exit. +4. Non-interactive, no saved roles, invoking agent detected (`CLAUDE_CODE`, `CODEX`, `GEMINI_CLI`, `COPILOT_CLI`, `PI_CODING_AGENT`) → use that agent as Role `Both`; print a one-line note (NOT persisted). +5. Non-interactive, no roles, no invoker → hard error pointing at setup. + +When two or more agents have `RoleReviewer` (or `RoleBoth`), all of them run concurrently via `RunMulti`. There is no spawn-time multi-select picker — `entire review setup` is the only place where reviewer membership is decided. + +`ENTIRE_REVIEW_SESSION` in env triggers a recursion guard on both `entire review` and `entire review fix` — refuses to start a nested review (prevents reviewer-prompted loops). + +## Pre-launch staging + post-review Fix UX + +- **Staging view** (`stagePerRunContext` in `post_review.go`, replaces the old `askPerRunPrompt`): after scope detection and BEFORE fan-out, the optional per-run prompt is collected together with the scope summary in one styled huh form — the "Add context for this run?" input on top, and a Note underneath listing the scope line plus the **itemised checkpoints (short id + commit summary) and in-progress sessions (short id + agent) in scope**. The user sees exactly what's about to be reviewed before agents launch. `--prompt` or non-interactive: the same summary is printed plainly and the ask is skipped. (`ContextResult` carries structured `CheckpointItems` / `SessionItems`; backticks are sanitised for huh's markdown renderer.) +- After the manifest is written, `RunPostReviewFixPrompt` (in `post_review.go`) drives the next step: + - `FixAfterReview == "always"` → launch the Fixer with `--all`, skip the prompt. + - Non-TTY → print the `Run:` footer (`entire review fix` / `--all` / `entire review findings`). + - TTY → prompt `[Y]es · [s]elect fixes · [n]o · [A]lways`. `[A]` persists `FixAfterReview = "always"`. Gated on a resolvable Fixer (`FixerOf(s)`); no fixer → footer hint instead. +- **`[s]` / `entire review fix` selection** (`fix.go`): for the interactive multi-source case, source-selection and findings-selection are ONE navigable huh form (two groups; findings recomputed from the selected sources via `OptionsFunc`), so the user can `shift+tab` back to change the source — e.g. to the **Aggregate** source — without restarting. Single-source / `--all` / non-interactive keep the linear path. +- `userExplicitlyOmittedFixer` (true when `--reviewers` was used WITHOUT `--fixer`) swaps the no-fixer footer from "Run: entire review setup" to a one-off `--fixer ` hint. ## Settings Schema -Review skills are configured per-agent in `.entire/settings.json`: +Review configuration is role-first; each entry in `.entire/settings.json` declares the agent's `role` plus per-agent `skills`, `prompt`, and optional per-spawn `model` / `reasoning_effort`: ```json { "review": { - "claude-code": {"skills": ["/pr-review-toolkit:review-pr"], "prompt": "Be thorough."}, - "codex": {"skills": ["/codex:adversarial-review"]} - } + "claude-code": {"role": "reviewer", "skills": ["/pr-review-toolkit:review-pr"], "prompt": "Be thorough."}, + "codex": {"role": "both", "skills": ["$code-reviewer"], "model": "gpt-5-mini", "reasoning_effort": "low"} + }, + "fix_after_review": "always" } ``` -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`. +Roles: `reviewer` (runs on `entire review`), `fixer` (runs on `entire review fix`), `both` (counts as the Fixer AND reviews), `skip` (configured but unused). The empty role on legacy entries is upgraded to `reviewer` in-memory by `settings.MigrateLegacyRoles` on load (the upgrade is idempotent and only persists on the next settings write). + +**Skill invocation form is per-agent.** Claude skills are slash-prefixed (`/review`, `/plugin:name`); **codex skills are dollar-prefixed (`$code-reviewer`, `$plugin:name`)** — the literal token a user types to invoke a skill in that CLI. Discovery emits each agent's `Name` already in its own form (see Skill Discovery below). + +**`model` / `reasoning_effort`** (`ReviewConfig.Model` / `ReviewConfig.ReasoningEffort`) are optional per-spawn overrides threaded into `reviewtypes.RunConfig` by `applyReviewConfig` and emitted by the codex reviewer as `-m ` / `-c model_reasoning_effort=`. Empty values defer to the agent's own config. Agents without these knobs ignore them. + +**`entire review setup` defaults new agents to `skip`** (reviewing is opt-in): every hook-installed agent is listed, but the user explicitly picks Reviewer/Both; a "≥1 reviewer" guard forces an explicit choice. Saved roles are never silently rewritten. + +**`fix_after_review`** lives on both `EntireSettings` and `ClonePreferences`. Values: `""` (unset — default, behaves as Ask and, clone-side, means "no override"), `"ask"` (explicit Ask — the non-empty token a clone writes to override a project-level `"always"` back to Ask), `"always"` (auto-run the Fixer). `settings.Load` validates `role` and `fix_after_review` after merge+migration, rejecting hand-edited unknown values. + +Settings field: `EntireSettings.Review` and `EntireSettings.FixAfterReview` in `cmd/entire/cli/settings/settings.go`. `RoleReviewer` / `RoleFixer` / `RoleBoth` / `RoleSkip` are exported constants. ## 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`). +1. `entire review` selects reviewers via `ReviewersOf(s)`, 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. @@ -41,6 +81,8 @@ The key is the agent name. The value is a `ReviewConfig` with `skills` (skill in 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). +Env adoption is generalized: `tryAdoptEnv` in `cmd/entire/cli/lifecycle.go` runs the shared env-present + agent-match + STARTING_SHA gate, and `adoptReviewEnv` / `adoptInvestigateEnv` supply the kind-specific `envAdoptionSpec` (env keys + `apply`). The session env value is always `"1"`. + ## Checkpoint Metadata Review metadata is stored at two levels on `entire/checkpoints/v1`: @@ -56,7 +98,7 @@ Review metadata is stored at two levels on `entire/checkpoints/v1`: - **`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. -- **Scope detection** (`cmd/entire/cli/review/scope.go`): `detectScopeBaseRef` returns the first existing ref from the fallback chain `origin/HEAD → origin/main → origin/master → main → master`. Overridable per-invocation via `--base ` (validated through go-git's `ResolveRevision`). Banner output: "Reviewing feat/X vs main: 3 commits, 7 files changed, 2 uncommitted". +- **Scope detection** (`cmd/entire/cli/review/scope.go`): `detectScopeBaseRef` returns the first existing ref from the fallback chain `origin/HEAD → origin/main → origin/master → main → master`. Overridable per-invocation via `--base ` (validated through go-git's `ResolveRevision`). `detectScope` (in `cmd.go`) returns the base ref plus a human-facing banner string ("Reviewing feat/X vs main: 3 commits, 7 files changed, 2 uncommitted") for the staging step to fold into the styled prompt. ## Multi-Agent UI @@ -65,12 +107,27 @@ When `RunMulti` is dispatched in a TTY, the sink slice is `[TUISink, DumpSink, S - **`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. - **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. +- **Output styling**: `DumpSink` (per-agent findings) and `SynthesisSink` (aggregate verdict) render through `mdrender.RenderMutedForWriter` — a low-chroma palette (`mdrender.mutedStyles`) that keeps coloured+bold headings for structure but neutralises the high-frequency inline noise (inline-code highlight blocks, coloured bullets/links) that makes markdown-dense findings read as a wall of colour. The shared palette (dispatch / explain / status) is untouched. The live TUI dashboard adds no per-cell colour; only its column header is dimmed (`reviewHeaderStyle`, `Faint`). + +## Skill Discovery (Claude Code + Codex) + +The generic SKILL.md/markdown scanners, version dedupe (`PickLatestVersion`), and frontmatter parsing live in the shared `skilldiscovery` package (`scan.go`), parameterized by an **invocation-form builder** (`SlashForm` for Claude, `DollarForm` for codex) — the only per-agent difference. Each agent's `DiscoverReviewSkills` supplies its roots + form; discovery emits `DiscoveredSkill.Name` already in that agent's invocation form so the shared prompt composer joins it verbatim. -## Skill Discovery (Claude Code) +- **Claude Code** (`claudecode/discovery.go`) walks: plugin cache (`~/.claude/plugins/cache////{skills,commands,agents}`), user skills (`~/.claude/skills`), user commands/agents (`~/.claude/commands`, `~/.claude/agents`). Emits `/name` / `/plugin:name`. +- **Codex** (`codex/discovery.go`) walks: `~/.codex/skills/`, plugin cache `~/.codex/plugins/cache////skills/`, and `~/.codex/superpowers/skills/`. Emits `$name` / `$plugin:name`. Codex has **no curated built-ins** in `skilldiscovery/registry.go` (its review skills are discovered on disk, not bundled in the binary; `/review` is a TUI-only built-in that doesn't fire in `codex exec`). -`DiscoverReviewSkills` (`cmd/entire/cli/agent/claudecode/discovery.go`) walks three roots: plugin cache (`~/.claude/plugins/cache////{skills,commands,agents}`), user skills (`~/.claude/skills`), user commands/agents (`~/.claude/commands`, `~/.claude/agents`). +For the plugin cache, `PickLatestVersion` picks ONE version directory per plugin: highest valid semver wins; if no entries parse as semver, the lexicographic max is picked (handles the `unknown` sentinel some plugins ship, and codex's opaque content-hash version dirs). Without this, multiple installed versions of a plugin produced duplicate skill entries in the picker and prompt. -For the plugin cache, `pickLatestVersion` picks ONE version directory per plugin: highest valid semver wins; if no entries parse as semver, the lexicographic max is picked (handles the `unknown` sentinel some plugins ship). Without this, multiple installed versions of a plugin produced duplicate skill entries in the picker and prompt. +When an agent has no saved skills and no per-run skill flags, `seedDefaultSkills` (`setup.go`) supplies a default: the agent's curated built-ins, or — for an agent with none (codex) — on-disk discovered skills whose name signals a review skill (contains "review"), so codex still invokes e.g. `$code-reviewer` instead of a generic scope-only prompt. Used by the invoker-only fallback and the `--reviewers` / `--fixer` override. + +## Codex specifics (exec fires no hooks) + +`codex exec` fires **no lifecycle hooks**, which drives several codex-only behaviors: + +- **Skill invocation**: configured skills are passed to codex **verbatim** (no paraphrase) — codex's skill system injects its installed-skill catalog into every exec session and loads the matching `SKILL.md`. Native `codex exec review` is intentionally NOT used (it rejects a prompt under a scope flag and can't carry Entire's scope/per-run/checkpoint context). `codex/reviewer.go` argv: `codex exec --skip-git-repo-check --json [-m ] [-c model_reasoning_effort=] -`. +- **Live tokens**: `codex exec --json` carries `usage` only on the terminal `turn.completed`, and a review is a single turn — so the TUI would show no tokens until the end. `codex/review_tokens.go` resolves codex's rollout transcript by `thread_id` (from the `thread.started` envelope), **tails it** (via `os.File.Read` — bufio is sticky on EOF), and emits cumulative `Tokens` per `token_count` event. The parser launches the tailer on `thread.started` and uses a `stop` channel + `WaitGroup` so it can't send on a closed channel. +- **Fix manifest sourcing**: because no hook tags a codex review session, `buildLocalReviewManifestFromSummary` includes an agent as a fix source when it has **live output OR a matched session** (not session-match-only) — codex's review narrative is captured in `run.Buffer`. Without this, codex was silently dropped from `entire review fix`. The completion footer prints for a session-less codex-only manifest too; `entire review fix` with no handle resolves to the most-recent run. +- **Skill verification**: `VerifyConfiguredSkillsInstalled` is **advisory for codex** (logs, doesn't block) — codex resolves skills by loose description match, and legacy saved configs may carry pre-`$`form invocations. ## Anti-Features (do NOT recreate) @@ -85,21 +142,40 @@ The redesign eliminated several constructs from the prior implementation. None s - `Launcher` + `HeadlessLauncher` as separate interfaces (single `AgentReviewer`) - Codex chrome-line filtering or any agent-specific stdout post-processing in shared multi-agent code (per-agent parsers own their format; shared code only sees `Event` variants) - `sync.Once`-guarded onCancel + parallel `signal.Notify` goroutine (single cancel from start) +- Spawn-time multi-select agent picker / `PickAgents` / `PickedAgents` / `multipicker.go` (roles answer "who reviews" up front via `entire review setup`) +- Spawn-time "Choose fix agent" prompt (`FixerOf(s)` is the single source of truth) +- First-run auto-picker on bare `entire review` (replaced by `Run: entire review setup` pointer + invoker-only non-interactive fallback) +- `ConfirmFirstRunSetup` / `RunReviewConfigPicker` (use `RunSetup` instead) +- `MultiPickerFn` on `review.Deps` and the runtime multi-agent picker dispatch fork + +NOTE — the unified source⇄findings picker in `entire review fix` (`fix.go`) is **not** a reintroduction of the spawn-time agent picker: it selects which findings to *fix* after a review, not which agents *review*. Reviewer membership is still decided only by roles / `entire review setup`. ## 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/cmd.go` — `NewCommand()`, `runReview` dispatch fork, `detectScope`, `composeMultiAgentSinks`, post-review fix prompt wiring +- `cmd/entire/cli/review/setup.go` — `entire review setup` two-step picker + `DetectInvokingAgent` / `invokerOnlyReviewConfig` / `seedDefaultSkills` for the non-interactive fallback +- `cmd/entire/cli/review/picker.go` — `PromptForAgent` single-select, `ComputeEligibleConfigured`, `SelectReviewAgent`, `VerifyConfiguredSkillsInstalled`, `SaveReviewConfig` +- `cmd/entire/cli/review/flags.go` — `resolveRolesFromFlags` + `mergeFlagOverrideWithSavedSkills` for `--reviewers` / `--fixer` one-off overrides +- `cmd/entire/cli/review/inline_prompt.go` — `ReadSingleKey` helper used by the `[Y]/[s]/[n]/[A]` post-review prompt +- `cmd/entire/cli/review/post_review.go` — `stagePerRunContext` (pre-launch staging) + `RunPostReviewFixPrompt` + `launchFixFromManifest` + `printFindingsFooter` +- `cmd/entire/cli/review/banner.go` — `formatContextBanner` (itemised checkpoints/sessions scope) + `reviewerSkillSuffix` (setup banner skill counts) +- `cmd/entire/cli/review/fix.go` — `entire review fix` subcommand, `runReviewFix`, `selectReviewFixInteractive`, `extractSourceFindings`/`reviewFindingTitle`, `resolveReviewFixAgent` (delegates to `FixerOf(s)`); launches via `agentlaunch.LaunchFixAgent` +- `cmd/entire/cli/review/manifest.go` — local fix manifest; `buildLocalReviewManifestFromSummary` includes live-output agents (codex) even without a tagged review session +- `cmd/entire/cli/review/roles.go` — `NormalizeRoles`, `ReviewersOf`, `FixerOf`, `MigrateLegacyRoles` thin-wrap - `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/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/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/types/{reviewer,sink,template}.go` — interface contracts +- `cmd/entire/cli/review/env.go` — `ENTIRE_REVIEW_*` constants + `EncodeSkills`/`DecodeSkills` + `AppendReviewEnv`; the recursion guard reads `EnvSession` here +- `cmd/entire/cli/agent/{claudecode,codex,geminicli}/reviewer.go` — per-agent `AgentReviewer` implementations +- `cmd/entire/cli/agent/skilldiscovery/scan.go` — shared SKILL.md scanners, `PickLatestVersion`, frontmatter parse, `SlashForm`/`DollarForm` invocation builders +- `cmd/entire/cli/agent/claudecode/discovery.go` + `cmd/entire/cli/agent/codex/discovery.go` — per-agent skill discovery (slash / `$name` form) over each agent's roots +- `cmd/entire/cli/agent/codex/review_tokens.go` — rollout-transcript token tailing for live codex tokens +- `cmd/entire/cli/mdrender/mdrender.go` — `RenderMuted` / `RenderMutedForWriter` low-chroma palette for dense review output +- `cmd/entire/cli/review_context.go` — `reviewCheckpointContext` builds scope context (agent prompt text + structured `ContextResult` checkpoint/session items for the staging banner) +- `cmd/entire/cli/lifecycle.go` — `tryAdoptEnv` + `adoptReviewEnv` / `adoptInvestigateEnv` read `ENTIRE_REVIEW_*` / `ENTIRE_INVESTIGATE_*` 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/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.Review`, `EntireSettings.FixAfterReview`, `Role` / `RoleReviewer` / `RoleFixer` / `RoleBoth` / `RoleSkip`