#3610: a shapes gate reported Pass over ZERO focus nodes — the vacuity check was global, so a directory hid it

ci/explicit-test-commands.d/450-aprender-contracts-cli-shapes-vacuity.cmd

@@ -0,0 +1 @@
+cargo test -p aprender-contracts-cli --test shapes_vacuity

crates/aprender-contracts-cli/tests/shapes_vacuity.rs

@@ -0,0 +1,305 @@
+//! #3610 — an armed shape that graded ZERO focus nodes must not be counted as clean.
+//!
+//! **The defect, measured on pv 0.68.2.** `NoFocus` asked the question GLOBALLY —
+//! `report.focus_nodes_n == 0` — which is only ever true for a lone contract. In a **directory**
+//! one empty shape is invisible: the other shapes carry the total above zero, the empty one
+//! contributes no violations, and the aggregate reports **`Pass` while listing it in
+//! `armed_shapes`**. A `type: jsonl` contract whose rows violate its own `sh:pattern` passed exactly
+//! that way, and `armed_shapes` is the tool's claim about what it measured.
+//!
+//! `by_shape` had carried `<shape>=0` all along. **Nothing read it** — the same *computed but
+//! unexposed* shape as #3597, this time inside the gate itself.
+//!
+//! **BOTH VENUES, because the lone-contract case already declined correctly and a fix proved only
+//! there proves nothing.** The directory case is the one that lied.
+//!
+//! | fixture | shape reach | armed? | expected |
+//! |---|---|---|---|
+//! | `shapes-one-empty` | `empty-shape=0`, `tool-status=1` | yes | **exit 2**, decline naming `empty-shape` |
+//! | `shapes-one-empty-unarmed` | same | no | **Pass**, `declines: ["empty-shape"]` |
+//! | `shapes-one-empty-and-violating` | `empty-shape=0`, `tool-status=1` (violating) | yes | **exit 1**, Fail, `declines: ["empty-shape"]` |
+//! | `json-ok` | `tool-status=1` | yes | Pass, `declines: []` |
+//! | `shapes-all-empty` | `empty-shape=0` (the only shape) | yes | **exit 2** via the global path, no report yet (#4100) |
+//!
+//! The second row is what keeps the fix from being "refuse whenever anything is empty": an UNARMED
+//! shape at zero did not affect the verdict, so it is **reported** rather than refused — and it is
+//! reported, not swallowed, which is the half the original defect got wrong.
+
+use std::path::{Path, PathBuf};
+use std::process::Command;
+
+fn pv_bin() -> PathBuf {
+    PathBuf::from(env!("CARGO_BIN_EXE_pv"))
+}
+
+fn repo_root() -> PathBuf {
+    Path::new(env!("CARGO_MANIFEST_DIR")).join("../..")
+}
+
+struct Run {
+    code: i32,
+    stdout: String,
+    stderr: String,
+}
+
+impl Run {
+    fn all(&self) -> String {
+        format!(
+            "exit {}\n--- stdout\n{}\n--- stderr\n{}",
+            self.code, self.stdout, self.stderr
+        )
+    }
+    fn extra(&self) -> serde_json::Value {
+        serde_json::from_str::<serde_json::Value>(&self.stdout)
+            .map(|v| v["extra"].clone())
+            .unwrap_or(serde_json::Value::Null)
+    }
+}
+
+fn shapes_on(fixture: &str) -> Run {
+    let contracts = repo_root()
+        .join("tests/fixtures/ont")
+        .join(fixture)
+        .join("contracts");
+    let out = Command::new(pv_bin())
+        .args([
+            "lint",
+            contracts.to_str().expect("utf-8 path"),
+            "--gate",
+            "shapes",
+            "--format",
+            "json",
+        ])
+        .output()
+        .expect("failed to spawn pv");
+    Run {
+        code: out.status.code().unwrap_or(-1),
+        stdout: String::from_utf8_lossy(&out.stdout).into_owned(),
+        stderr: String::from_utf8_lossy(&out.stderr).into_owned(),
+    }
+}
+
+#[test]
+fn an_armed_shape_that_graded_nothing_refuses_in_a_directory() {
+    // THE CASE THAT LIED. Before #3610 this was `Pass` with `empty-shape` in `armed_shapes`.
+    let r = shapes_on("shapes-one-empty");
+    assert_eq!(
+        r.code,
+        2,
+        "a vacuous armed shape must DECLINE, not pass and not fail\n{}",
+        r.all()
+    );
+    // THE REFUSAL IS THE EXIT CODE; THE REPORT IS THE EVIDENCE. Downstream consumers (infra's SLK
+    // gate) capture stdout and parse it REGARDLESS of exit code, because pv already exits non-zero
+    // on Fail. A refusal that printed only a bare `decline:` line would read to them as "no
+    // by_shape" and score UNMEASURED — indistinguishable, from their side, from a broken pv.
+    let extra = r.extra();
+    assert!(
+        !extra.is_null(),
+        "stdout must still parse as JSON on the exit-2 path\n{}",
+        r.all()
+    );
+    assert_eq!(
+        serde_json::from_str::<serde_json::Value>(&r.stdout).expect("json")["verdict"].as_str(),
+        Some("Unknown(NoFocus)"),
+        "the report must carry the declining verdict\n{}",
+        r.all()
+    );
+    assert!(
+        extra["by_shape"]
+            .as_array()
+            .expect("by_shape")
+            .iter()
+            .any(|s| s.as_str() == Some("empty-shape=0")),
+        "by_shape must NAME the zero-focus shape on the exit-2 path — it is the evidence\n{}",
+        r.all()
+    );
+    assert_eq!(
+        extra["declines"][0].as_str(),
+        Some("empty-shape"),
+        "the refusal must name the shape in declines\n{}",
+        r.all()
+    );
+    assert!(
+        !extra["armed_shapes"]
+            .as_array()
+            .expect("armed_shapes")
+            .iter()
+            .any(|s| s.as_str() == Some("empty-shape")),
+        "a shape that graded nothing has no place in armed_shapes\n{}",
+        r.all()
+    );
+    assert!(
+        !extra["not_armed_shapes"]
+            .as_array()
+            .expect("not_armed_shapes")
+            .iter()
+            .any(|s| s.as_str() == Some("empty-shape")),
+        "nor in not_armed_shapes: filing a vacuity as a policy choice is how it hid\n{}",
+        r.all()
+    );
+}
+
+#[test]
+fn an_unarmed_shape_that_graded_nothing_is_reported_not_refused() {
+    // The other direction, and the reason this is not "refuse whenever anything is empty".
+    let r = shapes_on("shapes-one-empty-unarmed");
+    assert_eq!(
+        r.code,
+        0,
+        "an UNARMED empty shape must not refuse\n{}",
+        r.all()
+    );
+    let extra = r.extra();
+    assert_eq!(
+        extra["declines"].as_array().map(|a| a.len()),
+        Some(1),
+        "the empty shape must be REPORTED in declines, not swallowed\n{}",
+        r.all()
+    );
+    assert_eq!(
+        extra["declines"][0].as_str(),
+        Some("empty-shape"),
+        "{}",
+        r.all()
+    );
+    assert!(
+        !extra["armed_shapes"]
+            .as_array()
+            .expect("armed_shapes")
+            .iter()
+            .any(|s| s.as_str() == Some("empty-shape")),
+        "a shape that graded nothing has no place in armed_shapes\n{}",
+        r.all()
+    );
+    // THE HALF THIS TEST WAS MISSING. The criterion is "neither list", and only
+    // the armed sibling above checked both — so an UNARMED vacuity could sit in
+    // `not_armed_shapes`, filed as a deliberate policy choice, which is exactly
+    // how the original defect hid. It did: the partition filtered on
+    // `vacuous_armed`, and two quorum lanes found it independently while this
+    // test stayed green.
+    assert!(
+        !extra["not_armed_shapes"]
+            .as_array()
+            .expect("not_armed_shapes")
+            .iter()
+            .any(|s| s.as_str() == Some("empty-shape")),
+        "an UNARMED shape that graded nothing must not be filed under \
+         not_armed_shapes either — `declines` is the only list it belongs in\n{}",
+        r.all()
+    );
+}
+
+/// A REAL VIOLATION OUTRANKS A VACUITY — found by all three lanes of the #3622 re-review.
+///
+/// `vacuous_armed` and `passed` are computed over DISJOINT shapes: one armed shape can grade
+/// nothing while another armed shape grades a node and finds it broken. The first cut checked
+/// vacuity first, so this corpus reported `Unknown(NoFocus)` / exit 2 — and a consumer that reads
+/// exit 2 as "unmeasured" never saw a Fail the gate had actually measured. The decline is still
+/// REPORTED in `declines`; it just may not hide the verdict.
+#[test]
+fn a_real_violation_is_a_fail_even_beside_a_vacuous_armed_shape() {
+    let r = shapes_on("shapes-one-empty-and-violating");
+    assert_eq!(
+        r.code,
+        1,
+        "a measured violation must FAIL, not decline, when another armed shape graded nothing\n{}",
+        r.all()
+    );
+    assert_eq!(
+        serde_json::from_str::<serde_json::Value>(&r.stdout).expect("json")["verdict"].as_str(),
+        Some("Fail"),
+        "{}",
+        r.all()
+    );
+    assert_eq!(
+        r.extra()["declines"][0].as_str(),
+        Some("empty-shape"),
+        "the vacuous shape must still be NAMED in declines on the Fail path\n{}",
+        r.all()
+    );
+}
+
+#[test]
+fn a_corpus_whose_shapes_all_graded_something_still_passes_with_an_empty_declines() {
+    // The control for the control: without this, a build that declined EVERYTHING would pass the
+    // two cases above.
+    let r = shapes_on("json-ok");
+    assert_eq!(r.code, 0, "{}", r.all());
+    assert_eq!(
+        r.extra()["declines"].as_array().map(Vec::len),
+        Some(0),
+        "nothing graded zero here, so declines must be empty\n{}",
+        r.all()
+    );
+}
+
+/// THE GLOBAL VENUE — every shape at zero, so `report.focus_nodes_n == 0` and `decline()` returns
+/// `ShapesOutcome::NoFocus` before the result path runs. It declined before #3610 and must still.
+///
+/// **Corrected by the #3622 re-review (all three lanes).** These two tests first ran
+/// `json-missing-ref`, which exits **3** on an unreadable `entity.ref` (`ont_extract_json.rs:128`)
+/// before `decline()` is consulted, so `code != 0` and "no report" both held for the wrong reason
+/// and neither test ever reached the path it named. `shapes-all-empty` reaches it: exit 2.
+#[test]
+fn a_corpus_whose_every_shape_graded_nothing_declines_through_the_global_path() {
+    let r = shapes_on("shapes-all-empty");
+    assert_eq!(
+        r.code,
+        2,
+        "every shape at zero must DECLINE (exit 2), not pass, fail, or error out\n{}",
+        r.all()
+    );
+    assert!(
+        r.stderr.contains("NoFocus"),
+        "the decline must be the NoFocus one, not some other refusal\n{}",
+        r.all()
+    );
+}
+
+/// THE ASYMMETRY, ASSERTED RATHER THAN LEFT UNSAID. The global path renders through
+/// `skipped_gate(...)` with no `extra`, so it declines with **no `by_shape` and no `declines`**,
+/// while the per-shape path (the one #3610 fixed) declines *with* the full report. The consumer that
+/// motivated the report requirement (infra's SLK gate) parses stdout whatever the exit code, so on
+/// this path it gets nothing. **Filed as #4100**, not fixed here: it needs the NoFocus outcome to
+/// carry the report, a change to the outcome path rather than to the verdict.
+#[test]
+fn the_global_nofocus_decline_prints_no_report_and_that_is_recorded() {
+    let r = shapes_on("shapes-all-empty");
+    assert_eq!(r.code, 2, "{}", r.all());
+    assert!(
+        r.extra().is_null(),
+        "if this venue has GAINED a report, #4100 is fixed — replace this test with an assertion of \
+         the report and add `shapes-all-empty` to the_three_answers_remain_distinct's loop\n{}",
+        r.all()
+    );
+}
+
+#[test]
+fn the_three_answers_remain_distinct() {
+    // pass / fail / decline must not collapse: a build that returned one code for everything
+    // would satisfy any single assertion above.
+    let pass = shapes_on("json-ok").code;
+    let fail = shapes_on("json-violation").code;
+    let decline = shapes_on("shapes-one-empty").code;
+    assert_eq!(
+        (pass, fail, decline),
+        (0, 1, 2),
+        "pass / fail / decline must differ"
+    );
+    // Every one of the three must also PRINT its report — a consumer that captures stdout and
+    // parses it regardless of exit code must get a document in all three cases, not two.
+    for fixture in ["json-ok", "json-violation", "shapes-one-empty"] {
+        let r = shapes_on(fixture);
+        assert!(
+            !r.extra().is_null(),
+            "{fixture} did not emit a parseable report\n{}",
+            r.all()
+        );
+        assert!(
+            r.extra()["by_shape"].is_array(),
+            "{fixture}: extra.by_shape must be present whatever the exit code\n{}",
+            r.all()
+        );
+    }
+}

