Retire the superseded evaluator, and stop the install scripts failing on a shared API quota by awsmadi · Pull Request #719 · aws-cloudformation/cloudformation-guard

awsmadi · 2026-08-17T19:48:01Z

Removes the superseded evaluator and the reporting cluster that only it reached, and fixes two problems
in the install scripts. 581 insertions against 6,033 deletions across 33 files: one added, eight deleted,
24 modified.

Rebased onto main at ef17f36, which contains #717. Main has since gained two commits, fcc079f and
ed7c703, both touching only the GitHub action and its lockfiles, so no Rust source moved under this branch.

Independent of #720 and of pr/filters-captures-and-reporter-silence (#727) in the sense that it shares no
commits with either. It does not merge cleanly alongside #727, and the order matters. Trial-merged into
main at 3e265bb with rerere disabled so a recorded resolution could not report a false clean: this branch
alone is clean, #727 alone is clean, and the two together conflict in five paths — and no single resolution
direction is correct for all of them.

path	this branch's side	#727's side	resolve to
`guard/src/rules/parser.rs`	the original unbounded `find_substring(">>")`	the 87-line `extract_message` grammar	#727
`guard/src/rules/path_value.rs`	deleted	288 lines of `QueryResolver` and its two `select` functions	this branch
`guard/src/rules/mod.rs`	deleted	the `add_merged_capture_key` trait method	#727, plus this branch's own deletion
`guard/src/commands/reporters/validate/common.rs`	an import list without `FileReport` and `Messages`	the same list with them	#727 (its superset)
`guard/src/commands/reporters/test/structured.rs`	`build_junit_test_cases` with an elided lifetime	`has_unchecked_expectations` and `number_of_unchecked_expectations`	both

path_value.rs — take this branch's side. Deleting that engine is the point of this PR; fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727 carries the
code only because it does not delete it. It does not touch the duplicate-key detection fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727 adds earlier in
the file, which sits well outside the conflict region.
parser.rs — this branch's side is the original extract_message, match input.find_substring(">>"),
which this branch reformatted in 51707bc Apply rustfmt without changing its behaviour. fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727's side replaces
the function entirely. Take fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727's side. The original searches all remaining input for the closing tag,
so one forgotten >> swallows every following rule as message text and the run reports PASS at exit 0 with
no diagnostic — the defect fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727 leads with. Resolving this hunk the other way reinstates it silently,
which is why it is the one to be careful about.
The other three are mechanical once the first two are settled: mod.rs and common.rs take fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727's side
because this branch simply predates the code there, and structured.rs wants both sides — this branch's
lifetime elision on a function whose body fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727 leaves alone, plus the two methods fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727 adds above it.

If this PR merges first, #727 rebases onto it and the question does not arise.

This table was re-derived after #727 gained 27 commits. It previously named two conflicted paths and a
272-line path_value.rs hunk; both numbers had drifted. Re-derive it again if either branch moves.

What it removes

walk_type and the query engine behind it were the pre-eval_context evaluator. Nothing on the current
path called them, and the reporting types that existed to render their output were unreachable with them
gone: two validate reporters, Reporter::report (the trait method the current reporters all implement as
Ok(())), and the rest of the legacy cluster. The crate builds with zero dead-code warnings, which is the
check that the removal is complete rather than partial.

40 test functions go with the deleted modules — rules::evaluate (32 of them), rules::path_value's
legacy QueryResolver tests, rules::values, commands::aws_meta_appender, and the two deleted
reporters. Nothing else changes: cargo test --all --release runs 830 tests on main and 752 here, and
the 78-execution difference is those 40 functions, each of which the lib compiles into both the lib and
the bin test target.

What it fixes

The install scripts failed on an anonymous GitHub API quota. install-guard.sh and
install-guard.ps1 resolve the latest release through the API, and an unauthenticated caller shares a
per-IP quota. On a CI runner or a shared NAT that quota is routinely exhausted, and the scripts read the
rate-limit response as "no release found" and exited non-zero with a message about the release. They now
handle the quota response as its own case.

