Fail the compliance suite on unresolved builtins - #190
Merged
sspaink merged 3 commits intoAug 10, 2026
Merged
Conversation
ComplianceTest swallowed FunctionNotFoundError, so every fixture for an unimplemented builtin reported green. The 18 providers.aws.sign_req cases passed for months with no implementation, and 14 walk cases still do; the missing-builtin report was printed to stderr behind a TODO and failed nothing. A case whose builtin cannot be resolved now fails unless that builtin is listed in known-missing-builtins.txt, and the list is a ratchet: the suite also fails on entries no fixture reports as missing, so implementing a builtin means deleting its line in the same change. Two other silent paths are closed too — a case declaring neither want_result nor want_error, and a want_error_code case that does not throw. The list records 37 builtins that ship in opa-builtins sub-modules but are unreachable because their BuiltinProvider entry is commented out, so ServiceLoader never finds them. Registering them is left to a follow-up: the fixtures then expose parity failures that need triage. Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
test-opa-evaluator is path-filtered, so a PR touching only opa-builtins skipped ComplianceTest entirely — exactly the change the new builtin gate is meant to police, since the suite resolves builtins from those modules over the BuiltinProvider SPI. A generator change (an OPA version bump) regenerates the fixtures it reads, so that path triggers it too. Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Close the known-missing list reader via try-with-resources; CodeQL flagged the InputStreamReader as not always closed on method exit. Signed-off-by: Sebastian Spaink <sebastianspaink@gmail.com>
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
2 participants
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.
Summary
The compliance suite was passing tests it never ran. When a fixture calls a
builtin the SDK doesn't have, the evaluator throws "function not found" — and
the harness caught that error and counted the case as passed. So a missing
builtin meant all of its fixtures were green:
providers.aws.sign_reqhad 18passing cases before the implementation existed, and
walkhas 16 today.Changes
gaps (
known-missing-builtins.txt). Implementing a builtin means deleting itsline — the suite fails on stale entries, so the list only shrinks.
want_result, nowant_error).opa-builtins/,which is where builtins are added. Now it does.
37 entries on the list are builtins that exist but are unreachable: their
META-INF/servicesentry is commented out, soServiceLoadernever registersthem. Un-commenting the five files surfaces 32 parity failures, so that needs
its own PR.
walk's line is tied to #141.Testing
./gradlew buildgreen,opa test tools/policy/13/13. Removingproviders.aws.sign_reqfrom the list fails all 18 of its cases as expected; asimplemented on
mainthey run and pass. A bogus entry trips the stale check.