docs(kg): re-measure the estate figures, and withdraw a wrong correction - #16
Conversation
The pending-re-measurement warning said the derived-edge figures could not have come from indexed data, because Go's package_clause was supposedly the only source of package entities before 5211bc2 and Go names carry no dots. Two facts say otherwise: package_clause is Scala's tree-sitter node as well as Go's, and indexer_treesitter.go matched it generically at 5211bc2^; and the estate's databases, indexed by v0.1.0-34, hold 4,385 package entities with three or more dotted segments, one of which resolves to .scala files. 5211bc2 added Java and Kotlin, not Scala. So the figures were sound. Re-derived at 3ac2f1a they are 2,532 derived edges (was 2,525), 3,355 ambiguous (was 3,359), 5,046 same-layer (was 5,031). The +7 is the wildcard-import fix: a Java `import com.x.y.*;` reaches the resolver as a string identical to the package name, and the original loop tested only proper prefixes, so those were dropped silently. Also records what the warning's remedy would actually buy. Package entities are minted at index time, so a newer binary over older databases changes nothing; re-indexing is what is required. By file count that is 75 .java files and no Kotlin against 12,537 already-indexed .scala — the unindexed mass is Python, TypeScript and Go, which 5211bc2 did not touch.
There was a problem hiding this comment.
Review Summary
- Files reviewed:
docs/kg-cli-reference.md,docs/kg-graph-linking-design.md(docs-only PR, no code changes) - Cross-checked against
src/kg/internal/knowledge/indexer_treesitter.goandgraph_link.goto verify the technical claims driving the correction - Overall verdict: REQUEST for one fix — a self-contradiction the PR leaves behind in the very doc it's correcting
Major Issues (should fix)
docs/kg-graph-linking-design.md:270-274(§Follow-up table) — This table, untouched by the diff, still states unconditionally:That directly contradicts the correction this PR just made three sections above (§Re-measured, lines 43-53) and in| Java, Kotlin, Scala | dotted namespace (`com.depop.auth.client`) | yes |kg-cli-reference.md's new table (lines 386-391), both of which now say Java/Kotlin are only linkable "once the graph is re-indexed with kg ≥5211bc2" — the current estate has zero Java/Kotlin package entities today. A reader who follows either of the two[§Follow-up](#follow-up)links this PR adds (lines 53, 282) lands on a table that flatly asserts the opposite of what they were just told. Since the entire point of this PR is eliminating exactly this class of stale/incorrect linkability claim, this leftover instance should be fixed in the same PR. Fix: update the table row to| Java, Kotlin | dotted namespace | yes, once re-indexed with kg ≥ 5211bc2 | / | Scala | dotted namespace | yes |(splitting the row) to match the qualification used elsewhere.
Minor Issues (optional)
docs/kg-graph-linking-design.md:50— "**0.kotlin**" — Kotlin's actual source file extension is.kt(used correctly elsewhere in this same PR, e.g. thekg-cli-reference.mdtable andindexer_treesitter.go's.kt/.ktsregistry keys), not.kotlin. Fix: change to**0 `.kt`**.
Security Findings
- None (docs-only change).
Positive Observations
- The core technical correction is verified accurate:
indexer_treesitter.goconfirms Go and Scala both usePackageNodeTypes: []string{"package_clause"}while Java usespackage_declarationand Kotlin usespackage_header— exactly the asymmetry the PR's narrative depends on. - The wildcard-import fix explanation matches the current
resolvePackageimplementation and its comment ingraph_link.go:156-176(loop starts atlen(parts), notlen(parts)-1, specifically to catchimport com.x.y.*;-style imports). - The anchor rewrite is correct and complete: the old
#pending-re-measurementanchor has no remaining references anywhere indocs/, and the new#re-measured--2026-08-29anchor correctly accounts for GitHub's double-hyphen slugification of the em-dash heading. - Cross-document figures are internally consistent (2,532/3,355 edge counts, 718k/1.2M and 7.6s/1.30 GB rounding match between the two files;
3ac2f1amatches the actual PR base commit). - The reworded Acceptance-section paragraph (845 vs. 2,525) was carefully re-scoped to past tense so it doesn't imply verification of the new 2,532 figure it doesn't actually cover — a good catch by the author.
The estate was re-indexed with 3ac2f1a (971 s, 61 scopes, no failures, all 420 observations preserved) and every figure re-derived. Re-indexing lowers the derived-edge count rather than raising it: 2,532 -> 2,240, while same-layer resolutions more than double. Both movements are the rule behaving correctly on better data. com.depop.common was indexed only in libraries, so every clients import of it looked like a cross-repo dependency; now that clients declares it too, the name resolves to two layers and the one-layer rule abstains. Elsewhere longest-prefix now finds a specific local package where it previously settled for a shorter one defined elsewhere. Edges drawn because a repository's own copy of a package had not been indexed were never dependencies, so fewer of them is a more truthful graph — and it sharpens the open question about ambiguity, which grows as coverage improves. Also corrects a miscount of my own: "seventy-five .java files and no Kotlin" came from find -maxdepth 4, which misses trees nested deeper, the Android app among them. The estate holds 30,906 .scala, 7,591 .kt and 1,835 .java files, so 5211bc2 gave package declarations to about nine thousand files. CHANGELOG: --federated was #6 not #5, linking was #10 not #7, and its package-indexing entry repeated the same wrong Scala claim this branch corrects. The kg graph flag table was missing --scope and --personal, both of which its own examples use.
|
Re-indexed the estate and re-derived everything (2cb6a15). The PR description above is now partly wrong — two corrections, one of them mine. Re-indexing lowers the derived-edge countThe expectation on record — including in my own description above — was that indexing more package declarations would raise it. It falls by 12%:
309 links disappeared, 21 appeared. Sampling them shows two mechanisms, both the rule working correctly on better data:
An edge drawn because the importing repository's own copy of a package hadn't been indexed was never a dependency. Fewer edges is a more truthful graph, and it sharpens open question 3: ambiguity grows as indexing coverage improves, so the one-layer rule discards more over time. My "seventy-five files" claim was wrongThe description says re-indexing was worth "75 Validation
Also in this push
|
There was a problem hiding this comment.
Review Summary
- Files reviewed:
CHANGELOG.md,docs/kg-cli-reference.md,docs/kg-graph-linking-design.md(docs-only PR, no code changes) - Cross-checked numeric claims for internal self-consistency (this PR's entire purpose is correcting stale/wrong figures) and re-verified the technical claims against
src/kg/internal/knowledge/indexer_treesitter.goandgraph_link.go - Overall verdict: one unreconciled arithmetic claim in the doc this PR is supposed to be making trustworthy
Major Issues (should fix)
docs/kg-graph-linking-design.md:34,41— The "Re-measured" table says DerivedDEPENDS_ONedges went 2,532 → 2,240, a drop of 292. The very next paragraph (line 41) says "309 previously-derived links disappeared and 21 appeared," a net change of -288. 292 ≠ 288 — the two numbers describing the same delta don't reconcile, a 4-edge gap. This is exactly the class of defect the PR exists to eliminate: elsewhere in this same doc (§Acceptance, "2,525 = 2,525, and the name count reproduced 845 exactly") the author explicitly holds these figures to exact-reconciliation standards, so an unreconciled ±4 here undercuts the credibility of the correction. Fix: recompute and correct either the table delta or the disappeared/appeared counts so they sum to the same number (e.g. if 309 disappeared and 21 appeared is right, the table's "After" value should be 2,532-309+21=2,244, not 2,240 — or the 309/21 sample counts need correcting).
Minor Issues (optional)
- PR description vs. shipped diff mismatch — The PR body states the estate holds "12,537
.scala... 75.java, 0.kt" files and "4,385 package entities with three or more dotted segments," while the actual doc text shipped in this diff (docs/kg-graph-linking-design.md:19,67-69) says 4,300 package entities and 30,906.scala, 7,591.kt, 1,835.javafiles — and explicitly frames "seventy-five.javafiles and no Kotlin" as the debunked figure from a buggyfind -maxdepth 4measurement. The PR description appears to be from an earlier push and asserts the very numbers the diff corrects. Fix: update the PR description to match the shipped doc content before merge, so reviewers/readers of the PR history aren't told the disproven figures are current.
Security Findings
- None (docs-only change).
Positive Observations
- The core technical claims check out against the code:
indexer_treesitter.go:279,370,380confirms Go and Scala both usePackageNodeTypes: []string{"package_clause"}while Java (package_declaration, line 318) and Kotlin (package_header, line 252) are separate node types — exactly the asymmetry the "warning this replaces was built on a wrong premise" section relies on. - The
.kotlin/.kttypo flagged in a prior review pass is gone — the whole file-count paragraph was rewritten and now correctly uses.kt. - No dangling references to the removed
#pending-re-measurementanchor remain anywhere indocs/. - Cross-document figures mostly reconcile well:
kg-cli-reference.md's "3,346 imports... 2,240 kept" (line 379) and "720k entities, 1.2M relations... 7 seconds and 1.3 GB" (line 402) match the design doc's "After" column exactly, and the "Same-layer, left alone" doubling claim (5,046→12,832) checks out arithmetically. - The reframing of the derived-edge drop ("fewer edges here is a more truthful graph, not a regression") is a defensible, well-argued interpretation backed by a concrete before/after example (
com.depop.common).
Question 3 still quoted the pre-re-index split (3,204 against 845) while the section above it reported 2,240 kept against 3,346 discarded, which read as the document contradicting itself. It also understated the problem. The discarded share grows as indexing coverage improves — a package indexed in more repositories resolves to more layers — so the linking weakens over time rather than strengthening. Records the option the re-measurement suggests: preferring the importing layer's own definition, which would have made com.depop.common same-layer and drawn nothing, instead of discarding it as ambiguous.
There was a problem hiding this comment.
Review Summary
- Files reviewed:
CHANGELOG.md,docs/kg-cli-reference.md,docs/kg-graph-linking-design.md(docs-only PR, no code changes) - Verified this push's technical claims against
src/kg/internal/knowledge/indexer_treesitter.go(package-node-type table, lines 247–384) and the new--scope/--personalflag docs againstsrc/kg/graph.goandsrc/kg/personal.go - Checked all figures in the new "Re-measured — 2026-08-29" section for internal arithmetic consistency, since that is this PR's stated purpose
- Confirmed the Major issue from an earlier review round (the
kg-graph-linking-design.md§Follow-up table asserting "Java, Kotlin, Scala … yes" unconditionally) is now consistent with reality: the doc states the estate actually was re-indexed (3ac2f1a), so the unconditional "yes" is no longer a contradiction — good catch by the author on the prior round.
Major Issues (should fix)
docs/kg-graph-linking-design.md:38,62-63— The "Federated merge" row in the Re-measured table gives Before/After as717,633 / 1,229,204→719,575 / 1,238,257, a delta of +1,942 entities / +9,053 relations. Two paragraphs later, the "What the re-index cost" section states the same re-index produced "entities +1,901 and relations +8,709" — off by 41 and 344 respectively, with no rounding involved (both are exact integers). This is the identical class of self-contradiction the PR exists to eliminate (already flagged once for the 2,532→2,240 vs. 309/21 delta), just in a different paragraph. Fix: recompute one of the two figures so the stated delta matches the table's before/after subtraction, or clarify if "+1,901/+8,709" is a genuinely different measurement (e.g. raw per-scope sum vs. federated/deduped total) and say so explicitly — as written, both read as describing the same re-index run.
Minor Issues (optional)
docs/kg-cli-reference.md:233-234— The new flags table adds--scopeand--personaltokg graphbut doesn't mention that--personaland--federatedare mutually exclusive (enforced insrc/kg/graph.go:275:"--personal and --federated are mutually exclusive: the personal store has no layers"). Since this table is the first place both flags appear together, a reader has no reason to expect the conflict until they hit the runtime error. Fix: add a one-line note near the--personalrow, e.g. "off | Read the personal store instead of this project's graph (mutually exclusive with--federated)".
Security Findings
- None (docs-only change).
Positive Observations
- The core technical narrative is verified accurate against code:
indexer_treesitter.goconfirms Go (.go, line 279) and Scala (.scala/.sc, lines 370/380) both usePackageNodeTypes: []string{"package_clause"}, while Java (line 318,package_declaration) and Kotlin (line 252,package_header) are distinct node types — exactly the asymmetry the "warning this replaces was built on a wrong premise" section relies on. --scopeand--personalare real, existing flags (src/kg/graph.go:421-422,src/kg/personal.go:45), not documentation of unshipped behavior.- The per-language linkability table in
kg-cli-reference.mdnow correctly splits Scala (linkable today) from Java/Kotlin (linkable only once re-indexed) instead of grouping all three under one unconditional "yes" — this resolves the contradiction flagged in an earlier review round. - Cross-document figures mostly reconcile: the "3,346 imports … 2,240 kept" and "~7 seconds and 1.3 GB" claims in
kg-cli-reference.mdcorrectly track the design doc's "After" column, not the stale "Before" figures a lazier update would have left in place.
Closes the "Pending re-measurement" item #15 opened, and corrects the claim it was built on.
The premise was wrong
That section said the derived-edge figures "cannot have come from an estate indexed by the binary that was current when they were written," because before
5211bc2the only package entitieskg indexminted were Go's bare identifiers, which the three-segment floor discards.Two facts contradict it:
package_clauseis Scala's tree-sitter node as well as Go's, andindexer_treesitter.go:599at5211bc2^already matched it generically.5211bc2added Java (package_declaration) and Kotlin (package_header) — it did not add Scala.v0.1.0-34-g6c5886e, hold 4,385 package entities with three or more dotted segments. Spot-checkingpackage:backend_driven.models.itemresolves it to.scalafiles undermobile-api-browse/app/backend_driven/models/item/.So the derived edges did come from indexed data — Scala's. The original draft understated Go; the correction overshot in the other direction. Both are now described in
§Follow-upso the next person doesn't re-run the same loop.Re-measured anyway
Against the estate with
kgat3ac2f1a:DEPENDS_ONedgesThe +7 is not noise — it's the wildcard-import fix that landed after the proposal shipped. A Java
import com.x.y.*;reaches the resolver as a string identical to the package name, and the original loop tested only proper prefixes, so every one was dropped silently.Estate size reads as 717,633 / 1,229,204 under the shipped join policy versus 667,858 / 1,223,605 in
§Problem. Same data, different merge —§Problem's numbers were taken when every entity type joined, which is the behaviour the proposal removed. Both are correct for their moment, and the doc now says so rather than looking self-contradictory.What the remedy would actually buy
The section asked for re-derivation "on a machine running
kgat5211bc2or later." That isn't sufficient: package entities are minted at index time, so a newer binary over older databases changes nothing. Re-indexing is what's required.And it's worth little for these numbers. By file count the estate holds 12,537
.scala(already indexed), 75.java, 0.kt. The genuinely unindexed mass is 2,634 Python, 852 TypeScript and 510 Go files, none of which5211bc2touched — that's the§Follow-upwork, unchanged.Also in here
kg-cli-reference.md: the staleness banner removed, the 3,359/2,525 split and load cost updated, and "JVM, Scala and Kotlin layers get derived links" replaced with a per-language table that distinguishes indexed today (Scala) from needs a re-index (Java, Kotlin) from not minted at all (TS/JS/Python/C++).Docs only — no code changes.