install-guard.ps1 had no lint gate. Added PSScriptAnalyzer, and fixed what it found.

Verification

The full suite passes and the crate builds with no dead-code warnings — the latter is what establishes
that nothing still references the removed code. cargo fmt --all -- --check, both clippy invocations
and typos are clean at the head.

Worth stating rather than leaving to be discovered: every commit builds, but the dead-code lint only
clears at the end of the sequence. The cluster is dead as a whole, so removing the evaluator first
leaves the reporters that rendered its output unconstructed, and clippy -D warnings says so until the
last deletion lands. The staging is for review — evaluator, then the trait method, then the two
reporters, then the rest — and squashing it into one 6,000-line deletion would land a clean lint at the
cost of a reviewable diff. Happy to squash if you would rather have the former.

The install-script changes are exercised against the quota response shape rather than against a live
quota exhaustion, which cannot be provoked on demand.

guard/src/rules/evaluate.rs was the pre-eval.rs evaluator. Nothing on the CLI path reached it: the compiler reported 15 items in it as never used, RootScope::new was called only from evaluate_tests.rs, and its one apparent outside consumer -- path_value.rs's QueryResolver::select -- takes &dyn EvaluationContext, the old context trait. select's only non-test callers were itself, evaluate.rs, and MetadataAppender, which is constructed only in its own test. One connected dead component, not a scattering. Removed: - evaluate.rs (1342) and evaluate_tests.rs (2345) - QueryResolver and PathAwareValue::select, plus the four helpers only select used: map_error_or_empty, map_some_or_error_all, retrieve_index, accumulate. eval_context.rs already has its own retrieve_index and accumulate as free functions, so the new engine loses nothing. - aws_meta_appender.rs and its test Kept: EvaluationType and StatusContext, which live reporter code still reads -- GenericSummary is constructed at helper.rs:66 and validate.rs:704. Display for GuardNamedRuleClause moved to exprs.rs beside the Display impls for its sibling clause types. It lived in evaluate.rs but eval.rs formats it, so it was the one piece of that file the live evaluator depended on. evaluate_tests.rs was also a near-duplicate suite: of five test names sampled, all five exist in eval_tests.rs against the live evaluator. The 39 tests dropped here (32 evaluate_tests, 4 path_value_tests, 2 values_tests, 1 appender) all exercised only the deleted engine; the same semantics stay covered by eval_context_tests for query retrieval, eval_tests::filter_based_* and test_map_keys_function for filters, and rule_test_type_blocks for type blocks. 358 -> 319 lib tests, 0 failed. 4693 lines deleted.

eval_conjunction_clauses built a user-visible string from std::any::type_name::<T>(): `context` becomes the `Context=` label on the Disjunction node in --verbose output, and four fixtures under guard/resources pin it exactly. type_name's output is explicitly unspecified -- std documents that it "must not be considered to uniquely identify a type" and may change between compiler versions, and it did. Newer rustc renders the elided lifetime, so cfn_guard::rules::exprs::GuardClause became ...::GuardClause<'_>, and test_data_file_verbose and test_with_rules_dir_verbose fail on any toolchain other than the 1.77.2 pinned in rust-toolchain.toml. Verified failing at both 320251c and 57bbdbf (upstream main) under rustc 1.97, so it is pre-existing and not introduced by this branch. Truncating at the first `<` restores the pre-change spelling for every T and is stable under further rendering changes, since only the generic/lifetime portion varies. Deliberately not changing the strings themselves: that a Rust module path is user-facing output at all is a real wart, but rewriting it is a visible output change that belongs with the fixtures. Also removes the EvaluationContext and Evaluate traits, which the previous commit left with no production implementations. `cargo clippy -- -D warnings` -- the gate at pr.yml:94 -- rejects `trait EvaluationContext is never used`, so leaving them would have failed CI. Differential clippy against upstream main showed this as the only new lint the branch introduces. Removing them takes with them: - StackTracker, the old evaluator's recorder, and StatusContext::new, its only caller. StatusContext itself stays: the validate reporters still destructure it and generic_summary.rs is live. - common_test_helpers.rs, whose only content was a DummyEval implementing the trait - a DummyEval in parser_tests.rs, constructed once into `let _dummy` and never used 324 lib tests, 0 failed, 3 ignored. test_command 19/19 -- both *_verbose tests now pass, where they were 17/2 before.

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. Verified this is genuinely unformatted branch code and not a rustfmt version artifact: upstream main at 57bbdbf passes `cargo fmt --check` cleanly under the same rustfmt 1.97, so the only files it can rewrite are ones this stack changed. Two of the six files -- eval_context.rs and outcome_tests.rs -- come from feat/status-type-migration rather than from this branch, so that branch is failing the same gate on its own. 324 lib tests, 0 failed. cargo fmt --check exit 0.

