fix(gateway): add trusted sender envelope for shared sessions - #69980
fix(gateway): add trusted sender envelope for shared sessions#69980xiaoyaner0201 wants to merge 26 commits into
Conversation
3cc6e75 to
be01c1f
Compare
|
Rebased onto current What drifted:
Verified on the rebased tree with the canonical runner: That includes the upstream event-loop test that caused the conflict, so the merge On the triage note about aggregation: it's correct, and I've reproduced it. Detail is in I'm working that fix on a separate chain and will send it as its own PR rather than growing |
…search#69961) merge_pending_message_event folds a follow-up event into the pending slot without consulting event.source, so a second participant's text/media is absorbed into the first participant's turn and rendered under the FIRST sender's [Verified sender: ...] envelope in shared multi-user sessions. Adds 14 behavioural tests asserting the invariant that every fragment of a pending turn must originate from the sender named by that turn's source, and that refusing to merge must never cost a message. Tests only — no production change; the guard implementation lands separately. 7 of 14 fail on real assertions at this head.
…search#69961) In a shared group session several senders share one session_key, so merge_pending_message_event could splice one human's text or photos into another human's pending turn. The combined turn was then rendered under a single [Verified sender: ...] envelope, claiming an author it did not have. Add module-level pending_merge_sender_conflict(existing, event), resolving identity as source.user_id_alt or source.user_id — the same precedence _can_merge_text_debounce_events already uses for the debounce buffer. A conflict is reported only when BOTH sides resolve and differ; a missing identity on either side means "unknown", never "different", so a refusal can never cost a message. merge_pending_message_event now returns bool: True = absorbed, False = refused. On refusal the caller owns the event and must give it its own turn. _queue_or_replace_pending_event falls through to the FIFO; adapter-level sites with no FIFO park the event via _defer_refused_pending_event, bounded by _REFUSED_PENDING_MAX, and get_pending_message promotes it once the slot drains. Debounce-flush call sites are same-sender by construction and left unchanged. The guard is unreachable in per-user/DM sessions: build_session_key appends user_id_alt or user_id under isolate_user, so distinct senders cannot share a session_key. No config surface, no envelope format change.
|
Thanks for narrowing this to shared sessions and preserving the existing Slack mention target. Problems
Suggested changes
This is an automated hermes-sweeper review. |
|
Thanks for the review — the blocking issue is now fixed in this PR rather than deferred, and the scope note has been withdrawn from the description. Head: On the blocking findingYou were right that the verified claim was unsound:
The predicate is intentionally conservative: What the refusal path had to get rightRefusing the merge is the easy half; not losing the refused message is the hard half. Iterative review turned up several ways it could still be dropped, each now covered:
Requested regressionsPHOTO+TEXT and PHOTO+PHOTO cross-sender cases both assert separate source-attributed model turns. Also added: asymmetric VerificationThe 25 full-suite failures are a pre-existing baseline unrelated to this change ( Happy to split the refusal-plumbing commits out of the envelope change if you'd prefer to review them separately. |
Resolves the only conflict, in tests/gateway/test_shared_group_sender_prefix.py. Upstream 3997561 (test prune wave 2) deleted three preprocess tests from that file while this branch appended the cross-sender aggregation suite to the same region. Resolution accepts upstream deletions verbatim and keeps every test this branch added; no assertion was weakened or rewritten.
|
Follow-up:
What conflicted: exactly one file, Re-verified on the merged tree The 12 remaining full-suite failures are a strict subset of the pre-merge baseline (the prune removed some of the previously failing cases): |
|
Post-publication exact-tree review found two additional loss/order defects in the blocking aggregation repair; both are now fixed by additive fast-forward commits and independently re-reviewed. Published repair
The first defect was interleaved FIFO ordering: A1 (Alice), B1 (Bob), A2 (Alice) could drain as A1, A2, B1 because A2 merged backward into the occupied head after B1 had entered overflow. Overflow is now authoritative: later arrivals append or merge only at the true tail, preserving A1, B1, A2 while retaining contiguous same-sender album aggregation. The second defect was silent loss of a PHOTO-typed event with Verification
Current upstream |
SummaryTwo PRs address Issue #69961. PR #69963 adds the shared-session trusted sender envelope, Slack mention preservation, forged-header stripping, and DM quietness; PR #69980 carries that envelope work forward and additionally repairs cross-sender pending aggregation, FIFO ordering, failed-media retention, and related persistence and regression coverage. Related pull requests
Duplicates#69963 is the closed predecessor of #69980 and is superseded by it; #69980 contains the overlapping sender-envelope change and extends it with the cross-sender aggregation and FIFO repairs. Suggested consolidationKeep #69980 open with a salvage path: preserve the trusted-envelope and lossless cross-sender FIFO changes, then obtain maintainer review of the 7-file/+1600/-58 implementation; do not recommend merging because no merge-lane best-fix finding is recorded. Close #69963 as duplicate of #69980, consistent with its documented supersession and the structural close edge. 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
I69961(["issue #69961 (open)"])
subgraph Dup69963 ["PRs duplicating each other"]
P69963["PR #69963 (closed)"]
P69980["PR #69980 (open)"]
end
P69980 -->|best fix| I69961
class I69961 open
class P69963 closed
class P69980 open
class P69963 best
class P69980 best
class P69980 target
click I69961 "https://github.com/NousResearch/hermes-agent/issues/69961"
click P69963 "https://github.com/NousResearch/hermes-agent/pull/69963"
click P69980 "https://github.com/NousResearch/hermes-agent/pull/69980"
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 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 92 kB of PR diffs, 9 kB of issue/PR text, 19 kB of discussion (11 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
|
Published the latest independently reviewed repair/reconciliation tip for #69961. Exact public artifact
This final repair chain additionally covers stable-ID precedence and session-aware, retry-safe FIFO shutdown recovery: real adapter-head and overflow events retain session/source attribution, recover in Verification
Upstream |
Summary
source.user_id/source.user_id_alt<@UID>mention target while extending trusted sender IDs to Discord/Telegram-style shared sessionsScope update — blocking review resolved
The earlier revision deferred cross-sender batching to #69961. Per the blocking review on this PR, the sender-identity guard now lands with this change; the scope note is withdrawn.
merge_pending_message_eventnow returnsbool:Truewhen the event was absorbed,Falsewhen the merge was refused because the pending slot belongs to a different sender (pending_merge_sender_conflict). AFalsereturn transfers ownership back to the caller, which must give the event its own turn. Every call site was audited and updated; refused events go to the runner's canonical per-session FIFO (_enqueue_fifo), never to a parallel store.The guard is deliberately conservative.
user_id_altanduser_idare compared independently; a match on either proves the same sender, and a conflict is reported only when at least one comparable pair exists and all comparable pairs differ. Missing identity means "unknown", so merging behaves exactly as before. Same-sender albums, photo bursts, and text follow-ups are unchanged. In per-user/DM sessions the guard is unreachable by construction, sincebuild_session_keyalready appends the sender ID underisolate_user.Behavior beyond the core guard, driven by review iterations:
media_urls=[]/stop,/new, and/resetclear both pending tiers (adapter head slot and runner FIFO), so refused work is not left executable after an interruptflush_queued_events_to_file, preserving FIFO order, session key, and JSON-safeSessionSourceattributionTest Plan
Final published and fresh-context Sol-reviewed head
edf643f638c6a89f16c43106fd52adf7ffd915ef, tree0efc104532cc4643cd3035ddb473cb9c5f181549, additively reconciled with upstreammainat43717123ca1566a073270c5a61431e2e0e4a0211:FIRST → SECOND → THIRDwith session identity/source and no consumed files left behindorigin/mainarchive (1 shutdown-forensics diagnostic-spawn failure and 4 local DNS/proxy-sensitive media-download failures)git diff --check: passedgpt-5.6-sol/openai-codex: PASS with 0 P0 / 0 P1 / 0 P2Publication was a normal non-force fast-forward from
f0ec41764ad7611c6b8f057878b56a264084faf8to the exact reviewed tip. Git fetch,ls-remote, and GitHub PR read-back all returned the same final head.After publication, upstream
mainadvanced by 4 disjoint commits to36cb5ae5530a75def7df3195e49b7a4aa2add482. Those commits touch 14 paths with no overlap with this PR's 11 paths,git merge-tree --write-tree origin/main HEADis clean, and GitHub still reports the PR as mergeable. The exact-tree review remains anchored to the published tree and its merged base above; the later disjoint drift is reported separately rather than silently presented as reviewed.Acceptance status
edf643f638/0efc104532+2516/-85, touching gateway/session-state/message-delivery and shutdown recovery; this requires one-to-one maintainer review rather than low-risk batch handlingRefs #69961
Supersedes #69963 because GitHub did not synchronize that PR object after the fork branch advanced past
a09e6db230b064d61c7d34b1630bce0e09872f51; the fork ref is current at this replacement branch.