From c5c762fb12485202d674222cc20584aa197214db Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Wed, 2 Sep 2026 16:23:26 -0700 Subject: [PATCH] fix(agentex): tell a user when SGP won't let their account hold a Slack config SGP can refuse an account two ways, and both leave the person with no config of their own: unable to CREATE one (POST /v5/agent_configs -> 403 "action=create, legacy_roles=['admin','manager','editor']"), or unable to LIST the directory at all. The consequence is identical -- every turn silently falls back to the shared bot -- so both are now treated the same. Before this, such a user kept getting bot-run turns forever with nothing said, having just completed the link flow, which told them their turns would run as them. They had no way to find out why their own integrations never work. Note this is NOT the same permission as running the agent, and lacking it does not imply lacking access. Agentex authorization (agent.execute) is checked separately and earlier: the users whose turns hung had tasks created and dispatched, so they could run golden-agent fine and only the config read failed. A viewer-role account is exactly the case with execute but not create. A 403 is an authorization decision, so retrying changes nothing. _list_configs and the create path both raise _SgpAccessForbidden for it, and _run_turn posts a one-per-day ephemeral naming the role to ask for before degrading to the bot as before. Everything else still degrades quietly, because the remedies differ: - 401 means the credential reached SGP and was rejected, which a re-link fixes, not a role grant -- and the re-link prompt is driven elsewhere, so this only needs to be diagnosable rather than messaged twice. Logged apart from the 4xx/5xx bucket. - other 4xx/5xx and transport errors may work on the next turn and are not worth telling anyone about. The raise is contained at the two call sites where propagating would be wrong: - selector resolution is best-effort and its caller falls back to the default config, so raising there would error a turn that can still be answered; - the post-create confirmation already has a working config, so a refused re-read keeps what was made rather than discarding it. Both broad `except Exception` handlers re-raise the signal explicitly, since either would otherwise swallow it and restore the silent behaviour. The cooldown is only kept if the notice was actually DELIVERED. Claiming it first is deliberate -- it stops two concurrent turns both posting -- but committing it before attempting delivery meant a rejected ephemeral suppressed the message for the whole window while the user was told nothing, which is the failure this is supposed to prevent. Slack refusing an ephemeral is not rare: it rejects them outside a channel context, i.e. an assistant pane (a DM with this app), and that rejection is PERMANENT for the conversation, so simply retrying would deliver nothing forever. So the notice falls back to an in-thread message -- in a pane the audience is identical, and in a channel a visible message beats silence for something the person has to act on. Only if that fails too is the cooldown released, letting the next turn try again. _post_ephemeral now returns whether it landed. Existing callers ignore it, so the change is additive. Once a day rather than once ever because role grants change, and a stale "ask an admin" is better repeated occasionally than never retracted. Same SET NX cooldown shape as the link offer, failing open for the same reason: saying it twice beats never saying it. Adds 9 tests: 403 on create raises, 403 on list raises (and does not then attempt a create), 503 stays quiet, 401 stays quiet, the selector path survives a forbidden directory, and the notice is suppressed on a second call. Verified non-vacuous by removing each guard in turn -- the raise tests fail with DID NOT RAISE, and the selector test fails by propagating _SgpAccessForbidden out of a turn that could have been answered. 154 passed. ruff check + format clean. Co-authored-by: Claude Opus 5 (1M context) --- .../use_cases/slack_gateway_use_case.py | 142 +++++++++++- .../use_cases/test_slack_gateway_use_case.py | 214 +++++++++++++++++- 2 files changed, 348 insertions(+), 8 deletions(-) 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 == []