Skip to content

docs(kg): re-measure the estate figures, and withdraw a wrong correction - #16

Merged
bawoodruff merged 3 commits into
mainfrom
docs/kg-linking-remeasured
Aug 30, 2026
Merged

bawoodruff merged 3 commits into
mainfrom
docs/kg-linking-remeasured

Conversation

@bawoodruff

Copy link
Copy Markdown
Contributor

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 5211bc2 the only package entities kg index minted were Go's bare identifiers, which the three-segment floor discards.

Two facts contradict it:

  • package_clause is Scala's tree-sitter node as well as Go's, and indexer_treesitter.go:599 at 5211bc2^ already matched it generically. 5211bc2 added Java (package_declaration) and Kotlin (package_header) — it did not add Scala.
  • The estate's databases, indexed 2026-08-26 by v0.1.0-34-g6c5886e, hold 4,385 package entities with three or more dotted segments. Spot-checking package:backend_driven.models.item resolves it to .scala files under mobile-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-up so the next person doesn't re-run the same loop.

Re-measured anyway

Against the estate with kg at 3ac2f1a:

Figure Was Now
Derived DEPENDS_ON edges 2,525 2,532
Ambiguous imports discarded 3,359 3,355
Same-layer resolutions 5,031 5,046
Full-load cost ~6 s, 1.2 GB 7.6 s, 1.30 GB

The +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 kg at 5211bc2 or 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 which 5211bc2 touched — that's the §Follow-up work, 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++).
  • All internal anchors in the design doc verified to resolve.

Docs only — no code changes.

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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go and graph_link.go to 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:
    | Java, Kotlin, Scala | dotted namespace (`com.depop.auth.client`) | yes |
    
    That directly contradicts the correction this PR just made three sections above (§Re-measured, lines 43-53) and in 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. the kg-cli-reference.md table and indexer_treesitter.go's .kt/.kts registry 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.go confirms Go and Scala both use PackageNodeTypes: []string{"package_clause"} while Java uses package_declaration and Kotlin uses package_header — exactly the asymmetry the PR's narrative depends on.
  • The wildcard-import fix explanation matches the current resolvePackage implementation and its comment in graph_link.go:156-176 (loop starts at len(parts), not len(parts)-1, specifically to catch import com.x.y.*;-style imports).
  • The anchor rewrite is correct and complete: the old #pending-re-measurement anchor has no remaining references anywhere in docs/, and the new #re-measured--2026-08-29 anchor 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; 3ac2f1a matches 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.
@bawoodruff

Copy link
Copy Markdown
Contributor Author

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 count

The expectation on record — including in my own description above — was that indexing more package declarations would raise it. It falls by 12%:

Figure Before re-index After
Package entities 5,138 7,031
…linkable (3+ segments) 4,300 6,140
Derived DEPENDS_ON edges 2,532 2,240
Distinct import names resolving 845 557
Same-layer, left alone 5,046 12,832
Ambiguous, skipped 3,355 3,346

309 links disappeared, 21 appeared. Sampling them shows two mechanisms, both the rule working correctly on better data:

  • com.depop.common was indexed only in libraries, so every clients import of it looked like a cross-repo dependency. Kotlin indexing means clients declares it too — two layers, so the one-layer rule abstains instead of guessing.
  • Longest-prefix now finds a specific local package (com.depop.backenddrivenui.models.parameter) where it previously settled for a shorter name defined elsewhere — same-layer, no edge. Hence that count more than doubling.

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 wrong

The description says re-indexing was worth "75 .java files and no Kotlin". That came from find -maxdepth 4, which misses source trees nested deeper than four levels — the entire Android app among them. Actual counts: 30,906 .scala, 7,591 .kt, 1,835 .java. 5211bc2 gave package declarations to roughly nine thousand files, not seventy-five. Corrected in the doc.

Validation

  • Re-index: 971 s, 61 scopes, no failures. Entities +1,901, relations +8,709, no scope lost any.
  • All 420 observations preserved — selective re-index works as designed. (Their journals were empty, so I captured them separately first; the backup went unused.)
  • kg health across all 61 scopes: 723,545 entities, 1,246,614 relations, 6,203 orphans (0.86%), zero untimestamped observations.
  • Derived links still render dashed and grouped by layer on the rebuilt graph.
  • make test green.

Also in this push

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.go and graph_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 Derived DEPENDS_ON edges 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 .java files — and explicitly frames "seventy-five .java files and no Kotlin" as the debunked figure from a buggy find -maxdepth 4 measurement. 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,380 confirms Go and Scala both use PackageNodeTypes: []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/.kt typo 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-measurement anchor remain anywhere in docs/.
  • 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.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/--personal flag docs against src/kg/graph.go and src/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 as 717,633 / 1,229,204719,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 --scope and --personal to kg graph but doesn't mention that --personal and --federated are mutually exclusive (enforced in src/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 --personal row, 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.go confirms Go (.go, line 279) and Scala (.scala/.sc, lines 370/380) both use PackageNodeTypes: []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.
  • --scope and --personal are 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.md now 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.md correctly track the design doc's "After" column, not the stale "Before" figures a lazier update would have left in place.

@bawoodruff
bawoodruff merged commit a212c92 into main Aug 30, 2026
6 checks passed
@bawoodruff
bawoodruff deleted the docs/kg-linking-remeasured branch August 30, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant