fix(gateway): stop multiplexed session recovery adopting a sibling profile's row - #74593
Conversation
…ofile's row The durable peer fallback in `find_latest_gateway_session_for_peer` drops `session_key` and matches only (source, user_id, chat_id, chat_type, thread_id). For a DM that tuple is byte-identical across profiles -- `chat_id == user_id` and `thread_id IS NULL` for every bot -- so it can return a row owned by another profile. `_recovered_row_allowed_for_active_profile` guards both recovery paths against exactly that, but returned True immediately when `multiplex_profiles` was enabled: the one configuration in which several profiles each own a bot token and can serve the same allowlisted user. A user DMing two of those bots had every session collapsed onto whichever profile created one first, and the sibling profile was then executed with its own persona, tools, credentials and filesystem scope -- a privilege boundary, not a session-list display detail. The multiplexed path now compares the recovered row's namespace against the requested key's profile, which is the profile the inbound message was routed to and therefore owns the turn; the process-wide active profile is meaningless when several profiles serve traffic at once. The non-multiplexed branch still compares against the active profile. Exact-key matches, same-profile rows, and rows whose key carries no parseable profile stay recoverable, so only the cross-profile case is removed. Also drops the "multiplex_profiles is disabled" claim from the two recovery warnings, which is no longer the only reason a row is rejected. Adds tests/gateway/test_multiplex_peer_fallback_profile.py -- this guard had no test coverage anywhere in tests/. Fixes NousResearch#74285
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the multiplex bypass; current main does still return early for multiplexing at gateway/session.py:1554-1555.
Problems
- The guard sees only one candidate.
hermes_state.py:3057-3073orders all peer matches and appliesLIMIT 1; if that row belongs to a newer sibling profile, the new rejection atgateway/session.py:1579creates a fresh session (gateway/session.py:2385-2402) instead of considering an older matching-profile fallback row. - Multiplex recovery still accepts an unparseable row namespace (
gateway/session.py:1565-1567), and the new test requires it attests/gateway/test_multiplex_peer_fallback_profile.py:99-106. The peer tuple has no profile discriminator, so this cannot establish ownership for the requested profile.
Suggested changes
- Select the requested profile's fallback candidate before
LIMIT 1, or return candidates for the session layer to select; add a real-SessionDB test that forces fallback with a newer sibling and older same-profile row. - Fail closed for unknown-profile fallback rows in multiplex mode unless another authoritative ownership field proves the match.
Automated hermes-sweeper review.
| requested_profile = self._profile_from_session_key(requested_session_key) | ||
| if requested_profile is None: | ||
| return True | ||
| return recovered_profile == requested_profile |
There was a problem hiding this comment.
This only validates the one globally newest peer-fallback row. find_latest_gateway_session_for_peer orders all matching profiles and applies LIMIT 1 (hermes_state.py:3057-3073), so rejecting a newer sibling row prevents recovery of an older valid row for requested_profile and falls through to a fresh session. Select/profile-filter candidates before the limit, or iterate candidates here.
| requested_session_key=store._generate_session_key(_dm_source("restricted")), | ||
| recovered={"id": "sid_legacy", "session_key": "legacy-unnamespaced-key"}, | ||
| ) | ||
| assert allowed is True |
There was a problem hiding this comment.
An unparseable fallback key has no authoritative profile ownership, while the peer tuple is shared across multiplexed profiles. Allowing it through can still attach a legacy sibling row to restricted; multiplex fallback should fail closed for this case unless ownership is established elsewhere.
teknium1
added
sweeper:risk-security-boundary
…on unnamespaced rows Rejecting a sibling's row is only half of it. The durable fallback orders every profile's rows for the peer tuple under one LIMIT 1, so whenever a sibling spoke more recently it is the only candidate the guard ever sees: our own older recoverable row is never offered and recovery ends in a needless fresh session. Give find_latest_gateway_session_for_peer an optional session_key_prefix and filter before LIMIT 1, so a multiplexed gateway selects its own most recent row. substr() rather than LIKE — LIKE folds ASCII case and reads _ / % in a profile name as wildcards. Only multiplexed callers pass it; a single-profile gateway owns every row the peer tuple can reach and must keep seeing rows written under an earlier profile name. Second, multiplex recovery no longer adopts a row whose key names no profile. The peer tuple carries no profile discriminator, so such a row is exactly as likely to be a sibling's, and adopting it runs that transcript under this profile's credentials and filesystem scope. Single-profile gateways still adopt it: one claimant, and pre-namespace rows have to stay recoverable. The fallback selection is covered against a real SessionDB — newer sibling row, older same-profile row — rather than a mocked finder.
|
Both points land. Pushed 553558d.
Unnamespaced rows now fail closed while multiplexing, including rows carrying no Fallback selection is now covered against a real |
SummaryThree PRs address #74285's cross-profile session-recovery defect. #74153 combines profile filtering with a guard and routing cleanup but mishandles default-profile persistence and does not truly test fallback; #74593 scopes candidate selection by exact session-key namespace before LIMIT 1, rejects ownership-ambiguous multiplex rows, and adds real SessionDB regressions; #75043 supplies only the minimal profile_name SQL plumbing without regression coverage. Related pull requests
Duplicates#74153, #74593, and #75043 address the same peer-fallback isolation defect. #74153 and #75043 substantially duplicate portions of #74593; #74593 contains the broader corrected implementation and regression coverage. Suggested consolidationKeep #74593 open with a salvage path: independently verify its updated exact-prefix pre-LIMIT selection, fail-closed handling for ownership-ambiguous multiplex rows, and real-SessionDB regressions, including the newer-sibling/older-same-profile case and SessionStore plumbing. Then close #74153 and #75043 as duplicates of #74593; this departs from their keep_open reviews because #74153's salvageable filtering is already represented without its documented default-profile mismatch, while #75043's Verify-selected SQL fix is subsumed by #74593 with the regression coverage its review requested. Complex graphflowchart 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
I74285(["issue #74285 (open)"])
subgraph Dup74153 ["PRs duplicating each other"]
P74153["PR #74153 (open)"]
P74593["PR #74593 (open)"]
P75043["PR #75043 (open)"]
end
P74593 -->|best fix| I74285
class I74285 open
class P74153 open
class P74593 open
class P75043 open
class P74593 best
class P75043 best
class P74593 target
click I74285 "https://github.com/NousResearch/hermes-agent/issues/74285"
click P74153 "https://github.com/NousResearch/hermes-agent/pull/74153"
click P74593 "https://github.com/NousResearch/hermes-agent/pull/74593"
click P75043 "https://github.com/NousResearch/hermes-agent/pull/75043"
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 (state tag in the node label). Cross-PR triage: Reviewed 3 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 36 kB of PR diffs, 17 kB of issue/PR text, 6 kB of discussion (10 comments), 6 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Thanks for this PR. Merged via #101248 (ad8b9e0) on current main — per-profile /voice state, off-loop startup hydration, session-recovery owner fence, int route ids. #101248 won as the consolidated fix because it covers the whole multiplex-profile bug class in one change (with tests) rather than the single symptom addressed here; this PR is superseded by it. If anything from your original change is still missing on main >= ad8b9e0, please open a fresh PR/issue against main and tag it. Thanks again. |
4 participants
What does this PR do?
On a multiplexed gateway (
GATEWAY_MULTIPLEX_PROFILES=1) where several profiles each own their own bot token, a user allowlisted on two or more of those bots had every DM session collapsed onto whichever profile created a session first. Subsequent messages to any of the bots were then handled by that one profile — its persona, tools, credentials and filesystem scope. Because the wrong profile is actually executed, this is a privilege boundary, not a session-list display detail.The collapse comes from the durable peer fallback.
SessionDB.find_latest_gateway_session_for_peerfirst matches the exact, profile-namespacedsession_key; when that misses it falls back to the peer tuple(source, user_id, chat_id, chat_type, thread_id). For a Telegram DM that tuple is byte-identical across profiles —chat_id == user_idandthread_id IS NULLfor every bot — so the fallback returns a sibling profile's row.SessionStorealready has a guard for exactly this, called on both recovery paths (_recover_session_from_dband_query_recoverable_session) right after the fallback. It just opted out of the multiplexed case:So the guard was disabled precisely where cross-profile collision is possible. This PR makes the multiplexed path compare profiles instead of waving the row through: the requested key carries the profile the inbound message was routed to, which is authoritative per-message, so it is compared against the recovered row's namespace via the existing
_profile_from_session_keyhelper. The non-multiplexed branch keeps comparing against the process-wide active profile, unchanged.Before / after,
restrictedasking whileadminowns the row:agent:admin:telegram:dm:<uid>agent:restricted:...key returnssession_id=sid_adminagent:admin:telegram:dm:<uid>restrictedgets its own sessionExact-key matches, same-profile rows, and rows whose key carries no parseable profile stay recoverable, so this only removes the cross-profile case.
Follow-up after review
Both review points were real; second commit covers them.
The guard only ever saw one candidate.
find_latest_gateway_session_for_peerorders every profile's rows for the peer tuple under a singleLIMIT 1, so whenever a sibling spoke more recently it is the only row the guard is offered — rejecting it then loses our own older recoverable row and recovery ends in a needless fresh session.find_latest_gateway_session_for_peernow takes an optionalsession_key_prefixand filters beforeLIMIT 1, so a multiplexed gateway selects its own most recent row.substr()rather thanLIKE:LIKEfolds ASCII case and reads_/%in a profile name as wildcards. Only multiplexed callers pass it — a single-profile gateway owns every row the peer tuple can reach and must keep seeing rows written under an earlier profile name.Unnamespaced rows now fail closed while multiplexing. The peer tuple has no profile discriminator, so a row whose key names no profile is exactly as likely to be a sibling's; adopting it runs that transcript under this profile's credentials and filesystem scope. Same for a row carrying no
session_keyat all. Single-profile gateways still adopt both — one claimant, and pre-namespace rows have to stay recoverable. The test that required the old behavior is inverted, with the non-multiplexed case pinned separately.Fallback selection is now covered against a real
SessionDB(newer sibling row + older same-profile row, andrestricted-2not matchingrestricted) rather than a mocked finder.Related Issue
Fixes #74285
Type of Change
Changes Made
gateway/session.py—_recovered_row_allowed_for_active_profilenow checks the recovered row's profile in both configurations, differing only in what counts as "ours": the requested key's profile when multiplexing, the active profile otherwise. Docstring records why the peer tuple can't distinguish profiles for a DM.gateway/session.py— the two recovery-path warnings no longer claim "multiplex_profiles is disabled" as the reason, which is no longer the only case that rejects a row.tests/gateway/test_multiplex_peer_fallback_profile.py— new file, 6 tests. This guard previously had no test coverage anywhere intests/.How to Test
On
mainthe two multiplexed tests fail, and the end-to-end one prints the exact collapse from the issue — a key namespaced to one profile holding another'ssession_id:Regression sweep over the gateway suite (CI parity, per-file isolation):
Manual: two profiles each with their own
TELEGRAM_BOT_TOKEN, the same user ID in bothTELEGRAM_ALLOWED_USERS, one gateway withGATEWAY_MULTIPLEX_PROFILES=1. DM bot A, then bot B, and ask each to identify itself — each now answers as its own profile.A note on the suggested fix
The issue proposes fixing this in SQL — adding
AND COALESCE(profile_name,'') = COALESCE(?,'')to the fallback predicate plusprofile_nameinidx_sessions_gateway_peer. I went with the gateway-layer guard instead, for three reasons:sessions.profile_nameis nullable and is written by several unrelated paths (run_agent.py,tui_gateway/server.py,agent/conversation_compression.py) that each supply their own value. A strictCOALESCEequality would silently stop recovering rows whoseprofile_nameis NULL — including rows written before the column was populated — which changes behavior for single-profile users who are not affected by this bug.agent:<profile>:…) and is what the exact-match query uses, so comparing on it keeps one source of truth.Happy to add the DB-level predicate as defense-in-depth on top of this if you'd prefer belt-and-braces — say the word and I'll push it here rather than open a second PR.
Checklist
Code
pytest tests/ -qand all tests pass — see note belowNote on the full suite: I ran
scripts/run_tests.sh tests/gateway/ -q(CI parity, per-file isolation) rather than the whole tree, plusruff checkon both touched files andty check gateway/session.py.tyreports 16 diagnostics ongateway/session.py, but that is the pre-existing count — I verified it is identical with my changes stashed, so this PR adds none. I didn't claim a full-tree pass because this checkout has baseline failures unrelated to this change (e.g. #74358 —tests/hermes_cli/exits early viaos._exit) and some suites need optional extras I don't have installed. Happy to run anything else you'd like.Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — N/A (no config keys changed)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A