Merge pull request #6036 from nesquena/stage/6035-approval

Release exp: fix 'approve once' persisting as session approval (#6035, #6017)

CHANGELOG.md

@@ -13,6 +13,8 @@
 
 - **Deep-link straight to a profile with `?profile=<name>`.** Opening the WebUI with a `?profile=NAME` query parameter now switches to that profile on boot. The name is validated against the existing profile allowlist (a nonexistent profile boots normally, path-traversal / cross-profile attempts are rejected), the switch goes through the same access-enforced `switchToProfile`, and boot fails safe on anything invalid. Thanks @rodboev. (#5730, #5682)
 
+- **"Approve once" no longer silently sticks for the whole session.** On local (non-gateway) deployments, choosing **Approve once** on a tool approval prompt was wrongly persisting as a session-wide approval, so the next matching guarded tool call skipped its approval card. "Once" now applies to only the single call that prompted it, and the next matching call re-prompts as intended; "Approve for session" and "Always allow" are unchanged. Thanks @rudidev08. (#6035, #6017)
+
 - **Conversation actions menu is now keyboard- and screen-reader-accessible.** The per-conversation actions popup (the ⋯ menu in the sidebar) now exposes proper ARIA (`aria-expanded`/`aria-controls` on the trigger, `role=menu`/`menuitem` on the items), moves focus to the first action when opened, supports Arrow/Home/End navigation and Escape-to-close, and restores focus to the originating trigger on close. The existing click/touch behavior is unchanged. Thanks @sheldon-im. (#6019)
 
 - **Model picker no longer duplicates or drops proxy/catalog models.** When the same model was reachable via both a custom proxy and a first-party catalog (e.g. `grok-4.5`), the picker could show it twice; the dedup fix collapses true duplicates to one selectable row while keeping genuinely-distinct models that merely share a name suffix — including colon-suffixed proxy models like `…:free` / `…:thinking` / `…:latest` — independently selectable. The currently-selected model always survives dedup, and model restore no longer substitutes a suffix-sharing sibling for a missing model. Thanks @rodboev. (#5997, #5989)

api/routes.py

@@ -23344,7 +23344,7 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool:
     # Collect keys from both _pending and _gateway_queues
     keys_from_pending = pending.get("pattern_keys") or [pending.get("pattern_key", "")] if pending else []
     all_keys = [k for k in keys_from_pending if k] + [k for k in gateway_keys if k]
-    if choice in ("once", "session"):
+    if choice == "session":
         for k in all_keys:
             approve_session(sid, k)
     elif choice == "always":
@@ -23352,6 +23352,11 @@ def _resolve_approval_legacy(sid: str, approval_id: str, choice: str) -> bool:
             approve_session(sid, k)
             approve_permanent(k)
         save_permanent_allowlist(_permanent_approved)
+    # choice == "once": no persistence — approval lasts this single call only.
+    # resolve_gateway_approval() below unblocks the parked agent thread for
+    # every choice, so "once" still lets the current tool run; we just must not
+    # call approve_session() here, or the next matching guarded call would find
+    # the pattern already session-approved and skip its approval card (#6017).
     # Unblock the agent thread waiting in the gateway approval queue.
     # This is the primary signal when streaming is active — the agent
     # thread is parked in entry.event.wait() and needs to be woken up.

tests/test_issue6017_approve_once_no_persist.py

@@ -0,0 +1,100 @@
+"""Regression test for #6017: "Approve once" must not persist as a session
+approval.
+
+Bug
+---
+In the local (non-gateway) approval path, ``_resolve_approval_legacy`` in
+``api/routes.py`` grouped ``"once"`` together with ``"session"`` and called
+``approve_session()`` for both. ``approve_session()`` records the pattern in the
+session-wide allowlist that the guard later consults, so a single "Approve
+once" click silently approved every later matching tool call for the whole
+session -- a destructive tool could then run with no approval card.
+
+Fix
+---
+Only ``"session"``/``"always"`` persist. ``"once"`` releases the currently
+parked call via ``resolve_gateway_approval()`` (which unblocks the waiting agent
+thread for every choice) and writes nothing to ``_session_approved``.
+
+State layer: ``tools.approval._session_approved`` -- the session-scoped approval
+allowlist that ``is_approved()`` consults. These tests drive the real
+``_resolve_approval_legacy`` against a seeded gateway approval entry and assert
+the persistence outcome directly via ``is_approved()``.
+"""
+from __future__ import annotations
+
+import importlib
+import uuid
+
+from tests.conftest import requires_agent_modules
+
+pytestmark = requires_agent_modules
+
+
+def _seed_gateway_entry(ta, sid, pattern_key):
+    """Park one guarded command in the gateway queue for ``sid``.
+
+    Mirrors the local in-process agent path: a guarded command blocks on an
+    ``_ApprovalEntry`` in ``_gateway_queues`` waiting for the user's choice.
+    """
+    entry = ta._ApprovalEntry({
+        "command": "rm -rf /tmp/6017",
+        "pattern_key": pattern_key,
+        "pattern_keys": [pattern_key],
+        "description": "recursive delete",
+    })
+    with ta._lock:
+        ta._pending.pop(sid, None)
+        ta._gateway_queues.setdefault(sid, []).append(entry)
+    return entry
+
+
+def _cleanup(ta, sid):
+    with ta._lock:
+        ta._gateway_queues.pop(sid, None)
+        ta._session_approved.pop(sid, None)
+        ta._pending.pop(sid, None)
+
+
+def test_approve_once_releases_call_but_does_not_persist():
+    """'once' unblocks the current call yet leaves nothing session-approved, so
+    the next matching guarded call still re-prompts."""
+    routes = importlib.import_module("api.routes")
+    ta = importlib.import_module("tools.approval")
+
+    sid = f"issue6017-once-{uuid.uuid4().hex[:8]}"
+    key = f"pattern-{uuid.uuid4().hex[:8]}"
+    entry = _seed_gateway_entry(ta, sid, key)
+    try:
+        assert not ta.is_approved(sid, key), "precondition: pattern not yet approved"
+        assert not entry.event.is_set(), "precondition: parked call not yet released"
+
+        routes._resolve_approval_legacy(sid, "", "once")
+
+        assert entry.event.is_set(), "'once' must release the currently parked call"
+        assert ta.is_approved(sid, key) is False, (
+            "'once' must NOT persist a session approval -- the next matching "
+            "guarded call must re-prompt (#6017)"
+        )
+    finally:
+        _cleanup(ta, sid)
+
+
+def test_approve_session_persists_session_approval():
+    """Regression guard: 'session' still persists, so a later matching call in
+    the same session is not re-prompted."""
+    routes = importlib.import_module("api.routes")
+    ta = importlib.import_module("tools.approval")
+
+    sid = f"issue6017-session-{uuid.uuid4().hex[:8]}"
+    key = f"pattern-{uuid.uuid4().hex[:8]}"
+    entry = _seed_gateway_entry(ta, sid, key)
+    try:
+        routes._resolve_approval_legacy(sid, "", "session")
+
+        assert entry.event.is_set(), "'session' must release the currently parked call"
+        assert ta.is_approved(sid, key) is True, (
+            "'session' must persist so later matching calls are not re-prompted"
+        )
+    finally:
+        _cleanup(ta, sid)