Skip to content

fix(gateway): read API_SERVER_* through profile secret scope in multiplex mode - #52399

Closed
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:fix/compression-feasible-fallback
Closed

fix(gateway): read API_SERVER_* through profile secret scope in multiplex mode#52399
Bartok9 wants to merge 1 commit into
NousResearch:mainfrom
Bartok9:fix/compression-feasible-fallback

Conversation

@Bartok9

@Bartok9 Bartok9 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

x

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/gateway Gateway runner, session dispatch, delivery area/auth Authentication, OAuth, credential pools area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jun 25, 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.

Code Review Summary

Verdict: Approved

Clean fix for API_SERVER_* credential scope in multiplex mode. Under multiplexing, the default profile's .env is loaded into global os.environ at import time, so raw os.getenv("API_SERVER_KEY") would find the DEFAULT profile's key while resolving a SECONDARY profile (#52307).

The fix introduces _scoped_env() that reads through get_secret() (which honors the active profile scope) with a safe fallback to os.environ for single-profile deployments. The 3 test cases cover: secondary profile without key (should not enable), secondary profile with key (should enable), single profile (legacy behavior).

Looks Good

  • Well-scoped: 27 additions to gateway/config.py, 50 lines of tests
  • Safe fallback to os.environ when no scope is active
  • 3 test cases covering the key scenarios
  • Clear documentation of the root cause in comments

Reviewed by Hermes Agent

@Bartok9

Bartok9 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! The os.environ fallback keeps single-profile deployments on the legacy path, so this should be safe to land. Ready to merge whenever convenient.

Preserves intent of NousResearch#52399 on main where _apply_env_overrides already uses
_getenv_str/scoped secret reads; plates clear drift (2026-07-11).
@Bartok9

Bartok9 commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Plate-clear 2026-07-11: Branch was massively stale vs main. Current main already routes env reads via scoped _getenv_str; PR rewritten to add TestApiServerEnvIsScoped regression tests for #52307 multiplex isolation. Source rewrite of _scoped_env is no longer needed.

@Bartok9
Bartok9 force-pushed the fix/compression-feasible-fallback branch from e121cb6 to e612cf9 Compare July 11, 2026 17:15
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for preserving the regression intent after rebasing the work.

Automated hermes-sweeper review found this guarantee is already implemented on current main:

  • 0f154e780e71c74f8a1cdccb25c97a6abd8e5a57 introduced scoped gateway configuration environment reads.
  • gateway/config.py:174-194 resolves environment values from the active profile secret scope, without falling back to process environment while scoped.
  • gateway/config.py:1755-1780 applies that helper to every API_SERVER_* value.
  • tests/gateway/test_config.py:1260-1295 already proves that a secondary scoped profile does not inherit default-profile API-server enablement.

The PR's July 11 note correctly identifies that the source fix is already on main; its remaining tests duplicate the implemented and covered behavior.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added the sweeper:implemented-on-main Sweeper: behavior already present on current main label Jul 15, 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 area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:implemented-on-main Sweeper: behavior already present on current main type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants