Merge pull request #1112 from entireio/feat/entire-review-v2-b2

refactor(review): cross-agent synthesis, plugin-cache dedupe, docs (2/2)

Author: Soph

.golangci.yaml

@@ -110,16 +110,14 @@ linters:
         - github.com/entireio/cli/cmd/entire/cli/agent\..+
         - github.com/entireio/cli/cmd/entire/cli/review/types.Process
         - github.com/entireio/cli/cmd/entire/cli/review/types.AgentReviewer
+        - github.com/entireio/cli/cmd/entire/cli/review.SynthesisProvider
         - github.com/entireio/cli/cmd/entire/cli/checkpoint.CommittedReader
         - github.com/entireio/cli/cmd/entire/cli/strategy.Strategy
         - github.com/go-git/go-git/v6/x/plugin.Signer
         - github.com/go-git/go-git/v6/plumbing/storer.ReferenceIter
         - github.com/go-git/go-git/v6/plumbing.EncodedObject
         - github.com/go-git/go-git/v6/storage.Storer
         - github.com/go-git/go-git/v6/plumbing/storer.EncodedObjectIter
-        - github.com/go-git/go-git/v6/x/plugin.Signer
-        - golang.org/x/crypto/ssh/agent.Agent
-        - github.com/entireio/cli/cmd/entire/cli/summarize.Generator
         - github.com/entireio/cli/e2e/agents.Session
         - github.com/go-git/go-billy/v6.Filesystem
         - golang.org/x/crypto/ssh/agent.Agent

CLAUDE.md

@@ -679,15 +679,17 @@ Trailers:
 #### Command Surface
 
 ```
-entire review                          # Normal run: load config, spawn configured agent
+entire review                          # Normal run: load config, run configured agent(s)
 entire review --edit                   # Re-open the skills picker before running
-entire review --agent <name>           # Override which configured agent runs
+entire review --agent <name>           # Force a specific configured agent (skips multi-picker)
 entire review attach <session-id>      # Tag an existing agent session as a review (post-hoc)
 entire review attach --force           # Skip confirmation
 entire review attach --agent <name>    # Agent that created the session
 entire review attach --skills <s,...>  # 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.
+
 #### Settings Schema
 
 Review skills are configured per-agent in `.entire/settings.json`:
@@ -724,11 +726,26 @@ Review metadata is stored at two levels on `entire/checkpoints/v1`:
 
 - **`AgentReviewer` interface** (`cmd/entire/cli/review/types/reviewer.go`): per-agent contract with `Name() string` and `Start(ctx, RunConfig) (Process, error)`. Each launchable agent implements this in its own package.
 - **`ReviewerTemplate`** (`cmd/entire/cli/review/types/template.go`): shared scaffolding (Spawn → pipe stdout → run parser → forward events → close). Each agent supplies only its `BuildCmd` (argv/env) and `Parser` (stdout-to-Event stream).
-- **`Sink` interface**: consumers of the event stream. `DumpSink` renders the per-agent narrative dump after the run. CU8 will add a TUI sink and a synthesis sink.
-- **`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. Multi-agent fan-out (CU8) preserves this serial-dispatch contract via fan-in.
+- **`Sink` interface**: consumers of the event stream. Production sinks: `DumpSink` (post-run per-agent narrative), `TUISink` (Bubble Tea live dashboard with Ctrl+O drill-in), `SynthesisSink` (opt-in y/N cross-agent verdict). Sinks are composed by `composeMultiAgentSinks` based on TTY detection.
+- **`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` finds the closest non-self ancestor branch by tip timestamp, with fallback chain `origin/HEAD → origin/main → origin/master → main → master`. Banner output: "Reviewing feat/X vs main: 3 commits, 7 files changed, 2 uncommitted".
 
+#### Multi-Agent UI
+
+When `RunMulti` is dispatched in a TTY, the sink slice is `[TUISink, DumpSink, SynthesisSink?]`:
+
+- **`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: <err>") 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.
+
+#### Skill Discovery (Claude Code)
+
+`DiscoverReviewSkills` (`cmd/entire/cli/agent/claudecode/discovery.go`) walks three roots: plugin cache (`~/.claude/plugins/cache/<market>/<plugin>/<version>/{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). Without this, multiple installed versions of a plugin produced duplicate skill entries in the picker and prompt.
+
 #### Anti-Features (do NOT recreate)
 
 The redesign eliminated several constructs from the prior implementation. None should be reintroduced without explicit design:
@@ -745,16 +762,19 @@ The redesign eliminated several constructs from the prior implementation. None s
 
 #### Key Files
 
