Skip to content

fix(config): fall back after scoped secret miss - #58102

Closed
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-dotenv-secret-env-fallback
Closed

fix(config): fall back after scoped secret miss#58102
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-dotenv-secret-env-fallback

Conversation

@tianma-if

Copy link
Copy Markdown
Contributor

Summary

  • fix get_env_value_prefer_dotenv() so a scoped secret miss can still read os.environ in non-multiplex deployments
  • keep multiplexed profile scopes authoritative so a missing profile secret does not leak a process-global provider key
  • add regression coverage for the BWS/cron-style scoped miss and the multiplex isolation case

Fixes #58100.

Test plan

  • source .venv/bin/activate && pytest tests/hermes_cli/test_config.py -q -k "TestGetEnvValuePreferDotenv"
  • source .venv/bin/activate && pytest tests/hermes_cli/test_xiaomi_provider.py tests/hermes_cli/test_api_key_providers.py -q -k "xiaomi or env or credential or provider"
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 4, 2026

@tonydwb tonydwb 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.

Verdict: Approved (read-only token - formal approval deferred)

Small, well-scoped fix. No concerns.

@tonydwb tonydwb 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 Summary

Verdict: Approved (LGTM)

Falls back after scoped secret miss. Improves config robustness.

What Looks Good

  • Targeted fix
  • No security concerns
  • No debug artifacts

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the narrowly scoped regression fix. The non-multiplex behavior is real on current main: hermes_cli/config.py:7873 directly returns a scoped miss, and the added tests cover the safe fallback plus the multiplex isolation boundary.

Problems

Suggested changes

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 15, 2026
@teknium1

teknium1 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor

Closing as superseded: commit c758ded (fix(secrets): fall back to os.environ on scope miss when multiplexing is off, merged Jul 20) implemented exactly this PR's semantic — scope-miss environ fallback gated on the multiplex flag — but centrally inside agent.secret_scope.get_secret rather than per-caller, and get_env_value_prefer_dotenv now routes through it. Your linked issue #58100 is closed COMPLETED by that commit. Thanks @tianma-if — the diagnosis (BWS-injected keys invisible under an unconditionally-installed scope) was exactly right, and the mode-conditional rule you proposed is now the documented design.

@teknium1 teknium1 closed this Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: get_env_value_prefer_dotenv returns None when get_secret returns None instead of falling through to os.environ

4 participants