Merge pull request #1749 from entireio/fix/review-interactive-codex-regressions

Fix review interactive setup and Codex defaults

Author: dipree

cmd/entire/cli/interactive/interactive.go

@@ -80,6 +80,17 @@ func isAgentSubprocessEnv() bool {
 		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.

cmd/entire/cli/review/cmd.go

@@ -210,7 +210,7 @@ To tag an already-finished session as a review, use
 				}, 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.")
@@ -270,6 +270,41 @@ type reviewConfigureOptions struct {
 	Slots  []string // reviewer slots as "agent[=model]" entries (--set-slot)
 }
 
+// reviewCommandIsInteractive requires the exact stdin consumed by huh and
+// Bubble Tea, plus stdout, to be terminals. CanPromptInteractively adds the
+// independent policy gate for tests, CI, and agent subprocess sentinels; a
+// controlling /dev/tty alone is insufficient because stdin may still be piped.
+func reviewCommandIsInteractive(cmd *cobra.Command) bool {
+	hardDisabled := reviewInteractivityHardDisabled(
+		os.Getenv(interactive.EnvTestTTY),
+		os.Getenv("CI"),
+		interactive.UnderTest(),
+	)
+	return reviewTTYIsInteractive(
+		interactive.IsTerminalReader(cmd.InOrStdin()),
+		interactive.IsTerminalWriter(cmd.OutOrStdout()),
+		interactive.CanPromptInteractively(),
+		hardDisabled,
+	)
+}
+
+func reviewInteractivityHardDisabled(testTTY, ci string, underTest bool) bool {
+	// Match CanPromptInteractively's precedence: ENTIRE_TEST_TTY=1 may opt an
+	// in-process test into interaction, while tests without that explicit
+	// override must never read from a developer's real terminal.
+	if testTTY != "" {
+		return testTTY != "1"
+	}
+	return underTest || (ci != "" && ci != "false")
+}
+
+func reviewTTYIsInteractive(stdinTTY, stdoutTTY, canPrompt, hardDisabled bool) bool {
+	// Real stdio terminals are necessary but not sufficient: agent shells can
+	// allocate a PTY while advertising that no human is available through the
+	// sentinels enforced by CanPromptInteractively.
+	return !hardDisabled && stdinTTY && stdoutTTY && 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
@@ -329,7 +364,7 @@ func runReviewConfigure(ctx context.Context, cmd *cobra.Command, profileOverride
 	// 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)
@@ -755,7 +790,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver
 	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
@@ -784,7 +819,7 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, modelOver
 		// 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)
@@ -942,12 +977,12 @@ func nonLaunchableEligibleNames(profile settings.ReviewProfileConfig, eligible [
 // (true, nil). In a non-interactive context it cannot prompt, so it proceeds
 // (the user explicitly invoked `entire review`) after printing a note rather
 // than blocking on a confirm form that would error out.
-func confirmReReviewOrProceed(ctx context.Context, out io.Writer, deps Deps) (bool, error) {
+func confirmReReviewOrProceed(ctx context.Context, out io.Writer, deps Deps, canPrompt bool) (bool, error) {
 	reviewed, meta := deps.HeadHasReviewCheckpoint(ctx)
 	if !reviewed {
 		return true, nil
 	}
-	if !interactive.CanPromptInteractively() {
+	if !canPrompt {
 		fmt.Fprintf(out, "Note: HEAD was already reviewed (%s); re-running.\n", meta)
 		return true, nil
 	}
@@ -1009,7 +1044,8 @@ func runSingleAgentPath(
 	}
 
 	// 4. Re-run guard: check if HEAD's checkpoint already has a review.
-	if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps); guardErr != nil {
+	canPrompt := reviewCommandIsInteractive(cmd)
+	if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps, canPrompt); guardErr != nil {
 		fmt.Fprintln(out, "prompt cancelled")
 		return silentErr(guardErr)
 	} else if !proceed {
@@ -1063,10 +1099,9 @@ func runSingleAgentPath(
 	defer cancelRun()
 
 	runCfg.EnrichSummary = reviewSummaryTokenEnricher(worktreeRoot, headSHA)
-	canPrompt := interactive.CanPromptInteractively()
 	sinks := composeSingleAgentSinks(singleAgentSinkInputs{
 		out:       out,
-		isTTY:     interactive.IsTerminalWriter(out) && canPrompt,
+		isTTY:     canPrompt,
 		canPrompt: canPrompt,
 		agentName: displayName,
 		cancelRun: cancelRun,
@@ -1153,7 +1188,8 @@ func runMultiAgentPath(
 		return fmt.Errorf("resolve HEAD: %w", shaErr)
 	}
 
-	if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps); guardErr != nil {
+	canPrompt := reviewCommandIsInteractive(cmd)
+	if proceed, guardErr := confirmReReviewOrProceed(ctx, out, deps, canPrompt); guardErr != nil {
 		fmt.Fprintln(out, "prompt cancelled")
 		return deps.NewSilentError(guardErr)
 	} else if !proceed {
@@ -1235,7 +1271,7 @@ func runMultiAgentPath(
 	masterLabel := judgeLabel(judge)
 	sinks := composeMultiAgentSinks(multiAgentSinkInputs{
 		out:               out,
-		isTTY:             interactive.IsTerminalWriter(out) && interactive.CanPromptInteractively(),
+		isTTY:             canPrompt,
 		agentNames:        agentNames,
 		cancelRun:         cancelRun,
 		runContext:        runCtx,
@@ -1309,11 +1345,10 @@ func handlePickerError(cmd *cobra.Command, silentErr func(error) error, pickErr
 // 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

cmd/entire/cli/review/cmd_test.go

@@ -1238,17 +1238,77 @@ func TestComposeMultiAgentSinks_TTYAutoSynthesisRunsBeforeTUIExit(t *testing.T)
 	}
 }
 
+// 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 (setupCmdTestRepo already precludes it via t.Chdir).
 	t.Setenv("HOME", t.TempDir())
 
@@ -1257,7 +1317,7 @@ func TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew(t *testing.T) {
 			Skills: []string{"/review"},
 		},
 		testCodexAgent: {
-			Skills: []string{"/review"}, // stale legacy entry
+			Skills: []string{"$missing-review"},
 		},
 	}); err != nil {
 		t.Fatal(err)
@@ -1301,7 +1361,7 @@ func TestDispatchFork_InvalidSkillExcludesWorkerNotWholeCrew(t *testing.T) {
 		t.Error("codex reviewer started despite failing skill validation")
 	}
 	stderr := errBuf.String()
-	if !strings.Contains(stderr, "/review") || !strings.Contains(stderr, "skipping") {
+	if !strings.Contains(stderr, "$missing-review") || !strings.Contains(stderr, "skipping") {
 		t.Errorf("stderr should warn about the excluded worker and its skill; got:\n%s", stderr)
 	}
 }
@@ -1314,7 +1374,7 @@ func TestDispatchFork_AllWorkersInvalidStillFails(t *testing.T) {
 	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)