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

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

cd6155e→main·

dipree·3d ago·6 files·+284 added/-33 removed

Fix review interactive setup and Codex defaults

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
297
298
299
300
301
302
303
304
305
306
307
308
309
310
53 unmodified lines

364
365
366
332
367
368
369
370
419 unmodified lines

790
791
792
758
793
794
795
796
22 unmodified lines

819
820
821
787
822
823
824
825
151 unmodified lines

977
978
979
945
980
981
982
983
984
950
985
986
987
988
55 unmodified lines

1044
1045
1046
1012
1047
1048
1049
1050
1051
47 unmodified lines

1099
1100
1101
1066
1102
1103
1069
1104
1105
1106
1107
80 unmodified lines

1188
1189
1190
1156
1191
1192
1193
1194
1195
75 unmodified lines

1271
1272
1273
1238
1274
1275
1276
1277
67 unmodified lines

1345
1346
1347
1312
1313
1314
1315
1316
1348
1349
1350
1351
1352
1353
1354

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.")
            }
    }
}

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
    // 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)
            }
        }
    }
}

Mcmd/entire/cli/review/cmd.go+51/-16

1237 unmodified lines

1238 1239 1240 1241 1242 1243 1244 1245 1246 1247 1248 1249 1250 1251 1252 1253 1254 1255 1256 1257 1258 1259 1260 1261 1262 1263 1264 1265 1266 1267 1268 1269 1270 1271 1272 1273 1274 1275 1276 1277 1278 1279 1280 1281 1282 1283 1284 1285 1286 1287 1288 1290 1291 1292 1293 1294 1295 1296 1297 1298 1299 1300 1301 1302 1303 1304 1243 1244 1245 1246 1247 1305 1306 1307 1308 1309 1250 1251 1310 1311 1312 1313 1314 2 unmodified lines

1317 1318 1319 1260 1320 1321 1322 1323 37 unmodified lines

1361 1362 1363 1304 1364 1365 1366 1367 6 unmodified lines

1374 1375 1376 1317 1377 1378 1379 1380

1237 unmodified lines

}

// 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())

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) } }


Mcmd/entire/cli/review/cmd_test.go+70/-10

42 unmodified lines

43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168

42 unmodified lines

}

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 TestConfirmReReviewOrProceed_NonInteractiveDoesNotPrompt(t *testing.T) { t.Parallel()

out := &bytes.Buffer{} proceed, err := confirmReReviewOrProceed(context.Background(), out, Deps{ HeadHasReviewCheckpoint: func(context.Context) (bool, string) { return true, "existing review" }, }, false) if err != nil { t.Fatalf("confirmReReviewOrProceed: %v", err) } if !proceed { t.Fatal("non-interactive re-review should proceed") } if !strings.Contains(out.String(), "already reviewed") { t.Fatalf("missing non-interactive re-review note: %q", out.String()) } }

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

tests := []struct { name string testTTY string ci string underTest bool want bool }{ {name: "go test defaults off", underTest: true, want: true}, {name: "test override enables", testTTY: "1", ci: "true", underTest: true, want: false}, {name: "test override disables", testTTY: "0", want: true}, {name: "CI disables", ci: "true", want: true}, {name: "CI false does not disable", ci: "false", want: false}, {name: "normal process", want: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() if got := reviewInteractivityHardDisabled(tt.testTTY, tt.ci, tt.underTest); got != tt.want { t.Fatalf("reviewInteractivityHardDisabled(%q, %q, %v) = %v, want %v", tt.testTTY, tt.ci, tt.underTest, got, tt.want) } }) } }

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

tests := []struct { name string stdinTTY bool stdoutTTY bool canPrompt bool hardDisabled bool want bool }{ {name: "direct human terminal", stdinTTY: true, stdoutTTY: true, canPrompt: true, want: true}, {name: "agent sentinel overrides real PTY", stdinTTY: true, stdoutTTY: true, canPrompt: false, want: false}, {name: "controlling terminal does not override piped stdin", stdinTTY: false, stdoutTTY: true, canPrompt: true, want: false}, {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, canPrompt: 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")