hrw4u: add an AST round-trip test over the whole corpus

[masaori335]

Render every corpus input's AST back to hrw4u and require the compiled config to be unchanged: whatever the AST drops, the config loses too. Unlike hand-written cases, nothing has to be enumerated in advance.

        corpus .hrw4u  --parse-->  AST  --unparse-->  regenerated .hrw4u
              |                                              |
            emit                                           emit
              |                                              |
              v                                              v
           config  <----------- must match ------------>  config

It found five losses, all fixed here:

• an empty else { } looked like no else clause, so a sandbox policy denying 'else' was evaded by writing one
• comments were discarded, though five .conf goldens carry them
• a bool assignment lost the spelling the emitter echoes back
• parentheses were unwrapped, dropping the cond %{GROUP} they emit
• a set and an iprange both became a tuple of IPValue, though in [1.2.3.4] emits (1.2.3.4) and in {1.2.3.4} emits {1.2.3.4}

IfBlock.has_else is required rather than defaulted, so a site that rebuilds the node and forgets it fails instead of reopening the bypass.

A second test asserts the corpus reaches every grammar rule; a bare $param value had no fixture, now added.

[Copilot]

🟡 Changes recommended

Unresolved round-trip gaps remain for boolean contexts and leading-zero integers, and the empty-else sandbox behavior is not exercised.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds corpus-wide HRW4U AST parse/unparse round-trip and grammar-coverage tests, preserving syntax needed for equivalent emitted configuration.

Changes:

• Adds round-trip and grammar-rule coverage.
• Preserves comments, grouping, empty else clauses, boolean spelling, sets, and IP ranges.
• Adds sandbox and procedure regression fixtures.

File summaries

File	Summary
tools/hrw4u/tests/test_ast_visitor.py	Expands AST visitor tests.
tools/hrw4u/tests/test_ast_roundtrip.py	Adds corpus round-trip coverage; the empty-else case does not exercise its sandbox.
tools/hrw4u/tests/data/sandbox/denied-language-else-empty.sandbox.yaml	Adds the denied-language sandbox fixture.
tools/hrw4u/tests/data/sandbox/denied-language-else-empty.input.txt	Adds empty-else sandbox input.
tools/hrw4u/tests/data/sandbox/denied-language-else-empty.error.txt	Adds expected sandbox error output.
tools/hrw4u/tests/data/procedures/local-bare-param.output.txt	Adds expected bare-parameter output.
tools/hrw4u/tests/data/procedures/local-bare-param.input.txt	Adds bare-parameter procedure input.
tools/hrw4u/tests/ast_unparse.py	Adds AST rendering; boolean contexts, leading-zero integers, and helper annotations need follow-up.
tools/hrw4u/src/ast_visitor.py	Preserves syntax, but raw boolean spelling remains limited to assignment RHS values.
tools/hrw4u/src/ast_nodes.py	Adds AST node types and metadata.

Review details

Suppressed comments (4)

tools/hrw4u/tests/ast_unparse.py:138

• This int arm loses lexical information that the compiler emits verbatim. The grammar accepts leading-zero numbers, and inputs such as set-config("proxy.config.x", 001) are passed through as 001 by HRW4UVisitor, while the AST/unparser produces 1, so the two compiled configs differ. Preserve the numeric token spelling in the AST (or otherwise canonicalize both sides) and add a leading-zero regression.

        case int():
            return str(v)

tools/hrw4u/tests/ast_unparse.py:132

• This new helper is the only function in ast_unparse.py without parameter and return annotations; neighboring helpers such as _condition (lines 107-123) and _comparison (lines 126-129) are fully typed. Add the signature annotation so the renderer follows the repository's typed Python convention.

def _value(v) -> str:

tools/hrw4u/tests/ast_unparse.py:135

