hrw4u: exit non-zero on compile errors - #13656
Conversation
The exit gate required `tree is None`, but ANTLR error recovery almost always yields a tree, so both syntax and semantic errors exited 0 while printing diagnostics and a partial .conf. Collecting every error and failing the build were mutually exclusive: only --stop-on-error exited 1. Sandbox denials were caught by the same gate, so the "denied" outcome the sandbox docs describe also exited 0. generate_output now reports failure by return value and run_main owns the exit, so a bad file in a bulk run no longer aborts the files after it. A failing compile still prints its partial .conf; the exit code now marks it untrustworthy. Suppressing those bytes would change behavior for existing pipelines and is left as a separate decision.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Updates hrw4u to exit non-zero on any compile error (syntax/semantic), even when ANTLR recovers and produces a parse tree, while still processing all inputs in multi-file/bulk runs before deciding the final status.
Changes:
generate_output()now returns a boolean indicating whether errors occurred, instead of exiting directly.run_main()accumulates per-input failures and exits1at the end of a multi-file/bulk run (or immediately for single-input paths).- Adds CLI/test coverage and documents the exit-status contract.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/hrw4u/src/common.py | Make generate_output() report errors via return value; accumulate failures in run_main() and exit non-zero appropriately. |
| tools/hrw4u/tests/test_common.py | Update unit tests to validate the new boolean failure-reporting contract. |
| tools/hrw4u/tests/test_cli.py | Add CLI integration tests asserting non-zero exit codes on syntax/semantic/multi-error and multi-file runs. |
| doc/admin-guide/configuration/hrw4u.en.rst | Document exit status behavior and multi-input processing semantics. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The failure test passed parser_obj=None, which only worked because a None tree short-circuits before the AST branch reads it. Parsing a real input instead also pins the regression: the input parses, so the tree is not None -- exactly what the old exit gate let through.
There was a problem hiding this comment.
🟡 Changes recommended
A few correctness/coverage/documentation gaps remain (notably type-hint consistency, doc wording vs fatal I/O behavior, and u4wrh regression coverage) before the exit-code contract can be considered fully locked in.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
tools/hrw4u/src/common.py:239
generate_output()is annotated as requiringparser_obj: ParserProtocol, but it is intentionally called withparser_obj=Nonein tests (and the function’s AST path already tolerates it). Adjusting the type hints to allowNone(and likewise fortree) keeps the annotations consistent with actual supported usage and avoids false-positive type-check failures.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
u4wrh drives the same run_main(), so the contract regresses just as easily there; verified the new test exits 0 against the pre-fix code. The doc said every input is processed before the status is decided, which reads as covering the fatal argument and I/O paths too -- those still exit immediately.
There was a problem hiding this comment.
🔵 Needs a closer look
The newly added exit-status documentation claims invalid CLI arguments return status 1, but argparse usage errors typically exit with status 2 unless explicitly normalized.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
doc/admin-guide/configuration/hrw4u.en.rst:126
- The docs state exit status 1 for an invalid command line, but
run_main()relies onargparse.ArgumentParser.parse_args()(default behavior), which exits with status 2 on usage/argument errors. The exit-status contract should document this (or adjust the code to normalize argparse failures to 1).
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
run_main() lets argparse handle the command line, so an unknown option, a bad option value, or conflicting output modes exit 2, not the 1 the table claimed. Normalizing them to 1 would merge "you typed the command wrong" into "your rules did not compile", so the doc follows the code. Row 1 now lists only what actually exits 1; the mixed bulk/stdout rejection is caught after parsing and belongs there, not with argparse.
cmcfarlen
left a comment
cmcfarlen
left a comment
There was a problem hiding this comment.
LGTM. Good find — the old gate needed tree is None, which ANTLR error recovery almost never produces, so syntax errors, semantic errors and sandbox denials were all exiting 0.
I generated the parsers in a worktree and ran the suite (714 passed, including the 26 CLI tests) plus manual CLI probes. Every failure path now yields 1 and clean input yields 0: semantic error with and without --stop-on-error, syntax error, --ast plus syntax error, bulk a:out1 b:out2 with one bad input (1, and both outputs still written), warnings-only still 0. Sandbox denials route through ErrorCollector.add_error, so they are covered now too. failed is initialized on both the pair and plain branches, so the trailing if failed cannot hit an unbound name, and no non-test caller of generate_output depended on the old sys.exit.
Two non-blocking notes, both about the contract rather than the code:
-
doc/admin-guide/configuration/hrw4u.en.rst:133— "A compile error does not stop the run: every input is still processed" is stated unconditionally, but--stop-on-errordoes the opposite:create_parse_treewithcollect_errors=Falsecallsemit_fatal_error->sys.exit(1), sohrw4u --stop-on-error bad.hrw4u good.hrw4unever processesgood.hrw4u. That contradicts the flag's own help text. The same sentence's "The fatal problems above still abort immediately" is also a bit loose for the mixed-format case: the":" not in paircheck is inside the loop (src/common.py:371), sohrw4u a.hrw4u:a.conf plain.hrw4uwritesa.confbefore aborting — which your owntest_cli_mixed_file_formats_exits_oneexercises. Worth carving out--stop-on-errorand softening "immediately". -
tools/hrw4u/src/common.py:286— the old gate hadnot args.ast, so--astalways exited 0. The new unconditional return makes--astexit 1 on a recoverable syntax error while still printing the tree. I think that is the right call (in--astmode the visitor never runs, so only syntax errors can trigger it), but it is currently undocumented and untested — not in the PR description, not in the new exit-status table, andtest_cli_ast_modeonly uses a valid file. A test plus a doc line would nail it down.
* hrw4u: exit non-zero on compile errors The exit gate required `tree is None`, but ANTLR error recovery almost always yields a tree, so both syntax and semantic errors exited 0 while printing diagnostics and a partial .conf. Collecting every error and failing the build were mutually exclusive: only --stop-on-error exited 1. Sandbox denials were caught by the same gate, so the "denied" outcome the sandbox docs describe also exited 0. generate_output now reports failure by return value and run_main owns the exit, so a bad file in a bulk run no longer aborts the files after it. A failing compile still prints its partial .conf; the exit code now marks it untrustworthy. Suppressing those bytes would change behavior for existing pipelines and is left as a separate decision. * hrw4u: parse real input in generate_output return-value tests The failure test passed parser_obj=None, which only worked because a None tree short-circuits before the AST branch reads it. Parsing a real input instead also pins the regression: the input parses, so the tree is not None -- exactly what the old exit gate let through. * hrw4u: cover u4wrh in the exit-code tests, scope the doc claim u4wrh drives the same run_main(), so the contract regresses just as easily there; verified the new test exits 0 against the pre-fix code. The doc said every input is processed before the status is decided, which reads as covering the fatal argument and I/O paths too -- those still exit immediately. * hrw4u: document exit status 2 for usage errors run_main() lets argparse handle the command line, so an unknown option, a bad option value, or conflicting output modes exit 2, not the 1 the table claimed. Normalizing them to 1 would merge "you typed the command wrong" into "your rules did not compile", so the doc follows the code. Row 1 now lists only what actually exits 1; the mixed bulk/stdout rejection is caught after parsing and belongs there, not with argparse. (cherry picked from commit 915543f)
|
Cherry-picked to the 10.2.x branch as 210980c for the 10.2.1 release. |
Fix #13618
The exit gate required
tree is None, but ANTLR error recovery almost always yields a tree, so both syntax and semantic errors exited 0 while printing diagnostics and a partial .conf. Collecting every error and failing the build were mutually exclusive: only --stop-on-error exited 1. Sandbox denials were caught by the same gate, so the "denied" outcome the sandbox docs describe also exited 0.generate_output now reports failure by return value and run_main owns the exit, so a bad file in a bulk run no longer aborts the files after it.
A failing compile still prints its partial .conf; the exit code now marks it untrustworthy. Suppressing those bytes would change behavior for existing pipelines and is left as a separate decision.