Skip to content

Conversation

@michaelneale
Copy link
Collaborator

fixes: #3682

.arg("json");
.arg(&filtered_system);

// Only pass model parameter if it's in the known models list
Copy link
Contributor

@kwsantiago kwsantiago Jul 28, 2025

Choose a reason for hiding this comment

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

Consider adding

// This allows users to use "default" or any future CLI models without updates

(and same for gemini_cli.rs)

@kwsantiago
Copy link
Contributor

kwsantiago commented Jul 28, 2025

@michaelneale Great solution! I think as is your solution is excellent, main feedback I could give is consider updating KNOWN_MODELS since the CLIs support more models than we list.

We could either:

  • Keep it minimal (current approach)
  • Or document that KNOWN_MODELS is intentionally incomplete to allow CLI flexibility

What do you think?

@michaelneale
Copy link
Collaborator Author

ideally if someone types it in then we would pass it through - otherwise we dont' pass anything, maybe that is better approach vs having any list/hard coding at all?

@kwsantiago
Copy link
Contributor

I still think having a known models list makes sense to prevent user error that could cause the CLI to fail (ie. invalid/outdated/unsupported model or typos etc).

The middle ground would be to only skip passing the model for specific values like "default" or empty string, otherwise let it pass through. That would avoid hardcoding model lists while allowing CLI defaults.

Either way, what you have in this PR should be sufficient to solve the regression issue, I think it's a great solution as is.

@michaelneale michaelneale merged commit 2382763 into main Jul 29, 2025
8 checks passed
@michaelneale michaelneale deleted the micn/fix-default-models-cli-providers branch July 29, 2025 00:27
michaelneale added a commit that referenced this pull request Jul 29, 2025
* main:
  blog: streamlining detection development w/ recipes (#3689)
  fix: have option for cli providers to use their configured or default model  (#3683)
  docs: new blog post and corrections to an old one on goosehints (#3657)
  Resolve sub recipe path relative to the parent recipe path (#3642)
  Speed up recipe loading from deeplinks and various fixes (#3662)
  fix cmd + , not opening settings (#3694)
  Add warning when JSON env parsing fails. (#3696)
  chore: refactor session naming into provider (#3678)
  feat (ui): File picker for scheduling recipes default to recipe dir (#3611)
  fix: address issue with streamable http interactions via mcp (#3693)
  Provider scenario tests (#3688)
  Fix conversations before they hit the LLM (#3660)
  cli: add detailed instruction for WSL users (#3496)
  feat: recipe runs will now prompt for missing extension secrets (#3668)
  fix: pricing integration tests -> trying more runs for cache and retries (#3546)
michaelneale added a commit that referenced this pull request Jul 29, 2025
* main:
  fix: Ensures final output tool is available when using vector tool search (#3701)
  chore: adding in some new models token limits (#3685)
  blog: streamlining detection development w/ recipes (#3689)
  fix: have option for cli providers to use their configured or default model  (#3683)
  docs: new blog post and corrections to an old one on goosehints (#3657)
  Resolve sub recipe path relative to the parent recipe path (#3642)
  Speed up recipe loading from deeplinks and various fixes (#3662)
  fix cmd + , not opening settings (#3694)
  Add warning when JSON env parsing fails. (#3696)
  chore: refactor session naming into provider (#3678)
  feat (ui): File picker for scheduling recipes default to recipe dir (#3611)
  fix: address issue with streamable http interactions via mcp (#3693)
  Provider scenario tests (#3688)
atarantino pushed a commit to atarantino/goose that referenced this pull request Aug 5, 2025
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.

regression: claude cli and gemini cli providers should have a default

4 participants