fix(graph): rebuild cached graph when schema is stale - #23
Merged
Conversation
graphAt caches a built graph on disk keyed only by commit, with no schema check, so a graph written by an older binary loads with unresolvedByFile missing and every count derived from it reads as zero. A silently wrong number is the one failure a document of derived facts cannot have. Version the record and rebuild when the version does not match, which also covers the next field rather than only this one. Claude-Session: https://claude.ai/code/session_01WW5sEutdtcbSUtgLQKBKpn
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
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.
Intent
Follow-up defect fix on the code-explainer feature merged in PR #22 (Thurbeen/thurview).
The problem, exactly: PR #22 added
unresolvedByFiletoCodeGraphso that a scoped code explainer can report only its own scope unresolved-reference count instead of the whole repository one. ButgraphAt(src/graph.ts) caches a built graph on disk under the review directory keyed ONLY by commit hash, with no schema check -readJson<CodeGraph>trusts whatever shape is on disk. So any user who already has a cached graph from a pre-upgrade thurview binary loads a CodeGraph withunresolvedByFilemissing, andcomputeCoveragesumsundefinedentries to 0. The Coverage tab then states "0 references the graph could not place" - a WRONG NUMBER with no error, for a feature whose entire thesis is that every fact it states is a count the reader can re-derive. The reviewing pipeline raised this as an open info finding on PR #22 and it merged with the finding open; this fixes it.The fix is deliberately schema-versioned rather than field-specific:
GRAPH_SCHEMAis a constant on the module,CodeGraphcarriesschema, andgraphAtrebuilds when the cached record version does not match. A field-specificif (!cached.unresolvedByFile)guard would rot the next time a field is added - and the reviewer noted the cache was never schema-versioned for any prior field either. The?.optional chain incomputeCoverageis dropped now that the field is guaranteed present (the?? 0stays, because a file with no unresolved references legitimately has no entry).Test: test/graph.test.ts drives
graphAtagainst a real scratch git repository with a cache file written in the OLD shape (right commit, missing fields) and asserts the graph is rebuilt with the true per-file counts rather than trusting the stale record, plus that the second call reuses the freshly written cache. This is behaviour through the real function, not a source grep.Scope: one commit, no behaviour change to reviews, no new CLI surface. The full gate is green locally: pnpm typecheck, prettier, rumdl, shellcheck, and 48 tests.
What Changed
GRAPH_SCHEMAconstant and aschemafield onCodeGraph;graphAtnow rebuilds the graph instead of reusing a disk cache whoseschemadoesn't match the current version, rather than trusting the cached record's commit hash alone.buildGraphstamps every graph it produces withGRAPH_SCHEMA.?.optional chain onunresolvedByFileaccess incomputeCoverage, since the field is guaranteed present once the schema check passes (the?? 0fallback stays for files with no unresolved references).graphAttest that seeds a cache file in the pre-schema shape (missing fields) and asserts the graph is rebuilt with correct per-file unresolved counts, plus that a subsequent call reuses the freshly written cache; updated the coverage test fixture to includeschema.Risk Assessment
✅ Low: The schema-version guard closes the exact reachable path described (stale on-disk graph missing unresolvedByFile), all CodeGraph reads go through the now-guarded graphAt, readJson already fails safe to null on malformed cache so no new crash path is introduced, and the new test exercises graphAt end-to-end against a scratch repo with a pre-upgrade cache shape, correctly failing before the fix and passing after.
Testing
The provided baseline (
pnpm test && bats scripts/ci/*.bats) already passed; on top of that I ran the two targeted suites touched by the fix (graph.test.ts, coverage.test.ts) and, to prove this is a real regression test rather than incidental coverage, temporarily reverted src/graph.ts and src/coverage.ts to their pre-fix shape and confirmed the newgraphAttest fails with exactly the reported symptom (stale cache trusted, unresolvedByFile silently undefined), then restored the fix and confirmed all tests pass again. The change matches the stated intent precisely: GRAPH_SCHEMA constant, schema field on CodeGraph, graphAt checking schema on cache-hit, and the?.guard dropped in computeCoverage now that the field is guaranteed present. No source or test files were left modified.Evidence: Regression reproduced pre-fix, passes post-fix (graphAt cache-schema test)
One thing to decide before merging
This PR carries a
cog.tomlline it did not set out to carry. The gate titledthe PR
fix(graph): ...,graphis not incog.toml's scope list, so thePR Titlecheck failed - and the fix round widened the scope list rather thanretitling. It is left in because it looks right on its own merits (
src/graph.tsis a ~590-line subsystem with its own test file, and both #20 and #21 touched
it), but adding a commit scope is a repo-convention call that belongs to you,
not to a one-line bug fix. Drop that line and retitle to an existing scope if
you would rather keep the vocabulary as it is.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
pnpm test && bats scripts/ci/*.batsnpx vitest run test/graph.test.ts test/coverage.test.tsat target commit a1a69dd — 11/11 passRegression reproduction: temporarily reverted src/graph.ts and src/coverage.ts to the pre-fix (ff05f94) shape while keeping the new test, rerantest/graph.test.ts— the newgraphAttest failed withexpected undefined to deeply equal { 'src/a.ts': 1 }, confirming it fails for the exact reported reason (stale cache trusted, unresolvedByFile undefined)Restored fixed sources and rerannpx vitest run test/graph.test.ts test/coverage.test.ts— 11/11 pass again, worktree left clean (git statusclean, no stray temp files)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.