spec: cross-layer entity linking for kg graph --federated by bawoodruff · Pull Request #7 · Cortexa-LLC/mcp

bawoodruff · 2026-08-26T22:52:11Z

Spec only — no code. Stacked on #6 (base feat/kg-graph-federated). This is the design for the next slice, opened for review before implementation because the measurement behind it changes what that slice should be.

The finding that prompted it

I measured the graph #6 actually produces, against the real 61-layer estate (667,858 entities, 1,223,605 relations):


Relations crossing a layer boundary	67,263 (5.5%)
…existing only because of the `(name, type)` join	67,263 (100%)

Not one cross-layer relation was written by an indexer. Relations are stored per-database and nothing indexes two repos into one database, so the federated union is genuinely 61 disconnected components — every bridge in it is manufactured by the join.

And the manufacturing is not subtle. Top identifiers generating crossings:

Foundation      1567     print   528     forEach  227
CodingKeys      1079     map     294     models   221

Swift and JS boilerplate. They evade #6's --join-max-layers 3 guard by appearing in two or three layers rather than sixty.

The guard I built catches the widespread case and misses the local-name case entirely. The real distinction isn't how many layers a name appears in — it's whether names of that entity type are chosen to be globally meaningful. Package names are. Function names are not.

What's proposed

Per-type join policy replacing the single count guard. package and import join by default; function, file, topic never; type opt-in. --join-types overrides.
Real derived edges: an import in layer A naming a package in layer B becomes an explicit DEPENDS_ON, marked derived and drawn dashed, rather than fusing two nodes.
Provenance on every edge — Derived bool, counted in the header, so a reader can always tell a synthesised edge from a recorded one.

Measurement gate

§2 has an explicit gate before implementation: the estate holds 5,996 package and 66,878 import entities, and it is not yet known how many imports resolve to a package in another layer. If that's near zero, the honest conclusion is that layers are disconnected and --federated should be a comparison tool (find the same name across repos) rather than a connection tool. That measurement is the first task, not the first commit.

Bearing on #6

#6 is still worth merging — the merge, the guard, the ID-collision fix and the per-layer subgraph rendering all stand. But its default join produces the artifact measured above, so either its defaults change here, or its docs need to say plainly that cross-layer edges are name-derived and not dependencies. I'd take the former, in this PR.

Rollup

Deliberately deferred to a follow-up rather than included. Aggregating today's graph to layer granularity would produce a confident-looking dependency map built entirely on print and CodingKeys. Edges get fixed first.

bawoodruff · 2026-08-28T02:35:33Z

Measurement gate run — it passed, but only after changing the matching rule. Spec updated (66ea985).

The gate asked: does import→package resolution find anything real? Against the 61-layer estate — 5,138 distinct package names, 50,993 distinct import names:

Rule	Cross-layer matches	Unambiguous
Exact name match (what this spec proposed for v1)	71	25
Dotted prefix, min 3 segments	3,204	845
Slash prefix (npm / Go), min 2 segments	0	0

Exact matching fails. Its 71 hits are api, auth, client, clients, app — and clients alone is "defined in" 40 layers. It reproduces the boilerplate problem one level up, which is the thing this spec exists to fix.

Prefix matching passes, and finds real dependencies:

com.depop.auth.client.AccessToken       imported in [ads, attribution] -> com.depop.auth.client in [libraries]
com.depop.auth.client.AuthClient        imported in [martech, user]    -> com.depop.auth.client in [libraries]
com.depop.auth.client.ClientCredentials imported in [engage, feature]  -> com.depop.auth.client in [libraries]

Layer pairs come out as a plausible dependency map rather than a coincidence map: libraries—mobileapi 174, clients—product 151, libraries—product 102, libraries—search 83, libraries—user 80.

So §2's rule is now longest-dotted-prefix, minimum three segments so com.depop can't claim every JVM import, ambiguous targets skipped.

One finding worth its own attention: slash-separated ecosystems yield exactly zero, and the cause is upstream. Not one of the 5,138 package entity names contains a /. The indexers never mint package entities for npm or Go module paths, so 8,690 slash-style import names have nothing to resolve against — the web, client and tooling layers would get no derived edges at all. Recorded as an indexer follow-up rather than worked around here; it is probably a larger change than this proposal.

Expected yield: ~845 derived edges replacing 67,263 manufactured ones. Two orders of magnitude fewer, and each one explicable.

New open question in the spec: 74% of prefix matches (3,204 → 845) are discarded because the target package is defined in more than one layer — genuinely duplicated packages across repos. v1 proposes accepting the loss and reporting the count.

Four real runs since the reviewer went live show 40 is too snug, and that turn count tracks how far the reviewer wanders rather than how big the diff is: PR lines turns outcome #8 584 24 approved #5 2,704 35 posted a review <- largest diff, fewest turns #4 848 41 hit the 40 cap #7 989 41 hit the 40 cap The largest PR by a factor of three used the fewest turns, so sizing the cap to the diff is the wrong model. Both failures also stopped exactly AT the cap rather than near it, so the real ceiling is unknown — a snug cap will keep catching runs that were about to finish. Treat the cap as a runaway guard rather than a budget. Reviews run on a subscription OAuth token, so the reported total_cost_usd is notional rather than billed, and the binding limit is the job's timeout-minutes. At the observed ~6s/turn, 120 turns is roughly 12 minutes against a 30-minute wall clock, so the timeout still catches a genuine runaway. This matters more than a short review would, because exceeding the cap fails as a non-zero exit with an empty result, not as a truncated review — the run produces nothing at all. #4 and #7 currently have no review for that reason. num_turns is logged on every run, so if reviews start landing near 120 the signal is there rather than showing up as an unexplained silence. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions

Review Summary

Files reviewed: docs/kg-cli-reference.md, docs/kg-graph-linking-design.md, src/kg/graph.go, src/kg/graph_test.go, src/kg/internal/knowledge/graph.go, src/kg/internal/knowledge/graph_federated.go, src/kg/internal/knowledge/graph_federated_test.go, src/kg/internal/knowledge/graph_link.go (new), src/kg/internal/knowledge/graph_link_test.go (new), src/kg/internal/knowledge/graph_render.go, plus blast-radius reads of src/kg/internal/knowledge/indexer_treesitter.go.
Note: the PR description says "Spec only — no code," but the diff contains ~450 lines of new/changed Go (graph_link.go, graph_federated.go, graph.go, graph_render.go) plus tests, and the design doc's own header says "Status: implemented · gate run and passed." Reviewed as shipped code, since that's what's in the diff.
Overall verdict: REQUEST CHANGES

Critical Issues (must fix before merge)

src/kg/internal/knowledge/graph_link.go:29,131 vs src/kg/internal/knowledge/indexer_treesitter.go:599-609 — The entire feature is gated on package entities having ≥3 dot-separated segments (minPackageSegments = 3, checked in packageIndex), but the only place in this codebase that mints EntityTypePackage entities is the Go case nodeType == "package_clause" handler, and a Go package clause (package auth) is always a single bare identifier — it can never contain a dot, let alone three segments. No other language config in langConfig/walkNode (Java, Kotlin, Scala, JS, etc.) ever produces a package entity. This means packageIndex() filters out every package entity this repo's own indexer can produce, so LinkPackages can only ever return Derived: 0 against real data — it works only in the hand-built fixtures in graph_link_test.go that fabricate pkgNode("...", "com.depop.auth.client", ...) directly. This directly contradicts docs/kg-graph-linking-design.md's own factual claim in "Follow-up" — "the indexers mint package entities only for dotted namespaces" — which is the reverse of what the code does, and it means the doc's cited acceptance numbers (5,996/66,878 package/import entities, 2,525 derived edges, "gate run and passed") cannot have been produced by running this code against indexer_treesitter.go as it stands in this branch. Fix: either land the missing JVM/Scala package-declaration indexing this feature depends on in the same change (or as an explicit, referenced blocking dependency), or correct the design doc and re-run/re-state the measurement against what this PR actually ships.

Major Issues (should fix)

src/kg/internal/knowledge/graph_link.go:159-166 (resolvePackage) — The prefix loop for cut := len(parts) - 1; cut >= minPackageSegments; cut-- never tests cut == len(parts), i.e. it never checks whether the whole import name equals a package name — only proper prefixes of it. Per src/kg/internal/knowledge/indexer_treesitter.go:288-301, Java's extractImportPath returns only the scoped_identifier child and drops the trailing asterisk node for wildcard imports (import com.depop.auth.client.*; → import path "com.depop.auth.client", identical to the package name). Such an import can never resolve, silently, because the equal-length candidate is never tried. Compare Kotlin's extractImportPath (graph_link.go neighbor indexer_treesitter.go:229), which keeps the raw .* suffix verbatim, so the same wildcard-import pattern happens to work there only by accident of a different extractor. This is an under-documented gap in the "longest prefix match" rule stated in docs/kg-cli-reference.md ("the longest package name that is a dotted prefix of it") — a string is trivially a prefix of itself, but the code disagrees — and it's untested (graph_link_test.go has no case where importName == packageName). Fix: change the loop bound to cut := len(parts); cut >= minPackageSegments; cut-- (or equivalent), and add a test asserting an import that exactly equals a package name resolves.

Minor Issues (optional)

