Skip to content

fix(model-switch): route list_authenticated_providers env probes through get_secret (multiplex) - #100

Merged
exiao merged 42 commits into
live-configfrom
fix/model-switch-list-getsecret
Jul 15, 2026
Merged

fix(model-switch): route list_authenticated_providers env probes through get_secret (multiplex)#100
exiao merged 42 commits into
live-configfrom
fix/model-switch-list-getsecret

Conversation

@exiao

@exiao exiao commented Jul 12, 2026

Copy link
Copy Markdown
Owner

What

Follow-up to #97. Routes list_authenticated_providers' raw
os.environ.get(...) provider-credential probes through
agent.secret_scope.get_secret, closing the display-only multiplexing gap that
#97's commit dcb3aa917 explicitly flagged as a tracked follow-up.

Why

/model (bare) shows the provider picker / text list via
list_picker_providers / list_authenticated_providers. #97 wrapped those calls
in _list_scoped, which installs _profile_runtime_scope for the requesting
profile — correctly scoping the auth.json / config / credential-pool detection
(they resolve get_hermes_home()). But _profile_runtime_scope intentionally
does not mutate os.environ, so the ~14 raw env-var probes inside
list_authenticated_providers still read the DEFAULT profile's process env under
gateway.multiplex_profiles. A secondary profile's bare /model could therefore
list an env-var-based provider as available using the default profile's keys.

The credential commit paths (switch resolution, config persist,
persist-default, user-provider ${VAR}/key_env) were already fully scoped in
#97; this was only the availability display.

Change

Convert the provider-credential env reads to get_secret (aliased
_get_secret), matching #97's switch_model user-provider fix (cf7547491):

  • base_url_env_var (_record_builtin_endpoint)
  • AWS_* fast signal (_has_fast_aws_sdk_signal)
  • LM_API_KEY / LM_BASE_URL (LM Studio block)
  • api_key_env_vars (section 1), overlay extra_env_vars + api_key_env_vars
    (section 2), canonical api_key_env_vars (section 2b)
  • key_env (section 3 user-providers, section 4 custom-providers)

get_secret reads the installed profile scope under multiplexing and falls back
to os.environ when no scope is active and multiplexing is off — so CLI/TUI
single-profile listing is byte-identical
and no UnscopedSecretError fires in
unscoped multiplex-inactive contexts. list_picker_providers delegates all
credential probing to list_authenticated_providers, so it is covered
transitively (no direct probes of its own). Removed the now-unused function-local
import os.

Out of scope (unchanged): the credential-pool auto-seed detection used by the
overlay/canonical sections resolves through agent.credential_pool
(_seed_from_env), a separate path; this PR only covers the direct env-var
probes named in the task.

Tests

tests/gateway/test_multiplex_credential_isolation.py::TestListProvidersEnvProbesUseScope
(real list_authenticated_providers, section-1 direct probe isolated by mocking
overlays/canonical empty):

  • test_scoped_profile_env_provider_detected_no_environ_leak — profile B's
    scoped DEEPSEEK_API_KEY lists deepseek even with no key in os.environ.
  • test_default_profile_environ_not_leaked_to_scoped_profileos.environ
    carries the default profile's DEEPSEEK_API_KEY, but a profile B whose scope
    lacks it does not list deepseek (the leak this closes).
  • test_single_profile_reads_environ_unchanged — multiplex off, no scope: the
    env-var provider still lists (CLI/TUI parity).

RED-before/GREEN-after verified (revert the section-1 probe → both scope tests
fail; restore → pass). Regression run: 149 model-switch/gateway/secret-scope +
126 picker/catalog tests all green.

Acceptance

  • Under multiplexing, a secondary profile's bare /model lists only its own
    env-var providers, not the default profile's.
  • CLI/TUI single-profile listing unchanged.
  • Regression test proving profile-B env-var provider detection.

Stacked on #97 (fix/model-switch-secret-scope); GitHub auto-retargets to
live-config when #97 merges.

exiao added 10 commits July 12, 2026 16:11
/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.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates hermes_cli/model_switch.py to resolve provider-credential environment variables through the profile secret scope using _get_secret instead of directly accessing os.environ. This ensures proper credential isolation under gateway profile multiplexing, and a comprehensive test suite has been added to verify this behavior. The review feedback highlights several opportunities to clean up the code by removing redundant str() type coercions and unnecessary or "" fallbacks where _get_secret already guarantees a string return value.

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.

Comment thread hermes_cli/model_switch.py Outdated
Comment thread hermes_cli/model_switch.py Outdated
Comment thread hermes_cli/model_switch.py Outdated
Comment thread hermes_cli/model_switch.py Outdated
Comment thread hermes_cli/model_switch.py Outdated
Comment thread hermes_cli/model_switch.py Outdated
…nto fix/model-switch-list-getsecret

# Conflicts:
#	tests/gateway/test_multiplex_credential_isolation.py

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1a31b92a68

ℹ️ 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".

Comment thread hermes_cli/model_switch.py Outdated
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown

Claude encountered an error after 2s —— View job


I'll analyze this and get back to you.

…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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b1410be326

ℹ️ 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".

Comment thread hermes_cli/model_switch.py
@exiao

exiao commented Jul 13, 2026

Copy link
Copy Markdown
Owner Author

🤖 Babysitter: merge-ready, deferring these nits.

This PR is green, mergeable, and has no blocking review threads. The only open items are non-blocking follow-ups, so no fix is being pushed here and no worker is spawned. Tracking them so they're not silently dropped:

In a multiplexed gateway where the requesting profile has its

Address in a follow-up PR if worth it, or merge as-is and let these close with the merge.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

…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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2a840c04b6

ℹ️ 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".

Comment thread hermes_cli/model_switch.py
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.
@exiao

exiao commented Jul 13, 2026

Copy link
Copy Markdown
Owner Author

🤖 Babysitter: merge-ready, deferring these nits.

This PR is green, mergeable, and has no blocking review threads. The only open items are non-blocking follow-ups, so no fix is being pushed here and no worker is spawned. Tracking them so they're not silently dropped:

For a multiplexed secondary profile that has a scoped AP

Address in a follow-up PR if worth it, or merge as-is and let these close with the merge.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 115a060c48

ℹ️ 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".

Comment thread hermes_cli/model_switch.py
exiao added 2 commits July 13, 2026 12:21
…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.
@exiao

exiao commented Jul 13, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d417553648

ℹ️ 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".

Comment thread agent/credential_pool.py
@exiao

exiao commented Jul 13, 2026

Copy link
Copy Markdown
Owner Author

🤖 Babysitter: merge-ready, deferring these nits.

This PR is green, mergeable, and has no blocking review threads. The only open items are non-blocking follow-ups, so no fix is being pushed here and no worker is spawned. Tracking them so they're not silently dropped:

When a multiplexed profile stores a direct KEY=op://... i

Address in a follow-up PR if worth it, or merge as-is and let these close with the merge.

@chatgpt-codex-connector

Copy link
Copy Markdown

To use Codex here, create an environment for this repo.

@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Fixed the P2 (default-profile shell provider keys) on head 2e515b9 — build_profile_secret_scope now seeds the default/process-owner profile's scope with its own os.environ as the lowest layer (below .env/sources), excluding genuinely-global vars and OP_* auth plumbing. Named profiles get no os.environ seed. Red/green regression added.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e515b9984

ℹ️ 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".

Comment thread agent/secret_sources/bitwarden.py
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.
@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Fixed the P2 (scoped Bitwarden fetch inheriting default-profile env) on head c11ac51 — the named-profile bws child now starts from a minimal OS/runtime allowlist instead of os.environ.copy(), so it no longer carries the default profile's provider secrets while resolving another profile's vault. Single-profile path unchanged. Red/green regression added.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c11ac51c56

ℹ️ 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".

Comment thread hermes_cli/auth.py Outdated
_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).
@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Fixed the P1 (unscoped multiplex API-key resolution leaking default-profile .env) on head 4bf3760 — _resolve_api_key_provider_secret now only takes the dotenv-preferred read when NOT multiplexing; under multiplexing it routes through get_secret and fails closed (UnscopedSecretError) when no scope is installed. Single-profile path unchanged. Red/green regression added.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4bf376033d

ℹ️ 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".

Comment thread hermes_cli/auth.py
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).
@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Fixed the P1 follow-on (pool fallback still reached on unscoped multiplex) on head 7718f22 — _resolve_api_key_provider_secret now returns no credential before the credential-pool fallback when multiplexing is active and no scope is installed. Single-profile/scoped paths unchanged. Red/green regression added.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7718f22081

