Merge pull request #993 from entireio/feat/entire-review

feat: entire review command

Author: Soph

.golangci.yaml

@@ -108,16 +108,16 @@ linters:
         - grpc.DialOption
         - github.com/entireio/cli/cmd/entire/cli/summarize.Generator
         - 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

@@ -672,6 +672,112 @@ Trailers:
 - Test with `mise run test` - strategy tests are in `*_test.go` files
 - **Update both CLAUDE.md and AGENTS.md** when modifying the strategy to keep documentation current
 
+### `entire review` Command
+
+`entire review` runs a set of configured review skills inside an agent session. The review session is an immutable fact attached to a checkpoint — no verdict, no status tracking, no empty commits. On the next `git commit`, the review session is condensed into the checkpoint metadata alongside normal sessions, permanently recording that the code was reviewed and which skills were run.
+
+#### 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 <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`:
+
+```json
+{
+  "review": {
+    "claude-code": {"skills": ["/pr-review-toolkit:review-pr"], "prompt": "Be thorough."},
+    "codex": {"skills": ["/codex:adversarial-review"]}
+  }
+}
+```
+
+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`.
+
+#### 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 (closest-ancestor branch via `review.ComputeScopeStats`).
+2. **For launchable agents** (claude-code, codex, gemini-cli): the spawned agent process is given env vars `ENTIRE_REVIEW_{SESSION,AGENT,SKILLS,PROMPT,STARTING_SHA}` that the agent's `UserPromptSubmit` lifecycle hook reads to tag the session as `Kind = "agent_review"` with the configured skills/prompt. Each spawned process has its own env, so multiple worktrees and multi-agent runs are correct by construction (no shared marker file, no race).
+3. **For non-launchable agents** (cursor, opencode, factoryai-droid): `RunMarkerFallback` writes a `PendingReviewMarker` file and prints guidance — the user opens the agent themselves and runs the skills. Single shared file (`review/marker_fallback.go`); adding new non-launchable agents is a registry entry, not a new file.
+4. The agent runs the review skills; the session ends naturally.
+5. On the next `git commit`, the PostCommit hook condenses the review session into the checkpoint on `entire/checkpoints/v1`, with `Kind` and `ReviewSkills` recorded in `CommittedMetadata`.
+6. The `CheckpointSummary` sets `HasReview = true` for O(1) lookup. `HasReview` is an umbrella "any review happened" flag — future review kinds (e.g. manual review) should also set it.
+7. `entire status` and the re-run guard read `HasReview` from the checkpoint metadata (no commit history walking).
+
+#### Checkpoint Metadata
+
+Review metadata is stored at two levels on `entire/checkpoints/v1`:
+
+- **`CommittedMetadata` (per-session)**: `kind: "agent_review"`, `review_skills: ["/skill1", "/skill2"]`, `review_prompt: "..."`
+- **`CheckpointSummary` (per-checkpoint)**: `has_review: true` (umbrella; set when any session in the checkpoint has a review-kind `Kind`)
+
+#### Architecture
+
+- **`AgentReviewer` interface** (`cmd/entire/cli/review/types/reviewer.go`): per-agent contract with `Name() string` and `Start(ctx, RunConfig) (Process, error)`. Each launchable agent implements this in its own package.
+- **`ReviewerTemplate`** (`cmd/entire/cli/review/types/template.go`): shared scaffolding (Spawn → pipe stdout → run parser → forward events → close). Each agent supplies only its `BuildCmd` (argv/env) and `Parser` (stdout-to-Event stream).
+- **`Sink` interface**: consumers of the event stream. Production sinks: `DumpSink` (post-run per-agent narrative), `TUISink` (Bubble Tea live dashboard with Ctrl+O drill-in), `SynthesisSink` (opt-in y/N cross-agent verdict). Sinks are composed by `composeMultiAgentSinks` based on TTY detection.
+- **`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:
+
+- `PendingReviewMarker` for launchable agents (env-var handshake makes it unnecessary)
+- `WorktreePath` field + worktree-scoping logic (env per process eliminates the multi-tenant problem)
+- `AgentEntries` map on the marker (each agent has its own env)
+- Marker overwrite tripwire / refuse-attach guard (the bug classes they defended against don't exist)
+- `--track-only` flag (intentionally removed by #1009)
+- `--postreview` / `--finalize` / empty review commits / `/entire-review:finish` skill installer
+- `Launcher` + `HeadlessLauncher` as separate interfaces (single `AgentReviewer`)
+- `filterCodexOutput` in shared multi-agent code (lives in codex's adapter)
+- `sync.Once`-guarded onCancel + parallel `signal.Notify` goroutine (single cancel from start)
+
+#### 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/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 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`, `lazySynthesisProvider`)
+- `cmd/entire/cli/checkpoint/checkpoint.go` — `Kind`, `ReviewSkills`, `ReviewPrompt` on `CommittedMetadata`; `HasReview` on `CheckpointSummary`
+- `cmd/entire/cli/settings/settings.go` — `EntireSettings.Review` field
+
 # Important Notes
 
 - **Before committing:** Follow the "Before Every Commit (REQUIRED)" checklist above - CI will fail without it

cmd/entire/cli/agent/agent.go

@@ -6,6 +6,7 @@ package agent
 import (
 	"context"
 	"io"
+	"os/exec"
 
 	"github.com/entireio/cli/cmd/entire/cli/agent/types"
 )
@@ -254,6 +255,50 @@ type TestOnly interface {
 	IsTestOnly() bool
 }
 
+// Launcher is implemented by agents that `entire` can subprocess-spawn.
+// This is used by `entire review` to start an agent with a pre-composed
+// initial prompt; other commands may use it later.
+//
+// Contract:
+//   - LaunchCmd builds an *exec.Cmd with stdin/stdout/stderr wired to the
+//     caller's TTY. The agent runs in the foreground and the call blocks.
+//   - The returned cmd is ready to Run() or Start(); it must NOT be modified
+//     by the caller except to set environment variables or working dir.
+//   - initialPrompt is the first user message to send to the agent.
+type Launcher interface {
+	LaunchCmd(ctx context.Context, initialPrompt string) (*exec.Cmd, error)
+}
+
+// DiscoveredSkill describes one review-adjacent skill found on disk by a
+// SkillDiscoverer. Name is the agent-native invocation form (e.g. a
+// slash-prefixed command); Description is scraped from on-disk metadata
+// if available; SourcePath is kept for debug logging and is not shown to
+// the user.
+type DiscoveredSkill struct {
+	Name        string
+	Description string
+	SourcePath  string
+}
+
+// SkillDiscoverer is implemented by agents that can enumerate review-adjacent
+// skills installed locally on disk (e.g. plugin skills under
+// ~/.claude/plugins/...). This powers the "Installed plugin skills" section
+// of the `entire review` picker and the runtime verification that configured
+// skills still exist before spawn.
+//
+// Contract:
+//   - Safe to call on fresh installs where no plugin dir exists yet —
+//     return (nil, nil), not an error.
+//   - Malformed individual skill metadata must be skipped with a Debug log,
+//     not propagated as an error.
+//   - A (nil, non-nil) error means "discovery could not run at all" (e.g.
+//     home dir inaccessible). Callers may treat all errors as "found nothing"
+//     and log at Debug — discovery must never block the picker.
+type SkillDiscoverer interface {
+	Agent
+	DiscoverReviewSkills(ctx context.Context) ([]DiscoveredSkill, error)
+}
+
 // SessionBaseDirProvider is implemented by agents that store transcripts in a
 // home-directory-based structure with per-project subdirectories. This enables
 // cross-project transcript search (e.g., when a session was started from a

cmd/entire/cli/agent/architecture_test.go

@@ -54,6 +54,7 @@ func TestAgentPackages_NoForbiddenImports(t *testing.T) {
 		repoPrefix + "telemetry",  // telemetry
 		repoPrefix + "validation", // validation utilities
 		repoPrefix + "settings",   // settings (read-only access)
+		repoPrefix + "review",     // review env contract + AgentReviewer types (used by per-agent reviewer.go files)
 	}
 
 	agentDir := findAgentDir(t)
@@ -126,9 +127,10 @@ func discoverAgentPackages(t *testing.T, agentDir string) []string {
 	t.Helper()
 
 	skipDirs := map[string]bool{
-		"types":    true, // contract types, not an agent implementation
-		"testutil": true, // shared test utilities
-		"external": true, // external agent adapter, not a self-registering agent
+		"types":          true, // contract types, not an agent implementation
+		"testutil":       true, // shared test utilities
+		"external":       true, // external agent adapter, not a self-registering agent
+		"skilldiscovery": true, // shared capability helper (registries, match), not an agent
 	}
 
 	entries, err := os.ReadDir(agentDir)

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

@@ -371,3 +371,20 @@ func (c *ClaudeCodeAgent) ChunkTranscript(_ context.Context, content []byte, max
 func (c *ClaudeCodeAgent) ReassembleTranscript(chunks [][]byte) ([]byte, error) {
 	return agent.ReassembleJSONL(chunks), nil
 }
+
+// LaunchCmd builds an exec.Cmd for `claude "<initialPrompt>"`. Stdio is wired
+// to the caller's TTY so the agent runs foreground and the user interacts
+// normally. The call site is expected to Run() and wait. Hooks inherit the
+// parent environment.
+func (c *ClaudeCodeAgent) LaunchCmd(ctx context.Context, initialPrompt string) (*exec.Cmd, error) {
+	bin, err := exec.LookPath("claude")
+	if err != nil {
+		return nil, fmt.Errorf("claude binary not on PATH: %w", err)
+	}
+	cmd := exec.CommandContext(ctx, bin, initialPrompt)
+	cmd.Stdin = os.Stdin
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	cmd.Env = os.Environ()
+	return cmd, nil
+}

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

@@ -4,9 +4,39 @@ import (
 	"context"
 	"errors"
 	"os/exec"
+	"strings"
 	"testing"
+
+	"github.com/entireio/cli/cmd/entire/cli/agent"
 )
 
+func TestClaudeCodeAgent_LaunchCmd(t *testing.T) {
+	t.Parallel()
+	a := NewClaudeCodeAgent()
+	launcher, ok := a.(agent.Launcher)
+	if !ok {
+		t.Fatal("ClaudeCodeAgent does not implement agent.Launcher")
+	}
+	// Binary may not be on PATH in CI; ErrNotFound is acceptable for this test.
+	cmd, err := launcher.LaunchCmd(context.Background(), "hello world")
+	if err != nil {
+		if errors.Is(err, exec.ErrNotFound) {
+			t.Skip("claude binary not on PATH; skipping cmd shape check")
+		}
+		t.Fatalf("LaunchCmd: %v", err)
+	}
+	if cmd == nil {
+		t.Fatal("nil cmd")
+	}
+	if cmd.Path == "" {
+		t.Error("cmd.Path empty")
+	}
+	joined := strings.Join(cmd.Args, " ")
+	if !strings.Contains(joined, "hello world") {
+		t.Errorf("args missing prompt: %v", cmd.Args)
+	}
+}
+
 func TestResolveSessionFile(t *testing.T) {
 	t.Parallel()
 	ag := &ClaudeCodeAgent{}