crates/aprender-contracts/src/lint/mod.rs

@@ -144,6 +144,23 @@ pub enum GateDetail {
 /// it and none can `match` it. That is what makes it extensible where `GateDetail` is
 /// not, and it is `#[non_exhaustive]` from birth so the *next* post-0.3.1 gate does
 /// not have to repeat this exercise.
+// `Shapes` is 304 bytes against a 88-byte second-largest, and adding `declines` to it is what
+// crossed the threshold — `origin/main` at 237fbc32f lints clean, this branch does not, so the
+// finding is this PR's and not inherited.
+//
+// ALLOWED RATHER THAN BOXED, deliberately. `large_enum_variant` is a cost heuristic about copying
+// and stack size: a `GateExtra` is built ONCE PER GATE RUN, moved a handful of times, and then
+// serialised. There is no hot path here for 216 bytes to matter on, so the lint is measuring a cost
+// this type does not pay.
+//
+// The alternatives are worse. Boxing one field (clippy suggests `pc_extract`) reclaims 16 bytes and
+// does not clear the ratio, so it would be churn that silences nothing. Boxing the whole payload —
+// `Shapes(Box<ShapesExtra>)` — turns a struct variant into a newtype variant, which CHANGES THE
+// SERDE REPRESENTATION of a `#[serde(tag = "type")]` enum that downstream consumers parse; the SLK
+// gate reads this JSON. Breaking a wire format to satisfy a stack-size heuristic is the wrong trade.
+//
+// If `GateExtra` ever ends up in a loop or a large collection, this allow is the thing to revisit.
+#[allow(clippy::large_enum_variant)]
 #[derive(Debug, Clone, Serialize)]
 #[serde(tag = "type")]
 #[non_exhaustive]
@@ -221,6 +238,27 @@ pub enum GateExtra {
         armed_shapes: Vec<String>,
         /// Shapes computed and reported but not armed — their violations are in `unarmed_violations`.
         not_armed_shapes: Vec<String>,
+        /// #3610: shapes that graded ZERO focus nodes, named — the reach the gate did not have.
+        ///
+        /// A separate list from `not_armed_shapes` on purpose. "Not armed by policy" and "armed and
+        /// measured nothing" are different facts, and folding the second into the first would file a
+        /// vacuity as a deliberate choice — which is how the defect hid in the first place.
+        ///
+        /// **This carries EVERY shape that graded zero, armed or unarmed**, and it is the only list
+        /// any of them appears in: a vacuity is in neither `armed_shapes` (the tool's claim about
+        /// what it MEASURED) nor `not_armed_shapes` (a policy choice it never made).
+        ///
+        /// The two differ in what they do to the verdict, not in whether they are listed here. An
+        /// ARMED vacuity drives the verdict to `Unknown(NoFocus)` and the run declines — unless an
+        /// armed shape that DID grade something found a violation, in which case the verdict is
+        /// `Fail` (a measured violation outranks a vacuity, #3622). An UNARMED one leaves the
+        /// verdict alone, because it never fed it. Both are named, because a reader
+        /// needs to know the gate looked at nothing for them either way.
+        ///
+        /// An earlier draft of this comment said an armed vacuity "does not reach here at all",
+        /// which was false — nothing returns early at the verdict, and both kinds reach this field.
+        /// A quorum lane caught the sentence; the code beside it had the matching bug.
+        declines: Vec<String>,
         /// Violations from unarmed shapes (named in the findings as warnings; never in the meet).
         unarmed_violations: usize,
         /// Focus nodes each extractor produced: `pv-contract`, `gguf`, `apr-model`.