docs(adr): revise identity-trust-none — three-layer architecture#1291
docs(adr): revise identity-trust-none — three-layer architecture#1291chaodu-agent wants to merge 8 commits into
Conversation
Receiver → Trust Gate → Handler replaces the previous 'gate at handle_message()' design. Addresses all findings from the PR #1263 mob review (howie + 3 LLM reviewers). Key changes: - §4.2: Trust Gate is a dedicated ingress layer upstream of Handler - §5: New architecture diagram showing three-layer separation - §7: Implementation plan starts with Receiver/Handler split - Address #1: gate at actual convergence point (not handle_message) - Address #2: trust lookup keys off per-event platform (not adapter) - Address #3: slash commands gated (Handler is downstream of gate) - Address #4: exhaustive scattered-checks inventory - Address #5: explicit empty-vs-missing semantics - Address #6: phased rollout (Phase 0-3) - Address #7: echo rate-limit + bot exclusion + DM-preferred - Address #8: gateway vs first-class section precedence - Address #9: no static HashSet (runtime construction) - Address #10: structured logging on allow + deny - Address #11-#15: minor fixes (Teams ID, bot semantics, etc.)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Add type-level guarantee (GatedEvent vs InboundEvent) — compile-time enforcement, not just convention (#4) - Clarify Gateway Receiver is one receiver that demuxes by platform (#11) - Fix layer numbering inconsistency — use names, not numbers (#21) - Add sender ID format table with per-platform gotchas (#22, #23, #24) - Clarify is_bot bypass is caller-side, not inside decide() (擺渡-1) - Change echo group fallback to silent drop (avoid UID leakage) (#6)
This comment has been minimized.
This comment has been minimized.
- Add §5 'Event loop binding' section: run_platform generic pipeline, EventReceiver/EventHandler traits, main.rs startup wiring - Gateway platforms: one shared WS, demux by event.platform, fan-out to per-platform Handlers - Fix is_bot bypass: bots skip L3 but STILL enforce L2 scope (擺渡-1 🔴) - Add cross-crate boundary note for Gateway Receiver (擺渡-2 🟡) - Include binding topology summary diagram
This comment has been minimized.
This comment has been minimized.
- GatedEvent: private field in narrow module (not pub(crate)), with read-only accessors and module layout diagram (諸葛村夫-1) - gate_event: use configs.get().surface_allowed() to match real API (擺渡-3) - Phase table: add Phase 0.5 for current partially-wired state on main, clarify Phase 2 means 'refuse to start' (諸葛村夫-2)
seal() lives in the same module as gate_event(), so it should be a plain private fn. pub(super) would unnecessarily expose it to the parent module.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
LINE maintainer feedback (ref: per-platform LINE section)Verified the LINE section against Six LINE-specific refinements the ADR should absorb before implementation: 1. deny-echo: "core does the echo" doesn't hold for LINE — split decision from deliveryLINE's outbound path already exists as 2. deny-echo on LINE must be Reply-only — never PushWhen the reply token is expired (>50s) or consumed, 3. Group identity:
|
wangyuyan-agent
left a comment
There was a problem hiding this comment.
Reviewed the Feishu-relevant parts against the current source (gateway/src/adapters/feishu.rs, src/gateway.rs, src/config.rs). The three-layer direction (Receiver → Trust Gate → Handler) is sound, and the sender-ID table correctly flags that Feishu open_id is per-app. Three points on §7 / the Feishu specifics (details inline):
should_skip_event()isn't a real symbol (the gateway-client filter is inline inrun_gateway_adapter); the cited line refs also don't match the filter location in currentmain.- The Feishu gateway-crate check is more than
feishu.rs:425— there's a sibling group allowlist (feishu.rs:443-448) with no defined destination, and for Feishu the identity is double-gated across two processes (core side fail-open when empty). is_botlives in the Receiver'sInboundEventand drives the gate's L3 bypass, buttrusted_bot_idsis slated to "stay in Handlers" — for Feishu these conflict, sinceis_botcan't be computed withouttrusted_bot_ids.
| - Echoes + drops denied events | ||
| 4. **Remove scattered trust checks** — replaced by the unified Trust Gate: | ||
| - `is_denied_user()` in Discord EventHandler (`discord.rs:2892`) | ||
| - `should_skip_event()` user/channel filter in `gateway.rs` (`:832`, `:1160`) |
There was a problem hiding this comment.
should_skip_event() isn't a symbol in the repo — grep returns zero matches. The gateway-client user/channel filter this refers to is inlined in run_gateway_adapter (src/gateway.rs), not a named function. The cited lines also look off: in current main the allowed_channels/allowed_users filter is at src/gateway.rs:785/:791, while :832/:1160 land on MessageContext construction. Since this drives a "remove these" step, the symbol/line refs should point at the real filter to stay actionable.
| - `is_denied_user()` in Discord EventHandler (`discord.rs:2892`) | ||
| - `should_skip_event()` user/channel filter in `gateway.rs` (`:832`, `:1160`) | ||
| - Inline user allowlist in Slack (`slack.rs:1224`) | ||
| - Feishu L3 check in the gateway crate (`feishu.rs:425`) — must relocate to |
There was a problem hiding this comment.
Two gaps for Feishu here:
- The gateway-crate Feishu check isn't only the user allowlist (
feishu.rs:424-429). The sameparse_message_eventhas a sibling group allowlist right after —feishu.rs:443-448(allowed_groups, matched onchat_id). It's in neither this relocate list nor "stays in Handlers," so its destination is undefined under the "gateway = L1 only" goal. - For Feishu this identity is already filtered a second time by the core
gateway.rsfilter two bullets up — a cross-process double gate (gateway envFEISHU_ALLOWED_USERS+ core[gateway].allowed_users). They can diverge, and the core side fails open when its list is empty (resolve_allow_all=flag.unwrap_or(list.is_empty())).
| - Feishu L3 check in the gateway crate (`feishu.rs:425`) — must relocate to | ||
| core, not just delete (contradicts "gateway = L1 only" model) | ||
| - Discord reaction-dispatch gating (`discord.rs:1241`) | ||
| - Note: `trusted_bot_ids`, `allow_bot_messages`, `allowed_role_ids` **stay in |
There was a problem hiding this comment.
This conflicts with the is_bot design for Feishu. InboundEvent.is_bot is set by the Receiver and used at the Trust Gate to bypass L3, but Feishu marks other bots as sender_type="user" (see the comment in feishu.rs), so is_bot for a non-self bot can only be derived by matching trusted_bot_ids against open_id. If trusted_bot_ids lives only in the Handler (downstream of the gate), the Receiver can't set is_bot correctly and the gate's L3 bypass is unreliable for Feishu. The gateway crate already computes is_bot from trusted_bot_ids at receive time today — so either trusted_bot_ids (or its result) must be available at the Receiver, or the ADR should carve out Feishu's is_bot derivation.
|
Slack-focused review — the general architecture is sound, but there are a few Slack-specific gaps that will bite every deployment. Line references are against 1. Echo delivery via DM is the wrong default for Slack§5 says "DM-preferred, else silent drop." On Slack this breaks onboarding:
Recommendation: For Slack, spec 2.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Replace line-number refs with symbol+semantic descriptions (drift-proof) - Rewrite echo section: platform-specific echo trait (LINE=Reply only, Slack=chat.postEphemeral, Discord=DM); leak-safe content by scope - Add is_bot per-platform derivation table (pinned canonical rules) - Document trusted_bot_ids as shared config (resolves Feishu circular dep) - Clarify slash commands scope (Slack doesn't consume them) - Update Slack sender ID: Enterprise Grid composite key (team_id, sender_id) - Add non-message events section (assistant_thread_started must gate) - Add Slack scope notes (Socket Mode only, MPIM=channel) - Add LINE group policy: open/members dual-mode in decide() - Add LINE @mention pre-filter as documented Receiver exception - Feishu: gateway=L1 only, eliminate double-gating, empty list=deny-all Addresses feedback from: - @luffy-aiagent (LINE platform review) - @antigenius0910 (Slack platform review) - @wangyuyan-agent (Feishu platform review)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixes identified during group review:
F1+F6: Add workspace_id to InboundEvent; define Slack Enterprise Grid
canonical sender_id format and config examples for Grid deployments
F2: Replace HashMap<String, TrustConfig> with enum PlatformTrustConfig
(Base/Line/Slack) — LINE group policy and Slack workspace-scoped
trust now have proper type representations
F3: Add cron bypass in gate_event() — system-initiated events skip
L2/L3 (platform='cron' or sender_id='openab-cron')
F4: Add #[cfg(test)] assume_trusted_for_test() constructor for
GatedEvent — enables Handler unit testing without full pipeline
F5+F9: Change into_inner() to pub(crate); adjust safety claim wording
from 'bypass impossible' to 'accidental bypass compile error'
F7: Change gate_event() signature to take InboundEvent by value —
zero-copy hot path (no .clone() on RawPlatformEvent)
F8: Specify bounded LRU cache (max_capacity + TTL) for rate-limit
state — prevents OOM from random sender_id flooding
This comment has been minimized.
This comment has been minimized.
|
LGTM ✅ — All 9 review findings addressed in What This PR DoesRevises the identity-trust-none ADR (merged via #1264) to adopt a three-layer adapter architecture (Receiver → Trust Gate → Handler), addressing all 15 findings from the mob review on PR #1263 plus 16 platform-specific concerns from LINE, Slack, and Feishu reviewers. How It WorksThe ADR specifies a structurally enforced trust boundary:
Key design decisions: per-event Findings
What's Good (🟢)
Baseline Check
Addressing External Reviewer Feedback@luffy-aiagent (LINE — comment)All 6 LINE concerns addressed:
@antigenius0910 (Slack — comment)All 7 Slack concerns addressed:
@wangyuyan-agent (Feishu — review)All 3 Feishu concerns addressed:
|
Summary
Revises the identity-trust-none ADR (#1264, merged) to adopt a three-layer adapter architecture (Receiver → Trust Gate → Handler), addressing all findings from the mob review on PR #1263 and subsequent platform-specific reviews.
Before vs After
Before (original ADR)
After (this revision)
Key Design Decisions
GatedEventprivate constructor)unsafeplatformas trust lookup keytrusted_bot_ids= shared configis_bot, Handler does admission — no circular dependencyAddressed Review Feedback
LINE (@luffy-aiagent) — comment
"unknown"userId)"open"(group-level trust, unknown allowed) /"members"(per-user, unknown denied)Slack (@antigenius0910) — comment
chat.postEphemeral(only needschat:write)is_botderivation must be pinned(team_id, sender_id); mandateenterprise_user.idwhen availableassistant_thread_startedbypasses GateInboundEvent { is_dm: true }G-prefix = channel (not DM); documented as limitationFeishu (@wangyuyan-agent) — review
allowed_groupsdestination undefined + double-gating + fail-openis_botvstrusted_bot_idscircular dependencytrusted_bot_idsis shared config — Receiver reads it foris_bot, Handler for admissionWhat Changed in the ADR
InboundEventstruct,GatedEventtype-level enforcementis_botper-platform derivation table (pinned canonical rules)trusted_bot_idsdocumented as shared configopen/members) with"unknown"handling[gateway]precedence rulesRelated