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

apps/api/channels/common.py

@@ -178,47 +178,38 @@ def notify_pending_approval_via_slack(
     or when Slack is not configured.  Always best-effort — never raises.
     """
     try:
-        from db import get_db
         import requests as _requests
 
-        bootstrap_id = (os.environ.get("WORKEROS_USER_ID") or "").strip() or "local-user"
-        candidate_ids = [owner_id]
-        slack_user_id: Optional[str] = None
-        team_id: Optional[str] = None
-
+        # Resolve the owner -> Slack binding through the overridable helper so
+        # the correct store is used per deployment. OSS single-tenant reads the
+        # local DB; cloud overrides _slack_binding_for_owner (via
+        # startup.apply_cloud_slack_overrides) to read the Supabase-backed
+        # slack_sender_bindings. Import lazily by module attribute so the cloud
+        # monkeypatch is picked up at call time (a raw inline get_db() here
+        # would always hit cloud's empty local sqlite sidecar — #the-bug).
         try:
-            with get_db() as conn:
-                if owner_id == bootstrap_id:
-                    try:
-                        admin_rows = conn.execute(
-                            "SELECT id FROM users WHERE role = 'admin'"
-                        ).fetchall()
-                        candidate_ids += [str(r["id"]) for r in admin_rows]
-                    except Exception:
-                        pass
-                else:
-                    candidate_ids.append(bootstrap_id)
-                seen: set[str] = set()
-                candidate_ids = [c for c in candidate_ids if c and not (c in seen or seen.add(c))]
-                placeholders = ",".join("?" * len(candidate_ids))
-                row = conn.execute(
-                    f"""
-                    SELECT slack_user_id, slack_team_id FROM slack_sender_bindings
-                    WHERE user_id IN ({placeholders}) AND status = 'active'
-                    LIMIT 1
-                    """,
-                    tuple(candidate_ids),
-                ).fetchone()
-            if row:
-                slack_user_id = str(row["slack_user_id"])
-                team_id = str(row["slack_team_id"])
+            from channels.slack import _slack_binding_for_owner
         except Exception:
-            logger.exception(
-                "Slack approval notify: binding lookup failed for owner %s", owner_id
-            )
+            logger.exception("Slack approval notify: could not import binding helper")
             return
 
+        binding = _slack_binding_for_owner(owner_id)
+        if not binding:
+            logger.info(
+                "Slack approval notify: no active Slack binding for owner %s (run %s); "
+                "skipping DM",
+                owner_id,
+                run_id,
+            )
+            return
+        slack_user_id, team_id = binding
         if not slack_user_id or not team_id:
+            logger.info(
+                "Slack approval notify: incomplete Slack binding for owner %s (run %s); "
+                "skipping DM",
+                owner_id,
+                run_id,
+            )
             return
 
         try:
@@ -229,6 +220,13 @@ def notify_pending_approval_via_slack(
 
         bot_token = _slack_bot_token_for_team(team_id)
         if not bot_token:
+            logger.info(
+                "Slack approval notify: no Slack bot token/install for team %s "
+                "(owner %s, run %s); skipping DM",
+                team_id,
+                owner_id,
+                run_id,
+            )
             return
 
         short_id = approval_id[-6:] if len(approval_id) >= 6 else approval_id
@@ -276,6 +274,12 @@ def notify_pending_approval_via_slack(
             )
             dm_channel = (resp.json() or {}).get("channel", {}).get("id") if resp.ok else None
             if not dm_channel:
+                logger.warning(
+                    "Slack approval notify: conversations.open returned no DM channel "
+                    "for slack_user %s (run %s)",
+                    slack_user_id,
+                    run_id,
+                )
                 return
             _requests.post(
                 "https://slack.com/api/chat.postMessage",
@@ -291,6 +295,13 @@ def notify_pending_approval_via_slack(
                 },
                 timeout=10,
             )
+            logger.info(
+                "Slack approval notify: sent Approve/Reject DM to slack_user %s "
+                "(team %s, run %s)",
+                slack_user_id,
+                team_id,
+                run_id,
+            )
         except Exception:
             logger.exception(
                 "Slack approval notify: DM send failed for slack_user %s (run %s)",

apps/api/channels/slack.py

@@ -981,6 +981,62 @@ def _slack_binding_user_id(team_id: str, slack_user_id: str) -> Optional[str]:
     return None
 
 
+def _slack_binding_for_owner(owner_id: str) -> Optional[tuple[str, str]]:
+    """Reverse-lookup a run owner's active Slack sender binding.
+
+    Given a Floom ``user_id`` (the run owner), return the
+    ``(slack_user_id, team_id)`` of their active binding so an approval DM can be
+    delivered, or ``None`` when the owner has no active binding.
+
+    This is the single overridable seam for the owner->Slack reverse lookup used
+    by ``notify_pending_approval_via_slack``. OSS/single-tenant reads the local
+    ``slack_sender_bindings`` table here; in cloud this function is monkeypatched
+    (``startup.apply_cloud_slack_overrides``) to read the Supabase-backed
+    bindings instead — the local sqlite sidecar is always empty in cloud, so the
+    lookup MUST route through this helper, not a raw ``get_db()`` (bug: the
+    approval DM never fired in cloud because the inline query hit empty sqlite).
+
+    Mirrors the bootstrap<->admin uuid aliasing in ``_slack_binding_user_id`` and
+    the WhatsApp notifier: an admin/FLOOM_SECRET-created run is owned by the
+    bootstrap id ('local-user'), while the human's binding may be keyed to their
+    real user uuid (or vice-versa), so both are considered.
+    """
+    from db import get_db
+
+    if not owner_id:
+        return None
+    bootstrap_id = (os.environ.get("WORKEROS_USER_ID") or "").strip() or "local-user"
+    candidate_ids = [owner_id]
+    try:
+        with get_db() as conn:
+            if owner_id == bootstrap_id:
+                try:
+                    admin_rows = conn.execute(
+                        "SELECT id FROM users WHERE role = 'admin'"
+                    ).fetchall()
+                    candidate_ids += [str(r["id"]) for r in admin_rows]
+                except Exception:
+                    pass  # no users table (legacy single-user) — owner_id alone is fine
+            else:
+                candidate_ids.append(bootstrap_id)
+            seen: set[str] = set()
+            candidate_ids = [c for c in candidate_ids if c and not (c in seen or seen.add(c))]
+            placeholders = ",".join("?" * len(candidate_ids))
+            row = conn.execute(
+                f"""
+                SELECT slack_user_id, slack_team_id FROM slack_sender_bindings
+                WHERE user_id IN ({placeholders}) AND status = 'active'
+                LIMIT 1
+                """,
+                tuple(candidate_ids),
+            ).fetchone()
+        if row and row["slack_user_id"] and row["slack_team_id"]:
+            return str(row["slack_user_id"]), str(row["slack_team_id"])
+    except Exception:
+        logger.exception("Slack approval notify: binding lookup failed for owner %s", owner_id)
+    return None
+
+
 def _slack_create_claim(team_id: str, slack_user_id: str, profile_name: str = "") -> Dict[str, str]:
     """Issue a new claim token for an unbound (team_id, slack_user_id) pair.
 

apps/api/tests/test_slack_approvals_1368.py

@@ -163,6 +163,99 @@ def _fake_post(url, *, headers=None, json=None, timeout=None):
     )
 
 
+def test_slack_notify_routes_through_overridable_helper(monkeypatch, tmp_path):
+    """The owner->binding lookup MUST go through channels.slack._slack_binding_for_owner
+    (the cloud-overridable seam), NOT a raw local-DB query.
+
+    Regression for the cloud approval-DM bug: in cloud the local sqlite sidecar
+    is always empty, so the binding lives only in Supabase. When the cloud
+    override replaces _slack_binding_for_owner + _slack_bot_token_for_team, the
+    DM must still fire with the Approve/Reject blocks — even though NOTHING is
+    seeded in the local DB here.
+    """
+    main = _load_api(monkeypatch, tmp_path, with_slack=True)
+    import channels.common as _common_mod
+    import channels.slack as _slack_mod
+
+    SLACK_USER = "U_CLOUD_BIND"
+    TEAM = "T_CLOUD_BIND"
+    OWNER = "cloud-owner-uuid"
+    RUN_ID = "run-cloud-binding-1"
+
+    # Simulate the cloud override: binding resolved from Supabase (not local DB),
+    # bot token resolved from the Supabase-backed installation.
+    monkeypatch.setattr(
+        _slack_mod, "_slack_binding_for_owner",
+        lambda owner_id: (SLACK_USER, TEAM) if owner_id == OWNER else None,
+    )
+    monkeypatch.setattr(
+        _slack_mod, "_slack_bot_token_for_team",
+        lambda team_id: "xoxb-cloud-token" if team_id == TEAM else "",
+    )
+
+    messages_posted: list[dict] = []
+    conversations_opened: list[str] = []
+
+    class _FakeResp:
+        ok = True
+        def json(self):
+            if "conversations.open" in self._url:
+                return {"channel": {"id": "D_CLOUD_DM"}}
+            return {"ok": True}
+
+    def _fake_post(url, *, headers=None, json=None, timeout=None):
+        r = _FakeResp()
+        r._url = url
+        if "conversations.open" in url:
+            conversations_opened.append(str(json or {}))
+        elif "chat.postMessage" in url:
+            messages_posted.append(json or {})
+        return r
+
+    import requests as _requests_mod
+    monkeypatch.setattr(_requests_mod, "post", _fake_post)
+
+    # NOTE: no local DB seeding — proves the raw get_db() path is not used.
+    _common_mod.notify_pending_approval_via_slack(
+        owner_id=OWNER,
+        run_id=RUN_ID,
+        worker_name="Cloud Worker",
+        label="Send a report",
+        approval_id="apr_cloud00112233",
+    )
+
+    assert any(SLACK_USER in s for s in conversations_opened), (
+        f"Expected {SLACK_USER!r} in conversations.open payload; got {conversations_opened!r}"
+    )
+    assert len(messages_posted) == 1, f"Expected 1 chat.postMessage; got {messages_posted!r}"
+    msg = messages_posted[0]
+    assert msg.get("channel") == "D_CLOUD_DM"
+    action_ids = [
+        elem.get("action_id")
+        for block in (msg.get("blocks") or [])
+        for elem in (block.get("elements") or [])
+    ]
+    assert "workeros_approval_approve" in action_ids, "Approve button missing"
+    assert "workeros_approval_reject" in action_ids, "Reject button missing"
+
+
+def test_slack_binding_for_owner_reads_local_binding(monkeypatch, tmp_path):
+    """OSS single-tenant path: _slack_binding_for_owner returns the local binding."""
+    main = _load_api(monkeypatch, tmp_path, with_slack=True)
+    import channels.slack as _slack_mod
+
+    SLACK_USER = "U_LOCAL_BIND"
+    TEAM = "T_LOCAL_BIND"
+    OWNER = "local-bound-owner"
+
+    with main.get_db() as conn:
+        now = main.now_iso()
+        _seed_slack_bound_user(conn, SLACK_USER, TEAM, OWNER, now)
+
+    assert _slack_mod._slack_binding_for_owner(OWNER) == (SLACK_USER, TEAM)
+    assert _slack_mod._slack_binding_for_owner("nobody-here") is None
+
+
 def test_slack_notify_no_binding_is_noop(monkeypatch, tmp_path):
     """notify_pending_approval_via_slack does nothing when the owner has no Slack binding."""
     main = _load_api(monkeypatch, tmp_path, with_slack=True)