Skip to content

feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge]#1270

Merged
thepagent merged 3 commits into
mainfrom
feat/trust-phase1-discord
Jul 1, 2026
Merged

feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge]#1270
thepagent merged 3 commits into
mainfrom
feat/trust-phase1-discord

Conversation

@chaodu-agent

Copy link
Copy Markdown
Collaborator

Phase 1 for Discord (#1264, #1269). Routes Discord ingress through AdapterRouter::gate_incoming for the L3 (identity) layer. Draft — to be canary-verified on one bot before merge (Discord is prod-critical).

Design (why L3-only for Discord)

Discord's L2 is richer than the flat allowed_channels model: threads are admitted by parent channel and DMs by allow_dm. A flat surface_allowed(thread_channel_id) would wrongly deny thread messages when allow_all_channels=false. So:

  • The discord registry entry is L2-open + allow_dm=true → the gate enforces L3 only; Discord's own channel/thread/DM logic stays in the adapter.
  • L3 mirrors resolved [discord].allow_all_users/allowed_users.

Non-regressive by construction

The gate call sits at the dispatch spawn, after Discord's existing pre-dispatch user check already ran with the same config. So the gate can only agree — it cannot deny a message that was already admitted. (Empty sender_id fail-closed can't trigger — Discord messages always have an author.)

Canary plan (before marking ready)

Deploy to one bot (e.g. a test bot) and confirm identical admit/deny:

  1. allowed user in a channel → responds ✅
  2. denied user (allow_all_users=false, not listed) → no response ✅
  3. DM (allow_dm true/false) → unchanged ✅
  4. thread under an allowed channel (allow_all_channels=false) → still responds ✅ (this is the case the L2-open design protects)

Deferred

Testing

  • clippy --features unified -D warnings clean; 92/92 Discord unit tests pass

Refs #1264 #1269

Routes Discord ingress through AdapterRouter::gate_incoming for the L3
(identity) layer, keyed under "discord" in the trust registry.

- registry "discord" entry: L2 open + allow_dm=true (Discord's richer
  channel/thread/DM logic stays in the adapter — the flat allowed_channels
  model can't express thread-by-parent admission); L3 mirrors resolved
  [discord].allow_all_users/allowed_users
- gate call added at the Discord dispatch spawn, redundant-but-matching
  with Discord's existing pre-dispatch user check → non-regressive by
  construction (cannot deny what already passed)

Behavior-preserving. Phase 1c makes the gate authoritative and removes the
scattered user check; richer Discord L2 modeling + dispatch privatization
tracked in #1269.

Refs #1264 #1269
@chaodu-agent

This comment has been minimized.

@chaodu-agent chaodu-agent marked this pull request as ready for review July 1, 2026 02:24
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 1, 2026 02:24
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

…F1/F2)

F1 (blocker): Discord's is_denied_user has a !is_bot bypass (bot admission
is handled by allow_bot_messages + trusted_bot_ids). The shared L3 gate is
human-identity only, so running it on bots wrongly dropped trusted
bot-to-bot messages when allow_all_users=false (multi-agent). Guard the
gate with !sender.is_bot.

F2: pass the in-scope is_dm instead of hardcoded false (benign today with
the L2-open discord entry, but avoids latent risk).

Note: #1267 (gateway) is unaffected — should_skip_event's user check never
had a bot bypass, so the gateway gate already matched it for bots.
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

