fix: admit trusted peer bots in SlackTransport.receive (#7463)

[chenmingwei23]

Problem / Motivation

slack.trusted_bot_ids is effective at the Socket Mode drop site (slack/events.py), but SlackTransport.receive in src/kiro_crew/slack/transport.py still drops every bot-authored event unconditionally (bot_id or subtype == "bot_message"), and its comment asserts the opposite of what the platform does ("a bot id is never in the owner-only allow-list anyway" -- trusted_bot_ids is a second allow-list that exists precisely to admit bot ids). The two Slack inbound paths disagree about whether an allow-listed peer bot is admissible.

Why it matters

The defect is latent -- SlackTransport has no production construction site, so no message is dropped today. The cost is deferred: whoever wires Slack inbound through the transport abstraction silently reintroduces the exact defect the trusted-bots feature fixed, in a file that looks settled, and the stale comment actively tells that reader the drop is intentional.

What changed (motivation -> approach -> change)

Symptom: trusted_bot_ids appears zero times in transport.py while events.py gates on exactly that set. Root cause: when the trusted-bots admission landed on the Socket Mode path, the parallel (unwired) transport inbound path kept the old blanket bot drop and its now-false rationale. Change: mirror the events.py trust decision in receive(), keeping its load-bearing properties:

• trusted_bot_ids is a new keyword-only constructor param (frozen snapshot, same immutability rationale as allowed_users; empty default keeps dropping every bot event, deny-by-default).
• Admission requires a positive bot_id match against the allow-list; the gateway's own bot id is never trusted even when listed (error=own_bot_id_never_trusted), and an unverified self identity (validated_self_bot_id() empty) fails closed (error=trusted_bot_requires_verified_self_id).
• The trust decision runs before the subtype == "bot_message" filter, so untrusted denials are SEL-audited (operation="slack_transport.receive", error=untrusted_bot) rather than silently subtype-dropped, and a trusted bot's bot_message is not eaten.
• An admitted bot's bot_id stands in as user_id, its admission is audited (outcome="allowed", resources="trusted_bot"), and it is dispatched without consulting the owner-only allowed_users list -- the positive allow-list match is its authorization, exactly as on the events.py path. Loop bounding (the per-thread trusted-bot turn cap) remains the dispatch layer's job.
• The stale comment is replaced with the trusted-bots rationale; docs/system-specs/modules/messaging.md (the SlackTransport paragraph) and docs/system-specs/modules/slack-gateway.md (the trusted-bots bullet) are updated in the same commit.
• Declared rider: the same messaging.md paragraph's stale capability figures are corrected to match SLACK_CAPABILITIES in code (max_message_chars 40000 -> 3900 = SLACK_MSG_LIMIT; max_buttons 5 -> 10), per the GPT review lane's advisory finding on this PR.

Tests

New TestReceiveTrustedBots class in test/test_slack_transport.py:

• test_trusted_bot_admitted -- a trusted_bot_ids member carrying subtype == "bot_message" reaches dispatch with bot_id as user_id (locks trust-before-subtype ordering).
• test_untrusted_bot_still_dropped -- a bot outside the allow-list never reaches dispatch.
• test_untrusted_bot_denial_is_audited -- the denial emits SEL (error=untrusted_bot).
• test_trusted_admission_is_audited -- the admission emits SEL (resources="trusted_bot").
• test_own_bot_never_trusted_even_when_listed -- self id in the allow-list still drops.
• test_unverified_self_id_fails_closed -- empty validated_self_bot_id() admits no bots.
• test_trusted_set_is_frozen_snapshot -- mutating the source set post-construction cannot widen admission.

All pre-existing transport tests pass unchanged (empty default preserves the blanket drop).

Manual verification

N/A -- unit coverage sufficient: the transport has no production construction site (nothing routes through receive() today), so the gate is pure in-process logic fully exercised by the unit tests.

Related Issues

Closes #7463

Pattern harvest

Rule candidate: review-prompt
Pattern: a security gate fixed on one inbound path was not mirrored to the parallel path guarding the same boundary, and the parallel path's comment still documented the pre-fix rationale as intentional.

Checklist

• At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ae1fce6

False positive or not applicable? A repository writer can comment:
/ai-review override gpt ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] ae1fce6

Verdict parsed from the review's SHA-scoped output markers for commit ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0.

False positive or not applicable? A repository writer can comment:
/ai-review override fable ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The admission is mirrored but its provenance is dropped: InboundMessage has no from_trusted_bot, so the loop bound this PR delegates downstream is undeliverable.

Watch

• The PR states "Loop bounding (the per-thread trusted-bot turn cap) remains the dispatch layer's job," but receive() normalizes an admitted bot into a plain InboundMessage (user_id=bot_id) with no trust flag — unlike events.py, which threads from_trusted_bot to the layer that enforces the cap and echo suppression. Cause: provenance erased at normalization → mechanism: the dispatch callback cannot distinguish a peer bot from a human → consequence: whoever wires this transport gets bot admission with an unboundable mutual-reply loop, while the comment tells them bounding is handled elsewhere. Add from_trusted_bot: bool = False to InboundMessage (compatible dataclass default) in this PR, since admission without it is half a mirror.
• The fix repeats the mechanism that caused the defect: the ~15-line admission predicate (self-exclusion, fail-closed unverified self, three deny reasons) is now duplicated verbatim in events.py and transport.py, so the next admission change must again be manually mirrored — the exact failure the PR's own pattern harvest names.

Suggestions

• Extract the admission predicate into one shared function (e.g. slack/trusted_bots.py: (bot_id, trusted_ids) -> admitted | deny_error) called from both drop sites; note also that events.py reads trusted_bot_ids from live config while the transport freezes it at construction, a residual disagreement between the "agreeing" paths.

[DESIGN-REVIEWED] ae1fce6

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ae1fce6b54d2365fdd3c0824f19e7ec7b23ecbd0 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is in. The production Slack inbound gate lives at events.py:949-995; SlackTransport.receive is contract-mandated (messaging/transport.py:353) but unconstructed in production (transport.py:4-6 says so; grep SlackTransport( under src/ = 0 construction sites). The new gate is a near line-for-line copy of the events.py one, and the messaging.md hunk also silently corrects two stale capability figures that do match the code (transport.py:62,67).

First-Principles-Verdict: CONCERNS

The trust decision is now spelled inline at two drop sites — this PR fixes today's divergence by installing the machine that produces tomorrow's.

What this change ships

Intent: make the unwired transport inbound path admit allow-listed peer bots exactly as the live Socket Mode path does — a FIX (latent; closes reported issue #7463, honestly declared as having no observable effect today).

1. Transport constructor takes a trusted-bot allow-list — justified (reported defect), zero production consumers, declared as such
2. Allow-listed peer bots pass the transport gate — duplicate spelling of events.py:949-995
3. Untrusted-bot drops on that path are SEL-audited, not silent — justified mirror
4. Own-bot exclusion and unverified-self-id both fail closed — justified mirror
5. Doc capability figures corrected (40000→3900 chars, 5→10 buttons) — undeclared rider, true to code
6. Trusted-bots spec paragraphs updated same-commit — justified (documented invariant)
7. False "a bot id is never in the allow-list" comment replaced — justified; the smallest honest fix for the named harm

Watch

• Second spelling of a security decision. Grepped the admission predicate: it now exists at exactly 2 sites, events.py:970-995 and transport.py:196-215, byte-similar logic and comments. The root cause of slack/transport.py still drops bot-authored events unconditionally after #6638 #7463 ("a gate fixed on one inbound path was not mirrored" — the PR's own pattern-harvest sentence) is that the decision has no single owner; copying it inline again means the next rule change must land twice or the paths re-diverge. The cause-level fix is in scope and no larger.
• The "agreement" is already inexact. events.py reads orch._cfg.slack.trusted_bot_ids live per event; the transport freezes a snapshot at construction. Whoever wires receive() inherits a config-reload behavior difference the updated docs describe as the two paths agreeing.
• Item 5 is a ride-along in a fix: PR the description never mentions — harmless, but undeclared.

Subtractions

• Replace both inline copies of the admission predicate with one shared function returning (from_trusted_bot, deny_error) — put it beside validated_self_bot_id in slack/enterprise.py, and shrink events.py:970-995 and transport.py:196-215 to callers of it.

[FIRST-PRINCIPLES-REVIEWED] ae1fce6

[chenmingwei23]

Disposition: rebutted (direction re-litigation, not an implementation defect)

Subtraction: ship the comment correction alone ... and drop the param, gate, audits, tests, and both doc additions.

The subtraction proposed here is exactly option 2 of the two shapes issue #7463 itself offers ("Either consult trusted_bot_ids here the same way events.py does, or ... say so in the comment"). This PR was dispatched on option 1 by the pipeline's triage decision, which the issue's own text supports: the ordering lesson, the audited denials, and the not-eaten trusted bot_message are all named there as applying "to the first option". So the review is re-opening a decided A-vs-B, not finding a defect in the chosen A.

On the substance of the block:

• "Zero callers" is not new information: the issue discloses it ("It is not reachable today ... filing this as latent"), and this PR's body repeats it. The issue's stated reason to fix NOW is precisely that latency: whoever wires inbound through the transport reintroduces the exact defect the trusted-bots feature fixed, in a file whose comment says the drop is intentional.
• "Two spellings that must stay in sync" is the cost of the transport abstraction existing, not of this PR: before this change the two paths ALREADY had two spellings -- one correct, one wrong, agreeing only by accident of the second being unreachable. After it, the agreement is pinned by seven tests that fail on divergence (each verified to fail against the pre-PR code), which is stronger than the proposed comment whose entire enforcement power is a reader noticing it.
• The comment-only alternative leaves executable code whose behavior contradicts the platform decision, guarded by prose. That is the shape that produced this issue in the first place: the pre-PR comment was itself a correct-sounding rationale that had silently become false.
• The Watch note's deeper subtraction (delete the dead inbound surface entirely, following the dead-outbound-surface deletions) is a third option with wider scope -- removing a public class's documented contract -- and belongs to its own issue/decision, not to this fix. If the maintainer prefers that direction, this PR does not obstruct it: deleting receive() deletes the mirrored gate with it.

Happy to defer to a maintainer ruling if one lands the other way; absent that, the PR keeps the issue's option 1 as dispatched.

[chenmingwei23]

Disposition: fixed -- span=d27520644eb8

"max_message_chars=40000, max_buttons=5" contradicts executable values 3900 and 10 -> Fix: update the documented values to 3900 and 10.

Legitimate: the spec paragraph this PR already rewrites carried pre-existing stale capability values contradicting SLACK_CAPABILITIES (max_message_chars=SLACK_MSG_LIMIT = 3900, max_buttons=10). Updated docs/system-specs/modules/messaging.md to state 3900 (SLACK_MSG_LIMIT, the shipped send path's split point) and 10 (the checkboxes-element options cap), amended into the single commit; fixed in ae1fce6.

[chenmingwei23]

Answering the Design Review CONCERNS verdict, one disposition per concern:

1. InboundMessage has no from_trusted_bot, so the delegated loop bound is undeliverable -- rebutted (disproportional here), folded into the tracked wiring obligations.

Add from_trusted_bot: bool = False to InboundMessage (compatible dataclass default) in this PR, since admission without it is half a mirror.

The mechanism concern is real and now tracked, but the prescribed change widens the channel-neutral messaging.transport.InboundMessage contract -- a field every transport's normalization carries -- for a path with zero production consumers, and a provenance field nothing reads is exactly the kind of dead surface this PR was told (by the same review fleet, one round earlier) not to grow. The obligation is recorded where the future wiring engineer will hit it: issue #7622 names threading from_trusted_bot to the cap/echo layer (which handle_message_transport already accepts) as a precondition of wiring receive(), whether via an InboundMessage field added when the consumer exists or re-derivation at the seam. This PR's scope stays what #7463 asked: the two paths' ADMISSION decisions agree; delivery of trust provenance downstream belongs to the layer that will consume it.

2. The admission predicate is duplicated verbatim across events.py and transport.py -- accepted-and-deferred to #7622.

Extract the admission predicate into one shared function (e.g. slack/trusted_bots.py) called from both drop sites; note also that events.py reads trusted_bot_ids from live config while the transport freezes it at construction.

Legitimate -- both this lane and First Principles independently converged on it, and it is the cause-level fix for the #7463 class. Deferred rather than folded in because the extraction rewrites the LIVE Socket Mode hot path (events.py) inside a PR whose whole risk posture is "touches only an unwired file", and events.py currently carries pending edits from several open PRs (#3754, #4552, #6307, #7265) that an extraction would conflict with. Issue #7622 (assignee chenmingwei23, due 2026-09-15, deferred-finding) carries the extraction, the config-read-timing decision (live read vs constructor snapshot -- the residual disagreement this comment acknowledges), and the provenance threading from concern 1. Not deferrable-forbidden material: both current copies are correct; the debt is divergence hazard, not a live security gap.

[chenmingwei23]

Answering the First Principles CONCERNS verdict, one disposition per concern:

1. Second spelling of a security decision (extraction into one shared owner) -- accepted-and-deferred to #7622.

Replace both inline copies of the admission predicate with one shared function returning (from_trusted_bot, deny_error) -- put it beside validated_self_bot_id in slack/enterprise.py.

Agreed on the cause-level diagnosis -- the decision needs a single owner, and your framing ("fixes today's divergence by installing the machine that produces tomorrow's") is fair. Deferred rather than folded in because the extraction rewrites the LIVE Socket Mode gate inside a PR whose risk posture is touching only the unwired file, and events.py carries pending edits from open PRs (#3754, #4552, #6307, #7265). The interim divergence hazard is mitigated in-PR: seven tests pin the transport gate's five properties to the events.py semantics, so a one-sided rule change breaks CI instead of passing silently -- prose-free enforcement until #7622 lands the real fix (assignee chenmingwei23, due 2026-09-15, deferred-finding label).

2. The "agreement" is already inexact (live config read vs constructor snapshot) -- accepted-and-deferred, same issue.

Real residual difference, disclosed in #7622's body as a decision the extraction must settle (the shared predicate takes the set as an argument; the wiring layer picks the read timing). Constructor snapshot was kept here deliberately: it matches the transport's existing allowed_users immutability contract, and the alternative -- giving the transport a live config dependency -- is a bigger design change than a latent-defect fix should carry.

3. Doc capability-figure correction (40000->3900, 5->10) is an undeclared rider -- fixed (declared).

Accurate: it landed via the GPT lane's advisory finding on the same spec paragraph this PR rewrites (see the span=d27520644eb8 disposition above). The PR body's Tests/What-changed sections now note it; declared in the body's "What changed" list as of this round.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.