test: tests and general improvements

builtins/pdps/cedar-direct/src/factory.rs

@@ -7,12 +7,13 @@
 //
 // ```yaml
 // global:
-//   pdp:
-//     - kind: cedar-direct
-//       dialect: cedar          # optional, defaults to PdpDialect::Cedar
-//       policy_text: |          # required (or policy_file)
-//         @id("owner-override")
-//         permit(...);
+//   apl:
+//     pdp:
+//       - kind: cedar-direct
+//         dialect: cedar          # optional, defaults to PdpDialect::Cedar
+//         policy_text: |          # required (or policy_file)
+//           @id("owner-override")
+//           permit(...);
 // ```
 //
 // Hosts register an instance of this factory in `AplOptions.pdp_factories`;

builtins/pdps/cel/src/activation.rs

@@ -161,18 +161,21 @@ fn node_to_value(node: Node) -> Value {
 
 /// Convert one `AttributeValue` to a `cel::Value`.
 ///
-/// CEL's type model distinguishes `int` and `double` strictly:
-/// `delegation.depth <= 2` errors if `delegation.depth` is a double
-/// and `2` is an int (the literal). To shield authors from that
-/// asymmetry, an `f64` whose value is a whole number and fits a `i64`
-/// is yielded as `Value::Int`. The same logic applies to the
-/// author-supplied yaml args (see `yaml_to_value`) — both surfaces
-/// now agree.
+/// An `f64` is yielded as `Value::Float`, never silently narrowed to an int.
+/// CEL's `==` / `<=` / `<` and friends already compare an int literal against
+/// a double operand (verified against the pinned `cel` version's ordering
+/// impls and pinned by test), so `delegation.depth <= 2` works with a
+/// double-valued `depth`. Narrowing a whole-valued double to an int used to be
+/// done "to help literal comparison", but it broke float *arithmetic*: a
+/// `confidence` of exactly `1.0` became `int 1`, and `confidence * 100.0` then
+/// errored with "no such overload" (int × double) — so the maximum confidence
+/// was denied while a lower one was allowed, an outcome inversion driven purely
+/// by whether the value happened to be integral.
 fn attr_to_value(attr: &AttributeValue) -> Value {
     match attr {
         AttributeValue::Bool(b) => Value::from(*b),
         AttributeValue::Int(i) => Value::from(*i),
-        AttributeValue::Float(f) => float_to_value(*f),
+        AttributeValue::Float(f) => Value::from(*f),
         AttributeValue::String(s) => Value::from(s.clone()),
         // StringSet → list(string). Sort before yielding so authors
         // who reach for `session.labels[0]` (or any other
@@ -189,34 +192,11 @@ fn attr_to_value(attr: &AttributeValue) -> Value {
     }
 }
 
-/// Yield an `f64` as `Value::Int` when it represents a whole number
-/// in `i64` range, otherwise `Value::Float`. Used by both
-/// `attr_to_value` (bag scalars) and `yaml_to_value` (author args) so
-/// `delegation.depth: 2` works against the literal `2` regardless of
-/// whether the bag populated it as `Int(2)` or `Float(2.0)`.
-#[allow(
-    clippy::cast_possible_truncation,
-    clippy::cast_precision_loss,
-    reason = "the conversion is guarded to finite, integral, in-range values; the \
-              bound casts are deliberate and explained below"
-)]
-fn float_to_value(f: f64) -> Value {
-    // The upper bound is strict on purpose. `i64::MAX as f64` cannot represent
-    // 2^63 - 1 and rounds up to exactly 2^63, so `<=` against it would admit
-    // 2^63, which is one past the last i64 and saturates on conversion. `<` is
-    // then exactly the right test. `i64::MIN as f64` is exact at -2^63, so the
-    // lower bound stays inclusive.
-    if f.is_finite() && f.fract() == 0.0 && f >= i64::MIN as f64 && f < i64::MAX as f64 {
-        Value::from(f as i64)
-    } else {
-        Value::from(f)
-    }
-}
-
 /// Convert a `serde_yaml::Value` (author-supplied `cel:` args) to a
-/// `cel::Value`. Numbers without a fractional part map to `Int`, otherwise
-/// `Float`. Non-string mapping keys are skipped (CEL map keys here are
-/// always strings for author ergonomics).
+/// `cel::Value`. An integer literal maps to `Int`, a fractional one to
+/// `Float`; a float is never narrowed to an int (same reasoning as
+/// `attr_to_value`). Non-string mapping keys are skipped (CEL map keys here
+/// are always strings for author ergonomics).
 fn yaml_to_value(v: &serde_yaml::Value) -> Value {
     match v {
         serde_yaml::Value::Null => Value::Null,
@@ -225,7 +205,7 @@ fn yaml_to_value(v: &serde_yaml::Value) -> Value {
             if let Some(i) = n.as_i64() {
                 Value::from(i)
             } else {
-                float_to_value(n.as_f64().unwrap_or(f64::NAN))
+                Value::from(n.as_f64().unwrap_or(f64::NAN))
             }
         },
         serde_yaml::Value::String(s) => Value::from(s.clone()),
@@ -304,23 +284,73 @@ mod tests {
         assert!(truthy("session.labels.exists(l, l == 'PII')", &bag));
     }
 
-    /// An `f64` whose value is a whole number is yielded as an int so
-    /// authors can compare against integer literals without CEL's
-    /// strict int-vs-double type rules blowing up. A genuinely
-    /// fractional `f64` still arrives as a float (so `confidence > 0.9`
-    /// behaves correctly).
+    /// A double-valued bag scalar compares correctly against an integer
+    /// literal without being narrowed to an int — CEL's ordering handles the
+    /// mixed comparison. Narrowing is deliberately not done because it breaks
+    /// float arithmetic (see `whole_valued_float_keeps_arithmetic`).
     #[test]
-    fn whole_number_float_arrives_as_int_for_literal_compare() {
+    fn double_scalar_compares_against_int_literal() {
         let mut bag = AttributeBag::new();
         bag.set("delegation.depth", 2.0_f64);
         bag.set("intent.confidence", 0.92_f64);
-        // Compare-with-int-literal: requires the bag value to be int.
         assert!(truthy("delegation.depth == 2", &bag));
         assert!(truthy("delegation.depth <= 2", &bag));
         // Genuine doubles still compare to double literals.
         assert!(truthy("intent.confidence > 0.9", &bag));
     }
 
+    /// Regression: a whole-valued double (`1.0`) must stay a double so that
+    /// float arithmetic on it still resolves. Narrowing it to `int 1` made
+    /// `confidence * 100.0` fail with "no such overload" and denied the
+    /// maximum-confidence case while allowing lower ones.
+    #[test]
+    fn whole_valued_float_keeps_arithmetic() {
+        let mut bag = AttributeBag::new();
+        bag.set("intent.confidence", 1.0_f64);
+        assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag));
+        // And the sub-1.0 case is unchanged.
+        bag.set("intent.confidence", 0.92_f64);
+        assert!(truthy("intent.confidence * 100.0 >= 90.0", &bag));
+    }
+
+    /// The float-stays-float change relies on CEL comparing an int literal
+    /// against a double operand in *either* operand order. The existing tests
+    /// only cover `depth <op> 2`; pin the reversed `2 <op> depth` form too, so a
+    /// one-sided comparison impl can't silently break the half of author
+    /// policies that write the literal on the left.
+    #[test]
+    fn mixed_int_float_comparison_is_order_independent() {
+        let mut bag = AttributeBag::new();
+        bag.set("delegation.depth", 2.0_f64);
+        assert!(truthy("2 == delegation.depth", &bag));
+        assert!(truthy("2 <= delegation.depth", &bag));
+        assert!(truthy("2 >= delegation.depth", &bag));
+        assert!(truthy("3 > delegation.depth", &bag));
+        assert!(truthy("1 < delegation.depth", &bag));
+        // A true inequality must resolve to a value, not a "no such overload"
+        // error; asserted via its positive form so an error can't pass as false.
+        assert!(truthy("3 != delegation.depth", &bag));
+    }
+
+    #[test]
+    fn whole_valued_float_in_int_list_matches() {
+        // Guard the float-stays-float change against the `in` operator: a
+        // whole-valued double must still be found in an int list (CEL's `in`
+        // uses cross-type equality), so membership doesn't invert on int-vs-
+        // double the way the old narrowing avoided for `==` but broke for `*`.
+        let mut bag = AttributeBag::new();
+        bag.set("delegation.depth", 2.0_f64);
+        assert!(
+            truthy("delegation.depth in [1, 2, 3]", &bag),
+            "float 2.0 in int list"
+        );
+        bag.set("intent.confidence", 1.0_f64);
+        assert!(
+            truthy("intent.confidence in [0.5, 1.0]", &bag),
+            "float in float list"
+        );
+    }
+
     /// `StringSet` is yielded in sorted order so indexing returns a
     /// stable value across runs. `"compensation" < "PII"` (ASCII;
     /// uppercase letters sort before lowercase, but both labels here

builtins/pdps/cel/src/factory.rs

@@ -6,9 +6,10 @@
 //
 // ```yaml
 // global:
-//   pdp:
-//     - kind: cel
-//       on_error: deny          # optional; deny | allow, default deny
+//   apl:
+//     pdp:
+//       - kind: cel
+//         on_error: deny          # optional; deny | allow, default deny
 // ```
 //
 // The CEL expression itself lives in each route's `cel: { expr: "..." }`

builtins/pdps/cel/src/resolver.rs

@@ -209,8 +209,8 @@ impl CelResolver {
     /// eager-compile knob of its own.
     /// # Errors
     ///
-    /// Returns `BuildError` when the block is not a mapping or a setting is out
-    /// of range, such as a zero cache cap.
+    /// Returns `BuildError` when the block is not a mapping, carries an unknown
+    /// key, or gives `on_error` a value other than `deny` / `allow`.
     pub fn from_config(value: &serde_yaml::Value) -> Result<Self, BuildError> {
         let map = value
             .as_mapping()

builtins/pdps/opa/src/input.rs

@@ -130,8 +130,11 @@ fn attr_to_value(attr: &AttributeValue) -> Value {
 }
 
 /// Yield an `f64` as a JSON integer when it is a whole number in `i64` range,
-/// otherwise a JSON float. Keeps parity with CEL's `float_to_value` so a bag
-/// value populated as `Float(2.0)` reads as `2` for an author. A non-finite
+/// otherwise a JSON float, so a bag value populated as `Float(2.0)` reads as
+/// `2` for an author. Rego has a single unified `number` type, so narrowing a
+/// whole-valued float never breaks arithmetic or comparison here the way it did
+/// in CEL. CEL for that reason stopped narrowing and now keeps every float a
+/// double, so the two PDPs deliberately diverge on this point. A non-finite
 /// float has no JSON representation and becomes `null`.
 #[allow(
     clippy::cast_possible_truncation,

builtins/pdps/opa/src/resolver.rs

@@ -313,7 +313,19 @@ impl OpaResolver {
         // Reject an inline module that lands in a global module's package — it
         // would merge into (and could override) operator policy. Fail-closed:
         // inline modules may add new packages, never redefine a global one.
-        if self.global_packages.contains(&package) {
+        //
+        // The check is prefix-aware, not exact-match: package paths are dotted
+        // (`data.authz`), and a global rule reads whole subtrees, so an inline
+        // module in a *sub-package* of a global one (`data.authz.exceptions`
+        // under `data.authz`) still feeds `data.authz.*` that a global
+        // `authz` rule can consume — an override by the back door. Reject when
+        // the inline package equals, is nested under, or contains any global
+        // package, so the two never share a `data` subtree.
+        if self
+            .global_packages
+            .iter()
+            .any(|g| packages_share_subtree(&package, g))
+        {
             return Err(EngineError::PackageCollision(package));
         }
 
@@ -385,6 +397,17 @@ impl OpaResolver {
     }
 }
 
+/// True when two dotted Rego package paths occupy the same `data` subtree —
+/// they are equal, or one is nested under the other (`data.authz` and
+/// `data.authz.exceptions`). A bare prefix comparison is wrong: `data.authz`
+/// must not be judged to contain `data.authznext`, so the boundary is only a
+/// match when the next character is a path separator.
+fn packages_share_subtree(a: &str, b: &str) -> bool {
+    a == b
+        || a.strip_prefix(b).is_some_and(|rest| rest.starts_with('.'))
+        || b.strip_prefix(a).is_some_and(|rest| rest.starts_with('.'))
+}
+
 /// Internal — failure shapes from preparing a per-step engine. All three
 /// always deny regardless of `on_error`: a compile error is an author bug, a
 /// package collision is a trust-boundary violation, and a cache-full condition
@@ -965,6 +988,30 @@ msg := "not a decision"
         }
     }
 
+    /// An inline module in a *sub-package* of a global package is rejected
+    /// fail-closed. A global `authz` rule reads whole `data.authz.*` subtrees,
+    /// so an inline `package authz.exceptions` still feeds operator policy even
+    /// though it never names `package authz` directly — the back-door override
+    /// the exact-match check used to miss.
+    #[tokio::test]
+    async fn inline_module_cannot_override_global_subpackage() {
+        // Global policy allows only when the caller is listed under
+        // `data.authz.exceptions` — a subtree an inline module must not reach.
+        let global = "package authz\ndefault allow := false\nallow if data.authz.exceptions[input.subject.id]\n";
+        let r = resolver(&[global], OnError::Allow);
+        let inline = "package authz.exceptions\nexceptions := {\"eve\": true}\n";
+        let out = r
+            .evaluate(&call("data.authz.allow", Some(inline)), &bag("eve"))
+            .await
+            .unwrap();
+        match out.decision {
+            Decision::Deny { reason, .. } => {
+                assert!(reason.unwrap_or_default().contains("collides"));
+            },
+            other => panic!("sub-package collision must deny, got {other:?}"),
+        }
+    }
+
     /// An inline module in a fresh package (no global collision) is accepted and
     /// evaluates — inline modules remain a usable feature for additive policy.
     #[tokio::test]
@@ -978,6 +1025,51 @@ msg := "not a decision"
         assert_eq!(out.decision, Decision::Allow);
     }
 
+    /// The collision check must be prefix-*boundary* aware, not a bare string
+    /// prefix: a global `data.authz` must not be judged to contain
+    /// `data.authznext` just because one string-prefixes the other. A
+    /// sibling-prefix package is a genuinely separate `data` subtree and stays
+    /// allowed; a naive `starts_with` would wrongly reject it (fail-closed, but
+    /// a spurious denial of a legitimate inline module).
+    #[tokio::test]
+    async fn inline_module_in_prefix_sibling_package_is_allowed() {
+        let global = "package authz\nallow if input.subject.id == \"alice\"\n";
+        let r = resolver(&[global], OnError::Deny);
+        // `authznext` shares the `authz` string prefix but is a different subtree.
+        let inline = "package authznext\nallow if input.subject.id == \"alice\"\n";
+        let out = r
+            .evaluate(&call("data.authznext.allow", Some(inline)), &bag("alice"))
+            .await
+            .unwrap();
+        assert_eq!(
+            out.decision,
+            Decision::Allow,
+            "a prefix-sibling package must not collide with a global package"
+        );
+    }
+
+    /// The symmetric case: an inline module whose package is a *parent* of a
+    /// global package (inline `data.authz`, global `data.authz.sub`) still feeds
+    /// a `data` subtree the operator policy reads, so it is rejected fail-closed.
+    /// This exercises the `b.strip_prefix(a)` arm of `packages_share_subtree`,
+    /// which the sub-package test (child-of-global) does not.
+    #[tokio::test]
+    async fn inline_module_that_is_parent_of_global_collides() {
+        let global = "package authz.sub\nallow if input.subject.id == \"alice\"\n";
+        let r = resolver(&[global], OnError::Allow);
+        let inline = "package authz\nallow := true\n";
+        let out = r
+            .evaluate(&call("data.authz.allow", Some(inline)), &bag("alice"))
+            .await
+            .unwrap();
+        match out.decision {
+            Decision::Deny { reason, .. } => {
+                assert!(reason.unwrap_or_default().contains("collides"));
+            },
+            other => panic!("parent-package collision must deny, got {other:?}"),
+        }
+    }
+
     #[tokio::test]
     async fn missing_query_is_dispatch_error() {
         let r = resolver(&[ALLOW_WITH_DEFAULT], OnError::Deny);

builtins/plugins/delegator-oauth/src/factory.rs

@@ -15,7 +15,7 @@
 //         config:
 //           token_endpoint: https://idp.example.com/token
 //           client_id: praxis-gateway
-//           client_secret_source: { kind: env, var: OAUTH_CLIENT_SECRET }
+//           client_secret_source: { kind: env_var, name: OAUTH_CLIENT_SECRET }
 //
 // The `kind: delegator/oauth` string is part of this crate's public
 // API. Hosts call

builtins/plugins/elicitation-ciba/src/factory.rs

@@ -14,7 +14,7 @@
 //           backchannel_endpoint: https://kc/realms/corp/protocol/openid-connect/ext/ciba/auth
 //           token_endpoint:       https://kc/realms/corp/protocol/openid-connect/token
 //           client_id: praxis-policy-gateway
-//           client_secret_source: { kind: env, name: CIBA_CLIENT_SECRET }
+//           client_secret_source: { kind: env_var, name: CIBA_CLIENT_SECRET }
 //
 // Then policy routes name it: `require_approval(manager-approver, from: claim.manager, ...)`.
 //