Skip to content

fix(send_message): sanitize active-wake snippets - #49749

Closed
verybigdog wants to merge 1 commit into
NousResearch:mainfrom
verybigdog:contrib/send-message-wake-sanitization
Closed

fix(send_message): sanitize active-wake snippets#49749
verybigdog wants to merge 1 commit into
NousResearch:mainfrom
verybigdog:contrib/send-message-wake-sanitization

Conversation

@verybigdog

Copy link
Copy Markdown
Contributor

Summary

  • sanitize token-like, path-like, and oversized fragments before active-wake snippets are injected into a gateway wake event
  • adds focused coverage for redaction and truncation behavior

Tests

  • python3 -m pytest tests/tools/test_send_message_tool.py::TestActiveWakeReceipt -q -o addopts=
  • python3 -m py_compile tools/send_message_tool.py

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists labels Jun 20, 2026
@verybigdog
verybigdog marked this pull request as ready for review June 20, 2026 18:24
(cherry picked from commit 722aedbf85058762686b479328d5c803d556bd89)
(cherry picked from commit 7add5c495782d2bbf82dce49ef182db3a99ca5f4)
@verybigdog
verybigdog force-pushed the contrib/send-message-wake-sanitization branch from 13eaaa2 to 0049cd9 Compare June 23, 2026 07:50

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for isolating the sensitive-text handling. The current draft needs substantive rework before it can provide the stated active-wake contract.

Problems

  • tools/send_message_tool.py:120 returns the full sanitized text; it never truncates it. The same value is injected into the wake event at :542 and :662, so the PR's claimed oversized-fragment behavior is absent.
  • tools/send_message_tool.py:654-656 hard-codes chat_type="group" and user_id="hermes-active-wake". Current gateway/session.py:936-949 derives isolated group sessions from that participant ID, so this can create a synthetic participant session instead of waking the intended operator session. The test currently asserts that synthetic ID at tests/tools/test_send_message_tool.py:3217.
  • The linked integration PR #49593 identifies this same limitation: its active-wake implementation removes the synthetic participant and distinguishes scheduled, accepted, and started receipt states.

Suggested changes

  • Bound and test the sanitized wake snippet after redaction.
  • Route through the resolved target session and add real session-key coverage for isolated groups, shared groups, threads, and DMs.
  • Consolidate the receipt semantics with #49593 rather than retaining scheduling as proof of session acceptance.

Automated hermes-sweeper review.

# Drop lines that only contained a stripped MEDIA directive. This preserves
# user-visible prose ("hello") while avoiding synthetic wake clutter.
cleaned_lines = [line.rstrip() for line in redacted.splitlines() if line.strip()]
return "\n".join(cleaned_lines).strip()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return never applies a size limit, so an arbitrarily large non-sensitive message is still injected into the synthetic wake event. Please add an explicit post-redaction bound and a regression test; the PR summary specifically promises truncation of oversized fragments.

chat_id=str(chat_id),
chat_type="group",
thread_id=str(thread_id) if thread_id else None,
user_id="hermes-active-wake",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fixed synthetic participant ID routes isolated group sessions to ...:<chat_id>:hermes-active-wake, not the intended operator session. Current gateway/session.py uses source.user_id when group-per-user isolation is enabled. Route using the resolved target session without this participant identity, and test that path against the real session-key builder.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 14, 2026
@verybigdog

Copy link
Copy Markdown
Contributor Author

Recommend closing as a duplicate superseded by #37865. This PR re-implements the active-wake path against a pre-#37865 main and reintroduces the exact patterns #37865 removes: SessionSource with chat_type=group and a synthetic user_id='hermes-active-wake' (which forges a participant session instead of waking the operator's real session), plus a model-facing trigger_agent field. Despite the title, _sanitize_active_wake_text only redacts — it never truncates — so the advertised bounded-snippet behavior is absent. #37865 already routes via replayed chat_type/user_id/user_id_alt and wakes with the composed, bounded notification text rather than raw bodies, so no raw-body bound is needed on that path. If any residual oversized-fragment concern remains, it is a ~2-line length cap on #37865's notification composition, not this redaction stack. Proposing duplicate-closure; nothing is closed by this note. (This PR is not closed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants