Skip to content

fix: repair theme polarity artifacts and Codex 0.144 harness contract - #200

Merged
Desperado merged 3 commits into
mainfrom
fix/theme-polarity-codex-contract
Sep 14, 2026
Merged

Desperado merged 3 commits into
mainfrom
fix/theme-polarity-codex-contract

Conversation

@Desperado

@Desperado Desperado commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • fix(tui): picker structural text (labels, dividers, status bar, selected rows) now uses terminal-default foreground/background instead of explicit neutrals frozen at startup — eliminates the black stair-step artifacts when switching between light and dark themes, and keeps working when the terminal changes appearance mid-session. Theme-colored accents keep theme identity; the reduced-contrast preview tradeoff is documented in code.
  • fix(codexrunner): Codex 0.144 emits turn.failed and exits non-zero — previously model/auth/plan-limit outcomes surfaced as generic process failures. Now: plan-limit keeps its user-facing outcome; other diagnostics classify to ErrModelUnavailable / ErrAuthentication via bounded 64KiB stderr scanned for fixed markers only (no raw diagnostics cross the boundary). item.completed/agent_message parsing already existed on main; the new fixtures exercise it.
  • fix(agent): classified failures print actionable hints (codex login / model picker) through codexTurnHint, with sentinels still wrapped for programmatic use.
  • ci: credential-free codex-compat.yml pins the CLI flag contract on 0.144.5 and latest; steps are named for their real scope and the event-schema-drift gap (needs an authenticated smoke test) is documented in the workflow.

Test evidence

ok  github.com/qualitymax/qmax-code/internal/tui     1.280s
ok  github.com/qualitymax/qmax-code/codexrunner      0.937s
ok  github.com/qualitymax/qmax-code/internal/agent   5.733s
go vet: clean, git diff --check: clean

New tests: agent-level fake-CLI coverage for both classifications (TestCodexAgentSurfacedClassifiedFailuresWithHints); fixture replay for success/failure/plan-limit in codexrunner.

Known limitations

  • The latest CI leg verifies flags only; JSONL schema drift in future Codex releases requires an authenticated smoke test to catch.
  • Previewing an opposite-polarity theme can render theme-colored accents with reduced contrast (structural text stays readable).

Picker panels now paint explicit backgrounds on every repaint so rows no
longer retain stale terminal polarity after switching between light and
dark themes.
@qualitymaxapp

qualitymaxapp Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

QualityMax Review

Verdict: COMMENT · Confidence: evidence-backed scan

Files eligible: 10 · Files reviewed: 10 · Files with findings: 1 · Findings: 1 · Inline cards: 1

Priority findings

priority location finding
P3 codexrunner/runner.go:635 The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written.

Review gates

gate status
AI diff review completed · eligible 10, reviewed 10 · LLM · served gemini-3.1-flash-lite
SAST completed · eligible 10, reviewed 10 · hybrid · served qwen3.7-plus
Overall review evidence non-blocking findings remain
Inline evidence posted

Important files

file risk note next step
codexrunner/runner.go P3 The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written. Inspect the inline card and apply the smallest safe fix.

Change diagram — Flow

flowchart TD
    A[Codex CLI Execution] --> B{Process Exits}
    B -- Success --> C[Result Returned]
    B -- Failure --> D[Capture Stderr in boundedBuffer]
    D --> E[classifyCodexDiagnostic]
    E -- Known Error --> F[Return Typed Error (e.g., ErrAuthentication)]
    E -- Unknown --> G[Return Generic ErrProcess]
Loading

Review lifecycle

Use the inline cards to inspect evidence and suggested remediation. Re-run the QualityMax review after pushing a fix; unchanged cards are identified by their stable finding marker. Dismiss with a reason through the existing QualityMax/GitHub review feedback flow. 1 prior card(s) are stale/resolved on this head. Thread reconciliation resolved 1/1 superseded QualityMax thread(s). @qmax Q&A is tracked separately.

Proof legend: VERIFIED independently judged patch · REPRODUCED verified finding · GROUNDED deterministic evidence · MODEL-ONLY model judgment.

QualityMax project results are available in the configured project.

Receipt · commit 94096829e8aa61fe1c2ca04b9e01d93dd0355ba1 · run 2026-09-12T20:52:13+00:00 · model served gemini-3.1-flash-lite · model requested gemini-3.1-flash-lite · model review recorded — 1902 model output tokens · model source not recorded · re-review 2 · proof counts {'MODEL-ONLY': 1}

@qualitymaxapp

qualitymaxapp Bot commented Sep 12, 2026

Copy link
Copy Markdown

⚠️ QualityMax Diff Analysis — WARN

The PR introduces robust error handling for the Codex CLI, including a bounded buffer for stderr to prevent memory exhaustion and a classification system to sanitize provider diagnostics. The TUI theme refactor improves terminal compatibility. However, the boundedBuffer.Write implementation violates the io.Writer interface contract, which may cause issues if reused.

🟡 Finding 1: logic

File: codexrunner/runner.go:615 | Severity: warning

What: The Write method of boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract.

Fix: Update the Write method to return the length of the slice actually written (the truncated length) to correctly adhere to the io.Writer contract.

Fix with your LLM agent
<your-llm-agent> "Fix the logic issue in codexrunner/runner.go at line 615: The Write method of boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract."
Fix all findings with your LLM agent
<your-llm-agent> "Fix all QualityMax review findings in this PR:\n- logic in codexrunner/runner.go:615: The Write method of boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract."

Change diagram — Flow

flowchart TD
    A[CLI Process] -->|Stderr| B(boundedBuffer)
    B -->|Truncated Output| C{classifyCodexDiagnostic}
    C -->|Match| D[ErrModelUnavailable / ErrAuthentication]
    C -->|No Match| E[ErrProcess]
Loading

Analyzed commit 4b029be7 with gemini-3.1-flash-lite (8 files, 12643 tokens) | Customize review preferences

@qualitymaxapp qualitymaxapp Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QualityMax Review — canonical overview updated; inline findings are attached to this review.

Comment thread codexrunner/runner.go
@qualitymaxapp qualitymaxapp Bot added bug Something isn't working qualitymax:reviewed labels Sep 12, 2026
@qualitymaxapp

qualitymaxapp Bot commented Sep 12, 2026 •

Copy link
Copy Markdown

⚠️ QualityMax Pipeline

Gate Result
🔍 AI diff review ⚠️ 1 warning(s) · gemini-3.1-flash-lite · completed · 10 eligible / 10 reviewed · gemini-3.1-flash-lite
🔍 SAST completed · 10 eligible / 10 reviewed · qwen3.7-plus
🔍 Canonical PR review delivery completed · 0 eligible / 0 reviewed · exact-head review #5188014024 and overview #5648478213 confirmed
🧪 Repo Tests ✅ 817/817 passed (go)

Powered by QualityMax — AI-Powered Test Automation

…ostics

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
- 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
@Desperado
Desperado force-pushed the fix/theme-polarity-codex-contract branch from 4b029be to 9409682 Compare September 12, 2026 20:50
@qualitymaxapp

qualitymaxapp Bot commented Sep 12, 2026

Copy link
Copy Markdown

⚠️ QualityMax Diff Analysis — WARN

The PR introduces robust error handling and sensitive data sanitization for the Codex CLI, including a memory-bounded stderr capture. While the logic is sound, the custom io.Writer implementation violates the interface contract by reporting more bytes written than actually processed, which could lead to unexpected behavior in the exec package.

🟡 Finding 1: logic

File: codexrunner/runner.go:635 | Severity: warning

What: The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written.

Fix: Update the Write method to return the length of the slice actually written (len(p)) instead of the original length.

Fix with your LLM agent
<your-llm-agent> "Fix the logic issue in codexrunner/runner.go at line 635: The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written."
Fix all findings with your LLM agent
<your-llm-agent> "Fix all QualityMax review findings in this PR:\n- logic in codexrunner/runner.go:635: The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written."

Change diagram — Flow

flowchart TD
    A[Codex CLI Execution] --> B{Process Exits}
    B -- Success --> C[Result Returned]
    B -- Failure --> D[Capture Stderr in boundedBuffer]
    D --> E[classifyCodexDiagnostic]
    E -- Known Error --> F[Return Typed Error (e.g., ErrAuthentication)]
    E -- Unknown --> G[Return Generic ErrProcess]
Loading

Analyzed commit 94096829 with gemini-3.1-flash-lite (10 files, 12877 tokens) | Customize review preferences

@qualitymaxapp qualitymaxapp Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QualityMax Review — canonical overview updated; inline findings are attached to this review.

Comment thread codexrunner/runner.go
original := len(p)
if len(p) > buffer.remaining {
p = p[:buffer.remaining]
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 · logic · MODEL-ONLY

The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written.

Evidence: codexrunner/runner.go:635
Impact / next step: Update the Write method to return the length of the slice actually written (len(p)) instead of the original length.

Detailed reasoning
  • Root cause: The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written.
  • Threat model: Review callers that can reach codexrunner/runner.go:635.
  • Existing protection: Reported by llm; confidence: high.
  • Alternatives considered: Preserve the current interface; prefer the smallest safe change.
  • Severity calibration: P3 based on the scanner severity.
Prompt to fix with AI
Fix this logic finding in codexrunner/runner.go:635. Evidence: The Write method in boundedBuffer returns the original length of the input slice even when it truncates the data, violating the io.Writer interface contract which requires returning the number of bytes actually written. Requested outcome: Update the Write method to return the length of the slice actually written (len(p)) instead of the original length.. Preserve existing behavior and add focused coverage.

QualityMax · proof: MODEL-ONLY · served model: gemini-3.1-flash-lite · requested: gemini-3.1-flash-lite

@Desperado
Desperado merged commit 41a65ac into main Sep 14, 2026
8 checks passed
@Desperado
Desperado deleted the fix/theme-polarity-codex-contract branch September 14, 2026 20:59
@Desperado Desperado mentioned this pull request Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working qualitymax:reviewed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant