From b7a1edf346e352590fa1c07d1807ac7c98c53a8c Mon Sep 17 00:00:00 2001
From: Henry Park <henrypark133@gmail.com>
Date: Wed, 18 Mar 2026 17:02:09 -0700
Subject: [PATCH] fix: remove debug_assert guards that panic on valid error
 paths (#1385)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* fix: remove debug_assert guards that panic on valid error paths (#1312)

Two debug_assert! calls added in #1312 fire on expected runtime error
paths (not programmer bugs), turning graceful error returns into panics
in debug/test builds:

- state.rs: Completed→Cancelled is a user-facing error handled by
  transition_to() returning Err — not a bug
- execute.rs: empty tool_name from malformed LLM output is handled by
  ToolError::NotFound — not a bug

Removes both asserts; keeps the circuit-breaker assert (genuinely guards
a caller invariant).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix: tighten empty tool name test to assert ToolError::NotFound variant

Address review feedback: assert the specific error variant instead of
just is_err() so the regression test actually enforces the expected
error path.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
---
 src/context/state.rs |  7 -------
 src/tools/execute.rs | 18 +++++++++++-------
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/src/context/state.rs b/src/context/state.rs
index bae5bdf1..f5307947 100644
--- a/src/context/state.rs
+++ b/src/context/state.rs
@@ -258,13 +258,6 @@ impl JobContext {
         new_state: JobState,
         reason: Option<String>,
     ) -> Result<(), String> {
-        debug_assert!(
-            self.state.can_transition_to(new_state),
-            "BUG: invalid job state transition {} -> {} for job {}",
-            self.state,
-            new_state,
-            self.job_id
-        );
         if !self.state.can_transition_to(new_state) {
             return Err(format!(
                 "Cannot transition from {} to {}",
diff --git a/src/tools/execute.rs b/src/tools/execute.rs
index fa52c59c..bb8a7b9d 100644
--- a/src/tools/execute.rs
+++ b/src/tools/execute.rs
@@ -22,10 +22,6 @@ pub async fn execute_tool_with_safety(
     params: &serde_json::Value,
     job_ctx: &JobContext,
 ) -> Result<String, Error> {
-    debug_assert!(
-        !tool_name.is_empty(),
-        "BUG: execute_tool_with_safety called with empty tool_name"
-    );
     let tool = tools
         .get(tool_name)
         .await
@@ -297,8 +293,8 @@ mod tests {
 
     #[tokio::test]
     async fn test_execute_empty_tool_name_returns_not_found() {
-        // Regression: execute_tool_with_safety must reject empty tool names before
-        // even attempting a registry lookup (the debug_assert guards this invariant).
+        // Regression: execute_tool_with_safety must reject empty tool names
+        // gracefully via ToolError::NotFound (not a panic).
         let registry = registry_with(vec![]).await;
         let safety = test_safety();
 
@@ -311,7 +307,15 @@ mod tests {
         )
         .await;
 
-        assert!(result.is_err(), "Empty tool name should return an error"); // safety: test-only assertion
+        assert!(
+            matches!(
+                result,
+                Err(crate::error::Error::Tool(
+                    crate::error::ToolError::NotFound { .. }
+                ))
+            ),
+            "Empty tool name should return ToolError::NotFound, got: {result:?}"
+        );
     }
 
     #[tokio::test]