Give the evaluator a value that can say "nothing to compare", and fix the empty-collection wrong passes - #720
Draft
awsmadi wants to merge 101 commits into
Draft
Conversation
emit_code computed its first context line as `max(1, line - 2)`, which
evaluates the subtraction before the clamp. On an unsigned line number of
0 or 1 that underflows:
- debug builds panic ("attempt to subtract with overflow") and exit 101,
which is not one of cfn-guard's documented exit codes
- release builds wrap to ~usize::MAX, so seek_line runs past EOF and the
source snippet is silently omitted from the violation report while the
exit code stays a correct 19
Only the violation path is affected, so the tool misbehaves precisely when
it has something to report. The trigger is input formatting rather than
content: a minified single-line template reports the violated property at
line 0 or 1.
Extract the arithmetic into context_start_line() and use saturating_sub so
the clamp applies to the input, and add unit tests covering lines 0-2 (the
underflowing inputs) plus the normal case.
Reachable from the library entry point, so it also affects guard-ffi, where
unwinding out of an extern "C" function is undefined behavior.
A leading `not` on a clause with a binary operator was parsed and stored as GuardAccessClause::negation (parser.rs:969, :1034) but never applied: the binary evaluation path called binary_operation() without passing gac.negation, while only the unary path consumed it. `grep negation` over eval.rs found three uses, none in the binary path. The effect is that `not <query> == <value>` evaluated as plain `<query> == <value>` -- the exact inverse of the author's intent. A rule written to reject an insecure value instead accepted it and rejected the secure one. The report compounded this by rendering the clause *with* the `not`, so the output gave no indication the negation had been dropped. Compose the clause negation with the operator's own not-flag (from `!=` / `not in`) by XOR at the call site. This matches invert_closure() in the superseded evaluator (evaluate.rs:293-307), which applies both flips independently and is the reference for the intended semantics -- evidence this was a regression in the v3 evaluator rather than a design decision. Tests assert the negated form now yields the author's intent, that the un-negated form is unchanged, and that double negation (`not ... !=`) composes correctly. Verified the first and third fail without the fix. No existing test covered clause negation on a binary access clause: all negation cases in parser_tests.rs are named-rule or parameterized-call forms, and eval_tests.rs had none.
eval_guard_named_clause collapsed SKIP into the same match arm as FAIL, so
with negation it produced PASS. `not <rule>` therefore reported compliance
on the strength of a dependent rule that never ran. This is worse than a
SKIPped clause: the enclosing rule reported PASS rather than SKIP, so the
output contained no indication that the check had been omitted.
The two contexts a named-rule reference is reached from need different
answers, so thread a strict_skip flag:
- rule body (GuardClause::NamedRule) -- the reference is an assertion, so
a SKIPped dependent rule fails closed. The un-negated case already
returned FAIL for a SKIP, so only the negated case changes and no
existing failure is relaxed.
- `when` condition (WhenGuardClause::NamedRule) -- gating on a rule that
did not apply is deliberate. `rule r when !other { ... }` is how a
ruleset expresses "apply this when that other rule did not apply", and
cross_rule_clause_when_checks asserts exactly that. Behavior preserved.
A first attempt failed SKIP unconditionally and broke
cross_rule_clause_when_checks, which is what surfaced the distinction.
Tests cover both contexts: the body case must not PASS (verified failing
without the fix), and the when-condition case must still gate, which guards
against over-correcting this in future.
CmpOperator::compare treated an empty operand on either side as reason to
SKIP the clause. SKIP is non-failing and exits 0, so a rule whose reference
list resolved to nothing reported compliance for a template it should have
rejected -- e.g. `Property IN %approved` where %approved is derived from a
resource type absent from the template. Reported externally.
The two empty operands are different situations and get different answers.
An empty LHS means the query selected nothing, so the clause has nothing to
say about this input. That is genuinely inapplicable and is what lets one
ruleset run across templates that do not all contain the resource type being
checked. Unchanged, and checked first so it keeps precedence when both sides
are empty.
An empty RHS means there ARE values to check but no reference to check them
against. Polarity decides:
positive (==, IN) unsatisfiable -- nothing qualifies, so FAIL
negated (!=, NOT IN) vacuously true -- nothing to collide with, so
non-failing
Failing the negated case would reject compliant templates, since a denylist
is legitimately empty whenever a template contains none of the denied values.
The vacuous case is reported as SKIP rather than PASS. eval_conjunction_clauses
treats PASS as short-circuiting (`continue 'conjunction`) but SKIP as
absorbing (`=> {}`), so a vacuous PASS satisfies an entire `or` block and
abandons the sibling disjuncts unevaluated:
Encrypted != %empty_denylist or Encrypted == true
would pass an unencrypted resource because the first disjunct is vacuously
true and the real check never runs. Base failed that ruleset correctly and an
intermediate version of this change did not -- per-clause soundness is not
sufficient, the status has to compose correctly under the folds. SKIP keeps
the clause non-failing while leaving the decision to its siblings.
The verdict is also context-dependent. In a `when` condition the unsatisfiable
case stays a SKIP: a FAIL there makes the gate not-PASS, and eval_rule treats
a non-PASS condition as "rule does not apply" and skips the whole body --
silently disarming every check in the guarded block. Hence strict_empty_rhs on
binary_operation and eval_guard_access_clause.
The unsatisfiable case emits a status rather than per-value comparison
records: there is no RHS value to report against, and building `from:` out of
a raw lhs entry panics when the lhs is a QueryResult::Literal (a `let`
literal), which three reporters treat as unreachable inside a comparison.
Tests pin both polarities, the disjunction composition, the when-condition
case, and the literal-LHS case. Each was verified to fail without the fix.
The two previous commits threaded `strict_skip: bool` and
`strict_empty_rhs: bool` to distinguish a rule-body assertion from a `when`
condition. That encoding was unsafe in two ways, and both bit.
A boolean argument carries no meaning at the call site -- `f(gac, resolver,
true)` says nothing -- and, worse, a missing one is invisible. Adding an enum
parameter turns every unthreaded path into a compile error instead.
Doing that surfaced a wrong PASS that the boolean version hid:
eval_when_clause threaded the flag into its Clause and NamedRule arms but not
ParameterizedNamedRule, and everything downstream defaulted to assertion
strictness. So a parameterized rule invoked as a gate --
`rule x when some_gate("p") { ... }` -- evaluated its body strictly, FAILed,
made the gate not-PASS, and eval_rule then treated the rule as inapplicable
and skipped the guarded body entirely. A violating template exited 0 where
base exited 19. Four shapes reached it: a plain parameterized gate, one whose
gate body held an empty-reference comparison, one nested in a rule body, and
one on a type block.
Introduce ClauseRole { Assertion, Gate } and propagate it through
eval_guard_access_clause, eval_guard_named_clause, eval_guard_clause,
eval_when_clause, eval_guard_block_clause, eval_type_block_clause,
eval_rule_clause, eval_rule, and eval_parameterized_rule_call. The compiler
identified all twelve sites, including three filter-predicate call sites in
eval_context.rs that no review had flagged: a filter selects values, so it is
a test rather than an assertion and an unevaluatable clause makes it select
nothing.
Role assignment per context:
- rules-file top level, named-rule resolution, `when`-guarded bodies, and
rule bodies -> Assertion
- `when` conditions, including parameterized gates, and filter predicates
-> Gate
- block clauses, type blocks, and rule clauses inherit from their caller
Behavior is unchanged for every case the previous commits already covered; the
only difference is the parameterized-gate path, which now matches base.
The regression test for it initially passed against the reintroduced bug
because its inline template used indented YAML, which PathAwareValue rejects
with a ParseError that the harness surfaced as a failed Result rather than a
wrong status. Rewritten in the brace form the neighbouring tests use, it now
fails without the fix and passes with it.
The parser accepts a leading `not` on a parameterized invocation and stores it on the call's named_rule (parser.rs:1145-1155), but eval_parameterized_rule_call returned the invoked rule's status unchanged and never read that flag. `not r(...)` therefore behaved identically to `r(...)`. Same defect class as the dropped clause-level negation on binary comparisons, in the one arm that commit "Apply clause-level negation to binary comparisons" did not reach. Demonstrated by three fixtures: `not inner(...)` and `inner(...)` were outcome-identical at exit 0, while the non-parameterized `not inner` correctly exited 19 -- so the mechanism existed and only the parameterized arm bypassed it. Apply the negation after evaluating the invoked rule, mirroring eval_guard_named_clause so both spellings agree: PASS inverts to FAIL under negation, a SKIPped rule fails closed where the reference is an assertion (a rule that never ran is not evidence for a negated claim), and otherwise the negation flips the outcome. The SKIP arm is role-dependent for the same reason it is there -- failing a gate would disarm the block it guards. Also tightens three under-constrained assertions in the tests added by earlier commits. They used assert_ne! and so admitted both SKIP and FAIL, which is exactly the distinction that decides whether a guarded body ran: a SKIP and a PASS both exit 0. The exact statuses were measured, not guessed, and each assertion now says why that value is the right one. One test whose name claimed only panic-freedom while asserting a status was renamed to match. Reported as WP-2 by a status-algebra analysis of the fold behaviour; the regression test was verified to fail without the fix.
Formatting only, no behaviour change. `cargo fmt --check` is a CI gate (pr.yml:46-54, actions-rust-lang/rustfmt@v1) and this branch was failing it, which would have shown as a red X the moment the workflow runs. Verified this is genuinely unformatted branch code rather than a rustfmt version artifact: upstream main at 57bbdbf passes `cargo fmt --check` cleanly under the same rustfmt, so the only files it can rewrite are ones this branch changed. eval.rs and eval_tests.rs also needed reformatting, but their formatting is folded into the two review-fix commits that follow because it lands on lines those commits write.
…ameterized gate SKIP
Two blocking review findings, both the same shape: a check that silently enforced
nothing. Both verified by mutation -- reverting each fix fails its new test and
nothing else.
EMPTY / !EMPTY on a boolean (eval.rs)
element_empty_operation's Bool arm computed `(*boolean).to_string().is_empty()`.
Neither "true" nor "false" is ever the empty string, so EMPTY on a boolean was
unconditionally false and !EMPTY unconditionally true. `Properties.Enabled
!EMPTY` was a clause that reads like a check and cannot fail for any input.
Worse, a comment block at the end of eval_tests.rs already claimed this "now
reports the same incompatible-type error as every other unsupported type". It did
not -- the arm was untouched -- and no test followed the comment, so the file
documented a fix that did not exist. That is worse than silence: a reader would
have trusted it.
Removing the arm lets a boolean reach the IncompatibleError every other
unsupported type already reached. boolean_empty_is_an_incompatible_type covers
all four combinations, and both axes matter: the old code made !EMPTY a silent
pass and EMPTY a silent fail, so one polarity alone would leave the other
unguarded, and true-vs-false is exactly the axis the old implementation was
insensitive to.
Parameterized gate returning SKIP (eval.rs)
eval_parameterized_rule_call sent any non-PASS, non-strict-SKIP result into a `_`
arm that converted a non-negated call to FAIL. With a single `when` condition
that is invisible, since FAIL and SKIP both make eval_rule drop the guarded body.
With two conditions it is not: eval_conjunction_clauses absorbs SKIP
(`Status::SKIP => {}`) but counts a FAIL, so one inapplicable gate condition
poisoned the whole `when` and dropped a body the passing sibling should have kept
enforced. ClauseRole::Gate is documented as "the block it guards is still decided
by the remaining conditions", so returning FAIL defeated the role propagation
this branch added for parameterized calls.
Negated calls deliberately keep falling through to `_`: `not r(...)` where `r`
did not apply must not report PASS on a check that never ran, matching
eval_guard_named_clause's fail-closed assertion case.
The regression test needed care. Its parameterized rule must SKIP, so its body is
a binary comparison whose left-hand query selects nothing -- `!empty` would FAIL
instead, since an unresolved query is EMPTY, and would reach `_` by a different
route without exercising the SKIP path at all. The first draft of the fixture did
exactly that and passed for the wrong reason.
303 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints
against upstream main.
A clause whose right-hand reference is a query that matched nothing had no
values to compare against, and the negated spelling treated that as vacuously
satisfied. `Property != %denied` with an empty `%denied` therefore reported
SKIP, and a rule whose only check was that clause enforced nothing while
exiting 0 -- indistinguishable from a pass at a CI gate.
Fail the clause instead when it is an assertion, and say why: the message names
the empty reference and points at the `when <reference> !empty { ... }` guard,
so an author who hits it is not left looking for a fault in the template.
The arm stays a SKIP for a gate. `eval_rule` reads any non-PASS condition as
"this rule does not apply" and drops the guarded body, so failing a gate that
cannot compare would trade one unenforced clause for an entire unenforced
block, still at exit 0. That asymmetry is also what keeps an intentionally
empty reference expressible: the `!empty` guard fails, the rule is skipped, and
the comparison never runs.
This is a deliberate behavior change. A ruleset that relied on the old SKIP
now fails until the expectation is stated with an explicit `!empty` guard.
Tests for the empty-reference semantics settled in review, at both the eval
layer and the CLI.
negated_comparison_against_empty_reference_fails replaces the test that asserted
the old SKIP; its comment records why the vacuous-truth reading was rejected, so
the next reader does not re-derive it.
an_empty_reference_can_be_guarded_with_a_when_not_empty_gate asserts the escape
hatch actually works rather than taking it on faith. If it did not, the change
would leave no way to express a permissibly-empty reference at all. It carries a
liveness row so a rule that never ran cannot satisfy it.
negated_empty_reference_in_a_when_condition_does_not_disarm_the_block pins why
the arm stays a SKIP for a `when` condition. It needs two ANDed conditions to
say anything: eval_conjunction_clauses absorbs a SKIP but counts a FAIL and
answers FAIL before PASS, so a FAIL only does damage when it outranks a sibling
condition that passed. A first version used one condition and passed whichever
status the arm returned, because eval_rule maps every non-PASS condition to a
rule-level SKIP. Mutating the arm to an unconditional FAIL now fails it, and the
comments on both arms were corrected -- they claimed the block was dropped by
failing the gate, when a SKIP drops it just the same.
Two validate cases cover the CLI, where the exit code is what a gate reads: the
unguarded rule must not reach 0, and the guarded one must. Their fixtures sit at
the resources/validate root because test_updated_summary_output evaluates every
file in rules-dir against every file in data-dir against a golden output, and
anything added there shifts it.
docs/CLAUSES.md gains the empty-reference case under binary operators, naming it
as a change from earlier versions and pointing at the `!empty` guard.
Three panic and assert messages in earlier commits interpolated nothing. This
crate is edition 2018, where a single-argument panic! is not a format string, so
`got: {message}` printed literally and the diagnostic lost the value it existed
to show.
"Two ANDed conditions" trips crate-ci/typos, which reads the "Ded" fragment as a misspelling of "Dead" and exits 2. The typocheck job runs over every file, so this failed the build on its own. Reworded rather than added to _typos.toml: the exception would be permanent and project-wide to excuse one piece of jargon in one comment, and "joined by AND" reads better anyway.
compare_write_buffer_with_file normalises captured output before comparing it to a
fixture, and did so by rewriting $HOME to ~ and then reducing ~/…/file.yaml to the
bare filename. Two bugs in that:
- $HOME was substituted by plain substring match, so a checkout whose path merely
*contains* $HOME was corrupted rather than normalised. With HOME=/home/u, the
path /local/home/u/repo/tests/resources/x.yaml became
/local~/repo/tests/resources/x.yaml, and reducing the tail then left /localx.yaml
welded together. /local/home is a real layout, not a hypothetical.
- a checkout outside $HOME produced no ~ at all, so the reduction never fired and
every comparison saw a full absolute path.
Either way the failures looked like product bugs. On this checkout 15 of the 96
validate tests failed for this reason alone, which is why that whole target had been
written off as environmental.
Now anchored on CARGO_MANIFEST_DIR, which is where get_full_path_for_resource_file
roots every resource path, so no assumption about the checkout location survives.
Anchoring there rather than matching bare absolute paths is deliberate: the SARIF
fixtures contain //docs.oasis-open.org/…/sarif-schema-2.1.0.json, and a regex over
absolute paths ending in .json would reduce that URL to its basename. It is the only
slash-bearing file reference in guard/resources, so the distinction matters for
exactly one fixture and is easy to lose.
replace_home_directory_with_tilde is deleted rather than fixed. Nothing else called
it, and no fixture in guard/resources contains a tilde, so the substitution existed
only as a sentinel for the regex that no longer needs one.
validate target 81 passed/15 failed -> 95 passed/1 failed. The remaining failure,
test_validate_with_failing_complex_rule, is a real output difference that the
sanitisation bug was masking, not a path artifact; it is diagnosed separately.
eval_when_condition_block hardcoded ClauseRole::Assertion for the guarded
block, so a `when` nested inside another `when` re-labelled the inner body as
an assertion. The role a caller passed to the outer block never reached the
leaf clauses.
The visible effect is the wrong direction: a parameterized gate whose body
should FAIL was reported PASS, because the leaf clause evaluated as an
assertion returned SKIP, and eval_conjunction_clauses absorbs SKIP
(`Status::SKIP => {}`) rather than counting it. One passing sibling then
answers PASS for the whole conjunction and the run exits 0. Repro:
rule denied(ty) { %ty == 'PUBLIC-INSECURE' } # parameterized: not run top-level
rule r { when %buckets !empty { when denied(%buckets) { ... } } }
Rather than default the parameter, make it required. A defaulted role reads as
a deliberate choice at every call site and silently reintroduces the bug the
first time someone adds a third caller; a required parameter makes the
omission a compile error. Both existing callers -- the WhenBlock arms of
eval_guard_clause and eval_rule_clause -- forward the role they were given.
Two tests. nested_when_inherits_the_enclosing_role pins the one-level case
against its unnested twin. the_role_reaching_a_leaf_clause_survives_every_nesting
generates the 12-cell cross product of {negated, positive} leaf x {0,1,2}
nesting depths x {assertion, gate} roles and asserts the status at each cell,
so the behaviour is pinned at every depth rather than at the one depth that
happened to be broken. SKIP is the signature of a leaked role: the matrix
asserts FAIL for assertion rows and PASS for gate rows, and a leak turns the
former into SKIP.
The gate fixture uses two ANDed conditions deliberately. With a single
condition, eval_rule maps every non-PASS condition to Status::SKIP, so a gate
that FAILs and a gate that SKIPs are indistinguishable and the test passes
even against an unconditional FAIL. Verified by mutation: reverting the fix
fails exactly these two tests.
report_all_failed_clauses_for_rules walked a record's children and returned whatever they reported. For a record whose failure is described by its own message and which has no failing children, that is the empty set: the message was constructed, stored, and dropped. Six record variants carry a message -- ClauseValueCheck, GuardClauseBlockCheck, BlockGuardCheck, WhenCheck, TypeCheck and Disjunction -- and only ClauseValueCheck had a rendering path. The empty-reference explanation added earlier in this branch is the case that made it visible. A comparison whose reference resolves to nothing now fails closed, and the record explains why, but an operator saw a bare exit 19 with no clause named. Worse, I claimed in review and in docs/CLAUSES.md that the failure "names the cause and the remedy". It did not reach a reporter, so the documentation was inaccurate rather than merely incomplete. Two changes. In the structured path, each message-bearing arm now falls back to its own explanation when no child reported: GuardClauseBlockCheck, TypeCheck and WhenCheck emit a Block whose error_message is the explanation, BlockGuardCheck prefers its stored message over the hardcoded retrieval string, and Disjunction falls back the same way. TypeBlock(FAIL) moves to its own arm because it has no message to fall back to and the shared arm would have to special-case it. In the console path, single_line collects Blocks that carry an explanation but no unresolved value -- the ones populate_hierarchy_path_trees cannot attribute to a resource, so pprint_clauses never reaches them -- and prints them under "Clauses that could not be evaluated:" with the context and the message. Tests. empty_reference_failure_explains_itself_in_the_output runs the console and JSON reporters over a rule whose reference is empty and asserts the text reaches the output. a_clean_run_does_not_print_an_unevaluated_clause_section is the negative control, so the first is not satisfied by a reporter that prints the section unconditionally. every_recorded_explanation_has_a_rendering_path counts the `message: Some` construction sites in eval.rs against a table of per-variant expectations, so adding a seventh message-bearing record without wiring a reporter fails the build rather than shipping another silent message. Verified by mutation: reverting the structured fallback fails both reporter cases, and suppressing only the console section fails only the console case.
awsmadi
force-pushed
the
pr/evaluator-outcome-cleanup
branch
from
August 17, 2026 20:50
21731be to
6db7bcc
Compare
compare_values matched Int/Int and Float/Float and nothing across the two, so
every mixed numeric comparison fell to the NotComparable catch-all. `Size > 10`
against a template carrying `Size: 50.5` reported "PathAwareValues are not
comparable float, int" and failed a compliant volume.
The wrong FAIL is the mild half. In a `when` condition the non-PASS becomes
Status::SKIP -- eval_rule maps every non-PASS condition that way -- and SKIP
exits 0, so the guarded body is dropped:
rule large_volumes_are_encrypted when ...Properties.Size > 10 {
...Properties.Encrypted == true
}
Size: 50, Encrypted: false -> FAIL, exit 19
Size: 50.5, Encrypted: false -> SKIP, exit 0, nothing reported
One character in a template turns the encryption rule off. Found by driving
branch coverage over the evaluator rather than by report: the ordering
operators' mixed-type arms had no test reaching them at all. Reproduced on the
upstream merge-base 57bbdbf, so it predates this branch.
The comparison is exact rather than `(i as f64).partial_cmp(f)`. i64 values
above 2^53 do not survive a round trip through f64, so the lossy spelling
answers Equal for 2^53+1 against 2^53. Casting the other way is safe once the
float is bounds-checked against 2^63: floor is exact on f64, and the bound is
2^63 rather than `i64::MAX as f64` because the latter rounds up to 2^63 and
would let a too-large float through to a saturating cast. NaN stays
NotComparable, which is the answer Float/Float already gave.
Verified by mutation twice: removing the arms restores NotComparable, and
substituting the lossy spelling fails on the 2^53+1 case specifically. No
existing test or golden file depended on the old behaviour.
…sserted
Branch coverage over the evaluator, not a bug report, is what surfaced the
mixed-numeric defect: the ordering operators had no test that reached them with
operands of differing types. These tests close that gap and two others in the
same shape.
the_comparison_matrix_over_operand_types_is_pinned evaluates 288 clauses -- six
operators against eight left-hand operand types and six right-hand literals --
end to end through eval_rules_file. What the grid asserts is the absence of a
wrong PASS: FAIL cells are pairings that are undecidable or genuinely false, and
PASS appears only where the comparison is both decidable and true. Two cells
look like typos and are not, so they are called out in the doc comment: a list
on the left distributes element-wise, and an absent property fails rather than
skips.
the_type_block_status_fold_is_pinned covers eval_type_block_clause's fold, which
has the same shape as eval_conjunction_clauses and the same SKIP absorption, and
had no test reaching any of its arms -- including the one deciding whether a
violating resource is reported at all.
a_skipped_type_block_is_indistinguishable_from_a_clean_run records a defect
rather than fixing it. A type block's clauses are resource-relative but its
`when` conditions are resolved from the file root, because
eval_type_block_clause evaluates them against the enclosing resolver before the
per-resource ValueScope exists. So
AWS::EC2::Volume when Properties.Size > 10 { Properties.Encrypted == true }
reads as "every volume over 10 GiB must be encrypted" and instead looks for
Properties at the file root, finds nothing, and skips -- reporting
not_applicable and exit 0 for every template, including the ones it was written
to catch. The root-qualified spelling is asserted beside it, since the contrast
is the whole finding.
An explanation was written onto that skip record and then removed. It could not
reach a reader: skipped rules arrive at the reporters as a set of names, so the
message would have been recorded and discarded, which is exactly the defect the
previous commits removed from five variants. every_recorded_explanation_has_a_-
rendering_path caught it, which is what that test is for. Surfacing it needs the
skip set to carry reasons and changes the `report` signature every reporter
implements; worth doing, not here. The comment on the arm and the test above
leave the next person a test to flip instead of a discovery to repeat.
unary_operation and the is_* family had no test walking them against the full set of value shapes, which is how the negation arm reached this branch with no coverage. 176 cells: eight operators, each in both polarities, against eleven operand shapes including the empty and absent cases that separate EMPTY from EXISTS. Two behaviours are pinned deliberately. EMPTY on a container answers, and on a scalar it is an incompatible-type error rather than a status -- an int is not empty, but calling it non-empty implies the question was meaningful, so neither status is right. And every negation must invert its positive form: a negated operator that answers the same status as the positive one has stopped discriminating, which is the shape of the role-propagation defect this branch opened with. IS_STRUCT is the surface keyword for CmpOperator::IsMap. Worth knowing before writing a matrix: `IS_MAP` parses as nothing and every cell using it errors, which reads like a broken operator rather than a wrong keyword.
awsmadi
force-pushed
the
pr/evaluator-outcome-cleanup
branch
from
August 17, 2026 21:36
6db7bcc to
8f2f6ea
Compare
The conditions were evaluated once, before the loop over matched resources,
against the enclosing resolver -- while the block's clauses were evaluated
against each resource under a ValueScope. Two scopes inside one construct, and
the split made the natural spelling a trap:
AWS::EC2::Volume when Properties.Size > 10 {
Properties.Encrypted == true
}
That reads as "every volume over 10 GiB must be encrypted". It looked for
`Properties` at the file root, found nothing, and skipped -- reporting
not_applicable at exit 0 for every template it was ever run against, including
the ones it was written to catch. A rule with no teeth and no diagnostic.
Conditions now run inside the loop, against the same ValueScope as the clauses
they guard, which also makes them per-resource: a resource the condition exempts
contributes to neither the pass nor the fail count, so an exempt resource cannot
shield a violating one, and a block that applied to nothing answers SKIP rather
than PASS.
The cost is the mirror image and it is real. A condition written as a literal
root-anchored path -- `when Resources.A.Properties.Size > 10` -- resolved before
and does not now, because ValueScope::query starts at the resource. Accepted for
two reasons. A condition over one named resource does not belong on a block that
iterates all of them. And the idiom real rulesets use is unaffected:
ValueScope::resolve_variable delegates to the parent, so `let volumes = ...`
followed by `when %volumes !empty` still resolves at the file root. Measured,
not assumed -- the test asserts all three spellings, so the trade is visible
rather than inferred from this message.
Nothing in the repository depends on the old behaviour: a search for type blocks
carrying a `when` found two occurrences, both fixtures added by this branch. No
shipped ruleset or example uses the construct.
Alternatives rejected. Leaving it and documenting the scoping keeps a rule that
silently never fires, which is the failure this branch exists to remove. Having
ValueScope fall back to the parent root when a query does not resolve locally
would keep both spellings working, but it makes every unresolved path retry
against a different scope, so a typo in a resource-relative path would silently
resolve somewhere else -- trading a visible skip for an invisible wrong answer.
A skipped rule reached the reporters as a name and nothing else. That is why an explanation written onto a skip record earlier in this branch had to be reverted: it was constructed and discarded, which is the defect the previous commits removed from five message-bearing record variants, reappearing in new code. every_recorded_explanation_has_a_rendering_path caught the attempt, which is what that test is for. Skips now carry their reason. `find_skip_reason` walks a rule's own record subtree for a block-shaped record that skipped with a message, and the three paths that report skips were widened to carry it: FileReport gained `not_applicable_reasons`, the console summary table prints the reason under the rule, and `GenericReporter::report` takes a `SkippedRules` map instead of a `HashSet<String>`. One map rather than a name set plus a parallel reason map, because the reason belongs to the skip and two collections keyed by rule name drift. Both new serialised fields are `skip_serializing_if` empty, so a run with nothing to explain produces byte-identical output to before -- no consumer of the JSON or YAML document sees a shape change. BTreeMap rather than HashMap so the ordering is stable across runs; the console printer sorts for the same reason, since the underlying map is unordered and would otherwise reshuffle its own lines. Two explanations put the mechanism to use, and they are the pair worth telling apart. A type block whose query matched nothing says the type is absent from the input, which is the ordinary and correct reason for a rule not to apply. A type block whose `when` condition exempted every matched resource says so, and that is the one worth a second look: a rule that never fires looks exactly like a rule that passes, because both report SKIP and exit 0. The counting in the invariant test changed too, and for a reason worth recording. It counted the literal string `message: Some`, and undercounted the moment a message was built in a `match` arm instead -- it read 14 where the real figure was 15. It now counts by exclusion: every `message:` field that is not None, not a type annotation, and not a forward of the author's own custom_message. That cannot be fooled by a new spelling, and it separates evaluator explanations from the custom-message feature, which is a different thing and always rendered. Verified by mutation. Severing find_skip_reason fails every case of a_skipped_type_block_explains_itself_in_the_output; disabling only the console loop fails exactly the two console cases and leaves the structured ones green.
`MapKeyFilterClause::comparator` was a `(CmpOperator, bool)`, which can express every operator in the language, while `map_keys_match` parses four: `==`, `!=`, `IN` and `NOT IN`. The gap was not free. `real_binary_operation` is reached only from `QueryPart::MapKeyFilter`, and it carried arms for `Ge`, `Gt`, `Lt` and `Le`. They recorded zero executions against a 288-clause matrix that uses those very operators, because no rules file can route an ordering comparison to that function. Dead code that looks live is worse than dead code that looks dead: those arms duplicate the comparison logic, so someone tracking down an ordering bug would find the arm calling `compare_ge`, edit it, and see no effect anywhere -- which is close to what happened while auditing this module. Deleting the arms and leaving the wider type would have kept them one parser change away from returning, and would have converted a currently-correct dead path into `unreachable!()` -- a panic where there had been an answer. A comment saying "unreachable" would have left the file permanently uncoverable and the trap intact. Narrowing the type is what makes the arms impossible rather than merely unused, so the enum lives next to the clause that owns it and the parser constructs it directly. The `Eq`-with-multiple-values promotion moved onto the type as `widened_for`, since it is a fact about the comparator rather than about the loop that used to perform it. One user-visible consequence, and it is an improvement rather than a regression: `parse-tree` now serialises a map key filter's comparator as `Eq` instead of the two-element sequence `[Eq, false]`, and `!=` as `NotEq` instead of `[Eq, true]`. Regenerating the three affected golden files changed exactly one site across them -- the other comparators in those documents belong to `AccessClause`, which keeps the pair. Hand-editing them first produced a wrong diff, because `AccessClause` also has a `compare_with` field and so cannot be told apart from a map key filter by its neighbours; regenerating from the binary was the reliable way. All four comparators verified end to end through the CLI, and the six `MapKeyFilterClause` fixtures in parser_tests.rs now name their comparator rather than spelling out a pair that could have been any of sixteen.
Every line below decides PASS, FAIL or SKIP and had never executed under any test. That is the shape both defects in the parent PR had, and the shape the mixed-numeric defect had: not an exotic input, just a decision nobody had asserted. Enumerating them from branch coverage and closing the reachable ones is cheaper than waiting for the next report. Three groups. A clause-level `not` in front of `EMPTY`, where the clause's negation composes with the operator's own and the arm applying the second had run in neither direction. A negated parameterized call whose invoked rule reached a verdict, so the negation has to invert it -- the SKIP case was already covered, these are the two that were not. And a disjunction in which every disjunct skipped, which must answer SKIP rather than PASS: PASS would report that one of the alternatives held when none was evaluated. The negation fixtures are fussier than they look and the doc comment says why. A first version used `not Resources.B.Properties.Missing EMPTY`, which gives the right answer through an entirely different path: `unary_operation` handles EMPTY on a filter-terminated or lone-variable query in a separate early-return block, and the clause-level flip lives inside that block. A plain key path never enters it, so the test passed while the arm stayed at zero. Coverage caught that the assertion was vacuous; reading the source alone would not have. None of the three turned out to be wrong, which is worth stating as plainly as a bug would be. An audit that reports only its finds cannot be told apart from one that stopped early. What remains uncovered in these four files is 13 error-propagation arms that require the resolver itself to fail, one `unreachable!()`, and the `EmptyRhs` arm already commented as unreached because its wrapper resolves the case into two other variants. Those are noted rather than chased: none of them decides a verdict for input a rules file can express.
Two of this branch's changes alter what a rule means, so they belong in the language docs rather than only in a commit message. Type blocks with a `when` condition were not documented anywhere -- the only mention of type blocks in docs/ is a passing note about variable scoping -- and this branch changed how their conditions are scoped. COMPLEX_COMPOSITION.md now covers the construct: clauses are relative to each matched resource, the condition is evaluated against that same resource, and the three outcomes (failure, pass, not applicable) follow from which resources the condition selected. It also states plainly what changed and how to migrate a condition that was written against the document root, since those resolved before and do not now. The distinction between the enclosing `rule`'s condition, still root-scoped, and the type block's own, now per resource, is called out because nothing else would tell a reader which is which. CLAUSES.md gains the numeric comparison rule under Binary Operators: integer and float compare as numbers, other cross-kind comparisons still cannot be decided and fail. The `when` case is spelled out with the volume example, because that is where the old behaviour cost enforcement rather than merely reporting a wrong verdict. KNOWN_ISSUES.md item 2 described incompatible-type comparison as an open problem. It still is for genuinely different kinds, so the entry stays; a note narrows it to exclude the numeric pair. Every rule sample added here was run through `parse-tree` to confirm it parses, and the worked example in COMPLEX_COMPOSITION.md was executed against five templates to confirm each documented outcome -- failure, pass, exempt, absent, and the decimal size. The last branch to write a docs claim about evaluator behaviour did not check it, and the claim was wrong.
awsmadi
force-pushed
the
pr/evaluator-outcome-cleanup
branch
from
August 18, 2026 16:28
8f2f6ea to
18fd823
Compare
The quietest wrong answer left in the evaluator. CloudFormation templates carry
numbers as strings routinely, and
rule large_volumes_are_encrypted when ...Properties.Size > 10 {
...Properties.Encrypted == true
}
against `Size: "50"` cannot compare a string to 10. The condition does not pass,
so the rule is reported not applicable and the unencrypted volume is never
checked. Exit 0, nothing named. `Size: 50` fails the same rule.
The rule still does not enforce, and it cannot be made to from here. On a
condition both FAIL and SKIP drop the block being guarded -- and a gate FAIL is
worse, because the condition fold counts it and it outranks sibling conditions
that passed, dropping a body those siblings would have kept enforced. That is why
the empty-reference arms added earlier in this stack are role-aware rather than
failing outright. Telling "could not decide" from "decided false" where it
matters needs a status meaning "could not tell", and `Status` has three variants,
none of which is that. Introducing one here is the Outcome lattice, which is the
next PR in the stack; doing it twice would leave the second attempt unpicking the
first.
What is available is saying so, which the skip-reason plumbing added two commits
ago now makes possible. The change is entirely reporter-side: `find_skip_reason`
recognises a comparison that failed *and* recorded an explanation.
That is a precise discriminator rather than a heuristic. Two things record an
explanation on a comparison -- a reference that resolved to no values, and
operands that cannot be compared -- and both mean the clause could not be
decided. The ordinary failure arm records `message: None`. So a rule that
legitimately does not apply stays silent, which is what makes the message worth
reading: on a large ruleset, a line under every inapplicable rule would be noise.
Both halves are asserted, in console and in structured output.
When the third state exists, `an_undecidable_condition_says_so_in_the_output` is
where the stronger behaviour gets asserted -- a test to flip rather than a
discovery to repeat.
`eval_conjunction_clauses` built its record context with `std::any::type_name::<T>()`. That function is documented as being for diagnostics with no stability guarantee across versions, and this use was not confined to diagnostics: the result goes into a record context, which reaches verbose output and is compared byte for byte by four golden-file tests. rustc 1.77.2 renders `cfn_guard::rules::exprs::GuardClause<'_>` and later versions render the path without the elided lifetime. The golden files hold the form without it, so the suite passed on the pinned toolchain and failed on every newer one. That is not hypothetical maintenance debt -- those four tests had to be skipped to measure branch coverage at all, since branch counters require nightly and a single failing binary aborts the run before the later ones execute. Taking the path before the generic arguments is stable on both. No golden file changed, which is the useful confirmation: they were already written for the normalised form, and 1.77.2 was the deviation. The full suite now passes on nightly as well as on 1.77.2, so coverage runs no longer need to skip anything. Rejected alternative: writing a fixed `disjunction` with no type at all is also stable, but the type is the only thing distinguishing one disjunction record from another inside a nested rule. The test asserts the absence of generic arguments rather than an expected literal. Pinning the full path would turn moving these types between modules into a test failure for no reason; what must not come back is the part that varies by compiler.
`query_retrieval_with_converter` resolves a variable used where a map key is
expected, then peeks at the following query part: an index there says which of
the resolved keys to use. Having consumed it, the recursion advanced by one
anyway, so the index was applied a second time -- to the value the key had just
selected.
let names = Resources.Pointer.Properties.Targets[*]
rule r { Resources.%names[0].Type == 'AWS::S3::Bucket' }
`%names[0]` picked `BucketA` correctly, then `[0]` was applied to `BucketA`
itself, the query resolved to nothing, and every part after `[0]` was discarded.
The rule failed a template it should have passed.
Two things kept it hidden. The form without an index -- `Resources.%names.Type` --
always worked, so the pair reads as a rule-authoring mistake rather than an
evaluator defect. And an unresolved query is reported as a retrieval failure
against the input, so the output blames the template.
Worse in a condition, and for the reason this stack keeps running into: an
unresolved gate does not pass, a rule whose condition does not pass is reported
not applicable, and its body never runs. The last case in the test is that shape --
before the fix it was SKIP at exit 0 with an unreported violation inside.
Byte-identical to origin/main in that block, so it predates this work. Found by
enumerating the QueryPart arms with no coverage in the query resolver, not from a
report; the arm handling an index after an interpolated key had never executed.
Verified by mutation, and by the four other cases in the test: the fix must let
the query resolve without making it resolve to something that passes regardless
of the value, so a wrong expected value still fails, an out-of-bounds index still
fails closed, and the no-index form still checks every key.
`binary_error_in_msg` computed its cut-off as `max(bc.to.len(), 5)`, which is never below the number of values. So the loop meant to stop early never stopped, and the branch reporting a `Total` was unreachable. A rule comparing against a denylist of five hundred entries printed all five hundred, in every failure message, for every non-compliant resource. The dead branch is what gives the intent away: it exists to say how many values there were when not all of them are shown. `min` restores that, and `take(cut_off)` replaces the loop, which also removes an off-by-one -- breaking on `idx >= cut_off` after pushing meant six values were collected for a cut-off of five. The right-hand side has to resolve to many separate values to reach this. A literal list is one value however long it is, so `IN ['a', ..., 'l']` printed a single twelve-element value and never came near the truncation path; the fixture compares against a variable spanning nine resources instead. An earlier version of the test used the literal and proved nothing, which is worth knowing before editing it. The test scopes its assertions to the ComparedWith line rather than the whole report, because the report echoes the offending template and the fixture's nine names all appear in that snippet -- searching the full output would find the withheld values there and pass regardless. Both halves are asserted, since a reporter printing no values at all would satisfy the withheld check on its own. Found by enumerating uncovered branches in the reporter rather than from a report. Verified by mutation.
awsmadi
force-pushed
the
pr/evaluator-outcome-cleanup
branch
from
August 18, 2026 16:56
18fd823 to
8266784
Compare
`Ports <= 100` certifies `Ports: []` as compliant, and so do `>`, `<` and `>=`. Same class as the IN case recorded in 6912f15 but a different impl -- CommonOperator, not contained_in -- so a fix for one does not touch the other. Pre-existing in v3.2.0 on all three pins. Measured with numeric fixtures and both controls on every rule, so each row is interpretable rather than confounded by string ordering: rule Ports:[80] Ports:[8080] Ports:[] Ports <= 100 0 19 0 Ports > 100 19 0 0 Ports < 100 0 19 0 Ports >= 100 19 0 0 `<= 100` and `> 100` are exact logical negations and both certify the same empty list. That is a stronger argument than the IN/NOT-IN pair: universal quantification over an empty set defends "every element satisfies P" for both P and not-P, but it cannot defend certifying `x <= 100` and `x > 100` for the same x. Lt and Ge measured separately and behave identically, which is expected since all four instantiate the same CommonOperator. The gate hazard is now measured rather than inferred, which is what settles where a fix belongs: rule r when ...Ports <= 100 { ...Name == 'safe' } on Ports: [] -> exit 19 It exits 19 *because* the vacuous PASS opens the gate and the body then catches the violating name. Making the comparison non-PASS in the comparator turns that into exit 0 with the body dropped -- one unenforced clause traded for an entire disarmed block, which is how two earlier attempts on this branch regressed. So the decision has to sit at the eval.rs EmptyLhsCollection arm where the clause role is visible. Corrects two things in 6912f15's comment. It described the defect as IN-specific, which would leave a future reader believing the ordering operators are fine; and it implied the comparator-side change is large. `flattened` -- the call that destroys per-result provenance and so blocks a per-element guard -- has exactly two callers, both inside CommonOperator::compare, so converting them to `selected` is local to one function rather than a change to every list comparison. The contract change is still real, just smaller than I wrote. Recorded at both sites: a matrix and the reasoning in contained_in, and a doc comment on CommonOperator::compare itself, since that is where someone fixing ordering operators would look. Plus an #[ignore]d reproduction asserting the two negations cannot both pass, and a non-ignored control pinning both polarities on populated collections. 358 unit tests, 4 ignored -- three live reproductions and one upstream failure from 2023, distinguished in the roster comment. Comment and test only, no behaviour change; 456-pair corpus differential 0 differences against the real parent.
The empty-collection FAIL drops the body of a rule that gates on it through a named
reference, even when that body would pass:
rule vac_eq { Resources.*[ Type == 'AWS::S3::Bucket' ].Properties.Tags == 'Owner' }
rule body when vac_eq {
Resources.*[ Type == 'AWS::S3::Bucket' ].Properties.Name == 'publicbucket'
}
with `Tags: []` and `Name: publicbucket`, so `body` is satisfiable.
v3.2.0 exit 0 compliant = [body, vac_eq]
this branch exit 19 not_compliant = [vac_eq] not_applicable = [body]
Measured on v3.2.0, on a9c7f96, and on HEAD: identical from 2224cb1 onward, so this
branch introduced it. Found only because a reviewer measured the satisfiable-body case --
every earlier gate fixture used a failing body, so "gate opened, body failed" and "gate
closed, body dropped" both exited 19 and the exit code could not distinguish them.
Cause is the named-rule boundary, not the comparator. eval_context.rs:1116 evaluates a
named rule's body with ClauseRole::Assertion whatever the reference site is, so
`role.is_strict()` is true even when the rule is being used as a gate; the arm fires,
`vac_eq` FAILs, and eval_rule reads the non-PASS condition as "does not apply".
Not reverted. Reverting restores the original wrong PASS -- `Tags == 'Owner'` certifying
`Tags: []` as compliant -- and a wrong PASS on a policy gate is worse than a wrong FAIL.
Scope is bounded: only rulesets that gate on a named rule whose body compares an empty
collection; the same shape with populated data is 0 on every pin.
Also corrects two false statements in the committed comments, both found by the same
review:
- "Failing here would close the gate and drop the guarded body" is wrong for a
*syntactic* `when`. `role.is_strict()` is false for a gate, so the arm contributes
nothing and the gate is left to its other conditions. Verified: a `when` gate on an
empty collection exits 19 with the failure on /Properties/Name and `not_applicable: []`
-- the gate opened and the body ran. Same for an IN gate; the negated pair inverts
correctly. So gate-closing is NOT the reason IN and the ordering operators are unfixed.
The reason is only that they never emit EmptyLhsCollection and so never reach the arm.
- A comment described `vacuously_satisfied` as recording state so the caller can return
SKIP. That variable was removed by fb64016 and now exists only in comments. What keeps
the positive case out of the disjunction trap is the FAIL push itself, which puts an
entry in `statues` so the fold never sees `fails == 0`.
358 unit tests, 5 ignored -- four live reproductions, two of them this branch's, and one
upstream failure from 2023, all distinguished in the roster comment. Comment and test
only, no behaviour change; 456-pair corpus differential 0 differences against the real
parent.
The test asserted `passes <= 1` over `{Ports <= 100, Ports > 100}` -- how many of two
exact negations return PASS. It was the only one of the parked reproductions with a
relational rather than absolute assertion, and `passes <= 1` is satisfied by `passes == 0`
as well as by 1. Two independent ways to reach green with the defect intact, both measured:
- Query rot. Rename one character in the Type filter and both rules SKIP. SKIP is not
PASS, so passes == 0 and the assertion goes green while the test is blind.
- Aggregation launder, and this one is worse because the rule text stays correct. Add one
resource that genuinely violates `<= 100`: the rule aggregates across resources, `> 100`
becomes FAIL overall, passes drops to 1, green. Blame paths confirm the empty resource is
never named while both negations still certify it -- exactly what the test exists to
detect.
Since the test is #[ignore]d the failure mode is not a green suite hiding a defect; it is
someone landing a partial fix, running `cargo test -- --ignored`, seeing green, and
concluding the class is resolved.
Going absolute per operator closes the second but NOT the first: rot yields SKIP and
`SKIP != PASS`, so an `assert_ne!(PASS)` claim alone still passes blind. Measured. The
shape that resists both asserts liveness first -- populated-low PASS, populated-high FAIL,
for both operators -- so under rot the liveness row fails before any claim is reached.
Both fixes verified by mutation: re-introducing the renamed Type filter now fails on the
liveness assertion naming the query, and re-introducing the extra violating resource still
fails on the claim.
Also strengthens the ordering control's documentation, which undersold it. It pins numeric
rather than lexicographic comparison, which matters because an earlier measurement of this
operator class was discarded as confounded by string ordering. `[8080]` discriminates
nothing -- false under both readings. `[80]` and the newly added `[9]` are the
discriminating rows: false lexicographically, true numerically, so a PASS on either can
only be numeric. `[9]` is the sharper of the two since a single digit cannot be read as
"shorter string sorts first".
And records a known weakness in the disjunction reproduction rather than leaving it
implicit: `assert_eq!(FAIL)` is satisfied by any part of the rule failing, so changing
`Tags: []` to `Tags: ['Owner']` reaches green without a fix. Left as-is -- the rule name
signals the intent, and tightening it would need liveness rows the fixture has no room for,
since the empty list is the whole point of it.
358 unit tests, 5 ignored. Test-only change; 456-pair corpus differential 0 differences
against the real parent.
…atus The reproduction added in 7aa4757 asserted `Status::PASS` for a two-rule file. That target was both unreachable and wrong. Unreachable: `eval_rules_file` evaluates every top-level rule with ClauseRole::Assertion unconditionally (eval.rs:2354) and folds `fails > 0 -> FAIL` (eval.rs:2379). `vac_eq` is the gate for `body` and a top-level rule in its own right, so it fails strictly whatever happens at the gate, and no file containing it can be PASS. Keying the rule-status cache on (rule, role) fixes the gate and cannot change that fold -- so the test would still have failed after the fix it documents, at the same line, for a reason its comment did not describe. Measured discriminator: the syntactic `when` spelling of the same condition reaches exit 0 with the body compliant; adding that condition as its own top-level rule gives exit 19 while the body stays compliant. Wrong: the PASS it demanded is the empty-collection wrong PASS. On v3.2.0 this file passes because `Tags == 'Owner'` reports *compliant* against `Tags: []` -- verified against origin/release/3.2.0, `compliant: ['body', 'vac_eq']`, not_applicable empty. So making the test green meant reintroducing the defect while holding a green suite that said otherwise. The comment fifteen lines above the assertion already argued against exactly that. Now asserts the reachable, wrong-PASS-free property: `body` is satisfiable, so it must not be reported not-applicable. `vac_eq` failing is correct and stays correct. Reads `not_applicable` from the constructed report rather than an exit code, because exit 19 cannot distinguish "gate opened, body failed" from "gate closed, body dropped" -- the distinction this whole finding turns on. An intermediate version of this rewrite used the inline `when` spelling to dodge the fold. It passed, which was the tell: the inline form is not the defect, so that fixture pinned nothing. Every named rule in Guard is also top-level, so the named-rule shape cannot avoid the strict fold and the assertion has to move instead. Liveness row kept and verified load-bearing, per the shape established in 11f3c17: an absence claim is satisfied when nothing runs at all. Mutating the Type filter now fails on the liveness assertion naming the query, rather than letting the claim pass blind. 358 unit tests, 5 ignored, all five still failing as documented. Test-only change; 456-pair corpus differential 0 differences against the real parent.
The doc comment cited v3.2.0's exit 0 as the behaviour to restore. That exit 0 comes from `Tags == 'Owner'` reporting compliant against `Tags: []` -- the empty-collection wrong PASS this branch removes -- so it is not a target. Names the not_applicable entry as the regression instead, and states why the exit code cannot express it: 19 covers both 'gate opened, body failed' and 'gate closed, body dropped'. Comment only.
The claim was `!dropped.iter().any(|name| name == "body")` -- keyed on a string literal, so
renaming the gated rule in the fixture makes it vacuously true. Measured: rename it and the
test goes green with the defect fully live, the renamed rule sitting in `not_applicable`
where nothing looks for it.
The liveness row cannot catch this. `live.is_empty()` is itself name-independent, so it
holds under any renaming.
Now asserts `dropped.is_empty()`. Name-independent, still fails today on `["body"]`, and
the stronger claim anyway: with the gate condition failing, NOTHING should be dropped, not
merely nothing called `body`.
Three mutations verified: baseline still fails on the claim; the rename now fails naming
`["renamed_gated_rule"]` where it previously passed; query rot still fails on liveness
first.
Also records what liveness does and does not buy, which I had stated too broadly. Rot makes
`body` not-applicable, which VIOLATES an absence claim rather than satisfying it -- so for
that mutation liveness gives a clearer diagnostic, not false-green protection. The mutation
that passes blind is the rename. The general form is narrower than "absence claims need
liveness": an absence claim needs a mutation probe on its own matching key, and liveness
guards the query rather than the key.
And corrects a false claim about the language. The comment said "every named rule in Guard
is also top-level"; a parameterized rule lands in a separate `parameterized_rules` vec
(exprs.rs:283) that the file-level fold never iterates (eval.rs:2352), so
`rule vac_eq(unused)` gated by `when vac_eq("x")` escapes the fold entirely. It escapes the
defect too, which is the real reason not to use that shape here -- the parameterized
boundary threads the reference-site role correctly, so a fixture built on it pins the
working path, and that path is already covered by
`parameterized_rule_used_as_a_gate_does_not_disarm_the_block`. So option 1 was structurally
available and still wrong to use, for a better reason than the one I gave.
358 unit tests, 5 ignored, all five failing as documented. Test-only change; 456-pair
corpus differential 0 differences against the real parent.
Six line-number citations in shipping comments pointed at the wrong code. Verified:
eval.rs:1353 cited as the SKIP `unreachable!()` -> is a closing brace; real site 1484
eval.rs:1325 cited as the composed not-flag -> real site 1446
eval.rs:2082 cited as eval_rule's non-PASS branch -> lands in eval_type_block_clause
(three sites: eval_tests.rs x2, outcome_tests.rs x1)
eval.rs:1636 cited as the `passes > 0` logic -> is a function signature line
eval.rs:867 cited as the EmptyRhsVacuouslyTrue arm -> is a comment line
Four of these were wrong when written, not drifted -- measured from the merge base rather
than the tip, since scanning from the tip cannot distinguish "drifted" from "never
correct". The 2082 sites have mixed provenance: correct when 44b70f6 wrote them, already
wrong when ca521cc and 2224cb1 did, and indistinguishable at the tip because the string is
identical.
The 2082 case is the one that mattered. It lands on a structurally identical
`return Ok(Status::SKIP);` inside a different function, so a reader who follows it sees
plausible code in the wrong scope with no signal they are lost. All six citations resolved
to real lines, so a dangling-pointer check would have found none of them.
Fixed by naming the function or the expression rather than renumbering. Renumbering
restarts the same clock -- any edit above a citation invalidates it, and the rot is
invisible because the pointer still resolves. A symbol name does not move when code moves
above it.
Untouched: the eleven citations that are exact -- parser.rs:969, exprs.rs:283,
eval_context.rs:1095/1116/1130, eval.rs:2352/2354/2379, evaluate.rs:293-307. Accurate
citations outnumbered broken ones about 2:1, so the failure is not sloppiness; it is that
nothing checks. That is note 28 one turn on: comments describing *locations* rot without
the described code changing at all.
Comment-only. 358 unit tests, 5 ignored; cargo build reports 0 errors.
fde86e1 fixed six wrong citations by naming symbols instead of line numbers, and in doing so added +3 lines to eval.rs above three citations that were correct and therefore left alone. All three now point at the wrong code: eval.rs:2352 -> resolver.start_record(...) real site 2355 eval.rs:2354 -> let mut passes = 0; real site 2357 eval.rs:2379 -> } real site 2382 Verified at the tip. The citing comments are in eval_tests.rs and were untouched by that commit, so they still assert the old numbers. This is the argument from fde86e1's own message, demonstrated by fde86e1: "renumbering restarts the same clock." I did not renumber -- but a comment-only edit ABOVE a surviving citation invalidates it just as effectively, and those three were left alone precisely because they were correct. Correct-and-untouched is not a stable state when the file is edited above them. Three of eleven, from a 17-line comment-only commit. Fixed the same way: all three sites are in `eval_rules_file`, so naming that function plus the specific expression covers all three and cannot shift again. Left as line numbers, verified exact at the tip: eval_context.rs:1095/1116/1130, parser.rs:969, exprs.rs:283, evaluate.rs:293. These point into files this branch does not edit, so no clock this branch runs can move them -- a materially different risk profile from the eval.rs self-references, which rotted with no code change at all. Comment-only across one file. 358 unit tests, 5 ignored; cargo build 0 errors. Unverifiable on this host, recorded rather than claimed clean: `cargo fmt --check` is a required PR job (pr.yml:46-54) and neither `cargo fmt` nor a rustfmt binary exists here. There is no rustfmt.toml, so defaults apply, and comment reflow (`wrap_comments`) is nightly-only and off by default -- so hand-edited comment text should be invisible to it. Confirm with `rustup component add rustfmt && cargo fmt --all -- --check` on a host where the pinned toolchain is installable.
…g them was false a24cf65's message justified leaving eval_context.rs:1095/1116/1130 as line numbers: "These point into files this branch does not edit, so no clock this branch runs can move them." That is false, and I asserted it without running the diff: git diff --stat 57bbdbf..a24cf65 -- guard/src/rules/eval_context.rs 1 file changed, 53 insertions(+), 4 deletions(-) Five hunks across two commits, three of them above all three citations. Worse, the clock had already run. At the merge base those same three numbers land on an error return, a blank line, and an unrelated `resolve_function` call. They are correct at the tip only because an earlier commit's +7 shift happened BEFORE the citations were written -- exact by timing, not by construction. Same class as the eval.rs self-references fixed in fde86e1 and a24cf65, and the next commit touching eval_context.rs above line 1130 would have broken all three silently. The claim was true for the other three files: parser.rs, exprs.rs and evaluate.rs have an empty diff against the merge base, verified. Fixed by naming symbols at all seven citing sites -- `rule_status` for the ClauseRole::Assertion call and the rules_status cache, `resolve_variable`'s `scope.literals` branch for the Literal binding. Every citation into a file this branch edits is now symbol-named; the four that remain point only into the three genuinely untouched files. Comment-only across two files. 358 unit tests, 5 ignored; cargo build 0 errors. Rounds 21-23 were one failure mode at increasing remove: wrong citations, then a correct citation invalidated by an edit above it, then a correct citation whose stated reason for being safe was wrong. No test or build could see any of them. The pattern worth keeping is that the justification got less scrutiny than the thing it justified -- "this file is untouched" reads as a structural guarantee, and I wrote it as one without spending the one command that would have checked it.
Formatting only, no behaviour change. `cargo fmt --check` is a CI gate (pr.yml:46-54, actions-rust-lang/rustfmt@v1) and this branch was failing it in five files: eval.rs, eval/operators.rs, eval/outcome_tests.rs, eval_context.rs and eval_tests.rs. Run under the toolchain the repository pins rather than whatever is on PATH. rust-toolchain.toml specifies 1.77.2, and its rustfmt is 1.7.0-stable; a newer rustfmt disagrees with it on this codebase, so checking with the wrong one produces both false failures and false passes. Verified: upstream main passes under 1.7.0, and the five files above genuinely did not.
The three Comparator impls each decided independently what an empty collection
meant, and two of them certified one as compliant:
- EqOperation emitted EmptyLhsCollection (correct)
- InOperation built an affirmative Success(ListIn) from an empty `diff`
- CommonOperator iterated an empty vec and pushed nothing, so the clause
vanished and the fold read zero results as "nothing to check" -> PASS
`Ports <= 100` and `Ports > 100` are exact logical negations and both returned
PASS on `Ports: []`. Universal quantification over an empty set defends "every
element satisfies P" for both P and not-P, but it cannot defend certifying
x <= 100 and x > 100 for the same x. Both defects pre-existed in v3.2.0.
Added `elements_or_record_empty` as the single place a comparator turns "nothing
to compare" into a record, and routed all three impls through it, EqOperation's
two existing sites included -- otherwise it is a helper used by two of three
rather than a unification.
The comparator still does not decide what an empty collection *means*. That stays
at the EmptyLhsCollection arm of binary_operation, which fails it as an assertion
and contributes no entry for a gate. Deciding it in the comparator closes any
`when` gate built on it and drops the guarded body, which is how two earlier
attempts regressed; that hazard is now pinned for CommonOperator specifically by
an_empty_collection_in_an_ordering_gate_does_not_disarm_the_block.
CommonOperator now uses `selected` for both sides instead of `flattened`, which
spliced list elements into one flat vec and destroyed per-result provenance -- an
empty list left no entry to attribute to the resource that had it. `flattened`
had exactly two callers, both here, and is deleted. Both sides rather than just
the left, because `%limit >= Ports` has to answer for `Ports: []` the same way
`Ports <= 100` does or the defect stays reachable by writing the clause
backwards, which is the argument EqOperation already makes for its mirrored
empty-RHS guard.
Two parked reproductions now pass and are un-ignored:
in_does_not_certify_an_empty_collection and
ordering_operators_do_not_certify_an_empty_collection. Verified they failed at
4febfbc and pass here, so they are testing the fix rather than having been
green all along. Their doc comments said "ignored rather than fixed because ..."
and are rewritten; leaving them would have been the comment-rot class.
One deliberate behaviour change beyond the wrong-PASS fixes: `Tags not in
['Owner']` on `Tags: []` moves from FAIL to PASS. No element of [] is in
['Owner'], so the old FAIL rejected a compliant template. It routes through the
same "negated clauses contribute nothing" path `!=` already used, inheriting that
path's known disjunction-absorption hazard rather than introducing one -- that
one is still open as a_vacuous_negated_clause_does_not_absorb_a_disjunction.
Three new tests, each with liveness rows first so a query that stopped selecting
cannot make the claim go green blind.
324 lib tests, 0 failed, 3 ignored (was 319/0/5). test_command's two verbose
tests still fail identically at 4febfbc and at 320251c -- rustc renders
`type_name` lifetimes differently from the checked-in fixture, unrelated.
rule_status evaluated a referenced rule's body with ClauseRole::Assertion whatever
the reference site was, so ClauseRole could not cross the named-rule boundary. A
`when` condition referencing a rule whose body held an unevaluatable clause got a
FAIL from that clause, the rule came back non-PASS, and eval_rule then dropped
every check in the guarded block while still exiting 0 -- one unenforced clause
traded for a whole disarmed block, the same hazard recorded on the
EmptyLhsCollection arm in eval.rs, reached one level further out.
Two changes, and both are needed:
- carry the reference site's role through rule_status into eval_rule, so an
unevaluatable clause in the body gets the strictness the *reference* deserves:
a failure for an assertion, inapplicable for a gate. That is the Unevaluatable
split Outcome::to_status describes.
- key rules_status on (rule, role) instead of the rule name. The same rule
reached from a body and from a gate are two different questions; sharing a
cache slot lets whichever reference runs first decide the answer for the other.
a_named_rule_gate_does_not_drop_a_satisfiable_body now passes and is un-ignored,
leaving 2 ignored (down from 3).
The cache key is load-bearing, measured rather than assumed. Mutating rule_status
to keep the role parameter but look up and store under a fixed
ClauseRole::Assertion -- role threaded, cache keyed on the name -- leaves the
original reproduction green and fails only the new
the_same_named_rule_answers_both_roles_independently. So the reproduction alone
would have ratified a half-fix whose answer depends on rule declaration order.
That test puts the assertion reference before the gate reference deliberately;
swapping them hides the defect.
Also adds a_named_rule_gate_does_not_soften_a_real_violation for the adversarial
direction: carrying Gate strictness into a body must not launder a real violation
into a SKIP, which would open gates that should close. It does not, because
ClauseRole is consulted for exactly one outcome and a populated violating
collection is Violated, which is FAIL under either role. Pinned because reasoning
it out is not the same as measuring it.
ClauseRole becomes pub(crate): EvalContext::rule_status now takes one and that
trait is pub(crate), so pub(super) made the method expose a more private type than
itself and clippy rejected it as private_interfaces.
327 lib tests, 0 failed, 2 ignored. cargo fmt --check clean. Zero new clippy
lints against upstream main.
The EmptyLhsCollection arm's commentary said the fix needed "the reference-site role threaded into rule evaluation and the status cache keyed on (rule, role)", described the named-rule regression as live, and asserted ClauseRole "cannot carry it across a named-rule boundary". The previous commit did exactly that threading and keying, so all three statements were false as written. Rewritten to say what is now true: the boundary carries the role, the named-rule regression is fixed and pinned, and the twice-reverted SKIP is worth a third attempt with those two tests as its oracle -- but is not a local edit, because statues is a PASS/FAIL vector whose fold treats SKIP as unreachable!(). That makes the Outcome conversion the actual prerequisite now, and the comment says so. Comment-only. No behaviour change. 327 lib tests, 0 failed.
This is the Status type-system migration the ClauseRole and Outcome work was
building toward, and it closes the last defect this branch introduced.
The per-value fold in eval_guard_access_clause counted passes and fails and
matched `Status::SKIP => unreachable!()`, so a clause that was neither satisfied
nor violated -- a negated comparison with nothing to compare -- had no way to say
so and contributed no entry at all. That is not neutral: with match_all the fold
then saw `fails == 0` and returned PASS, and PASS short-circuits
eval_conjunction_clauses, so
Tags != 'Owner' or Name == 'safebucket'
reported a violating template as compliant. The vacuous first disjunct satisfied
the whole `or` and the real check never ran.
The fold now lifts each entry with Outcome::from_status and folds with
Outcome::all/Outcome::any, whose identity is NotApplicable. A fold over zero
elements therefore returns "did not apply" rather than "satisfied", and only
Satisfied absorbs under `or`, so an inapplicable disjunct cannot stand in for one
that passed.
The empty-collection arm is split three ways by role and polarity:
- positive assertion -> FAIL (unchanged)
- negated assertion -> SKIP, so it cannot absorb a disjunction
- gate, either polarity -> PASS, because eval_rule reads any non-PASS condition
as "does not apply" and drops the guarded block
That last branch is why three earlier attempts were reverted, including one made
while writing this commit: pushing SKIP for gates fails five gate tests at once
(a_vacuous_negated_gate_still_opens_and_runs_its_body,
an_empty_collection_in_a_when_condition_does_not_disarm_the_guarded_block,
an_empty_collection_in_an_ordering_gate_does_not_disarm_the_block,
a_mirrored_empty_collection_in_a_when_condition_does_not_disarm_the_block,
a_vacuous_negation_nested_in_a_when_block_still_runs_the_inner_body). The split is
only sound because the previous commit made the role survive the named-rule
boundary.
a_vacuous_negated_clause_does_not_absorb_a_disjunction now passes and is
un-ignored. One test remains ignored in the crate -- test_string_in_comparison, an
upstream failure parked in 2023 and failing identically on the pre-branch tree.
Two user-visible reporting changes, both exit-code neutral: a rule whose only
clause is a vacuous negated comparison moves from compliant to not_applicable, and
`not in` over an empty collection does the same. Arguably the honest answer, since
nothing was verified.
outcome.rs loses its blanket #[allow(dead_code)] and the outcome_tests tripwire
that asserted the module was unwired is deleted, per its own instructions. Three
methods -- blocks, closes_gate, negate -- keep a targeted allow with a note naming
what would call them: statuses still enter the fold as Status and are lifted, so
nothing yet asks an Outcome directly, and gate closure is still decided by
eval_rule comparing against PASS.
327 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints
against upstream main.
The empty-collection work on this branch is combinatorial: what a clause reports
depends on the operator, the polarity, whether the clause is an assertion or a
gate, and whether the left-hand collection is empty, satisfying or violating.
Every defect found on this branch was a cell nobody had looked at -- IN and the
four ordering operators certifying an empty collection, the mirrored spelling, the
negated assertion absorbing a disjunction, the named-rule gate dropping its body.
Individual tests pinned individual cells, so the surface was covered by accretion
and there was no way to tell what remained.
the_empty_collection_decision_surface_is_covered_exhaustively enumerates it:
6 operators x 2 polarities x 2 roles x 3 collection states = 72 cells
Rows are generated by nested loops over the axes rather than written out, so a
missing row is impossible rather than unlikely, and the row count is asserted
against the product so dropping an axis value fails loudly instead of quietly
shrinking coverage. Failures are collected and reported together with the axis
values, so a regression names every cell it broke rather than the first one.
Expected values come from expected_status, written as the specification -- role,
polarity and emptiness composed from first principles -- not transcribed from
observed behaviour. Writing it the other way round would make the matrix agree
with whatever the code does, including the defects.
Discriminating power measured by mutation rather than assumed, because a
72-cell table that passes first try is exactly what a vacuous test looks like:
- gate arm pushing SKIP instead of PASS: 12/72 fail, precisely the gate x empty
region across all six operators and both polarities.
- positive assertion over empty pushing PASS instead of FAIL, which is the
original wrong PASS: 6/72 fail, all six operators.
role_does_not_change_a_populated_comparison asserts separately that a real
comparison over real values gives the same verdict in either role. ClauseRole
exists to decide what an *unevaluatable* clause reports -- Outcome::to_status
branches on it for exactly one variant -- and the matrix alone would still pass if
a future change made populated comparisons role-sensitive in a way its own
specification encoded.
329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints
against upstream main.
Two of the three uncalled Outcome predicates now have callers, so their and the 329 tests, including the 72-cell decision-surface matrix, confirm it. closes_gate replaces `status != Status::PASS` at the three sites that decide whether a `when` condition drops the block it guards -- eval_when_condition_block, eval_type_block_clause and eval_rule. Identical in behaviour, since from_status maps PASS to Satisfied and both FAIL and SKIP to variants that close. What it buys is naming the decision: this is the branch that makes a rule inapplicable and silences every check in its body, which is a different question from whether anything failed. Keeping the two apart in the code is the point -- a gate that closes blocks nothing and still drops everything it guarded, and conflating them is what makes a "the gate never failed" test look like a safety property when it is a tautology. blocks replaces `role.is_strict()` at the two empty-collection role tests. Equivalent because to_status maps Unevaluatable to FAIL for an assertion and SKIP for a gate, but it states the premise rather than leaving it implicit: the value at hand is a clause that could not be evaluated, and the question is whether an unevaluatable clause blocks in this role. Deliberately `blocks` and not `closes_gate` here, because the clause is being reported and a FAIL is what blocks a deployment. negate keeps its allow, and its note says why: negation is still applied to per-value Status inside binary_operation before the fold sees it, so the inversion never passes through an Outcome. Moving it is part of having the comparators produce Outcome directly, which is the remaining step of this migration rather than something to force a caller for now. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main.
EvaluationResult::QueryValueResult carried Vec<(QueryResult, Status)>, so every comparator decided a per-value answer in the reported vocabulary and the fold in eval_guard_access_clause lifted it back with from_status. That lift was the last place the migration was cosmetic: Status cannot say "nothing to compare", which is exactly the distinction the fold needs, so the information had to be reconstructed from context instead of carried. The vector now carries Outcome and the fold consumes it directly. Migrated producers: report_value (the single per-value decision every binary comparison routes through), report_all_values and report_at_least_one, unary_operation's own pass/fail pushes, and the sixteen push sites in binary_operation. The map-key-filter consumer in eval_context.rs matches on Outcome::Satisfied. unary_operation still lifts once, at the point where it forwards an already-decided Status, and that one is faithful rather than lossy: the unary path decides a plain pass/fail per value, so from_status discards nothing there. Behaviour-preserving, and this is the change where that claim needed real evidence rather than a clean build. The 72-cell decision-surface matrix passes unchanged, and mutating the gate arm to Outcome::NotApplicable after the migration still fails exactly 12/72 cells -- the gate x empty region -- so the matrix is guarding the migrated code and not merely agreeing with it. Still on Status: the ValueEvalResult/ComparisonResult layer inside eval/operators.rs, where the not-flag inverts Success and Fail before report_value sees them. That is why Outcome::negate keeps its allow(dead_code); giving it a caller means migrating that layer too, which is a separate change with its own blast radius, not a line to move. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main.
The note on negate said it was uncalled because negation is applied to per-value Status inside binary_operation before the fold sees it. That was true when written and is not any more: the per-value vector now carries Outcome end to end. The real reason is that this evaluator has no verdict-negation site at all. Negation applies to flags before a comparison happens (comparator.1 ^ gac.negation) and to evidence payloads afterwards (the (CmpOperator, bool) wrapper rebuilds Compare values with reversed diffs so the report names the right values). Neither is an Outcome being inverted. The two places that do invert a verdict -- eval_guard_named_clause and eval_parameterized_rule_call -- deliberately fail closed on SKIP rather than leaving it unchanged, which is the opposite of what negate does, so routing them through it would be wrong and not merely verbose. Considered deleting it and the four tests that use it. Rejected because de_morgan_holds_over_the_whole_domain is stated in terms of negate, and that law is part of the algebra's specification -- verified over all sixteen pairs, with a comment recording an earlier version that restricted itself to the evidence-bearing variants on reasoning that turned out to be wrong. Deleting the function deletes the law. Comment-only. 329 lib tests, 0 failed.
…tors' eval.rs defined `ComparisonResult` and `LhsRhsPair`, and so does its child module eval/operators.rs. Both pairs were live and neither was interchangeable with the other, so the only way to tell which a given mention meant was to check for an `operators::` prefix -- and the unprefixed one was the parent's, which is the opposite of what a reader expects from a name that also exists in the child. Renamed the eval.rs pair to RhsComparison and ComparedPair, after what they are: one left-hand value staged against one right-hand value for reporting. `each_lhs_compare` builds them and hands them to `report_value`. The two models differ in more than name. This one pairs resolved values and records whether the comparison held as a plain bool; the operators one carries its verdict in the Success/Fail constructor together with the evidence payload the reporter needs, which is why the negation path there has to rebuild payloads rather than flip a flag. Mechanical, and the mechanism needed two corrections: a negative lookbehind on `operators::` still caught `ValueEvalResult::ComparisonResult`, which is a variant of the operators enum reached through the enum name rather than the module path, and eval_tests.rs names the private types directly. 329 lib tests, 0 failed. cargo fmt --check clean. Zero new clippy lints against upstream main.
test_string_in_comparison carried a bare #[ignore] and was described in this branch
as "an upstream failure parked in 2023, not ours". Accurate but useless: nobody could
act on it without redoing the investigation.
The mechanism, measured rather than inferred. `IN` applies substring semantics only
when its right-hand side is a literal string; when the right-hand side comes from a
query it falls back to equality:
<query> in '<literal string>' PASS -- substring, via string_in
'literal' in <query> FAIL -- equality, contained_in -> compare_eq
So the same spelling is a different operator depending on where the value came from.
In InOperation::compare, a literal left side against a queried right side takes the
(Some(l), None) arm, which for two scalars calls contained_in, whose scalar/scalar
case ends in match_value(.., compare_eq). string_in -- the function that actually
does rhs.contains(lhs) -- is only reachable from the literal/literal arm and from the
(None, Some(r)) arm where the right side is a literal String.
Three hypotheses ruled out by probing each separately: the `bucket_names |` capture
syntax resolves, the %s3_buckets variable resolves, and the
Properties.PolicyDocument.Statement.Resource.'Fn::Sub' query resolves through the
intrinsic. The failure reproduces with a plain literal 's3' in place of the captured
variable, which is what localised it to the comparison rather than the plumbing. My
first guess was the Fn::Sub intrinsic moving the value off the queried path; that was
wrong.
Still ignored, and the ignore reason now says why rather than just that. Fixing it
means making scalar-against-query use string_in, which turns every IN between a
scalar and a queried value into a substring test -- `Properties.Name in
Resources.*.Tags` would start matching fragments -- so it is a visible semantics
change for every existing ruleset and belongs with upstream. docs/CLAUSES.md
documents only scalar left-hand sides for IN and does not settle it.
Comment-only. 329 lib tests, 0 failed, 1 ignored.
…oc contradiction
Recorded on this branch as an unfixed defect: the same clause written backwards gives
different answers when its query selects nothing. Measured on a template with no S3
bucket, all four cells:
<query> == %lit SKIP
%lit == <query> FAIL <- the asymmetry, and only here
<query> != %lit SKIP
%lit != <query> SKIP
Two things that changes. The disagreement is confined to the positive spelling --
both negated forms already agree -- and the comment calling the FAIL a contradiction
of docs/QUERY_AND_FILTERING.md:222 was too strong. That sentence is about clauses
whose *subject* is the empty query.
The reading that makes all four cells right is that a Guard comparison is not
operand-symmetric even when the operator is. The left side is the subject being
checked, the right side the reference it is checked against. No subject values means
nothing to assert, so the rule does not apply -- which is exactly what lets one
ruleset run against templates that do not all contain the resource type, the case the
doc describes. No reference values means the assertion cannot hold, since nothing is
among zero references.
So this is pinned, not changed, and the choice is forced rather than lazy: "fixing the
asymmetry" means picking one of the two to break. Making the mirrored form SKIP
reintroduces the wrong PASS where an allowlist that resolved empty reported
compliance, which positive_comparison_against_empty_reference_fails exists to prevent.
Making the forward form FAIL breaks every ruleset run against a template lacking the
resource type. v3.2.0 exits 0 for both spellings, so it had the wrong PASS in both
directions.
zero_selection_is_asymmetric_by_operand_role asserts all four cells, each with a
liveness row on a template that does contain a bucket -- without those the
zero-selection assertions are satisfied by a rule that never ran, which is the failure
mode the ordering-operator reproduction on this branch was rewritten twice to avoid.
330 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints
against upstream main.
…trix The mirrored negated cell -- `%literal != <query>` where the query selects nothing -- asserted SKIP. PR aws-cloudformation#717 settled that a comparison whose reference resolved to no values fails closed in either polarity, so it is now FAIL. This makes the matrix state a simpler rule than it did before. Emptiness of the subject excuses the clause and emptiness of the reference does not, uniformly across polarity, instead of the positive spelling disagreeing while both negated forms sat at SKIP. The doc comment loses its "confined to the positive spelling" half for the same reason, and gains a pointer to where the decision and its rejected alternative are recorded, so a reader who finds this cell surprising is not left to reconstruct the argument from the assertion alone. Landed as its own commit rather than folded into the commit that introduced the matrix: the change comes from review feedback that arrived after it, and the sequence is worth keeping legible.
Six assertion messages on this branch interpolated inline captures -- `{seq:?}`,
`{role:?}`, `{o:?}`, `{other}` -- into a single string literal. The crate is on
edition 2018, where a one-argument `assert!` or `unreachable!` message is not a
format string, so each of these would have printed the braces and not the value.
A test whose failure message says `{o:?} must close a gate` costs whoever reads it
a round trip to find out what `o` was.
Two of them also bound `other` for no other purpose, so the unused-variable
warning and the unused-placeholder warning were the same mistake counted twice.
Found by `cargo clippy --all-targets`, which the CI gate does not run: pr.yml
invokes `cargo clippy -- -D warnings`, and that excludes test targets, so all six
passed CI while being wrong. Worth running the wider form locally for exactly this
reason -- these are in test code, where a misleading message is the whole cost.
Three things the rebase onto `2b53c97` had to settle, all of them cases where taking this
branch's version wholesale was measurably wrong.
**The unevaluatable-clause arm keeps its role split.** Carrying `Outcome::Unevaluatable` for
both roles reads better and reintroduces the defect it was meant to remove:
`to_status(Gate)` maps it to SKIP, `eval_rule` maps every non-PASS condition to a rule-level
SKIP, so `rule r when Enabled !EMPTY { MustBeTrue == true }` exits 0 with the violation
inside it unreported. That is the case a reviewer found against aws-cloudformation#717 and `4c8c650` fixed, and
it came straight back when this arm was resolved in this branch's favour. Measured, not
reasoned: the repro exits 0 before the correction and 19 after.
An assertion is expressed as `Outcome::Unevaluatable` so the lattice applies the role once. A
condition still travels as an error, which the three condition sites catch. Wiring
`Outcome::closes_gate` into those sites would make the arm uniform and is the obvious next
step here -- the vocabulary already exists on this branch and only its tests use it.
**The vacuous-comparison deprecation notice is removed, because this branch is the release it
warned about.** `e9b143c` makes a comparison against an empty collection report a failure in
its plain polarity, so a notice saying the answer changes later has outlived its own change. A
warning that survives the thing it warned about is how warnings become noise. The membership
notice stays: `NOT IN` on an operand comparable with no element still passes here.
Worth stating precisely, because the notice's wording only half fits what this branch does.
Plain polarity now fails; the negated polarity answers SKIP, because an empty fold returns
`Outcome::identity()`, which is `NotApplicable`. That is an improvement on passing vacuously
and still not the failure `docs/QUERY_AND_FILTERING.md` asks for, which lists `Tags: []` beside
a missing key and an empty map and says all retrieval errors are failures.
**The oracle's two lists move by four and one.** Three spec violations leave -- the plain
polarity of the empty-collection comparison -- and `in_list/empty_list/not/gate` leaves the
conformant list. One joins: `in_list/empty_list/not/assert` answered FAIL on the parent branch
and answers SKIP here, so it is the single entry this branch makes worse rather than better.
The lists are regenerated from measurement rather than edited by hand, and the note above them
records the movement so a reader is not left comparing bare counts.
`SITES_EXPECTED` goes to 19: the `NotComparable` arm records `Some(nc.reason)` on a
`ClauseValueCheck`, a variant that already renders. The parent branch is at 18 rather than the
17 the previous note claimed.
948 tests, `cargo clippy -- -D warnings` and `--all-targets` clean, fmt and typos clean, and
190 of 190 aws-guard-rules-registry rule/test pairs unchanged.
`guard/src` has no non-ASCII byte anywhere at the merge-base. The comments added by this branch had 53 em dashes, which is a silent style break: it does not fail a build, and a reviewer reading a diff has no reason to expect it, so it would have landed and then propagated by copy-paste. Found by scanning for non-ASCII rather than by reading, which is the only way this class shows up. The scan also caught a CJK character that had slipped into a sentence about gate evaluation, where it read as a typo with no plausible origin. `typos` does not catch either case, since neither is a misspelled word. Line widths are left alone. Replacing one character with two pushed some comments to 101-104 columns, and the merge-base already has comment lines up to 134 columns in this file, so rewrapping 60 lines would be churn for a rule the crate does not hold. One assertion *message* changed with the comments, which is human-readable text rather than a value compared against binary output. All 948 tests pass.
The parent branch added a test asserting that `cfn-guard test` prints both deprecation notices, over a fixture with two deprecated clauses. This branch deletes one of them: an empty-collection comparison reports a failure here, so the notice that warned about it went with the behaviour it described, and the shared rules file already had that clause removed. So the data fixture drops the expectations for the rule that no longer exists, and the test asserts one notice instead of two. It also asserts the empty-collection notice is *absent*, which pins the relationship between the two branches: if this branch's fix were reverted, the notice would return and the test would say so rather than passing on a weaker claim. Dropping the stale expectations is not cosmetic. Since `Say when a test expectation matched no rule`, an expectation naming a rule that is not in the file prints a line on every run, so leaving them would have made this branch emit a warning about its own fixture.
Fourteen functions on the clause path returned `Result<Status>` and used `Err` for a fourth answer the 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. `an_unevaluatable_gate_fails_the_rule_closed` has held since the parent branch. What it cost was that the same 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, and taking the value in both roles reintroduced the reviewer's wrong-PASS because the consumers were not yet asking. Now the clause path returns `Result<Outcome>` and `Err` means an error. The consumers ask: `eval_rule` and `eval_when_condition_block` match `Unevaluatable` before asking `closes_gate`, so a gate that cannot be decided fails its rule and a gate that did not match leaves the rule inapplicable. `unary_operation` no longer takes a role at all -- a clause's answer does not depend on how it was reached, only the status that answer maps to does, and that mapping happens at the consumer. `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. Two consequences worth reading: `A or B` is now evaluated to the end when `A` cannot be answered. The counting version returned the error 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 was never reported; now both the undecidable branch and the real violation 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 FAIL, because reporting a violation there blames the input for a reference that never resolved. The three filter-predicate sites keep failing the query rather than selecting nothing, and now say so. Two of them claimed in a comment that an unevaluatable predicate selected nothing, 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. `is_unevaluatable` has one caller left, at the boundary where a comparator error becomes a lattice value. The comparators still signal an unsupported operand by returning an error, so something has to translate; nothing above that point asks an error what kind it is. Verification. The 440-cell operator/shape/polarity/position matrix is identical across this change: zero cells moved, still 42 disagreements with the stated oracle. The registry differential is unchanged: 190 rule files, 1,956 expectation checks, zero exit-code changes and zero content changes, the same five deprecation notices and the same eleven unmatched expectations. 955 tests pass, clippy clean on all targets, and the reviewer's boolean-gate repro still fails closed. One message did not survive review by the tests. The `WhenCheck` record for an undecidable condition got a sentence explaining itself, and `every_recorded_explanation_has_a_rendering_path` rejected it: measured, the output carries the clause's own explanation and nothing on that record reaches a reader. It is `message: None` with the measurement recorded beside it.
`docs/CLAUSES.md` said a condition that cannot be decided leaves its rule not applicable, so the guarded block is never checked and the run exits 0. That was true when it was written and is now true of only half the cases, which makes it the more dangerous kind of stale: a reader checks the document, finds their case described, and stops. The two halves are different questions. A cross-kind comparison -- `Size > 10` where the template carries `Size: "50"` -- is decided, in the negative, by a conversion `docs/KNOWN_ISSUES.md` records as a defect. The rule is inapplicable, the run exits 0, and the skip explains itself; every sentence already in the document about that case still holds, including the sample output, which was re-measured rather than assumed. A condition Guard cannot evaluate at all is not decided in either direction: `EMPTY` on a number or a boolean, or a reference that resolved to no values. Those now fail their rule. Reporting them as not applicable exits 0 and takes the guarded check with it, and a rule that never fires looks exactly like a rule that holds. So the section keeps what it had and gains the distinction, with the worked example measured at this revision: `FAIL` and exit 19, naming the operation and the path that could not support it. The closing sentence says which case a reader is in when a rule starts failing where it used to skip, because that is the question they will arrive with. The four em dashes this branch had added to the file are now `--`, matching the merge-base, which has none. The three curly quotes predate the branch and are left alone.
The cache was keyed on `(rule, role)` but stored `to_status(role)`, and that conversion is where a
verdict went. `Outcome::Unevaluatable.to_status(Gate)` is SKIP, so a `when` condition referencing a
rule that could not be evaluated read back "the rule did not apply", and `eval_rule` reported the
enclosing rule inapplicable with the guarded check unrun:
rule inner_gate {
when Enabled !EMPTY { Enabled == true }
}
rule guarded when inner_gate {
MustBeTrue == true
}
merge-base guarded FAIL
before this guarded SKIP, and the reason line said so out loud: "the rule did not apply
because a condition referenced rule [inner_gate], which did not apply to this
input" -- for a rule that had not decided anything
after guarded FAIL
This is the same conflation the branch removed from the clause path, surviving in the one place that
still converted early. Keying on the role was necessary and not sufficient: the reference asked its own
question and then threw the answer away.
`eval_guard_named_clause` now maps the four-valued answer with the same table
`eval_parameterized_rule_call` uses. That table already existed there, and its comment claimed to
mirror this function while the arms disagreed -- the two spellings of one gate drifting apart is what
made the named form worse than the parameterized one. `Unevaluatable` propagates rather than being
converted; `NotApplicable` keeps the fail-closed-for-an-assertion arm and the two gate arms that let
`when not other` open.
`status` survives inside the function for the records a reporter reads, and only for those.
Verification: 959 tests, clippy clean on all targets, fmt and typos clean. The 440-cell matrix moves
zero cells and still shows 42 disagreements, and the registry differential is unchanged at 190 rule
files, 1,956 expectation checks, zero exit-code changes and zero content changes. Both gate spellings
now report the rule as failing with the reason rendered, and the parent branch's regression tests for
them pass here unmodified.
awsmadi
force-pushed
the
pr/evaluator-outcome-cleanup
branch
from
August 20, 2026 21:40
40e45a3 to
c45268e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #717. The diff shown here includes #717's 50 commits, because GitHub needs the base branch to exist upstream and
pr/negation-and-reporting-fixesonly exists on the fork. The 46 commits that belong to this PR are the ones fromde59fa9onward; reviewing this after #717 merges will show the real diff.Two things, and the second is why the first exists.
The evaluator's verdict was a three-valued
Status: PASS, FAIL, SKIP. Every defect #717 fixes is a case where a fourth answer was needed and got squeezed into one of those three. A clause that could not be evaluated is not a clause that passed, and not a clause the input violated, and not a rule that did not apply. #717 makes each of those cases behave correctly one at a time, and routes the fourth answer through the error channel to do it. This PR gives the fourth answer a name and deletes the channel.What changes for a rule author
Measured over the same 440 cells as #717 (10 operators, 11 operand shapes, both polarities, assertion and gate), the entire observable behaviour change over #717 is 7 cells, all of them an empty collection:
PASStoFAILSizes == 50,Sizes > 10,Sizes IN [...], each as an assertion overSizes: []PASStoSKIPFAILtoSKIPINas an assertionSKIPtoFAILINas a gateThe first row is the headline: a comparison against an empty collection reported
PASS.docs/QUERY_AND_FILTERING.mdlistsTags: []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 was the outlier rather than a design choice.The negated form does not fail with it, and that is deliberate.
no element of [] is in ['Owner']is vacuously true, so failing it rejects a compliant template. It answersSKIP, because an empty fold returnsOutcome::identity(), which is "not applicable" -- the clause examined nothing, so it reports neither pass nor fail. An earlier revision forced both polarities to fail and a reviewer argued it down; the argument is recorded at the site rather than in this description.The last row is the collapse this PR exists to remove. On #717 that condition resolved to FAIL,
eval_rulemapped every non-PASS condition to a rule-level SKIP, and the guarded violation went unreported at exit 0.One further change is not visible in the cell matrix, because the matrix has one clause per rule. A gate with one undecidable branch and one that decides is now decided by the second:
Enabledis a boolean, so!EMPTYhas no answer;Name == "keep"holds. Before, the undecidable branch left the evaluator as an error and returned from the conjunction on the spot, so the second branch never ran, the rule failed closed on its condition, and the violation inside the body was never reported. Now both the undecidable branch and the realMustBeTrueviolation appear. Same exit code, one more finding. This is not a relaxation of failing closed:Outcome::orabsorbs onlySatisfied, so a disjunction with nothing satisfied and something undecidable is still undecidable and still fails its rule closed.The type
Outcomeis a four-valued lattice with two operations and one conversion:Satisfied,Violated,NotApplicable,Unevaluatableandandor, Kleene semantics, withNotApplicableas the identity of both and onlySatisfiedabsorbing underorto_status(role), which is where the assertion-versus-gate distinction is applied, and the only place it is appliedThe identity is the part that closes a family of defects rather than one defect. A fold over zero elements returns
NotApplicable, notSatisfied, so an empty collection cannot satisfy amatch_allblock by producing no failures. The counting folds it replaces had to remember that case individually, and one of them did not.Outcome::negatehas no caller and says so in its own doc comment, with the reason: clause-level negation is applied before the fold, on the comparator's own not-flag, so there is nothing left to negate afterwards. It is tested rather than deleted because the lattice is easier to reason about complete.What the migration removed
Fourteen functions on the clause path returned
Result<Status>and usedErrfor the fourth answer. Every consumer askedis_unevaluatablewhat kind of error it was holding, and the two condition sites that had to tell "could not be answered" from "did not match" did it by catching one.That worked, and #717's regression test for it has held since. What it cost was two representations of one question, chosen per site and invisible in the signature.
unary_operationtook aClauseRolesolely to decide whether to answer with a value or an error; it no longer takes one, because a clause's answer does not depend on how the clause was reached. Only the status that answer maps to does, and that mapping now happens at the consumer.eval_conjunction_clausesfolds throughandandorinstead of counting passes and fails.is_unevaluatablehas one caller left, at the boundary where a comparator error becomes a lattice value, because the comparators still signal an unsupported operand by returning an error.The three filter-predicate sites deliberately keep failing the query rather than selecting nothing, and now say so. Two of them claimed in a comment that an unevaluatable predicate selected nothing, which was never what the code did. The behaviour is the conservative one on purpose: a filter that drops the resources it could not judge selects fewer of them, and a rule written to catch violations catches fewer. That is the mechanism by which a fail-closed change inside a filter turned five registry security rules from FAIL to PASS, which #717 reverted.
Verification
cargo testcargo clippy -- -D warningscargo clippy --all-targets -- -D warningscargo fmt --all -- --checktyposThe 440-cell figure is the one that matters for a change this wide: running the matrix before and after the migration, no cell moved, and the count of disagreements with #717's stated oracle stayed at 42. The migration is a change of representation, and that is the evidence for it rather than the claim.
Against that oracle this branch has 42 disagreements where #717 has 49: 6 that contradict the specification against #717's 12, and 36 that the documentation describes on purpose against 37. Seven cells left those lists and none joined.
The registry differential compares report content as a multiset of lines rather than a sequence, which is the only meaningful comparison against the merge-base:
cfn-guard testlisted rule names inHashMaporder there, so two runs of the same merge-base binary differ. It was also run against a negative control, a rule asserting PASS where it must FAIL, which exits 7, so the zeroes mean the harness can fail and did not.docs/CLAUSES.mdis updated here rather than in #717, so no merged state has the document disagreeing with the code. It documented a condition that cannot be decided as leaving its rule not applicable at exit 0; that is now true of only half the cases, and the section gains the distinction with both examples measured at this revision.The cache was the last site that flattened the answer
The parent branch fixed three places that turned an undecidable answer into a status too early. This
branch had its own instance of the third: the rule-status cache was keyed on
(rule, role)but storedto_status(role), andOutcome::Unevaluatable.to_status(Gate)is SKIP. So awhenconditionreferencing a rule that could not be evaluated read back "the rule did not apply" -- the reason line
said exactly that, for a rule that had not decided anything -- and the enclosing rule was reported
inapplicable with its guarded check unrun.
The cache stores the
Outcomenow, andeval_guard_named_clausemaps it with the same tableeval_parameterized_rule_calluses. That table already existed there and its comment claimed to mirrorthe named form while the arms disagreed, which is what made the named spelling of a gate worse than the
parameterized one.
What is deliberately not here
NOT INagainst operands that cannot be compared with anything in the list still passes. #717 fixed it, measured that it turns five registry security rules from FAIL to PASS, and reverted; both branches emit a deprecation notice a release ahead of the change instead. Fixing it needs those rules updated first, which is not this PR's to do.The one ignored test,
test_string_in_comparison, carries its reason in the attribute rather than a bare#[ignore]:INuses equality against a queried right-hand side and substring matching against a literal one, which predates this work by two years. It counts twice in acargo testsummary because that module compiles into both the lib and the bin test target.