feat(engine): add code execution failure categorization instrumentation

[serrrfirat]

Summary

• Adds CodeExecutionFailure enum (7 variants: SyntaxError, RuntimeError, NameLookup, VmPanic, ResourceLimit, ToolError, OsDenied) to classify why code execution fails
• Threads failure through CodeExecutionResult (replacing had_error: bool) and emits structured CodeExecutionFailed events for aggregate analysis of failure modes (Monty limitation vs LLM logic error vs tool dispatch failure)
• Upgrades trace analysis to use structured events with severity escalation (VmPanic/ResourceLimit → Error), with backward-compatible fallback for pre-instrumentation threads

Caller audit (Err → Ok(failure=VmPanic) shift)

execute_code / execute_code_with_skills previously returned Err(EngineError::Effect) on VM panic; now returns Ok(CodeExecutionResult { failure: Some(VmPanic) }). The only production call site is orchestrator.rs:handle_execute_code_step which correctly dispatches on result.failure. Test-only callers in scripting.rs were also updated. No other callers exist.

Known limitation: mixed-era fallback

The trace analyzer's backward-compatible fallback (message-scraping for pre-instrumentation threads) is all-or-nothing: if a thread has any CodeExecutionFailed event, message-scraping is skipped entirely. Errors from pre-instrumentation steps in a mixed-era thread will go unreported. This is acceptable during the transition period since all new threads will have full instrumentation.

Test plan

• 11 new unit tests for error classifier, code hash, and trace detection
• cargo test -p ironclaw_engine --lib — 395 pass
• cargo clippy --all --all-features — zero engine warnings

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces structured instrumentation for code execution failures by adding a CodeExecutionFailure enum and emitting CodeExecutionFailed events during the execution process. The analyze_trace logic is updated to utilize these events for more precise issue reporting, while maintaining a fallback for older message-level patterns. Review feedback recommends replacing DefaultHasher with a stable hashing algorithm like XxHash64 to ensure consistency across compiler versions for persisted events, and suggests using to_ascii_lowercase() for error classification to follow project conventions.

[gemini-code-assist]

DefaultHasher is not guaranteed to produce stable hashes across different compiler versions. Since these hashes are used for correlating events that may be persisted, this could lead to inconsistencies over time. It's recommended to use a hashing algorithm with a specified, stable output, such as XxHash64 from the twox-hash crate.

[gemini-code-assist]

Per project conventions, to_ascii_lowercase() should be used for case-insensitive comparisons on ASCII text. This avoids locale-dependent behavior and is more efficient. The error messages from Monty are expected to be ASCII.

Suggested change

	let lower = error_msg.to_lowercase();
	let lower = error_msg.to_ascii_lowercase();

References

1. For case-insensitive comparisons, use to_ascii_lowercase() as it is the project convention, especially when the text is known to be ASCII.

[serrrfirat]

Medium Severity — error_text takes first 500 chars of stdout instead of last

let error_text: String = result.stdout.chars().take(500).collect();

When code has print output before the error (e.g., a loop printing 100 items then hitting a NameError), the first 500 chars capture the print statements, not the error traceback. The project's own compact_output_metadata() in scripting.rs:88-96 takes the last N chars for exactly this reason.

Suggestion: Extract from the tail instead:

let chars: Vec<char> = result.stdout.chars().collect();
let start = chars.len().saturating_sub(500);
let error_text: String = chars[start..].iter().collect();

Note: the existing ActionFailed path at line 806 (result.stdout.chars().take(500)) has the same issue — worth fixing both with a shared tail-extract helper.

[serrrfirat]

Medium Severity — GatePaused sets failure_category but the event is never emitted

At this return site, the code sets failure_category: Some(GatePause) but had_error is false (verified: every had_error = true in this function precedes an early return, so it's still false when GatePaused is reached).

In orchestrator.rs, the CodeExecutionFailed event emission is nested inside if result.had_error { ... }, so the GatePause category is silently discarded — it's dead data that will never appear in aggregate analysis.

Suggestion: Either:

1. Don't set failure_category here (a gate pause is a suspension, not a failure), or
2. Emit CodeExecutionFailed in orchestrator.rs based on failure_category.is_some() independently of had_error, so gate pauses are tracked in the instrumentation

[ilblackdragon]

Review

Useful instrumentation, low blast radius. Three things worth fixing before merge: (1) error-text truncation captures the wrong end, (2) duration_ms: 0 is a misleading placeholder, (3) classifier is too loose and untested against real Monty errors.

Correctness

• Error preview takes the wrong end of stdout. orchestrator.rs:~785 does result.stdout.chars().take(500).collect() — but tracebacks land at the end of stdout (after any print() output). For a script that prints 600 chars then errors, the recorded error field will contain only the prints and miss the actual failure. Use the last 500 chars, or extract the trailing Traceback (most recent call last): block.

• duration_ms: 0 with comment "not timed at this layer". Half-instrumented telemetry is worse than absent telemetry — aggregations will silently average toward zero. Either thread an actual elapsed time through CodeExecutionResult (Instant::now() around the executor call), or drop the field until you can populate it.

• VmPanic is declared but never emitted in the diff. No code path sets failure_category = Some(VmPanic). If the catch_unwind site lives in a section not in the diff, fine — otherwise this variant is dead. Confirm and add a test.

• classify_runtime_error substring matches are too loose.

  • lower.contains(\"syntax\") will misclassify a runtime error whose message happens to include the word "syntax". Tighten to \"syntaxerror\" or anchor on :.
  • Check order matters: \"Resource limit: syntax error in ...\" would be tagged SyntaxError because syntax is checked first. Reorder so the most specific (resource-limit, OS-denied) precedes the most generic (syntax → runtime).
  • lower.contains(\"os\") && lower.contains(\"denied\") will fire on benign text like \"close: permission denied\" ("os" appears as a substring of many words).

• had_error + failure_category semantic drift. Several Ok paths return had_error: true, failure_category: None. After this PR the canonical "did it fail" signal is ambiguous — orchestrator only emits CodeExecutionFailed when failure_category.is_some(), so a had_error: true / failure_category: None result fails silently from the instrumentation's perspective. Either make failure_category required when had_error == true (encode the invariant: enum CodeExecutionOutcome { Success(...), Failed(CodeExecutionFailure, ...) }), or default to a RuntimeError/Unknown variant.

• Serialization vs Display casing mismatch. #[derive(Serialize)] on CodeExecutionFailure yields \"SyntaxError\" (PascalCase) on the wire, while Display yields \"syntax_error\" (snake_case). Trace category strings are code_syntax_error; persisted events are \"category\": \"SyntaxError\". Pick one (snake_case throughout via #[serde(rename_all = \"snake_case\")]).

Trace fallback

• The fallback runs only when code_failures.is_empty(). A mixed-era thread (some pre-, some post-instrumentation events) silently skips the pre-instrumentation portion. Probably rare; worth a comment acknowledging it.
• Fallback patterns now include the literal RuntimeError — but that string rarely appears verbatim in Monty stderr. Will likely fire on user-printed text more than real errors. Verify against a real corpus.

Persistence / forward-compat

• New EventKind::CodeExecutionFailed variant: confirm the v2 persistence layer (libSQL + Postgres event tables) round-trips this without a migration. If events are stored as JSON in a column, deserialization in older binaries reading new rows will fail unless EventKind uses #[serde(other)] or similar — a real concern during rolling deploys.

Conventions

• No .unwrap()/.expect() in production paths. ✅
• Repeated long path crate::types::step::CodeExecutionFailure::* — add a use at file top.
• code_hash uses DefaultHasher — deterministic across processes (unlike RandomState), safe for cross-process correlation. ✅ A u64-truncated hash has a ~2^32 birthday collision; fine for dedup, not for security. Add a doc note.
• code_hash is pub — intentional for orchestrator to call; fine.

Test coverage

• 11 new unit tests on classifier/hash/trace are good.
• Missing: no test that the orchestrator actually emits the CodeExecutionFailed event. Per .claude/rules/testing.md ("Test Through the Caller"), the classifier is a predicate gating an event side effect — needs a test driving handle_execute_code_step and asserting both the broadcast tx and thread.events receive the event.
• Missing: integration test against the real Monty runtime that classifies each enum variant from a real failure (SyntaxError from bad python, ResourceLimit from while True: pass, OsDenied from import os; os.listdir()). Substring matching over Monty's exact error text is brittle — pin it with real outputs so a Monty version bump that changes wording surfaces in CI rather than silently degrading the categorization.
• No e2e test that surfaces aggregated failure counts the way analytics will consume them.

Suggestions (file:line)

• executor/orchestrator.rs:~785 — slice the tail of stdout, not the head.
• executor/orchestrator.rs:~790 — wrap the executor call with Instant::now() and pass real duration_ms through CodeExecutionResult. Or drop the field.
• executor/scripting.rs:~835 — reorder classify_runtime_error checks (resource-limit, os-denied, syntax, fallthrough); tighten \"syntax\" → \"syntaxerror\"; replace \"os\" && \"denied\" heuristic with explicit error-class strings Monty actually emits.
• types/step.rs:~140 — add #[serde(rename_all = \"snake_case\")] so wire and Display agree.
• types/step.rs — consider replacing had_error: bool + failure_category: Option<_> with a sum type to make invalid states unrepresentable.
• Add an integration test in crates/ironclaw_engine/tests/ driving a real failed exec end-to-end through the orchestrator against real Monty.

[serrrfirat]

Addressed review feedback (88d3f63)

All feedback from @ilblackdragon, @gemini-code-assist[bot], and self-review has been addressed:

Correctness

• error_text truncation — now takes last 500 chars (where tracebacks are), not first
• duration_ms: 0 — threads real Instant::now() timing through the event
• VmPanic never emitted — all 9 catch_unwind Err(_) paths now return Ok(CodeExecutionResult { failure: Some(VmPanic) }) instead of propagating EngineError::Effect
• Classifier too loose — tightened "syntax" → "syntaxerror", removed "os" && "denied" substring match, reordered checks (specific before generic)
• had_error + failure_category semantic drift — replaced with single failure: Option<CodeExecutionFailure> field. GatePause variant removed (gate pauses are suspensions, not failures)
• Serialize vs Display mismatch — added #[serde(rename_all = "snake_case")]

Stability

• DefaultHasher — replaced with inline FNV-1a (stable across Rust versions)
• Forward-compat — added #[serde(other)] Unknown variant to EventKind

Tests

• Added orchestrator caller test verifying CodeExecutionFailed event emission
• Added classifier edge case tests (syntax-word-not-syntaxerror, VmPanic serialization)

Style

• to_ascii_lowercase() per convention
• Added use CodeExecutionFailure import, removed verbose paths
• Expanded fallback comment acknowledging mixed-era thread limitation
• Fixed misleading CodeExecutionFailed docstring

All 395 tests pass, cargo fmt clean.

[serrrfirat]

@ilblackdragon — addressed all your feedback in 88d3f63. Here's a walkthrough of the design decisions:

---

1. had_error + failure_category → sum type

Replaced both fields with a single failure: Option<CodeExecutionFailure>. This makes invalid states unrepresentable — every failure has a category, and failure.is_some() replaces had_error. The JSON key remains "had_error" in the orchestrator result dict for Python compat (default.py:659).

Removed the GatePause variant entirely. Gate pauses aren't failures — they're suspensions represented by need_approval: Some(ThreadOutcome::GatePaused { .. }). The old code set failure_category: Some(GatePause) with had_error: false, which was dead data (the CodeExecutionFailed event was gated on had_error). Clean separation now: failure tracks errors, need_approval tracks suspensions.

2. VmPanic emission

All 9 catch_unwind Err(_) paths now return Ok(CodeExecutionResult { failure: Some(VmPanic), ... }) instead of Err(EngineError::Effect). Previously these panics:

• Propagated as EngineError → thread failure, no instrumentation
• Made VmPanic a dead variant

Now they:

• Flow through the orchestrator's event emission path → proper CodeExecutionFailed event
• Include stdout captured up to the panic point (valuable for diagnosing Monty bugs)
• Show up in trace analysis with IssueSeverity::Error

This is safe because each Err(_) path is a return — the Monty VM handle is consumed and dropped, no corrupt state leaks.

3. Error text truncation (first → last)

Both ActionFailed and CodeExecutionFailed now take the last 500 chars of stdout. Python tracebacks appear at the end, after any print() output. This matches what compact_output_metadata() already does for the LLM context.

4. Classifier tightening

Reordered checks from most specific to most generic:

1. ResourceLimit (multi-keyword: "timed out", "timeout", "memory limit", "fuel", etc.)
2. OsDenied (only "os operations are not permitted" and "oserror" — removed the loose contains("os") && contains("denied") that matched "compose", "diagnosis", etc.)
3. SyntaxError ("syntaxerror" instead of "syntax" — the actual Python exception type)
4. RuntimeError (catch-all)

5. Stable hash

FNV-1a (64-bit) inline — 6 lines, no new dependency, deterministic across Rust versions forever. DefaultHasher is explicitly documented as unstable across versions.

6. Serde consistency

#[serde(rename_all = "snake_case")] on CodeExecutionFailure — wire format now matches Display output. Since this type is new in this PR, no backward compat concern.

#[serde(other)] Unknown on EventKind — older binaries reading events from newer binaries will deserialize unknown variants as Unknown instead of failing. Unit variant per serde's externally-tagged enum requirement.

7. Real timing

Instant::now() wraps the execute_code() call in the orchestrator. duration_ms is now the actual wall-clock time of code execution, not a placeholder zero.

Not addressed (noted as follow-up)

• Integration test against real Monty runtime for each failure category — worth doing but requires pinning specific Monty error formats, better as a separate PR
• e2e test for aggregated failure counts — analytics consumer doesn't exist yet

[ilblackdragon]

Review

Overview

Introduces CodeExecutionFailure and threads it through CodeExecutionResult (replacing had_error: bool). Adds a CodeExecutionFailed structured event emitted from orchestrator::handle_execute_code_step, upgrades trace.rs to consume it with a message-scraping fallback, and adds EventKind::Unknown for forward-compat deserialization.

Strengths

• Clean shape: category travels with the result; orchestrator emits both ActionFailed and the new CodeExecutionFailed so existing observers aren't broken.
• #[serde(other)] Unknown on EventKind is a nice rolling-deploy hedge.
• classify_runtime_error is well-tested — specific vs. substring false positives are covered (e.g. "unexpected syntax" is not SyntaxError).
• code_hash uses FNV-1a rather than DefaultHasher — correct call for cross-version stability.
• Tests exercise the caller (execute_code_step_emits_code_execution_failed_event), not just the helper.
• trace.rs severity escalation (VmPanic/ResourceLimit → Error) is sensible.

Correctness concerns

1. Behavior change on VM panic (scripting.rs, 7 sites). Previously catch_unwind failures returned Err(EngineError::Effect { … }); now they return Ok(CodeExecutionResult { failure: Some(VmPanic), … }). This is a semantic shift — any caller that matched on Err to escalate/abort will now silently continue. The orchestrator path handles this correctly via result.failure, but please audit other call sites of execute_code / execute_code_with_skills.

2. PR description mismatch — GatePause variant missing. The description lists 8 variants including GatePause, but CodeExecutionFailure has only 7 (no GatePause). Gate-paused execution sets failure: None, which is consistent, so the description is just wrong. Please update.

3. Unrelated migration churn. migrations/checksums.lock adds V24__llm_calls_created_at_index but no V24 migration file is in this PR. Almost certainly a stray artifact from rebase/merge — drop it or it'll produce a phantom checksum mismatch against whatever branch actually adds that migration.

4. Fallback branch is all-or-nothing. Per the comment in trace.rs: a thread with any CodeExecutionFailed event skips message-scraping entirely, so errors from pre-instrumentation steps in a mixed-era thread go unreported. Consider scraping messages that predate the earliest CodeExecutionFailed event, or accept a brief dup window. At minimum call this out as a known limitation in the PR body.

Nits / quality

• The "last 500 chars of stdout" logic is duplicated in orchestrator.rs — extract a small tail_chars(&str, usize) helper (called twice back-to-back in the same function).
• classify_runtime_error: lower.contains("fuel") is broad; a runtime error mentioning the word "fuel" would be miscategorized as ResourceLimit. Tightening to "out of fuel" / "fuel exhausted" removes the risk at zero cost to the tests.
• CodeExecutionFailure::Display + Serialize(rename_all = "snake_case") duplicate the snake-case mapping in two places. A single source (e.g. as_str() used by both) avoids drift if a variant is added — you even added a test specifically to guard this.
• EventKind::Unknown has no doc on expected consumer behavior (probably: log + skip). Worth a one-liner.
• Typo in test message: "syntax error should set had_error" — the field is now failure.

Security / perf

No new I/O or allocations in a hot path. chars().count() + chars().skip() walks stdout twice on error; acceptable for a failure path capped at ~500-char output. error is truncated in the event; code_hash is non-cryptographic but fine for correlation.

Summary

Solid, well-tested instrumentation. Three things to resolve before merge:

1. Drop the checksums.lock change (or explain it).
2. Audit callers affected by the Err → Ok(failure=VmPanic) shift.
3. Fix the PR description (no GatePause variant exists).

[serrrfirat]

Addressed review feedback from #2483 (comment):

Code fixes (d5baf70):

1. Extract tail_chars helper — deduplicated the last-500-chars logic in handle_execute_code_step (was copy-pasted for both ActionFailed and CodeExecutionFailed events)
2. Tighten fuel match — contains("fuel") → contains("out of fuel") || contains("fuel exhausted") to avoid miscategorizing runtime errors mentioning "fuel"
3. Fix test typo — "should set had_error" → "should set failure"
4. Fix clippy warnings — !x.is_some() → x.is_none(), removed tautological u64 >= 0 assertion

PR description updates:

• Fixed variant count (7, not 8 — no GatePause variant)
• Added caller audit section confirming only orchestrator.rs calls execute_code in production
• Documented mixed-era fallback as a known limitation
• checksums.lock was already dropped in the previous fix commit

[henrypark133]

Review: Code execution failure categorization

No verified findings in the current diff. The change threads structured code-execution failure categories from the scripting runtime into the orchestrator caller and trace analyzer, and it keeps the fallback message-based detection for older threads that predate the new event.

[claude]

Code review

Found 5 issues:

1. [HIGH:95] Overly broad error classification in classify_runtime_error — The function checks lower.contains("oserror") which will misclassify legitimate Python OSErrors (FileNotFoundError, PermissionError, etc.) as OsDenied. This field should only represent Monty sandbox rejections, not generic OSError exceptions. Check for the exact message "OS operations are not permitted" only.

https://github.com/anthropics/ironclaw/blob/017218d0a342ba6bda4c9b9f755db3c352a0e65b/crates/ironclaw_engine/src/executor/scripting.rs#L515-L535

2. [HIGH:90] Double character iteration in tail_chars — The function calls .chars().count() then .chars().skip(), iterating the entire string twice. For large stdout (common in code execution), this is O(2n). Function is called twice per failure (lines 756, 784), compounding the cost.

https://github.com/anthropics/ironclaw/blob/017218d0a342ba6bda4c9b9f755db3c352a0e65b/crates/ironclaw_engine/src/executor/orchestrator.rs#L2196-L2204

Consider iterating once: collect into a Vec, or use byte-based slicing if ASCII.

3. [HIGH:85] Unbounded string allocation in classification hot path — classify_runtime_error() calls .to_ascii_lowercase() on potentially multi-KB error messages, creating a full copy. Called 4+ times per failure (lines 367, 506, 544, 761). Use case-insensitive matching directly or only on prefixes.

https://github.com/anthropics/ironclaw/blob/017218d0a342ba6bda4c9b9f755db3c352a0e65b/crates/ironclaw_engine/src/executor/scripting.rs#L513-L535

4. [MEDIUM:90] DRY violation: tail_chars computed twice per failure — Lines 756 and 784 both call tail_chars(&result.stdout, 500), producing the same 500-char tail for both error message and event. Extract to a variable to avoid redundant iteration.

https://github.com/anthropics/ironclaw/blob/017218d0a342ba6bda4c9b9f755db3c352a0e65b/crates/ironclaw_engine/src/executor/orchestrator.rs#L749-L800

5. [MEDIUM:75] Event cloning on broadcast includes large String fields — At lines 777, 796, ThreadEvent is cloned when broadcasting, including the full error String field (up to 500 chars per failure). Consider Arc for cheaper cloning if event volume becomes significant.

https://github.com/anthropics/ironclaw/blob/017218d0a342ba6bda4c9b9f755db3c352a0e65b/crates/ironclaw_engine/src/executor/orchestrator.rs#L777-L800

---

Notes:

• No security vulnerabilities found. Panic handling, truncation, and error classification are sound.
• Tests follow CLAUDE.md "test through the caller" pattern correctly.
• Forward compatibility (#[serde(other)] Unknown) is well done.