Repository navigation
Conversation
Map typing to processing and clearing to active for the current API, preserving legacy SDK fallback and display text. Keep failures nonfatal and log endpoint plus exception type without exception contents. Replace permissive migration expectations with two parameterized transport invariants. Offline canonical runner: base 12 failed/14 passed; fixed 26 passed; Slack and clarification regression 528 passed across 37 files.
alt-glitch
added
type/bug
|
Independent review passed for exact head c412267. The complete changed test file passed 261 tests. A separate request-boundary probe passed 60 cases across slack-sdk 3.44.1 and 3.43.0, exercising typed SDK methods, endpoint/body construction, synthetic authorization preparation, and response validation with HTTP transport stubbed. Verified processing-to-active transitions, reusable sessions, workspace/thread isolation, final-response clearing, legacy fallback, and nonfatal secret-safe adapter logging. Network tracing confirmed no successful external traffic. Five AsyncMock warnings appeared in the existing test file; no failures. Live Slack UI verification and deployment remain separate. GitHub currently reports no checks for this head. |
Preserve the existing PR head while carrying the verified status lifecycle fix onto current main.
|
Live-verified this before reading the diff, hermes-agent @ 1ad89ac, slack-sdk 3.44.1, slack-bolt 1.30.0, real thread in a workspace with
Diagnosis confirmed, and the enum translation does restore the indicator. Before it merges, though, the cost is worth deciding explicitly: The display text disappears permanently. Translating at the transport boundary collapses every phrase to The legacy text API is not gone yet. It still accepts text today and the deprecation lands Feb 2027, so this is not "enum now or no indicator until 2027". A hybrid keeps both, which is what I run locally:
Worth copying either way: gate the enum write on transitions rather than on every 2s refresh, which closes the stuck-spinner risk in trap 2 of the issue regardless of which route lands. Smaller point: the clear path calls the setter for every The diagnosis here is right and this is a clear improvement over the silent failure. If maintainers want the text preserved I can open the hybrid as a PR; if enum-only is the intended direction then the config keys should be handled in this PR. |
|
Reviewed this against the three traps in #110374. The enum translation and the older-SDK / missing-method fallback are right, and I verified this head locally. Two pieces from the issue are still missing, and one of them is currently asserted against: 1. A rejected Agents call never falls back to the legacy free text. 2. Nothing is remembered, and the rejection stays invisible. A rejected endpoint is retried every turn, and failure stays at DEBUG, which is the original silent-breakage mode. Logging the first rejection at WARNING is what makes it non-silent. Both are local to What it adds
Verification (this exact patch on top of
Happy for you to fold this in (or I can open it as a follow-up PR) — your call. diff --git a/plugins/platforms/slack/adapter.py b/plugins/platforms/slack/adapter.py
index 60708a48d..396a1b866 100644
--- a/plugins/platforms/slack/adapter.py
+++ b/plugins/platforms/slack/adapter.py
@@ -288,6 +288,39 @@ def _session_title_method(client: Any):
return client.assistant_threads_setTitle
+# ``agents.sessions.setStatus`` rejects any status that is not one of its lifecycle
+# enums. These are the ways Slack says the endpoint will not take the call at all.
+# A rejection is a property of the install (workspace/token/SDK), not of the turn, so
+# the adapter degrades to the legacy API for the rest of the process rather than
+# retrying a call that cannot succeed (#110374).
+_AGENT_SESSIONS_REJECTION_ERRORS = (
+ "invalid_arguments",
+ "invalid_status",
+ "unknown_method",
+ "method_not_supported",
+ "not_allowed",
+)
+
+
+def _agent_sessions_rejection(exc: BaseException) -> Optional[str]:
+ """Return the rejection code when ``exc`` means the Agents status endpoint is unusable.
+
+ Reads the Slack error code off a ``SlackApiError`` response when present, and
+ otherwise scans the exception text, so a mapped or wrapped error still degrades.
+ Returns ``None`` for ordinary transport failures, which stay nonfatal and do not
+ change the selected endpoint.
+ """
+ response = getattr(exc, "response", None)
+ error = response.get("error") if isinstance(response, dict) else None
+ if isinstance(error, str) and error in _AGENT_SESSIONS_REJECTION_ERRORS:
+ return error
+ text = str(exc)
+ for code in _AGENT_SESSIONS_REJECTION_ERRORS:
+ if code in text:
+ return code
+ return None
+
+
def slack_deps_present() -> bool:
"""PASSIVE probe: are slack-bolt/slack-sdk importable right now?
Registry ``check_fn`` (status displays, config loading) — must never install. The active
@@ -1084,6 +1117,10 @@ class SlackAdapter(BasePlatformAdapter):
# Set once startStream reports the app lacks streaming (Agents & AI Apps
# off / missing scope); later responses skip straight to edit-based streaming.
self._native_stream_unsupported = False
+ # Set once Slack rejects agents.sessions.setStatus (non-enum status on an install
+ # whose endpoint refuses the call). Later turns then stay on the legacy free-text
+ # assistant.threads.setStatus instead of re-issuing a call that cannot succeed.
+ self._agent_sessions_status_degraded = False
# Socket Mode self-healing state for silently dropped websockets; the monotonic
# start time is the grace window for the first ping/pong.
self._app_token: Optional[str] = None
@@ -2554,23 +2591,44 @@ class SlackAdapter(BasePlatformAdapter):
async def _set_thread_status(
self, chat_id: str, team_id: str, thread_ts: str, status: str, fail_label: str) -> None:
- """Translate legacy display text/clear to the selected API's status contract."""
- api_method = "session status"
+ """Translate legacy display text/clear to the selected API's status contract.
+
+ ``agents.sessions.setStatus`` takes only ``active|processing|suspended|closed``,
+ so a display phrase maps to ``processing`` while working and ``active`` on clear
+ (clearing leaves the reusable session active; prose never implies suspension or
+ closure). If Slack rejects that call the endpoint is unusable on this install:
+ replay the original phrase over the legacy free-text
+ ``assistant.threads.setStatus`` and remember the degradation so later turns skip
+ the Agents call instead of hammering it (#110374).
+ """
+ api_method = "assistant.threads.setStatus"
try:
client = self._get_client(chat_id, team_id=team_id)
- _set_status = (
- getattr(client, "agents_sessions_setStatus", None)
- if _sdk_supports_agent_sessions() else None
- )
- if _set_status is not None:
- api_method = "agents.sessions.setStatus"
- # Agent Sessions accepts lifecycle enums, not display prose. Clearing typing
- # leaves the reusable session active; prose never implies suspension/closure.
- status = "processing" if status else "active"
- else:
- api_method = "assistant.threads.setStatus"
- _set_status = client.assistant_threads_setStatus
- await _set_status(channel_id=chat_id, thread_ts=thread_ts, status=status)
+ if not self._agent_sessions_status_degraded and _sdk_supports_agent_sessions():
+ _set_status = getattr(client, "agents_sessions_setStatus", None)
+ if _set_status is not None:
+ api_method = "agents.sessions.setStatus"
+ try:
+ await _set_status(
+ channel_id=chat_id,
+ thread_ts=thread_ts,
+ status="processing" if status else "active",
+ )
+ return
+ except Exception as e:
+ rejected = _agent_sessions_rejection(e)
+ if rejected is None:
+ raise
+ # The flag is what silences later turns, so WARN only on the first.
+ self._agent_sessions_status_degraded = True
+ logger.warning(
+ "[Slack] %s rejected (%s); falling back to "
+ "assistant.threads.setStatus %s",
+ api_method, rejected, fail_label,
+ )
+ api_method = "assistant.threads.setStatus"
+ await client.assistant_threads_setStatus(
+ channel_id=chat_id, thread_ts=thread_ts, status=status)
except Exception as e:
# SDK exception text/responses can contain credentials or request content.
logger.debug("[Slack] %s %s (%s)", api_method, fail_label, type(e).__name__)
diff --git a/tests/gateway/test_slack.py b/tests/gateway/test_slack.py
index df05f6cf9..602e49e45 100644
--- a/tests/gateway/test_slack.py
+++ b/tests/gateway/test_slack.py
@@ -6063,6 +6063,119 @@ class TestAgentSessionsApiRouting:
and "RuntimeError" in record.getMessage() for record in records)
assert all(record.exc_info is None and record.stack_info is None for record in records)
+ _AGENT_SESSIONS_ENUM = frozenset({"active", "processing", "suspended", "closed"})
+
+ class _Rejected(Exception):
+ """Stands in for SlackApiError on a rejected agents.sessions.setStatus call."""
+
+ def __init__(self, error: str):
+ super().__init__(f"The request to the Slack API failed: {error}")
+ self.response = {"ok": False, "error": error}
+
+ def _agent_transport_adapter(self):
+ """Adapter whose workspace client exposes both transports explicitly.
+
+ Agent Sessions starts healthy (the mock succeeds) so a test can arm it with a
+ rejection and watch the adapter degrade to the legacy free-text endpoint.
+ """
+ a = self._adapter()
+ client = SimpleNamespace(
+ agents_sessions_setStatus=AsyncMock(),
+ assistant_threads_setStatus=AsyncMock(),
+ )
+ a._team_clients = {"T_TARGET": client}
+ a._channel_team["C123"] = "T_TARGET"
+ return a, client.agents_sessions_setStatus, client.assistant_threads_setStatus
+
+ @pytest.mark.parametrize("configured,live,elapsed", [
+ pytest.param(None, None, 0, id="default"),
+ pytest.param("is checking…", None, 0, id="configured"),
+ pytest.param("is checking…", "is reading docs…", 0, id="live"),
+ pytest.param(None, None, 123, id="elapsed"),
+ pytest.param(None, "suspended", 0, id="lifecycle-word-is-prose"),
+ ])
+ @pytest.mark.asyncio
+ async def test_agent_sessions_never_receives_free_form_status(
+ self, monkeypatch, configured, live, elapsed,
+ ):
+ """Every phrase the adapter can generate must be translated to the enum."""
+ monkeypatch.setattr(_slack_mod, "_AGENT_SESSIONS_SUPPORTED", True)
+ a, agents, _legacy = self._agent_transport_adapter()
+ a.config.typing_status_text = configured
+ a.set_status_text("C123", live)
+ clock = [1000.0]
+ monkeypatch.setattr(_slack_mod.time, "monotonic", lambda: clock[0])
+ metadata = {"thread_id": "171.000", "message_id": "171.500", "slack_team_id": "T_TARGET"}
+
+ await a.send_typing("C123", metadata=metadata)
+ clock[0] += elapsed
+ await a.send_typing("C123", metadata=metadata)
+ await a.stop_typing("C123", metadata=metadata)
+
+ sent = {c.kwargs["status"] for c in agents.await_args_list}
+ assert sent and sent <= self._AGENT_SESSIONS_ENUM
+
+ @pytest.mark.parametrize("code", [
+ "invalid_arguments", "invalid_status", "unknown_method",
+ "method_not_supported", "not_allowed",
+ ])
+ @pytest.mark.asyncio
+ async def test_agents_rejection_falls_back_to_legacy_text_and_warns_once(
+ self, monkeypatch, caplog, code,
+ ):
+ """A rejected Agents call replays the phrase on legacy and warns exactly once."""
+ monkeypatch.setattr(_slack_mod, "_AGENT_SESSIONS_SUPPORTED", True)
+ a, agents, legacy = self._agent_transport_adapter()
+ agents.side_effect = self._Rejected(code)
+ caplog.set_level("DEBUG", logger=_slack_mod.__name__)
+ metadata = {"thread_id": "171.000", "slack_team_id": "T_TARGET"}
+
+ await a.send_typing("C123", metadata=metadata)
+
+ legacy.assert_awaited_once_with(
+ channel_id="C123", thread_ts="171.000", status="is thinking...")
+ warnings = [r for r in caplog.records
+ if r.name == _slack_mod.__name__ and r.levelname == "WARNING"]
+ assert len(warnings) == 1
+ assert "agents.sessions.setStatus" in warnings[0].getMessage()
+ assert code in warnings[0].getMessage()
+
+ @pytest.mark.asyncio
+ async def test_agents_rejection_clears_via_legacy_fallback(self, monkeypatch):
+ """The clear path degrades too: stop_typing empties through legacy, not the enum."""
+ monkeypatch.setattr(_slack_mod, "_AGENT_SESSIONS_SUPPORTED", True)
+ a, agents, legacy = self._agent_transport_adapter()
+ metadata = {"thread_id": "171.000", "slack_team_id": "T_TARGET"}
+
+ await a.send_typing("C123", metadata=metadata) # tracked; Agents still healthy
+ agents.side_effect = self._Rejected("invalid_status")
+ legacy.reset_mock()
+
+ await a.stop_typing("C123", metadata=metadata)
+
+ legacy.assert_awaited_once_with(channel_id="C123", thread_ts="171.000", status="")
+
+ @pytest.mark.asyncio
+ async def test_agents_skipped_on_later_turns_after_rejection(self, monkeypatch, caplog):
+ """The degraded decision is remembered, so a live endpoint is not called again."""
+ monkeypatch.setattr(_slack_mod, "_AGENT_SESSIONS_SUPPORTED", True)
+ a, agents, legacy = self._agent_transport_adapter()
+ agents.side_effect = self._Rejected("invalid_arguments")
+ caplog.set_level("DEBUG", logger=_slack_mod.__name__)
+ metadata = {"thread_id": "171.000", "slack_team_id": "T_TARGET"}
+
+ await a.send_typing("C123", metadata=metadata)
+ # Endpoint healthy again: only the remembered degradation can keep it off.
+ agents.side_effect = None
+ agents.reset_mock()
+ await a.send_typing("C123", metadata=metadata)
+
+ agents.assert_not_awaited()
+ assert legacy.await_args_list[-1].kwargs["status"] == "is thinking..."
+ warnings = [r for r in caplog.records
+ if r.name == _slack_mod.__name__ and r.levelname == "WARNING"]
+ assert len(warnings) == 1 # warned on the first rejection, not on every turn
+
@pytest.mark.asyncio
async def test_thread_title_uses_agents_sessions_rename(self):
_slack_mod._AGENT_SESSIONS_SUPPORTED = True |
|
Superseded by #123457, rebuilt on current main. |
4 participants
Summary
Fix the status payload contract introduced by #101477. The SDK methods have compatible signatures but different semantics:
agents.sessions.setStatusaccepts lifecycle enums, not Assistant display text.processingwhile typing andactivewhen clearing typing, keeping the session reusable.No authorization, profile routing, message content, title handling, or service lifecycle changes.
Contract
Slack documents
active,processing,suspended, andclosedas accepted values:https://docs.slack.dev/reference/methods/agents.sessions.setStatus/
Existing code sends display strings such as
is thinking...and an empty string when clearing. This PR translates at the transport boundary. It does not infer suspension or closure from free-form status prose.Validation
Refreshed onto current
mainat9b199246e59276bb8d3b73087ee079a3047ed485while preserving the existing PR head as ancestry.git diff --check: passed.Tests exercise actual adapter send and stop paths with synthetic transports, status variants, exact workspace and thread routing, older SDK fallback, missing-method fallback, session reuse, single-endpoint failure handling, and secret-safe logs. No live Slack status mutation was used as a test. The full repository suite was not run.