fix(connectivity): stop the note promising dead_code rows are safe to remove - #637
Merged
zzet merged 1 commit intoAug 20, 2026
Merged
Conversation
… remove
The `note` shipped on every connectivity_health response ended with
"genuinely unreachable code that is safe to remove", and the two
doc-comments feeding it said the same ("the code is unused and can be
removed", "a real finding to act on (delete it)").
That contradicts the shared vocabulary this analyzer says it stays in
lockstep with. ClassifyZeroEdge reaches the opposite conclusion from the
same zero-incoming shape: likely_unused is "evidence of no callers, not
proof: confirm with a text search for the symbol name before removing
it", and coverage_incomplete is "not as proof the symbol is unused or
safe to remove". FindDeadCode's own contract claims no more than "zero
incoming calls or references" — the removal verdict was added on top of
it, and only here.
The hedge is load-bearing, not stylistic: an ambiguous member call is
deliberately left unresolved rather than misattributed to a same-name
candidate, and a name-only match never earns a usage edge. Both leave a
live symbol with the exact zero-incoming shape dead_code reports on, so
a reader who trusts the note deletes working code. Measured on a 2.5k-file
Go repository, three of four dead_code rows were refuted by grep; two were
methods reached through a nested field selector.
Reword all three sites to state the signal and withhold the verdict. The
isolated-vs-dead-code distinction the note exists to draw is unchanged.
The new test pins the property rather than the prose: it builds the
dead-code shape, asserts the shared caveat for that same symbol still says
"not proof", then asserts the note keeps naming dead_code and its INCOMING
signal while carrying no removal verdict. Restoring the old sentence fails
it on the "safe to remove" assertion.
Contributor
Author
|
Heads-up so this doesn't cost you a debugging round: the red
Locally, |
zzet
approved these changes
Aug 20, 2026
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.
Problem
Every
connectivity_healthresponse ships anotethat ends:The two doc-comments feeding it agree —
internal/analysis/connectivity.gosays "the code is unused and can be removed",internal/mcp/tools_analyze_connectivity.gosays "a real finding to act on (delete it)".That contradicts the vocabulary this analyzer explicitly says it stays in lockstep with.
ClassifyZeroEdgereaches the opposite conclusion from the same zero-incoming shape:likely_unused→ "this is evidence of no callers, not proof: confirm with a text search for the symbol name before removing it" (extraction_gap.go:296)coverage_incomplete→ "not as proof the symbol is unused or safe to remove" (extraction_gap.go:310)And
FindDeadCode's own contract claims no more than "all symbols with zero incoming calls or references, excluding entry points, test functions, exported symbols, and user-excluded patterns" (deadcode.go:233). The removal verdict is layered on top of that, and only in these three places.Why the hedge is load-bearing
Two ordinary situations leave a live symbol with exactly the zero-incoming shape
dead_codereports on:resolveMethodCalldeliberately declines to bind rather than misattribute to a same-name candidate — "ambiguous stays unresolved rather than misattributing to a same-name". Every losing candidate keeps zero incoming edges.Measured on a 2,475-file Go repository (
gortex v0.63.7+0988412):analyze dead_codereturned 4 rows andgit greprefuted 3 of them. Two were methods reached through a nested field selector (p.provider.recordSubmit(...), called 16 lines below their own definition); one was a method value (var cryptorandRead = randReader). A reader who trusted the note would have deleted three working symbols.Note the
callerscaveat next door is already correctly hedged — this is the one surface that still isn't.Change
Reword all three sites to state the signal and withhold the verdict. The isolated-vs-dead-code distinction the note exists to draw is unchanged; only the removal promise goes.
Test
TestConnectivityNote_MatchesSharedZeroEdgeStancepins the property, not the prose, so a future rewording is free but a re-promotion is not:definesedge, zero incoming usage),CaveatForZeroEdgefor that same symbol islikely_unusedand still says"not proof",connectivityNote,dead_codeand itsINCOMINGsignal,safe to remove/safe to delete/can be removed/can be deleted).Sabotage-verified: restoring the old sentence fails the test on assertion 5 —
should not contain "safe to remove". Reverting makes it green again.Limitations I'd rather declare than have found
GraphConnectivity's return value. The MCP handler's own doc-comment is changed for coherence but is not test-bound (it is a comment).Verification
go test ./internal/analysis/ -run 'TestConnectivityNote|TestGraphConnectivity' -count=1— 6/6 pass, plus the sabotage run above.go build ./internal/mcp/ ./internal/analysis/— clean.golangci-lint run --timeout=10m ./internal/analysis/... ./internal/mcp/...— 0 issues.git diff --checkclean; diff carries no CR bytes. (gofmt -lflags all 83 files ininternal/analysison my Windows clone, untouched files included — a localcore.autocrlfartifact, not this change.).md/.json/.yamlmirrors the old wording; no test referenced the note before this one.Windows 11, go1.26.6, branched from
mainata71cbedb.