Skip to content

feat(gateway): bring Signal groups to Telegram parity (authz, mention, owner detection) - #53348

Open
sdugoten wants to merge 5 commits into
NousResearch:mainfrom
sdugoten:feat/signal-group-parity-owner-detection
Open

feat(gateway): bring Signal groups to Telegram parity (authz, mention, owner detection)#53348
sdugoten wants to merge 5 commits into
NousResearch:mainfrom
sdugoten:feat/signal-group-parity-owner-detection

Conversation

@sdugoten

@sdugoten sdugoten commented Jun 27, 2026

Copy link
Copy Markdown

What & why

Brings a Signal group to Telegram-parity: any allowed member can talk to the bot, and the bot reliably knows which sender is the ownerwithout changing upstream's [name] sender prefix or any other platform's behavior.

Supersedes the draft #44706 (which makes only fix (1) below).

The fixes

1. Group-member authorization via SIGNAL_GROUP_ALLOWED_USERSgateway/authz_mixin.py
_is_user_authorized's group-allowlist bypass only enumerated Telegram and QQBOT, so every Signal group sender fell through to the per-user SIGNAL_ALLOWED_USERS check and was rejected (Unauthorized user: <uuid> on signal). Adds Platform.SIGNAL: "SIGNAL_GROUP_ALLOWED_USERS" — the env platforms/signal.py already honors for its own group filter. (This is what #44706 does.)

2. Reply-to-bot counts as a mentiongateway/platforms/signal.py
With require_mention: true, quoting a message the bot itself sent now triggers a reply, no @mention needed (parity with Telegram/WhatsApp).

3. /slash commands bypass the mention requirementgateway/platforms/signal.py
A message starting with / is handled without an @mention; command-level gating still decides which commands a non-owner may run.

4. Owner detectiongateway/platforms/signal.py, gateway/authz_mixin.py, gateway/session.py
Group senders arrive as a UUID while the owner allowlist holds a phone; the Signal adapter resolves UUID→phone via its number↔uuid cache and sets SessionSource.is_owner at intake. A generic _is_owner() fallback covers the DM path. Wildcard * is never an owner.

5. Surfacing the owner — gated, additive, no [name] changegateway/run.py, gateway/session.py

  • single-user sessions → an **Owner:** yes/no context line;
  • cache-shared group sessions → keep [name] and prepend an owner-only [SYSTEM: sender NAME is the owner] marker (guests byte-identical to upstream).

Both outputs are gated to Platform.SIGNAL, so Telegram/Slack/Discord/etc. are completely unaffected. The gate is a source.platform in {...} set with a comment inviting other platforms to opt in once their adapter sets source.is_owner.

Scope notes

How to test

.env: SIGNAL_GROUP_ALLOWED_USERS=*, SIGNAL_ALLOWED_USERS=<owner phone> (concrete, not *).
config.yaml: signal: { require_mention: true }.

  1. A non-owner member @mentions the bot → it replies (previously Unauthorized user). (1)
  2. Quote-reply to one of the bot's messages, no @mention → it replies. (2)
  3. /help from a non-owner → handled; an owner-only / command from a non-owner → rejected. (3)
  4. The owner's group messages arrive marked [SYSTEM: sender … is the owner]; everyone else plain [name]. (4,5)

Tests

New tests/gateway/test_shared_group_sender_prefix.py cases: owner gets the additive marker (Signal); guest stays plain [name]. All existing gateway prefix/session/signal tests pass under scripts/run_tests.sh (per-file isolation).

Platforms tested

macOS (Apple Silicon), signal-cli REST. Owner-surfacing + parity changes are additive/gated; non-Signal adapters unaffected.

Related

@sdugoten
sdugoten marked this pull request as ready for review June 27, 2026 00:52
@sdugoten
sdugoten marked this pull request as draft June 27, 2026 00:54
@sdugoten
sdugoten force-pushed the feat/signal-group-parity-owner-detection branch from d07b698 to 45668ef Compare June 27, 2026 00:54
@alt-glitch alt-glitch added type/feature New feature or request comp/gateway Gateway runner, session dispatch, delivery platform/signal Signal CLI adapter P2 Medium — degraded but workaround exists 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 labels Jun 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Related: #52240 and draft #44706 both add only fix (1) (Signal in the group-authz dict). This PR is the broader superset — it includes that change plus mention-parity, /slash bypass, and owner detection. Not a duplicate; flagging the cluster so a maintainer can pick the canonical Signal-group-parity PR.

…, owner detection)

Signal group chats lagged Telegram/WhatsApp. This brings parity:

- authz_mixin: authorize Signal group members via SIGNAL_GROUP_ALLOWED_USERS
  (same env signal.py already honors); add a generic _is_owner() fallback.
- signal.py: reply-to-bot and /slash bypass require_mention; owner detection
  resolves group UUID->phone and sets source.is_owner at intake.
- run.py/session.py: surface owner status WITHOUT changing upstream's [name]
  sender prefix. A **Owner:** context line covers single-user sessions; for
  cache-shared group sessions (where the context prompt is sender-agnostic) an
  additive [SYSTEM: sender NAME is the owner] marker is prepended ONLY for the
  owner, so guest lines stay byte-identical to upstream.

Supersedes NousResearch#44706. Refs NousResearch#7269.
@sdugoten
sdugoten force-pushed the feat/signal-group-parity-owner-detection branch from 0ed2f61 to 31e76ec Compare June 27, 2026 13:09
@sdugoten
sdugoten marked this pull request as ready for review June 27, 2026 13:32

@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 broader Signal-group pass. The underlying authorization gap is real, but two parts need correction before this can be salvaged.

Problems

  • The added Signal map entry still compares source.chat_id with the allowlist. Signal emits chat_id="group:<id>" at gateway/platforms/signal.py:607, while its allowlist contains raw IDs (:593-603) and the raw value is available as chat_id_alt (:709). Thus explicit configured groups remain unauthorized; only * works.
  • The new reply-to-bot gate recognizes only quote authors equal to the account phone. Current Signal logic already resolves own replies through timestamps and UUID/phone mappings at gateway/platforms/signal.py:790-807; the new branch should use that path.

Suggested changes

  • Match chat_id_alt (or normalize group:) and add authorization-path tests for explicit IDs, wildcard, and deny cases.
  • Reuse _quote_references_own_message() and add UUID/timestamp reply tests. The current added tests cover only marker formatting.

Automated hermes-sweeper review.

Comment thread gateway/authz_mixin.py
chat_allowlist_env = {
Platform.TELEGRAM: "TELEGRAM_GROUP_ALLOWED_CHATS",
Platform.QQBOT: "QQ_GROUP_ALLOWED_USERS",
Platform.SIGNAL: "SIGNAL_GROUP_ALLOWED_USERS",

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.

Signal's configured group IDs are raw, but the adapter sets source.chat_id to group:<id> and stores the raw ID in chat_id_alt. The existing comparison will therefore still reject every explicitly configured Signal group; match chat_id_alt or normalize the prefix here.

Comment thread gateway/platforms/signal.py Outdated
# treat a quote of a message the bot itself sent as "addressed to me".
_q = data_message.get("quote") or {}
_q_author = _q.get("author") or _q.get("authorNumber") or _q.get("authorUuid")
replied_to_bot = bool(_q_author) and _q_author in {

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.

Please use the adapter's existing _quote_references_own_message() path here. It also recognizes our sent-message timestamps and account UUID/UUID-to-phone mappings, while this comparison only accepts a phone-form quote author.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
sdugoten and others added 4 commits July 15, 2026 11:20
…tch + robust reply-to-bot

Two correctness fixes from the hermes-sweeper review:

1. Group chat-allowlist matching (gateway/authz_mixin.py)
   Signal emits chat_id="group:<id>" while SIGNAL_GROUP_ALLOWED_USERS holds the
   raw <id> (also exposed as chat_id_alt), so explicitly configured groups were
   never authorized — only "*" worked. Match chat_id, chat_id_alt, and the
   "group:"-stripped form.

2. Reply-to-bot mention bypass (gateway/platforms/signal.py)
   The new reply-to-bot gate compared the quote author only to the account phone,
   missing UUID-only authors and timestamp-only quotes. Hoist the quote extraction
   above the mention filter and reuse _quote_references_own_message (outbound
   timestamp cache + number<->uuid mapping).

Tests:
- tests/gateway/test_signal_group_authz.py: explicit-id (via chat_id_alt and
  group:-strip), wildcard, and deny cases.
- tests/gateway/test_signal.py::TestSignalReplyToBotMention: reply-to-bot by
  timestamp and by UUID author bypass require_mention; reply to a non-bot message
  still requires a mention.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
# Conflicts:
#	gateway/authz_mixin.py
#	gateway/session.py
Upstream _adapter_authorization_is_upstream now takes a profile kwarg; widen the
test stub lambda so the Signal group-authz tests reach the allowlist branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arity-owner-detection

# Conflicts:
#	gateway/run.py
#	tests/gateway/test_shared_group_sender_prefix.py
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 platform/signal Signal CLI adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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 type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants