From d97707095ee91b26d50c6e49c69787a0d30da1d3 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Wed, 5 Aug 2026 13:12:32 -0400 Subject: [PATCH 1/2] Fix --architecture mode's fabricated edge; bump codegraph to 1.5.0 The committed architecture diagram had a spurious render/callgraph.js -> test/run.js edge. Root cause was a codegraph 1.4.1 bug (fixed upstream in 1.5.0 by its LITERAL_RECEIVER_TYPES change): a call on a literal receiver, e.g. /regex/.test(x), fell through to bare-name matching against any same-named project symbol instead of being recognized as a builtin. render/callgraph.js uses .test() heavily (isTestRef, matchNotInitialized, argument parsing); its own test runner happens to define a `test(name, fn)` helper, so those builtin calls were fabricating an edge into test/run.js by name collision. Confirmed by reproducing it directly against a locally-installed 1.4.1 and comparing to a fresh 1.5.0 index, side by side. Fixing the diagram surfaced a real, separate, pre-existing gap: --architecture mode has never captured the actual dependency (test files calling into production code from inside anonymous callbacks, e.g. `test('...', () => { realCall() })`) in either codegraph version, because filterCallableSymbols excluded file-kind nodes from probing. codegraph attributes such calls to the enclosing file when no named function contains them, and `codegraph callees ` is a real, working query against that file node -- codeshot just never issued it. Renamed to unwrapQueryNodes and stopped dropping file-kind entries, so the file node itself gets probed too. The regenerated diagram now shows the real edge (test/run.js -> render/callgraph.js) in the correct direction. CI's codegraph pin moves from 1.4.1 to 1.5.0 so --check validates against the version that actually behaves correctly, instead of faithfully re-validating a fabricated artifact on every push. --- .github/workflows/ci.yml | 7 ++++++- README.md | 2 +- TECHNICAL.md | 7 ++++--- docs/architecture.svg | 34 +++++++++++++++++----------------- render/callgraph.js | 27 ++++++++++++++++++--------- test/run.js | 11 ++++++----- 6 files changed, 52 insertions(+), 36 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 37d1c14..b6c2530 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,8 +47,13 @@ jobs: run: sudo apt-get update && sudo apt-get install -y graphviz # The index codeshot reads. Pinned (repo convention: pin dependency versions). + # Floor is 1.5.0, not just latest-at-time-of-writing: 1.4.1 has a real call- + # resolution bug (fixed by codegraph's LITERAL_RECEIVER_TYPES change) that + # silently fabricates cross-file edges from unrelated builtin calls whose + # name happens to collide with a real project symbol — confirmed on this + # exact repo (see README.md#install and TECHNICAL.md's Known Limitations). - name: Install codegraph - run: npm install -g @colbymchenry/codegraph@1.4.1 + run: npm install -g @colbymchenry/codegraph@1.5.0 # `init` builds the initial index in a fresh checkout (`index` rebuilds an # existing one and errors if the repo was never initialized — which a CI diff --git a/README.md b/README.md index 8bc298f..e45636a 100644 --- a/README.md +++ b/README.md @@ -24,7 +24,7 @@ npm install -g github:inth3shadows/codeshot **Requirements:** - Node.js ≥ 18 -- [`codegraph`](https://github.com/colbymchenry/codegraph) CLI on PATH, with the target repo indexed (`codegraph init`) +- [`codegraph`](https://github.com/colbymchenry/codegraph) CLI on PATH, **1.5.0 or later**, with the target repo indexed (`codegraph init`). `--architecture` mode needs 1.5.0+ specifically: earlier versions have a call-resolution bug (fixed by codegraph's `LITERAL_RECEIVER_TYPES` fix) that can silently fabricate cross-file edges from unrelated builtin method calls (e.g. `/regex/.test(x)`) whose name happens to collide with a real project symbol. - `graphviz` (`dot`) on PATH — `brew install graphviz` / `apt install graphviz` Codeshot checks for both on startup and tells you exactly what's missing and how to install it. diff --git a/TECHNICAL.md b/TECHNICAL.md index 6c41616..ff46750 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -67,7 +67,7 @@ enumerateSymbols(repoPath, maxSymbols) --- runCodegraph(['query', '--path', repo --- an empty-string query WITHOUT --limit silently caps around 50 results regardless of repo size; passing a large --limit instead makes codegraph return everything it has (see External Dependencies) --- the real cap is - applied client-side via filterCallableSymbols + a slice to maxSymbols + applied client-side via unwrapQueryNodes + a slice to maxSymbols | v symbolBudgetWarning(...) --- warns on stderr if enumeration was capped by --max-symbols @@ -120,7 +120,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)` (pure: returns a warning string if `results.length` hit `limit` exactly, else `null` — the *fetch* cap), `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 the internal node budget 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`), and `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). Everything else (`requireOnPath`, `runCodegraph`, `parseCodegraphOutput`, `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:** `filterCallableSymbols(queryResults)` (pure: unwraps `query`'s `{node, score}` result shape and drops `kind === 'file'` entries — a file object isn't a callable symbol, so it's never probed; deliberately does NOT allowlist "callable" kinds like `function`/`method` — 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), `symbolBudgetWarning(truncated, budget)` (pure: same shape as `depthBudgetWarning`, fires when `--max-symbols` cut enumeration short), `duplicateNameWarning(symbols)` (pure: warns — with a few real examples — when any probed symbol name appears in more than one file, since `codegraph callees ` has no way to disambiguate which file's symbol it means; 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), `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). Unexported CLI plumbing: `enumerateSymbols`, `probeFileEdges`, `runArchitectureMode` (real I/O, only exercised via the CLI-level test), 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), `symbolBudgetWarning(truncated, budget)` (pure: same shape as `depthBudgetWarning`, fires when `--max-symbols` cut enumeration short), `duplicateNameWarning(symbols)` (pure: warns — with a few real examples — when any probed symbol name appears in more than one file, since `codegraph callees ` has no way to disambiguate which file's symbol it means; 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), `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). Unexported CLI plumbing: `enumerateSymbols`, `probeFileEdges`, `runArchitectureMode` (real I/O, only exercised via the CLI-level test), 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). - **`test/run.js`** — assertion-based test suite (Node's built-in `assert`, no framework) covering all of the pure functions above directly. Run via `npm test`. - **`package.json`** — declares the `codeshot` bin pointing at `render/callgraph.js`, and the `test` script. - **`.runechoguardignore`** — false-positive suppression list for the RunEcho pre-commit symbol-resolution guard (a local hook, not part of codeshot itself). Bare-call identifiers the guard can't resolve (e.g. Node builtins passed as function parameters) get listed here instead of disabling the guard. @@ -172,7 +172,8 @@ There is no service to restart, no rollback beyond `npm uninstall -g codeshot` / - `--depth`'s `NODE_BUDGET` (200) is a fixed internal constant, not exposed as a flag — a genuinely well-connected symbol at `--depth 3`+ in a large repo can still hit it and produce an incomplete graph (with a stderr warning), and there's currently no way to raise the cap short of editing the constant. - `--depth`'s traversal treats `--limit`/`--max-render` as global, not per-hop — a symbol with a huge fan-out at hop 2 fetches up to `--limit` results for *each* newly discovered node at that hop, which is the main driver of `NODE_BUDGET` exhaustion; there's no independent per-hop limit to trade off against total node count. - A cyclic call graph (recursion, or A and B calling each other) can cause `--depth`'s transitive traversal to rediscover the root symbol or an already-drawn depth-1 node as a "from"/"to" endpoint of a deeper edge. This is harmless (graphviz just draws the extra edge; `dedupeEdges` still collapses exact repeats) but can occasionally show what looks like a redundant edge back into an already-visible node. -- **`--architecture` mode's edges can be misattributed to the wrong file when symbol names collide.** `codegraph callees ` takes a bare name with no way to disambiguate which file's symbol is meant (unlike `codegraph node -f `, which does support this). In symbol mode this ambiguity affects exactly one user-chosen name — a corner case. In `--architecture` mode, Codeshot probes `codegraph callees` for every enumerated symbol in the whole repo, where generically-named methods (`render`, `init`, `get`, `run`, `String`) existing in more than one file is common, not rare, in most real codebases (confirmed: 12 duplicate names out of 500 probed symbols on a real ~1,900-node Go repo). `duplicateNameWarning` surfaces this on stderr with real examples from the current run, but Codeshot has no way to fix the underlying ambiguity — same as the other `codegraph` indexing gaps documented above, it can only draw what `codegraph` returns. +- **`--architecture` mode's edges can be misattributed to the wrong file when symbol names collide.** `codegraph callees ` takes a bare name with no way to disambiguate which file's symbol is meant (unlike `codegraph node -f `, which does support this). In symbol mode this ambiguity affects exactly one user-chosen name — a corner case. In `--architecture` mode, Codeshot probes `codegraph callees` for every enumerated symbol in the whole repo, where generically-named methods (`render`, `init`, `get`, `run`, `String`) existing in more than one file is common, not rare, in most real codebases (confirmed: 12 duplicate names out of 500 probed symbols on a real ~1,900-node Go repo). Since `unwrapQueryNodes` also keeps file nodes in the probed set (see below), the same ambiguity now applies to file basenames too — two files named `index.js` in different directories are indistinguishable to a bare-name `callees` probe. `duplicateNameWarning` surfaces both cases on stderr with real examples from the current run, but Codeshot has no way to fix the underlying ambiguity — same as the other `codegraph` indexing gaps documented above, it can only draw what `codegraph` returns. +- **`--architecture` mode probes file nodes' `callees`, not just named symbols', specifically to catch calls made from inside a top-level anonymous callback** (e.g. `test('...', () => { realCall() })` — a common pattern in test suites, including this repo's own `test/run.js`). codegraph attributes such a call to the enclosing file, not any named function, since no named function contains it; without probing the file node itself, `--architecture` mode would be structurally blind to that entire category of real cross-file dependency. This was confirmed against a real regression-turned-non-regression in codegraph itself: a codegraph 1.4.1 bug (fixed in 1.5.0, see `git log` for `LITERAL_RECEIVER_TYPES` in codegraph's history) briefly caused a *different*, spurious file-node-unrelated edge to appear in this project's own diagram — a call like `/regex/.test(x)` in `render/callgraph.js` got bare-name-matched to `test/run.js`'s own `test(name, fn)` helper purely by name collision. That edge is gone as of codegraph 1.5.0+ (correctly — it was never real); probing file nodes is what makes the *actual* dependency (`test/run.js` calling into `render/callgraph.js`) visible in its place. - **`--architecture` mode's enumeration query (`codegraph query --json --limit -- ''`) has confirmed, inconsistent `--limit` behavior worth knowing before trusting it.** Without `--limit`, an empty-string query silently caps around 50 results regardless of actual repo size (confirmed on a real 1,870-node index). Passing a large `--limit` (confirmed with both 500 and 2000 against that same index) instead returns *every* result codegraph has — more than the requested number, not capped at it. Codeshot works around this by always passing a very large `--limit` to force the "return everything" behavior, then applying the real `--max-symbols` cap client-side — but the *order* codegraph returns results in in that case is unknown (untested whether it's insertion order, alphabetical, ID-based, or something else), so on a repo larger than `--max-symbols`, the kept subset should not be assumed to sample evenly across the whole repo — it could be clustered by file, directory, or however codegraph happens to have stored them. diff --git a/docs/architecture.svg b/docs/architecture.svg index 8fdc8f6..6c3f556 100644 --- a/docs/architecture.svg +++ b/docs/architecture.svg @@ -1,32 +1,32 @@ - - + architecture - - + + -render/callgraph.js - -render/callgraph.js +test/run.js + +test/run.js - + -test/run.js - -test/run.js +render/callgraph.js + +render/callgraph.js - + -render/callgraph.js->test/run.js - - -2 +test/run.js->render/callgraph.js + + +29 diff --git a/render/callgraph.js b/render/callgraph.js index 8bff508..8569b3f 100755 --- a/render/callgraph.js +++ b/render/callgraph.js @@ -462,13 +462,19 @@ async function collectTransitive(direction, repoPath, limit, maxDepth, seedNodes // --- --architecture mode: whole-repo file-level dependency graph --------- // `query ""` results come back as `{ node: {...}, score }`, unlike -// callers/callees' flat `{ name, kind, filePath }` shape. A "kind":"file" -// entry is the file object itself, not a callable symbol — codegraph has no -// `callees ` concept, so it's dropped rather than probed. -function filterCallableSymbols(queryResults) { - return (queryResults || []) - .map(r => r.node) - .filter(n => n && n.kind !== 'file'); +// callers/callees' flat `{ name, kind, filePath }` shape — this just unwraps +// that envelope. A "kind":"file" entry (the file itself, not a function) is +// KEPT, not dropped: `codegraph callees ` is a real, working +// query against it (verified against a live index), and it's the only way to +// see calls made from inside a top-level anonymous callback — codegraph +// attributes those to the enclosing file node, since no named function +// contains them (e.g. this repo's own test/run.js: every assertion inside a +// `test('...', () => { ... })` body calls into render/callgraph.js this way, +// and none of those calls are reachable from any named symbol codeshot could +// otherwise probe). Without this, --architecture mode is structurally blind +// to that whole category of cross-file call. +function unwrapQueryNodes(queryResults) { + return (queryResults || []).map(r => r.node).filter(Boolean); } function symbolBudgetWarning(truncated, budget) { @@ -490,6 +496,9 @@ function emptyArchitectureWarning(fileEdges) { // (unlike `codegraph node -f`), so two same-named symbols in different files // are genuinely ambiguous to a `codegraph callees ` probe — a real risk // at --architecture's scale (probing hundreds of names), not a corner case. +// Since unwrapQueryNodes now keeps file nodes in the probed set too, this also +// catches two files sharing a basename in different directories (e.g. two +// `index.js`) — the same ambiguity, just on a file's own name. function duplicateNameWarning(symbols) { const counts = new Map(); for (const s of symbols || []) counts.set(s.name, (counts.get(s.name) || 0) + 1); @@ -571,7 +580,7 @@ function architectureOutputBaseName(repoPath) { const ENUMERATION_QUERY_LIMIT = 100000; async function enumerateSymbols(repoPath, maxSymbols) { const results = await runCodegraph(['query', '--path', repoPath, '--json', '--limit', String(ENUMERATION_QUERY_LIMIT), '--', '']); - const symbols = filterCallableSymbols(results); + const symbols = unwrapQueryNodes(results); const truncated = symbols.length > maxSymbols; return { symbols: symbols.slice(0, maxSymbols), truncated }; } @@ -1019,7 +1028,7 @@ if (require.main === module) { module.exports = { buildDot, nodeIdentities, isTestRef, truncationWarning, dedupeNodes, renderTruncationNote, dedupeEdges, depthColor, depthBudgetWarning, allocateRenderBudget, formatMismatchWarning, matchSymbolNotFound, - filterCallableSymbols, symbolBudgetWarning, duplicateNameWarning, aggregateFileEdges, + unwrapQueryNodes, symbolBudgetWarning, duplicateNameWarning, aggregateFileEdges, topFilesByWeight, buildArchitectureDot, architectureOutputBaseName, applyEmbed, embedMarkers, embedRelLink, parseUnresolvedRefs, svgStructure, decodeXmlEntities, diff --git a/test/run.js b/test/run.js index 58c82ba..11e4c54 100644 --- a/test/run.js +++ b/test/run.js @@ -5,7 +5,7 @@ const assert = require('assert'); const { buildDot, nodeIdentities, isTestRef, truncationWarning, dedupeNodes, renderTruncationNote, dedupeEdges, depthColor, depthBudgetWarning, allocateRenderBudget, formatMismatchWarning, matchSymbolNotFound, - filterCallableSymbols, symbolBudgetWarning, duplicateNameWarning, aggregateFileEdges, + unwrapQueryNodes, symbolBudgetWarning, duplicateNameWarning, aggregateFileEdges, topFilesByWeight, buildArchitectureDot, architectureOutputBaseName, applyEmbed, embedMarkers, embedRelLink, parseUnresolvedRefs, svgStructure, decodeXmlEntities, @@ -537,15 +537,16 @@ test('CLI resolves a fuzzy/partial query to its canonical name for the rendered // --- --architecture mode --------------------------------------------- -test('filterCallableSymbols unwraps .node and drops kind:file entries', () => { +test('unwrapQueryNodes unwraps .node and keeps kind:file entries (probed for anonymous-callback calls)', () => { const results = [ { node: { name: 'Foo', kind: 'function', filePath: 'a.js' } }, { node: { name: 'a.js', kind: 'file', filePath: 'a.js' } }, { node: { name: 'BAR', kind: 'constant', filePath: 'b.js' } }, + { node: null }, ]; - const symbols = filterCallableSymbols(results); - assert.strictEqual(symbols.length, 2); - assert.deepStrictEqual(symbols.map(s => s.name), ['Foo', 'BAR']); + const symbols = unwrapQueryNodes(results); + assert.strictEqual(symbols.length, 3); + assert.deepStrictEqual(symbols.map(s => s.name), ['Foo', 'a.js', 'BAR']); }); test('symbolBudgetWarning fires only when enumeration was truncated', () => { From 50af41a37e2cf1f6fbcd9f30fa2a7571ed607099 Mon Sep 17 00:00:00 2001 From: Eric Minish Date: Wed, 5 Aug 2026 14:27:34 -0400 Subject: [PATCH 2/2] Fix file-node probing: don't count kind:file callees, guard bad names Two real gaps from code review: - probeFileEdges was counting "kind":"file" callees (unresolved module-level references) as full-weight real edges -- the exact fabricated-edge failure this branch exists to fix, just via a different mechanism. Symbol mode already treats these as unverified (dotted/gray, not a real call edge); architecture mode now skips them too instead of aggregating them in. Verified empirically against this repo's own index (0 kind:file callees among 100 probed currently, but the guard is real and cheap). - unwrapQueryNodes now also drops any entry with no usable string name, not just a missing .node -- probeFileEdges passes a symbol's name straight into a codegraph subprocess's argv, where an undefined/empty value would throw before codegraph gets a chance to report its own "not found", bypassing the fatal:false resilience meant to let one bad index entry skip past without aborting the whole scan. Also verified (empirically, not just by inspection) that probing a file node's callees doesn't double-count calls already reachable from a named function in the same file: 0 overlaps found across all 68 probed symbols on this repo's real index. Documented the two remaining real, un-mitigated costs (max-symbols budget now shared between files and real symbols in an unspecified order; probe count and wall-clock grow with file count) in TECHNICAL.md rather than adding scope to fix them here. --- TECHNICAL.md | 2 +- render/callgraph.js | 18 ++++++++++++++++-- test/run.js | 4 +++- 3 files changed, 20 insertions(+), 4 deletions(-) diff --git a/TECHNICAL.md b/TECHNICAL.md index ff46750..f9791d6 100644 --- a/TECHNICAL.md +++ b/TECHNICAL.md @@ -173,7 +173,7 @@ There is no service to restart, no rollback beyond `npm uninstall -g codeshot` / - `--depth`'s traversal treats `--limit`/`--max-render` as global, not per-hop — a symbol with a huge fan-out at hop 2 fetches up to `--limit` results for *each* newly discovered node at that hop, which is the main driver of `NODE_BUDGET` exhaustion; there's no independent per-hop limit to trade off against total node count. - A cyclic call graph (recursion, or A and B calling each other) can cause `--depth`'s transitive traversal to rediscover the root symbol or an already-drawn depth-1 node as a "from"/"to" endpoint of a deeper edge. This is harmless (graphviz just draws the extra edge; `dedupeEdges` still collapses exact repeats) but can occasionally show what looks like a redundant edge back into an already-visible node. - **`--architecture` mode's edges can be misattributed to the wrong file when symbol names collide.** `codegraph callees ` takes a bare name with no way to disambiguate which file's symbol is meant (unlike `codegraph node -f `, which does support this). In symbol mode this ambiguity affects exactly one user-chosen name — a corner case. In `--architecture` mode, Codeshot probes `codegraph callees` for every enumerated symbol in the whole repo, where generically-named methods (`render`, `init`, `get`, `run`, `String`) existing in more than one file is common, not rare, in most real codebases (confirmed: 12 duplicate names out of 500 probed symbols on a real ~1,900-node Go repo). Since `unwrapQueryNodes` also keeps file nodes in the probed set (see below), the same ambiguity now applies to file basenames too — two files named `index.js` in different directories are indistinguishable to a bare-name `callees` probe. `duplicateNameWarning` surfaces both cases on stderr with real examples from the current run, but Codeshot has no way to fix the underlying ambiguity — same as the other `codegraph` indexing gaps documented above, it can only draw what `codegraph` returns. -- **`--architecture` mode probes file nodes' `callees`, not just named symbols', specifically to catch calls made from inside a top-level anonymous callback** (e.g. `test('...', () => { realCall() })` — a common pattern in test suites, including this repo's own `test/run.js`). codegraph attributes such a call to the enclosing file, not any named function, since no named function contains it; without probing the file node itself, `--architecture` mode would be structurally blind to that entire category of real cross-file dependency. This was confirmed against a real regression-turned-non-regression in codegraph itself: a codegraph 1.4.1 bug (fixed in 1.5.0, see `git log` for `LITERAL_RECEIVER_TYPES` in codegraph's history) briefly caused a *different*, spurious file-node-unrelated edge to appear in this project's own diagram — a call like `/regex/.test(x)` in `render/callgraph.js` got bare-name-matched to `test/run.js`'s own `test(name, fn)` helper purely by name collision. That edge is gone as of codegraph 1.5.0+ (correctly — it was never real); probing file nodes is what makes the *actual* dependency (`test/run.js` calling into `render/callgraph.js`) visible in its place. +- **`--architecture` mode probes file nodes' `callees`, not just named symbols', specifically to catch calls made from inside a top-level anonymous callback** (e.g. `test('...', () => { realCall() })` — a common pattern in test suites, including this repo's own `test/run.js`). codegraph attributes such a call to the enclosing file, not any named function, since no named function contains it; without probing the file node itself, `--architecture` mode would be structurally blind to that entire category of real cross-file dependency. This was confirmed against a real regression-turned-non-regression in codegraph itself: a codegraph 1.4.1 bug (fixed in 1.5.0, see `git log` for `LITERAL_RECEIVER_TYPES` in codegraph's history) briefly caused a *different*, spurious file-node-unrelated edge to appear in this project's own diagram — a call like `/regex/.test(x)` in `render/callgraph.js` got bare-name-matched to `test/run.js`'s own `test(name, fn)` helper purely by name collision. That edge is gone as of codegraph 1.5.0+ (correctly — it was never real); probing file nodes is what makes the *actual* dependency (`test/run.js` calling into `render/callgraph.js`) visible in its place. `probeFileEdges` skips any `"kind":"file"` *callee* it gets back from a probe — the same unverified-reference status that makes symbol mode draw it dotted/gray rather than as a real call (see Visual Encoding) means it must not be counted as a real cross-file edge here either, or this exact fabricated-edge problem reappears via a different mechanism. Verified empirically against this repo's own index (0 `"kind":"file"` callees among 100 probed) — but a repo where a file's *only* top-level reference is an unresolved `require(...)` with no other calls would exercise this path, and there's no dedicated test for it (real I/O against a live index would be needed to construct one). Two costs of probing file nodes, both real but not separately mitigated: enumeration and `--max-symbols` now compete real symbols against file nodes for the same fixed slot budget in an order codegraph doesn't guarantee (see the `--limit` note below) — on a repo near the cap, file nodes could crowd out real-symbol coverage with no warning distinguishing the two; and the probe count (and thus the already-"multi-minute" wall-clock cost) grows by roughly the repo's file count, since `probeFileEdges` is strictly sequential. - **`--architecture` mode's enumeration query (`codegraph query --json --limit -- ''`) has confirmed, inconsistent `--limit` behavior worth knowing before trusting it.** Without `--limit`, an empty-string query silently caps around 50 results regardless of actual repo size (confirmed on a real 1,870-node index). Passing a large `--limit` (confirmed with both 500 and 2000 against that same index) instead returns *every* result codegraph has — more than the requested number, not capped at it. Codeshot works around this by always passing a very large `--limit` to force the "return everything" behavior, then applying the real `--max-symbols` cap client-side — but the *order* codegraph returns results in in that case is unknown (untested whether it's insertion order, alphabetical, ID-based, or something else), so on a repo larger than `--max-symbols`, the kept subset should not be assumed to sample evenly across the whole repo — it could be clustered by file, directory, or however codegraph happens to have stored them. diff --git a/render/callgraph.js b/render/callgraph.js index 8569b3f..a061318 100755 --- a/render/callgraph.js +++ b/render/callgraph.js @@ -472,9 +472,16 @@ async function collectTransitive(direction, repoPath, limit, maxDepth, seedNodes // `test('...', () => { ... })` body calls into render/callgraph.js this way, // and none of those calls are reachable from any named symbol codeshot could // otherwise probe). Without this, --architecture mode is structurally blind -// to that whole category of cross-file call. +// to that whole category of cross-file call. Also drops any entry with no +// usable `name` (a malformed/partial index record) — probeFileEdges passes +// this straight into a `codegraph` subprocess's argv, where `undefined` would +// throw before codegraph ever gets to report its own "not found", bypassing +// the fatal:false resilience that's supposed to let one bad entry skip past +// without aborting the whole scan. function unwrapQueryNodes(queryResults) { - return (queryResults || []).map(r => r.node).filter(Boolean); + return (queryResults || []) + .map(r => r.node) + .filter(n => n && typeof n.name === 'string' && n.name.length > 0); } function symbolBudgetWarning(truncated, budget) { @@ -600,6 +607,13 @@ async function probeFileEdges(symbols, repoPath, limit) { ); if (result === null) continue; for (const c of result.callees || []) { + // A "kind":"file" callee is a module-level/import reference codegraph + // couldn't resolve to a real call site — symbol mode already treats + // these as unverified (edgeStyleAttrs draws them dotted/gray, not a + // real call edge); counting one as a full-weight file-to-file edge + // here would fabricate exactly the kind of edge this file-node-probing + // change exists to stop fabricating. + if (c.kind === 'file') continue; edges.push({ fromFile: s.filePath, toFile: c.filePath }); } if ((i + 1) % 25 === 0) { diff --git a/test/run.js b/test/run.js index 11e4c54..4ae89a9 100644 --- a/test/run.js +++ b/test/run.js @@ -543,9 +543,11 @@ test('unwrapQueryNodes unwraps .node and keeps kind:file entries (probed for ano { node: { name: 'a.js', kind: 'file', filePath: 'a.js' } }, { node: { name: 'BAR', kind: 'constant', filePath: 'b.js' } }, { node: null }, + { node: { name: '', kind: 'function', filePath: 'c.js' } }, + { node: { name: undefined, kind: 'function', filePath: 'd.js' } }, ]; const symbols = unwrapQueryNodes(results); - assert.strictEqual(symbols.length, 3); + assert.strictEqual(symbols.length, 3, 'a missing/empty name must be dropped, not passed through to a codegraph subprocess call'); assert.deepStrictEqual(symbols.map(s => s.name), ['Foo', 'a.js', 'BAR']); });