Skip to content

Fix: two spec-discovery defects that understate coverage and invent drift #19

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
0xLeif merged 1 commit into from
Sep 19, 2026
Merged

Fix: two spec-discovery defects that understate coverage and invent drift #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
253 changes: 231 additions & 22 deletions crates/atlas-cli/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1060,8 +1060,15 @@ fn week_range(ts: i64) -> String {

fn load_specs(root: &Path) -> Result<Vec<Spec>> {
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,
Expand All @@ -1071,12 +1078,14 @@ fn load_specs(root: &Path) -> Result<Vec<Spec>> {
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;
}
Expand Down Expand Up @@ -1131,6 +1140,18 @@ fn find_companions(root: &Path, spec_path: &Path) -> Vec<String> {
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]) {
Expand All @@ -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::<serde_json::Value>(&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
/// `"<module>"` 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<String> {
// 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: `.../<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"))
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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<String> = 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<String> = 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 `"<module>"` 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
Expand Down
14 changes: 12 additions & 2 deletions specs/engine/engine.spec.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Vec<Spec>>` | 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<Vec<Spec>>` | 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<Spec>` | 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<Source>` | 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. |
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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. |
Loading