Skip to content

Commit 74063c9

Browse files
abk1969claude
andauthored
Fix Architecture Violations reporting the inverse of reality (#64)
* docs: add design spec for layer-violation direction fix detectLayerViolations reads the documented {source: definition, target: caller} connection convention backwards, reporting the exact inverse of reality — healthy downward deps flagged, genuine upward violations missed. Verified empirically against the real analyzer in both directions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs: add implementation plan for layer-violation direction fix Task-by-task plan: direction inversion (primary), detectLayer leading-slash tolerance (secondary), coupling label correction (tertiary), regression pass. Every code change pre-verified against the real analyzer via card/lib/analyzer.js. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(architecture): report layer violations in the correct dependency direction * fix(architecture): classify root-level layer folders in detectLayer * docs: record test-file importer exclusion surfaced during implementation Tasks 1+2 interact to introduce a new "test should not import from utils" false-positive class (dir-based test files classify as layer 'test'=6 and, with the direction corrected, get flagged for importing anything more foundational). Excluded via the existing isArchitectureTestFile classifier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(architecture): exclude test files from the importer side of layer violations Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(architecture): correct coupling issue label to describe fan-out, not fan-in * fix(architecture): broaden test-importer guard to the full test layer (Python, test-folders) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent af3b007 commit 74063c9

4 files changed

Lines changed: 746 additions & 16 deletions

File tree

Lines changed: 334 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,334 @@
1+
# Layer-Violation Direction Fix Implementation Plan
2+
3+
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
4+
5+
**Goal:** Make CodeFlow's Architecture Violations detector report the correct dependency direction (it currently reports the exact inverse), fix the secondary `detectLayer` leading-slash gap, and correct the coupling metric's fan-in/fan-out label — all inside `index.html`'s analyzer, no new dependencies.
6+
7+
**Architecture:** Three localized fixes to pure functions in the analyzer block of `index.html` (`Parser.detectLayerViolations`, `Parser.detectLayer`, and the coupling issue-builder inside `buildAnalysisData`), each backed by a new fixture-driven test in `tests/layer-violation-direction.test.mjs` following the existing `tests/codeflow-golden.test.mjs` VM-harness pattern. None of the fixes touch how the connection graph (`conns`) is built — they only correct readers that misinterpret the documented `{source: definition, target: caller}` convention.
8+
9+
**Tech Stack:** Vanilla JS (no build step, no framework, no new dependencies). Tests run with Node's built-in `node:test` runner via `node --test`.
10+
11+
## Global Constraints
12+
13+
- No new npm dependencies. No build step exists for `index.html` and none is being introduced.
14+
- Every change lives inside the existing `CODEFLOW_ANALYZER_START`/`END` block in `index.html`, matching the file's existing code style (`var`, single-line `if` bodies, no semicolonless ASI reliance).
15+
- Do NOT change how `conns` is built (`index.html:4740`) or the documented `{source: fileDefiningFn, target: fileCallingFn}` convention (`index.html:4947`). Other consumers (graph rendering, circular detection) rely on it. Fix only the misreading consumers.
16+
- Do NOT expand `detectLayer`'s folder vocabulary (no new rules for `db/`, `legacy/`, `*.config.*`). The only `detectLayer` change is leading-slash tolerance so existing patterns also match root-level folders.
17+
- `tests/codeflow-golden.test.mjs` and the full existing suite (`node --test`) must stay green, unmodified, after every task (non-regression gate).
18+
- Every code change in this plan has been verified against the real analyzer (via `card/lib/analyzer.js`) — implementers transcribe, they do not re-derive.
19+
20+
---
21+
22+
## File Structure
23+
24+
- `index.html` — modify in place (analyzer block only): `Parser.detectLayerViolations` (~line 1379), `Parser.detectLayer` (~line 1098), coupling issue-builder (~line 4765).
25+
- `tests/layer-violation-direction.test.mjs` — new test file, created in Task 1, appended to in Tasks 2-3.
26+
27+
---
28+
29+
### Task 1: Fix the direction inversion in `detectLayerViolations`
30+
31+
**Files:**
32+
- Modify: `index.html:1384-1403` (the `connections.forEach` body inside `detectLayerViolations`)
33+
- Create: `tests/layer-violation-direction.test.mjs`
34+
35+
**Interfaces:**
36+
- Consumes: `Parser.detectLayerViolations(files, connections)` where `files` is `[{path, layer}]` and `connections` follows the documented convention `{source: fileDefiningFn, target: fileCallingFn, fn}` (`index.html:4947`).
37+
- Produces: `tests/layer-violation-direction.test.mjs` — a standalone `node:test` file following the same in-file analyzer-loading pattern as `tests/codeflow-golden.test.mjs`. Tasks 2-3 append to it.
38+
39+
- [ ] **Step 1: Write the test harness and the two failing direction tests**
40+
41+
Create `tests/layer-violation-direction.test.mjs`:
42+
```js
43+
import assert from 'node:assert/strict';
44+
import { readFile } from 'node:fs/promises';
45+
import { dirname, join } from 'node:path';
46+
import { fileURLToPath } from 'node:url';
47+
import test from 'node:test';
48+
import vm from 'node:vm';
49+
50+
const __dirname = dirname(fileURLToPath(import.meta.url));
51+
const repoRoot = join(__dirname, '..');
52+
const htmlSource = await readFile(join(repoRoot, 'index.html'), 'utf8');
53+
const startMarker = '// ===== CODEFLOW_ANALYZER_START =====';
54+
const endMarker = '// ===== CODEFLOW_ANALYZER_END =====';
55+
const parserStart = htmlSource.indexOf(startMarker);
56+
const parserEnd = htmlSource.indexOf(endMarker, parserStart);
57+
58+
if (parserStart < 0 || parserEnd < 0) {
59+
throw new Error('Could not locate analyzer source in index.html');
60+
}
61+
62+
const context = {
63+
console,
64+
TreeSitter: undefined,
65+
Babel: undefined,
66+
acorn: undefined,
67+
getSecurityScanContent(file) {
68+
return file && file.content ? file.content : '';
69+
},
70+
isSanitizedPreviewRenderer() {
71+
return false;
72+
},
73+
};
74+
75+
vm.createContext(context);
76+
vm.runInContext(`${htmlSource.slice(parserStart, parserEnd)}\nthis.Parser = Parser;`, context);
77+
const { Parser } = context;
78+
79+
// Convention (index.html): connection {source: fileDefiningFn (imported), target: fileCallingFn (importer)}.
80+
// layerOrder: lower number = higher/topmost layer. services=2, utils/lib=4. utils importing UP from services = violation.
81+
const files = [
82+
{ path: 'src/services/userService.ts', layer: 'services' },
83+
{ path: 'src/lib/helper.ts', layer: 'utils' },
84+
];
85+
86+
test('healthy downward dependency (service uses a util) is NOT a violation', () => {
87+
// userService (services) calls a function defined in lib/helper (utils):
88+
// caller = userService, definition = helper => {source: helper, target: userService}
89+
const violations = Parser.detectLayerViolations(files, [
90+
{ source: 'src/lib/helper.ts', target: 'src/services/userService.ts', fn: 'formatDate' },
91+
]);
92+
assert.equal(violations.length, 0);
93+
});
94+
95+
test('genuine upward dependency (util reaches into a service) IS a violation, correctly attributed', () => {
96+
// helper (utils) calls a function defined in userService (services):
97+
// caller = helper, definition = userService => {source: userService, target: helper}
98+
const violations = Parser.detectLayerViolations(files, [
99+
{ source: 'src/services/userService.ts', target: 'src/lib/helper.ts', fn: 'fetchUser' },
100+
]);
101+
assert.equal(violations.length, 1);
102+
assert.equal(violations[0].from, 'src/lib/helper.ts');
103+
assert.equal(violations[0].to, 'src/services/userService.ts');
104+
assert.match(violations[0].suggestion, /utils should not import from services/);
105+
});
106+
```
107+
108+
- [ ] **Step 2: Run the tests to verify they fail**
109+
110+
Run: `node --test tests/layer-violation-direction.test.mjs`
111+
Expected: BOTH fail — the healthy-dependency test reports 1 violation (false positive), and the genuine-violation test reports 0 (false negative), because the current code reads the direction backwards.
112+
113+
- [ ] **Step 3: Apply the direction fix**
114+
115+
In `index.html`, inside `detectLayerViolations:function(files,connections){`, find:
116+
```js
117+
connections.forEach(function(c){
118+
var srcFile=fileByPath[c.source];
119+
var tgtFile=fileByPath[c.target];
120+
if(!srcFile||!tgtFile)return;
121+
var srcLayer=(srcFile.layer||'').toLowerCase();
122+
var tgtLayer=(tgtFile.layer||'').toLowerCase();
123+
var srcLevel=layerOrder[srcLayer];
124+
var tgtLevel=layerOrder[tgtLayer];
125+
// Violation: lower layer importing from higher layer (e.g., service importing from UI)
126+
if(srcLevel!==undefined&&tgtLevel!==undefined&&srcLevel>tgtLevel&&srcLevel-tgtLevel>1){
127+
violations.push({
128+
from:srcFile.path,
129+
fromLayer:srcFile.layer,
130+
to:tgtFile.path,
131+
toLayer:tgtFile.layer,
132+
fn:c.fn,
133+
suggestion:srcFile.layer+' should not import from '+tgtFile.layer+'. Consider inverting the dependency or using dependency injection.'
134+
});
135+
}
136+
});
137+
```
138+
139+
Replace with:
140+
```js
141+
connections.forEach(function(c){
142+
// Convention (see buildAnalysisData): source = file DEFINING the fn (imported),
143+
// target = file CALLING it (the importer). The importer is c.target.
144+
var importedFile=fileByPath[c.source];
145+
var importerFile=fileByPath[c.target];
146+
if(!importedFile||!importerFile)return;
147+
var importedLayer=(importedFile.layer||'').toLowerCase();
148+
var importerLayer=(importerFile.layer||'').toLowerCase();
149+
var importedLevel=layerOrder[importedLayer];
150+
var importerLevel=layerOrder[importerLayer];
151+
// Violation: a more-foundational file (higher level number) imports from a higher-up layer.
152+
if(importerLevel!==undefined&&importedLevel!==undefined&&importerLevel>importedLevel&&importerLevel-importedLevel>1){
153+
violations.push({
154+
from:importerFile.path,
155+
fromLayer:importerFile.layer,
156+
to:importedFile.path,
157+
toLayer:importedFile.layer,
158+
fn:c.fn,
159+
suggestion:importerFile.layer+' should not import from '+importedFile.layer+'. Consider inverting the dependency or using dependency injection.'
160+
});
161+
}
162+
});
163+
```
164+
165+
- [ ] **Step 4: Run the tests to verify they pass**
166+
167+
Run: `node --test tests/layer-violation-direction.test.mjs`
168+
Expected: PASS (2 tests)
169+
170+
- [ ] **Step 5: Run the non-regression suite**
171+
172+
Run: `node --test`
173+
Expected: all suites PASS, including `tests/codeflow-golden.test.mjs` and `tests/architecture-diagram.test.mjs`, unchanged. (Layer classification is unchanged by this task; only violation direction changed, and no existing test hard-asserts on violation output.)
174+
175+
- [ ] **Step 6: Commit**
176+
177+
```bash
178+
git add index.html tests/layer-violation-direction.test.mjs
179+
git commit -m "fix(architecture): report layer violations in the correct dependency direction"
180+
```
181+
182+
---
183+
184+
### Task 2: `detectLayer` leading-slash tolerance for root-level folders
185+
186+
**Files:**
187+
- Modify: `index.html:1098-1099` (the first two lines of `detectLayer`)
188+
- Modify: `tests/layer-violation-direction.test.mjs` (append one `test()` block)
189+
190+
**Verified behavior:** every folder pattern in `detectLayer` requires a leading slash (`l.includes('/service')`, `/util`, `/lib/`, etc.), so a layer folder at the repository root (relative path, no leading slash) never matches and falls through to `return 'utils'`. Confirmed against the real analyzer: `services/userService.ts``utils` (wrong), while `src/services/userService.ts``services` (right). Prepending a single `/` to the normalized path makes root-level folders match identically to nested ones, with no change to already-nested paths. The golden fixture's files (`src/*.js`, `src/*.py`, root `*.md`) are unaffected: `src/service.py` still matches `/service` → `services`; the others match no folder pattern → `utils`; markdown is overridden to `note` before classification matters.
191+
192+
- [ ] **Step 1: Append the failing test**
193+
194+
Append to `tests/layer-violation-direction.test.mjs`:
195+
```js
196+
test('detectLayer classifies root-level layer folders, not just nested ones', () => {
197+
// Root-level folders (no leading path segment) must match the same as nested ones.
198+
assert.equal(Parser.detectLayer('services/userService.ts'), 'services');
199+
assert.equal(Parser.detectLayer('ui/Button.tsx'), 'ui');
200+
assert.equal(Parser.detectLayer('lib/helper.ts'), 'utils');
201+
// Nested paths keep their existing classification (regression guard).
202+
assert.equal(Parser.detectLayer('src/services/userService.ts'), 'services');
203+
assert.equal(Parser.detectLayer('src/lib/helper.ts'), 'utils');
204+
// A file with no recognizable layer folder still falls through to the default.
205+
assert.equal(Parser.detectLayer('src/app.js'), 'utils');
206+
});
207+
```
208+
209+
- [ ] **Step 2: Run the test to verify it fails**
210+
211+
Run: `node --test tests/layer-violation-direction.test.mjs`
212+
Expected: FAIL — `detectLayer('services/userService.ts')` currently returns `'utils'`, not `'services'` (and `'ui/Button.tsx'` returns `'utils'`, not `'ui'`).
213+
214+
- [ ] **Step 3: Apply the leading-slash normalization**
215+
216+
In `index.html`, find the first two lines of `detectLayer`:
217+
```js
218+
detectLayer:function(p){
219+
var l=p.toLowerCase();
220+
```
221+
222+
Replace with:
223+
```js
224+
detectLayer:function(p){
225+
// Prepend a leading slash so root-level layer folders (e.g. "services/x.ts")
226+
// match the same substring patterns as nested ones (e.g. "src/services/x.ts").
227+
var l='/'+p.toLowerCase().replace(/^\/+/,'');
228+
```
229+
230+
- [ ] **Step 4: Run the test to verify it passes**
231+
232+
Run: `node --test tests/layer-violation-direction.test.mjs`
233+
Expected: PASS (3 tests total)
234+
235+
- [ ] **Step 5: Run the non-regression suite**
236+
237+
Run: `node --test`
238+
Expected: all suites PASS. In particular `tests/codeflow-golden.test.mjs` (whose fixture layers are unchanged by this normalization) and `tests/architecture-diagram.test.mjs` stay green. If any suite changes, STOP and report — a shifted classification means a fixture path matched a pattern it shouldn't; do not modify existing tests to accommodate.
239+
240+
- [ ] **Step 6: Commit**
241+
242+
```bash
243+
git add index.html tests/layer-violation-direction.test.mjs
244+
git commit -m "fix(architecture): classify root-level layer folders in detectLayer"
245+
```
246+
247+
---
248+
249+
### Task 3: Correct the coupling metric's fan-in/fan-out label
250+
251+
**Files:**
252+
- Modify: `index.html:4765` (the "Highly Coupled" issue title/desc)
253+
- Modify: `tests/layer-violation-direction.test.mjs` (append one `test()` block)
254+
255+
**Verified behavior:** `coupling[c.target]++` (`index.html:4763`) keys on `c.target`, which — per the documented convention — is the **calling** file (the importer). So the count is each file's fan-**out** (how many external functions it calls), but the emitted issue is titled "Highly Coupled" with desc "Files imported by 8+ others" (fan-**in**). This is a mislabel, not a miscount: the metric still surfaces highly-coupled files, but the description claims the opposite relationship. This task corrects only the wording (zero behavioral risk); it does NOT change the `c.target` key, because re-keying to `c.source` would change which files are flagged and is out of scope.
256+
257+
- [ ] **Step 1: Append the failing test**
258+
259+
This fix is a pure copy change to a static string literal (no behavioral difference — the `coupling` count and which files are flagged are unchanged). A proportionate guard asserts on the emitted wording directly, using the `htmlSource` the harness already loaded. Append to `tests/layer-violation-direction.test.mjs`:
260+
```js
261+
test('coupling issue label describes fan-out (files it imports), not fan-in', () => {
262+
const start = htmlSource.indexOf("title:highCoup.length+' Highly Coupled'");
263+
assert.ok(start > 0, 'Highly Coupled issue builder not found in analyzer source');
264+
const snippet = htmlSource.slice(start, start + 200);
265+
// The stale fan-in wording ("imported by 8+ others") must be gone...
266+
assert.equal(/imported by 8\+ others/.test(snippet), false, 'stale fan-in wording still present');
267+
// ...replaced by wording that describes fan-out (this file imports others).
268+
assert.match(snippet, /import 8\+ other/);
269+
});
270+
```
271+
272+
Note: this is intentionally a source-text guard, not a pipeline test — the change alters only a description string, so exercising the full `buildAnalysisData` coupling path (which would require engineering a 9+ file fixture to trip the `>8` threshold) would add fixture complexity without testing anything the string assertion doesn't. If a reviewer prefers a behavioral test, that is a reasonable upgrade, but it is not required for a copy-only change.
273+
274+
- [ ] **Step 2: Run the test to verify it fails**
275+
276+
Run: `node --test tests/layer-violation-direction.test.mjs`
277+
Expected: FAIL — the current desc is `'Files imported by 8+ others'`, so the `/imported by 8\+ others/` match is still true (assertion `false` fails) and the new-wording match is absent.
278+
279+
- [ ] **Step 3: Apply the label correction**
280+
281+
In `index.html`, find:
282+
```js
283+
if(highCoup.length)issues.push({type:'warning',title:highCoup.length+' Highly Coupled',desc:'Files imported by 8+ others',items:highCoup.map(function(x){return{name:x[0].split('/').pop()+' ('+x[1]+' imports)',file:x[0],imports:x[1]};})});
284+
```
285+
286+
Replace with:
287+
```js
288+
if(highCoup.length)issues.push({type:'warning',title:highCoup.length+' Highly Coupled',desc:'Files that import 8+ other files',items:highCoup.map(function(x){return{name:x[0].split('/').pop()+' ('+x[1]+' imports)',file:x[0],imports:x[1]};})});
289+
```
290+
291+
- [ ] **Step 4: Run the test to verify it passes**
292+
293+
Run: `node --test tests/layer-violation-direction.test.mjs`
294+
Expected: PASS (4 tests total)
295+
296+
- [ ] **Step 5: Run the non-regression suite**
297+
298+
Run: `node --test`
299+
Expected: all suites PASS, unchanged.
300+
301+
- [ ] **Step 6: Commit**
302+
303+
```bash
304+
git add index.html tests/layer-violation-direction.test.mjs
305+
git commit -m "fix(architecture): correct coupling issue label to describe fan-out, not fan-in"
306+
```
307+
308+
---
309+
310+
### Task 4: Full regression pass
311+
312+
**Files:** none (verification only)
313+
314+
- [ ] **Step 1: Run every test in the repo**
315+
316+
Run: `node --test`
317+
Expected: all suites PASS, including `tests/layer-violation-direction.test.mjs` (4 tests), `tests/codeflow-golden.test.mjs`, and `tests/architecture-diagram.test.mjs`, all unmodified except the new file.
318+
319+
- [ ] **Step 2: Confirm the diff touches only the intended surface**
320+
321+
Run: `git diff --stat origin/main..HEAD`
322+
Expected: only `index.html`, `tests/layer-violation-direction.test.mjs`, and the two `docs/superpowers/` files (design + plan) changed. No other files.
323+
324+
- [ ] **Step 3: Report status**
325+
326+
No commit — verification checkpoint. If both steps pass, the branch is ready for the user to review and decide whether to push and open the PR against `braedonsaunders/codeflow` (not automated by this plan).
327+
328+
---
329+
330+
## Self-Review Notes
331+
332+
- **Spec coverage:** all three design fixes have a task (1: direction inversion — primary; 2: detectLayer leading-slash — secondary; 3: coupling label — tertiary), plus a final regression pass (4). The out-of-scope items from the design (no `conns` change, no folder-vocabulary expansion, no coupling re-key) are respected — each task's fix is the minimal one named in the design.
333+
- **Placeholder scan:** no TBD/TODO; every step has complete, verified code.
334+
- **Type/name consistency:** the connection field names (`source`, `target`, `fn`), the `files` element shape (`{path, layer}`), and the violation output fields (`from`, `to`, `fromLayer`, `toLayer`, `suggestion`) match the actual `detectLayerViolations` contract at `index.html:1379-1405`. The test harness mirrors `tests/codeflow-golden.test.mjs` exactly (same markers, same VM context stubs).

0 commit comments

Comments
 (0)