Skip to content

Give the evaluator a value that can say "nothing to compare", and fix the empty-collection wrong passes - #720

Open
awsmadi wants to merge 150 commits into
aws-cloudformation:mainfrom
awsmadi:pr/evaluator-outcome-cleanup
Open

Give the evaluator a value that can say "nothing to compare", and fix the empty-collection wrong passes#720
awsmadi wants to merge 150 commits into
aws-cloudformation:mainfrom
awsmadi:pr/evaluator-outcome-cleanup

Conversation

@awsmadi

@awsmadi awsmadi commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Replaces the error channel the clause path used for a fourth answer with an explicit value, so that "this
clause could not be evaluated" stops being represented as an Err that every consumer has to
interrogate.

Stacked on #727 (pr/filters-captures-and-reporter-silence). Read the diff against that branch, not against
main.
GitHub will only accept a base branch that exists upstream and this branch's base exists only on the
fork, so the diff shown here contains #727's commits as well.

#727 has gained twenty-seven further commits since this branch last merged it, and both lines change
guard/src/rules/eval.rs and guard/src/rules/eval_tests.rs. That is a merge rather than a rebase,
deliberately: rebasing this branch's commits one at a time through a test file both lines append to conflicts
once per commit, and each resolution moves code the next patch expects to find in place, so the second commit
conflicts on the first's resolution. A merge resolves it once and records both resolutions.

The first merge's conflict was textual in the test file and semantic in eval.rs: #727 had added two arms
answering Ok(Status::FAIL) in functions this branch changed to return Result<Outcome>, git merged the text
cleanly and the types disagreed. They now answer Outcome::Unevaluatable role-free, which is this branch's
vocabulary for the same intent — the answer is a value and the consumer applies the role through to_status,
rather than the producer splitting on role.is_strict().

The second merge's conflict was not a conflict at all but a disagreement, and this branch was on the wrong
side of it.
Both branches had written an arm for what a reference to a rule that did not apply means, with
opposite answers and separate written rationales. This branch mapped it onto a violation for assertions —
(Outcome::NotApplicable, _) if role.is_strict() => Outcome::Violated — arguing that a rule body asserting
r(...) claims r holds and a rule that never ran is not evidence that it does. That is true, and the
conclusion drawn from it is not: the answer to "no evidence" is no verdict, not a violation. It is resolved
toward #727, and the section below says what it changes.

The base changed because #717 was merged upstream at 567c0d7 while work continued on its branch.

The problem

Fourteen functions on the clause path returned Result<Status> and used Err for an answer Status
could not hold. A clause that could not be evaluated left as an error; every consumer asked
is_unevaluatable what kind of error it was holding; and the two conditions that had to tell "could not
be answered" from "did not match" did it by catching one.

That worked. What it cost was that one question had two representations, so each site chose one and the
choice was invisible in the signature — unary_operation took a ClauseRole purely to decide whether to
answer with a value or an error.

The change

The clause path returns Result<Outcome>, where Outcome is Satisfied, Violated, NotApplicable or
Unevaluatable, and Err means an error. NotApplicable and Unevaluatable are separate variants
because the status a clause should report depends on the role it played: an assertion that cannot be
evaluated is a failure, and a gate that cannot be evaluated must not silently drop everything it guards.
The role is applied at the consumer, through to_status(role), closes_gate() and blocks(role), so a
clause's answer no longer depends on how it was reached.

eval_conjunction_clauses folds through and and or instead of counting passes and fails. The counters
were that fold for three values and had no representation for the fourth.

Three behaviour changes worth reading

A reference to a rule that did not apply now contributes nothing, in both polarities. It used to be a
violation wherever the reference is an assertion. Measured with two helper rules each restricted to their own
resource type:

template before after
one clean instance of the first type, none of the second FAIL 19 PASS 0
none of the covered types at all FAIL 19 PASS 0
a violating instance beside a clean sibling FAIL 19 FAIL 19

The first two rows were reporting a violation about a resource type the template does not contain. The third
row is the one that matters for the change's correctness and it does not move, so this is not a slide into
treating a conjunction of references as a disjunction. The parameterized spelling r(...) carried the identical
arm, with a comment claiming to mirror the first, and is fixed with it; one test asserts both so a change to
one cannot pass on the other's coverage.

Two things make this the right answer rather than a coin flip. It removes a false positive on a template that
violates nothing. And the old arm contradicted this branch's own algebra: Outcome::and has identity
NotApplicable and absorbing Violated, so one level down an inapplicable clause conjoined with a satisfied
one yields satisfied, while the reference site mapped that same NotApplicable onto the absorbing element.
Whichever way the question is answered, the reference site and Outcome::and have to answer it the same way,
and only one answer is expressible without carving an exception out of the type.

A negated reference used as a when gate is deliberately unchanged. A gate that closes silently disables
the rule it guards, and the one negated rule reference in the AWS rule registry is a gate. Both matches are now
exhaustive over the enum rather than ending in _, which is not cosmetic: that catch-all had been silently
covering both a failing dependent and a negated gate on an inapplicable one, while the comments above it named
it for whichever case the author had in mind.

The practical consequence is that a ruleset can now be decomposed over disjoint resource types at all. Before,
neither shape worked — a conjunction of references failed whenever any type was absent, and a disjunction
returns PASS when any one helper passes, so a violating IAM Role beside a clean DynamoDB table passes a rule
that should fail it.

A or B is now evaluated to the end when A cannot be answered. The counting version returned from the
first undecidable branch, so B never ran even when B decided the disjunction outright. Measured on
when Enabled !EMPTY or Name == "keep" guarding a violation: before, the rule failed closed on its
condition and the violation inside the body went unreported; now both appear. Same exit code, one more
finding. Pinned by a_gate_is_decided_by_the_branch_that_can_be.

A disjunction of undecidable branches is Unevaluatable rather than Violated, because reporting a
violation there blames the input for a reference that never resolved. Outcome::or absorbs only
Satisfied, so this is not a relaxation of failing closed —
an_unevaluatable_gate_fails_the_rule_closed holds that line for the single-condition case.

The three filter-predicate sites keep failing the query rather than selecting nothing, and now say so in
a comment that matches the code. Two of them claimed the opposite, which was never what the code did. The
behaviour is deliberate: a filter that drops the resources it could not judge selects fewer of them, and a
rule written to catch violations catches fewer — the mechanism that turned five registry security rules
from FAIL to PASS when a fail-closed change was tried inside a filter.

One thing the value cost, and how it was paid

Replacing the error channel with a value cost the error's text. A rule that fails on an undecidable
condition explains the verdict — that this is a failure rather than an inapplicable rule — and the parent
branch appended the cause by interpolating the error, because there the answer was the error. Outcome is
Copy and carries no payload, so three outputs lost the type error: same exit code, less in them, and
absent from the JSON too so nothing downstream could recover it.

RecordTracer::reason_from_last_closed_record reads it back instead of routing it twice. The leaf that
could not evaluate the clause has already recorded e.to_string(), and end_record pops a finished record
onto its parent's children, so the condition's subtree is the last child of the rule record still open above
it. Giving the variant a payload would have broken Copy at forty-six sites and forced and and or to
choose between two reasons on every fold.

Found by differencing this branch against its base over the whole fixture corpus, not by a test. That
differential is now 1,140 pairs with one exit-code difference — this branch catching a violation the base
misses — and zero text differences.

A second reason the value cost, and it was not free either

Outcome::and absorbs Violated, so a gate written as a conjunction of a condition that cannot be evaluated and
one that is decidably false answers Violated on the second conjunct alone. The verdict is right — the author
asked for both conditions, one definitively does not hold, so the rule does not apply and SKIP is correct, and
Kleene agrees that false and unknown is false. The reason the first condition could not be evaluated disappeared
with it.

Enabled !EMPTY against Enabled: true is a defect in the rule text, not a property of the data. Measured over
two templates differing only in the value of Name:

template      this branch          its base
Name: keep    exit 0, silent       exit 19, names the type error
Name: nope    exit 19, names it    exit 19, names it

So whether an author was told their rule is malformed depended on which template they ran it against. The reason
now reaches stderr, on the channel the deprecation notices already use, with stdout and every exit code and
structured field unchanged. It is read at the point the clause's record closes rather than afterwards, because a
violated sibling records a reason of its own — an incomparable pair and an empty collection both do — and a later
read would quote the wrong clause and call a violation undecidable. A rule's own condition names the rule; a
nested when block does not, because a rule name would have to be threaded through every clause evaluator
beneath it, and the recorded reason already carries the path and position. A type block's per-resource condition
is deliberately not wired, since its reason carries the resource path and would give one line per resource. The
channel is renamed from record_deprecation to record_diagnostic, because a type error in a rule is not a
deprecation.

Outcome::and itself is unchanged. Its truth table was checked by hand over all sixteen combinations and is
total and commutative.

Verification

1631 tests pass on top of #727 across 16 targets, 0 failures;
cargo clippy --release --all-targets -- -D warnings and cargo fmt --all -- --check are clean.

For the reference-to-inapplicable change specifically: the six scenarios in the table above plus the
parameterized spelling and both negated spellings, each measured against this branch's previous head so the
direction of every change is recorded rather than inferred; and all 44 directories of the AWS rule registry
run as cfn-guard test --dir against git archive snapshots at a pinned upstream ref, showing zero
change relative to #727's head — so nothing this merge does moves a registry verdict that #727 had not
already moved.

Against #717's head:

  • the 440-cell oracle matrix improves from 413 agreements to 420, with zero cells in the category that
    matters most — a gate over a violating body that does not fail
  • the registry differential over 190 pairs and 1,956 expectation checks shows 0 exit-code, 0 content and
    0 stderr changes
  • the 980-pair fixture cross product differs on four pairs, all of them undecidable-gate fixtures at exit
    19 on both sides with the same number of explanatory lines, and none exiting 19 without an explanation

The rebase onto #717 conflicted in five hunks in eval.rs and three in tests/validate.rs. The eval.rs
hunks resolved to this branch's Outcome side, which subsumes #717's narrower fix for the same
disjunction defect; the generic bound #717 added to eval_general_block_clause is orthogonal and was
kept. The tests/validate.rs conflict was two different tests appended at the same line — both are kept.
One further site the merge did not flag, eval_guard_block_clause's error arm, still split on
role.is_strict() and no longer typechecked against Result<Outcome>; it now answers Unevaluatable
role-free, like the per-value arm above it.

That arm is the reason for the last commit here. A filter predicate runs as a gate, and the predicate in
filter_predicate_that_cannot_be_judged.guard is a block clause, so it is the shape where the old
role.is_strict() split and the role-free answer differ. Three verdicts are possible and only one is
right: exit 0 would mean the filter dropped the resource it could not judge, exit 255 that the file
aborted, and exit 19 naming the predicate is the answer — so the test asserts the reason and not only the
code. Two later rebases onto #717's head were clean.

@awsmadi
awsmadi force-pushed the pr/evaluator-outcome-cleanup branch 4 times, most recently from 18fd823 to 8266784 Compare August 18, 2026 16:56
@awsmadi
awsmadi force-pushed the pr/evaluator-outcome-cleanup branch 5 times, most recently from 86281e0 to 565f552 Compare August 18, 2026 19:46
@awsmadi

awsmadi commented Aug 18, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto #717 at b116f48 (was 7268688). 40 commits unchanged in content; two resolved against #717's newest work, both in unary_operation.

b116f48 on #717 stops an incompatible type from aborting the whole rules file, and it had to pick a status for the unanswerable clause by hand — FAIL for an assertion, SKIP for a gate — because that branch has no lattice. Here it collapses into Outcome::Unevaluatable, whose own documentation already named the case: "a reference resolved to no values, or a type did not support the operation". The role is applied once, by to_status, instead of at the point the value is pushed.

The same applies to the fold. #717 grew a skips counter with a guard so a fold over zero elements kept its previous answer; Outcome::identity() is NotApplicable, so no guard is needed. That comparison is a fair miniature of the argument for this PR: the third answer becomes a value rather than a special case each fold has to remember. The comment at that site is corrected too — it said SKIP there only ever means "nothing to compare", and there are now two sources.

934 tests pass, cargo clippy -- -D warnings and --all-targets clean, cargo fmt --check and typos clean. The three cases from #717 were re-checked against this branch's binary: the two-rule abort file reports both rules, a gate with a passing sibling exits 19 with the guarded body's real violation, and the reviewer's boolean repro reports the unrelated violation alongside the boolean clause.

Still stacked on #717 and best read after it merges. Against main the diff shows 72 commits rather than its own 40, since the base branch lives on the fork.

awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
Reported by a reviewer against the previous commit, and it is the failure mode this
whole branch exists to remove -- introduced by the fix that was meant to remove it.

    rule guarded when Enabled !EMPTY { MustBeTrue == true }
    {"Enabled": true, "MustBeTrue": false}

    base 57bbdbf   exit 19, the MustBeTrue violation reported
    b116f48        exit  0, the guarded body never evaluated

`b116f48` answered SKIP for an unevaluatable clause in a gate, reasoning that declining
lets a gate's remaining conditions decide, so a decided gate runs more checks rather than
fewer. That is true when there are remaining conditions and worthless when there are not:
with one condition the rule simply does not apply and the violation inside it is never
looked for. The base exited 19 only because `!EMPTY` on a boolean was unconditionally
true there, so fixing the boolean clause without fixing the gate turned a bug that
over-reported into one that under-reported, which is the worse direction.

Neither status can express this, which is why the first attempt got it wrong rather than
merely incomplete. `eval_rule` collapses *both* FAIL and SKIP on a condition into a
rule-level SKIP, because "the condition did not match" is the ordinary gating idiom and
has to stay a skip -- `rule r when Region == 'x'` on another region must not fail. So an
unevaluatable condition travels as an error and is caught at each of the three condition
sites, which fail their own rule or block rather than letting it escape and abort the
file. aws-cloudformation#720 replaces this with `Outcome::Unevaluatable`, a value a gate can return.

An assertion is unchanged: it still fails per clause, so
`an_incompatible_type_does_not_discard_other_rules` still holds and a file whose first
rule found a violation still reports it.

Two things this costs, both stated rather than discovered later. A passing sibling no
longer rescues the gate -- `when <unevaluatable> or <passing>` fails the rule where it
used to let the sibling open it -- because an error leaves `eval_conjunction_clauses`
immediately. That is the fail-closed direction, and the lattice in aws-cloudformation#720 fixes it properly
by absorbing `Unevaluatable` under `or`. And a nested `when` block with an unevaluatable
condition now fails its rule where the base passed it, which is the same correction as
the headline case one level in.

`record_unary_clause` records FAIL in both roles now, so the record cannot disagree with
the verdict, and it no longer needs the role at all -- the parameter and the macro
threading `b116f48` added are removed again.

The generated corpus gets the invariant that would have caught this: where the body fails
on its own, wrapping it in a gate the evaluator cannot answer must not report success.
Whether a clause can be answered is derived per (clause, template) rather than declared,
because `Size EMPTY` is unanswerable against an integer and answerable against a string,
where "not empty" is right and skipping the rule is correct -- a first version keyed off a
clause-level list and failed on exactly that.

That invariant immediately found 22 more cells, all of them an undecidable *comparison*
used as a gate: an unresolved query or a type mismatch, unchanged from the merge-base, and
the case `f3c919f` already records as needing a status meaning "could not tell". They are
listed by name instead of counted, so a new one identifies itself and the day aws-cloudformation#720 lands
the list fails and asks to be deleted. No `empty_on_scalar` cell is among them, which is
what says this commit closed that clause in every shape.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
…oracle

Three changes, all decided by a maintainer after a combinatorial matrix over operator x
operand shape x polarity x position turned up disagreements the existing corpus could not
see.

**`IN` and `NOT IN` now agree with `==` and `!=` about incomparability.** They did not:

    Size: "50"
    "50" != 50                 ->  FAIL   fails closed, as docs/CLAUSES.md states
    "50" NOT IN [10, 50, 100]  ->  PASS   the same undecidability, opposite answer

`contained_in` asked `Vec::contains`, which folds "no element compared equal" together with
"no element could be compared at all" into one `false`, and the not-flag then inverted that
`false` into a pass. Three answers squeezed into a boolean. The site now distinguishes them
and reports the incomparable case as `NotComparable`, which the not-flag passes through
unchanged, so both spellings fail closed.

Scoped so denylists keep working: failing closed applies only when *nothing* in the list is
comparable. `7 NOT IN [10, 50, 100]` still passes, and a mixed list where one element is
comparable still decides on that element.

**A comparison whose left-hand variable resolved to nothing fails closed.** `3f8466e` closed
this on the right and left the mirror open, so `%x == 'abc'`, `%x != 'abc'` and `%x > 5` all
exited 0 when `%x` held no values -- the same bypass, other operand.

Scoped to a lone variable, which is the part that matters. A filtered query that matched
nothing -- `Resources.*[ Type == 'AWS::S3::Bucket' ]` against a template with no buckets --
is the idiom that lets one ruleset run over templates that do not all contain the resource
being checked, and it stays a SKIP. `an_empty_filtered_query_still_skips` asserts that half,
because a fix that failed every template omitting a resource type would be worse than the
defect. The distinction uses the same test `unary_operation` already applies for `EMPTY`, so
there is one definition of "the query is just a variable".

**The matrix is now a test.** `every_operator_and_operand_shape_agrees_with_a_stated_oracle`
carries 110 judgments -- can this operator be answered against this operand shape, and what
is the answer -- and derives all 440 cells from them by four stated rules.

That is the difference from `generated_rule_shapes_hold_the_evaluator_invariants`, which
cannot catch a wrong answer: its invariants are all satisfiable by a verdict that is wrong
but self-consistent, which is exactly what a boolean `!EMPTY` used as a gate was. 252 cells
passed it and a reviewer found that defect by reading the code.

Writing the judgments before running anything mattered, and not only in principle. The first
version called a comparison against a list undecidable and reported three defects that were
not defects: a query expands a list, so `Size == 50` against `[1, 2, 3]` compares 1, 2 and 3
and records three checks at `/Size/0`, `/Size/1` and `/Size/2`. Unary operators do not expand
-- `Tags !empty` is documented as a check on the list itself -- and the table now says both.

48 cells still disagree, listed by name rather than counted so a new one identifies itself.
All are aws-cloudformation#720's scope: an undecidable comparison used as a gate disarms the body it guards,
and a comparison against an empty collection passes vacuously. Both need a status meaning
"could not tell". Two `in_list/*/string_size` cells joined the deferred gate list rather than
leaving it, which is worth understanding before trusting the count -- making both polarities
fail closed made those cells undecidable by the test's own definition, exposing that they
disarm their body. Fixing one defect made a second visible rather than creating it.

Both behaviour changes are mutation-verified, and `docs/CLAUSES.md` documents the membership
rule since authors will see it.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
`30237d8` made `NOT IN` fail closed on an operand it cannot compare, so that it agreed
with `!=`. Correct for an assertion, and wrong here. Backed out; the empty-left-hand-side
fix and the oracle test in that commit stay.

The differential against aws-guard-rules-registry caught it. 190 rule/test pairs pass on
the merge-base and five fail with the change, all in the direction that matters:

    Expected = FAIL, Evaluated = [PASS]

    amazon_mq_broker_users_no_plaintext_password
    kinesis_firehose_redshift_destination_configuration_no_plaintext_password
    kinesis_firehose_splunk_destination_configuration_no_plaintext_password
    iam_user_login_profile_no_plaintext_password
    secretsmanager_using_cmk

Bisected to this change rather than assumed: reverting it alone returns all 190 to passing.

The mechanism inverts the safety argument, which is the part worth keeping. Those rules use
`NOT IN` inside a *filter predicate*:

    let violations = %users[
        Properties.LoginProfile.Password not in [ /{{resolve\:secretsmanager\:.*}}/, ... ]
    ]
    %violations empty

A `!Ref`-shaped password is a map, which cannot be compared with a regex. Reading that as
"not one of the secure patterns" selects the resource as a violation, which is what the rule
intends -- its own comment says a Ref to a parameter with a default is a violation. Failing
closed instead makes the filter clause fail, the resource is not selected, `%violations` is
empty, and the rule reports compliance for a plaintext password.

Failing closed is the safe direction for an assertion, where the clause is the verdict.
Inside a filter it means *fewer resources selected*, and fewer resources checked is the
unsafe direction. The two positions want opposite defaults, and `NOT IN` is used in both.

So the `!=` / `NOT IN` disagreement stays a recorded finding rather than a fix. It is in the
oracle test's KNOWN list, and the evidence now includes what the obvious repair costs --
which is more useful than the finding alone. Closing it properly needs the filter position
distinguished from the assertion position, and that is the same three-valued problem aws-cloudformation#720
addresses.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
…CHANGELOG

Both clauses pass today and are documented not to. Each now prints a `DEPRECATION` line to
stderr naming the clause, so a ruleset can be audited before the answer moves. No verdict
changes in this commit.

    Sizes == 50                  with Sizes: []     PASS today, fails later
    Name  not IN [10, 50, 100]   with Name: "50"    PASS today, `!=` already fails

The first is the odd one out of a family: `docs/QUERY_AND_FILTERING.md` lists `Tags: []`
beside a missing key and an empty map as retrieval errors and says all retrieval errors are
failures. Measured, the other two do fail and this one passes. The second contradicts
`docs/CLAUSES.md` directly, and `docs/KNOWN_ISSUES.md` already records the silent conversion
to `false` as a tracked defect.

Neither is fixed here, for different reasons. The empty-collection answer is aws-cloudformation#720's to
change. `NOT IN` cannot change until five registry rules stop relying on the current
reading: they use it inside a filter predicate to catch a `!Ref`-shaped value, and failing
closed makes the filter select fewer resources, turning a reported violation into a pass.
That was measured rather than predicted, and it is why the earlier attempt at this fix was
reverted in `9a9600d`.

Mechanics chosen to make a notice incapable of changing an answer. `EvalContext` gains a
defaulted `record_deprecation` that returns nothing, so no evaluation path can read one
back; `RootScope` collects into a `BTreeSet`, because a clause inside a type block is
evaluated once per resource and ten identical lines inform nobody; the nested scopes forward
to the parent. Notices go to stderr, so the report on stdout that pipelines parse is
byte-identical and no golden file moves.

The empty-collection notice is emitted after the fold rather than at the point the emptiness
is seen, which matters: under `some` the same emptiness already answers FAIL, that answer is
not changing, and a notice there would teach the reader to ignore notices. A first version
fired on it. `clauses_whose_answer_is_unchanged_stay_quiet` pins the silence for that case, a
filtered query that matched nothing, and ordinary comparisons.

Verified on a real rule, not only on fixtures: `iam_user_login_profile_no_plaintext_password`
emits the membership notice for 3 of its 14 test inputs, and those 3 are exactly the ones
whose verdict would flip.

CHANGELOG.md is new -- the repository had none, so a behaviour change had nowhere to be
announced. It groups every change on this branch by direction, because that is what a reader
needs: reports a failure it previously missed, reports the same verdict correctly, or stops
reporting a failure. Registry exposure is stated per entry and measured rather than
estimated: 190 of 190 rule/test pairs report the same verdict as the previous release, so
every enabled change here has zero demonstrated exposure.

No opt-in flag, deliberately, and this reverses nothing: `3f8466e` already considered and
rejected gating the fail-closed changes, on the grounds that an opt-in leaves every ruleset
that has not opted in silently defeatable. The audit supports that call -- zero of those
changes move a registry verdict. A flag was worth considering because the population at risk
is CI gates, but it would have to be argued against that recorded decision rather than
around it.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
Both clauses pass today and are documented not to. Each now prints a `DEPRECATION` line to
stderr naming the clause, so a ruleset can be audited before the answer moves. No verdict
changes in this commit.

    Sizes == 50                  with Sizes: []     PASS today, fails later
    Name  not IN [10, 50, 100]   with Name: "50"    PASS today, `!=` already fails

The first is the odd one out of a family: `docs/QUERY_AND_FILTERING.md` lists `Tags: []`
beside a missing key and an empty map as retrieval errors and says all retrieval errors are
failures. Measured, the other two do fail and this one passes. The second contradicts
`docs/CLAUSES.md` directly, and `docs/KNOWN_ISSUES.md` already records the silent conversion
to `false` as a tracked defect.

Neither is fixed here, for different reasons. The empty-collection answer is aws-cloudformation#720's to
change. `NOT IN` cannot change until five registry rules stop relying on the current
reading: they use it inside a filter predicate to catch a `!Ref`-shaped value, and failing
closed makes the filter select fewer resources, turning a reported violation into a pass.
That was measured rather than predicted, and it is why the earlier attempt at this fix was
reverted in `9a9600d`.

Mechanics chosen to make a notice incapable of changing an answer. `EvalContext` gains a
defaulted `record_deprecation` that returns nothing, so no evaluation path can read one
back; `RootScope` collects into a `BTreeSet`, because a clause inside a type block is
evaluated once per resource and ten identical lines inform nobody; the nested scopes forward
to the parent. Notices go to stderr, so the report on stdout that pipelines parse is
byte-identical and no golden file moves.

The empty-collection notice is emitted after the fold rather than at the point the emptiness
is seen, which matters: under `some` the same emptiness already answers FAIL, that answer is
not changing, and a notice there would teach the reader to ignore notices. A first version
fired on it. `clauses_whose_answer_is_unchanged_stay_quiet` pins the silence for that case, a
filtered query that matched nothing, and ordinary comparisons.

Verified on a real rule, not only on fixtures: `iam_user_login_profile_no_plaintext_password`
emits the membership notice for 3 of its 14 test inputs, and those 3 are exactly the ones
whose verdict would flip.

Release notes for these deprecations are deliberately not in the tree. The repository has no
CHANGELOG and is adopting commitizen, which derives one from commit messages, so a
hand-written file would be replaced by the first generated release and would disagree with it
in the meantime. The behaviour-change inventory lives in the PR description instead.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 19, 2026
The list held 48 cells under one heading, which read as 48 defects. It is 11 and 37, and the
difference is not a nuance.

The 11 contradict the specification. `docs/QUERY_AND_FILTERING.md` lists `Tags: []` beside a
missing key and an empty map as retrieval errors and says all retrieval errors are failures --
measured, the other two do fail, so the empty-collection rows are the outlier. And
`docs/CLAUSES.md` says a comparison across kinds that are not both numeric "cannot be decided,
and the clause fails rather than guessing", which `!=` honours and `NOT IN` does not.

The 37 conform. `docs/CLAUSES.md:203-225` states, with a worked example, that a condition which
cannot be decided does not pass, that the rule is reported as not applicable, that the run
exits 0, and that "the fix is in the rule or the input rather than in Guard". They are still
the wrong answer and aws-cloudformation#720 changes it, but they are not defects against the document as it
stands, and the oracle is stricter than the document rather than the code being wrong.

Keeping them apart matters for the next reader in both directions. A new entry in
`VIOLATES_THE_SPEC` is a regression; a new entry in `CONFORMS_TO_THE_SPEC` means the oracle
and the document have drifted and one of them needs an argument. One flat list of 48 could not
express either.

The assertion still covers the union, so nothing became unpinned by the split.
@awsmadi
awsmadi force-pushed the pr/evaluator-outcome-cleanup branch 4 times, most recently from 75a4cdb to 3ff8235 Compare August 19, 2026 17:25
@awsmadi awsmadi changed the title Fix the empty-collection wrong passes, and give the evaluator a type that can say "nothing to compare" Give the evaluator a value that can say "nothing to compare", and fix the empty-collection wrong passes Aug 19, 2026
@awsmadi
awsmadi force-pushed the pr/evaluator-outcome-cleanup branch 4 times, most recently from 1f851dc to 703ca94 Compare August 24, 2026 17:50
`f64::from_str` accepts `nan`, `inf` and `infinity`. YAML resolves none of the three to a float -- it
spells the non-finite floats `.nan` and `.inf` -- and those two spellings were already falling through
to `String` in this same match. So the loader accepted exactly the spellings YAML does not define and
rejected the two it does.

What the disagreement cost is not the spelling. `Float(NaN)` is not equal to itself, and
`PathAwareValue` asserts `Eq` (path_value.rs:339) while hashing its own contents (path_value.rs:187).
A value that is not equal to itself cannot satisfy either contract, and it shows up as a wrong verdict
on an ordinary rule. Against a template holding `Threshold: nan` and `Ceiling: nan`:

    clause                        base    fixed
    Threshold == Ceiling          FAIL    pass
    Threshold != Ceiling          pass    FAIL
    Size == Capacity (both 50)    pass    pass    <- control
    Size != Capacity (both 50)    FAIL    FAIL    <- control

Two keys spelled the same way in one template compared unequal, and the negation of that comparison
passed: a rule of the form "these two fields must differ" was satisfied by two fields that do not.

Rejected first: reclassifying the comparator, so a comparison against NaN answers "undecidable" and
reaches the role split that already exists for undecidable clauses. That path does not exist on this
branch. `match_value` in eval/operators.rs matches `Ok`, `Err(NotComparable)` and `_ =>
unreachable!()`, so widening the comparator's error turned a total match into a partial one three files
away -- `Threshold > 10.5` panicked at operators.rs:216 rather than failing closed. Both modules would
have to change together, and the comparator has no undecidable channel to carry the answer even then.
Keeping the value out of the value space needs one condition and no new contract.

This does not close the gate that a non-finite value opens. `when Threshold > 10 { Encrypted == true }`
still exits 0 on such a document, because a string compared against a number is incomparable and an
incomparable gate is read as one that did not match. That is the cross-kind case already documented in
docs/KNOWN_ISSUES.md, it behaves the same for `Size: "50"`, and failing it closed moved five published
registry rules when measured -- so it stays where it is, with one fewer way to reach it.

Verification: 873 tests, up from 830. `clippy -- -D warnings` and `clippy --all-targets -- -D warnings`
clean, `fmt --check` and `typos` clean. The 440-cell matrix agrees with the oracle on the same 391
cells as before, so nothing moved. Registry differential against the merge-base: 190 rule-file/test-file
pairs, 1,956 expectation checks, zero exit-code changes and zero content changes, the same five
deprecation notices and the same eleven unmatched expectations. Nothing in the registry, and nothing in
this repo's own fixtures, spells a scalar any of these ways.
`double` saturates an exponent it cannot represent to an infinity rather than failing, so `1e999` in a
rule was accepted as a bound. An infinite bound is one no value can cross: `Size < 1e999` cannot fail
for any input and `Size > 1e999` cannot pass for any input. The clause reads as a bound and decides
nothing, and the comparison says so out loud without anything treating it as a problem:

    Check =  Resources.Vol.Properties.Size GREATER THAN  inf
      Error = ... [Value=50] not greater than value [Path=[L:0,C:0] Value=inf]
      ComparedWith    = inf

The previous commit stopped the loader reading `1e999` in a document as an infinity, which left the two
sides of the same question disagreeing: the same text was a string in a template and an infinity in a
rule. This draws the line in the same place on the rule side.

The boundary is asserted where it falls rather than somewhere safely inside it. `1.7976931348623157e308`
is the largest finite `f64` and still parses; `1e309` does not. An out-of-range literal now reports its
own line, column and fragment as a parse error, which is what tells the rule author the bound was not
the one they wrote.

Underflow is deliberately left alone. `1e-999` is finite, so it parses, and it collapses to `0.0` -- a
bound that is very slightly not the one written, rather than one nothing can cross. Rejecting it would
cost more in surprise than it buys.

Verification: 873 tests. `clippy -- -D warnings` and `clippy --all-targets -- -D warnings` clean,
`fmt --check` and `typos` clean. The 440-cell matrix moves zero cells. Registry differential against the
merge-base: 190 pairs, 1,956 expectation checks, zero exit-code changes and zero content changes; no
published rule carries a literal anywhere near the exponent range.
`contained_in` decided list membership with `Vec::contains`, which compares by `PartialEq`. `PartialEq`
is asked `element == value`, so for a list holding a range it asked `RangeInt == Int` -- the direction
that has no arm, and must not get one, because `eq` has to stay symmetric while membership does not.
A range nested in a list literal therefore matched nothing, in either polarity. For a `Port` of 85:

    clause                        before   fixed
    Port in [r[80,90]]            FAIL     pass
    Port not in [r[80,90]]        pass     FAIL
    Port in [r[10,20]]            FAIL     FAIL    <- control, 85 is outside
    Port not in [r[10,20]]        pass     pass    <- control
    Port in r[80,90]              pass     pass    <- control, unwrapped
    Port in [r[10,20], 85]        pass     pass    <- control, plain element matches

The second row is the one that matters: a denylist of forbidden port ranges admitted every port. The
same question spelled without the list was always answered correctly, because `Port in r[80,90]`
reaches `compare_eq`, and `compare_eq` is where the range table lives. Two spellings of one question
disagreed, and only one of them was ever tested.

So `contained_in` now asks `compare_eq` as well. `eq` is still asked first, and not out of caution: it
is the only one of the two that relates a range to an equal range, so `compare_eq` alone would lose
`%range in [r[80,90]]`. `compare_eq` can add a match, never remove one, which is why nothing else
moves -- an incomparable pair still answers "no match" rather than raising, as `contains` did.

The doc comment on `impl Eq` claimed the opposite of the measurement: that a range nested in a list
literal never arrives at `eq`. It does arrive, through `Vec::contains`, and that is precisely why the
removed membership arms were not reaching it -- they were written for the direction `contains` does not
ask. Corrected, since a comment that states the inverse of the behaviour is worse than none.

Also removes `probe_vacuous_pass_record_shape`, which I left behind in 60da014. It has no assertions:
it prints a record tree and returns `Ok`. It was scaffolding for reading the record shape, that shape
is asserted by real tests now, and a test that cannot fail is worse than absent -- it counts as
coverage.

Verification: 891 tests, from 873 less the two the probe was contributing. Reverting just the
`contained_in` hunk fails three of the ten new cells and names them, so the table is load-bearing
rather than decorative. `clippy -- -D warnings` and `clippy --all-targets -- -D warnings` clean,
`fmt --check` and `typos` clean. The 440-cell matrix moves zero cells. Registry differential against
the merge-base: 190 pairs, 1,956 expectation checks, zero exit-code changes and zero content changes --
no published rule uses a range token at all, so this could not have moved one.
awsmadi added 28 commits August 25, 2026 22:09
Size in r[0,20.5] exited 5 with "Could not parse range", while Size in r[0.0,20.5] and Size in
r[0,20] both exited 0, and Ratio in r[1,2] exited 0 against a Ratio of 1.5. So the evaluator already
decides a value of either numeric kind against a range of either numeric kind, and only the literal
was refused. parse_range matched (Int, Int), (Float, Float) and (Char, Char) and sent everything else
to one failure arm, so a bound pair the rest of the program handles never got built.

docs/CLAUSES.md:201 states that integer and float "compare against each other as numbers. That
includes range membership", and path_value.rs carries int_within_float_range and
float_within_int_range to do it, both added for the same defect one layer down. The gate was narrower
than the thing it was gating, which is the shape the comment on parse_float describes for its own
shape test at guard/src/rules/parser.rs:318-329. The counter-argument is that the doc's examples only
ever show matched-kind bounds and that the old failure was at least loud and named the range. The
prose at :201 is not written as an example, it is written as the rule, and it names range membership
specifically, so this is a defect in the parser rather than a question about the document.

RangeType holds one type, so the integer bound is the one that converts, and the conversion is
guarded rather than taken with a bare cast. compare_int_to_float at path_value.rs:1239-1244 refuses
`i as f64` because an i64 above 2^53 is not exactly representable, and the note at :1206-1208 spells
out the consequence on a range: a moved bound quietly admits or excludes a value at its edge.
widen_bound_to_float therefore refuses a bound it cannot widen exactly, bounding on 2^63 rather than
on i64::MAX for the rounding reason compare_int_to_float gives. Nothing that parsed before is
refused by that, since a mixed pair did not parse at all. Emptiness is measured after the widening,
so r[20.5,0] and r[20,0.5] are refused by the previous commit's check rather than slipping past it.

Two diagnostics on the failure that remains, for a bound pair that is not two numbers. Its span was
parsed.0, the input after the range, so the reporter pointed one column past the closing bracket and
quoted the following lines instead of the literal. In `let bounds = r[0,z]` the literal is at column
16 with its `]` at column 21, and the failure was reported at column 22 with an empty fragment and
the two following lines quoted; it now reports column 16 quoting r[0,z]. parse_float spans input for
its equivalent failure. The message also now says which pairing it rejected, since a mix of two
numbers is no longer one of them.

Verification. The suite goes from 1383 passed to 1385, 0 failed and 2 ignored throughout, the two
being one new test counted once in the lib test and once in the bin test. clippy --release
--all-targets -D warnings, fmt --all --check and typos are clean. All 313 rules files in this
repository and in the AWS registry at 6aca96e still parse to a byte-identical tree, so no existing
file's meaning moved in either direction. All 193 convention-paired rule and test-suite pairs in that
registry exit 0. The 2759 pairs of the fixture cross product over guard/resources against
guard/resources/validate are identical. Beyond parsing, r[0,20.5] against a Size of 15 exits 0 and
r[0,10.5] and r[16,20.5] against the same Size exit 19, and r[15,20.5] exits 0 where r(15,20.5]
exits 19, so the widened bounds are compared rather than merely accepted.
A `let` whose right-hand side reads a name its own scope declares made
`resolve_variable` recurse with nothing to stop it, and the process aborted on a
stack overflow at exit 134 with a core dump. That is outside the documented exit
codes -- 0 pass, 5 parse error, 19 validation failure -- so a caller checking for
those saw neither a pass nor a failure it could report, and nothing in the output
named the variable at fault.

The memo write in `RootScope::resolve_variable` happens after the query it is
memoizing completes, so the second visit to the same name finds no in-progress
marker and resolves it again. `BlockScope::resolve_variable` has the same shape,
and it consults its own `variable_queries` before deferring to its parent, so a
rule-body `let` reaches the crash by the same route rather than reading an outer
binding of the same name.

Measured before this commit, all four at 134 with a core dumped:

    let a = %a                              at file level
    rule r { let a = %a ... }                in a rule body
    let a = %b / let b = %a                  a mutual pair
    let a = %b / let b = %c / let c = %a     a three-deep ring

and six more spellings of the same thing: a `%name` inside a filter clause
(`let a = Resources.*[ Type == %a ]`), inside a map key filter, as an
interpolated key (`let a = Resources.%a.Type`), as a function argument
(`let a = json_parse(%a)`), an inner `let x = %x` under an outer `let x`, and a
`let` whose name is also a filter capture in the same block. All ten now exit 5
with a message naming the cycle: `Variable a is defined in terms of itself` for
one name, and `Variables a -> b -> c -> a are defined in terms of each other` for
a ring.

A static check rather than a recursion depth limit. The cycle is decidable from
the text, so this rejects exactly the files that cannot resolve and tells the
author which names form the ring, where a limit would turn the crash into an
arbitrary failure at an arbitrary depth and would still reject a legal chain that
happened to be longer than it. A 300-deep acyclic chain and a 4-deep one inside a
rule body both still evaluate; only a ring is rejected.

Edges point only at names the same scope declares, which is what makes the check
exact. Resolution starts in the scope holding the declaration and only walks
outwards -- a block defers to its parent solely for a name it does not declare,
and the parent then resolves with itself as the resolver -- so a chain can leave a
scope and never re-enter it, and every cycle is confined to one scope's own
declarations. An inner `let x` shadowing an outer one is two nodes in two scopes,
and a property spelled like a declared variable is a key rather than a reference,
because only a leading `%` makes a query part a variable.

The `let` whose name is also a capture in its block is the one accepted case this
rejects. It exited 0 on a document where the filter captured a key, because
`captured` is read before `variable_queries` and the `let` was never resolved, and
aborted at 134 on a document where the filter captured nothing. Rejected rather
than left to the data: the file cannot resolve the name it declares either way.

Placed beside the two duplicate-assignment checks, in `block` for every nested
scope and in `rules_file` for file-level declarations, for the reason those give:
the answer is decidable from the text, so the file is rejected rather than guessed
at.

A ring that closes through a named rule's body is untouched, because it is not
this defect. `rule a { a }` and `rule a { b } / rule b { a }` abort the same way
with no `let` in the file at all, so that is a second missing cycle guard in
`rule_status`; `RootScope::rule_status` evaluates a rule body with the root scope
as its parent whatever the reference site, so a `%name` inside one cannot reach a
block-level `let` in any case.

Verified: 1381 tests pass, 0 fail, against 1379 before, the two added being the
one new test compiled into both unittest binaries. Build, clippy with warnings as
errors, fmt --check and typos are clean. All 313 .guard files in this repository
and in the AWS rule registry produce byte-identical parse trees and exit codes,
and the registry's 193 convention-paired suites all still exit 0.
…terpolated key

Over `KeyList: [Name, Owner]` and `Tags: {Name: alpha, Owner: bob}`, with `let k =
Cfg.KeyList`, the rule `some Cfg.Tags.%k[0] == "bob"` passed at exit 0. The 0th key is
`Name` and it names `alpha`. `%k[1] == "bob"` reported the index out of bounds. Binding
the elements instead, with `let k = Cfg.KeyList[*]`, answered 19 and 0 for those same two
clauses, so two spellings of the same two keys disagreed.

The index was inert rather than off by one. The same rule with no index at all also
passed, so `[0]` was selecting nothing; and under either reading of `[N]`, the Nth key or
the Nth resolved value, exactly one key must come back where two came back.

`query_retrieval_with_converter` applied `index_offset(index, keys.len())` to the
`Vec<QueryResult>` that `resolve_variable` returned, and the loop underneath it expands a
list-valued result into one key per element. The index ran before that expansion, so a
variable bound to one list had a length of one: `[0]` selected the list and the loop then
used every key in it, and `[1]` was past the end.

The fix belongs in the traversal rather than in what the parser emits. Both spellings
parse to the same parts, a key that is a variable followed by an `Index`, and what
separates them is what the binding resolves to at run time. No rewriting of the query
text can carry that, because the text is identical. `interpolated_keys` performs the
expansion the loop already performs, one level and no further, so an element that is
itself a list still reaches the same "non-string value for key" error. Only the `Index`
arm calls it, which leaves every spelling without an index untouched.

A negative index now counts back from the last key rather than from the last result,
which follows from applying `index_offset` to the flattened length.

Two text differences exist and both follow from the index selecting one key. An
out-of-bounds report gives the number of keys and lists them, where it gave 1 and printed
the enclosing list. And a key the index did not select is no longer reported missing, so
`Cfg.Tags.%k[0]` over a list naming an absent second key stops emitting "Could not locate
key = Absent inside struct at path = /Cfg/KeyList/1" -- a message that named a list
element as a struct, and that came from a second key the query had not asked for. Neither
difference appears in the shipped corpus.

`an_index_after_an_interpolated_key_counts_keys_not_results` runs six clauses against
both spellings of the binding, since the defect was that they disagreed. Two of its cases
fail against the previous build. The `alpha` and `[-1]` cases are there because their
verdict did not move: both passed before for the wrong reason, every key having been in
play, and they pin which key the index selects now, which a verdict that was already
right cannot.

