diff --git a/gateway/run.py b/gateway/run.py index 2cff27be45..08f6b37448 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -19496,30 +19496,54 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew async def _on_confirm(choice: str): if choice == "cancel": return f"🟡 /{command} cancelled. Conversation unchanged." + persisted = False if choice == "always": try: from cli import save_config_value - save_config_value("approvals.destructive_slash_confirm", False) - logger.info( - "User opted out of destructive slash confirm (session=%s)", - session_key, + # save_config_value swallows its own errors and reports the + # outcome in the return value, so the try block alone says + # nothing about whether the write landed. + persisted = bool( + save_config_value("approvals.destructive_slash_confirm", False) ) + if persisted: + logger.info( + "User opted out of destructive slash confirm (session=%s)", + session_key, + ) + else: + logger.warning( + "Could not persist destructive_slash_confirm=false " + "(session=%s); config.yaml is not writable", + session_key, + ) except Exception as exc: logger.warning( "Failed to persist destructive_slash_confirm=false: %s", exc, ) result = await execute() if choice == "always": - note = ( - "\n\nℹ️ Future /clear, /new, /reset, and /undo will run " - "without confirmation. Re-enable via " - "`approvals.destructive_slash_confirm: true` in config.yaml." - ) + if persisted: + note = ( + "\n\nℹ️ Future /clear, /new, /reset, and /undo will run " + "without confirmation. Re-enable via " + "`approvals.destructive_slash_confirm: true` in config.yaml." + ) + else: + # The user did approve this run, so the action still goes + # ahead, but the preference did not stick and the prompt + # will be back next time. Say so rather than promising an + # opt-out that was never written. + note = ( + "\n\n⚠️ Could not save that preference (config.yaml is not " + "writable), so /clear, /new, /reset, and /undo will ask " + "again next time. To silence it permanently, set " + "`approvals.destructive_slash_confirm: false` in config.yaml." + ) if isinstance(result, str): return result + note - # EphemeralReply or other — leave untouched; the opt-out note - # would otherwise mangle structured replies. The persist itself - # already happened above; user gets the same UX next time. + # EphemeralReply or other: leave untouched, since the note would + # mangle structured replies. return result return result diff --git a/tests/gateway/test_destructive_slash_always_persist_report.py b/tests/gateway/test_destructive_slash_always_persist_report.py new file mode 100644 index 0000000000..6779e6d9c9 --- /dev/null +++ b/tests/gateway/test_destructive_slash_always_persist_report.py @@ -0,0 +1,132 @@ +"""Answering "Always Approve" must not claim an opt-out that was not saved. + +``save_config_value`` swallows its own errors and reports the outcome through +its return value, so the caller's ``try``/``except`` never sees a failed write. +``_on_confirm`` ignored that return value and appended "Future /clear, /new, +/reset, and /undo will run without confirmation" unconditionally, so on any +install whose ``config.yaml`` is not writable (read-only bind mount, container +recreation) the user was told the preference stuck when it had not. The prompt +comes back on the next restart with no explanation. +""" + +from __future__ import annotations + +import sys +import types + +import pytest + +import gateway.run as gw + + +class _Source: + platform = "telegram" + + +class _Event: + source = _Source() + + +def _runner(): + """Bare object carrying only what _maybe_confirm_destructive_slash touches.""" + obj = types.SimpleNamespace() + obj._read_user_config = lambda: {"approvals": {"destructive_slash_confirm": True}} + obj._session_key_for_source = lambda source: "sess-1" + obj._typed_command_prefix_for = lambda platform: "/" + captured = {} + + async def _request_slash_confirm(*, event, command, title, message, handler): + captured["handler"] = handler + return "prompted" + + obj._request_slash_confirm = _request_slash_confirm + obj._captured = captured + return obj + + +async def _resolve(runner, choice, *, result="🧹 Conversation cleared."): + """Drive the gate, then invoke the captured handler with *choice*.""" + async def _execute(): + return result + + await gw.GatewayRunner._maybe_confirm_destructive_slash( + runner, + event=_Event(), + command="clear", + title="Clear conversation", + detail="This discards history.", + execute=_execute, + ) + return await runner._captured["handler"](choice) + + +@pytest.fixture +def fake_cli(monkeypatch): + """Install a stub `cli` module whose save_config_value outcome is settable.""" + calls = [] + + module = types.ModuleType("cli") + + def save_config_value(key_path, value): + calls.append((key_path, value)) + return module._outcome + + module.save_config_value = save_config_value + module._outcome = True + monkeypatch.setitem(sys.modules, "cli", module) + module.calls = calls + return module + + +@pytest.mark.asyncio +async def test_failed_persist_is_reported_as_failed(fake_cli): + fake_cli._outcome = False + + out = await _resolve(_runner(), "always") + + assert fake_cli.calls == [("approvals.destructive_slash_confirm", False)] + # The action the user approved still ran. + assert out.startswith("🧹 Conversation cleared.") + # ...but the message must not promise an opt-out that was never written. + assert "will run without confirmation" not in out + assert "Could not save that preference" in out + + +@pytest.mark.asyncio +async def test_successful_persist_still_reports_success(fake_cli): + fake_cli._outcome = True + + out = await _resolve(_runner(), "always") + + assert fake_cli.calls == [("approvals.destructive_slash_confirm", False)] + assert "will run without confirmation" in out + assert "Could not save that preference" not in out + + +@pytest.mark.asyncio +async def test_raising_persist_is_also_reported_as_failed(fake_cli): + def _boom(key_path, value): + raise OSError(30, "Read-only file system") + + fake_cli.save_config_value = _boom + + out = await _resolve(_runner(), "always") + + assert out.startswith("🧹 Conversation cleared.") + assert "will run without confirmation" not in out + + +@pytest.mark.asyncio +async def test_once_neither_persists_nor_annotates(fake_cli): + out = await _resolve(_runner(), "once") + + assert fake_cli.calls == [] + assert out == "🧹 Conversation cleared." + + +@pytest.mark.asyncio +async def test_cancel_does_not_persist(fake_cli): + out = await _resolve(_runner(), "cancel") + + assert fake_cli.calls == [] + assert "cancelled" in out.lower()