fix(security): handle tirith SIGSEGV crash and disable broken binary (#41400) - #41446
fix(security): handle tirith SIGSEGV crash and disable broken binary (#41400)#41446kyssta-exe wants to merge 1 commit into
Conversation
|
One edge-case issue: The guard try:
sig_name = _signal.Signals(-exit_code).name
except (ValueError, OverflowError):
sig_name = f"signal {-exit_code}"This prevents an unhandled |
e87d8d0 to
dacef36
Compare
egilewski
left a comment
There was a problem hiding this comment.
Recommendation: needs rework before merge.
Checked current GitHub main b5f8996ccc2163ef06b4265d0882019fc24b0682 against PR head dacef36f41aef4bb0039433ad6bfcbc0bb29f385.
What validates: a mocked Tirith SIGSEGV probe (subprocess.run returning -11) shows current main keeps invoking Tirith on every call, while the PR stops invoking it after three failures and returns tirith disabled (circuit breaker). The focused Tirith test file passes on both sides:
PYTHONDONTWRITEBYTECODE=1 HERMES_HOME=<run-root>/tmp/hermes-home-base python -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_tirith_security.py -q->92 passedPYTHONDONTWRITEBYTECODE=1 HERMES_HOME=<run-root>/tmp/hermes-home-pr python -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_tirith_security.py -q->92 passed
Blocker: the open-circuit path ignores tirith_fail_open: false. In the PR replay, the same mocked -11 sequence with tirith_fail_open=False returns block for the first three calls, then returns allow with tirith disabled (circuit breaker) for all later calls. Current main blocks every mocked -11 under fail-closed mode. The early _circuit_open return in tools/tirith_security.py runs before fail-open policy is applied, so this changes a configured fail-closed scanner failure into fail-open after the circuit opens.
CodeRabbit also ran successfully and reported one critical finding: the new global _crash_count / _circuit_open updates are unsynchronized across concurrent check_command_security calls, so increments and the circuit-open transition can race.
Other validation:
git diff --check HEADfails ontests/tools/test_tirith_security.py:1249due added trailing whitespace.- GitHub reports
mergeable=MERGEABLE,mergeStateStatus=BLOCKED;test (3)is failing intests/run_agent/test_run_agent.py::TestConcurrentToolExecution::test_concurrent_handles_tool_error, separate from the focused Tirith test pass.
Signed: GPT-5.5-xhigh in Codex
|
Merged via #52997. Your circuit breaker was chosen as the base — clean, focused, matching the existing code patterns. Cherry-picked with authorship preserved (rebase-merge). A follow-up extracted _record_tirith_crash() helper to dedup the 3 copy-pasted blocks and documented thread safety. Thanks! |
Fixes #41400. When the tirith binary crashes with a signal (e.g. SIGSEGV), the return code is negative (-11). The old code treated this as an unknown exit code and allowed it through, but kept retrying the broken binary on every tool invocation. Now signal-killed processes are detected via negative return codes, tirith is disabled for the process lifetime after the first crash, and the failure is logged with the signal name.