ℹ️ 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".

Comment thread hermes_cli/auth.py
Comment thread hermes_cli/auth.py Outdated
…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.
@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Head 0d2beca fixes both prior findings on the unscoped multiplex path (both cross-profile leaks):

  • P1: _resolve_copilot_raw_token now fails closed (returns '','') instead of falling through to the process/gh token when current_secret_scope() is None and is_multiplex_active().
  • P2: resolve_api_key_provider_credentials no longer calls get_secret() for the base URL on that path (which raised UnscopedSecretError); it resolves the registry-default base URL instead.

Red-before/green-after regression tests added.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0d2beca482

ℹ️ 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".

Comment thread agent/secret_scope.py
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.
@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

@codex review

Head 2717a2b fixes the profile-context propagation P2: _fetch_with_timeout now submits each source.fetch() via contextvars.copy_context().run(...), so the ThreadPoolExecutor worker inherits the HERMES_HOME override + secret scope installed by _profile_runtime_scope. This fixes Bitwarden's find_bws resolving the default profile's bin path in the worker thread. Red-before/green-after regression added; full secret suite (181 tests) green.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 2717a2bee2

ℹ️ 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".

@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Babysit check (head 2717a2b): PR is merge-ready — OPEN, MERGEABLE, APPROVED, all real CI green (tests 8/8, lints, security, check-author, "All required checks pass"), 36 review threads all resolved.

The only red check is claude-review, which crashed with a Claude Code Action infra error (result reported subtype success with is_error:true — run did not complete successfully) — it errored while replaying the PR's already-resolved review history, before submitting a fresh review. Reran the failed job once; it crashed identically (persistent action-side infra flake, not a code finding). The base branch live-config is unprotected and this check is non-gating, so it does not block merge.

@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Local Codex-persona review (review bots budget-exhausted)

Ran the repo's review rubric locally against live HEAD 2717a2be in an isolated pull/100/head worktree off NousResearch/hermes-agent, verified against the real test suite via scripts/run_tests.sh (CI parity). Reviewer != producer.

Verdict: PASS — no blocking issues.

What I checked

Traced every changed credential-resolution site and reproduced the single-profile-unchanged invariant plus the multiplex isolation invariants against real tests (no mocks for the scope-build path).

  • agent/secret_scope.py::build_profile_secret_scope — os.environ seed for the default/process-owner scope (lowest layer), root .env inheritance for named profiles, secret-source resolution into the isolated mapping, op:// resolution at build time, fail-closed pop of unresolved refs.
  • hermes_cli/model_switch.py::list_authenticated_providersos.environ.getget_secret at every probe; _scoped_listing correctly restricted to NAMED profiles (get_hermes_home().parent.name == "profiles"), matching the is_named_profile seam in secret_scope.py.
  • hermes_cli/auth.py_resolve_api_key_provider_secret keeps the get_env_value_prefer_dotenv dotenv-over-process preference on the single-profile path (current_secret_scope() is None and not is_multiplex_active()), routes through get_secret with UnscopedSecretError fail-closed otherwise. Copilot gh-fallback preserved for default profile, authoritative for named.
  • hermes_cli/models.py — openai discovery, _credential_fingerprint, _fetch_anthropic_models (scoped-pool token helper avoids resolve_anthropic_token() = default identity when scoped).
  • agent/credential_pool.py::_get_env_prefer_dotenv — unscoped op:// and non-op:// return paths are behaviorally equivalent to the pre-PR logic; scoped path drops the raw os.environ leak.
  • agent/secret_sources/{base,registry,bitwarden,onepassword}.py — new environ/scoped fetch contract, contextvars.copy_context() for pool-thread HERMES_HOME propagation, legacy env-less source fail-closed for named profiles only, bws runtime-env allowlist, op isolated-home + auth-fingerprint changes.

Correctness / regressions (highest weight): clean

  • Single-profile (multiplex off, no scope) is byte-identical. get_secret falls back to os.environ; verified by test_single_profile_reads_environ_unchanged, test_single_profile_still_lists_copilot_from_pool, test_discovery_single_profile_reads_environ, test_base_url_single_profile_reads_environ, and 183 pre-existing model-switch/models tests passing unchanged.
  • Named-profile isolation holds (no borrow of default process env / gh pool / anthropic Claude files / bws vault) — test_named_profile_does_not_seed_process_env, test_scoped_profile_does_not_borrow_default_copilot_pool, test_scoped_profile_canonical_pass_does_not_borrow_default_pool, test_named_profile_does_not_borrow_shell_bitwarden_bootstrap.
  • Default profile under multiplex keeps its own credstest_default_profile_under_multiplex_keeps_copilot_pool, test_default_profile_scope_keeps_shell_only_provider_key, .env-over-shell precedence via test_default_profile_env_file_overrides_shell.

