From 7ce65b97df70a4fa4fd4577a605cb6d119eff974 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 19 May 2026 12:11:23 -0400 Subject: [PATCH 1/3] trigger 1 Entire-Checkpoint: 0c902d094d5c --- t1.txt | 1 + 1 file changed, 1 insertion(+) create mode 100644 t1.txt diff --git a/t1.txt b/t1.txt new file mode 100644 index 0000000000..6f670c0fb5 --- /dev/null +++ b/t1.txt @@ -0,0 +1 @@ +test 1 From 87412e6838f6b9bd86fea0c04e45967ece011a72 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 20 May 2026 15:45:05 -0400 Subject: [PATCH 2/3] feat(review): introduce per-agent Role + FixAfterReview settings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Role enum (Reviewer/Fixer/Both/Skip) to ReviewConfig and a FixAfterReview mode to EntireSettings + ClonePreferences. Provides NormalizeRoles / MigrateLegacyRoles / ReviewersOf / FixerOf helpers and runs an idempotent in-memory migration on settings.Load that derives roles from the legacy ReviewFixAgent field. The schema is persisted lazily — the next code path that writes settings (e.g., `entire review setup`) writes the migrated form. Foundation for the entire review redesign — no behavior change yet for users running entire review. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 5ecddc347c0f --- cmd/entire/cli/review/roles.go | 84 +++++++++ cmd/entire/cli/review/roles_test.go | 106 ++++++++++++ cmd/entire/cli/settings/migration.go | 63 +++++++ cmd/entire/cli/settings/migration_test.go | 199 ++++++++++++++++++++++ cmd/entire/cli/settings/settings.go | 67 ++++++++ cmd/entire/cli/settings/settings_test.go | 44 +++++ 6 files changed, 563 insertions(+) create mode 100644 cmd/entire/cli/review/roles.go create mode 100644 cmd/entire/cli/review/roles_test.go create mode 100644 cmd/entire/cli/settings/migration.go create mode 100644 cmd/entire/cli/settings/migration_test.go diff --git a/cmd/entire/cli/review/roles.go b/cmd/entire/cli/review/roles.go new file mode 100644 index 0000000000..34c8b42b8e --- /dev/null +++ b/cmd/entire/cli/review/roles.go @@ -0,0 +1,84 @@ +// Package review — see env.go for package-level rationale. +// +// roles.go provides role helpers used by runReview and the setup +// subcommand. The migration body lives in the settings package +// (MigrateLegacyRoles); this file thin-wraps it so review-package +// tests can exercise everything in one place. +package review + +import ( + "sort" + + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// NormalizeRoles enforces the at-most-one-fixer invariant. Empty roles +// upgrade to Reviewer. Duplicates: alphabetical winner keeps its role; +// the rest are demoted to Reviewer. Returns a new map. +func NormalizeRoles(in map[string]settings.ReviewConfig) map[string]settings.ReviewConfig { + out := make(map[string]settings.ReviewConfig, len(in)) + if len(in) == 0 { + return out + } + var fixerCandidates []string + for name, cfg := range in { + if cfg.Role == "" { + cfg.Role = settings.RoleReviewer + } + if cfg.Role.IsFixer() { + fixerCandidates = append(fixerCandidates, name) + } + out[name] = cfg + } + if len(fixerCandidates) > 1 { + sort.Strings(fixerCandidates) + for _, loser := range fixerCandidates[1:] { + cfg := out[loser] + cfg.Role = settings.RoleReviewer + out[loser] = cfg + } + } + return out +} + +// MigrateLegacyRoles is a thin wrapper around settings.MigrateLegacyRoles +// to keep the review-package test surface cohesive. +func MigrateLegacyRoles(s *settings.EntireSettings) bool { + return settings.MigrateLegacyRoles(s) +} + +// ReviewersOf returns the sorted set of agent names with RoleReviewer +// or RoleBoth. +func ReviewersOf(s *settings.EntireSettings) []string { + if s == nil { + return nil + } + var out []string + for name, cfg := range s.Review { + if cfg.Role.IsReviewer() { + out = append(out, name) + } + } + sort.Strings(out) + return out +} + +// FixerOf returns the agent name with RoleFixer or RoleBoth. Returns "" +// when no Fixer is configured. Assumes NormalizeRoles has been called; +// in the duplicate-fixer case returns the alphabetically-first. +func FixerOf(s *settings.EntireSettings) string { + if s == nil { + return "" + } + var candidates []string + for name, cfg := range s.Review { + if cfg.Role.IsFixer() { + candidates = append(candidates, name) + } + } + if len(candidates) == 0 { + return "" + } + sort.Strings(candidates) + return candidates[0] +} diff --git a/cmd/entire/cli/review/roles_test.go b/cmd/entire/cli/review/roles_test.go new file mode 100644 index 0000000000..547f7531e7 --- /dev/null +++ b/cmd/entire/cli/review/roles_test.go @@ -0,0 +1,106 @@ +package review_test + +import ( + "reflect" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/review" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +func TestNormalizeRoles_DefaultsEmptyToReviewer(t *testing.T) { + t.Parallel() + in := map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review"}}, + } + out := review.NormalizeRoles(in) + if out["claude-code"].Role != settings.RoleReviewer { + t.Errorf("expected RoleReviewer, got %q", out["claude-code"].Role) + } +} + +func TestNormalizeRoles_AtMostOneFixer(t *testing.T) { + t.Parallel() + in := map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleFixer}, + "codex": {Role: settings.RoleFixer}, + "gemini": {Role: settings.RoleBoth}, + } + out := review.NormalizeRoles(in) + fixers := 0 + for _, c := range out { + if c.Role.IsFixer() { + fixers++ + } + } + if fixers != 1 { + t.Errorf("expected exactly 1 fixer, got %d", fixers) + } + if !out["claude-code"].Role.IsFixer() { + t.Errorf("expected claude-code (alphabetical first) to keep fixer role, got %+v", out) + } +} + +func TestNormalizeRoles_EmptyInputReturnsFreshEmptyMap(t *testing.T) { + t.Parallel() + in := map[string]settings.ReviewConfig{} + out := review.NormalizeRoles(in) + if out == nil { + t.Fatal("expected non-nil empty map, got nil") + } + // Mutating `out` must not mutate `in`. + out["x"] = settings.ReviewConfig{Role: settings.RoleReviewer} + if _, leaked := in["x"]; leaked { + t.Errorf("NormalizeRoles returned the input map; mutations leaked") + } +} + +func TestNormalizeRoles_SkipPreserved(t *testing.T) { + t.Parallel() + in := map[string]settings.ReviewConfig{"gemini": {Role: settings.RoleSkip}} + out := review.NormalizeRoles(in) + if out["gemini"].Role != settings.RoleSkip { + t.Errorf("Skip role should be preserved, got %q", out["gemini"].Role) + } +} + +func TestReviewersOf_FiltersByRole(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + "codex": {Role: settings.RoleFixer}, + "gemini": {Role: settings.RoleBoth}, + }, + } + got := review.ReviewersOf(s) + want := []string{"claude-code", "gemini"} + if !reflect.DeepEqual(got, want) { + t.Errorf("ReviewersOf = %v, want %v", got, want) + } +} + +func TestFixerOf_AlphabeticalWinnerWhenMultiple(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "codex": {Role: settings.RoleFixer}, + "gemini": {Role: settings.RoleBoth}, + }, + } + if got := review.FixerOf(s); got != "codex" { + t.Errorf("FixerOf = %q, want codex (alphabetical first)", got) + } +} + +func TestFixerOf_EmptyWhenNoFixer(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + }, + } + if got := review.FixerOf(s); got != "" { + t.Errorf("FixerOf = %q, want empty", got) + } +} diff --git a/cmd/entire/cli/settings/migration.go b/cmd/entire/cli/settings/migration.go new file mode 100644 index 0000000000..85d78c14bc --- /dev/null +++ b/cmd/entire/cli/settings/migration.go @@ -0,0 +1,63 @@ +package settings + +import "sort" + +// MigrateLegacyRoles upgrades pre-role ReviewConfig entries in-place. +// Idempotent: re-running on an already-migrated map is a no-op. +// Returns true if any entry was modified. +// +// Rules: +// - Entries with non-empty Role are left untouched. +// - The agent named in s.ReviewFixAgent becomes: +// - RoleBoth if it has Skills or Prompt configured +// - RoleFixer otherwise +// - All other entries with empty Role become RoleReviewer. +// - At-most-one-fixer is enforced (alphabetical winner if duplicates exist). +func MigrateLegacyRoles(s *EntireSettings) bool { + if s == nil || len(s.Review) == 0 { + return false + } + anyEmpty := false + for _, cfg := range s.Review { + if cfg.Role == "" { + anyEmpty = true + break + } + } + if !anyEmpty { + return false + } + + for name, cfg := range s.Review { + if cfg.Role != "" { + continue + } + if name == s.ReviewFixAgent { + if len(cfg.Skills) > 0 || cfg.Prompt != "" { + cfg.Role = RoleBoth + } else { + cfg.Role = RoleFixer + } + } else { + cfg.Role = RoleReviewer + } + s.Review[name] = cfg + } + + // Enforce at-most-one-fixer (alphabetical winner). + var fixers []string + for name, cfg := range s.Review { + if cfg.Role.IsFixer() { + fixers = append(fixers, name) + } + } + if len(fixers) > 1 { + sort.Strings(fixers) + for _, loser := range fixers[1:] { + cfg := s.Review[loser] + cfg.Role = RoleReviewer + s.Review[loser] = cfg + } + } + return true +} diff --git a/cmd/entire/cli/settings/migration_test.go b/cmd/entire/cli/settings/migration_test.go new file mode 100644 index 0000000000..5d5b461eff --- /dev/null +++ b/cmd/entire/cli/settings/migration_test.go @@ -0,0 +1,199 @@ +package settings_test + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +func TestLoad_MigratesLegacyRolesInMemory(t *testing.T) { + // Not t.Parallel(): t.Chdir cannot be combined with parallel tests. + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + settingsPath := filepath.Join(dir, ".entire", "settings.json") + if err := os.MkdirAll(filepath.Dir(settingsPath), 0o755); err != nil { + t.Fatal(err) + } + initial := []byte(`{ + "review": { + "claude-code": {"skills": ["/review"]}, + "codex": {"skills": ["/review"]} + }, + "review_fix_agent": "codex" + }`) + if err := os.WriteFile(settingsPath, initial, 0o644); err != nil { + t.Fatal(err) + } + + s, err := settings.Load(context.Background()) + if err != nil { + t.Fatalf("load: %v", err) + } + if s.Review["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q, want reviewer", s.Review["claude-code"].Role) + } + if s.Review["codex"].Role != settings.RoleBoth { + t.Errorf("codex role = %q, want both (skills + was fix agent)", + s.Review["codex"].Role) + } + + // CRITICAL: Load must NOT persist the migration. The file on disk + // should still be the original legacy form, so re-loading must yield + // the same migrated result deterministically (idempotent). + raw, err := os.ReadFile(settingsPath) + if err != nil { + t.Fatal(err) + } + var disk map[string]any + if err := json.Unmarshal(raw, &disk); err != nil { + t.Fatal(err) + } + review, ok := disk["review"].(map[string]any) + if !ok { + t.Fatalf("review key missing or wrong type in on-disk JSON: %v", disk["review"]) + } + cc, ok := review["claude-code"].(map[string]any) + if !ok { + t.Fatalf("claude-code entry missing or wrong type: %v", review["claude-code"]) + } + if _, hasRole := cc["role"]; hasRole { + t.Errorf("Load auto-persisted role field; expected on-disk schema unchanged") + } +} + +func TestMigrateLegacyRoles_NilSettings(t *testing.T) { + t.Parallel() + if got := settings.MigrateLegacyRoles(nil); got { + t.Errorf("MigrateLegacyRoles(nil) = true, want false") + } +} + +func TestMigrateLegacyRoles_EmptyReviewMap(t *testing.T) { + t.Parallel() + s1 := &settings.EntireSettings{Review: nil} + if got := settings.MigrateLegacyRoles(s1); got { + t.Errorf("MigrateLegacyRoles(Review=nil) = true, want false") + } + s2 := &settings.EntireSettings{Review: map[string]settings.ReviewConfig{}} + if got := settings.MigrateLegacyRoles(s2); got { + t.Errorf("MigrateLegacyRoles(Review={}) = true, want false") + } +} + +func TestMigrateLegacyRoles_Idempotent(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review"}}, + "codex": {Skills: []string{"/review"}}, + }, + ReviewFixAgent: "codex", + } + if got := settings.MigrateLegacyRoles(s); !got { + t.Fatalf("first call returned false, want true (entries needed migration)") + } + if got := settings.MigrateLegacyRoles(s); got { + t.Errorf("second call returned true, want false (already migrated)") + } + // Sanity: roles should still be set correctly after second call. + if s.Review["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q, want reviewer", s.Review["claude-code"].Role) + } + if s.Review["codex"].Role != settings.RoleBoth { + t.Errorf("codex role = %q, want both", s.Review["codex"].Role) + } +} + +func TestMigrateLegacyRoles_LegacyFixAgentWithoutSkills(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review"}}, + "codex": {}, // no Skills, no Prompt + }, + ReviewFixAgent: "codex", + } + if got := settings.MigrateLegacyRoles(s); !got { + t.Fatalf("MigrateLegacyRoles = false, want true") + } + if s.Review["codex"].Role != settings.RoleFixer { + t.Errorf("codex role = %q, want fixer (empty Skills/Prompt should not yield Both)", + s.Review["codex"].Role) + } + if s.Review["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q, want reviewer", s.Review["claude-code"].Role) + } +} + +func TestMigrateLegacyRoles_PreservesExistingRoles(t *testing.T) { + t.Parallel() + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleSkip, Skills: []string{"/review"}}, + "codex": {Skills: []string{"/review"}}, // empty Role, will migrate + "gemini": {Role: settings.RoleReviewer, Skills: []string{"/review"}}, + }, + ReviewFixAgent: "codex", + } + if got := settings.MigrateLegacyRoles(s); !got { + t.Fatalf("MigrateLegacyRoles = false, want true (codex needed migration)") + } + if s.Review["claude-code"].Role != settings.RoleSkip { + t.Errorf("claude-code role mutated: got %q, want skip (preserved)", + s.Review["claude-code"].Role) + } + if s.Review["gemini"].Role != settings.RoleReviewer { + t.Errorf("gemini role mutated: got %q, want reviewer (preserved)", + s.Review["gemini"].Role) + } + // codex was the migration target; it had skills + matched ReviewFixAgent → RoleBoth. + if s.Review["codex"].Role != settings.RoleBoth { + t.Errorf("codex role = %q, want both (has skills + is fix agent)", + s.Review["codex"].Role) + } +} + +// TestMigrateLegacyRoles_MultipleEmpty_AlphabeticalFixerWins: +// MigrateLegacyRoles alone cannot produce two fixers from empty-role entries +// (only the ReviewFixAgent gets fixer-bearing roles; all other empty entries +// become RoleReviewer). The alphabetical-winner branch is exercised when a +// pre-existing fixer collides with a migrated fixer; that scenario is covered +// by TestMigrateLegacyRoles_PreservesExistingRoles above, and the pure +// at-most-one-fixer behavior is locked down by TestNormalizeRoles_AtMostOneFixer +// in roles_test.go. + +func TestSave_RoundtripsMigratedSchema(t *testing.T) { + // Not t.Parallel(): t.Chdir cannot be combined with parallel tests. + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + + s := &settings.EntireSettings{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer, Skills: []string{"/review"}}, + "codex": {Role: settings.RoleFixer}, + }, + FixAfterReview: settings.FixAfterReviewAlways, + } + if err := settings.Save(context.Background(), s); err != nil { + t.Fatalf("save: %v", err) + } + + loaded, err := settings.Load(context.Background()) + if err != nil { + t.Fatalf("load: %v", err) + } + if loaded.Review["claude-code"].Role != settings.RoleReviewer { + t.Errorf("role lost: %+v", loaded.Review["claude-code"]) + } + if loaded.FixAfterReview != settings.FixAfterReviewAlways { + t.Errorf("fix_after_review lost: %q", loaded.FixAfterReview) + } +} diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index cd1513df11..24c620680c 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -82,6 +82,11 @@ type EntireSettings struct { // multi-agent review findings with `entire review --fix`. ReviewFixAgent string `json:"review_fix_agent,omitempty"` + // FixAfterReview controls whether `entire review` automatically runs + // the Fixer after a review completes. Default empty ("") prompts the + // user inline; "always" skips the prompt and just runs. + FixAfterReview FixAfterReviewMode `json:"fix_after_review,omitempty"` + // CommitLinking controls how commits are linked to agent sessions. // "always" = auto-link without prompting, "prompt" = ask on each commit. // Defaults to "prompt" (preserves existing user behavior). @@ -126,6 +131,11 @@ type ClonePreferences struct { Review map[string]ReviewConfig `json:"review,omitempty"` ReviewFixAgent string `json:"review_fix_agent,omitempty"` + // FixAfterReview mirrors EntireSettings.FixAfterReview at the clone-local + // layer; when set it wins over the project-settings value. Symmetric with + // ReviewFixAgent's clone-local override. + FixAfterReview FixAfterReviewMode `json:"fix_after_review,omitempty"` + // ReviewMigrationDismissed records that the user declined the one-shot // migration of review keys from project settings to clone-local prefs. // Once true, `entire review` stops prompting on every invocation; the @@ -222,6 +232,47 @@ func (s *EntireSettings) SummaryTimeoutValue() time.Duration { return time.Duration(s.SummaryTimeoutSeconds) * time.Second } +// Role expresses how a configured agent participates in `entire review`. +// Stored as a string for stable JSON encoding; consumers should compare +// against the Role* constants below rather than untyped strings. +type Role string + +const ( + RoleReviewer Role = "reviewer" // runs on `entire review`; N allowed + RoleFixer Role = "fixer" // runs on `entire review fix`; exactly 1 across the configured map + RoleBoth Role = "both" // reviews AND fixes; counts toward the one-fixer cap + RoleSkip Role = "skip" // configured but not used by review +) + +// Valid reports whether r is a recognized role. The empty string is +// considered valid because legacy ReviewConfig entries written before +// roles existed have no role field; NormalizeRoles assigns Reviewer +// to such entries on first load. +func (r Role) Valid() bool { + switch r { + case "", RoleReviewer, RoleFixer, RoleBoth, RoleSkip: + return true + } + return false +} + +// IsFixer reports whether r participates in the Fixer role (Fixer or Both). +func (r Role) IsFixer() bool { return r == RoleFixer || r == RoleBoth } + +// IsReviewer reports whether r participates in the Reviewer role (Reviewer or Both). +func (r Role) IsReviewer() bool { return r == RoleReviewer || r == RoleBoth } + +// FixAfterReviewMode controls whether `entire review` auto-runs the +// Fixer after a review session completes. +type FixAfterReviewMode string + +const ( + // FixAfterReviewAsk prompts the user inline (default). + FixAfterReviewAsk FixAfterReviewMode = "" + // FixAfterReviewAlways skips the prompt and runs the Fixer automatically. + FixAfterReviewAlways FixAfterReviewMode = "always" +) + // ReviewConfig holds the per-agent review configuration. Both fields are // optional; together they describe what `entire review` should ask the // agent to do. @@ -235,6 +286,12 @@ func (s *EntireSettings) SummaryTimeoutValue() time.Duration { // which path composed the prompt — they're the structured, queryable // tag alongside ReviewPrompt (which is the ground truth). type ReviewConfig struct { + // Role expresses how this agent participates in `entire review`. + // Empty Role is legacy data — settings.MigrateLegacyRoles upgrades it + // to RoleReviewer (or RoleFixer/RoleBoth based on ReviewFixAgent) the + // first time settings are loaded after upgrading. + Role Role `json:"role,omitempty"` + // Skills is the list of slash-prefixed skill invocations configured // for this agent. May be empty when Prompt carries the full request. Skills []string `json:"skills,omitempty"` @@ -328,6 +385,10 @@ func loadMergedSettings(settingsFileAbs, preferencesFileAbs, localSettingsFileAb return nil, fmt.Errorf("merged settings invalid: %w", err) } + // In-memory role migration: idempotent, no disk write. Persistence + // happens lazily on the next settings write (e.g. `entire review setup`). + _ = MigrateLegacyRoles(settings) + return settings, nil } @@ -542,6 +603,9 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { if prefs.ReviewFixAgent != "" { settings.ReviewFixAgent = prefs.ReviewFixAgent } + if prefs.FixAfterReview != "" { + settings.FixAfterReview = prefs.FixAfterReview + } } // mergeJSON merges JSON data into existing settings. @@ -625,6 +689,9 @@ func mergeScalarFields(settings *EntireSettings, raw map[string]json.RawMessage) if err := mergeRawStringNonEmpty(raw, "review_fix_agent", &settings.ReviewFixAgent); err != nil { return err } + if err := mergeRawStringNonEmpty(raw, "fix_after_review", (*string)(&settings.FixAfterReview)); err != nil { + return err + } if err := mergeRawInt(raw, "summary_timeout_seconds", &settings.SummaryTimeoutSeconds); err != nil { return err } diff --git a/cmd/entire/cli/settings/settings_test.go b/cmd/entire/cli/settings/settings_test.go index 27e7b22b90..cad3332cbb 100644 --- a/cmd/entire/cli/settings/settings_test.go +++ b/cmd/entire/cli/settings/settings_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "os" "path/filepath" + "reflect" "strings" "testing" "time" @@ -1199,3 +1200,46 @@ func TestReviewConfig_IsZero(t *testing.T) { }) } } + +func TestRole_Valid(t *testing.T) { + t.Parallel() + cases := []struct { + in Role + want bool + }{ + {RoleReviewer, true}, + {RoleFixer, true}, + {RoleBoth, true}, + {RoleSkip, true}, + {Role(""), true}, // empty = legacy; normalization upgrades to Reviewer + {Role("bogus"), false}, + } + for _, c := range cases { + if got := c.in.Valid(); got != c.want { + t.Errorf("Role(%q).Valid() = %v, want %v", c.in, got, c.want) + } + } +} + +func TestEntireSettings_RoundtripWithRoles(t *testing.T) { + t.Parallel() + in := EntireSettings{ + Enabled: true, + Review: map[string]ReviewConfig{ + "claude-code": {Role: RoleReviewer, Skills: []string{"/review"}}, + "codex": {Role: RoleFixer}, + }, + FixAfterReview: FixAfterReviewAlways, + } + raw, err := json.Marshal(in) + if err != nil { + t.Fatalf("marshal: %v", err) + } + var out EntireSettings + if err := json.Unmarshal(raw, &out); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !reflect.DeepEqual(in, out) { + t.Errorf("roundtrip mismatch:\n in = %+v\n out = %+v", in, out) + } +} From 891b8fc8200213646b04489eb3e54c2ae57352bd Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 20 May 2026 16:11:31 -0400 Subject: [PATCH 3/3] feat(review): add `entire review setup` with role-first picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two-step picker: pick a role per installed agent (Reviewer/Fixer/Both/Skip), then pick skills only for Reviewer/Both agents. At-most-one-fixer is enforced via NormalizeRoles. The "Additional instructions" widget uses huh.NewInput so plain Enter submits — no modifier key needed on any platform. The legacy in-flow picker on `entire review` is unchanged in this commit; Chunk 3 replaces it with a `Run: entire review setup` pointer. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 4efdb600636d --- cmd/entire/cli/review/cmd.go | 1 + cmd/entire/cli/review/setup.go | 444 ++++++++++++++++++++++++++++ cmd/entire/cli/review/setup_test.go | 216 ++++++++++++++ 3 files changed, 661 insertions(+) create mode 100644 cmd/entire/cli/review/setup.go create mode 100644 cmd/entire/cli/review/setup_test.go diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 2f05cd22d8..dbe6729905 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -209,6 +209,7 @@ Subcommands: if deps.AttachCmd != nil { cmd.AddCommand(deps.AttachCmd) } + cmd.AddCommand(newReviewSetupCmd(deps)) return cmd } diff --git a/cmd/entire/cli/review/setup.go b/cmd/entire/cli/review/setup.go new file mode 100644 index 0000000000..fd4ef91126 --- /dev/null +++ b/cmd/entire/cli/review/setup.go @@ -0,0 +1,444 @@ +// Package review — see env.go for package-level rationale. +// +// setup.go implements `entire review setup`: a two-step picker for +// role-first review configuration. Step 1 collects a role per installed +// agent; step 2 collects skills + optional instructions for each agent +// with the Reviewer or Both role. +// +// The legacy in-flow picker on `entire review` is unchanged; Chunk 3 +// replaces it with a `Run: entire review setup` pointer. +package review + +import ( + "context" + "errors" + "fmt" + "io" + "log/slog" + "sort" + "strings" + + "charm.land/huh/v2" + "github.com/spf13/cobra" + + "github.com/entireio/cli/cmd/entire/cli/agent" + "github.com/entireio/cli/cmd/entire/cli/agent/external" + "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" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +// SetupForms collects the form constructors RunSetup uses. Production +// passes a zero value (uses the real huh forms); tests inject stubs. +type SetupForms struct { + PickRoles func(ctx context.Context, agents []string, current map[string]settings.Role) (map[string]settings.Role, error) + PickSkills func(ctx context.Context, agentName string, prefill settings.ReviewConfig) (settings.ReviewConfig, error) +} + +// RunSetup runs the role-first configuration flow. Returns the persisted +// per-agent review map (mirrors the in-memory settings.Review post-save). +// +// Step 1: present a role picker per installed agent (Reviewer/Fixer/Both/Skip), +// 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. +func RunSetup( + ctx context.Context, + out io.Writer, + getInstalled func(context.Context) []types.AgentName, + forms SetupForms, +) (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 ' " + + "or 'entire enable' first", + ) + } + + agentNames := make([]string, 0, len(installed)) + for _, a := range installed { + agentNames = append(agentNames, string(a)) + } + 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. + current := make(map[string]settings.Role, len(agentNames)) + saved, loadErr := settings.Load(ctx) + if loadErr != nil { + logging.Warn(ctx, "review setup: settings.Load failed, proceeding with empty preselects", + slog.String("error", loadErr.Error())) + } + for _, name := range agentNames { + if saved != nil { + if cfg, ok := saved.Review[name]; ok && cfg.Role != "" { + current[name] = cfg.Role + continue + } + } + current[name] = settings.RoleReviewer + } + + pickRoles := forms.PickRoles + if pickRoles == nil { + pickRoles = realPickRoles + } + chosen, err := pickRoles(ctx, agentNames, current) + if err != nil { + return nil, fmt.Errorf("roles picker: %w", err) + } + + // Convert chosen roles into a ReviewConfig map for NormalizeRoles; carry + // over saved skills/prompt so users don't re-enter them on every setup + // run when the role is unchanged. + configMap := make(map[string]settings.ReviewConfig, len(chosen)) + for name, role := range chosen { + cfg := settings.ReviewConfig{Role: role} + if saved != nil { + if prev, ok := saved.Review[name]; ok { + cfg.Skills = prev.Skills + cfg.Prompt = prev.Prompt + } + } + configMap[name] = cfg + } + normalized := NormalizeRoles(configMap) + + pickSkills := forms.PickSkills + if pickSkills == nil { + pickSkills = realPickSkills + } + + result := make(map[string]settings.ReviewConfig, len(normalized)) + for _, name := range agentNames { + cfg := normalized[name] + if cfg.Role.IsReviewer() { + prefill := settings.ReviewConfig{} + if saved != nil { + prefill = saved.Review[name] + } + prefill.Role = cfg.Role + chosenSkill, err := pickSkills(ctx, name, prefill) + if err != nil { + return nil, fmt.Errorf("skills picker for %s: %w", name, err) + } + chosenSkill.Role = cfg.Role + result[name] = chosenSkill + } else { + result[name] = settings.ReviewConfig{Role: cfg.Role} + } + } + + if saved == nil { + saved = &settings.EntireSettings{} + } + saved.Review = result + if err := settings.Save(ctx, saved); err != nil { + return nil, fmt.Errorf("save settings: %w", err) + } + + PrintSetupBanner(out, result) + return result, nil +} + +// BuildPickRolesFields constructs one huh.Select[settings.Role] per agent. +// Each Select binds to the pointer in ptrs[agent], so callers can read back +// the user's selection by dereferencing *ptrs[agent] after the form runs. +// +// Ranging over a map yields non-addressable values, so callers must supply +// a pointer-valued map to give huh a stable address per row. realPickRoles +// is the production caller; tests construct their own pointer map. +func BuildPickRolesFields(agents []string, ptrs map[string]*settings.Role) []huh.Field { + fields := make([]huh.Field, 0, len(agents)+1) + opts := []huh.Option[settings.Role]{ + huh.NewOption("Reviewer", settings.RoleReviewer), + huh.NewOption("Fixer", settings.RoleFixer), + huh.NewOption("Both", settings.RoleBoth), + huh.NewOption("Skip", settings.RoleSkip), + } + for _, name := range agents { + // Inline(true) collapses each Select to a single-line + // dropdown row (← / → cycle the value) so the form reads + // as one row per agent rather than expanding the full + // option list under each. + // + // Validate enforces at-most-one Fixer/Both INLINE so the + // user sees the conflict immediately. NormalizeRoles still + // runs after the form as a defensive backstop. + fields = append(fields, + huh.NewSelect[settings.Role](). + Title(displayLabelFor(name)). + Options(opts...). + Value(ptrs[name]). + Inline(true). + Validate(func(r settings.Role) error { + if !r.IsFixer() { + return nil + } + for other, p := range ptrs { + if other == name { + continue + } + if p.IsFixer() { + return fmt.Errorf("%s already has the %s role; only one agent can be Fixer or Both", + displayLabelFor(other), *p) + } + } + return nil + }), + ) + } + // Legend Note at the bottom — describes the role choices once, + // rather than repeating long Option labels per row. + fields = append(fields, huh.NewNote(). + Description( + "Reviewer = runs on entire review\n"+ + "Fixer = runs on entire review fix\n"+ + "Both = reviews and fixes (counts as the Fixer)\n"+ + "Skip = exclude from review", + )) + return fields +} + +// 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. +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 + } + ptrs[name] = &v + } + fields := BuildPickRolesFields(agents, ptrs) + form := newAccessibleForm(huh.NewGroup(fields...)) + if err := form.RunWithContext(ctx); err != nil { + return nil, fmt.Errorf("roles form: %w", err) + } + for k, p := range ptrs { + current[k] = *p + } + return current, nil +} + +// BuildSetupSkillsFields is a copy-and-adapt of BuildReviewPickerFields +// that swaps the "Additional instructions" huh.NewText for huh.NewInput. +// huh.NewText requires a modifier key (e.g. Ctrl+D) to submit, which is +// ambiguous on macOS/Linux/Windows; huh.NewInput accepts plain Enter so +// users can move forward without consulting docs. +// +// The signature mirrors BuildReviewPickerFields exactly so realPickSkills +// can call it as a drop-in replacement. +func BuildSetupSkillsFields( + agentName string, + builtins []skilldiscovery.CuratedSkill, + discovered []agent.DiscoveredSkill, + activeHints []skilldiscovery.InstallHint, + previousPrompt string, + builtinPicksOut, discoveredPicksOut *[]string, + promptOut *string, +) []huh.Field { + var fields []huh.Field + + if builtinPicksOut != nil && len(*builtinPicksOut) == 0 && + len(builtins) == 1 && strings.TrimSpace(previousPrompt) == "" { + *builtinPicksOut = []string{builtins[0].Name} + } + + builtinPreselected := preselectedSet(builtinPicksOut) + discoveredPreselected := preselectedSet(discoveredPicksOut) + + if len(builtins) > 0 { + opts := make([]huh.Option[string], 0, len(builtins)) + for _, b := range builtins { + opt := huh.NewOption(b.Name, b.Name) + if _, ok := builtinPreselected[b.Name]; ok { + opt = opt.Selected(true) + } + opts = append(opts, opt) + } + ms := huh.NewMultiSelect[string](). + Title("Built-in commands"). + Options(opts...). + Height(len(opts) + 1) + if builtinPicksOut != nil { + ms = ms.Value(builtinPicksOut) + } + fields = append(fields, ms) + } else { + fields = append(fields, huh.NewNote(). + Title("Built-in commands"). + Description(fmt.Sprintf("No built-in review commands in %s.", agentName))) + } + + if len(discovered) > 0 { + opts := make([]huh.Option[string], 0, len(discovered)) + for _, d := range discovered { + opt := huh.NewOption(d.Name, d.Name) + if _, ok := discoveredPreselected[d.Name]; ok { + opt = opt.Selected(true) + } + opts = append(opts, opt) + } + ms := huh.NewMultiSelect[string](). + Title("Installed plugin skills"). + Options(opts...). + Height(len(opts) + 1) + if discoveredPicksOut != nil { + ms = ms.Value(discoveredPicksOut) + } + fields = append(fields, ms) + } else { + fields = append(fields, huh.NewNote(). + Title("Installed plugin skills"). + Description("No plugin review skills detected on disk.")) + } + + if len(activeHints) > 0 { + var sb strings.Builder + for i, h := range activeHints { + if i > 0 { + sb.WriteString("\n") + } + sb.WriteString("• ") + sb.WriteString(h.Message) + } + fields = append(fields, huh.NewNote(). + Title("Install more"). + Description(sb.String())) + } + + // The key difference from BuildReviewPickerFields: Input not Text. + // huh.NewInput submits on plain Enter; huh.NewText requires Ctrl+D + // (or a similar modifier) which the user has no in-form way to learn. + input := huh.NewInput(). + Title("Additional instructions (optional)"). + Description("Added after selected skills. If no skills are selected, this becomes the full review prompt.") + if promptOut != nil { + *promptOut = previousPrompt + input = input.Value(promptOut) + } + fields = append(fields, input) + + return fields +} + +// 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). +func realPickSkills(ctx context.Context, agentName string, prefill settings.ReviewConfig) (settings.ReviewConfig, error) { + ag, err := agent.Get(types.AgentName(agentName)) + if err != nil { + return settings.ReviewConfig{}, fmt.Errorf("resolve agent %s: %w", agentName, err) + } + curated := skilldiscovery.CuratedBuiltinsFor(agentName) + var discovered []agent.DiscoveredSkill + if d, ok := ag.(agent.SkillDiscoverer); ok { + if ds, dErr := d.DiscoverReviewSkills(ctx); dErr == nil { + discovered = ds + } else { + logging.Debug(ctx, "review setup discovery failed", + slog.String("agent", agentName), 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(agentName, discoveredSet) + + builtinPicks, discoveredPicks := SplitSavedPicks(prefill.Skills, curated, discovered) + prompt := prefill.Prompt + + fields := BuildSetupSkillsFields( + agentName, curated, discovered, activeHints, prompt, + &builtinPicks, &discoveredPicks, &prompt, + ) + + form := newAccessibleForm(huh.NewGroup(fields...)) + if err := form.RunWithContext(ctx); err != nil { + return settings.ReviewConfig{}, fmt.Errorf("skills form: %w", err) + } + return settings.ReviewConfig{ + Skills: dedupeStrings(append(builtinPicks, discoveredPicks...)), + Prompt: strings.TrimSpace(prompt), + }, nil +} + +// PrintSetupBanner prints the post-setup summary banner. Reviewers and +// Fixer are listed by display label (e.g. "Claude Code"), not registry +// name, so users see the same string the picker rendered. +func PrintSetupBanner(out io.Writer, review map[string]settings.ReviewConfig) { + var reviewers []string + var fixer string + for name, cfg := range review { + if cfg.Role.IsReviewer() { + reviewers = append(reviewers, displayLabelFor(name)) + } + if cfg.Role.IsFixer() { + fixer = displayLabelFor(name) + } + } + sort.Strings(reviewers) + fmt.Fprintln(out) + fmt.Fprintln(out, "Review configured.") + if len(reviewers) > 0 { + fmt.Fprintf(out, " Reviewers: %s\n", strings.Join(reviewers, ", ")) + } else { + fmt.Fprintln(out, " Reviewers: (none)") + } + if fixer != "" { + fmt.Fprintf(out, " Fixer: %s\n", fixer) + } else { + fmt.Fprintln(out, " Fixer: (none)") + } + fmt.Fprintln(out) + fmt.Fprintln(out, "Edit later: entire review setup") + fmt.Fprintln(out, "Run: entire 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" +// instead of "claude-code". +func displayLabelFor(agentName string) string { + ag, err := agent.Get(types.AgentName(agentName)) + if err != nil { + return agentName + } + if t := string(ag.Type()); t != "" { + return t + } + return agentName +} + +// newReviewSetupCmd returns the `entire review setup` cobra subcommand. +// It's wired in NewCommand alongside the existing attach subcommand. +func newReviewSetupCmd(deps Deps) *cobra.Command { + return &cobra.Command{ + Use: "setup", + Short: "Configure reviewers, fixer, and per-agent review skills", + Long: `Two-step picker: choose a role for each installed agent (Reviewer, +Fixer, Both, or Skip), then choose skills + optional instructions +for each Reviewer/Both agent. Saves to .entire/settings.json.`, + RunE: func(cmd *cobra.Command, _ []string) error { + // External agents (e.g., cursor, opencode) need to be registered before + // RunSetup can offer them as Reviewer/Fixer/Both choices. Mirrors the + // same call in NewCommand's RunE. + external.DiscoverAndRegister(cmd.Context()) + _, err := RunSetup(cmd.Context(), cmd.OutOrStdout(), + deps.GetAgentsWithHooksInstalled, SetupForms{}) + return err + }, + } +} diff --git a/cmd/entire/cli/review/setup_test.go b/cmd/entire/cli/review/setup_test.go new file mode 100644 index 0000000000..90010cdd40 --- /dev/null +++ b/cmd/entire/cli/review/setup_test.go @@ -0,0 +1,216 @@ +package review_test + +import ( + "bytes" + "context" + "io" + "strings" + "testing" + + "charm.land/huh/v2" + + "github.com/entireio/cli/cmd/entire/cli/agent/types" + "github.com/entireio/cli/cmd/entire/cli/review" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +// SetExistingConfigForTest writes a minimal .entire/settings.json into the +// current working directory so RunSetup's preselect-from-saved branch has +// something to read. +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) + } +} + +func TestRunSetup_NoInstalledAgents_ReturnsClearError(t *testing.T) { + t.Parallel() + getInstalled := func(_ context.Context) []types.AgentName { return nil } + _, err := review.RunSetup(context.Background(), io.Discard, getInstalled, review.SetupForms{}) + if err == nil { + t.Fatal("expected error, got nil") + } + if !strings.Contains(err.Error(), "no agents") { + t.Errorf("error should mention no agents, got: %v", err) + } +} + +func TestBuildPickRolesFields_BuildsOneSelectPerAgentPlusLegend(t *testing.T) { + t.Parallel() + ptrs := map[string]*settings.Role{ + "claude-code": new(settings.Role), + "codex": new(settings.Role), + } + *ptrs["claude-code"] = settings.RoleReviewer + *ptrs["codex"] = settings.RoleFixer + fields := review.BuildPickRolesFields([]string{"claude-code", "codex"}, ptrs) + // 2 Selects (one per agent) + 1 Note (the role legend). + if len(fields) != 3 { + t.Fatalf("expected 2 selects + 1 legend note, got %d fields", len(fields)) + } + selectCount := 0 + noteCount := 0 + for _, f := range fields { + switch f.(type) { + case *huh.Select[settings.Role]: + selectCount++ + case *huh.Note: + noteCount++ + } + } + if selectCount != 2 { + t.Errorf("expected 2 Select fields, got %d", selectCount) + } + if noteCount != 1 { + t.Errorf("expected 1 Note (legend), got %d", noteCount) + } +} + +func TestRunSetup_DefaultsRolesFromExistingConfig(t *testing.T) { + // Note: uses t.Chdir, so cannot use t.Parallel(). + dir := t.TempDir() + testutil.InitRepo(t, dir) + t.Chdir(dir) + SetExistingConfigForTest(t, map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer, Skills: []string{"/review"}}, + }) + + getInstalled := func(context.Context) []types.AgentName { + return []types.AgentName{"claude-code", "codex"} + } + forms := review.SetupForms{ + PickRoles: func(_ context.Context, agents []string, current map[string]settings.Role) (map[string]settings.Role, error) { + 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"]) + } + _ = agents + return map[string]settings.Role{ + "claude-code": settings.RoleReviewer, + "codex": settings.RoleFixer, + }, nil + }, + PickSkills: func(_ context.Context, _ string, _ settings.ReviewConfig) (settings.ReviewConfig, error) { + return settings.ReviewConfig{Skills: []string{"/review"}}, nil + }, + } + out, err := review.RunSetup(context.Background(), io.Discard, getInstalled, forms) + if err != nil { + t.Fatalf("setup: %v", err) + } + if out["codex"].Role != settings.RoleFixer { + t.Errorf("codex role = %q, want fixer", out["codex"].Role) + } + if out["claude-code"].Role != settings.RoleReviewer { + t.Errorf("claude-code role = %q, want reviewer", out["claude-code"].Role) + } +} + +func TestRunSetup_EnforcesAtMostOneFixerAfterPick(t *testing.T) { + // Note: uses t.Chdir, so cannot use 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.RoleFixer, + }, nil + }, + PickSkills: func(context.Context, string, settings.ReviewConfig) (settings.ReviewConfig, error) { + return settings.ReviewConfig{}, nil + }, + } + out, err := review.RunSetup(context.Background(), io.Discard, + func(context.Context) []types.AgentName { + return []types.AgentName{"claude-code", "codex"} + }, forms) + if err != nil { + t.Fatalf("setup: %v", err) + } + fixers := 0 + for _, cfg := range out { + if cfg.Role.IsFixer() { + fixers++ + } + } + if fixers != 1 { + t.Errorf("expected 1 fixer after normalization, got %d", fixers) + } +} + +func TestBuildSetupSkillsFields_UsesInputNotTextForInstructions(t *testing.T) { + t.Parallel() + var builtinPicks, discoveredPicks []string + var prompt string + fields := review.BuildSetupSkillsFields( + "claude-code", nil, nil, nil, "", + &builtinPicks, &discoveredPicks, &prompt, + ) + foundInput, foundText := false, false + for _, f := range fields { + if _, ok := f.(*huh.Input); ok { + foundInput = true + } + if _, ok := f.(*huh.Text); ok { + foundText = true + } + } + if !foundInput { + t.Errorf("expected a huh.Input for instructions") + } + if foundText { + t.Errorf("expected NO huh.Text — plain Enter would be ambiguous") + } +} + +func TestPrintSetupBanner_MultipleReviewersWithDisplayLabels(t *testing.T) { + t.Parallel() + var buf bytes.Buffer + review.PrintSetupBanner(&buf, map[string]settings.ReviewConfig{ + "claude-code": {Role: settings.RoleReviewer}, + "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) + } + if !strings.Contains(got, "Fixer: Codex") { + t.Errorf("expected fixer line, got:\n%s", got) + } + if !strings.Contains(got, "Edit later: entire review setup") { + t.Errorf("expected edit-later pointer, got:\n%s", got) + } + if !strings.Contains(got, "Run: entire review") { + t.Errorf("expected Run: pointer, got:\n%s", got) + } +} + +func TestSetupSubcommand_Registered(t *testing.T) { + t.Parallel() + root := review.NewCommand(testDepsForSetupSubcommand(t)) + setup, _, err := root.Find([]string{"setup"}) + if err != nil { + t.Fatalf("setup subcommand not found: %v", err) + } + if setup.Use != "setup" { + t.Errorf("got %q, want setup", setup.Use) + } +} + +func testDepsForSetupSubcommand(t *testing.T) review.Deps { + t.Helper() + return review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { return nil }, + NewSilentError: func(err error) error { return err }, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { return false, "" }, + ReviewerFor: func(string) reviewtypes.AgentReviewer { return nil }, + } +}