Repository navigation
Conversation
…t and user labels The relay text lane gets chat_name, chat_topic and user_display_name from the connector; a forwarded Discord interaction is the raw body and was built with the raw username and no chat labels. Both land in one session and the pinned session-context prompt renders those labels, so a relayed slash command re-rendered the cached prefix and the next message rendered it back. Remember the text lane's labels per chat and user and use them on the interaction lane; with none seen yet, name the user by the text lane's contract (guild nick, else global name, else username).
|
Independent verification on the PR head (40322be): relay-labels suite 4/4 green on Linux. Relayed Discord interactions carrying the text lane's chat and user labels is intact. No findings. |
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Premise confirmed on origin/main @ 82bbc694. _event_from_wire gives the text lane user_name = user_display_name or user_name or user_handle plus connector-resolved chat_name/chat_topic; _discord_interaction_to_event built user_name from the raw username with no chat labels. All three are in _ephemeral_change_key (gateway/run_agent_cache.py), so a relayed slash/button turn re-renders the pinned session-context prompt and the next message renders it back. Same class as #128722 (Slack) and #128788 (native Discord); this is the third and last lane of it that I can find.
Fix direction is right. The interaction body carries only ids (no guild name), so the Server / #channel label cannot be derived from it; remembering what the text lane last carried per (guild, channel) / (guild, user) is the only agent-side source. Bounded FIFO via the existing _evict_oldest, stored after the replay-dedupe gate, keyed on str() of the same fields both lanes stamp. Display-name fallback (nick > global_name > username) matches discord.py's display_name order. Session-key fields untouched, and _on_passthrough still runs _capture_scope + the prompt-token consume gate before handle_message.
Verified locally: test file red on base (3 failed: assert 2 == 1, 'ben' == 'Benny', 'ben' == 'Ben D'), 4 passed on head; all 46 tests/gateway/relay/ files 390 passed / 0 failed; ruff and the Windows-footgun check clean.
One non-blocking residual inline (threads). Approving on correctness; merge is Teknium's call.
| @@ -1180,7 +1208,9 @@ def _discord_interaction_to_event(self, forward): | |||
| # native Discord adapter key guild channels as "group". | |||
| chat_type="group" if guild_id else "dm", | |||
There was a problem hiding this comment.
Non-blocking residual, same family: the interaction lane still never sets thread_id / parent_chat_id, both of which are in the Discord tuple of _ephemeral_change_key (and thread_id is in the session-key _ALWAYS_FIELDS). So a slash command inside a thread still diverges from the text lane there. The interaction body does carry enough to close it: channel.type in (11, 12) marks a thread and channel.parent_id is the parent, so thread_id=channel_id, parent_chat_id=parent_id can be derived without any remembered state. Fine as a follow-up; noting it so the class is closed knowingly rather than by omission.
There was a problem hiding this comment.
Closed in 6aad417 rather than left as a follow-up, since the divergence is a session split, not only a pin flip. On the previous head, with the text lane's thread message keyed per the contract (chat_id = thread_id = the thread channel, chat_type: thread):
text key= agent:main:discord:thread:th1:th1
slash key= agent:main:discord:group:th1:u1
so a slash command inside a relayed thread ran in a per-user session beside the thread's shared one. The interaction lane now derives chat_type: thread, thread_id = channel_id and parent_chat_id = channel.parent_id when the partial channel.type is 10, 11 or 12 (announcement threads included, which is what discord.Thread covers natively). No remembered state, as you noted. A body without the channel object (or a DM) keys exactly as before.
One dependency, stated in the contract doc (§3.1): the session source mirrors the connector's interactionSessionSource, which binds the discord.interaction_token capability, so the connector's binding for a thread interaction has to derive the same fields before a follow_up caller lands. There is no in-tree caller of send_follow_up today (only tests), so nothing changes at runtime until then.
Test: the thread row of the parity test asserts the two session keys are equal and the pinned prompt stays one across message → slash → message. Red on cd581c70 with the key diff above, green now; the 46 tests/gateway/relay/ files stay green.
| await self.handle_message(event) | ||
|
|
||
| _SEEN_INBOUND_MAX = 512 | ||
| _DISCORD_LABELS_MAX = 2048 |
There was a problem hiding this comment.
Nit: 2048 per map is generous but cheap (a tuple of two short strings). The only behavioral cost of a miss is one extra re-render on the first interaction after eviction/restart, which is the same as today. No change requested.
…ls from the session origin The label maps live in memory, so after a gateway restart a slash command or component that was the first event in a chat rendered the session-context prompt without chat_name/chat_topic, missing the provider cache the pre-restart turns built; the next message rendered it back. On a miss, seed the chat labels from the persisted session origin, which the text lane wrote.
…s session like the text lane The text lane keys a message inside a Discord thread on chat_type "thread" + thread_id (both session-key fields) with parent_chat_id from the connector; the interaction lane built chat_type "group" with no thread or parent, so a slash command sent inside a thread landed in a per-user group session beside the thread's shared one, and the pinned session-context prompt differed on chat_type, thread and parent. Derive the three from the interaction's partial channel object (type 10/11/12 marks a thread, parent_id names the parent), the same fields the native slash path takes from interaction.channel. Docs: relay-connector-contract.md states the derivation and that the connector's capability binding for a thread interaction must match it (no in-tree caller of send_follow_up today). No zh-Hans mirror of that page.
|
Two commits since the reviewed head 40322be, both on the same lane-parity class:
The 2048-entry maps stay as they are. The body's Changes Made / How to Test are updated for both. |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Request changes conclusion
GitHub blocks a formal request-changes event because this account does not have explicit repository review access. The technical conclusion is still request changes.
Reviewed head: 6aad417bee1a19b0c6a46b1936c7a4bcc3635e3b
Motivation
This PR addresses a real prompt-cache stability bug in relayed Discord interactions. Text messages carry connector-resolved chat labels and display names, while raw interaction bodies do not. Without parity, a slash command or component can rebuild the pinned session prompt with different metadata.
Approach
The adapter now keeps bounded chat and user label caches. It restores chat labels from the session store after restart, applies Discord's display-name fallback order, and maps thread interactions to the same session-key fields as text messages.
Changed surfaces
gateway/relay/adapter.pyadds the label caches, session-origin fallback, display-name fallback, and thread field derivation.tests/gateway/relay/test_relay_discord_interaction_labels.pycovers warm, restart, thread, and user-name cases.website/docs/developer-guide/relay-connector-contract.mddocuments the thread interaction contract.
The warm cache path, user-name fallback, and thread session-key change are consistent with the existing relay contracts. The earlier thread-key finding is fixed on this head.
Risk to main
One restart path is still incomplete. A reused session keeps its creation-time origin, so a Discord channel rename updates the live label cache but not the labels used by the restart fallback. After restart, the first interaction gets the old name and topic. The next text message gets the new values, which changes the pinned prompt again. The inline finding has the reproduction and impact.
The branch is behind current main, but the merge composition is clean. GitHub reports no hosted checks for this head.
Validation
- Read the full three-file diff and the complete changed files.
- Rechecked the exact remote head, author, merge state, prior reviews, and inline comments before publication.
- Ran the new test file and the adjacent relay interaction suite on the exact head. Result: 23 passed.
- Ran the same 23 tests on the clean composition with current
main. Result: 23 passed. - Ran a direct rename and restart probe on both versions. The interaction restored the old chat name and topic instead of the latest text-lane labels.
Overall judgment
The main design is sound, but the persisted fallback does not yet preserve its stated invariant after chat metadata changes. I am requesting changes for that focused restart case.
| logger.debug("relay: session origins unreadable for Discord labels", exc_info=True) | ||
| origins = [] | ||
| if origins: | ||
| newest = max(origins, key=lambda e: e.created_at).origin |
There was a problem hiding this comment.
[Bug] (blocking) The restart fallback can restore an outdated chat name and topic. SessionEntry.origin is captured when a session is created. Reusing the same session in get_or_create_session() advances updated_at, but it does not replace origin. If Discord channel labels change, the next text message updates the live adapter cache only. After a restart, this line selects the same session and reads its original labels. The next slash command gets the old channel metadata, then the next text message gets the new metadata, so the pinned prompt flips again.
I reproduced this on 6aad417bee1a19b0c6a46b1936c7a4bcc3635e3b and on its clean composition with current main: create a session with old labels, reuse it with renamed labels, restart, then convert an interaction. The restored interaction contained the old labels. The six added cases still pass because the restart case never changes labels before the restart. Please cover this sequence and make the persisted fallback return the latest text-lane labels.
There was a problem hiding this comment.
Confirmed and fixed in 0f3aa869.
Reproduced on 6aad417b with the sequence you describe (create with old labels, reuse with renamed labels, restart, convert an interaction): the reuse path in get_or_create_session only advances updated_at, and reset_session inherits the old origin, so the fallback returned the creation-time name and topic. New rename row: assert 2 == 1 on 6aad417b.
Fix: when the text lane's labels for a chat are new to the process or changed, the adapter calls SessionStore.refresh_origin_labels(source), which writes chat_name / chat_topic onto the origin of every session in that chat (same platform, scope and chat id). Every session rather than the sender's, because a guild channel has one session per user and the rename can arrive on someone else's message; the peer-rename row covers that plus a reset before the restart, and fails alone when the refresh is limited to the sender's session. Nothing is written when the labels already match.
tests/gateway/relay/ plus the session-store suites: 512 passed. Body updated (Changes Made, How to Test, and a comparison with #129032).
…interaction lane falls back to A session origin is written when the session is created and inherited by a reset; reusing the session never touched it. After a channel rename the text lane carried the new chat_name/chat_topic, but the restart fallback still read the creation-time labels, so the first slash command after a restart rendered the old ones and the next message rendered the new ones back. When the text lane's labels for a chat are new to the process or changed, refresh them on the origin of every session in that chat (a group chat has one session per user, and the rename may arrive on another user's message). Found in review by ehz0ah. Docs: N/A, no page describes how origin labels are persisted.
There was a problem hiding this comment.
Conclusion
Reviewed 0f3aa8696de5fe7ae9302ce44884c861319d7dad. The warm interaction path and the ordinary rename/reset path are improved, but I would fix the four P2 issues below before merging. Three leave the stated label-parity guarantee incomplete across cache/store lifetimes; the fourth adds synchronous whole-index persistence to inbound message handling.
Motivation and implementation
#128796 identifies a real mismatch: the text lane carries connector-resolved chat labels and display names, while the forwarded interaction carries a raw Discord body. These fields participate in the pinned session-context prompt, so differing observations can change the prefix between message → interaction → message.
The four-file change remembers labels after inbound replay deduplication, restores chat labels from matching Discord session origins, derives thread routing fields from the partial channel object, and refreshes every matching origin when text labels change. The scope/platform/chat filtering and preservation of the rest of each origin are appropriate. The contract documentation also makes the external connector's thread capability binding explicit; I have not validated an external connector deployment.
Remaining correctness and operational risks
Cold lookup can suppress durable reconciliation. The chat cache serves both as a lookup result and as evidence that all persisted origins were refreshed. Those facts differ. With pre-upgrade mixed origins, a slash-first lookup selects current labels from a newer user's session. A subsequent text message with those same labels skips refreshing an older user's origin. Resetting that older session gives its stale origin a newer creation time, and a further restart restores the old channel name/topic. This is a distinct interleaving from the ordinary rename finding already addressed. Separate lookup caching from successful text-driven reconciliation and retain the current platform/scope/chat restrictions.
User-label parity still depends on cache lifetime. The accepted warm fixture deliberately has text user_display_name="Ben D" and interaction member.nick="Benny"; warm conversion chooses Ben D. Persisting that text session and restarting, or evicting its user-cache entry with 2048 distinct users, makes the identical interaction choose Benny. The real rendered prompt differs in its User field. The current restart fixtures change the nick to match the text label, hiding this case. Recover the last authoritative text observation for the matching user with an explicit freshness policy, and test restart/eviction without changing the payload. This is prompt-label instability, not a demonstrated session-key split.
A failed origin read becomes a successful negative cache entry. A single list_sessions() exception stores (None, None). A second interaction does not call the now-working store, so labels remain absent until another text observation, eviction, or restart. Gracefully processing the failed-read interaction is useful, but the failure should remain retryable. Keep failures distinguishable from an authoritative empty result and cover recovery with two interactions and no intervening text.
Origin refresh writes on the event loop while holding the entry lock. The new synchronous call from _on_inbound reaches _update_all_entries_locked → _save, which persists the whole routing index and legacy JSON mirror. Recording a real os.fsync during a changed-origin refresh showed both the gateway event-loop thread and store._lock held. The relay reader awaits this callback, so slow storage blocks event-loop progress and delays inbound completion/acknowledgment; other store consumers also wait on the entry lock. Snapshot under the entry lock, persist after releasing it with existing generation ordering, and await an offloaded store operation before dispatch. This evidence establishes the blocking execution path; it does not measure a production outage or latency.
Earlier finding reconciliation
The earlier missing-thread-fields finding is addressed by the current derivation and the passing thread parity case. The previous ordinary rename/reuse/restart finding is also repaired: my independent probe returns old labels on 6aad417b and current labels on this head. The added peer-rename case passes. Those repaired paths are not being repeated as open findings; the slash-first mixed-origin sequence above exercises a different gate.
Validation and limits
On Linux, Python 3.14.7, using the repository runner with its clean environment and per-file isolation:
HERMES_PYTHON=/path/to/python3.14 HERMES_TEST_WORKERS=4 scripts/run_tests.sh \
tests/gateway/relay/ tests/gateway/test_session.py \
tests/gateway/test_session_identity.py tests/gateway/test_session_identity_restore.py \
tests/gateway/test_session_store_lock_io.py tests/gateway/test_session_recovery.py \
tests/gateway/test_restart_resume_pending.py tests/gateway/test_session_store_prune.py- Exact head, committed tests: 512 passed, 0 failed, including all eight new cases.
- Six independent lifecycle probe cases: five failed, one passed. Restart and eviction are two cases for the same user-label finding; ordinary rename is the passing control. Each inline comment gives its setup, observed result, and regression obligation.
- Clean patch composition with
main@f42f579cf8bac4918ac9599bece71618afadd846: the added eight cases and 12 adjacent passthrough/store-lock cases pass; all five counterexamples persist. - The synchronous-fsync probe passes on merge base
38d3dbce(no write through this path) and fails on this head.
The probes use the real wire decoder, interaction converter, SessionStore with SQLite/file persistence, and prompt renderer. Final agent dispatch is replaced by AsyncMock; the transient read fault is deliberately injected, while the successful read and fsync remain real. I did not run a live Discord conversation, model request, full repository suite, or Windows/macOS validation.
Hosted CI is not established as green for the four-commit series: the initial commit has queued checks, the next has a startup-failed workflow, and the last two have workflows concluded action_required. The reviewed head reports zero check runs and an empty combined-status list. Local test success does not substitute for those hosted checks.
| if source.chat_name or source.chat_topic: | ||
| key, labels = (scope, str(source.chat_id)), (source.chat_name, source.chat_topic) | ||
| store = getattr(self, "_session_store", None) | ||
| if store is not None and self._discord_chat_labels.get(key) != labels: |
There was a problem hiding this comment.
Follow-up status: Rechecked at fa9259fdc1346c8d2ba5c1bd052358b559777789: the origin-selection mechanism below has been removed. The new per-chat observation record addresses reset/creation ordering. Older origins are deliberately not back-filled; that upgrade limitation is now documented.
[P2] Do not treat lookup-filled cache labels as reconciled origins
A cold interaction populates this map in _discord_chat_labels_for, without refreshing any origins. Equality here therefore does not prove that every stored origin already carries these labels.
Reproduction on this head: create and persist two group sessions in the same Discord guild/channel: u1 with ("Old server / #ops", "Old topic"), then u2 with ("New server / #ops", "New topic"). Reload the store and adapter. A slash-first conversion correctly picks u2's new labels. Deliver u2's current text event through _on_inbound, then reuse its session through get_or_create_session. This condition skips the refresh; inspecting u1 still shows the old origin. Reset u1, reload again, and convert another slash: actual old name/topic; expected new name/topic. Reset makes the stale origin newest by created_at, so the cold fallback selects it.
This is reachable with the mixed creation-time origins left by an older gateway and restores the prompt-label flip despite a current text observation. The simpler text-first rename path now passes; this is the lookup-first residual.
Track successful durable reconciliation separately from lookup results. The first authoritative text observation should reconcile matching origins even if a lookup already cached identical labels, and a failed refresh should remain retryable. Add this exact mixed-origin → slash → same-label text → stale-peer reset → restart regression, asserting both every matching origin and the restored prompt; preserve platform/scope/chat isolation.
There was a problem hiding this comment.
Confirmed by reading the gate: equality with the map did not prove the origins were reconciled. Fixed in fa9259fd by removing the thing that needed reconciling.
The interaction lane no longer reads session origins at all. The store records the chat's labels when the text lane carries them (record_chat_labels, one state_meta row per chat), and the adapter tracks "what the store is known to hold" in a map separate from the lookup map, so a lookup-filled entry never suppresses a write and a failed write stays retryable on the next message. Your mixed-origin sequence now ends on the record the text message wrote: resetting u1 changes an origin nothing reads.
One behaviour change to note: origins written by an older gateway are not back-filled, so in that sequence the slash-first conversion has no labels until the first message of this version records them.
| scope, user_id = str(guild_id or ""), str(user.get("id") or "") | ||
| # The text lane's user_display_name is the native author.display_name (guild nick, else | ||
| # global name, else username); prefer what that lane last carried for this user. | ||
| user_name = self._discord_user_labels.get((scope, user_id)) or next( |
There was a problem hiding this comment.
Follow-up status: Rechecked at fa9259fdc1346c8d2ba5c1bd052358b559777789: addressed by changing the policy to current interaction identity (nick → global name → username) and removing the user cache. The old fixture's differing names represent a nickname change; persisting its stale text name would contradict the new policy. This finding is closed under that policy.
[P2] Preserve text-lane user labels across restart and eviction
Only chat labels get a persisted fallback. Once this user map is empty, the same interaction switches back to its raw nickname even when a matching persisted text origin contains the label the warm path chose.
Reproduction on this head: use the existing warm-case inputs, text user_display_name="Ben D" and raw interaction member.nick="Benny", for the same guild/channel/user. Deliver the text through _on_inbound and persist its session. Warm conversion returns Ben D. Reload the store and create a fresh adapter without changing the interaction body: actual Benny; expected the previously chosen Ben D under this PR's warm parity policy. The real session-context renderer differs in its User field. Keeping the adapter alive and admitting 2048 distinct users through the text lane reproduces the same result through FIFO eviction.
The restart rows currently use nick="Ben D", so they hide the mismatch already permitted by the warm fixture. Repeated interactions retain the raw label until another text observation restores the cached label, recreating message/interaction prompt changes.
Recover the matching user's last authoritative text label on a miss, with an explicit freshness policy for genuine name changes and guild/user isolation. Add restart and user-map eviction cases that keep the warm fixture's differing names unchanged and assert one rendered prompt. Routing IDs remain equal in this probe; this finding concerns label parity.
There was a problem hiding this comment.
Reproduced the dependence on cache lifetime. I resolved it in the other direction, because your /new nickname finding on #128796 pulls against persisting the text-lane name: a remembered name outlives a nickname change, whatever freshness policy sits on it.
fa9259fd drops the user-label map. The interaction names its user from its own member.nick → user.global_name → user.username, which is what the text lane's user_display_name (discord.py display_name) resolves to at the same moment. Warm, restarted and evicted processes now give the same answer by construction, and your nickname probe passes (New Nick on both turns, one prompt).
As you noted on the issue, the old warm fixture paired Ben D with a fresh Benny, which is a name change rather than a stationary name. The prompt cases now use compatible snapshots, and the second test pins the current-identity rule after an earlier message carried another name. The contract doc now says the connector's user_display_name must resolve the same way.
| if origins: | ||
| newest = max(origins, key=lambda e: e.created_at).origin | ||
| labels = (newest.chat_name, newest.chat_topic) | ||
| self._discord_chat_labels[key] = labels |
There was a problem hiding this comment.
Follow-up status: Rechecked at fa9259fdc1346c8d2ba5c1bd052358b559777789: failed chat_labels reads now return without memoizing the failure. The committed read-retry case passes. The original failed-origin-read mechanism below is removed.
[P2] Leave failed origin lookups eligible for retry
The exception branch sets origins=[] and falls through to this assignment. That converts an unsuccessful read into a memoized (None, None), so the key not in ... guard prevents every subsequent interaction from trying the recovered store.
Reproduction on this head: persist a real labelled Discord session, reload it, then wrap list_sessions so the first call raises OSError and subsequent calls delegate to the real method. Convert two interactions for that same guild/channel with no text between them. The first correctly survives with missing labels. On the second, actual read count 1 and labels (None, None); expected a second read and the persisted current labels. The exception is the only injected behavior; the store, saved origin, and successful read are real.
This makes a transient read failure affect later healthy turns until a text event, eviction, or process restart repairs the cache. Those turns render a different prefix from the persisted text lane.
Do not memoize a read exception as an authoritative absence. Keep failed reads retryable, or represent failure separately with a bounded retry policy. Add the two-interaction recovery case and verify restored chat name/topic and prompt parity without relying on another text message.
There was a problem hiding this comment.
Confirmed: the exception branch fell through to the memoizing assignment. Fixed in fa9259fd: a failed read returns no labels for that interaction and caches nothing, so the next interaction reads again. SessionStore.chat_labels raises on an unreadable store and returns None only for "never recorded", so the two are distinguishable.
Covered by the read-retry case: the first read after a restart raises, that conversion has no labels, the second conversion has the recorded ones and the prompt matches the text lane's. Caching the failure instead turns that case red (assert 2 == 1).
| entry.origin = replace_source(origin, chat_name=labels[0], chat_topic=labels[1]) | ||
| return True | ||
|
|
||
| return self._update_all_entries_locked(_refresh) |
There was a problem hiding this comment.
Follow-up status: Rechecked at fa9259fdc1346c8d2ba5c1bd052358b559777789: the bulk origin refresh and index save below are removed. The new single-row record write is awaited through asyncio.to_thread; all ten committed label/identity tests pass. This original write-side finding is addressed. Cold read-side locking is being examined separately.
[P2] Keep label refresh persistence off the inbound event loop and entry lock
This helper saves the entire routing index while self._lock is held. The new caller, RelayAdapter._remember_discord_labels, invokes it synchronously from async _on_inbound, before message dispatch. The save acquires the persistence lock, replaces routing rows, and writes/fsyncs the legacy JSON mirror.
Reproduction on this head: create a real store session with old channel labels, install it on a Discord relay adapter, and deliver an event with new labels through await adapter._on_inbound(event). Wrap os.fsync to record threading.get_ident() == event_loop_thread and store._lock.locked(), while still calling the real fsync. Observed [(True, True)]. The same probe on the merge base records no fsync through this path. No artificial storage delay is needed to establish where the blocking write runs.
The transport reader awaits this callback before acknowledging the inbound delivery. Slow storage therefore stalls the gateway event loop during what is now routine label learning after a cold start/rename, while unrelated entry access also waits on the lock.
Capture the mutation and immutable snapshot under the entry lock, release it before persistence, and preserve the existing routing-generation/stale-write protections. Await an offloaded refresh using the async-store pattern before dispatch, so ordering remains deterministic. Add a regression requiring disk writes off the loop and outside _lock; a controlled blocked write should still allow an unrelated event-loop heartbeat to run.
There was a problem hiding this comment.
Confirmed: _update_all_entries_locked → _save ran on the loop thread under _lock. Fixed in fa9259fd; this helper is removed and session_lifecycle.py is back to main.
What replaced it is a single-row state_meta upsert (record_chat_labels) that takes no entry lock and saves no routing index. The adapter awaits it through asyncio.to_thread before dispatch, only when the chat's labels are new or changed; the reader delivers events one at a time, so records land in observation order.
Every case of the prompt test asserts the write happened and not on the loop thread. Making the call synchronous turns warm / thread red on that assertion.
…tion's user from its own payload The interaction lane took a restarted gateway's chat labels from the newest session origin of the chat. An origin is written when its session is created and inherited by a reset, so creation order stood in for observation order: a delayed first turn (older message, session created after a rename was observed) or a reset peer put the old labels back after a restart. The session store now records (chat_name, chat_topic) per chat at the moment the text lane carries them (one state_meta row, written off the event loop and only when new or changed), and the interaction lane reads that record. The all-origins refresh and its whole-index save under the entry lock are gone. A failed read is not cached, so the next interaction asks again. The user name is derived from the interaction's member/user (guild nick, else global name, else username), which is what the text lane's user_display_name resolves to at the same moment. The remembered text-lane name outlived a nickname change and depended on cache lifetime.
|
@andrexibiza thanks for both reviews (this PR and the comparison on #128796). All six findings reproduced or confirmed on What changed Four of the six shared one root: the interaction lane used session origins, ordered by
Your published probes (three files from #128796, unchanged): Declined / limits, stated in the body
Tests stay at two functions (10 parametrized cases). Red/green per head, the sabotage runs and the updated "Compared with #129032" table are in the PR body. Hosted CI on this fork PR is still waiting for a maintainer's approval to run. |
There was a problem hiding this comment.
Follow-up review at fa9259fdc1346c8d2ba5c1bd052358b559777789, after the new fifth commit, also tested as a clean composition onto upstream 8d30c4eaabd85edb77a02fef6c5388d9344ef80c. This is the current-head review; the earlier review was anchored to 0f3aa869. Its four original inline comments now acknowledge the corresponding fixes.
The revised design addresses the original origin-ordering, user-cache-lifetime, failed-read retry, and synchronous bulk-write findings. Observation-time state independent of session creation/reset is the right authority. Current interaction identity also removes the stale-name policy conflict.
Four verified P2 findings remain, detailed inline: cold reads still block the event loop behind a database writer; shared durable observations do not invalidate peer adapters' positive or negative caches; partial thread payloads lose known parents and disagree with parent-based routing; and message identity remains inconsistent between the wire frame, interaction, SessionSource, MessageEvent and runtime context. The last two include existing manifestations of the repair class. Their age does not remove the need to close them.
The important invariant is broader than equal labels or equal session keys: every ingress lane must derive a coherent canonical source, cache readers must track the authoritative observation, blocking storage must remain off-loop, and actual message identity must stay distinct from interaction identity. Checking the real tools-enabled prompt is necessary; disabling tools hides the parent and message-presence differences.
Validation: the head's ten committed label/identity tests pass; 25 adjacent relay adapter/interactive tests pass. Independent record-boundary probes produce one pass (a failed write retries, and recorded labels survive session pruning) and three failures (cold read plus two peer-adapter cases). Tools-enabled prompt/message probes produce three failures. The exact current-main composition reproduces the same six failures with eleven passing cases. A cold-read merge-base control passes; the message-identity probes also reproduce on the merge base.
Tests used scripts/run_tests.sh with a clean temporary HERMES_HOME, real SessionStore/SQLite, real boundary conversion, real profile-route matching, real session-context binding and the actual prompt renderer. Final message dispatch was mocked. The contended-writer case uses a controlled 300ms release to prove where the wait occurs; it is not a production performance measurement. No live Discord or model calls were made, and no author implementation was changed.
The head currently has zero hosted check runs; CI, Docker and Nix workflows report action_required. Local passing cases are not evidence of hosted CI success. The revised PR explicitly documents no back-fill of older session origins and no durable record in JSONL fallback; those limits remain. These results establish the listed failures, not exhaustive proof that every remaining manifestation has been eliminated.
| return self._discord_chat_labels[key] | ||
| store = getattr(self, "_session_store", None) | ||
| try: | ||
| recorded = store.chat_labels(Platform.DISCORD, scope, chat_id) if store is not None else None |
There was a problem hiding this comment.
[P2] Keep the cold database read off the event loop
Moving record_chat_labels to asyncio.to_thread fixes the write side, but this synchronous call reaches SessionDB.get_meta, which takes the main connection's writer lock. A cold interaction can therefore freeze the loop behind an unrelated database write, including the WS reader, heartbeats and other chats.
Reproduction: record a labelled text event through real _on_inbound; create a cold relay adapter over that same store. Start a background SessionDB._execute_write transaction whose callback waits on a threading Event. Once entered, schedule an asyncio.sleep(0.01) heartbeat, convert a forwarded interaction, and release the transaction after 300ms with a separate threading Timer. The heartbeat completes after 310ms, failing the <150ms responsiveness assertion. The same cold-conversion control passes on the merge base. This is a controlled contention proof, not a production latency estimate.
Make the cold record lookup asynchronous/off-loop too, before constructing the interaction source. Regress with a real contended store and an independent coroutine that remains responsive. The reproduction uses the real transaction and getter; neither is stubbed.
Verified on fa9259fdc1346c8d2ba5c1bd052358b559777789 and its clean composition onto upstream 8d30c4eaabd85edb77a02fef6c5388d9344ef80c, using scripts/run_tests.sh, a temporary HERMES_HOME, real SQLite/SessionStore, and real boundary conversion. Only final message dispatch was mocked.
There was a problem hiding this comment.
Reproduced: _discord_chat_labels_for called store.chat_labels on the loop, and get_meta takes the writer lock. Fixed in 10b955a3. _on_passthrough now awaits the cold read in a worker before admission, and the converter no longer touches the store. Every restart case asserts that both the read and the write ran off the loop thread. Putting the read back on the loop fails 9 cases.
| if recorded is not None: | ||
| self._discord_labels_recorded[key] = recorded | ||
| self._evict_oldest(self._discord_labels_recorded, self._DISCORD_LABELS_MAX) | ||
| self._discord_chat_labels[key] = recorded or (None, None) |
There was a problem hiding this comment.
[P2] Invalidate peer adapters' positive and negative label caches
This result, including a successful None, is cached indefinitely. A later record_chat_labels from another adapter updates the shared record without invalidating this adapter's map. The runner installs the same self.session_store on primary and secondary adapters in _wire_adapter_handlers; the record key is per platform/scope/chat.
Reproduction: construct two relay adapters A and B over one real SessionStore. A converts an interaction before any observation, memoizing (None, None). B receives a labelled message for that guild/channel through _on_inbound. A direct real store.chat_labels read now returns the new name/topic, but A's next interaction still returns missing labels. A second parameterized case first lets A observe old labels, then lets B observe the rename: the persisted record is new but A's interaction remains old. Both cases fail on the head and current-main composition. Restart/eviction makes A read the newer record, so the same interaction's result depends on cache lifetime.
Use a shared observation generation/invalidation owner, or another mechanism that makes a changed authoritative record visible to all adapter readers. Cover both first observation after a successful miss and a rename after a positive hit. Avoid fixing only exception retry: these reads succeeded.
Verified on fa9259fdc1346c8d2ba5c1bd052358b559777789 and its clean composition onto upstream 8d30c4eaabd85edb77a02fef6c5388d9344ef80c, using scripts/run_tests.sh, a temporary HERMES_HOME, real SQLite/SessionStore, and real boundary conversion. Only final message dispatch was mocked.
There was a problem hiding this comment.
Not reachable in a supported topology, so not changed. One gateway process runs one RelayAdapter: under multiplex, run_adapters.py refuses a secondary's Relay (shared process-level ingress), and the adapter map is keyed by Platform.RELAY. The only writer of a chat's record is the adapter that reads it, and it updates its own map first. Your audit narrows this the same way (Q2).
| chat_type="group" if guild_id else "dm", | ||
| chat_type="thread" if is_thread else ("group" if guild_id else "dm"), | ||
| thread_id=chat_id if is_thread else None, | ||
| parent_chat_id=str(channel["parent_id"]) if is_thread and channel.get("parent_id") else None, |
There was a problem hiding this comment.
[P2] Preserve a known thread parent when the partial channel omits it
The new thread derivation fixes the session split, but unconditionally loses a known parent when channel.parent_id is absent. Discord describes the interaction's channel as partial; the Channel schema marks parent_id optional. This finding concerns that valid omission, not a claim that every live interaction omits it.
Reproduction: admit and persist a text source with chat_id=thread_id=th1, chat_type=thread, parent_chat_id=ch1. Convert a same-user interaction carrying channel={id:th1,type:11} without the parent field. The session keys now match, but the interaction's parent is None. Enable the real Discord tool capability through temporary config/secret scope and render the real pinned session context: text shows Parent channel + Thread, while the interaction shows Channel. The actual ProfileRoute(chat_id=ch1).matches returns True for text, False for the interaction, so this is also a parent-based routing gap rather than just presentation.
Recover an already-known parent from an authoritative thread source, or resolve it at the boundary, and preserve it consistently across prompt and routing consumers. Add a partial-channel regression with tools enabled; the tools-disabled renderer hides this difference. Parent omission predates this PR's thread repair; it remains part of the class being repaired here.
Verified on fa9259fdc1346c8d2ba5c1bd052358b559777789 and its clean composition onto upstream 8d30c4eaabd85edb77a02fef6c5388d9344ef80c, using scripts/run_tests.sh, a temporary HERMES_HOME, real SQLite/SessionStore, and real boundary conversion. Only final message dispatch was mocked.
There was a problem hiding this comment.
Not changed. Discord's channel docs say interactions "always receive full channel data" for channels the app can act on, and parent_id is never obfuscated, so a thread interaction carries its parent. Recovering a parent from the text lane would cover a payload shape Discord does not send. Your audit marks it conditional too (Q1).
| chat_topic=chat_topic, | ||
| scope_id=str(guild_id) if guild_id else None, | ||
| message_id=str(payload.get("id")) if payload.get("id") else None, | ||
| # Same upstream-trust marker the relay text lane stamps. Set locally, never |
There was a problem hiding this comment.
[P2] Keep interaction identity separate from actual message identity
This source still uses payload.id as message_id. Discord's interaction schema distinguishes the interaction ID from the component's attached message.id. The resulting MessageEvent also leaves its own message_id empty. These existing boundary defects are relevant to the full prompt/input-parity class even though this assignment predates the diff.
Reproduction: forward a type-3 component with id=i1, message.id=actual-message-77, and a foreign custom_id that follows the normal text/agent path. The real converter returns source.message_id=i1, event.message_id=None. Passing its real SessionContext through GatewayRunner._set_session_env exposes HERMES_SESSION_MESSAGE_ID=i1, not the actual attached message. Thus source-based consumers receive an interaction identifier as a message target, while event-based reply/message-note consumers receive no message identifier.
A second real-wire probe provides a text frame's root message_id=platform-message-1 but omits the optional nested source.message_id. The text decoder leaves the source ID empty; the interaction source always has one. With actual Discord tool capability enabled and equal labels/user/session keys, the pinned renderer still changes its Triggering message guidance. Label parity alone therefore does not close the prefix class.
Normalize event/source message identity at the boundary and represent the interaction identifier separately. Use the attached actual message for components; slash invocations with no platform message must not invent one. Regress both component identity and root-only text frames with tools enabled. Both probes also fail on the merge base; they require repair but are not new regressions introduced by this PR.
Verified on fa9259fdc1346c8d2ba5c1bd052358b559777789 and its clean composition onto upstream 8d30c4eaabd85edb77a02fef6c5388d9344ef80c, using scripts/run_tests.sh, a temporary HERMES_HOME, real SQLite/SessionStore, and real boundary conversion. Only final message dispatch was mocked.
There was a problem hiding this comment.
Confirmed, and older than this PR (same on the merge base). Filed as #130306 with your probes, crediting you. It stays out of this PR on purpose. teknium1's review of #129032 measured what an identity split does to the slash prompt when source.message_id presence changes. This PR keeps today's identity, and the test now renders the prompt with Discord tools loaded, so a split that drops source.message_id fails 11 cases.
This is not a code review. Please do not spam the repo. |
alt-glitch
added
the
sweeper:risk-session-state
There was a problem hiding this comment.
P0 class audit: relayed Discord identity, admission, persistence and response ownership
Reviewed head: fa9259fdc1346c8d2ba5c1bd052358b559777789
Merge base: 38d3dbce4adbb9074a34214eb330237a129e5904
Target branch at inspection: 8d30c4eaabd85edb77a02fef6c5388d9344ef80c
Assessment
The chat-scoped observation record is a substantive improvement. The current interaction's nickname is a better input than a remembered prior nickname, and deriving thread identity repairs a real conversation split. Those changes deserve to remain. They do not establish closure of #128796's P0 class audit.
This review reconciles Teknium's original assessment, the rename/restart review, my first review, my same-head follow-up, the author's replies, and the expanded issue audit. It preserves repaired findings and explicitly narrows earlier assertions where the evidence does not support them. The historical records remain untouched.
The missing shape is not one cache. This path crosses independently owned state machines: connector receipt, adapter admission, logical conversation identity, mutable chat observation, agent-incarnation initialization, restored transport ownership, model-input preparation, transcript/effect identity, and provider response delivery. A value can be right at one boundary and absent, stale, or misclassified at the next.
Fresh evidence includes 409 passing committed tests in 48 files, the PR's own test copied unchanged to the merge base (9 failed / 1 passed) and head (10 passed), three test-sensitivity mutations, and separately classified seam probes. Those green committed tests coexist with demonstrated admission, recovery, content-normalization and lifecycle failures. The exact evidence table below distinguishes tests of desired behavior from verifiers that pass by observing a defect.
What the earlier wording establishes, and what it cannot establish
| Original assessment | Evidence-based reconciliation |
|---|---|
| “Fix direction is right.” | Supported for the narrower repair: chat observations are recorded before dispatch, independently of session creation, and current interaction display names win. Direction is not proof of admission/ACK, DB recovery, restoration, or response-route correctness. |
| “One non-blocking residual inline (threads).” | At the historical pin, a thread interaction used a different conversation key, not merely different prompt bytes. The key builder explains why; the author's thread repair reply recognized the session split. The current thread derivation repairs it. It is closed here, not recycled as an open defect. |
| “The only behavioral cost of a miss is one extra re-render.” | Not a sufficient current-head bound. A miss now reaches the synchronous durable getter, whose writer-lock acquisition can prevent unrelated loop progress. A failed handle open can also become a permanent negative cache entry after recovery (F07). |
| The interaction body “carries only ids.” | Too broad as a schema description: Discord documents optional partial guild/channel objects and structured interaction data. The exact connector-rendered Server / #channel label is still not guaranteed by those optional objects. Remembering the observed label is a reasonable source; it is not evidence that the whole interaction has only IDs or that no other normalization contract matters. |
| Green focused/full relay suites establish correctness | They establish their asserted cases. The prompt fixture, seven sequential label cases and three naming cases do not assert buffered settlement, cancellation before admission, DB-open recovery, or payload preservation. F01–F13 give the distinct mechanisms. |
| Earlier issue triage: no data loss or wrong output, only cold prefixes | That is not an adequate class-wide conclusion. The new cancellation window admits no message yet ACKs its replay (F02); supported modal input is normalized to empty content (F05); restoration can resolve no delivery adapter (F03). These are different observed consequences, with explicit local-test limits below. |
This is a technical disagreement about the reach of the evidence, not about anyone's intent or contribution. Jonpol's iterative fixes address real counterexamples; JoaoMarcos44's current-payload naming approach is credited in the PR. Teknium identified the original label mismatch and the thread dimension. The remaining obligations come from following those paths through their owners, not from treating any reviewer's conclusion as evidence in itself.
1. SSA — subsystem architecture and ownership
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","primaryTextColor":"#f0f6fc","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":16,"rankSpacing":30}}}%%
flowchart TD
subgraph FLOW["Relay intake, state and response ownership"]
direction TB
W["Authenticated relay transport"] --> T["Normalized text frame"]
W --> I["Raw interaction forward"]
T --> D["Process seen set"]
D --> L["Await chat-label observation<br/>write"]
L --> A["Adapter admission and<br/>background task"]
I --> N["Interaction conversion"]
N --> P["Prompt-response gate"]
P --> A
A --> R["Runner command or agent turn"]
R --> S["Session and restored<br/>transport identity"]
R --> M["Model input and prompt pins"]
R --> E["Transcript and persistence<br/>owner"]
R --> O["Ordinary send response"]
T -->|after callback| ACK["Inbound buffer ACK"]
I -. missing settlement path .-> ACK
O -. not selected automatically .-> F["Interaction follow_up API"]
click W href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L692-L715" "Pinned implementation at historical reviewed revision"
click T href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L174-L247" "Pinned implementation at historical reviewed revision"
click I href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L250-L290" "Pinned implementation at historical reviewed revision"
click D href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Pinned implementation at historical reviewed revision"
click L href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Pinned implementation at historical reviewed revision"
click A href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/platforms/base.py#L3987-L4027" "Pinned implementation at historical reviewed revision"
click N href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L1197-L1287" "Pinned implementation at historical reviewed revision"
click P href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L2180-L2225" "Pinned implementation at historical reviewed revision"
click R href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_inbound.py#L1312-L1390" "Pinned implementation at historical reviewed revision"
click S href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session.py#L891-L974" "Pinned implementation at historical reviewed revision"
click M href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_agent_cache.py#L665-L691" "Pinned implementation at historical reviewed revision"
click E href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_turn.py#L2129-L2139" "Pinned implementation at historical reviewed revision"
click O href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L1508-L1543" "Pinned implementation at historical reviewed revision"
click ACK href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L633-L639" "Pinned implementation at historical reviewed revision"
click F href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L1788-L1826" "Pinned implementation at historical reviewed revision"
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Gray: context / transient"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class W,T,I entry
class D,M context
class L,N,R,O,ACK,F transition
class P,A,S,E authority
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
SSA node → pinned implementation: W · authenticated relay transport · T · normalized text frame · I · raw interaction envelope · D · process seen set · L · awaited label write · A · adapter admission · N · interaction conversion · P · prompt-response gate · R · runner dispatch · S · session identity · M · prompt pins · E · transcript/action owner · O · ordinary send · ACK · buffer acknowledgement · F · follow-up capability
SSA transition → controlling call site: W→T · W→I · T→D · D→L · L→A · I→N · N→P · P→A · A→R · R→S · R→M · R→E · R→O · T→ACK · I↛ACK · missing · O↛F · not selected
Dotted transitions link to the code showing the missing/nonselected path; they are not claims of implemented edges.
Solid arrows are inspected current paths; dotted arrows mark missing or nonselected paths, not implemented repairs. The connector's edge response is outside this process. Its provider ACK does not establish that this gateway has durably accepted the work.
2. Findings — causal mechanism, evidence, repair and closure
P0 denotes the class's durable-admission acceptance obligation. P1 marks demonstrated loss of admission, initialization, submitted content, restored ownership, or the documented response path. P2 marks narrower identity, recovery and responsiveness failures. Conditions and evidence limits remain attached to each finding; these priorities do not imply a demonstrated exploit or live outage.
F01 · P0 — Buffered passthrough has no ACK/settlement path, and duplicate admission is not fenced
inherited. The real frame dispatcher invokes the adapter twice for the same benign buffered interaction and emits zero buffer ACKs. Ordinary buffered text dispatches once and ACKs both deliveries. Interaction preparation also produces a fresh persistence owner because event.message_id is absent; slash replies skip the outbound delivery ledger. These are separate pieces of missing settlement, not proof that a real tool effect executed twice.
Controlling seams: passthrough dispatcher; the receiving callback. Evidence: lifecycle-parent / lifecycle-base-parent: frame_ack_replay, turn_owner, delivery_accounting. Closure: Stable interaction action identity; durable handoff/settlement and ACK agree under repeated delivery and crash-state tests.
F02 · P1 — The awaited label write opens a cancellation window before admission but after the seen bit
new pre-admission exposure on an inherited early-dedupe design. The new await is between seen insertion and handle_message. Cancelling intake while the label worker waits leaves the ID in the seen map. The replay returns at dedupe and the transport ACKs it: zero admissions, one ACK. The uncancelled head control and both base arms admit once. This is more consequential than a metadata-only cache change.
Controlling seams: The message is marked seen before this new await; message admission remains later. Evidence: cancellation-head: 1 failed / 1 passed; cancellation-base: 2 passed. Closure: No ACK/seen-only success for a cancelled, unadmitted message; replay reaches admission exactly once.
F03 · P1 — Persisted relay sessions lose transport kind even when logical platform and profile survive
inherited. The new session-key assertion is useful but does not exercise restored transport ownership. A real persisted/reopened relay-origin Discord row loses the live relay marker. The restored resolver returns no adapter in relay-only configurations, or selects a registered native Discord adapter. Persisting transport_profile alone does not distinguish those transport kinds.
Controlling seams: The relay marker is intentionally wire-invisible; source serialization omits it. Evidence: lifecycle-parent and lifecycle-base-parent: restored_owner. Closure: Restored rows resolve their real receiving transport, without making ordinary serialized sources capable of forging live provenance.
F04 · P1 — Read-only status consumes first-agent-turn initialization through timestamp-based newness
inherited. The fixture names /status but the label test never dispatches that command. The real status handler creates a session; the subsequent first agent lookup touches updated_at before newness is evaluated. session:start is then omitted. A direct-first-turn control emits it. The skill guard is also suppressed for a manually seeded skill, but the current relay decoder does not supply that field.
Controlling seams: status handler gets or creates the row; The next existing-row lookup updates activity. Evidence: lifecycle-parent and lifecycle-base-parent: status_first_turn, manual_skill_guard, auto_skill_wire. Closure: Read-only commands cannot suppress the first real session:start; any skill consequence is verified from an actual skill-bearing producer.
F05 · P1 — Admitted modal submissions lose their user-entered values before the consumer
inherited. Type 5 is described as supported, but the converter only projects type-2 and type-3 content. A modal's submitted strings become an empty MessageEvent. Real inbound text preparation retains that empty input, and history reconstruction drops the empty user row. Correct thread identity does not preserve the user's answer.
Controlling seams: The interaction-kind branch; The resulting event. Evidence: consumers-isolated: modal value correctness fails; empty input/replay consequence passes; lifecycle both pins confirm. Closure: Every supported modal value reaches the intended consumer or the submission is explicitly rejected; no successful empty conversion.
F06 · P2 — Select component identity survives while the actual selected values disappear
inherited. Prompt equality is not an input-preservation oracle. A type-3 select with custom_id=choose and values=[item-b] becomes only 'choose'; text, metadata and prompt_response contain no item-b. Distinct selections therefore collapse at this boundary. Ordinary button custom_id remains a passing control.
Controlling seams: Type-3 conversion; hp1 prompt-token path. Evidence: consumers-isolated: test_select_component_preserves_user_choices fails; exact-base adapter equivalence passes. Closure: Two different choices on one select remain distinguishable after normalization; ordinary buttons and hp1 controls retain their behavior.
F07 · P2 — A recoverable DB-open failure becomes an indefinitely cached absent-label read
introduced. The exception branch retries, but the actual unavailable-handle path never raises here. RecoverableHandleCache returns None; _routing_db_method finds no getter; chat_labels returns None; the adapter memoizes absence. After recovery a real direct read and a fresh adapter see the saved labels while this adapter continues returning no labels.
Controlling seams: chat_labels' docstring; RecoverableHandleCache. Evidence: state-head-final: unavailable-read fails; getter-exception control passes; base record API absent. Closure: After the DB handle recovers, the next interaction can retrieve the existing record without requiring text, restart or eviction.
F08 · P2 — A no-op record call is cached as successful persistence and suppresses recovery writes
introduced. With no setter available, record_chat_labels returns normally without writing. The adapter then fills _discord_labels_recorded. When SQLite recovers, a later message with identical labels skips the write; the actual durable row is still absent. A changed-label control writes normally. The comment promising retry after a failed write covers exceptions, not this real no-op path.
Controlling seams: the adapter marks the labels recorded; equality suppresses later writes. Evidence: state-head-final: unavailable-write fails; setter-exception control passes; base API absent. Closure: Only a confirmed durable write suppresses same-label retries; unchanged observations populate the recovered database.
F09 · P2 — Context-menu commands are flattened to slash names without their selected target
inherited. The fixture represents only the chat-input subtype of type-2 interactions. A message-context command carrying target_id and resolved message content becomes '/summarize' with neither target nor resolved data. Nested slash subcommands work as a control. A valid route and display name cannot recover the command's selected object.
Controlling seams: The current converter; event construction. Evidence: consumers-isolated: context-menu target assertion fails; exact-base equivalence passes. Closure: Supported context-menu commands retain their selected object; unsupported ones fail explicitly rather than silently becoming targetless slash text.
F10 · P2 — Interaction action IDs are advertised and used as provider message targets
inherited. The converter assigns payload.id to source.message_id and leaves event.message_id empty. A component's actual attached message is ignored outside recognized prompt handling. Tool context receives the interaction ID, reactions target it, and the cached prompt claims an ID will be supplied per-turn even though the event-based note supplies none. Slash commands without an attached message invent the same target.
Controlling seams: The converter conflates the former with source.message_id; only hp1 prompt responses retain attached message identity. Evidence: consumers-isolated: component/slash identity assertions fail; lifecycle both pins: reaction_id_kind, delivery_accounting, turn_owner. Closure: Action identity survives without becoming a false reply/react target; attached and absent message targets are handled explicitly.
F11 · P2 — Root-only text message IDs never reach source-based context consumers
inherited. The new text fixture always puts an ID inside source, hiding the separate root-only wire shape. The real decoder accepts root message_id and populates event.message_id, but leaves source.message_id empty. With real Discord tools enabled, an otherwise identical nested-ID frame produces different pinned guidance. This is a separate text-side repair from F10's interaction ID typing.
Controlling seams: Source construction; event construction. Evidence: consumers-isolated: root-only source-ID assertion fails; unchanged base/head decoder blob. Closure: Equivalent accepted text-ID representations yield coherent event, source and tool-context identity, with an explicit conflict policy.
F12 · P1 — Normal interaction replies select ordinary send rather than the documented follow-up capability
inherited. The contract says the forwarded interaction's reply uses follow_up. Real inline and final reply paths emit op=send, reply_to=None and no follow-up. Calling send_follow_up explicitly produces the expected op, but there is no in-tree production caller selecting it. Correcting the thread key alone does not connect this response path.
Controlling seams: inline response path; final reply path. Evidence: lifecycle-parent and lifecycle-base-parent: interaction_reply_route; production caller census. Closure: Normal interaction response routes select the intended capability and correlate the specific action; ordinary text remains ordinary send.
F13 · P2 — Cold label reads synchronously wait on the database writer lock on the event loop
introduced; same-head prior finding retained. The off-loop write repair is real, but this getter reaches SessionDB.get_meta on the loop and waits for its writer lock. A controlled real transaction prevents an unrelated heartbeat from running until release; the base converter has no new read and its control remains responsive. The fix needs both storage directions, not just to_thread around writes.
Controlling seams: The getter; SessionDB.get_meta. Evidence: state-head-final: cold-read-contention fails; state-base: same responsiveness assertion passes. Closure: The event loop remains responsive during contended cold reads and recovered read states retain their distinct meaning.
3. Reconciliation of every earlier material item
“Closed” below means the named old mechanism is removed or its tested contract now passes; it does not imply the entire class is closed. Prior IDs and sources are retained even where their reachability or consequence needs narrowing.
| Prior source / priority | Original claim | Current adjudication and evidence | Owner / closure |
|---|---|---|---|
| 5360899055 P1 historical premise |
Missing text/interaction display labels and raw usernames | repaired within healthy-storage cases. Unchanged author tests: base 9 failed/1 passed, head 10 passed; three sensitivity checks detect broken rules. | relay normalization: Retain current-name and observed-chat-label controls; broader recovery in F07/F08. |
| 4140332325 P1 historical conversation split |
Thread fields missing, characterized as non-blocking residual | repaired; historical impact broader than cache. Base and head lifecycle key controls; author thread test red at base and green at head. Author reply 4140544377 acknowledged distinct keys. | thread identity: Retain thread key repair; do not imply it establishes transport ownership or content parity. |
| 4140332331 P2 current boundary |
FIFO miss costs only one extra rerender | not a sufficient current-head bound. F13 real contended getter prevents coroutine progress; F07 negative cache survives recovery. | label reader: Off-loop cold read and recovery-aware cache decisions. |
| 4141040356 P2 historical |
Rename/reuse/restart restores old creation-time origin | old mechanism removed; healthy restart repaired. Current record no longer reads origins; author rename and peer-rename tests pass. Reply 4142131677 describes intermediate repair, not current architecture. | chat record: Retain per-chat observation authority independently of session resets. |
| 4144456810 P2 historical |
Lookup-filled map suppresses origin reconciliation | old mechanism removed. No origin scan or refresh remains. The initial slash-first upgrade without a record is explicitly limited; later observation writes chat state. Reply 4144785755 reconciled. | chat record / cache receipts: Origin freshness is no longer the acceptance model; false recorded success remains a distinct F08. |
| 4144456818 P2 historical |
User label varies with cache lifetime; proposal to persist text name | closed under changed current-identity policy. Current nick/global-name/username tests pass and mutation detects raw-username regression. Reply 4144785980 explains why stale text name must not win. | interaction identity: Do not resurrect earlier proposed persistence policy; current interaction name wins. |
| 4144456829 P2 historical |
Read exception memoized as absence | exception branch repaired; unavailable handle remains distinct. Author read-retry and independent getter-exception control pass. Reply 4144786214 is supported for exceptions, not for unavailable-handle None. | getter availability: Keep exception retry and separately repair F07's real handle-open path. |
| 4144456838 P2 historical |
Whole-index refresh/fsync under entry lock on event loop | old bulk-write mechanism removed. Current code writes one metadata row via to_thread; author off-loop assertion passes and synchronous mutation fails. Reply 4144786463 supported. Cold reads and cancellation are separate F13/F02. | storage async boundary: Preserve off-loop writes; close read and cancellation ownership without reverting. |
| 4146317512 P2 |
Cold getter blocks loop behind DB writer | retained as F13. Real writer/getter contention; heartbeat only after release on head, before release at base. | cold read: Off-loop read plus availability-aware result; no production latency extrapolation. |
| 4146317521 P2 prior; conditional now |
Peer RelayAdapters over shared store require cache invalidation | normal multiplex reachability not established. Real secondary startup refuses RelayAdapter and never calls sentinel constructor; two manually created adapters are insufficient production evidence. | deployment topology / shared record ownership: Require an actual supported multiwriter topology before retaining a production defect; Q2 preserves the record. |
| 4146317534 P2 prior; conditional now |
Partial channel without parent loses route and prompt parity | local boundary result confirmed; normal provider omission unverified. Parent predicate/model override/system prompt diverge under omitted-parent fixture; explicit-parent control passes. Current Discord docs state full interaction channel data, so generic optional schema alone does not prove normal omission. | thread parent producer and consumer contract: Q1: establish reachable omission/connector contract, then recover known parent or reject unsupported shape. |
| 4146317543 P2 |
Interaction/source/event message identity inconsistent; root-only text omitted | retained, decomposed into F10/F11; F01 shares action identity dependency. Actual context var, per-turn note, reaction op and root-only decoder tests; exact-base projection checks. | typed action/message identity: Separate action identity, message target and reply anchor; normalize text source/event IDs. |
| 5911422165:buffered-passthrough P0 |
No durable ACK/replay fence; repeated model/effect execution | F01 retains gateway failure; stronger live effect claim not freshly verified. Duplicate inert admission, zero ACK; unstable prepared owner and slash ledger exclusion at head/base. No model/tool effect executed. | admission and settlement: Durable action-state and ACK crash matrix; not merely a new in-memory dedupe bit. |
| 5911422165:restored-owner P1 |
Restored relay source loses transport owner | retained as F03 within local selection bounds. Real persistence/reopen; live relay resolves, restored relay-only does not; native control resolves. | trusted transport restoration: Persist trusted adapter kind with profile and exercise full resume separately. |
| 5911422165:status-initialization P1 |
Status suppresses first hook and bound channel skill | hook confirmed as F04; skill consequence narrowed. Real status/store/open-session hook count 0 vs direct 1. Skill guard demonstrated only with manually seeded auto_skill; relay producer drops it. | incarnation initialization and actual skill producer: Initialization bit independent of activity; producer-grounded skill regression. |
| 5911422165:modal P1 |
Type-5 values discarded | retained as F05; select and context-menu siblings F06/F09 newly mapped. Modern and legacy-shaped modal inputs empty at dispatch; input/replay consequence; positive slash/button controls. | shape-aware normalization: Lossless supported-shape representation or explicit unsupported rejection. |
| 5911422165:thread-key P1 historical |
Thread split and historical reset/clarify consequences | thread key repaired; historical full flows not re-executed here. Direct cross-pin key and same SessionStore ID controls; preserved full-parent thread test. Earlier reset/clarify execution remains labeled historical. | conversation identity: Retain key repair and public reset/clarify regressions; no claim of newly executed full flows. |
| 5911422165:chat-clock P2 historical |
Delayed first session, peer reset and A-to-B-to-A label ordering | healthy current-head record design and controls pass. Author delayed-session/peer-reset cases and independent single-adapter A-to-B-to-A control pass. No current per-user clock is used. | chat observation clock: Preserve chat-scoped observation; recovery F07/F08 remains independent. |
| 5911422165:jsonl-fallback P2 documented limitation |
DB-degraded mode lacks durable label fallback | source-confirmed and explicitly disclosed; Q4. No setter -> no write; no getter -> no record; PR body discloses no JSONL durability. Transient recovery bugs freshly reproduced in F07/F08. | degraded persistence: Any broader durability promise needs fallback/recovery representation and its own tests. |
| 5911422165:competitor-ordering P2 historical comparison |
#129032 A/B/C ordering defects and old six-of-eight comparison | not a current-head verdict about the competitor. Live API head differs from comparison pin. No sibling composition or new sibling test result is claimed here. | separate PR review: Re-run at the sibling's actual head in its owning review; do not carry stale counts forward. |
Explicit limits and contrary evidence that must not disappear
Q1 — missing thread parent is a conditional boundary result, not established normal Discord delivery. The prior parent finding is reproducible when a partial thread object omits parent_id: the same SessionStore ID can be retained while the real route predicate misses; parent-configured model and system prompt also fall back. The real tools-enabled renderer changes; tools-disabled bytes can match. However, the generic optional parent_id? field is not sufficient evidence that a normal thread interaction omits it. Discord's current channel documentation explicitly says interactions use a separate, non-obfuscated path and apps receive full channel data where they can act. No captured connector/provider interaction here proves the missing-parent condition occurs. The source behavior and its consequences remain in the ledger, but I am not presenting that condition as a demonstrated production regression. A future connector contract allowing omission needs an authoritative parent recovery or explicit rejection policy; it does not need a guessed parent.
Q2 — two-peer-RelayAdapter invalidation was overclaimed as a normal multiplex topology. The earlier peer-cache inline manually constructed two adapters. The real secondary startup path instead refuses a secondary RelayAdapter. A sentinel that would raise on construction was never reached; the actual method returned zero connected adapters and recorded the secondary as unserved. The generic cache-coherence concern is conditional on a separately evidenced multiwriter deployment. It is not retained as a confirmed normal single-gateway defect. F07/F08 establish real recovery failures with one adapter; they are not a silent replacement for the old claim.
Q3 — all-empty observation has unresolved snapshot semantics. A text frame with a prior topic and then chat_name=None, chat_topic=None is accepted by the decoder and is permitted by the null-capable wire fields. The learner's truthiness guard retains the old topic. A nonempty name with a cleared topic updates correctly. The probe establishes this asymmetry, and base has no remembered stale topic, but it does not establish whether all-null means “clear” or “not supplied” for the deployed connector. This is a contract/coverage question, not an unconditional deletion-regression claim. Both meanings need explicit testable treatment.
Q4 — DB-degraded restart and old-origin upgrade coverage are documented limitations. The PR explicitly states no back-fill of older origins and no durable chat record in JSONL fallback. The store's degraded continuity and new record's missing fallback confirm the latter. That limitation remains visible, but it is not evidence the author claimed fallback durability. F07/F08 are stronger and distinct: a transient unavailable handle can poison the state after healthy SQLite returns. The record API also has no version/observation generation or explicit success result; requiring one should follow the actual recovery contract, not a speculative new framework.
Q5 — skill/channel-prompt parity and history backfill need producer evidence. The text decoder and interaction construction do not supply auto_skill or channel_prompt. The real first-turn guard is affected by /status, but an unconditional claim that a relayed configured skill was lost conflates an unimplemented producer leg with that lifecycle defect. Similarly, optional text-lane context does not promise interaction backfill. Neither omission establishes a deployment-specific failure without the corresponding producer/configuration contract.
Q6 — related PRs and historical counts are pinned claims, not current facts by inheritance. #129032 is now bf91bc7f78781f3d8d03b3b4c7a26ac9a12acae2, not the 7192d782 used in the PR's comparison. I am not restating the old 6/8 result as a verdict about that newer head. #128722 and #128788 remain separate Slack/native-Discord changes; their existence is not proof that this relay path is closed. The inspected issue timeline links both competing relay PRs; this review does not merge their implementations or manufacture a combined test result.
4. RCA synthesis and repair dependencies
The recurring causal error is substituting a nearby fact for the fact a consumer needs:
- Seen-set membership substitutes for successful admission (F02).
- Callback return substitutes for durable handoff (F01).
Nonefrom a missing DB handle substitutes for an authoritative absent row (F07).- A no-op record call substitutes for successful persistence (F08).
- Session timestamp inequality substitutes for initialized incarnation (F04).
- Logical platform plus profile substitutes for a persisted transport kind (F03).
- Source interaction ID substitutes for message target; source presence substitutes for per-turn event-ID delivery (F10/F11).
- Correct session key/labels substitute for preserved submitted content (F05/F06/F09).
- Ordinary send substitutes for the promised interaction response route (F12).
- Off-loop writes substitute for off-loop storage access generally (F13).
These are independently owned contracts. Repairing one cannot be used as evidence that another repaired itself.
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","primaryTextColor":"#f0f6fc","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":16,"rankSpacing":30}}}%%
flowchart TD
subgraph FLOW["Cancellation before admission"]
direction TB
W["WS intake task"] --> SEEN["Mark message seen"]
SEEN --> WAIT["Await off-loop label write"]
WAIT --> CANCEL["Intake task cancelled while<br/>worker waits"]
CANCEL -. may still finish .-> WORKER["Label worker finishes<br/>metadata write"]
CANCEL --> REPLAY["Connector buffer replays same<br/>message"]
REPLAY --> CHECK["Seen map already contains<br/>message"]
CHECK --> RETURN["Return without admission"]
RETURN --> ACK["inbound_ack sent to connector"]
RETURN -. observed probe outcome .-> A["No message admitted"]
click W href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L174-L215" "Pinned implementation at historical reviewed revision"
click SEEN href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Pinned implementation at historical reviewed revision"
click WAIT href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Pinned implementation at historical reviewed revision"
click CANCEL href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Pinned implementation at historical reviewed revision"
click WORKER href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Pinned implementation at historical reviewed revision"
click REPLAY href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L935-L943" "Pinned implementation at historical reviewed revision"
click CHECK href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Pinned implementation at historical reviewed revision"
click RETURN href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Pinned implementation at historical reviewed revision"
click ACK href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L633-L639" "Pinned implementation at historical reviewed revision"
click A href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/platforms/base.py#L3987-L4027" "Pinned implementation at historical reviewed revision"
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Red: failure"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class W,REPLAY entry
class SEEN,WAIT,WORKER transition
class CHECK authority
class CANCEL,RETURN,ACK,A failure
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
This is a controlled cancellation/interleaving proof, not a live outage measurement. The existing early seen-set ownership predates the PR; the new awaited storage leg opens this specific pre-admission exposure. The passing no-cancellation control and base arm distinguish it from a broken transport fixture.
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","primaryTextColor":"#f0f6fc","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":16,"rankSpacing":30}}}%%
flowchart TD
subgraph FLOW["Unavailable storage and sticky cache decisions"]
direction TB
O["Recoverable DB open"] -->|unavailable or backoff| U["No handle"]
U --> G["chat_labels returns None"]
G --> N["Cache negative result"]
N --> NR["Later interaction skips<br/>recovered DB"]
U --> R["record_chat_labels does<br/>nothing"]
R --> K["Adapter marks labels recorded"]
K --> NW["Same-label text skips<br/>recovered DB"]
O -->|healthy again| DB["Real row can be read or<br/>written"]
DB -. no recovery signal reaches cached decisions .-> NR
DB -. no success receipt invalidates false recorded state .-> NW
click O href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_db_recovery.py#L84-L131" "Pinned implementation at historical reviewed revision"
click U href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_persistence.py#L84-L106" "Pinned implementation at historical reviewed revision"
click G href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_persistence.py#L237-L259" "Pinned implementation at historical reviewed revision"
click N href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L929-L949" "Pinned implementation at historical reviewed revision"
click NR href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L929-L949" "Pinned implementation at historical reviewed revision"
click R href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_persistence.py#L237-L259" "Pinned implementation at historical reviewed revision"
click K href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Pinned implementation at historical reviewed revision"
click NW href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Pinned implementation at historical reviewed revision"
click DB href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_persistence.py#L237-L259" "Pinned implementation at historical reviewed revision"
end
LEGEND["Legend<br/>Amber: transition / pending<br/>Green: success<br/>Red: failure<br/>Gray: context / transient"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class O transition
class U,G,R context
class N,NR,K,NW failure
class DB success
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","primaryTextColor":"#f0f6fc","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":16,"rankSpacing":30}}}%%
flowchart TD
subgraph FLOW["Required repair boundaries"]
direction TB
ID["Define delivery ID,<br/>interaction ID and message<br/>target separately"] --> AD["Durable admission and effect<br/>settlement"]
AD --> ACK["Buffer ACK after the<br/>specified durable boundary"]
ID --> REPLY["Select interaction response<br/>capability"]
ID --> TOOLS["Message guidance and<br/>reactions use actual message<br/>targets"]
DB["Represent unavailable, absent<br/>and committed distinctly"] --> CACHE["Recovery-aware lookup and<br/>write caches"]
DB --> LOOP["Off-loop reads and writes"]
CANCEL["Cancellation-safe admission<br/>ownership"] --> AD
OWNER["Persist trusted transport<br/>kind plus profile"] --> RESTORE["Restore owner before resume<br/>or delivery"]
INIT["Track first-agent-turn<br/>initialization explicitly"] --> HOOK["Session hook and available<br/>skill inputs"]
SHAPE["Normalize supported payload<br/>types without data loss"] --> INPUT["Model input and durable<br/>replay"]
click ID href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/platforms/event.py#L45-L59" "Existing repair owner at reviewed revision; not a claim of completed repair"
click AD href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/platforms/base.py#L3987-L4027" "Existing repair owner at reviewed revision; not a claim of completed repair"
click ACK href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/ws_transport.py#L633-L639" "Existing repair owner at reviewed revision; not a claim of completed repair"
click REPLY href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L1788-L1826" "Existing repair owner at reviewed revision; not a claim of completed repair"
click TOOLS href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run.py#L4261-L4284" "Existing repair owner at reviewed revision; not a claim of completed repair"
click DB href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session_persistence.py#L237-L259" "Existing repair owner at reviewed revision; not a claim of completed repair"
click CACHE href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L929-L949" "Existing repair owner at reviewed revision; not a claim of completed repair"
click LOOP href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L902-L927" "Existing repair owner at reviewed revision; not a claim of completed repair"
click CANCEL href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L876-L897" "Existing repair owner at reviewed revision; not a claim of completed repair"
click OWNER href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/session.py#L124-L161" "Existing repair owner at reviewed revision; not a claim of completed repair"
click RESTORE href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_startup.py#L575-L616" "Existing repair owner at reviewed revision; not a claim of completed repair"
click INIT href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_turn.py#L512-L543" "Existing repair owner at reviewed revision; not a claim of completed repair"
click HOOK href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_turn.py#L2080-L2085" "Existing repair owner at reviewed revision; not a claim of completed repair"
click SHAPE href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/relay/adapter.py#L1208-L1223" "Existing repair owner at reviewed revision; not a claim of completed repair"
click INPUT href "https://github.com/NousResearch/hermes-agent/blob/fa9259fdc1346c8d2ba5c1bd052358b559777789/gateway/run_inbound.py#L1731-L1762" "Existing repair owner at reviewed revision; not a claim of completed repair"
end
LEGEND["Legend<br/>Purple: ownership / policy<br/>Amber: transition / pending<br/>Teal: durable state"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class ID,DB,CANCEL authority
class OWNER durable
class AD,ACK,REPLY,TOOLS,CACHE,LOOP,RESTORE,INIT,HOOK,SHAPE,INPUT transition
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
These are repair dependencies, not a proposal to accumulate everything into another manager. Existing owners should be extended where possible. In particular:
- A new
inbound_ackcall alone does not provide effect idempotency across a crash after an effect but before ACK. - Assigning the provider interaction ID blindly to
event.message_idcan make reply/reaction consumers use a non-message ID. Assigning the attached message ID blindly can collapse multiple distinct clicks on the same message. Identity roles need separate representation. - Persisting
delivered_via_upstream_relay=Truethrough arbitrary source serialization is not a valid restoration repair. The current omission protects a real trust boundary. - Moving writes off-loop does not resolve cold getter lock acquisition, and it creates cancellation ownership that must be accounted for.
- Deriving current display names does not justify suppressing genuine metadata changes. Stable inputs should stay byte-stable; real changes need coherent propagation, not forced equality.
5. Coverage, reproducibility and acceptance conditions
Fresh execution, separated by what the assertions mean
| Run | Exact result | What it establishes |
|---|---|---|
head-relay-complete |
48 files, 409 tests passed, 0 failed; exit 0 | Committed relay directory + store-lock/recovery suites; genuine suite pass. |
state-head-final |
1 files, 4 tests passed, 4 failed; exit 1 | 4 desired-contract failures; 4 positive/exception/topology controls. All-empty row remains conditional Q3. |
state-base |
1 files, 3 tests passed, 0 failed, 5 skipped; exit 0 | 3 controls pass; 5 new-record API cases skipped because that API does not exist at base. |
consumers-isolated |
2 files, 10 tests passed, 9 failed; exit 1 | Head: 9 intended-contract failures / 3 controls; baseline adapter substitution: 7 equivalence controls. Not 19 passing product tests. |
lifecycle-parent |
1 files, 11 tests passed, 0 failed; exit 0 | 11 observation verifiers pass, confirming listed defects and healthy controls. Not product closure. |
lifecycle-base-parent |
1 files, 11 tests passed, 0 failed; exit 0 | Identical 11 observation verifiers at the true base; thread repair is the discriminated delta. |
cancellation-head |
1 files, 1 tests passed, 1 failed; exit 1 | Cancellation/replay assertion fails; no-cancellation control passes. |
cancellation-base |
1 files, 2 tests passed, 0 failed; exit 0 | Both arms admit; no new label-write suspension exists. |
author-baseline-final |
1 files, 1 tests passed, 9 failed; exit 1 | Unchanged head test file on pristine base, isolated fixture; expected functional RED. |
author-controls-final |
2 files, 13 tests passed, 0 failed; exit 0 | 10 unchanged author cases + 3 sensitivity verifiers. Mutations are expected to be caught. |
Commands, runtime, pin and log hashes are recorded in execution-summary.json and each *.receipt.json. The broad command is scripts/run_tests.sh tests/gateway/relay/ tests/gateway/test_session_store_lock_io.py tests/gateway/test_session_db_recovery.py -j 4. Focused commands pass the named reviewer-local probe file(s) to the same runner. Source and assertions are included in the evidence bundle; no reviewer-local test is misrepresented as a committed test.
Environment: native Windows 11; Python 3.11.11 and pytest 9.1.1 from an existing isolated test virtualenv. Repository Python tests and final focused probes ran through scripts/run_tests.sh; homes, platform data directories and temporary state were redirected. Probe imports were bound to the selected worktree. The first broad one-worker attempt was interrupted before completion and is not counted as green; the complete 48-file four-worker rerun is the broad receipt. A first external copy of the author's test hit the home-isolation guard; that attempt is discarded for behavioral attribution, and the final run adds only a per-test isolation fixture while preserving the copied test bytes. A delegated preliminary consumer run unexpectedly loaded a credential bootstrap; it is not used as clean-environment evidence. The final parent rerun with all platform data roots redirected reproduced the same behavioral results without that bootstrap notice.
The exact changed test file's content hash was checked against its unchanged baseline-copy input. The three in-memory sensitivity checks deliberately replaced cold lookup, off-loop recording and nickname selection, and each made the corresponding author assertion fail; all replacements ended with their pytest fixture. No product source was edited. The committed suite's success is real, but its coverage is narrower than the class claim.
Receipt interpretation: lifecycle verifiers pass by asserting observed defects and healthy controls; they do not assert the product is repaired. Consumer correctness assertions intentionally fail at the missing values. Counts from overlapping test selections and repeated runs are not added into a fictional coverage total. Raw logs, machine-readable case rows, probe sources, ledger and a hash manifest accompany the review artifact; probe filenames below are reviewer-local artifacts, not claimed repository test files.
Class closure matrix
| Dimension | Required observable closure | Current evidence |
|---|---|---|
| Conversation shape | Text/slash/component/modal agree on DM, channel and thread identity; all thread types 10/11/12 | Ordinary public-thread key repair verified; all three types are source-covered, not all independently exercised here. |
| Current names and labels | Nick/global-name/username policy; chat rename, peer observation, delayed session, reset, restart | PR's ten cases green; independent A→B→A and scope-separation controls green. |
| Recovery | Getter exception, setter exception, unavailable handle, backoff, reopen, unchanged-label retry, cold restart | Exception controls pass; F07/F08 fail after real handle recovery. |
| Admission cancellation | No seen/ACK outcome without retained admission when metadata work is interrupted | F02 fails; no-cancellation and merge-base controls pass. |
| Loop responsiveness | Unrelated coroutine progresses while cold reader waits behind writer | F13 fails at head; merge-base control passes. |
| Durable replay | Same authenticated interaction retains action identity; crash-state settlement and ACK agree | Duplicate admission and absent ACK observed; full process-crash/effect matrix remains unexecuted. |
| Restored owner | Relay-only and multiplexed durable rows resolve their actual transport | Local persistence/reopen/resolver probes fail the intended ownership contract; full startup send is not exercised. |
| Initialization | Read-only status before first real turn does not suppress session:start | F04 reproduces; skill consequence is conditional on an actual producer. |
| Submitted content | Modal fields, select values, context-menu targets survive or are explicitly rejected | Modal loss demonstrated; select/menu loss demonstrated at the generic boundary, deployment registration unverified. |
| Message/effect IDs | Interaction action distinct from attached message target; text root/source IDs coherent | F10/F11 reproduce; downstream reactions and per-turn guidance inspected/exercised locally. |
| Response completion | Inline and final interaction replies select the documented response operation | Gateway emits ordinary send; explicit follow-up control works; external connector behavior remains unverified. |
| Partial/unknown metadata | Define omission versus clearing and recovery of known parents | Q1/Q3 are explicitly conditional; no normal-provider omission claim. |
| Provider cache | Measure real request prefix and provider accounting if claiming a billed cache miss | Local rendered-byte differences only; no provider-cost or live cache-hit claim. |
Scope of verification: real pinned Python producers/consumers, real SQLite/SessionStore where specified, real renderer/config predicates and local transport dispatch. Connector/socket I/O, model/tool effects, provider-side follow-up completion, deployed menu registration, full gateway restart and destructive crash injection were not exercised. No claim of exploitation, cross-profile disclosure or measured production outage follows from these tests. The full repository suite and a Linux/macOS rerun were not performed in this review.
The merge-base and target-tip blobs were compared for the 18 load-bearing source files in the receipt: all 18 match between those two pins. The PR changes two of those implementation files; the other inspected ownership paths match head as well. That is pinned source-equivalence evidence, not a claim that a full current-main composition suite ran.
Hosted CI is separate: the head's PR check rollup is absent, and CI, Docker and Nix workflow runs are action_required; local tests do not turn those into executed hosted checks.
Conclusion
The current PR repairs the original label lifecycle and thread-key defects in its tested healthy-storage paths. The P0 class remains open. The evidence does not support reducing the remaining work to a single harmless prompt-cache residual: the unresolved seams include admission before metadata cancellation, retry settlement, restored response ownership, first-turn initialization, loss of submitted input, identifier roles, response-operation selection and healthy-after-failure cache recovery.
The acceptance condition is the observable contract at each producer → owner → consumer boundary. A plausible direction, a passing aggregate, and an inherited review conclusion cannot substitute for it. Equally, a conditional fixture or an overbroad earlier finding cannot be promoted into a production fact merely because it makes the review sound stronger.
| last carried for that chat (recorded as observed, so they survive a restart), and | ||
| names the user from the interaction itself: guild nick, else global name, else | ||
| username. The connector's `user_display_name` on a text message must resolve the | ||
| same way, or the two lanes name one user differently. `bufferId` is |
There was a problem hiding this comment.
[P0] F01 — Buffered passthrough has no ACK/settlement path, and duplicate admission is not fenced
Attribution: inherited.
The modified contract says a buffered forward is acknowledged after durable handoff. The passthrough dispatcher forwards bufferId, but the receiving callback does not consume or acknowledge it and bypasses the text lane's replay set.
Observed: local real WS frame dispatch → real adapter conversion, with final admission replaced by an inert recorder: identical buffer/provider interaction delivered twice → two admissions, zero ACK frames. A new interaction dispatches normally. Ordinary inbound control → one admission, two ACKs. The same results occur at merge base. Separately, the actual owner constructor generates two owners for repeated preparation of that keyless event; ordinary message-ID control is stable. The outbound ledger deliberately excludes slash replies; it cannot be cited as an inbound effect fence.
Consequence and limit: the gateway violates its documented buffer-settlement contract and admits duplicates. This run did not execute a model/tool twice, inspect a live connector cursor, or inject a process crash. The prior issue's stronger duplicate-effect claim is not fresh evidence from this run.
Repair/closure: define a distinct interaction action identity and durable admission/settlement record; ACK only after the specified durable handoff. Assert repeated delivery before claim, after claim, after effect/before ACK, and after ACK against harmless effects. An in-memory seen set or ACK immediately after task scheduling does not close the crash boundary.
Fresh receipt: lifecycle-parent / lifecycle-base-parent: frame_ack_replay, turn_owner, delivery_accounting. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Confirmed: _on_passthrough ignores buffer_id, so no inbound_ack is sent, and passthrough has no seen-set either. Older than this PR and a different class (transport settlement). It is already filed as #52581, with an open fix in #52583 (Adolanium) that acks after the handler returns. The missing replay fence belongs with that fix. Not duplicated here.
| self._seen_inbound[dedupe_key] = None | ||
| self._evict_oldest(self._seen_inbound, self._SEEN_INBOUND_MAX) | ||
| self._capture_scope(event) | ||
| await self._remember_discord_labels(event.source) |
There was a problem hiding this comment.
[P1] F02 — The awaited label write opens a cancellation window before admission but after the seen bit
Attribution: new pre-admission exposure on an inherited early-dedupe design.
The message is marked seen before this new await, while message admission remains later. The transport ACKs ordinary inbound after the callback returns. Its teardown can cancel the reader, which directly awaits frame dispatch.
Probe: hold the real record method behind a threading Event, start the real transport inbound handler, cancel the intake task after the worker enters but before admission, release the worker, then replay the identical buffered text frame on the same adapter. Final dispatch is an inert AsyncMock. Observed first admissions=0, total admissions=0, replay ACKs=1, seen entries=1. The worker can finish writing metadata despite cancellation. In the no-cancellation control: admissions=1, ACKs=2. On the exact base, this new label wait does not exist and both arms admit once.
Attribution: early seen insertion predates this PR; the awaited storage leg newly exposes this specific pre-admission interruption. This is a same-adapter cancellation/replay experiment, not a claimed full gateway reconnect or production incident. A fresh adapter may clear the process map; that does not make an ACK without admission valid.
Repair/closure: retain or release admission ownership explicitly across cancellation. A message not admitted must remain retryable; an admitted message must not execute twice. Cover worker completion after cancellation, same-adapter replay, duplicate overlap, and uncancelled control. Moving the write off-loop must remain; reverting to a synchronous write would trade this defect for loop blocking.
Fresh receipt: cancellation-head: 1 failed / 1 passed; cancellation-base: 2 passed. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Reproduced, fixed in 10b955a3. The window was older than this PR: _consume_prompt_response and _localize_inbound_media already awaited between the seen bit and handle_message, and the label write widened it. A cancellation before admission now releases the seen bit, so the replay is admitted once. It is limited to CancelledError (teardown); a frame that raises keeps today's behaviour, so a poison frame does not replay forever. Row cancelled: the intake task is cancelled while the write is held, then the frame is replayed and admitted once. Keeping the seen bit fails it.
| assert adapter._discord_interaction_to_event(_forward(**interaction)).source.chat_name is None | ||
| slash = adapter._discord_interaction_to_event(_forward(**interaction)) | ||
|
|
||
| assert build_session_key(slash.source) == build_session_key(message.source) |
There was a problem hiding this comment.
[P1] F03 — Startup auto-resume skips restored relay-backed Discord sessions
Updated 2026-10-01 after the author's call-site challenge. Attribution: inherited. This replaces the broader transport-kind claim and its required persistence redesign. Detailed caller evidence and reconciliation: follow-up in this thread.
Exact caller: GatewayStartupMixin._schedule_resume_pending_sessions, lines 597–604, invoked by the startup sequence. It restores the session source, calls _delivery_adapter_for(source), and skips the pending session when the result is None.
Mechanism: source serialization deliberately omits the live relay marker. After restoration, the direct selectors look for the logical Platform.DISCORD key; the registered adapter is keyed by Platform.RELAY. This path never asks fronts_platform. By comparison, resolve_delivery_transport and _resolve_injection_adapter select the same real relay correctly. The author's counterexample is valid for those paths.
New evidence: Linux / Python 3.14.7, original fa9259fdc1346c8d2ba5c1bd052358b559777789 and current 10b955a34d584fa30daf92353f45460e489e00d0; standalone and multiplexed default/primary; text-derived and interaction-derived session origins. The test uses actual GatewayRunner(config), WebSocketRelayTransport, RelayAdapter, production adapter publication, real session persistence/reload and the actual startup scheduler. Discord is present in the transport's real _identities. With one eligible persisted session and a passing real owner-authorization control, startup schedules zero and leaves resume_pending set. Routing the lookup through the existing relay-aware resolver yields one captured resume dispatch with the other gates unchanged.
Evidence boundary: the original probe established selector behavior only. The new control executes startup scheduling after adapter registration; no connector handshake, model turn or provider delivery is claimed. The positive-control terminal dispatch is a recorder.
Repair and regression: resolve the correct owner-scoped relay capability in the affected delivery path while preserving pinned transport/profile ownership and existing native-adapter behavior. Cover the real scheduler in the two modes above, plus disconnected-owner and native controls. The evidence does not establish that a new persisted transport-kind field is necessary. The lookup substitution is diagnostic, not a complete patch. The heartbeat and plugin-injection direct-selector consumers should be covered by the same repair; their consequences are source-traced here rather than independently reproduced.
There was a problem hiding this comment.
Not reproduced with the real transport. The marker is wire-invisible on purpose (SessionSource comment: a peer cannot forge it). A restored relay-origin Discord row in a relay-only config does resolve its transport: resolve_delivery_transport returns the RelayAdapter, because fronts_platform reads the transport's configured _identities, which a real WebSocketRelayTransport(url, "discord", ...) carries. Probe: from_dict(to_dict()) drops the marker, and resolution still returns RelayAdapter. "Native wins when both are configured" is that resolver's documented rule. I could not get the relay-only miss from any caller I traced (_resolve_injection_adapter, the restart-notice path). If your probe resolves through a different call site, please name it and I will file it.
There was a problem hiding this comment.
F03 call-site answer: startup auto-resume uses a different selector
The caller is GatewayStartupMixin._schedule_resume_pending_sessions(), specifically gateway/run_startup.py:597–604. The real startup sequence invokes it from _start_finish_wiring.
Your counterexample is correct for resolve_delivery_transport and the injection/restart-notice paths you traced. I independently confirmed the generic resolver and _resolve_injection_adapter select the real relay after restoration. Startup auto-resume takes this other branch:
source = self._restored_source(entry)
adapter = self._delivery_adapter_for(source)
if adapter is None:That branch logs the unavailable adapter and continues without scheduling the pending session.
Why the results differ
_delivery_adapter_for first calls _intake_adapter_for. A live source's relay marker selects the relay. A restored source deliberately lacks that marker; the remaining lookups use the logical Platform.DISCORD key in the applicable adapter map. The production primary registration stores the relay under Platform.RELAY. Those direct lookups do not consult fronts_platform.
resolve_delivery_transport does consult fronts_platform, whose advertised identity set comes from the real WebSocketRelayTransport constructor. _resolve_injection_adapter reaches that resolver. Your observation that the configured identity survives without the source marker is therefore part of the explanation.
%%{init: {"theme":"dark","themeVariables":{"darkMode":true,"background":"#090d14","primaryTextColor":"#f8fafc","lineColor":"#94a3b8","clusterBkg":"#0f172a","clusterBorder":"#475569","edgeLabelBackground":"#111827"}}}%%
flowchart TD
subgraph CANVAS[" "]
direction TB
subgraph FLOW["Startup resume and completion delivery"]
direction TB
S["Restored Discord session"]
S --> A["Startup auto-resume"]
S --> I["Watch / completion injection"]
A --> D["_delivery_adapter_for"]
D --> K["Literal Discord adapter lookup"]
K --> X["No native adapter: skip resume"]
I --> R["resolve_delivery_transport"]
R --> F["Relay fronts Discord"]
F --> L["RelayAdapter selected"]
end
subgraph LEGEND["Legend"]
direction TB
L_authority["Ownership / policy checks"]:::authority
L_transition["Transitions / pending work"]:::transition
L_durable["Durable state"]:::durable
L_success["Successful outcomes"]:::success
L_failure["Failures"]:::failure
end
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class D,K,R,F authority
class A,I transition
class S durable
class L success
class X failure
linkStyle default stroke:#94a3b8,stroke-width:1.5px
FLOW ~~~ LEGEND
style FLOW fill:#0f172a,stroke:#475569,color:#f8fafc
style LEGEND fill:#0f172a,stroke:#475569,color:#f8fafc
click S href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_startup.py#L575-L616" "Pinned call-site implementation"
click A href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_startup.py#L575-L616" "Pinned call-site implementation"
click I href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_notifications.py#L1245-L1265" "Pinned call-site implementation"
click D href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/authz_mixin.py#L285-L313" "Pinned call-site implementation"
click K href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/authz_mixin.py#L254-L283" "Pinned call-site implementation"
click X href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_startup.py#L595-L616" "Pinned call-site implementation"
click R href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/delivery.py#L57-L74" "Pinned call-site implementation"
click F href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L1164-L1170" "Pinned call-site implementation"
click L href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/delivery.py#L57-L74" "Pinned call-site implementation"
end
style CANVAS fill:#090d14,stroke:#090d14,color:#f8fafc
Source map: startup · direct selectors · watch/completion selector · relay-aware resolver. Original diagram rendered and visually checked with Mermaid CLI.
Fresh caller-level evidence
I reran this on fa9259fdc1346c8d2ba5c1bd052358b559777789 and 10b955a34d584fa30daf92353f45460e489e00d0, in fresh processes using Linux / Python 3.14.7. Each revision was checked with multiplexing off and with the default/primary profile under multiplexing, using both a decoded text source and an interaction-converter source.
The stronger control uses the actual GatewayRunner(config) constructor, WebSocketRelayTransport, RelayAdapter, and _publish_primary_adapter(Platform.RELAY, relay). Only Relay is enabled and registered. It creates a real session, calls SessionStore.mark_resume_pending(..., "restart_timeout"), constructs another runner/store over the same temporary home, restores the persisted origin, and executes the real startup scheduler. The shutdown path uses that same pending-marker producer.
| Observation | Standalone | Multiplexed primary/default |
|---|---|---|
Real transport advertises Discord; fronts_platform(DISCORD) |
Yes; True |
Yes; True |
| Restored relay marker / persisted transport profile | False / None |
False / "default" |
resolve_delivery_transport and _resolve_injection_adapter |
Same real RelayAdapter | Same real RelayAdapter |
_delivery_adapter_for(restored) |
None |
None |
| Real owner authorization under the explicit temporary allowlist | Passes | Passes |
| Eligible persisted sessions / scheduled resumes | 1 / 0 | 1 / 0 |
| Pending marker after the skipped attempt | Remains set | Remains set |
| Lookup-only positive control: scheduled resumes | 1 | 1 |
All unmodified scheduling runs log Skipping auto-resume … adapter not ready for discord. For sensitivity, I changed only the instance's delivery lookup to the existing resolve_delivery_transport and recorded the terminal resume dispatch. The same scheduler then dispatched once to the same relay. Candidate selection, freshness checks, persisted state and authorization were unchanged.
Boundary: this exercises constructed runtime objects and the production scheduling method after adapter registration. It does not exercise a connector handshake, an agent turn, or Discord delivery. The terminal dispatch recorder is the endpoint of the positive control. The lookup substitution is a diagnostic control, not a complete production patch.
Correction to my earlier finding
My earlier probe stopped at selector resolution. Linking startup as a consumer did not establish its scheduling result; this follow-up executes that branch. F03 should be narrowed to the direct-selector path demonstrated above. Your working alias-aware routes should remain explicit controls.
The wire-invisible relay marker is deliberate. I withdraw the earlier requirement that closure must introduce a persisted transport-kind discriminator: these results do not establish that a new field is necessary. The repair obligation is to make restored delivery consult the correct owner-scoped relay capability while preserving the existing profile/transport ownership rules.
For the issue you offered to file, the concrete title is “Startup auto-resume skips restored relay-backed Discord sessions through the direct adapter selector.” This is an inherited P1 resume failure, present at both checked revisions. The regression must cover the actual scheduler in both modes above. Restored heartbeat polling and plugin-message injection are additional direct-selector consumers to cover in the same repair; those sibling consequences are source-traced here, not separately reproduced.
There was a problem hiding this comment.
Thanks for naming the caller. Confirmed: _schedule_resume_pending_sessions goes through _delivery_adapter_for, and a restored source has no relay marker, so the Platform.DISCORD lookups miss a Relay registered under Platform.RELAY. It is filed as #131089 under your title and fixed in #131090, outside this PR.
In #131090, _delivery_adapter_for falls back to the Relay in the owning profile's own adapter map, using resolve_delivery_transport's rule: a live native adapter still wins, and a secondary profile whose bot is offline still gets None. Two more gaps on the same path are fixed there too. After a restart the Relay's egress caches are empty, so the revived turn's reply would have been declined; startup resume, the restored heartbeat poll and plugin injection now warm them before dispatch. And on Relay reconnect, resume now covers the platforms it fronts.
The regression drives the real scheduler over startup and reconnect, standalone and multiplexed-primary: red on main (0 == 1 scheduled), green with the fix. The negative controls (native wins; secondary fails closed) are pinned by sabotage checks. Credit to you in both.
|
|
||
|
|
||
| def _forward(**payload): | ||
| body = {"type": 2, "id": "i1", "channel_id": "ch1", "guild_id": "g1", "data": {"name": "status"}} |
There was a problem hiding this comment.
[P1] F04 — Read-only status consumes first-agent-turn initialization through timestamp-based newness
Attribution: inherited.
This /status fixture is converted and rendered, not run through its lifecycle. The real status handler gets or creates the row. The next existing-row lookup updates activity, before created_at == updated_at is used as newness.
Probe: real status handler and real SessionStore, with expensive model-route/status-display leaves substituted. After /status then first agent lookup, _hmwa_open_session returns (False, False) and emits zero session:start hooks. Direct-first control returns (False, True), emits once. Both head and base reproduce. Driving the real turn-preparation guard with manually supplied auto_skill calls the skill loader in the direct control but not after status; execution stops before the model.
Important narrowing: relay text reconstruction and interaction construction do not populate auto_skill. Thus the hook failure is demonstrated; unconditional relayed configured-skill loss is not established by that manually seeded fixture. The earlier issue wording needs that qualification.
Repair/closure: represent initialization for an agent/session incarnation separately from last activity. Regress real /status → first text and /status → first model-reaching command, with direct-first and later-turn controls. If relay skill parity is intended, exercise its actual producer separately rather than manufacturing auto_skill in every test.
Fresh receipt: lifecycle-parent and lifecycle-base-parent: status_first_turn, manual_skill_guard, auto_skill_wire. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Confirmed by trace: _hmwa_open_session decides newness by created_at == updated_at, /status creates the entry, and the first real message's lookup moves updated_at. Older than this PR, gateway-wide, different class. Filed as #130304, crediting you.
|
|
||
| store._routing_db.get_meta = flaky | ||
| assert adapter._discord_interaction_to_event(_forward(**interaction)).source.chat_name is None | ||
| slash = adapter._discord_interaction_to_event(_forward(**interaction)) |
There was a problem hiding this comment.
[P1] F05 — Admitted modal submissions lose their user-entered values before the consumer
Attribution: inherited.
The interaction-kind branch identifies type 5 in its support comment but supplies text only for types 2 and 3. The resulting event carries neither modal data nor raw payload. Discord's modal-submit schema identifies submitted components as the user's values.
Probe: registered passthrough callback → real converter with a thread-scoped modal carrying Résumé and Next week using Label/Text Input components. Dispatch receives text='', raw_message=None and no submitted values, while the thread key is valid. A separate legacy-shaped input with Unicode/empty optional field gives the same loss. Normal command and button controls retain their input. Real inbound preparation returns empty modal input; history reconstruction excludes that empty user row. Both baseline and head share the normalization omission.
Consequence/limit: user-authored values are discarded at the local accepted boundary. No model call or provider UI was exercised, so this is not a claim about a particular live bot's modal registration or the final displayed response.
Repair/closure: normalize supported modal component trees into a defined, lossless structured/text representation, or explicitly reject/route unsupported modal submissions. Assert multiple values, Unicode, empty optional values and thread identity at the actual consumer, not only source labels.
Fresh receipt: consumers-isolated: modal value correctness fails; empty input/replay consequence passes; lifecycle both pins confirm. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Confirmed on the converter: a type-5 body becomes text == "". Older than this PR, different class (content normalization). Filed with F06 and F09 as #130305, crediting you.
|
|
||
| def _forward(**payload): | ||
| body = {"type": 2, "id": "i1", "channel_id": "ch1", "guild_id": "g1", "data": {"name": "status"}} | ||
| body.update(payload) |
There was a problem hiding this comment.
[P2] F09 — Context-menu commands are flattened to slash names without their selected target
Attribution: inherited.
Outer interaction type 2 is not synonymous with a slash/chat-input command. Discord's application-command data schema distinguishes the inner type and context-menu target_id. The current converter emits only /name and option values; event construction preserves neither target nor resolved message.
Probe: a type-2 message command with inner data.type=3, target_id=selected-message-99 and matching resolved content becomes '/summarize', metadata={}, raw_message=None. The target cannot be recovered from the event. The nested slash control becomes '/skill run deploy' with its expected arguments. The merge-base adapter gives the same projection.
Consequence/limit: a context-menu request reaching this generic boundary loses what it was invoked on; additionally unknown slash dispatch is not a general substitute for processing that selected message. This does not establish that the external connector currently registers or forwards this command type.
Repair/closure: branch by inner command subtype and preserve the resolved target in a defined consumer contract, or explicitly reject unsupported context-menu types. Tests need target-distinct cases, user/message command controls and existing slash/subcommand compatibility. Do not equate a parsed slash string with preserved command intent.
Fresh receipt: consumers-isolated: context-menu target assertion fails; exact-base equivalence passes. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Confirmed: a context-menu command arrives as a bare /summarize, with no target_id and no resolved object. Filed in #130305.
| class Forward: | ||
| platform, method, path = "discord", "POST", "/interactions/bot1" | ||
|
|
||
| Forward.body = json.dumps(body).encode() |
There was a problem hiding this comment.
[P2] F10 — Interaction action IDs are advertised and used as provider message targets
Attribution: inherited.
Discord defines payload.id as interaction ID and message.id as the attached message when present. The converter conflates the former with source.message_id, while only hp1 prompt responses retain attached message identity. The new fixture's interaction ID therefore needs consumer assertions, not merely a label assertion.
Observed: component id=interaction-1 attached to message=attached-message-77 → real session context exposes HERMES_SESSION_MESSAGE_ID=interaction-1, event.message_id=None. Cached guidance says the triggering message is supplied per-turn, but the event-ID note supplies none. Actual processing lifecycle emits local stub react operations targeting the interaction ID; normal text targets its message ID. Slash without any attached message has the same misleading source target. These behaviors predate the PR. No external reaction API or message disclosure was tested.
Repair/closure: represent interaction action identity separately from an actual message target and any reply anchor. Components may have a real attached target; slash interactions may have none. Preserve action identity for F01 without treating it as a reply/react ID. Multiple clicks on one attached message must remain distinct actions. Tests should cover tool context, rendered promise, per-turn note, reactions and persistence owner, not only constructor fields.
Fresh receipt: consumers-isolated: component/slash identity assertions fail; lifecycle both pins: reaction_id_kind, delivery_accounting, turn_owner. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
- Provider interaction ID assigned as source message ID
- Actual attached message retained only for hp1
- Declared message target semantics
- Event ID and distinct reply/ledger fields
- Task-local tool context receives source ID
- Cached promise of a per-turn ID
- Per-turn note reads event ID
- Event/source ID fallback for reactions
- Processing reaction lifecycle consumers
- Separate action/persistence-owner consequence
There was a problem hiding this comment.
Confirmed and older than this PR. Filed as #130306 with F11 and the earlier inline. See the reply on line 1260 for why it stays out of this PR.
| return _event_from_wire({"text": "hi", "message_type": "text", "source": { | ||
| "platform": "discord", **chat, "scope_id": "g1", | ||
| "user_id": user, "user_name": "ben", "user_display_name": "Ben D", | ||
| "chat_name": chat_name, "chat_topic": chat_topic, "message_id": f"{user}:{chat_name}"}}) |
There was a problem hiding this comment.
[P2] F11 — Root-only text message IDs never reach source-based context consumers
Attribution: inherited.
Source construction reads only source.message_id, whereas event construction reads the frame's root message_id. This added fixture supplies the nested field every time, so it cannot observe that split.
Probe: two accepted text frames with equal chat/user/labels and the same root message_id; only one additionally supplies source.message_id. Both events have text-message-1. The root-only source has None. The actual tools capability gate is enabled with temporary configuration and a synthetic nonfunctional marker, and the pinned renderer returns different guidance because source ID presence is part of the key. Disabling tools makes rendered bytes match, which explains why tools-disabled parity assertions conceal it. The wire decoder is blob-identical at base/head.
Consequence/limit: source context and event consumers disagree about a known real message. This is local accepted-wire evidence, not a claim that every connector chooses the root-only representation.
Repair/closure: define and enforce event/source message-ID normalization and a conflict policy at the wire boundary. A root-only frame should not lose its known message ID in tool context; equal root/nested forms should converge. Include missing-ID and disagreeing-ID cases without conflating interaction action IDs from F10.
Fresh receipt: consumers-isolated: root-only source-ID assertion fails; unchanged base/head decoder blob. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Confirmed in _event_from_wire: root and nested ids are filled independently. The fixture now carries both. Filed in #130306.
| an HTTP POST. The gateway processes the decoded request through its normal agent | ||
| path (a Discord interaction is decoded to a `MessageEvent` and handled like a | ||
| message; the reply egresses over the outbound / `follow_up` path). `bufferId` is | ||
| message; the reply egresses over the outbound / `follow_up` path). An |
There was a problem hiding this comment.
[P1] F12 — Normal interaction replies select ordinary send rather than the documented follow-up capability
Attribution: inherited.
This changed paragraph retains the promise that a deferred interaction reply egresses through follow_up. The actual inline response path and final reply path route to ordinary send. send_follow_up exists separately; the source census finds its adapter entry called by tests, not the normal reply path.
Probe: convert a real interaction; capture its scope; drive actual inline reply and send_final_ledgered with a harmless response and the in-memory connector. Both emit op='send', reply_to=None, zero follow-ups. Explicit send_follow_up on the same key emits op='follow_up'. Both base and head reproduce.
Consequence/limit: the gateway's operation selection does not implement its written capability-response contract. A separate channel post is not proof that the provider's deferred interaction was completed. The external connector may have behavior not present here; this review did not inspect a live provider response or claim an observed stuck Discord UI.
Repair/closure: select the interaction response capability through both inline/busy and normal final paths while retaining ordinary send for ordinary text. Test emitted op, session/action correlation and content with the connector contract, then verify deferred response completion externally. The documented thread capability-key obligation remains necessary once that actual caller exists; a method-only green test is not integration evidence.
Fresh receipt: lifecycle-parent and lifecycle-base-parent: interaction_reply_route; production caller census. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Verified that nothing in the tree calls send_follow_up; replies leave as op: send. Not shown to be a defect. §3.1 says the reply goes out "over the outbound / follow_up path". Whether a send into an interaction's channel leaves the deferred response open is decided by the connector, which is not in this repo. If it does, the fix needs the reply anchor from #130306, so it is noted there rather than filed alone.
| return self._discord_chat_labels[key] | ||
| store = getattr(self, "_session_store", None) | ||
| try: | ||
| recorded = store.chat_labels(Platform.DISCORD, scope, chat_id) if store is not None else None |
There was a problem hiding this comment.
[P2] F13 — Cold label reads synchronously wait on the database writer lock on the event loop
Attribution: introduced; same-head prior finding retained.
The getter reaches SessionDB.get_meta, which deliberately uses the main writer lock, not a WAL read context. A write transaction holds that lock. Since conversion is called synchronously by the async passthrough callback, a cold chat stalls loop progress while waiting.
Probe: real stored labels, cold adapter and real _execute_write transaction held on a threading Event. Schedule an independent coroutine heartbeat; release the transaction via a separate 300ms timer. The heartbeat runs only after release at head, while the same assertion passes at the merge base. The transaction/getter are real, not mocked; the hold/release is controlled. A source-bound receipt confirms the expected labels still return and the writer completes without error.
Consequence/limit: this establishes where a contended read waits and that unrelated loop work is prevented during it. The hold duration is not a measured production latency or outage. It confirms the earlier cold-read finding.
Repair/closure: move the cold read behind an awaited off-loop boundary or an equivalent nonblocking store API before constructing the source. Preserve failure/absence distinctions from F07. Regress a real contended writer plus an independent coroutine, alongside warm-hit and exception-retry controls. A point SELECT is not necessarily nonblocking when obtaining its lock is synchronous.
Fresh receipt: state-head-final: cold-read-contention fails; state-base: same responsiveness assertion passes. Head fa9259fdc1346c8d2ba5c1bd052358b559777789; native Windows; inert external I/O. Full reconciliation and test boundaries are in the review body.
Code under discussion — producer, state owner, and consumers:
There was a problem hiding this comment.
Same as the 15:24 inline on line 938. Fixed in 10b955a3 (cold read in a worker, before admission). The failure and absence cases (F07) are kept apart.
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Follow-up review at fa9259f.
This PR now closes the rename and restart bug I raised on the earlier head. Discord chat labels are recorded when the text event is observed, instead of being recovered from whichever session origin happens to be newest. That keeps the interaction lane aligned after a channel rename, restart, peer activity, session reset, or late session creation. The interaction's current nickname, global name, or username also wins over an older cached user label.
I read all four changed files and traced text ingress through the label recorder, durable SessionStore metadata, restart lookup, thread key construction, and interaction conversion. The new tests cover warm state, restart, threads, rename, peer rename, late session creation, read retry, and user-name precedence. The focused label and passthrough suites passed on the exact head and on a clean composition with current main, with 17 tests in each run. The diff check also passed.
GitHub still reports no hosted check runs or commit statuses, and all four workflow runs are action_required. I am not repeating the remaining current-head findings because the existing reviews already cover them. This review closes only my previous stale-label blocker. It is not an approval of the PR as a whole.
…while the database is away - The interaction lane's cold read of the recorded chat labels ran on the event loop inside the converter. get_meta takes the database's writer lock, so a cold interaction could hold the loop behind an unrelated write. The read now runs in a worker from _on_passthrough, before admission. - With no database handle (failed open, backoff, JSONL fallback), chat_labels returned None, the same as "never labelled", and the adapter cached it until restart. It now raises, so the next interaction asks again. - record_chat_labels returned normally without a handle, and the adapter marked the labels recorded, so the same labels were never written after the database came back. It now returns whether it wrote; only a write counts as recorded. - A message with no labels at all is recorded too, so an interaction does not bring back a name or topic the text lane no longer carries. - The reader can be cancelled while a frame is being prepared (now including the label write), after the frame was marked seen but before admission. The connector replays the un-ACKed frame and the seen bit dropped the replay. A cancellation before admission now releases the seen bit. The test renders the pinned prompt with Discord tools loaded too, where the thread parent and the triggering-message line also have to match. Found by andrexibiza (review of NousResearch#128797: F02, F07, F08, F13, and the all-null observation question). The unavailable-write and label-removal cases were also covered by NousResearch#129032. Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com>
|
@andrexibiza thanks for the follow-up and the class audit. @ehz0ah thanks for confirming the rename/restart fix.
Tests: still two functions, now 14 cases. Each fix fails its own row when sabotaged ( #129032, cross-tested both ways at
Hosted CI still waits for a maintainer to approve the run. |
There was a problem hiding this comment.
Batch review at 10b955a.
Keep Relay as a separate cohesive PR: it owns durable replay, interaction conversion, delivery settlement and persistent label observation. It belongs in the same cross-platform turn review as #128788 and #128722, while retaining its transport implementation boundary.
The new 10b955a commit repairs the pre-admission cancellation window: my same stable-message cancellation check is RED at fa9259f and GREEN here. It also moves cold label reads off the loop, distinguishes unavailable storage from absent labels, requires an actual successful write before memoizing persistence, and records cleared labels. The refreshed supplied Relay selection passes 41 tests; its 14-case label file now exercises real passthrough handoff with fake gateway delivery. Credit these repairs rather than retaining the old F02/F07/F08/cold-read findings unchanged.
One cache-consistency failure remains consumer-verified: two adapters sharing a SessionStore do not share cache invalidation. A peer warmed with an old label keeps it after another adapter observes a rename; a peer warmed with absence keeps None after the first label arrives. Both variants fail through real _on_passthrough at this head and in the full batch composition. The existing peer-invalidation thread therefore remains current even though cold reads and storage recovery are repaired. Use observation generations/invalidation or revalidation for both positive and negative cache entries, retaining restart and storage-fault controls.
The other existing ledger groups—partial thread ancestry, interaction payload/identifier fidelity, reply settlement, persisted transport identity and actual first-agent-turn initialization—still need explicit dispositions. I did not freshly reproduce each of those in this pass. #130309 remains an open related identifier proposal; it cannot be credited as part of this head.
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":20,"rankSpacing":32}}}%%
flowchart TD
subgraph FLOW["Shared labels and local cache lifetime"]
direction TB
O["Initial shared label or absence"] --> C["Peer warms local label cache"]
C --> N["Writer observes a new label"]
N --> S["Shared store now has current label"]
C --> R["Peer interaction uses warm cache"]
S -.->|no invalidation| R
R --> X["Old label or absence persists"]
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Amber: transition / pending<br/>Teal: durable state<br/>Red: failure<br/>Gray: context / transient"]
FLOW ~~~ LEGEND
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class O,N entry
class C context
class S durable
class R transition
class X failure
click O href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L947-L962" "Pinned cache owner"
click C href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L957-L962" "Pinned cache owner"
click N href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L909-L928" "Pinned cache owner"
click S href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L927-L938" "Pinned cache owner"
click R href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L946" "Pinned cache owner"
click X href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L946" "Pinned cache owner"
Diagram source: gateway/relay/adapter.py:945. Related-set consolidation map: #128788.
I reviewed the 13 open PRs in this burst as one set, including existing review threads, changed producers, downstream consumers, and a local composition of all 13 full PR diffs. The composition applied without conflicts and passed 419 tests in 18 files, with 6 skips, using the repository's canonical per-file test runner. This is targeted regression evidence, not a full repository test run or a live Slack, Discord, Relay or provider test. Five additional local consumer/lifecycle mechanisms still fail (6 failing checks, including positive and negative peer caches): native Discord role handoff, Slack middle-cancellation ordering, Slack reply-context ownership, lazy provider-stream permit reentry, and Relay stale peer label caches. Their owning reviews contain runnable checks and precise boundaries. GitHub returned no check runs or commit status entries for the reviewed heads; local results do not establish CI success.
Runnable current-head regression (temporary real SessionStore, fake transport and final gateway callback; no live connector): save the block at tests/gateway/relay/test_review_peer_label_cache.py, then run the canonical command. Both cache variants intentionally assert the expected invariant and fail.
from unittest.mock import AsyncMock
import pytest
from gateway.config import GatewayConfig, Platform, PlatformConfig
from gateway.session import SessionStore
from tests.gateway.relay.test_relay_discord_interaction_labels import _message, _forward
from tests.gateway.relay.test_relay_interactive import _adapter
@pytest.mark.asyncio
@pytest.mark.parametrize("warm_positive", [True, False])
async def test_warm_peer_cache_sees_current_shared_label_observation(tmp_path, warm_positive):
config = GatewayConfig(platforms={Platform.DISCORD: PlatformConfig(enabled=True, token="x")})
store = SessionStore(tmp_path, config)
writer, _ = _adapter(platform="discord")
peer, _ = _adapter(platform="discord")
for adapter in (writer, peer):
adapter.set_session_store(store)
adapter.handle_message = AsyncMock()
chat = {"chat_id": "ch1", "chat_type": "group"}
if warm_positive:
await writer._on_inbound(_message(chat))
async def interaction():
await peer._on_passthrough(_forward(member={"user": {"id": "u1", "username": "ben"}}))
return peer.handle_message.await_args.args[0].source.chat_name
assert await interaction() == ("Hermes Server / #ops" if warm_positive else None)
await writer._on_inbound(_message(chat, user="u2", chat_name="Hermes Server / #triage"))
assert await interaction() == "Hermes Server / #triage"scripts/run_tests.sh -j 2 tests/gateway/relay/test_review_peer_label_cache.pyIndividual pinned-head check at 10b955a: 41 supplied Relay tests pass; the additional pre-admission cancellation check passes; 2 peer-cache regression variants fail.
scripts/run_tests.sh -j 2 tests/gateway/relay/test_relay_discord_interaction_labels.py tests/gateway/relay/test_relay_interactive.py tests/gateway/relay/test_ws_transport.py| map is empty until the text lane speaks again, so ask the session store for what that lane | ||
| last recorded; otherwise a first interaction re-renders the prompt the cache still holds.""" | ||
| key = (scope, chat_id) | ||
| if key in self._discord_chat_labels: |
There was a problem hiding this comment.
[P2] Invalidate positive and negative warm caches across peer adapters. Revalidating the existing peer-cache finding: with two adapters sharing a real SessionStore, warm the peer via _on_passthrough, then let the writer observe a new label. The peer keeps the old label, or None if it was warmed before any record existed. Both current-head and full-composition variants fail. Moving cold reads off the loop and making unavailable storage retryable does not invalidate an already populated local entry. Give label observations a shared generation/invalidation mechanism or revalidation policy; preserve both positive and negative controls. The body includes the runnable real-consumer check with inert transport delivery.
There was a problem hiding this comment.
The check still needs two RelayAdapters writing labels into one SessionStore, and the gateway doesn't run that topology. Under multiplex, run_adapters.py refuses Relay on a secondary profile, because it is shared process-level ingress. So one process has one RelayAdapter, and two gateway processes can't share a home because of the per-home host lock. Within that single adapter, a new observation updates its own entry, so the positive and negative entries this cache holds can't go stale from another writer.
I'm leaving this unchanged in this PR. If you can show a supported configuration where a second writer exists (for example a native Discord adapter and Relay both recording into the same store), I'll reproduce it and add the invalidation with both controls.
| if await self._consume_prompt_response(event): | ||
| return | ||
| await self._localize_inbound_media(event) | ||
| except asyncio.CancelledError: |
There was a problem hiding this comment.
Verified closure of the pre-admission cancellation finding. The stable-message review check that produced 0 deliveries after cancellation/replay at fa9259f now passes and produces one delivery at 10b955a. The supplied cancelled case also exercises the real label write boundary. Keep this RED-to-GREEN regression with the cache and database-retry repairs in this cohesive Relay PR. This establishes cancellation before admission only; settlement after gateway handoff and the wider transport ledger still need their own explicit acceptance contracts.
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Approval conclusion
GitHub does not allow this account to submit a formal approval because it lacks explicit repository review access. The technical conclusion is approval.
Reviewed head: 10b955a34d584fa30daf92353f45460e489e00d0
What this changes
This PR fixes a real identity mismatch between relayed Discord text messages and interactions. Before this change, text messages carried connector-resolved chat labels and display names, while slash commands and component interactions could use raw or missing values. Those differences changed the pinned session context even when both events belonged to the same conversation.
The relay adapter now records bounded chat-label observations after inbound replay checks, restores them through the session store, derives the current Discord display name, maps thread interactions to the text lane's session fields, and handles cancellation before admission. SessionStore provides the metadata-backed label record, the regression suite covers warm, restart, storage-fault, thread, user-name, and cancellation paths, and the connector contract documents the resulting identity rules.
Assessment
The state owner and call path are appropriate. Chat labels are learned at the relay ingress boundary, durable reads are kept off the event loop, failed storage reads remain retryable, and successful writes are required before persistence is memoized. The change does not alter the public session-key schema or add an unrelated runtime abstraction.
I also rechecked the earlier partial-thread-parent concern. Discord still describes the interaction channel as partial, but its current channel documentation explicitly says interaction payloads contain full channel data for channels the app can act on and that parent_id is not obfuscated. On that current contract, a thread interaction without its parent is not established as a reachable platform payload. The synthetic omission probe demonstrates how the code would behave if that contract changed, but it is not sufficient evidence for a blocking defect today.
Validation
- The 41 focused relay interaction and transport tests passed.
- The 64 focused session-store and session tests passed.
- I read the full four-file diff and traced the changed adapter and persistence paths through their consumers.
- GitHub reports no hosted check runs or commit-status entries for this head. Local validation therefore does not establish hosted CI status.
Overall judgment
No blocking finding remains in this PR's verified scope. The fix is focused and proportionate to the prompt-stability defect. I approve this head; hosted CI remains a separate merge gate.
There was a problem hiding this comment.
Current-head follow-up: two persistence/lifecycle defects remain
Reviewed head: 10b955a34d584fa30daf92353f45460e489e00d0
Base: 8d30c4eaabd85edb77a02fef6c5388d9344ef80c
I do not consider this head merge-ready yet. The latest commit does close several things from the earlier audit: the cold label read is off the event loop, unavailable storage is distinguished from an absent label record, a cancelled pre-admission frame releases the process seen-bit, cleared labels are recorded, and the interaction user name comes from the current payload.
Two corrections to my own prior review are important:
- Withdrawn: the two-
RelayAdapterpeer-cache invalidation probe used a topology the gateway does not support. The supported runtime has one process-level Relay adapter, and I do not have evidence for a second supported writer into that same label cache. I am not retaining that finding. Author's topology rebuttal. - Narrowed / externalized: the restored-relay delivery miss is real specifically at startup auto-resume. The author reproduced the concrete
_schedule_resume_pending_sessions → _delivery_adapter_forcaller, filed #131089, and opened #131090. I am not duplicating it as an inline defect on this diff. Confirmation.
What remains on this head is introduced by the current label-persistence implementation itself:
- P1 — the label
state.dbwriter is not lifecycle-owned.asyncio.to_thread(record_chat_labels)can survive cancellation of the relay reader.disconnect()considers the reader cancelled and returns, while shutdown's DB-close gate cannot see this arbitrary default-executor worker. A late write can therefore arrive after the close/checkpoint boundary;SessionDBexplicitly reopens a writer in that case (#94736), which is the same post-close generation class the shutdown code is designed to fence. - P2 — the observation write inherits the generic 20-second SQLite patience budget.
record_chat_labelscallsset_meta, which reaches_execute_writewith_WRITE_PATIENCE_S = 20.0. This repository already defines_ACTIVITY_WRITE_PATIENCE_S = 0.5and explicitly says observation-only heartbeat/label writes must not ride the full routine patience budget. The event loop is free now, but the serialized relay reader is still awaiting this write before dispatch/ACK, so one contended label observation can hold every later frame on that websocket behind it.
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":20,"rankSpacing":32}}}%%
flowchart TD
subgraph FLOW["Relay write lifetime and shutdown"]
direction TB
A["Relay inbound frame"] --> B["_remember_discord_labels"]
B --> C["await to_thread(record_chat_labels)"]
C --> D["set_meta → generic 20s write patience"]
D --> E["WS reader waits: ACK + next frame blocked"]
C --> F["disconnect cancels relay reader"]
F --> G["awaiter exits; worker thread survives"]
G --> H["pre-close gates see no tracked worker"]
H --> I["SessionDB close / checkpoint"]
I --> J["late write reopens state.db after close"]
click A href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/ws_transport.py#L930-L940" "Relay handler and ACK ordering"
click C href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L923-L935" "Changed label-write call"
click D href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/hermes_state.py#L470-L480" "Write patience budgets"
click E href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/ws_transport.py#L930-L940" "ACK happens after the inbound callback"
click F href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/ws_transport.py#L493-L502" "Reader cancellation during disconnect"
click H href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_shutdown.py#L2044-L2084" "Shutdown liveness gates"
click I href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/run_shutdown.py#L2085-L2104" "SessionDB close phase"
click J href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/hermes_state.py#L965-L1026" "Late-write reopen path"
click B "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L909-L938" "Label observation and persistence"
click G "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L923-L935" "Awaited worker call"
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Amber: transition / pending<br/>Red: failure"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class A entry
class B,C,F,I transition
class D,E,G,H,J failure
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
The labels retain the review's evidence distinctions.
Evidence boundary
For the P1 ordering, I ran a controlled primitive-level probe with real SQLite WAL contention and the exact asyncio.to_thread cancellation primitive: worker entered at +0.0002s; the awaiting task completed cancellation at +0.0514s; a simulated DB-close boundary was reachable at +0.0515s; after the lock was released at +0.1520s, the worker committed at +0.1524s. That probe establishes the cancellation/worker ordering. The repository source establishes the production endpoints: reader cancellation, shutdown's tracked-worker gate, and _reopen_after_close_locked. I am not claiming the primitive harness itself is a full gateway-shutdown reproduction; the missing regression is exactly that lifecycle integration test.
For P2, the budget is direct source evidence: record_chat_labels → set_meta, set_meta → _write_sql with no patience override, and the 20.0s routine / 0.5s observation budgets plus the explicit label-write comment. The relay transport awaits the inbound callback before the buffered ACK, so moving the write off-loop does not make this protocol lane non-blocking.
CI / closure state
| Commit | Hosted check state observed |
|---|---|
40322be |
Nix succeeded; CI and Docker were cancelled |
cd581c7 |
workflow startup_failure |
6aad417 |
CI / Docker / Nix require action |
0f3aa86 |
CI / Docker / Nix require action |
fa9259f |
CI / Docker / Nix require action |
10b955a |
CI / Docker / Nix require action |
So targeted local evidence may be green, but hosted CI has not established a green current head, and the commit chain is not all green.
The broader interaction audit also still has open, separately filed class defects (#130304, #130305, #130306, plus #131089/#131090). I am not relabeling those as newly introduced by this patch; they do mean the larger relayed-interaction class should not be described as closed yet.
Closure for this PR head: lifecycle-own the label writer through DB shutdown; give label observations the repository's short observation-write contention budget with retry semantics; add regressions that hold a real writer across both the inbound path and shutdown; then rerun the current-head targeted suites and hosted checks.
| # events one at a time, so records land in observation order) but off the loop: it is a | ||
| # disk write. | ||
| try: | ||
| recorded = await asyncio.to_thread(store.record_chat_labels, source) |
There was a problem hiding this comment.
[P1] Keep this state.db writer inside shutdown ownership
asyncio.to_thread gets the blocking SQLite call off the event loop, but it does not make the worker cancellable. The transport's disconnect() cancels _reader and awaits the reader task. If cancellation lands on this await, _on_inbound can unwind while record_chat_labels is still running in the default executor.
That matters because the later DB-close gate only fences the gateway-owned executor and its explicitly counted cron/API/deferred workers. An arbitrary to_thread label writer is not in those counts. The shutdown code explicitly says a live writer after SessionDB.close() can reopen the handle and split the WAL generation, and _execute_write really does call _reopen_after_close_locked() when _conn is None.
Controlled evidence with real SQLite WAL + this cancellation primitive: worker entered +0.0002s; awaiting task completed cancellation +0.0514s; the caller could cross the DB-close boundary +0.0515s; lock release +0.1520s; worker commit +0.1524s. That proves the worker can outlive the cancellation boundary. The repository source above supplies the production close/reopen consequence.
The current cancelled test does not cover this: it calls held.set() immediately after intake.cancel() and never drives the gateway DB-close phase, so it does not hold the writer alive across shutdown.
Please put this write on a lifecycle-owned future/executor that the pre-close gate can see, or explicitly register these metadata DB workers and refuse/await DB close while one is alive. Merely swapping to another helper that also uses an untracked to_thread would preserve the hole.
Regression contract: block a real label write, invoke the real relay disconnect/shutdown sequence, and assert that state.db is not closed/checkpointed while the worker is live and that no post-close _reopen_after_close_locked write occurs. Keep the existing un-ACKed-frame replay assertion as the companion control.
| setter = self._routing_db_method("set_meta") | ||
| if setter is None: | ||
| return False | ||
| setter(self._chat_labels_meta_key(source.platform, source.scope_id, source.chat_id), |
There was a problem hiding this comment.
[P2] Give this observation write the short label/activity contention budget
record_chat_labels calls generic set_meta without a patience override. That reaches _execute_write with _WRITE_PATIENCE_S = 20.0. The same storage class already defines _ACTIVITY_WRITE_PATIENCE_S = 0.5 and explicitly documents that observation-only activity heartbeat/label writes must never wait out the full routine patience budget.
This call is awaited before message dispatch. The websocket reader processes inbound frames serially, and its _on_inbound sends the buffered ACK only after the adapter callback returns. So the event loop is no longer blocked, but one unrelated SQLite writer can still hold the entire relay ingress lane — current frame, ACK, and every later frame on that socket — for up to the generic 20-second retry window.
The failure semantics already make a short budget safe: if this observation cannot be written, the adapter catches the exception, leaves _discord_labels_recorded unset, dispatches the message, and retries on a later observation.
Use a specialized observation-meta setter (or a set_meta(..., patience_s=...) API) with the existing short observation budget rather than changing generic metadata writes globally.
Regression contract: hold state.db with an independent BEGIN IMMEDIATE longer than the observation budget; deliver a labeled relay message; assert dispatch/reader progress is bounded by the short budget rather than ~20s and the record is left retryable; release the writer, deliver the next observation, and assert the durable labels are then written.
There was a problem hiding this comment.
Supplemental review — the cache finding has a supported one-adapter trigger
Reviewed head: 10b955a34d584fa30daf92353f45460e489e00d0.
This supplements my existing review and specifically reconciles the topology rebuttal and the later “approval conclusion”. The repairs already credited at this head remain credited: pre-admission cancellation, off-loop cold reads, unavailable-store retry, successful-write accounting, cleared labels, current interaction user identity, and thread-key parity.
[P2] The warm label cache is not bound to the durable store generation
The topology rebuttal establishes one narrow fact: normal multiplex startup does not intentionally configure a second RelayAdapter. It does not establish that the per-instance cache cannot diverge from the SessionStore.
This PR puts the durable label record in the routing state.db via record_chat_labels / chat_labels, while a warm interaction returns _discord_chat_labels[key] immediately without consulting that store or any store generation.
There is a supported mutation source that needs one RelayAdapter, not two: live snapshot restore.
restore_quick_snapshotrestores.dbfiles through_safe_restore_db._safe_restore_dbexplicitly uses SQLitebackup()so a gateway, dashboard, or another CLI session may keep the database open while its contents are restored.- The repository has a dedicated regression,
test_restore_state_db_live_connection, whose whole point is that the live holder must observe restored state rather than continue serving pre-restore state.
That gives a supported one-adapter sequence:
| Step | Observable state |
|---|---|
| Relay text lane observes labels A | warm cache = A; gateway_chat_labels:... = A |
| quick snapshot is taken | snapshot contains A |
| same live RelayAdapter observes rename B | warm cache = B; durable row = B |
| another CLI restores the snapshot while the gateway remains live | durable row becomes A; RelayAdapter remains alive |
| next interaction hits line 945 | returns B without reading the restored store |
| cache eviction / adapter restart | miss reads A |
The same restored application state therefore produces B or A solely from cache lifetime. That is the invariant the earlier exact-head peer probe exposed; live restore supplies the supported state transition that the two-adapter fixture was standing in for. The host lock and secondary-Relay prohibition do not protect this path.
The negative-cache half is the same mechanism: a cached (None, None) also bypasses every later durable generation until eviction. The existing two-control probe already demonstrated that the hit branch is insensitive to a changed durable row; the supported restore path shows that such a change does not require a second RelayAdapter.
Required closure: bind the cache to the durable-store generation/restore epoch, invalidate it when the backing state generation changes, or revalidate warm entries under an explicit freshness policy. Keep both positive and negative controls. If the intended contract is instead that live Relay observations intentionally survive a state restore, encode and test that rule explicitly—including the required behavior after eviction—because the current implementation silently changes semantics when the entry falls out of the map.
Evidence boundary: I am not claiming two configured RelayAdapters are a production topology. That part of the rebuttal is correct. The defect is that cache correctness currently depends on that being the only possible writer/generation transition, while the repository explicitly supports live state.db restoration.
The later approval conclusion does not reconcile the review ledger
The approval conclusion says “No blocking finding remains in this PR's verified scope” after reporting 41 Relay tests and 64 session tests. Those passing selections are not counter-evidence to the open mechanisms below; none of them exercises the live-restore/cache-generation path above, and the review does not adjudicate the outstanding threads.
Two additional facts were already established before that approval was posted:
-
[P1] Startup auto-resume miss is now author-confirmed. After I named the actual caller in F03, the author confirmed at 2026-10-02 01:56 UTC that
_schedule_resume_pending_sessionsuses the wrong direct selector for a restored relay-backed Discord source, filed #131089, and opened #131090. The approval was submitted at 2026-10-02 08:11 UTC. I am not re-attributing that inherited defect to this PR; I am saying an open separate fix is not evidence that the reviewed head or merge composition is already clear. -
[P1] The changed connector contract still contradicts the implementation. This PR says an interaction reply “egresses over the outbound /
follow_uppath”. On the same head, both inline replies and normal final replies route through ordinarysend, which emitsop: "send".send_follow_upis a separate capability. The author already confirmed in the existing F12 thread that nothing in the tree calls it for the normal reply path. The external Discord consequence remains bounded/unverified; the source-vs-changed-contract contradiction is verified. Closure is either to wire the documented capability and test emitted operation/correlation, or change the contract text to describe what actually owns deferred-interaction completion.
So the approval's aggregate green counts cannot establish “no blocking finding remains.” They establish only that those selected assertions passed.
Ownership / generation map
%%{init: {"theme":"dark","themeVariables":{"background":"#0d1117","lineColor":"#94a3b8","clusterBkg":"#0d1117","clusterBorder":"#30363d"},"flowchart":{"nodeSpacing":20,"rankSpacing":32}}}%%
flowchart TD
subgraph FLOW["Restored durable state and warm cache"]
direction TB
A["Text lane observes labels A"] --> S["state.db meta = A"]
S --> Q["Quick snapshot S"]
Q --> B["Same RelayAdapter observes rename B"]
B --> C["Warm cache = B"]
B --> D["state.db meta = B"]
Q --> R["Supported live snapshot restore"]
R --> E["state.db meta restored to A"]
C --> H["Interaction cache hit"]
H --> X["Returns B; store not read"]
E --> M["Eviction / restart"]
M --> Y["Cache miss reads A"]
X --> Z["Same restored state, result depends on cache lifetime"]
Y --> Z
click C href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L962" "Warm cache owner"
click H href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L962" "Cache-hit return"
click S href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/session_persistence.py#L238-L264" "Durable label record"
click D href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/session_persistence.py#L238-L264" "Durable label record"
click E href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/hermes_cli/backup.py#L1480-L1548" "Snapshot restore"
click R href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/hermes_cli/backup_restore.py#L74-L141" "Live-safe SQLite restore"
click Y href "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/tests/hermes_cli/test_backup.py#L1534-L1567" "Live-connection restore regression"
click A "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L909-L928" "Pinned implementation at reviewed head"
click B "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L909-L928" "Pinned implementation at reviewed head"
click Q "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/hermes_cli/backup.py#L1400-L1417" "Pinned implementation at reviewed head"
click M "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L962" "Pinned implementation at reviewed head"
click X "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L944-L951" "Pinned implementation at reviewed head"
click Z "https://github.com/NousResearch/hermes-agent/blob/10b955a34d584fa30daf92353f45460e489e00d0/gateway/relay/adapter.py#L940-L962" "Pinned implementation at reviewed head"
end
LEGEND["Legend<br/>Blue: entry / incoming<br/>Amber: transition / pending<br/>Teal: durable state<br/>Green: success<br/>Red: failure<br/>Gray: context / transient"]
FLOW ~~~ LEGEND
classDef entry fill:#172554,stroke:#60a5fa,color:#eff6ff,stroke-width:2px
classDef authority fill:#3b0764,stroke:#c084fc,color:#faf5ff,stroke-width:2px
classDef transition fill:#451a03,stroke:#fbbf24,color:#fffbeb,stroke-width:2px
classDef durable fill:#134e4a,stroke:#2dd4bf,color:#f0fdfa,stroke-width:2px
classDef success fill:#052e16,stroke:#4ade80,color:#f0fdf4,stroke-width:2px
classDef failure fill:#450a0a,stroke:#f87171,color:#fef2f2,stroke-width:2px
classDef context fill:#1e293b,stroke:#94a3b8,color:#f8fafc,stroke-width:2px
class A,B entry
class S,D,E,Q durable
class C,H context
class R,M transition
class Y success
class X,Z failure
style FLOW fill:#0d1117,stroke:#30363d,color:#f0f6fc
style LEGEND fill:#1e293b,stroke:#94a3b8,color:#f8fafc,font-size:11px
In this diagram, state.db and snapshots are teal; the warm cache is gray; restore and eviction/restart are amber. Labels state the evidence; color does not imply independent verification.
Current disposition
At this head, the positive repairs remain real, but the record does not support the statement that no blocking finding remains. Close the cache-generation invariant above, reconcile the changed follow_up contract, and ensure the already-confirmed restored-Relay delivery fix is present in the actual merge composition rather than merely open elsewhere.
Hosted CI is also not green at this head: GitHub currently reports the CI, Docker, and Nix workflow runs as action_required, with no combined commit-status entries. That may be a fork-approval gate rather than a test failure, but it is not a green merge receipt.
| map is empty until the text lane speaks again, so ask the session store for what that lane | ||
| last recorded; otherwise a first interaction re-renders the prompt the cache still holds.""" | ||
| key = (scope, chat_id) | ||
| if key in self._discord_chat_labels: |
There was a problem hiding this comment.
[P2] One RelayAdapter is enough to reproduce the generation split
This new evidence supersedes my earlier withdrawal of the peer-cache finding in the 12:35 UTC current-head follow-up. I withdrew it there because the two-RelayAdapter fixture used a topology the gateway does not configure and, at that point, I had not established another supported writer/generation transition. The live-restore path below supplies exactly that missing reachability evidence.
The topology reply is correct that normal multiplex startup does not intentionally create a second RelayAdapter. That is not the only supported way the backing record changes.
This cache hit returns the local value without consulting the SessionStore or a store generation. Hermes explicitly supports live state.db restore: restore_quick_snapshot writes snapshot pages through SQLite backup() so a gateway/dashboard/CLI connection may remain open, and test_restore_state_db_live_connection pins that behavior.
So the supported sequence is: observe A → snapshot → same adapter observes B → restore snapshot while the gateway stays live → durable gateway_chat_labels:... is A again → this line still returns B until eviction/restart, after which the same restored state reads A. No second gateway and no second RelayAdapter are involved.
That is the same cache-coherence failure the earlier positive/negative peer probe exposed; the supported live restore supplies the legal generation transition. Bind warm entries to a durable generation/restore epoch, invalidate/revalidate them, or explicitly define and test why live observations intentionally survive restore and how eviction preserves that policy.
| an HTTP POST. The gateway processes the decoded request through its normal agent | ||
| path (a Discord interaction is decoded to a `MessageEvent` and handled like a | ||
| message; the reply egresses over the outbound / `follow_up` path). `bufferId` is | ||
| message; the reply egresses over the outbound / `follow_up` path). An |
There was a problem hiding this comment.
[P1] This changed contract still describes a call path the head does not execute
The paragraph says a normal decoded interaction reply egresses through follow_up. On this exact head, inline and final replies both enter the ordinary send lifecycle and RelayAdapter emits op: "send"; send_follow_up remains a separate session-bound method. The author already confirmed in the existing F12 thread that the normal reply path has no send_follow_up caller.
The external provider consequence is still bounded: this does not by itself prove a stuck Discord UI. But the changed contract is presently false as a description of this repository. Either route the intended interaction response through the documented capability and assert emitted op/session correlation, or rewrite this paragraph to name the actual owner of deferred-response completion.
…while the database is away - The interaction lane's cold read of the recorded chat labels ran on the event loop inside the converter. get_meta takes the database's writer lock, so a cold interaction could hold the loop behind an unrelated write. The read now runs in a worker from _on_passthrough, before admission. - With no database handle (failed open, backoff, JSONL fallback), chat_labels returned None, the same as "never labelled", and the adapter cached it until restart. It now raises, so the next interaction asks again. - record_chat_labels returned normally without a handle, and the adapter marked the labels recorded, so the same labels were never written after the database came back. It now returns whether it wrote; only a write counts as recorded. - A message with no labels at all is recorded too, so an interaction does not bring back a name or topic the text lane no longer carries. - The reader can be cancelled while a frame is being prepared (now including the label write), after the frame was marked seen but before admission. The connector replays the un-ACKed frame and the seen bit dropped the replay. A cancellation before admission now releases the seen bit. The test renders the pinned prompt with Discord tools loaded too, where the thread parent and the triggering-message line also have to match. Found by andrexibiza (review of NousResearch#128797: F02, F07, F08, F13, and the all-null observation question). The unavailable-write and label-removal cases were also covered by NousResearch#129032. Co-authored-by: JoaoMarcos44 <joaomarcosdias444@gmail.com> (cherry picked from commit 10b955a)
…observation budget Two lifecycle gaps in the label record, from review of NousResearch#134586 / NousResearch#128797: - A reader cancelled mid write left the worker running on a thread shutdown does not track, and shutdown closes SessionDB right after the adapters disconnect, so the write could land after the close and reopen state.db behind its checkpoint (NousResearch#101093). The writer is now started on first use and RelayAdapter.disconnect() waits (bounded by the disconnect budget) for it. - The write used the routine 20 s write patience while the relay reader awaits it before the next frame. set_meta takes patience_s, and the label record uses SessionDB._ACTIVITY_WRITE_PATIENCE_S; a busy database raises and the next message retries, as before. Both new tests fail on the previous head (write lands after disconnect returns; 21 s stall) and pass here. Also rewords the interaction decoder docstring to name the text-lane session key it matches.
…t and user labels A relayed Discord message carries connector-resolved chat_name / chat_topic and user_display_name; a relayed interaction (slash command, component, modal) was built from the raw body: user_name = raw username, no chat labels, and inside a thread chat_type "group" with no thread_id / parent_chat_id. All of these feed _ephemeral_change_key, so message -> interaction -> message re-rendered the pinned session-context prompt each way, and a slash in a thread keyed another session. - the interaction names its user from its own payload: guild nick, else global name, else username (discord.py display_name order); - the text lane's last (chat_name, chat_topic) per (guild, chat) is kept in a bounded map and recorded in state_meta when observed (off the loop, retryable when the database is away); the interaction lane reads the map, then the record after a restart; - a thread interaction derives chat_type=thread, thread_id, parent_chat_id from the partial channel object; - a cancellation before admission releases the inbound seen bit so the connector's replay is admitted. Squash-port of #128797 (six commits) onto current main, where _discord_interaction_to_event lives in gateway/relay/adapter_discord_interaction.py; _on_passthrough moves there with it to keep adapter.py under its health cap.
What does this PR do?
A relayed Discord message carries connector-resolved
chat_name,chat_topicanduser_display_name; a forwarded interaction (slash command, component) is the raw Discord body and carried none of them. Both land in one session, and the pinned session-context prompt renders those labels, so a slash turn re-rendered the cached prefix and the next message rendered it back.Chat labels. The relay adapter keeps the last
chat_name/chat_topicthe text lane carried per (guild, channel) (bounded, FIFO) and the interaction lane uses them. A message with no labels at all counts too, so an interaction never brings back a name or topic the text lane stopped carrying. The session store also records them, per chat, at the moment they are observed: onestate_metarow, written only when new or changed, in a worker thread, awaited before the message is dispatched. After a restart or an eviction the interaction lane reads that record, also in a worker thread, becauseget_metatakes the database's writer lock. It does not read session origins: an origin is written when its session is created and inherited by a reset, so creation order is not observation order (a delayed first turn creates its session after a newer rename was seen; a reset peer gets a freshcreated_aton old labels).Store faults stay retryable. A failed read is not cached. Neither is a read that found no database handle (open failed, backoff):
chat_labelsraises then instead of answering "never labelled". A write only counts as recorded when it happened (record_chat_labelsreturns whether it wrote), so labels seen while the database was away are written by the next message once it is back.Cancellation before admission.
_on_inboundmarks a frame seen before its pre-admission awaits (the label write, prompt-response handling, media download). Teardown can cancel the reader there, before admission and before the ACK; the connector then replays the frame, and the seen bit dropped the replay. A cancellation before admission now releases the seen bit, so the replay is admitted once.User name. The interaction names its user from its own
member/user: guild nick, else global name, else username, which is what the text lane'suser_display_name(discord.py'sdisplay_name) resolves to at the same moment. Nothing is remembered from the text lane, so a nickname change shows up on the next interaction and the result does not depend on cache lifetime.Threads. An interaction sent inside a thread keys like the text lane's thread message:
chat_type: thread,thread_id= the thread channel,parent_chat_idfrom the interaction's partialchannelobject (type 10/11/12 marks a thread,parent_idnames the parent). Before, it was keyedgroupwith no thread, so a slash command inside a relayed thread landed in a per-user session beside the thread's shared one (discord:group:th1:u1vsdiscord:thread:th1:th1). Plain channel and DM interactions keep their session keys.Found while checking every surface for the #128722 bug class. Limits: a chat's labels are recorded from the first message this version sees there (labels in session origins written by an older gateway are not back-filled), and a slash command in a chat the text lane never spoke in has no labels anywhere; the first message then renders them once. Without
state.db(JSONL fallback) nothing is recorded and only the in-process map applies.Not in this PR (same review, different classes, filed separately): first-turn
session:startafter a read-only command (#130304), interaction content normalization for modals, selects and context menus (#130305), relay interaction/message identity (#130306). Bufferedpassthrough_forwardACK is #52581 / #52583.Related Issue
Fixes #128796
Type of Change
Changes Made
gateway/relay/adapter.py:_remember_discord_labels(text lane) updates the in-process map, label-less messages included. When the labels are new or changed it awaitsSessionStore.record_chat_labelsin a worker thread. They count as recorded only if it wrote._on_passthroughfills the interaction's chat labels from_discord_chat_labels_for, now async: a map miss readsSessionStore.chat_labelsin a worker thread and caches the answer unless the read failed._on_inboundreleases the seen bit when it is cancelled before admission.chat_type,thread_idandparent_chat_idfrom the partialchannelobject.gateway/session_persistence.py:record_chat_labels(source) -> bool/chat_labels(platform, scope_id, chat_id)over the routing store's existingstate_metakey/value table (gateway_chat_labels:<platform>:<scope>:<chat>). No database handle: the write returns False, the read raises. No entry lock, no routing-index save.tests/gateway/relay/test_relay_discord_interaction_labels.py: two invariant tests.website/docs/developer-guide/relay-connector-contract.md§3.1 states the thread derivation, where an interaction's chat labels come from, and that the connector'suser_display_namemust resolve nick → global name → username. No zh-Hans mirror of that page. The store-fault and cancellation fixes make the code match what §3.1 and the inbound ACK comment already say; no doc change for those.How to Test
scripts/run_tests.sh tests/gateway/relay/test_relay_discord_interaction_labels.pymain(sources at currentorigin/main): 13 failed, 1 passed (the username-only row passes on both).fa9259fd: 9 failed, 5 passed. Every restart case fails the off-loop assertion: the cold read ran on the loop thread.__pycache__cleared between runs):read-unavailablefails;write-unavailablefails;cancelledfails (the replay is not admitted);clearedfails;source.message_id(the identity split measured on fix(relay): preserve Discord identity and durable buffered inputs #129032): 11 cases fail on the tools-loaded rendering.tests/gateway/relay/(all files),tests/gateway/test_session.py,tests/gateway/test_session_store_lock_io.py: green on this head and on a merge with currentorigin/main(62c42e9d).tests/gateway/test_session_db_recovery.py::test_recovered_db_rows_survive_fallback_structural_savefails under parallel load on the merge base too, and passes alone. Ruff clean. Python 3.14.Compared with #129032
Both PRs fix #128796 and agree on the thread derivation. Cross-tested both ways at
10b955a3(this PR) andf93d93e3(#129032, itsgateway/tree swapped into this checkout):source.message_id, so the prompt flips (teknium1's review, finding 1)memberchannelomitsparent_idcancelled, and the absent-nick row)._on_passthroughinstead of calling the converter directly (the labels are filled there now).relay_discord_context,observe_relay_discord_context,cached_relay_discord_context). Their behaviour (off-loop cold read, unavailable database, DM payload identity) is covered by this PR's rows.membercarriesnick(null when unset).channel.parent_idis missing: Discord documents that interactions get full channel data for channels the app can act on, andparent_idis never obfuscated.RelayAdapter(run_adapters.pyrefuses a secondary relay under multiplex).Co-authored-by): the user name is derived from the interaction alone; a label removed upstream clears the record; a write without a database handle is not counted.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass. I ran only the related files listed above viascripts/run_tests.sh, not the full suite.Documentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A