diff --git a/plugins/platforms/discord/adapter.py b/plugins/platforms/discord/adapter.py index 5fab2307c30a..3a673da99d76 100644 --- a/plugins/platforms/discord/adapter.py +++ b/plugins/platforms/discord/adapter.py @@ -844,6 +844,19 @@ def _read_dm_role_auth_guild() -> Optional[int]: _DISCORD_PROMPT_TIMEOUT_MIN = 30 _DISCORD_PROMPT_TIMEOUT_MAX = 900 +# Grace window (seconds) granted after a clarify's BUTTONS expire, during +# which a typed reply still resolves the prompt. Keep it strictly shorter +# than ``agent.clarify_timeout`` minus the view timeout, otherwise the +# agent-side wait fires first and the window is dead time. 0 disables the +# window: an expired view releases the agent immediately. +_CLARIFY_TEXT_GRACE_DEFAULT = 300 +_CLARIFY_TEXT_GRACE_MAX = 3600 + +# Strong references to in-flight clarify-expiry tasks. asyncio only keeps a +# weak reference to a running task, so without this the GC can collect one +# mid-sleep and the agent never gets released. +_CLARIFY_EXPIRY_TASKS: set = set() + def _env_bool(name: str, default: bool = False) -> bool: raw = os.getenv(name, "").strip().lower() @@ -883,6 +896,44 @@ def _read_discord_prompt_timeout() -> int: return seconds +def _read_clarify_text_grace() -> int: + """Return the typed-answer grace window (seconds) after buttons expire. + + Reads ``approvals.discord_clarify_text_grace`` from config.yaml, falling + back to ``_CLARIFY_TEXT_GRACE_DEFAULT``. Clamped to + ``[0, _CLARIFY_TEXT_GRACE_MAX]``; 0 means "release the agent as soon as + the view times out". + """ + raw: Any = None + try: + from hermes_cli.config import read_raw_config + cfg = read_raw_config() or {} + approvals_cfg = cfg.get("approvals", {}) or {} + raw = approvals_cfg.get("discord_clarify_text_grace") + except Exception: + return _CLARIFY_TEXT_GRACE_DEFAULT + if raw is None or raw == "": + return _CLARIFY_TEXT_GRACE_DEFAULT + try: + seconds = int(raw) + except (TypeError, ValueError): + return _CLARIFY_TEXT_GRACE_DEFAULT + if seconds < 0: + return 0 + if seconds > _CLARIFY_TEXT_GRACE_MAX: + return _CLARIFY_TEXT_GRACE_MAX + return seconds + + +def _clarify_entry_pending(clarify_id: str) -> bool: + """True while ``clarify_id`` is still waiting for an answer.""" + try: + from tools.clarify_gateway import _entries as _clarify_entries # type: ignore + return _clarify_entries.get(clarify_id) is not None + except Exception: + return False + + class DiscordAdapter(BasePlatformAdapter): """ Discord bot adapter. @@ -9128,21 +9179,101 @@ async def _on_other(self, interaction: "discord.Interaction") -> None: except Exception: pass + async def _edit_expired_embed(self, footer: str, color) -> None: + """Repaint the prompt message with an expiry footer.""" + msg = getattr(self, "_message", None) + if not msg: + return + try: + embed = msg.embeds[0] if msg.embeds else None + if embed: + embed.color = color + embed.set_footer(text=footer) + await msg.edit(embed=embed, view=self) + except Exception: + pass + + async def _resolve_after_grace(self, grace: int) -> None: + """Unblock the agent once the typed-answer grace window closes. + + The view's timeout only kills the BUTTONS. The agent thread is + still parked in ``clarify_gateway.wait_for_response`` until its + own ``agent.clarify_timeout`` fires — an hour by default, and + never when that is set to 0. Without this the session stays + pinned behind a prompt the user can no longer answer by + clicking, and every follow-up message queues up behind a turn + that cannot finish. + """ + try: + if grace > 0: + await asyncio.sleep(grace) + if not _clarify_entry_pending(self.clarify_id): + return # user typed an answer during the grace window + from tools.clarify_gateway import resolve_gateway_clarify + resolved = resolve_gateway_clarify(self.clarify_id, "") + logger.info( + "Discord clarify expired unanswered (id=%s, grace=%ds, ok=%s) — " + "released the agent with an empty response", + self.clarify_id, grace, resolved, + ) + await self._edit_expired_embed( + "⏱ Prompt expired — no action taken", + discord.Color.greyple(), + ) + except asyncio.CancelledError: + raise + except Exception as exc: + logger.warning( + "Discord clarify expiry release failed (id=%s): %s", + self.clarify_id, exc, + ) + async def on_timeout(self): self.resolved = True for child in self.children: child.disabled = True - # Visually update the Discord message so buttons appear disabled. - msg = getattr(self, '_message', None) - if msg: - try: - embed = msg.embeds[0] if msg.embeds else None - if embed: - embed.color = discord.Color.greyple() - embed.set_footer(text="⏱ Prompt expired — no action taken") - await msg.edit(embed=embed, view=self) - except Exception: - pass + + # The entry is already gone when the agent moved on by itself + # (answered elsewhere, run interrupted, session cleared) — then + # this really is a no-op expiry. + if not _clarify_entry_pending(self.clarify_id): + await self._edit_expired_embed( + "⏱ Prompt expired — no action taken", + discord.Color.greyple(), + ) + return + + # Buttons are dead but the clarify is still live agent-side, so + # flip it into text-capture mode: a typed reply now resolves the + # prompt instead of being rejected as "arbitrary prose for a + # multi-choice clarify" and silently queued behind the very turn + # it was meant to unblock. + grace = _read_clarify_text_grace() + flipped = False + try: + from tools.clarify_gateway import mark_awaiting_text + flipped = bool(mark_awaiting_text(self.clarify_id)) + except Exception as exc: + logger.warning( + "Discord clarify mark_awaiting_text on timeout failed (id=%s): %s", + self.clarify_id, exc, + ) + + if flipped and grace > 0: + await self._edit_expired_embed( + "⏱ Buttons expired — reply with a message to answer", + discord.Color.orange(), + ) + else: + await self._edit_expired_embed( + "⏱ Prompt expired — no action taken", + discord.Color.greyple(), + ) + + # Never leave the agent parked behind an unanswerable prompt. + task = asyncio.create_task(self._resolve_after_grace(grace)) + _CLARIFY_EXPIRY_TASKS.add(task) + task.add_done_callback(_CLARIFY_EXPIRY_TASKS.discard) if DISCORD_AVAILABLE: _define_discord_view_classes() diff --git a/tests/gateway/test_discord_clarify_buttons.py b/tests/gateway/test_discord_clarify_buttons.py index 39c77e2cfd97..e46efa53c8ea 100644 --- a/tests/gateway/test_discord_clarify_buttons.py +++ b/tests/gateway/test_discord_clarify_buttons.py @@ -294,3 +294,172 @@ async def test_unwrap_does_not_pick_value_or_name_alone(self): for label in choice_labels: assert "only_name_here" not in label, f"name leaked: {label!r}" assert "only_value_here" not in label, f"value leaked: {label!r}" + + +# =========================================================================== +# ClarifyChoiceView.on_timeout — expiry must release the agent +# =========================================================================== + +def _make_view_message(): + """Mock the prompt message the view repaints on expiry.""" + embed = MagicMock() + embed.color = None + embed.set_footer = MagicMock() + return SimpleNamespace(embeds=[embed], edit=AsyncMock()) + + +def _expiry_tasks(): + """Live expiry tasks. getattr keeps the behavioural assertions below the + real failure point when the release logic is missing entirely.""" + from plugins.platforms.discord import adapter as adapter_mod + return list(getattr(adapter_mod, "_CLARIFY_EXPIRY_TASKS", ())) + + +async def _drain_expiry_tasks(): + """Await whatever ``on_timeout`` scheduled, so assertions see the result.""" + tasks = _expiry_tasks() + for task in tasks: + await task + return tasks + + +def _cancel_expiry_tasks(): + for task in _expiry_tasks(): + task.cancel() + + +class TestClarifyChoiceViewTimeout: + """An expired view must never leave the agent parked on a dead prompt. + + The view timeout only kills the buttons; the agent thread stays inside + ``clarify_gateway.wait_for_response`` until ``agent.clarify_timeout`` + fires (an hour by default, never when set to 0). Until then the prompt + is unanswerable: the buttons are disabled and typed prose is rejected + by the multi-choice coercion, so every follow-up message queues behind + the very turn it was meant to unblock. + """ + + def setup_method(self): + _clear_clarify_state() + _cancel_expiry_tasks() + + def teardown_method(self): + _cancel_expiry_tasks() + + @pytest.mark.asyncio + async def test_timeout_flips_live_entry_to_awaiting_text(self): + from tools import clarify_gateway as cm + cm.register("cidT1", "sk-T1", "Pick", ["x", "y"]) + + view = ClarifyChoiceView( + choices=["x", "y"], clarify_id="cidT1", allowed_user_ids={"42"}, + ) + view._message = _make_view_message() + + await view.on_timeout() + + # Entry survives the button expiry and now accepts free text. + with cm._lock: + entry = cm._entries.get("cidT1") + assert entry is not None + assert entry.awaiting_text is True + assert not entry.event.is_set() + assert all(b.disabled for b in view.children) + footer = view._message.embeds[0].set_footer.call_args.kwargs["text"] + assert "reply with a message" in footer.lower() + + @pytest.mark.asyncio + async def test_typed_prose_answers_the_prompt_after_timeout(self): + """Regression: prose was rejected while the buttons were dead.""" + from tools import clarify_gateway as cm + cm.register("cidT2", "sk-T2", "What first?", ["Fix the bug", "Do the docs"]) + + # Live multi-choice prompt: prose is (correctly) not an answer. + assert cm.resolve_text_response_for_session("sk-T2", "cancel that") is False + + view = ClarifyChoiceView( + choices=["Fix the bug", "Do the docs"], + clarify_id="cidT2", + allowed_user_ids={"42"}, + ) + view._message = _make_view_message() + await view.on_timeout() + + # Buttons are gone, so the same prose must now resolve the clarify. + assert cm.resolve_text_response_for_session("sk-T2", "cancel that") is True + with cm._lock: + entry = cm._entries.get("cidT2") + assert entry.response == "cancel that" + assert entry.event.is_set() + + @pytest.mark.asyncio + async def test_timeout_releases_agent_when_grace_disabled(self, monkeypatch): + from plugins.platforms.discord import adapter as adapter_mod + from tools import clarify_gateway as cm + monkeypatch.setattr(adapter_mod, "_read_clarify_text_grace", lambda: 0) + cm.register("cidT3", "sk-T3", "Pick", ["x", "y"]) + + view = ClarifyChoiceView( + choices=["x", "y"], clarify_id="cidT3", allowed_user_ids={"42"}, + ) + view._message = _make_view_message() + + await view.on_timeout() + await _drain_expiry_tasks() + + # Empty response = "user did not answer"; the waiter unblocks. + with cm._lock: + entry = cm._entries.get("cidT3") + assert entry is not None + assert entry.response == "" + assert entry.event.is_set() + + @pytest.mark.asyncio + async def test_grace_task_leaves_answered_prompt_alone(self, monkeypatch): + """A reply during the grace window wins; the task must not overwrite it.""" + from plugins.platforms.discord import adapter as adapter_mod + from tools import clarify_gateway as cm + monkeypatch.setattr(adapter_mod, "_read_clarify_text_grace", lambda: 0) + cm.register("cidT4", "sk-T4", "Pick", ["x", "y"]) + + view = ClarifyChoiceView( + choices=["x", "y"], clarify_id="cidT4", allowed_user_ids={"42"}, + ) + view._message = _make_view_message() + await view.on_timeout() + + # Simulate wait_for_response returning: the waiter pops its entry. + with cm._lock: + entry = cm._entries.pop("cidT4") + cm._session_index.pop("sk-T4", None) + entry.response = "y" + + await _drain_expiry_tasks() + assert entry.response == "y" + + @pytest.mark.asyncio + async def test_timeout_without_entry_is_a_plain_expiry(self): + view = ClarifyChoiceView( + choices=["x"], clarify_id="cidGoneT", allowed_user_ids={"42"}, + ) + view._message = _make_view_message() + + await view.on_timeout() + + footer = view._message.embeds[0].set_footer.call_args.kwargs["text"] + assert "no action taken" in footer.lower() + assert not _expiry_tasks() + + @pytest.mark.asyncio + async def test_timeout_survives_missing_message_reference(self): + from tools import clarify_gateway as cm + cm.register("cidT5", "sk-T5", "Pick", ["x"]) + + view = ClarifyChoiceView( + choices=["x"], clarify_id="cidT5", allowed_user_ids={"42"}, + ) + # No view._message (send_clarify never stored one). + await view.on_timeout() + + with cm._lock: + assert cm._entries.get("cidT5") is not None diff --git a/tests/gateway/test_discord_prompt_timeout_config.py b/tests/gateway/test_discord_prompt_timeout_config.py index f5ae1c3153a0..981b87e7beaa 100644 --- a/tests/gateway/test_discord_prompt_timeout_config.py +++ b/tests/gateway/test_discord_prompt_timeout_config.py @@ -96,3 +96,93 @@ def test_default_matches_previous_hardcoded_value(): assert _DISCORD_PROMPT_TIMEOUT_DEFAULT == 300 +def test_clamp_range_includes_default(): + """Sanity: the default must lie inside the clamp range, or every fresh + install would hit the clamp on its very first read. + """ + assert _DISCORD_PROMPT_TIMEOUT_MIN <= _DISCORD_PROMPT_TIMEOUT_DEFAULT <= _DISCORD_PROMPT_TIMEOUT_MAX + + +# --------------------------------------------------------------------------- +# approvals.discord_clarify_text_grace +# --------------------------------------------------------------------------- +# Grace window granted after a clarify's buttons expire, during which a typed +# reply still answers the prompt. Same reader shape as the timeout above, with +# one semantic difference: 0 is a meaningful value (release the agent as soon +# as the view expires), so it is NOT clamped up to a minimum. + +from plugins.platforms.discord.adapter import ( # noqa: E402 + _CLARIFY_TEXT_GRACE_DEFAULT, + _CLARIFY_TEXT_GRACE_MAX, + _read_clarify_text_grace, +) + + +def test_grace_default_when_config_absent(monkeypatch): + _patch_config(monkeypatch, {}) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_DEFAULT + + +def test_grace_default_when_key_missing(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_prompt_timeout": 600}}) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_DEFAULT + + +def test_grace_explicit_int_value(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": 120}}) + assert _read_clarify_text_grace() == 120 + + +def test_grace_numeric_string_accepted(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": "90"}}) + assert _read_clarify_text_grace() == 90 + + +def test_grace_malformed_value_falls_back_to_default(monkeypatch): + _patch_config( + monkeypatch, + {"approvals": {"discord_clarify_text_grace": "five minutes"}}, + ) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_DEFAULT + + +def test_grace_zero_is_preserved(monkeypatch): + """0 = release the agent the moment the buttons die. Not a typo guard.""" + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": 0}}) + assert _read_clarify_text_grace() == 0 + + +def test_grace_negative_floors_at_zero(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": -60}}) + assert _read_clarify_text_grace() == 0 + + +def test_grace_clamped_to_maximum(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": 999999}}) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_MAX + + +def test_grace_empty_string_falls_back_to_default(monkeypatch): + _patch_config(monkeypatch, {"approvals": {"discord_clarify_text_grace": ""}}) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_DEFAULT + + +def test_grace_config_read_exception_falls_back_to_default(monkeypatch): + import hermes_cli.config + def _boom(): + raise RuntimeError("config file corrupt") + monkeypatch.setattr(hermes_cli.config, "read_raw_config", _boom) + assert _read_clarify_text_grace() == _CLARIFY_TEXT_GRACE_DEFAULT + + +def test_default_view_timeout_plus_grace_fits_default_clarify_timeout(): + """The release task must not fire after the agent-side wait already gave + up — on stock defaults the whole expiry dance has to fit inside + ``agent.clarify_timeout``. Guards against any of the three defaults + drifting apart again, which is what left sessions pinned for ~55 min. + """ + from tools.clarify_gateway import resolve_clarify_timeout + assert ( + _DISCORD_PROMPT_TIMEOUT_DEFAULT + _CLARIFY_TEXT_GRACE_DEFAULT + <= resolve_clarify_timeout({}) + )