Tests run: tests/agent/test_secret_scope.py, tests/secret_sources/test_secret_source_registry.py, tests/test_bitwarden_secrets.py, tests/test_onepassword_secrets.py (170 pass); tests/hermes_cli/test_auth_secret_scope.py, tests/hermes_cli/test_models_secret_scope.py, tests/gateway/test_multiplex_credential_isolation.py, tests/tools/test_credential_pool_env_fallback.py (71 pass); 5 pre-existing model-switch/models suites (183 pass). 424 passing, 0 failing. ruff check clean on all 9 changed source files.

Data integrity / security: clean

Fail-closed discipline is consistent: unscoped multiplex reads raise UnscopedSecretError and every live call site catches it (or the path is provably always scoped). op:// refs are resolved-or-dropped so a reference string can never be handed out as a key. The 1Password auth fingerprint no longer folds the whole os.environ into the cache key (correctly narrowed to auth material + auth_mode).

Non-blocking

P3-1 — inconsistent exception guard in fetch_ollama_cloud_models (hermes_cli/models.py:3792-3803). The two get_secret reads are wrapped in except ImportError, but under multiplex-active-with-no-scope get_secret raises UnscopedSecretError, not ImportError. Everywhere else in this PR that edge is caught as UnscopedSecretError. Not a live defect: list_authenticated_providers (the only caller in the listing path) always runs inside _profile_runtime_scope, so a scope is installed and get_secret never raises there. Worth aligning the guard to except (ImportError, UnscopedSecretError) for consistency with the rest of the file.

