Conversation
iainlane
changed the title
alt-glitch
added
type/bug
|
Additional source-backed validation for this direction (no private event content): I checked this against the Matrix Client-Server spec and related Hermes issues/PRs. Matrix rich replies are identified by That matches #18396 and the surrounding implementation attempts (#18680 cache, #28962 fallback parsing + parent fetch, #39611 API fetch). In practice, the important acceptance boundary is:
This PR looks aligned with the durable fix because it moves the actual replied-to message text/author into the agent-facing event instead of relying on stripped fallback text. Happy to test direct-reply plus digest/cron-reply scenarios if useful. Sources checked: Matrix rich replies and threading in the Client-Server spec — https://spec.matrix.org/latest/client-server-api/#rich-replies and https://spec.matrix.org/latest/client-server-api/#threading. |
|
+1 for that! |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a verified Matrix reply-context gap. Current main records the parent event id but no parent text (plugins/platforms/matrix/adapter.py:2736-2774), while gateway injection requires reply_to_text (gateway/run.py:10592-10606).
Problems
- The new
reply_to_author_id/reply_to_author_namefields are not injected into the agent-facing prefix:gateway/run.py:10592-10606reads only reply text and the own-message flag. The claimed author context is therefore not delivered. await get_event(...)at proposedplugins/platforms/matrix/adapter.py:3588has no timeout. The current sync loop awaits dispatch (adapter.py:2313), and dispatch awaits its handler tasks (adapter.py:2359), so a stuck lookup can delay sync processing.
Suggested changes
- Render the resolved author in the per-turn reply prefix and add an assertion against the constructed agent input.
- Bound
get_eventwithasyncio.wait_for, degrade to empty context on timeout, and test a non-completing lookup.
Automated hermes-sweeper review.
| message_id=event_id, | ||
| reply_to_message_id=reply_to, | ||
| reply_to_text=reply_ctx.text, | ||
| reply_to_author_id=reply_ctx.author_id, |
There was a problem hiding this comment.
gateway/run.py:10592-10606 never reads reply_to_author_id or reply_to_author_name; it injects only reply text and the own-message flag. Please include the author in the per-turn reply prefix (with an integration assertion), or remove these fields and narrow the claimed behavior.
There was a problem hiding this comment.
Done in f727d73e8: the gateway now reads these fields and names the author in the per-turn prefix, preferring reply_to_author_name and falling back to reply_to_author_id, producing [Replying to Bob: "..."]. Own-message replies keep the "your previous message" wording, and events without author fields keep the old prefix, so other platforms are unaffected (Signal, which also sets these fields, picks up the author for free). The integration assertion is test_reply_author_reaches_agent_prefix in 808cfd136: it dispatches a Matrix reply through the adapter and asserts the author the adapter resolved appears in the prefix built by _prepare_inbound_message_text.
| return _EMPTY_REPLY_CONTEXT | ||
|
|
||
| try: | ||
| event = await get_event(RoomID(room_id), EventID(reply_to_event_id)) |
There was a problem hiding this comment.
Please bound this lookup with asyncio.wait_for and degrade on TimeoutError. Current main awaits handler tasks in _dispatch_sync (adapter.py:2359), so a hung homeserver request delays sync-cycle completion despite the intended best-effort behavior; add a never-completing get_event regression test.
There was a problem hiding this comment.
Done in 808cfd136: the lookup is now wrapped in asyncio.wait_for with a 10 second bound (_reply_context_timeout_seconds). On timeout it logs at debug, matching the existing fetch-failure path, and returns the empty context so the message is handled without reply context and the sync cycle is not held up. The regression test is test_hung_get_event_times_out_and_degrades, which drives a never-completing get_event through the text handler and asserts the message still reaches handle_message with the context fields unset.
teknium1
added
sweeper:risk-message-delivery
|
Additional local integration and adversarial validation against current
The PR's focused reply/injection tests passed after a clean local rebase ( Before merge, I recommend classifying the parent sender through |
…#51803 followup) Addresses two trust-boundary gaps found during review of PR NousResearch#51803: 1. Matrix adapter _resolve_reply_context() now checks _matches_ignored_user_pattern() before surfacing replied-to sender identity or text. A reply referencing a message from a blocked/ignored sender will no longer leak their content to the agent. 2. gateway/run.py _prepare_inbound_message_text() now neutralizes reply_to_text and reply_to_author_name via neutralize_untrusted_inline_text() to collapse embedded newlines. Prevents prompt injection via fake headings or override blocks in the [Replying to ...] prefix. Regression tests added: - test_ignored_sender_reply_context_suppressed (test_matrix.py) - test_reply_text_neutralized_collapses_newlines (test_reply_to_injection.py) - test_reply_author_neutralized_collapses_newlines (test_reply_to_injection.py) - test_reply_snippet_truncated_to_neutralize_limit (updated)
When a user replies to a message on Matrix, the inbound event only carries a reference to the replied-to event via `m.relates_to.m.in_reply_to.event_id`. It does not include the body or sender of that earlier message. The adapter recorded the id in `reply_to_message_id` but never fetched the referenced event, so the agent had the pointer without the content and could not tell what the user was replying to. Fetch the replied-to event from the homeserver with the client's `get_event` API and populate the `reply_to_text`, `reply_to_author_id`, `reply_to_author_name` and `reply_to_is_own_message` fields that `run.py` already injects as a quoted-context prefix. Threaded messages whose `m.in_reply_to` is a thread fallback (`is_falling_back`) are thread linkage rather than a genuine reply, so they are skipped to avoid noisy context. The fetch is best-effort: a failed or forbidden lookup degrades to no context and never blocks handling of the new message. The reply-fallback stripping that was inline in the text handler is lifted into a shared helper and now also applies to the fetched body. The media handler, which previously dropped reply metadata entirely, carries the same context.
MessageEvent carries reply_to_author_id and reply_to_author_name (set by the Signal and Matrix adapters), but _prepare_inbound_message_text never read them: the per-turn prefix said only [Replying to: "..."], so the agent knew which text was referenced but not whose message it was. Include the author in the prefix, preferring the display name and falling back to the platform id, producing [Replying to Bob: "..."]. Replies to the bot's own message keep the existing 'your previous message' wording. Events without author fields keep the old prefix unchanged.
_resolve_reply_context awaits client.get_event with no bound, and the sync loop awaits message handlers in _dispatch_sync, so a homeserver that accepts the request but never answers would stall sync-cycle completion on what is meant to be a best-effort context fetch. Wrap the lookup in asyncio.wait_for with a 10 second bound. On timeout, log at debug (matching the existing fetch-failure path) and return the empty context so the message is handled without reply context. A regression test drives a never-completing get_event through the text handler and asserts the message still goes out with the context fields unset. Also add an integration test asserting the author resolved by the adapter reaches the gateway's per-turn reply prefix.
808cfd1 to
96f8abf
Compare
Resolving a Matrix reply fetches the parent event from the homeserver, so its body and display name are content from another participant that the adapter now pulls in itself. Two trust boundaries were missing on that path. The parent's sender was never classified. The adapter has an authorization check registered by GatewayRunner, but _resolve_reply_context copied the sender and body without consulting it, so a reply to someone off the allowlist reached the agent as ordinary context. Classify the sender through _is_sender_authorized() and carry the tri-state result on the event, so the gateway can label off-list content [unverified] the way the Slack and Discord thread-context paths already do. Our own messages skip the check: the allowlist governs who may drive the agent, not what the agent said. The reply prefix also interpolated both the display name and the quoted body raw. Both are attacker-controlled and the prefix is prepended verbatim to the model turn, so an embedded newline let either break out of the bracketed line and pose as a fresh markdown section. Run both through neutralize_untrusted_inline_text() before building the prefix; the quote keeps its existing 500-char bound rather than the helper's default.
|
Both trust-boundary findings are addressed in
Confirmed, and the reproduction was accurate: the callback is registered for every adapter by
Also confirmed. Both now go through Worth flagging for a maintainer: this prefix was unneutralised on Regressions cover both scenarios you named: an unauthorized participant in an allowed room (asserting the |
Name the replied-to author in the [Replying to ...] prefix, preferring the display name and falling back to the platform id, so the agent knows whose message is being referenced rather than only its text. Both the quoted snippet and the author name come from another participant and are prepended raw to the model turn, so collapse them to a single inert line with neutralize_untrusted_inline_text. An embedded newline would otherwise let either pose as a fresh markdown section (a fake "## SYSTEM" heading). This overlaps with the snippet neutralisation in flight as PR NousResearch#65184; the call shape is the same. Add a tri-state reply_to_author_authorized field to MessageEvent, mirroring _is_sender_authorized (None means no check registered). The Matrix adapter classifies the parent's sender through the registered allowlist check on the fetch path only: fetched content from someone off the allowlist is tagged "[unverified] " in the prefix so the agent treats it as background rather than instructions. Inline fallback quotes carry no verdict, and the bot's own messages are never checked; the allowlist governs who may drive the agent, not what it said. The requeued-event copy in _busy_queue_command carries the field too. _resolve_reply_context now returns a frozen _MatrixReplyContext dataclass instead of a widening tuple. Ported from NousResearch#51803 (@iainlane), retargeted onto the spec-correct threading model of this branch.
|
Retiring this PR in favour of #62088, which now carries everything here: the adapter-side reply resolution is subsumed by its typed-relation model, and the hardening this PR added (author naming, tri-state reply_to_author_authorized with [unverified] labelling, and neutralisation of the reply prefix) has been ported there with tests and credit. The prefix neutralisation itself also exists independently as #65184, which predates both. |
|
@iainlane why did you close the PR? |
Name the replied-to author in the [Replying to ...] prefix, preferring the display name and falling back to the platform id, so the agent knows whose message is being referenced rather than only its text. Both the quoted snippet and the author name come from another participant and are prepended raw to the model turn, so collapse them to a single inert line with neutralize_untrusted_inline_text. An embedded newline would otherwise let either pose as a fresh markdown section (a fake "## SYSTEM" heading). This overlaps with the snippet neutralisation in flight as PR NousResearch#65184; the call shape is the same. Add a tri-state reply_to_author_authorized field to MessageEvent, mirroring _is_sender_authorized (None means no check registered). The Matrix adapter classifies the parent's sender through the registered allowlist check on the fetch path only: fetched content from someone off the allowlist is tagged "[unverified] " in the prefix so the agent treats it as background rather than instructions. Inline fallback quotes carry no verdict, and the bot's own messages are never checked; the allowlist governs who may drive the agent, not what it said. The requeued-event copy in _busy_queue_command carries the field too. _resolve_reply_context now returns a frozen _MatrixReplyContext dataclass instead of a widening tuple. Ported from NousResearch#51803 (@iainlane), retargeted onto the spec-correct threading model of this branch.
5 participants
When you reply to an earlier message on Matrix, the agent could see that your message was a reply but not what it replied to. So it could not use the quoted message, and answers to "what about this one?" had no idea what "this" was. Now the agent is handed the replied-to message's text and author, so it can respond in context.
When a user replies to a message on Matrix, the inbound event only carries a reference to the replied-to event via
m.relates_to.m.in_reply_to.event_id. It does not include the body or sender of that earlier message. The adapter recorded the id inreply_to_message_idbut never fetched the referenced event, so the agent had the pointer without the content and could not tell what the user was replying to.Fetch the replied-to event from the homeserver with the client's
get_eventAPI and populate thereply_to_text,reply_to_author_id,reply_to_author_nameandreply_to_is_own_messagefields. On the gateway side,run.pypreviously injected only the quoted text; it now reads the author fields too and names the author in the per-turn reply prefix, preferring the display name and falling back to the platform id, so the agent sees[Replying to Bob: "..."]. Replies to the bot's own message keep the existing "your previous message" wording, and events without author fields keep the plain prefix, so other platforms are unaffected.Threaded messages whose
m.in_reply_tois a thread fallback (is_falling_back) are thread linkage rather than a genuine reply, so they are skipped to avoid noisy context. The fetch is best-effort and bounded by a 10 second timeout: a slow, failed or forbidden lookup degrades to no context and never blocks handling of the new message or holds up the sync loop.The reply-fallback stripping that was inline in the text handler is lifted into a shared helper and now also applies to the fetched body. The media handler, which previously dropped reply metadata entirely, carries the same context.