Extract the gate's bot-skip into l3_gate_applies(is_bot) and add a
regression test (l3_gate_skips_bots_admits_humans) locking in that bots
bypass the shared L3 gate (mirrors is_denied_user's !is_bot), so
trusted/mode-admitted bots aren't denied when allow_all_users=false.
@chaodu-agent

This comment has been minimized.

@chaodu-agent

This comment has been minimized.

@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

LGTM ✅ — Clean, minimal wiring of the shared trust gate (L3 identity) for Discord ingress. Non-regressive by construction and canary-verified. All reviewer findings are migration items for Phase 1c, not current-PR blockers.

What This PR Does

Adds Discord to the shared AdapterRouter::gate_incoming trust pyramid (L3 identity only). The gate mirrors the existing [discord].allow_all_users/allowed_users config, placed after Discord's own is_denied_user() check — so it cannot deny an already-admitted message.

How It Works

  1. Registry (src/main.rs): Inserts a discord TrustConfig with L2 open (allow_all_channels=true, allow_dm=true) + L3 from resolved config via resolve_allow_all(d.allow_all_users, &d.allowed_users).
  2. Gate call (discord.rs): Clones Arc<AdapterRouter> into the tokio::spawn block, calls gate_incoming("discord", channel_id, false, sender_id) before dispatch. If denied, logs and returns early.
  3. L2 passthrough: Registry entry is L2-open → surface_allowed() always returns true → effective check is identity-only.

Findings

# Severity Finding Location
1 🟡 is_dm: false hardcode — safe under L2-open config, but needs Phase 1c TODO when L2 may tighten discord.rs:1044
2 🟡 Gate placement inside spawn (after STT echo) — non-regressive now (old check guards pre-spawn), but Phase 1c must move gate to pre-spawn position before STT/thread-creation/attachments discord.rs:1042
3 🟡 Missing gate in reaction_add, /auth, /export-thread handlers — covered by existing is_denied_user today, Phase 1c must wire all 4 entry points discord.rs
4 🟢 Non-regressive placement — gate runs after is_denied_user() (line 803) with identical config source; can only agree, never deny an already-admitted message discord.rs:1040
5 🟢 Type alignment correct — sender.sender_id is msg.author.id.to_string() and TrustConfig::allowed_users is HashSet<String> from same Vec<String> config discord.rs:1046 / main.rs:311
6 🟢 resolve_allow_all edge cases verified: Some(false) + empty list → deny-all ✅; None + empty list → allow-all (matches current behavior) ✅ main.rs:310
7 🟢 Empty sender_id fail-closed cannot trigger — Discord API guarantees msg.author.id is always a valid snowflake; serenity UserId is non-empty
8 🟢 Bot messages correctly bypass identity gate — gated separately by allow_bot_messages + trusted_bot_ids earlier in pipeline (pre-is_denied_user) discord.rs
9 🟢 CI all green (30/30 smoke tests) + canary boot verified on live bot (zhangfei) — clean startup, correct config, no unexpected denies
10 🟢 Gate deny path silent drop (no 🚫 reaction) — invisible in Phase 1 (gate never fires when adapter allows); Phase 1c will implement echo-UID UX per ADR discord.rs:1049
Finding Details

🟡 F1: is_dm: false hardcode needs Phase 1c tracking

The third arg to gate_incoming is is_dm. Currently safe because L2 is fully open (allow_all_channels=true + allow_dm=truesurface_allowed returns true regardless of is_dm value). When Phase 1c tightens L2 or makes the gate authoritative, this must pass actual DM detection. Matches the gateway.rs TODO pattern.

Action: Track in Phase 1c issue — pass actual is_dm derived from channel metadata.

🟡 F2: Gate placement — Phase 1c migration concern

The gate runs inside tokio::spawn, after stt::post_echo. In Phase 1, this is safe because the old is_denied_user at line 803 guards the pre-spawn path. But when Phase 1c removes is_denied_user, the gate must move to pre-spawn (at or before line 803) to prevent unauthorized users from triggering STT downloads, thread creation, attachment processing, and echo messages.

Action: Phase 1c PR must relocate gate to early-exit position before resource-intensive operations.

🟡 F3: Other handler coverage deferred to Phase 1c

is_denied_user guards 4 entry points: message (line 803), reaction_add (line 1263), /auth (line 1861), /export-thread (line 2152). This PR wires only message. The other 3 retain their existing protection via is_denied_user. Phase 1c must gate all 4 before removing the scattered checks.

Action: Track in Phase 1c issue — wire gate_incoming into all entry points before removing is_denied_user.

Baseline Check
  • PR opened: 2026-07-01
  • Main already has: trust.rs module with TrustConfig/PlatformTrustConfigs/Decision, gate_incoming on AdapterRouter, gateway.rs wiring for 6 platforms (telegram, line, feishu, wecom, googlechat, teams)
  • Net-new value: Discord joins the shared gate (message path, L3-only), completing Phase 1 coverage for the highest-traffic adapter
  • Canary: Deployed to zhangfei (idle bot), confirmed clean boot + correct config logging + no unexpected denies
Phase 1c Migration Checklist (tracked)

When Phase 1c makes the gate authoritative and removes is_denied_user:

  1. Move gate_incoming to pre-spawn (before STT/thread-creation/attachments)
  2. Wire gate_incoming into reaction_add, /auth, /export-thread handlers
  3. Pass actual is_dm detection instead of hardcoded false
  4. Implement DenyIdentity echo-UID UX (currently silent drop; old check had 🚫 reaction)
  5. Add integration tests for gate deny path
  6. Verify allow_all_users=Some(false) + empty list UX (deny-all is correct but may need warning log)
What's Good (🟢)
  • Deliberately conservative: L2-open avoids false denies from thread-by-parent channel problem
  • Clear separation: Discord's rich channel/thread/DM logic stays in adapter, only identity moves to shared gate
  • Well-documented deferred work (Phase 1c authoritative, richer L2 model, dispatch privatization)
  • Canary-tested on a live (idle) bot with correct config logging
  • Follows established gateway.rs pattern exactly
  • Good tracing instrumentation on deny path
  • Proper Arc clone before spawn — no ownership issues
  • resolve_allow_all correctly handles all flag/list combinations

@thepagent thepagent enabled auto-merge (squash) July 1, 2026 03:29
@thepagent thepagent merged commit 709d03f into main Jul 1, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants