-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: don't return full shell output when very large #3750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
DOsinga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find!
* main: Increase req body limit (#2965) Stable goose info -v (#3760) Speed up app initialization and improve refresh crashing (#3717) docs: consolidate search session content, doc import recipe (#3759) Improve power save blocker mechanism (#3698) Ensure adding/removing extensions refreshes extensions list (#3695) Env parsing for primitive types (#3706) Autocompact + One Shot Summarization algorithm (#3559) fix: initial prompt not filled in after accepting new recipe (#3637) fix not being able to click on searchbar buttons in chat (#3723) center session summary modal description text (#3737) Persist first message to local history in case of failure or cancellation (#3744) Make the client more secure (#3742) feat: Allow configuring hints filename(s) (#3269) Add support for mouse back nav button to Settings screen (#3195) chore: Remove the wrong tailwind package (#3754) chore: fix typo in desktop readme for goosed (#3752) feat: upgrade rmcp (#3738) feat: allow users view and edit their non-secret config's (#3005) fix: View extensions link (#3751)
* main: fix: cli tool logging (#3749)
|
thanks @cgwalters for optimisation - will do a fast follow for long docs/lines I think |
| id: generateId(), | ||
| role: apiMessage.role as Role, | ||
| created: apiMessage.created, | ||
| created: apiMessage.created ?? 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* main: fix: don't return full shell output when very large (#3750) fix: cli tool logging (#3749) Increase req body limit (#2965) Stable goose info -v (#3760) Speed up app initialization and improve refresh crashing (#3717) docs: consolidate search session content, doc import recipe (#3759) Improve power save blocker mechanism (#3698) Ensure adding/removing extensions refreshes extensions list (#3695) Env parsing for primitive types (#3706) Autocompact + One Shot Summarization algorithm (#3559)
…ipe-chat-via-deeplink * 'main' of github.com:block/goose: Ensure more client (#3787) fix(ui): extension command text overflow (#3785) No tool role means we should not collapse messages (#3778) fix: bundle workflows (#3780) Update goose hints (#3758) integrate MCP UI (#2948) Fix claude model names (#3765) fix: don't return full shell output when very large (#3750) fix: cli tool logging (#3749)
|
➡️ #3816 |
| ToolError::ExecutionError(format!("Failed to write to temporary file: {}", e)) | ||
| })?; | ||
|
|
||
| let (_, path) = tmp_file.keep().map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's nothing that's garbage collecting these temporary files (apart from the OS level tempfile cleaner right?)
That seems very suboptimal...I did this IMO a better way in #2817
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long to retain the tempfiles is an interesting topic - in #2817 notice that the tempfile is owned in Rust by the mcp server (so we get cleanup on drop) and the semantics basically are that the next shell command that outputs large data replaces it.
My thought is that in a flow of doing make/cargo/whatever build (gets redirected), then the model can do e.g. grep <tmpfile> error: and as long as that doesn't overflow 100 lines the tempfile is preserved, which should hopefully be a common case. But it might needs careful explaining to the model, I didn't really hammer on it from that angle.
Leaking the tempfiles entirely avoids that problem but...yeah I don't think we want to do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the os will clean them up which is preferred (and isn't the end of the world if they are gone in an old session, hopefully we got the useful information). Anything more durable and yeah it should be in the session structure itself (but not sent to the LLM)
The shell (bash) command can result in a lot of pointless context filling, if is running a compile, or log tailing and so on.
In reality only some of it is relevant, in this case it will limit it to around 100 lines (a few percentage points of context) and allow it to then search for more content when needed.
For example, you can tell it to run a command like:
can you run find /System/Library -type f | head -n 2000(that was just an example I could consistently use)
by default - this eats half the context window in one fell swoop:

with this change:
that is a contrived example but it isn't uncommon to hit this.