Skip to content

Conversation

@DOsinga
Copy link
Collaborator

@DOsinga DOsinga commented Aug 1, 2025

If you capture the output of goose, you don't need the ansi stuff

fixes #3736

@DOsinga DOsinga requested a review from jamadeo August 1, 2025 10:28
@@ -1,26 +1,27 @@
use std::collections::HashMap;

use crate::g_println;
Copy link
Contributor

Choose a reason for hiding this comment

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

There's https://docs.rs/anstream/latest/anstream/ (and already in the depchain as it's used by clap)

Copy link
Contributor

Choose a reason for hiding this comment

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

And when using it it's way easier to just shadow the builtin println! via use anstream::println; as the docs suggest.

(With an approach like this inevitably some will be missed)

@cgwalters
Copy link
Contributor

BTW reminderping, I reviewed this PR, will trade for a re-review of #2821

@DOsinga
Copy link
Collaborator Author

DOsinga commented Aug 3, 2025

thanks, yeah good point - i had not seen that. let me get on that.

@DOsinga
Copy link
Collaborator Author

DOsinga commented Aug 4, 2025

cleaned up @cgwalters

@DOsinga DOsinga requested a review from lifeizhou-ap August 4, 2025 16:21
@@ -1,5 +1,6 @@
use std::collections::HashMap;

use anstream::println;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we also need to import the eprintln wrapper here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's a good question - this PR is fixing automations based on recipe. stderr is not useful at this point for much when it comes to recipes

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the surrounding context but my understanding of the bug we're aiming to fix is "don't write ANSI to non-ttys" which is definitely a good idea globally.

This PR is fixing that for println! but is also changing some things to use eprintln! (why?) but crucially also is not using the anstyle wrapper so we'll still output ANSI styles to stderr which seems wrong to me.

IOW we should consistently use anstream::{println, eprintln}; at least right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mostly agree, just not sure I want this to be held up because of that. eprint vs print in the code is kind of a mess right now

