fix: provenance and syntax highlighting respect --no-color and pipe detection#2281
Merged
fix: provenance and syntax highlighting respect --no-color and pipe detection#2281
Conversation
…hen piping Provenance output (`describe component --provenance`) emitted ANSI escape codes when stdout was piped or redirected, and did not respect `--no-color`. Two root causes: (1) provenance rendering used `lipgloss.NewStyle()` unconditionally without checking stdout TTY status, and (2) `HighlightCodeWithConfig` checked `IsTTYSupportForStdout() || IsTTYSupportForStderr()` which always returned true when piping since stderr remains a TTY. - Add `isProvenanceColorEnabled()` that checks NoColor, ForceColor, then stdout TTY — matching the pattern used by list commands - Thread `useColor` bool through all provenance rendering functions so lipgloss calls are skipped entirely when color is disabled - Fix `HighlightCodeWithConfig` to check only stdout TTY (data channel output) - Route provenance output through `data.Write()` instead of `fmt.Print()` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRoutes provenance stdout through the shared writer, threads a computed color toggle through provenance rendering to honor terminal color settings, restricts syntax-highlight terminal detection to stdout TTY only, normalizes whitespace in provenance snapshot goldens, and adds tests for color/no-color rendering behavior. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/exec/describe_component.go (1)
257-265:⚠️ Potential issue | 🟠 MajorDon't render ANSI before the file branch.
RenderInlineProvenanceWithStackFile()runs before this decides between stdout andwriteOutputToFile(), sodescribe component --provenance --file out.yamlfrom a TTY will still save ANSI escape sequences. Please pass a destination-aware plain/color override into the renderer and its highlighting path so file output stays plain unless color is explicitly forced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exec/describe_component.go` around lines 257 - 265, RenderInlineProvenanceWithStackFile is being called before we decide whether output goes to a file, so ANSI escapes are embedded even when writeOutputToFile(file, ...) is used; change the call site to determine the destination first and pass a destination-aware color/plain override into RenderInlineProvenanceWithStackFile (or its internal highlighting helper) so it renders plain for file output and colored for TTY stdout (unless color is explicitly forced). Concretely, compute the output target (file != ""), derive a boolean or enum like useColor based on that target and existing force-color flags, and pass that flag into p.RenderInlineProvenanceWithStackFile (and any highlighting functions it calls) instead of rendering first and then choosing writeOutputToFile or data.Write.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/exec/describe_component.go`:
- Around line 257-265: RenderInlineProvenanceWithStackFile is being called
before we decide whether output goes to a file, so ANSI escapes are embedded
even when writeOutputToFile(file, ...) is used; change the call site to
determine the destination first and pass a destination-aware color/plain
override into RenderInlineProvenanceWithStackFile (or its internal highlighting
helper) so it renders plain for file output and colored for TTY stdout (unless
color is explicitly forced). Concretely, compute the output target (file != ""),
derive a boolean or enum like useColor based on that target and existing
force-color flags, and pass that flag into p.RenderInlineProvenanceWithStackFile
(and any highlighting functions it calls) instead of rendering first and then
choosing writeOutputToFile or data.Write.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 29ef25ba-98fb-4824-be87-31bbef514816
📒 Files selected for processing (3)
internal/exec/describe_component.gopkg/provenance/tree_renderer.gopkg/utils/highlight_utils.go
… removal The prior commit skips lipgloss styling when color is disabled (non-TTY), which removes trailing whitespace padding from provenance legend lines. Update the golden snapshot files to match the new clean output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2281 +/- ##
==========================================
+ Coverage 77.16% 77.22% +0.05%
==========================================
Files 1045 1045
Lines 98367 98388 +21
==========================================
+ Hits 75909 75981 +72
+ Misses 18209 18157 -52
- Partials 4249 4250 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Cover the new useColor=false branches in tree_renderer.go: - isProvenanceColorEnabled (nil, NoColor, ForceColor, default) - colorize with useColor disabled - formatProvenanceCommentWithStackFile plain-text output - renderProvenanceLegend with/without stack file - renderFileTree with empty/single/multiple file trees - addProvenanceToLine padding, wrapping, and nil entry Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/provenance/tree_renderer_test.go (1)
199-205: Add a compile-time sentinel for schema field references used here.This test relies on
schema.Terminal.NoColorandschema.Terminal.ForceColor; add a sentinel so field renames fail fast at compile time.Suggested sentinel
+var _ = schema.Terminal{ + NoColor: true, + ForceColor: true, +} + func TestBuildYAMLPathMap_MultilineSupport(t *testing.T) {As per coding guidelines "Add compile-time sentinels for schema field references in tests: when a test uses a specific struct field ..., add ... as a compile guard so a field rename immediately fails the build."
Also applies to: 214-215
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/provenance/tree_renderer_test.go` around lines 199 - 205, Add a compile-time sentinel that references the schema.Terminal fields used by the test so renames fail the build: declare a package-level unused variable like var _ = schema.Terminal{NoColor: false, ForceColor: false} (placed in pkg/provenance/tree_renderer_test.go near the test) to pin schema.Terminal.NoColor and schema.Terminal.ForceColor; also add the same sentinel or extend it to cover the other usage at the lines referenced (214-215) so both test references are guarded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/provenance/tree_renderer_test.go`:
- Around line 249-253: The test "useColor true returns styled text" currently
only checks that colorize(text, color, true) contains the original text, which
can pass if no styling is applied; update the test to assert the colored branch
actually changes the output by adding a non-equality check (e.g.,
assert.NotEqual(t, text, result)) or assert that the result contains ANSI escape
sequences; target the colorize call and the t.Run block to add this additional
assertion so the true branch is validated.
- Around line 221-229: The test hardcodes expected:false for the "default config
without TTY" case which couples it to the test runner environment; instead set
expected to the actual stdout TTY probe result at test runtime (e.g., compute
expected := term.IsTerminal(int(os.Stdout.Fd())) or use the package's existing
stdout/TTY probe helper) and assert against that value so the test checks
behavior not a fixed environment assumption; update the test case that
constructs schema.AtmosConfiguration / schema.AtmosSettings / Terminal to use
that computed expected value.
---
Nitpick comments:
In `@pkg/provenance/tree_renderer_test.go`:
- Around line 199-205: Add a compile-time sentinel that references the
schema.Terminal fields used by the test so renames fail the build: declare a
package-level unused variable like var _ = schema.Terminal{NoColor: false,
ForceColor: false} (placed in pkg/provenance/tree_renderer_test.go near the
test) to pin schema.Terminal.NoColor and schema.Terminal.ForceColor; also add
the same sentinel or extend it to cover the other usage at the lines referenced
(214-215) so both test references are guarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6b1ad406-74a1-45a8-8a92-3b10e7ab4ff5
📒 Files selected for processing (1)
pkg/provenance/tree_renderer_test.go
Make TTY test case dynamic instead of hardcoding expected value, and strengthen colorize assertion to verify ANSI codes are actually produced. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
aknysh
approved these changes
Apr 3, 2026
|
These changes were released in v1.215.0-rc.1. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
describe component --provenance) now strips ANSI color codes when stdout is piped or redirected, and respects--no-colorHighlightCodeWithConfigalso properly degrades when stdout is pipeddata.Write()instead offmt.Print()for proper I/O channel handlingwhy
lipgloss.NewStyle()unconditionally without checking stdout TTY status, so ANSI escape codes leaked into piped/redirected outputHighlightCodeWithConfigcheckedIsTTYSupportForStdout() || IsTTYSupportForStderr()— since stderr remains a TTY when piping stdout, this always returned true and defeated pipe detectionfmt.Print()bypassed the data channel I/O layer, skipping secret maskingreferences
pkg/list/list_values.go)HighlightCodeWithConfig's precedence:NoColorwins, thenForceColor, then stdout TTY detectionSummary by CodeRabbit
Improvements
Bug Fixes
Tests