`Reporter` had two methods: `report`, taking `&[&StatusContext]`, and `report_eval`, taking an `EventRecord`. Only the second is called. `report` was the old evaluator's reporting entry point and nothing has invoked it since the new recorder replaced it -- several implementations were already `_`-prefixed stubs returning `Ok(())`, which is what a vestigial required method decays into. Removed the trait method and its eight implementations. Two traits in the same files also have a `report`, and both are live: `GenericReporter::report` and the `report(&mut self) -> Result<i32>` on the structured reporters. They are kept, and the removal was filtered on whether the parameter list mentions `StatusContext` rather than on the method name, because name-matching removed live impls twice while writing this. This orphans the legacy reporting cluster rather than removing it: StatusContext and EvaluationType are now unreferenced except by each other, along with find_all_failing_clauses, extract_name_info, print_partition, print_compliant_skipped_info, pprint_failed_sub_tree, and the CfnReporter / SingleLineReporter / ConsoleReporter / StructuredSummary / DataOutput / DataOutputNewForm / StructureType / SarifRule set. Deleting that cluster is the follow-up; splitting it out keeps this commit to one reviewable question. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main.

cfn_reporter.rs and console_reporter.rs existed only to implement the Reporter::report method removed in the previous commit. Neither CfnReporter, SingleLineReporter nor ConsoleReporter was ever constructed, and after that removal the files were referenced by nothing but their own `pub mod` declarations in reporters/validate/mod.rs -- verified by search before deleting rather than inferred from the dead-code warnings, since those warnings say an item is unused and not that its file is unreferenced. The live validate reporting path is unaffected: GenericSummary is constructed at helper.rs and validate.rs and implements report_eval, which is the method the evaluator actually calls. Dead-code warnings 20 -> 15. What remains is the second half of the same cluster -- StructureType, StructuredSummary, DataOutput and DataOutputNewForm in common.rs, SarifRule, the print_partition / print_compliant_skipped_info / pprint_failed_sub_tree / extract_name_info / find_all_failing_clauses helpers, and finally StatusContext and EvaluationType once nothing names them. Left for a follow-up because each needs its own check that no live reporter destructures it, and this commit is already one reviewable question. 329 lib tests, 0 failed, 1 ignored. cargo fmt --check clean. Zero new clippy lints against upstream main.

Finishes what the previous two commits started. Removing Reporter::report left a connected set of items reachable only from each other, and deleting them in dependency order collapses it entirely: common.rs extract_name_info, find_all_failing_clauses, print_compliant_skipped_info, StructuredSummary and its impls, StructureType, DataOutput, DataOutputNewForm summary_table print_partition sarif.rs SarifRule tracker.rs deleted -- StatusContext was the whole file once StackTracker went mod.rs EvaluationType and its Display impl, freed by StatusContext going; add_variable_capture_index, a never-called default trait method operators.rs UnaryComparator, a trait with no implementors exprs.rs WhenGuardBlockClause, never constructed by the parser Dead-code warnings 15 -> 0. Upstream main has 43, so this branch and its two parents account for all of them. Order mattered and was followed rather than guessed: the four dead functions were the last things naming StatusContext, StatusContext was the last thing naming EvaluationType, and each step was verified by search before deleting rather than inferred from the warning list -- a warning says an item is unused, not that removing it is safe. The live validate path is untouched. GenericSummary is still constructed at helper.rs and validate.rs, GenericReporter and the structured reporters keep their own `report` methods, and report_eval remains the evaluator's entry point. 329 lib tests, 0 failed, 1 ignored. All other targets green except validate, which fails 81/15 identically on upstream main -- a path artifact where the fixture comparison strips a directory prefix and a /local/... checkout defeats it. cargo fmt --check clean. Clippy 85 -> 20 errors under 1.97, zero of them new against main.

The Windows install job failed on an unrelated pull request with {"message": "API rate limit exceeded for 20.9.183.48."} from install-guard.ps1's release lookup. That address is a shared GitHub Actions runner, and the anonymous API allows 60 requests an hour per source IP, counted across everyone behind it. The same limit reaches real users: a corporate NAT, a VPN, or a second person installing from the same office is enough. Resolution now prefers whatever needs least from the caller. The gh CLI first, when it is installed and authenticated, because it reuses credentials the caller already has. Then the REST API with GITHUB_TOKEN when the environment supplies one. Then anonymously, which is the only path the limit applies to. An explicit version skips the lookup entirely, and install-guard.ps1 gains -Version for that, matching -v in install-guard.sh. Retries honour what the API says rather than guessing: retry-after on a secondary limit, x-ratelimit-reset when the primary one is exhausted, exponential backoff only when neither header is readable. Total waiting is capped at five minutes, after which it stops and names GITHUB_TOKEN, gh auth login and -Version as the ways out. Waiting for a primary reset can mean an hour, and an installer that looks hung for an hour is worse than one that explains itself. The token is passed to curl through a config file on stdin rather than argv. An Authorization header on a command line is readable from ps by anyone else on the host for the life of the request. It is only ever sent to api.github.com; the release archive redirects to a separate download host. install-guard.sh could not fail. Its err() exits, but it was reached from the left side of a pipeline feeding a while-read loop, so exit 1 ended only that subshell and the pipeline took its status from the loop, which had read nothing. A failed lookup left the script exiting 0 with nothing installed -- which is why only the Windows job went red when all three platforms hit the same wall. get_version's result is assigned now, so the status propagates. Get-ArchType read Win32_Processor through Get-WmiObject, a cmdlet PowerShell 6 removed; this workflow runs pwsh 7. It reads OSArchitecture from RuntimeInformation instead, which is part of the framework, present in every supported host, and exercisable outside Windows -- the WMI and CIM cmdlets are both Windows-only, so neither could be tested before CI ran. The install jobs now build cfn-guard from the branch under test, package it into the release layout, and install that, asserting the installed binary's checksum matches the one just built. They previously resolved the latest release and installed it, so they tested the installer against a binary unrelated to the change under review, and depended on the API that failed above.

install-guard.sh has been guarded by shellcheck since it was written; install-guard.ps1 had no static analysis at all. That is how a Get-WmiObject call survived in it: PowerShell 6 removed the WMI cmdlets and this workflow runs pwsh 7, so the script referenced a cmdlet its own CI shell does not have, and nothing in the repository was looking. PSScriptAnalyzer is the PowerShell counterpart, and it reports that call as PSAvoidUsingWMICmdlet. Run against the script as it stood before the preceding commit it finds two real problems and fourteen instances of one deliberate choice; run against it now it finds none, so the gate starts clean rather than with a backlog to grandfather in. Configured by .github/PSScriptAnalyzerSettings.psd1 at Error, Warning and Information, so a new finding fails the build. PSAvoidUsingWriteHost is the one exclusion, with the reasoning recorded next to it: Get-ArchType and Get-GuardVersion return their values through the pipeline, so routing progress commentary to Write-Output as the rule advises would mix that text into their return values and break them. Runs on ubuntu rather than windows: the analyser is platform independent, and a Linux runner is cheaper. Ordering: this depends on the script fixes in the preceding commit. Applied to main as it stands today the gate fails, on the WMI cmdlet and on Get-Versions using a plural noun.

awsmadi · 2026-09-01T00:16:16Z

CI has never run on this PR — the workflow runs sit at action_required, which needs a maintainer to approve them for the branch.

The PR is mergeable. Could a maintainer enable the run? Retiring the superseded evaluator touches enough surface that I would rather the suite confirmed it than take my own word for it.

download() wrote to whatever descriptor the caller had redirected, and main() redirected once, before the call. Every attempt therefore wrote to the same already-advanced fd, so a retry after a transfer that died partway appended to the bytes the failed attempt left behind instead of replacing them. Measured against a stub curl that emits 40 bytes and exits 18, then serves the whole archive: 233 bytes written against a 193-byte reference, "gzip: invalid compressed data", and an exit 1 blaming the tarball rather than the download. The retry only helped when the failed attempt wrote nothing, which is the opposite of the failure it was added for. It now recovers: same stub, 193 bytes, valid archive, exit 0. Both the curl and wget branches were exercised. download() takes the destination path and hands it to curl -o / wget -O, which truncate on every attempt. install-guard.ps1 already had this shape through DownloadFile($url, $outputFile); its download path is unchanged. Two comments credited mechanisms that are not what protects this script. The note above VERSION=$(get_version "$@") said command substitution catches a failed release lookup. It does not. get_latest_release ends in a pipeline whose last command is awk, awk exits 0 having read nothing, and so get_version returns 0 with empty output and the || exit 1 never fires. Measured at function level under a 403: get_latest_release exits 0, get_version exits 0, both with empty stdout. What fails closed is the [ -z "$VERSION" ] check below, and deleting it on the strength of the old comment sends an exhausted API quota on to request an archive from a URL built out of an empty version, which then reports a 404 for what was a quota problem. The comment now says that, and says the check has to stay. download()'s header pointed at auth_header_args, which has never existed in this repository under any ref. Replaced with the reason it does not authenticate, which install-guard.ps1 already gives: the archive redirects to a separate download host. GUARD_DOWNLOAD_BASE_URL's documentation offered http:// as an equal alternative to file://. Nothing downstream verifies a checksum or a signature, so whatever that variable points at is installed as-is. Both scripts now recommend https:// and file:// and say what accepting plaintext costs. Adding verification is a separate change and is not attempted here.

awsmadi mentioned this pull request Aug 19, 2026

Fix evaluator and reporter defects where a rule reported compliance, or nothing at all, for input it should have rejected #717

Merged

awsmadi added 8 commits August 24, 2026 18:09

awsmadi force-pushed the pr/installers-lint-and-legacy-cleanup branch from c953a25 to d9531a4 Compare August 24, 2026 18:25

awsmadi mentioned this pull request Aug 25, 2026

fix: filter and capture handling in the query engine, the reporter aborts it exposed, and four parser boundaries #727

Open

Navigation Menu

Uh oh!

Retire the superseded evaluator, and stop the install scripts failing on a shared API quota - #719

Retire the superseded evaluator, and stop the install scripts failing on a shared API quota#719
awsmadi wants to merge 9 commits into
aws-cloudformation:mainfrom
awsmadi:pr/installers-lint-and-legacy-cleanup

awsmadi commented Aug 17, 2026 •

edited

Loading

Uh oh!

awsmadi commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

awsmadi commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it removes

What it fixes

Verification

Uh oh!

awsmadi commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

awsmadi commented Aug 17, 2026 •

edited

Loading