Skip to content

fix(gateway): honor explicit api_server enabled:false under env key - #51374

Closed
davidgut1982 wants to merge 1 commit into
NousResearch:mainfrom
davidgut1982:fix/api-server-env-override-secondary-profile
Closed

davidgut1982 wants to merge 1 commit into
NousResearch:mainfrom
davidgut1982:fix/api-server-env-override-secondary-profile

Conversation

@davidgut1982

@davidgut1982 davidgut1982 commented Jun 23, 2026 •

Copy link
Copy Markdown

What does this PR do?

Honor an explicit platforms.api_server.enabled: false under the presence of an API-server env key.

The contract: when config.yaml explicitly pins platforms.api_server.enabled: false (recorded via the _enabled_explicit marker in PlatformConfig.extra), that explicit disable takes precedence over the mere presence of API_SERVER_ENABLED / API_SERVER_KEY in the same scope. Today _apply_env_overrides() unconditionally re-enables api_server whenever either env var is present, silently overriding the explicit disable.

This aligns api_server with the already-migrated plugin platforms (telegram / matrix / slack), which already consult _enabled_explicit (via _enable_from_env()) before re-enabling. The api_server block re-enables inline and was the one platform that did not honor the marker.

Related Issue

Fixes #

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Root cause

gateway/config.py — _apply_env_overrides(), the # API Server block:

if api_server_enabled or api_server_key:
    if Platform.API_SERVER not in config.platforms:
        config.platforms[Platform.API_SERVER] = PlatformConfig()
    config.platforms[Platform.API_SERVER].enabled = True   # <-- unconditional

The presence of API_SERVER_KEY / API_SERVER_ENABLED in the environment force-enables the platform even when the same scope's config.yaml explicitly set enabled: false. The migrated plugin platforms read _enabled_explicit before re-enabling; api_server did not.

Changes Made

  • gateway/config.py: guard the api_server.enabled = True assignment on the explicit-disable marker. Use extra.pop("_enabled_explicit", False): the api_server branch is terminal (unlike the migrated plugin platforms, no later registry pass re-enables api_server), so popping consumes the flag in a single read and avoids a double-read; the per-platform _enabled_explicit cleanup at the end of _apply_env_overrides remains a no-op for api_server. The normal env-enable path (no explicit disable) is unchanged.
  • tests/gateway/test_config.py: retain the focused regression test TestApiServerEnvOverride::test_env_key_does_not_reenable_explicitly_disabled_api_server.

Scope: same-scope precedence, not cross-profile multiplexing

This PR is about the same-scope explicit-disable precedence contract only. It is deliberately orthogonal to the cross-profile (multiplex) contract already covered on main:

  • The profile-scoped gateway env reads were added on main in 0f154e780e....
  • tests/gateway/test_config.py on main already verifies that a default-profile API-server env setting does not enable a secondary profile.

That orthogonal contract answers "does a default-profile env var leak into another profile's scope?" — this PR answers a different question: "within one scope, does an explicit enabled: false in config.yaml survive the presence of an env key in that same scope?" It does not, today; this PR makes it so. No cross-profile / multiplexing behavior is added or changed here.

How to test

pytest tests/gateway/test_config.py::TestApiServerEnvOverride::test_env_key_does_not_reenable_explicitly_disabled_api_server -v

With the fix the test passes; removing the _enabled_explicit guard makes it fail (the enabled flag flips back to True under API_SERVER_KEY). Full config regression: pytest tests/gateway/test_config.py -q (100 passed) and pytest tests/gateway/test_multiplex_phase0.py -q (28 passed).

🤖 Generated with Claude Code

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists labels Jun 23, 2026
@davidgut1982
davidgut1982 marked this pull request as ready for review June 23, 2026 14:08

@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

Targeted fix for the gateway config: an explicit enabled: false for api_server in config.yaml now survives _apply_env_overrides(), even when API_SERVER_KEY is present in the environment.

Looks Good

  • Uses _enabled_explicit marker (pop convention) to distinguish explicit config from default
  • The guard if not api_server_explicit or config.platforms[Platform.API_SERVER].enabled correctly handles both cases
  • Test verifies: explicit disable wins over env-var presence, but the key is still wired through
  • 2 files changed, clean and focused

Reviewed by Hermes Agent

phantom-instruction-set pushed a commit to phantom-instruction-set/hermes-agent that referenced this pull request Jul 5, 2026
The email adapter (plugins/platforms/email/adapter.py) read
EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST,
EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv()
directly. In a multiplexed gateway, os.environ holds the default
profile's .env values, so every secondary profile inherited the
default profile's email credentials instead of its own.

