feat(slack): umbrella Slack UX upgrade — buttons, status, reactions, slash commands#1757
Merged
Conversation
…slash commands Single Slack adapter PR pulling together the in-thread interactivity primitives the team will need on a shared instance: - Interactive Block Kit Approve/Reject buttons on approval gates - Cancel button on a per-run status message edited in place as DAG nodes progress - Lifecycle reactions on the triggering message (🔄 → ✅ / ❌) - Native `/archon` and `/archon-workflow` slash commands (Socket Mode, no URL needed) - `_part i/n_` annotations on long replies split across multiple messages - Italic cost/token footer after direct-chat replies and on terminal workflow status Approve/Reject/Cancel buttons call existing platform-agnostic operations (approveWorkflow / rejectWorkflow / abandonWorkflow); no schema or workflow engine changes. Authorization re-uses the existing SLACK_ALLOWED_USER_IDS whitelist for button clicks and slash commands. Per-user attribution in thread context is intentionally deferred to a separate PR — it needs a user_id column on conversations/messages/workflow_runs and orchestrator plumbing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds interactive in-thread workflow visualization to Slack by introducing Block Kit builder helpers, extending the Slack adapter to track triggering messages and emit cost footers, implementing a workflow-event bridge that mirrors state changes as reactions and editable status messages, hooking the orchestrator to emit result footers, and updating type contracts and documentation to complete the feature. ChangesSlack Workflow Integration with In-Thread UX
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/docs-web/src/content/docs/adapters/slack.md (1)
57-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
im:readscope description (it’s basic DM info, not DM message history).Update the
im:readbullet inpackages/docs-web/src/content/docs/adapters/slack.md(around line 63), sinceim:readis for “basic information about direct messages that your Slack app has been added to”; DM content/message reading corresponds toim:history.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/docs-web/src/content/docs/adapters/slack.md` around lines 57 - 67, Update the `im:read` bullet in packages/docs-web/src/content/docs/adapters/slack.md to say it grants basic information about direct messages the app has been added to (not DM message history); keep `im:history` described as the permission for reading DM message history. Locate the `im:read` and `im:history` bullets in the list and change only the `im:read` description accordingly so it no longer claims it reads DM messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adapters/src/chat/slack/adapter.ts`:
- Around line 422-431: The current unauthorized branch in the Slack adapter
calls respond with an ephemeral denial; change it to silently drop the request
per adapter policy: inside the check using isSlackUserAuthorized(actorId,
this.allowedUserIds) (and referencing actorId, this.allowedUserIds, kind,
getLog(), respond), keep the masked logging via getLog().info({ maskedUserId:
`${actorId.slice(0,4)}***`, kind }, 'slack.slash_unauthorized') but remove the
await respond(...) call and any user-facing text, and simply return to stop
processing the slash command; ensure no additional responses are sent for
unauthorized users.
In `@packages/adapters/src/chat/slack/blocks.ts`:
- Around line 223-225: The footer currently only shows failureReason when
snapshot.terminal === 'failed'; update the condition in the block that builds
footerParts (referencing snapshot.terminal, snapshot.failureReason, and
footerParts) to also include cancelled runs—i.e., check for both 'failed' and
'cancelled' terminal states (or that terminal is not 'completed' and
failureReason exists) and push the truncated failureReason into footerParts so
cancellation reasons are preserved in the terminal footer.
In `@packages/adapters/src/chat/slack/workflow-bridge.ts`:
- Around line 498-507: Replace the user-facing Slack message that includes
err.message with a generic, non-sensitive message while keeping the detailed
error in logs: in the code block that calls
this.adapter.getApp().client.chat.postMessage (using state.channel,
state.threadTs, runId), change the text payload to something like "Could not
cancel run `<runId>`. Please try again or contact support." and ensure the
original err is logged server-side (do not remove existing logging). Do not send
err.message to Slack or other end-users.
---
Outside diff comments:
In `@packages/docs-web/src/content/docs/adapters/slack.md`:
- Around line 57-67: Update the `im:read` bullet in
packages/docs-web/src/content/docs/adapters/slack.md to say it grants basic
information about direct messages the app has been added to (not DM message
history); keep `im:history` described as the permission for reading DM message
history. Locate the `im:read` and `im:history` bullets in the list and change
only the `im:read` description accordingly so it no longer claims it reads DM
messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b453f60c-0b09-41d6-8492-95e244248a17
📒 Files selected for processing (11)
packages/adapters/src/chat/slack/adapter.tspackages/adapters/src/chat/slack/blocks.test.tspackages/adapters/src/chat/slack/blocks.tspackages/adapters/src/chat/slack/index.tspackages/adapters/src/chat/slack/workflow-bridge.test.tspackages/adapters/src/chat/slack/workflow-bridge.tspackages/adapters/src/index.tspackages/core/src/orchestrator/orchestrator-agent.tspackages/core/src/types/index.tspackages/docs-web/src/content/docs/adapters/slack.mdpackages/server/src/index.ts
CI's stricter package-resolution caught that @archon/adapters imports @archon/providers/types (TokenUsage) without declaring the workspace dependency. Locally bun resolved it transitively via @archon/core; CI's clean install does not.
- Drop ephemeral denial from slash command auth path so unauthorized users are silently rejected, matching the existing app_mention / message.im pattern. Posting a denial leaks that a bot is listening. - Surface failureReason on cancelled runs too, not just failed. The type already documents this for both terminal states. - Stop forwarding raw error messages to Slack when a cancel click fails. Backend / DB errors stay in server logs; user sees a generic "check the server logs or try again" line. Adds a test for the cancelled-with-reason rendering.
Collaborator
Author
PR Review Summary — 6-Agent Pass
Critical Issues
Important Issues
Suggestions
Strengths
Verdict: NEEDS FIXESC1–C3 are blocking. I1–I5 are real correctness/UX gaps that should land before merge; the test gaps (I6–I10) and docs (D1–D3) can land alongside or in a fast follow. Recommended Order
|
Critical:
- Declare @archon/workflows as an explicit workspace dep on @archon/adapters
(same class of fix as the providers one). Resolves today via hoisting but
breaks under stricter installs.
- Split workflow-bridge.test.ts into its own bun test invocation so its
irreversible mock.module() calls on @archon/core and
@archon/workflows/event-emitter cannot leak into the slack/telegram
batch.
- Fix "trailing-edge" debounce comments — the implementation is
leading-edge. Document the Slack chat.update rate limit as the 500ms
rationale.
Important:
- Wire slackBridge.detach() into the server graceful shutdown path so the
event subscription doesn't leak and a pending chat.update can't fire
against a closed Bolt socket.
- Drop dead `comment` plumbing through handleApprovalDecision /
applyResolutionEdit / buildApprovalResolutionBlocks — Block Kit buttons
have no UI to capture it.
- Widen the action-handler try/catch to also cover applyResolutionEdit so
block-builder or chat.update failures don't bubble as unhandled
rejections.
- Cancel-click with missing run state now logs and posts an ephemeral
acknowledgement (using the button message's channel/ts) so the user
isn't left wondering whether the click registered.
- Use Bolt's BlockButtonAction / ButtonAction types directly on the
app.action() registrations instead of the ad-hoc ActionBody /
ActionElement aliases.
Test coverage:
- Slash command silent-rejection of unauthorized users.
- triggeringMessages 1000-entry FIFO eviction at the cap boundary.
- Slash command seed-post failure → ephemeral error + handler not called.
- Single-chunk message path skips the _part i/n_ footer.
- rejectWorkflow → { cancelled: true, maxAttemptsReached: true } branch.
Docs:
- architecture.md IPlatformAdapter listing includes sendResultFooter.
- approval-nodes.md mentions the Slack in-thread Approve button.
- CLAUDE.md test-isolation batch count for @archon/adapters updated to 6
(was 3 — pre-existing drift, now also accounts for workflow-bridge).
Polish:
- removeReactionSafe gets the same intentional-fallback comment as
addReactionSafe (no_reaction is a normal terminal-state interleave).
- IPlatformAdapter.sendResultFooter signature uses TokenUsage directly.
- Drop "for v1" tag on the unhandled-event comment.
- Remove what-comments from blocks.ts / blocks.test.ts / adapter.ts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/archon+/archon-workflowslash commands over Socket Mode,_part i/n_annotations on split messages, and an italic cost/token footer after assistant replies.user_idplumbing (deferred — needs schema migration), no other adapters (Telegram/Discord/GitHub untouched), no workflow engine / event emitter changes, no DB schema changes.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
WorkflowEventEmitterSlackWorkflowBridgeadapter.getTriggeringMessageSlackWorkflowBridgeSlackAdapter.getApp().client.chat.*SlackWorkflowBridgeSlackAdapter.getApp().client.reactions.*SlackWorkflowBridgeworkflowOperations.{approveWorkflow,rejectWorkflow,abandonWorkflow}SlackWorkflowBridgeworkflowDb.getWorkflowRunmetadata.total_cost_usdon terminal eventsSlackAdapter.start()app.command('/archon'|'/archon-workflow')OrchestratorIPlatformAdapter.sendResultFooterserver/index.tsSlackWorkflowBridgeslack.start()Label Snapshot
risk: lowsize: Ladaptersadapters:slackChange Metadata
featureadaptersLinked Issue
Validation Evidence (required)
bun run validateexit-code 0 on the branch tip.commands,reactions:write) installed on a real Slack app. Listed under Human Verification.Security Impact (required)
commands(slash commands) andreactions:write(lifecycle emoji). Documented inslack.md.SLACK_BOT_TOKEN/SLACK_APP_TOKEN.SLACK_ALLOWED_USER_IDSwhitelist inside every action handler. Unauthorized clicks are silently dropped and logged with masked user IDs. Verified byunauthorized click is silently droppedtest inworkflow-bridge.test.ts.Compatibility / Migration
sendResultFooteris an optional method onIPlatformAdapter; only Slack implements it. Telegram/Discord/GitHub/Web flow unchanged.commands+reactions:writescopes and register the two slash commands in the Slack app config and reinstall to workspace (one-time manual step, documented).dev, deploycommandsandreactions:writeto Bot Token Scopes/archonand/archon-workflowslash commands (blank Request URL)Human Verification (required)
bun run validate(10 packages, all tests pass). The two new unit test files cover: cost footer formatting (cost-only, tokens-only, combined, NaN/Infinity edge cases), approval/resolution/status block builders, reaction name constants, workflow_started → status post + reaction add, approval_pending → buttons in-thread, approve button →approveWorkflowcalled + message edited, reject under retry threshold → "will retry" note, cancel →abandonWorkflow, unauthorized click silently dropped, workflow_completed → reaction swap + cost in context block, workflow_failed → x reaction + reason._part i/n_; bridge gracefully drops events for non-Slack conversations (verified bydoes nothing when there is no Slack triggertest); reactions API failures are swallowed-and-logged (won't fail the workflow);chat.updateon a missing approval message is non-fatal (logged at warn).chat.updateif a DAG has very fast nodes (debounced 500ms but theoretical) andnot_in_channelerrors if the bot was added to a channel withoutchannels:joinalready there.Side Effects / Blast Radius (required)
sendResultFootercall on adapters that implement it (no-op for Telegram/Discord/GitHub/Web).chat.updaterequires the bot to own the message — guaranteed since the bridge tracksresult.tsfrom its ownchat.postMessage. If the message is deleted manually, edits fail silently (logged).triggeringMessagesmap is bounded at 1000 entries with FIFO eviction; chat-only conversations that never run a workflow won't grow it.sendResultFootercall after the final assistant chunk — currently only Slack implements it, but any adapter that adopts it should be prepared for it to run before the orchestrator decides to dispatch a workflow (current code skips it correctly on dispatch via early-return).getLog().warn/.debugwith named events (slack.bridge_reaction_add_failed,slack.bridge_status_update_failed, etc.) — easy to grep in production logs.Rollback Plan (required)
git revert <merge-commit>ondev. The PR is self-contained — no schema changes, no migrations, no other-adapter touch points. Reverting restores the pre-PR Slack adapter behavior exactly.SLACK_BOT_TOKENempty (skips the whole Slack adapter init).slack.bridge_approval_action_failedwith the run/node IDapp.start()failing post-attach →slack.bot_startedlog never fires; existingslack_adapter_skippedpath triggers on missing tokensorchestrator.result_footer_failedwarn logsRisks and Mitigations
SLACK_ALLOWED_USER_IDSaccess could click Cancel on another teammate's workflow.chat.updaterate limit hit on DAGs with rapid node transitions.ack()within 3 seconds, andapproveWorkflow/rejectWorkflowmake DB writes.await ack()before invoking operations. Operations are pure DB writes (no AI calls, no network) — fast in practice.<@U123> ran /archon-workflow list) to create a thread root, which is more visible than a pure ephemeral response.slack.md; chosen deliberately so the conversation has a realtsto thread under. An ephemeral acknowledgement is also sent to the invoker so they see "see thread for output."Summary by CodeRabbit
New Features
Documentation
Tests