Skip to content

fix: replace get_secret with os.environ.get in WeixinAdapter.__init__ - #66073

Closed
176206564-pixel wants to merge 1 commit into
NousResearch:mainfrom
176206564-pixel:fix/weixin-get-secret-unscoped-error
Closed

fix: replace get_secret with os.environ.get in WeixinAdapter.__init__#66073
176206564-pixel wants to merge 1 commit into
NousResearch:mainfrom
176206564-pixel:fix/weixin-get-secret-unscoped-error

Conversation

@176206564-pixel

Copy link
Copy Markdown

Problem

When multiplexing is enabled (multiplex_profiles: true), the gateway crashes in a restart loop during startup:

UnscopedSecretError: get_secret('WEIXIN_CDN_BASE_URL') called with no profile secret scope active while multiplexing is on.

Root Cause

WeixinAdapter.__init__ calls get_secret() (lines 1163-1168) during gateway startup, before any profile secret scope is established. get_secret() requires a scope when multiplexing is active (fail-closed design to prevent cross-profile credential leaks).

This is inconsistent with other env var reads in the same __init__ (lines 1169-1178), which already use os.getenv() directly.

Fix

Replace the 4 get_secret() calls in __init__ with os.environ.get(), consistent with existing patterns in the same method.

Testing

Verified on a WSL2 deployment with multiplex_profiles: true and weixin credentials in .env — gateway now starts without error and runs stably.

get_secret() requires a profile secret scope when multiplexing is active,
but WeixinAdapter.__init__ is called during gateway startup before any scope
is set. This causes UnscopedSecretError and gateway crash loop.

Replace the 4 get_secret calls in __init__ with os.environ.get(), consistent
with how other env vars (WEIXIN_SEND_CHUNK_DELAY_SECONDS, etc.) are already
read in the same method.

Fixes: gateway crash loop with 'UnscopedSecretError: get_secret(...) called
with no profile secret scope active while multiplexing is on'

@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: Comment

Scope: +4/-4, replace get_secret with os.environ.get

Notes

  • Fix: replace get_secret with os.environ.get in WeixinAdapter.init.
  • No hardcoded credentials.
  • No debug artifacts.
  • LGTM.

Reviewed by Hermes Agent

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/wecom WeCom / WeChat Work adapter area/auth Authentication, OAuth, credential pools needs-decision Awaiting maintainer decision before any implementation labels Jul 17, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: this availability workaround conflicts with the scoped-secret direction in #59674; maintainers should decide the intended initialization contract before merge.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating the startup failure.

Problems

  • gateway/platforms/weixin.py:1163-1167 must not fall back to raw os.environ in multiplex mode. agent/secret_scope.py:149-157 defines the unscoped exception as a fail-closed signal and directs callers to install a profile scope; raw process environment can contain another profile's credentials.
  • This reverses commit 6160a8025, which deliberately migrated these exact Weixin fallbacks to get_secret() for profile-scoped resolution.
  • The primary startup loop calls _create_adapter() unscoped at gateway/run.py:7314, whereas secondary profile startup scopes equivalent construction at gateway/run.py:8899-8900. That asymmetry is the safer place to investigate. No regression test accompanies this PR.

Suggested changes

  • Keep the scoped get_secret() calls and scope the relevant primary initialization lifecycle consistently with the secondary adapter path.
  • Add a multiplex regression covering conflicting global and scoped Weixin values, plus the empty-scope fail-closed case.

Automated hermes-sweeper review.

self._token = str(config.token or extra.get("token") or get_secret("WEIXIN_TOKEN", "")).strip()
self._base_url = str(extra.get("base_url") or get_secret("WEIXIN_BASE_URL", ILINK_BASE_URL)).strip().rstrip("/")
self._account_id = str(extra.get("account_id") or os.environ.get("WEIXIN_ACCOUNT_ID", "")).strip()
self._token = str(config.token or extra.get("token") or os.environ.get("WEIXIN_TOKEN", "")).strip()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

get_secret() is deliberately fail-closed here in multiplex mode (agent/secret_scope.py:149-157). Reading os.environ instead can bind this adapter to another profile's token; scope the primary adapter lifecycle rather than bypassing the credential boundary.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 18, 2026

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

This was generated by AI during triage.

Summary

Two PRs address the same reported multiplex-mode startup failure by replacing unscoped Weixin get_secret() fallbacks with process-environment reads. #66073 changes four adapter-initialization calls, while #68854 applies the same approach there and to send_weixin_direct(); neither diff establishes the lifecycle asymmetry as the root cause or adds a regression test.

