fix(model-switch): keep default-profile Anthropic catalog under multiplexing - #112
Closed
exiao wants to merge 43 commits into
Closed
fix(model-switch): keep default-profile Anthropic catalog under multiplexing#112exiao wants to merge 43 commits into
exiao wants to merge 43 commits into
Conversation
/model <alias> for an OpenRouter-backed provider failed under multiplex_profiles with UnscopedSecretError: switch_model -> resolve_runtime_provider reads OPENAI_BASE_URL via get_secret, but the multiplexer only wraps the agent run (not slash-command dispatch) in the per-profile secret scope. Wrap switch_model in _profile_runtime_scope on the slash path. No-op when multiplexing is off. Patch note: ~/.hermes/plans/hermes-patches/model-switch-secret-scope.md
…ed wrapper
The profile-scope fix wraps switch_model in _switch_model_scoped before
offloading to asyncio.to_thread, so the offloaded callable's __name__ is
now the wrapper, not the patched switch_model. Assert the wrapper is
offloaded and that the (single-profile pass-through) switch still runs
('nope' in result), preserving the test's real invariant: the blocking
switch is offloaded to a worker thread, not run on the event loop.
The credential-scope fix wrapped only the switch_model resolver in the profile secret scope, but both /model persist paths (picker _on_model_selected and text _finish_switch) read/wrote config.yaml via the module-level gateway home OUTSIDE the scope. Under multiplex_profiles a secondary profile's plain /model <name> then rewrote the DEFAULT profile's config.yaml and left the requesting profile unchanged (corrupting the active profile). Extract the config read+persist into _persist_switched_model and run it under _profile_runtime_scope for the requesting source, so get_hermes_home() (honored by save_config) and the config read both target the requesting profile. No-op scope when multiplexing is off. Drop the now-unused config_path local in _handle_model_command. Regression test: persist under a source-profile scope writes that profile's config.yaml and leaves the default profile's untouched.
Follow-on to the persist-scope fix: current_provider / current_base_url / user_provs / custom_provs are read from _load_gateway_config() at the top of _handle_model_command and fed into switch_model's resolution. That read used the module-level gateway home, so under multiplex_profiles a secondary profile's /model <name> resolved against the DEFAULT profile's provider/custom-provider map (wrong endpoint, or missing the profile's own provider) even though the resolver and persist paths were already scoped. Wrap the current-config read in _profile_runtime_scope for the requesting source so the whole /model path (read -> resolve -> persist) is consistently profile-scoped. No-op when multiplexing is off. Regression test asserts the read resolves the requesting profile's provider + custom-provider map.
… scope Follow-on: resolve_persist_behavior() reads model.persist_switch_by_default via load_config() -> get_hermes_home(), but persist_global was computed before any profile scope. Under multiplex_profiles a plain /model <name> (no --global/--session) from a secondary profile then applied the DEFAULT profile's persist-by-default decision — e.g. persisting even when the secondary profile set persist_switch_by_default: false. Recompute persist_global inside the same source-profile scope as the current-config read, so the whole /model path (persist-default + resolve + persist) honors the requesting profile. No-op when multiplexing is off. Regression test asserts a profile that opted out stays session-only even when the default profile opts in.
switch_model's user-provider branch resolved `api_key: ${VAR}` / `key_env`
with os.environ.get directly. Under multiplex_profiles the /model wrapper
installs the profile secret scope but intentionally does NOT mutate
os.environ, so a secondary profile switching to a providers: entry read the
DEFAULT profile's env var (or an empty key) instead of its own .env.
Route those key refs through agent.secret_scope.get_secret, which reads the
installed scope (requesting profile's .env) under multiplexing and falls back
to os.environ when no scope is active and multiplexing is off — single-profile
behavior unchanged. Regression test proves the scoped key wins over an
os.environ leak for a key_env user provider.
…le scope Bare /model shows the picker / text list via list_picker_providers / list_authenticated_providers, which were offloaded to a worker thread OUTSIDE the profile scope. Their auth-store / config / credential-pool provider detection resolves get_hermes_home(), so under multiplex_profiles a secondary profile's listing detected the DEFAULT profile's authenticated providers. Route both listing calls through a _list_scoped wrapper that installs _profile_runtime_scope for the requesting source (contextvars propagate into the to_thread worker), so auth.json / config / pool-based detection resolves the requesting profile. No-op when multiplexing is off. Note: the listing's raw provider-env probes (base_url_env_var / AWS / LM Studio / api_key_env_vars read via os.environ) are NOT covered by a home override — those need routing through get_secret in list_authenticated_providers and are tracked as a follow-up. The credential COMMIT paths (switch + persist + user-provider key refs) are fully scoped by the earlier commits.
…pe wrapper The prior fix wraps the offloaded provider-listing (list_authenticated_providers / list_picker_providers) in _list_scoped so it reads creds under the requesting profile's scope. The raw fn still runs on the to_thread worker, but it is now passed as _list_scoped's first positional arg rather than being the direct to_thread target. Update the offload spy to inspect callable positional args too so the invariant (listing runs off the event loop) holds under both the plain and scope-wrapped seams, staying mutation-survivable.
…ugh get_secret Follow-up to #97. The bare /model listing (_list_scoped) installs the profile secret scope via _profile_runtime_scope, but that intentionally does NOT mutate os.environ, so the ~14 raw os.environ.get provider-credential probes in list_authenticated_providers still read the DEFAULT profile's env under multiplexing. A secondary profile's /model could therefore list an env-var provider as available using the default profile's keys (display-only gap; commit dcb3aa9 flagged it as a tracked follow-up). Route those probes (base_url_env_var, AWS_* fast signal, LM_API_KEY/LM_BASE_URL, api_key_env_vars, overlay extra_env_vars, canonical api_key_env_vars, section-3 and section-4 key_env) through agent.secret_scope.get_secret, matching #97's switch_model user-provider fix (cf75474). get_secret reads the installed scope under multiplexing and falls back to os.environ when unscoped + multiplex off, so CLI/TUI single-profile listing is byte-identical. list_picker_providers delegates all probing here, so it is covered transitively. Removed the now-unused function-local import os. Regression: tests/gateway/test_multiplex_credential_isolation.py TestListProvidersEnvProbesUseScope (scoped-detect, no-leak, single-profile parity). RED-before/GREEN-after verified against the section-1 probe.
…ofile Codex P2: under gateway.multiplex_profiles, /model --refresh called clear_provider_models_cache() before any profile scope was installed, while the listing path now reads the requesting profile's provider_models_cache.json under _list_scoped. So for a secondary profile, refresh wiped the DEFAULT profile's cache and then reused the requesting profile's stale entry — the picker/text list did not actually refresh. Clear the cache under the same source-profile scope (_profile_runtime_scope redirects get_hermes_home(), honored by _provider_models_cache_path). No-op when multiplexing is off.
…nto fix/model-switch-list-getsecret # Conflicts: # tests/gateway/test_multiplex_credential_isolation.py
…reds The AWS SDK availability probe in list_authenticated_providers read AWS creds through the profile secret scope (_get_secret). Under multiplexing a secondary profile's scoped AWS creds would then advertise Bedrock as available, but the Bedrock runtime (bedrock_adapter -> boto3.client with no explicit creds) authenticates via boto3's default chain from os.environ, not the scope. So a selected row would fail or run against the default profile's AWS identity. Read os.environ directly here to keep the listing aligned with what the runtime can actually use. (Codex #100.) Also drop redundant str()/or"" wrappers around _get_secret(...) probes flagged by gemini-code-assist.
…profile Follow-up to the #100 listing fix. The bare /model listing marks the openai-api row available from the requesting profile's scoped OPENAI_API_KEY (list_authenticated_providers now reads get_secret), but the downstream discovery path still read os.getenv directly: - provider_model_ids('openai-api') fetched /v1/models with os.getenv('OPENAI_API_KEY')/OPENAI_BASE_URL, so profile B's picker could call OpenAI with profile A's default-env key. - _credential_fingerprint hashed os.environ values, so profile B's disk-cache entry could collide with profile A's cached model list. Route both through agent.secret_scope.get_secret, which reads the installed profile scope under multiplexing and falls back to os.environ when unscoped + multiplex off (single-profile CLI/TUI byte-identical). Regression tests prove the scoped key wins over an os.environ leak for discovery, that the fingerprint differs per scoped key, and that single-profile still reads env.
Follow-on to the openai-api discovery fix. resolve_api_key_provider_credentials read the provider base URL with os.getenv(base_url_env_var), so the scoped /model listing (which now admits a scoped API-key provider like deepseek/stepfun with a scoped *_BASE_URL) then discovered its catalog from the DEFAULT profile's base URL via cached_provider_model_ids -> provider_model_ids -> here, while using the secondary profile's api key. Route the base-URL env resolution through agent.secret_scope.get_secret so it honors the requesting profile's scope, aligned with its api key. get_secret falls back to os.environ when unscoped + multiplex off (single-profile byte-identical). Regression test proves the scoped base URL wins over an os.environ leak, and that single-profile still reads env.
…ve scope Address the P1 Codex finding on #100. When bare /model lists an env-var provider for a multiplexed secondary profile that lacks the key, a scoped miss fell through to load_pool(), whose _seed_from_env auto-seed helper (_get_env_prefer_dotenv) fell back to os.environ.get(key) after the profile .env + scope came up empty — then _upsert_entry + write_credential_pool PERSISTED the default profile's key as an env:<VAR> entry into the SECONDARY profile's auth store. That's a cross-profile credential write, not just a display leak. When a secret scope is installed (multiplexing), drop the raw os.environ fallback in _get_env_prefer_dotenv: get_secret (the scope) is authoritative, so a scoped miss stays a miss instead of leaking + persisting another profile's key. Single-profile (no scope) preserves the os.environ fallback exactly. _seed_from_singletons already reads only .env + scope, so no change there. Regression tests prove os.environ does not seed under an active scope, a scoped key still seeds, and single-profile still reads env.
# Conflicts: # tests/gateway/test_multiplex_credential_isolation.py
Regression tests for the #100 P2: the Copilot catalog fetch must resolve its GitHub token from the active profile secret scope (not the process env / gh auth token fallback) when a scope is active, and still fall back to gh for single-profile CLI/TUI with no scope. Red before: the process token was returned even under an active scope.
…oint + op:// retry Two P2 scope-leak fixes flagged by codex on PR #100: - Bitwarden: fetch_bitwarden_secrets/_run_bws_list now take the profile's scoped env, so a multiplexed secondary profile's BWS_SERVER_URL reaches the bws child and the default profile's process-env BWS_SERVER_URL can no longer leak in. server_url resolves from cfg, then the scoped .env. - secret_scope: build_profile_secret_scope no longer fail-closed-drops a configured op:// ref that the secret-source registry (apply_all) already resolved when the redundant manual op re-fetch transiently fails. Track registry-resolved names and skip them in the fail-closed pass; genuinely unresolved refs and stale plaintext still fail closed. Regression tests cover both paths (red-before/green-after).
…exing Codex P2 on PR #100: _resolve_copilot_raw_token_from_active_scope treated every scoped miss as authoritative, but under multiplexing the DEFAULT profile also gets a scope installed and the process gh credential IS the default profile's own identity. So a default-profile scoped miss must still fall through to gh auth token; only a NAMED profile's miss stays authoritative (so it can never borrow the process/gh token). Detect via get_hermes_home().parent == 'profiles'. Regression tests cover both default (falls back) and named (authoritative) profiles.
Codex follow-up P2 on PR #100: build_profile_secret_scope decided registry resolution by value inequality, so a configured op:// ref that resolves to the SAME plaintext already in .env was NOT marked resolved and could still be fail-closed-dropped on a transient manual-refetch failure. Use apply_all's ApplyReport.provenance (authoritative record of which vars each source applied) instead of comparing values. Added a same-value regression (red-before/green-after).
Codex P2 on PR #100: OnePasswordSource.fetch passed auth_env=os.environ in process-auth mode (apply_all environ=None), so _auth_fingerprint hashed the ENTIRE environment. Unrelated changes (PWD/OLDPWD or any non-OP var) produced a different disk-cache key and forced every op read to re-run, defeating the cross-process cache. Now pass auth_env only for an isolated scoped environment, filtered to the OP auth vars (OP_ACCOUNT/OP_CONNECT_*/OP_SESSION_*); process mode passes none, since _auth_fingerprint already folds the relevant os.environ OP vars. Regression test: unrelated env change still hits the cache.
…-auth apply_all passed the MATERIALIZED env (os.environ when environ=None) into _fetch_with_timeout, which forwards it as environ= to source.fetch(). OnePasswordSource treats any non-None environ as isolated mode (include_process_auth=False), so on the default load_hermes_dotenv path (environ=None) users relying on an interactive 'op' CLI session instead of OP_SERVICE_ACCOUNT_TOKEN ran op with an isolated HOME and lost their signed-in 1Password credentials at startup. Forward the raw None-preserving environ to the fetch; keep the materialized env only for the local merge/precedence phase. Regression tests: default path (environ omitted) forwards None to fetch; scoped path forwards the explicit dict (red before: materialized os.environ).
A profile-scoped apply_all (a named profile under gateway.multiplex_profiles) now refuses a secret source whose fetch() lacks the 'environ' parameter instead of running it env-less. Such a source would read bootstrap credentials from the process os.environ — the DEFAULT profile's env under multiplexing — populating a named profile from another profile's vault. The default single-profile apply path (scoped=False) keeps the legacy env-less call for backward compat. Addresses codex P1 on registry.py.
…rces build_profile_secret_scope runs apply_all with an isolated environ (parsed .env only), so a documented default-profile setup that supplies a source's bootstrap token (e.g. Bitwarden's BWS_ACCESS_TOKEN) from the shell/systemd environment was invisible — the source reported NOT_CONFIGURED and its secrets dropped out of the scope. For the DEFAULT profile only (the same scope that keeps 1Password's include_process_auth shell fallback), overlay each enabled source's protected_env_vars from os.environ when .env didn't define them. Named profiles are never seeded, so they can't borrow the gateway's vault auth. Addresses codex P2 on secret_scope.py.
The prior commit passed scoped=True to apply_all for both the default and named profiles, so a default-profile legacy source whose fetch() lacks the 'environ' parameter was rejected NOT_CONFIGURED — dropping its default-profile vault secrets even though the shell/systemd bootstrap token was just re-seeded. The scoped fail-closed is a NAMED-profile isolation guard (a named profile must not read the gateway's os.environ); the default profile IS the process owner, so its own env is in-scope and legacy sources still run env-less. Pass scoped=is_named_profile. Addresses codex P2 follow-on on secret_scope.py.
build_profile_secret_scope started a named profile's scoped map from only the profile's own .env, but load_hermes_dotenv treats the root ~/.hermes/.env as a shared base layer every profile inherits before its own overrides. Since this PR routes /model provider detection through get_secret() reading this scope, a named profile with an empty/partial .env that relies on a shared root OPENAI_API_KEY/CPE_GITHUB_TOKEN no longer saw it. Load the root .env first (via get_default_hermes_root(home), same as load_hermes_dotenv), then override with the profile's own .env. Default/root home resolves to itself — no double load. Addresses codex P2 (secret_scope.py root-env inheritance).
… listing A named-profile /model listing marked a provider available via PROCESS-GLOBAL credential fallbacks even when its scoped env probe missed: the credential-pool auto-seed (load_pool auto-seeds copilot from gh auth token / COPILOT_GITHUB_TOKEN / GH_TOKEN / GITHUB_TOKEN on the default process) and the anthropic Claude-Code / Hermes-OAuth credential files. Both belong to the DEFAULT profile, so a secondary profile's picker listed providers from another profile's identity. Gate both fallbacks behind (multiplex active AND a scope installed); single-profile CLI/TUI (no scope, multiplex off) keeps every fallback unchanged. Addresses codex P1 follow-on (model_switch.py copilot pool seeding).
The CANONICAL_PROVIDERS cross-check (section 2b) had its own load_pool() fallback that was NOT gated by _scoped_listing, unlike the overlay pass (section 2). A named-profile /model listing could therefore still surface copilot/anthropic from the default process's gh-auth / Claude credential files via load_pool auto-seed, even after the scoped env + auth-store probes correctly left _cp_has_creds false. Gate the canonical pool fallback behind (multiplex active AND scope installed), matching the overlay pass. Single-profile CLI/TUI (no scope, multiplex off) keeps the fallback unchanged. Addresses codex P1 on #100 (model_switch.py:1963 canonical pool seeding). Red/green regression: canonical-pass scoped listing must not borrow the default pool; single-profile canonical listing still detects copilot.
…xing _profile_runtime_scope installs a secret scope for EVERY profile under multiplexing, including the active/default one. So _scoped_listing (which required only 'multiplex active AND scope installed') became true for the default profile too, suppressing its own load_pool() and Anthropic external-file fallbacks — a bare /model listing dropped the default profile's own copilot gh-auth / Claude Code OAuth creds the moment multiplexing was enabled. Restrict _scoped_listing to NAMED profiles only (home under <home>/profiles/<name>, matching the is_named_profile seam in build_profile_secret_scope). The default profile IS the process owner: its gh-auth / Claude-file / pool creds belong to it and stay listed. Addresses codex P2 on #100. Red/green regression: test_default_profile_under_multiplex_keeps_copilot_pool (fails before, passes after); named-profile scoped tests now install a profiles/<name> home so they still assert suppression.
Under gateway.multiplex_profiles, _profile_runtime_scope installs a secret scope for the default profile too, and get_secret becomes authoritative once any scope is present (no os.environ fallback). The default scope only overlaid secret-source bootstrap vars (e.g. BWS_ACCESS_TOKEN), so a default/ process-owner profile whose provider keys (OPENAI_API_KEY, etc.) come from the shell/systemd env rather than .env lost them from /model + runtime resolution the moment multiplexing was enabled. Seed the default profile's scope with its own os.environ as the lowest layer (below .env / sources, which still override), excluding genuinely- global vars (get_secret reads those from os.environ directly) and the 1Password auth/session plumbing (OP_*, handled by the dedicated op path with token_value=None + include_process_auth=True). Named profiles are a hard isolation boundary and get NO os.environ seed — they never borrow the gateway/default identity. Addresses codex P2 on #100. Red/green regression: test_default_profile_scope_keeps_shell_only_provider_key (fails before, passes after) + env-file-overrides-shell + named-profile-does-not-seed.
A named profile's bws secret-source fetch under gateway.multiplex_profiles built the child env from os.environ.copy() and only adjusted BWS_* vars. The gateway process env belongs to the DEFAULT profile, so the bws subprocess resolving a named profile's vault still received the default profile's provider secrets (OPENAI_API_KEY, ANTHROPIC_API_KEY, …) — a cross-profile leak that undermines the per-profile isolation this PR enforces. For the scoped (named-profile) case, build a minimal allowlisted child env (_is_bws_runtime_env: PATH/HOME/TLS-cert/proxy/locale + Windows loader essentials + bws toggles) instead of inheriting the full process env, then overlay only the scoped BWS_* plumbing + access token. bws authenticates solely via BWS_ACCESS_TOKEN, so no provider keys are needed. The single-profile (no-scope) path is unchanged — it still inherits the process env so manual shell/proxy overrides keep working. Addresses codex P2 on #100. Red/green regression: test_fetch_scoped_env_excludes_default_profile_provider_secrets (fails before, passes after) + test_fetch_unscoped_env_still_inherits_process_env.
_resolve_api_key_provider_secret read via get_env_value_prefer_dotenv (dotenv-over-process) whenever current_secret_scope() was None. Under gateway.multiplex_profiles, a call reached OUTSIDE _profile_runtime_scope has no scope installed, so this branch returned the process/default profile's .env key to an unscoped multiplex caller — bypassing get_secret's UnscopedSecretError fail-closed guard. For API-key providers with no base_url_env_var (e.g. kimi-coding-cn) this leaked the default profile's credential. Only take the dotenv-preferred lookup when NOT multiplexing (single-profile CLI/TUI). When multiplexing is active, route through get_secret: it reads the installed scope when present and raises UnscopedSecretError (caught → skip) when unscoped, keeping the read fail-closed. Addresses codex P1 on #100. Red/green regression: test_api_key_provider_fails_closed_when_multiplex_active_but_unscoped (fails before, passes after) + test_api_key_provider_single_profile_still _reads_dotenv (single-profile unchanged).
Catching UnscopedSecretError in the env-var loop only skipped that loop; execution still fell through to the credential-pool fallback, which load_pool()._seed_from_env can seed/read from the default profile (it treats current_secret_scope() is None as the single-profile path and prefers the raw .env value), returning a default-profile key as credential_pool:<provider> despite the fail-closed guard. Return no credential BEFORE the pool fallback when multiplexing is active and no scope is installed. Single-profile and scoped paths reach the pool unchanged. Addresses codex P1 follow-on on #100. Red/green regression: test_api_key_provider_unscoped_multiplex_skips_credential_pool (fails before, passes after).
…URL read Two codex findings on #100, both on hermes_cli/auth.py, both cross-profile leaks on the unscoped multiplex path (gateway.multiplex_profiles active, reached outside a profile secret scope): - P1: _resolve_copilot_raw_token fell through to resolve_copilot_token() (process env / gh auth token = default profile identity) when the scoped helper returned None. Now fails closed (returns '','') under unscoped multiplex, mirroring the generic API-key path. Single-profile (multiplex off) still uses the gh fallback. - P2: resolve_api_key_provider_credentials read the base URL via get_secret(), which raises UnscopedSecretError under unscoped multiplex, crashing instead of failing closed. Now returns empty base_url (registry default) on that path. Red-before/green-after regression tests added for both.
Codex P2 on #100. _fetch_with_timeout runs each source.fetch() in a plain ThreadPoolExecutor worker, which does NOT inherit the caller's ContextVars. The profile scope installed by _profile_runtime_scope sets the HERMES_HOME override as a contextvar, so a source that consults get_hermes_home() internally resolved the DEFAULT profile's path in the worker thread. Concretely Bitwarden's fetch() calls find_bws() -> _hermes_bin_dir() -> get_hermes_home()/bin, so a named profile with its own profiles/<name>/bin/bws (or auto_install:false) would miss its binary and use/install the default profile's copy. Fix: submit each fetch via contextvars.copy_context().run(...) so the worker inherits the HERMES_HOME override AND the active secret scope, mirroring the copy_context() pattern already used in agent/account_usage.py. Red-before/green-after regression: TestFetchInheritsProfileContext asserts the fetch worker sees the caller's HERMES_HOME override.
…plexing Follow-up to #100. Under gateway.multiplex_profiles every profile — including the default/process-owner one — runs inside a secret scope, so _fetch_anthropic_models saw active_scope is not None for the DEFAULT profile and never fell back to resolve_anthropic_token(). An Anthropic identity that lives only in ~/.claude.json / ~/.claude/.credentials.json (Claude Code login) is not carried in the scope's env vars or auth.json pool, so a bare /model dropped the default profile's live catalog to the static list the moment multiplexing turned on — the discovery-only regression the #100 review flagged. Mirror the named-vs-default seam #100 gave the Copilot path: for the DEFAULT profile only, fall back to resolve_anthropic_token() when the scoped lookup is empty. Named profiles (<home>/profiles/<name>) stay fail-closed so they can't attribute the gateway's Claude-file identity to a secondary profile. Ollama needs no change: it authenticates only via OLLAMA_API_KEY/OLLAMA_BASE_URL env vars, which #100's build_profile_secret_scope already seeds into the default scope from the process env (verified: default scope carries the shell OLLAMA key; named scope does not). Adding a fallback there would be dead code. Regression: rename the borrow-guard test to a NAMED profile (its contract only holds there) and add a default-profile test asserting the Claude-file catalog survives. Red-before/green-after on the source line.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a fallback to Claude-file auto-discovery for the default profile under multiplexing, while ensuring named profiles remain fail-closed. It adds a helper function _is_named_profile_scope to distinguish between default and named profiles based on the HERMES_HOME directory structure, along with corresponding unit tests. The review feedback suggests absolutizing the path in _is_named_profile_scope using os.path.abspath to prevent potential profile isolation bypasses when relative paths are used.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b5eb684c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…llback on explicit key Address review findings on #112: - _is_named_profile_scope now absolutizes HERMES_HOME via os.path.abspath (not Path.resolve, to avoid following symlinks) so a relative HERMES_HOME can't bypass the named-profile check (gemini-code-assist). - _fetch_anthropic_models skips all scoped-token resolution — including the default-profile resolve_anthropic_token() Claude-file fallback — when an explicit api_key is supplied, since api_key always wins (codex P2). - Add regression test asserting an explicit api_key wins and never calls resolve_anthropic_token().
Owner
Author
|
Both review findings addressed in 2bb147a (threads resolved):
Required checks green ("All required checks pass" = SUCCESS, all 8 test slices pass). The remaining |
Owner
Author
Local Codex/Claude-persona review — PR #112Review bots' verdicts are already addressed; the failing Verdict: APPROVE — 0 P0, 0 P1, 0 P2, 1 P3 (out of scope). Rubric
Tests run (real counts)All 20 required CI test/lint/supply-chain checks are green; only the API-keyless P3 (non-blocking, out of this PR's scope — follow-up)The canonical named-vs-default seams this PR mirrors — (Stand-in review: the repo's |
Owner
Author
exiao
added a commit
that referenced
this pull request
Jul 15, 2026
…plexing (#115) * fix(model-switch): keep default-profile Anthropic catalog under multiplexing Follow-up to #100. Under gateway.multiplex_profiles the default profile lost its live Anthropic /model catalog on a bare /model, because _fetch_anthropic_models did not mirror the named-vs-default scoped-token fallback #100 gave the Copilot path. Resolve the Anthropic pool token through the active scope, preserving the default/process-owner profile's own credentials while keeping named profiles strictly scoped. Rebased onto live-config: #100 squash-merged, so the original PR #112 branch carried #100's now-orphaned commits and showed unrelated histories. This is the clean 2-file delta (models.py + its test) applied on the merged base. * refactor(models): use module-level os in _is_named_profile_scope Drop the redundant local 'import os as _os' in _is_named_profile_scope; os is already imported at module level (line 11). Addresses Gemini review nit.
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
1 participant
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.
What
Follow-up to #100 (the Codex/claude-review "0 blocking, 1 minor — worth a follow-up" note). Under
gateway.multiplex_profilesthe default profile loses its live Anthropic/modelcatalog on a bare/model, because_fetch_anthropic_modelsdoesn't mirror the named-vs-default fallback #100 gave the Copilot path.Root cause
_profile_runtime_scopeinstalls a secret scope for every profile under multiplexing — including the default/process-owner one — soactive_scope is not Noneis true for the default profile too._fetch_anthropic_modelstherefore took the scoped-only branch and never calledresolve_anthropic_token(). An Anthropic identity that lives only in~/.claude.json/~/.claude/.credentials.json(Claude Code login) is not carried in the scope's env vars or theauth.jsonpool, so the scoped lookup came back empty →None→ static fallback catalog. Discovery-only degradation, no security/correctness impact.Fix
Mirror the named-vs-default seam from #100 (
model_switch._is_named_profile_scope/secret_scope.build_profile_secret_scope): for the default profile only, fall back toresolve_anthropic_token()(Claude-file auto-discovery) when the scoped token is empty. Named profiles (<home>/profiles/<name>) stay fail-closed — reading the process Claude file would attribute the gateway's identity to a secondary profile.Why Ollama needs no change (task also named
fetch_ollama_cloud_models)Ollama authenticates only via
OLLAMA_API_KEY/OLLAMA_BASE_URLenv vars — it has no file-based or process-global credential path. #100'sbuild_profile_secret_scopealready seeds those from the process env into the default scope, so the default profile's Ollama catalog already survives. Verified empirically: the default scope carries the shellOLLAMA_API_KEY; a named scope does not. Adding a fallback there would be dead code (rubric: no dead code without a real path), so it's documented instead.Tests
tests/hermes_cli/test_models_secret_scope.py:test_anthropic_catalog_does_not_borrow_process_credentials→test_named_profile_catalog_does_not_borrow_process_credentialsand pinned it to a named profile home — its fail-closed contract only holds there (the old test implicitly ran as the default profile, where the new fallback legitimately fires).test_default_profile_catalog_keeps_claude_file_fallback— asserts the default profile recovers its live catalog from the Claude-file token (routes viaBearer, proving the token flowed).Red-before/green-after verified on the single source line (revert the fallback → the default-profile test fails; restore → passes). Named-profile test stays green both ways.
Stacking
Branched off
fix/model-switch-list-getsecret(#100), which is not yet merged. GitHub will auto-retarget this PR tomainwhen #100 merges.