slack-bot: DM manual permission approvals instead of auto-approving - #1707
Merged
Merged
Conversation
…ving Replaces the hardcoded onPermissionAsk: () => "allow_once" with a real human-in-the-loop flow: every ordinary tool-permission ask is DM'd to the Slack user who started the run as a Block Kit card (Allow once / Allow always / Deny), the session shows suspended while pending, and the card updates in place (still a card, no more buttons) once decided or once the run ends/cancels with no answer. - src/approvals.ts (new): PermissionApprovalGateway drives the DM/card lifecycle, correlated purely by the SDK's already-unique askId, and resolves the SDK's onPermissionAsk responder Promise directly rather than calling run.resolveAsk from Slack code. - Args render as a Block Kit table (bold keys, code-styled/raw_number values) rather than raw JSON, since a table can't nest inside a card block Slack-side. - bridge.ts/agentSessions.ts thread the responder through; the old one-time DM greeting (unrelated noise, reset on every bot restart) is removed. - Fails closed (denies) and logs if the approval DM itself can't be sent, instead of hanging or silently proceeding. Closes #1397. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
samuv
requested changes
Sep 18, 2026
samuv
left a comment
samuv
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the clear implementation and thorough test coverage. I left two inline comments about keeping Slack status aligned with the bridge queue and respecting the configured Block Kit length limits.
…iew) Addresses samuv's two review comments on #1707: - Slack session status (processing/suspended/active) was set from agentSessions.ts's runPrompt wrapper, which runs eagerly at call time — before the prompt even joins bridge.ts's per-thread queue. A second same-thread message could overwrite a still-pending run's "suspended" status with "processing" before its own turn arrived, and the first run's final "active" write could later clobber the second run's real state. Fixes by adding onStart/onSettle lifecycle hooks to MecatlBridge.handlePrompt, fired from inside the actual queued execution (#runPrompt) instead of by the caller; agentSessions now drives status from those hooks. Pinned by a new bridge.test.ts case that holds a first run pending on an approval and asserts a second same-thread call's onStart never fires before the first's onSettle. - clamp() appended the ellipsis after a full max-length slice, producing max+1 characters whenever truncation actually triggered — enough to push a card title/subtitle one character past Slack's exact 150-char limit, get the block rejected, and have the ask fail closed (denied) for a reason unrelated to the ask itself. Fixed to slice(0, max - 1) so truncated output is always exactly max characters. Pinned by a 151-character boundary test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
samuv
requested changes
Sep 18, 2026
samuv
left a comment
samuv
left a comment
Contributor
There was a problem hiding this comment.
Thanks for addressing both original comments. The queue-ordering fix and truncation boundary fix look correct. I left one inline follow-up for a session-creation failure path that currently skips the new settlement hook.
Follow-up to samuv's second review pass on #1707: #sessionFor ran before the try/finally that guards onSettle, so onStart had already fired (Slack status set to "processing") by the time a session-creation failure (daemon unreachable, etc.) could skip onSettle entirely, leaving status stuck at "processing" with no run to ever resolve it. Moves the session lookup inside the same try/finally as the run itself, so any failure from #sessionFor onward still reaches onSettle. Pinned by a new bridge.test.ts case pointing MecatlBridge at an unreachable address and asserting onSettle still fires after onStart. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
samuv
approved these changes
Sep 21, 2026
samuv
left a comment
samuv
left a comment
Contributor
There was a problem hiding this comment.
The session-creation failure now crosses the same lifecycle boundary as the run, so onSettle reliably restores status. The focused regression test exercises the real bridge failure path, and CI is green. Approved with one non-blocking test-hardening suggestion inline.
| () => { | ||
| events.push("start"); | ||
| }, | ||
| () => { |
Contributor
There was a problem hiding this comment.
Non-blocking: could this callback return a delayed promise, with the test asserting that handlePrompt does not settle until that promise resolves? The synchronous marker proves onSettle is invoked, but it would still pass if the production await onSettle?.() were accidentally removed. An assertion for the resulting active status call in agentSessions.test.ts would also pin the user-visible outcome.
Member
Author
There was a problem hiding this comment.
thansk, let me fold this into the next PR!
This was referenced Sep 21, 2026
Merged
JAORMX
added a commit
that referenced
this pull request
Sep 21, 2026
Merge origin/main at 686b5e5. Incoming commits after the approved cleanup base: - 79fae4d chore(catalog): refresh models.dev curated subset (#1721) - 73309df chore(deps): bump anthropic-sdk-go (#1713) - e14c88d slack-bot: DM manual permission approvals (#1707) - 5b0db5a feat(sdk): add MCP authorization lifecycle (#1692) - 39715ed chore(deps): bump SDK npm dependencies (#1715) - 498971e plan(sdk): harden malformed-success decode errors (#1698) - 9ac4798 slack-bot: relabel session approval (#1724) - 8de41cd docs(process): route approved cleanup directly (#1720) - a711451 release v0.0.39 (#1727) - 44786d1 fix(sdk): sanitize malformed-success decode errors (#1700) - 85f97fa release TypeScript SDK v0.3.0 (#1729) - 6501b59 fix(ci): repair live compaction slot and metadata (#1728) - 8047188 chore(deps): bump toolhive-core (#1712) - 686b5e5 docs: add README diagrams (#1731) Conflict resolutions preserve the MCP authorization lifecycle from main while applying the approved alpha cleanup: Run and MCP continuation results consume canonical payload usage only; removed EventCommon usage and deprecated aliases stay removed. Keep the landed authorization acceptance record and both the Unreleased cleanup note and released v0.3.0 changelog. Regenerate API reports and user reference from the merged canonical TypeScript sources. Co-Authored-By: mecatl <noreply@mecatl.dev>
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
2 participants
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
sdk/typescript/examples/slack-bot/src/bridge.ts's hardcodedonPermissionAsk: () => "allow_once"with a real human-in-the-loop approval flow: every ordinary tool-permission ask is DM'd to the Slack user who started the run as a Block Kit card (Allow once / Allow always / Deny), backed by a newPermissionApprovalGateway(src/approvals.ts).suspendedwhile an ask is pending; the DM card updates in place (still a card, no buttons) once decided, or if the run ends/is cancelled with no answer.table(bold keys, code-styled/raw_numbervalues) rather than raw JSON, since acardblock can't nest atableon the Slack side.Map<askId, ...>(the SDK'saskIdis already globally unique); resolution goes through the SDK'sonPermissionAskresponder Promise directly, never a manualrun.resolveAskcall from Slack code.app_home_opened) — unrelated noise that reset on every bot restart.Closes #1397.
Test plan
task --dir sdk/typescript/examples/slack-bot typechecktask --dir sdk/typescript/examples/slack-bot linttask --dir sdk/typescript/examples/slack-bot test(75 tests, including an offlinemecated --mock --mock-scriptintegration test that drives a real permission ask end-to-end)mecated(DM approval card, Allow once/Allow always/Deny, session suspend/resume, retracted-card update on run end)Fully or partially written by an AI agent.
🤖 Generated with Claude Code