From 3a35b032eaf3a2f5ee75b70de7774bc90b101adc Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Sun, 27 Sep 2026 16:02:38 -0400 Subject: [PATCH] feat(architecture): draw --forbid violations red A violating file edge is drawn red and thicker. Under --group-depth the directory edge it rolls up into is marked. --max-render is not overridden to force a violation into the picture; a stderr note counts the hidden ones, and the run still exits 1. Without violations the DOT is byte-identical to a run without --forbid. --- README.md | 5 +++-- TECHNICAL.md | 4 ++-- USAGE.md | 6 ++++-- render/callgraph.js | 46 +++++++++++++++++++++++++++++++++++++++++---- test/run.js | 34 +++++++++++++++++++++++++++++++-- 5 files changed, 83 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 6d5b6da..840a001 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,9 @@ specifically for this mode: - `--forbid ->` — an architecture rule; repeat it for several. Each side is a repo-relative path: ending in `/` it means every file under that directory, otherwise one exact file (`--forbid 'src/ui/->src/db/'`). Any file - edge a rule matches is listed on stderr with its weight and the run exits 1, - after the diagram is written, so CI fails with the picture as evidence. Rules + edge a rule matches is listed on stderr with its weight and drawn red, and + the run exits 1 after the diagram is written, so CI fails with the picture as + evidence. Rules are checked on the per-file edges, before `--group-depth` and `--max-render`, so neither can hide a violation. - `--depth` has no effect here and is rejected if passed — there's no diff --git a/TECHNICAL.md b/TECHNICAL.md index cd035df..c63ea8c 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -200,7 +200,7 @@ It doubles as a live legend for the [Visual Encoding](#visual-encoding) rules be - **`render/callgraph.js`** — the entire tool. Exports `buildDot(symbol, callers, callees, { maxRender, transitiveEdges })` (pure: turns caller/callee arrays into a DOT digraph string, deduplicating entries with the same `name`+`filePath` so repeated JSON rows don't render as duplicate edges; via `nodeIdentities` it gives each drawn node a graphviz id unique per `name`+`filePath`, so two *distinct* symbols that share a name but live in different files render as two separate boxes instead of silently collapsing into one — graphviz keys a node by the exact string in its edge, so without this the second same-named caller/callee vanishes from the picture; a name that occurs in only one file keeps name-as-id, leaving collision-free graphs byte-for-byte identical to before this existed, and only a colliding name gains a file-qualified id plus a `name\n(basename)` label to tell the boxes apart; when `tooltips` is set (`main` sets it for `svg`/`svgz` output only, since graphviz renders node tooltips as `` there and ignores them for raster formats), every drawn node is declared with its `filePath` as a hover tooltip so you can read which file a symbol lives in without cluttering the box — the root symbol is left un-tooltipped since `buildDot` isn't passed its file; if `maxRender` is given, `allocateRenderBudget` splits it as one shared allowance across callers, callees, and `transitiveEdges` — spent in that priority order, so the direct trail is never starved to make room for deeper hops; a `"kind":"file"` node — codegraph's way of saying "this is a module-level/import reference, not a verified function call" — is styled dotted/gray/`"file"` via `edgeStyleAttrs` instead of looking like a real call edge; `transitiveEdges`, an optional array of `{ from, to, depth }` pairs from `--depth > 1` traversal, is colored by `depthColor(depth)` unless it's file-kind — omitting `transitiveEdges`/`maxRender` renders exactly as before either feature existed), `allocateRenderBudget(maxRender, counts)` (pure: the shared-budget split described above, also called from `main` so its stderr notes report what actually got drawn), `dedupeNodes(nodes)` (pure: collapses same-`name`+`filePath` entries, used by both `buildDot` and `main` so distinct-count logic has one source of truth), `dedupeEdges(edges)` (pure: the same idea as `dedupeNodes` but keyed on a `from`+`to` pair, used only for `transitiveEdges`), `depthColor(depth)` (pure: maps hop distance ≥2 to a progressively lighter shade, clamped at the palette's last entry for very deep hops), `isTestRef(node)` (true if a node's name or filePath looks test-related, used to render those edges dashed — applies to `transitiveEdges` too, checked against each edge's `from` node, and is overridden by file-kind styling when both apply; also reused by `buildArchitectureDot` below, passed `{name: '', filePath}` since architecture-mode nodes have no symbol name — the name-based heuristics degrade harmlessly to `false` on an empty name, leaving the path-based ones intact), `truncationWarning(kind, results, limit, response)` (pure: returns a warning string if the fetch was cut off by `limit`, else `null` — the *fetch* cap; it names the real total when codegraph reported one), `readTruncation(response, results, limit)` (pure: the single source of that decision for symbol and `--diff` mode — codegraph's own `truncated`/`total` fields when the response carries them, read by type so a reported `false` suppresses the warning, else the older "result count hit `limit` exactly" heuristic), `emptyGraphWarning(symbol, callers, callees)` (pure: fires when the symbol has no callers and no callees, so a lone-box diagram reads as a finding rather than a glitch), `renderTruncationNote(kind, distinctCount, cap)` (pure: returns a warning string if the deduplicated count exceeds `cap`, else `null` — the *render* cap; `main` passes each dimension's actual `allocateRenderBudget` allotment as `cap`, not the raw `--max-render`; also reused as-is by `--architecture` mode with `kind: 'files'`), `depthBudgetWarning(truncated, budget)` (pure: returns a warning string if `--depth` traversal hit its node-discovery cap (`--max-depth-nodes`) before finishing, else `null`), `formatMismatchWarning(outFile, format)` (pure: returns a warning string if `--out`'s extension is a real `dot`-recognized format that disagrees with `--format`, else `null`), `matchSymbolNotFound(out)` (pure: extracts the symbol name from codegraph's plain-text "Symbol not found" message, or `null` if `out` doesn't match that shape), `matchNotInitialized(out)` / `argRepoPath(args)` (pure: recognize codegraph's plain-text "not initialized" response and recover the `--path` it was called with, so a never-indexed repo gets a clean `codegraph init ` instruction instead of a "did not return JSON" dump), `parseCodegraphOutput` (JSON parse plus both of those plain-text checks), `parseUnresolvedRefs(statusOutput)` (pure: the unresolved-reference count out of `codegraph status`, which `indexHealthWarning` turns into the mid-rebuild warning), and `nodeKey(n)` (pure: the one `name`+`filePath` key string every dedupe and `--diff` root lookup shares). Everything else (`requireOnPath`, `runCodegraph`, `indexHealthWarning`, `resolveSymbol`, `collectTransitive`, `main`) is CLI plumbing, not exported — `resolveSymbol` and `collectTransitive` in particular do real I/O (`codegraph` calls), so like `runCodegraph` they're only exercised by the CLI-level tests, not unit-tested directly. - **`--architecture` mode adds:** `unwrapQueryNodes(queryResults)` (pure: unwraps `query`'s `{node, score}` result shape, dropping only a missing `node` — a `"kind":"file"` entry is deliberately KEPT rather than dropped: `codegraph callees ` is a real, working probe against it, and it's the only way to surface calls made from inside a top-level anonymous callback, which codegraph attributes to the enclosing file rather than any named function — see the note on `probeFileEdges` and Known Limitations. Also deliberately does NOT allowlist "callable" kinds like `function`/`method` for the non-file entries — probing a `constant` or `variable` just harmlessly returns an empty `callees` array, which is more robust across languages than maintaining a per-language kind list. The one exception is `"kind":"import"`: an import statement's node never has callee edges of its own (0 of 3,659 on codegraph's repo), but its text probed as a name (`terse.capture`, `../src/mcp/tools`) resolves to some *other* symbol and borrows its callees, so `probeFileEdges` skips import nodes instead of probing them), `symbolBudgetWarning(truncated, budget)` (pure: same shape as `depthBudgetWarning`, fires when `--max-symbols` cut enumeration short), `duplicateNames(symbols)` (pure: the `Set` of names appearing in more than one distinct **file** — counting files rather than symbol occurrences is what keeps same-file collisions off the file-qualified route; the set `probeFileEdges` and `--diff` attribute by file), `parseNodeCalls(out, expectedFile)` (pure: reads the file-qualified trail line out of `codegraph node -f`'s text output into `{name, filePath}` callees; returns `[]` for a recognized symbol that calls nothing, `null` on any of the four untrustworthy responses listed under Known Limitations so the caller falls back rather than under-reporting, and drops a callee whose name equals its own file's basename — the structural stand-in for the `"kind":"file"` filter, since the trail carries no kind), `probeSymbolCallees(s, dupe, repoPath, limit, run)` (one symbol's callees: bare-name `callees --json`, narrowed to the definition whose `id` matches the enumerated symbol's when codegraph reports per-definition results, else for a duplicate name to its own file via `pickDefinitionResult`, else `node -f`, else the union; any answer that isn't the symbol's own definition is shared by every same-named symbol in that file, so `probeFileEdges` counts it once per file rather than once per symbol — otherwise two `run` methods in one file would double every edge weight; exported with `run` injectable so tests replay codegraph output recorded from npm codegraph into `test/fixtures/npm-codegraph/` — the `node -f` fallback's coverage in the codegraph-free test jobs. Re-record with `npm run fixtures:record -- --codegraph `, never by hand; the recorder refuses a build with per-definition JSON, whose output would never reach `node -f`), `architectureTruncationWarning(truncatedNames, limit)` (pure: one aggregated stderr line naming the symbols whose callee fetch `--limit` cut, decided per answer by `readTruncation` on the slice actually used — a matched definition's own `truncated`, else the response's, else the hit-the-limit heuristic; the `node -f` trail is never flagged, it has no `--limit` and `parseNodeCalls` already refuses a cut trail), `parseForbidRule(raw)` / `forbiddenEdges(fileEdges, rules)` / `forbidViolationReport(violations)` (pure: `--forbid`'s parse, per-file match and stderr report — see Configuration), `duplicateNameWarning(unresolvedNames)` (pure: names only the duplicate names `probeFileEdges` could not attribute by file and drew as the bare-name union; see Known Limitations), `aggregateFileEdges(symbolEdges)` (pure: dedupes/sums `{fromFile, toFile}` pairs from every probed symbol into weighted `{from, to, weight}` file edges, dropping self-file edges and any edge missing a real `filePath` on either end — an unresolved external/stdlib callee has no file of its own and would otherwise render as a bogus `""` node), `groupPath(filePath, depth)` (pure: the group a file rolls up into at `--group-depth` — its first `depth` *directory* segments with a trailing slash, so a group reads as a directory rather than a file; a repo-root file has no directory to roll into and is returned unchanged, since grouping it under `""` would merge every root file into one nameless box, and a file shallower than `depth` simply keeps the directories it has, so an over-deep `--group-depth` degrades to per-file output instead of erroring), `rollupFileEdges(fileEdges, depth)` (pure: rewrites both endpoints of every file edge through `groupPath` and re-sums the weights, dropping edges that become self-group — the directory-level restatement of `aggregateFileEdges`'s self-file rule, since once a directory is the unit an intra-directory call is internal structure, not cross-module coupling; returns the same `{from,to,weight}` shape so `topFilesByWeight`, `emptyArchitectureWarning`, `isTestRef`'s dashed-test-node styling and `buildArchitectureDot` all operate on groups with no change, and an unset `depth` returns the input untouched), `groupCollapseWarning(beforeCount, afterCount, depth)` (pure: fires only when a rollup consumed *every* edge — a repo whose files all sit under one directory at `--group-depth 1` — because that blank diagram is the flag's doing, and `emptyArchitectureWarning` would otherwise blame a missing/stale index for it; `main` suppresses the latter when this one fires so the user gets the actionable cause, not two contradictory explanations), `sortSymbolsForEnumeration(symbols)` (pure: orders the enumerated symbol set by `filePath` then `name` before the `--max-symbols` slice — see the enumeration-order note under Known Limitations for why an unsorted slice made a truncated scan non-reproducible), `topFilesByWeight(fileEdges, maxRender)` (pure: ranks files by total in+out edge weight and returns the top `maxRender` as a `Set`, or `null` meaning "no cap" — deliberately a simple weight cutoff, not a connected-component/centrality algorithm), `buildArchitectureDot(fileEdges, { maxRender })` (pure: the architecture-mode analog of `buildDot` — a dedicated function rather than a `buildDot` branch, since the semantics genuinely differ: no root-symbol highlight, no caller/callee direction split, no file-kind dotted-edge concept since every node already IS a file), and `architectureOutputBaseName(repoPath)` (pure: `sanitizeForFilename(path.basename(path.resolve(repoPath)))`, used for the default `--out` filename). `readIndexRows(repoPath)` (I/O: every edge of codegraph's own callee kinds — `calls`, `references`, `imports`, `instantiates`, `navigates`, from its `getCallees` — joined to source and target nodes, one read-only `node:sqlite` query on `/.codegraph/codegraph.db`; `null` means "use the CLI probes": `CODESHOT_INDEX_READ=cli`, no `node:sqlite` (Node 18/20), no index at that path, or any failure such as a schema change or a lock; `node:sqlite`'s ExperimentalWarning is suppressed around the require only), `fileEdgesFromIndexRows(symbols, rows)` (pure: `probeFileEdges`' answer from those rows — a definition is every node sharing the symbol's name, file and qualified name, codegraph's own overload grouping; file-kind targets and targets reached only by `imports` are dropped, the latter possible here because the rows hold every edge, unlike the CLI JSON's one-per-callee; each definition counted once), `probeFileEdges(symbols, repoPath, limit, run)` (returns the file edges plus the names that fell back and the names `--limit` cut; `run` injectable like `probeSymbolCallees`'s, and memoized for repeated names). Unexported CLI plumbing: `enumerateSymbols`, `probeCallsInFile`, `runArchitectureMode` (real I/O, exercised via the CLI-level tests — including a purpose-built fixture repo covering both a cross-file collision (two `handle`s, which must be split) and a same-file one (two `run`s, which must not be), since this repo's own index has no duplicate names to trip either path), and `renderDotToFile` (shared with symbol mode — the write-tempfile/`dot -T`/delete-tempfile tail, previously inline in `main`, extracted once a second call site needed it). + **`--architecture` mode adds:** `unwrapQueryNodes(queryResults)` (pure: unwraps `query`'s `{node, score}` result shape, dropping only a missing `node` — a `"kind":"file"` entry is deliberately KEPT rather than dropped: `codegraph callees ` is a real, working probe against it, and it's the only way to surface calls made from inside a top-level anonymous callback, which codegraph attributes to the enclosing file rather than any named function — see the note on `probeFileEdges` and Known Limitations. Also deliberately does NOT allowlist "callable" kinds like `function`/`method` for the non-file entries — probing a `constant` or `variable` just harmlessly returns an empty `callees` array, which is more robust across languages than maintaining a per-language kind list. The one exception is `"kind":"import"`: an import statement's node never has callee edges of its own (0 of 3,659 on codegraph's repo), but its text probed as a name (`terse.capture`, `../src/mcp/tools`) resolves to some *other* symbol and borrows its callees, so `probeFileEdges` skips import nodes instead of probing them), `symbolBudgetWarning(truncated, budget)` (pure: same shape as `depthBudgetWarning`, fires when `--max-symbols` cut enumeration short), `duplicateNames(symbols)` (pure: the `Set` of names appearing in more than one distinct **file** — counting files rather than symbol occurrences is what keeps same-file collisions off the file-qualified route; the set `probeFileEdges` and `--diff` attribute by file), `parseNodeCalls(out, expectedFile)` (pure: reads the file-qualified trail line out of `codegraph node -f`'s text output into `{name, filePath}` callees; returns `[]` for a recognized symbol that calls nothing, `null` on any of the four untrustworthy responses listed under Known Limitations so the caller falls back rather than under-reporting, and drops a callee whose name equals its own file's basename — the structural stand-in for the `"kind":"file"` filter, since the trail carries no kind), `probeSymbolCallees(s, dupe, repoPath, limit, run)` (one symbol's callees: bare-name `callees --json`, narrowed to the definition whose `id` matches the enumerated symbol's when codegraph reports per-definition results, else for a duplicate name to its own file via `pickDefinitionResult`, else `node -f`, else the union; any answer that isn't the symbol's own definition is shared by every same-named symbol in that file, so `probeFileEdges` counts it once per file rather than once per symbol — otherwise two `run` methods in one file would double every edge weight; exported with `run` injectable so tests replay codegraph output recorded from npm codegraph into `test/fixtures/npm-codegraph/` — the `node -f` fallback's coverage in the codegraph-free test jobs. Re-record with `npm run fixtures:record -- --codegraph `, never by hand; the recorder refuses a build with per-definition JSON, whose output would never reach `node -f`), `architectureTruncationWarning(truncatedNames, limit)` (pure: one aggregated stderr line naming the symbols whose callee fetch `--limit` cut, decided per answer by `readTruncation` on the slice actually used — a matched definition's own `truncated`, else the response's, else the hit-the-limit heuristic; the `node -f` trail is never flagged, it has no `--limit` and `parseNodeCalls` already refuses a cut trail), `parseForbidRule(raw)` / `forbiddenEdges(fileEdges, rules)` / `forbidViolationReport(violations)` / `violatingDrawnKeys(violations, groupDepth)` / `hiddenViolationNote(fileEdges, violatingKeys, maxRender)` (pure: `--forbid`'s parse, per-file match and stderr report — see Configuration), `duplicateNameWarning(unresolvedNames)` (pure: names only the duplicate names `probeFileEdges` could not attribute by file and drew as the bare-name union; see Known Limitations), `aggregateFileEdges(symbolEdges)` (pure: dedupes/sums `{fromFile, toFile}` pairs from every probed symbol into weighted `{from, to, weight}` file edges, dropping self-file edges and any edge missing a real `filePath` on either end — an unresolved external/stdlib callee has no file of its own and would otherwise render as a bogus `""` node), `groupPath(filePath, depth)` (pure: the group a file rolls up into at `--group-depth` — its first `depth` *directory* segments with a trailing slash, so a group reads as a directory rather than a file; a repo-root file has no directory to roll into and is returned unchanged, since grouping it under `""` would merge every root file into one nameless box, and a file shallower than `depth` simply keeps the directories it has, so an over-deep `--group-depth` degrades to per-file output instead of erroring), `rollupFileEdges(fileEdges, depth)` (pure: rewrites both endpoints of every file edge through `groupPath` and re-sums the weights, dropping edges that become self-group — the directory-level restatement of `aggregateFileEdges`'s self-file rule, since once a directory is the unit an intra-directory call is internal structure, not cross-module coupling; returns the same `{from,to,weight}` shape so `topFilesByWeight`, `emptyArchitectureWarning`, `isTestRef`'s dashed-test-node styling and `buildArchitectureDot` all operate on groups with no change, and an unset `depth` returns the input untouched), `groupCollapseWarning(beforeCount, afterCount, depth)` (pure: fires only when a rollup consumed *every* edge — a repo whose files all sit under one directory at `--group-depth 1` — because that blank diagram is the flag's doing, and `emptyArchitectureWarning` would otherwise blame a missing/stale index for it; `main` suppresses the latter when this one fires so the user gets the actionable cause, not two contradictory explanations), `sortSymbolsForEnumeration(symbols)` (pure: orders the enumerated symbol set by `filePath` then `name` before the `--max-symbols` slice — see the enumeration-order note under Known Limitations for why an unsorted slice made a truncated scan non-reproducible), `topFilesByWeight(fileEdges, maxRender)` (pure: ranks files by total in+out edge weight and returns the top `maxRender` as a `Set`, or `null` meaning "no cap" — deliberately a simple weight cutoff, not a connected-component/centrality algorithm), `buildArchitectureDot(fileEdges, { maxRender })` (pure: the architecture-mode analog of `buildDot` — a dedicated function rather than a `buildDot` branch, since the semantics genuinely differ: no root-symbol highlight, no caller/callee direction split, no file-kind dotted-edge concept since every node already IS a file), and `architectureOutputBaseName(repoPath)` (pure: `sanitizeForFilename(path.basename(path.resolve(repoPath)))`, used for the default `--out` filename). `readIndexRows(repoPath)` (I/O: every edge of codegraph's own callee kinds — `calls`, `references`, `imports`, `instantiates`, `navigates`, from its `getCallees` — joined to source and target nodes, one read-only `node:sqlite` query on `/.codegraph/codegraph.db`; `null` means "use the CLI probes": `CODESHOT_INDEX_READ=cli`, no `node:sqlite` (Node 18/20), no index at that path, or any failure such as a schema change or a lock; `node:sqlite`'s ExperimentalWarning is suppressed around the require only), `fileEdgesFromIndexRows(symbols, rows)` (pure: `probeFileEdges`' answer from those rows — a definition is every node sharing the symbol's name, file and qualified name, codegraph's own overload grouping; file-kind targets and targets reached only by `imports` are dropped, the latter possible here because the rows hold every edge, unlike the CLI JSON's one-per-callee; each definition counted once), `probeFileEdges(symbols, repoPath, limit, run)` (returns the file edges plus the names that fell back and the names `--limit` cut; `run` injectable like `probeSymbolCallees`'s, and memoized for repeated names). Unexported CLI plumbing: `enumerateSymbols`, `probeCallsInFile`, `runArchitectureMode` (real I/O, exercised via the CLI-level tests — including a purpose-built fixture repo covering both a cross-file collision (two `handle`s, which must be split) and a same-file one (two `run`s, which must not be), since this repo's own index has no duplicate names to trip either path), and `renderDotToFile` (shared with symbol mode — the write-tempfile/`dot -T`/delete-tempfile tail, previously inline in `main`, extracted once a second call site needed it). **`--diff` mode adds:** `matchRootSymbols(symbols, changedFiles)` (pure: the enumerated symbols whose `filePath` is in `git diff`'s changed set, file-kind and import-kind entries excluded — an import statement's text probed as a name can resolve to another symbol — an exact string match; see Known Limitations), `buildDiffDot(roots, edges, { maxRender, tooltips })` (pure: the multi-root analog of `buildDot` — every root drawn bold and always kept, `maxRender` spent only on the callers/callees around them, edges deduped by `nodeKey` so a root-to-root call found from both ends draws once), and the warning/refusal strings `diffNoChangesWarning`, `diffNoSymbolsWarning`, `diffSymbolBudgetWarning`, `diffEmptyRootsWarning`, `diffTruncationWarning`, `diffDuplicateNameWarning` (names only the colliding roots that fell back to the bare-name union, so it never claims a mitigation that didn't happen), `resolveRootResults(callersResult, calleesResult, filePath)` (pure: applies `pickDefinitionResult` to each direction independently and reports whether either fell back), `pickDefinitionResult(result, key, filePath)` (pure: the slice of a bare-name `callers`/`callees --json` response belonging to the definition(s) in `filePath`, from the per-definition `definitions[]` of a codegraph with upstream #1801, in a shape `readTruncation` reads unchanged; `null` when the response has no `definitions` or none match — see Known Limitations), `diffEmbedRefusal`/`diffEmbedRefusalNoSymbols` and `diffNothingToCheck`/`diffNothingToCheckNoSymbols`, all routed through `diffHandleEmptyRoots(embedFile, check, …)`, which picks warn / refuse / exit-0 for a zero-root diff. Unexported plumbing: `gitDiffFiles` and `runDiffMode` (real I/O). @@ -232,7 +232,7 @@ No environment variables, no config file. All behavior is controlled by CLI argu | `--max-depth-nodes` | `200` (`DEFAULT_NODE_BUDGET`) | Only meaningful with `--depth > 1`: raises or lowers `collectTransitive`'s total-discovered-node safety cap (formerly the fixed, unconfigurable `NODE_BUDGET` constant). Must be a positive integer. Exposed as a flag for the same reason `--max-symbols` below is — a genuinely well-connected symbol at `--depth 3`+ can legitimately need a higher cap to finish, and that's a coverage/speed tradeoff only the caller can make. Its `--architecture` rejection follows `--depth`'s precedent above, not `--max-symbols`'s: like `--depth`, it's rejected only when an *explicit, non-default* value is passed alongside `--architecture` (`codeshot --architecture --max-depth-nodes 200` is a silent no-op, same as `--depth 1`) — unlike `--max-symbols`, which has no `--architecture`-only guard at all and is simply ignored outside that mode. | | `--architecture` | `false` | Switches to whole-repo file-level dependency graph mode instead of a single symbol's trail — see the second pipeline diagram above. Mutually exclusive with the `` positional (rejected if both given); `` becomes optional-and-forbidden rather than required. | | `--group-depth` | unset (one node per file) | `--architecture`-only: rolls each file up into its first N directory segments and draws those as the graph's nodes, summing the weights of every file pair that collapses into the same group pair (`rollupFileEdges`). The answer to the per-file hairball that `--max-render` only half-solves: `--max-render` is lossy at *every* level (the least-busy files are dropped outright, and any edge touching one goes with them), while grouping is lossless at the *module* level specifically — every cross-module call still appears, drawn between directories instead of files. It is not lossless in total: calls that become intra-group are dropped exactly as same-file calls already are, so a grouped diagram's summed weights are legitimately lower (often far lower) than the per-file one's. That difference is the repo's intra-module traffic, and the docs say so, because a user reconciling the two pictures would otherwise read it as a weight-aggregation bug. Composable with `--max-render`, which then caps groups rather than files (`renderTruncationNote` says `groups`). Must be a positive integer. Rejected — not silently ignored — outside `--architecture`, the opposite direction from `--depth`/`--max-depth-nodes` and unlike `--max-symbols`' quiet no-op: symbol mode has no file-level graph to roll up, so passing it there is always a mistake, and a silently-dropped flag reads as "grouping applied" in exactly the diagram you would then trust. Applied *after* probing, never during: probes must stay file-exact (`parseNodeCalls`' `expectedFile` check, `isTestRef`, duplicate-name attribution all key off the real path), so this stays a pure view over the same data rather than a second scan mode. With `--embed` it claims its own marker id (`codeshot:arch-d`) and default image name rather than the plain `codeshot:arch` — the per-file and per-module pictures answer different questions, so embedding one must not silently overwrite the other's committed block; unset `--group-depth` keeps the original id, so existing docs are unaffected. | -| `--forbid` | unset (no rules) | `--architecture`-only, repeatable: `FROM->TO` architecture rules (`parseForbidRule`; a side ending in `/` is a directory prefix, any other side one exact file, a leading `./` is stripped). `forbiddenEdges` checks them against the per-file edges from `aggregateFileEdges`, *before* `rollupFileEdges` and `--max-render`: those are view choices, and a rule must not pass because the view hid the edge. Each violation is printed by `forbidViolationReport` (heaviest first, with weight and rule); the diagram is still rendered and embedded, then the process exits 1. Malformed rules and use outside `--architecture` are rejected before any codegraph work. Exact/prefix matching was chosen over globs to keep slice 1 predictable. | +| `--forbid` | unset (no rules) | `--architecture`-only, repeatable: `FROM->TO` architecture rules (`parseForbidRule`; a side ending in `/` is a directory prefix, any other side one exact file, a leading `./` is stripped). `forbiddenEdges` checks them against the per-file edges from `aggregateFileEdges`, *before* `rollupFileEdges` and `--max-render`: those are view choices, and a rule must not pass because the view hid the edge. Each violation is printed by `forbidViolationReport` (heaviest first, with weight and rule) and drawn red (`#dc2626`, penwidth 1.8 — the one loud color in the muted house style) by `buildArchitectureDot`'s `violating` option; `violatingDrawnKeys` maps a per-file violation onto the drawn edge, the directory edge under `--group-depth` (one that rolls into a single group isn't drawn). `--max-render` is not overridden to force a violating edge into the picture; `hiddenViolationNote` counts the ones it leaves out, since the exit code already fails the run. With no violations the DOT is byte-identical to a run without `--forbid`, and `svgStructure` ignores colour, so `--check` is unaffected either way. The diagram is still rendered and embedded, then the process exits 1. Malformed rules and use outside `--architecture` are rejected before any codegraph work. Exact/prefix matching was chosen over globs to keep slice 1 predictable. | | `--max-symbols` | `500` | `--architecture`: caps how many enumerated symbols get probed, exposed as a flag for the same reason `--max-depth-nodes` above is — `--architecture` is a multi-minute O(symbols) sequential scan by nature (confirmed: 500 symbols took over 5 minutes against a real ~1,900-node repo), so users legitimately need to trade coverage for speed themselves rather than wait on a hidden safety net tuned for an already-fast operation. Must be a positive integer. `symbolBudgetWarning` prints a stderr note when this cuts enumeration short. **`--diff`/`--diff-ref` reuses the same flag for a differently-scoped budget**: it caps how many *changed, matched* symbols get probed for callers/callees (two calls each), not how many of the whole repo get enumerated — `enumerateAllSymbols` (the untruncated sibling of `enumerateSymbols`) always fetches the full index first so filtering down to the changed files never loses a symbol to this cap before matching happens; `diffSymbolBudgetWarning` prints the diff-scoped stderr note. | | `--diff` | `false` | Switches to diff-scoped mode: diagrams the symbols codegraph attributes to files `git diff` (working tree vs `HEAD`, no ref) reports changed, each drawn as a bold root with its direct callers/callees fanned out — see `buildDiffDot`/`runDiffMode`. `matchRootSymbols` excludes codegraph's `"kind":"file"` and `"kind":"import"` index entries from becoming roots (an import's text probed as a name can resolve to another symbol and borrow its edges) — otherwise a changed file itself (not just its symbols) would be drawn as a bold root, since `enumerateAllSymbols` deliberately keeps those entries for `--architecture`'s sake. Mutually exclusive with `--architecture` and the `` positional. `--depth`/`--max-depth-nodes`/`--group-depth` have no meaning here (no multi-hop or file-rollup concept) and are rejected if passed. A diff with zero changed files skips the full-index `enumerateAllSymbols` scan entirely (it could only ever match nothing); with `--embed` (and not `--check`, which never writes and so has nothing to protect — see `diffHandleEmptyRoots`) it refuses outright rather than rendering a blank diagram over a real, committed one — see `diffEmbedRefusal`/`diffEmbedRefusalNoSymbols` (the latter for the same zero-roots outcome when real changed files match no indexed symbol). Any changed root with no callers/callees still draws (a lone box), and a root whose callers/callees fetch was cut off by `--limit` (decided by `readTruncation`, as in symbol mode), reported once in aggregate via `diffEmptyRootsWarning`/`diffTruncationWarning` respectively rather than per-root, to avoid stderr noise on a large diff. | | `--diff-ref` | unset | Implies `--diff`; gives `gitDiffFiles` an explicit ref/range (e.g. `origin/main...HEAD`) instead of the default `git diff --name-only -z --end-of-options HEAD` (working tree — staged and unstaged — vs `HEAD`; deliberately NOT bare `git diff`, which diffs against the index and silently misses fully-staged changes). `--end-of-options` closes off an argv-injection risk: without it, a `--diff-ref` value starting with `-` (e.g. templated from untrusted CI input) is parsed by git as a flag instead of a revision — confirmed exploitable, the same class of risk `resolveSymbol`'s `--` guards against for `codegraph`, just using git's own equivalent flag since a bare `--` there would mark `ref` as a pathspec instead of a revision. `-z` gives NUL-terminated, unquoted path output, sidestepping `core.quotePath`'s default C-style octal-escaping of non-ASCII filenames (which the file's earlier line-based parsing would have left mangled and unmatchable against codegraph's `filePath`, rather than decoded). The ref form is what to use for `--embed --check` in CI, where there is no working tree to diff against and the default would make the check flap for reasons unrelated to the code. Must not be empty if given. Also suffixes `--diff`'s `--embed` marker id and default output filename (`codeshot:diff-` / `codeshot-diff--.`) so two differently-scoped diagrams can be embedded in the same doc without colliding — same reasoning as `--group-depth`'s `arch-d` marker below. | diff --git a/USAGE.md b/USAGE.md index 870efa0..3268c87 100644 --- a/USAGE.md +++ b/USAGE.md @@ -86,8 +86,10 @@ codeshot --architecture --path . --forbid 'src/ui/->src/db/' --out arch.svg --fo A side ending in `/` is a directory (every file under it); anything else is one exact file. Repeat `--forbid` for more rules. If any call crosses a forbidden -line, Codeshot lists each offending file edge and its weight, still writes the -diagram, and exits 1 — so a CI step fails and the image shows where. Rules are +line, Codeshot lists each offending file edge and its weight, draws it red in +the diagram (under `--group-depth`, the directory edge it rolls up into), and +exits 1 — so a CI step fails and the image shows where. Without a broken rule +nothing is red and the diagram is unchanged. Rules are checked on every per-file edge, even ones `--group-depth` merges or `--max-render` leaves out of the picture. They can only see calls the scan found, so raise `--limit`/`--max-symbols` if Codeshot warns the scan was cut. diff --git a/render/callgraph.js b/render/callgraph.js index 8e24098..d0cbb38 100755 --- a/render/callgraph.js +++ b/render/callgraph.js @@ -738,7 +738,37 @@ function topFilesByWeight(fileEdges, maxRender) { return new Set(ranked.map(([file]) => file)); } -function buildArchitectureDot(fileEdges, { maxRender } = {}) { +// The key a drawn edge and a --forbid violation share, so buildArchitectureDot +// can tell which drawn edges break a rule. Pure. +function drawnEdgeKey(from, to) { + return `${from}\0${to}`; +} + +// The drawn edges (after --group-depth) that carry a --forbid violation: a +// violation is per file, so under --group-depth it marks the directory edge it +// rolls up into. One that rolls up into a single group isn't drawn at all. Pure. +function violatingDrawnKeys(violations, groupDepth) { + const at = f => (Number.isFinite(groupDepth) ? groupPath(f, groupDepth) : f); + const keys = new Set(); + for (const v of violations || []) { + const from = at(v.from); + const to = at(v.to); + if (from !== to) keys.add(drawnEdgeKey(from, to)); + } + return keys; +} + +// How many violating drawn edges --max-render leaves out of the picture. The +// run still fails on them; this only keeps the image from reading as clean. Pure. +function hiddenViolationNote(fileEdges, violatingKeys, maxRender) { + const keep = topFilesByWeight(fileEdges, maxRender); + if (!keep || !violatingKeys || !violatingKeys.size) return null; + const hidden = fileEdges.filter(e => violatingKeys.has(drawnEdgeKey(e.from, e.to)) && !(keep.has(e.from) && keep.has(e.to))).length; + if (!hidden) return null; + return `codeshot: ${hidden} edge(s) that break a --forbid rule fall outside --max-render ${maxRender} and aren't drawn; they're listed above.`; +} + +function buildArchitectureDot(fileEdges, { maxRender, violating } = {}) { const esc = s => String(s).replace(/"/g, '\\"'); const keep = topFilesByWeight(fileEdges, maxRender); const kept = keep ? fileEdges.filter(e => keep.has(e.from) && keep.has(e.to)) : fileEdges; @@ -760,7 +790,11 @@ function buildArchitectureDot(fileEdges, { maxRender } = {}) { lines.push(` "${esc(file)}"${attrs};`); } for (const e of kept) { - lines.push(` "${esc(e.from)}" -> "${esc(e.to)}" [label="${e.weight}"];`); + // A --forbid violation is the one loud color in the muted house style. + const alarm = violating && violating.has(drawnEdgeKey(e.from, e.to)) + ? ', color="#dc2626", fontcolor="#dc2626", penwidth=1.8' + : ''; + lines.push(` "${esc(e.from)}" -> "${esc(e.to)}" [label="${e.weight}"${alarm}];`); } lines.push('}'); return lines.join('\n'); @@ -1147,7 +1181,11 @@ async function runArchitectureMode(repoPath, { limit, maxSymbols, maxRender, gro const note = renderTruncationNote(groupDepth ? 'groups' : 'files', totalFiles, maxRender); if (note) console.error(note); - return { dot: buildArchitectureDot(fileEdges, { maxRender }), violations }; + const violating = violatingDrawnKeys(violations, groupDepth); + const hiddenNote = hiddenViolationNote(fileEdges, violating, maxRender); + if (hiddenNote) console.error(hiddenNote); + + return { dot: buildArchitectureDot(fileEdges, { maxRender, violating }), violations }; } // --- --diff mode: call graph scoped to a set of changed files ------------ @@ -2120,6 +2158,6 @@ module.exports = { emptyGraphWarning, emptyArchitectureWarning, matchNotInitialized, argRepoPath, parseCodegraphOutput, matchRootSymbols, diffNoChangesWarning, diffNoSymbolsWarning, diffSymbolBudgetWarning, buildDiffDot, - diffEmbedRefusal, diffEmptyRootsWarning, diffEmbedRefusalNoSymbols, diffDuplicateNameWarning, pickDefinitionResult, resolveRootResults, probeSymbolCallees, parseForbidRule, forbiddenEdges, forbidViolationReport, fileEdgesFromIndexRows, readIndexRows, probeFileEdges, architectureTruncationWarning, + diffEmbedRefusal, diffEmptyRootsWarning, diffEmbedRefusalNoSymbols, diffDuplicateNameWarning, pickDefinitionResult, resolveRootResults, probeSymbolCallees, parseForbidRule, forbiddenEdges, forbidViolationReport, violatingDrawnKeys, hiddenViolationNote, fileEdgesFromIndexRows, readIndexRows, probeFileEdges, architectureTruncationWarning, diffHandleEmptyRoots, diffTruncationWarning, nodeKey, diffNothingToCheck, diffNothingToCheckNoSymbols, }; diff --git a/test/run.js b/test/run.js index 630641e..4190a04 100644 --- a/test/run.js +++ b/test/run.js @@ -13,7 +13,7 @@ const { emptyGraphWarning, emptyArchitectureWarning, matchNotInitialized, argRepoPath, parseCodegraphOutput, matchRootSymbols, diffNoChangesWarning, diffNoSymbolsWarning, diffSymbolBudgetWarning, buildDiffDot, - diffEmbedRefusal, diffEmptyRootsWarning, diffEmbedRefusalNoSymbols, diffDuplicateNameWarning, pickDefinitionResult, resolveRootResults, probeSymbolCallees, parseForbidRule, forbiddenEdges, forbidViolationReport, fileEdgesFromIndexRows, readIndexRows, probeFileEdges, architectureTruncationWarning, + diffEmbedRefusal, diffEmptyRootsWarning, diffEmbedRefusalNoSymbols, diffDuplicateNameWarning, pickDefinitionResult, resolveRootResults, probeSymbolCallees, parseForbidRule, forbiddenEdges, forbidViolationReport, violatingDrawnKeys, hiddenViolationNote, fileEdgesFromIndexRows, readIndexRows, probeFileEdges, architectureTruncationWarning, diffHandleEmptyRoots, diffTruncationWarning, nodeKey, diffNothingToCheck, diffNothingToCheckNoSymbols, isBlankDot, blankEmbedRefusal, } = require('../render/callgraph.js'); @@ -595,6 +595,35 @@ test('forbiddenEdges matches a trailing-slash side as a directory and any other assert.deepStrictEqual(forbiddenEdges(edges, []), []); }); +test('buildArchitectureDot draws only the violating edges red, and nothing changes without violations', () => { + const edges = [{ from: 'ui/page.js', to: 'db/query.js', weight: 2 }, { from: 'ui/page.js', to: 'core/api.js', weight: 1 }]; + const plain = buildArchitectureDot(edges); + assert.strictEqual(buildArchitectureDot(edges, { violating: new Set() }), plain, 'no violations must render byte-identically'); + assert.doesNotMatch(plain, /#dc2626/); + const red = buildArchitectureDot(edges, { violating: violatingDrawnKeys([{ from: 'ui/page.js', to: 'db/query.js' }]) }); + assert.match(red, /"ui\/page\.js" -> "db\/query\.js" \[label="2", color="#dc2626", fontcolor="#dc2626", penwidth=1\.8\];/); + assert.match(red, /"ui\/page\.js" -> "core\/api\.js" \[label="1"\];/); +}); + +test('violatingDrawnKeys marks the directory edge a violation rolls up into, and drops one that rolls into a single group', () => { + const v = [{ from: 'src/ui/page.js', to: 'src/db/query.js' }, { from: 'lib/a.js', to: 'test/b.js' }]; + assert.deepStrictEqual([...violatingDrawnKeys(v)].map(k => k.split('\0')), [['src/ui/page.js', 'src/db/query.js'], ['lib/a.js', 'test/b.js']]); + assert.deepStrictEqual([...violatingDrawnKeys(v, 1)].map(k => k.split('\0')), [['lib/', 'test/']], 'src/ui -> src/db is one group at depth 1'); + assert.deepStrictEqual([...violatingDrawnKeys(v, 2)].map(k => k.split('\0')), [['src/ui/', 'src/db/'], ['lib/', 'test/']]); +}); + +test('hiddenViolationNote counts violating edges --max-render leaves out, and is silent otherwise', () => { + const edges = [ + { from: 'a.js', to: 'b.js', weight: 9 }, + { from: 'c.js', to: 'd.js', weight: 1 }, + ]; + const violating = violatingDrawnKeys([{ from: 'c.js', to: 'd.js' }]); + assert.match(hiddenViolationNote(edges, violating, 2), /^codeshot: 1 edge\(s\) that break a --forbid rule fall outside --max-render 2/); + assert.strictEqual(hiddenViolationNote(edges, violating, 4), null, 'all four files fit'); + assert.strictEqual(hiddenViolationNote(edges, violating, undefined), null, 'no cap'); + assert.strictEqual(hiddenViolationNote(edges, new Set(), 2), null, 'no violations'); +}); + test('forbidViolationReport lists each violating edge with its weight and rule, and is silent with none', () => { assert.strictEqual(forbidViolationReport([]), null); const r = forbidViolationReport([{ rule: 'a/->b/', from: 'a/x.js', to: 'b/y.js', weight: 3 }]); @@ -634,7 +663,7 @@ test('--architecture --forbid fails the run on a forbidden edge, still writes th const broken = spawnSync('node', [cli, '--architecture', '--path', dir, '--out', out, '--format', 'dot', '--forbid', 'ui/->db/'], { encoding: 'utf8', timeout: 180000 }); assert.strictEqual(broken.status, 1, `expected exit 1, got ${broken.status}: ${broken.stderr}`); assert.match(broken.stderr, /ui\/page\.js -> db\/query\.js \(weight 1\) breaks --forbid 'ui\/->db\/'/); - assert.match(fs.readFileSync(out, 'utf8'), /"ui\/page\.js" -> "db\/query\.js"/, 'the diagram is still written as evidence'); + assert.match(fs.readFileSync(out, 'utf8'), /"ui\/page\.js" -> "db\/query\.js"\s*\[[^\]]*color="#dc2626"/, 'the diagram is still written, with the violation drawn red'); // --group-depth 1 draws only ui/ -> db/, with no file names left in the // graph; an exact-file rule still fires because rules read per-file edges. @@ -643,6 +672,7 @@ test('--architecture --forbid fails the run on a forbidden edge, still writes th const clean = spawnSync('node', [cli, '--architecture', '--path', dir, '--out', out, '--format', 'dot', '--forbid', 'db/->ui/'], { encoding: 'utf8', timeout: 180000 }); assert.strictEqual(clean.status, 0, `expected exit 0 with no violations: ${clean.stderr}`); + assert.doesNotMatch(fs.readFileSync(out, 'utf8'), /#dc2626/, 'nothing is red when no rule is broken'); assert.doesNotMatch(clean.stderr, /--forbid rule/); } finally { fs.rmSync(dir, { recursive: true, force: true });