Merge branch 'rebase-6035' into stage/6035-approval

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)