This was a sibling of the api_server env-leak bug (NousResearch#52307/NousResearch#50051):
the same os.getenv→get_secret migration that PR NousResearch#50094 applies to
gateway/config.py, but for the email adapter itself, which neither
PR NousResearch#50094 nor NousResearch#51374 covers.

Changes:
- plugins/platforms/email/adapter.py: replace os.getenv with
  agent.secret_scope.get_secret for all EMAIL_* credential reads
  (adapter __init__, check_email_requirements, _allowlist_in_effect,
  _dispatch_message allowlist gate, _send_email SMTP helper).
- gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers
  (from PR NousResearch#50094) and replace os.getenv with _getenv for the email
  block in _apply_env_overrides, so config.platforms[EMAIL].extra
  is populated from the scoped value.
- tests/gateway/test_email_secret_scope.py: 5 new tests covering
  scoped credential reads, environ fallback without scope, missing-
  key-no-leak, allowlist scoping, and check_email_requirements scoping.

Related: NousResearch#50051, NousResearch#52307, PR NousResearch#50094, PR NousResearch#51374
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused regression coverage. Current main still has the direct precedence behavior this patch changes: gateway/config.py:1761-1764 enables api_server whenever API_SERVER_ENABLED or API_SERVER_KEY is present, while the loader records explicit platform enablement at gateway/config.py:1125-1126.

Problems

  • The multiplex-specific rationale is now stale. Commit 0f154e780e71c74f8a1cdccb25c97a6abd8e5a57 added profile-scoped gateway env reads (gateway/config.py:174-189), and tests/gateway/test_config.py:1282-1295 verifies that a default-profile API-server env setting does not enable a secondary profile. The patch therefore changes the separate same-scope precedence contract between explicit enabled: false and API-server env settings.

Suggested changes

  • State that direct precedence contract explicitly in the PR description and retain the focused regression test for it.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
_apply_env_overrides() force-set ``api_server.enabled = True`` whenever
API_SERVER_KEY (or API_SERVER_ENABLED) was present in the environment.

In multiplex mode, a secondary profile pins
``platforms.api_server.enabled: false`` in its config.yaml so that it
shares the default profile's API-server listener instead of binding its
own port. That profile still inherits the process-level env, including
API_SERVER_KEY, so the unconditional re-enable flipped api_server back on
and tripped the MultiplexConfigError check.

Honor an explicit disable, flagged by ``_enabled_explicit`` in the
platform's extra. Use ``extra.pop("_enabled_explicit", False)``: the
api_server branch is terminal (unlike the migrated plugin platforms, no
later registry pass re-enables api_server), so popping consumes the flag
in a single read and avoids the double-read hazard, while the final
per-platform cleanup remains a no-op.

Adds a regression test asserting that with API_SERVER_KEY set, a config
with api_server explicitly enabled:false + _enabled_explicit:true survives
_apply_env_overrides() as enabled=False (fails without the fix), while the
key is still wired through for the shared listener.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@davidgut1982
davidgut1982 force-pushed the fix/api-server-env-override-secondary-profile branch from 16491e4 to 59195b0 Compare July 15, 2026 11:50
@davidgut1982

Copy link
Copy Markdown
Author

Addressed your review — reworked & rebased onto current main (now 59195b074), which also clears the merge conflict.

  • Reframed the description around the direct same-scope precedence contract: an explicit platforms.api_server.enabled: false (recorded via _enabled_explicit) takes precedence over the presence of API_SERVER_ENABLED/API_SERVER_KEY, aligning api_server with the migrated plugin platforms that already consult that marker.
  • Explicitly distinguished this from the orthogonal cross-profile contract already on main (profile-scoped env reads from 0f154e780e; the default→secondary test).
  • Retained the focused regression test TestApiServerEnvOverride::test_env_key_does_not_reenable_explicitly_disabled_api_server.

Please re-review.

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #70164 — your commit was cherry-picked onto current main with your authorship preserved in git log (c7fd3eb). The explicit-disable guard now follows the same _enabled_explicit convention the plugin platforms use. Thanks for the clean fix and the multiplex context in the PR body!

teknium1 pushed a commit that referenced this pull request Aug 2, 2026
The email adapter (plugins/platforms/email/adapter.py) read
EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST,
EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv()
directly. In a multiplexed gateway, os.environ holds the default
profile's .env values, so every secondary profile inherited the
default profile's email credentials instead of its own.

This was a sibling of the api_server env-leak bug (#52307/#50051):
the same os.getenv→get_secret migration that PR #50094 applies to
gateway/config.py, but for the email adapter itself, which neither
PR #50094 nor #51374 covers.

Changes:
- plugins/platforms/email/adapter.py: replace os.getenv with
  agent.secret_scope.get_secret for all EMAIL_* credential reads
  (adapter __init__, check_email_requirements, _allowlist_in_effect,
  _dispatch_message allowlist gate, _send_email SMTP helper).
- gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers
  (from PR #50094) and replace os.getenv with _getenv for the email
  block in _apply_env_overrides, so config.platforms[EMAIL].extra
  is populated from the scoped value.
- tests/gateway/test_email_secret_scope.py: 5 new tests covering
  scoped credential reads, environ fallback without scope, missing-
  key-no-leak, allowlist scoping, and check_email_requirements scoping.

Related: #50051, #52307, PR #50094, PR #51374
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
The email adapter (plugins/platforms/email/adapter.py) read
EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST,
EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv()
directly. In a multiplexed gateway, os.environ holds the default
profile's .env values, so every secondary profile inherited the
default profile's email credentials instead of its own.

This was a sibling of the api_server env-leak bug (NousResearch#52307/NousResearch#50051):
the same os.getenv→get_secret migration that PR NousResearch#50094 applies to
gateway/config.py, but for the email adapter itself, which neither
PR NousResearch#50094 nor NousResearch#51374 covers.

Changes:
- plugins/platforms/email/adapter.py: replace os.getenv with
  agent.secret_scope.get_secret for all EMAIL_* credential reads
  (adapter __init__, check_email_requirements, _allowlist_in_effect,
  _dispatch_message allowlist gate, _send_email SMTP helper).
- gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers
  (from PR NousResearch#50094) and replace os.getenv with _getenv for the email
  block in _apply_env_overrides, so config.platforms[EMAIL].extra
  is populated from the scoped value.
- tests/gateway/test_email_secret_scope.py: 5 new tests covering
  scoped credential reads, environ fallback without scope, missing-
  key-no-leak, allowlist scoping, and check_email_requirements scoping.

Related: NousResearch#50051, NousResearch#52307, PR NousResearch#50094, PR NousResearch#51374
prmartinow pushed a commit to prmartinow/hermes-agent that referenced this pull request Aug 26, 2026
The email adapter (plugins/platforms/email/adapter.py) read
EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST,
EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv()
directly. In a multiplexed gateway, os.environ holds the default
profile's .env values, so every secondary profile inherited the
default profile's email credentials instead of its own.

This was a sibling of the api_server env-leak bug (NousResearch#52307/NousResearch#50051):
the same os.getenv→get_secret migration that PR NousResearch#50094 applies to
gateway/config.py, but for the email adapter itself, which neither
PR NousResearch#50094 nor NousResearch#51374 covers.

Changes:
- plugins/platforms/email/adapter.py: replace os.getenv with
  agent.secret_scope.get_secret for all EMAIL_* credential reads
  (adapter __init__, check_email_requirements, _allowlist_in_effect,
  _dispatch_message allowlist gate, _send_email SMTP helper).
- gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers
  (from PR NousResearch#50094) and replace os.getenv with _getenv for the email
  block in _apply_env_overrides, so config.platforms[EMAIL].extra
  is populated from the scoped value.
- tests/gateway/test_email_secret_scope.py: 5 new tests covering
  scoped credential reads, environ fallback without scope, missing-
  key-no-leak, allowlist scoping, and check_email_requirements scoping.

Related: NousResearch#50051, NousResearch#52307, PR NousResearch#50094, PR NousResearch#51374
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
The email adapter (plugins/platforms/email/adapter.py) read
EMAIL_ADDRESS, EMAIL_PASSWORD, EMAIL_IMAP_HOST, EMAIL_SMTP_HOST,
EMAIL_ALLOWED_USERS, and EMAIL_ALLOW_ALL_USERS via os.getenv()
directly. In a multiplexed gateway, os.environ holds the default
profile's .env values, so every secondary profile inherited the
default profile's email credentials instead of its own.

This was a sibling of the api_server env-leak bug (NousResearch#52307/NousResearch#50051):
the same os.getenv→get_secret migration that PR NousResearch#50094 applies to
gateway/config.py, but for the email adapter itself, which neither
PR NousResearch#50094 nor NousResearch#51374 covers.

Changes:
- plugins/platforms/email/adapter.py: replace os.getenv with
  agent.secret_scope.get_secret for all EMAIL_* credential reads
  (adapter __init__, check_email_requirements, _allowlist_in_effect,
  _dispatch_message allowlist gate, _send_email SMTP helper).
- gateway/config.py: add _getenv/_getenv_str/_getenv_int helpers
  (from PR NousResearch#50094) and replace os.getenv with _getenv for the email
  block in _apply_env_overrides, so config.platforms[EMAIL].extra
  is populated from the scoped value.
- tests/gateway/test_email_secret_scope.py: 5 new tests covering
  scoped credential reads, environ fallback without scope, missing-
  key-no-leak, allowlist scoping, and check_email_requirements scoping.

Related: NousResearch#50051, NousResearch#52307, PR NousResearch#50094, PR NousResearch#51374
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants