Skip to content

Fail closed when Slack mention detection is unavailable#91584

Merged
steipete merged 4 commits into
openclaw:mainfrom
hiragram:fix/slack-require-mention-fail-closed
Jul 5, 2026
Merged

Fail closed when Slack mention detection is unavailable#91584
steipete merged 4 commits into
openclaw:mainfrom
hiragram:fix/slack-require-mention-fail-closed

Conversation

@hiragram

@hiragram hiragram commented Jun 9, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

What Problem This Solves

Slack channels with requireMention: true could accept ordinary messages when startup could not establish a trustworthy bot identity. A failed auth.test left mention detection unavailable, while a successful user-token auth.test could incorrectly treat the human token owner as the bot mention target.

Why This Change Was Made

The Slack adapter now fails closed only at its own admission boundary after sender, bot-message, and command authorization. It accepts trusted app_mention events, configured mention patterns, and authorized control commands. Bot mention identity is populated only when Slack returns the documented bot-token bot_id shape.

The implementation also coordinates Slack's duplicate message and app_mention delivery paths. A trusted retry removes only the matching routed pending-history entry, and the history writer rechecks the race guard at the actual mutation boundary so late async sender/media resolution cannot reinsert stale context.

User Impact

  • Required-mention Slack channels no longer activate on ordinary messages when native bot identity is unavailable.
  • User-token identities cannot make mentions of a human look like bot mentions.
  • Trusted Slack mentions, configured mention patterns, and authorized control commands continue to work.
  • Startup and probe warnings explain that explicit bot-mention detection is disabled until a Bot User OAuth Token is configured.

Evidence

  • Official Slack auth.test contract inspected: bot-token responses include both user_id and bot_id; invalid authentication returns an error.
  • Configured live probe: pnpm openclaw channels status --channel slack --probe --json returned probe.ok: true, HTTP 200, and bot/team identity in 150 ms; no token was printed.
  • Live negative probe: a deliberately invalid literal token returned invalid_auth.
  • Focused Blacksmith Testbox tbx_01kwqwq41acy7s12nh7kjcxstj: 4 files, 126 tests passed.
  • Final changed-surface Blacksmith Testbox tbx_01kwqwx5hzpczf12srwrjfh801: extension production/test typechecks, lint, dependency/architecture guards, and import-cycle checks passed.
  • Fresh final Codex autoreview: no accepted or actionable findings.

@openclaw-barnacle openclaw-barnacle Bot added channel: slack Channel integration: slack size: S triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels Jun 9, 2026
@hiragram hiragram changed the title Slackのメンション検出不能時にfail-closedする Fail closed when Slack mention detection is unavailable Jun 9, 2026
@clawsweeper

clawsweeper Bot commented Jun 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 9:32 PM ET / 01:32 UTC.

Summary
The branch changes Slack auth.test identity trust, required-mention Slack admission, warning copy, app_mention/message duplicate-history suppression, and Slack regression tests.

PR surface: Source +1108, Tests +2536, Docs +23, Other +4. Total +3671 across 66 files.

Reproducibility: yes. from source inspection: current main can leave botUserId unavailable or untrustworthy while the shared mention decision only skips required-mention traffic when mention detection is available. I did not run a live Slack workspace reproduction in this read-only review.

Review metrics: 3 noteworthy metrics.

  • Effective merge surface: 8 Slack files, +405/-38 against current main. The GitHub base diff is inflated by stale-base drift, while the computed merge result is a focused Slack change.
  • Slack fail-closed admission: 1 drop path added. Required-mention Slack messages can now be suppressed when bot mention detection is unavailable, which is intentional but upgrade-sensitive.
  • Slack auth identity source: 1 identity rule changed. auth.test user_id no longer becomes botUserId unless Slack also returns bot_id, affecting user-token configurations.

Stored data model
Persistent data-model change detected: persistent cache schema: src/gateway/server-maintenance.test.ts, serialized state: src/agents/embedded-agent-runner/run/preemptive-compaction.ts, serialized state: src/auto-reply/reply/queue/state.test.ts, serialized state: src/auto-reply/reply/queue/state.ts, serialized state: src/sessions/user-turn-transcript.ts, serialized state: src/tui/tui-pty-local.e2e.test.ts, and 1 more. Confirm migration or upgrade compatibility proof before merge.

Merge readiness
Overall: 🦐 gold shrimp
Proof: 🦐 gold shrimp
Patch quality: 🐚 platinum hermit
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add redacted real Slack proof for the unavailable-identity required-mention message drop and a trusted app_mention or configured-pattern success path.
  • [P1] Have a maintainer explicitly accept the fail-closed compatibility impact before merge.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: Live auth.test probes and Testbox results are useful, but they do not directly show the after-fix unavailable-identity required-mention Slack message drop path; add redacted logs, live output, terminal output, or a short recording and update the PR body so ClawSweeper can re-review.

Mantis proof suggestion
A real Slack proof lane would materially show the transport-visible message admission and drop behavior that unit tests cannot fully demonstrate. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis slack desktop smoke: verify a required-mention Slack channel drops an ordinary message when bot mention identity is unavailable and still accepts a trusted app_mention.

Risk before merge

  • [P1] Existing Slack workspaces with a user token in the bot-token slot, or with auth.test unable to establish bot identity at startup, can stop delivering ordinary required-mention room messages until operators configure a Bot User OAuth Token, a matching mention pattern, a trusted app_mention path, or an authorized command path.
  • [P1] The branch changes Slack duplicate message/app_mention history suppression, so maintainers should explicitly accept the message-delivery behavior even though focused tests cover both race orderings.
  • [P1] The current proof does not directly show the central live Slack unavailable-identity required-mention drop path after the fix.

Maintainer options:

  1. Require real Slack drop proof (recommended)
    Ask for redacted logs, live output, terminal output, or a short recording that shows an ordinary required-mention Slack channel message being dropped when bot identity is unavailable and a trusted app_mention still activating.
  2. Accept the fail-closed upgrade impact
    Maintainers can decide the stricter Slack behavior is worth the compatibility cost for misconfigured or auth.test-degraded required-mention workspaces.
  3. Pause for a compatibility design
    If silently stopping ordinary messages is too disruptive, pause this PR and replace it with a warning/doctor or opt-in strict-mode path.

Next step before merge

  • [P1] Human review should require the missing real Slack drop proof and decide whether the fail-closed Slack upgrade behavior is acceptable before merge.

Security
Cleared: No supply-chain, workflow, dependency-source, permission, or secret-handling expansion was found; the token-sensitive change tightens Slack identity trust.

Review details

Best possible solution:

Land the Slack owner-boundary fix after redacted real Slack proof and explicit maintainer acceptance of the fail-closed upgrade behavior; if that behavior is too disruptive, rework toward an operator-visible warning, doctor repair, or opt-in strict mode.

Do we have a high-confidence way to reproduce the issue?

Yes, from source inspection: current main can leave botUserId unavailable or untrustworthy while the shared mention decision only skips required-mention traffic when mention detection is available. I did not run a live Slack workspace reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the code shape: the fix belongs in the Slack adapter admission boundary and preserves trusted app_mention, configured-pattern, and authorized-command paths. The remaining question is upgrade acceptance and real Slack proof, not a line-level correctness defect.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 1b8d83767445.

Label changes

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.

Label justifications:

  • P2: This is a focused Slack channel admission bug fix with limited blast radius but real message-delivery impact.
  • merge-risk: 🚨 compatibility: Existing Slack setups using user tokens or degraded auth.test identity can stop receiving ordinary required-mention room messages after upgrade.
  • merge-risk: 🚨 message-delivery: The patch changes which Slack message and app_mention events are dropped, dispatched, or retained in pending history.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Live auth.test probes and Testbox results are useful, but they do not directly show the after-fix unavailable-identity required-mention Slack message drop path; add redacted logs, live output, terminal output, or a short recording and update the PR body so ClawSweeper can re-review.
