fix: review follow-ups for #5 and the Swift lexical preview #8
2 changed files with 54 additions and 5 deletions
fix(check): warn, don't fail, when stored metrics differ from recomputed
`check` treated any difference between stored and recomputed metrics as a gate failure, so a model written by an older brick (without the `metrics.resolution` field, or with an older formula) always failed, even with every threshold met. CI checking an archived or cached model broke on upgrade. The gate already uses recomputed metrics, so a mismatch is now a stderr warning. Only fields present in both copies are compared, so a missing newer field does not count as a mismatch. Tampered metrics still cannot pass: the existing test's recomputed cycle count fails the gate, and it still reports "stored metrics". Test: a freshly built model with `metrics.resolution` removed passes `check --min-score 0`; it failed before this change.
commit
d0d52edfd1
30
src/main.rs
30
src/main.rs
|
|
@ -244,11 +244,15 @@ fn run(cli: Cli) -> Result<()> {
|
|||
let mut recomputed = building.clone();
|
||||
brick::metrics::calculate(&mut recomputed);
|
||||
let mut failures = Vec::new();
|
||||
if serde_json::to_value(&recomputed.metrics)?
|
||||
!= serde_json::to_value(&building.metrics)?
|
||||
{
|
||||
failures.push(
|
||||
"stored metrics do not match the graph; gating on recomputed metrics".into(),
|
||||
// Compare only fields both copies carry: a model from an older build lacks
|
||||
// newer fields (`metrics.resolution`) and may use an older formula. That is
|
||||
// worth a warning, never a failure; the gate below uses recomputed values.
|
||||
if stored_metrics_differ(
|
||||
&serde_json::to_value(&building.metrics)?,
|
||||
&serde_json::to_value(&recomputed.metrics)?,
|
||||
) {
|
||||
eprintln!(
|
||||
"brick: warning: stored metrics do not match the graph; gating on recomputed metrics"
|
||||
);
|
||||
}
|
||||
let metrics = &recomputed.metrics;
|
||||
|
|
@ -430,6 +434,22 @@ fn write_json(building: &Building, path: &PathBuf) -> Result<()> {
|
|||
.with_context(|| format!("cannot write graph {}", path.display()))
|
||||
}
|
||||
|
||||
/// True when a stored metrics value disagrees with the recomputed one on a field
|
||||
/// both carry. Fields missing from the stored copy (written by an older build)
|
||||
/// are not compared.
|
||||
fn stored_metrics_differ(stored: &serde_json::Value, recomputed: &serde_json::Value) -> bool {
|
||||
match (stored, recomputed) {
|
||||
(serde_json::Value::Object(stored), serde_json::Value::Object(recomputed)) => {
|
||||
stored.iter().any(|(key, value)| {
|
||||
recomputed
|
||||
.get(key)
|
||||
.is_some_and(|fresh| stored_metrics_differ(value, fresh))
|
||||
})
|
||||
}
|
||||
_ => stored != recomputed,
|
||||
}
|
||||
}
|
||||
|
||||
fn read_json(path: &PathBuf) -> Result<Building> {
|
||||
let raw = fs::read_to_string(path)
|
||||
.with_context(|| format!("cannot read graph {}", path.display()))?;
|
||||
|
|
|
|||
|
|
@ -306,6 +306,35 @@ fn check_recomputes_metrics_instead_of_trusting_the_file() {
|
|||
assert!(text.contains("stored metrics"), "{text}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_model_from_an_older_build_passes_check_on_recomputed_metrics() {
|
||||
// Any difference between stored and recomputed metrics failed the gate, so a
|
||||
// model written before `metrics.resolution` existed always failed, even with
|
||||
// every threshold met. Gate on the recomputed values; a mismatch is a warning.
|
||||
let directory = tempfile::tempdir().unwrap();
|
||||
let project = directory.path().join("project");
|
||||
fs::create_dir(&project).unwrap();
|
||||
fs::write(project.join("lib.rs"), "pub fn alpha() -> u32 { 1 }\n").unwrap();
|
||||
let model = scan::scan(&project).unwrap();
|
||||
let mut value: Value = serde_json::to_value(&model).unwrap();
|
||||
value["metrics"]
|
||||
.as_object_mut()
|
||||
.unwrap()
|
||||
.remove("resolution");
|
||||
let older = directory.path().join("older.json");
|
||||
fs::write(&older, serde_json::to_vec_pretty(&value).unwrap()).unwrap();
|
||||
let output = Command::new(env!("CARGO_BIN_EXE_brick"))
|
||||
.args(["check", older.to_str().unwrap(), "--min-score", "0"])
|
||||
.output()
|
||||
.unwrap();
|
||||
assert!(
|
||||
output.status.success(),
|
||||
"{}{}",
|
||||
String::from_utf8_lossy(&output.stdout),
|
||||
String::from_utf8_lossy(&output.stderr)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_scan_with_no_supported_source_is_reported_and_fails_check() {
|
||||
// A Go/Java-only directory built "1 nodes / 0 edges; score 100.0" with no
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue