feat(gateway): Discord approval prompts for sensitive tool calls#737
Conversation
Consume permission.asked/permission.replied over SSE; in-memory registry keyed by requestID with reject-cascade reconciliation. Reorder run-core to create-subscribe-prompt to eliminate the permission.asked race.
…/program wiring Approval embed+buttons (custom_id codec), program-scoped approval registry (channel-binding + single-winner claim + reply POST), per-run coordinator wiring with a sub-deadline of runTimeoutMs, button interaction handler with live authorization, and shutdown-triggered fail-closed settle. Reply routes through the V1 session-scoped permission endpoint with directory routing.
Cross-seam integration tests (approve/deny exactly-once, single-winner races, channel-binding, reject cascade, deadline fail-close, shutdown drain) and a run-core reply-route regression guard. Operator docs for the permission: ask knob in AGENTS.md and deploy/README. Strip internal taxonomy from approval doc comments.
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Thorough, well-reasoned implementation. The single-owner settlement model (open → claimed → confirmed) is coherent, the deadline/button/dispose race ordering is carefully documented and matches the code, and the full gateway suite is green (676 passing, verified locally). The Discord side-effect injection keeps the registry/coordinator pure and unit-testable, and redaction-safety (no raw patterns/inputs/requestID in embeds, allowedMentions: {parse: []} everywhere) is consistently applied. Fail-closed behavior on deadline, dispose, and shutdown is correct.
Blocking issues
None
Non-blocking concerns
-
deferReplyhappens after the auth REST call (program.ts:171-179).userIsAuthorizedperformsguild.members.fetch()(REST) beforeinteraction.deferReply(). Discord's interaction ack window is 3s; a slow member fetch could expire the interaction before the defer lands, causing theeditReplyto throw (caught/logged, but the click silently does nothing from the user's view). The decision itself still settles correctly via the registry, so this is a UX/observability edge, not a correctness break. Consider deferring first, then authorizing, theneditReply. -
Button approval authorization is guild-scoped, not binding-scoped (
program.ts:171). Any guild member holding the trigger role /ManageChannelscan approve a tool call in any thread, not just channels they're associated with. This is consistent with the mention trigger gate (documented model), so it's intentional — flagging only so it's a conscious choice. The channel-binding check inhandleButtonDecisionprevents cross-channel clicks, but not cross-user-within-guild. -
disposeAllruns before awaiting in-flight runs on shutdown (program.ts:286-287). A run still draining its SSE loop couldregister()a new approval afterdisposeAllclears the map. That late approval is only failed-closed by the run's owncoordinator.dispose()in itsfinally. Functionally safe (still fail-closes), but the global drain isn't a hard barrier — worth a comment noting the per-run dispose is the backstop. -
Re-ask overwrites and re-sends an embed (
registry.ts:230-240). A duplicatepermission.askedfor the same requestID clears the old timer and overwrites the entry, thenonPendingposts a second embed. The old embed's buttons become orphaned (not-foundon click). Low likelihood given globally-uniqueper_*IDs, and handled gracefully, but leaves a stale prompt in the thread.
Missing tests
None blocking. Coverage is strong across coordinator, registry, run/program wiring, and end-to-end approve/deny/deadline/cascade/concurrent paths. Two gaps worth adding later: (1) a test asserting the defer-vs-auth timing path when userIsAuthorized is slow, and (2) the re-ask/duplicate-requestID embed-overwrite path at the run.ts integration level (registry-level overwrite is covered).
Risk assessment (LOW)
Blast radius is contained to the new approvals/ module and additive wiring in run/program; existing paths are untouched when no tool is set to ask (default no-op). Security exposure is low — fail-closed on every settlement path, redaction-safe embeds, and reuse of the existing authorization gate. Regression likelihood is low given the comprehensive race-focused test suite and clean Effect/Result boundary adherence per the package's AGENTS.md.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26853827698 |
| Cache | hit |
| Session | ses_1755bddc0ffeZieoepsbvV2X5F |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review after commit 743cf94. The new commit cleanly resolves both actionable non-blocking items from my prior (dismissed) review, and the rest of the implementation stands as previously assessed: a coherent single-owner settlement model, carefully ordered deadline/button/dispose race handling, redaction-safe embeds, and fail-closed behavior on every settlement path. Full gateway suite green — 679 passing (up from 676, the three added tests cover the fixes), verified locally.
Resolved since last review:
- ✅ defer-before-auth (
program.ts:169) —interaction.deferReply()now acks within Discord's 3 s window before the (potentially slow)userIsAuthorizedREST call; all subsequent responses correctly useeditReply. - ✅ stale re-ask embed (
registry.ts:238-243) — a duplicateregister()now best-effort renders the prior embed with the newsupersededreason (🔄 title inbuildSettledEmbed), so orphaned buttons become visibly inert instead of silently goingnot-found. Thesupersededvariant is wired through theSettlementReasonunion and covered by a registry test.
Blocking issues
None
Non-blocking concerns
-
Button approval authorization is guild-scoped, not binding-scoped (
program.ts:177). Any guild member with the trigger role /ManageChannelscan approve a tool call in any thread. Consistent with the mention trigger gate (documented model) — flagged only as a conscious-choice confirmation. ThechannelIDbinding check inhandleButtonDecisionstill prevents cross-channel clicks. -
disposeAllruns before awaiting in-flight runs on shutdown (program.ts:294-295). A run still draining its SSE loop couldregister()a late approval after the map is cleared; it is then failed-closed by that run's owncoordinator.dispose()in itsfinally. Functionally safe (still fail-closes) — the per-run dispose is the backstop, just not a single hard global barrier.
Missing tests
Minor: the buildSettledEmbed variant loop in approvals.test.ts:302 iterates ['replied','cascade','deadline','disposed'] and omits 'superseded'. The superseded branch is exercised at the registry level, so this is a small directness gap, not a coverage hole.
Risk assessment (LOW)
Blast radius is contained to the new approvals/ module and additive run/program wiring; existing paths are untouched when no tool is set to ask (default no-op). Security exposure is low — fail-closed on every settlement path, redaction-safe embeds, reuse of the existing authorization gate, and the defer-first fix removes the interaction-expiry edge. Regression likelihood is low given the comprehensive race-focused suite and clean Effect/Result boundary adherence.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26854880903 |
| Cache | hit |
| Session | ses_1755bddc0ffeZieoepsbvV2X5F |
fro-bot
left a comment
There was a problem hiding this comment.
Verdict: PASS
Re-review after commit 8a87940. This closes the only outstanding note from my prior review — the buildSettledEmbed variant test (approvals.test.ts:302) now includes 'superseded' in the coverage loop. Every actionable item raised across all three review passes is now resolved. The implementation continues to stand as assessed: a coherent single-owner settlement model (open → claimed → confirmed), carefully ordered deadline/button/dispose race handling, redaction-safe embeds, defer-before-auth interaction acking, and fail-closed behavior on every settlement path. Full gateway suite green — 679 passing, verified locally.
Resolved across the review thread:
- ✅ defer-before-auth (
program.ts:169) — acks within Discord's 3 s window before the slowuserIsAuthorizedREST call. - ✅ stale re-ask embed (
registry.ts:238-243) — duplicateregister()renders the prior embed assuperseded, neutralizing orphaned buttons. - ✅ superseded embed coverage (
approvals.test.ts:302) — the variant is now exercised directly.
Blocking issues
None
Non-blocking concerns
-
Button approval authorization is guild-scoped, not binding-scoped (
program.ts:177). Any guild member with the trigger role /ManageChannelscan approve a tool call in any thread. Consistent with the mention trigger gate (documented model) — flagged as a conscious-choice confirmation, not a defect. ThechannelIDcheck inhandleButtonDecisionstill prevents cross-channel clicks. -
disposeAllruns before awaiting in-flight runs on shutdown (program.ts:294-295). A run still draining its SSE loop couldregister()a late approval after the map is cleared; that approval is then failed-closed by the run's owncoordinator.dispose()in itsfinally. Functionally safe — the per-run dispose is the backstop, just not a single hard global barrier.
Missing tests
None
Risk assessment (LOW)
Blast radius is contained to the new approvals/ module and additive run/program wiring; existing paths are untouched when no tool is set to ask (default no-op). Security exposure is low — fail-closed on every settlement path, redaction-safe embeds, reuse of the existing authorization gate, and the defer-first acking removes the interaction-expiry edge. Regression likelihood is low given the comprehensive race-focused suite and clean Effect/Result boundary adherence per the package's AGENTS.md.
Run Summary
| Field | Value |
|---|---|
| Event | pull_request |
| Repository | fro-bot/agent |
| Run ID | 26855116864 |
| Cache | hit |
| Session | ses_1755bddc0ffeZieoepsbvV2X5F |
Adds an interactive approval gate so workspace tool calls configured as
asksurface in Discord with Approve/Deny buttons before they run.What this does
When a deployed workspace runs OpenCode with a tool gated to
ask(e.g.permission: { bash: "ask" }), the gateway now:A single owner tracks each request's lifecycle (
open → claimed → confirmed); OpenCode's reply event is the authoritative settlement signal, with the button click, deadline, and shutdown all resolving through that one owner so a request settles exactly once. Per-run teardown only releases that run's pending approvals; concurrent runs in other channels are untouched.Operator-facing
Tool approval activates automatically whenever a workspace's OpenCode config marks a tool
ask. No new required secrets.deploy/README.mdand the gateway notes document how to enable it.Tests
Coordinator, registry, run/program wiring, and end-to-end approval/deny/deadline/cascade/concurrent-run paths are covered. Full gateway suite green.