diff --git a/coworker/connectors/adapters.py b/coworker/connectors/adapters.py index 3d64d9528..233856b5e 100644 --- a/coworker/connectors/adapters.py +++ b/coworker/connectors/adapters.py @@ -137,6 +137,7 @@ async def send( class SlackAdapter(BasePlatformAdapter): platform = "slack" + supports_interactive = True # Block Kit buttons, resolved via the interactions handler # Watchdog cadence: how often to check the live Socket Mode connection and force a reconnect # if it has silently died. `start_async()` sleeps forever, so a dead socket looks alive to us diff --git a/coworker/connectors/base.py b/coworker/connectors/base.py index e269c09e9..9e24619e6 100644 --- a/coworker/connectors/base.py +++ b/coworker/connectors/base.py @@ -143,6 +143,10 @@ class BasePlatformAdapter(ABC): `handle_message` for inbound events.""" platform: str = "base" + # Whether this platform renders choice buttons. False here is the honest + # default: `send_interactive` below falls back to plain text, and a caller + # that assumes buttons would leave the reader with no way to answer. + supports_interactive: bool = False def __init__(self) -> None: self._handler: Optional[MessageHandler] = None diff --git a/coworker/connectors/gateway.py b/coworker/connectors/gateway.py index 226966ee7..caede1e6a 100644 --- a/coworker/connectors/gateway.py +++ b/coworker/connectors/gateway.py @@ -197,6 +197,15 @@ async def deliver(self, target: str, text: str) -> SendResult: return SendResult(False, error=f"no adapter for {platform}") return await adapter.send(chat_id, text, thread_id=thread_id) + def supports_interactive(self, target: str) -> bool: + """Whether this target's platform actually renders buttons. Callers that offer a + choice need to know: `deliver_interactive` silently degrades to plain text on an + adapter without button support, which would strand the reader with a question and + no way to answer it.""" + platform, _chat_id, _thread_id = parse_target(target) + adapter = self._adapters.get(platform) + return bool(getattr(adapter, "supports_interactive", False)) + async def deliver_interactive(self, target: str, text: str, buttons) -> SendResult: """Send a prompt with choice buttons (adapters without interactive support show text only).""" platform, chat_id, thread_id = parse_target(target) diff --git a/coworker/inbox_routing.py b/coworker/inbox_routing.py index bcbf33f8a..74edd7217 100644 --- a/coworker/inbox_routing.py +++ b/coworker/inbox_routing.py @@ -125,6 +125,10 @@ def deliver(item, binding: InboxBinding, sender: Optional[Sender]) -> bool: # "No." / "👍" working; everything else is a free-text answer, which the approval path # already maps to deny — the safe default for an approval gate. _ALLOW_WORDS = frozenset({"approve", "approved", "allow", "allowed", "yes"}) +# "Always" is the answer that makes a routine stop asking. It existed only as a +# button in an app: over a chat you could approve a scheduled run's prompt but never +# stop it recurring, so the same question arrived every morning. +_ALWAYS_WORDS = frozenset({"always", "alwaysallow"}) _DENY_WORDS = frozenset({"deny", "denied", "reject", "rejected", "no"}) _ALLOW_EMOJI = ("👍", "✅") _DENY_EMOJI = ("👎", "❌") @@ -139,6 +143,8 @@ def _reply_intent(text: str) -> Optional[str]: if first.startswith(_DENY_EMOJI): return "deny" word = first.strip(_TOKEN_TRIM).lower() + if word in _ALWAYS_WORDS: + return "always_task" if word in _ALLOW_WORDS: return "allow" if word in _DENY_WORDS: @@ -151,8 +157,8 @@ def resolve_from_reply( ) -> Optional[bool]: """Correlate an inbound channel reply to its item (by the embedded id) and resolve it. - Looks for the ``[ow:]`` token (or legacy ``[ocw:…]``) and an allow/deny intent in the - reply's leading word; falls back to treating the whole message as a free-text answer. + Looks for the ``[ow:]`` token (or legacy ``[ocw:…]``) and an allow/always/deny intent + in the reply's leading word; falls back to treating the whole message as a free-text answer. ``resolve(item_id, resolution)`` is the InboxStore.resolve. Returns the resolve() result, or None if no item id was found.""" m = _ID_TOKEN.search(reply or "") diff --git a/coworker/server/manager.py b/coworker/server/manager.py index cf9b740fa..c67f2efe9 100644 --- a/coworker/server/manager.py +++ b/coworker/server/manager.py @@ -27,7 +27,7 @@ SessionConnectionStore, effective as effective_connections, ) -from ..inbox import InboxStore, args_preview +from ..inbox import KIND_APPROVAL, InboxStore, args_preview from ..inbox_routing import InboxRouting from ..personas import PersonaRegistry from ..personas.registry import set_registry as set_persona_registry @@ -171,6 +171,25 @@ def _grant_offered(outcome, request) -> bool: return True +def _reply_hint(item, buttons) -> str: + """How to answer this item in a chat that can't draw buttons. Naming the words the + reply parser accepts is the difference between a prompt and a dead end.""" + if not buttons: + return "(Open the app to respond.)" + if getattr(item, "kind", "") == KIND_APPROVAL: + # `always` earns the owning routine a standing grant, so the same question + # stops arriving on every run. Offered only where there is a routine to grant + # it on — in a plain session it resolves as a one-off and would read as a lie. + if (getattr(item, "data", None) or {}).get("task_id"): + return ( + "Reply `approve`, `always` (stop asking for this routine), or `deny` " + "— keep the tag below." + ) + return "Reply `approve` or `deny`, keeping the tag below." + labels = ", ".join(b.label for b in buttons) + return f"Reply with one of: {labels} — keeping the tag below." + + def _approval_body(request) -> str: """Approval card body: the tool's reason (if any) plus a compact preview of its args, so a mirrored 'Run `write_file`?' shows the path/content rather than just the tool name. @@ -4492,8 +4511,12 @@ def _build_task_engine(self, task, *, session_id: str) -> TurnEngine: # -- mirroring inbox items to a bound channel ------------------------------- async def mirror_inbox_item(self, item) -> None: """Mirror an Inbox item to its bound channel. Discrete choices (approve/deny, ask_user - options) render as BUTTONS — the item id rides in each, so a click resolves it - unambiguously. Free-text answers aren't offered over messaging (open the app). + options) render as BUTTONS where the platform has them — the item id rides in each, so a + click resolves it unambiguously. Where it doesn't (any adapter that hasn't implemented + `send_interactive`, Telegram included), the same choice goes out as text carrying the + `[ow:id]` tag and a line saying how to answer, because `_resolve_inbox_reply` needs that + tag to correlate a reply. Without it the reader is shown a question they cannot answer + from the surface it arrived on, and an agent suspended on that prompt waits forever. """ from ..interactions import buttons_for @@ -4510,12 +4533,12 @@ async def mirror_inbox_item(self, item) -> None: body = "\n".join(p for p in (item.title, item.body) if p).strip() buttons = buttons_for(item) try: - if buttons: + if buttons and self.gateway.supports_interactive(target): await self.gateway.deliver_interactive(target, body, buttons) else: await self.gateway.deliver( target, - f"{body}\n(Open the app to respond.)\n[ow:{item.id}]".strip(), + f"{body}\n{_reply_hint(item, buttons)}\n[ow:{item.id}]".strip(), ) except Exception: pass @@ -4580,6 +4603,10 @@ def _resolve(item_id: str, resolution: str) -> bool: item = self.inbox.get(item_id) if item is None: return False + if resolution == "always_task" and item.kind != KIND_APPROVAL: + # "always" is meaningless as an answer to a question — take it as a + # plain yes rather than storing it as the answer text. + resolution = "allow" if ( getattr(event.source, "platform", "") == "slack" and item.kind in {"approval", "directory", "plan"} diff --git a/tests/test_inbox_mirror_answerable.py b/tests/test_inbox_mirror_answerable.py new file mode 100644 index 000000000..38f2b046e --- /dev/null +++ b/tests/test_inbox_mirror_answerable.py @@ -0,0 +1,116 @@ +"""A mirrored prompt must be answerable from the surface it arrived on. + +`buttons_for` returns buttons for approvals and option questions, but only an adapter +that implements `send_interactive` can draw them — the base class quietly sends plain +text instead. Mirroring on the presence of buttons alone therefore produced, on every +adapter but Slack, a question with no buttons, no `[ow:id]` tag for the reply parser to +correlate against, and no instructions: an agent suspended on that prompt waits forever. +""" + +from __future__ import annotations + +import asyncio + +from coworker.inbox import KIND_APPROVAL +from coworker.providers import ModelCapabilities, ProviderClient +from coworker.server.manager import SessionManager + + +class NoTurnsProvider(ProviderClient): + def complete(self, *, model, messages, tools=None, **settings): + raise AssertionError("no model turns expected") + + def capabilities(self, model): + return ModelCapabilities() + + +class GatewayStub: + """Stands in for a platform pair: one that draws buttons, one that cannot.""" + + def __init__(self, interactive: bool) -> None: + self._interactive = interactive + self.texts: list[str] = [] + self.interactive_sends: list[tuple] = [] + + def supports_interactive(self, target: str) -> bool: + return self._interactive + + async def deliver(self, target, text): + self.texts.append(text) + + async def deliver_interactive(self, target, text, buttons): + self.interactive_sends.append((target, text, buttons)) + + +def _manager(tmp_path) -> SessionManager: + manager = SessionManager(data_dir=tmp_path / "data", provider=NoTurnsProvider()) + manager.inbox_routing.set_binding("default", channel="telegram", target="12345") + return manager + + +def test_approval_without_buttons_carries_tag_and_instructions(tmp_path): + manager = _manager(tmp_path) + manager.gateway = GatewayStub(interactive=False) + item = manager.inbox.add_approval("s1", "Run `web_fetch`?", body="url: https://example.com") + + asyncio.run(manager.mirror_inbox_item(item)) + + assert manager.gateway.interactive_sends == [] + (text,) = manager.gateway.texts + assert f"[ow:{item.id}]" in text, "the reply parser correlates on this tag" + assert "approve" in text.lower() and "deny" in text.lower() + assert item.kind == KIND_APPROVAL + + +def test_option_question_without_buttons_names_the_options(tmp_path): + manager = _manager(tmp_path) + manager.gateway = GatewayStub(interactive=False) + item = manager.inbox.add_question("s1", "Which one?", options=["Blue", "Green"]) + + asyncio.run(manager.mirror_inbox_item(item)) + + (text,) = manager.gateway.texts + assert f"[ow:{item.id}]" in text + assert "Blue" in text and "Green" in text + + +def test_routine_card_offers_always(tmp_path): + """A scheduled run's card is the one place "always" does something: it earns the + routine a standing grant, so the same question stops arriving every morning.""" + manager = _manager(tmp_path) + manager.gateway = GatewayStub(interactive=False) + item = manager.inbox.add_approval( + "__run__r1", + "Run `web_search`?", + body="query: anything", + data={"task_id": "task-1", "task_title": "Daily watch"}, + ) + + asyncio.run(manager.mirror_inbox_item(item)) + + (text,) = manager.gateway.texts + assert "always" in text + assert f"[ow:{item.id}]" in text + + +def test_plain_session_card_does_not_offer_always(tmp_path): + """No routine to grant it on — offering it would resolve as a one-off and lie.""" + manager = _manager(tmp_path) + manager.gateway = GatewayStub(interactive=False) + item = manager.inbox.add_approval("s1", "Run `web_fetch`?", body="url: https://x") + + asyncio.run(manager.mirror_inbox_item(item)) + + (text,) = manager.gateway.texts + assert "always" not in text.lower() + + +def test_buttons_still_used_where_the_platform_draws_them(tmp_path): + manager = _manager(tmp_path) + manager.gateway = GatewayStub(interactive=True) + item = manager.inbox.add_approval("s1", "Run it?") + + asyncio.run(manager.mirror_inbox_item(item)) + + assert manager.gateway.texts == [] + assert len(manager.gateway.interactive_sends) == 1 diff --git a/tests/test_inbox_routing.py b/tests/test_inbox_routing.py index 1ebf97893..4931e4bb2 100644 --- a/tests/test_inbox_routing.py +++ b/tests/test_inbox_routing.py @@ -169,3 +169,24 @@ def test_emoji_reactions_still_resolve(tmp_path): resolve_from_reply(f"❌ [ow:{b.id}]", store.resolve) assert store.get(a.id).resolution == "allow" assert store.get(b.id).resolution == "deny" + + +def test_always_is_an_intent_the_parser_understands(): + """Over a chat there was no way to say "stop asking": approve and deny were the + only words, so a scheduled run's prompt could be answered but never silenced.""" + from coworker.inbox_routing import resolve_from_reply + + seen = {} + + def resolve(item_id, resolution): + seen[item_id] = resolution + return True + + assert resolve_from_reply("always [ow:abc123]", resolve) is True + assert seen["abc123"] == "always_task" + + assert resolve_from_reply("approve [ow:def456]", resolve) is True + assert seen["def456"] == "allow" + + assert resolve_from_reply("deny [ow:c0ffee]", resolve) is True + assert seen["c0ffee"] == "deny"