Skip to content

fix(slack): route approval-DM binding lookup through overridable helper (cloud Slack approvals were dead) - #2325

Open
federicodeponte wants to merge 1 commit into
mainfrom
fix/slack-approval-dm-cloud-binding
Open

fix(slack): route approval-DM binding lookup through overridable helper (cloud Slack approvals were dead)#2325
federicodeponte wants to merge 1 commit into
mainfrom
fix/slack-approval-dm-cloud-binding

Conversation

@federicodeponte

Copy link
Copy Markdown
Member

Problem (confirmed by live cloud test)

A worker with `approvals.required: true` pauses correctly at `pending_approval`, but the Slack Approve/Reject button DM is never sent in hosted cloud. The approve endpoint works; zero Slack DM fires. The whole Slack-approval feature is dead in cloud.

Root cause: `notify_pending_approval_via_slack` (channels/common.py) did a raw local-DB (`get_db()`) reverse lookup for the owner->slack_user binding. In cloud, `get_db()` is the local sqlite sidecar, which is always empty — cloud writes bindings only to Supabase (`slack_sender_bindings`). The lookup returned None and the function silently returned. The cloud's `apply_cloud_slack_overrides()` patches `_slack_bot_token_for_team` / `_slack_binding_user_id` to Supabase, but this reverse lookup went through none of them.

Fix (boundary-respecting: one overridable seam)

  • Extract the reverse lookup into `channels.slack._slack_binding_for_owner(owner_id) -> (slack_user_id, team_id) | None` — the single overridable seam, imported lazily by module attribute so a cloud monkeypatch takes effect at call time.
  • OSS single-tenant keeps reading the local `slack_sender_bindings` table (unchanged behavior).
  • Cloud overrides this helper in `workeros-cloud` `startup.apply_cloud_slack_overrides()` to read the Supabase-backed bindings (companion cloud PR).
  • Bot-token resolution already routed correctly through the cloud-patched `_slack_bot_token_for_team`; unchanged.
  • Added clear log lines: `info` on successful DM send; `info` when there is genuinely no binding / no install / no bot token (diagnosable, not silent); `warning` when `conversations.open` returns no channel.

No change to the approve/resume logic, the Block Kit Approve/Reject buttons, or the `/slack/interactivity` handler.

Tests

  • `test_slack_notify_routes_through_overridable_helper`: overrides `_slack_binding_for_owner` + `_slack_bot_token_for_team` (cloud-style), seeds nothing in the local DB, asserts the DM + Approve/Reject blocks are still posted — proving the raw `get_db()` path is no longer used.
  • `test_slack_binding_for_owner_reads_local_binding`: OSS path returns the local binding / None.
  • Existing `test_slack_approvals_1368` + `test_slack_sender_bindings` + `test_whatsapp_approvals` + approval suite remain green (56 tests).

🤖 Generated with Claude Code

notify_pending_approval_via_slack did a raw local-DB (get_db) reverse
lookup for the owner->slack_user binding. In cloud the local sqlite
sidecar is always empty (bindings live in Supabase), so the lookup
returned None and the Approve/Reject DM never fired — the whole
Slack-approval feature was dead in hosted cloud.

Extract the reverse lookup into channels.slack._slack_binding_for_owner
(the single overridable seam, imported lazily by module attribute so a
cloud monkeypatch takes effect). OSS single-tenant keeps reading the
local slack_sender_bindings table; cloud overrides this helper (in
startup.apply_cloud_slack_overrides) to read the Supabase-backed
bindings. Bot-token resolution already routed through the cloud-patched
_slack_bot_token_for_team.

Add clear log lines: info on successful DM send, info when there is
genuinely no binding / no install / no bot token (diagnosable, not
silent). No change to approve/resume logic or the Block Kit buttons.

Tests: prove the send routes through the overridable helper with a
cloud-style binding and NOTHING seeded in the local DB, and that the
OSS helper reads the local binding.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@federicodeponte

Copy link
Copy Markdown
Member Author

⚠️ Scope note before anyone bumps this into workeros-cloud prod:

On cloud main / prod (build_sha 67fb119c, PR floomhq/workeros-cloud#1218 "native approval buttons", merged 2026-07-19) the engine's `channels.common.notify_pending_approval_via_slack` is replaced wholesale at startup by the cloud-native CHANNEL-post implementation (`apps/api/cloud_slack_approvals.py::apply_cloud_slack_approval_override`, wired via `register_cloud_components` -> `apply_cloud_slack_overrides`). So on prod the engine DM function in this PR is never called — this change is inert there.

This PR IS correct + useful for the paths where the engine function is live: OSS single-tenant and the cloud ci/staging-migrate-0050 staging branch (which predates #1218 and has no cloud_slack_approvals override — that's the env the original "empty local sqlite" diagnosis actually matches).

It does not fix the prod customer report on its own. Prod's Slack approval notification is channel-based and requires (a) the worker manifest to set `notify.slack_channel_id` with `on` including a pending event, and (b) an active native Slack install + bot token for the workspace. Whether prod should ALSO DM the run owner (or fall back to DM when no channel is configured) is a product decision, not covered here. Safe to merge as engine hygiene; do not treat a submodule bump as the prod fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant