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

* 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) <[email protected]>

* 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) <[email protected]>

* style: cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Henry Park
2026-03-18 17:02:09 -07:00
committed by GitHub
co-authored by Claude Opus 4.6
parent 4566181f40
commit b7a1edf346
2 changed files with 11 additions and 14 deletions
-7
View File
@@ -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 {}",
+11 -7
View File
@@ -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]