From 0c630e2ce8350f58396a26f728cf22df1e6a36a7 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 13:30:12 -0700 Subject: [PATCH 1/7] fix(review): include checkpoint context in prompts Entire-Checkpoint: c8c88071cba4 --- cmd/entire/cli/review/cmd.go | 33 +++- cmd/entire/cli/review/cmd_test.go | 2 +- cmd/entire/cli/review/prompt.go | 3 + cmd/entire/cli/review/prompt_test.go | 32 +++- cmd/entire/cli/review/tui_model.go | 6 +- cmd/entire/cli/review/types/reviewer.go | 7 + cmd/entire/cli/review_bridge.go | 1 + cmd/entire/cli/review_context.go | 230 ++++++++++++++++++++++++ cmd/entire/cli/review_context_test.go | 220 +++++++++++++++++++++++ 9 files changed, 516 insertions(+), 18 deletions(-) create mode 100644 cmd/entire/cli/review_context.go create mode 100644 cmd/entire/cli/review_context_test.go diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 75d0d43f1..db7b44ad4 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -55,6 +55,11 @@ type Deps struct { // 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 + // ReviewerFor maps an agent registry name to its AgentReviewer // implementation. Returns nil for non-launchable agents (cursor, opencode, // factoryai-droid, copilot-cli). Injected to break the import cycle: @@ -323,12 +328,17 @@ func runSingleAgentPath( return fmt.Errorf("resolve HEAD: %w", shaErr) } scopeBaseRef := detectScope(ctx, worktreeRoot, out) + checkpointContext := "" + if deps.ReviewCheckpointContext != nil { + checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) + } runCfg := reviewtypes.RunConfig{ - PromptOverride: cfg.Prompt, - Skills: cfg.Skills, - ScopeBaseRef: scopeBaseRef, - StartingSHA: headSHA, + PromptOverride: cfg.Prompt, + Skills: cfg.Skills, + ScopeBaseRef: scopeBaseRef, + CheckpointContext: checkpointContext, + StartingSHA: headSHA, } // 7. Branch on launchability. @@ -407,6 +417,10 @@ func runMultiAgentPath( } scopeBaseRef := detectScope(ctx, worktreeRoot, out) + checkpointContext := "" + if deps.ReviewCheckpointContext != nil { + checkpointContext = deps.ReviewCheckpointContext(ctx, worktreeRoot, scopeBaseRef) + } // Build per-agent reviewers with individual RunConfigs (each agent has // its own skills + always-prompt from s.Review[name]). @@ -428,11 +442,12 @@ func runMultiAgentPath( reviewers = append(reviewers, &perAgentConfiguredReviewer{ inner: reviewer, cfg: reviewtypes.RunConfig{ - PromptOverride: agentCfg.Prompt, - Skills: agentCfg.Skills, - PerRunPrompt: picked.PerRun, - ScopeBaseRef: scopeBaseRef, - StartingSHA: headSHA, + PromptOverride: agentCfg.Prompt, + Skills: agentCfg.Skills, + PerRunPrompt: picked.PerRun, + ScopeBaseRef: scopeBaseRef, + CheckpointContext: checkpointContext, + StartingSHA: headSHA, }, }) } diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index ba20e40e0..2062e0fcc 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -327,7 +327,7 @@ type stubDispatchReviewer struct { } func (r *stubDispatchReviewer) Name() string { return r.name } -func (r *stubDispatchReviewer) Start(_ context.Context, _ reviewtypes.RunConfig) (reviewtypes.Process, error) { +func (r *stubDispatchReviewer) Start(context.Context, reviewtypes.RunConfig) (reviewtypes.Process, error) { return &stubDispatchProcess{}, nil } diff --git a/cmd/entire/cli/review/prompt.go b/cmd/entire/cli/review/prompt.go index 664066bbe..3204e8f9b 100644 --- a/cmd/entire/cli/review/prompt.go +++ b/cmd/entire/cli/review/prompt.go @@ -45,6 +45,9 @@ func ComposeReviewPrompt(cfg reviewtypes.RunConfig) string { if cfg.ScopeBaseRef != "" { sections = append(sections, "Scope: review only the commits unique to this branch vs "+cfg.ScopeBaseRef+".") } + if trimmed := strings.TrimRight(cfg.CheckpointContext, "\n\r "); trimmed != "" { + sections = append(sections, trimmed) + } return strings.Join(sections, "\n\n") } diff --git a/cmd/entire/cli/review/prompt_test.go b/cmd/entire/cli/review/prompt_test.go index 3c9e0db16..b9eed0ad0 100644 --- a/cmd/entire/cli/review/prompt_test.go +++ b/cmd/entire/cli/review/prompt_test.go @@ -1,6 +1,7 @@ package review import ( + "strings" "testing" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -60,14 +61,35 @@ func TestComposeReviewPrompt_AllSectionsWithScope(t *testing.T) { } } +func TestComposeReviewPrompt_IncludesCheckpointContext(t *testing.T) { + t.Parallel() + cfg := reviewtypes.RunConfig{ + Skills: []string{"/x"}, + ScopeBaseRef: "main", + CheckpointContext: "Commits in scope (newest first):\n abc123 checkpoint data\n", + } + got := ComposeReviewPrompt(cfg) + for _, want := range []string{ + "/x", + "Scope: review only the commits unique to this branch vs main.", + "Commits in scope (newest first):", + "abc123 checkpoint data", + } { + if !strings.Contains(got, want) { + t.Errorf("prompt missing %q:\n%s", want, got) + } + } +} + func TestComposeReviewPrompt_PromptOverrideIsVerbatim(t *testing.T) { t.Parallel() cfg := reviewtypes.RunConfig{ - Skills: []string{"/review"}, - AlwaysPrompt: "always-on instructions", - PerRunPrompt: "per-run focus", - ScopeBaseRef: "main", - PromptOverride: "custom prompt\nleave untouched", + Skills: []string{"/review"}, + AlwaysPrompt: "always-on instructions", + PerRunPrompt: "per-run focus", + ScopeBaseRef: "main", + CheckpointContext: "Commits in scope (newest first):\n abc123 checkpoint data\n", + PromptOverride: "custom prompt\nleave untouched", } got := ComposeReviewPrompt(cfg) want := "custom prompt\nleave untouched" diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index b21f6d23e..e801dba44 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -108,7 +108,7 @@ func (m reviewTUIModel) Init() tea.Cmd { } // Update handles all incoming messages. -func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { //nolint:ireturn // bubbletea interface +func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. switch msg := msg.(type) { case agentEventMsg: return m.handleAgentEvent(msg) @@ -166,7 +166,7 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { //nolint:iret } // handleAgentEvent processes an agentEventMsg, updating the relevant row. -func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // bubbletea interface +func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. idx, ok := m.rowIdx[msg.agent] if !ok { return m, nil @@ -229,7 +229,7 @@ func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) } // handleKey processes keyboard input. -func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // bubbletea interface +func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. // Any key after finished dismisses. if m.finished { return m, tea.Quit diff --git a/cmd/entire/cli/review/types/reviewer.go b/cmd/entire/cli/review/types/reviewer.go index 28381e71d..0074227a6 100644 --- a/cmd/entire/cli/review/types/reviewer.go +++ b/cmd/entire/cli/review/types/reviewer.go @@ -91,6 +91,13 @@ type RunConfig struct { // base for `git diff` operations the agent may perform. ScopeBaseRef string + // CheckpointContext is best-effort context derived from checkpoints in the + // branch review scope. It is appended to generated prompts so every agent + // can use checkpoint IDs, file summaries, and transcript lookup commands + // while reviewing. PromptOverride remains verbatim and does not receive + // this context. + CheckpointContext string + // StartingSHA is HEAD at invocation time, propagated to the lifecycle // hook via ENTIRE_REVIEW_STARTING_SHA so checkpoint metadata records // the commit that was reviewed. diff --git a/cmd/entire/cli/review_bridge.go b/cmd/entire/cli/review_bridge.go index ad919db5f..6d33edebe 100644 --- a/cmd/entire/cli/review_bridge.go +++ b/cmd/entire/cli/review_bridge.go @@ -40,6 +40,7 @@ func buildReviewDeps(attachCmd *cobra.Command) cliReview.Deps { return NewSilentError(err) }, HeadHasReviewCheckpoint: headHasReviewCheckpoint, + ReviewCheckpointContext: reviewCheckpointContext, ReviewerFor: launchableReviewerFor, PromptForAgentFn: nil, // use real PromptForAgent AttachCmd: attachCmd, diff --git a/cmd/entire/cli/review_context.go b/cmd/entire/cli/review_context.go new file mode 100644 index 000000000..68bffad84 --- /dev/null +++ b/cmd/entire/cli/review_context.go @@ -0,0 +1,230 @@ +package cli + +import ( + "context" + "errors" + "fmt" + "log/slog" + "os/exec" + "strings" + + git "github.com/go-git/go-git/v6" + + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + checkpointid "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/checkpoint/remote" + "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/trailers" +) + +const reviewContextMaxDetailRunes = 320 + +type reviewContextSessionMetadataReader interface { + ReadSessionMetadata(ctx context.Context, checkpointID checkpointid.CheckpointID, sessionIndex int) (*checkpoint.CommittedMetadata, error) +} + +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 { + if scopeBaseRef == "" { + return "" + } + + commits, err := reviewContextGitLines(ctx, worktreeRoot, "rev-list", scopeBaseRef+"..HEAD") + if err != nil || len(commits) == 0 { + if err != nil { + logging.Debug(ctx, "review checkpoint context: list commits", slog.String("error", err.Error())) + } + return "" + } + + repo, err := git.PlainOpen(worktreeRoot) + if err != nil { + logging.Debug(ctx, "review checkpoint context: open repo", slog.String("error", err.Error())) + return "" + } + v1 := checkpoint.NewGitStore(repo) + v2URL, urlErr := remote.FetchURL(ctx) + if urlErr != nil { + logging.Debug(ctx, "review checkpoint context: no v2 fetch remote", slog.String("error", urlErr.Error())) + } + v2 := checkpoint.NewV2GitStore(repo, v2URL) + + var lines []string + seen := map[checkpointid.CheckpointID]bool{} + for _, commit := range commits { + message := reviewContextGitString(ctx, worktreeRoot, "show", "-s", "--format=%B", commit) + cpID, ok := trailers.ParseCheckpoint(message) + if !ok || seen[cpID] { + continue + } + seen[cpID] = true + + reader, summary, err := checkpoint.ResolveCommittedReaderForCheckpoint(ctx, cpID, v1, v2, settings.IsCheckpointsV2Enabled(ctx)) + if err != nil || summary == nil { + lines = append(lines, fmt.Sprintf("- %s: checkpoint metadata unavailable", cpID)) + continue + } + detail := reviewCheckpointDetail(ctx, reader, cpID, summary) + if detail == "" { + detail = "no summary or prompt recorded" + } + lines = append(lines, fmt.Sprintf("- %s: %s", cpID, detail)) + } + if len(lines) == 0 { + return "" + } + + 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." +} + +func reviewCheckpointDetail( + ctx context.Context, + reader checkpoint.CommittedReader, + cpID checkpointid.CheckpointID, + summary *checkpoint.CheckpointSummary, +) string { + for i := len(summary.Sessions) - 1; i >= 0; i-- { + meta, err := readReviewContextSessionMetadata(ctx, reader, cpID, i) + if err != nil || meta == nil || session.Kind(meta.Kind).IsReview() { + continue + } + if text := reviewSummaryText(meta.Summary); text != "" { + return "summary: " + text + } + } + for i := len(summary.Sessions) - 1; i >= 0; i-- { + meta, err := readReviewContextSessionMetadata(ctx, reader, cpID, i) + if err != nil || meta == nil || session.Kind(meta.Kind).IsReview() { + continue + } + prompts, err := readReviewContextSessionPrompts(ctx, reader, cpID, i) + if err == nil { + if text := reviewPromptText(prompts); text != "" { + return "prompt: " + text + } + } + } + return "" +} + +func readReviewContextSessionMetadata( + ctx context.Context, + reader checkpoint.CommittedReader, + cpID checkpointid.CheckpointID, + sessionIndex int, +) (*checkpoint.CommittedMetadata, error) { + if r, ok := reader.(reviewContextSessionMetadataReader); ok { + return r.ReadSessionMetadata(ctx, cpID, sessionIndex) //nolint:wrapcheck // Best-effort prompt context. + } + content, err := reader.ReadSessionContent(ctx, cpID, sessionIndex) + if err != nil { + return nil, err //nolint:wrapcheck // Best-effort prompt context. + } + if content == nil { + return nil, errors.New("session content is nil") + } + return &content.Metadata, nil +} + +func readReviewContextSessionPrompts( + ctx context.Context, + reader checkpoint.CommittedReader, + cpID checkpointid.CheckpointID, + sessionIndex int, +) (string, error) { + if r, ok := reader.(reviewContextSessionMetadataPromptsReader); ok { + content, err := r.ReadSessionMetadataAndPrompts(ctx, cpID, sessionIndex) + if err != nil { + return "", err //nolint:wrapcheck // Best-effort prompt context. + } + if content == nil { + return "", errors.New("session content is nil") + } + return content.Prompts, nil + } + content, err := reader.ReadSessionContent(ctx, cpID, sessionIndex) + if err != nil { + return "", err //nolint:wrapcheck // Best-effort prompt context. + } + if content == nil { + return "", errors.New("session content is nil") + } + return content.Prompts, nil +} + +func reviewSummaryText(summary *checkpoint.Summary) string { + if summary == nil { + return "" + } + parts := []string{ + compactReviewContextText(summary.Intent), + compactReviewContextText(summary.Outcome), + } + for _, item := range summary.OpenItems { + if text := compactReviewContextText(item); text != "" { + parts = append(parts, "open: "+text) + break + } + } + return truncateReviewContextText(strings.Join(nonEmptyReviewContextParts(parts), "; ")) +} + +func reviewPromptText(promptContent string) string { + prompts := checkpoint.SplitPromptContent(promptContent) + for i := len(prompts) - 1; i >= 0; i-- { + if text := compactReviewContextText(prompts[i]); text != "" { + return truncateReviewContextText(text) + } + } + return "" +} + +func nonEmptyReviewContextParts(parts []string) []string { + result := parts[:0] + for _, part := range parts { + if part != "" { + result = append(result, part) + } + } + return result +} + +func compactReviewContextText(value string) string { + return strings.Join(strings.Fields(value), " ") +} + +func truncateReviewContextText(value string) string { + runes := []rune(value) + if len(runes) <= reviewContextMaxDetailRunes { + return value + } + return strings.TrimSpace(string(runes[:reviewContextMaxDetailRunes-3])) + "..." +} + +func reviewContextGitLines(ctx context.Context, repoRoot string, args ...string) ([]string, error) { + full := append([]string{"-C", repoRoot}, args...) + output, err := exec.CommandContext(ctx, "git", full...).Output() + if err != nil { + return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + trimmed := strings.TrimSpace(string(output)) + if trimmed == "" { + return nil, nil + } + return strings.Split(trimmed, "\n"), nil +} + +func reviewContextGitString(ctx context.Context, repoRoot string, args ...string) string { + lines, err := reviewContextGitLines(ctx, repoRoot, args...) + if err != nil { + return "" + } + return strings.Join(lines, "\n") +} diff --git a/cmd/entire/cli/review_context_test.go b/cmd/entire/cli/review_context_test.go new file mode 100644 index 000000000..e0fb576dc --- /dev/null +++ b/cmd/entire/cli/review_context_test.go @@ -0,0 +1,220 @@ +package cli + +import ( + "bytes" + "context" + "os" + "path/filepath" + "strings" + "testing" + + git "github.com/go-git/go-git/v6" + + "github.com/entireio/cli/cmd/entire/cli/agent" + "github.com/entireio/cli/cmd/entire/cli/agent/types" + "github.com/entireio/cli/cmd/entire/cli/checkpoint" + checkpointid "github.com/entireio/cli/cmd/entire/cli/checkpoint/id" + "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/testutil" + "github.com/entireio/cli/redact" +) + +func TestReviewCheckpointContext_IncludesSummaryAndPromptFallback(t *testing.T) { + t.Parallel() + + repoRoot := newReviewContextRepo(t) + const summaryCheckpointID = "a1b2c3d4e5f6" + writeReviewContextCheckpoint(t, repoRoot, summaryCheckpointID, reviewContextCheckpointOptions{ + filesTouched: []string{"summary.go"}, + agentType: agent.AgentTypeClaudeCode, + summary: &checkpoint.Summary{ + Intent: "add checkpoint context to review prompts", + Outcome: "review prompt sees checkpoint summaries", + OpenItems: []string{"cover prompt fallback"}, + }, + prompts: []string{"summary fallback prompt should not appear"}, + transcript: `{"event":"raw summary transcript"}` + "\n", + }) + commitReviewContextChange(t, repoRoot, "summary.go", "summary\n", "summary change", "Entire-Checkpoint: "+summaryCheckpointID) + + const promptCheckpointID = "b1b2c3d4e5f6" + writeReviewContextCheckpoint(t, repoRoot, promptCheckpointID, reviewContextCheckpointOptions{ + filesTouched: []string{"prompt.go"}, + agentType: agent.AgentTypeClaudeCode, + prompts: []string{"Implement prompt fallback when summaries are missing"}, + transcript: `{"event":"raw prompt transcript"}` + "\n", + }) + commitReviewContextChange(t, repoRoot, "prompt.go", "prompt\n", "prompt change", "Entire-Checkpoint: "+promptCheckpointID) + + got := reviewCheckpointContext(context.Background(), repoRoot, "master") + for _, want := range []string{ + "Checkpoint context from commits in scope:", + summaryCheckpointID, + "summary: add checkpoint context to review prompts; review prompt sees checkpoint summaries; open: cover prompt fallback", + promptCheckpointID, + "prompt: Implement prompt fallback when summaries are missing", + "entire explain ", + "entire explain --raw-transcript", + } { + if !strings.Contains(got, want) { + t.Fatalf("review checkpoint context missing %q:\n%s", want, got) + } + } + for _, unwanted := range []string{ + "summary fallback prompt should not appear", + "raw summary transcript", + "raw prompt transcript", + } { + if strings.Contains(got, unwanted) { + t.Fatalf("review checkpoint context contains %q:\n%s", unwanted, got) + } + } +} + +func TestReviewCommandSmoke_IncludesCheckpointContextInPrompt(t *testing.T) { + repoRoot := newReviewContextRepo(t) + t.Chdir(repoRoot) + paths.ClearWorktreeRootCache() + t.Cleanup(paths.ClearWorktreeRootCache) + + installReviewContextClaudeHooks(t) + writeReviewContextSettings(t, repoRoot) + + stubDir := t.TempDir() + promptPath := filepath.Join(t.TempDir(), "prompt.txt") + writeReviewContextClaudeStub(t, stubDir) + t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("ENTIRE_SMOKE_PROMPT_FILE", promptPath) + + const checkpointID = "f1b2c3d4e5f6" + writeReviewContextCheckpoint(t, repoRoot, checkpointID, reviewContextCheckpointOptions{ + filesTouched: []string{"checkpointed.go"}, + agentType: agent.AgentTypeClaudeCode, + summary: &checkpoint.Summary{ + Intent: "smoke checkpoint summary", + Outcome: "review smoke receives checkpoint summary", + }, + transcript: `{"event":"test"}` + "\n", + }) + commitReviewContextChange(t, repoRoot, "checkpointed.go", "checkpointed\n", "implement checkpointed change", "Entire-Checkpoint: "+checkpointID) + + cmd := NewRootCmd() + var out bytes.Buffer + var errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + cmd.SetArgs([]string{"review", "--agent", string(agent.AgentNameClaudeCode)}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("entire review failed: %v\nstdout:\n%s\nstderr:\n%s", err, out.String(), errOut.String()) + } + + promptBytes, err := os.ReadFile(promptPath) + if err != nil { + t.Fatalf("read captured prompt: %v\nstdout:\n%s\nstderr:\n%s", err, out.String(), errOut.String()) + } + prompt := string(promptBytes) + for _, want := range []string{ + "/review", + "Scope: review only the commits unique to this branch vs master.", + "Checkpoint context from commits in scope:", + checkpointID, + "summary: smoke checkpoint summary; review smoke receives checkpoint summary", + } { + if !strings.Contains(prompt, want) { + t.Fatalf("captured review prompt missing %q:\n%s", want, prompt) + } + } +} + +func newReviewContextRepo(t *testing.T) string { + t.Helper() + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + testutil.WriteFile(t, tmp, "base.txt", "base\n") + testutil.GitAdd(t, tmp, "base.txt") + testutil.GitCommit(t, tmp, "base") + testutil.GitCheckoutNewBranch(t, tmp, "feat/review") + return tmp +} + +func commitReviewContextChange(t *testing.T, repoRoot, path, content, subject, body string) { + t.Helper() + testutil.WriteFile(t, repoRoot, path, content) + testutil.GitAdd(t, repoRoot, path) + message := subject + if body != "" { + message += "\n\n" + body + } + testutil.GitCommit(t, repoRoot, message) +} + +type reviewContextCheckpointOptions struct { + filesTouched []string + agentType types.AgentType + summary *checkpoint.Summary + prompts []string + transcript string +} + +func writeReviewContextCheckpoint(t *testing.T, repoRoot string, checkpointID string, opts reviewContextCheckpointOptions) { + t.Helper() + repo, err := git.PlainOpen(repoRoot) + if err != nil { + t.Fatalf("open repo: %v", err) + } + cpID := checkpointid.MustCheckpointID(checkpointID) + err = checkpoint.NewGitStore(repo).WriteCommitted(context.Background(), checkpoint.WriteCommittedOptions{ + CheckpointID: cpID, + SessionID: checkpointID, + Strategy: "manual-commit", + Branch: "feat/review", + Transcript: redact.AlreadyRedacted([]byte(opts.transcript)), + Prompts: opts.prompts, + FilesTouched: opts.filesTouched, + CheckpointsCount: 1, + Agent: opts.agentType, + Summary: opts.summary, + }) + if err != nil { + t.Fatalf("write checkpoint: %v", err) + } +} + +func installReviewContextClaudeHooks(t *testing.T) { + t.Helper() + ag, err := agent.Get(agent.AgentNameClaudeCode) + if err != nil { + t.Fatalf("agent.Get(%q): %v", agent.AgentNameClaudeCode, err) + } + hs, ok := agent.AsHookSupport(ag) + if !ok { + t.Fatalf("agent %q does not support hooks", agent.AgentNameClaudeCode) + } + if _, err := hs.InstallHooks(context.Background(), false, false); err != nil { + t.Fatalf("InstallHooks(%q): %v", agent.AgentNameClaudeCode, err) + } +} + +func writeReviewContextSettings(t *testing.T, repoRoot string) { + t.Helper() + entireDir := filepath.Join(repoRoot, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("create .entire dir: %v", err) + } + settingsJSON := `{"enabled":true,"review":{"claude-code":{"skills":["/review"]}}}` + "\n" + if err := os.WriteFile(filepath.Join(entireDir, "settings.json"), []byte(settingsJSON), 0o600); err != nil { + t.Fatalf("write review settings: %v", err) + } +} + +func writeReviewContextClaudeStub(t *testing.T, stubDir string) { + t.Helper() + script := `#!/bin/sh +printf '%s' "$2" > "$ENTIRE_SMOKE_PROMPT_FILE" +printf 'smoke review ok\n' +` + if err := os.WriteFile(filepath.Join(stubDir, "claude"), []byte(script), 0o700); err != nil { + t.Fatalf("write claude stub: %v", err) + } +} From 52df8e0d7df806cc42ce297f93ce85aace70d0e2 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 14:19:38 -0700 Subject: [PATCH 2/7] fix(review): address checkpoint context feedback Entire-Checkpoint: fad39a003ff7 --- cmd/entire/cli/review/tui_model.go | 6 +- cmd/entire/cli/review_context.go | 127 ++++++++++++++++++-------- cmd/entire/cli/review_context_test.go | 116 +++++++++++++++++++++++ 3 files changed, 207 insertions(+), 42 deletions(-) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index e801dba44..0256b1a92 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -108,7 +108,7 @@ func (m reviewTUIModel) Init() tea.Cmd { } // Update handles all incoming messages. -func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. +func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case agentEventMsg: return m.handleAgentEvent(msg) @@ -166,7 +166,7 @@ func (m reviewTUIModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { //nolint:iret } // handleAgentEvent processes an agentEventMsg, updating the relevant row. -func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. +func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) { idx, ok := m.rowIdx[msg.agent] if !ok { return m, nil @@ -229,7 +229,7 @@ func (m reviewTUIModel) handleAgentEvent(msg agentEventMsg) (tea.Model, tea.Cmd) } // handleKey processes keyboard input. -func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { //nolint:ireturn // tea.Model is an interface; required by Bubble Tea. +func (m reviewTUIModel) handleKey(msg tea.KeyPressMsg) (tea.Model, tea.Cmd) { // Any key after finished dismisses. if m.finished { return m, tea.Quit diff --git a/cmd/entire/cli/review_context.go b/cmd/entire/cli/review_context.go index 68bffad84..72476c73d 100644 --- a/cmd/entire/cli/review_context.go +++ b/cmd/entire/cli/review_context.go @@ -6,6 +6,7 @@ import ( "fmt" "log/slog" "os/exec" + "strconv" "strings" git "github.com/go-git/go-git/v6" @@ -16,10 +17,16 @@ import ( "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/session" "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/stringutil" "github.com/entireio/cli/cmd/entire/cli/trailers" ) -const reviewContextMaxDetailRunes = 320 +const ( + reviewContextMaxDetailRunes = 320 + reviewContextMaxCheckpoints = 20 + reviewContextMaxCommitScans = 200 + reviewContextCommitSeparator = "\x1e" +) type reviewContextSessionMetadataReader interface { ReadSessionMetadata(ctx context.Context, checkpointID checkpointid.CheckpointID, sessionIndex int) (*checkpoint.CommittedMetadata, error) @@ -34,10 +41,10 @@ func reviewCheckpointContext(ctx context.Context, worktreeRoot string, scopeBase return "" } - commits, err := reviewContextGitLines(ctx, worktreeRoot, "rev-list", scopeBaseRef+"..HEAD") - if err != nil || len(commits) == 0 { + messages, commitsTruncated, err := reviewContextCommitMessages(ctx, worktreeRoot, scopeBaseRef, reviewContextMaxCommitScans) + if err != nil || len(messages) == 0 { if err != nil { - logging.Debug(ctx, "review checkpoint context: list commits", slog.String("error", err.Error())) + logging.Debug(ctx, "review checkpoint context: list commit messages", slog.String("error", err.Error())) } return "" } @@ -53,31 +60,44 @@ func reviewCheckpointContext(ctx context.Context, worktreeRoot string, scopeBase logging.Debug(ctx, "review checkpoint context: no v2 fetch remote", slog.String("error", urlErr.Error())) } v2 := checkpoint.NewV2GitStore(repo, v2URL) + preferCheckpointsV2 := settings.IsCheckpointsV2Enabled(ctx) var lines []string seen := map[checkpointid.CheckpointID]bool{} - for _, commit := range commits { - message := reviewContextGitString(ctx, worktreeRoot, "show", "-s", "--format=%B", commit) - cpID, ok := trailers.ParseCheckpoint(message) - if !ok || seen[cpID] { - continue - } - seen[cpID] = true + omittedCheckpoints := 0 + for _, message := range messages { + for _, cpID := range trailers.ParseAllCheckpoints(message) { + if seen[cpID] { + continue + } + seen[cpID] = true - reader, summary, err := checkpoint.ResolveCommittedReaderForCheckpoint(ctx, cpID, v1, v2, settings.IsCheckpointsV2Enabled(ctx)) - if err != nil || summary == nil { - lines = append(lines, fmt.Sprintf("- %s: checkpoint metadata unavailable", cpID)) - continue - } - detail := reviewCheckpointDetail(ctx, reader, cpID, summary) - if detail == "" { - detail = "no summary or prompt recorded" + if len(lines) >= reviewContextMaxCheckpoints { + omittedCheckpoints++ + continue + } + + reader, summary, err := checkpoint.ResolveCommittedReaderForCheckpoint(ctx, cpID, v1, v2, preferCheckpointsV2) + if err != nil || summary == nil { + lines = append(lines, fmt.Sprintf("- %s: checkpoint metadata unavailable", cpID)) + continue + } + detail := reviewCheckpointDetail(ctx, reader, cpID, summary) + if detail == "" { + detail = "no summary or prompt recorded" + } + lines = append(lines, fmt.Sprintf("- %s: %s", cpID, detail)) } - lines = append(lines, fmt.Sprintf("- %s: %s", cpID, detail)) } if len(lines) == 0 { return "" } + if omittedCheckpoints > 0 { + lines = append(lines, fmt.Sprintf("- ... %d more %s omitted", omittedCheckpoints, reviewContextCheckpointNoun(omittedCheckpoints))) + } + if commitsTruncated { + lines = append(lines, fmt.Sprintf("- ... older commits omitted after scanning latest %d commits", reviewContextMaxCommitScans)) + } return "Checkpoint context from commits in scope:\n" + strings.Join(lines, "\n") + @@ -90,21 +110,21 @@ func reviewCheckpointDetail( cpID checkpointid.CheckpointID, summary *checkpoint.CheckpointSummary, ) string { + sessions := make([]reviewContextSessionDetail, 0, len(summary.Sessions)) for i := len(summary.Sessions) - 1; i >= 0; i-- { meta, err := readReviewContextSessionMetadata(ctx, reader, cpID, i) if err != nil || meta == nil || session.Kind(meta.Kind).IsReview() { continue } + sessions = append(sessions, reviewContextSessionDetail{ + index: i, + }) if text := reviewSummaryText(meta.Summary); text != "" { return "summary: " + text } } - for i := len(summary.Sessions) - 1; i >= 0; i-- { - meta, err := readReviewContextSessionMetadata(ctx, reader, cpID, i) - if err != nil || meta == nil || session.Kind(meta.Kind).IsReview() { - continue - } - prompts, err := readReviewContextSessionPrompts(ctx, reader, cpID, i) + for _, sessionDetail := range sessions { + prompts, err := readReviewContextSessionPrompts(ctx, reader, cpID, sessionDetail.index) if err == nil { if text := reviewPromptText(prompts); text != "" { return "prompt: " + text @@ -114,6 +134,10 @@ func reviewCheckpointDetail( return "" } +type reviewContextSessionDetail struct { + index int +} + func readReviewContextSessionMetadata( ctx context.Context, reader checkpoint.CommittedReader, @@ -197,7 +221,7 @@ func nonEmptyReviewContextParts(parts []string) []string { } func compactReviewContextText(value string) string { - return strings.Join(strings.Fields(value), " ") + return stringutil.CollapseWhitespace(value) } func truncateReviewContextText(value string) string { @@ -208,23 +232,48 @@ func truncateReviewContextText(value string) string { return strings.TrimSpace(string(runes[:reviewContextMaxDetailRunes-3])) + "..." } -func reviewContextGitLines(ctx context.Context, repoRoot string, args ...string) ([]string, error) { - full := append([]string{"-C", repoRoot}, args...) - output, err := exec.CommandContext(ctx, "git", full...).Output() +func reviewContextCheckpointNoun(count int) string { + if count == 1 { + return "checkpoint" + } + return "checkpoints" +} + +func reviewContextCommitMessages(ctx context.Context, repoRoot string, scopeBaseRef string, maxCommits int) ([]string, bool, error) { + if maxCommits <= 0 { + return nil, false, nil + } + records, err := reviewContextGitRecords( + ctx, + repoRoot, + "log", + "--max-count="+strconv.Itoa(maxCommits+1), + "--format="+reviewContextCommitSeparator+"%B", + scopeBaseRef+"..HEAD", + ) if err != nil { - return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + return nil, false, err } - trimmed := strings.TrimSpace(string(output)) - if trimmed == "" { - return nil, nil + truncated := len(records) > maxCommits + if truncated { + records = records[:maxCommits] } - return strings.Split(trimmed, "\n"), nil + return records, truncated, nil } -func reviewContextGitString(ctx context.Context, repoRoot string, args ...string) string { - lines, err := reviewContextGitLines(ctx, repoRoot, args...) +func reviewContextGitRecords(ctx context.Context, repoRoot string, args ...string) ([]string, error) { + full := append([]string{"-C", repoRoot}, args...) + output, err := exec.CommandContext(ctx, "git", full...).Output() if err != nil { - return "" + return nil, fmt.Errorf("git %s: %w", strings.Join(args, " "), err) + } + parts := strings.Split(string(output), reviewContextCommitSeparator) + records := make([]string, 0, len(parts)) + for _, part := range parts { + trimmed := strings.TrimSpace(part) + if trimmed != "" { + records = append(records, trimmed) + } } - return strings.Join(lines, "\n") + return records, nil } diff --git a/cmd/entire/cli/review_context_test.go b/cmd/entire/cli/review_context_test.go index e0fb576dc..3c4e709a5 100644 --- a/cmd/entire/cli/review_context_test.go +++ b/cmd/entire/cli/review_context_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" "context" + "fmt" "os" "path/filepath" "strings" @@ -71,6 +72,73 @@ func TestReviewCheckpointContext_IncludesSummaryAndPromptFallback(t *testing.T) } } +func TestReviewCheckpointContext_CapsCheckpointLines(t *testing.T) { + t.Parallel() + + repoRoot := newReviewContextRepo(t) + var oldestCheckpointID string + for i := range reviewContextMaxCheckpoints + 1 { + checkpointID := fmt.Sprintf("c%011x", i) + if i == 0 { + oldestCheckpointID = checkpointID + } + writeReviewContextCheckpoint(t, repoRoot, checkpointID, reviewContextCheckpointOptions{ + filesTouched: []string{fmt.Sprintf("checkpoint-%02d.go", i)}, + agentType: agent.AgentTypeClaudeCode, + summary: &checkpoint.Summary{ + Intent: fmt.Sprintf("checkpoint summary %02d", i), + }, + transcript: `{"event":"test"}` + "\n", + }) + commitReviewContextChange( + t, + repoRoot, + fmt.Sprintf("checkpoint-%02d.go", i), + fmt.Sprintf("checkpoint %02d\n", i), + fmt.Sprintf("checkpoint change %02d", i), + "Entire-Checkpoint: "+checkpointID, + ) + } + + got := reviewCheckpointContext(context.Background(), repoRoot, "master") + if count := strings.Count(got, "summary: checkpoint summary"); count != reviewContextMaxCheckpoints { + t.Fatalf("checkpoint context summary count = %d, want %d:\n%s", count, reviewContextMaxCheckpoints, got) + } + if strings.Contains(got, oldestCheckpointID) { + t.Fatalf("checkpoint context includes oldest checkpoint %s despite cap:\n%s", oldestCheckpointID, got) + } + if !strings.Contains(got, "1 more checkpoint omitted") { + t.Fatalf("checkpoint context missing truncation notice:\n%s", got) + } +} + +func TestReviewCheckpointDetail_ReadsSessionMetadataOnceForPromptFallback(t *testing.T) { + t.Parallel() + + cpID := checkpointid.MustCheckpointID("d1b2c3d4e5f6") + reader := &countingReviewContextReader{ + metadata: checkpoint.CommittedMetadata{ + CheckpointID: cpID, + SessionID: "session-1", + }, + prompts: "Fallback prompt from checkpoint", + } + summary := &checkpoint.CheckpointSummary{ + Sessions: []checkpoint.SessionFilePaths{{}}, + } + + got := reviewCheckpointDetail(context.Background(), reader, cpID, summary) + if got != "prompt: Fallback prompt from checkpoint" { + t.Fatalf("reviewCheckpointDetail() = %q", got) + } + if reader.metadataCalls != 1 { + t.Fatalf("metadata calls = %d, want 1", reader.metadataCalls) + } + if reader.promptCalls != 1 { + t.Fatalf("prompt calls = %d, want 1", reader.promptCalls) + } +} + func TestReviewCommandSmoke_IncludesCheckpointContextInPrompt(t *testing.T) { repoRoot := newReviewContextRepo(t) t.Chdir(repoRoot) @@ -218,3 +286,51 @@ printf 'smoke review ok\n' t.Fatalf("write claude stub: %v", err) } } + +type countingReviewContextReader struct { + metadata checkpoint.CommittedMetadata + prompts string + metadataErr error + promptErr error + metadataCalls int + promptCalls int +} + +func (r *countingReviewContextReader) ReadCommitted( + context.Context, + checkpointid.CheckpointID, +) (*checkpoint.CheckpointSummary, error) { + return nil, checkpoint.ErrCheckpointNotFound +} + +func (r *countingReviewContextReader) ReadSessionContent( + context.Context, + checkpointid.CheckpointID, + int, +) (*checkpoint.SessionContent, error) { + return &checkpoint.SessionContent{ + Metadata: r.metadata, + Prompts: r.prompts, + }, nil +} + +func (r *countingReviewContextReader) ReadSessionMetadata( + context.Context, + checkpointid.CheckpointID, + int, +) (*checkpoint.CommittedMetadata, error) { + r.metadataCalls++ + return &r.metadata, r.metadataErr +} + +func (r *countingReviewContextReader) ReadSessionMetadataAndPrompts( + context.Context, + checkpointid.CheckpointID, + int, +) (*checkpoint.SessionContent, error) { + r.promptCalls++ + return &checkpoint.SessionContent{ + Metadata: r.metadata, + Prompts: r.prompts, + }, r.promptErr +} From acc0b9bf4f4c6e0c9f1dae6c45580ef4574bb531 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 14:35:26 -0700 Subject: [PATCH 3/7] fix(review): stabilize review context lint Entire-Checkpoint: 0341c011b42c --- .golangci.yaml | 3 +++ cmd/entire/cli/review_context.go | 12 ++++-------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/.golangci.yaml b/.golangci.yaml index e2934ba28..7536b1ef8 100644 --- a/.golangci.yaml +++ b/.golangci.yaml @@ -156,6 +156,9 @@ linters: - path: ^internal/sim/ linters: - wrapcheck + - path: ^cmd/entire/cli/review/tui_model\.go$ + linters: + - ireturn - text: "`strat` is a misspelling" linters: - misspell diff --git a/cmd/entire/cli/review_context.go b/cmd/entire/cli/review_context.go index 72476c73d..49476b938 100644 --- a/cmd/entire/cli/review_context.go +++ b/cmd/entire/cli/review_context.go @@ -188,11 +188,11 @@ func reviewSummaryText(summary *checkpoint.Summary) string { return "" } parts := []string{ - compactReviewContextText(summary.Intent), - compactReviewContextText(summary.Outcome), + stringutil.CollapseWhitespace(summary.Intent), + stringutil.CollapseWhitespace(summary.Outcome), } for _, item := range summary.OpenItems { - if text := compactReviewContextText(item); text != "" { + if text := stringutil.CollapseWhitespace(item); text != "" { parts = append(parts, "open: "+text) break } @@ -203,7 +203,7 @@ func reviewSummaryText(summary *checkpoint.Summary) string { func reviewPromptText(promptContent string) string { prompts := checkpoint.SplitPromptContent(promptContent) for i := len(prompts) - 1; i >= 0; i-- { - if text := compactReviewContextText(prompts[i]); text != "" { + if text := stringutil.CollapseWhitespace(prompts[i]); text != "" { return truncateReviewContextText(text) } } @@ -220,10 +220,6 @@ func nonEmptyReviewContextParts(parts []string) []string { return result } -func compactReviewContextText(value string) string { - return stringutil.CollapseWhitespace(value) -} - func truncateReviewContextText(value string) string { runes := []rune(value) if len(runes) <= reviewContextMaxDetailRunes { From 3a8d8748aba56e98034aedef1d62ad31f1ceb895 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 15:15:32 -0700 Subject: [PATCH 4/7] fix(review): show single-agent progress and invoke codex review Entire-Checkpoint: d192da863bd9 --- cmd/entire/cli/agent/codex/reviewer.go | 34 +++- cmd/entire/cli/agent/codex/reviewer_test.go | 45 +++++ cmd/entire/cli/review/cmd.go | 62 ++++++- cmd/entire/cli/review/cmd_test.go | 184 ++++++++++++++++++++ cmd/entire/cli/review/export_test.go | 20 +++ cmd/entire/cli/review/picker.go | 2 +- cmd/entire/cli/review/tui_model.go | 4 +- cmd/entire/cli/review/tui_sink.go | 5 +- cmd/entire/cli/settings/settings.go | 6 +- 9 files changed, 343 insertions(+), 19 deletions(-) diff --git a/cmd/entire/cli/agent/codex/reviewer.go b/cmd/entire/cli/agent/codex/reviewer.go index 9d20d75e0..83067aece 100644 --- a/cmd/entire/cli/agent/codex/reviewer.go +++ b/cmd/entire/cli/agent/codex/reviewer.go @@ -30,13 +30,43 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { // buildCodexReviewCmd builds the exec.Cmd for a codex review run. // Exposed at package level for test inspection of argv, stdin, and env. func buildCodexReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd { - prompt := review.ComposeReviewPrompt(cfg) - cmd := exec.CommandContext(ctx, "codex", "exec", "--skip-git-repo-check", "-") + promptCfg := cfg + args := []string{"exec", "--skip-git-repo-check", "-"} + if usesCodexBuiltinReview(cfg.Skills) { + promptCfg.Skills = withoutCodexBuiltinReview(cfg.Skills) + args = []string{"exec", "review", "--skip-git-repo-check"} + if cfg.ScopeBaseRef != "" { + args = append(args, "--base", cfg.ScopeBaseRef) + } + args = append(args, "-") + } + prompt := review.ComposeReviewPrompt(promptCfg) + cmd := exec.CommandContext(ctx, "codex", args...) cmd.Stdin = strings.NewReader(prompt) cmd.Env = review.AppendReviewEnv(os.Environ(), "codex", cfg, prompt) return cmd } +func usesCodexBuiltinReview(skills []string) bool { + for _, skill := range skills { + if skill == "/review" { + return true + } + } + return false +} + +func withoutCodexBuiltinReview(skills []string) []string { + out := make([]string, 0, len(skills)) + for _, skill := range skills { + if skill == "/review" { + continue + } + out = append(out, skill) + } + return out +} + // parseCodexOutput wraps the reader with the chrome filter and converts // remaining lines into a stream of Events. // On clean EOF emits Finished{Success: true}. On a scanner error (including diff --git a/cmd/entire/cli/agent/codex/reviewer_test.go b/cmd/entire/cli/agent/codex/reviewer_test.go index 71087dfed..2edfc5320 100644 --- a/cmd/entire/cli/agent/codex/reviewer_test.go +++ b/cmd/entire/cli/agent/codex/reviewer_test.go @@ -86,6 +86,42 @@ func TestCodexReviewer_ArgvShape(t *testing.T) { } } +func TestCodexReviewer_BuiltinReviewUsesExecReview(t *testing.T) { + t.Parallel() + cfg := reviewtypes.RunConfig{ + Skills: []string{"/review"}, + AlwaysPrompt: "Focus on auth regressions.", + ScopeBaseRef: "main", + CheckpointContext: "Commits in scope (newest first):\n abc123 summary\n", + } + cmd := buildCodexReviewCmd(context.Background(), cfg) + + want := []string{wantCodexAgentName, "exec", "review", "--skip-git-repo-check", "--base", "main", "-"} + if len(cmd.Args) != len(want) { + t.Fatalf("len(Args) = %d, want %d: %v", len(cmd.Args), len(want), cmd.Args) + } + for i, w := range want { + if cmd.Args[i] != w { + t.Errorf("Args[%d] = %q, want %q", i, cmd.Args[i], w) + } + } + + prompt := readCodexCmdStdin(t, cmd) + if strings.Contains(prompt, "/review") { + t.Fatalf("builtin review prompt should not include /review when using codex exec review:\n%s", prompt) + } + for _, wantText := range []string{ + "Focus on auth regressions.", + "Scope: review only the commits unique to this branch vs main.", + "Commits in scope (newest first):", + "abc123 summary", + } { + if !strings.Contains(prompt, wantText) { + t.Fatalf("builtin review prompt missing %q:\n%s", wantText, prompt) + } + } +} + func TestCodexReviewer_NoBinaryRequiredAtConstruction(t *testing.T) { // No t.Parallel — uses t.Setenv. t.Setenv("PATH", "") @@ -256,6 +292,15 @@ func drainCodexEvents(ch <-chan reviewtypes.Event) { } } +func readCodexCmdStdin(t *testing.T, cmd *exec.Cmd) string { + t.Helper() + b, err := io.ReadAll(cmd.Stdin) + if err != nil { + t.Fatalf("read stdin: %v", err) + } + return string(b) +} + func envToMap(env []string) map[string]string { m := make(map[string]string, len(env)) for _, e := range env { diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index db7b44ad4..c61559b43 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -334,12 +334,11 @@ func runSingleAgentPath( } runCfg := reviewtypes.RunConfig{ - PromptOverride: cfg.Prompt, - Skills: cfg.Skills, ScopeBaseRef: scopeBaseRef, CheckpointContext: checkpointContext, StartingSHA: headSHA, } + applyReviewConfig(&runCfg, cfg) // 7. Branch on launchability. reviewer := deps.ReviewerFor(agentName) @@ -348,8 +347,24 @@ func runSingleAgentPath( return RunMarkerFallback(ctx, agentName, runCfg, worktreeRoot, out) } - _, waitErr := Run(ctx, reviewer, runCfg, []reviewtypes.Sink{DumpSink{W: out}}) - if waitErr != nil && ctx.Err() == nil { + runCtx, cancelRun := context.WithCancel(ctx) + defer cancelRun() + + canPrompt := interactive.CanPromptInteractively() + sinks := composeSingleAgentSinks(singleAgentSinkInputs{ + out: out, + isTTY: interactive.IsTerminalWriter(out) && canPrompt, + canPrompt: canPrompt, + agentName: agentName, + cancelRun: cancelRun, + }) + if tuiSink, ok := findTUISink(sinks); ok { + tuiSink.Start() + defer tuiSink.Wait() + } + + _, waitErr := Run(runCtx, reviewer, runCfg, sinks) + if waitErr != nil && runCtx.Err() == nil && ctx.Err() == nil { // Non-cancellation error: surface to caller. return fmt.Errorf("review run: %w", waitErr) } @@ -441,14 +456,12 @@ func runMultiAgentPath( // RunConfig before forwarding to the underlying reviewer. reviewers = append(reviewers, &perAgentConfiguredReviewer{ inner: reviewer, - cfg: reviewtypes.RunConfig{ - PromptOverride: agentCfg.Prompt, - Skills: agentCfg.Skills, + cfg: runConfigWithReviewConfig(reviewtypes.RunConfig{ PerRunPrompt: picked.PerRun, ScopeBaseRef: scopeBaseRef, CheckpointContext: checkpointContext, StartingSHA: headSHA, - }, + }, agentCfg), }) } @@ -533,6 +546,14 @@ type multiAgentSinkInputs struct { perRunPrompt string } +type singleAgentSinkInputs struct { + out io.Writer + isTTY bool + canPrompt bool + agentName string + cancelRun context.CancelFunc +} + // composeMultiAgentSinks builds the sink slice for a multi-agent run. // // - Non-TTY: [DumpSink] alone — narrative dump only, no live UI, no prompts. @@ -565,6 +586,31 @@ func composeMultiAgentSinks(in multiAgentSinkInputs) []reviewtypes.Sink { return sinks } +func composeSingleAgentSinks(in singleAgentSinkInputs) []reviewtypes.Sink { + if !in.isTTY || !in.canPrompt { + fmt.Fprintf(in.out, "Running review with %s...\n", in.agentName) + return []reviewtypes.Sink{DumpSink{W: in.out}} + } + return []reviewtypes.Sink{ + NewTUISink([]string{in.agentName}, in.cancelRun, in.out), + DumpSink{W: in.out}, + } +} + +func runConfigWithReviewConfig(base reviewtypes.RunConfig, cfg settings.ReviewConfig) reviewtypes.RunConfig { + applyReviewConfig(&base, cfg) + return base +} + +func applyReviewConfig(runCfg *reviewtypes.RunConfig, cfg settings.ReviewConfig) { + runCfg.Skills = cfg.Skills + if len(cfg.Skills) == 0 { + runCfg.PromptOverride = cfg.Prompt + return + } + runCfg.AlwaysPrompt = cfg.Prompt +} + // findTUISink returns the first *TUISink in the slice (if any). Used by the // caller to wire Start/Wait around the run without re-running composition. func findTUISink(sinks []reviewtypes.Sink) (*TUISink, bool) { diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index 2062e0fcc..e6103bb98 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -347,6 +347,105 @@ func (p *stubDispatchProcess) Wait() error { return nil } var _ reviewtypes.AgentReviewer = (*stubDispatchReviewer)(nil) var _ reviewtypes.Process = (*stubDispatchProcess)(nil) +type captureRunConfigReviewer struct { + name string + got reviewtypes.RunConfig +} + +func (r *captureRunConfigReviewer) Name() string { return r.name } +func (r *captureRunConfigReviewer) Start(_ context.Context, cfg reviewtypes.RunConfig) (reviewtypes.Process, error) { + r.got = cfg + return &stubDispatchProcess{}, nil +} + +func TestRunReview_ConfigPromptAugmentsSelectedSkills(t *testing.T) { + setupCmdTestRepo(t) + + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + "claude-code": { + Skills: []string{"/review"}, + Prompt: "Focus on auth regressions.", + }, + }); err != nil { + t.Fatal(err) + } + + reviewer := &captureRunConfigReviewer{name: "claude-code"} + deps := review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { + return []types.AgentName{"claude-code"} + }, + NewSilentError: func(err error) error { return err }, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { + return false, "" + }, + ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { + if agentName == "claude-code" { + return reviewer + } + return nil + }, + } + + cmd := review.NewCommand(deps) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if reviewer.got.PromptOverride != "" { + t.Fatalf("PromptOverride = %q, want empty so skills still run", reviewer.got.PromptOverride) + } + if reviewer.got.AlwaysPrompt != "Focus on auth regressions." { + t.Fatalf("AlwaysPrompt = %q, want saved prompt as additional instructions", reviewer.got.AlwaysPrompt) + } + if len(reviewer.got.Skills) != 1 || reviewer.got.Skills[0] != "/review" { + t.Fatalf("Skills = %v, want [/review]", reviewer.got.Skills) + } +} + +func TestRunReview_SingleAgentNonTTYPrintsRunningLine(t *testing.T) { + setupCmdTestRepo(t) + + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + "claude-code": {Skills: []string{"/review"}}, + }); err != nil { + t.Fatal(err) + } + + reviewer := &captureRunConfigReviewer{name: "claude-code"} + deps := review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { + return []types.AgentName{"claude-code"} + }, + NewSilentError: func(err error) error { return err }, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { + return false, "" + }, + ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { + if agentName == "claude-code" { + 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("unexpected error: %v", err) + } + if !strings.Contains(out.String(), "Running review with claude-code...") { + t.Fatalf("output missing running line:\n%s", out.String()) + } +} + // 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). @@ -648,6 +747,91 @@ func TestComposeMultiAgentSinks(t *testing.T) { } } +func TestComposeSingleAgentSinks(t *testing.T) { + t.Parallel() + + noopCancel := func() {} + provider := &stubCmdSynthesisProvider{} + + tests := []struct { + name string + isTTY bool + canPrompt bool + wantTUI bool + wantDump bool + wantSynth bool + wantTotal int + wantOutput string + }{ + { + name: "non-tty prints running line and uses dump only", + wantDump: true, + wantTotal: 1, + wantOutput: "Running review with agent-a...", + }, + { + name: "tty uses tui and dump", + isTTY: true, + canPrompt: true, + wantTUI: true, + wantDump: true, + wantTotal: 2, + }, + { + name: "tty without prompt falls back to running line", + isTTY: true, + canPrompt: false, + wantDump: true, + wantTotal: 1, + wantOutput: "Running review with agent-a...", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + out := &bytes.Buffer{} + sinks := review.ExposedComposeSingleAgentSinks(review.SingleAgentSinkComposeInputs{ + Out: out, + IsTTY: tt.isTTY, + CanPrompt: tt.canPrompt, + AgentName: "agent-a", + CancelRun: noopCancel, + SynthesisProvider: provider, + }) + if got := len(sinks); got != tt.wantTotal { + t.Fatalf("len(sinks)=%d, want %d", got, tt.wantTotal) + } + _, hasTUI := review.ExposedFindTUISink(sinks) + if hasTUI != tt.wantTUI { + t.Errorf("findTUISink found=%v, want %v", hasTUI, tt.wantTUI) + } + var hasDump, hasSynth bool + for _, s := range sinks { + switch s.(type) { + case review.DumpSink: + hasDump = true + case review.SynthesisSink: + hasSynth = true + } + } + if hasDump != tt.wantDump { + t.Errorf("DumpSink present=%v, want %v", hasDump, tt.wantDump) + } + if hasSynth != tt.wantSynth { + t.Errorf("SynthesisSink present=%v, want %v", hasSynth, tt.wantSynth) + } + if tt.wantOutput != "" && !strings.Contains(out.String(), tt.wantOutput) { + t.Errorf("output missing %q:\n%s", tt.wantOutput, out.String()) + } + if tt.wantOutput == "" && out.Len() != 0 { + t.Errorf("expected no pre-run output, got:\n%s", out.String()) + } + }) + } +} + // TestFindTUISink_NoTUIInSlice covers the not-found path so the caller's // `if tuiSink, ok := findTUISink(sinks); ok` branch is exercised in both // directions. diff --git a/cmd/entire/cli/review/export_test.go b/cmd/entire/cli/review/export_test.go index 26e35e2c9..908c73ce5 100644 --- a/cmd/entire/cli/review/export_test.go +++ b/cmd/entire/cli/review/export_test.go @@ -26,6 +26,15 @@ type SinkComposeInputs struct { PerRunPrompt string } +type SingleAgentSinkComposeInputs struct { + Out io.Writer + IsTTY bool + CanPrompt bool + AgentName string + CancelRun context.CancelFunc + SynthesisProvider SynthesisProvider +} + // ExposedComposeMultiAgentSinks exposes composeMultiAgentSinks for tests. func ExposedComposeMultiAgentSinks(in SinkComposeInputs) []reviewtypes.Sink { return composeMultiAgentSinks(multiAgentSinkInputs{ @@ -40,6 +49,17 @@ func ExposedComposeMultiAgentSinks(in SinkComposeInputs) []reviewtypes.Sink { }) } +// ExposedComposeSingleAgentSinks exposes composeSingleAgentSinks for tests. +func ExposedComposeSingleAgentSinks(in SingleAgentSinkComposeInputs) []reviewtypes.Sink { + return composeSingleAgentSinks(singleAgentSinkInputs{ + out: in.Out, + isTTY: in.IsTTY, + canPrompt: in.CanPrompt, + agentName: in.AgentName, + cancelRun: in.CancelRun, + }) +} + // ExposedFindTUISink exposes findTUISink for tests. func ExposedFindTUISink(sinks []reviewtypes.Sink) (*TUISink, bool) { return findTUISink(sinks) diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index f47246a74..01571e3e4 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -501,7 +501,7 @@ func BuildReviewPickerFields( text := huh.NewText(). Title("Additional instructions (optional)"). - Description("Used verbatim as the review prompt when set. Leave blank to use the default 'run these skills in order' template.") + Description("Added after selected skills. If no skills are selected, this becomes the full review prompt.") if promptOut != nil { *promptOut = previousPrompt text = text.Value(promptOut) diff --git a/cmd/entire/cli/review/tui_model.go b/cmd/entire/cli/review/tui_model.go index 0256b1a92..e2b950e65 100644 --- a/cmd/entire/cli/review/tui_model.go +++ b/cmd/entire/cli/review/tui_model.go @@ -1,7 +1,7 @@ // Package review — see env.go for package-level rationale. // // tui_model.go provides reviewTUIModel, the Bubble Tea Model for the -// multi-agent review dashboard. The model renders a per-agent status table +// review dashboard. The model renders a per-agent status table // during the run and supports Ctrl+O drill-in mode for inspecting one agent's // live event buffer on the alt screen. package review @@ -48,7 +48,7 @@ type runFinishedMsg struct { // tickMsg triggers spinner and duration column updates. type tickMsg time.Time -// reviewTUIModel is the Bubble Tea model for the multi-agent review dashboard. +// reviewTUIModel is the Bubble Tea model for the review dashboard. type reviewTUIModel struct { rows []agentRow rowIdx map[string]int // agent name → row index (O(1) lookup) diff --git a/cmd/entire/cli/review/tui_sink.go b/cmd/entire/cli/review/tui_sink.go index caf92b0cf..ad3d29602 100644 --- a/cmd/entire/cli/review/tui_sink.go +++ b/cmd/entire/cli/review/tui_sink.go @@ -1,10 +1,9 @@ // Package review — see env.go for package-level rationale. // // tui_sink.go provides TUISink, a Sink implementation that renders a Bubble -// Tea status dashboard during a multi-agent review run and supports Ctrl+O +// Tea status dashboard during a review run and supports Ctrl+O // drill-in mode for inspecting one agent's live event buffer. Used in -// interactive (TTY) multi-agent runs; non-TTY runs and single-agent runs use -// DumpSink instead. +// interactive (TTY) runs; non-TTY runs use DumpSink instead. package review import ( diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 54128e7c8..3654f5ccd 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -204,9 +204,9 @@ type ReviewConfig struct { // for this agent. May be empty when Prompt carries the full request. Skills []string `json:"skills,omitempty"` - // Prompt, when non-empty, is used verbatim as the review prompt - // instead of the skills-composed template. Lets users include - // context (e.g. "focus on security issues, then run /X"). + // Prompt, when non-empty, carries saved review instructions. When + // Skills is non-empty it is appended after the selected skills; when + // Skills is empty it is the full prompt for prompt-only review configs. Prompt string `json:"prompt,omitempty"` } From 7f2c27027eee48937ca65387749be487f553cf1b Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 15:32:45 -0700 Subject: [PATCH 5/7] test(review): trim review flow regression coverage Entire-Checkpoint: 41d5c4dd4836 --- cmd/entire/cli/review/cmd_test.go | 57 +++++----------------------- cmd/entire/cli/review/export_test.go | 11 +++--- 2 files changed, 14 insertions(+), 54 deletions(-) diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index e6103bb98..89eb95e40 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -387,8 +387,9 @@ func TestRunReview_ConfigPromptAugmentsSelectedSkills(t *testing.T) { }, } + out := &bytes.Buffer{} cmd := review.NewCommand(deps) - cmd.SetOut(&bytes.Buffer{}) + cmd.SetOut(out) cmd.SetErr(&bytes.Buffer{}) cmd.SetArgs([]string{}) @@ -404,43 +405,6 @@ func TestRunReview_ConfigPromptAugmentsSelectedSkills(t *testing.T) { if len(reviewer.got.Skills) != 1 || reviewer.got.Skills[0] != "/review" { t.Fatalf("Skills = %v, want [/review]", reviewer.got.Skills) } -} - -func TestRunReview_SingleAgentNonTTYPrintsRunningLine(t *testing.T) { - setupCmdTestRepo(t) - - if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ - "claude-code": {Skills: []string{"/review"}}, - }); err != nil { - t.Fatal(err) - } - - reviewer := &captureRunConfigReviewer{name: "claude-code"} - deps := review.Deps{ - GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { - return []types.AgentName{"claude-code"} - }, - NewSilentError: func(err error) error { return err }, - HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { - return false, "" - }, - ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { - if agentName == "claude-code" { - 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("unexpected error: %v", err) - } if !strings.Contains(out.String(), "Running review with claude-code...") { t.Fatalf("output missing running line:\n%s", out.String()) } @@ -751,7 +715,6 @@ func TestComposeSingleAgentSinks(t *testing.T) { t.Parallel() noopCancel := func() {} - provider := &stubCmdSynthesisProvider{} tests := []struct { name string @@ -759,7 +722,6 @@ func TestComposeSingleAgentSinks(t *testing.T) { canPrompt bool wantTUI bool wantDump bool - wantSynth bool wantTotal int wantOutput string }{ @@ -793,12 +755,11 @@ func TestComposeSingleAgentSinks(t *testing.T) { out := &bytes.Buffer{} sinks := review.ExposedComposeSingleAgentSinks(review.SingleAgentSinkComposeInputs{ - Out: out, - IsTTY: tt.isTTY, - CanPrompt: tt.canPrompt, - AgentName: "agent-a", - CancelRun: noopCancel, - SynthesisProvider: provider, + Out: out, + IsTTY: tt.isTTY, + CanPrompt: tt.canPrompt, + AgentName: "agent-a", + CancelRun: noopCancel, }) if got := len(sinks); got != tt.wantTotal { t.Fatalf("len(sinks)=%d, want %d", got, tt.wantTotal) @@ -819,8 +780,8 @@ func TestComposeSingleAgentSinks(t *testing.T) { if hasDump != tt.wantDump { t.Errorf("DumpSink present=%v, want %v", hasDump, tt.wantDump) } - if hasSynth != tt.wantSynth { - t.Errorf("SynthesisSink present=%v, want %v", hasSynth, tt.wantSynth) + if hasSynth { + t.Error("SynthesisSink should not be present for single-agent reviews") } if tt.wantOutput != "" && !strings.Contains(out.String(), tt.wantOutput) { t.Errorf("output missing %q:\n%s", tt.wantOutput, out.String()) diff --git a/cmd/entire/cli/review/export_test.go b/cmd/entire/cli/review/export_test.go index 908c73ce5..15a5cc2c1 100644 --- a/cmd/entire/cli/review/export_test.go +++ b/cmd/entire/cli/review/export_test.go @@ -27,12 +27,11 @@ type SinkComposeInputs struct { } type SingleAgentSinkComposeInputs struct { - Out io.Writer - IsTTY bool - CanPrompt bool - AgentName string - CancelRun context.CancelFunc - SynthesisProvider SynthesisProvider + Out io.Writer + IsTTY bool + CanPrompt bool + AgentName string + CancelRun context.CancelFunc } // ExposedComposeMultiAgentSinks exposes composeMultiAgentSinks for tests. From 73b7e882276599fdb293ecdc53fef6db8189d036 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 17:28:35 -0700 Subject: [PATCH 6/7] fix(review): restore agent output flow Ensure review picker options remain visible and selected across agents. Expand Codex built-in review prompts, stream Codex assistant/tool events, and harden markdown rendering so code-block output cannot crash review dumps. Entire-Checkpoint: 80ad30770d35 --- .../cli/agent/claudecode/reviewer_test.go | 5 + cmd/entire/cli/agent/codex/output_filter.go | 38 ++--- .../cli/agent/codex/output_filter_test.go | 39 +++-- cmd/entire/cli/agent/codex/reviewer.go | 153 +++++++++++++++--- cmd/entire/cli/agent/codex/reviewer_test.go | 94 ++++++++++- cmd/entire/cli/mdrender/mdrender.go | 28 ++-- cmd/entire/cli/mdrender/mdrender_test.go | 29 ++++ cmd/entire/cli/review/cmd_test.go | 87 +++++++++- cmd/entire/cli/review/export_test.go | 6 + cmd/entire/cli/review/multipicker.go | 13 +- cmd/entire/cli/review/multipicker_test.go | 21 +++ cmd/entire/cli/review/picker.go | 15 +- cmd/entire/cli/review/picker_test.go | 31 ++++ 13 files changed, 482 insertions(+), 77 deletions(-) diff --git a/cmd/entire/cli/agent/claudecode/reviewer_test.go b/cmd/entire/cli/agent/claudecode/reviewer_test.go index d9dd17229..5b696e2e2 100644 --- a/cmd/entire/cli/agent/claudecode/reviewer_test.go +++ b/cmd/entire/cli/agent/claudecode/reviewer_test.go @@ -95,6 +95,11 @@ func TestReviewer_ArgvShape(t *testing.T) { if cmd.Args[2] == "" { t.Error("Args[2] (prompt) is empty") } + for _, arg := range cmd.Args { + if arg == "--continue" || arg == "-c" || arg == "--resume" || arg == "-r" { + t.Fatalf("Args must start a fresh Claude review, got resume/continue flag in %v", cmd.Args) + } + } // Stdin must be nil — claude receives prompt via argv, not stdin. if cmd.Stdin != nil { t.Errorf("cmd.Stdin = %v, want nil (claude uses argv, not stdin)", cmd.Stdin) diff --git a/cmd/entire/cli/agent/codex/output_filter.go b/cmd/entire/cli/agent/codex/output_filter.go index 1c3cdcbf9..8c83fce6e 100644 --- a/cmd/entire/cli/agent/codex/output_filter.go +++ b/cmd/entire/cli/agent/codex/output_filter.go @@ -101,9 +101,8 @@ func Strip(r io.Reader) io.Reader { scanner := bufio.NewScanner(r) scanner.Buffer(make([]byte, 1024*1024), 16*1024*1024) state := codexStripNormal - var currentAssistant []string for scanner.Scan() { - done, err := collectFinalCodexLine(scanner.Text(), &state, ¤tAssistant, pw) + done, err := collectFinalCodexLine(scanner.Text(), &state, pw) if err != nil { _ = pw.CloseWithError(err) return @@ -116,12 +115,6 @@ func Strip(r io.Reader) io.Reader { _ = pw.CloseWithError(err) return } - if state != codexStripAfterTokens { - if err := writeCodexAssistantBlock(pw, currentAssistant); err != nil { - _ = pw.CloseWithError(err) - return - } - } _ = pw.Close() }() return pr @@ -137,7 +130,7 @@ const ( codexStripAfterTokens ) -func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant *[]string, w io.Writer) (bool, error) { +func collectFinalCodexLine(raw string, state *codexStripState, w io.Writer) (bool, error) { cleaned := csiRegex.ReplaceAllString(raw, "") trimmed := strings.TrimSpace(cleaned) trimmedRight := strings.TrimRight(cleaned, " \t") @@ -146,26 +139,24 @@ func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant return true, nil } if isTokensUsedMarker(trimmed) { - return true, writeCodexAssistantBlock(w, *currentAssistant) + return true, nil } switch *state { case codexStripUserBlock: if isCodexRoleMarker(trimmed) { *state = codexStripAssistantBlock - *currentAssistant = nil } return false, nil case codexStripAssistantBlock: if isCodexRoleMarker(trimmed) { - *currentAssistant = nil return false, nil } if isUserRoleMarker(trimmed) { *state = codexStripUserBlock return false, nil } - if trimmedRight == "exec" || execBlockRegex.MatchString(trimmedRight) { + if trimmedRight == codexExecCommand || execBlockRegex.MatchString(trimmedRight) { *state = codexStripExecBlock return false, nil } @@ -173,7 +164,9 @@ func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant return false, nil } if line, ok := FilterLine(raw); ok { - *currentAssistant = append(*currentAssistant, line) + if err := writeCodexLine(w, line); err != nil { + return false, err + } } return false, nil case codexStripExecBlock: @@ -183,7 +176,6 @@ func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant } if isCodexRoleMarker(trimmed) { *state = codexStripAssistantBlock - *currentAssistant = nil return false, nil } return false, nil @@ -199,13 +191,12 @@ func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant } if isCodexRoleMarker(trimmed) { *state = codexStripAssistantBlock - *currentAssistant = nil return false, nil } if isCodexMetadataLine(trimmed) { return false, nil } - if trimmedRight == "exec" { + if trimmedRight == codexExecCommand { *state = codexStripExecBlock return false, nil } @@ -213,14 +204,17 @@ func collectFinalCodexLine(raw string, state *codexStripState, currentAssistant return false, nil } + if line, ok := FilterLine(raw); ok { + if err := writeCodexLine(w, line); err != nil { + return false, err + } + } return false, nil } -func writeCodexAssistantBlock(w io.Writer, lines []string) error { - for _, line := range lines { - if _, err := w.Write([]byte(line + "\n")); err != nil { - return fmt.Errorf("write filtered codex output: %w", err) - } +func writeCodexLine(w io.Writer, line string) error { + if _, err := w.Write([]byte(line + "\n")); err != nil { + return fmt.Errorf("write filtered codex output: %w", err) } return nil } diff --git a/cmd/entire/cli/agent/codex/output_filter_test.go b/cmd/entire/cli/agent/codex/output_filter_test.go index 4c5cdf329..ff043c0a3 100644 --- a/cmd/entire/cli/agent/codex/output_filter_test.go +++ b/cmd/entire/cli/agent/codex/output_filter_test.go @@ -182,6 +182,31 @@ func TestFilterLine_VersionNarrativeKept(t *testing.T) { } } +func TestStrip_PlainExecOutputPassesThrough(t *testing.T) { + t.Parallel() + + input := strings.Join([]string{ + "Review findings:", + "- Missing regression coverage in review picker.", + "- Codex output should appear in the final dump.", + }, "\n") + + data, err := io.ReadAll(Strip(strings.NewReader(input))) + if err != nil { + t.Fatalf("Strip read: %v", err) + } + output := string(data) + for _, want := range []string{ + "Review findings:", + "- Missing regression coverage in review picker.", + "- Codex output should appear in the final dump.", + } { + if !strings.Contains(output, want) { + t.Fatalf("plain output missing %q:\n%s", want, output) + } + } +} + func TestStrip_FullFixture(t *testing.T) { t.Parallel() @@ -229,6 +254,8 @@ func TestStrip_FullFixture(t *testing.T) { // Narrative must survive. narrativeMustSurvive := []string{ + "This is the narrative output from the agent.", + "It spans multiple lines.", "Final conclusion: no issues found.", } for _, want := range narrativeMustSurvive { @@ -236,14 +263,6 @@ func TestStrip_FullFixture(t *testing.T) { t.Errorf("narrative %q missing from filtered output; got:\n%s", want, output) } } - for _, unwanted := range []string{ - "This is the narrative output from the agent.", - "It spans multiple lines.", - } { - if strings.Contains(output, unwanted) { - t.Errorf("pre-final narrative %q should not appear in filtered output; got:\n%s", unwanted, output) - } - } } func TestStrip_DropsExecBlocksAndDuplicateSummary(t *testing.T) { @@ -291,7 +310,6 @@ func TestStrip_DropsExecBlocksAndDuplicateSummary(t *testing.T) { "OpenAI Codex", "workdir:", "Please run these review skills", - "I will inspect the code.", "git status", "cmd/entire/cli/review.go", "go test ./...", @@ -306,6 +324,9 @@ func TestStrip_DropsExecBlocksAndDuplicateSummary(t *testing.T) { if strings.Count(output, "No findings.") != 1 { t.Fatalf("filtered output should contain final response once, got:\n%s", output) } + if !strings.Contains(output, "I will inspect the code.") { + t.Fatalf("filtered output missing live assistant progress line:\n%s", output) + } if !strings.Contains(output, "Residual risk: tests were not run in this sandbox.") { t.Fatalf("filtered output missing final residual-risk line:\n%s", output) } diff --git a/cmd/entire/cli/agent/codex/reviewer.go b/cmd/entire/cli/agent/codex/reviewer.go index 83067aece..6887b92d9 100644 --- a/cmd/entire/cli/agent/codex/reviewer.go +++ b/cmd/entire/cli/agent/codex/reviewer.go @@ -31,15 +31,8 @@ func NewReviewer() *reviewtypes.ReviewerTemplate { // Exposed at package level for test inspection of argv, stdin, and env. func buildCodexReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.Cmd { promptCfg := cfg - args := []string{"exec", "--skip-git-repo-check", "-"} - if usesCodexBuiltinReview(cfg.Skills) { - promptCfg.Skills = withoutCodexBuiltinReview(cfg.Skills) - args = []string{"exec", "review", "--skip-git-repo-check"} - if cfg.ScopeBaseRef != "" { - args = append(args, "--base", cfg.ScopeBaseRef) - } - args = append(args, "-") - } + promptCfg.Skills = expandCodexBuiltinReview(cfg.Skills) + args := []string{codexExecCommand, "--skip-git-repo-check", "-"} prompt := review.ComposeReviewPrompt(promptCfg) cmd := exec.CommandContext(ctx, "codex", args...) cmd.Stdin = strings.NewReader(prompt) @@ -47,19 +40,19 @@ func buildCodexReviewCmd(ctx context.Context, cfg reviewtypes.RunConfig) *exec.C return cmd } -func usesCodexBuiltinReview(skills []string) bool { - for _, skill := range skills { - if skill == "/review" { - return true - } - } - return false -} +// Codex's native `exec review --base ` rejects an additional prompt, +// so expand `/review` into text and run normal `codex exec -`. That preserves +// Entire's scoped base clause, per-run instructions, and checkpoint context. +const codexBuiltinReviewPrompt = "Review the current branch changes and report actionable findings. " + + "Prioritize correctness, regressions, security, and missing test coverage. Do not make code changes." -func withoutCodexBuiltinReview(skills []string) []string { +const codexExecCommand = "exec" + +func expandCodexBuiltinReview(skills []string) []string { out := make([]string, 0, len(skills)) for _, skill := range skills { if skill == "/review" { + out = append(out, codexBuiltinReviewPrompt) continue } out = append(out, skill) @@ -79,14 +72,13 @@ func parseCodexOutput(r io.Reader) <-chan reviewtypes.Event { go func() { defer close(out) out <- reviewtypes.Started{} - scanner := bufio.NewScanner(Strip(r)) + scanner := bufio.NewScanner(r) scanner.Buffer(make([]byte, 1024*1024), 16*1024*1024) + state := codexEventNormal for scanner.Scan() { - line := scanner.Text() - if line == "" { - continue + for _, ev := range collectCodexEventsLine(scanner.Text(), &state) { + out <- ev } - out <- reviewtypes.AssistantText{Text: line} } if err := scanner.Err(); err != nil { out <- reviewtypes.RunError{Err: fmt.Errorf("read stdout: %w", err)} @@ -97,3 +89,118 @@ func parseCodexOutput(r io.Reader) <-chan reviewtypes.Event { }() return out } + +type codexEventState int + +const ( + codexEventNormal codexEventState = iota + codexEventUserBlock + codexEventAssistantBlock + codexEventExecAwaitCommand + codexEventExecBlock + codexEventAfterTokens +) + +func collectCodexEventsLine(raw string, state *codexEventState) []reviewtypes.Event { + cleaned := csiRegex.ReplaceAllString(raw, "") + trimmed := strings.TrimSpace(cleaned) + trimmedRight := strings.TrimRight(cleaned, " \t") + + if *state == codexEventAfterTokens { + return nil + } + if isTokensUsedMarker(trimmed) { + *state = codexEventAfterTokens + return nil + } + + switch *state { + case codexEventUserBlock: + if isCodexRoleMarker(trimmed) { + *state = codexEventAssistantBlock + } + return nil + case codexEventAssistantBlock: + return collectCodexAssistantLine(raw, trimmed, trimmedRight, state) + case codexEventExecAwaitCommand: + return collectCodexExecCommandLine(trimmed, trimmedRight, state) + case codexEventExecBlock: + switch { + case trimmed == "": + *state = codexEventNormal + case isCodexRoleMarker(trimmed): + *state = codexEventAssistantBlock + case isUserRoleMarker(trimmed): + *state = codexEventUserBlock + } + return nil + case codexEventNormal: + // Continue below. + case codexEventAfterTokens: + return nil + } + + if isUserRoleMarker(trimmed) { + *state = codexEventUserBlock + return nil + } + if isCodexRoleMarker(trimmed) { + *state = codexEventAssistantBlock + return nil + } + if isCodexMetadataLine(trimmed) { + return nil + } + if trimmedRight == codexExecCommand { + *state = codexEventExecAwaitCommand + return nil + } + if execBlockRegex.MatchString(trimmedRight) { + *state = codexEventExecBlock + return []reviewtypes.Event{reviewtypes.ToolCall{Name: codexExecCommand, Args: trimmedRight}} + } + if line, ok := FilterLine(raw); ok { + return []reviewtypes.Event{reviewtypes.AssistantText{Text: line}} + } + return nil +} + +func collectCodexAssistantLine(raw, trimmed, trimmedRight string, state *codexEventState) []reviewtypes.Event { + switch { + case isCodexRoleMarker(trimmed): + return nil + case isUserRoleMarker(trimmed): + *state = codexEventUserBlock + return nil + case trimmedRight == codexExecCommand: + *state = codexEventExecAwaitCommand + return nil + case execBlockRegex.MatchString(trimmedRight): + *state = codexEventExecBlock + return []reviewtypes.Event{reviewtypes.ToolCall{Name: codexExecCommand, Args: trimmedRight}} + case isCodexMetadataLine(trimmed): + return nil + default: + if line, ok := FilterLine(raw); ok { + return []reviewtypes.Event{reviewtypes.AssistantText{Text: line}} + } + return nil + } +} + +func collectCodexExecCommandLine(trimmed, trimmedRight string, state *codexEventState) []reviewtypes.Event { + switch { + case trimmed == "": + *state = codexEventNormal + return nil + case isCodexRoleMarker(trimmed): + *state = codexEventAssistantBlock + return nil + case isUserRoleMarker(trimmed): + *state = codexEventUserBlock + return nil + default: + *state = codexEventExecBlock + return []reviewtypes.Event{reviewtypes.ToolCall{Name: codexExecCommand, Args: trimmedRight}} + } +} diff --git a/cmd/entire/cli/agent/codex/reviewer_test.go b/cmd/entire/cli/agent/codex/reviewer_test.go index 2edfc5320..c47e3a01c 100644 --- a/cmd/entire/cli/agent/codex/reviewer_test.go +++ b/cmd/entire/cli/agent/codex/reviewer_test.go @@ -8,6 +8,7 @@ import ( "os/exec" "strings" "testing" + "time" "github.com/entireio/cli/cmd/entire/cli/review" reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" @@ -86,7 +87,7 @@ func TestCodexReviewer_ArgvShape(t *testing.T) { } } -func TestCodexReviewer_BuiltinReviewUsesExecReview(t *testing.T) { +func TestCodexReviewer_BuiltinReviewExpandsToScopedExecPrompt(t *testing.T) { t.Parallel() cfg := reviewtypes.RunConfig{ Skills: []string{"/review"}, @@ -96,7 +97,7 @@ func TestCodexReviewer_BuiltinReviewUsesExecReview(t *testing.T) { } cmd := buildCodexReviewCmd(context.Background(), cfg) - want := []string{wantCodexAgentName, "exec", "review", "--skip-git-repo-check", "--base", "main", "-"} + want := []string{wantCodexAgentName, "exec", "--skip-git-repo-check", "-"} if len(cmd.Args) != len(want) { t.Fatalf("len(Args) = %d, want %d: %v", len(cmd.Args), len(want), cmd.Args) } @@ -108,9 +109,10 @@ func TestCodexReviewer_BuiltinReviewUsesExecReview(t *testing.T) { prompt := readCodexCmdStdin(t, cmd) if strings.Contains(prompt, "/review") { - t.Fatalf("builtin review prompt should not include /review when using codex exec review:\n%s", prompt) + t.Fatalf("builtin review prompt should not include raw /review:\n%s", prompt) } for _, wantText := range []string{ + "Review the current branch changes and report actionable findings.", "Focus on auth regressions.", "Scope: review only the commits unique to this branch vs main.", "Commits in scope (newest first):", @@ -230,10 +232,17 @@ func TestCodexReviewer_EventStream(t *testing.T) { // Verify narrative content appears and chrome is absent. var combined strings.Builder + sawToolCall := false for _, ev := range events { - if at, ok := ev.(reviewtypes.AssistantText); ok { + switch e := ev.(type) { + case reviewtypes.AssistantText: + at := e combined.WriteString(at.Text) combined.WriteString("\n") + case reviewtypes.ToolCall: + if e.Name == codexExecCommand && strings.Contains(e.Args, "git status --short") { + sawToolCall = true + } } } text := combined.String() @@ -252,7 +261,6 @@ func TestCodexReviewer_EventStream(t *testing.T) { "workdir:", "[hooks]", "firing user-prompt-submit", - "I will inspect the reviewer contracts.", "git status", "go test ./cmd/entire/cli/review", "TestExample", @@ -266,6 +274,12 @@ func TestCodexReviewer_EventStream(t *testing.T) { if strings.Count(text, "No findings.") != 1 { t.Errorf("final response should appear once after duplicate summary filtering; got:\n%s", text) } + if !strings.Contains(text, "I will inspect the reviewer contracts.") { + t.Error("expected live assistant progress in AssistantText events") + } + if !sawToolCall { + t.Errorf("expected exec block to emit a ToolCall event; got %#v", events) + } // CSI escape sequences must not leak into AssistantText events. for _, ev := range events { @@ -277,6 +291,76 @@ func TestCodexReviewer_EventStream(t *testing.T) { } } +func TestParseCodexOutput_StreamsEventsBeforeEOF(t *testing.T) { + t.Parallel() + + r, w := io.Pipe() + events := parseCodexOutput(r) + + select { + case ev, ok := <-events: + if !ok { + t.Fatal("events closed before Started event arrived") + } + if _, ok := ev.(reviewtypes.Started); !ok { + t.Fatalf("first event = %T, want Started", ev) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for Started event") + } + + input := strings.Join([]string{ + "codex", + "I will inspect the code before finalizing.", + "exec", + `/bin/zsh -lc "git status --short" in /repo`, + }, "\n") + "\n" + if _, err := w.Write([]byte(input)); err != nil { + t.Fatalf("write streaming input: %v", err) + } + + sawText := false + sawToolCall := false + deadline := time.After(2 * time.Second) + for !sawText || !sawToolCall { + select { + case ev, ok := <-events: + if !ok { + t.Fatalf("events closed before streaming assertions passed") + } + switch e := ev.(type) { + case reviewtypes.AssistantText: + if strings.Contains(e.Text, "I will inspect the code") { + sawText = true + } + case reviewtypes.ToolCall: + if e.Name == codexExecCommand && strings.Contains(e.Args, "git status --short") { + sawToolCall = true + } + } + case <-deadline: + t.Fatalf("timed out waiting for streaming events before EOF; sawText=%v sawToolCall=%v", sawText, sawToolCall) + } + } + + if err := w.Close(); err != nil { + t.Fatalf("close writer: %v", err) + } + sawFinished := false + for ev := range events { + if fin, ok := ev.(reviewtypes.Finished); ok { + if !fin.Success { + t.Fatalf("Finished.Success = false, want true") + } + sawFinished = true + } + } + if !sawFinished { + t.Fatal("expected Finished event after EOF") + } +} + +//nolint:ireturn // test helper intentionally records heterogeneous event values. func collectCodexEvents(ch <-chan reviewtypes.Event) []reviewtypes.Event { var events []reviewtypes.Event for ev := range ch { diff --git a/cmd/entire/cli/mdrender/mdrender.go b/cmd/entire/cli/mdrender/mdrender.go index 040315123..d78fe1c74 100644 --- a/cmd/entire/cli/mdrender/mdrender.go +++ b/cmd/entire/cli/mdrender/mdrender.go @@ -34,8 +34,16 @@ const DefaultTerminalWidth = 80 // // Errors only on glamour renderer construction or render failure — both // of which indicate a malformed StyleConfig (programmer error) rather -// than a runtime condition. -func Render(markdown string, width int, darkBackground bool) (string, error) { +// 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) { + defer func() { + if r := recover(); r != nil { + rendered = "" + err = fmt.Errorf("render markdown panic: %v", r) + } + }() + renderer, err := glamour.NewTermRenderer( glamour.WithStyles(stylesForBackground(darkBackground)), glamour.WithWordWrap(width), @@ -44,7 +52,7 @@ func Render(markdown string, width int, darkBackground bool) (string, error) { if err != nil { return "", fmt.Errorf("initialize markdown renderer: %w", err) } - rendered, err := renderer.Render(markdown) + rendered, err = renderer.Render(markdown) if err != nil { return "", fmt.Errorf("render markdown: %w", err) } @@ -167,12 +175,12 @@ func chromaForBackground(darkBackground bool) *ansi.Chroma { textColor := "#2A2A2A" commentColor := "#8D8D8D" punctColor := "#7A7A7A" - bgColor := "254" + bgColor := "#E4E4E4" if darkBackground { - textColor = "252" - commentColor = "245" - punctColor = "244" - bgColor = "236" + textColor = "#D0D0D0" + commentColor = "#8A8A8A" + punctColor = "#808080" + bgColor = "#303030" } return &ansi.Chroma{ Text: ansi.StylePrimitive{Color: strPtr(textColor)}, @@ -188,8 +196,8 @@ func chromaForBackground(darkBackground bool) *ansi.Chroma { LiteralNumber: ansi.StylePrimitive{Color: strPtr("#fbbf24")}, Operator: ansi.StylePrimitive{Color: strPtr(punctColor)}, Punctuation: ansi.StylePrimitive{Color: strPtr(punctColor)}, - GenericDeleted: ansi.StylePrimitive{Color: strPtr("1")}, - GenericInserted: ansi.StylePrimitive{Color: strPtr("2")}, + GenericDeleted: ansi.StylePrimitive{Color: strPtr("#EF4444")}, + GenericInserted: ansi.StylePrimitive{Color: strPtr("#22C55E")}, Background: ansi.StylePrimitive{BackgroundColor: strPtr(bgColor)}, } } diff --git a/cmd/entire/cli/mdrender/mdrender_test.go b/cmd/entire/cli/mdrender/mdrender_test.go index b1bd31e6a..3a3c7fc3d 100644 --- a/cmd/entire/cli/mdrender/mdrender_test.go +++ b/cmd/entire/cli/mdrender/mdrender_test.go @@ -45,6 +45,35 @@ func TestRender_LightBackgroundAlsoProducesAnsi(t *testing.T) { } } +func TestRender_CodeBlockDoesNotPanic(t *testing.T) { + t.Parallel() + + const md = "```go\nfunc main() {\n\tprintln(\"ok\")\n}\n```\n" + cases := []struct { + name string + dark bool + }{ + {name: "dark", dark: true}, + {name: "light", dark: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + out, err := mdrender.Render(md, 80, tc.dark) + if err != nil { + t.Fatalf("Render: %v", err) + } + if !strings.Contains(out, "func") { + t.Errorf("expected rendered code block to contain code, got: %q", out) + } + if !strings.Contains(out, "\x1b[") { + t.Errorf("expected rendered code block to contain ANSI styling, got: %q", out) + } + }) + } +} + // TestRenderForWriter_NonTTYReturnsRawMarkdown verifies the TTY-aware path // passes markdown through unchanged when w is a *bytes.Buffer (not a TTY). // This is the path entire review uses when stdout is redirected, so the diff --git a/cmd/entire/cli/review/cmd_test.go b/cmd/entire/cli/review/cmd_test.go index 89eb95e40..ff1cb53ea 100644 --- a/cmd/entire/cli/review/cmd_test.go +++ b/cmd/entire/cli/review/cmd_test.go @@ -348,12 +348,14 @@ var _ reviewtypes.AgentReviewer = (*stubDispatchReviewer)(nil) var _ reviewtypes.Process = (*stubDispatchProcess)(nil) type captureRunConfigReviewer struct { - name string - got reviewtypes.RunConfig + name string + called bool + got reviewtypes.RunConfig } func (r *captureRunConfigReviewer) Name() string { return r.name } func (r *captureRunConfigReviewer) Start(_ context.Context, cfg reviewtypes.RunConfig) (reviewtypes.Process, error) { + r.called = true r.got = cfg return &stubDispatchProcess{}, nil } @@ -450,6 +452,87 @@ func TestDispatchFork_TwoLaunchableNoOverride(t *testing.T) { } } +func TestDispatchFork_MultiAgentPassesPerAgentConfigs(t *testing.T) { + setupCmdTestRepo(t) + + if err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ + "claude-code": { + Skills: []string{"/review"}, + Prompt: "Claude saved prompt.", + }, + "codex": { + Skills: []string{"/review"}, + Prompt: "Codex saved prompt.", + }, + }); err != nil { + t.Fatal(err) + } + + claudeReviewer := &captureRunConfigReviewer{name: "claude-code"} + codexReviewer := &captureRunConfigReviewer{name: "codex"} + multiPickerFn := func(_ context.Context, _ []review.AgentChoice) (review.PickedAgents, error) { + return review.PickedAgents{ + Names: []string{"claude-code", "codex"}, + PerRun: "Focus this run on regressions.", + }, nil + } + + deps := review.Deps{ + GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName { + return []types.AgentName{"claude-code", "codex"} + }, + NewSilentError: func(err error) error { return err }, + MultiPickerFn: multiPickerFn, + HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) { + return false, "" + }, + ReviewerFor: func(agentName string) reviewtypes.AgentReviewer { + switch agentName { + case "claude-code": + return claudeReviewer + case "codex": + return codexReviewer + default: + return nil + } + }, + } + + cmd := review.NewCommand(deps) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetArgs([]string{}) + + if err := cmd.Execute(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + for _, tc := range []struct { + name string + reviewer *captureRunConfigReviewer + wantPrompt string + }{ + {name: "claude-code", reviewer: claudeReviewer, wantPrompt: "Claude saved prompt."}, + {name: "codex", reviewer: codexReviewer, wantPrompt: "Codex saved prompt."}, + } { + if !tc.reviewer.called { + t.Fatalf("%s reviewer was not started", tc.name) + } + if got := tc.reviewer.got.Skills; len(got) != 1 || got[0] != "/review" { + t.Fatalf("%s Skills = %v, want [/review]", tc.name, got) + } + 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 diff --git a/cmd/entire/cli/review/export_test.go b/cmd/entire/cli/review/export_test.go index 15a5cc2c1..42926a160 100644 --- a/cmd/entire/cli/review/export_test.go +++ b/cmd/entire/cli/review/export_test.go @@ -4,6 +4,8 @@ import ( "context" "io" + "charm.land/huh/v2" + reviewtypes "github.com/entireio/cli/cmd/entire/cli/review/types" ) @@ -59,6 +61,10 @@ 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/multipicker.go b/cmd/entire/cli/review/multipicker.go index 612e2aca1..b292813fd 100644 --- a/cmd/entire/cli/review/multipicker.go +++ b/cmd/entire/cli/review/multipicker.go @@ -64,10 +64,7 @@ func PickAgents(ctx context.Context, eligible []AgentChoice) (PickedAgents, erro var picked []string multiForm := newAccessibleForm(huh.NewGroup( - huh.NewMultiSelect[string](). - Title("Which agents should run this review?"). - Options(options...). - Value(&picked), + buildAgentMultiSelect(options, &picked), )) if err := multiForm.RunWithContext(ctx); err != nil { return PickedAgents{}, ErrPickerCancelled @@ -96,3 +93,11 @@ func PickAgents(ctx context.Context, eligible []AgentChoice) (PickedAgents, erro 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 index 26f86f01a..dd8cd99fd 100644 --- a/cmd/entire/cli/review/multipicker_test.go +++ b/cmd/entire/cli/review/multipicker_test.go @@ -3,8 +3,11 @@ package review_test import ( "context" "errors" + "strings" "testing" + "charm.land/huh/v2" + "github.com/entireio/cli/cmd/entire/cli/review" ) @@ -73,3 +76,21 @@ func TestPickedAgentsSentinels(t *testing.T) { 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 01571e3e4..144f8ccfa 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -442,6 +442,11 @@ func BuildReviewPickerFields( ) []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) @@ -454,7 +459,10 @@ func BuildReviewPickerFields( } opts = append(opts, opt) } - ms := huh.NewMultiSelect[string]().Title("Built-in commands").Options(opts...) + ms := huh.NewMultiSelect[string](). + Title("Built-in commands"). + Options(opts...). + Height(len(opts) + 1) if builtinPicksOut != nil { ms = ms.Value(builtinPicksOut) } @@ -474,7 +482,10 @@ func BuildReviewPickerFields( } opts = append(opts, opt) } - ms := huh.NewMultiSelect[string]().Title("Installed plugin skills").Options(opts...) + ms := huh.NewMultiSelect[string](). + Title("Installed plugin skills"). + Options(opts...). + Height(len(opts) + 1) if discoveredPicksOut != nil { ms = ms.Value(discoveredPicksOut) } diff --git a/cmd/entire/cli/review/picker_test.go b/cmd/entire/cli/review/picker_test.go index b13eed97b..49f8a210b 100644 --- a/cmd/entire/cli/review/picker_test.go +++ b/cmd/entire/cli/review/picker_test.go @@ -258,6 +258,37 @@ func TestBuildReviewPickerFields_HintSectionOmittedWhenEmpty(t *testing.T) { } } +func TestBuildReviewPickerFields_SingleBuiltinDefaultsSelectedAndRenders(t *testing.T) { + t.Parallel() + + var builtinPicks []string + fields := review.BuildReviewPickerFields( + "codex", + []skilldiscovery.CuratedSkill{{Name: "/review", Desc: "Review current changes"}}, + nil, + nil, + "", + &builtinPicks, nil, nil, + ) + + if len(fields) == 0 { + t.Fatal("expected picker fields") + } + got, ok := fields[0].GetValue().([]string) + if !ok { + t.Fatalf("built-in field value has type %T, want []string", fields[0].GetValue()) + } + if !reflect.DeepEqual(got, []string{"/review"}) { + t.Fatalf("built-in defaults = %v, want [/review]", got) + } + + field := fields[0].WithWidth(80) + field.Focus() + if got := field.View(); !strings.Contains(got, "/review") { + t.Fatalf("single built-in option did not render:\n%s", got) + } +} + // TestSaveReviewConfig_PersistsSettings verifies SaveReviewConfig writes and // the settings can be read back. func TestSaveReviewConfig_PersistsSettings(t *testing.T) { From da536804f868dcf7986776dad62e4bca2ae58ef0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 6 May 2026 17:38:14 -0700 Subject: [PATCH 7/7] fix(review): remove stale lint suppression Entire-Checkpoint: 72ed6df497ff --- cmd/entire/cli/agent/codex/reviewer_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/entire/cli/agent/codex/reviewer_test.go b/cmd/entire/cli/agent/codex/reviewer_test.go index c47e3a01c..02f135857 100644 --- a/cmd/entire/cli/agent/codex/reviewer_test.go +++ b/cmd/entire/cli/agent/codex/reviewer_test.go @@ -360,7 +360,6 @@ func TestParseCodexOutput_StreamsEventsBeforeEOF(t *testing.T) { } } -//nolint:ireturn // test helper intentionally records heterogeneous event values. func collectCodexEvents(ch <-chan reviewtypes.Event) []reviewtypes.Event { var events []reviewtypes.Event for ev := range ch {