Comment on lines +480 to +481
if std::io::stdout().is_terminal() {
bat::PrettyPrinter::new()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am less familiar with bat and this cliclack crate but IME with https://docs.rs/indicatif/latest/indicatif/ it's pretty easy to use e.g. https://docs.rs/indicatif/latest/indicatif/struct.ProgressBar.html#method.println to automate these kinds of conditional prints.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

looks cool. here we are just skipping the formatting of markdown though

@DOsinga DOsinga merged commit bef7622 into main Aug 5, 2025
9 checks passed
@DOsinga DOsinga deleted the suppress-ansi-with-pipes branch August 5, 2025 09:37
katzdave added a commit that referenced this pull request Aug 5, 2025
* 'main' of github.com:block/goose:
  Changed app settings configuration form to match settings panels (#3829)
  Tell the user to hit compact (#3851)
  Pin @mcp-ui/client in package.json (#3860)
  blog for mcp-jupyter server (#3059)
  docs: Adding dev.to Tutorial & Update CLI Component (#3828)
  Detect client disconnects and cancel tool calls (#3782)
  Suppress ansi with pipes (#3775)
  Fix leaky env variable causing flaky test (#3761)
  Update gemini error msg (#3847)
  Generic retry and error parsing (#3558)
  Clear the current line on ctrl-c in line with other tools (#3764)
  chore: upgrade morph to use new model with instruction (#3745)
  add CODEOWNERS file with /documentation owners (#3840)
michaelneale added a commit that referenced this pull request Aug 5, 2025
* main:
  fix: replace glob/grep tool with shell (#3834)
  docs: Add Youtube Link to dev.to tutorial (#3869)
  Changed app settings configuration form to match settings panels (#3829)
  Tell the user to hit compact (#3851)
  Pin @mcp-ui/client in package.json (#3860)
  blog for mcp-jupyter server (#3059)
  docs: Adding dev.to Tutorial & Update CLI Component (#3828)
  Detect client disconnects and cancel tool calls (#3782)
  Suppress ansi with pipes (#3775)
kathawthorne added a commit to kathawthorne/goose that referenced this pull request Aug 5, 2025
…-files

* upstream/main: (150 commits)
  fix: replace glob/grep tool with shell (block#3834)
  docs: Add Youtube Link to dev.to tutorial (block#3869)
  Changed app settings configuration form to match settings panels (block#3829)
  Tell the user to hit compact (block#3851)
  Pin @mcp-ui/client in package.json (block#3860)
  blog for mcp-jupyter server (block#3059)
  docs: Adding dev.to Tutorial & Update CLI Component (block#3828)
  Detect client disconnects and cancel tool calls (block#3782)
  Suppress ansi with pipes (block#3775)
  Fix leaky env variable causing flaky test (block#3761)
  Update gemini error msg (block#3847)
  Generic retry and error parsing (block#3558)
  Clear the current line on ctrl-c in line with other tools (block#3764)
  chore: upgrade morph to use new model with instruction (block#3745)
  add CODEOWNERS file with /documentation owners (block#3840)
  Token counting in Auto-compact uses provider metadata (block#3788)
  docs: Add YouTube link to Git MCP Tutorial (block#3831)
  feat: more robust client initialization for the app (block#3830)
  Build app bundles on release branches always (block#3789)
  fix param order of debug_conversation_fixer (block#3796)
  ...

# Conflicts:
#	crates/goose-mcp/src/developer/mod.rs
kathawthorne added a commit to kathawthorne/goose that referenced this pull request Aug 5, 2025
…e-editable-displayable-title

* upstream/main: (134 commits)
  fix: optimise reading large file content (block#3767)
  fix: replace glob/grep tool with shell (block#3834)
  docs: Add Youtube Link to dev.to tutorial (block#3869)
  Changed app settings configuration form to match settings panels (block#3829)
  Tell the user to hit compact (block#3851)
  Pin @mcp-ui/client in package.json (block#3860)
  blog for mcp-jupyter server (block#3059)
  docs: Adding dev.to Tutorial & Update CLI Component (block#3828)
  Detect client disconnects and cancel tool calls (block#3782)
  Suppress ansi with pipes (block#3775)
  Fix leaky env variable causing flaky test (block#3761)
  Update gemini error msg (block#3847)
  Generic retry and error parsing (block#3558)
  Clear the current line on ctrl-c in line with other tools (block#3764)
  chore: upgrade morph to use new model with instruction (block#3745)
  add CODEOWNERS file with /documentation owners (block#3840)
  Token counting in Auto-compact uses provider metadata (block#3788)
  docs: Add YouTube link to Git MCP Tutorial (block#3831)
  feat: more robust client initialization for the app (block#3830)
  Build app bundles on release branches always (block#3789)
  ...
michaelneale added a commit that referenced this pull request Aug 5, 2025
* main: (33 commits)
  fix: optimise reading large file content (#3767)
  fix: replace glob/grep tool with shell (#3834)
  docs: Add Youtube Link to dev.to tutorial (#3869)
  Changed app settings configuration form to match settings panels (#3829)
  Tell the user to hit compact (#3851)
  Pin @mcp-ui/client in package.json (#3860)
  blog for mcp-jupyter server (#3059)
  docs: Adding dev.to Tutorial & Update CLI Component (#3828)
  Detect client disconnects and cancel tool calls (#3782)
  Suppress ansi with pipes (#3775)
  Fix leaky env variable causing flaky test (#3761)
  Update gemini error msg (#3847)
  Generic retry and error parsing (#3558)
  Clear the current line on ctrl-c in line with other tools (#3764)
  chore: upgrade morph to use new model with instruction (#3745)
  add CODEOWNERS file with /documentation owners (#3840)
  Token counting in Auto-compact uses provider metadata (#3788)
  docs: Add YouTube link to Git MCP Tutorial (#3831)
  feat: more robust client initialization for the app (#3830)
  Build app bundles on release branches always (#3789)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Print only structured response at goose run --recipe

4 participants