-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: add a max tokens env var #6264
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
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.
Pull request overview
This PR adds support for configuring max tokens via the GOOSE_MAX_TOKENS environment variable, allowing users to control the maximum number of tokens in model responses.
- Implements
parse_max_tokens()method that reads and validates theGOOSE_MAX_TOKENSenvironment variable - Integrates max tokens parsing into
ModelConfig::new_with_context_env() - Adds comprehensive test coverage for valid values, invalid inputs, and edge cases
crates/goose/src/model.rs
Outdated
| return Err(ConfigError::InvalidRange( | ||
| "GOOSE_MAX_TOKENS".to_string(), | ||
| "must be greater than 0".to_string(), | ||
| )); |
Copilot
AI
Dec 24, 2025
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.
The second parameter to InvalidRange is inconsistent with parse_temperature. In parse_temperature (line 179), the actual value is passed (val), but here a descriptive message is passed. For consistency, pass val here to match the existing pattern.
crates/goose/src/model.rs
Outdated
| } | ||
|
|
||
| fn parse_max_tokens() -> Result<Option<i32>, ConfigError> { | ||
| if let Ok(val) = std::env::var("GOOSE_MAX_TOKENS") { |
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.
rather than using an environment variable, should we make this go through the config module, which checks environment variables anyway and has some light templating for types going on?
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 sounds good. i changed just this one field for now rather than the full model config. can come back to doing the other ones in this file that also don't use config
* 'main' of github.com:block/goose: refactor: when changing provider/model,load existing provider/model (#6334) chore: refactor configure_extensions_dialog to reduce line count (#6277) chore: refactor handle_configure to reduce line count (#6276) chore: refactor interactive session to reduce line count (#6274) chore: refactor docx_tool to reduce function size (#6273) chore: refactor cli() function to reduce line count (#6272) make sure the models are using streaming properly (#6331) feat: add a max tokens env var (#6264) docs: slash commands topic (#6333) fix(ci): prevent gh-pages branch bloat (#6340) chore(deps): bump qs and body-parser in /documentation (#6338) Skip the smoke tests for dependabot PRs (#6337)
No description provided.