fix(review): restore interactive setup and Codex defaults · Entire

fix(review): restore interactive setup and Codex defaults

a71b44a→main·

dipree·3d ago·6 files·+217 added/-29 removed

Sessions

01KXGQW0RDC3PF5QKT23GN0B83View transcript

?\ Fix Review Interactive Setup and DefaultsPi·GPT-5.6-sol·7 steps

Changes

6

79 unmodified lines

80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96

79 unmodified lines

os.Getenv("GIT_TERMINAL_PROMPT") == "0"
}

// IsTerminalReader reports whether r is an *os.File backed by a terminal.
// It is useful when an explicitly interactive command needs to distinguish a
// human at stdin from an agent process that merely inherited a controlling TTY.
func IsTerminalReader(r io.Reader) bool {
    f, ok := r.(*os.File)
    if !ok {
        return false
    }
    return term.IsTerminal(int(f.Fd())) //nolint:gosec // G115: uintptr->int is safe for fd
}

// IsTerminalWriter reports whether w is an *os.File backed by a terminal.
// Use for deciding on color, pager, progress bars, or other writer-scoped
// TTY formatting. For "can I prompt the user?" use CanPromptInteractively.

Mcmd/entire/cli/interactive/interactive.go+11

209 unmodified lines

210
211
212
213
213
214
215
216
53 unmodified lines

270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
53 unmodified lines

350
351
352
332
353
354
355
356
419 unmodified lines

776
777
778
758
779
780
781
782
22 unmodified lines

805
806
807
787
808
809
810
811
272 unmodified lines

1084
1085
1086
1066
1087
1088
1089
1069
1090
1091
1092
1093
162 unmodified lines

1256
1257
1258
1238
1259
1260
1261
1262
67 unmodified lines

1330
1331
1332
1312
1313
1314
1315
1316
1333
1334
1335
1336
1337
1338
1339

209 unmodified lines

}, deps)
        }
        if edit {
            if !interactive.IsTerminalWriter(cmd.OutOrStdout()) || !interactive.CanPromptInteractively() {
                if !reviewCommandIsInteractive(cmd) {
                    err := errors.New("--edit requires an interactive terminal")
                    cmd.SilenceUsage = true
                    fmt.Fprintln(cmd.ErrOrStderr(), "--edit requires an interactive terminal.")
53 unmodified lines

Slots  []string // reviewer slots as "agent[=model]" entries (--set-slot)
}

// reviewCommandIsInteractive treats a real terminal on both stdin and stdout
// as authoritative for this explicitly user-invoked command. This avoids
// suppressing the review wizard when a normal shell inherits an agent sentinel
// or GIT_TERMINAL_PROMPT=0. The fallback preserves controlling-TTY detection
// for callers whose stdio is not wired directly to the terminal.
func reviewCommandIsInteractive(cmd *cobra.Command) bool {
    testTTY := os.Getenv(interactive.EnvTestTTY)
    ci := os.Getenv("CI")
    hardDisabled := (testTTY != "" && testTTY != "1") || (ci != "" && ci != "false")
    return reviewTTYIsInteractive(
        interactive.IsTerminalReader(cmd.InOrStdin()),
        interactive.IsTerminalWriter(cmd.OutOrStdout()),
        interactive.CanPromptInteractively(),
        hardDisabled,
    )
}

func reviewTTYIsInteractive(stdinTTY, stdoutTTY, canPrompt, hardDisabled bool) bool {
    return !hardDisabled && stdoutTTY && (stdinTTY || canPrompt)
}

func (o reviewConfigureOptions) scripted() bool {
    // Local selects the destination only; by itself it must not force the
    // non-interactive/scripted path. `entire review --configure --local` should
53 unmodified lines

// duplicate the catalog here. Pass the raw --profile value (empty when not
    // given) so the guided setup runs the "what kind of review?" type picker
    // instead of being silently defaulted to the general profile.
    if interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively() {
        if reviewCommandIsInteractive(cmd) {
            name, profile, setupErr := RunReviewGuidedSetup(ctx, out, installed, deps.ReviewerFor, strings.TrimSpace(profileOverride), false, s)
            if setupErr != nil {
                return handlePickerError(cmd, silentErr, setupErr)
            }
        }
419 unmodified lines

applyLegacyReviewProfileFallback(s)

profileOverride = strings.TrimSpace(profileOverride)
    interactiveTTY := interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively()
    interactiveTTY := reviewCommandIsInteractive(cmd)

// Bare `entire review` never auto-runs a profile. Without a TTY we cannot
    // prompt, so list the profiles (or point at setup) and require an explicit
22 unmodified lines

// Non-interactive first run writes the shared project settings; interactive
        // setup asks the user where to save below.
        saveScope := reviewScopeProject
        guidedSetup := interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively()
        guidedSetup := interactiveTTY
        if guidedSetup {
            var setupErr error
            profileForSetup, profile, setupErr = RunReviewGuidedSetup(ctx, out, installed, deps.ReviewerFor, profileForSetup, true, s)
            if setupErr != nil {
                return handlePickerError(cmd, silentErr, setupErr)
            }
272 unmodified lines

defer cancelRun()

runCfg.EnrichSummary = reviewSummaryTokenEnricher(worktreeRoot, headSHA)
        canPrompt := interactive.CanPromptInteractively()
        canPrompt := reviewCommandIsInteractive(cmd)
        sinks := composeSingleAgentSinks(singleAgentSinkInputs{
            out:       out,
            isTTY:     interactive.IsTerminalWriter(out) && canPrompt,
            isTTY:     canPrompt,
            canPrompt: canPrompt,
            agentName: displayName,
            cancelRun: cancelRun,
        })
        
        masterLabel := judgeLabel(judge)
            sinks := composeMultiAgentSinks(multiAgentSinkInputs{
            out:               out,
            isTTY:             interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively(),
            isTTY:             reviewCommandIsInteractive(cmd),
            agentNames:        agentNames,
            cancelRun:         cancelRun,
            runContext:        runCtx,
        })

// instead of monkey-patching interactive helpers at run time.

// isTTY here means "the TUI sink is safe to compose" — production callers
// AND IsTerminalWriter(out) with CanPromptInteractively() before passing
// it in, since the TUI both writes ANSI to stdout AND reads keypresses
// from stdin. A terminal-stdout-but-non-interactive-stdin scenario (an
// agent host like Claude Code invoking `entire review`) must NOT use the
// TUI — its dismissal loop would block forever.
// use reviewCommandIsInteractive before passing it in, since the TUI both
// writes ANSI to stdout and reads keypresses from stdin. A terminal stdout
// with non-interactive stdin must not use the TUI; its dismissal loop would
// block forever.
type multiAgentSinkInputs struct {
    out               io.Writer
    isTTY             bool
}

// TestDispatchFork_LegacyGeneratedCodexSkillIsRepairedAndLaunched prevents
// guided setup's historical /review default from silently removing Codex from
// a multi-agent run. The compatibility repair must reach dispatch, not merely
// make the profile look valid in listing/configuration code.
func TestDispatchFork_LegacyGeneratedCodexSkillIsRepairedAndLaunched(t *testing.T) {
    setupCmdTestRepo(t)
    t.Setenv("HOME", t.TempDir())

if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{
        testAgentName: {Skills: []string{"/review"}},
        testCodexAgent: {
            Skills: []string{"/review"},
        },
    }); err != nil {
        t.Fatal(err)
    }

claudeReviewer := &captureRunConfigReviewer{name: testAgentName}
    codexReviewer := &captureRunConfigReviewer{name: testCodexAgent}
    deps := review.Deps{
        GetAgentsWithHooksInstalled: func(_ context.Context) []types.AgentName {
            return []types.AgentName{testAgentName, testCodexAgent}
        },
        NewSilentError: func(err error) error { return err },
        HeadHasReviewCheckpoint: func(_ context.Context) (bool, string) {
            return false, ""
        },
        ReviewerFor: func(agentName string) reviewtypes.AgentReviewer {
            switch agentName {
            case testAgentName:
                return claudeReviewer
            case testCodexAgent:
                return codexReviewer
            default:
                return nil
            }
        },
    }

cmd := review.NewCommand(deps)
    cmd.SetOut(&bytes.Buffer{})
    errBuf := &bytes.Buffer{}
    cmd.SetErr(errBuf)
    cmd.SetArgs([]string{"general"})

if err := cmd.Execute(); err != nil {
            t.Fatalf("run legacy generated profile: %v", err)
    }
    if !codexReviewer.called {
            t.Fatalf("Codex was silently excluded; stderr:\n%s", errBuf.String())
    }
    if len(codexReviewer.got.Skills) != 0 {
            t.Fatalf("Codex received obsolete generated skills %v, want none", codexReviewer.got.Skills)
    }
    if codexReviewer.got.AlwaysPrompt != "Review the change according to the profile task." {
            t.Fatalf("Codex repaired prompt = %q", codexReviewer.got.AlwaysPrompt)
    }
    if strings.Contains(errBuf.String(), "skipping reviewer codex") {
            t.Fatalf("Codex was reported as skipped:\n%s", errBuf.String())
    }
}

// TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew pins the blast
// radius of spawn-time skill validation in multi-agent runs: a worker whose
// configured skill no longer validates (e.g. codex's legacy auto-preselected
// "/review", orphaned when the curated builtin was removed) is excluded with
// a loud warning, and the remaining reviewers still run. Aborting the whole
// crew for one stale entry held every other agent hostage to a codex
// reconfigure.
// explicitly configured skill no longer validates is excluded with a loud
// warning, and the remaining reviewers still run. Aborting the whole crew for
// one stale entry would hold every other agent hostage to a reconfigure.
func TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew(t *testing.T) {
    setupCmdTestRepo(t)
    // Controlled empty HOME: codex discovery finds nothing, so its "/review"
    // (no longer a curated builtin) fails validation. Cannot t.Parallel —
    // Controlled empty HOME: Codex discovery finds nothing, so the configured
    // custom skill fails validation. Cannot t.Parallel —
            t.Setenv("HOME", t.TempDir())

if err := seedReviewConfig(context.Background(), map[string]settings.ReviewConfig{
        testCodexAgent: {Skills: []string{"/review"}},
        testCodexAgent: {Skills: []string{"$missing-review"}},
        "gemini":       {Skills: []string{"$also-missing"}},
    }); err != nil {
        t.Fatal(err)
    }
}

func TestDefaultReviewAgentConfig_CodexIsPromptOnly(t *testing.T) {
    t.Parallel()

cfg := defaultReviewAgentConfig(DefaultProfileName, tAgentCodex)
    if len(cfg.Skills) != 0 {
        t.Fatalf("Codex default skills = %v, want none", cfg.Skills)
    }
    if cfg.Prompt != defaultAgentReviewPrompt {
        t.Fatalf("Codex default prompt = %q, want %q", cfg.Prompt, defaultAgentReviewPrompt)
    }
}

func TestApplyLegacyReviewProfileFallback_RepairsGeneratedCodexSkill(t *testing.T) {
    t.Parallel()

s := &settings.EntireSettings{ReviewProfiles: map[string]settings.ReviewProfileConfig{
        DefaultProfileName: {Agents: map[string]settings.ReviewConfig{
            tAgentCodex: {Skills: []string{"/review"}},
            "codex-opus": {
                Agent:  tAgentCodex,
                Model:  "o3",
                Skills: []string{"/review"},
            },
            "codex-custom": {
                Agent:  tAgentCodex,
                Skills: []string{"$security-audit"},
            },
        },
        }}
        applyLegacyReviewProfileFallback(s)

got := s.ReviewProfiles[DefaultProfileName].Agents[tAgentCodex]
        if len(got.Skills) != 0 || got.Prompt != defaultAgentReviewPrompt {
            t.Fatalf("repaired Codex config = %+v, want prompt-only default", got)
        }
        alias := s.ReviewProfiles[DefaultProfileName].Agents["codex-opus"]
        if len(alias.Skills) != 0 || alias.Prompt != defaultAgentReviewPrompt || alias.Model != "o3" {
            t.Fatalf("repaired aliased Codex config = %+v", alias)
        }
        custom := s.ReviewProfiles[DefaultProfileName].Agents["codex-custom"]
        if len(custom.Skills) != 1 || custom.Skills[0] != "$security-audit" {
            t.Fatalf("custom Codex config changed: %+v", custom)
        }
}

func TestReviewTTYIsInteractive(t *testing.T) {
    t.Parallel()

tests := []struct {
        name         string
        stdinTTY     bool
        stdoutTTY    bool
        canPrompt    bool
        hardDisabled bool
        want         bool
    }{
        {name: "direct terminal overrides inherited sentinel", stdinTTY: true, stdoutTTY: true, canPrompt: false, want: true},
        {name: "controlling terminal fallback", stdinTTY: false, stdoutTTY: true, canPrompt: true, want: true},
        {name: "captured stdout", stdinTTY: true, stdoutTTY: false, canPrompt: true, want: false},
        {name: "agent with piped stdin", stdinTTY: false, stdoutTTY: true, canPrompt: false, want: false},
        {name: "explicitly forced non-interactive", stdinTTY: true, stdoutTTY: true, hardDisabled: true, want: false},
    }
    for _, tt := range tests {
        t.Run(tt.name, func(t *testing.T) {
            t.Parallel()
            if got := reviewTTYIsInteractive(tt.stdinTTY, tt.stdoutTTY, tt.canPrompt, tt.hardDisabled); got != tt.want {
                t.Fatalf("reviewTTYIsInteractive(%v, %v, %v, %v) = %v, want %v", tt.stdinTTY, tt.stdoutTTY, tt.canPrompt, tt.hardDisabled, got, tt.want)
            }
        })
    }
}

func TestBuildConfiguredProfile_FromFlags(t *testing.T) {
    t.Parallel()
    deps := configureTestDeps("claude-code", "codex")

// TestApplyLegacyReviewProfileFallback_RepairsGeneratedCodexSkill(t *testing.T) {
    // ... other lines ...
}