Add --diff mode: diagram just what a diff touched - #26
Merged
Conversation
Diagrams the symbols codegraph attributes to changed files (git diff, or --diff-ref <range> for a reproducible CI range) as bold roots, with their direct callers/callees fanned out — reusing symbol mode's per-symbol callers/callees fetch instead of --architecture's whole-repo scan. Composes with --embed/--check.
…bed collisions Code review on PR #26 found four confirmed bugs, fixed here: - gitDiffFiles defaulted to bare `git diff`, which diffs against the index not HEAD and misses fully-staged changes (e.g. after `git add -A`). Now explicitly diffs against HEAD. - buildDiffDot's test/file edge styling depended on which of two probes (a root's caller-fetch vs another root's callee-fetch) happened to discover a root-to-root edge first, since dedupeEdges collapses on from/to only. Styling is now based solely on the edge's (from, to) pair, independent of discovery order. - --diff-ref reused the same --embed marker id/filename as bare --diff, so two differently-scoped diagrams (e.g. a release range and a PR range) embedded in the same doc would silently overwrite each other. --diff-ref now gets its own suffixed marker/filename, matching --group-depth's precedent for --architecture. - runDiffMode ran a full-repo codegraph enumeration even when the diff had zero changed files, which can only ever yield zero matches. Now short- circuits before that scan. Docs and tests updated to match; 138/138 passing.
…ate-name check, empty-diff data loss Second code-review round on PR #26 found five more confirmed bugs, fixed here: - gitDiffFiles spliced --diff-ref straight into git's argv with no --end-of-options guard, so a value starting with '-' (e.g. templated CI input) was parsed by git as a flag instead of a revision — verified exploitable (e.g. a crafted --output=<path> value). Closed the same way resolveSymbol's '--' guards codegraph calls, using git's own equivalent since bare '--' would mark the ref as a pathspec instead of a revision. - Line-based git diff parsing left core.quotePath's C-style octal-escaping of non-ASCII filenames un-decoded, silently dropping such files from matching. Switched to -z (NUL-terminated, unquoted) output. - duplicateNameWarning(roots) only compared changed roots against each other, missing a changed root colliding with an unrelated, unchanged symbol elsewhere in the repo. Now scoped against the full enumerated symbol set filtered to root names. - An empty diff with --embed rendered a blank graph and silently overwrote a real, previously-committed diagram at exit 0. Now refuses outright (exit 1) instead of touching the doc when there's nothing to diagram. - Added an aggregate warning (not per-root, to avoid noise) when a changed symbol has no callers/callees in the index, matching emptyGraphWarning's stance in single-symbol mode. New CLI-level tests cover the git-diff pipeline end-to-end (--diff-ref HEAD~1 against this repo's own history) and the empty-diff embed refusal. 144/144 passing.
…dupe-name warning Third code-review round on PR #26 found two more confirmed bugs, fixed here: - The --embed data-loss guard only covered the zero-changed-files case, not the zero-matched-symbols case (real changed files, but none define a symbol codegraph indexed — a docs/config-only diff). That path still fell through to a blank buildDiffDot render with no refusal, silently overwriting a committed diagram the same way the earlier fix closed for the other empty case. Added diffEmbedRefusalNoSymbols as its sibling. - runDiffMode reused duplicateNameWarning's message for a colliding root name, but that text explicitly claims a file-qualified 'codegraph node -f' re-probe happens — true for --architecture, never true for --diff (which always probes duplicates by bare name, an acknowledged unfixed gap). Added diffDuplicateNameWarning so the warning doesn't claim a mitigation that didn't happen. Also corrected an inaccurate test comment (--diff-ref HEAD~1 is working-tree- vs-ref, not a two-commit comparison, so it isn't fully independent of ambient uncommitted state — the assertions just don't need it to be). 146/146 passing.
…missing truncation warning Fourth code-review round on PR #26 found the most severe bug yet, plus several smaller ones, fixed here: - main() never passed --check into runDiffMode, so `--diff --embed --check` ALWAYS hit the empty-diff embed refusal instead of the documented stale/fresh comparison — breaking the exact CI drift-guard workflow README/USAGE prescribe. --check never writes (finishOutput's check branch only compares), so the refusal's "don't overwrite" rationale never applied to it in the first place. Fixed via a shared diffRefuseOrWarn helper (also de-duplicates the two identical refuse-or-warn branches) that bypasses the refusal whenever check is true. - matchRootSymbols matched codegraph's "kind":"file" index entries (kept in enumerateAllSymbols' output for --architecture's sake), so a changed FILE itself — not just its symbols — was drawn as a bold diagram root, silently eating into --max-symbols/--max-render budget. Confirmed live before the fix. Excluded now. - Diff mode's per-root callers/callees probes had no equivalent of symbol mode's truncationWarning, so a root with exactly --limit results rendered as complete with no signal more might exist. Added an aggregate warning (diffTruncationWarning), same noise tradeoff diffEmptyRootsWarning already makes. - diffDuplicateNameWarning understated a collision between two ROOTS specifically: both run the identical bare-name query, so codegraph's ambiguous merged result is attached to BOTH in full, not just "an edge." Wording corrected. - gitDiffFiles trimmed each NUL-split path, which could corrupt a (rare but legal) filename with real leading/trailing spaces — the exact class of mangling -z was chosen to avoid. Removed; -z's NUL terminator needs no trimming. - Extracted a shared nodeKey(n) helper for the (name, filePath) identity-key format that was hand-rolled at 6+ sites across dedupeNodes, dedupeEdges, collectTransitive, buildDiffDot, and runDiffMode. 151/151 passing, including a new CLI regression test that pins the --check fix directly (asserts the refusal message never appears and the normal drift report does).
… warning, minor wording/perf fixes Fifth code-review round on PR #26 found the most severe bug yet, plus five smaller ones, fixed here: - --diff --embed --check had NO reachable passing state on a zero-root diff. Round 4 made --check bypass the embed refusal, but that just moved the dead end: the blank render then got compared by finishOutput against the real committed diagram, reported "out of date", and suggested a rerun with --embed — which itself hit the refusal for the same zero-root diff. Fixed properly: a zero-root diff means there is nothing THIS invocation would diagram, so --check now reports that directly (exit 0) and skips finishOutput's compare-and-report path entirely, instead of either refusing or falsely flagging drift. New CLI test pins this exact scenario. - roots was never deduped by (name, filePath), so two enumerated records that collapse to one identity (e.g. two same-named methods on different receiver types in one Go file — the codebase already treats that as one identity everywhere else) desynced diffEmptyRootsWarning's math from what buildDiffDot actually drew (false "has no callers" on a root that did), and wasted a duplicate codegraph probe. Deduped once, right after slicing. - --group-depth under --diff reused symbol mode's rejection wording ("a symbol trail has no file-level graph to roll up"), misdiagnosing the command as looking like symbol mode. Given a --diff-specific reason. - gitDiffFiles used Node's default 1MB execFileSync maxBuffer instead of the 64MB MAX_CODEGRAPH_BUFFER already defined for this exact class of large-output problem; a huge --diff-ref range could overflow it with a misleading "not a valid ref" error. - diffSymbolBudgetWarning didn't explain the cut is a path-sorted prefix (files sorting later are simply missing, not "calls nothing") — the same shape symbolBudgetWarning already discloses for --architecture. - A stray inline copy of the (name, filePath) key format survived at --depth's traversal seed in main(), despite nodeKey's "single source of truth" claim; pointed at nodeKey. Declined as out of scope (filed as issue #27 instead): centralizing the "--embed must not silently overwrite with blank output" guard in finishOutput, since --architecture --embed has the identical hole and fixing it belongs to a separate, --diff-unrelated PR. 153/153 passing.
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
--diff [--diff-ref <range>]mode: diagrams the symbols codegraph attributes to changed files as bold roots, with their direct callers/callees fanned out around them — same house style as symbol mode, reusing its per-symbolcallers/calleesfetch instead of--architecture's whole-repo scan.--diffdiffs the working tree vsHEAD(git's own default);--diff-ref <range>(e.g.origin/main...HEAD) gives a reproducible range for--embed --checkin CI.--limit/--max-render/--max-symbols(now capping how many changed symbols get probed) and composes with--embed/--check.--depth/--max-depth-nodes/--group-depthare rejected (no meaning in this mode).--architecture, this mode does not yet port thenode -fsame-name-across-files fix —duplicateNameWarningsurfaces the risk instead of resolving it.Test plan
npm test— 136/136 passing (11 new:matchRootSymbols,buildDiffDot, warning builders)--diff+symbol,--diff+--architecture,--diff+--depth,--diff+--group-depth) and the empty---diff-refcase--embed --checkround-trip (write, then confirm up-to-date)