goose icon indicating copy to clipboard operation
goose copied to clipboard

fix: enable markdown syntax highlighting in macOS Terminal.app

Open ConstantTime opened this issue 1 month ago • 4 comments

Summary

print_markdown used to bail out whenever Stdout::is_terminal() returned false. On macOS Terminal.app that check is flaky, so the CLI fell back to literal markdown (no colored headers/bold/syntax highlighting). This change centralizes the decision in should_use_colored_output(), layering multiple signals so we still feed the response through bat whenever the terminal can actually understand ANSI.

Key changes:

  • treat NO_COLOR as the top-level kill-switch (env_no_color() now returns true when the var is set)
  • add should_use_colored_output() that:
    • returns false immediately if NO_COLOR/TERM=dumb is present
    • returns true if stdout().is_terminal() succeeds
    • otherwise inspects TERM, TERM_PROGRAM, and COLORTERM so macOS Terminal.app (and other color-capable terms such as xterm-256color) still get color even when is_terminal() lies
  • reuse that helper in both print_markdown and render_text_no_newlines so all CLI paths respect the same policy
  • switch bat::PrettyPrinter::colored_output(!env_no_color()) to line up with the inverted, corrected env_no_color() semantics

Type of Change

  • [ ] Feature
  • [x] Bug fix
  • [ ] Refactor / Code quality
  • [ ] Performance improvement
  • [ ] Documentation
  • [ ] Tests
  • [ ] Security fix
  • [ ] Build / Release
  • [ ] Other (specify below)

Testing

  • cargo fmt
  • cargo build
  • Manual:
    • Terminal.app interactive session (main vs branch) with markdown-heavy prompts
    • Pipe to cat and cat -v on both builds to validate the ANSI output behaviour
    • NO_COLOR=1 run for plain-text regression coverage

Related Issues

Relates to #4468

Screenshots/Demos (for UX changes)

Before:

Screenshot 2025-10-27 at 9 52 06 PM

After:

Screenshot 2025-10-27 at 9 44 22 PM

Email: [email protected]

ConstantTime avatar Oct 27 '25 16:10 ConstantTime

Thank you for the contribution, @ConstantTime ! As we wait for the dev team to review, can you also fix the DCO check? You can follow instructions here to do so

taniandjerry avatar Oct 28 '25 18:10 taniandjerry

@DOsinga @alexhancock @zanesq @jamadeo markdown PR for review!

taniandjerry avatar Oct 28 '25 18:10 taniandjerry

thanks for your contribution. however I am still not sure what we're trying to fix here.

your example screenshot shows that if we go through a pipe, we will now be using colors. this is not what we want, we only want to use colors when we are displaying directly to the screen. when the output is redirected you want non-ansi code because you are probably going to programmatically process it.

DOsinga avatar Oct 28 '25 19:10 DOsinga

thanks for your contribution. however I am still not sure what we're trying to fix here.

your example screenshot shows that if we go through a pipe, we will now be using colors. this is not what we want, we only want to use colors when we are displaying directly to the screen. when the output is redirected you want non-ansi code because you are probably going to programmatically process it.

The main fix we want is as described in the original bug,

goose CLI does not display markdown syntax highlighting when running in Terminal.app on macOS, despite the terminal supporting full color and formatting capabilities

taniandjerry avatar Oct 28 '25 20:10 taniandjerry

Since I didn't get any followup items addressed on this issue, I'll be taking the hacktoberfest label off of this contribution in the interim.

I will also close this issue in the interim, and hopefully you @ConstantTime or another contributor are able to get your contribution in for something like this, knowing the direction the devs want as mentioned by @DOsinga .

taniandjerry avatar Oct 30 '25 14:10 taniandjerry

Since I didn't get any followup items addressed on this issue, I'll be taking the hacktoberfest label off of this contribution in the interim.

I will also close this issue in the interim, and hopefully you @ConstantTime or another contributor are able to get your contribution in for something like this, knowing the direction the devs want as mentioned by @DOsinga .

oh sorry @taniandjerry. It's been a super busy week for me at office and I missed this thread somehow. Closing this PR makes sense as per your reasoning. Going through comments from @.DOsinga to understand the gap better.

ConstantTime avatar Oct 31 '25 15:10 ConstantTime