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>
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