Skip to content

Commit 30adbc9

Browse files
authored
fix: refuse a vacuous pass in three more gates (#3200 findings 4, 5, 6) (#3214)
* test(processing): refuse a vacuous pass in the module-size ratchet (#3200) Finding 5 of the vacuity audit, reproduced by running it rather than by reading: pointed at an empty directory, and again at one that does not exist, `no_module_grows_past_its_ratchet_budget` reported success over zero files both times. Every offender it can report is pushed inside `for (rel, lines) in files`, so an empty walk gives an empty message gives a green test. Three guards now stand in the way: - `collect_rs_files` no longer turns a failed `read_dir` into an empty result. A missing scan root and an unreadable one are both hard errors, and they are distinguished: the first means the walk roots are wrong, the second means the environment is. A per-entry read error is loud too, where `entries.flatten()` used to drop it. - `line_count` no longer reports an unreadable file as 0 lines. 0 is under LIMIT and under every allowlist budget, so a file this gate could not open was indistinguishable from one that passes it. - `FILE_FLOOR`, a measured lower bound on the walk. The healthy tree reaches 362 non-exempt `.rs` files under `rust/` + `apps/`; the floor is 240, roughly two thirds. The number only has to separate "the walk works" from "the walk went blind", and every way it goes blind takes it to zero or to a handful, never to a plausible fraction. Watched fail before being trusted: empty directory -> the floor message; missing directory -> the missing-root message; a `.rs` file that cannot be read -> the line-count message. Real tree still green at 362 files. Refs #3200 * test(processing): refuse a vacuous pass in the styling parity guards (#3200) Finding 6 of the vacuity audit, reproduced by running it rather than by reading: pointed at an empty directory, and again at one that does not exist, both `no_duplicate_default_color_tables` and `no_duplicate_surface_style_color_extraction` reported success over zero files, with not even a note on stderr. Both conclude from an ABSENCE, so zero files scanned gives zero offenders gives green. Four guards now stand in the way: - `collect_rs_files` no longer turns a failed `read_dir` into an empty result. Missing and unreadable scan roots are both hard errors, told apart because they call for different fixes; a per-entry read error is loud too, where `entries.flatten()` used to drop it. - a walked file that cannot be read is a hard error, not a `continue`. A forbidden declaration sitting in an unreadable file used to read as "not found", which is the one answer these guards must never give by accident. - `SCANNED_FLOOR`, a measured lower bound. The healthy tree reaches 659 `.rs` files under `rust/` + `apps/`; the floor is 440, roughly two thirds, for the same reason as the module-size ratchet's. - positive controls, because a file count shows the walk is alive and nothing about whether the detector still detects. Both detectors are extracted into named functions and exercised against known-positive and known-negative inputs, and the surface-style guard additionally asserts end-to-end that it still matches the one occurrence it is allowed to find, in `rust/geometry/examples/`. Watched fail before being trusted: empty directory and missing directory -> the floor and missing-root messages; a detector stubbed to return false -> the positive control, "matched nothing at all across 659 walked file(s)". Real tree still green. Refs #3200 * fix(scripts): refuse an empty schema registry in the attr-index generator (#3200) Finding 4 of the vacuity audit. `generate-server-attr-indices.mjs` had no floor on `rows.length`, unlike its sibling `generate-bim-globals.mjs` which refuses the identical condition in so many words. Reproduced with a `packages/parser/dist` that imports cleanly and exports `SCHEMA_REGISTRY = { name: 'EMPTY', entities: {} }`: $ node scripts/generate-server-attr-indices.mjs --check ✗ ... is out of sync ... stale row (not in registry): IFCACTIONREQUEST EXIT=1 $ node scripts/generate-server-attr-indices.mjs # its own remedy wrote .../attr_indices.rs (0 types, registry EMPTY) $ node scripts/generate-server-attr-indices.mjs --check ✓ attr_indices.rs in sync (0 types, registry EMPTY) EXIT=0 Correct verdict, ruinous advice: the emitted `match` has no arms at all, so `root_attr_indices` returns `None` for every type and — per this file's own header — every KNOWN type falls back to the unknown-type indices [3,4,7]. That is the parity break the generator exists to prevent, and `--check` prints ✓ from then on. Following the in-repo precedent rather than inventing one: the registry is checked before EITHER mode proceeds, the way the sandbox generator already refuses its empty schema. Behind it, a measured floor — `ROW_FLOOR = 500` against the 776 types a healthy IFC4_ADD2_TC1 build yields, headroom deep enough to survive a smaller schema (IFC2X3 has roughly 650) while still catching what it is for, since every way this extraction goes blind takes the count to zero, not to 499. Same synthetic tree after the fix: both modes exit 1 naming the count, the floor and what they expected, and the committed 776-arm table is left untouched. Real repo still `✓ attr_indices.rs in sync (776 types, registry IFC4_ADD2_TC1)`. Refs #3200 * fix(scripts): count RESOLVED attributes, not rows, in the attr-index floor (#3200) ROW_FLOOR counted rows, so the bug it was written for survived one level down. `allAttributes` is optional on the schema registry's entity metadata and `getAllAttributesForEntity` returns `metadata?.allAttributes || []`, so a registry whose `entities` map is fully populated while `allAttributes` stops being emitted yields 776 rows every one of which is [-1,-1,-1,-1]. Measured on exactly that shape: the generator wrote all 776 arms and exited 0, and `--check` printed "in sync" on the next run. Per the generated file's own contract -1 means "known type, does not declare that attribute, never fall back" — so the server-parse path would report Description / ObjectType / Tag / PredefinedType absent for every entity while the browser resolves them normally. RESOLVED_FLOOR = 400 closes that: 488 of today's 776 rows resolve at least one of the four (the other 288 legitimately declare none), so the floor costs nothing today and cannot be cleared by a table that resolves nothing. Also raises ROW_FLOOR from 500 to 700 and rewrites its comment, which justified the headroom with something that is not true: there is no runtime schema selection here. SCHEMA_REGISTRY is a single committed codegen artifact pinned to IFC4_ADD2_TC1; the multi-schema union lives in ifc-schema.ts's ENTITY_INFO_BY_UPPER behind getAttributeNamesAcrossSchemas, which this generator never calls. Repinning would be a regenerate-and-commit event, which the failure message already instructs. Measured on the old value: 499 rows refused and 500 rows WROTE, replacing all 776 committed arms and exiting 0 — that band bought nothing. Adds scripts/generate-server-attr-indices.test.mjs, a black-box harness in the established scripts/*.test.mjs shape (picked up by the glob catch-all step in test.yml). It pins both floors' boundaries, both refusal modes, and that a refusal leaves the committed table byte-identical. It fails 4 of 6 against the pre-fix generator. * fix(scripts): correct three overclaims and pin ROW_FLOOR byte-identity (#3214 followups) Adversarial review of #3214 (sound, ship it) found accuracy gaps in the comments and one coverage gap in the test: - RESOLVED_FLOOR's comment claimed every blind-extraction failure takes the resolved count to zero, never partway. True for the generator-level failure (typescript-generator.ts emits allAttributes unconditionally, so that failure is all-or-nothing) but false for the inheritance-walk failure in express-parser.ts's getAllAttributes, which silently stops at a missing supertype instead of erroring. Measured: 201 of 488 resolved entities resolve ONLY via inherited attributes; the costliest single ancestor is IfcRoot at 70 rows (IfcRelationship 47, IfcObject 31, IfcTypeProduct 21, IfcElement 20) — all comfortably inside the floor's slack, so a partial loss there is reachable and RESOLVED_FLOOR alone would wave it through. --check's drift comparison is the real backstop for this failure mode. - "comfortably under the 444" understated the real worst case: a 700-row subset that keeps all 288 non-resolvers costs 412, not 444, so the true margin over RESOLVED_FLOOR=400 is 12 rows (2.9%), not 44. The floors still don't contradict. - "reachable only through getAttributeNamesAcrossSchemas" was imprecise: ENTITY_INFO_BY_UPPER is also read by getEntityInfoAcrossSchemas and getInheritanceChainFromSchemaUnion. Also seeds a committed table in the ROW_FLOOR test and asserts its hash survives the refusal, closing a gap where a half-writing ROW_FLOOR mutant could clobber a seeded table with the whole suite green. And adds a case for the empty-registry guard's distinct value (a raw missing/non-object entities key, not just the {} shape ROW_FLOOR already catches). * fix(tests): refuse the no-repo-root skip under CI, the way around all the #3200 guards The three source-walking gates hardened by #3200 each open with let Some(root) = repo_root() else { eprintln!("packaged context"); return; }; which returns BEFORE the walk. That single early return bypasses the missing-root panic, the unreadable-file panic and the scan floor at once -- every guard the hardening added, skipped by the one path it did not cover. Reproduced with `repo_root` stubbed to `None`, `CI=true`, at 907f115: test no_module_grows_past_its_ratchet_budget ... ok test no_duplicate_default_color_tables ... ok test no_duplicate_surface_style_color_extraction ... ok test result: ok. 5 passed ... finished in 0.00s test result: ok. 6 passed ... finished in 0.00s Three green gates over a tree never opened, in zero seconds. It cannot simply become a panic. `tests/` is packaged into the published `.crate` (no `exclude` in `rust/processing/Cargo.toml`), so a downstream `cargo test` on `ifc-lite-processing` runs these files with no `rust/` or `apps/` above them, and skipping is correct there. The two cases cannot be told apart by looking for the repo -- its absence is the thing being explained. `CI` is the discriminator, using the truthiness form already in `rust/geometry/tests/triangulation_invariance.rs` rather than a second convention for the same variable. One implementation in `tests/common/mod.rs`, not a copy per binary: two copies of a gate's escape hatch are two chances to loosen one and not the other. RED under the stub, verbatim: thread 'no_module_grows_past_its_ratchet_budget' panicked at rust/processing/tests/common/mod.rs:36:5: module-size ratchet: no repo root above CARGO_MANIFEST_DIR (...), but CI is set. Under CI this gate must scan the tree, not skip it ... GREEN on the real tree with CI=true: 5 passed / 6 passed, 0.19s and 0.86s -- non-zero, because the walk now happens. The packaged path still skips: with the stub and CI unset, 5 passed / 6 passed. clippy --tests clean. Refs #3200
1 parent 006c7b3 commit 30adbc9

5 files changed

Lines changed: 693 additions & 31 deletions

File tree

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// This Source Code Form is subject to the terms of the Mozilla Public
2+
// License, v. 2.0. If a copy of the MPL was not distributed with this
3+
// file, You can obtain one at https://mozilla.org/MPL/2.0/.
4+
5+
//! Shared helper for the source-walking CI gates in this crate
6+
//! (`module_size_ratchet`, `styling_parity`).
7+
//!
8+
//! Lives here rather than being copied into each test binary so the rule has
9+
//! ONE implementation: two copies of a gate's escape hatch are two chances to
10+
//! loosen one of them and not the other.
11+
12+
/// Refuse to skip a source-walking gate when running under CI.
13+
///
14+
/// Both gates begin `let Some(root) = repo_root() else { eprintln!(…); return; }`.
15+
/// That early return is the widest hole the #3200 hardening left open: it
16+
/// happens BEFORE the walk, so it bypasses the missing-root panic, the
17+
/// unreadable-file panic and the scan floor all at once. Reproduced with
18+
/// `repo_root` stubbed to `None` — `no_module_grows_past_its_ratchet_budget`,
19+
/// `no_duplicate_default_color_tables` and
20+
/// `no_duplicate_surface_style_color_extraction` each report
21+
/// `ok … finished in 0.00s`: three green gates over a tree never opened.
22+
///
23+
/// It cannot simply become a panic. `tests/` is packaged into the published
24+
/// `.crate` (there is no `exclude` in `rust/processing/Cargo.toml`), so a
25+
/// downstream `cargo test` on `ifc-lite-processing` runs these files with no
26+
/// `rust/` or `apps/` above them, and skipping is correct there. Nor can the
27+
/// two situations be told apart by looking for the repo — its absence is the
28+
/// thing being explained.
29+
///
30+
/// `CI` is the discriminator: GitHub Actions sets it on every run, and a
31+
/// packaged consumer's machine does not have it. The truthiness test follows
32+
/// the form already used by `rust/geometry/tests/triangulation_invariance.rs`
33+
/// rather than inventing a second convention for the same variable.
34+
pub fn refuse_to_skip_in_ci(gate: &str) {
35+
let ci = std::env::var_os("CI").is_some_and(|v| !v.is_empty() && v != "0" && v != "false");
36+
assert!(
37+
!ci,
38+
"{gate}: no repo root above CARGO_MANIFEST_DIR ({}), but CI is set. Under CI \
39+
this gate must scan the tree, not skip it — skipping bypasses the \
40+
missing-root panic, the unreadable-file panic AND the scan floor at once, \
41+
and reports success over a tree that was never opened (#3200). Either the \
42+
checkout is incomplete or the walk roots are wrong; both block a release, \
43+
and neither of them means 'no offenders'.",
44+
env!("CARGO_MANIFEST_DIR")
45+
);
46+
}

rust/processing/tests/module_size_ratchet.rs

Lines changed: 89 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,19 @@
2020
//!
2121
//! This runs in the required `rust-tests` lane (`cargo test --workspace`), so a
2222
//! violation blocks merge. Cross-crate file walking mirrors `styling_parity`.
23+
//!
24+
//! ANTI-VACUITY (#3200): every offender this gate reports is produced by
25+
//! iterating the walked file list, so an empty walk produced an empty message
26+
//! and a green test - success reported over a tree that was never opened. Four
27+
//! guards now stand between an empty walk and that pass: a missing or unreadable
28+
//! scan root is a hard error instead of an empty result (and the two are told
29+
//! apart, because they call for different fixes), a file that cannot be read is
30+
//! a hard error instead of 0 lines, the walk must reach at least `FILE_FLOOR`
31+
//! non-exempt files before any verdict below it counts, and under CI the
32+
//! no-repo-root skip is refused outright (`common::refuse_to_skip_in_ci`) - that
33+
//! skip returns before the walk, so it bypassed all three of the others at once.
34+
35+
mod common;
2336

2437
const LIMIT: usize = 400;
2538
const ALLOWLIST: &str = include_str!("module_size_allowlist.txt");
@@ -43,6 +56,24 @@ const ALLOWLIST: &str = include_str!("module_size_allowlist.txt");
4356
/// would rewrite the digest and fail CI for no reason.
4457
const ALLOWLIST_DIGEST: u64 = 2635552733029908705;
4558

59+
/// Lower bound on how many non-exempt `.rs` files the walk must reach before
60+
/// its verdict means anything. Every offender this gate can report is pushed
61+
/// inside `for (rel, lines) in files`, so an empty `files` produces an empty
62+
/// message and a green test - a pass over a region the gate never examined
63+
/// (#3200).
64+
///
65+
/// MEASURED, not guessed: the walk over `rust/` + `apps/` reaches 362
66+
/// non-exempt files on a healthy tree (raise the floor and run the test to see
67+
/// the real figure in the failure message). The floor below sits at roughly two
68+
/// thirds of that, which is the right shape of
69+
/// headroom here: this number only has to separate "the walk works" from "the
70+
/// walk went blind", and every way it can go blind - a wrong scan root, a
71+
/// `read_dir` that fails, a crate tree that moved - takes the count to zero or
72+
/// to a handful, never to a plausible-looking fraction. Deleting a whole crate
73+
/// is the only ordinary event that would approach it, and that is a change
74+
/// worth editing this line for.
75+
const FILE_FLOOR: usize = 240;
76+
4677
/// Repo root = first ancestor holding both `rust/` and `apps/`. `None` in a
4778
/// packaged/standalone context (the test then skips, like `styling_parity`).
4879
fn repo_root() -> Option<std::path::PathBuf> {
@@ -57,11 +88,39 @@ fn repo_root() -> Option<std::path::PathBuf> {
5788
}
5889
}
5990

91+
/// Walk `dir`, collecting every `.rs` file underneath it.
92+
///
93+
/// A directory this cannot list is a hard error, and the two reasons are told
94+
/// apart. The previous `let Ok(entries) = read_dir(dir) else { return; };`
95+
/// collapsed "the scan root is not there" and "the scan root cannot be opened"
96+
/// into the same result as "this directory holds no Rust files" - which is how
97+
/// this ratchet could report success over a tree it never opened (#3200). A
98+
/// missing directory means the walk roots are wrong; an unreadable one means
99+
/// the environment is. Neither means there are no offenders.
60100
fn collect_rs_files(dir: &std::path::Path, out: &mut Vec<std::path::PathBuf>) {
61-
let Ok(entries) = std::fs::read_dir(dir) else {
62-
return;
101+
let entries = match std::fs::read_dir(dir) {
102+
Ok(entries) => entries,
103+
Err(err) if err.kind() == std::io::ErrorKind::NotFound => panic!(
104+
"module-size ratchet: {} does not exist. Refusing to treat a missing \
105+
directory as one holding no .rs files - a walk root that is not \
106+
there means this gate is looking in the wrong place, not that the \
107+
tree is clean.",
108+
dir.display()
109+
),
110+
Err(err) => panic!(
111+
"module-size ratchet: {} could not be read ({err}). Refusing to treat \
112+
an unreadable directory as one holding no .rs files.",
113+
dir.display()
114+
),
63115
};
64-
for entry in entries.flatten() {
116+
for entry in entries {
117+
let entry = entry.unwrap_or_else(|err| {
118+
panic!(
119+
"module-size ratchet: an entry of {} could not be read ({err}). \
120+
Refusing to walk past a file this gate could not classify.",
121+
dir.display()
122+
)
123+
});
65124
let path = entry.path();
66125
if path.is_dir() {
67126
let skip = matches!(
@@ -114,10 +173,19 @@ fn parse_allowlist() -> std::collections::HashMap<String, usize> {
114173
map
115174
}
116175

176+
/// Line count for one file. An unreadable file is a hard error, not zero: 0 is
177+
/// under LIMIT and under every allowlist budget, so `unwrap_or(0)` made a file
178+
/// this gate could not open indistinguishable from a file that passes it.
117179
fn line_count(path: &std::path::Path) -> usize {
118-
std::fs::read_to_string(path)
119-
.map(|s| s.lines().count())
120-
.unwrap_or(0)
180+
match std::fs::read_to_string(path) {
181+
Ok(s) => s.lines().count(),
182+
Err(err) => panic!(
183+
"module-size ratchet: {} could not be read ({err}). Refusing to count \
184+
an unreadable file as 0 lines - 0 is under every budget, so the file \
185+
would pass the ratchet without ever being measured.",
186+
path.display()
187+
),
188+
}
121189
}
122190

123191
/// Pure ratchet decision: given `(relpath, line_count)` for every non-exempt
@@ -148,6 +216,7 @@ fn evaluate(
148216
#[test]
149217
fn no_module_grows_past_its_ratchet_budget() {
150218
let Some(root) = repo_root() else {
219+
common::refuse_to_skip_in_ci("module-size ratchet");
151220
eprintln!("repo root not found (packaged context) - skipping module-size ratchet");
152221
return;
153222
};
@@ -169,6 +238,20 @@ fn no_module_grows_past_its_ratchet_budget() {
169238
.filter(|(rel, _)| !is_exempt(rel))
170239
.collect();
171240

241+
// Anti-vacuity (#3200). Placed above every verdict below, because all of
242+
// them are computed by iterating `files`: zero files gives zero offenders
243+
// gives a green test, which is this gate reporting success over a tree it
244+
// never looked at.
245+
assert!(
246+
files.len() >= FILE_FLOOR,
247+
"module-size ratchet walked rust/ and apps/ and reached only {} non-exempt \
248+
.rs file(s); the floor is {FILE_FLOOR}. Refusing a vacuous pass: every \
249+
check below iterates this list, so a count this low means the walk \
250+
stopped working, not that the modules went away. If crates were \
251+
genuinely removed, lower FILE_FLOOR in the same commit.",
252+
files.len()
253+
);
254+
172255
// Advisory only (never fails the build, to avoid merge-order coupling): an
173256
// allowlisted file that dropped to <= LIMIT or vanished should have its row
174257
// removed so the list keeps trending down.

0 commit comments

Comments
 (0)