Fail the compliance suite on unresolved builtins

AGENTS.md

@@ -55,10 +55,14 @@ Use the Gradle wrapper. Java **17** toolchain.
 ## Testing expectations
 
 - Unit tests use JUnit 5 (`useJUnitPlatform()`).
-- Builtin behavior is additionally covered by OPA compliance fixtures. Note that
-  the harness may silently skip cases for unimplemented builtins (a missing
-  function is not a hard failure), so a newly-added builtin needs its own
-  assertion, not just a fixture that "passes."
+- Builtin behavior is additionally covered by OPA compliance fixtures. A fixture
+  whose builtin the SDK cannot resolve fails `ComplianceTest` unless that builtin
+  is listed in
+  `opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt`, so an
+  unimplemented builtin can no longer make its cases pass silently. The list is a
+  ratchet: implementing a builtin (or registering its `BuiltinProvider`) means
+  deleting its line in the same change, since the suite also fails on entries no
+  fixture reports as missing.
 - Add or update tests for the behavior you change; verify against OPA parity for
   builtins.
 

CHANGELOG.md

@@ -5,6 +5,11 @@ project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+### Build and CI
+
+- Fail the compliance suite on fixtures whose builtin cannot be resolved, instead
+  of skipping them silently, and run it for `opa-builtins` changes
+
 ## 0.3.0
 
 This release raises the minimum Java version to 17, adds the `opa-proto` module

CONTRIBUTING.md

@@ -16,8 +16,13 @@ When contributing, please consider the following pointers:
   and `builtin_metadata.json` / `capabilities.json`). When implementing or
   fixing a builtin, verify the corresponding Go behavior for edge cases (key/JWK
   types, error vs. `false`, strict-mode behavior, pre-hashing, etc.) rather than
-  guessing. Note the compliance-test harness may silently skip cases for
-  unimplemented builtins, so a new builtin needs its own explicit assertion.
+  guessing. The compliance-test harness skips a case only when the builtin it
+  calls is listed in
+  `opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt`, so
+  implementing a builtin means deleting its line there in the same change.
+  Behavior the upstream fixtures do not cover still needs its own test — and
+  consider contributing the missing case to OPA, so every implementation is held
+  to it.
 - **Public APIs:** This SDK is meant to be embedded, so keep the public surface
   minimal. Prefer package-private types and methods; only make something `public`
   when consumers genuinely need it. A published API is a long-term commitment.

opa-evaluator/src/test/java/io/github/open_policy_agent/opa/ir/ComplianceTest.java

@@ -32,9 +32,13 @@
 import org.skyscreamer.jsonassert.JSONAssert;
 
 import java.io.ByteArrayInputStream;
+import java.io.BufferedReader;
 import java.io.IOException;
+import java.io.InputStreamReader;
+import java.io.UncheckedIOException;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -47,7 +51,9 @@
 import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.Stack;
+import java.util.TreeSet;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 
@@ -58,10 +64,21 @@ public class ComplianceTest {
       Pattern.compile("^[A-Za-z0-9_-]+\\.[A-Za-z0-9_-]*\\.[A-Za-z0-9_-]*$");
   private static final PolicyReader POLICY_READER =
       ServiceLoader.load(PolicyReader.class).findFirst().orElseThrow();
+  private static final String KNOWN_MISSING_RESOURCE = "compliance/known-missing-builtins.txt";
   private static String complianceDir;
-  private static  Set<String> missingFunctions = ConcurrentHashMap.newKeySet();
 
-  private static  boolean ignoreFunctionNotFoundTests = true;
+  /**
+   * Builtins the fixtures call but this SDK cannot resolve. A case that calls one of these is
+   * skipped; a case calling any other unresolved builtin fails, so an unimplemented builtin can no
+   * longer make its fixtures pass silently.
+   */
+  private static final Set<String> knownMissingBuiltins = loadKnownMissingBuiltins();
+
+  /** Builtins from {@link #knownMissingBuiltins} actually hit during this run. */
+  private static final Set<String> missingFunctions = ConcurrentHashMap.newKeySet();
+
+  private static final AtomicInteger casesExecuted = new AtomicInteger();
+  private static final AtomicInteger casesDiscovered = new AtomicInteger();
 
   /**
    * Alternate acceptable error message substrings for tests where the Java IR evaluator produces a
@@ -96,15 +113,55 @@ public class ComplianceTest {
   @AfterAll
   public static void reportMissingFunctions() {
     if (!missingFunctions.isEmpty()) {
-      String report = buildMissingFunctionsReport();
-      // TODO: add missing builtins
-      System.err.println(report);
+      System.err.println(buildMissingFunctionsReport());
+    }
+    // A filtered run (e.g. --tests "...ComplianceTest$some case") only reaches a subset of the
+    // fixtures, so entries could look stale when they are simply not exercised. Only ratchet when
+    // every discovered case ran.
+    if (casesExecuted.get() != casesDiscovered.get()) {
+      return;
+    }
+    Set<String> stale = new TreeSet<>(knownMissingBuiltins);
+    stale.removeAll(missingFunctions);
+    if (!stale.isEmpty()) {
+      fail(
+          "These builtins are listed in "
+              + KNOWN_MISSING_RESOURCE
+              + " but no compliance case reported them missing — they are either implemented now or"
+              + " no longer covered by a fixture. Remove them from the list: "
+              + stale);
     }
   }
 
+  /**
+   * Reads the known-missing builtin list. One name per line; blank lines and {@code #} comments are
+   * ignored.
+   */
+  private static Set<String> loadKnownMissingBuiltins() {
+    URL resource = ComplianceTest.class.getClassLoader().getResource(KNOWN_MISSING_RESOURCE);
+    if (resource == null) {
+      throw new IllegalStateException(KNOWN_MISSING_RESOURCE + " not found on the classpath");
+    }
+    Set<String> names = new TreeSet<>();
+    try (BufferedReader reader =
+        new BufferedReader(new InputStreamReader(resource.openStream(), StandardCharsets.UTF_8))) {
+      reader
+          .lines()
+          .map(line -> line.replaceFirst("#.*", "").trim())
+          .filter(line -> !line.isEmpty())
+          .forEach(names::add);
+    } catch (IOException e) {
+      throw new UncheckedIOException(e);
+    }
+    return Set.copyOf(names);
+  }
+
   private static String buildMissingFunctionsReport() {
     StringBuilder sb = new StringBuilder();
     sb.append("\n=== Missing Builtin Report ===\n");
+    sb.append("Cases skipped for builtins listed in ")
+        .append(KNOWN_MISSING_RESOURCE)
+        .append(":\n");
     sb.append("Total missing builtins: ").append(missingFunctions.size()).append("\n");
     missingFunctions.stream().sorted().forEach(fn -> sb.append("  - ").append(fn).append("\n"));
     sb.append("================================\n");
@@ -123,6 +180,7 @@ public static Stream<Object[]> getComplianceTestData() throws IOException {
                       JsonNode root = mapper.readTree(f);
                       List<JsonNode> cases = new ArrayList<>();
                       root.get("cases").forEach(cases::add);
+                      casesDiscovered.addAndGet(cases.size());
                       return cases.stream()
                               .map(c -> new Object[]{
                                       c.get("note").asText("unknown"), c
@@ -343,6 +401,7 @@ private static RegoValue jsonNodeToRegoValue(
   @ParameterizedTest(name = "{0}")
   @MethodSource("getComplianceTestData")
   public void testEvaluate(String caseName, JsonNode root) {
+    casesExecuted.incrementAndGet();
     try {
       ObjectMapper mapper = new ObjectMapper().registerModule(new io.github.open_policy_agent.opa.jackson.RegoValueModule());
 
@@ -422,11 +481,14 @@ public void testEvaluate(String caseName, JsonNode root) {
 
         JsonNode want = root.get("want_result");
         if (want == null) {
-          JsonNode wantError = root.get("want_error");
-          if (wantError != null) {
-            fail("error wanted but not thrown: " + wantError.asText());
+          if (root.has("want_error") || root.has("want_error_code")) {
+            fail(
+                "error wanted but not thrown: "
+                    + (root.has("want_error")
+                        ? root.get("want_error").asText()
+                        : root.get("want_error_code").asText()));
           }
-          System.out.println("no want_result for: " + caseName);
+          fail("case declares neither want_result nor want_error, so it asserts nothing");
           return;
         }
 
@@ -448,15 +510,21 @@ public void testEvaluate(String caseName, JsonNode root) {
           }
         }
       } catch (OpaException re) {
-        // Track missing functions for reporting
+        // A builtin the SDK does not resolve makes the case unrunnable. Skip it only when the
+        // builtin is a known gap, so that an unlisted one fails instead of passing silently.
         if (re instanceof FunctionNotFoundError) {
           String functionName = (String) re.getContext().get("name");
-          if (functionName != null) {
-            missingFunctions.add(functionName);
-          }
-          if (ignoreFunctionNotFoundTests) {
-            return;
+          if (functionName == null || !knownMissingBuiltins.contains(functionName)) {
+            fail(
+                "builtin '"
+                    + functionName
+                    + "' could not be resolved, so this case asserted nothing. Implement the"
+                    + " builtin (and register its provider in META-INF/services), or add it to "
+                    + KNOWN_MISSING_RESOURCE
+                    + " with a reason.");
           }
+          missingFunctions.add(functionName);
+          return;
         }
 
         if (root.get("want_error_code") == null && root.get("want_error") == null) {

opa-evaluator/src/test/resources/compliance/known-missing-builtins.txt

@@ -0,0 +1,117 @@
+# Builtins that the OPA compliance fixtures exercise but this SDK does not
+# resolve at evaluation time. ComplianceTest skips a case only when the builtin
+# it calls is listed here, and fails the case otherwise — so a fixture can no
+# longer pass silently just because its builtin is absent (see AGENTS.md).
+#
+# The list is a ratchet: ComplianceTest also fails when an entry here is no
+# longer missing, so implementing a builtin means deleting its line in the same
+# change. Format is one builtin name per line; `#` starts a comment.
+
+# --- Implemented, but never registered ---------------------------------------
+# These live in opa-builtins sub-modules and are listed as supported in
+# opa-builtins/README.md, but the BuiltinProvider entry in each module's
+# META-INF/services file is commented out, so ServiceLoader never finds them
+# and they are unreachable for consumers. Tracked separately from this list;
+# un-commenting the five files is the fix, after which the parity failures the
+# fixtures then expose need triage.
+
+# opa-builtins-crypto
+crypto.hmac.equal
+crypto.hmac.md5
+crypto.hmac.sha1
+crypto.hmac.sha256
+crypto.hmac.sha512
+crypto.md5
+crypto.sha1
+crypto.sha256
+
+# opa-builtins-json
+json.filter
+json.is_valid
+json.marshal
+json.marshal_with_options
+json.match_schema
+json.patch
+json.remove
+json.unmarshal
+json.verify_schema
+yaml.is_valid
+yaml.marshal
+yaml.unmarshal
+
+# opa-builtins-net
+net.cidr_contains
+net.cidr_contains_matches
+net.cidr_expand
+net.cidr_intersects
+net.cidr_is_valid
+net.cidr_merge
+net.lookup_ip_addr
+
+# opa-builtins-regex
+regex.find_all_string_submatch_n
+regex.find_n
+regex.globs_match
+regex.is_valid
+regex.match
+regex.replace
+regex.split
+regex.template_match
+
+# opa-builtins-semver
+semver.compare
+semver.is_valid
+
+# --- Not implemented anywhere in the SDK -------------------------------------
+
+bits.and
+bits.lsh
+bits.negate
+bits.or
+bits.rsh
+bits.xor
+
+crypto.parse_private_keys
+crypto.x509.parse_and_verify_certificates
+crypto.x509.parse_certificate_request
+crypto.x509.parse_certificates
+crypto.x509.parse_keypair
+crypto.x509.parse_rsa_private_key
+
+glob.match
+glob.quote_meta
+
+graph.reachable
+graph.reachable_paths
+
+graphql.is_valid
+graphql.parse
+graphql.parse_and_verify
+graphql.parse_query
+graphql.parse_schema
+graphql.schema_is_valid
+
+http.send
+
+internal.template_string
+
+io.jwt.verify_eddsa
+
+rego.parse_module
+
+strings.render_template
+strings.split_n
+
+units.parse
+units.parse_bytes
+
+urlquery.decode
+urlquery.decode_object
+urlquery.encode
+urlquery.encode_object
+
+uuid.parse
+uuid.rfc4122
+
+# Delete this line along with the walk implementation in #141.
+walk

tools/policy/pr-check/pr_check.rego

@@ -9,6 +9,17 @@ build_change_files := [
 changes["opa-evaluator"] if {
 	some changed_file in input
 	startswith(changed_file.filename, "opa-evaluator/")
+} else if {
+	# ComplianceTest lives in opa-evaluator but resolves builtins from the opa-builtins sub-modules
+	# over the BuiltinProvider SPI, and fails on any fixture whose builtin it cannot resolve. A
+	# builtins-only change (implementing a builtin, or registering its provider) therefore has to
+	# re-run the compliance suite.
+	some changed_file in input
+	startswith(changed_file.filename, "opa-builtins/")
+} else if {
+	# An OPA version bump here regenerates the compliance fixtures the suite reads.
+	some changed_file in input
+	startswith(changed_file.filename, "tools/generate-compliance-tests/")
 } else if {
 	some changed_file in input
 	changed_file.filename in build_change_files

tools/policy/pr-check/pr_check_test.rego

@@ -68,6 +68,15 @@ test_builtins_change_triggers_builtins if {
 	pr_check.changes["opa-builtins"] with input as example_builtins_changelist
 }
 
+# The compliance suite that gates builtin coverage runs in opa-evaluator.
+test_builtins_change_triggers_evaluator if {
+	pr_check.changes["opa-evaluator"] with input as example_builtins_changelist
+}
+
+test_generator_change_triggers_evaluator if {
+	pr_check.changes["opa-evaluator"] with input as example_generator_changelist
+}
+
 test_slf4j_change_triggers_slf4j if {
 	pr_check.changes["opa-slf4j"] with input as example_slf4j_changelist
 }