From eeaba3a88db906dc1bc7e86946644adbce4bbe75 Mon Sep 17 00:00:00 2001 From: MaxFreedomPollard <272618364+MaxFreedomPollard@users.noreply.github.com> Date: Fri, 31 Jul 2026 01:59:54 -0400 Subject: [PATCH] fix(gateway): do not claim a destructive-slash opt-out that was not saved Answering "Always Approve" on the /clear, /new, /reset and /undo confirmation calls save_config_value("approvals.destructive_slash_confirm", False) and then appended "Future /clear, /new, /reset, and /undo will run without confirmation" unconditionally. save_config_value catches its own exceptions and reports the outcome in the return value, so the caller's try/except could never observe a failed write, and the return value was ignored. On any install whose config.yaml is not writable the user was told the preference stuck when it had not, and the prompt returned on the next restart with no explanation. Check the return value. The approved action still runs either way, but when the write did not land, say so and point at the config key instead of promising an opt-out that was never written. --- gateway/run.py | 48 +++++-- ...destructive_slash_always_persist_report.py | 132 ++++++++++++++++++ 2 files changed, 168 insertions(+), 12 deletions(-) create mode 100644 tests/gateway/test_destructive_slash_always_persist_report.py 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()