Skip to content

feat(trust): Phase 1 (slack) — L3 identity via shared gate#1363

Merged
thepagent merged 4 commits into
mainfrom
feat/1361-slack-trust-registry
Jul 11, 2026
Merged

feat(trust): Phase 1 (slack) — L3 identity via shared gate#1363
thepagent merged 4 commits into
mainfrom
feat/1361-slack-trust-registry

Conversation

@chaodu-agent

@chaodu-agent chaodu-agent commented Jul 11, 2026

Copy link
Copy Markdown
Collaborator

What problem does this solve?

Slack is the only configured platform absent from the PlatformTrustConfigs registry: src/main.rs inserts the six gateway platforms, Discord (#1270), and Telegram — but never Slack. Slack trust relies entirely on the adapter's inline checks, and the registry's fallback for unregistered platforms is the deny-all default, so the shared ingress gate could never be made authoritative for Slack (Phase 1c) without this wiring.

This implements the first task of #1361 (umbrella #1356), mirroring the Discord Phase 1 pattern from #1270.

At a Glance

Before — Slack is the only configured platform outside the shared gate

┌─ Slack event ─────────────────────┐  ┌─ PlatformTrustConfigs ────────────┐
│                                   │  │                                   │
│  ┌─────────────────────────────┐  │  │  ┌─────────────────────────────┐  │
│  │ inline channel check        │  │  │  │ discord    ← [discord]      │  │
│  │ (adapter)                   │  │  │  │            (#1270)          │  │
│  └──────────────┬──────────────┘  │  │  └─────────────────────────────┘  │
│                 ▼                 │  │  ┌─────────────────────────────┐  │
│  ┌─────────────────────────────┐  │  │  │ telegram   ← [telegram]     │  │
│  │ inline user check           │  │  │  └─────────────────────────────┘  │
│  │ (adapter)                   │  │  │  ┌─────────────────────────────┐  │
│  └──────────────┬──────────────┘  │  │  │ line │ feishu │ wecom       │  │
│                 ▼                 │  │  │ googlechat │ teams          │  │
│  ┌─────────────────────────────┐  │  │  │            ← GATEWAY_* env  │  │
│  │ dispatch                    │  │  │  └─────────────────────────────┘  │
│  └─────────────────────────────┘  │  │  ┌─────────────────────────────┐  │
│                                   │  │  │ slack      ✗ MISSING        │  │
│  shared trust gate not in path ✗  │  │  │ (deny-all fallback if the   │  │
│                                   │  │  │  gate ever ran for slack)   │  │
└───────────────────────────────────┘  │  └─────────────────────────────┘  │
                                       └───────────────────────────────────┘

After — Slack joins the same choke point (behavior-preserving)

┌─ Slack event ─────────────────────┐  ┌─ PlatformTrustConfigs ────────────┐
│                                   │  │                                   │
│  ┌─────────────────────────────┐  │  │  ┌─────────────────────────────┐  │
│  │ inline channel check        │  │  │  │ discord    ← [discord]      │  │
│  │ (adapter, unchanged)        │  │  │  └─────────────────────────────┘  │
│  └──────────────┬──────────────┘  │  │  ┌─────────────────────────────┐  │
│                 ▼                 │  │  │ slack      ✓ NEW ← [slack]  │  │
│  ┌─────────────────────────────┐  │  │  │                             │  │
│  │ inline user check           │  │  │  │ L2 open — adapter's channel │  │
│  │ (adapter, unchanged)        │  │  │  │ allowlist stays             │  │
│  └──────────────┬──────────────┘  │  │  │ authoritative               │  │
│                 ▼                 │  │  │                             │  │
│  ┌─────────────────────────────┐  │  │  │ L3 mirrors resolved         │  │
│  │ gate_incoming("slack")  NEW │◀─┼──┼─▶│ allow_all_users /           │  │
│  │                             │  │  │  │ allowed_users               │  │
│  │ · redundant-but-matching    │  │  │  └─────────────────────────────┘  │
│  │ · bots bypass L3            │  │  │  ┌─────────────────────────────┐  │
│  │ · is_dm from D… prefix      │  │  │  │ telegram   ← [telegram]     │  │
│  └──────────────┬──────────────┘  │  │  └─────────────────────────────┘  │
│                 ▼                 │  │  ┌─────────────────────────────┐  │
│  ┌─────────────────────────────┐  │  │  │ line │ feishu │ wecom       │  │
│  │ dispatch                    │  │  │  │ googlechat │ teams          │  │
│  └─────────────────────────────┘  │  │  │            ← GATEWAY_* env  │  │
│                                   │  │  └─────────────────────────────┘  │
└───────────────────────────────────┘  └───────────────────────────────────┘

Phase 1c (future) flips the gate from redundant-but-matching to authoritative and removes the scattered inline checks — this PR makes that possible for Slack.

Proposed Solution

  • src/main.rs — insert a "slack" entry into the registry: L2 open (Slack's own channel allowlist stays authoritative in the adapter), L3 mirrors the resolved [slack].allow_all_users / allowed_users via config::resolve_allow_all, so the gate agrees exactly with Slack's existing user check.
  • crates/openab-core/src/slack.rs — thread Arc<AdapterRouter> through run_slack_adapter into handle_message, and evaluate gate_incoming("slack", …) after the existing user check. The gate is redundant-but-matching: it cannot deny anything the inline check already admitted (non-regressive), and the existing 🚫-reaction deny UX is unchanged.
  • Bot bypassl3_gate_applies(is_bot_msg) skips the gate for bot senders, mirroring the inline check's !is_bot_msg bypass and Discord's rationale (feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 review F1): bot admission is allow_bot_messages + trusted_bot_ids; L3 is a human-identity allowlist. Running it on bots would wrongly drop trusted bot-to-bot messages when allow_all_users = false (multi-agent).
  • Truthful is_dm — passed via Slack conversation-ID prefix (D… = DM, C…/G… = channel/group), cf. feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 review F2. Informational today: the registry entry is L2-open with allow_dm = true, so the decision is identical either way.
  • crates/openab-core/src/trust.rs (added in 7908cb7 after self-review) — l3_gate_applies now has a single definition next to Decision, imported by both the Discord and Slack gate call sites, instead of being copy-pasted per adapter.

Why this approach?

It is the exact shape of the accepted Discord Phase 1 change (#1270: registry insert + redundant-but-matching gate call + bot bypass + tests), keeping the two core platforms symmetrical ahead of Phase 1c (gate becomes authoritative, scattered checks removed). Behavior-preserving by construction — no config field changes, no default changes.

Self-review

Reviewed with the standard SOP after opening:

  • Coverage audit: the only dispatcher.submit site in slack.rs is inside handle_message, and both event paths (app_mention, message) route through it — no Slack dispatch path bypasses the new gate.
  • Non-regression audit: gate runs after the inline check with identical resolved inputs; identity_allowed's empty-sender fail-closed cannot fire (handle_message returns early when the event has no user/bot_id).
  • 🟡→fixed l3_gate_applies duplication: the original commit copy-pasted the helper from discord.rs. Consolidated into trust.rs in 7908cb7; both adapters now import the shared definition, each keeping its own pinning test.

Platform-facts KB sync

main gained the schema-driven platform facts KB (#1295) after this branch was cut; docs/platforms/schema/slack.toml recorded "Slack does NOT use the shared gate," which this PR makes stale. Commit f6606e1 updates the trust_gate entry (partial → implemented, phrasing mirrors discord.toml) and the Native-trust-divergence quirk. The conformance CI check now runs on this PR and passes.

Validation

At 7908cb7 (macOS arm64):

  • cargo clippy --workspace --all-features -- -D warnings — clean
  • cargo clippy --workspace -- -D warnings (default features) — clean
  • cargo check -p openab-core --no-default-features — pass
  • cargo check --no-default-features --features discord (slack feature off) — pass (1 pre-existing session_ttl_dur warning, present on unmodified main)
  • cargo test -p openab-core --all-features — 656 passed, 1 failed: secrets::tests::resolve_exec_nonzero_exit, the known pre-existing macOS-only failure (also fails on unmodified main)
  • Gate pinning tests (l3_gate*) — 2/2 across both adapters
  • rustfmt --check — no new drift introduced (per-file diff counts identical to main baseline: trust.rs 8→8, slack.rs 25→25, discord.rs 55→55, main.rs 4→4)

Tests

Refs #1361 (first task — Enterprise Grid workspace_users remains), umbrella #1356, ADR #1291.

Wire Slack into the PlatformTrustConfigs registry and call the shared
ingress trust gate from the Slack message path, mirroring the Discord
wiring from #1270. Slack was the only configured platform absent from
the registry — the gate would have fallen back to the deny-all default
had it ever run for slack events.

- main.rs: insert "slack" TrustConfig — L2 open (Slack's own channel
  allowlist stays authoritative in the adapter), L3 mirrors the
  resolved [slack].allow_all_users/allowed_users, so the gate agrees
  with Slack's existing user check (behavior-preserving).
- slack.rs: thread Arc<AdapterRouter> through run_slack_adapter into
  handle_message; evaluate gate_incoming after the existing user check
  (redundant-but-matching, non-regressive). Bots bypass L3 — same
  rationale as Discord (#1270 review F1): bot admission is
  allow_bot_messages + trusted_bot_ids, and L3 is human-identity only.
- is_dm passed truthfully via Slack conversation-ID prefix (D… = DM),
  cf. #1270 review F2; decision is identical either way today since the
  entry is L2-open with allow_dm=true.
- tests: pin the L3 bot-bypass and the DM prefix classification.

Refs #1361 (first task), umbrella #1356, ADR #1291.
@chaodu-agent chaodu-agent requested a review from thepagent as a code owner July 11, 2026 11:29
@chaodu-agent

This comment has been minimized.

Self-review finding: the Slack wiring copy-pasted l3_gate_applies from
discord.rs (identical 3-line fn + doc). Move the single definition next
to Decision in trust.rs and import it from both gate call sites. Each
adapter keeps its own pinning test against the shared fn.
@chaodu-agent

This comment has been minimized.

The platform-facts KB (#1295) landed on main after this branch was cut
and records 'Slack does NOT use the shared gate' — which this PR makes
stale. Update the trust_gate feature entry (partial → implemented,
mirroring discord.toml's phrasing) and the Native-trust-divergence
quirk. Conformance suite passes against this branch's code refs.
@chaodu-agent

Copy link
Copy Markdown
Collaborator Author

Note

LGTM ✅ — Slack joins the shared L3 identity gate with a clean, behavior-preserving wiring that mirrors the accepted Discord Phase 1 pattern.

What This PR Does

Slack was the only configured platform absent from the PlatformTrustConfigs registry, meaning the shared ingress gate could never be made authoritative for Slack without this wiring. This PR adds Slack to the registry (L2 open, L3 mirrors resolved allow_all_users/allowed_users) and calls gate_incoming("slack", …) after the existing inline user check — exactly the same Phase 1 (redundant-but-matching) approach as Discord (#1270).

How It Works

  1. Registry entry (src/main.rs): Inserts "slack" with L2 open (channel allowlist stays in the adapter) and L3 mirroring the resolved [slack].allow_all_users/allowed_users via config::resolve_allow_all, so the gate agrees with the inline check by construction.
  2. Gate call (slack.rs): After the inline user check (which already denies + 🚫-reacts), l3_gate_applies(is_bot_msg) gates the shared L3 check. Bots bypass (bot admission is a separate concern). If the gate denies, it logs and returns silently — this path is unreachable in practice since the inline check fires first with the same resolved inputs.
  3. Shared helper (trust.rs): l3_gate_applies is extracted from discord.rs into the trust module — single definition, imported by both adapters.
  4. Router threading (main.rs): Arc<AdapterRouter> cloned as slack_router and passed through run_slack_adapterhandle_message.
  5. is_dm_channel (slack.rs): Prefix-typed detection (D… = DM) keeps the gate's inputs truthful, matching the Discord pattern (cf. feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270 F2).

Findings

# Severity Finding Location
1 🟢 Registry config mirrors inline check by construction — resolve_allow_all + allowed_users.clone() ensures the gate can never disagree with the adapter src/main.rs:347-360
2 🟢 l3_gate_applies consolidation eliminates duplication and establishes a single source of truth for the bot-bypass predicate crates/openab-core/src/trust.rs:57-64
3 🟢 Gate placement after inline check is non-regressive — identical deny criteria means the gate path is unreachable today (defense-in-depth for Phase 1c) crates/openab-core/src/slack.rs:1441-1467
4 🟢 Tests pin both key behaviors (bot-bypass semantics + conversation-ID prefix classification) crates/openab-core/src/slack.rs tests
5 🟢 Documentation update is accurate — status, notes, and quirks all reflect the new wiring correctly docs/platforms/schema/slack.toml
Finding Details

🟢 F1: Registry agrees with inline check by construction

The registry entry uses config::resolve_allow_all(s.allow_all_users, &s.allowed_users) — the exact same resolution the adapter uses for its own allow_all_users variable (line 541 on main). Combined with s.allowed_users.clone(), the gate's L3 config is structurally identical to the inline check's inputs. There is no way for them to disagree.

🟢 F2: Shared l3_gate_applies eliminates duplication

Previously the function was defined in discord.rs. Now it lives in trust.rs next to Decision, making it the canonical definition for all adapters. Both Discord and Slack import it, each with a pinning test.

🟢 F3: Non-regressive gate placement

The gate runs after the inline user check (!is_bot_msg && !allow_all_users && !allowed_users.contains(&user_id)). Since both use the same resolved inputs, any user denied by the gate was already denied by the inline check. The gate can only fire on already-admitted messages — it's a no-op safety net today.

🟢 F4: Test coverage

  • l3_gate_skips_bots_applies_to_humans — pins the core invariant that bots bypass L3
  • is_dm_channel_by_prefix — pins the Slack-specific D…/C…/G… classification, including the empty-string edge case

🟢 F5: Docs accuracy

The schema doc correctly updates trust_gate status from partial to implemented, accurately describes the two-layer model, and the quirks section correctly notes Phase 1c as the next step.

Baseline Check
What's Good (🟢)
  • Exact structural symmetry with the accepted Discord Phase 1 pattern (feat(trust): Phase 1 (discord) — L3 identity via shared gate [DRAFT: canary before merge] #1270) — reviewers can reason about both adapters identically
  • Self-review caught and fixed l3_gate_applies duplication before external review
  • Thorough PR description with ASCII architecture diagrams, validation matrix, and explicit non-regression argument
  • Clean separation of concerns: L2 stays in the adapter, only L3 is wired through the shared gate
  • Tests cover the critical invariants without over-testing the redundant path

5️⃣ Three Reasons We Might Not Need This PR

  1. The gate is a no-op today — Since the inline check fires first with identical inputs, the gate can never deny anything the adapter hasn't already denied. The PR adds code that cannot change runtime behavior until Phase 1c.
  2. Phase 1c might redesign the gate interface — If Phase 1c significantly restructures how the gate is called (e.g. moving it before the inline check or changing the Decision enum), this wiring may need rework anyway.
  3. Complexity for zero immediate value — Threading Arc<AdapterRouter> through the Slack call stack adds cognitive overhead for contributors reading the code today, with no user-visible benefit until Phase 1c lands.

Counter-argument: All three are intentional trade-offs acknowledged by the phased approach. The PR establishes the wiring contract and proves non-regression now, so Phase 1c can focus on making the gate authoritative without simultaneously debugging correctness. The asymmetry cost (Slack being the only missing platform) is also a real maintenance hazard for Phase 1c.


f6606e1 · CI: check ✅ · smoke-tests in progress

@thepagent thepagent merged commit 06416b1 into main Jul 11, 2026
40 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