fix(agent): fail closed when a fail_closed shell hook exits non-zero without a directive - #122651
bablobanov wants to merge 1 commit into
Conversation
…without a directive `_evaluate_result` decided the fail-closed outcome from stdout alone. A hook that died with an uncaught exception writes its traceback to stderr and leaves stdout empty, so `parsed` was None and the action was permitted: the failure shape most likely in practice was the one the gate did not cover. A non-zero exit without a directive now blocks on a fail_closed hook; exit 0 with empty stdout still permits, and a non-zero exit that carries a JSON directive still uses that directive.
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head e3ae5f21d882454e81f2b0922a343846f7dc4def against current main / PR base 59004a62356f3a4697ab0fe8ad5086d2b405e2a6. The merge base is fdec926ef54391edcf6caad5f7f6761fdcccdaa2; this branch is one commit ahead and four commits behind current main.
The source change itself is correct. I traced the path from the registered callback through _spawn, _evaluate_result, _parse_response, and the pre_tool_call dialect parser. The important boundary is parsed is None: that means the hook produced no recognized directive, not merely that stdout was empty. On this head a non-zero exit with empty stdout blocks, a non-zero exit with a syntactically valid JSON object that contains no directive also blocks, a clean exit with no directive still permits, and a recognized block / modify / approve directive still returns before the fail-closed guard. Exit code 2 retains its existing blocking precedence. I do not see a runtime defect in the two-file implementation.
There is, however, a repository-level blocker before this should merge: this is a second live carrier for the same defect class. #122576 was opened at 11:35 UTC, fixes the same #102405 invariant, changes the same agent/shell_hooks.py and tests/agent/test_shell_hooks.py surfaces, and already carries the lineage from jorgefusterr's original report and marcelobernardo-cf-stikcky's closed-unmerged #108664. I reviewed #122576 at 1e9104b360befce1d02d9bcc9809b2b96bfcab10 earlier today: #122576 (review). That review also verified the broader response matrix and the exact historical counterexample that made the earlier narrow fix insufficient.
This PR's body mentions #108664 and the unavailable #102417, but it does not acknowledge the already-open #122576. We should not land both implementations and erase the earlier carrier's work by accident. Pick one carrier explicitly. If #122651 is retained, preserve #122576 / Enough1122 and #108664 / marcelobernardo-cf-stikcky as prior work, carry over the stronger regression coverage, and close/reconcile the other PR as superseded rather than silently duplicating it. If #122576 remains the carrier, #122651 should close as the later duplicate. Either route is technically workable; two independent merges are not.
There is one verification gap on this branch that matters because of that history. The new tests pin the common empty-stdout crash, the default fail-open control, the clean-empty control, and a recognized block directive. They do not pin the exact second escape discovered during #108664: non-zero exit + valid JSON + no recognized directive (for example {"foo":"bar"}). The implementation here handles it correctly because parsed stays None, but that is precisely the case an earlier attempted fix missed. If this branch becomes the carrier, add that regression rather than relying on reasoning alone; keep recognized approve / modify as positive controls so the guard does not become broader than the contract.
The other open shell-hook work is adjacent rather than substitutive. #91017 also touches agent/shell_hooks.py / its tests but changes cwd projection in _serialize_payload; #32613 touches the spawn path for slow-hook visibility; #84439 adds gateway route context. #111315 and #119421 address registration on other execution surfaces, and #100942 remains the separate non-TTY registration/fail-open problem. Those compose with this evaluator fix: a correct evaluator cannot protect a hook that was never registered. #92553, the separate dropped-approve parser bug, is closed; the current parser recognizes approve, and this change preserves that path.
Exact-head GitHub checks have not executed. CI (https://github.com/NousResearch/hermes-agent/actions/runs/36139420670), Docker (https://github.com/NousResearch/hermes-agent/actions/runs/36139420151), and Nix (https://github.com/NousResearch/hermes-agent/actions/runs/36139420170) are all action_required, and there are no commit status contexts. I am not treating action_required as a test failure, but it is also not green evidence. The local counts in the PR description are useful author receipts; they do not make this exact commit CI-green. Whichever branch survives consolidation still needs passing exact-head checks before merge.
So the code path is sound. The remaining work is repository truth: consolidate the duplicate carrier with credit intact, pin the previously discovered JSON/no-directive counterexample on the surviving branch, and get that resulting exact head green.
| if parsed is None and fail_closed and stdout and not _is_json_object(stdout): | ||
| # A fail-closed gate must not silently allow on garbage stdout (e.g. a stack trace). | ||
| return _fail_closed_block(spec, "unparseable stdout (expected a JSON object)") | ||
| if parsed is None and fail_closed: |
There was a problem hiding this comment.
This is the right class-level boundary: parsed is None means “no recognized directive,” so this also correctly blocks exit 1 + {"foo":"bar"}, not just empty stdout. Please pin that historical counterexample if this branch survives consolidation. #108664's closing analysis is where an empty-output-only fix was shown to be insufficient, and #122576 already carries regression coverage for it. The current tests here only exercise empty stdout plus a recognized block directive; keep the valid-JSON/no-directive case (and a recognized approve/modify positive control) so the invariant cannot narrow again later.
|
Closing in favour of #122576, which was opened two hours earlier, fixes the same invariant in For the record, this branch reached the same boundary independently ( |
|
For anyone reading the three red checks here: the |
3 participants
What does this PR do?
A
pre_tool_callshell hook declared withfail_closed: trueis meant to deny the action whenever the hook cannot be trusted to have produced a directive. One failure shape slipped through and permitted instead: the hook process exits non-zero with nothing on stdout, which is exactly what an uncaught exception in a Python hook script looks like (traceback on stderr, stdout empty)._evaluate_resultkeyed the fail-closed decision off "is stdout non-empty and unparseable". With empty stdout,parsedwasNone, the guard was false, andNonereached the caller as "no directive", i.e. permit. The non-zero exit code only produced a warning a few lines earlier.Now, on a fail-closed hook, a non-zero exit without a directive blocks with
hook <command> failed closed: exited <code> without a directive. Everything else keeps its current meaning:fail_closedare unchanged (non-zero exit with empty stdout still permits)Related Issue
Fixes #102405
Type of Change
Changes Made
agent/shell_hooks.py: in_evaluate_result, the fail-closed branch also blocks whenparsed is Noneandreturncode != 0; docstring updatedtests/agent/test_shell_hooks.py:TestEvaluateResultgains the regression case (exit 1, traceback on stderr, empty stdout,fail_closed=True→ block) and three controls: the same result withoutfail_closedstill permits, exit 0 with empty stdout underfail_closedstill permits, and a non-zero exit with a JSON directive underfail_closedstill uses the directiveHow to Test
scripts/run_tests.sh tests/agent/test_shell_hooks.py::TestEvaluateResult -qmainwithout the fix the regression case fails and the three controls pass (they pin existing behaviour); with the fix all pass:fail_closedmain{"decision": "block", ...}scripts/run_tests.sh tests/agent/test_shell_hooks.py -q: 46 passed, 10 skipped.shell_hooks: 304 passed, 1 failed; the one failure (tests/hermes_cli/test_plugins.py::TestPluginDiscovery::test_enabled_portable_plugin_registers_components) fails the same way on unmodifiedmainon this machine.scripts/check-windows-footguns.py: no findings in the changed lines (the two bareread_text()calls it reports intests/agent/test_shell_hooks.pyare pre-existing, lines 199 and 217, and untouched here).Checklist
Code
pytest tests/ -qin full was not run on this machine)Documentation & Housekeeping
fail_closedas deny-by-default; behaviour now matches it)cli-config.yaml.example— N/ACONTRIBUTING.md/AGENTS.md— N/A