fix(copilot): respect COPILOT_API_BASE_URL and fix follow-up failing check regressions#664
fix(copilot): respect COPILOT_API_BASE_URL and fix follow-up failing check regressions#664badMade wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures that Copilot base URL overrides (such as COPILOT_API_BASE_URL) are correctly respected during credential pool seeding and runtime provider resolution, and updates Copilot token resolution logic. The review feedback suggests simplifying the environment variable resolution in both agent/credential_pool.py and hermes_cli/runtime_provider.py by removing redundant os.getenv fallbacks and preferring .env file lookups during pool seeding to prevent persisting stale inherited environment variables.
| if pconfig and pconfig.base_url_env_var: | ||
| env_url = ( | ||
| get_env_value(pconfig.base_url_env_var) | ||
| or os.getenv(pconfig.base_url_env_var, "") | ||
| ).strip().rstrip("/") |
There was a problem hiding this comment.
During pool seeding, we should prefer the .env file over os.environ to prevent stale inherited environment variables from being persisted into auth.json (consistent with the fix for NousResearch#18254). Additionally, get_env_value already falls back to .env if not in os.environ, making the or os.getenv(...) redundant.
We can use load_env() (which is already imported) to check the .env file first.
| if pconfig and pconfig.base_url_env_var: | |
| env_url = ( | |
| get_env_value(pconfig.base_url_env_var) | |
| or os.getenv(pconfig.base_url_env_var, "") | |
| ).strip().rstrip("/") | |
| if pconfig and pconfig.base_url_env_var: | |
| env_file = load_env() | |
| env_url = ( | |
| env_file.get(pconfig.base_url_env_var) | |
| or os.getenv(pconfig.base_url_env_var, "") | |
| ).strip().rstrip("/") |
| if pconfig.base_url_env_var: | ||
| env_url = ( | ||
| get_env_value(pconfig.base_url_env_var) | ||
| or os.getenv(pconfig.base_url_env_var, "") | ||
| ).strip().rstrip("/") |
There was a problem hiding this comment.
There was a problem hiding this comment.
@gemini-code-assist, is this suggestion still valid? If so, what are the steps and line numbers to make this fix?
There was a problem hiding this comment.
Yes, the suggestion remains valid. get_env_value is designed to handle the environment lookup internally, making the explicit os.getenv fallback redundant. You can apply this simplification to hermes_cli/runtime_provider.py at lines 231-235.
| if pconfig.base_url_env_var: | |
| env_url = ( | |
| get_env_value(pconfig.base_url_env_var) | |
| or os.getenv(pconfig.base_url_env_var, "") | |
| ).strip().rstrip("/") | |
| if pconfig.base_url_env_var: | |
| env_url = (get_env_value(pconfig.base_url_env_var) or "").strip().rstrip("/") |
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
3 |
unresolved-attribute |
1 |
First entries
run_agent.py:13568: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:7311: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
gateway/platforms/qqbot/adapter.py:1142: [unresolved-attribute] unresolved-attribute: Object of type `Self@_handle_c2c_message` has no attribute `gateway_runner`
run_agent.py:13565: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
✅ Fixed issues (4):
| Rule | Count |
|---|---|
invalid-argument-type |
3 |
unresolved-reference |
1 |
First entries
run_agent.py:13568: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:7311: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
gateway/run.py:5544: [unresolved-reference] unresolved-reference: Name `team_id` used when not defined
run_agent.py:13565: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
Unchanged: 4354 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
There was a problem hiding this comment.
Pull request overview
This PR ensures operator-configured Copilot base URL overrides (COPILOT_API_BASE_URL) are respected consistently when Copilot credentials are sourced from GH_TOKEN / other env vars or from gh auth token, both during credential-pool seeding and at runtime provider resolution.
Changes:
- Update Copilot credential-pool seeding to classify token sources more precisely (
gh_clivsenv:…), applyCOPILOT_API_BASE_URLwhen creating pool entries, and store the exchanged Copilot API token for env-seeded entries. - Update runtime provider resolution for Copilot to prefer (when the pool base URL is empty/default)
model.base_url(if configured provider is Copilot), thenCOPILOT_API_BASE_URL, then the provider default. - Add regression tests for GH_TOKEN/env seeding + base URL override behavior and runtime resolution override precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
agent/credential_pool.py |
Adjusts Copilot seeding source classification and base URL selection; exchanges env Copilot tokens before storing in the pool entry. |
hermes_cli/runtime_provider.py |
Adds Copilot-specific base URL override precedence using config/env/provider-default rules when pool URL is empty/default. |
tests/agent/test_credential_pool.py |
Adds tests for GH_TOKEN and gh auth token pool seeding with COPILOT_API_BASE_URL and API-token exchange. |
tests/hermes_cli/test_runtime_provider_resolution.py |
Adds tests asserting Copilot runtime resolution respects env/config base URL overrides when pool URL is default. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| env_url = "" | ||
| if pconfig and pconfig.base_url_env_var: | ||
| env_url = ( | ||
| get_env_value(pconfig.base_url_env_var) | ||
| or os.getenv(pconfig.base_url_env_var, "") | ||
| ).strip().rstrip("/") | ||
| base_url = env_url or (pconfig.inference_base_url if pconfig else "") |
| monkeypatch.setenv("COPILOT_API_BASE_URL", "https://enterprise-proxy.example/copilot/") | ||
| monkeypatch.setattr(rp, "resolve_provider", lambda *a, **k: "copilot") | ||
| monkeypatch.setattr(rp, "load_pool", lambda provider: _Pool()) | ||
| monkeypatch.setattr(rp, "_copilot_runtime_api_mode", lambda *a, **k: "chat_completions") | ||
|
|
| if pconfig.base_url_env_var: | ||
| env_url = ( | ||
| get_env_value(pconfig.base_url_env_var) | ||
| or os.getenv(pconfig.base_url_env_var, "") | ||
| ).strip().rstrip("/") |
|
@claude code review |
|
@jules code review |
Auto-merge: review received — action requiredReview activity detected (6 inline comment(s) by non-author(s); 2 comment(s)/review(s) from review bot(s): gemini-code-assist[bot], copilot-pull-request-reviewer[bot]). To confirm you have reviewed and accepted the feedback, please add the |
|
@copilot, fix all failing checks. |
Auto-merge: checks failingThe following checks did not pass:
Please fix the failing checks before this PR can be merged. |
Implemented fixes for the failing checks in commit |
Motivation
COPILOT_API_BASE_URLfrom being bypassed when Copilot credentials come fromGH_TOKENorgh auth tokensources.Description
agent/credential_pool.pynow classifiesgh auth tokenexplicitly asgh_cliand treatsGH_TOKEN(and other env variants) asenv:…; it reads the provider'sbase_url_env_varand applies that value before falling back to the providerinference_base_urlwhen creating pool entries.agent/credential_pool.pyusesget_copilot_api_token()for Copilot env tokens and stores the resolved API token in the pool entry'saccess_token.hermes_cli/runtime_provider.pynow treats Copilot specially: when a pool entry'sbase_urlis empty or equals the provider default, it first prefersmodel.base_url(if the configured provider matches), then theCOPILOT_API_BASE_URLenv override (viaget_env_value), and only then the provider default.gh auth tokenseeding, and runtime resolution override behavior intests/agent/test_credential_pool.pyandtests/hermes_cli/test_runtime_provider_resolution.py.gateway/run.py: initialize the missingteam_idscope in_is_user_authorizedto eliminateNameErrorin authorization paths.gateway/platforms/webhook.py: reject empty/unresolved placeholder webhook secrets during signature validation.gateway/platforms/qqbot/adapter.py: avoid attachment processing for unauthorized QQ C2C traffic.tools/process_registry.py: harden stdin guard handling for empty/malformed approval responses.Testing
tests/agent/test_credential_pool.py(new Copilot env/gh_cli cases) andtests/hermes_cli/test_runtime_provider_resolution.py(Copilot base_url override cases).scripts/run_tests.shfor:feishu,discord,wecom_callback, unauthorized DM behavior, pre-dispatch, signal)ruff checkon modified modules (passed).Codex Task