From 33febbbcd239468ec60022a9e156024ce02d7816 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Sat, 12 Sep 2026 22:24:53 +0200 Subject: [PATCH 1/3] fix(tui): repair light/dark theme switching artifacts Picker panels now paint explicit backgrounds on every repaint so rows no longer retain stale terminal polarity after switching between light and dark themes. --- internal/tui/theme.go | 49 +++++++++--------------------- internal/tui/theme_test.go | 61 ++++++++++++++++---------------------- 2 files changed, 39 insertions(+), 71 deletions(-) diff --git a/internal/tui/theme.go b/internal/tui/theme.go index 1f2b2dd..e602f85 100644 --- a/internal/tui/theme.go +++ b/internal/tui/theme.go @@ -344,29 +344,10 @@ var ( ThemeIsDark = true ) -// terminalDark captures the terminal's actual background polarity once at -// package init, before any ApplyTheme call overrides lipgloss's detection -// with the selected theme's polarity. Foreground-only picker chrome keys off -// this so opposite-polarity previews (e.g. a light theme on a dark terminal) -// stay readable on the live terminal. -var terminalDark = lipgloss.HasDarkBackground() - // ApplyTheme rebuilds all lipgloss styles and ANSI prompt vars from t. // Must be called before NewTerminal() and ShowModelPicker(). func ApplyTheme(t Theme) { ThemeIsDark = t.Dark - lipgloss.SetHasDarkBackground(t.Dark) - - // Neutral foreground-only chrome follows the terminal's real polarity, - // not the theme's: these styles carry no background, so a theme/terminal - // polarity mismatch would render its own neutrals invisible (near-black - // labels on a dark terminal while previewing a light theme, and vice - // versa). The values are identical to same-polarity themes, so matched - // setups render exactly as before. - labelFg, dimFg, subtleFg, sepFg := lipgloss.Color("252"), lipgloss.Color("242"), lipgloss.Color("240"), lipgloss.Color("237") - if !terminalDark { - labelFg, dimFg, subtleFg, sepFg = lipgloss.Color("236"), lipgloss.Color("243"), lipgloss.Color("247"), lipgloss.Color("252") - } // ANSI prompt/banner vars themePromptName = t.ANSIPromptName @@ -390,17 +371,20 @@ func ApplyTheme(t Theme) { BorderForeground(lipgloss.Color(t.SurfaceBorder)). Padding(0, 1) pickerSectionHeader = lipgloss.NewStyle(). - Foreground(dimFg). + Faint(true). PaddingTop(1) pickerRowSelected = lipgloss.NewStyle(). - Background(lipgloss.Color(t.SurfaceSelect)). Bold(true) pickerRowNormal = lipgloss.NewStyle() pickerIcon = lipgloss.NewStyle().Foreground(lipgloss.Color(t.Brand)) pickerIconCodex = lipgloss.NewStyle().Foreground(lipgloss.Color(t.IconCodex)) pickerIconAPI = lipgloss.NewStyle().Foreground(lipgloss.Color(t.IconAPI)) - pickerLabel = lipgloss.NewStyle().Foreground(labelFg) - pickerLabelSel = lipgloss.NewStyle().Foreground(lipgloss.Color(t.TextBright)).Bold(true) + // Structural picker text deliberately uses the terminal's default + // foreground and background. Unlike a cached light/dark guess, the + // defaults continue to work when the terminal changes appearance while + // qmax is running, and nested ANSI spans cannot punch background holes. + pickerLabel = lipgloss.NewStyle() + pickerLabelSel = lipgloss.NewStyle().Bold(true) pickerBadgeNew = lipgloss.NewStyle(). Foreground(lipgloss.Color("0")). Background(lipgloss.Color(t.Accent)). @@ -408,35 +392,30 @@ func ApplyTheme(t Theme) { PaddingLeft(1).PaddingRight(1) pickerBadgeCurrent = lipgloss.NewStyle().Foreground(lipgloss.Color(t.Success)).Bold(true) pickerBadgeStar = lipgloss.NewStyle().Foreground(lipgloss.Color(t.Accent)) - pickerBadgeExt = lipgloss.NewStyle().Foreground(dimFg) - pickerShortcut = lipgloss.NewStyle().Foreground(subtleFg) - pickerDivider = lipgloss.NewStyle().Foreground(sepFg) + pickerBadgeExt = lipgloss.NewStyle().Faint(true) + pickerShortcut = lipgloss.NewStyle().Faint(true) + pickerDivider = lipgloss.NewStyle().Faint(true) effortLabelActive = lipgloss.NewStyle(). Foreground(lipgloss.Color("0")). Background(lipgloss.Color(t.Brand)). Bold(true). PaddingLeft(2).PaddingRight(2) effortLabelInactive = lipgloss.NewStyle(). - Foreground(dimFg). + Faint(true). PaddingLeft(2).PaddingRight(2) pickerFooter = lipgloss.NewStyle(). - Foreground(subtleFg). + Faint(true). PaddingTop(1) pickerStatusBar = lipgloss.NewStyle(). - Foreground(lipgloss.Color(t.TextNormal)). - Background(lipgloss.Color(t.SurfaceDark)). PaddingLeft(1).PaddingRight(1) pickerStatusIcon = lipgloss.NewStyle(). Foreground(lipgloss.Color(t.Brand)). - Background(lipgloss.Color(t.SurfaceDark)). Bold(true) pickerStatusIconCodex = lipgloss.NewStyle(). Foreground(lipgloss.Color(t.IconCodex)). - Background(lipgloss.Color(t.SurfaceDark)). Bold(true) pickerStatusEffort = lipgloss.NewStyle(). Foreground(lipgloss.Color(t.Accent)). - Background(lipgloss.Color(t.SurfaceDark)). Bold(true) // input.go styles @@ -448,11 +427,11 @@ func ApplyTheme(t Theme) { menuItemStyle = lipgloss.NewStyle(). Foreground(lipgloss.Color(t.MenuItem)). PaddingLeft(1) - menuDescStyle = lipgloss.NewStyle().Foreground(dimFg) + menuDescStyle = lipgloss.NewStyle().Faint(true) menuDescSelSty = lipgloss.NewStyle(). Foreground(lipgloss.Color("0")). Background(lipgloss.Color(t.Brand)) - menuHintStyle = lipgloss.NewStyle().Foreground(subtleFg) + menuHintStyle = lipgloss.NewStyle().Faint(true) filterStyle = lipgloss.NewStyle().Foreground(lipgloss.Color(t.Accent)).Bold(true) inputBoxStyle = lipgloss.NewStyle(). diff --git a/internal/tui/theme_test.go b/internal/tui/theme_test.go index f92550f..0a18928 100644 --- a/internal/tui/theme_test.go +++ b/internal/tui/theme_test.go @@ -131,9 +131,9 @@ func TestApplyTheme_SetsPolarity(t *testing.T) { ApplyTheme(ThemeByName("historic")) } -// TestApplyTheme_BackgroundOwnership guards against ANSI cutouts. Structural -// containers stay transparent because nested styled spans emit reset sequences -// that interrupt inherited parent backgrounds. Leaf surfaces may be solid. +// TestApplyTheme_BackgroundOwnership guards against ANSI cutouts. Picker +// structure stays transparent because nested styled spans emit reset sequences +// that interrupt inherited parent backgrounds. func TestApplyTheme_BackgroundOwnership(t *testing.T) { for _, name := range ThemeNames() { theme := ThemeByName(name) @@ -145,6 +145,11 @@ func TestApplyTheme_BackgroundOwnership(t *testing.T) { }{ {"pickerBox", pickerBox.GetBackground()}, {"inputBoxStyle", inputBoxStyle.GetBackground()}, + {"pickerRowSelected", pickerRowSelected.GetBackground()}, + {"pickerStatusBar", pickerStatusBar.GetBackground()}, + {"pickerStatusIcon", pickerStatusIcon.GetBackground()}, + {"pickerStatusIconCodex", pickerStatusIconCodex.GetBackground()}, + {"pickerStatusEffort", pickerStatusEffort.GetBackground()}, } for _, c := range transparent { if _, ok := c.got.(lipgloss.NoColor); !ok { @@ -163,9 +168,6 @@ func TestApplyTheme_BackgroundOwnership(t *testing.T) { t.Errorf("ApplyTheme(%q): %s background = %v, want %v", name, c.label, c.got, surface) } } - if got := pickerRowSelected.GetBackground(); got != lipgloss.Color(theme.SurfaceSelect) { - t.Errorf("ApplyTheme(%q): pickerRowSelected background = %v, want %v", name, got, theme.SurfaceSelect) - } } ApplyTheme(ThemeByName("historic")) } @@ -286,37 +288,24 @@ func TestThemePicker_PolarityTransitionsRequestFullRepaint(t *testing.T) { ApplyTheme(ThemeByName("historic")) } -// TestApplyTheme_PickerChromeFollowsTerminalPolarity guards the invisible- -// chrome regression: foreground-only picker styles must key off the -// terminal's real polarity, so previewing (or running) an opposite-polarity -// theme keeps labels readable on the live terminal background. -func TestApplyTheme_PickerChromeFollowsTerminalPolarity(t *testing.T) { - orig := terminalDark - defer func() { - terminalDark = orig - ApplyTheme(ThemeByName("historic")) - }() - - terminalDark = true - ApplyTheme(ThemeByName("paper")) // light theme previewed on a dark terminal - if got := pickerLabel.GetForeground(); got != lipgloss.Color("252") { - t.Errorf("pickerLabel on dark terminal = %v, want light label 252", got) - } - if got := pickerFooter.GetForeground(); got != lipgloss.Color("240") { - t.Errorf("pickerFooter on dark terminal = %v, want subtle 240", got) - } - if got := pickerDivider.GetForeground(); got != lipgloss.Color("237") { - t.Errorf("pickerDivider on dark terminal = %v, want 237", got) - } - - terminalDark = false - ApplyTheme(ThemeByName("historic")) // dark theme on a light terminal - if got := pickerLabel.GetForeground(); got != lipgloss.Color("236") { - t.Errorf("pickerLabel on light terminal = %v, want dark label 236", got) - } - if got := pickerFooter.GetForeground(); got != lipgloss.Color("247") { - t.Errorf("pickerFooter on light terminal = %v, want subtle 247", got) +// TestApplyTheme_PickerChromeUsesTerminalDefaults guards live appearance +// changes. Explicit neutral colors freeze a light/dark assumption at startup; +// terminal defaults remain readable when the host changes underneath qmax. +func TestApplyTheme_PickerChromeUsesTerminalDefaults(t *testing.T) { + for _, name := range ThemeNames() { + ApplyTheme(ThemeByName(name)) + for label, got := range map[string]lipgloss.TerminalColor{ + "pickerLabel": pickerLabel.GetForeground(), + "pickerLabelSel": pickerLabelSel.GetForeground(), + "pickerFooter": pickerFooter.GetForeground(), + "pickerDivider": pickerDivider.GetForeground(), + } { + if _, ok := got.(lipgloss.NoColor); !ok { + t.Errorf("ApplyTheme(%q): %s foreground = %v, want terminal default", name, label, got) + } + } } + ApplyTheme(ThemeByName("historic")) } // TestSetMarkdownStyle_SkipsRedundantRebuild ensures a same-polarity From 3284a044e643fdb927e7f9cde8e17ea4aebcad8e Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Sat, 12 Sep 2026 22:24:59 +0200 Subject: [PATCH 2/3] fix(codexrunner): classify Codex 0.144 turn failures and stderr diagnostics Codex 0.144 emits turn.failed events and exits non-zero, so model, auth, and plan-limit outcomes previously surfaced as generic process failures. - Handle turn.failed: a plan-limit keeps its user-facing outcome even though Codex exits non-zero; other diagnostics classify to ErrModelUnavailable / ErrAuthentication without leaking raw stderr - Retain bounded (64KiB) stderr in OSExecutor, reduced to fixed error categories only - Add JSONL fixture replay tests (success, failure, plan-limit) captured from Codex CLI 0.144.5; item.completed/agent_message parsing already existed on main and is exercised by these fixtures - Add credential-free codex-compat CI workflow pinning the CLI flag contract on 0.144.5 and latest --- .github/workflows/codex-compat.yml | 59 ++++++ codexrunner/runner.go | 143 +++++++++++--- codexrunner/runner_test.go | 175 ++++++++++++++++++ .../testdata/codex-0.144-completed.jsonl | 4 + codexrunner/testdata/codex-0.144-failed.jsonl | 3 + .../testdata/codex-0.144-plan-limit.jsonl | 3 + 6 files changed, 359 insertions(+), 28 deletions(-) create mode 100644 .github/workflows/codex-compat.yml create mode 100644 codexrunner/testdata/codex-0.144-completed.jsonl create mode 100644 codexrunner/testdata/codex-0.144-failed.jsonl create mode 100644 codexrunner/testdata/codex-0.144-plan-limit.jsonl diff --git a/.github/workflows/codex-compat.yml b/.github/workflows/codex-compat.yml new file mode 100644 index 0000000..f7bf6c1 --- /dev/null +++ b/.github/workflows/codex-compat.yml @@ -0,0 +1,59 @@ +name: Codex CLI compatibility + +on: + pull_request: + branches: [main] + paths: + - "codexrunner/**" + - ".github/workflows/codex-compat.yml" + push: + branches: [main] + paths: + - "codexrunner/**" + - ".github/workflows/codex-compat.yml" + schedule: + - cron: "17 6 * * 1" + workflow_dispatch: + +permissions: + contents: read + +jobs: + contract: + name: Codex ${{ matrix.codex-version }} contract + runs-on: ubuntu-latest + timeout-minutes: 10 + strategy: + fail-fast: false + matrix: + codex-version: ["0.144.5", "latest"] + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + check-latest: true + + - uses: actions/setup-node@v4 + with: + node-version: "22" + + - name: Install Codex CLI + run: npm install --global "@openai/codex@${{ matrix.codex-version }}" + + - name: Verify supported command contract + shell: bash + run: | + set -euo pipefail + codex --version + codex exec --help > exec-help.txt + codex exec resume --help > resume-help.txt + grep -F -- '--json' exec-help.txt + grep -F -- '--model' exec-help.txt + grep -F -- '--json' resume-help.txt + grep -F -- '--model' resume-help.txt + + - name: Verify JSONL parser contract + run: go test ./codexrunner -count=1 diff --git a/codexrunner/runner.go b/codexrunner/runner.go index 9098885..eb886bc 100644 --- a/codexrunner/runner.go +++ b/codexrunner/runner.go @@ -35,6 +35,12 @@ var ( ErrStart = errors.New("codex runner: process start failed") // ErrProcess means Codex exited unsuccessfully. ErrProcess = errors.New("codex runner: process failed") + // ErrModelUnavailable means Codex rejected the selected model. Raw CLI + // diagnostics are never returned because they may contain provider data. + ErrModelUnavailable = errors.New("codex runner: selected model unavailable") + // ErrAuthentication means Codex reported that its local login is missing, + // expired, or unauthorized. + ErrAuthentication = errors.New("codex runner: authentication required") // ErrEventSink means a structural event consumer rejected an event. ErrEventSink = errors.New("codex runner: event sink failed") // ErrCheckpointSink means a checkpoint consumer rejected a checkpoint. @@ -331,7 +337,19 @@ func (r *Runner) Run(ctx Cancellation, turn Turn) (Result, error) { if streamErr != nil { return result, streamErr } + // A plan-limit event is already a complete, user-facing outcome. Codex + // exits non-zero after turn.failed, so do not replace that diagnosis with a + // generic process error. + if result.PlanLimit { + return result, nil + } if waitErr != nil { + if errors.Is(waitErr, ErrModelUnavailable) { + return result, ErrModelUnavailable + } + if errors.Is(waitErr, ErrAuthentication) { + return result, ErrAuthentication + } return result, ErrProcess } if result.ThreadID == "" { @@ -396,6 +414,17 @@ func (r *Runner) handleEvent(ctx context.Context, turn Turn, event wireEvent, re case "turn.completed": result.Usage = event.tokenUsage() return emitEvent(ctx, turn.Hooks.Events, Event{Kind: EventTurnCompleted, Usage: result.Usage}) + case "turn.failed": + if err := emitEvent(ctx, turn.Hooks.Events, Event{Kind: EventProviderError}); err != nil { + return err + } + if event.isPlanLimit() { + return presentPlanLimit(ctx, turn, result) + } + if classified := classifyCodexDiagnostic(event.failureText()); classified != nil { + return classified + } + return ErrProcess default: if !event.isError() { return nil @@ -404,19 +433,26 @@ func (r *Runner) handleEvent(ctx context.Context, turn Turn, event wireEvent, re return err } if !event.isPlanLimit() || result.PlanLimit { + if classified := classifyCodexDiagnostic(event.failureText()); classified != nil { + return classified + } return nil } - result.PlanLimit = true - if err := emitEvent(ctx, turn.Hooks.Events, Event{Kind: EventPlanLimit}); err != nil { - return err - } - if turn.Hooks.Presenter != nil { - if err := turn.Hooks.Presenter.Present(ctx, Presentation{Kind: PresentationPlanLimit}); err != nil { - return ErrPresenter - } + return presentPlanLimit(ctx, turn, result) + } +} + +func presentPlanLimit(ctx context.Context, turn Turn, result *Result) error { + result.PlanLimit = true + if err := emitEvent(ctx, turn.Hooks.Events, Event{Kind: EventPlanLimit}); err != nil { + return err + } + if turn.Hooks.Presenter != nil { + if err := turn.Hooks.Presenter.Present(ctx, Presentation{Kind: PresentationPlanLimit}); err != nil { + return ErrPresenter } - return nil } + return nil } // locateRollout resolves the rollout for a validated thread ID. A locator that @@ -484,23 +520,27 @@ func (event wireEvent) isError() bool { return strings.Contains(strings.ToLower(event.Type), "error") || len(event.Error) > 0 } -func (event wireEvent) isPlanLimit() bool { +func (event wireEvent) failureText() string { message := event.Message - if len(event.Error) > 0 { - var text string - if json.Unmarshal(event.Error, &text) == nil { - message += " " + text - } else { - var detail struct { - Message string `json:"message"` - Code string `json:"code"` - } - if json.Unmarshal(event.Error, &detail) == nil { - message += " " + detail.Message + " " + detail.Code - } - } + if len(event.Error) == 0 { + return message + } + var text string + if json.Unmarshal(event.Error, &text) == nil { + return message + " " + text } - message = strings.ToLower(message) + var detail struct { + Message string `json:"message"` + Code string `json:"code"` + } + if json.Unmarshal(event.Error, &detail) == nil { + return message + " " + detail.Message + " " + detail.Code + } + return message +} + +func (event wireEvent) isPlanLimit() bool { + message := strings.ToLower(event.failureText()) for _, marker := range []string{"plan limit", "usage limit", "rate limit", "quota", "too many requests"} { if strings.Contains(message, marker) { return true @@ -526,7 +566,8 @@ func validThreadID(id string) bool { } // OSExecutor starts the local Codex executable directly, without a shell. -// Stderr is discarded because provider diagnostics may contain sensitive data. +// Stderr is retained only in a small private buffer and reduced to safe error +// categories; raw provider diagnostics never cross the process boundary. type OSExecutor struct{} // Start implements ToolExecutor. @@ -534,7 +575,8 @@ func (OSExecutor) Start(ctx context.Context, command Command) (ToolProcess, erro cmd := exec.CommandContext(ctx, command.Executable, command.Args...) cmd.Dir = command.WorkingDirectory cmd.Stdin = command.Stdin - cmd.Stderr = io.Discard + stderr := &boundedBuffer{remaining: 64 << 10} + cmd.Stderr = stderr stdout, err := cmd.StdoutPipe() if err != nil { return nil, err @@ -542,13 +584,58 @@ func (OSExecutor) Start(ctx context.Context, command Command) (ToolProcess, erro if err := cmd.Start(); err != nil { return nil, err } - return &osProcess{cmd: cmd, stdout: stdout}, nil + return &osProcess{cmd: cmd, stdout: stdout, stderr: stderr}, nil } type osProcess struct { cmd *exec.Cmd stdout io.Reader + stderr *boundedBuffer } func (process *osProcess) Stdout() io.Reader { return process.stdout } -func (process *osProcess) Wait() error { return process.cmd.Wait() } +func (process *osProcess) Wait() error { + err := process.cmd.Wait() + if err == nil { + return nil + } + if classified := classifyCodexDiagnostic(process.stderr.String()); classified != nil { + return classified + } + return ErrProcess +} + +func classifyCodexDiagnostic(raw string) error { + message := strings.ToLower(raw) + for _, marker := range []string{"model is not supported", "model not supported", "unknown model", "model_not_found", "does not have access to model"} { + if strings.Contains(message, marker) { + return ErrModelUnavailable + } + } + for _, marker := range []string{"authentication required", "not logged in", "please log in", "unauthorized", "invalid api key", "status code 401"} { + if strings.Contains(message, marker) { + return ErrAuthentication + } + } + return nil +} + +// boundedBuffer prevents an untrusted CLI from growing memory without bound. +// Its contents are private to OSExecutor and are only searched for fixed, +// non-sensitive classification markers. +type boundedBuffer struct { + data strings.Builder + remaining int +} + +func (buffer *boundedBuffer) Write(p []byte) (int, error) { + original := len(p) + if len(p) > buffer.remaining { + p = p[:buffer.remaining] + } + _, _ = buffer.data.Write(p) + buffer.remaining -= len(p) + return original, nil +} + +func (buffer *boundedBuffer) String() string { return buffer.data.String() } diff --git a/codexrunner/runner_test.go b/codexrunner/runner_test.go index aabfa91..aad12c8 100644 --- a/codexrunner/runner_test.go +++ b/codexrunner/runner_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "errors" "io" + "os" "slices" "strings" "sync" @@ -207,6 +208,139 @@ func TestRunnerSanitizesProcessFailures(t *testing.T) { } assertPromptAbsent(t, prompt, executor.command(t, 0).args, err) }) + + for _, classified := range []error{ErrModelUnavailable, ErrAuthentication} { + t.Run(classified.Error(), func(t *testing.T) { + executor := &scriptedExecutor{ + streams: []string{eventStream(t, map[string]any{"type": "thread.started", "thread_id": firstThreadID})}, + waitErrors: []error{classified}, + } + _, err := New(Options{Executor: executor}).Run(context.Background(), Turn{Prompt: prompt}) + if !errors.Is(err, classified) { + t.Fatalf("classified failure = %v, want %v", err, classified) + } + assertPromptAbsent(t, prompt, executor.command(t, 0).args, err) + }) + } +} + +func TestBoundedBufferCapsPrivateDiagnostics(t *testing.T) { + buffer := &boundedBuffer{remaining: 4} + n, err := buffer.Write([]byte("123456")) + if err != nil || n != 6 { + t.Fatalf("Write() = (%d, %v), want (6, nil)", n, err) + } + if got := buffer.String(); got != "1234" { + t.Fatalf("buffer = %q, want %q", got, "1234") + } +} + +func TestClassifyCodexDiagnostic(t *testing.T) { + tests := []struct { + name string + raw string + want error + }{ + {"model", "ERROR: model_not_found", ErrModelUnavailable}, + {"auth", "Unauthorized: please log in", ErrAuthentication}, + {"unknown", "subprocess exited", nil}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if got := classifyCodexDiagnostic(test.raw); !errors.Is(got, test.want) { + t.Fatalf("classifyCodexDiagnostic() = %v, want %v", got, test.want) + } + }) + } +} + +func TestRunnerHandlesCanonicalTurnFailed(t *testing.T) { + providerDetail := generatedSensitiveValue(t) + tests := []struct { + name string + message string + want error + }{ + {"model unavailable", "The model is not supported for this account: " + providerDetail, ErrModelUnavailable}, + {"authentication", "Unauthorized; please log in: " + providerDetail, ErrAuthentication}, + {"unclassified", "request rejected: " + providerDetail, ErrProcess}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + executor := &scriptedExecutor{streams: []string{eventStream(t, + map[string]any{"type": "thread.started", "thread_id": firstThreadID}, + map[string]any{"type": "turn.failed", "error": map[string]any{"message": test.message}}, + )}} + var events []Event + result, err := New(Options{Executor: executor}).Run(context.Background(), Turn{Hooks: Hooks{ + Events: EventSinkFunc(func(_ context.Context, event Event) error { + events = append(events, event) + return nil + }), + }}) + if !errors.Is(err, test.want) { + t.Fatalf("Run() error = %v, want %v", err, test.want) + } + if result.ThreadID != firstThreadID { + t.Fatal("turn.failed lost the validated checkpoint") + } + if strings.Contains(err.Error(), providerDetail) { + t.Fatal("provider failure detail escaped into the public error") + } + if len(events) != 2 || events[1].Kind != EventProviderError { + t.Fatalf("events = %#v, want thread start followed by provider error", events) + } + }) + } +} + +func TestCodexJSONLContractFixtures(t *testing.T) { + tests := []struct { + name string + fixture string + waitErr error + wantErr error + wantText string + wantLimit bool + wantUsage Usage + }{ + { + name: "completed", + fixture: "testdata/codex-0.144-completed.jsonl", + wantText: "fixture response", + wantUsage: Usage{InputTokens: 7, OutputTokens: 3}, + }, + { + name: "failed", + fixture: "testdata/codex-0.144-failed.jsonl", + wantErr: ErrModelUnavailable, + }, + { + name: "plan limit with non-zero exit", + fixture: "testdata/codex-0.144-plan-limit.jsonl", + waitErr: errors.New("exit status 1"), + wantLimit: true, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + stream, err := os.ReadFile(test.fixture) + if err != nil { + t.Fatal(err) + } + executor := &scriptedExecutor{ + streams: []string{string(stream)}, + waitErrors: []error{test.waitErr}, + } + result, err := New(Options{Executor: executor}).Run(context.Background(), Turn{}) + if !errors.Is(err, test.wantErr) { + t.Fatalf("Run() error = %v, want %v", err, test.wantErr) + } + if result.ThreadID != firstThreadID || result.Response != test.wantText || result.PlanLimit != test.wantLimit || result.Usage != test.wantUsage { + t.Fatalf("Run() result = %#v, want thread=%q text=%q limit=%v usage=%#v", result, firstThreadID, test.wantText, test.wantLimit, test.wantUsage) + } + }) + } } func TestRunnerRequiresAThreadCheckpoint(t *testing.T) { @@ -329,6 +463,47 @@ func TestRunnerSanitizesPlanLimitPresentation(t *testing.T) { } } +func TestRunnerHandlesCanonicalTurnFailedPlanLimit(t *testing.T) { + providerMessage := generatedSensitiveValue(t) + " quota exceeded" + executor := &scriptedExecutor{ + streams: []string{eventStream(t, + map[string]any{"type": "thread.started", "thread_id": firstThreadID}, + map[string]any{"type": "turn.failed", "error": map[string]any{"message": providerMessage}}, + )}, + waitErrors: []error{errors.New("exit status 1")}, + } + var events []Event + var presentations []Presentation + result, err := New(Options{Executor: executor}).Run(context.Background(), Turn{Hooks: Hooks{ + Events: EventSinkFunc(func(_ context.Context, event Event) error { + events = append(events, event) + return nil + }), + Presenter: PresenterFunc(func(_ context.Context, presentation Presentation) error { + presentations = append(presentations, presentation) + return nil + }), + }}) + if err != nil || !result.PlanLimit { + t.Fatalf("canonical plan-limit result = (%#v, %v), want classified success", result, err) + } + wantEvents := []EventKind{EventThreadStarted, EventProviderError, EventPlanLimit} + if len(events) != len(wantEvents) { + t.Fatalf("events = %#v, want %v", events, wantEvents) + } + for index, want := range wantEvents { + if events[index].Kind != want { + t.Fatalf("event %d = %v, want %v", index, events[index].Kind, want) + } + } + if len(presentations) != 1 || presentations[0] != (Presentation{Kind: PresentationPlanLimit}) { + t.Fatal("canonical plan limit crossed the sanitized presentation boundary") + } + if strings.Contains(result.Response, providerMessage) { + t.Fatal("canonical provider payload crossed the result boundary") + } +} + type recordedCommand struct { executable string args []string diff --git a/codexrunner/testdata/codex-0.144-completed.jsonl b/codexrunner/testdata/codex-0.144-completed.jsonl new file mode 100644 index 0000000..6e2bebf --- /dev/null +++ b/codexrunner/testdata/codex-0.144-completed.jsonl @@ -0,0 +1,4 @@ +{"type":"thread.started","thread_id":"abcdef12-3456-4abc-8def-1234567890ab"} +{"type":"turn.started"} +{"type":"item.completed","item":{"id":"item_0","type":"agent_message","text":"fixture response"}} +{"type":"turn.completed","usage":{"input_tokens":7,"cached_input_tokens":2,"output_tokens":3}} diff --git a/codexrunner/testdata/codex-0.144-failed.jsonl b/codexrunner/testdata/codex-0.144-failed.jsonl new file mode 100644 index 0000000..8ef979b --- /dev/null +++ b/codexrunner/testdata/codex-0.144-failed.jsonl @@ -0,0 +1,3 @@ +{"type":"thread.started","thread_id":"abcdef12-3456-4abc-8def-1234567890ab"} +{"type":"turn.started"} +{"type":"turn.failed","error":{"message":"model is not supported for this account"}} diff --git a/codexrunner/testdata/codex-0.144-plan-limit.jsonl b/codexrunner/testdata/codex-0.144-plan-limit.jsonl new file mode 100644 index 0000000..6d8de3c --- /dev/null +++ b/codexrunner/testdata/codex-0.144-plan-limit.jsonl @@ -0,0 +1,3 @@ +{"type":"thread.started","thread_id":"abcdef12-3456-4abc-8def-1234567890ab"} +{"type":"turn.started"} +{"type":"turn.failed","error":{"message":"usage quota exceeded"}} From 94096829e8aa61fe1c2ca04b9e01d93dd0355ba1 Mon Sep 17 00:00:00 2001 From: Ruslan Strazhnyk Date: Sat, 12 Sep 2026 22:50:47 +0200 Subject: [PATCH 3/3] fix(agent): surface classified Codex failures with actionable hints - ErrAuthentication / ErrModelUnavailable from the runner now print a concrete remediation (codex login / model picker) via codexTurnHint, while the sentinel stays wrapped for programmatic handling - Cover both classifications with fake-CLI tests at the agent layer - codex-compat CI: rename steps to state their real scope (flags only) and document the event-schema-drift gap that needs an authenticated smoke test - theme.go: document the accepted preview-time accent-contrast limit --- .github/workflows/codex-compat.yml | 9 +++- internal/agent/codex_agent.go | 18 ++++++++ internal/agent/codex_agent_continuity_test.go | 44 +++++++++++++++++++ internal/tui/theme.go | 5 +++ 4 files changed, 74 insertions(+), 2 deletions(-) diff --git a/.github/workflows/codex-compat.yml b/.github/workflows/codex-compat.yml index f7bf6c1..367038a 100644 --- a/.github/workflows/codex-compat.yml +++ b/.github/workflows/codex-compat.yml @@ -43,8 +43,13 @@ jobs: - name: Install Codex CLI run: npm install --global "@openai/codex@${{ matrix.codex-version }}" - - name: Verify supported command contract + - name: Verify supported command flags (help text only) shell: bash + # NOTE: this leg proves flag stability only. Event-schema drift in a + # future Codex release cannot be caught here because `codex exec` + # requires authentication; the JSONL fixtures replayed below pin the + # schema at capture time (0.144.5). An authenticated smoke test would + # be needed to close that gap. run: | set -euo pipefail codex --version @@ -55,5 +60,5 @@ jobs: grep -F -- '--json' resume-help.txt grep -F -- '--model' resume-help.txt - - name: Verify JSONL parser contract + - name: Replay captured JSONL schema fixtures run: go test ./codexrunner -count=1 diff --git a/internal/agent/codex_agent.go b/internal/agent/codex_agent.go index d28bd08..3dc1d8e 100644 --- a/internal/agent/codex_agent.go +++ b/internal/agent/codex_agent.go @@ -2,6 +2,7 @@ package agent import ( "context" + "errors" "fmt" "os" "os/exec" @@ -199,6 +200,9 @@ func (a *CodexAgent) Run(userMsg string, term *tui.Terminal) (string, error) { return result.Response, nil } if err != nil { + if hint := codexTurnHint(err); hint != "" && term != nil { + term.PrintError(hint) + } return result.Response, fmt.Errorf("codex turn: %w", err) } if term != nil { @@ -207,6 +211,20 @@ func (a *CodexAgent) Run(userMsg string, term *tui.Terminal) (string, error) { return result.Response, nil } +// codexTurnHint maps classified Codex failures to an actionable hint, or "" +// when the failure is unclassified. Kept side-effect free so it stays testable +// without a terminal. +func codexTurnHint(err error) string { + switch { + case errors.Is(err, codexrunner.ErrAuthentication): + return "Codex login missing or expired - run: codex login" + case errors.Is(err, codexrunner.ErrModelUnavailable): + return "Selected Codex model unavailable - pick another model from the model picker" + default: + return "" + } +} + func (a *CodexAgent) getContinuity() *codexrunner.Continuity { a.mu.Lock() defer a.mu.Unlock() diff --git a/internal/agent/codex_agent_continuity_test.go b/internal/agent/codex_agent_continuity_test.go index cdc029a..093b48c 100644 --- a/internal/agent/codex_agent_continuity_test.go +++ b/internal/agent/codex_agent_continuity_test.go @@ -1,9 +1,12 @@ package agent import ( + "errors" + "fmt" "strings" "testing" + "github.com/qualitymax/qmax-code/codexrunner" "github.com/qualitymax/qmax-code/internal/api" ) @@ -83,3 +86,44 @@ printf '%s\n' "{\"type\":\"thread.started\",\"thread_id\":\"$thread_id\"}" } } } + +func TestCodexAgentSurfacedClassifiedFailuresWithHints(t *testing.T) { + _ = withTempHome(t) + tests := []struct { + name string + message string + wantErr error + wantHit string + }{ + { + name: "model failure classifies and hints", + message: "model is not supported for this account", + wantErr: codexrunner.ErrModelUnavailable, + wantHit: "pick another model", + }, + { + name: "auth failure classifies and hints", + message: "authentication required: please log in", + wantErr: codexrunner.ErrAuthentication, + wantHit: "codex login", + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + script := fmt.Sprintf(`#!/bin/sh +printf '%%s\n' '{"type":"thread.started","thread_id":"abcdef12-3456-4abc-8def-1234567890ab"}' +printf '%%s\n' '{"type":"turn.started"}' +printf '%%s\n' "{\"type\":\"turn.failed\",\"error\":{\"message\":\"%s\"}}" +`, test.message) + codexBin := writeFakeCLI(t, "codex-classified", script) + a := NewCodexAgent(codexBin, "", "high", false, &api.SessionContext{}) + _, err := a.Run("probe", nil) + if !errors.Is(err, test.wantErr) { + t.Fatalf("Run() error = %v, want %v", err, test.wantErr) + } + if hint := codexTurnHint(err); !strings.Contains(hint, test.wantHit) { + t.Fatalf("codexTurnHint() = %q, want substring %q", hint, test.wantHit) + } + }) + } +} diff --git a/internal/tui/theme.go b/internal/tui/theme.go index e602f85..8b324eb 100644 --- a/internal/tui/theme.go +++ b/internal/tui/theme.go @@ -383,6 +383,11 @@ func ApplyTheme(t Theme) { // foreground and background. Unlike a cached light/dark guess, the // defaults continue to work when the terminal changes appearance while // qmax is running, and nested ANSI spans cannot punch background holes. + // Accepted limitation: theme-colored accents below (Brand, Accent, + // IconCodex, Success) still come from the previewed theme, so previewing + // an opposite-polarity theme can render accents with reduced contrast. + // Structural text stays readable in every combination, which is the + // guarantee this style set owes the picker. pickerLabel = lipgloss.NewStyle() pickerLabelSel = lipgloss.NewStyle().Bold(true) pickerBadgeNew = lipgloss.NewStyle().