(tooling-gofannon) 03/06: make AgentStopped a BaseException so user code cant swallow stops

[ak...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

andrewmusselman pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tooling-gofannon.git

commit 8ab8956420cbfb341ded53d9d521ae6e10af4421
Author: Andrew Musselman <ak...@apache.org>
AuthorDate: Fri Jun 5 16:42:35 2026 +0000

    make AgentStopped a BaseException so user code cant swallow stops
    
    The asvs agents log warnings like
    
      WARNING: Could not load ASVS levels: Run was stopped by user
      WARNING: dropping 345 unknown section ID(s)
      ... (repeated for every chapter)
    
    and roll through to a 0-section output after Stop is pressed. The
    'Run was stopped by user' string is the AgentStopped exception
    message; the agent has a broad
    
      try:
          load_asvs_levels()
      except Exception as e:
          print(f'  WARNING: Could not load ASVS levels: {e}', flush=True)
    
    around the ASVS load call. check_should_stop raises AgentStopped,
    the except Exception swallows it, the agent logs a warning, then
    continues to the next chapter where check_should_stop raises
    again, and again, and again until the agent reaches a 0-section
    'completion' that's effectively garbage.
    
    This is the same shape as Python before 3.8 when asyncio
    .CancelledError was Exception and broad except clauses would
    quietly absorb cancellation requests. PEP 622-era discussion led to
    reparenting CancelledError onto BaseException so it would
    propagate through user code regardless of try/except hygiene.
    Apply the same fix to AgentStopped: cancellation signals must not
    be catchable by code that's only meant to recover from ordinary
    errors.
    
    routes.py's run_agent_task needs the matching update: with
    AgentStopped no longer an Exception, the streaming endpoints
    cleanup-and-mark-complete handler doesn't cover it via
    'except Exception' anymore. Broaden to '(Exception, AgentStopped)'
    explicitly. Same handler logic for both, just named for clarity
    rather than catching BaseException unscoped.
    
    The asvs_* agent files dont need editing -- their broad except
    clauses are now harmless for cancellation, since AgentStopped
    bypasses them and reaches the worker's catch directly.
    
    Adds a regression test: an agent that wraps check_should_stop in
    'except Exception' must still propagate AgentStopped to its
    caller once the cancel bit is set.
---
 webapp/packages/api/user-service/routes.py         |  7 +++-
 .../api/user-service/services/cancel_token.py      | 16 ++++++++--
 .../tests/unit/services/test_agent_executor.py     | 37 ++++++++++++++++++++++
 3 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/webapp/packages/api/user-service/routes.py b/webapp/packages/api/user-service/routes.py
index b0387f3..2e923fe 100644
--- a/webapp/packages/api/user-service/routes.py
+++ b/webapp/packages/api/user-service/routes.py
@@ -809,7 +809,12 @@ async def run_agent_code_stream(
                 "Agent code executed successfully (streaming).",
                 metadata={"request": get_sanitized_request_data(req)},
             )
-        except Exception as e:
+        except (Exception, AgentStopped) as e:
+            # AgentStopped now inherits from BaseException so user code
+            # can't swallow it via except Exception; we have to name it
+            # explicitly here to keep our cleanup-and-mark-complete
+            # handler covering it. Same handler logic for error and
+            # stop cases below.
             final["error"] = f"{type(e).__name__}: {e}"
             # Distinguish a user-initiated stop from a genuine error.
             # AgentStopped is raised by check_should_stop() when the
diff --git a/webapp/packages/api/user-service/services/cancel_token.py b/webapp/packages/api/user-service/services/cancel_token.py
index a8ffaaa..8a1078a 100644
--- a/webapp/packages/api/user-service/services/cancel_token.py
+++ b/webapp/packages/api/user-service/services/cancel_token.py
@@ -22,8 +22,20 @@ from dataclasses import dataclass
 from typing import Optional
 
 
-class AgentStopped(Exception):
-    """Raised when a run's cancel token is set and a structural check fires."""
+class AgentStopped(BaseException):
+    """Raised when a run's cancel token is set and a structural check fires.
+
+    Inherits from BaseException (not Exception) deliberately, mirroring
+    asyncio.CancelledError, KeyboardInterrupt, and SystemExit:
+    cancellation signals must propagate through user code unimpeded. If
+    AgentStopped were Exception, a single ``except Exception`` anywhere
+    in the agent code would swallow the cancel and the agent would
+    continue. The asvs_* agents in particular do exactly this -- broad
+    try/except around data store loads -- and a stop request would log
+    a warning per chapter and roll on regardless. Making AgentStopped
+    BaseException forces it to bubble past those handlers to the
+    streaming endpoint's explicit catch where mark_complete fires.
+    """
 
 
 @dataclass
diff --git a/webapp/packages/api/user-service/tests/unit/services/test_agent_executor.py b/webapp/packages/api/user-service/tests/unit/services/test_agent_executor.py
index f977da0..c6bffa9 100644
--- a/webapp/packages/api/user-service/tests/unit/services/test_agent_executor.py
+++ b/webapp/packages/api/user-service/tests/unit/services/test_agent_executor.py
@@ -133,3 +133,40 @@ async def test_stop_interrupts_mid_await():
     assert elapsed < 1.5, (
         f"stop should take effect mid-await; took {elapsed:.2f}s"
     )
+
+
+@pytest.mark.asyncio
+async def test_agent_stopped_escapes_except_exception():
+    """Regression: agents' broad `except Exception as e: log; continue`
+    patterns must not swallow AgentStopped. The asvs_orchestrate agent
+    in particular wraps an ASVS-load call in try/except Exception; with
+    AgentStopped as Exception the agent caught it, logged a warning per
+    chapter, dropped sections it couldn't classify, and rolled through
+    to a useless 0-section output instead of stopping. Making
+    AgentStopped a BaseException (mirroring asyncio.CancelledError's
+    Python 3.8 reparenting) makes broad except clauses pass it through."""
+    from services.cancel_token import (
+        AgentStopped, CancelToken, bind_token, check_should_stop,
+    )
+
+    cancel = CancelToken()
+
+    async def agent_with_broad_except():
+        bind_token(cancel)
+        survivors = 0
+        for _ in range(5):
+            try:
+                check_should_stop()
+                survivors += 1
+            except Exception:  # the asvs_* pattern
+                survivors += 100  # marker: if this fires, test fails
+        return survivors
+
+    # Sanity: with bit not set, agent runs 5 iters normally.
+    assert await agent_with_broad_except() == 5
+
+    # Flip the bit. AgentStopped should escape the except Exception
+    # and abort the agent.
+    cancel.request_stop()
+    with pytest.raises(AgentStopped):
+        await agent_with_broad_except()


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@tooling.apache.org
For additional commands, e-mail: commits-help@tooling.apache.org