Related pull requests

  • #66073 related — (+4/-4) — needs revision: Replaces four get_secret() fallbacks in WeixinAdapter.init with os.environ.get(), avoiding the reported UnscopedSecretError but bypassing profile-scoped secret isolation. The contributor keep_open review on #66073 should remain controlling: it identifies the primary-versus-secondary initialization asymmetry only as a safer investigation path, not an established root cause, and requires scoped lifecycle handling plus multiplex isolation tests before merge.
  • #68854 duplicate — (+10/-9) — duplicate approach, broader unsafe scope: Makes the same environment-fallback substitution as #66073, removes the get_secret import, and additionally changes all four send_weixin_direct() lookups. This also suppresses the reported exception by bypassing fail-closed secret resolution, without addressing the isolation objection documented in the contributor review on #66073 or adding the requested regression coverage.

Duplicates

#68854 substantially duplicates #66073's four initialization substitutions and extends the same raw-environment approach to send_weixin_direct().

Suggested consolidation

Revise, then merge #66073 as the consolidation target only after explicitly addressing its contributor keep_open review: preserve scoped get_secret() resolution, investigate and correct the relevant initialization scope, and add multiplex tests for conflicting global/scoped values and empty-scope fail-closed behavior. Do not merge either current diff as-is; #68854 can be closed as a broader duplicate of #66073 once any useful direct-send coverage is carried into the corrected solution.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup66073 ["PRs duplicating each other"]
        P66073["PR #66073 (open)"]
        P68854["PR #68854 (open)"]
    end
    class P66073 open
    class P68854 open
    class P66073 target
    click P66073 "https://github.com/NousResearch/hermes-agent/pull/66073"
    click P68854 "https://github.com/NousResearch/hermes-agent/pull/68854"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 5 kB of PR diffs, 2 kB of issue/PR text, 2 kB of discussion (3 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

teknium1 added a commit that referenced this pull request Aug 2, 2026
The adapter's __init__ and send_weixin_direct read WEIXIN_ACCOUNT_ID/
TOKEN/BASE_URL/CDN_BASE_URL via bare get_secret, which raises
UnscopedSecretError when the DEFAULT profile's adapter constructs or
sends unscoped under multiplexing (corrects the direction of #66073 /
#68854, which tried to solve this by borrowing os.environ on every
read — a cross-profile leak).

Add a module-level _wx_secret helper following the established Slack
SLACK_APP_TOKEN pattern (#59739) and WhatsApp's _get_wsecret: a SCOPED
miss returns the default (the scope is authoritative — no environ
borrow), while an UNSCOPED read under multiplex falls back to
os.environ, which is the default profile's own value.

Regression tests cover both directions: scoped construction reads the
scope's value and a scoped miss yields empty (no borrow); unscoped
construction falls back to os.environ instead of raising.
@teknium1

teknium1 commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Closing in favor of the merged correction (#76663): the crash you hit was real — the default profile's Weixin adapter constructs unscoped under multiplexing and bare get_secret raises — but reverting to os.environ.get would also run during secondary-profile construction, silently inheriting the default profile's WEIXIN_TOKEN when a secondary's .env lacks it (the exact cross-profile leak the migration exists to prevent). Main now uses the Slack-pattern helper wx_secret (try get_secret / except UnscopedSecretError → os.getenv) on all 8 WEIXIN* reads: your crash is fixed AND secondary-profile isolation holds, with regression tests for both. Thanks for the report-quality diagnosis @176206564-pixel.

@teknium1 teknium1 closed this Aug 2, 2026
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
The adapter's __init__ and send_weixin_direct read WEIXIN_ACCOUNT_ID/
TOKEN/BASE_URL/CDN_BASE_URL via bare get_secret, which raises
UnscopedSecretError when the DEFAULT profile's adapter constructs or
sends unscoped under multiplexing (corrects the direction of NousResearch#66073 /
NousResearch#68854, which tried to solve this by borrowing os.environ on every
read — a cross-profile leak).

Add a module-level _wx_secret helper following the established Slack
SLACK_APP_TOKEN pattern (NousResearch#59739) and WhatsApp's _get_wsecret: a SCOPED
miss returns the default (the scope is authoritative — no environ
borrow), while an UNSCOPED read under multiplex falls back to
os.environ, which is the default profile's own value.

Regression tests cover both directions: scoped construction reads the
scope's value and a scoped miss yields empty (no borrow); unscoped
construction falls back to os.environ instead of raising.
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/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists platform/wecom WeCom / WeChat Work adapter 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 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.

5 participants