Add bash result validation to prevent silent failures

[ssevera1]

What

Add validate_bash_result() function to check for None or empty output from bash tool execution.

Why

Bash commands that produce no output or error silently fail, making it difficult to diagnose issues in automation workflows.

[claude]

Requesting changes — the diff contains a breaking, out-of-scope regex change, and the new function is dead code.

Blocking: agent/utils/safety.py:86 — the fork-bomb pattern no longer compiles.

-        (r":\(\)\{.*\|.*&\}", "Fork bomb detected"),
+        (r":(\)\{.*\|.*&\}", "Fork bomb detected"),

Un-escaping the ( turns it into a capture-group opener, and \) is now a literal ), so the group is never closed. The pattern is an unterminated subpattern and re.compile raises re.error: missing ), unterminated subpattern.

Because sanitize_command compiles patterns lazily inside the loop at safety.py:96, this raises on every call — not just on fork-bomb input. sanitize_command("echo hello") now throws instead of returning (True, ""), so the entire command-safety gate is broken (and the fork-bomb guard itself is disabled). tests/test_tools.py:350 should fail on all three assertions. Please revert line 86 to r":\(\)\{.*\|.*&\}".

This change is also unrelated to the stated scope ("add bash result validation") — it looks like an accidental edit and should not be in this PR either way.

agent/utils/safety.py:114 — validate_bash_result is never called.

grep finds no callers outside its own definition. The PR title says it "prevents silent failures," but as written nothing is prevented; it is dead code. Please either wire it into the bash tool execution path or drop it.

agent/utils/safety.py:121 — empty output is not a failure signal.

Plenty of successful commands legitimately produce no stdout — mkdir -p x, touch f, grep -q pattern f, git add .. Returning (False, "Command produced no output") for these would report false failures once this is actually wired up. Exit code is the correct success signal; empty output is at most informational. If the goal is diagnosing silent failures, key the check on a non-zero exit code with empty stderr instead.

Minor: the isinstance(result, str) guard at line 121 is redundant given the None check above it and the Optional[str] annotation.

[claude]

Resolved the requested changes — all three points checked out against the code, so I fixed them by reverting the PR's changes to agent/utils/safety.py rather than salvaging any part of it:

• Fork-bomb regex (safety.py:86): confirmed. Un-escaping the ( while keeping \) literal left an unterminated capture group; python3 -c "re.compile(...)" raised re.error: missing ), unterminated subpattern. Since patterns are compiled lazily inside sanitize_command's loop, this broke the whole safety gate on every call, not just fork-bomb input. Reverted to the original r":\(\)\{.*\|.*&\}".
• Dead code (validate_bash_result): confirmed, no callers anywhere (grep for the name only finds the definition; not even imported in tests). Rather than wire it in, I removed it — see next point for why wiring it in as-written would have been wrong anyway.
• Empty output ≠ failure: confirmed, and it also explains why "wire it in" wasn't the better option. agent/tools/bash.py:135 already handles this correctly today — it returns ToolResult(True, output or "(no output)") on exit code 0 and only fails on non-zero exit code (bash.py:128-133). validate_bash_result only receives the output string (no exit code), so it structurally can't apply the reviewer's suggested "key on exit code" fix — flagging empty stdout as failure would have broken commands like mkdir -p, touch, git add .. Dropping it was the only option that didn't reintroduce a bug.
• Minor isinstance redundancy: moot, the function is gone.

Net effect: this reverts agent/utils/safety.py to its pre-PR state. I added a regression test (tests/test_tools.py, TestSafety.test_sanitize_command) asserting sanitize_command(":(){:|:&};:") is flagged as a fork bomb — this fails with re.error on the broken regex and passes after the revert.

Checks: pytest -q → 73 passed. mypy --ignore-missing-imports agent → Success, no issues.

[claude]

The previous blockers are resolved — agent/utils/safety.py:86 is back to the working r":\(\)\{.*\|.*&\}" and the unused validate_bash_result is gone. I traced the remaining test at tests/test_tools.py:360-362 against that pattern: :(){:|:&};: matches, no earlier entry in dangerous_patterns intercepts it, and the reason "Fork bomb detected" satisfies the assertIn. The code is fine.

The one thing left is metadata, not code. After the revert the diff is a 4-line test-only change, but the title ("Add bash result validation to prevent silent failures") and the description ("Add validate_bash_result() function to check for None or empty output") describe a function that is no longer in the branch — grep -rn validate_bash_result returns nothing. Since this will land via squash merge, that title becomes the permanent commit message on main and would advertise a feature that was never merged.

Please retitle to something like "test: cover fork-bomb detection in sanitize_command" and update the description to match the actual change. No code changes needed — happy to approve as soon as the title reflects the diff.