P3-2 — scope note (reviewer awareness, not a defect). This PR is materially larger than its title suggests: beyond routing list_authenticated_providers through get_secret, it reworks build_profile_secret_scope (process-env seeding, root inheritance, secret-source + 1Password resolution at build time) and adds an environ/scoped contract to the secret-source ABC + Bitwarden/1Password. Traced through, all of it is a genuine prerequisite for the titled fix to not regress (a scope holding only raw .env would surface op:// literals, drop shared root + shell-exported + vault secrets, and mis-list providers). The 41-commit history is a disciplined fix(...) series, several commits explicitly closing prior Codex P1/P2 findings on #100 that the new tests now lock in as regressions. Coherent, not scope creep — flagging only so the merge is a conscious "yes, this is the whole isolation rework," not a rubber-stamp of a one-line title.

0 P0, 0 P1, 0 P2, 2 P3 — top issue: fetch_ollama_cloud_models guards get_secret with except ImportError where UnscopedSecretError is the real edge (harmless: the listing path is always scoped).

@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Merge-ready (babysitter pass). Live head 2717a2bee2:

  • Required CI: all green — the "All required checks pass" gate is SUCCESS; every Python test slice, lints, supply-chain, and attribution checks pass.
  • Review: APPROVED (latest claude review at head); Codex "no major issues" on 2717a2b; 0 unresolved review threads (36/36 resolved).
  • The only red mark is the non-required claude-review job, which is a Claude Code Action infra flake: it delivers its full "LGTM — 0 blocking" review and then the action wrapper exits with is_error:true. Reran once — reproduced identically, so it's infra, not a code finding. It is not a required check and does not gate merge.
  • The reviewer's one minor, non-blocking note (_fetch_anthropic_models / fetch_ollama_cloud_models lack the Copilot default-profile fallback under multiplexing — discovery-only degradation) is out of scope for this PR's get_secret conversion; tracked as a follow-up.

No code change needed. Ready to merge.

@exiao

exiao commented Jul 14, 2026

Copy link
Copy Markdown
Owner Author

Local independent review — PR #100 (fix/model-switch-list-getsecret)

Reviewed at live HEAD 2717a2be against base ee116841 (17 files, +2645/-73) in an isolated pull/100/head worktree. Ran the repo's own test runner (scripts/run_tests.sh) on every changed test file. This is an independent second pass, not a bot echo.

Verdict: APPROVE (comment-only; I do not merge). 0 P0 / 0 P1 / 0 P2, 1 P3.

Premise — verified

The change routes list_authenticated_providers' raw os.environ.get(...) provider-credential probes through agent.secret_scope.get_secret, and (organically, across the review rounds) closes the whole class of cross-profile credential leaks in the bare-/model listing + live-discovery paths under gateway.multiplex_profiles: OpenAI/API-key discovery, scoped base-URL resolution, credential-pool env auto-seeding, Anthropic/Ollama/Copilot catalog fetches, Bitwarden child-env isolation, 1Password named-profile auth isolation, and secret-source registry population of the isolated scope. The get_secret fail-closed contract (secret_scope.py:123-160) is the linchpin and it behaves exactly as claimed: global vars → os.environ; scope installed → scope-authoritative, no os.environ fallthrough; unscoped + multiplex-active → UnscopedSecretError; unscoped + multiplex-off → os.environ (byte-identical single-profile parity).

Correctness spot-checks (independently traced)

  • _seed_from_env._get_env_prefer_dotenv (credential_pool.py:2145-2173): a scoped miss (empty {} scope is non-None) returns "" with no os.environ fallback — the exact P1 leak fix. Confirmed current_secret_scope() returns the mapping for an empty scope, so scoped=True.
  • _resolve_api_key_provider_secret (auth.py:680-701): fails closed before the credential-pool fallback on the unscoped-multiplex path, and catches UnscopedSecretError on the env-var loop. This is what prevents _seed_from_env's bare _get_secret(key, "") (line 2173) from ever crashing /model in the multiplex case — that line only runs single-profile, where it can't raise. Good.
  • Named-vs-default asymmetry is consistent: default profile keeps its process/gh/shell identity (it is the process owner), named profiles are a hard isolation boundary (no os.environ seed, include_process_auth=False, minimal bws allowlist).
  • ThreadPoolExecutor fetch workers inherit the HERMES_HOME override + secret scope via copy_context() (registry.py) — regression test proves the worker sees the profile home, not the default.

Tests

  • Local, CI-parity runner: 241 passed, 0 failed across the 8 changed test files (163 in credential_pool/registry/bitwarden/onepassword + 78 in secret_scope/multiplex_isolation/auth/models).
  • CI: all 8 Run tests slice jobs + e2e + ruff/ty green. The only red check is the claude-review GitHub Action (bot-infra failure, 53s) — claude itself posted APPROVED reviews on the last 6 commits, so this is not a test signal.
  • Test quality: assertions are scope-vs-environ invariants (does the child env carry the scoped key / drop the process key), not change-detector snapshots of catalog data. Matches the repo's testing policy.

Scope / plan

The PR description frames this as a small follow-up to #97; the actual diff is 2645 LOC across the whole secret-source scope-building machinery. That undersells the size — but every hunk traces to a real cross-profile leak Codex surfaced during review (all 36 bot findings are replied-to and fixed), and there are no drive-by refactors, renames, or unrelated behavior changes. This is the "fix the whole bug class, sibling paths included" pattern the contribution rubric explicitly wants, not scope creep. Prompt-cache safe: every change is on /model listing/discovery + scope construction; nothing mutates the conversation prefix, rebuilds the system prompt, or swaps toolsets mid-conversation.

Findings

P3 (non-blocking, cosmetic)agent/credential_pool.py:2172: return resolved or env_val or ("" if scoped else raw) sits inside the if raw.startswith("op://") branch, which is only reachable on the unscoped path (the scoped branch returns at line 2159). So ("" if scoped else raw) is always raw here — the scoped ternary is dead-conditioned. Harmless, but simplifying to return resolved or env_val or raw would remove the misleading condition. Not worth a revision on its own.

No blocking issues. Recommend merge once maintainer is comfortable with the size.

— Local code-reviewer pass (ran the repo's test suite locally against live HEAD; verified the fail-closed contract and scoped-miss behavior by tracing, not just reading).

@exiao

exiao commented Jul 15, 2026

Copy link
Copy Markdown
Owner Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 141635dfaa

ℹ️ 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".

@exiao
exiao merged commit bbdaed9 into live-config Jul 15, 2026
31 of 32 checks passed
@exiao
exiao deleted the fix/model-switch-list-getsecret branch July 15, 2026 03:00
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.
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.

1 participant