Skip to content

fix(gateway): keep shared-group sender prefix outermost - #67886

Open
jsky-james wants to merge 1 commit into
NousResearch:mainfrom
jsky-james:fix/speaker-tag-order
Open

fix(gateway): keep shared-group sender prefix outermost#67886
jsky-james wants to merge 1 commit into
NousResearch:mainfrom
jsky-james:fix/speaker-tag-order

Conversation

@jsky-james

Copy link
Copy Markdown

Summary

Fix shared-group sender attribution when inbound messages receive prepend-style enrichment.

Previously _prepare_inbound_message_text() applied [DisplayName] before image/STT/audio/video/document, Discord, and reply context processing. Later enrichment could prepend text ahead of the sender tag, so media turns no longer began with the sender identity.

This change:

  • applies the sanitized sender prefix once after all inbound enrichments (including reply and @ context expansion),
  • keeps channel_context outermost, so only the triggering turn is attributed,
  • preserves untagged behavior for per-user group sessions and missing sender names.

Regression coverage

Adds a shared-group regression covering an audio attachment, reply-to context, channel backfill, control-character sanitization, and exactly-once sender attribution. Also covers missing sender names.

Verification

  • python -m pytest -q tests/gateway/test_shared_group_sender_prefix.py tests/gateway/test_reply_to_injection.py tests/gateway/test_video_context_note.py
    • 11 passed
  • python -m compileall -q gateway/run.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 20, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #44705, #13939, and #16850's sender-attribution work. This PR is a narrower ordering fix: it keeps the existing shared-group prefix outermost after media/reply enrichment.

@x7peeps

x7peeps commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Closing as duplicate

@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 the focused regression and ordering fix. The current main implementation still has the reported ordering issue: gateway/run.py:12873-12898 adds sender/backfill before later media and reply prepend paths (:13014-13031, :13118-13132).

Problems

  • The moved block predates main's Slack current-author-ID behavior. Current main adds | Slack user <@...> at gateway/run.py:12888-12891, with coverage in tests/gateway/test_shared_group_sender_prefix.py:74-131; preserve that nested branch when moving the prefix or shared Slack turns lose their verified mention target.

Suggested changes

  • Salvage the reorder over current gateway/run.py and add an enriched shared-Slack regression asserting the outer prefix retains <@U...>.

This is an automated hermes-sweeper review.

Comment thread gateway/run.py
# a hostile name can masquerade as a fake markdown section
# (mirrors the same field's treatment in
# build_session_context_prompt via _format_untrusted_prompt_value).
_safe_user_name = neutralize_untrusted_inline_text(source.user_name)

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.

When salvaging this moved construction onto current main, retain the Slack-specific | Slack user <@{source.user_id}> augmentation added by commit 503c0c0e51 before constructing this prefix. Otherwise enriched shared Slack turns lose the verified current-author mention target.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Five PRs address or reference the two issues: #18711 exposes the verified Slack author mention, #66507 fixes internal-event impersonation and redundant async follow-ups, #67886 repairs sender-prefix ordering, while #13939 broadens ID-qualified attribution and #44705 labels unnamed shared-session participants. The latter three attribution changes overlap in gateway code but address distinct causes and carry documented compatibility or rebase requirements.

Related pull requests

Duplicates

No listed PRs are exact duplicates: #13939 and #44705 overlap on sender attribution, while #67886 changes ordering in the same path but fixes a separate enrichment bug. #18711 is the closed source implementation cherry-picked through #69320, and #66507 addresses the separate internal-event/async-completion issue.

Suggested consolidation

Keep #66507 open with a salvage path for #66480, and keep #67886 open with a salvage path that rebases its ordering change onto current main while preserving the Slack mention augmentation and adding the requested regression. Author action for #13939: rebase or split out the viable attribution work after resolving the contributor's identity-precedence, configuration, documentation, and coverage findings; author action for #44705: split the participant-labeling fix from unrelated hunks and add the Yuanbao no-double-prefix guard. Leave #18711 closed as the source already integrated via #69320; none of the remaining listed PRs has sufficient evidence to close as a duplicate of another listed PR.

Cross-PR triage: Reviewed 5 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 42 kB of PR diffs, 18 kB of issue/PR text, 9 kB of discussion (10 comments), 6 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants