diff --git a/agentex/src/domain/use_cases/slack_gateway_use_case.py b/agentex/src/domain/use_cases/slack_gateway_use_case.py index df18b053..9c3b70b8 100644 --- a/agentex/src/domain/use_cases/slack_gateway_use_case.py +++ b/agentex/src/domain/use_cases/slack_gateway_use_case.py @@ -279,6 +279,35 @@ def _cache_put(key: tuple[str, str, str], config_id: str) -> None: # - enabling an MCP is their own decision and affects nobody else. _USER_CONFIG_NAME = "slack-agentex-bot" +# A 403 from SGP is PERMANENT — the account lacks the access — unlike a timeout or a +# 5xx. It covers both halves: reading the config directory and creating a config in it. Worth telling the person once: they have just been through the +# whole link flow, which promised their turns would run as them, and without a config +# every turn silently falls back to the shared bot. Left unsaid they would never learn +# why their own integrations never work. +# +# Once per day per user, not once ever: role grants change, and a stale "ask an admin" +# is better repeated occasionally than never retracted. +_SGP_FORBIDDEN_NOTICE_COOLDOWN_S = 86400 +_SGP_FORBIDDEN_MESSAGE = ( + "I'll answer as the shared bot for now — your SGP account doesn't have access to " + "agent configs, which is where your own integrations get connected. Ask an SGP " + "admin for *editor* access on this account, then mention me again." +) + + +class _SgpAccessForbidden(Exception): + """SGP refused this account (HTTP 403) — reading configs or creating one. + + Distinct from returning None, which means "could not resolve one right now" and is + handled by degrading quietly. A 403 is an authorization decision: retrying changes + nothing, so it warrants telling the person, once. + + Raised for BOTH halves deliberately. An account can be unable to create a config, + or unable to list them at all, and the consequence is identical — no config of + their own, so every turn silently falls back to the shared bot. Treating only the + create half as permanent left the read half looking like a transient blip forever. + """ + # Seed for a freshly created user config. Hardcoded rather than copied from a canonical # config on purpose: copying would need a service-account read of a config the new user @@ -901,7 +930,12 @@ async def _run_turn( # which fails turn-1 resolution, drops the event, and shows up in Slack as # a hang with nothing logged in the channel. if sgp_user_id: - config_id = await self._own_config_id(auth_headers, sgp_user_id) + try: + config_id = await self._own_config_id(auth_headers, sgp_user_id) + except _SgpAccessForbidden: + # Permanent, so say it once instead of degrading silently forever. + config_id = None + await self._notify_config_forbidden(inbound) if not config_id: # Couldn't find or create theirs. Staying as them and pointing at # any other config fails identically, so drop to the bot: they lose @@ -1267,7 +1301,15 @@ async def _resolve_config_id( cached = _cache_get(cache_key) if cached: return cached - items = await self._list_configs(auth_headers, name) + try: + items = await self._list_configs(auth_headers, name) + except _SgpAccessForbidden: + # Selector resolution is best-effort and its caller falls back to the + # default config. The user-facing notice belongs to _own_config_id, the + # path that actually needs a config of their own; raising here would error + # a turn that can still be answered. + logger.warning("[slack] config directory forbidden; using the default") + return None if items is None: return None match = _canonical_named(items, name) @@ -1299,12 +1341,23 @@ async def _list_configs( headers=auth_headers, params={"name": name}, ) + if resp.status_code == 403: + raise _SgpAccessForbidden("list") + if resp.status_code == 401: + # The credential reached SGP and was rejected. Logged apart from the + # 4xx/5xx bucket because the remedy is a re-link, not a role grant, + # and the re-link prompt is driven elsewhere — this only needs to be + # diagnosable, not messaged twice. + logger.warning("[slack] agent_config lookup unauthorized (401)") + return None if resp.status_code >= 400: logger.warning( "[slack] agent_config lookup failed: status=%s", resp.status_code ) return None return list((resp.json() or {}).get("items") or []) + except _SgpAccessForbidden: + raise except Exception: # noqa: BLE001 - unknown, not empty; caller must not create logger.warning("[slack] agent_config lookup errored", exc_info=True) return None @@ -1365,6 +1418,10 @@ async def _own_config_id( headers={**auth_headers, "content-type": "application/json"}, json=_USER_CONFIG_TEMPLATE, ) + if resp.status_code == 403: + # Permanent: the account lacks the role. Surfaced to the caller so it + # can say so once, rather than degrading silently on every turn. + raise _SgpAccessForbidden(_USER_CONFIG_NAME) if resp.status_code >= 400: logger.warning( "[slack] creating %r failed: status=%s", @@ -1373,6 +1430,8 @@ async def _own_config_id( ) return None config_id = (resp.json() or {}).get("id") + except _SgpAccessForbidden: + raise except Exception: # noqa: BLE001 - degrade to a bot-run turn, never a silent drop logger.warning( "[slack] creating %r errored", _USER_CONFIG_NAME, exc_info=True @@ -1385,7 +1444,12 @@ async def _own_config_id( # is process-local, and a list issued right after a create may not show the # other worker's yet. Re-resolving makes both converge on the same id instead # of each caching its own creation and serving a different config per turn. - confirmed = await self._list_configs(auth_headers, _USER_CONFIG_NAME) + try: + confirmed = await self._list_configs(auth_headers, _USER_CONFIG_NAME) + except _SgpAccessForbidden: + # Creating succeeded but re-reading is refused: nothing to converge on, so + # keep what we made rather than discarding a working config. + confirmed = None canonical = _canonical_named(confirmed or [], _USER_CONFIG_NAME) or config_id if canonical != config_id: logger.warning( @@ -1976,12 +2040,78 @@ async def _claim_offer_cooldown(self, inbound: InboundSlack) -> bool: logger.warning("[slack] link offer cooldown check failed", exc_info=True) return True - async def _post_ephemeral(self, inbound: InboundSlack, text: str) -> None: - """Post a message only the invoking user sees. Best-effort. + async def _notify_config_forbidden(self, inbound: InboundSlack) -> None: + """Tell this user, at most once a day, that their account cannot be set up. + + Rate-limited for the same reason the link offer is: a busy channel would + otherwise repeat it on every mention. Ephemeral because it concerns one + person's account and nobody else in the channel can act on it. + """ + key = f"slack:config_forbidden:{inbound.team_id}:{inbound.user}" + try: + pool = GlobalDependencies().redis_pool + if pool is not None: + import redis.asyncio as redis + + client = redis.Redis(connection_pool=pool) + claimed = await client.set( + key, "1", nx=True, ex=_SGP_FORBIDDEN_NOTICE_COOLDOWN_S + ) + if not claimed: + return + except Exception: # noqa: BLE001 - fail open: saying it twice beats never + logger.warning("[slack] forbidden-notice cooldown failed", exc_info=True) + logger.warning( + "[slack] %s has no SGP config access (403); answering as the bot", + inbound.user, + ) + if await self._post_ephemeral(inbound, _SGP_FORBIDDEN_MESSAGE): + return + # The ephemeral was rejected. Slack refuses them outside a channel context — + # an assistant pane, i.e. a DM with this app — and that rejection is permanent + # for that conversation, so retrying it forever would deliver nothing. Post + # into the thread instead: in a pane the audience is identical, and in a + # channel a visible message beats silence for something the person has to act + # on. _deliver has no success signal, so this is the last attempt. + logger.info("[slack] ephemeral refused; delivering the notice in-thread") + try: + await self._deliver(inbound, _SGP_FORBIDDEN_MESSAGE) + return + except Exception: # noqa: BLE001 - fall through to releasing the cooldown + logger.warning("[slack] in-thread notice failed too", exc_info=True) + # Nothing was delivered, so drop the cooldown and let the next turn retry + # rather than staying quiet for the whole window having told them nothing. + await self._release_notice_cooldown(key) + + async def _release_notice_cooldown(self, key: str) -> None: + """Drop a claimed cooldown so the next turn may try again. + + Claiming before delivering is deliberate — it stops two concurrent turns both + posting — but it means a failed delivery has to give the claim back, or the + window passes with the user never told. + """ + try: + pool = GlobalDependencies().redis_pool + if pool is None: + return + import redis.asyncio as redis + + await redis.Redis(connection_pool=pool).delete(key) + except Exception: # noqa: BLE001 - best-effort; worst case is one quiet window + logger.warning("[slack] releasing notice cooldown failed", exc_info=True) + + async def _post_ephemeral(self, inbound: InboundSlack, text: str) -> bool: + """Post a message only the invoking user sees. Best-effort; True if it landed. Ephemeral so a channel isn't cluttered with onboarding nudges aimed at one person — and Slack rejects it outside a channel context (e.g. an assistant pane), which we swallow. + + The return value exists for callers that pair this with a rate limit. Slack + rejecting an ephemeral is not rare — the assistant-pane case is a *permanent* + rejection for that conversation — so a caller that records "already told them" + before knowing whether they were told will suppress the message for the whole + window and deliver nothing. """ body = await self._slack_api( "chat.postEphemeral", @@ -1994,6 +2124,8 @@ async def _post_ephemeral(self, inbound: InboundSlack, text: str) -> None: ) if not body.get("ok"): logger.info("[slack] postEphemeral -> %s", body.get("error")) + return False + return True async def _set_status(self, inbound: InboundSlack, status: str) -> None: """AI-app 'thinking…' indicator (assistant.threads.setStatus). Shows in the diff --git a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py index a84c6a44..913bebdc 100644 --- a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py +++ b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py @@ -2456,18 +2456,64 @@ async def test_two_users_do_not_share_a_cached_config_id(monkeypatch): @pytest.mark.asyncio -async def test_user_config_returns_none_when_creation_fails(monkeypatch): - """Caller must be able to tell, so it can drop to the bot rather than point a - linked user at a config they cannot read (which would be a silent drop).""" +async def test_forbidden_creation_raises_rather_than_degrading_quietly(monkeypatch): + """403 is permanent — the account lacks the role — so it must be distinguishable + from a transient failure. Silently degrading forever would leave someone who just + completed the link flow wondering why their integrations never work.""" monkeypatch.setattr(sg, "_SGP_BASE_URL", "https://sgp.example") monkeypatch.setattr(sg, "_CONFIG_ID_CACHE", {}) fake = _FakeSGP(existing=[], create_status=403) monkeypatch.setattr(sg.httpx, "AsyncClient", fake.client()) + with pytest.raises(sg._SgpAccessForbidden): + await SlackGatewayUseCase()._own_config_id(_user_headers("carol"), "u-carol") + + +@pytest.mark.asyncio +async def test_transient_creation_failure_stays_quiet(monkeypatch): + """The other half: a 5xx might succeed next turn, so it degrades without telling + anyone. Only the permanent case is worth a message.""" + monkeypatch.setattr(sg, "_SGP_BASE_URL", "https://sgp.example") + monkeypatch.setattr(sg, "_CONFIG_ID_CACHE", {}) + fake = _FakeSGP(existing=[], create_status=503) + monkeypatch.setattr(sg.httpx, "AsyncClient", fake.client()) + cid = await SlackGatewayUseCase()._own_config_id(_user_headers("carol"), "u-carol") assert cid is None +@pytest.mark.asyncio +async def test_forbidden_notice_is_rate_limited(monkeypatch): + """A busy channel would otherwise repeat it on every mention.""" + uc = SlackGatewayUseCase() + posted = [] + monkeypatch.setattr( + uc, "_post_ephemeral", AsyncMock(side_effect=lambda i, t: posted.append(t)) + ) + + claims = iter([True, False]) # SET NX wins once, then loses + + class _Redis: + async def set(self, *a, **k): + return next(claims) + + import redis.asyncio as redis_asyncio + + monkeypatch.setattr( + sg, "GlobalDependencies", lambda: SimpleNamespace(redis_pool=object()) + ) + monkeypatch.setattr(redis_asyncio, "Redis", lambda **k: _Redis()) + inbound = sg.InboundSlack( + team_id="T", channel="C1", user="U1", text="hi", thread_ts="1.0", selector=None + ) + + await uc._notify_config_forbidden(inbound) + await uc._notify_config_forbidden(inbound) + + assert len(posted) == 1, "second call must be suppressed by the cooldown" + assert "editor" in posted[0], "should name what to ask for" + + @pytest.mark.asyncio async def test_resolve_target_prefers_the_users_own_config(monkeypatch): inbound = sg.InboundSlack( @@ -2856,3 +2902,165 @@ async def test_cache_expires_so_a_divergence_can_heal(monkeypatch): ) assert sg._cache_get(key) is None, "expired entries must not be served" assert key not in sg._CONFIG_ID_CACHE, "and should be evicted" + + +# --- no SGP access at all -------------------------------------------------- + + +class _ForbiddenSGP(_FakeSGP): + """SGP refuses this account on the READ path, not just on create.""" + + def __init__(self, status=403): + super().__init__() + self.status = status + + def client(self): + outer = self + + class _Client: + def __init__(self, **kw): + pass + + async def __aenter__(self): + return self + + async def __aexit__(self, *a): + return False + + async def get(self, url, headers=None, params=None): + class _R: + status_code = outer.status + + def json(self): + return {} + + return _R() + + async def post(self, url, headers=None, json=None): + outer.posts.append({"url": url}) + + class _R: + status_code = 200 + + def json(self): + return {"id": "nope"} + + return _R() + + return _Client + + +@pytest.mark.asyncio +async def test_forbidden_listing_is_also_permanent(monkeypatch): + """An account can be unable to LIST configs, not just unable to create one. The + consequence is identical — no config of their own — so it must be reported the + same way rather than looking like a transient blip forever.""" + monkeypatch.setattr(sg, "_SGP_BASE_URL", "https://sgp.example") + monkeypatch.setattr(sg, "_CONFIG_ID_CACHE", {}) + fake = _ForbiddenSGP(403) + monkeypatch.setattr(sg.httpx, "AsyncClient", fake.client()) + + with pytest.raises(sg._SgpAccessForbidden): + await SlackGatewayUseCase()._own_config_id(_user_headers("hank"), "u-hank") + assert fake.posts == [], "must not try to create when reading is refused" + + +@pytest.mark.asyncio +async def test_unauthorized_listing_stays_quiet(monkeypatch): + """401 means the credential was rejected, which a re-link fixes — a different + remedy from a role grant, and the re-link prompt is driven elsewhere. Degrade + without a second message.""" + monkeypatch.setattr(sg, "_SGP_BASE_URL", "https://sgp.example") + monkeypatch.setattr(sg, "_CONFIG_ID_CACHE", {}) + monkeypatch.setattr(sg.httpx, "AsyncClient", _ForbiddenSGP(401).client()) + + cid = await SlackGatewayUseCase()._own_config_id(_user_headers("iris"), "u-iris") + assert cid is None + + +@pytest.mark.asyncio +async def test_selector_resolution_survives_a_forbidden_directory(monkeypatch): + """The selector path is best-effort and its caller falls back to the default + config. Raising there would error a turn that can still be answered.""" + monkeypatch.setattr(sg, "_SGP_BASE_URL", "https://sgp.example") + monkeypatch.setattr(sg, "_CONFIG_ID_CACHE", {}) + monkeypatch.setattr(sg.httpx, "AsyncClient", _ForbiddenSGP(403).client()) + + got = await SlackGatewayUseCase()._resolve_config_id( + "some-config", _user_headers("jane"), sgp_user_id="u-jane" + ) + assert got is None # falls back, does not raise + + +# --- the notice must actually be delivered before it counts ---------------- + + +def _redis_stub(monkeypatch, claims, deleted): + """Redis whose SET NX returns each value in `claims`, recording DELETEs.""" + + class _Redis: + async def set(self, *a, **k): + return next(claims) + + async def delete(self, key): + deleted.append(key) + return 1 + + import redis.asyncio as redis_asyncio + + monkeypatch.setattr( + sg, "GlobalDependencies", lambda: SimpleNamespace(redis_pool=object()) + ) + monkeypatch.setattr(redis_asyncio, "Redis", lambda **k: _Redis()) + + +@pytest.mark.asyncio +async def test_refused_ephemeral_falls_back_to_the_thread(monkeypatch): + """Slack refuses ephemerals outside a channel context (an assistant pane), and + that refusal is permanent for the conversation. Retrying would deliver nothing, so + post in-thread — in a pane the audience is identical anyway.""" + uc = SlackGatewayUseCase() + deleted: list[str] = [] + _redis_stub(monkeypatch, iter([True]), deleted) + monkeypatch.setattr(uc, "_post_ephemeral", AsyncMock(return_value=False)) + delivered: list[str] = [] + monkeypatch.setattr( + uc, "_deliver", AsyncMock(side_effect=lambda i, t: delivered.append(t)) + ) + + await uc._notify_config_forbidden(_inbound(team_id="T", user="U1")) + + assert len(delivered) == 1, "must not stay silent when the ephemeral is refused" + assert deleted == [], "delivery succeeded, so the cooldown stands" + + +@pytest.mark.asyncio +async def test_cooldown_is_released_when_nothing_could_be_delivered(monkeypatch): + """The bug this guards: the cooldown was committed before delivery was attempted, + so a rejected ephemeral suppressed the notice for the whole window and the user + was never told why they were on the shared bot.""" + uc = SlackGatewayUseCase() + deleted: list[str] = [] + _redis_stub(monkeypatch, iter([True]), deleted) + monkeypatch.setattr(uc, "_post_ephemeral", AsyncMock(return_value=False)) + monkeypatch.setattr(uc, "_deliver", AsyncMock(side_effect=RuntimeError("nope"))) + + await uc._notify_config_forbidden(_inbound(team_id="T", user="U1")) + + assert deleted == [ + "slack:config_forbidden:T:U1" + ], "nothing was delivered, so the next turn must be allowed to retry" + + +@pytest.mark.asyncio +async def test_successful_ephemeral_keeps_the_cooldown(monkeypatch): + uc = SlackGatewayUseCase() + deleted: list[str] = [] + _redis_stub(monkeypatch, iter([True]), deleted) + monkeypatch.setattr(uc, "_post_ephemeral", AsyncMock(return_value=True)) + monkeypatch.setattr(uc, "_deliver", AsyncMock()) + + await uc._notify_config_forbidden(_inbound(team_id="T", user="U1")) + + uc._deliver.assert_not_awaited() # no need for the fallback + assert deleted == []