diff --git a/apps/api/channels/common.py b/apps/api/channels/common.py index b19a22a3a..8a333b477 100644 --- a/apps/api/channels/common.py +++ b/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)", diff --git a/apps/api/channels/slack.py b/apps/api/channels/slack.py index 42129cf3a..d1c9580c5 100644 --- a/apps/api/channels/slack.py +++ b/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. diff --git a/apps/api/tests/test_slack_approvals_1368.py b/apps/api/tests/test_slack_approvals_1368.py index a23176c76..c8512d2b3 100644 --- a/apps/api/tests/test_slack_approvals_1368.py +++ b/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)