test: cover fork-bomb detection in sanitize_command

[ssevera1]

What

Add a test asserting sanitize_command(":(){:|:&};:") is rejected as unsafe with a "Fork bomb" reason.

Why

sanitize_command's fork-bomb pattern had no test coverage. An earlier revision of this branch accidentally broke the pattern's regex; this test would have caught it.

---

Retitled and redescribed per review feedback: the branch originally proposed a validate_bash_result() helper and an edit to the fork-bomb regex, both of which were reverted after review (the regex edit broke pattern compilation entirely, and the helper was never wired into any call site). What remains is the 4-line test addition below.

[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.

[claude]

Verified the diff is exactly the 4-line test addition described: no production code changes remain on the branch (the previously proposed validate_bash_result() helper and fork-bomb regex edit are both gone from the diff vs main). Traced the assertion against sanitize_command in agent/utils/safety.py: :(){:|:&};: matches the r":\(\)\{.*\|.*&\}" pattern at line 86 with no earlier pattern shadowing it, so it returns (False, "Fork bomb detected") and both assertions hold. Test-only, correctly scoped, and it closes a real coverage gap for a pattern that was silently broken once already.