Fix two graph-correctness bugs found auditing the whole codebase - #6
Conversation
Every Go TestXxx is an exported name, so the ladder's "exported/public function" fallback tagged every test in the blast radius MEDIUM. The review score's -6 test penalty was far too small to sink them, so on a change whose only direct callers are tests, the entire top of the ranking a reviewer is told to read first was test code. Add a rung above the export fallback: a test with no production callers is LOW. Guarded on zero production callers so it cannot shadow any rule above it, all of which require at least one. Measured against the indexed graph, top-of-list becomes real code: seed before after linesOverlap 15 med, 4 low 2 med, 17 low canonicalPath 44 med, 21 low 5 med, 60 low matchPattern 16 med, 9 low 3 med, 22 low Note this changes the CI contract for test-only impact: a change whose blast radius is entirely tests now exits 0 rather than 2. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
docs/tracescope-motion.html: a single self-contained page (React + Framer Motion via importmap, no build step) explaining the tool in the order the code runs it — index-time, analyse-time, then the mechanisms that decide whether any of it is true. Every figure is measured, not asserted. Graph counts come from the committed .tracescope/graph.json; scenario results come from replaying graph/query.go, analyzer/risk_scorer.go and analyzer/blast_radius.go against it; eval numbers come from docs/EVALUATION.md. Where a diagram draws a subset, it prints the real N beside it. No SCIP node counts are quoted, because no SCIP-built graph of this repo is checked in. Each stage states its own limitation, including the unflattering ones: 23% call-site resolution, a HIGH gate only 2 of 283 functions can reach, and hand-picked scoring constants the eval shows are close to inert. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
Build appended a CALLS edge for every call expression, so a function calling another eleven times produced eleven edges. Caller counts come from counting those edges (analyzer.buildCallerCountMaps), so that function was reported as having eleven callers, and could cross the HIGH threshold on its own. Measured on this repo's own graph: 928 CALLS edges collapse to 705 distinct pairs — 223 were duplicates. Four functions were rated HIGH purely because of the inflation: resolveUnique 12 -> 2 distinct callers findChildContent 21 -> 7 httpError 11 -> 5 Match 6 -> 4 The SCIP backend already deduped this way (scipGraphBuilder.addEdge), so the two backends disagreed about risk for identical code. ResolutionStats still counts call sites — its unresolved and ambiguous counters are a per-reference diagnostic, not an edge count. Where one pair is reached both heuristically and exactly, the surviving edge is exact: an exact resolution anywhere is stronger evidence the edge is real. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
findChildContent scans direct children only, so for a chained call like logger.output.write() the object is itself a member_expression and no identifier child exists. The receiver came back empty, which made the call indistinguishable from a bare write() — and bare-name resolution then bound it to any local function named write, with EXACT confidence. A fabricated edge, reported as a certainty. Python already handled this correctly via flattenAttribute; JS and TS did not. Flatten the object chain instead, so logger.output.write() keeps the receiver "logger.output" and this.helper() keeps "this". An object with no name at all (f().c()) is marked "?" rather than left empty, since an empty receiver is what triggers the bare-name path. Also stop claiming EXACT for any call that still falls through to bare-name matching with a receiver present. The match may be right — a same-file method call whose receiver type go/types could not infer — but x.Foo() matching some definition of Foo chosen without knowing what x is is not a certainty. It now resolves as heuristic, so the review score discounts it and the reviewer is told to check the path. Measured by re-indexing this repo: RiskExitError.Error went from 75 inbound edges (3 of them EXACT) to 46, all heuristic. Unresolved calls rose 3059 -> 3181, which is the intended trade: fewer invented edges, more admitted gaps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
serve ignored graph_path. It passed no GraphFile, so the server fell back to ./.tracescope/graph.json and silently ignored the configured path that analyze, why and hotspots all honour via loadGraph. The server advertised GET /api/why, which was never registered and returned 404. Replaced with the routes that do exist (/api/repo, /api/analyze/diff). The HTTP server had no timeouts — the zero-value http.Server has none, so a stalled client could hold a connection open indefinitely. Write and idle timeouts are generous because analysing a large diff is slow. SaveCache wrote a fixed path+".tmp", so two indexes against the same tree could rename a half-written cache into place. Uses a unique temp file now, matching graph.Store.Save. A Python class whose name could not be extracted returned without walking its body, dropping every call inside it from the graph. git log fields were separated by "|", which an author name may contain; that shifted every later field and left the date unparseable. Uses 0x1f. The dead alternate-format fallback went with it — %aI is RFC3339, so a failure means the field was not a date, and the zero time renders as "unknown" rather than inventing a timestamp. Not fixed, deliberately: FileMetadata.ParsedAt is set from time.Now() and never read anywhere, so graph.json differs on every index even when nothing changed. Removing it is a schema change for a cosmetic gain. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
The audit that produced this page found two defects in the graph builder, and fixing them changed the graph it describes. Every figure here is re-derived from a rebuild at HEAD. nodes 614 -> 659 (repo also grew) CALLS edges 928 -> 766 duplicates removed unresolved 3059 -> 3204 chained JS/TS calls no longer mis-bind rated HIGH 7 -> 6 four phantoms out, three real ones in The gate scenario changed outright: Codeowners.Match reached HIGH only because six call sites from four callers were counted as six callers. Replaced with cleanDiffPath -> ParseUnifiedDiff, which is HIGH on five genuine production callers. The stale-graph stage no longer claims the warning fires today — the artifact is now bound to HEAD. It records what it looked like when it did fire instead, which is a better demonstration than an assertion. Dropped the mis-resolution example that named doublestar.Match binding to Codeowners.Match: that edge no longer exists, because it was one of the fabricated ones the parser fix removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3274c1fa5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if id, status := resolveUnique(sameFileMatches); id != "" { | ||
| return id, EdgeConfidenceExact, resolutionResolved | ||
| return id, bareConfidence, resolutionResolved |
There was a problem hiding this comment.
When a receiver-aware lookup fails, this still lets calls such as logger.output.write() resolve to an unrelated, uniquely named local write function; changing bareConfidence to heuristic only relabels the fabricated CALLS edge, and graph traversal and caller counting still consume it. This means the chained JS/TS false-positive that the parser change is intended to eliminate remains whenever the property name has a same-file match; unresolved receiver calls should not use this bare-name fallback, at least for JS/TS chained or unnameable receivers.
Useful? React with 👍 / 👎.
| G.callSites = G.rExact + G.rHeur + G.rAmb + G.rUnres; // 4031 | ||
| G.resolvedPct = Math.round((G.calls / G.callSites) * 1000) / 10; |
There was a problem hiding this comment.
After CALLS-edge deduplication, G.calls is the number of distinct caller/callee edges while G.rExact and G.rHeur count resolved call sites, so dividing G.calls by all call sites mixes incompatible units and displays 18.0% instead of approximately 23.5% for the values above. Use (G.rExact + G.rHeur) / G.callSites; the adjacent 4031 comment is also inconsistent with the listed counters, which sum to 4246.
Useful? React with 👍 / 👎.
TraceScope — Blast Radius
Reviewer FocusPrioritize high-score public/runtime paths first; manually inspect heuristic or ambiguous paths before merging.
Resolution Diagnostics
Showing first 10 of 200 diagnostics. 🔴 High Risk
🟡 Medium Risk
🟢 Low Risk (7)
Showing top 15 of 57 affected functions. Generated by TraceScope |
Running TraceScope's own PR review over this branch surfaced the
placeholder in user-facing output:
unresolved call ?.filter (web/app/analyze/page.tsx:37)
unresolved call ?.split (web/app/analyze/page.tsx:56)
Those are remote.split("/").filter(Boolean) and parts.slice(idx).join("/")
— methods chained onto a call result. Treating them as having a receiver
is correct, and unresolved is the honest answer. But "?" renders as
"?.filter", which reads as JavaScript optional chaining and points a
reviewer at a language feature that is not there.
Named the sentinel "(expr)" behind a documented constant shared by both
parsers, and pinned it in the test so it cannot drift back to something
that collides with JS syntax.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d80e51de88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| default: | ||
| return right |
There was a problem hiding this comment.
When a chain is rooted in an expression, such as make().client.send(), the recursive call returns an empty left for make() and this branch returns only client. The resolver then treats that property as a real receiver and can bind send to an imported client or a qualified client.send definition, fabricating an exact or heuristic CALLS edge. Propagate the unnameable-expression state instead of converting the trailing property into a standalone receiver.
Useful? React with 👍 / 👎.
| return server.ListenAndServe(server.Config{ | ||
| Host: serveHost, | ||
| Port: servePort, | ||
| Host: serveHost, | ||
| Port: servePort, | ||
| GraphFile: graphFile, |
There was a problem hiding this comment.
When serve is launched from a repository subdirectory, config loading finds the ancestor .tracescope.yaml and this now loads its graph correctly, but RepoRoot remains unset, so ListenAndServe defaults it to the subdirectory. For SCIP graphs whose node paths are repository-relative, ownership runs git log with those paths relative to the subdirectory and misses authors, while LoadCodeowners also searches below the actual root; resolve and pass RepoRoot alongside GraphFile.
Useful? React with 👍 / 👎.
Graph nodes store absolute paths from whichever machine ran `tracescope index`, and FormatMarkdownComment printed them raw. Running the tool on its own pull request put this in a public comment: C:\Users\anura\Downloads\Devlopment\TraceScope\internal\analyzer\blast_radius.go:95 Render paths relative to the working directory instead — in CI that is the repository root — across the risk tables and the diagnostics table. The terminal output already did this via shortPathCwd; the markdown surface never got the same treatment. A path that is not under the base keeps its absolute form rather than becoming a ../../ fragment that looks repo-relative but is not. Every path is slash-normalised either way, so Windows separators cannot reach the comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61f2b7404f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if confidence == EdgeConfidenceHeuristic { | ||
| gd.ResolutionStats.HeuristicCallEdges++ | ||
| } else { | ||
| gd.ResolutionStats.ExactCallEdges++ | ||
| } |
There was a problem hiding this comment.
When a caller invokes the same callee multiple times, these counters increment before the pair is deduplicated, so exact_call_edges/heuristic_call_edges count call sites even though the parser graph contains one edge; the SCIP backend instead increments them only after its edge-set deduplication. A mixed heuristic/exact pair can even increment both counters while the surviving edge is exact, making the persisted resolution statistics and terminal/GitHub confidence summaries backend-dependent. Count the surviving edge once (adjusting on confidence upgrade), or introduce explicitly named call-site counters for the per-reference diagnostics.
Useful? React with 👍 / 👎.
1 participant
Started as a walkthrough page. Reading every file in
internal/to write ittruthfully turned up two defects that put edges in the graph which do not exist
in the source, plus several smaller ones.
Every fix has a test that fails without it.
go build,go vetand all 11packages pass on each commit.
The two that matter
One CALLS edge per call site, not per caller.
Buildappended an edge forevery call expression, and caller counts come from counting those edges, so a
function calling another eleven times was reported as having eleven callers.
On this repo, 928 call edges collapsed to 705 distinct pairs. Four functions
were rated HIGH purely from the inflation:
resolveUniquefindChildContenthttpErrorMatchThe SCIP backend already deduped (
scipGraphBuilder.addEdge), so the twobackends disagreed about risk for identical code.
Chained JS/TS calls bound to unrelated local functions.
findChildContentscans direct children only, so for
logger.output.write()the object is itselfa
member_expressionand no identifier child exists. The receiver came backempty, making the call indistinguishable from a bare
write()— whichbare-name resolution then bound to any local function of that name, with
EXACT confidence. A fabricated edge reported as a certainty. Python already
handled this correctly via
flattenAttribute.Also stops claiming EXACT for any call that falls through to bare-name matching
with a receiver present:
x.Foo()matching someFoochosen without knowingwhat
xis has not been resolved with certainty.Also fixed
TestXxxis anexported name, so the ladder's export fallback tagged them MEDIUM and the
-6 test penalty was too small to sink them.
serveignoredgraph_pathfrom.tracescope.yaml.GET /api/why, which was never registered.SaveCachewrote a fixed.tmppath; two indexes could race.git logfields were|-separated, which author names may contain.Effect on the graph, measured by re-indexing
RiskExitError.ErrorinboundUnresolved rising is the intended trade: fewer invented edges, more admitted gaps.
Behaviour changes to review before merging
test functions.
pass now fail, and vice versa.
Not fixed, deliberately
FileMetadata.ParsedAtis set fromtime.Now()and never read, sograph.jsondiffers on every index. Removing it is a schema change for a cosmetic gain.
Scope note
I read every file in
internal/. The parts skimmed rather than tracedline-by-line are the SCIP protobuf decoding internals and the tree-sitter walk
functions. There may be defects in there I did not see.
🤖 Generated with Claude Code
https://claude.ai/code/session_01A8PMni59sFvQFLAQyABGEs