diff --git a/crates/atlas-cli/src/main.rs b/crates/atlas-cli/src/main.rs index b65372e..a02ca68 100644 --- a/crates/atlas-cli/src/main.rs +++ b/crates/atlas-cli/src/main.rs @@ -1060,8 +1060,15 @@ fn week_range(ts: i64) -> String { fn load_specs(root: &Path) -> Result> { let mut specs = Vec::new(); - let mut stack = vec![root.to_path_buf()]; - while let Some(dir) = stack.pop() { + // The flag is "this directory is inside a `specs/` tree". `SKIP_DIRS` is a + // list of build and vendor directory *names*, and it must not apply below + // that point: a spec module is named after the thing it governs, and + // `out`, `build`, `dist`, `target` and `coverage` are all ordinary module + // names. Applying the list there made `specs/out/out.spec.md` undiscoverable, + // so every file that spec governed was reported as an orphan and the + // project's coverage was understated by the whole module. + let mut stack = vec![(root.to_path_buf(), false)]; + while let Some((dir, in_specs)) = stack.pop() { let entries = match fs::read_dir(&dir) { Ok(e) => e, Err(_) => continue, @@ -1071,12 +1078,14 @@ fn load_specs(root: &Path) -> Result> { if path.is_dir() { let name = entry.file_name(); let name = name.to_string_lossy(); + if name.starts_with('.') { + continue; + } // `specs/` is skipped for source walking, but it is exactly // where specs live, so descend for spec discovery. - if name == "specs" - || (!SKIP_DIRS.contains(&name.as_ref()) && !name.starts_with('.')) - { - stack.push(path); + let descending = in_specs || name == "specs"; + if descending || !SKIP_DIRS.contains(&name.as_ref()) { + stack.push((path, descending)); } continue; } @@ -1131,6 +1140,18 @@ fn find_companions(root: &Path, spec_path: &Path) -> Vec { docs } +/// Sync verdicts `fledge spec check --json` may report for one spec. Matched as +/// whole values, never as substrings of the document. +const DRIFT_VERDICTS: &[&str] = &[ + "in_sync", + "in-sync", + "drifted", + "out_of_sync", + "stale", + "drift", + "ok", +]; + /// Best-effort drift enrichment via `fledge spec check --json`, only where a /// `.specsync/config.toml` exists. A no-op otherwise. fn enrich_drift(root: &Path, specs: &mut [Spec]) { @@ -1145,26 +1166,77 @@ fn enrich_drift(root: &Path, specs: &mut [Spec]) { Ok(o) => String::from_utf8_lossy(&o.stdout).into_owned(), Err(_) => return, }; + let Ok(report) = serde_json::from_str::(&out) else { + return; + }; for spec in specs.iter_mut() { - let needle = format!("\"{}\"", spec.module); - if let Some(pos) = out.find(&needle) { - let window = &out[pos..(pos + 240).min(out.len())]; - for verdict in [ - "in_sync", - "in-sync", - "drifted", - "out_of_sync", - "stale", - "drift", - "ok", - ] { - if window.contains(verdict) { - spec.drift = Some(verdict.replace('_', " ")); - break; - } + spec.drift = drift_for(&report, &spec.module); + } +} + +/// The sync verdict a `spec check` report carries for one module, or `None`. +/// +/// This reads the report as JSON. It used to scan the raw text: find the first +/// `""` anywhere in the document, take the next 240 bytes, and report +/// the first verdict word appearing in that window. Three things were wrong +/// with that. The name matched inside any string — another spec's `depends_on`, +/// a file path — so the window often described a different spec. The window ran +/// past the end of the entry, so the *last* spec in the list picked up whatever +/// top-level key came next: `"stale": []` reported every such spec as stale, +/// which is a review flag on a spec that is in sync. And `out[pos..pos + 240]` +/// panics outright when the cut lands inside a multi-byte character. +fn drift_for(report: &serde_json::Value, module: &str) -> Option { + // A module named in the top-level `stale` list is stale, whether the list + // holds bare names or objects carrying one. + if let Some(entries) = report.get("stale").and_then(|v| v.as_array()) { + if entries.iter().any(|entry| names_module(entry, module)) { + return Some("stale".into()); + } + } + // Otherwise, an explicit verdict on this module's own entry. + let entry = report + .get("specs") + .and_then(|v| v.as_array())? + .iter() + .find(|entry| names_module(entry, module))?; + for field in ["drift", "sync", "sync_status", "verdict"] { + let Some(value) = entry.get(field).and_then(|v| v.as_str()) else { + continue; + }; + if DRIFT_VERDICTS.contains(&value) { + return Some(value.replace('_', " ")); + } + } + None +} + +/// Whether a `stale` or `specs` entry is about this module. An entry is either +/// the bare name or an object carrying it under one of the keys spec-sync uses. +fn names_module(entry: &serde_json::Value, module: &str) -> bool { + if let Some(name) = entry.as_str() { + return name == module || spec_path_names(name, module); + } + for key in ["name", "module", "spec"] { + if entry.get(key).and_then(|v| v.as_str()) == Some(module) { + return true; + } + } + for key in ["path", "spec", "file"] { + if let Some(path) = entry.get(key).and_then(|v| v.as_str()) { + if spec_path_names(path, module) { + return true; } } } + false +} + +/// Whether a path is the spec document of this module: `.../.spec.md`, +/// compared whole so `out` never matches `checkout.spec.md`. +fn spec_path_names(path: &str, module: &str) -> bool { + path.rsplit(['/', '\\']) + .next() + .is_some_and(|file| file == format!("{module}.spec.md")) } // --------------------------------------------------------------------------- @@ -1759,6 +1831,143 @@ mod tests { let _ = fs::remove_dir_all(&dir); } + #[test] + fn load_specs_finds_a_module_named_after_a_build_directory() { + // `SKIP_DIRS` holds build and vendor directory names, and a spec module + // is named after what it governs. `specs/out/out.spec.md` was pruned by + // that list, so every file the spec governed was reported as an orphan. + let dir = tmp(); + for module in ["out", "build", "dist", "target", "coverage", "engine"] { + fs::create_dir_all(dir.join("specs").join(module)).unwrap(); + fs::write( + dir.join("specs") + .join(module) + .join(format!("{module}.spec.md")), + format!("---\nmodule: {module}\nfiles:\n - src/{module}.rs\n---\nbody"), + ) + .unwrap(); + } + let found: Vec = load_specs(&dir) + .unwrap() + .into_iter() + .map(|s| s.module) + .collect(); + assert_eq!( + found, + vec!["build", "coverage", "dist", "engine", "out", "target"] + ); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn load_specs_still_skips_a_build_tree_outside_specs() { + // The list is not disabled, only scoped: a `.spec.md` that a build + // copied into `out/` is still not a spec of this project. + let dir = tmp(); + fs::create_dir_all(dir.join("specs")).unwrap(); + fs::write( + dir.join("specs/real.spec.md"), + "---\nmodule: real\nfiles:\n - x.rs\n---\nbody", + ) + .unwrap(); + for skipped in ["out", "node_modules", "target"] { + fs::create_dir_all(dir.join(skipped)).unwrap(); + fs::write( + dir.join(skipped).join("copied.spec.md"), + "---\nmodule: copied\nfiles:\n - x.rs\n---\nbody", + ) + .unwrap(); + } + let found: Vec = load_specs(&dir) + .unwrap() + .into_iter() + .map(|s| s.module) + .collect(); + assert_eq!(found, vec!["real"]); + let _ = fs::remove_dir_all(&dir); + } + + #[test] + fn the_last_spec_in_a_report_is_not_made_stale_by_the_next_key() { + // The scrape took 240 bytes after the first `""` it found and + // reported the first verdict word in them. For the last entry in + // `specs`, those bytes are the next top-level key: `"stale": []` marked + // every such spec stale, and a spec-sync run reporting nothing wrong + // produced a needs-review flag. + let report = serde_json::json!({ + "specs": [ + {"name": "capture", "errors": [], "warnings": []}, + {"name": "workspace", "errors": [], "warnings": []} + ], + "stale": [], + "totals": {"checked": 2, "errors": 0, "warnings": 0} + }); + assert_eq!(drift_for(&report, "workspace"), None); + assert_eq!(drift_for(&report, "capture"), None); + } + + #[test] + fn a_module_named_in_the_stale_list_is_stale() { + let by_name = serde_json::json!({ "specs": [], "stale": ["workspace"] }); + assert_eq!(drift_for(&by_name, "workspace"), Some("stale".into())); + + let by_object = serde_json::json!({ + "specs": [], + "stale": [{"name": "workspace", "days": 12}] + }); + assert_eq!(drift_for(&by_object, "workspace"), Some("stale".into())); + + let by_path = serde_json::json!({ + "specs": [], + "stale": [{"path": "specs/workspace/workspace.spec.md"}] + }); + assert_eq!(drift_for(&by_path, "workspace"), Some("stale".into())); + } + + #[test] + fn an_explicit_verdict_on_the_module_s_own_entry_is_reported() { + let report = serde_json::json!({ + "specs": [ + {"name": "capture", "drift": "in_sync"}, + {"name": "workspace", "drift": "drifted"} + ], + "stale": [] + }); + assert_eq!(drift_for(&report, "workspace"), Some("drifted".into())); + assert_eq!(drift_for(&report, "capture"), Some("in sync".into())); + assert_eq!(drift_for(&report, "absent"), None); + } + + #[test] + fn a_module_name_appearing_in_another_spec_s_entry_is_not_its_verdict() { + // `out.find("\"out\"")` hit the `depends_on` of whichever spec came + // first, and described that spec instead. + let report = serde_json::json!({ + "specs": [ + {"name": "main", "depends_on": ["out"], "drift": "drifted"}, + {"name": "out", "drift": "in_sync"} + ], + "stale": [] + }); + assert_eq!(drift_for(&report, "out"), Some("in sync".into())); + } + + #[test] + fn a_report_carrying_multibyte_text_does_not_panic() { + // `&out[pos..pos + 240]` panics when the cut lands inside a character. + let report = serde_json::json!({ + "specs": [{"name": "café", "note": "un résumé — with an em dash, ünd more"}], + "stale": [] + }); + assert_eq!(drift_for(&report, "café"), None); + } + + #[test] + fn a_report_that_is_not_json_is_ignored_rather_than_guessed_at() { + let report = serde_json::json!("fledge: unknown command"); + assert_eq!(drift_for(&report, "workspace"), None); + } + // ---- integration: the full analysis + render pipeline on a fixture repo ---- /// A small but realistic project: three specs governing four code files, with diff --git a/specs/engine/engine.spec.md b/specs/engine/engine.spec.md index b5c7753..6bfbda3 100644 --- a/specs/engine/engine.spec.md +++ b/specs/engine/engine.spec.md @@ -127,7 +127,7 @@ feeds them (walking the tree, reading files and lcov, mining `git log`). | Function | Crate | Signature | Description | |----------|-------|-----------|-------------| -| `load_specs` | cli | `fn(&Path) -> Result>` | Walk the tree (descending into `specs/`, skipping build/vendor), parse every `*.spec.md` with `parse_spec_str`, attach companions, sorted by module. | +| `load_specs` | cli | `fn(&Path) -> Result>` | Walk the tree, parse every `*.spec.md` with `parse_spec_str`, attach companions, sorted by module. `SKIP_DIRS` prunes build and vendor trees **outside** `specs/` and does not apply below it: a spec module is named after what it governs, so `specs/out/`, `specs/build/` and `specs/target/` are ordinary modules and must be discoverable. | | `parse_spec_str` | core | `fn(&str, &str) -> Option` | Parse one spec from its relative path and text, rendering its prose to HTML at parse time. Pure. | | `load_sources` | cli | `fn(&Path, &IgnoreSet) -> Vec` | Walk the real source tree, count LOC per code file, skip `SKIP_DIRS`, generated/minified/vendored files, and any path the `IgnoreSet` scopes out. | | `parse` / `len` / `is_empty` / `matches` | core | `fn(&str) -> IgnoreSet`, `fn(&self) -> usize`, `fn(&self) -> bool`, `fn(&self, &str) -> bool` | The fully qualified methods are `IgnoreSet::parse`, `IgnoreSet::len`, `IgnoreSet::is_empty`, and `IgnoreSet::matches`. They parse an `.atlasignore` file (read from the project root by the CLI), inspect its pattern count or emptiness, and test a repo-relative path against it. Pattern forms, root-anchored: `dir/` (a directory and its contents), `*.ext` (an extension), or a bare `path` (that exact file, or a directory of that name). Scoped-out files leave the source set entirely, weighing on neither coverage nor orphans. | @@ -168,7 +168,16 @@ feeds them (walking the tree, reading files and lcov, mining `git log`). 4. Generated, minified, and vendored files are excluded from the source set via `looks_generated`, and directories in `SKIP_DIRS` (target, node_modules, .git, dist, specs, and so on) are never walked, so they cannot distort - coverage, the verdict, or the treemap. + coverage, the verdict, or the treemap. `SKIP_DIRS` is a list of directory + *names*, so it applies to the source walk and to spec discovery **above** + `specs/` only. Below `specs/` it must not apply: a module named after what it + governs is free to be called `out` or `build`, and pruning it there hides the + spec and reports every file it governs as an orphan. +4a. Drift is read from `fledge spec check --json` as JSON, matching a module by + its entry in `specs` or by name in `stale`. It is never scraped from the + report's text: a name matches inside any string, a fixed-width window past an + entry picks up the next top-level key, and slicing a byte offset out of a + document with multi-byte characters panics. 5. `--json` and the HTML atlas derive from the same `Model`; `render_html` embeds the exact `Model` JSON that `--json` prints, so the picture and the data never disagree. @@ -304,3 +313,4 @@ Then every file under Tests/ and the Package.swift manifest leave the source | 5 | 2026-07-03 | Added two more `--svg` components: `sunburst` (the directory tree as coverage rings, tinted clay-to-teal, with the overall percentage in the center) and `calendar` (a GitHub-style commit-activity grid colored spec/code/both), rounding out the deterministic, browser-free component set. | | 6 | 2026-07-03 | Added `IgnoreSet` and `.atlasignore` support: a project can scope the coverage denominator (test trees, generated output, a marketing site) with a small root-anchored ignore file, so the percentage reflects the code its specs are actually meant to govern. `load_sources` now takes an `&IgnoreSet`. | | 7 | 2026-07-12 | Add a flat source-audited inventory of every public `atlas-core` export so the detailed grouped API below is machine-checkable without changing the engine contract. | +| 8 | 2026-09-19 | Two discovery defects. `load_specs` applied `SKIP_DIRS` below the `specs/` root, so a module named after a build directory was undiscoverable and every file it governed was counted as an orphan (found on a project whose `specs/out/out.spec.md` was invisible, understating its coverage by 1,252 lines). And `enrich_drift` scraped verdicts out of the report's raw text with a 240-byte window, so the last spec in `specs` was marked stale by the following `"stale": []` key, a module name matched inside another spec's fields, and a multi-byte cut would have panicked; it now parses the JSON. |