feat(output): honor NO_COLOR for JSON output - #170
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Castiron custom code✅ No new custom-code files detected. 5 mixed files remain; 0 existing customizations changed. Compared 5 existing customizations unchanged
A changed generated baseline means this report cannot reliably identify which handwritten lines changed. Inspect the custom-code diffDownload the exact patch produced by this run (requires repository access): gh run download 34200521124 --repo openai/openai-cli \
--name castiron-custom-code-34200521124-1 --dir /tmp/castiron-custom-code-34200521124-1
git apply --stat /tmp/castiron-custom-code-34200521124-1/custom-code.patch
cat /tmp/castiron-custom-code-34200521124-1/custom-code.patchOr reproduce it from an SDK checkout containing the vendored reporter: git fetch --no-tags origin 265cf6378ab2e6a27c17bc7c0b6a6cbe6dbd477b 043aeaee842956f516fcac7abe4018e70a86bc1b
python3 scripts/castiron/custom_code_report.py report \
--base 265cf6378ab2e6a27c17bc7c0b6a6cbe6dbd477b \
--head 043aeaee842956f516fcac7abe4018e70a86bc1b --fetch --require-head-hash --public \
--out /tmp/castiron-custom-code-043aeaee8429
cat /tmp/castiron-custom-code-043aeaee8429/custom-code.patchThis is the current full custom patch for mixed files, not an attribution of only the handwritten lines changed by this PR. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped to JSON/JSONL color gating and pager env behavior, with clear precedence semantics and comprehensive targeted regression tests.
Pull request overview
This PR updates the CLI’s JSON/JSONL (and default auto) output coloring behavior to honor NO_COLOR while preserving FORCE_COLOR precedence, including when output is routed through a pager, and adds targeted regression coverage plus documentation.
Changes:
- Update JSON/JSONL color selection to disable default terminal color when
NO_COLORis nonempty, while keepingFORCE_COLOR=1/0overrides. - Prevent both pager backends from injecting
FORCE_COLOR=1whenNO_COLORis active, keeping pre-pager and in-pager formatting consistent. - Add focused environment-precedence tests (including pager backend coverage) and document the precedence rules in the README.
File summaries
| File | Description |
|---|---|
| README.md | Documents NO_COLOR/FORCE_COLOR precedence for JSON/JSONL/auto output. |
| pkg/cmd/cmdutil.go | Implements NO_COLOR handling in color selection and avoids forcing color for the pipe pager backend when NO_COLOR is set. |
| pkg/cmd/cmdutil_unix.go | Applies the same “don’t inject FORCE_COLOR=1 under NO_COLOR” rule to the Unix socket-pair pager backend. |
| pkg/cmd/cmdutil_test.go | Stabilizes existing color-related terminal tests by explicitly clearing inherited NO_COLOR. |
| pkg/cmd/cmdutil_color_unix_test.go | Adds regression coverage ensuring both pager backends preserve color env precedence and don’t override user choices under NO_COLOR. |
| pkg/cmd/cmdutil_color_test.go | Adds matrix tests for NO_COLOR/FORCE_COLOR precedence across formats and destinations, plus a pager iterator regression for NO_COLOR. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jbeckwith-oai
left a comment
jbeckwith-oai
left a comment
There was a problem hiding this comment.
Reviewed the complete six-file change at the current exact head. Nonempty NO_COLOR now disables automatic JSON, JSONL, and auto-format terminal color, including both pager backends, while FORCE_COLOR=1 and FORCE_COLOR=0 keep explicit precedence and other values retain automatic behavior. I traced the buffered/pre-pager and streamed/post-pager paths and found payload, escaping, transforms, destinations, and iterator bounds unchanged; the output-control change does not introduce a new terminal-escape path. Focused color and pager tests, the full internal package suite, lint/build, Go vet, and whitespace checks pass locally. All hosted checks, including the mock-backed suite and CodeQL, are green. I found no blocking issue.
Summary
Honor nonempty
NO_COLOR(including0) when JSON, JSONL, or defaultautooutput would otherwise add terminal color. Empty or unsetNO_COLORpreserves automatic behavior. ExplicitFORCE_COLOR=1andFORCE_COLOR=0retain precedence, including redirected output.Both pager backends now avoid injecting
FORCE_COLOR=1whenNO_COLORis active, keeping buffered and streamed output consistent. The change adds focused regression tests and documents precedence; payloads, escaping, transforms, newlines, destinations, and iterator limits retain their behavior.Validation
Local Go 1.27.0 on Linux:
NO_COLOR.go test ./internal/...;go test ./... -run '^$'(compilation only).go mod verify;./scripts/lint(Go build); focused vet and selected output race tests.043aeaee842956f516fcac7abe4018e70a86bc1b: 495/1000 lines, verified generated snapshot, isolation passed.Full local mock suite not run: the supported pinned Steady launcher reports
Missing or linked Steady cache. The previously blocked setup route was not retried. Hosted CI and full mock tests passed on the published head via test merge254a0be7bc0847a447bd68f6b1bcb2051136418c, using Go 1.25.14/Linux. The log confirms pinned mock integrity/health checks, all-package tests, Windows test cross-compilation, and cleanup.