fix(describegpt): honor explicit --base-url when value matches default#3893
Merged
jqnatividad merged 3 commits intoMay 23, 2026
Merged
Conversation
Codex review (job 2363, MEDIUM) flagged that the sentinel-based precedence couldn't distinguish an omitted --base-url from an explicit --base-url http://localhost:1234/v1: both produced flag_base_url == Some(DEFAULT_BASE_URL), so QSV_LLM_BASE_URL silently overrode an explicit-default CLI flag. Switch to the suggested approach — remove the docopt default for --base-url (and the matching --model default) and apply CLI > env > built-in default manually: - docopt USAGE: drop `[default: ...]` lines for --base-url and --model. flag_base_url / flag_model are now Some IFF the user explicitly passed the flag. - New `resolve_base_url(args)` and `resolve_model(args)` helpers apply the documented precedence in code. Used at all call sites that need a resolved value (check_model, the reqwest client builder, the api-key localhost check). - `get_prompt_file` now uses a plain `if let Some(cli_base_url)` check instead of `!= Some(DEFAULT_BASE_URL)`. Same for model. - DEFAULT_BASE_URL / DEFAULT_MODEL doc comments updated; they are no longer sentinels, just the final fallback when neither CLI nor env nor the prompt file supplies a value. Regression test (describegpt_baseurl_precedence_cli_default_over_env): runs describegpt with QSV_LLM_BASE_URL pointed at an obviously wrong URL AND --base-url http://localhost:1234/v1 explicit on the CLI. The new test fails on the pre-fix code (env URL leaks through) and passes after the fix. Help docs regenerated via `qsv --generate-help-md` to reflect the docopt USAGE change. All 72 describegpt tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -7 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codex review (job 2372, MEDIUM) flagged that the api-key localhost gate at run()'s top level now uses `resolve_base_url(&args)`, but that helper only walks CLI > env > built-in default. With `--prompt-file` pointing at a non-localhost provider and no CLI/env base URL, the helper returns "http://localhost:1234/v1", passes the localhost check, and lets describegpt proceed without an API key — then send unauthenticated requests to the remote provider URL stored in the prompt file. The fix uses the EFFECTIVE base URL (CLI > env > prompt_file > built-in default) at the gate, by reading `get_prompt_file(args)?.base_url`. `get_prompt_file` already applies that precedence and writes the result into `prompt_file.base_url`, so it's the canonical source. Also tightened two related call sites: - `check_model`: the no-`--prompt-file` branch used to call `resolve_base_url(args)` which had the same gap. Since `get_prompt_file` is already called at the top of the function and falls back to the bundled default prompt's base_url, the branch collapses to `prompt_file.base_url.clone()` for both cases. - `run_inference_options`: pass the effective URL to `create_reqwest_blocking_client` so reqwest's `for_host` retry classifier matches the actual request host, not the (possibly different) CLI > env > default. With those call sites switched, `resolve_base_url` and `resolve_model` are no longer referenced; deleted them. Kept DEFAULT_BASE_URL / DEFAULT_MODEL under `#[allow(dead_code)]` because the test helpers still reference them. Regression test (describegpt_baseurl_remote_prompt_file_requires_api_key): writes a prompt TOML with `base_url = "https://api.openai.com/v1"`, runs describegpt with neither --api-key nor QSV_LLM_APIKEY set. The new test fails on the pre-fix code (the localhost gate let the run proceed, hitting BLAKE3 hashing) and passes after the fix (the api-key error fires before any work begins). All 73 describegpt tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex review (job 2373, MEDIUM) flagged that my prior fix (job 2372) made the api-key localhost gate consult the prompt file's base_url for correctness — but the gate runs ahead of the --prepare-context branch in run(), so `qsv describegpt --prepare-context --prompt-file remote.toml` now errors out asking for credentials even though that mode only emits prompt/context JSON locally and never calls the LLM API. Guard the gate with `args.flag_prepare_context`: when set, skip the localhost / api-key resolution entirely and use an empty String placeholder for the unused `api_key` variable. --process-response (a similar no-network mode) already returns earlier in run() and is unaffected. Regression test (describegpt_prepare_context_remote_prompt_file_no_api_key): runs --prepare-context with a prompt file pointing at https://api.openai.com/v1 and no API key set — fails on the pre-fix code, succeeds after the fix. All 74 describegpt tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
jqnatividad
deleted the
fix/describegpt-baseurl-precedence-explicit-default
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
[default: ...]from--base-urland--modelsoflag_base_url/flag_modelareSomeonly when the user explicitly passed the flag.resolve_base_url(args)/resolve_model(args)helpers that apply CLI > env > built-in default manually; replaces the sentinel-based comparisons inget_prompt_fileand the api-key localhost check with plainSome/Nonechecks.describegpt_baseurl_precedence_cli_default_over_envthat pins the precedence:QSV_LLM_BASE_URL=fake-env qsv describegpt --base-url http://localhost:1234/v1 ...must honor the explicit CLI value even though it matches the documented default.Why
Codex review (job 2363, MEDIUM) flagged that the sentinel-based precedence couldn't distinguish an omitted
--base-urlfrom an explicit--base-url http://localhost:1234/v1— both producedflag_base_url == Some(DEFAULT_BASE_URL). As a result,QSV_LLM_BASE_URLsilently overrode an explicit-default CLI flag, contradicting the documented precedence (CLI > env > default).Test plan
cargo test -F all_features test_describegpt::— 72/72 passingdescribegpt_baseurl_precedence_cli_over_envanddescribegpt_baseurl_precedence_env_over_defaultstill passqsv --generate-help-md🤖 Generated with Claude Code