• This still normalizes boolean spellings in function-call arguments and procedure defaults because those paths use _extract_value() and produce plain bool. The emitter consumes those tokens verbatim (visitFunctionCall() and _bind_proc_args()), so an input such as run-plugin(TRUE); or a procedure default used in an assignment is regenerated as true and can change the emitted config. Preserve the raw spelling for all non-condition boolean values, not only assignment RHS values.

def _value(v) -> str:
    # bool first: it is a subclass of int, so the int arm would swallow it.
    if isinstance(v, bool):
        return "true" if v else "false"

tools/hrw4u/tests/test_ast_roundtrip.py:83

• This corpus test uses the default empty sandbox, so denied-language-else-empty does not exercise the security behavior the fixture protects. If the unparser drops an empty else again, both configs still match and this test passes; run regenerated text through its per-test sandbox (or add a sandboxed AST round-trip assertion).

    regenerated = ast_unparse.unparse(ASTVisitor().visit(tree))

    expected = _compile(source, input_file)
    # An empty config would pass no matter what the AST drops.
    assert expected, f"{input_file} compiles to nothing; it cannot witness a round trip"

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Numeric source spellings remain normalized, and a valid sandbox corpus case is excluded from coverage.

Review details

Suppressed comments (3)

tools/hrw4u/src/ast_visitor.py:131

• int(ctx.number.text) still loses source spelling for valid numeric literals with leading zeros, while the emitter forwards ctx.value().getText() and formats that raw value into the command. For example, inbound.status = 0200 regenerates as 200, so the config changes; preserve the raw numeric token (including procedure defaults and function arguments) and add a fixture.

            # Drops a leading zero the emitter would echo; no corpus input writes one.

tools/hrw4u/tests/ast_unparse.py:135

• NUMBER accepts leading-zero spellings, and the compiler keeps the original token text for assignments and function arguments. Converting 01 to int in ASTVisitor and then using str() here changes a valid input such as set-config("proxy.config.http.cache.http", 01); to ... 1, so the compiled config is not preserved for that input. Preserve the numeric lexeme (for example with a raw-number AST value) instead of normalizing it.

        case int():
            return str(v)

tools/hrw4u/tests/test_ast_roundtrip.py:57

• _compile() constructs HRW4UVisitor without a sandbox, so tests/data/sandbox/per-test-sandbox.input.txt is a valid PRE_REMAP program and produces config; this exclusion therefore skips a valid corpus case from both round-trip and grammar-rule coverage. Remove this entry (or pass the fixture's sandbox explicitly if it is intentionally meant to be rejected) so the claimed whole-corpus checks do not silently omit it.

DOES_NOT_COMPILE = frozenset({"sandbox/per-test-sandbox"})

• Files reviewed: 13/13 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The corpus round-trip checks still exclude a valid input file, leaving coverage incomplete.

Review details

Suppressed comments (1)

tools/hrw4u/tests/test_ast_roundtrip.py:57

• This exclusion leaves a valid corpus input outside both new checks: sandbox/per-test-sandbox.input.txt is a normal TXN_START section, and _compile() does not load its .sandbox.yaml, so it produces a non-empty config and can round-trip. Removing it means the test actually covers the whole corpus (the sandbox denial remains covered by the separate sandbox tests).

# Rejected before any sandbox check; named so a newly-broken input fails instead of dropping out.
DOES_NOT_COMPILE = frozenset({"sandbox/per-test-sandbox"})

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The unparser must preserve bare operation statements instead of adding ().

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 16/16 changed files
• Comments generated: 1
• Review effort level: Lite

[masaori335]

no-op; / skip-remap; parse but never compile, so nothing that produces
output loses anything here.

Out of scope for this PR: the conflation is unchanged from the branch point
(src/ast_visitor.py:118-119, already pinned by test_ast_visitor.py), and a
node/flag only means something once we decide whether to drop the grammar
alternative or make the bare form work — a language change, not a test change.
Filing that separately.

[masaori335]

Filed as #13701