fix(gateway): scope fallback session lookup to profile in multiplex mode (#74285) - #75043
webtecnica wants to merge 1 commit into
Conversation
…query Root cause: `find_latest_gateway_session_for_peer()` in hermes_state.py has a fallback SQL query (peer-tuple match) that did not include profile_name in the WHERE clause. In a multiplexed gateway, this allowed the fallback to return a session row from a sibling profile, causing DM routing to the wrong profile. Fix: - Add `profile_name` keyword-only parameter to find_latest_gateway_session_for_peer() - Add AND COALESCE(profile_name, '') = COALESCE(?, '') to the fallback SQL WHERE clause - Pass profile_name=source.profile from the sole caller SessionStore._find_gateway_session_row() in gateway/session.py The primary session_key-based query was already correctly scoped because build_session_key() includes the profile in the key.
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing this to the durable recovery fallback. The premise is confirmed on current main: hermes_state.py:3052-3072 falls back on the peer tuple without profile_name, and gateway/session.py:1547-1555 deliberately permits recovered rows in multiplex mode.
Problems
- The PR has no regression coverage. Current
tests/test_hermes_state.py:2437-2461covers only a single-profile recovery row, not two same-peer rows differentiated byprofile_name. This is important becausegateway/session.py:2425persists that value as the durable isolation key.
Suggested changes
- Add a real-
SessionDBtest with identical peer tuples across two profiles and assert fallback recovery returns only the requested profile. Also cover theSessionStoreplumbing atgateway/session.py:1689-1696, so a future caller omission cannot reintroduce the issue.
This is an automated hermes-sweeper review.
| chat_id=source.chat_id if allow_peer_fallback else None, | ||
| chat_type=source.chat_type if allow_peer_fallback else None, | ||
| thread_id=source.thread_id, | ||
| profile_name=source.profile, |
There was a problem hiding this comment.
Please add a regression test for this plumbing and the SQL predicate: create two recoverable rows with the same peer tuple but different profile_name values, then prove recovery for this source selects only its profile's row.
alt-glitch
added
type/bug
teknium1
added
sweeper:risk-security-boundary
SummaryThree PRs address #74285's cross-profile session-recovery defect: #74153 combines profile filtering with a guard and routing cleanup, #74593 scopes candidate selection by exact session-key namespace and rejects ambiguous multiplex ownership, and #75043 provides the minimal profile_name-based SQL fix. Related pull requests
Duplicates#74153, #74593, and #75043 target the same peer-fallback isolation defect; #74153 and #75043 are substantially subsumed by #74593. Suggested consolidationKeep #74593 open with a salvage path: independently verify its exact-prefix filtering before LIMIT 1, fail-closed handling of ownership-ambiguous multiplex rows, and real-SessionDB regression coverage. Then close #74153 and #75043 as duplicates of #74593; this explicitly departs from their keep_open reviews because #74153 retains the documented default-profile/test defects, while #75043's valid best-fix SQL change is fully represented in #74593 together with the regression coverage its review requires. 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
P75043 -->|best fix| I74285
class I74285 open
class P74153 open
class P74593 open
class P75043 open
class P74593 best
class P75043 best
class P75043 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. |
|
The peer-fallback fence this PR proposed is now structural: PR-1/PR-3 (#115665 |
This branch has not been deployed
4 participants
Fixes #74285 — fallback session query now includes profile_name in WHERE clause. Primary query was already scoped (session_key embeds profile), but the fallback returned sibling profile's session.