fix(checks): stop ImportFanOutOutlier flagging shared utilities (CD-333)

CHANGELOG.md

@@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Refactor.DuplicateBlock` missed clones that differ only in their literal values (CD-331). The report blamed unmodified identifiers, which was wrong: identifiers were already canonicalised to window-relative positional indices, confirmed with a fixture that renamed every one and still fired. String and number literals were the gap — they were hashed by value, so three worker files differing in three strings produced nothing. Literals are now normalised to positional placeholders too, behind a `normalize_literals` option that is on by default.
 - Import blocks are no longer eligible to be duplicates. Normalising module specifiers made every run of `import` statements match every other one — one repo went from 8 findings to 66, almost all on line 1 — and an import list cannot be extracted into a shared helper, so the finding was never actionable. `import`, `export * from` and `export { x } from` are excluded from statement windows outright; a plain `export { x }` or `export function` still participates. **On upgrade this can move a finding's primary span further down a file.** Suppression targets a comment's next non-blank line rather than a range, so a `cofferdam-ignore` anchored to an import may no longer cover the block it was written for, surfacing a duplicate that had been suppressed. Move the comment to the first statement of the block itself.
 - `Refactor.NearDuplicateBlock` is new, and reports the near-clones that literal normalisation newly finds. They are worth seeing — a copy that has drifted is where bugs hide — but on real projects they are mostly test setup blocks differing in their fixture values, and `--fail-on` defaults to `medium`, so reporting them under the existing check would turn CI red for duplication nobody chose to gate on. The engine stamps severity per check id rather than per issue, so a second id is the only way to separate them. `Refactor.DuplicateBlock` keeps `medium` and now reports verbatim clones only; `NearDuplicateBlock` defaults to `low`, printing without failing a build. Raise it with `[checks."Refactor.NearDuplicateBlock"] severity = "medium"` to gate on it deliberately. Both read one corpus slot and share a single overlap-claim pass, so they never report the same region twice.- `Design.LayerViolation`'s catalogue page no longer recommends `.cofferdamignore` for excluding tests. That is the exact configuration that makes `Design.OrphanExport` claim a symbol is "never imported in the project" when its only importers are the ignored tests — `.cofferdamignore` prunes files before discovery, so they contribute no import edges either (CD-325). The page now points at an `[[overrides]]` block with `disabled = true`, which turns off the one check without removing the files from the graph. The underlying behaviour is unchanged and tracked separately.
+- `Design.ImportFanOutOutlier`'s fan-in branch no longer fires on fan-in alone (CD-333). Any successfully shared module is a fan-in outlier by construction, so the check was flagging leaf utilities for being reused: a manual audit across five local repos found 23 fan-in findings, every one a false positive (buttons, config, utils, type modules, an API client), and not one genuine over-centralised god object. The fan-in branch now also requires the file's fan-out to be a statistical outlier, matching the check's stated target of a hub with high fan-in *and* high fan-out. Fan-in findings are rare by design as a result; the fan-out branch is unchanged.
+- `Design.LayerViolation`'s catalogue page no longer recommends `.cofferdamignore` for excluding tests. That is the exact configuration that makes `Design.OrphanExport` claim a symbol is "never imported in the project" when its only importers are the ignored tests — `.cofferdamignore` prunes files before discovery, so they contribute no import edges either (CD-325). The page now points at an `[[overrides]]` block with `disabled = true`, which turns off the one check without removing the files from the graph. The underlying behaviour is unchanged and tracked separately.
 - `Refactor.PurityHeuristic` can be switched on (CD-324). Its one option is called `enabled`, and the config loader stripped that key out of every check's option bag before validation as a forward-compatible placeholder — so the check's own catalogue page told the user to write `enabled = true`, and the check never saw it. A registered check no user could reach is worse than an unrecognised key, so `enabled` now flows through to the option bag like any other option, and is dropped again only for the checks that do not declare it. Configs carrying a stray `enabled` therefore keep loading rather than failing on upgrade, which is why the fix is silent rather than a warning: making the inert case loud needs a warning sink `options_for_raw` does not have, and is not worth a breaking change to reach.
 - `scripts/version.mjs set X.Y.Z --regen` could stamp the previous version into `docs/public/checks.json` and `docs/public/llms.txt` (CD-317). A stale cargo fingerprint after a `Cargo.toml` revert-then-rebump left `cargo build --workspace` reporting `Finished` without recompiling `cofferdam-cli`, so the subsequent `gen-docs` ran the old binary and wrote the old version, and `node scripts/version.mjs check` then failed on locations that regen was meant to fix. `--regen` now runs `cargo clean -p cofferdam-cli` before the build to force that crate's recompile, checks the built binary's own `--version` against the target before trusting it to run `gen-docs`, and re-reads `checks.json`/`llms.txt` afterwards to confirm they landed on the target version — failing loudly and naming both versions if not.
 - `--hide-baselined` now applies to every output format, not text alone (CD-315). The flag filtered findings inside the text formatter, so `--format=json`, `compact` and `sarif` received the untouched list and printed baselined findings anyway, marked `"baselined": true`. Nothing in the help text said so, and a CI pipeline reading JSON was quietly counting pre-existing findings as new work. The filter now runs once in the CLI, ahead of the per-format branch, so all four formats render the same list. Summary counts are deliberately left whole — `total`, `new` and `baselined` still describe the full run, matching what the text formatter has always done — so the gate count stays visible when the entries are hidden.

crates/cofferdam-checks/docs/Design.ImportFanOutOutlier.md

@@ -6,7 +6,7 @@ default_severity: Medium
 options: []
 ---
 
-A file's import fan-in (files that import it) or fan-out (files it imports) is a statistical outlier versus the rest of the project — a likely "god module" (doing too much) or over-centralized dependency (too many things depend on one module), found without a hardcoded threshold.
+A file's import fan-out (files it imports) is a statistical outlier versus the rest of the project — a likely "god module" doing too much, found without a hardcoded threshold. A file whose fan-in (files that import it) and fan-out are both outliers is flagged as a genuine hub — over-centralised, pulling in half the project while everything else pulls it in too.
 
 ```ts
 // god.ts — imports a dozen unrelated modules; flagged for fan-out
@@ -16,10 +16,7 @@ import { c } from "./c";
 // ...ten more...
 ```
 
-```ts
-// utils.ts — imported by nearly every other file; flagged for fan-in
-export function formatDate() { /* ... */ }
-```
+Fan-in alone is never flagged: a shared utility imported by nearly every other file is doing exactly what a shared utility should. Only a file that is both heavily imported and itself imports heavily — high fan-in and high fan-out together — earns the fan-in finding.
 
 Not flagged — `index.ts` and `types.ts` barrels are excluded entirely, since a real aggregator's high fan-in/fan-out is by design, not a smell:
 
@@ -30,7 +27,7 @@ export * from "./b";
 export * from "./c";
 ```
 
-Statistics: computed only over in-project (resolved) import edges — an external package import (`react`, `lodash`) doesn't count toward either metric, including a bare specifier the resolver traced into `node_modules` (a vendor package's own internal import graph never counts toward fan-in/fan-out, and vendor files never enter the population). `index.*`/`types.*` basenames are excluded from the statistical population entirely (not just from being flagged), since including their legitimately extreme counts would inflate the mean/stddev for every other file. Below 8 non-excluded files in the project, or when a metric's standard deviation is 0 (every file has the same count), nothing is flagged for that metric — there isn't enough variation to call anything an outlier. A file's fan-in (or fan-out) must exceed the project mean plus 3 standard deviations to be flagged; a file can be flagged for both metrics independently.
+Statistics: computed only over in-project (resolved) import edges — an external package import (`react`, `lodash`) doesn't count toward either metric, including a bare specifier the resolver traced into `node_modules` (a vendor package's own internal import graph never counts toward fan-in/fan-out, and vendor files never enter the population). `index.*`/`types.*` basenames are excluded from the statistical population entirely (not just from being flagged), since including their legitimately extreme counts would inflate the mean/stddev for every other file. Below 8 non-excluded files in the project, or when a metric's standard deviation is 0 (every file has the same count), nothing is flagged for that metric — there isn't enough variation to call anything an outlier. A file's fan-out must exceed the project mean plus three standard deviations to be flagged on its own. Fan-in requires both: the file's fan-in must clear its own three-standard-deviation bar and its fan-out must clear the fan-out bar too. Fan-in alone can never distinguish a healthy leaf module — reused everywhere, importing little — from a real god object, so the check no longer treats it as sufficient by itself. This makes fan-in findings rare by design: most true hubs show up on the fan-out branch anyway, and the fan-in branch now exists only to catch a hub that also gets pulled in from everywhere.
 
 Suppressing a legitimate hub: findings are pinned to line 1, column 1 of the file (it's a whole-file metric, not tied to a specific import), so the usual next-line `// cofferdam-ignore: Design.ImportFanOutOutlier` comment has no real "line 0" to sit on. Use the file-wide directive instead — `// cofferdam-ignore-file: Design.ImportFanOutOutlier: <reason>` anywhere in the file — which matches regardless of the finding's line number.
 

crates/cofferdam-checks/src/design/import_fan_out_outlier.rs

@@ -224,12 +224,16 @@ fn compute_outliers(
     let mut sorted_population = population;
     sorted_population.sort_by(|a, b| a.display.cmp(&b.display));
     for stats in sorted_population {
-        if stddev_in > 0.0 && stats.fan_in as f64 > threshold_in {
+        if stddev_in > 0.0
+            && stats.fan_in as f64 > threshold_in
+            && stddev_out > 0.0
+            && stats.fan_out as f64 > threshold_out
+        {
             issues.push(Issue {
                 check_id: META.id.to_string(),
                 message: format!(
-                    "unusually high import fan-in: {} files import this one (project mean {mean_in:.1}, stddev {stddev_in:.1}) — possible over-centralized dependency",
-                    stats.fan_in
+                    "unusually high import fan-in and fan-out: {} files import this one and it imports {} others (project mean fan-in {mean_in:.1}, stddev {stddev_in:.1}) — possible over-centralized hub",
+                    stats.fan_in, stats.fan_out
                 ),
                 file: stats.display.clone(),
                 location: Location::from_span(&stats.display, zero_span),
@@ -255,3 +259,111 @@ fn compute_outliers(
     }
     issues
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use cofferdam_core::graph::{ImportKind, ImportedName};
+    use std::path::PathBuf;
+
+    fn edge(from: &Path, to: &Path) -> ImportRecord {
+        ImportRecord {
+            from_file: from.to_path_buf(),
+            source_specifier: "./m".to_string(),
+            resolved: Some(to.to_path_buf()),
+            names: vec![ImportedName {
+                source_name: "x".to_string(),
+                local_name: "x".to_string(),
+                kind: ImportKind::Named,
+                type_only: false,
+                local_use_count: 1,
+            }],
+            type_only: false,
+            span: Span {
+                start_byte: 0,
+                end_byte: 0,
+                line: 1,
+                column: 1,
+            },
+        }
+    }
+
+    fn messages_for<'a>(issues: &'a [Issue], needle: &str) -> Vec<&'a Issue> {
+        issues
+            .iter()
+            .filter(|i| i.message.contains(needle))
+            .collect()
+    }
+
+    /// CD-333 regression: a leaf utility with many importers but no
+    /// imports of its own (fan-in high, fan-out low) must not be
+    /// flagged — it is a shared module doing its job, not a hub.
+    #[test]
+    fn high_fan_in_alone_is_not_flagged() {
+        let leaf = PathBuf::from("/proj/leaf.ts");
+        let mut all_files = HashSet::new();
+        all_files.insert(leaf.clone());
+        let mut imports = Vec::new();
+        for i in 0..15 {
+            let importer = PathBuf::from(format!("/proj/importer_{i}.ts"));
+            all_files.insert(importer.clone());
+            imports.push(edge(&importer, &leaf));
+        }
+
+        let issues = compute_outliers(&imports, &[], &all_files);
+        assert!(
+            issues.is_empty(),
+            "expected no findings for a high-fan-in-only leaf, got {issues:?}"
+        );
+    }
+
+    /// A genuine god module — high fan-in AND high fan-out — must
+    /// still fire the fan-in finding.
+    #[test]
+    fn high_fan_in_and_fan_out_together_is_flagged() {
+        let god = PathBuf::from("/proj/god.ts");
+        let mut all_files = HashSet::new();
+        all_files.insert(god.clone());
+        let mut imports = Vec::new();
+        for i in 0..15 {
+            let sibling = PathBuf::from(format!("/proj/sibling_{i}.ts"));
+            all_files.insert(sibling.clone());
+            imports.push(edge(&sibling, &god)); // sibling -> god: god's fan-in
+            imports.push(edge(&god, &sibling)); // god -> sibling: god's fan-out
+        }
+
+        let issues = compute_outliers(&imports, &[], &all_files);
+        let fan_in_hits = messages_for(&issues, "fan-in");
+        assert_eq!(
+            fan_in_hits.len(),
+            1,
+            "expected exactly one fan-in finding, got {issues:?}"
+        );
+        assert_eq!(fan_in_hits[0].file, god);
+    }
+
+    /// The fan-out branch is untouched: high fan-out with low fan-in
+    /// still produces exactly the fan-out finding, nothing else for
+    /// that file.
+    #[test]
+    fn high_fan_out_alone_is_still_flagged() {
+        let hub = PathBuf::from("/proj/hub.ts");
+        let mut all_files = HashSet::new();
+        all_files.insert(hub.clone());
+        let mut imports = Vec::new();
+        for i in 0..15 {
+            let target = PathBuf::from(format!("/proj/target_{i}.ts"));
+            all_files.insert(target.clone());
+            imports.push(edge(&hub, &target));
+        }
+
+        let issues = compute_outliers(&imports, &[], &all_files);
+        let hub_issues: Vec<&Issue> = issues.iter().filter(|i| i.file == hub).collect();
+        assert_eq!(
+            hub_issues.len(),
+            1,
+            "expected exactly one finding for hub, got {issues:?}"
+        );
+        assert!(hub_issues[0].message.contains("fan-out"));
+    }
+}

crates/cofferdam-checks/src/design/mod.rs

@@ -1724,10 +1724,12 @@ export function computeTotal(items: number[]): number {
     }
 
     #[test]
-    fn fan_in_outlier_is_flagged() {
-        // 14 files that each import a single shared `utils.ts` — utils
-        // has fan-in 14 against a background of zero, the fan-in mirror
-        // of `fan_out_outlier_is_flagged`.
+    fn high_fan_in_alone_is_not_flagged() {
+        // CD-333: 14 files that each import a single shared `utils.ts`
+        // — utils has fan-in 14 against a background of zero, but zero
+        // fan-out of its own. A shared leaf module doing its job must
+        // not be flagged; fan-in alone can't distinguish it from a
+        // real hub.
         let utils = PathBuf::from("/p/utils.ts");
         let imports: Vec<ImportRecord> = (0..14)
             .map(|i| {
@@ -1736,13 +1738,36 @@ export function computeTotal(items: number[]): number {
             })
             .collect();
         let issues = run_fan_out_outlier(imports);
+        assert!(
+            issues.is_empty(),
+            "high fan-in alone must not be flagged; got {issues:?}"
+        );
+    }
+
+    #[test]
+    fn high_fan_in_and_fan_out_together_is_flagged() {
+        // CD-333: a genuine god module — high fan-in AND high fan-out
+        // — must still fire the fan-in finding. 14 sibling files each
+        // import `hub.ts` (hub's fan-in) and `hub.ts` imports each of
+        // them back (hub's fan-out).
+        let hub = PathBuf::from("/p/hub.ts");
+        let mut imports = Vec::new();
+        for i in 0..14 {
+            let sibling = PathBuf::from(format!("/p/sibling{i}.ts"));
+            imports.push(internal_import(&sibling, &hub));
+            imports.push(internal_import(&hub, &sibling));
+        }
+        let issues = run_fan_out_outlier(imports);
+        let fan_in_issues: Vec<&CoreIssue> = issues
+            .iter()
+            .filter(|i| i.message.contains("fan-in"))
+            .collect();
         assert_eq!(
-            issues.len(),
+            fan_in_issues.len(),
             1,
-            "expected one fan-in finding for utils.ts; got {issues:?}"
+            "expected one fan-in finding for the god module; got {issues:?}"
         );
-        assert_eq!(issues[0].file, utils);
-        assert!(issues[0].message.contains("fan-in"));
+        assert_eq!(fan_in_issues[0].file, hub);
     }
 
     #[test]