Evidence reviewed

PR surface:

Source +1108, Tests +2536, Docs +23, Other +4. Total +3671 across 66 files.

View PR surface stats
Area Files Added Removed Net
Source 35 1395 287 +1108
Tests 27 2664 128 +2536
Docs 2 23 0 +23
Config 0 0 0 0
Generated 0 0 0 0
Other 2 7 3 +4
Total 66 4089 418 +3671

What I checked:

  • Repository policy read: The full root AGENTS.md and relevant scoped AGENTS.md files for extensions, gateway, TUI, docs, scripts, agents, and tests were read; the compatibility-sensitive fail-closed and message-delivery review rules apply. (AGENTS.md:1, 1b8d83767445)
  • Current main still trusts auth.test user_id: Current main assigns botUserId directly from auth.user_id, so a user-token auth.test response can still populate the bot mention target before this PR. (extensions/slack/src/monitor/provider.ts:349, 1b8d83767445)
  • Current main lacks unavailable-detection drop: Current main only skips required-mention traffic when canDetectMention is true through the shared mention decision, leaving Slack without a local drop when bot mention detection is unavailable. (extensions/slack/src/monitor/message-handler/prepare.ts:946, 1b8d83767445)
  • PR head implements bot_id-gated identity: The PR head normalizes auth.user_id but only assigns botUserId when Slack auth.test also returns bot_id, and updates the startup warning for fail-closed required-mention behavior. (extensions/slack/src/monitor/provider.ts:349, 03befdabb07d)
  • PR head implements Slack-local fail closed: The PR head drops required-mention Slack room messages when mention detection is unavailable and there is no effective trusted mention, after sender, bot-message, and command authorization gates. (extensions/slack/src/monitor/message-handler/prepare.ts:1041, 03befdabb07d)
  • Computed merge surface is Slack-only: GitHub's computed merge commit against current main changes the same eight Slack files as the direct current-main-to-head diff, so the 66-file GitHub base diff is stale-base inflation rather than the effective merge surface. (17b796c4a8b0)

Likely related people:

  • steipete: Recent current-main work includes the centralized mention-policy refactor and the merged Slack user-token warning PR, and this branch's latest commits refine the Slack owner-boundary fix. (role: recent adjacent contributor and PR refiner; confidence: high; commits: 625fd5b3e3e2, 65e2819136ad, 7e46ae7771fb; files: src/channels/mention-gating.ts, extensions/slack/src/monitor/provider.ts, extensions/slack/src/monitor/message-handler.ts)
  • Hana Chang: Added the nearby ignoreOtherMentions Slack admission path that shares the same mention-filtering and dropped-history area this PR extends. (role: adjacent Slack mention filtering contributor; confidence: medium; commits: 4ec7842be0af; files: extensions/slack/src/monitor/message-handler/prepare.ts)
  • Xavier Coulon: Recently changed Slack auth.test handling in provider startup tests and code, which is adjacent to this PR's auth.test identity trust rule. (role: adjacent Slack auth.test contributor; confidence: medium; commits: fbc12e0879f7; files: extensions/slack/src/monitor/provider.ts, extensions/slack/src/monitor/provider.auth-test-token.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (3 earlier review cycles)
  • reviewed 2026-06-21T16:34:37.196Z sha 0fecc9b :: needs real behavior proof before merge. :: [P1] Keep fail-closed behavior out of the shared gate | [P3] Use a detection-unavailable Slack drop reason
  • reviewed 2026-07-05T01:14:55.709Z sha 5321b35 :: needs real behavior proof before merge. :: none
  • reviewed 2026-07-05T01:20:55.477Z sha 5321b35 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. labels Jun 9, 2026
@steipete
steipete force-pushed the fix/slack-require-mention-fail-closed branch from 0fecc9b to 5321b35 Compare July 5, 2026 01:10
@openclaw-barnacle openclaw-barnacle Bot added size: M triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. and removed size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 5, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jul 5, 2026
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Land-ready verification for 5321b3509963d470fb72b4d153e8fd2503baa236:

  • Reworked the change at the Slack owner boundary; shared channel mention gating remains unchanged.
  • Added fail-closed admission after sender, bot-message, and command authorization, preserving trusted app_mention, configured-pattern, and authorized-command activation.
  • Required Slack auth.test to provide bot_id before its user_id is trusted as bot mention identity; team/app metadata remains available.
  • Coordinated duplicate message / app_mention delivery in both orderings, including mutation-time guards after async sender/media work.
  • Live configured probe: pnpm openclaw channels status --channel slack --probe --json returned healthy bot/team identity; a deliberately invalid literal token returned invalid_auth.
  • Focused proof: corepack pnpm test extensions/slack/src/monitor/message-handler/prepare.test.ts extensions/slack/src/monitor/message-handler.app-mention-race.test.ts extensions/slack/src/monitor/provider.auth-test-token.test.ts extensions/slack/src/probe.test.ts — 126 passed on Blacksmith Testbox tbx_01kwqwq41acy7s12nh7kjcxstj.
  • Broad proof: OPENCLAW_CHECK_CHANGED_REMOTE_CHILD=1 OPENCLAW_CHANGED_LANES_RAW_SYNC=1 corepack pnpm check:changed — passed on Blacksmith Testbox tbx_01kwqwx5hzpczf12srwrjfh801.
  • Fresh final Codex autoreview: no accepted or actionable findings.
  • Hosted gates: CI run 28725291084 green (44 passed, 10 skipped); CodeQL Critical Quality 28725291094, OpenGrep 28725291086, and Real behavior proof 28725301953 green.

Known proof gaps: none.

@steipete
steipete requested a review from a team as a code owner July 5, 2026 01:26
@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui gateway Gateway runtime scripts Repository scripts agents Agent runtime and tooling size: XL and removed size: M labels Jul 5, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jul 5, 2026
@steipete
steipete merged commit fe82346 into openclaw:main Jul 5, 2026
148 of 154 checks passed
@steipete

steipete commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Merged via squash.

@github-actions github-actions Bot mentioned this pull request Jul 5, 2026
Open
github-actions Bot pushed a commit to Desicool/openclaw that referenced this pull request Jul 5, 2026
@hiragram @openclaw-agent @steipete
Fail closed when Slack mention detection is unavailable (openclaw#91584)
8cedb32 
* fix(slack): fail closed when mention detection is unavailable

* fix(slack): coordinate app mention retry history

* fix(slack): reject user-token mention identity

* fix(slack): reject user-token mention identity

---------

Co-authored-by: OpenClaw Agent <openclaw-agent@users.noreply.github.com>
Co-authored-by: Peter Steinberger <steipete@gmail.com>
wheakerd pushed a commit to wheakerd/clawdbot that referenced this pull request Jul 15, 2026
@hiragram @openclaw-agent
@steipete @RomneyDa
Fail closed when Slack mention detection is unavailable (openclaw#91584)
a20d9d7 
* fix(slack): fail closed when mention detection is unavailable

* fix(slack): coordinate app mention retry history

* fix(slack): reject user-token mention identity

* fix(slack): reject user-token mention identity

---------

Co-authored-by: OpenClaw Agent <openclaw-agent@users.noreply.github.com>
Co-authored-by: Peter Steinberger <steipete@gmail.com>
(cherry picked from commit fe82346)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

agents Agent runtime and tooling app: web-ui App: web-ui channel: slack Channel integration: slack docs Improvements or additions to documentation gateway Gateway runtime merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 message-delivery 🚨 May drop, duplicate, misroute, suppress, or wrongly target messages. P2 Normal backlog priority with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. scripts Repository scripts size: XL status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hiragram @steipete @openclaw-agent