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