src/kg/internal/knowledge/graph_federated.go:166 / printFederationReport in src/kg/graph.go:360 — report.JoinTypes is stored with whatever casing the user passed to --join-types (matching is case-insensitive via lowerSet, but the stored/report slice isn't canonicalized), so --join-types Package,IMPORT prints Joining identities across layers for: Package, IMPORT instead of a normalized form. Cosmetic only; consider lower-casing before storing into report.JoinTypes for consistent report output.
PR description accuracy — the description states "Spec only — no code," which will mislead anyone using PR metadata to gauge review scope; either split the spec and implementation into separate PRs as the title suggests, or update the description to match what's actually shipped.

Security Findings

None found — this is pure in-memory graph computation over already-trusted local data; no new external input parsing, no injection surface.

Positive Observations

joinTypePolicy's nil-vs-empty-slice distinction for "use defaults" vs. "--join-types none" is a clean, well-tested piece of API design (src/kg/graph_test.go:238-262).
Determinism is taken seriously throughout graph_link.go (sorted iteration, lowest-ID tie-break, TestLinkPackagesIsDeterministic), which matters a lot for a tool whose output people diff between runs.
Derived-edge provenance (Derived bool, dashed rendering, header counts, --no-derived) is a good, honest design choice that keeps inferred edges visually distinct from recorded ones.
Ambiguity handling (skip rather than guess, with a reported sample of names) is the right default for a tool whose credibility depends on not manufacturing edges — consistent with the PR's own stated motivation.

…nTypes Two of the three findings from #7's first review. The Critical is not addressed here — see below. resolvePackage tested only PROPER prefixes: the loop started at len(parts)-1, so it never compared the whole import name against a package name. The documented rule is "the longest package name that is a dotted prefix of it", and a string is a prefix of itself. That gap has a specific, ordinary victim. Java wildcard imports arrive as exactly the package name — extractImportPath keeps only the scoped_identifier and drops the trailing asterisk, so `import com.depop.auth.client.*;` becomes "com.depop.auth.client". Every one of those silently failed to resolve. Kotlin escaped it by accident: its extractor keeps the ".*" verbatim, leaving a longer string that the proper-prefix loop does reach. Reproduced before fixing (Derived = 0, want 1) and mutation-checked: restoring len(parts)-1 fails the new test with that same figure. Also normalised report.JoinTypes to lower case. Matching was already case-insensitive via lowerSet, but the raw input was stored, so `--join-types Package,IMPORT` printed back "Package, IMPORT". Not fixed: the Critical, that minPackageSegments = 3 filters out every package entity this repo's indexer can produce. Verified — EntityTypePackage is written in exactly one place, the Go package_clause handler, and a Go package name is a bare identifier with no dots. No Kotlin/Java/Scala package declaration is indexed at all. So LinkPackages returns Derived: 0 against anything this indexer produced, and the design doc's claim that "the indexers mint package entities only for dotted namespaces" is backwards for this codebase. That one needs a decision rather than a patch: either land JVM package indexing, or correct the doc and re-state where the estate's 2,525 edges actually came from. Both change what this PR claims to be, so they are the author's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions

Review Summary

Files reviewed: docs/kg-cli-reference.md, docs/kg-graph-linking-design.md, src/kg/graph.go, src/kg/graph_test.go, src/kg/internal/knowledge/graph.go, src/kg/internal/knowledge/graph_federated.go, src/kg/internal/knowledge/graph_federated_test.go, src/kg/internal/knowledge/graph_link.go, src/kg/internal/knowledge/graph_link_test.go, src/kg/internal/knowledge/graph_render.go, plus blast-radius reads of src/kg/internal/knowledge/indexer_treesitter.go and kg_types.go.
The off-by-one prefix-match bug and the wildcard-import gap flagged in the previous review pass are fixed (resolvePackage now starts at cut := len(parts), with a dedicated regression test TestLinkPackagesResolvesAnImportEqualToThePackageName). That issue is resolved and not re-raised.
The previous review's Critical finding about package-entity minting is still unresolved (see below) — this PR does not touch indexer_treesitter.go, and the doc's "Follow-up" framing changed wording but the underlying factual gap remains.
A new, self-contained functional bug was found: --join-types none does not actually suppress derived cross-layer links, contradicting its own documentation and producing a self-contradictory federation report.

Critical Issues (must fix before merge)

src/kg/internal/knowledge/graph_link.go:29,125-131 vs src/kg/internal/knowledge/indexer_treesitter.go:599-609 (re-raised, still unfixed) — packageIndex filters package entities to segments(node.Name) >= minPackageSegments (3). Grepping the whole knowledge package for EntityTypePackage shows there is exactly one place that ever mints a package entity: indexer_treesitter.go:606, guarded by nodeType == "package_clause", which the adjacent comment says outright is Go-specific (// Go package: child is package_identifier). There is no package_declaration (Java), package_header (Kotlin), or Scala equivalent anywhere in the file. So in this codebase's actual indexer: (a) only Go files ever produce a package entity, and (b) Go's package name is always a single bare identifier (e.g. auth), which never has 3 dot-segments and is therefore always filtered out by packageIndex. The net effect is that LinkPackages can never derive a single edge against anything this repo's own indexer produces — it only works in graph_link_test.go's hand-built fixtures that fabricate pkgNode("...", "com.depop.auth.client", ...) directly. This directly contradicts docs/kg-cli-reference.md's claim ("JVM, Scala and Kotlin layers get derived links") and docs/kg-graph-linking-design.md's cited acceptance numbers (5,138 package names, 2,525 derived edges, "gate run and passed"), none of which can have come from running this shipped code against indexer_treesitter.go as it stands. Fix: land the missing JVM/Kotlin/Scala package-declaration indexing in the same change (or as an explicit, referenced blocking dependency before merge), or correct the design doc's status/acceptance claims to reflect that the feature is inert against this repo's indexer today.

Major Issues (should fix)

src/kg/internal/knowledge/graph_federated.go:323 vs src/kg/graph.go:360-386 — LinkPackages(merged) is called whenever !opts.NoDerived, completely independent of opts.JoinTypes/eligible (which only gates the joinable map used for identity fusion, computed at lines 163-174 and never passed into LinkPackages). packageIndex (graph_link.go:125) scans every package/import node in the merged graph by Name, regardless of whether that type was eligible to join. Consequence: kg graph --federated --join-types none — documented at docs/kg-graph-linking-design.md:169 as producing "union with no bridges at all", and at docs/kg-cli-reference.md:292 as joining "nothing" — still derives and draws DEPENDS_ON bridges between layers for every package defined in exactly one layer (the common case, since a package present in only one layer never even needs cross-layer identity-joining to be found by packageIndex). This also makes printFederationReport self-contradictory: graph.go:360-362 prints Joining identities across layers for: \n (nothing — layers are shown as the disconnected components they are) and then, a few lines later, graph.go:384 prints Derived N cross-layer package link(s), drawn dashed. for the same invocation. No test exercises this combination (graph_federated_test.go has no case combining an empty/none JoinTypes with an assertion on report.Link). Fix: skip LinkPackages (or filter packageIndex/import resolution) when package and/or import are not in the eligible join-types set, so --join-types none actually means zero bridges as documented; add a regression test asserting report.Link.Derived == 0 when JoinTypes excludes package/import.

Minor Issues (optional)

src/kg/internal/knowledge/graph_link.go:47-48 — The AmbiguousNames doc comment says "samples the package names responsible, worst first," but the implementation (graph_link.go ~200-207) collects names into a set, sort.Stringss them (alphabetical), then truncates to 10 — i.e. it's an alphabetical sample of arbitrary severity, not "worst first" by any ambiguity metric. Either rank by occurrence count before truncating, or fix the comment to say "sampled alphabetically."

Security Findings

None found.

Positive Observations

The previously-flagged off-by-one in resolvePackage's prefix loop is properly fixed, with a clear regression test and an explanatory comment referencing the Java-wildcard-import root cause.
LinkPackages determinism (sortedNodesOfType + lowest-ID tie-break + sorted AmbiguousNames) is well tested (TestLinkPackagesIsDeterministic).
Derived-edge provenance (Derived field, dashed rendering in both Mermaid and DOT, header counts) is implemented consistently and covered by TestDerivedEdgesRenderDistinctly.
joinTypePolicy's nil-vs-empty-slice distinction (default policy vs. "join nothing") is a good design and is directly unit tested.

The indexer minted a `package` entity for Go only. Go's package clause is a bare identifier ("auth"), while the JVM languages declare a dotted namespace ("com.depop.auth.client") — so on a Scala/Java estate the graph contained no package entities at all. That gap is why cross-layer package linking (#7) cannot work on the corpus it was designed for: matching an import to a package needs a namespace specific enough to be worth matching, and a bare Go identifier never is. Verified this handling has never existed here — `git log -S package_declaration` and `-S package_header` return nothing across all branches, langConfig had no package field, and EntityTypePackage was written at exactly one site. Generalise rather than add three more special cases. langConfig gains PackageNodeTypes and an optional extractPackageName; the switch arm that was hardcoded to Go's "package_clause" now consults the config like every other node kind. One default extractor covers all four languages, because each spells the name as a single named child and they differ only in its type: Go package_clause -> package_identifier Scala package_clause -> package_identifier Java package_declaration -> scoped_identifier Kotlin package_header -> identifier Node types were confirmed by parsing real sources through tree-sitter rather than read off the grammars — Scala reusing Go's node names, and Kotlin using a plain "identifier" for a dotted name, are both easy to get wrong from docs. Go behaviour is unchanged; it is now declared rather than hardcoded. Tests index a real file per language end to end, so each case covers the grammar node type, the extractor, and the config wiring together. All three mutations caught: removing either JVM node type fails that language, and narrowing the extractor to package_identifier fails Java and Kotlin while Go and Scala still pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Measuring the federated graph PR #6 produces says the join is wrong. Of the 67,263 relations that cross a layer boundary in the real 61-layer estate, all 67,263 exist only because of the (name, type) join — no indexer writes a relation across databases, so the union is genuinely disconnected and every bridge is manufactured. The identifiers manufacturing them are Foundation, CodingKeys, print, map, forEach: Swift and JS boilerplate that evades the --join-max-layers guard by appearing in two or three layers rather than sixty. The proposal is a per-type join policy (names identify packages, not functions) plus real derived DEPENDS_ON edges from import→package resolution, with a measurement gate before implementation because it is not yet known whether that resolution finds anything. Rollup is deliberately deferred to a follow-up: aggregating over manufactured edges would only make them look authoritative.

The gate the proposal put before its own implementation, run against the 61-layer estate. It changed the design rather than confirming it. Exact package-name matching, which the spec proposed for v1, fails: 71 cross-layer hits, and they are api/auth/client/clients — the boilerplate problem one level up. Longest-dotted-prefix matching with a three-segment minimum finds 845 unambiguous cross-layer dependencies, and they are real: services importing com.depop.auth.client resolving to the package in libraries. Layer pairs come out as a plausible dependency map. Slash-separated ecosystems yield exactly zero, and the reason is upstream: not one of the 5,138 package entity names contains a "/", so npm and Go imports have nothing to resolve against. Recorded as an indexer follow-up rather than worked around here. Expected yield: ~845 derived edges replacing 67,263 manufactured ones.

…10) * feat(kg): per-type join policy and derived cross-layer package links Implements docs/kg-graph-linking-design.md. The join is now keyed on entity type rather than on a count alone. Names identify packages and imports across repositories because that is what those names are for; they do not identify functions, files or documentation headings. Measured before this change, every one of the 67,263 cross-layer relations in the estate came from the latter kind — Foundation, CodingKeys, print, map, forEach. DefaultJoinTypes is package and import; --join-types widens or narrows it, and an explicitly empty policy joins nothing. Joining says two rows are the same thing, which is still not a dependency. LinkPackages recovers those from what the indexers already record: an import named com.depop.auth.client.AuthClient resolves to the package com.depop.auth.client in another layer, and becomes a DEPENDS_ON edge. Longest prefix wins, with a three-segment floor so com.depop cannot claim every JVM import in the estate, and an ambiguous target is skipped rather than guessed at — on the estate that discards more matches (3,359) than it keeps (2,525). Derived edges carry a flag and render dashed, with their own line in the header. An inference and a recorded fact must not arrive looking alike. On the estate: 2,525 derived links replacing 67,263 manufactured ones, and `--root package:com.depop.auth.client` now draws the eleven layers that depend on the shared auth library. * feat(kg): --join-types and --no-derived --join-types none has to mean "join nothing" while an unset flag means "the default policy"; joinTypePolicy keeps those apart, and is separated from cobra state so that distinction is testable without driving a command. The federated report now says which types were eligible to join, how many links were derived, and how many imports were skipped as ambiguous with examples — a graph that quietly dropped the majority of its matches would read as complete. * docs(kg): document the join policy and derived links Records the corrected yield figure too. The gate predicted 845 and the implementation produces 2,525; both are right and the units differ — 845 distinct import names, 2,525 import nodes, since a name appears separately in each layer that imports it. Verified by recomputing the rule independently against the shipped output.

…oins Rebasing onto #5 brought this test alongside #10's per-type join policy, which restricts joining to package and import — "this type's names are local to a repository; two matches are a coincidence, not an identity". The fixture used `type`, which no longer joins, so it reported 3 config nodes instead of 2: nothing fused, but only because nothing joined at all. The test would have kept passing while exercising none of the path it was written for. Switch the fixture to two packages named "config" in one layer plus a third in another — internal/api/config alongside internal/worker/config is an ordinary Go layout, so this is a more representative case than the original, not just a working one. Add a premise guard: assert JoinTypes actually covers package before drawing any conclusion from the count. Without it, a future narrowing of the policy silently turns this back into a test that passes for the wrong reason, which is exactly what the rebase just did. Both directions mutation-checked: dropping the same-layer guard fails with "config nodes = 1" (the fusion this test exists to catch), and narrowing DefaultJoinTypes to import alone fails the premise guard rather than passing vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…nTypes Two of the three findings from #7's first review. The Critical is not addressed here — see below. resolvePackage tested only PROPER prefixes: the loop started at len(parts)-1, so it never compared the whole import name against a package name. The documented rule is "the longest package name that is a dotted prefix of it", and a string is a prefix of itself. That gap has a specific, ordinary victim. Java wildcard imports arrive as exactly the package name — extractImportPath keeps only the scoped_identifier and drops the trailing asterisk, so `import com.depop.auth.client.*;` becomes "com.depop.auth.client". Every one of those silently failed to resolve. Kotlin escaped it by accident: its extractor keeps the ".*" verbatim, leaving a longer string that the proper-prefix loop does reach. Reproduced before fixing (Derived = 0, want 1) and mutation-checked: restoring len(parts)-1 fails the new test with that same figure. Also normalised report.JoinTypes to lower case. Matching was already case-insensitive via lowerSet, but the raw input was stored, so `--join-types Package,IMPORT` printed back "Package, IMPORT". Not fixed: the Critical, that minPackageSegments = 3 filters out every package entity this repo's indexer can produce. Verified — EntityTypePackage is written in exactly one place, the Go package_clause handler, and a Go package name is a bare identifier with no dots. No Kotlin/Java/Scala package declaration is indexed at all. So LinkPackages returns Derived: 0 against anything this indexer produced, and the design doc's claim that "the indexers mint package entities only for dotted namespaces" is backwards for this codebase. That one needs a decision rather than a patch: either land JVM package indexing, or correct the doc and re-state where the estate's 2,525 edges actually came from. Both change what this PR claims to be, so they are the author's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…sign doc The reviewer's Critical was that this feature could not derive anything from real data: minPackageSegments filtered out every package entity the indexer produced, because Go's bare identifier was the only source and never has a dot. That was accurate. JVM package indexing has since landed separately, so the dependency now exists and the claim can be demonstrated rather than argued. Add an end-to-end test that indexes actual Java source into two scope databases — a library layer declaring com.depop.auth.client, a consumer layer importing from it — federates them, and asserts a derived cross-layer edge. Nothing is hand-built: the graph is what `kg index` produces. Mutation-checked against the pre-indexing world: removing package_declaration from the Java config fails this test with "Derived = 0", which is exactly the symptom the review described. That makes the test a regression guard on the dependency, not just on this code. Correct the design doc's Follow-up, which stated the inverse of reality — that the indexers mint package entities "only for dotted namespaces", when Go's undotted name was the only kind minted. Replaced with a table of what each language actually yields and why Go cannot link, and the remaining npm/go.mod gap restated on that footing. Flag the acceptance figures rather than delete them. The 2,525 edges cannot have come from `kg index` as it shipped when they were recorded, for the reason above, so they are marked as predating JVM package indexing and needing a re-run against a freshly indexed estate. Re-deriving them needs that estate, so it is not something this change can settle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

github-actions

Review Summary

Reviewed the diff (gh pr diff 7, 1465 lines) plus blast-radius reads of src/kg/internal/knowledge/indexer_treesitter.go, kg_types.go, graph.go, graph_federated.go, graph_link.go, graph_link_test.go, graph_link_integration_test.go, graph_render.go, and both docs.

Both previously-flagged Critical issues are confirmed fixed:

Package-entity minting is no longer Go-only — indexer_treesitter.go (already in the base branch, not touched by this diff) now has PackageNodeTypes configured for Java (package_declaration), Kotlin (package_header) and Scala (package_clause), each emitting dotted namespaces that clear minPackageSegments. The new graph_link_integration_test.go::TestLinkPackagesDerivesFromIndexedJVMSource exercises this end-to-end through the real indexer rather than hand-built fixtures, and its own comment correctly documents that this test could not have passed before that indexer change landed.
The design doc's Follow-up section now states the corrected history plainly and adds an explicit caveat that the cited 2,525/845 figures predate JVM package indexing and haven't been reproduced since — no longer an unverifiable/contradicted claim.

The previously-flagged prefix-matching off-by-one (resolvePackage skipping the case where an import equals the package name, breaking Java wildcard imports) is also fixed — the loop now starts at cut := len(parts), with a dedicated regression test (TestLinkPackagesResolvesAnImportEqualToThePackageName) and an inline comment explaining why the Kotlin extractor happened to dodge the same bug.

No new Critical or Major issues found in this revision's changes (CLI flag plumbing in graph.go, Derived edge plumbing in graph.go/graph_render.go, and the new graph_link.go/tests).

Minor Issues (optional)

src/kg/graph.go:329-337 (joinTypePolicy) — --join-types="" (an explicit empty string, distinct from none) is Changed == true with values == [""], which doesn't match the len==1 && EqualFold(..., "none") branch, so it falls through to return values and eligible becomes {"": true}. Functionally this still joins nothing (no real type is named ""), but report.JoinTypes ends up [""] (length 1, not 0), so printFederationReport skips the "(nothing — layers are shown as the disconnected components they are)" message and instead prints Joining identities across layers for: with a blank value — a confusing report line for a rare but reachable input. Consider treating an empty/blank single value the same as "none".

Positive Observations

graph_link_test.go is thorough and each test's doc comment states the real-world scenario it guards against (ambiguity, split nodes by join-guard, determinism, wildcard imports, slash-namespace gap).
The new integration test closes the gap the previous review's Critical finding was about — it proves the feature against actual indexer output instead of only synthetic fixtures.
Derived-edge rendering (dashed mermaid/DOT, header counts) is covered by both unit assertions and a dedicated render test.

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.

…ion (#16) * docs(kg): re-measure the estate figures, and withdraw a wrong correction 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. * docs(kg): re-measure on a re-indexed estate; correct two miscounts 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. * docs(kg): bring the open questions in line with the re-measurement 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.

woodruffba mentioned this pull request Aug 26, 2026

feat(kg): kg graph --federated — one graph across a scope and all its layers #6

Merged

bawoodruff mentioned this pull request Aug 28, 2026

feat(kg): per-type join policy and derived cross-layer package links #10

Merged

Base automatically changed from feat/kg-graph-federated to feat/kg-graph-export August 28, 2026 03:11

bawoodruff force-pushed the feat/kg-graph-linking-spec branch from a63a985 to 4a95b48 Compare August 28, 2026 03:17

bawoodruff closed this Aug 28, 2026

bawoodruff reopened this Aug 28, 2026

This was referenced Aug 28, 2026

ci: raise the reviewer turn cap to 120 #11

Merged

ci: enforce gofmt, and format the files that were failing it #12

Merged

bawoodruff force-pushed the feat/kg-graph-export branch from bf17caf to ea1e8c4 Compare August 29, 2026 14:35

Base automatically changed from feat/kg-graph-export to main August 29, 2026 14:42

bawoodruff force-pushed the feat/kg-graph-linking-spec branch from 4a95b48 to dfe4572 Compare August 29, 2026 14:46

github-actions Bot requested changes Aug 29, 2026

View reviewed changes

bawoodruff mentioned this pull request Aug 29, 2026

feat(kg): index package declarations for Java, Kotlin, and Scala #14

Merged

bawoodruff and others added 6 commits August 29, 2026 14:59

bawoodruff force-pushed the feat/kg-graph-linking-spec branch from 66cffc7 to 03200e7 Compare August 29, 2026 22:01

github-actions Bot approved these changes Aug 29, 2026

View reviewed changes

bawoodruff merged commit 0ae49e6 into main Aug 29, 2026
6 checks passed

bawoodruff deleted the feat/kg-graph-linking-spec branch August 29, 2026 22:26

bawoodruff mentioned this pull request Aug 30, 2026

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

Merged

Navigation Menu

spec: cross-layer entity linking for kg graph --federated - #7

spec: cross-layer entity linking for kg graph --federated#7
bawoodruff merged 6 commits into
mainfrom
feat/kg-graph-linking-spec

bawoodruff commented Aug 26, 2026

Uh oh!

bawoodruff commented Aug 28, 2026

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot left a comment

Uh oh!

github-actions Bot left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

bawoodruff commented Aug 26, 2026

The finding that prompted it

What's proposed

Measurement gate

Bearing on #6

Rollup

Uh oh!

bawoodruff commented Aug 28, 2026

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Critical Issues (must fix before merge)

Major Issues (should fix)

Minor Issues (optional)

Security Findings

Positive Observations

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Critical Issues (must fix before merge)

Major Issues (should fix)

Minor Issues (optional)

Security Findings

Positive Observations

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Review Summary

Minor Issues (optional)

Positive Observations

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant