Fix --architecture mode's fabricated edge; bump codegraph pin to 1.5.0 - #23
Merged
Merged
Conversation
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 <fileBasename>`
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
render/callgraph.js -> test/run.jsedge, caused by a codegraph 1.4.1 bug (fixed upstream in 1.5.0'sLITERAL_RECEIVER_TYPESchange): calls on a literal receiver (e.g./regex/.test(x)) fell through to bare-name matching against any same-named project symbol.render/callgraph.jsuses.test()heavily, and its own test runner definestest(name, fn)— so those unrelated builtin calls were bare-name-colliding into a fabricated edge. Verified by direct side-by-side reproduction against a locally-installed 1.4.1 vs. a fresh 1.5.0 index.--architecturemode has never captured calls made from inside anonymous top-level callbacks (e.g.test('...', () => { realCall() })) in either codegraph version, becausefilterCallableSymbolsdropped file-kind nodes from the probed set. codegraph attributes such calls to the enclosing file, andcodegraph callees <fileBasename>is a real, working query against it — codeshot just never issued it. Renamed tounwrapQueryNodes, stopped dropping file-kind entries. The regenerated diagram now shows the real, correctly-directed edge (test/run.js -> render/callgraph.js, weight 29).--checkvalidates against a version that behaves correctly, instead of faithfully re-validating a fabricated artifact on every push.TECHNICAL.md's Known Limitations gains a note on the new file-basename collision risk (same class as the existing symbol-name collision limitation) and documents the new file-node probing.Test plan
npm test— 89/89 pass (both previously-failing--architecturetests now pass for real, not just skip).--architecturemode before/after: confirmed the fabricated edge is gone and the real one (correct direction) appears.--checkpasses clean against the regenerateddocs/architecture.svg+TECHNICAL.mdembed.docs/buildDot-callgraph.svg, symbol mode) was unaffected — still passes--checkunchanged.