-- `cmd/entire/cli/review/cmd.go` — `NewCommand()`, `runReview` flow (launchable vs non-launchable dispatch)
-- `cmd/entire/cli/review/picker.go` — config-edit picker, first-run setup, agent selection helpers
+- `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/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` — core machinery (CU4–CU5)
+- `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 with chrome filter, gemini-cli)
+- `cmd/entire/cli/agent/claudecode/discovery.go` — skill discovery + `pickLatestVersion` plugin-cache dedupe
 - `cmd/entire/cli/lifecycle.go` — `adoptReviewEnv` reads `ENTIRE_REVIEW_*` from process env; replaces marker-file adoption
-- `cmd/entire/cli/review_bridge.go` / `review_helpers.go` — bridge code in `cli` package for cycle-bound functions (`headHasReviewCheckpoint`, `launchableReviewerFor`, `newReviewAttachCmd`)
+- `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/agent/claudecode/discovery.go

@@ -6,8 +6,11 @@ import (
 	"log/slog"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 
+	"golang.org/x/mod/semver"
+
 	"github.com/entireio/cli/cmd/entire/cli/agent"
 	"github.com/entireio/cli/cmd/entire/cli/agent/skilldiscovery"
 	"github.com/entireio/cli/cmd/entire/cli/logging"
@@ -50,6 +53,12 @@ func (c *ClaudeCodeAgent) DiscoverReviewSkills(ctx context.Context) ([]agent.Dis
 
 // scanPluginCache walks <root>/<marketplace>/<plugin>/<version>/{skills,commands,agents}/
 // One plugin can contribute through any or all three directories.
+//
+// Multiple version directories per plugin are common after upgrades. Walking
+// every version produces duplicate skills (same invocation name, same
+// description) — confusing in the picker and wasteful in the prompt. We pick
+// a single version per plugin via pickLatestVersion: prefer valid semver
+// (highest), fall back to lexicographic max.
 func scanPluginCache(ctx context.Context, root string) []agent.DiscoveredSkill {
 	entries, err := os.ReadDir(root)
 	if err != nil {
@@ -77,20 +86,65 @@ func scanPluginCache(ctx context.Context, root string) []agent.DiscoveredSkill {
 			if err != nil {
 				continue
 			}
-			for _, verEntry := range versionEntries {
-				if !verEntry.IsDir() {
-					continue
-				}
-				versionRoot := filepath.Join(pluginRoot, verEntry.Name())
-				found = append(found, readSkillsDir(ctx, filepath.Join(versionRoot, "skills"), pluginName)...)
-				found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "commands"), pluginName)...)
-				found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "agents"), pluginName)...)
+			versionDir, ok := pickLatestVersion(versionEntries)
+			if !ok {
+				continue
 			}
+			versionRoot := filepath.Join(pluginRoot, versionDir)
+			found = append(found, readSkillsDir(ctx, filepath.Join(versionRoot, "skills"), pluginName)...)
+			found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "commands"), pluginName)...)
+			found = append(found, scanFlatMarkdownDir(ctx, filepath.Join(versionRoot, "agents"), pluginName)...)
 		}
 	}
 	return found
 }
 
+// pickLatestVersion returns the name of the "newest" version directory among
+// entries. Strategy:
+//
+//   - If any entry name parses as semver (with or without a leading "v"), pick
+//     the highest semver among those that parse. Non-semver entries are
+//     ignored when at least one semver entry exists.
+//   - Otherwise, fall back to the lexicographic max of all directory names.
+//     This handles the "unknown" sentinel some plugins ship and one-off names.
+//
+// Returns ("", false) if no usable directory entry exists.
+func pickLatestVersion(entries []os.DirEntry) (string, bool) {
+	var dirs []string
+	for _, e := range entries {
+		if e.IsDir() {
+			dirs = append(dirs, e.Name())
+		}
+	}
+	if len(dirs) == 0 {
+		return "", false
+	}
+	var semverDirs []string
+	for _, d := range dirs {
+		if semver.IsValid(semverWithV(d)) {
+			semverDirs = append(semverDirs, d)
+		}
+	}
+	if len(semverDirs) > 0 {
+		sort.Slice(semverDirs, func(i, j int) bool {
+			return semver.Compare(semverWithV(semverDirs[i]), semverWithV(semverDirs[j])) > 0
+		})
+		return semverDirs[0], true
+	}
+	sort.Sort(sort.Reverse(sort.StringSlice(dirs)))
+	return dirs[0], true
+}
+
+// semverWithV ensures a version string has the "v" prefix that
+// golang.org/x/mod/semver requires. Plugin version dirs are usually bare
+// (e.g. "0.1.0"), but we tolerate either form.
+func semverWithV(s string) string {
+	if strings.HasPrefix(s, "v") {
+		return s
+	}
+	return "v" + s
+}
+
 // scanUserSkills walks ~/.claude/skills/<skill>/SKILL.md.
 func scanUserSkills(ctx context.Context, root string) []agent.DiscoveredSkill {
 	return readSkillsDir(ctx, root, "" /* no plugin prefix */)

cmd/entire/cli/agent/claudecode/discovery_test.go

@@ -252,6 +252,111 @@ func TestDiscoverReviewSkills_SkipsReadme(t *testing.T) {
 	}
 }
 
+// TestDiscoverReviewSkills_DedupesPluginVersions verifies that when a plugin
+// has multiple version directories (common after an upgrade — old version
+// isn't always cleaned up), only the latest version's skills appear once.
+//
+// Without dedupe, the picker would show every review skill twice with no
+// way to tell the entries apart, and the prompt to the agent would list
+// the same skill multiple times.
+func TestDiscoverReviewSkills_DedupesPluginVersions(t *testing.T) {
+	home := withFakeHome(t)
+	old := filepath.Join(home, ".claude", "plugins", "cache",
+		"fake-market", "pr-review-toolkit", "0.1.0", "skills", "review-pr")
+	newer := filepath.Join(home, ".claude", "plugins", "cache",
+		"fake-market", "pr-review-toolkit", "0.2.0", "skills", "review-pr")
+	if err := os.MkdirAll(old, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.MkdirAll(newer, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	oldContent := "---\nname: review-pr\ndescription: Old review\n---\n"
+	newContent := "---\nname: review-pr\ndescription: New review\n---\n"
+	if err := os.WriteFile(filepath.Join(old, "SKILL.md"), []byte(oldContent), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(newer, "SKILL.md"), []byte(newContent), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	a := &claudecode.ClaudeCodeAgent{}
+	skills, err := a.DiscoverReviewSkills(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(skills) != 1 {
+		t.Fatalf("expected 1 deduped skill, got %d: %+v", len(skills), skills)
+	}
+	if skills[0].Description != "New review" {
+		t.Errorf("Description = %q, want %q (latest version should win)", skills[0].Description, "New review")
+	}
+}
+
+// TestDiscoverReviewSkills_NonSemverVersionFallback verifies that a plugin
+// version dir like "unknown" (which pr-review-toolkit ships) is still
+// scanned when no semver dirs are present.
+func TestDiscoverReviewSkills_NonSemverVersionFallback(t *testing.T) {
+	home := withFakeHome(t)
+	skillDir := filepath.Join(home, ".claude", "plugins", "cache",
+		"fake-market", "pr-review-toolkit", "unknown", "skills", "review-pr")
+	if err := os.MkdirAll(skillDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(skillDir, "SKILL.md"),
+		[]byte("---\nname: review-pr\ndescription: Review\n---\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	a := &claudecode.ClaudeCodeAgent{}
+	skills, err := a.DiscoverReviewSkills(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(skills) != 1 {
+		t.Fatalf("expected 1 skill from non-semver version, got %d: %+v", len(skills), skills)
+	}
+}
+
+// TestDiscoverReviewSkills_SemverWinsOverNonSemver verifies that when a
+// plugin has both a semver-shaped version and a non-semver one (e.g.
+// "0.2.0" alongside "unknown"), the semver dir is picked. This matches
+// real upgrade flows where the old "unknown" stub remains alongside the
+// installed semver version.
+func TestDiscoverReviewSkills_SemverWinsOverNonSemver(t *testing.T) {
+	home := withFakeHome(t)
+	semverSkill := filepath.Join(home, ".claude", "plugins", "cache",
+		"fake-market", "pr-review-toolkit", "0.2.0", "skills", "review-pr")
+	unknownSkill := filepath.Join(home, ".claude", "plugins", "cache",
+		"fake-market", "pr-review-toolkit", "unknown", "skills", "review-pr")
+	if err := os.MkdirAll(semverSkill, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.MkdirAll(unknownSkill, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(semverSkill, "SKILL.md"),
+		[]byte("---\nname: review-pr\ndescription: From 0.2.0\n---\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(unknownSkill, "SKILL.md"),
+		[]byte("---\nname: review-pr\ndescription: From unknown\n---\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	a := &claudecode.ClaudeCodeAgent{}
+	skills, err := a.DiscoverReviewSkills(context.Background())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(skills) != 1 {
+		t.Fatalf("expected 1 skill, got %d: %+v", len(skills), skills)
+	}
+	if skills[0].Description != "From 0.2.0" {
+		t.Errorf("Description = %q, want From 0.2.0 (semver should win)", skills[0].Description)
+	}
+}
+
 func TestDiscoverReviewSkills_UserSkillsDir(t *testing.T) {
 	home := withFakeHome(t)
 	userSkillDir := filepath.Join(home, ".claude", "skills", "my-review")