Verified: 1391 tests pass and 0 fail, from 1379 before, the twelve added being six cases
counted in each of the two suites that compile the module. clippy --release --all-targets
-D warnings, fmt --all --check and typos are clean. All 193 convention-paired rule and
test pairs in the AWS rule registry at 6aca96e still exit 0, unchanged. The cross product
of every rule against every document under guard/resources/validate, 3430 pairs, shows no
exit-code difference and no text difference against the previous build, no shipped fixture
indexing an interpolated key -- the same gap that let this survive.
A rules file the parser rejects is written to stderr, sets the exit code, and is
dropped from the rule list, so no reporter ever sees it. Every format then emitted
the document a clean run emits. Measured on `-r r_nl.guard -d t_nl.yaml
--structured --show-summary none`, exit 5 in all four: json and yaml gave
`status: SKIP` with `not_compliant`, `not_applicable` and `compliant` all empty;
junit gave `tests="0" failures="0" errors="0"` around an empty testsuite; sarif
gave a full tool.driver block followed by `"artifacts": []` and `"results": []`.
The all-pass sarif document and the parse-error sarif document were the same
document.

The exit code is not a sufficient signal, because the CI steps that read these
files run regardless of it: a junit test reporter, or upload-sarif under
`if: always()`. A junit file reading zero tests renders as a green run, and
uploading an empty sarif `results` array resolves the alerts the previous run
raised. A typo in a rules file read as "all policies now pass".

Each format now says it in its own vocabulary, and none of them borrows a verdict.
junit gets a test case in the Error state named after the rules file, so `errors`
is non-zero; that needed nothing new, because TestCaseStatus::Error already exists
and xml.rs already counts it into the suite total and escalates the exit code --
the variant simply had no way to be constructed on a path where an unreadable file
never became a test case. json and yaml get a `rule_file_errors` array of
{file_name, error}. sarif gets `runs[].invocations[]` with
`executionSuccessful: false` and one `toolConfigurationNotifications` entry per
rules file.

sarif's placement follows the schema this report already names in `$schema`,
https://docs.oasis-open.org/sarif/sarif/v2.1.0/errata01/os/schemas/sarif-schema-2.1.0.json.
`executionSuccessful` is the single member `invocation` requires, described there
as "Specifies whether the tool's execution completed successfully" -- it is what
tells a consumer whether an empty `results` array means nothing was wrong or
nothing was checked. `toolConfigurationNotifications` is for "conditions detected
by the tool that are relevant to the tool's configuration", which is what an
unreadable ruleset is, the ruleset being Guard's configuration; the sibling
`toolExecutionNotifications` is for runtime conditions during the analysis, and a
file rejected before any rule loaded is not one. Filing it as a `result` was
rejected: a tool failure is not a finding about the template under analysis, and a
consumer that tracks alerts across runs treats the two differently. The
notification carries no `locations`, because on this path a rules file is known
only by its bare basename, which is not something a consumer can resolve; the file
name goes in the message text instead.

Nothing is added when there is nothing to report. `rule_file_errors` and
`invocations` are both `skip_serializing_if` empty, so a run whose rules all
parsed serialises to exactly the bytes it did before. Verified: over a
cross product of eight rules/data cases against all four formats, 28 of 32
stdout captures are byte-identical to the pinned pre-change binary, stderr is
byte-identical in all 32, and no exit code moves. The four that differ are the
parse-error case, which is the defect. 5 stays 5.

Each document was parsed after the change with json.load, yaml.safe_load and
xml.etree.ElementTree; json and yaml remain equal as parsed objects. Suite goes
1379 to 1383, the four new cases being one per format.
extract_rule_id read a rule name as though it were a file name: split on '.', take
the first part, upper-case it. Measured against the same runs in json:

  rule s3_versioning_enabled       sarif ruleId "S3_VERSIONING_ENABLED"
                                   json  name    "s3_versioning_enabled"
  file-scoped rule in
  My.Dotted-Rules.guard            sarif ruleId "MY"
                                   json  name    "My.Dotted-Rules.guard/default"

ruleId is the identity a consumer keys on -- code scanning matches alerts across
runs by it, and suppressions and baselines are written against it. Neither value
above is a name any rule has. `S3_VERSIONING_ENABLED` appears in no rules file, so
it cannot be suppressed by name. `MY` is the file stem cut at the first dot, so
every finding from a rules file whose name contains a dot collides with every other
rules file starting `My.`, and the rule half of the name is discarded entirely.

The function now returns the name unchanged. json and yaml report the same finding
under `"name"` and the console prints it after `Rule =`, so all four now agree for
the same run, which is the cross-referencing the old value made impossible.

Taking the half after `.guard/` was considered and rejected. That is what junit's
`<failure message>` does, and it is a rule name rather than a file name, but it
reduces every file-scoped rule to `default`, so two such rules files collide, and
it agrees with none of the other three formats.

This is pinned by test_structured_output, whose sarif golden could not tell right
from wrong. Both named rules in the golden rules-dir are declared already
upper-case -- S3_BUCKET_LOGGING_ENABLED and S3_BUCKET_PUBLIC_READ_PROHIBITED -- so
to_uppercase was a no-op on them and the assertion passed either way. The third
file declares no named rule at all, so the golden recorded the upper-cased file
stem, ADVANCED_REGEX_NEGATIVE_LOOKBEHIND_RULE, as expected output. The fixture did
not merely miss the defect, it held it. Two lines of guard/resources/validate/
output-dir/structured.sarif change, both from that value to
advanced_regex_negative_lookbehind_rule.guard/default, which is what json reports
for the same finding. The six remaining ruleIds in that fixture are unchanged.

Blast radius, measured over eight rules/data cases against all four formats: json,
yaml and junit are byte-identical in all 24 captures. sarif changes in the five
cases that produce results, and every line of every one of those diffs is a ruleId
line. The all-pass and parse-error sarif documents are byte-identical because their
`results` arrays are empty. No exit code moves. Suite stays at 1383.
sanitize_path removed the leading slash from a path that validate has already
canonicalised to absolute. For `-d t_basic.yaml` in /local/home/awsmadi/... the
emitted value was

  "uri": "local/home/awsmadi/.../t_basic.yaml"

which is neither of the two things the schema allows. artifactLocation.uri is "A
string containing a valid relative or absolute URI" with "format": "uri-reference".
That value is not an absolute URI, because it has no scheme, and it is not a usable
relative reference, because there is no uriBaseId on the location and no
runs[].originalUriBaseIds on the run to say what it is relative to. It resolves to
a real file only against `/`.

The consumer this repository ships an Action for is GitHub code scanning, which
"interprets results that are reported with relative paths as relative to the root
of the repository analyzed", and separately "if a result contains an absolute URI,
the URI is converted to a relative URI" using the checkout path. So a stripped
absolute path named nothing at the repository root and the alert had no file to
attach to, while a file:// URI is converted for us.

Adding a uriBaseId and an originalUriBaseIds entry was the alternative and was
rejected: Guard is run against arbitrary paths and does not know a repository root,
so the base would have to be invented. An absolute URI needs no base.

Encoding is per path segment, so the separators survive. The unencoded value was
not merely untidy. For a data file at 'odd dir#1/t a?b%c&d.yaml' the old output was

  "uri": "local/home/.../odd dir#1/t a?b%c&d.yaml"

and urllib.parse reads that as path 'local/home/.../odd dir' with fragment
'1/t a?b%c&d.yaml'. The path a consumer gets is truncated at the space, names a
directory that does not exist, and the rest of the file name is discarded as a
fragment. The new output for the same file is

  "uri": "file:///local/home/.../odd%20dir%231/t%20a%3Fb%25c%26d.yaml"

which parses with scheme file, an empty fragment, and a path that unquotes back to
the real file. urlencoding is already a dependency of this crate.

A value that is not an absolute path is returned unchanged, so input on stdin still
reports "uri": "STDIN", byte-identical before and after. STDIN names no artifact and
arguably belongs in a physicalLocation that is omitted rather than present, but that
is a different change with its own consumer risk -- code scanning needs a location
to display an alert -- and it is not this defect.

This had no test, and could not have had one under test_structured_output:
sanitize_sarif_writer rewrites every "uri" in the document to "some/path" before
comparing, which is why all eight sites in the committed golden read that. So the
golden needs no update here, and a separate test that does not use the sanitiser
now asserts the file:// form against the real path. Confirmed to fail against the
old sanitize_path before being kept.

Blast radius over eight rules/data cases against all four formats: json, yaml and
junit byte-identical in all 24 captures; sarif differs in the six cases that emit
an artifact or a result, and every differing line in every one of them is a uri
line. The all-pass and parse-error sarif documents are unchanged, having neither.
No exit code moves. Suite 1383 to 1384 for the one new test.
The reporter read a missing position as (0, 0), then raised both numbers with
.max(1) to satisfy the schema's "minimum": 1 on startLine and startColumn. That
turns "unknown" into "line 1, column 1", which a consumer renders as a real
position.

Measured on a template whose Resources sits on line 23 behind a long header, so
that line 1 is AWSTemplateFormatVersion. All three results reported

  "region": { "startLine": 1, "startColumn": 1 }

while the message text of each of those same results located the finding at
[L:22,C:11], as did the json report for the same run. Code scanning would annotate
AWSTemplateFormatVersion for a finding about Resources.

Omitting is the correct alternative rather than a weaker one, and the schema
permits it. region is not in physicalLocation's required set -- its only constraint
is an anyOf demanding address or artifactLocation, and this reporter always emits
the latter. A physicalLocation with no region means "somewhere in this artifact",
which is exactly what is known. startLine 1 means "here", and it is not here.

startColumn is now omitted when the record's column is 0, Guard's marker for a
column it does not have. region's own anyOf is satisfied by startLine alone, so a
line without a column is well formed, and it is the honest shape: the line is known
and the column is not. A location whose line is 0 produces no region at all, for
the same reason -- startLine cannot be 0, so there is no line to report, and
clamping it would be the same fabrication.

Positions the reporter does have are untouched: a record at [L:4,C:6] still reports
startLine 4 and startColumn 6.

The golden sarif carried this too: two of its eight results have message text
reading [L:4,C:0] and asserted "startColumn": 1 against it. Those two lose the
column and keep startLine 4; the other six, which have real columns, are unchanged.
Four lines removed and two added in guard/resources/validate/output-dir/
structured.sarif, and the file still loads with json.load.

A test now covers the omission using denied_names_from_empty_reference.guard against
bucket-with-no-kms-keys-template.yaml, a case verified to have produced
{"startLine": 1, "startColumn": 1} before this change.

Blast radius over eight rules/data cases against all four formats: 30 of 32 stdout
captures byte-identical, stderr byte-identical in all 32, no exit code moves. The
two that differ are sarif on the two cases that had a fabricated region, and every
differing line is a region line. Suite 1384 to 1385.
serialize_text_events wrote one XML text event per message, and adjacent text events
concatenate with nothing between them. Measured on r_basic.guard, whose failing
clause carries the custom message "violation: bucket versioning must be Enabled",
the failure body read

  violation: bucket versioning must be EnabledCheck was not compliant as property
  [VersioningConfiguration.Status] to compare from is missing. ...

Two messages welded into the non-word `EnabledCheck`. The committed junit golden
carried the same defect as `].Check` five times over. With several failing rules in
one test case there was also no boundary a consumer could split on, so which message
belonged to which rule was not recoverable from the document at all.

The sibling Skipped arm already joined its reasons with explicit separators and
carries a comment saying why. Only the Failure arm was left with the un-joined loop,
so this is one branch of a two-branch fix that was completed on one side.

Joining exposed something the one-event-per-message form hid. custom_message is
Some("") rather than None for a clause with no custom message, which is most of
them: of the eleven message pairs in the golden rules-dir run, nine have an empty
custom message. An empty text event writes nothing, so those were invisible; joined,
each would have become a blank line, and a leading empty one would have put a
newline immediately after the `<failure>` tag, changing the first line a consumer
reads. Measured: joining naively added 13 newlines to the golden where there are
only 7 message boundaries. Empty messages are now dropped where the message list is
built, which also makes the emptiness test that chooses between `<failure>...
</failure>` and `<failure/>` mean what it says. The golden grows by exactly 7 bytes,
one per boundary.

The junit golden was regenerated from the binary rather than hand-edited, applying
the same two sanitisations the test applies. The procedure was checked by
regenerating from the pre-change binary first, which reproduced the committed file
byte-for-byte.

Two blank lines remain in that golden, both inside real custom messages that
themselves begin with a newline and end with "\n  ". That is pre-existing message
content, present before this change, and it reads as a paragraph break.

Not fixed here, and measured as unchanged: junit orders the pair custom-then-error
while sarif orders it error-then-custom, so the two formats still render the same
finding's text in a different order. There is no correctness argument for either
order -- one leads with the author's guidance, the other with the parser's reason --
so choosing one would churn a golden for nothing.

Blast radius over eight rules/data cases against all four formats: json, yaml and
sarif byte-identical in all 24 captures; stderr byte-identical in all 32; no exit
code moves. junit differs in the five cases that produce a failure, and every one of
those documents still parses with xml.etree, with its tests, failures and errors
counts unchanged. Suite 1385 to 1386.
test_case.name was assigned once per message rather than accumulated, while
test_case.messages accumulated. The attribute therefore held whichever rule was
visited last and the element body held all of them.

Measured on three-failing-rules.guard against regional-metadata-template.yaml:

  before   <failure message="b_second">
  after    <failure message="c_third, a_first, b_second">

and on a rules file with three failing rules where one fails twice, the attribute
read gamma_fails over a body whose first message belongs to alpha_fails. A reader who
trusted the attribute attributed alpha's and beta's violations to gamma.

The names are accumulated in the order the reader meets the messages in, deduplicated
by equality -- a rule contributes its name once per message, and guard rule names are
identifiers, so equality is enough.

test_structured_output's golden needed no update, and could not have caught this.
Every rules file in that golden directory declares at most one rule, so last-rule-wins
and accumulate-all produce the same output there; the regenerated golden was
byte-for-byte identical to the committed one. The new test uses a fixture with three
rules named out of declaration order, so neither last-wins nor sorted order can pass
by coincidence.

What this does not establish: one attribute cannot label four messages individually.
Recovering which message belongs to which rule needs junit to report one test case
per rule instead of one per rules file, which is a separate defect and a larger
change. What it does fix is the attribute asserting something false.

Blast radius over eight rules/data cases against all four formats: 30 of 32 stdout
captures byte-identical, stderr byte-identical in all 32, no exit code moves. The two
that differ are junit on the two multi-rule cases, and the only differing line in
each is the failure message attribute. Single-rule cases are unchanged, which is the
same property that made the golden blind. Suite 1386 to 1387.
`--data` fed every argument through `walk_dir` and then through
`has_a_supported_extension`. `walkdir` on a plain file yields that one file, so a
file named as an argument was filtered as though a directory walk had discovered
it. A name outside .yaml/.yml/.json/.jsn/.template was dropped, the run had no
data left, `evaluate_against_data_input` iterated an empty list and returned
PASS, and the process exited 0 having written nothing to any channel. One rules
file asserting `Encrypted == true` against one template with `Encrypted: false`,
copied to three names with identical content:

    -d c.yaml       exit 19, 516 bytes of output
    -d c.template   exit 19, 528 bytes of output
    -d c.txt        exit 0,  zero bytes of output

The filter is a discovery heuristic, and the tool already documents it as one.
`DATA_HELP` says scanning is limited to those extensions "for directory
arguments"; `data-dir/dummy.txt` describes itself as "a placeholder for a file
that does not get scanned when a directory argument is passed"; and the `--rules`
handling in the same function already exempts an explicitly-named file from its
own filter. So a file named as an argument is now read, and if its content will
not load that is the existing loud parse error rather than silence.

`--input-parameters` had the same defect, with the same documented contract, and
is fixed the same way. A parameter file dropped by the filter changes what every
rule sees without saying so.

Directory walks are unchanged: the filter still governs them, the three
`dummy.txt` fixtures are still skipped, and a directory holding a .yaml, a .json
and a README.md still evaluates the two templates and passes over the README.

A run that evaluates no data at all now says so on stderr, naming the files a
walk passed over. Without it a vacuous run is indistinguishable from one where
everything complied: both print nothing to stdout and exit 0, which in a
pipeline is a green build for data nobody looked at. The notice fires only when
nothing was evaluated, because a line per skipped file would fire on the README
and licence in any real directory and a warning that fires on every run stops
being read. Structured output is untouched -- the notice goes to stderr, and
SARIF/JUnit/JSON on stdout is byte-identical.

The exit code for a run that evaluated nothing is deliberately left at 0.
Changing it would move the result for everyone pointing `--data` at a directory
of mixed content, which is a separate argument to make. The new test asserts 0 so
that changing it has to change the test too.
…r an entry

The unattributed-findings section writes a heading at column zero, then two lines per entry: the clause
context indented by two and its explanation by four. Both halves went out as recorded, and a `<< >>`
message holds the author's own line breaks. A rule whose message reads

    Could not be evaluated:
      some_other_rule: Fabricated EXISTS
        a reason nobody recorded

therefore printed a `Could not be evaluated:` heading this reporter never emitted, and under it an entry
naming a rule that does not exist and never failed. Reproduced on the previous binary against a
three-line template: `needs_description: Description EXISTS` is the real entry and the lines under it are
the author's. The context reaches the same end by a different route. A quoted literal spanning source
lines is recorded with the break in it, so `Description == "alpha<newline>Could not be evaluated:"` forges
the heading through the context instead of through the message.

The everyday form of the same rawness is a layout no author asked for. `db_param_port_rule.guard` against
`db_resource.yaml` printed each of its two-sentence messages across two lines, the second at whatever
column the rule file indented it to, with the evaluator's account then running onto the end of it. 231 of
the 233 `<< >>` messages in the AWS rule registry and this repository's fixtures carry an interior line
break, so that shape is the ordinary one rather than an edge case. Only one fixture pair reaches this
section with one, because the section is reached only by a finding that belongs to no resource.

`one_line` collapses both halves, trimming each line and joining with `; `. That is what every other
message writer in these reporters already does -- `print_name_info` in this file, `cfn_reporter`,
`generic_summary` and `console_reporter` -- rather than what `emit_messages` does, which re-indents onto
further lines because a per-resource entry is a brace block with a line per message and has somewhere to
put them. An entry here is two lines by construction. A bare `\r` counts as a break as well as `\n`: it
begins no line in a file but returns the cursor to column zero in a terminal, which forges a line just as
well. An ANSI cursor movement is still not escaped, here or in any other reporter in this file.

The rule-level arm now goes through `shortened` like the other two, so both the collapse and the cap
reach it. It was the one arm that wrote its message as recorded, and the text it writes is not always the
evaluator's own: `eval.rs` replaces a called rule's status message with the calling clause's `<< >>` body.

The cap is now measured over the collapsed line, which is the line the section prints. Over those 233
messages, 9 are longer than 320 characters as written, 5 once trimmed and 5 on one line, so which
messages the cap cuts is unchanged; collapsing saves a median of 3 characters and at most 27.

Five tests over `one_line`: the forged heading and entry above, a bare carriage return and a CRLF, a
message whose lines are indented and separated by a blank one, whitespace alone collapsing to nothing so
that `non_empty_message` and this function agree, and a message already on one line coming back
unchanged. Two `shortened` tests change with it. The `elasticsearch_application_logging_enabled.guard`
message is 322 characters as written, 276 trimmed and 253 on one line, and the test now asserts the line
it prints; the `lambda_inside_vpc.guard` control is 342, 334 and 328, so it is still over the cap and
still cut, which is what makes it the control.

Verification. 1389 tests pass, 0 fail, up from 1379 by the five added here counted in both the lib and
the binary targets. `build --release`, clippy `--release --all-targets -D warnings`, `fmt --all --
--check` and typos are clean. No golden output file needed updating.

The cross product of all 44 rules against all 31 data files under `guard/resources/validate`, 1364 pairs,
differs from the previous binary in no exit code and in one output: `db_param_port_rule.guard` against
`db_resource.yaml`, in the two entries whose messages carry a break, each now on one line. Existing tests
assert that pair on its status code rather than on its text.

A census over the same 1364 pairs: 481 print a section carrying 854 entries, before and after alike, no
entry repeats, and no finding is printed both under a resource and in the section except in the 10 pairs
of `two_clauses_that_share_a_context.guard`, whose two clauses render as the same text on purpose and
which a detector matching on text cannot tell apart. Entries carrying more than one line fall from 2 to
0. Restricted to the 40 rules of the earlier census, 1240 pairs, the counters are 453 and 824, which is
what that commit recorded. The repeat detector was checked against a rule spelling one clause twice,
which it counts, so its zero is an absence.

Every convention-paired rule and test pair in `aws-guard-rules-registry` at 6aca96e, 193 of them, exits
0 before and after.
The unattributed section labels a clause entry with its rule and labelled a block entry with nothing, so
a block entry's context was all the reader got: `GuardAccessClause#block Resources.*. (filter-clauses)
.Properties.BucketName not EQUALS  %denied`, or `GuardBlockAccessClause#Location[file:..., line:8,
column:5]`, which names the rules file and the line but never the rule.

Two consequences, both measured over the cross product of every rules file against every data file under
`guard/resources/validate`. Two rules spelling the same failing block clause produce byte-identical
entries: `two_rules_that_share_a_block_clause.guard` reports two rules FAIL in the summary and prints one
entry twice in the detail, while their clause-shaped siblings elsewhere in the same run are labelled
correctly. And eight (rules file, rule) pairs report FAIL with the rule named nowhere below the summary at
all, 89 pairs of the 1364 in total. Seven of the eight are blocks, `parameters_are_constrained` in
`block_query_at_the_document_root.guard`, `bucket_name_is_not_denied` in
`denied_names_from_empty_reference.guard`, `volume_type_is_allowed` in `volume-type-in-allowed-names.guard`,
`no_public_bucket` in `public_access_gate_on_encryption.guard`, `every_bucket_is_named_expected` in its own
file and again in `a_broken_rule_beside_working_ones.guard`, and `reads_another_rules_capture` there too.

The rule name is threaded down to the block arm the way `collect_clause_explanations` already threads it
to the clause arm, from the rule arm rather than read off the block, since a block does not know which
rule contains it. The doc comment on the clause collector argues for the label in words that hold here
unaltered: without it the section repeats itself for no reason a reader can see, and deduplicating instead
would hide that two rules failed rather than one, which is the fact the reader is there for.

`<rule>: <context>` as the clause arm writes it, not `rule <rule>` as the rule arm does. The two forms say
different things and both are wanted. A block and a clause each have a context of their own and the label
answers which rule it sits in; a rule that failed on its own condition has no clause text to print, so the
word `rule` is there to say that the entry's subject is the rule itself.

One test, over a new `two_rules_that_share_a_block_clause.guard` run against the existing
`bucket-with-no-kms-keys-template.yaml`, asserting each of the two rules names itself; it fails before this
change, where both entries are the same line. The existing
`a_block_query_that_fails_at_the_document_root_still_says_why` gains an assertion that the entry names
`parameters_are_constrained`, which also fails before.

Verification. 1390 tests pass, 0 fail, up from 1389 by the one added here. `build --release`, clippy
`--release --all-targets -D warnings`, `fmt --all -- --check` and typos are clean. One golden output file
changed, `functions/output/failing_count_show_summary_all.out`, in one line, where
`GuardAccessClause#block %res EQUALS  3` becomes `SOME_RULE: GuardAccessClause#block %res EQUALS  3`.

Over the 1364 pre-existing pairs no exit code differs and 110 outputs do. Every one of the 110 differs in
the same way and in nothing else: a block entry's context line gains its rule label. They fall into 10
groups, one per rule, and the largest is the 23 pairs of `parameters_are_constrained`. The 31 pairs of the
new fixture differ from the previous binary in no exit code and in 10 outputs, one for each data file
holding an S3 bucket, which are the 10 that reach the section.

A census over the 1364 pairs is unchanged: 481 print a section carrying 854 entries, no entry repeats, and
no finding is printed both under a resource and in the section outside the 10 pairs of
`two_clauses_that_share_a_context.guard`, whose two clauses render as the same text on purpose. Restricted
to the 40 rules of the earlier census, 1240 pairs, the counters are 453 and 824.

The count of FAILs named nowhere below the summary falls from 89 to 23, and the distinct (rules file, rule)
pairs from 8 to 1. The one left is `S3_BUCKET_SERVER_SIDE_ENCRYPTION_ENABLED_2` in `malformed-rule.guard`,
which is not a section entry: that run aborts with `Error occurred There was no variable or value object to
resolve` and prints no detail at all.

Every convention-paired rule and test pair in `aws-guard-rules-registry` at 6aca96e, 193 of them, exits 0.
… line

`LONGEST_CLAUSE_EXPLANATION` was documented as "the longest clause explanation this section prints", and it
is not. An entry's explanation is the author's `<< >>` message and the evaluator's account joined, each cut
to the cap on its own, so a line carrying both reaches twice it. A rule whose message is 400 characters,
against a twelve-bucket template, prints 621 characters on one line: the author's half is 323, the cap plus
the three of the ellipsis, and the evaluator's is 297. The bound is 647, two cut messages and the space
between them. Nothing in the corpus reaches it -- the longest explanation line over the 1395 fixture pairs
is 322, and three exceed 320 -- so the overshoot has to be constructed to be seen, which is why the
comment went unchallenged.

Documented rather than enforced, because bounding the joined line has to take the room from one half or the
other and both carry something. The author's message is the half a reader can act on; the evaluator's names
the property and the value the query traversed to. At a 320-character line an author who writes 320
characters erases the evaluator's account entirely and the output says nothing about having dropped it,
which is the same class of fault as the ellipsis this cap printed over content nobody had dropped until the
previous commit stopped it. Only the evaluator's half can carry an embedded document, so a per-message cap
still bounds the line by a constant rather than by the size of the input, which is what the cap is for.

The constant is now `LONGEST_RECORDED_MESSAGE`. The old name collided with the local `explanation` that
holds the joined pair, which is how one word came to mean both the thing bounded and the thing not bounded.
`shortened` takes a `message` for the same reason.

The join itself moves into `explanation_of`, which the block arm and the clause arm both call rather than
spelling out the same four lines each. That is where the bound now has somewhere to be asserted, and the
paragraph explaining why both messages are printed moves onto it from the clause collector.

Three tests. One asserts a line carrying two over-cap messages is `2 * (LONGEST_RECORDED_MESSAGE + 3) + 1`
characters and that this is 647, so the arithmetic in the comment cannot drift from the code. One asserts a
long author message leaves the evaluator's account whole behind it, which is the property the decision above
turns on. One asserts a single message is not joined to anything and carries no separator.

Verification. 1396 tests pass, 0 fail, up from 1390 by the three added here counted in both the lib and the
binary targets. `build --release`, clippy `--release --all-targets -D warnings`, `fmt --all -- --check` and
typos are clean. No golden output file needed updating.

No output changes, which is the point of a commit that renames a constant, extracts a function and rewrites
a comment. The cross product of all 45 rules against all 31 data files under `guard/resources/validate`,
1395 pairs, is byte-identical to the previous binary over all of them, and no exit code differs. Every
convention-paired rule and test pair in `aws-guard-rules-registry` at 6aca96e, 193 of them, exits 0.
Two live comments said the unattributed section carries a block's own `<< >>` text, and it cannot. A block
report's `custom_message` is never there to carry: three of the four constructors of `GuardBlockReport` in
`eval_context.rs` set it to `None` outright, and the fourth, `MissingBlockValue`, copies
`missing.custom_message` from a record whose one producer at `eval.rs:1911` sets `None` as well. That copy
is why the JSON for `block_query_at_the_document_root.guard` shows the empty string in that slot rather
than null, and why `non_empty_message` drops it: the block arm reads a half that no evaluation puts there.

The fixture's own comment was the more misleading of the two, because its `<< >>` looks like a
counterexample. That message is written on the clause inside the block, and a block whose query resolved to
nothing never runs the clause, so the message is recorded nowhere at all -- not on the block, not on the
clause. There is no spelling of that rule that would put an author's message in the entry.

Comments only. The read stays where it is rather than being deleted, because the block arm's shape is the
clause arm's and putting an author's message through to it changes what the section prints for every block
finding, which wants its own change and its own verification.

The fixture comment is rewritten to the same six lines it occupied before. A block entry's context is
`GuardBlockAccessClause#Location[file:..., line:8, column:5]`, so a comment that grew by five lines moved
the clause to line 13 and rewrote that context in 23 outputs. Same line count, same output.

Verification. 1396 tests pass, 0 fail, unchanged. `build --release`, clippy `--release --all-targets -D
warnings`, `fmt --all -- --check` and typos are clean. The cross product of all 45 rules against all 31
data files under `guard/resources/validate`, 1395 pairs, is byte-identical to the previous commit over all
of them, and no exit code differs.
…he union

Two blocks over `Resources.*[ Type == 'AWS::S3::Bucket' ]`, the first capturing
`cfg` from `Properties.Config[ cfg | Enabled == true ]` and the second asserting
`some %cfg == "alpha"`, exited 0 on a template whose second bucket has only a
`beta` config -- in either document order -- and exited 19 when that bucket was
alone in the file. Adding a compliant resource made a non-compliant one pass, so
a rule of this shape looks correct when it is tested one resource at a time.
Repeating the capturing clause inside the second block gives 19, so declaring
the name was the whole difference.

`merge_captures_into_parent` hands a block's keys to the enclosing scope so that
a clause after the block can read them, and it put them in the same `captured`
map that a lookup deferring out of a block reaches. The second block declares no
capture, so `BlockScope::resolve_variable` fell through to the parent and found
the union of the first block's iterations.

This is the third case in one family, and it is the one neither earlier fix
reaches because both key on declaration. Holding captures in the block rather
than the root scope stopped iteration two of a block from reading iteration
one's key. Reading capture names out of the rule text, so that an iteration
which captured nothing under a name its own block declares answers empty instead
of deferring, stopped the `or` and nested-`when` shapes. A block that declares
nothing at all was left, and the same rule applied to it is: a key that has left
the block that made it goes into a separate map, and a lookup starting inside a
nested block is not offered that map. A clause at the enclosing level still is,
which is the second reading the merge exists for.

Two things a narrower reading of the same rule would have broken, both now
asserted. A key captured by a clause at rule-body level never belonged to an
iteration, so a block reading it is not asking a per-iteration question; and a
key of an enclosing iteration that is still running belongs to the resource the
nested block is inside. Neither has left a block, and withholding either would
turn a working rule into a failure.

The doc comment on `merge_captures_into_parent` described the `or` shape as an
open hole and set out a two-part plan for it. `capture_names` closed it on its
own -- `a_capture_does_not_leak_from_one_iteration_of_a_block_into_the_next`
covers it and the binary gives 19 -- so the paragraph is replaced rather than
extended, and the deferred-merge half of that plan is recorded as unnecessary:
a name the block declares never reaches the parent from inside the block.

Verification. The two-block rule exits 19 on both document orders, agreeing with
the control that declares the capture twice. The deliberate reading -- a clause
after the block seeing every iteration's keys -- still exits 0, and a nested
block still reads its own iteration's key and fails on a sibling resource's.
1387 tests pass, up from 1379; `cargo clippy --release --all-targets -D
warnings`, `cargo fmt --all -- --check` and `typos` are clean. All 193
convention-paired rule and test pairs in aws-guard-rules-registry at 6aca96e
exit 0, unchanged. The 3430-pair cross product of every rule against every data
file under guard/resources/validate is byte-identical to the binary built before
this change, in exit code and in text.
…er than the rule

Two rules containing the same two clauses -- `%a_names !empty` then
`%nm == "Alpha"`, over `let a_names = Resources[ nm | Type == 'AWS::S3::Bucket' ]`
-- gave PASS and then exit 255, `Could not resolve variable by name nm across
scopes`. Swapping those two clauses inside one rule did the same: the assignment
first gave exit 0 and the capture first gave 255. And adding `%b_names !empty`,
over a second assignment that mentions nothing the assertion reads and only
happens to spell its capture `nm` too, turned exit 0 into 19.

A capture is a side effect of running the assignment's query. The query runs
once, because its result is memoised in `resolved_variables` for the file and
never invalidated, and the keys went into `captured`, which `reset_captures`
clears between top-level rules. So the keys existed only in the rule that first
forced the assignment, and only after the clause that forced it. Both `let`s
spelling their capture `nm` appended into that one list, so what the name held
also depended on which of them had been forced.

Reading a capture name now resolves the assignments that declare it. The names
each assignment's right-hand side declares are read from the rule text at
construction, so the resolution does not wait for a clause to mention the
assignment, and the keys are kept in `assignment_captures`, which
`reset_captures` leaves alone: they belong to the assignment and the assignment
belongs to the file. A name two assignments declare reads as the union of both,
which is what makes it independent of the clause list. Such a file declares one
name twice in one scope -- `docs/QUERY_AND_FILTERING.md` already forbids that and
the parser ought to refuse it -- and the union is what keeps the verdict legible
until it does.

The same split `BlockScope` makes between a declared capture and an undeclared
name is now made here too. A file-level capture whose query matched nothing
resolves to an empty selection, so the clause reading it fails, where it used to
end the run at exit 255: whether the file produced a report at all depended on
the template it was run against, since the same clause resolves when the query
does match. A name no assignment declares is still an error.

Resolving on read is a new way for one resolution to ask for another, so the loop
it closes is guarded rather than assumed absent. `Resources[ nm | Type == %nm ]`
is a predicate reading the capture its own filter declares, and it reaches the
recursion in one line; the assignments in progress are tracked and one already
being resolved is not resolved again on its own behalf. That set is also what
tells `add_variable_capture_key` which map a key belongs in, since a capture
arriving at the root scope otherwise came from a rule's `when` condition, which
is the rule's.

Verification. The two rules with identical bodies now agree, in six clause
arrangements each asserted with its mirror so the passes are the key being read.
The two-assignment file gives the same verdict with and without the unrelated
clause, asserted on `some %nm == "Inst"`, which is false if only one assignment
contributed. The file-level capture that matched nothing exits 19 with the
resolved-to-no-values reason, and a name declared nowhere still errors. 1411
tests pass, up from 1387; `cargo clippy --release --all-targets -D warnings`,
`cargo fmt --all -- --check` and `typos` are clean. All 193 convention-paired
rule and test pairs in aws-guard-rules-registry at 6aca96e exit 0, unchanged. The
3430-pair cross product over guard/resources/validate is byte-identical to the
binary built before this branch's scope work, in exit code and in text.
…pture

In one block, over a bucket whose enabled config is named `alpha` and whose
`Properties.Name` is `fromquery`:

    let cfg = "fromlet"        + Properties.Config[ cfg | Enabled == true ]
        some %cfg == "fromlet"  -> exit 0        some %cfg == "alpha" -> exit 19

    let cfg = Properties.Name  + Properties.Config[ cfg | Enabled == true ]
        some %cfg == "fromquery" -> exit 19      some %cfg == "alpha" -> exit 0

Same position, same name, opposite winner, decided by the kind of the assigned
value. Writing the `let` after the capturing clause instead of before it changed
neither, so declaration order -- the one cue an author would look for -- carried
nothing. Both readings were silent.

This is the duplicate-assignment defect reaching the one namespace that check
does not compare against. `first_duplicate_assignment` matches assignment names
to each other, and its argument is that `extract_variables` files literals,
queries and function calls into separate maps which `resolve_variable` consults
in a fixed order, so the winner is kind precedence rather than order. A filter's
capture name is a variable defined in that scope as well -- it is read back as
`%name` like any other -- and its keys are a fourth map in that same fixed order,
sitting between the literals and the queries. Hence a literal assignment beats
the capture and a query assignment loses to it. `docs/QUERY_AND_FILTERING.md`
already says there can be only one same-named variable in a scope; only the
assignment-against-assignment half of that was enforced.

The file is now refused with the same argument the existing check gives, and the
line is drawn on lexical nesting: a capture written inside a nested `{ ... }`
belongs to that nested scope, so an assignment outside a block with a capture
inside it is ordinary shadowing and is accepted. That is what every other pair of
nested bindings in this language does, and it is the one rule an author can carry
between them.

Drawing the line on where a block's keys land at runtime instead was the first
attempt and it is wrong for a reason worth recording: it made two files an author
cannot tell apart disagree. A rule-body `let` with a capture in a block inside
the rule was refused, while the very same capture with the `let` moved out to the
file level was accepted, and both read as "an assignment outside, a capture in a
block inside". Nothing visible in the text explains the difference.

What the lexical line costs is one measured case, accepted deliberately. A
rule-body assignment still decides by kind against the keys a nested block merges
up, for a clause reading the name at rule-body level after the block: with
`let cfg = "fromlet"` that read is `"fromlet"` and with
`let cfg = Resources.Alpha.Properties.Name` it is the captured key `"alpha"`, at
exit 0 and exit 19. Refusing it is what made the check unexplainable, and the
reading that matters -- `%cfg` from inside the block -- is the capture's under
both spellings.

A rule's `when` conditions were the one case the text does not settle, since they
sit at the rule's head: outside the body's braces but attached to the rule. That
is measured rather than reasoned about. A file-level `let cfg = "fromlet"` with
`rule r when Resources[ cfg | Type == 'AWS::S3::Bucket' ] !EMPTY` resolves `%cfg`
to `"fromlet"`, and the same file with `let cfg = Resources.Alpha.Properties.Name`
resolves it to the captured key `"Alpha"`. Each was run in both polarities and the
failing one named the value it read. The literal winning and the query losing is
kind precedence within one scope, and two scopes cannot produce it, because a more
local capture would win against both. So the conditions are the file scope's and
that file stays rejected.

Every scope is checked, each against the names written directly in it: a block's
own assignments against the captures its own clauses declare, including a capture
on a block clause's own query since that query is evaluated where the clause is
written; and the file's assignments against the captures their own right-hand
sides and the rules' `when` conditions declare. `let cfg = Resources[ cfg | ... ]`,
which does both in one statement, is refused too. The walk is in exprs.rs beside
`block_capture_names` rather than in `block`, which is generic over its clause
type and cannot enumerate the scopes; it deliberately does not reuse
`block_capture_names`, which descends into nested blocks and so answers the other
question.

Verification. Six collision shapes exit 5 with a diagnostic naming `cfg` and the
scope, where all six exited 0, 19 or 0 before. Three shadowing shapes parse,
including the two an author cannot tell apart, and the block still reads the
captured key. 1417 tests pass, up from 1411; `cargo clippy --release
--all-targets -D warnings`, `cargo fmt --all -- --check` and `typos` are clean.
All 193 convention-paired rule and test pairs in aws-guard-rules-registry at
6aca96e exit 0, which is the measurement that matters most for a new parse
rejection: registry rules use `let` and captures throughout and none of them
spells one name both ways in one scope. The 3430-pair cross product over
guard/resources/validate is byte-identical to the binary built before this
branch's scope work.
…e !empty guard does

With `let allowed = "fromlet"` at the top of the file and a block containing a
capturing clause inside a `when` whose condition failed, `%allowed == "fromlet"`
in that block exited 19. Renaming the capture to `other` and changing nothing
else exited 0, so the capture name was what did it. Two more shapes reach the
same place: `Properties.Ports[ allowed ]` over a list, where `accumulate` has an
index rather than a key and the name can never be populated on any input; and a
parameterized rule's parameter, where the argument the call site passed became
unreadable for the whole block.

`BlockScope::resolve_variable` answered the lookup itself for any name in
`capture_names`, before asking the parent. That set exists for a good reason: an
iteration that captured nothing under a name its own block declares must not read
a neighbour's key, and an empty selection is the honest answer for it. The
short-circuit went further than that argument. An outer `let` or an enclosing
parameter of the same name is a real binding, and this iteration capturing
nothing says nothing about it, so making the empty selection the *first* answer
rather than the last hid a binding that was there all along -- for every
iteration, and even where the capturing clause provably never ran.

Every name now defers, and what the block contributes is what to answer if no
scope on the chain binds the name: `UnboundName::EmptySelection` for a name it
declares as a capture, and the unresolved-variable error otherwise. Only a block
can decide that, because only a block reads the capture names out of its own
clauses, and the error is produced at the far end of the chain, so the answer
travels with the lookup. A name no block on the chain declares still errors,
which keeps a typo and a name belonging to another rule loud: a capture made in
one rule and read in another still exits 255.

The diagnostic was the other half, and it was worse than the resolution. It
reported "resolved to no values" for a variable whose `let` is visible at the top
of the file, and then told the author to guard the clause with `when <variable>
!empty { ... }`. That is not a remedy. The gate's own `!empty` check fails when
the variable is empty, so the block is skipped and the comparison never runs: an
author following the advice replaced a check that was failing for a reason with a
check that does not run, at exit 0. Both messages now point at what binds the
name and say what the guard would actually do. The right-hand-side wording is
changed with the left-hand-side one because it is the same sentence with the same
effect, and leaving one of the pair would make the doc comment that contrasts
them wrong.

`LONGEST_CLAUSE_EXPLANATION` was chosen to hold this exact message whole -- its
doc comment names the 261 characters and records that at 240 the last sentence
was cut off mid-way. The new wording is 292, still under the 320 cap, and the
comment's figure is corrected rather than left to mislead the next person tuning
it. The console output is asserted to reach that last sentence, so the cap being
outgrown fails a test instead of silently clipping the part that matters.

Verification. All three shadowing shapes exit 0 where they exited 19, each paired
with its mirror asserting the other value, and the mirror reports `Value="fromlet"`
as what it read -- so the enclosing binding was used rather than the clause being
skipped. A declared capture with nothing enclosing it still fails its clause
rather than ending the run, and a name declared nowhere still errors. 1423 tests
pass, up from 1417; `cargo clippy --release --all-targets -D warnings`, `cargo fmt
--all -- --check` and `typos` are clean. All 193 convention-paired rule and test
pairs in aws-guard-rules-registry at 6aca96e exit 0, unchanged. The 3430-pair
cross product over guard/resources/validate has no exit-code difference from the
binary built before this branch's scope work, and 200 text differences in which
all 454 changed lines are the two message rewrites and nothing else.
A test expectation naming a rule the rules file does not have was reported and
then ignored: every output format said so, and the run exited 0. So a rule
renamed without its test file being updated leaves behind an expectation that
asserts nothing, and the suite stays green -- the author believes they are
checking something and they are not. This repo had one such suite of its own,
described below.

Measured on a real one. `cfn_no_explicit_resource_names_tests.yml` in
aws-guard-rules-registry, against the rules file it pairs with, has 33 cases
carrying 30 expectations that name no rule in that file -- 11 distinct names
spread over 28 of the cases. Before: exit 0 in every format, junit `tests="63"
failures="0" errors="0"`. After: exit 1, `errors="30"`. Thirty assertions that
ran nothing, in a suite that reported clean.

Note for anyone sizing this from the plaintext output: stderr shows 11 lines
there, not 30. `Diagnostics` is a set, because a rules file is evaluated once per
test case and an un-deduplicated note repeats every line for every case. The
structured reports are per-case and carry all 30. The exit code is set from the
per-case names either way.

Exit code 1 and not 7. The command's two codes already draw this line, and the
test suite states it where a rule cannot be evaluated: an expectation that could
not be evaluated is a different answer from an expectation that was not met. An
expectation whose rule produced no verdict was not evaluated -- there is nothing
to compare it against, so `Expected = PASS, Evaluated = []` would be a fiction.
The sibling case decides it the same way: an expectation whose *value* will not
parse is already the error code, and a stale rule name is the same authoring
defect one field over. Rejected: 7, on the argument that CI wants a test failure.
Junit has a first-class `<error>` element and an `errors` attribute that every
consumer treats as red, and the reporter already uses both, so the error code
costs nothing in visibility.

The structured result stays a `TestResult::Ok`. Reaching the error code through
`TestResult::Err` -- the other way there -- would replace the whole document with
one error object, so every rule that did get a verdict would vanish from the
report over one stale name. That is the defect the preceding commits on this
branch removed from the same file, and it is not worth reintroducing.

Junit changes from `<skipped>` to `status="error"` with an `<error>` body,
counted into the suite's `errors`. A skipped case counts into `tests` and nowhere
else, so a CI step watching `failures` and `errors` read a suite where every
expectation named a stale rule as entirely green. Json and yaml gain a `reason`
beside the name, because there is now more than one reason and they call for
different fixes. The plaintext note stays on stderr, once, where the run's other
diagnostics go: printing it per case as well would duplicate every line in the
terminal, which is what collecting them was for.

Directory mode does not need an exemption, which was the question that decided
whether failing outright is right. If one test file could run against several
rules files, an expectation naming a sibling file's rule would be legitimate.
It cannot: `OrderedTestDirectory::from` filters the rules files whose stem
prefixes the test file name and reduces them with `min_by_key`, which yields
exactly one claimant. Measured, not inferred -- a directory with `encryption.guard`,
`logging.guard` and a single `tests/encryption_tests.yml` naming a rule from each
checks the first and reports `logging.guard` as having no tests at all, so
`LOGGING_ON: PASS` asserted nothing about it. Pinned by
`an_expectation_for_a_sibling_rules_files_rule_fails`, which asserts both halves:
a future change that ran each test file against every rules file in the directory
would make that expectation real and this failure wrong.

The two ways an expectation goes unchecked are now told apart by what the rules
file declares rather than by what ran, and only one of them is a name the file
does not have. A parameterized rule is evaluated where a clause invokes it, so it
is recorded under the invoking rule rather than under the file and never appears
among the rules an expectation can match -- `No rule named encryption_is_on is in
this file` was false, and that sentence is now the stated reason for a failing run
rather than a note beside a passing one. It gets its own sentence and the same
verdict. A rule that exists and could not be evaluated is not in either bucket:
`eval_rule` closes its record as a failure before the error leaves
`eval_rules_file`, so it has a verdict, which was measured rather than read off
the comment that says so.

`test_data_file_with_shorthand_reference` relied on the old behaviour. It paired
`s3_bucket_logging_enabled_tests.{json,yaml}` -- five cases, every expectation
naming `S3_BUCKET_LOGGING_ENABLED` -- against
`s3_bucket_server_side_encryption_enabled.guard`, which does not define that rule,
and its recorded output was five cases of `No Test expectation was set for Rule
S3_BUCKET_SERVER_SIDE_ENCRYPTION_ENABLED` with no PASS or FAIL section anywhere
in it. The test's name says what it is for, a data file carrying the YAML
shorthand tag `!Ref`, and it did prove the file parses; it proved nothing about
the five expectations. Fixed by pairing the data file with the rules file it was
written for, not by relaxing the check that caught it. Both rstest cases produce
identical stdout, so the one golden file still serves both.

Known limitation: in directory mode the plaintext path keeps the first non-zero
code it sees across rules files, so a directory whose first file has a failing
expectation and whose second has an unchecked one exits 7 rather than 1. That
predates this change and applies equally to the eval-error path beside it. Within
a file the precedence now holds in both reporters.

Verified: the reproduction exits 1 in all four output formats and under
`--verbose`, still naming both missing rules on stderr. The same rules file with
a test file naming only rules it defines is byte-identical to before across
stdout, stderr and exit code in all four formats, as is the neighbouring
eval-error fixture. Every directory in the repo with a `tests/` subdirectory was
run through the new binary; the two that exit 7 still exit 7. `cargo test
--release` green, `cargo fmt --check` and `cargo clippy --all-targets -D warnings`
clean.
A rule reference asked whether the dependent rule's status was PASS, and SKIP is not
PASS, so referencing a rule that never ran failed the referencing rule:

    rule H_A when Resources.*[ Type == 'AWS::IAM::Role' ] !empty {
        Resources.*[ Type == 'AWS::IAM::Role' ].Properties.RoleName not exists
    }
    rule H_B when Resources.*[ Type == 'AWS::DynamoDB::Table' ] !empty {
        Resources.*[ Type == 'AWS::DynamoDB::Table' ].Properties.TableName not exists
    }
    rule MAIN { H_A H_B }

Against a template holding one clean IAM Role and no DynamoDB table: H_A PASS, H_B
SKIP, MAIN FAIL at exit 19 with "dependent rule [H_B] did not PASS". Nothing in that
template violates anything.

That makes the natural decomposition of a ruleset unusable. A helper per resource type,
each guarded by a `when` on its own type, means most helpers do not apply to any real
template, and the aggregate failed once per inapplicable helper. Neither workaround is
behaviour-preserving: conjunction fails whenever a type is absent, and disjunction
passes as soon as any single helper passes, which is a false negative in a compliance
rule. The only shape that preserved behaviour duplicated every type check in the file.

The clause path had already answered this correctly one level down -- a clause whose
query selects nothing SKIPs, and eval_conjunction_clauses absorbs a SKIP. So both
reference sites now answer SKIP for an inapplicable dependency, and the reference
contributes nothing.

At the Outcome level (PR aws-cloudformation#720's Satisfied/Violated/NotApplicable/Unevaluatable, which is
not on this branch yet but is the algebra this has to be consistent with):

  Satisfied     unchanged, inverts under negation
  Violated      unchanged, inverts under negation -- a reference to a failing rule must
                still fail, which is the whole point of a reference
  NotApplicable now NotApplicable, in both polarities, because NotApplicable is the
                identity of Outcome::and and there is no operation on that lattice that
                maps it to the absorbing element
  Unevaluatable not folded in, and nothing to decide: rule_status evaluates the
                dependent rule with the reference site's role, so an unevaluatable
                clause in its body is already resolved to FAIL for an assertion or SKIP
                for a gate before a status reaches here. A genuine Err still propagates.

Rejected: keeping FAIL for a negated assertion. An earlier commit chose that to stop
`not <rule>` reporting compliance for a check that never ran, and pre-emptively rejected
SKIP as "merely inert". SKIP is not compliance -- the referencing rule reports SKIP, so
the omission stays visible and find_skip_reason names the rule -- and FAIL went a step
past withholding compliance into reporting a violation that does not exist:
`rule deny when Resources.*.Type exists { not inner }` failed on a template holding one
S3 bucket and no KMS key. That is the same false positive with a `not` in front of it, so
fixing only the non-negated spelling would have left half the defect in place. What SKIP
gives up is exit 0 rather than 19 for that idiom, and that is the deliberate trade.
negated_reference_to_skipped_rule_does_not_pass_in_rule_body now asserts SKIP; its name
still holds.

`when` conditions are deliberately unchanged, both polarities. A non-negated gate on an
inapplicable rule already answered SKIP, because eval_conjunction_clauses counts a FAIL
and absorbs a SKIP, so FAIL there would outrank sibling conditions that passed and drop a
body they would have enforced, at exit 0. A negated gate answers PASS and keeps doing so:
`rule r when not other { ... }` is how a ruleset says "apply this when that other rule did
not apply", and answering SKIP would close a gate that currently opens and silently
disable the guarded rule. A gate is not making a compliance claim, so it may read "did not
apply" as a condition that is met; an assertion is, so it may not -- the split ClauseRole
already exists to express.

eval_parameterized_rule_call carries its own copy of the arm and had the same defect
reached through `r(...)`. Both are fixed together: the two spellings have drifted apart
before, and the new test asserts both so a change to one cannot pass on the other's
coverage. Both matches are now exhaustive over Status rather than ending in `_`, which is
what let one arm quietly cover two unrelated situations.

Blast radius measured, not assumed. Every .guard file in aws-guard-rules-registry pinned
at be20abd (210 files, 366464 bytes) and in this repository was swept before and after --
1626 rules-file/fixture pairs through `cfn-guard test` and `cfn-guard validate` -- and the
output is byte-identical. Only 10 of the 210 registry files contain a rule reference at
all. Nine of their references are non-negated assertions composed with OR, where the
helpers are alternative paths to one conclusion and at least one always decides, so no
disjunction is ever left with every disjunct SKIPped; the tenth is a negated reference
inside a multi-line `when`, which this does not touch. The sweep was confirmed sensitive
by a fixture whose expectation moves from exit 7 to exit 0 across the two binaries.
…an internal failure

A rules file that uses a variable without declaring it with a `let` exited 255. The table in
guard/tests/utils.rs names that code INTERNAL_FAILURE and names 5 PARSING_ERROR, so the tool was
answering one of the commonest authoring mistakes with the code it reserves for its own defects.
The message does not name the variable as the thing to fix either, which left the exit code as the
only signal, pointing at the wrong party.

guard/resources/validate/malformed-rule.guard is a real instance: it parses, and every clause reads
%s3_buckets_server_side_encryption_2, which nothing declares.

Two sites propagated it. validate.rs's `evaluated?` on the single-line path, and structured.rs's
`Some(e) => Err(e)` on the json/yaml/sarif path. Both carry comments from the commits that made the
report render before the error propagates, reasoning that the exit code should still say "the ruleset
is broken". That intent was right and the mechanism was not: propagating to `main` spells it -1. The
justification went stale between the two commits.

The strongest evidence for 5 was already in the tree. `-o junit` reported this same input as
ERROR_STATUS_CODE, because JunitReporter folds an eval error into the suite's `errors` total instead
of returning `Err`, while json, yaml and sarif exited 255. One binary gave two answers about one file
depending only on `-o`.

Classified per command rather than in `main`. Mapping error variants centrally was considered and
rejected: `main` cannot pick a code for a command whose code space it does not know, and `test`
deliberately uses 1/7 rather than 5/19. `Error::ParseError` is excluded from the predicate for a
related reason -- it also carries the empty-data-file case, which exits 255 today with a test pinning
its code and its message.

`Error::is_undeclared_name` covers MissingValue and MissingVariable. All four sites that produce them
were read first: an undeclared variable, a missing rule and a missing parameterized rule in
eval_context.rs, and the same condition on evaluate.rs's older path. Every one means a name with no
declaration behind it, so the predicate holds for the variant and not just for the reported case.

Exit-code precedence had to be stated as part of this. While an undeclared name returned `Err`, the
first unusable rules file ended the run, so `exit_code = status` could not lose it. Now that it
returns 5 and the run continues, a later file's 19 would overwrite it. `more_severe` holds the rule --
ERROR outranks FAILURE -- and JunitReporter::update_exit_code, which already had it right, delegates
there rather than stating it twice.

Two tests asserted the old code and so encoded the defect rather than a requirement:

  - a_rule_that_cannot_be_evaluated_does_not_discard_the_structured_document called 255 "unchanged".
    It was unchanged by that commit, which is not the same as correct, and it left the path
    disagreeing with -o junit.
  - a_rule_that_cannot_be_evaluated_does_not_discard_the_other_rules_findings asserted 255 so that a
    broken ruleset "stays distinguishable from a non-compliant template", reasoning that 255 rather
    than 19 is what says so. 5 is also not 19 and does not additionally claim cfn-guard broke. That
    case now asserts PARSING_ERROR and adds an assert_ne! against VALIDATION_ERROR, so the
    distinction is pinned directly instead of as a side effect of the code chosen.

The malformed-rule.guard case in test_single_data_file_single_rules_file_status moves the same way,
and a_broken_rule_beside_working_ones.guard's own comment said "the run says so with exit 255", so it
is updated too rather than left contradicting the tests that use it.

Well-formed input is untouched: 34 invocations across every subcommand, output format and summary
setting compared byte-for-byte on stdout, stderr and exit code against the previous binary, with no
differences. Suite: 1485 passed, 0 failed.
…t an internal failure

`parse-tree -r <file>` exited 255 for a rules file with a syntax error, where `validate -r` on the
same file exits 5. Two subcommands disagreed about whose fault one file was, and 255 is the code the
table in guard/tests/utils.rs names INTERNAL_FAILURE, so a build step could not tell a bad ruleset
from a broken tool.

The asymmetry was not a subtlety of the parser, it was a missing arm. validate.rs's `evaluate_rule`
has an explicit `Err` arm that reports the parse error and returns Ok(ERROR_STATUS_CODE).
parse_tree.rs used `?` on `rules_file`, which sent the error to `main`'s catch-all and `exit(-1)`.

A missing file still returns `Err` from `File::open`, so it keeps the 255 that validate, test and
parse-tree already agree on. That distinction is deliberate: only the parse error moves.

Reported by the command rather than left to `main`, because once the command classifies the error,
`main`'s "Error occurred" prefix -- its vocabulary for an unexpected failure -- no longer describes
what happened. Keeping the prefix was the alternative, and it would have preserved stderr byte for
byte, but it would have meant a command announcing an internal failure it had just decided was not
one. The wording is validate's verbatim, since agreeing on the wording is the same fix as agreeing on
the code.

The message names the file's final path component rather than the path as given, sharing
`validate::file_name_of` rather than reimplementing it. That matches what validate puts in the same
message, and it is also what makes the message assertable: the path reducer in guard/tests/utils.rs
normalises .yaml, .yml and .json and not .guard, so an expected string holding an absolute .guard path
would only hold for the checkout that produced it.

Well-formed input is unaffected, including `parse-tree` in both yaml and json, verified by comparing
stdout, stderr and exit code byte-for-byte against the previous binary.
test_yaml_output_with_expected_failures had two #[case]s sharing one expected output, and that output
was an I/O error:

    "Error occurred I/O error when reading No such file or directory (os error 2)\n"

Only both files being missing can satisfy that. dne.guard is missing deliberately; the name says so.
The second case pointed at validate/rules-dir/malformed-rule.guard, and that file is at
validate/malformed-rule.guard, one directory up. So it never reached the parser: it failed at
File::open, matched the shared I/O string, and passed green.

The consequence was that parse-tree's parse-error exit code had no coverage at all, which is how it
reached `main`'s catch-all and exited 255 unnoticed. The one test that looked like it covered the
parser was exercising File::open.

Three cases now, each with its own expected stderr so that no case can be satisfied by another's
failure mode:

  - a path that does not exist -> INTERNAL_FAILURE, the OS message, platform-split as before. Kept,
    and given its own expectation instead of one shared with a case that should assert something else.
  - a rules file the parser rejects -> PARSING_ERROR.
  - a rules file naming an undeclared variable -> SUCCESS, empty stderr.

Three rather than two because the third is a boundary rather than a bug. parse-tree parses and does
not resolve, so 0 is correct for it, and malformed-rule.guard is that file -- it parses, and its
mistake is that nothing declares %s3_buckets_server_side_encryption_2. Asserting 0 keeps a future
"warn on unresolved names" change out of the one command with no data to resolve against, and stops
the fixture's name from luring someone into treating it as a syntax error again.

The middle case reuses validate/unparsable-rule.guard, whose custom-message marker is left
unterminated. Adding a new fixture was the first approach and it was wrong: the structured validate
tests already assert PARSING_ERROR against that file, so borrowing it demonstrates the two subcommands
agreeing on one file rather than on two files that merely resemble each other. What made it easy to
miss is that searching for "malformed" finds only the misleading fixture, and the directory the broken
case pointed into does not contain either file.

Renamed, since one of the three cases is now a success and "expected_failures" no longer describes the
set.
The README said only that a successful validate exits 0. Nothing documented 5, 19, 1 or 7, and
parse-tree's codes were not mentioned at all; `--help` does not list them either. The only statements
of intent in the repository were two constant tables, one of them in the test sources.

That is what let 255 sit on two authoring mistakes without contradicting anything written down. A
reader cannot use an exit code they have to infer from a `match` arm, and a build step that wants to
tell "your rules are wrong" from "cfn-guard fell over" needs to know that 5 and 255 mean different
things.

Documents validate and parse-tree's 0/5/19, test's separate 0/1/7 and why it is separate, that
anything else means cfn-guard itself failed, and that 5 outranks 19 when several rules files are
given. Codes were read from the source and confirmed against the binary rather than recalled: rulegen
turned out to reach its 1 through a bare process::exit(1) rather than any named constant, so it is
left out of both tables.
…carding it

parse_regex_inner formats two messages and throws both away. Hash == /a(/ exits 5 and the engine's
explanation, "Parsing error at position 2: Opening parenthesis without closing parenthesis", appears
nowhere in the output; grepping for "Could not parse regular expression" returns 0 matches. Hash ==
/abc exits 5 and says nothing about a missing delimiter, also 0 matches. What the author reads in both
cases is the generic alternation fallthrough, expecting either a property access "engine.core" or
value like "string" or ["this", "that"]. Both messages now arrive, 1 match each.

Both returns were nom::Err::Error, which alt treats as recoverable, so the error is swallowed and the
enclosing context wrapper reports only its own string. The rule is already stated in this file, on
parse_float at guard/src/rules/parser.rs:422-426: "Failure, not Error. A recoverable error sends alt
back to try the other value productions" and the message "never reached the author at all". That
comment names parse_range as following the same reasoning, so the convention is stated once and
applied at two sites; these two were the exception. ParserError::add_context joins a non-empty inner
context with the outer one, which is why the range failures on this base report their text and these
did not: an Error never reached the wrapper to be joined.

The comment removed here argued the other way, that "parse_regex is the last arm of the value
alternation, so the caller sees the same error either way". The first half is true, verified at
parse_scalar_value on parser.rs:715-721 where parse_regex is indeed last. The second half does not
hold at the level the author reads, which the zero occurrences above measure directly.

Refusing to backtrack gives up nothing. parse_regex_inner is only reached after char('/'), and no
other production in the grammar starts a value with a slash: the four earlier arms of
parse_scalar_value start with a quote, a digit, a sign or a keyword, parse_list starts with an open
bracket, parse_map with an open brace, and a property access is alphanumeric or quoted. A literal
beginning with a slash is a regular expression or it is nothing, so the recoverable error bought a
worse diagnostic and no second chance. The sweep below is the empirical form of that argument, since a
Failure escaping an enclosing alternation is exactly what would change a file that parses today.

The unterminated case is reached three ways after 19509c2 replaced the scan, and the test covers all
three rather than the one the old code had. scan_escaped_literal answers None at a line ending with no
unescaped delimiter, at end of input with none, and at a backslash whose escapee is a line ending or
which has no next character at all. The first of those is the ordinary forgotten-slash typo and it did
not reach the old escape branch at all; under the previous delimited it was the trailing char('/')
that failed, with an empty context.

Two assertions move because moving them is the change rather than collateral, and a third turned up
that the census of parser_tests.rs had missed. test_parse_regex and the renamed
test_parse_regex_inner_when_regex_is_not_terminated pinned nom::Err::Error by assert_eq! on the whole
error; both now assert the variant once with matches! and match the context by substring, the shape
a_float_literal_out_of_range_is_rejected already uses in this file, so neither re-breaks if the variant
moves again. The third is test_parse_error_when_guard_rule_has_syntax_error at
guard/tests/test_command.rs:265, which asserts the entire user-visible message for invalid_rule.guard
by string equality. Its update is purely additive and its comment now names why a fixture called
syntax error reports a regular expression problem: line 8 of that file is
== {"Fn::ImportValue":/{"Fn::Sub":"${pSecretKmsKey}"}} and the slash in the middle of it opens a
regular expression that never closes before the line ends.

Verification. The suite is 1484 passed, 0 failed and 2 ignored across 16 binaries, before and after;
the diff adds and removes no test marker and renames one function, so no test was dropped. clippy
--release --all-targets -D warnings, fmt --all --check and typos are clean. Every one of the 318 rules
files in this repository and in the AWS registry at 6aca96e was compared before and after: 316 produce
byte-identical output, 0 changed between parsing and failing in either direction, and 2 differ in
message only. Those 2 are the intentional-failure fixtures invalid_regex.guard and invalid_rule.guard,
which error before and after at the same line, column and fragment with the diagnosis appended. All
193 convention-paired rule and test-suite pairs in that registry exit 0, unchanged. A valid regular
expression is unaffected byte for byte.
…erence-to-inapplicable question toward aws-cloudformation#727

Two conflicted paths, and eval.rs was not a textual conflict: the two branches
held opposite, separately-argued positions on what a reference to a rule that
did not apply means.

This branch mapped it onto a violation for assertions --
`(Outcome::NotApplicable, _) if role.is_strict() => Outcome::Violated` -- on the
ground that "a rule body asserting `r(...)` claims that `r` holds, and a rule
that never ran is not evidence that it does", and flagged the resulting change
from main as deliberate. That reasoning is true and the conclusion drawn from it
is not: the answer to "no evidence" is no verdict, not a violation.

Resolved toward aws-cloudformation#727 for two reasons.

It produced a false positive. With two helper rules each gated on their own
resource type, a template carrying one clean instance of the first type and
none of the second reported FAIL at exit 19, and so did a template carrying
neither type -- a violation asserted about a resource type the template does
not contain. The row that matters for correctness, a violating instance beside a
clean sibling, still FAILs, so this is not a slide into disjunction.

It contradicted this PR's own algebra. `Outcome::and` has identity
`NotApplicable` and absorbing `Violated`, so one level down an inapplicable
clause conjoined with a satisfied one yields satisfied. Mapping that same
`NotApplicable` onto the absorbing element at the reference site left the type
saying "not applicable is neutral" while this arm said "not applicable is
fatal", for one input. Whichever way the question is answered, the reference
site and `Outcome::and` have to answer it the same way, and only one answer is
expressible without carving an exception out of the type.

Both sites are resolved, `eval_guard_named_clause` and
`eval_parameterized_rule_call`, and both matches stay exhaustive over the enum
rather than ending in `_`: that catch-all had been silently covering both a
failing dependent and a negated gate on an inapplicable one. The negated `when`
gate is unchanged -- a gate that closes silently disables the rule it guards,
and the AWS rule registry's one negated reference is a gate. The comments
arguing for the old behaviour are removed rather than left beside code that now
does the opposite.

eval_tests.rs was additive on both sides; all twelve test functions are kept.
A selection can come up empty two ways and the two got opposite verdicts. A filter that runs and keeps
nothing returns no values, and eval.rs:583 answers that with SKIP -- the rule does not apply. A
collection that is absent or empty never reached the filter: retrieval produced an unresolved marker,
the clause's operator ran on that marker, and not exists read it as vacuously true. So

    rule H {
        Resources.*[ Type == 'AWS::DynamoDB::Table' ].Properties.TableName not exists
    }

reported PASS for {} and for {"Resources":{}}, and SKIP for a document holding one S3 bucket. A
document containing nothing was called compliant while a document containing one unrelated resource
correctly reported the rule inapplicable -- less information yielding the stronger claim, and the
stronger claim is the unsafe one for a compliance tool.

This is a consistency fix, not a decision about what vacuous truth over an empty selection should mean.
The engine already ships an answer for this situation and the other path disagreed with it. The two
empty-collection branches even said so out loud: their comment read "It is an error if there are no
elements in the map", which is the opposite of what the filter path does with the same state.

The asymmetry was not confined to not exists. Over the query above, twelve of sixteen operator and gate
shapes disagreed between the two flavours -- exists, not exists, empty, !empty, ==, !=, in, not in, two
filter-terminated forms and two when gates. All sixteen agree now. Worth recording that SKIP was the
safe direction for only three of the twelve: the other nine answered FAIL, which is a false positive of
the mirror kind, "a table is missing its TableName" about a document containing no tables. SKIP is right
in both directions, but the safety argument only carries the three PASS rows and should not be leaned on
for the rest.

A when condition over an empty selection now closes its gate, which is what the two already-agreeing
gate shapes did. A gate answers "does this rule apply to this document?", and an empty selection is the
definitive no; opening it runs the body against a document the rule was never about, where the body
meets the same empty selection and returns whichever of PASS or FAIL its operator happens to give. The
cost is real and belongs on the record: closing a gate disables the body it guards, so a
when ... not exists gate that used to open on an empty document stops running its body. That is correct
here because the body has nothing to check, not because closing gates is generally safe.

Two approaches were tried and rejected. Treating any all-unresolved result as an empty selection fails
immediately: a missing property on a resource that *was* selected is the same unresolved marker, so that
version turns Properties.BucketEncryption exists from FAIL into SKIP, and that clause is the single most
common one in any real ruleset. Keying off "an expansion with query parts after it" is subtler and got
further -- it built, and it produced all five rows above -- then it failed rule_test_type_blocks and
test_support_for_atleast_one_match_clause, which require an IAM role carrying no tags to fail its
tag-content checks and some Tags[*].Key == /PROD/ to fail on an empty tag list. Those are requirements,
not tests encoding the defect, and they are the reason the predicate is what it is: whether a filter is
still pending at the point the collection came up empty, meaning selection is still to come rather than
a projection of a subject already chosen. A filter that has already run does not count, which is why
Resources.*[ Type == 'X' ].Properties.Tags[*].Key == /PROD/ still fails on an empty tag list.

Verification. 1489 passed, 0 failed, 2 ignored across 13 test binaries and 3 doc-test targets; the
merge-base is 1485/0/2, and the four added instances are the two new tests over the lib and bin targets.
clippy --release --all-targets -D warnings and fmt --all --check are clean.
both_flavours_of_empty_selection_agree fails on the merge-base with SKIP against PASS on its first row,
so it pins the defect rather than the fix; an_empty_selection_never_excuses_a_missing_subject passes on
the merge-base by design, since every row in it is behaviour that had to survive, and it is what caught
the rejected second approach.

Blast radius: all 210 aws-guard-rules-registry rules files at 7f7340c and all 108 .guard fixtures in
this repository, each against three empty-ish documents. 12 of 954 pairs change, every one FAIL -> SKIP,
every one in this repository, none in the registry, and none on a document that actually holds a
resource. The registry is immune because every rule in it is gated by when %var !empty, whose verdict is
unchanged. The 26 pairs an earlier sweep left unclassified are accounted for: 16 registry files are
comment-only stubs with zero non-comment lines, 3 repo fixtures are deliberate parse-failure fixtures,
and 2 are rule-free files.

Two verbose snapshots move. Their empty-document cases recorded
GuardClauseUnaryCheck(Status=FAIL, ..., Value-At=(unresolved, ...)) while the same file already recorded
GuardClause(Status=FAIL, Empty, ) for its filtered-to-zero case; the four lines now agree with the two
that were already there. The verdicts in those snapshots do not change and every expectation in them
still holds.

generated_rule_shapes_hold_the_evaluator_invariants loses its eight absent_root cells, and not because a
gate stopped losing a verdict. absent_root carries no Resources key and the shared FILTER selects from
it, so on that template the body alone used to report FAIL -- "a volume is unencrypted" about a document
with no volumes -- and it now reports SKIP, so the loop's body_alone != FAIL guard drops the cell and
there is no verdict left to lose. Measured both ways rather than argued: on the merge-base the body alone
is FAIL and the gated form SKIP, and with this change both are SKIP.
aws-cloudformation#727 gained one commit: an empty expansion yields an empty selection only when a
filter is still pending at or after the point the collection came up empty, so
the two flavours of empty selection stop disagreeing.

Merged clean textually. Verified behaviourally rather than on that basis, because
the previous merge from this branch was also textually clean and had aws-cloudformation#727's
Ok/Status::FAIL arms meeting this branch's Result/Outcome return type, which git
cannot see.
awsmadi added a commit to awsmadi/cloudformation-guard that referenced this pull request Aug 26, 2026
…ion#720's

Sweeping every backticked identifier in the comments of eval.rs, eval_context.rs, path_value.rs,
parser.rs and the reporters turned up two names in one comment block that exist nowhere on this
branch: `EmptyLhsCollection` and `Outcome::to_status`. Both are aws-cloudformation#720's, with 10 and 309 non-comment
occurrences there and none here, so a reader following either one at this base finds nothing and no
indication why. eval.rs:693 already states the convention for this -- it writes "aws-cloudformation#720's `Outcome`
lattice" -- and these two were the only mentions missing the marker.

`Outcome::to_status` takes the marker unchanged. `EmptyLhsCollection` is replaced by the arm that
records the hazard here, `EmptyRhsUnsatisfiable` in eval.rs, with aws-cloudformation#720's arm named as an addition
rather than a rename. That distinction was checked rather than assumed: the first draft of this
change called it a rename, and aws-cloudformation#720 keeps `EmptyRhsUnsatisfiable` at operators.rs:106 and eval.rs:1292
while adding `EmptyLhsCollection` at operators.rs:79 as a separate ValueEvalResult variant. The two
answer different questions -- an empty right-hand reference against an empty left-hand collection --
and asserting a rename would have replaced one unverifiable name with another.

`EmptyRhsUnsatisfiable` is the right arm for the sentence to point at because it carries the same
argument the comment is making: eval.rs:1128-1133 records that a FAIL there "overrides sibling
conditions that passed and drops a body those siblings would have enforced, at exit 0", which is the
trade this comment calls one unenforced clause for a whole disarmed block. eval.rs:1099 already
cross-references that arm under that name from 800 lines away, so the spelling has a precedent in the
file.

Verification. Comment-only: 8 changed lines, 0 of them non-comment. 1489 passed, 0 failed, 2 ignored;
clippy --release --all-targets -D warnings and fmt --all --check clean.
@awsmadi

awsmadi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

The workflow runs here are held at action_required and have never executed, so this PR's checks report nothing rather than passing.

Could a maintainer approve the workflow run? The change is about the evaluator returning a value that can represent "nothing to compare", which alters empty-collection outcomes across rules — precisely the case where the existing suite is the useful signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant