fix: two checks that could not do what they claimed

CHANGELOG.md

@@ -12,6 +12,23 @@ under the pre-1.0 conventions in [VERSIONING.md](docs/governance/VERSIONING.md):
 
 ### Fixed
 
+- **A GTIN check on the create path could not fail, and read as though it were
+  the only thing checking.** `POST /api/v1/dpp` re-validated the GS1 check digit
+  of `ProductGroupData::Battery`'s GTIN — a value that had already been through
+  `Gtin::parse`, so the second check could only ever succeed.
+
+  The branch matched on the battery variant alone, which made it look like ten
+  other product groups carrying a `gtin` were going unchecked. They are not.
+  Every typed payload declares `gtin: Gtin`, `Gtin`'s `Deserialize` calls
+  `Gtin::parse`, and `Gtin`'s inner field is private with `parse` as its only
+  constructor — so an invalid GTIN cannot be deserialised, cannot be constructed,
+  and never reaches a handler, for all eleven product groups at once.
+
+  The dead branch is removed and three tests (`gtin_boundary`) pin where the
+  rejection actually happens, so the next reader does not have to re-derive it.
+  **No behaviour change**: a malformed GTIN was refused before this change and is
+  refused after it, at the same point in the request.
+
 - **A restricted product-group field could have reached the public view.** The
   resolver filters a passport in two passes — the envelope, then
   `productGroupData` handed on as its own root document. The core filter now

CLAUDE.md

@@ -437,6 +437,35 @@ The internal endpoints are mTLS-gated (`CN=odal-vault`).
 `just test` (unit, no Docker) and `just test-integration` (the Docker tiers). See
 Build and Development above for the full recipe set.
 
+### Answer "what actually happens here?" with a test you keep
+
+When you need to establish a fact about behaviour — does this reject that input,
+which layer enforces this rule, is this branch reachable — **write a `#[test]`
+and commit it.** Not a scratch binary, not a `python - <<EOF` probe, not a
+throwaway `main` you delete afterwards.
+
+The reason is not tidiness. A throwaway probe answers the question once, for the
+person running it, and then the answer lives only in their head — so the next
+person re-derives it, or worse, assumes the opposite. A committed test answers it
+permanently *and* fails when the answer changes.
+
+This is not hypothetical. A handler here re-validated a GTIN that
+`Gtin::Deserialize` had already validated, so the check could not fail; it read
+as the thing enforcing GTIN validity for one product group and no other, which
+was the reverse of the truth. Three small tests (`gtin_boundary` in
+`dpp-vault/src/handlers/create.rs`) now pin where the rejection actually happens,
+and the dead branch is gone.
+
+Name such a test for the fact it pins, not the function it calls:
+`a_bad_check_digit_is_refused_while_the_body_is_parsed`, not `test_gtin`.
+
+Two practical notes:
+- **Never run a foreground command that can wait on stdin** (`python - <<EOF`,
+  an interactive REPL). It hangs the session rather than failing.
+- **A probe that "passes" proves nothing until you have seen it fail.** Confirm
+  the assertion actually bites — change the input, watch it go red — before
+  trusting a green result.
+
 Test tiers:
 - **Tier 1 (no DB)**: route mounting, health endpoints, auth middleware, validators, parsers, and the pure-logic unit tests inside each crate.
 - **Tier 2 (testcontainers)**: the full lifecycle through real PostgreSQL. Gated behind the `integration-tests` feature, so `just test` never builds them — which is why `just check` also runs `check-integration` to prove they still *compile*.

crates/dpp-integrator/src/handlers/schemas.rs

@@ -163,12 +163,16 @@ fn unknown_version(product_group: &str, version: &str) -> Response {
 /// a naive walk — taking a real field out of the contract an SDK validates
 /// against. No schema declares one today; this costs nothing and stops that
 /// being a latent trap for whoever adds the first.
-fn strip_descriptions(node: &mut Value) {
-    /// Keywords whose object values are keyed by author-chosen names, not by
-    /// schema keywords — descend into the values, never treat the keys as
-    /// keywords.
-    const NAME_KEYED: [&str; 4] = ["properties", "$defs", "definitions", "patternProperties"];
+/// Keywords whose object values are keyed by author-chosen names, not by
+/// schema keywords — descend into the values, never treat the keys as
+/// keywords.
+///
+/// Module-level rather than local so the test that checks the result walks by
+/// the same list. Two copies of this would drift, and the direction they drift
+/// is a check that quietly stops looking.
+const NAME_KEYED: [&str; 4] = ["properties", "$defs", "definitions", "patternProperties"];
 
+fn strip_descriptions(node: &mut Value) {
     match node {
         Value::Object(map) => {
             map.remove("description");
@@ -268,19 +272,87 @@ mod tests {
         );
     }
 
+    /// Collect the paths of any surviving `description` **keyword**, walking the
+    /// tree the way `strip_descriptions` does.
+    ///
+    /// A substring search over the serialised schema cannot answer this, because
+    /// a property may legitimately be *named* `description` — unsold-goods
+    /// v2.0.0 has one, the line description of Impl. Reg. (EU) 2026/2 Annex I
+    /// note (e). Its name has to survive stripping; its own description keyword
+    /// must not. Only a structural walk can tell those apart.
+    fn surviving_description_keywords(node: &Value, path: &str, out: &mut Vec<String>) {
+        match node {
+            Value::Object(map) => {
+                if map.contains_key("description") {
+                    out.push(path.to_owned());
+                }
+                for (key, value) in map {
+                    if NAME_KEYED.contains(&key.as_str()) {
+                        if let Value::Object(named) = value {
+                            for (name, schema) in named {
+                                surviving_description_keywords(
+                                    schema,
+                                    &format!("{path}/{key}/{name}"),
+                                    out,
+                                );
+                            }
+                        }
+                    } else {
+                        surviving_description_keywords(value, &format!("{path}/{key}"), out);
+                    }
+                }
+            }
+            Value::Array(items) => {
+                for (i, item) in items.iter().enumerate() {
+                    surviving_description_keywords(item, &format!("{path}/{i}"), out);
+                }
+            }
+            _ => {}
+        }
+    }
+
     #[test]
     fn no_embedded_schema_keeps_a_description_after_stripping() {
         let registry = VersionedSchemaRegistry::new();
         for (product_group, version) in registry.list() {
             let raw = registry.get(product_group, version).expect("just listed");
             let mut schema: Value = serde_json::from_str(raw).unwrap();
             strip_descriptions(&mut schema);
+
+            let mut surviving = Vec::new();
+            surviving_description_keywords(&schema, "", &mut surviving);
             assert!(
-                !serde_json::to_string(&schema)
-                    .unwrap()
-                    .contains("\"description\""),
-                "{product_group} v{version} still carries a description keyword after stripping"
+                surviving.is_empty(),
+                "{product_group} v{version} still carries a description keyword at: {}",
+                surviving.join(", ")
             );
         }
     }
+
+    #[test]
+    fn the_detector_reports_keywords_and_ignores_a_property_of_that_name() {
+        // A green check proves nothing until it has been seen to fail, and this
+        // one replaced an assertion that could not distinguish these two cases
+        // at all. So: one schema, un-stripped, holding both.
+        let schema: Value = json!({
+            "description": "root prose",
+            "properties": {
+                // A field named `description` — a contract, not prose. Its own
+                // description keyword *is* prose and must be reported.
+                "description": { "type": "string", "description": "Note (e)" },
+                // No description keyword anywhere: must not be reported.
+                "unitsDiscarded": { "type": "integer" }
+            }
+        });
+
+        let mut found = Vec::new();
+        surviving_description_keywords(&schema, "", &mut found);
+        found.sort();
+
+        assert_eq!(
+            found,
+            vec!["".to_owned(), "/properties/description".to_owned()],
+            "the root keyword and the one on the `description` field, and nothing else"
+        );
+    }
 }

crates/dpp-vault/src/handlers/create.rs

@@ -8,7 +8,6 @@ use axum::{
 
 use chrono::Utc;
 use dpp_common::url_guard::validate_public_https_url;
-use dpp_digital_link::validate_gtin;
 use dpp_domain::{
     ProductGroupCatalog,
     passport::{Passport, PassportId, PassportRef},
@@ -34,8 +33,12 @@ pub use dpp_types::CreatePassportRequest as CreateRequest;
 /// `POST /api/v1/dpp` — validate fields and create a new passport in `Draft` status.
 ///
 /// Rejects blank required fields, unsafe Unicode characters (null bytes, bidi
-/// overrides), out-of-range numeric values, invalid product group data, and malformed
-/// GTINs before touching the database.
+/// overrides), out-of-range numeric values and invalid product group data before
+/// touching the database.
+///
+/// A malformed GTIN never reaches here: every typed payload declares
+/// `gtin: Gtin`, whose `Deserialize` validates the GS1 check digit, so the body
+/// fails to parse. See the `gtin_boundary` tests.
 pub async fn create_handler(
     State(state): State<AppState>,
     Extension(auth): Extension<AuthContext>,
@@ -399,6 +402,65 @@ mod schema_validation {
     }
 }
 
+#[cfg(test)]
+mod gtin_boundary {
+    //! Where a malformed GTIN is actually refused.
+    //!
+    //! Every typed payload declares `gtin: Gtin`, and `Gtin`'s `Deserialize`
+    //! calls `Gtin::parse`. So a bad check digit is rejected while the request
+    //! body is being parsed, for every product group at once, before any handler
+    //! validation runs. These tests pin that, because the handler's own GTIN
+    //! check reads as if it were the thing enforcing it.
+    use super::*;
+
+    fn tyre_body(gtin: &str) -> serde_json::Value {
+        serde_json::json!({
+            "productName": "All-season 205/55R16",
+            "manufacturer": { "name": "M", "address": "A" },
+            "productGroupData": {
+                "productGroup": "tyre",
+                "gtin": gtin,
+                "tyreClass": "C1",
+                "fuelEfficiencyClass": "A",
+                "wetGripClass": "A",
+                "externalRollingNoiseDb": 70.0
+            }
+        })
+    }
+
+    #[test]
+    fn a_bad_check_digit_is_refused_while_the_body_is_parsed() {
+        // Valid 14-digit shape, wrong GS1 mod-10 check digit.
+        let err = serde_json::from_value::<CreateRequest>(tyre_body("09506000134353"))
+            .expect_err("a bad check digit must not deserialize");
+        assert!(
+            err.to_string().to_lowercase().contains("check digit"),
+            "the rejection should name the check digit, got: {err}"
+        );
+    }
+
+    #[test]
+    fn a_valid_gtin_parses_and_the_request_is_accepted() {
+        let body = serde_json::from_value::<CreateRequest>(tyre_body("09506000134352"))
+            .expect("a valid GTIN must deserialize");
+        assert!(
+            validate_create_request(&body).is_none(),
+            "a well-formed tyre body must pass every create validation"
+        );
+    }
+
+    #[test]
+    fn the_handler_reads_the_gtin_generically_not_battery_only() {
+        // The handler asks the payload rather than matching on the variant. If
+        // this ever returns `None` for a product group that declares a `gtin`,
+        // the check silently stops covering it — which is how it came to cover
+        // battery alone.
+        let body = serde_json::from_value::<CreateRequest>(tyre_body("09506000134352")).unwrap();
+        let data = body.product_group_data.expect("payload present");
+        assert_eq!(data.gtin(), Some("09506000134352"));
+    }
+}
+
 /// Every validation `POST /api/v1/dpp` applies to a request body, with no side
 /// effects. Returns the rejection response, or `None` when the body would be
 /// accepted.
@@ -473,16 +535,17 @@ pub fn validate_create_request(body: &CreateRequest) -> Option<axum::response::R
             );
         }
 
-        // GS1 GTIN check-digit validation for Battery passports.
-        if let ProductGroupData::Battery(battery) = sd
-            && let Err(e) = validate_gtin(battery.gtin.as_str())
-        {
-            return api_error(
-                StatusCode::UNPROCESSABLE_ENTITY,
-                "VALIDATION_ERROR",
-                &format!("productGroupData.gtin: {e}"),
-            );
-        }
+        // No GTIN check here, deliberately. Every typed payload declares
+        // `gtin: Gtin`, and `Gtin`'s `Deserialize` calls `Gtin::parse`, so a bad
+        // check digit is refused while this body is being parsed — for all
+        // eleven product groups that carry one, before this function is
+        // reached. `Gtin`'s inner field is private and `parse` is the only
+        // constructor, so a `Gtin` that has not been validated cannot exist.
+        //
+        // What stood here re-validated `ProductGroupData::Battery`'s already-parsed
+        // GTIN and could not fail. It read as the thing enforcing GTIN validity
+        // for battery and no other product group, which is the opposite of what
+        // was true. See the `gtin_boundary` tests.
 
         // JSON-Schema validation against the product group's current versioned schema —
         // catches schema-only constraints (string patterns, enum sets, numeric