From 6e8be68860690f20bb65a1b2ce91e41faa37e33a Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Wed, 9 Sep 2026 20:39:51 +0900 Subject: [PATCH 1/3] 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. --- doc/admin-guide/configuration/hrw4u.en.rst | 15 +++++++ tools/hrw4u/src/common.py | 24 ++++++++--- tools/hrw4u/tests/test_cli.py | 50 ++++++++++++++++++++++ tools/hrw4u/tests/test_common.py | 17 +++++--- 4 files changed, 94 insertions(+), 12 deletions(-) diff --git a/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst index 4a0fb3f6961..11df046b797 100644 --- a/doc/admin-guide/configuration/hrw4u.en.rst +++ b/doc/admin-guide/configuration/hrw4u.en.rst @@ -115,6 +115,21 @@ This is particularly useful for build systems or when processing many configurat files at once. All files are processed in a single invocation, improving performance for large batches of files. +Exit Status +^^^^^^^^^^^ + +====== ========================================================================== +Status Meaning +====== ========================================================================== +0 Every input compiled. Warnings may still have been reported. +1 At least one input had an error, or the command line was invalid. +====== ========================================================================== + +Every input is processed before the status is decided, so one bad file in a +multi-file or bulk run does not stop the files after it. A failing compile +still writes its partial output; the exit status is what marks that output +untrustworthy. + Reverse Tool (u4wrh) ^^^^^^^^^^^^^^^^^^^^ diff --git a/tools/hrw4u/src/common.py b/tools/hrw4u/src/common.py index 15f1885f4bd..22ee24e7939 100644 --- a/tools/hrw4u/src/common.py +++ b/tools/hrw4u/src/common.py @@ -238,8 +238,12 @@ def generate_output( filename: str, args: Any, error_collector: ErrorCollector | None = None, - extra_kwargs: dict[str, Any] | None = None) -> None: - """Generate and print output based on mode with optional error collection.""" + extra_kwargs: dict[str, Any] | None = None) -> bool: + """Generate and print output based on mode with optional error collection. + + Returns True when the input produced errors, so the caller can set the exit + status after every input has been processed rather than aborting mid-run. + """ if args.ast: if tree is not None: print(tree.toStringTree(recog=parser_obj)) @@ -278,8 +282,8 @@ def generate_output( if error_collector and (error_collector.has_errors() or error_collector.has_warnings()): print(error_collector.get_error_summary(), file=sys.stderr) - if error_collector.has_errors() and not args.ast and tree is None: - sys.exit(1) + + return bool(error_collector and error_collector.has_errors()) def run_main( @@ -363,10 +367,12 @@ def run_main( emit_fatal_error(args.error_format, e) tree, parser_obj, error_collector = create_parse_tree( content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors, args.error_format) - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + if generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs): + sys.exit(1) return if any(':' in f for f in args.files): + failed = False for pair in args.files: if ':' not in pair: emit_fatal_message( @@ -398,12 +404,13 @@ def run_main( original_stdout = sys.stdout try: sys.stdout = output_file - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) finally: sys.stdout = original_stdout except Exception as e: emit_fatal_message(args.error_format, f"Error writing to '{output_path}': {e}", filename=output_path) else: + failed = False for i, input_path in enumerate(args.files): if i > 0: print("# ---") @@ -426,4 +433,7 @@ def run_main( content, filename, lexer_class, parser_class, error_prefix, not args.stop_on_error, args.max_errors, args.error_format) - generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + failed |= generate_output(tree, parser_obj, visitor_class, filename, args, error_collector, extra_kwargs) + + if failed: + sys.exit(1) diff --git a/tools/hrw4u/tests/test_cli.py b/tools/hrw4u/tests/test_cli.py index 886728de052..7b37405ced3 100644 --- a/tools/hrw4u/tests/test_cli.py +++ b/tools/hrw4u/tests/test_cli.py @@ -244,3 +244,53 @@ def test_cli_help_lists_error_format_flag() -> None: assert "--error-format" in result.stdout for choice in ("plain", "json", "markdown"): assert choice in result.stdout + + +# +# Exit-code contract: a compile error must fail the build. +# + + +def test_cli_exits_nonzero_on_syntax_error(tmp_path: Path) -> None: + """A syntax error must exit non-zero even though ANTLR recovers and yields a tree.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n inbound.req.X-Foo = \n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert ": error:" in result.stderr + + +def test_cli_exits_nonzero_on_semantic_error(tmp_path: Path) -> None: + """A semantic error must exit non-zero; the parse tree exists, so only sema catches it.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n test::add-debug-header(\"foo\");\n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert "unknown procedure" in result.stderr + + +def test_cli_collects_all_errors_and_still_exits_nonzero(tmp_path: Path) -> None: + """Multi-error mode must report every diagnostic AND fail; the two are not exclusive.""" + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n bogus.one = \"a\";\n bogus.two = \"b\";\n}\n") + + result = run_hrw4u([str(bad)]) + + assert result.returncode != 0 + assert result.stderr.count(": error:") >= 2 + + +def test_cli_multi_file_exits_nonzero_if_any_fails(sample_hrw4u_files: tuple[Path, Path, Path], tmp_path: Path) -> None: + """One bad file among good ones fails the run, but the good ones are still processed.""" + good, _, _ = sample_hrw4u_files + bad = tmp_path / "bad.hrw4u" + bad.write_text("REMAP {\n test::nope(\"x\");\n}\n") + + result = run_hrw4u([str(bad), str(good)]) + + assert result.returncode != 0 + assert "no-op" in result.stdout, "processing must continue past the failing file" diff --git a/tools/hrw4u/tests/test_common.py b/tools/hrw4u/tests/test_common.py index d17cdf6ad5d..ca7b69ddcab 100644 --- a/tools/hrw4u/tests/test_common.py +++ b/tools/hrw4u/tests/test_common.py @@ -164,14 +164,21 @@ def test_ast_mode_tree_none_with_errors(self, capsys): out = capsys.readouterr().out assert "Parse tree not available" in out - def test_error_collector_exits_on_parse_failure(self, capsys): - """When tree is None and errors exist in non-AST mode, should exit(1).""" + def test_error_collector_reports_failure_to_caller(self, capsys): + """generate_output reports errors via its return value; run_main owns the exit status.""" errors = ErrorCollector() errors.add_error(Hrw4uSyntaxError("", 1, 0, "parse failed", "bad")) args = SimpleNamespace(ast=False, debug=False, no_comments=False) - with pytest.raises(SystemExit) as exc_info: - generate_output(None, None, HRW4UVisitor, "", args, errors) - assert exc_info.value.code == 1 + + assert generate_output(None, None, HRW4UVisitor, "", args, errors) is True + + def test_clean_input_reports_no_failure(self, capsys): + """A clean parse must report False so a multi-file run keeps exit status 0.""" + tree, parser_obj, errors = create_parse_tree( + 'REMAP { no-op(); }', "", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True) + args = SimpleNamespace(ast=False, debug=False, no_comments=False) + + assert generate_output(tree, parser_obj, HRW4UVisitor, "", args, errors) is False def test_visitor_exception_collected(self, capsys): """When visitor.visit() raises, error is collected and reported.""" From 6eef24c1d220f66f19392b8c37f3e04e4528ca89 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Thu, 10 Sep 2026 13:53:19 +0900 Subject: [PATCH 2/3] 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. --- tools/hrw4u/tests/test_common.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tools/hrw4u/tests/test_common.py b/tools/hrw4u/tests/test_common.py index ca7b69ddcab..1997f28cbed 100644 --- a/tools/hrw4u/tests/test_common.py +++ b/tools/hrw4u/tests/test_common.py @@ -164,15 +164,20 @@ def test_ast_mode_tree_none_with_errors(self, capsys): out = capsys.readouterr().out assert "Parse tree not available" in out - def test_error_collector_reports_failure_to_caller(self, capsys): - """generate_output reports errors via its return value; run_main owns the exit status.""" - errors = ErrorCollector() - errors.add_error(Hrw4uSyntaxError("", 1, 0, "parse failed", "bad")) + def test_error_collector_reports_failure_to_caller(self): + """generate_output reports errors via its return value; run_main owns the exit status. + + The input parses, so ``tree`` is not None -- the exact shape the old + ``tree is None`` exit gate let through. + """ + tree, parser_obj, errors = create_parse_tree( + 'REMAP { test::nope("x"); }', "", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True) args = SimpleNamespace(ast=False, debug=False, no_comments=False) - assert generate_output(None, None, HRW4UVisitor, "", args, errors) is True + assert tree is not None + assert generate_output(tree, parser_obj, HRW4UVisitor, "", args, errors) is True - def test_clean_input_reports_no_failure(self, capsys): + def test_clean_input_reports_no_failure(self): """A clean parse must report False so a multi-file run keeps exit status 0.""" tree, parser_obj, errors = create_parse_tree( 'REMAP { no-op(); }', "", hrw4uLexer, hrw4uParser, "hrw4u", collect_errors=True) From 361995c0bd80675b953e6b20d085743c59a53320 Mon Sep 17 00:00:00 2001 From: Masaori Koshiba Date: Thu, 10 Sep 2026 14:54:50 +0900 Subject: [PATCH 3/3] 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. --- doc/admin-guide/configuration/hrw4u.en.rst | 10 ++++++---- tools/hrw4u/tests/test_cli.py | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/doc/admin-guide/configuration/hrw4u.en.rst b/doc/admin-guide/configuration/hrw4u.en.rst index 11df046b797..f7b8510ec08 100644 --- a/doc/admin-guide/configuration/hrw4u.en.rst +++ b/doc/admin-guide/configuration/hrw4u.en.rst @@ -125,10 +125,12 @@ Status Meaning 1 At least one input had an error, or the command line was invalid. ====== ========================================================================== -Every input is processed before the status is decided, so one bad file in a -multi-file or bulk run does not stop the files after it. A failing compile -still writes its partial output; the exit status is what marks that output -untrustworthy. +A compile error does not stop the run: every input is still processed before +the status is decided, so one bad file in a multi-file or bulk run does not +skip the files after it. Fatal problems outside the compile itself, such as an +invalid command line, a missing or unreadable input, or an unwritable output, +still abort immediately. A failing compile writes its partial output; the exit +status is what marks that output untrustworthy. Reverse Tool (u4wrh) ^^^^^^^^^^^^^^^^^^^^ diff --git a/tools/hrw4u/tests/test_cli.py b/tools/hrw4u/tests/test_cli.py index 7b37405ced3..ff2cfa95d8d 100644 --- a/tools/hrw4u/tests/test_cli.py +++ b/tools/hrw4u/tests/test_cli.py @@ -48,6 +48,14 @@ def run_hrw4u(args: list[str], stdin: str | None = None) -> subprocess.Completed return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd()) +def run_u4wrh(args: list[str], stdin: str | None = None) -> subprocess.CompletedProcess: + """Run u4wrh script with given arguments.""" + script = Path("scripts/u4wrh").resolve() + cmd = [sys.executable, str(script)] + args + + return subprocess.run(cmd, capture_output=True, text=True, input=stdin, cwd=Path.cwd()) + + def test_cli_single_file_to_stdout(sample_hrw4u_files: tuple[Path, Path, Path]) -> None: """Test compiling a single file to stdout.""" file1, _, _ = sample_hrw4u_files @@ -294,3 +302,14 @@ def test_cli_multi_file_exits_nonzero_if_any_fails(sample_hrw4u_files: tuple[Pat assert result.returncode != 0 assert "no-op" in result.stdout, "processing must continue past the failing file" + + +def test_cli_u4wrh_exits_nonzero_on_error(tmp_path: Path) -> None: + """u4wrh shares run_main(), so it must honor the same exit-status contract.""" + bad = tmp_path / "bad.conf" + bad.write_text("cond %{READ_REQUEST_HDR_HOOK}\n set-header X-Foo\n") + + result = run_u4wrh([str(bad)]) + + assert result.returncode != 0 + assert ": error:" in result.stderr