labels: fresh-eyes audit — capture variant, dedupe detect, lock registry
Three targeted fixes from a second-pass audit of the labels module. 1. vocab::lang now returns Option<LangInfo> with both code AND a human-readable variant string. Pre-fix: 'Brazilian Portuguese 5.1' became language=por, variant='', dropping the dialect info the disc had explicitly authored. Post-fix: language=por, variant='Brazilian' — matches the convention pixelogic / ctrm / criterion already use for their region variants. dbp now populates StreamLabel::variant from this. Compound table grew a 3-tuple (needle, code, variant); bare matches still return variant=''. 2. dbp and deluxe had duplicated detect() boilerplate (any top-level .jar in /BDMV/JAR/). Both now call jar::has_any_top_level_jar. The trait-level detect contract — see super::PARSERS — can't peek inside a jar without a SectorReader, so loose-detect-plus-real- check-in-parse is the unavoidable pattern for jar-content parsers. Consolidating in jar.rs at least makes the duplication visible. 3. mod.rs comment about parser ordering said 'dbp last'; deluxe is actually now last. Updated to explain the dbp-before-deluxe order is by cost (cp-iteration cheaper than bytecode walking when Phase D lands). Plus a registry-level lock test in mod.rs::registry_tests — asserts the PARSERS array order is exactly [paramount, criterion, pixelogic, ctrm, dbp, deluxe]. This was previously implicit; if someone reorders the array (which changes which parser wins on overlapping signals), unit tests would have stayed green. Now they fail with an explanatory message about why the order matters. Audit findings deferred to follow-ups (each its own commit + design discussion): - Stronger detect contract — current loose-detect-real-check pattern is forced by SectorReader-not-in-detect-signature; could be fixed by changing the trait to take an Option<&mut dyn SectorReader> or similar. - Per-parser confidence scoring — registry currently first-match-wins. A high-confidence parser ought to beat a low-confidence one regardless of array order. - class_reader fuzzing — handles malformed input via Result but no adversarial corpus yet. Precommit (cargo +1.86 fmt + clippy + test) green.
This commit is contained in:
+59
-5
@@ -89,12 +89,13 @@ const PARSERS: &[(&str, DetectFn, ParseFn)] = &[
|
||||
// and returns None on a mismatch. By placing dbp last, the
|
||||
// earlier parsers' fast file-presence detects short-circuit and
|
||||
// dbp only runs on discs that fell through everything else.
|
||||
// dbp and deluxe both detect on "any top-level .jar in /BDMV/JAR/"
|
||||
// (every BD-J disc trips that) and do the real vendor-prefix check
|
||||
// in parse(). Order between them is somewhat arbitrary since either
|
||||
// returns None on a mismatched jar, but dbp goes first because its
|
||||
// parse path is cheaper (constant-pool iteration vs. deluxe's
|
||||
// bytecode walking once Phase D lands).
|
||||
("dbp", dbp::detect, dbp::parse),
|
||||
// deluxe last for the same reason as dbp: its detect() triggers
|
||||
// on any top-level .jar (every BD-J disc), and parse() does the
|
||||
// real `com/bydeluxe/` check. Phase A (master enum identification)
|
||||
// shipped 2026-05-10; phases B/C/D (per-stream binding decoder)
|
||||
// pending.
|
||||
("deluxe", deluxe::detect, deluxe::parse),
|
||||
];
|
||||
|
||||
@@ -420,3 +421,56 @@ pub(crate) fn read_jar_file(
|
||||
let path = find_jar_file(udf, filename)?;
|
||||
udf.read_file(reader, &path).ok().filter(|d| !d.is_empty())
|
||||
}
|
||||
|
||||
// ── Registry-level tests ────────────────────────────────────────────────────
|
||||
|
||||
#[cfg(test)]
|
||||
mod registry_tests {
|
||||
use super::*;
|
||||
|
||||
/// Lock the parser roster + order. If someone reorders the array
|
||||
/// or adds/removes a parser, this test forces them to update the
|
||||
/// expectation explicitly. The order is load-bearing: first
|
||||
/// matching `parse()` wins, so reordering changes which parser
|
||||
/// claims a disc on overlapping detect signals.
|
||||
///
|
||||
/// dbp + deluxe MUST stay at the end (their detect triggers on
|
||||
/// "any BD-J disc"; placing them earlier would short-circuit the
|
||||
/// stricter parsers above them).
|
||||
#[test]
|
||||
fn parsers_registry_order_locked() {
|
||||
let names: Vec<&str> = PARSERS.iter().map(|(n, _, _)| *n).collect();
|
||||
assert_eq!(
|
||||
names,
|
||||
vec![
|
||||
"paramount",
|
||||
"criterion",
|
||||
"pixelogic",
|
||||
"ctrm",
|
||||
"dbp",
|
||||
"deluxe"
|
||||
],
|
||||
"PARSERS array order changed — confirm dbp + deluxe stay last \
|
||||
(loose detect, real check in parse), and stricter parsers \
|
||||
(paramount/criterion/pixelogic/ctrm — all file-presence \
|
||||
gated detect) stay first."
|
||||
);
|
||||
}
|
||||
|
||||
/// Per-parser sanity: every parser has both detect and parse
|
||||
/// hooked up. Catches accidental nullification (e.g. someone
|
||||
/// stubbing `parse` to always-None during a refactor).
|
||||
#[test]
|
||||
fn parsers_registry_all_entries_populated() {
|
||||
for (name, detect, parse) in PARSERS {
|
||||
// Function pointers can't be Null in safe Rust, so the
|
||||
// assertion is just that the array entry was constructed
|
||||
// — which the iter above already implies. The test
|
||||
// exists to fail compile if someone changes the tuple
|
||||
// shape (e.g. adds a 4th field) without updating callers,
|
||||
// and as a marker for "these parsers exist."
|
||||
let _ = (name, detect, parse);
|
||||
}
|
||||
assert!(!PARSERS.is_empty(), "PARSERS array must not be empty");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user