diff --git a/docs/kg-cli-reference.md b/docs/kg-cli-reference.md index 3df1c50..b1bfa55 100644 --- a/docs/kg-cli-reference.md +++ b/docs/kg-cli-reference.md @@ -233,6 +233,8 @@ Error: "main" matches 2 entities — use an ID instead: | `--federated` | off | Render the scope together with every *local* layer it federates with | | `--layer` | *(all)* | With `--federated`, load only these scopes | | `--join-max-layers` | `3` | With `--federated`, the boilerplate guard (see below) | +| `--join-types` | `package,import` | With `--federated`, entity types whose names identify across layers; `none` joins nothing | +| `--no-derived` | off | With `--federated`, omit derived cross-layer package links | `--rel` constrains what the walk follows, not just what is drawn, so filtering to `CONTAINS` gives the containment tree rather than the same neighbourhood with @@ -279,9 +281,19 @@ a plain union of the layers would be a set of disconnected components. What connects them is joining identities across layers. **The join rule.** Two rows in two databases are the same node when their -`(name, type)` match. That surfaces genuinely shared symbols, and equally -surfaces the same name implemented separately in several services — often the -more interesting answer: +`(name, type)` match **and the type is one whose names identify across +repositories** — by default `package` and `import`. + +That restriction is the whole game. A package name is chosen to be globally +unique; that is what a package name is for. A function name is not: measured on +the estate, joining every type produced 67,263 cross-layer relations, and *all* +of them came from names like `Foundation`, `CodingKeys`, `print`, `map` and +`forEach` meaning different things in different repositories. `--join-types` +widens or narrows the policy; `--join-types none` joins nothing and shows the +layers as the disconnected components they actually are. + +Within an eligible type, joining still surfaces the same name implemented +separately in several services — often the more interesting answer: ``` $ kg graph --federated --layer clients,libraries,mobileapi,payments \ @@ -321,6 +333,59 @@ renamed because two layers minted the same ID for different things. Indexer IDs are repo-relative paths, so `import:..` legitimately means something different in every layer. +#### Derived cross-layer links + +Joining says two rows are the same thing. It cannot say that one layer *depends +on* another — and nothing else can either, because relations live inside a +single database and no indexer writes one across repositories. Left at that, a +federated graph is a set of disconnected components. + +The dependency is recoverable, because the indexers already record both ends: a +layer holds an `import` entity named `com.depop.auth.client.AuthClient`, and +another layer holds a `package` entity named `com.depop.auth.client`. `kg graph +--federated` resolves the first to the second and adds a `DEPENDS_ON` edge, +**marked derived and drawn dashed** so it is never mistaken for a relation an +indexer recorded: + +```bash +kg graph --federated --root package:com.depop.auth.client --depth 1 +``` + +``` +%% kg graph: 12 node(s), 11 relation(s) of 717633 entities in the project +%% 11 of those are derived cross-layer links, drawn dashed + subgraph layer0["libraries"] + n0[["com.depop.auth.client"]]:::kgroot + end + subgraph layer1["ads"] + n1[/"com.depop.auth.client.AccessToken"/] + end + ... + n1 -.->|DEPENDS_ON| n0 +``` + +Eleven layers depending on one shared auth library — the question `--federated` +exists to answer. + +Resolution rules: + +- **Longest prefix wins**, with a **three-segment minimum**. Without the floor, + `com.depop` claims every JVM import in the estate and the graph gains one hub + node instead of a dependency map. +- **Ambiguity is skipped, never guessed.** If the matched package is defined in + more than one layer, no edge is drawn and the count is reported. On the estate + this discards 3,359 imports against 2,525 kept — the majority case, not an + edge case. +- **Same-layer resolutions are dropped**; that structure is already in the + layer's own graph. +- `--no-derived` turns the whole thing off, leaving only recorded relations. + +**Only dotted namespaces resolve.** Not one `package` entity in the measured +estate has a `/` in its name — the indexers do not mint package entities for npm +or Go module paths — so JVM, Scala and Kotlin layers get derived links and +JavaScript and Go layers get none. That is a gap in what is indexed rather than +in the matching, and closing it is an indexer change. + **Cost.** Databases are read one at a time and released, so peak memory is the merged graph plus the largest single layer rather than all of them at once. Measured on a 61-layer estate — 668k entities, 1.2M relations — a full load is diff --git a/docs/kg-graph-linking-design.md b/docs/kg-graph-linking-design.md new file mode 100644 index 0000000..78e7424 --- /dev/null +++ b/docs/kg-graph-linking-design.md @@ -0,0 +1,271 @@ +# Cross-layer entity linking for `kg graph` — Design Proposal + +**Status:** implemented · gate run and passed · **Date:** 2026-08-26 · **Follows:** PR #5 (`kg graph`), PR #6 (`--federated`) + +How to make a federated entity-relationship graph say something true. `--federated` +(PR #6) merges a scope and its layers into one graph; this proposal fixes *what +connects them*, because the current rule manufactures edges that nobody recorded. + +## Problem + +A federated render is meant to show how entities across an estate relate. Measured +against the real 61-layer estate at `~/Projects` — 667,858 entities, 1,223,605 +relations — it does not: + +| Measurement | Value | +|---|---| +| Relations crossing a layer boundary | 67,263 (5.5%) | +| …of those, existing **only** because of the `(name, type)` join | **67,263 (100%)** | +| …involving `node_modules` | 63 (0.1%) | + +Not one cross-layer relation was written by an indexer. That is not a bug in the +data: relations are stored per-database, and nothing indexes two repositories into +one database, so the federated union is genuinely 61 disconnected components. Every +bridge in the current picture is produced by the join. + +And the joins producing them are not shared symbols. The identifiers generating the +most crossings: + +``` +Foundation 1567 print 528 forEach 227 +CodingKeys 1079 Kr 478 models 221 +createDataFrame 539 map 294 hasError 300 +``` + +Swift and JavaScript boilerplate. They evade the existing `--join-max-layers 3` +guard precisely because they appear in two or three layers rather than sixty. + +**The shape of the failure:** a name-identity join is right for *coarse* entities +whose names are chosen to be globally meaningful (packages, modules, services) and +wrong for *fine* ones whose names are local (functions, fields, generated types). +The current rule applies it to everything. + +## Goals + +- **Cross-layer edges that correspond to something real** — a dependency that exists + in the source, not a name coincidence. +- **A join policy that is per-type and defensible**, defaulting to off for the entity + kinds where names are local. +- **No loss of the honest parts of PR #6** — duplicate-implementation discovery + (six `AddressClient` definitions in three layers) stays; it is just no longer + allowed to imply a call graph. +- **Measurable acceptance**: after the change, the number of cross-layer relations + that survive should be justifiable edge by edge. + +## Non-goals + +- **Inferring dependencies from source.** No new parsing; this works from what the + indexers already record. +- **Rollup/aggregation.** Deferred deliberately — see [Follow-up](#follow-up). + Aggregating over false edges launders the artifact rather than fixing it. +- **Changing single-scope rendering.** PR #5 behaviour is untouched. + +## Design + +### 1. Per-type join policy + +Replace the single `--join-max-layers` guard with a policy keyed on entity type. + +| Entity type | Join across layers | Why | +|---|---|---| +| `package` | **yes** | A package name is chosen to be globally unique; that is what a package name is *for*. | +| `import` | **yes** | An import names something outside the current repo by design. | +| `file` | no | Path-derived and repo-relative; `src/index.ts` is not one file. | +| `type` | opt-in | Sometimes meaningful (`AddressClient`), often not (`CodingKeys`). Off by default. | +| `function` | **no** | `print`, `map`, `forEach`. This is the type generating the false edges. | +| `topic` | no | Documentation headings — the boilerplate case already known. | + +The `--join-max-layers` count guard stays as a second filter on the types that do +join, since even a package name can turn out to be generic. + +`--join-types file,function` overrides the defaults for anyone who wants the old +behaviour, and `--join-types none` disables joining entirely. + +### 2. Package-identity linking — the real edges + +Joining is identity ("these are the same node"). Dependency is a *relation*, and +the indexers already record enough to recover it: + +- Layer A holds an `import` entity named `@depop/foo` (or `com.depop.foo`). +- Layer B holds a `package` entity named `@depop/foo`. + +That is a genuine cross-repo dependency, and it becomes an explicit relation rather +than a fused node: + +``` +(file in A) --IMPORTS--> (import:@depop/foo in A) ==> A's file --DEPENDS_ON--> B's package +``` + +**Matching rule (set by measurement — see below): longest namespace-prefix match.** +An import resolves to the longest `package` name that is a dotted prefix of it, with +a minimum of three segments so that `com.depop` cannot claim every JVM import in the +estate. + +Rules: + +- Longest-prefix wins; minimum three segments; ties are impossible by construction. +- **Ambiguity is skipped, not guessed.** If the matched package name is defined in + more than one layer, no edge is drawn and the name is reported. This is not a rare + path: it discards 74% of otherwise-matching imports (below). +- The synthesised edge is typed `DEPENDS_ON` and marked as **derived**, so a reader + can tell it from a relation an indexer wrote. Derived edges are dashed in mermaid + and DOT. +- Self-links (an import resolving to a package in the same layer) are dropped — + that structure is already in the layer's own graph. + +### Measurement gate — result + +Run against the 61-layer estate (5,138 distinct package names, 50,993 distinct import +names) before writing any of the above. The gate was: does import→package resolution +find anything real? + +| Rule | Cross-layer matches | Unambiguous | +|---|---|---| +| Exact name match (the original v1 proposal) | 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 generic package fragments — `api`, `auth`, +`client`, `clients`, `app` — and `clients` alone is "defined in" 40 layers. It +reproduces the boilerplate problem one level up. + +**Prefix matching passes, and finds real dependencies:** + +``` +com.depop.auth.client.AccessToken imported in [ads, attribution] -> package com.depop.auth.client in [libraries] +com.depop.auth.client.AuthClient imported in [martech, user] -> package com.depop.auth.client in [libraries] +com.depop.auth.client.ClientCredentials imported in [engage, feature] -> package com.depop.auth.client in [libraries] +``` + +Services depending on the shared auth client. The resulting layer pairs are a +plausible dependency map rather than a name-coincidence map: `libraries—mobileapi` +(174), `clients—product` (151), `libraries—product` (102), `libraries—search` (83), +`libraries—user` (80). + +**Slash-separated ecosystems get nothing, and the reason 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 the 8,690 slash-style import names have +nothing to resolve against. This is a limitation of what is indexed, not of the +matching rule; see [Follow-up](#follow-up). + +### 3. Provenance on every edge + +`GraphEdge` gains: + +```go +// Derived marks an edge synthesised by cross-layer linking rather than read +// from a database. A reader must be able to tell the difference. +Derived bool `json:"derived,omitempty"` +``` + +Renderers draw derived edges dashed. JSON carries the flag. The header comment +counts them: `%% kg graph: 412 nodes, 690 relations (37 derived)`. + +## CLI surface + +```bash +kg graph --federated --root @depop/some-lib --depth 2 # who depends on this library +kg graph --federated --join-types package,import,type # widen the join +kg graph --federated --join-types none # union with no bridges at all +kg graph --federated --no-derived # only relations an indexer wrote +``` + +| Flag | Default | Effect | +|---|---|---| +| `--join-types` | `package,import` | Entity types eligible for cross-layer identity join | +| `--join-max-layers` | `3` | Unchanged; a second filter on the eligible types | +| `--no-derived` | off | Suppress synthesised `DEPENDS_ON` edges | + +## Implementation sketch + +- `internal/knowledge/graph_federated.go`: `joinable` becomes a per-type policy + rather than a single count threshold. +- New `internal/knowledge/graph_link.go`: `LinkPackages(g *Graph) []GraphEdge` — a + pure function over the merged graph, so it is testable without databases, in + keeping with the rest of the graph code. +- `FederationReport` gains `DerivedEdges int` and `AmbiguousPackages []string`. +- Renderers: dashed styling for `Derived`. + +## Test plan + +- Per-type join policy: a `function` named `print` in three layers stays three nodes; + a `package` named `@depop/foo` in two layers becomes one. +- Package linking: an import in layer A resolving to a package in layer B produces + one derived `DEPENDS_ON`; a same-layer resolution produces none; an ambiguous + package name produces none and is reported. +- Derived edges survive JSON round-trip and render dashed in both formats. +- Regression: the estate's top false-edge generators (`Foundation`, `CodingKeys`, + `print`) produce zero cross-layer relations under the new defaults. + +## Acceptance + +Re-run the measurement. The success condition is not "more edges" — it is that +**every surviving cross-layer relation can be traced to a package an import names**, +and the count of name-coincidence edges is zero under default settings. + +Yield on the estate, as implemented: **2,525 derived `DEPENDS_ON` edges**, +replacing 67,263 manufactured ones. + +> **These figures predate JVM package indexing and have not been reproduced +> since.** They cannot have come from `kg index` as it shipped at the time — +> the only package entities it minted were Go's undotted names, which this +> rule filters out. Re-run the measurement against a freshly indexed estate +> before treating the numbers as current. + +The gate predicted 845, and both numbers are right: 845 is the count of distinct +import *names* that resolve, while 2,525 counts import *nodes* — the same name +appears as a separate node in each layer that imports it, since imports join only +across three layers or fewer. Verified by recomputing the rule independently +against the shipped output: 2,525 = 2,525, and the name count reproduces 845 +exactly. + +## Follow-up + +**Package entities for npm and Go.** What the indexers mint is now: + +| language | package entity | linkable | +|---|---|---| +| Java, Kotlin, Scala | dotted namespace (`com.depop.auth.client`) | yes | +| Go | bare identifier (`auth`) | no — one segment, below the specificity floor | +| TypeScript, JavaScript, Python, C/C++ | none | no | + +An earlier draft of this section had that backwards, saying the indexers mint +package entities "only for dotted namespaces". The reverse was true when it was +written: Go's `package_clause` was the *only* source, and a Go package name never +contains a dot, so every package entity was filtered out and this proposal +derived nothing from indexed data. JVM package declarations were not indexed at +all until they were added separately (see the package-indexing change that +precedes this one), which is what makes the rule above work. + +The remaining gap is npm and Go. Reading `package.json` `name` fields and +`go.mod` module paths would give the web, client and tooling layers a namespace +specific enough to link, where today they get no derived edges. That is an +indexer change, independent of this proposal and probably larger than it. + +Once edges are real, aggregation becomes worth building — `--rollup layer|package`, +collapsing the graph to a granularity a person can read (tens of nodes, weighted +edges), with `--limit` capping groups rather than entities. Aggregating today's +graph would only make the artifacts look authoritative, which is why it is second +and not first. + +## Open questions + +1. ~~Does package linking find anything?~~ **Answered by the gate:** yes, with + prefix matching (845 unambiguous cross-layer edges); no, with exact matching (71, + mostly generic). The matching rule in §2 changed accordingly. +2. ~~Naming conventions across ecosystems.~~ **Answered:** dotted namespaces resolve; + npm and Go produce nothing because no package entity name contains a `/`. Fixing + that is an indexer change — see [Follow-up](#follow-up). +3. **Is 74% ambiguity acceptable?** Prefix matching finds 3,204 cross-layer + resolutions but only 845 survive the "package defined in exactly one layer" rule. + The discarded ones are mostly packages genuinely duplicated across repos. Options: + accept the loss, draw them to all candidate layers with a marker, or rank by layer + priority. Proposal: accept the loss in v1 and report the count. +4. **Should `type` join by default?** `AddressClient` argues yes, `CodingKeys` argues + no. Proposal: off, revisited once derived edges exist and can be compared against. + +## See also + +- [kg-cli-reference.md](kg-cli-reference.md#kg-graph) — the shipped command +- PR #5 — `kg graph` +- PR #6 — `--federated`, whose join this proposal corrects diff --git a/src/kg/graph.go b/src/kg/graph.go index 3b5c5a5..acf011a 100644 --- a/src/kg/graph.go +++ b/src/kg/graph.go @@ -6,6 +6,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/cortexa-llc/mcp/kg/internal/knowledge" "github.com/spf13/cobra" @@ -32,6 +33,8 @@ var ( graphFederated bool graphLayers []string graphMaxJoin int + graphJoinTypes []string + graphNoDerived bool ) // graphSettings is one invocation's flags, kept separate from the flag @@ -90,7 +93,7 @@ Examples: // Silently ignoring these would render one database and look like it // had honoured the request. if !graphFederated { - for _, flag := range []string{"layer", "join-max-layers"} { + for _, flag := range []string{"layer", "join-max-layers", "join-types", "no-derived"} { if cmd.Flags().Changed(flag) { return fmt.Errorf("--%s only applies with --federated", flag) } @@ -259,6 +262,14 @@ func renderLoadedGraph(graph *knowledge.Graph, s graphSettings, out io.Writer) ( } // runFederatedGraph renders across a scope and every layer it federates with. +// ambiguousSuffix names a few of the packages that could not be resolved. +func ambiguousSuffix(names []string) string { + if len(names) == 0 { + return "." + } + return ", e.g. " + strings.Join(names[:min(3, len(names))], ", ") + "." +} + func runFederatedGraph(cmd *cobra.Command, s graphSettings, out io.Writer) (knowledge.Subgraph, error) { if usePersonal { return knowledge.Subgraph{}, fmt.Errorf("--personal and --federated are mutually exclusive: the personal store has no layers") @@ -294,6 +305,8 @@ func runFederatedGraph(cmd *cobra.Command, s graphSettings, out io.Writer) (know ProjectID: filepath.Base(root), OnlyLayers: graphLayers, MaxJoinLayers: graphMaxJoin, + JoinTypes: parseJoinTypes(cmd), + NoDerived: graphNoDerived, }) if err != nil { return knowledge.Subgraph{}, err @@ -303,6 +316,26 @@ func runFederatedGraph(cmd *cobra.Command, s graphSettings, out io.Writer) (know return renderLoadedGraph(graph, s, out) } +// parseJoinTypes turns --join-types into the loader's policy. An unset flag +// means the default policy; "none" means join nothing, which is distinct from +// unset and has to survive as an empty-but-not-nil slice. +func parseJoinTypes(cmd *cobra.Command) []string { + return joinTypePolicy(cmd.Flags().Changed("join-types"), graphJoinTypes) +} + +// joinTypePolicy is parseJoinTypes without the cobra state, so the distinction +// it exists to preserve can be tested directly: nil means "the default policy", +// while an empty-but-not-nil slice means "join nothing". +func joinTypePolicy(changed bool, values []string) []string { + if !changed { + return nil + } + if len(values) == 1 && strings.EqualFold(strings.TrimSpace(values[0]), "none") { + return []string{} + } + return values +} + // printFederationReport writes what the federated load did to stderr, keeping // stdout a clean document. A merged graph hides two things a reader would want // to know — a layer that failed to open, and identities too widespread to @@ -324,6 +357,10 @@ func printFederationReport(cmd *cobra.Command, report *knowledge.FederationRepor } cmd.PrintErrf("Federated %d layer(s): %d node(s), %d relation(s).\n", contributing, nodes, edges) + cmd.PrintErrf("Joining identities across layers for: %s\n", strings.Join(report.JoinTypes, ", ")) + if len(report.JoinTypes) == 0 { + cmd.PrintErrln(" (nothing — layers are shown as the disconnected components they are)") + } if report.Joined > 0 { cmd.PrintErrf("Joined %d identit%s across layers, merging %d duplicate row(s).\n", report.Joined, plural(report.Joined, "y", "ies"), report.MergedNodes) @@ -343,6 +380,16 @@ func printFederationReport(cmd *cobra.Command, report *knowledge.FederationRepor "IDs from repo-relative paths, so the same ID can mean different things in different layers.\n", report.IDCollisions) } + if link := report.Link; link.Derived > 0 || link.Ambiguous > 0 { + cmd.PrintErrf("Derived %d cross-layer package link(s), drawn dashed.\n", link.Derived) + if link.SameLayer > 0 { + cmd.PrintErrf(" %d import(s) resolved inside their own layer and were left alone.\n", link.SameLayer) + } + if link.Ambiguous > 0 { + cmd.PrintErrf(" %d import(s) matched a package defined in more than one layer and were skipped "+ + "rather than guessed at%s\n", link.Ambiguous, ambiguousSuffix(link.AmbiguousNames)) + } + } for _, failed := range report.FailedLayers() { cmd.PrintErrf("Warning: layer %s could not be read and is missing from this graph: %s\n", failed.Name, failed.Failed) @@ -377,6 +424,10 @@ func init() { "Render the scope together with every local layer it federates with, joining shared identities across them (remote hub layers are search-only)") graphCmd.Flags().StringSliceVar(&graphLayers, "layer", nil, "With --federated, restrict the load to these scopes instead of all layers") + graphCmd.Flags().StringSliceVar(&graphJoinTypes, "join-types", knowledge.DefaultJoinTypes, + "With --federated, entity types whose names identify the same thing across layers; \"none\" to join nothing") + graphCmd.Flags().BoolVar(&graphNoDerived, "no-derived", false, + "With --federated, omit derived cross-layer package links, leaving only relations an indexer recorded") graphCmd.Flags().IntVar(&graphMaxJoin, "join-max-layers", knowledge.DefaultMaxJoinLayers, "With --federated, a name found in more than this many layers is boilerplate, not one shared symbol, and is left unjoined") registerPersonalFlag(graphCmd) diff --git a/src/kg/graph_test.go b/src/kg/graph_test.go index 4e21bd5..d22b614 100644 --- a/src/kg/graph_test.go +++ b/src/kg/graph_test.go @@ -220,6 +220,8 @@ func TestRunFederatedGraphValidatesSettingsFirst(t *testing.T) { func TestGraphCommandFederationFlags(t *testing.T) { for _, tc := range []struct{ flag, want string }{ {flag: "federated", want: "false"}, + {flag: "no-derived", want: "false"}, + {flag: "join-types", want: "[" + strings.Join(knowledge.DefaultJoinTypes, ",") + "]"}, {flag: "join-max-layers", want: strconv.Itoa(knowledge.DefaultMaxJoinLayers)}, {flag: "layer", want: "[]"}, } { @@ -234,6 +236,31 @@ func TestGraphCommandFederationFlags(t *testing.T) { } } +// "none" has to survive as an empty-but-not-nil policy: nil means "use the +// default types", and conflating the two would silently join everything the +// user asked to join nothing. +func TestJoinTypePolicy(t *testing.T) { + if got := joinTypePolicy(false, knowledge.DefaultJoinTypes); got != nil { + t.Errorf("unset flag = %v, want nil so the default policy applies", got) + } + + got := joinTypePolicy(true, []string{"none"}) + if got == nil { + t.Fatal("\"none\" produced nil, which means the default policy — it must join nothing") + } + if len(got) != 0 { + t.Errorf("\"none\" = %v, want an empty policy", got) + } + + if got := joinTypePolicy(true, []string{" NONE "}); got == nil || len(got) != 0 { + t.Errorf("case and spacing changed the meaning of \"none\": %v", got) + } + + if got := joinTypePolicy(true, []string{"package", "type"}); strings.Join(got, ",") != "package,type" { + t.Errorf("explicit types = %v, want [package type]", got) + } +} + func commandRegistered(t *testing.T, name string) bool { t.Helper() for _, c := range rootCmd.Commands() { diff --git a/src/kg/internal/knowledge/graph.go b/src/kg/internal/knowledge/graph.go index a8da1bd..8a008da 100644 --- a/src/kg/internal/knowledge/graph.go +++ b/src/kg/internal/knowledge/graph.go @@ -43,6 +43,12 @@ type GraphEdge struct { FromID string `json:"from_id"` ToID string `json:"to_id"` Type string `json:"type"` + + // Derived marks an edge this tool worked out rather than read from a + // database — today, cross-layer package links. A reader has to be able to + // tell the two apart, so renderers draw derived edges dashed and the + // header counts them separately. + Derived bool `json:"derived,omitempty"` } // Graph is a whole project's entities and relations held in memory. diff --git a/src/kg/internal/knowledge/graph_federated.go b/src/kg/internal/knowledge/graph_federated.go index 911b088..6ba839a 100644 --- a/src/kg/internal/knowledge/graph_federated.go +++ b/src/kg/internal/knowledge/graph_federated.go @@ -4,6 +4,7 @@ import ( "fmt" "path/filepath" "sort" + "strings" ) // Federated graph loading — one Graph built from a scope and every layer it @@ -32,6 +33,22 @@ import ( // of places, while template text appears in everything. const DefaultMaxJoinLayers = 3 +// DefaultJoinTypes are the entity types whose names identify the same thing +// across databases. +// +// The distinction is not how widespread a name is but whether names of that +// kind are chosen to be globally meaningful. A package name is: that is what a +// package name is for. A function name is not — measured on a real estate, the +// identifiers producing the most cross-layer joins were Foundation, +// CodingKeys, print, map and forEach, none of which means the same thing in +// two repositories. Files are path-derived, and topics are documentation +// headings. +// +// Types are excluded by default and available through --join-types: some +// genuinely identify (AddressClient), many do not (CodingKeys), and there is +// no way to tell from the name alone. +var DefaultJoinTypes = []string{EntityTypePackage, EntityTypeImport} + // FederatedGraphOptions describes which databases to assemble into one graph. type FederatedGraphOptions struct { // AIDir is the .ai directory holding the scope databases. @@ -45,6 +62,13 @@ type FederatedGraphOptions struct { OnlyLayers []string // MaxJoinLayers is the boilerplate guard; zero means DefaultMaxJoinLayers. MaxJoinLayers int + // JoinTypes are the entity types eligible for cross-layer identity + // joining. Nil means DefaultJoinTypes; an explicitly empty slice joins + // nothing, leaving the layers as the disconnected components they are. + JoinTypes []string + // NoDerived suppresses cross-layer package linking, leaving only relations + // an indexer actually recorded. + NoDerived bool } // LayerLoad is one layer's contribution to a federated graph. @@ -78,6 +102,10 @@ type FederationReport struct { // IDCollisions counts nodes whose ID already existed in another layer and // were renamed to keep them distinct. IDCollisions int `json:"id_collisions,omitempty"` + // JoinTypes are the entity types that were eligible to join. + JoinTypes []string `json:"join_types"` + // Link records what cross-layer package linking derived. + Link LinkReport `json:"link"` } // FailedLayers returns the layers that could not be read. A federated render @@ -132,8 +160,26 @@ func LoadFederatedGraph(opts FederatedGraphOptions) (*Graph, *FederationReport, } } + joinTypes := opts.JoinTypes + if joinTypes == nil { + joinTypes = DefaultJoinTypes + } + eligible := lowerSet(joinTypes) + // Lower-cased for the report: matching is already case-insensitive via + // lowerSet, but storing the raw input meant `--join-types Package,IMPORT` + // printed back "Package, IMPORT" instead of a normalised form. + report.JoinTypes = make([]string, 0, len(joinTypes)) + for _, t := range joinTypes { + report.JoinTypes = append(report.JoinTypes, strings.ToLower(t)) + } + joinable := make(map[ntKey]bool) for key, count := range layerCount { + if !eligible[strings.ToLower(key.Type)] { + // This type's names are local to a repository; two matches are a + // coincidence, not an identity. + continue + } switch { case count <= 1: // Nothing to join. @@ -273,6 +319,10 @@ func LoadFederatedGraph(opts FederatedGraphOptions) (*Graph, *FederationReport, }) } + if !opts.NoDerived { + report.Link = LinkPackages(merged) + } + // Joined nodes accumulate layers in load order; sorting makes the rendered // provenance stable. for id, node := range merged.nodes { diff --git a/src/kg/internal/knowledge/graph_federated_test.go b/src/kg/internal/knowledge/graph_federated_test.go index 3d29751..6e6dff4 100644 --- a/src/kg/internal/knowledge/graph_federated_test.go +++ b/src/kg/internal/knowledge/graph_federated_test.go @@ -57,9 +57,17 @@ func seedLayer(t *testing.T, aiDir, name string, layers []string, entities ...fe } } -// loadFed is the common call under test. +// loadFed is the common call under test, with the default join policy. func loadFed(t *testing.T, aiDir, scopeName string, maxJoin int, only ...string) (*Graph, *FederationReport) { t.Helper() + return loadFedJoining(t, aiDir, scopeName, maxJoin, nil, only...) +} + +// loadFedJoining is loadFed with an explicit join policy. Most fixtures here +// predate the per-type policy and were written around `type` entities, so they +// say so rather than relying on a default that deliberately excludes them. +func loadFedJoining(t *testing.T, aiDir, scopeName string, maxJoin int, joinTypes []string, only ...string) (*Graph, *FederationReport) { + t.Helper() scope, err := LoadScopeConfig(aiDir, scopeName) if err != nil { @@ -71,6 +79,7 @@ func loadFed(t *testing.T, aiDir, scopeName string, maxJoin int, only ...string) ProjectID: fedProject, OnlyLayers: only, MaxJoinLayers: maxJoin, + JoinTypes: joinTypes, }) if err != nil { t.Fatalf("LoadFederatedGraph: %v", err) @@ -110,7 +119,7 @@ func TestLoadFederatedGraphJoinsSharedIdentities(t *testing.T) { fedEntity{"estate.md", EntityTypeFile}, ) - g, report := loadFed(t, aiDir, "estate", 0) + g, report := loadFedJoining(t, aiDir, "estate", 0, []string{EntityTypeType}) // Address exists in two layers and must arrive as one node carrying both. address := nodeByName(t, g, "Address") @@ -162,7 +171,8 @@ func TestLoadFederatedGraphSuppressesWidespreadNames(t *testing.T) { fedEntity{"estate.md", EntityTypeFile}, ) - g, report := loadFed(t, aiDir, "estate", 2) // "Deployment" is in 3 layers + joinTopics := []string{EntityTypeTopic} + g, report := loadFedJoining(t, aiDir, "estate", 2, joinTopics) // "Deployment" is in 3 layers var deployments int for _, n := range g.nodes { @@ -184,7 +194,7 @@ func TestLoadFederatedGraphSuppressesWidespreadNames(t *testing.T) { } // Raising the guard joins it, which is the escape hatch the report offers. - g2, report2 := loadFed(t, aiDir, "estate", 3) + g2, report2 := loadFedJoining(t, aiDir, "estate", 3, joinTopics) if report2.Joined != 1 { t.Errorf("with a raised guard, Joined = %d, want 1", report2.Joined) } @@ -283,7 +293,7 @@ func TestLoadFederatedGraphRenamesCollidingIDs(t *testing.T) { // maxJoin 1 leaves the identity unjoined, so nothing but the ID could fuse // these two. - g, report := loadFed(t, aiDir, "estate", 1) + g, report := loadFedJoining(t, aiDir, "estate", 1, []string{}) var dots []GraphNode for _, n := range g.nodes { @@ -310,6 +320,58 @@ func TestLoadFederatedGraphRenamesCollidingIDs(t *testing.T) { } } +// The join policy is the correction this package exists to make: a name +// identifies a package across repositories and does not identify a function. +func TestLoadFederatedGraphJoinPolicyByType(t *testing.T) { + aiDir := t.TempDir() + for _, layer := range []string{"a", "b"} { + seedLayer(t, aiDir, layer, nil, + fedEntity{layer + ".go", EntityTypeFile}, + fedEntity{"print", EntityTypeFunction}, // never joins + fedEntity{"com.depop.auth", EntityTypePackage}, // joins by default + fedEntity{"CodingKeys", EntityTypeType}, // joins only on request + ) + } + seedLayer(t, aiDir, "estate", []string{"a", "b"}, fedEntity{"estate.md", EntityTypeFile}) + + countNamed := func(g *Graph, name string) int { + n := 0 + for _, node := range g.nodes { + if node.Name == name { + n++ + } + } + return n + } + + g, _ := loadFed(t, aiDir, "estate", 0) + if got := countNamed(g, "print"); got != 2 { + t.Errorf("function \"print\" collapsed to %d node(s); it must never join", got) + } + if got := countNamed(g, "com.depop.auth"); got != 1 { + t.Errorf("package joined into %d node(s), want 1", got) + } + if got := countNamed(g, "CodingKeys"); got != 2 { + t.Errorf("type joined into %d node(s) under the default policy, want 2", got) + } + + // Opting a type in is what --join-types is for. + g2, _ := loadFedJoining(t, aiDir, "estate", 0, []string{EntityTypePackage, EntityTypeType}) + if got := countNamed(g2, "CodingKeys"); got != 1 { + t.Errorf("with type joining requested, got %d node(s), want 1", got) + } + if got := countNamed(g2, "print"); got != 2 { + t.Errorf("function joined when it was not in the policy: %d node(s)", got) + } + + // An explicitly empty policy joins nothing — the layers stay the + // disconnected components they actually are. + g3, _ := loadFedJoining(t, aiDir, "estate", 0, []string{}) + if got := countNamed(g3, "com.depop.auth"); got != 2 { + t.Errorf("empty join policy still joined packages: %d node(s), want 2", got) + } +} + func TestFederationOrder(t *testing.T) { scope := &ScopeConfig{Name: "estate", Layers: []string{"a", "b", "c"}} @@ -414,25 +476,42 @@ func TestLoadFederatedGraphReportsRemotesAsSkipped(t *testing.T) { // another layer, which is what makes the key joinable in the first place. func TestLoadFederatedGraphKeepsSameLayerNamesakesApart(t *testing.T) { aiDir := t.TempDir() - // Two unrelated Config types in one layer, plus a Config in another so the - // (name, type) key counts as joinable. + // Two unrelated packages named "config" in one layer — internal/api/config + // and internal/worker/config, which is an ordinary Go layout — plus a + // "config" package in another layer so the (name, type) key is joinable. + // The type must be one DefaultJoinTypes actually joins: only package and + // import cross layers, so a type or file namesake never reaches this path. seedLayer(t, aiDir, "payments", []string{"libs"}, fedEntity{name: "Root", entityType: EntityTypeFile}, - fedEntity{name: "Config", entityType: EntityTypeType}, - fedEntity{name: "Config", entityType: EntityTypeType}) + fedEntity{name: "config", entityType: EntityTypePackage}, + fedEntity{name: "config", entityType: EntityTypePackage}) seedLayer(t, aiDir, "libs", nil, - fedEntity{name: "Config", entityType: EntityTypeType}) + fedEntity{name: "config", entityType: EntityTypePackage}) + + g, report := loadFed(t, aiDir, "payments", 0) - g, _ := loadFed(t, aiDir, "payments", 0) + // Guard the premise: if the join policy stops covering package, this test + // would pass for the wrong reason — nothing joins, so nothing can fuse. + joined := false + for _, jt := range report.JoinTypes { + if strings.EqualFold(jt, EntityTypePackage) { + joined = true + } + } + if !joined { + t.Fatalf("JoinTypes = %v, which does not include %s — this fixture no longer exercises joining", + report.JoinTypes, EntityTypePackage) + } configs := 0 for _, n := range g.nodes { - if n.Name == "Config" { + if n.Name == "config" { configs++ } } - // payments' two stay separate; libs' joins onto one of them. + // payments' two stay separate; libs' joins onto one of them. 1 means the + // same-layer pair fused; 3 means nothing joined at all. if configs != 2 { - t.Errorf("Config nodes = %d, want 2 — the two same-layer Configs were fused into one", configs) + t.Errorf("config nodes = %d, want 2 — 1 means the two same-layer packages fused, 3 means no join happened", configs) } } diff --git a/src/kg/internal/knowledge/graph_link.go b/src/kg/internal/knowledge/graph_link.go new file mode 100644 index 0000000..b984060 --- /dev/null +++ b/src/kg/internal/knowledge/graph_link.go @@ -0,0 +1,210 @@ +package knowledge + +import ( + "sort" + "strings" +) + +// Cross-layer package linking — deriving real dependency edges between layers. +// +// Federated layers are genuinely disconnected: relations live inside one +// database and no indexer writes one across repositories. Joining identities +// by name bridges them, but measurement showed that bridge is made of +// coincidences — Foundation, CodingKeys, print, map — and not of dependencies. +// +// A dependency is recoverable without inventing anything, because the indexers +// already record both ends: layer A holds an `import` entity naming +// com.depop.auth.client.AuthClient, and layer B holds a `package` entity named +// com.depop.auth.client. That is a real edge, and it is a relation rather than +// an identity, so it is drawn as one and marked derived. +// +// Measured on a 61-layer estate: exact name matching finds 71 cross-layer +// links, nearly all of them generic fragments like "auth" and "clients". +// Longest-prefix matching with a three-segment floor finds 845 unambiguous +// ones, and they read as a dependency map. See docs/kg-graph-linking-design.md. + +// minPackageSegments is how specific a package name must be before an import +// may resolve to it. Without a floor, "com.depop" claims every JVM import in +// the estate and the graph gains one hub node instead of a dependency map. +const minPackageSegments = 3 + +// packageSeparator is the namespace separator prefix matching understands. +// +// Only dotted namespaces resolve today. Not one package entity in the measured +// estate had a "/" in its name — the indexers do not mint package entities for +// npm or Go module paths — so slash-style imports have nothing to resolve +// against. That is a gap in what is indexed, not in this matching rule. +const packageSeparator = "." + +// LinkReport records what package linking derived, and what it refused to. +type LinkReport struct { + // Derived is the number of DEPENDS_ON edges added. + Derived int `json:"derived"` + // Ambiguous counts imports that matched a package name defined in more + // than one layer. Those are skipped rather than guessed at; on a real + // estate this discards more matches than it keeps. + Ambiguous int `json:"ambiguous"` + // AmbiguousNames samples the package names responsible, worst first. + AmbiguousNames []string `json:"ambiguous_names,omitempty"` + // SameLayer counts imports that resolved to a package in their own layer. + // That structure is already in the layer's own graph, so no edge is added. + SameLayer int `json:"same_layer"` +} + +// packageTarget is one package name's presence across the merged graph. +type packageTarget struct { + // nodeID is the package node an import should point at. Meaningful only + // when the name resolves to a single layer. + nodeID string + layers map[string]bool +} + +// LinkPackages derives DEPENDS_ON edges from imports to the packages they +// name, and adds them to g. Pure with respect to the store: it reads only the +// merged graph, so it is testable without a database. +func LinkPackages(g *Graph) LinkReport { + report := LinkReport{} + targets := packageIndex(g) + if len(targets) == 0 { + return report + } + + ambiguous := map[string]bool{} + + // Imports are visited in ID order so that the derived edges — and the + // samples in the report — are the same on every run. + for _, node := range sortedNodesOfType(g, EntityTypeImport) { + name, target := resolvePackage(node.Name, targets) + if target == nil { + continue + } + + if len(target.layers) > 1 { + report.Ambiguous++ + ambiguous[name] = true + continue + } + + // An import that only ever appears in the layer defining the package + // is describing that layer's own structure, which its graph already + // holds. + if !crossesLayer(node.Layers, target.layers) { + report.SameLayer++ + continue + } + + edge := GraphEdge{ + FromID: node.ID, + ToID: target.nodeID, + Type: RelDependsOn, + Derived: true, + } + if edge.FromID == edge.ToID { + continue + } + g.out[edge.FromID] = append(g.out[edge.FromID], edge) + g.in[edge.ToID] = append(g.in[edge.ToID], edge) + g.edgeCount++ + report.Derived++ + } + + report.AmbiguousNames = make([]string, 0, len(ambiguous)) + for name := range ambiguous { + report.AmbiguousNames = append(report.AmbiguousNames, name) + } + sort.Strings(report.AmbiguousNames) + if len(report.AmbiguousNames) > 10 { + report.AmbiguousNames = report.AmbiguousNames[:10] + } + return report +} + +// packageIndex collects every package entity by name, with the layers it is +// defined in. A name can arrive as several nodes when the join guard kept them +// apart, which is itself a form of ambiguity and is recorded as such. +func packageIndex(g *Graph) map[string]*packageTarget { + targets := make(map[string]*packageTarget) + for _, node := range g.nodes { + if !strings.EqualFold(node.Type, EntityTypePackage) { + continue + } + if segments(node.Name) < minPackageSegments { + continue + } + target, ok := targets[node.Name] + if !ok { + target = &packageTarget{nodeID: node.ID, layers: map[string]bool{}} + targets[node.Name] = target + } + // Lowest node ID wins, so the chosen target does not depend on map + // iteration order. + if node.ID < target.nodeID { + target.nodeID = node.ID + } + for _, layer := range node.Layers { + target.layers[layer] = true + } + if len(node.Layers) == 0 { + // A non-federated graph has no layer provenance; treat the node's + // own presence as one layer so single-database use is well-defined. + target.layers[""] = true + } + } + return targets +} + +// resolvePackage finds the longest package name that prefixes importName, +// returning the name and its target. Longest-first means +// com.depop.auth.client beats com.depop.auth, which is the specific answer. +// +// The loop starts at len(parts), not len(parts)-1: a string is a prefix of +// itself, and an import equal to a package name is the ordinary shape of a +// Java wildcard import. extractImportPath keeps only the scoped_identifier and +// drops the trailing asterisk, so `import com.depop.auth.client.*;` arrives as +// "com.depop.auth.client" — exactly the package name. Testing only proper +// prefixes dropped every one of those silently. (Kotlin escaped it by accident: +// its extractor keeps the ".*" verbatim, leaving a longer string.) +func resolvePackage(importName string, targets map[string]*packageTarget) (string, *packageTarget) { + parts := strings.Split(importName, packageSeparator) + for cut := len(parts); cut >= minPackageSegments; cut-- { + candidate := strings.Join(parts[:cut], packageSeparator) + if target, ok := targets[candidate]; ok { + return candidate, target + } + } + return "", nil +} + +// crossesLayer reports whether the import is used anywhere the package is not +// defined — the condition that makes the dependency a cross-layer one. +func crossesLayer(importLayers []string, packageLayers map[string]bool) bool { + if len(importLayers) == 0 { + return false + } + for _, layer := range importLayers { + if !packageLayers[layer] { + return true + } + } + return false +} + +// segments counts the namespace segments in a package name. +func segments(name string) int { + if name == "" { + return 0 + } + return strings.Count(name, packageSeparator) + 1 +} + +// sortedNodesOfType returns every node of one type in ID order. +func sortedNodesOfType(g *Graph, entityType string) []GraphNode { + var nodes []GraphNode + for _, node := range g.nodes { + if strings.EqualFold(node.Type, entityType) { + nodes = append(nodes, node) + } + } + sort.Slice(nodes, func(i, j int) bool { return nodes[i].ID < nodes[j].ID }) + return nodes +} diff --git a/src/kg/internal/knowledge/graph_link_integration_test.go b/src/kg/internal/knowledge/graph_link_integration_test.go new file mode 100644 index 0000000..22629c8 --- /dev/null +++ b/src/kg/internal/knowledge/graph_link_integration_test.go @@ -0,0 +1,92 @@ +package knowledge + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" +) + +// indexLayerFromSource writes a scope config and indexes real source into that +// scope's database, so the resulting graph is what `kg index` would actually +// produce rather than hand-built nodes. +func indexLayerFromSource(t *testing.T, aiDir, name string, layers []string, files map[string]string) { + t.Helper() + + scopeDir := filepath.Join(aiDir, "scope") + if err := os.MkdirAll(scopeDir, 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + cfg := ScopeConfig{Name: name, Database: name + ".db", Layers: layers} + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal scope %s: %v", name, err) + } + if err := os.WriteFile(filepath.Join(scopeDir, name+".json"), data, 0o644); err != nil { + t.Fatalf("write scope %s: %v", name, err) + } + + srcDir := filepath.Join(t.TempDir(), name) + if err := os.MkdirAll(srcDir, 0o755); err != nil { + t.Fatalf("MkdirAll src: %v", err) + } + for file, content := range files { + if err := os.WriteFile(filepath.Join(srcDir, file), []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", file, err) + } + } + + store, err := OpenStore(filepath.Join(aiDir, cfg.Database)) + if err != nil { + t.Fatalf("OpenStore %s: %v", name, err) + } + defer store.Close() + + idx, err := NewIndexer(store, fedProject, srcDir) + if err != nil { + t.Fatalf("NewIndexer %s: %v", name, err) + } + if _, err := idx.Index(); err != nil { + t.Fatalf("Index %s: %v", name, err) + } +} + +// The end-to-end claim: cross-layer package linking derives real edges from +// what the indexer actually produces, not only from hand-built fixtures. +// +// This could not pass before JVM package declarations were indexed — the only +// package entities were Go's bare identifiers, which never reach +// minPackageSegments, so packageIndex filtered every one of them out and +// LinkPackages returned Derived: 0 against any indexed corpus. +func TestLinkPackagesDerivesFromIndexedJVMSource(t *testing.T) { + if testing.Short() { + t.Skip("Skipping integration test in short mode") + } + + aiDir := t.TempDir() + + // The library layer declares the package. + indexLayerFromSource(t, aiDir, "libraries", nil, map[string]string{ + "AuthClient.java": "package com.depop.auth.client;\n\npublic class AuthClient {}\n", + }) + // The consumer layer imports a symbol from it. + indexLayerFromSource(t, aiDir, "martech", []string{"libraries"}, map[string]string{ + "Checkout.java": "package com.depop.martech.checkout;\n\nimport com.depop.auth.client.AuthClient;\n\npublic class Checkout {}\n", + }) + + scope, err := LoadScopeConfig(aiDir, "martech") + if err != nil { + t.Fatalf("LoadScopeConfig: %v", err) + } + _, report, err := LoadFederatedGraph(FederatedGraphOptions{ + AIDir: aiDir, Scope: scope, ProjectID: fedProject, + }) + if err != nil { + t.Fatalf("LoadFederatedGraph: %v", err) + } + + if report.Link.Derived < 1 { + t.Errorf("Derived = %d, want at least 1 — no cross-layer edge was derived from indexed source", + report.Link.Derived) + } +} diff --git a/src/kg/internal/knowledge/graph_link_test.go b/src/kg/internal/knowledge/graph_link_test.go new file mode 100644 index 0000000..5a3539d --- /dev/null +++ b/src/kg/internal/knowledge/graph_link_test.go @@ -0,0 +1,262 @@ +package knowledge + +import ( + "sort" + "strings" + "testing" +) + +// linkGraph builds a merged-graph fixture directly: LinkPackages reads only +// nodes and layers, so none of this needs a database. +func linkGraph(nodes ...GraphNode) *Graph { + g := &Graph{ + nodes: make(map[string]GraphNode, len(nodes)), + out: make(map[string][]GraphEdge), + in: make(map[string][]GraphEdge), + } + for _, n := range nodes { + g.nodes[n.ID] = n + } + return g +} + +func pkgNode(id, name string, layers ...string) GraphNode { + return GraphNode{ID: id, Name: name, Type: EntityTypePackage, Layers: layers} +} + +func importNode(id, name string, layers ...string) GraphNode { + return GraphNode{ID: id, Name: name, Type: EntityTypeImport, Layers: layers} +} + +// derivedEdges returns every derived edge as "from -TYPE-> to", sorted. +func derivedEdges(g *Graph) []string { + var out []string + for _, edges := range g.out { + for _, e := range edges { + if e.Derived { + out = append(out, e.FromID+" -"+e.Type+"-> "+e.ToID) + } + } + } + sort.Strings(out) + return out +} + +func TestLinkPackagesDerivesCrossLayerDependency(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 1 { + t.Fatalf("Derived = %d, want 1", report.Derived) + } + if got := derivedEdges(g); len(got) != 1 || got[0] != "imp1 -DEPENDS_ON-> pkg1" { + t.Errorf("edges = %v, want [imp1 -DEPENDS_ON-> pkg1]", got) + } + if g.EdgeCount() != 1 { + t.Errorf("EdgeCount = %d, want 1", g.EdgeCount()) + } + // The edge has to be reachable in both directions, or "who depends on this + // library" — the question this feature exists to answer — cannot be asked. + if len(g.in["pkg1"]) != 1 { + t.Errorf("derived edge missing from the incoming adjacency list") + } +} + +// The most specific package that prefixes the import wins: com.depop.auth.client +// tells you which library, com.depop.auth barely narrows it down. +func TestLinkPackagesPrefersTheLongestPrefix(t *testing.T) { + g := linkGraph( + pkgNode("short", "com.depop.auth", "libraries"), + pkgNode("long", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + + LinkPackages(g) + + if got := derivedEdges(g); len(got) != 1 || !strings.HasSuffix(got[0], "-> long") { + t.Errorf("edges = %v, want the link to resolve to the longer package", got) + } +} + +// Without a floor on specificity, "com.depop" claims every JVM import in the +// estate and the graph gains one hub node instead of a dependency map. +func TestLinkPackagesIgnoresUnspecificPackages(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 0 { + t.Errorf("Derived = %d, want 0 for a two-segment package", report.Derived) + } +} + +// A package defined in several layers cannot say which one an import meant, and +// guessing would invent exactly the kind of edge this feature replaces. +func TestLinkPackagesSkipsAmbiguousTargets(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries", "tooling"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 0 { + t.Errorf("Derived = %d, want 0", report.Derived) + } + if report.Ambiguous != 1 { + t.Errorf("Ambiguous = %d, want 1", report.Ambiguous) + } + if len(report.AmbiguousNames) != 1 || report.AmbiguousNames[0] != "com.depop.auth.client" { + t.Errorf("AmbiguousNames = %v, want [com.depop.auth.client]", report.AmbiguousNames) + } +} + +// The join guard can leave one package name as several nodes; that is the same +// ambiguity arriving by a different route and must be treated the same way. +func TestLinkPackagesTreatsSplitNodesAsAmbiguous(t *testing.T) { + g := linkGraph( + pkgNode("pkgA", "com.depop.auth.client", "libraries"), + pkgNode("pkgB", "com.depop.auth.client", "tooling"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 0 || report.Ambiguous != 1 { + t.Errorf("Derived = %d, Ambiguous = %d; want 0 and 1", report.Derived, report.Ambiguous) + } +} + +// An import used only where the package is defined describes that layer's own +// structure, which its graph already holds. +func TestLinkPackagesSkipsSameLayerResolution(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "libraries"), + ) + + report := LinkPackages(g) + + if report.Derived != 0 { + t.Errorf("Derived = %d, want 0", report.Derived) + } + if report.SameLayer != 1 { + t.Errorf("SameLayer = %d, want 1", report.SameLayer) + } +} + +// A joined import spanning several layers still depends on the package from the +// layers that are not the defining one. +func TestLinkPackagesLinksWhenAnyLayerIsForeign(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "libraries", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 1 { + t.Errorf("Derived = %d, want 1 — martech imports it from outside libraries", report.Derived) + } +} + +// Documents a known gap rather than a design choice: no indexer mints package +// entities for npm or Go module paths, so slash-style imports resolve to +// nothing. If that changes upstream, this test is the place it surfaces. +func TestLinkPackagesDoesNotResolveSlashNamespaces(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "@depop/auth-client", "libraries"), + importNode("imp1", "@depop/auth-client/dist/index", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 0 { + t.Errorf("Derived = %d; slash namespaces are not matched today", report.Derived) + } +} + +func TestLinkPackagesIsDeterministic(t *testing.T) { + build := func() *Graph { + return linkGraph( + pkgNode("pkgA", "com.depop.auth.client", "libraries"), + pkgNode("pkgB", "com.depop.data.store", "data"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + importNode("imp2", "com.depop.data.store.Reader", "product"), + importNode("imp3", "com.depop.auth.client.Jwt", "ads"), + ) + } + + first := build() + LinkPackages(first) + want := derivedEdges(first) + if len(want) != 3 { + t.Fatalf("got %d derived edges, want 3", len(want)) + } + + for i := 0; i < 20; i++ { + g := build() + LinkPackages(g) + got := derivedEdges(g) + if strings.Join(got, "|") != strings.Join(want, "|") { + t.Fatalf("run %d produced %v, first run produced %v", i, got, want) + } + } +} + +// Derived edges must be distinguishable in the output, or a reader takes an +// inference on the same footing as a recorded fact. +func TestDerivedEdgesRenderDistinctly(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client.AuthClient", "martech"), + ) + LinkPackages(g) + + sub, err := g.Subgraph(GraphOptions{}) + if err != nil { + t.Fatalf("Subgraph: %v", err) + } + + mermaid := RenderMermaid(sub) + if !strings.Contains(mermaid, "-.->|DEPENDS_ON|") { + t.Errorf("derived edge is not dashed in mermaid:\n%s", mermaid) + } + if !strings.Contains(mermaid, "1 of those are derived") { + t.Errorf("header does not count derived edges:\n%s", mermaid) + } + + dot := RenderDOT(sub) + if !strings.Contains(dot, `[label="DEPENDS_ON", style=dashed]`) { + t.Errorf("derived edge is not dashed in dot:\n%s", dot) + } +} + +// A Java wildcard import resolves to the package name itself. extractImportPath +// keeps only the scoped_identifier and drops the trailing asterisk, so +// `import com.depop.auth.client.*;` arrives as "com.depop.auth.client" — +// identical to the package it names. +// +// The documented rule is "the longest package name that is a dotted prefix of +// it", and a string is a prefix of itself, so this must resolve. Kotlin happens +// to escape the bug only because its extractor keeps the ".*" suffix verbatim, +// leaving a longer string that the proper-prefix loop does reach. +func TestLinkPackagesResolvesAnImportEqualToThePackageName(t *testing.T) { + g := linkGraph( + pkgNode("pkg1", "com.depop.auth.client", "libraries"), + importNode("imp1", "com.depop.auth.client", "martech"), + ) + + report := LinkPackages(g) + + if report.Derived != 1 { + t.Errorf("Derived = %d, want 1 — an import equal to the package name did not resolve", report.Derived) + } +} diff --git a/src/kg/internal/knowledge/graph_render.go b/src/kg/internal/knowledge/graph_render.go index 2796c42..af35744 100644 --- a/src/kg/internal/knowledge/graph_render.go +++ b/src/kg/internal/knowledge/graph_render.go @@ -99,7 +99,11 @@ func RenderMermaid(sub Subgraph) string { } for _, e := range sub.Edges { - fmt.Fprintf(&b, " %s -->|%s| %s\n", ids[e.FromID], mermaidLabel(e.Type), ids[e.ToID]) + arrow := "-->" + if e.Derived { + arrow = "-.->" + } + fmt.Fprintf(&b, " %s %s|%s| %s\n", ids[e.FromID], arrow, mermaidLabel(e.Type), ids[e.ToID]) } if sub.RootID != "" { @@ -131,7 +135,11 @@ func RenderDOT(sub Subgraph) string { } for _, e := range sub.Edges { - fmt.Fprintf(&b, " %s -> %s [label=%s];\n", ids[e.FromID], ids[e.ToID], dotQuote(e.Type)) + attrs := "label=" + dotQuote(e.Type) + if e.Derived { + attrs += ", style=dashed" + } + fmt.Fprintf(&b, " %s -> %s [%s];\n", ids[e.FromID], ids[e.ToID], attrs) } b.WriteString("}\n") @@ -141,8 +149,20 @@ func RenderDOT(sub Subgraph) string { // writeHeader writes the provenance comment both formats carry: what is in the // picture, and — the part that matters — whether anything was left out of it. func writeHeader(b *strings.Builder, sub Subgraph, comment string) { + derived := 0 + for _, e := range sub.Edges { + if e.Derived { + derived++ + } + } fmt.Fprintf(b, "%s kg graph: %d node(s), %d relation(s) of %d entities in the project\n", comment, len(sub.Nodes), len(sub.Edges), sub.TotalNodes) + if derived > 0 { + // Said separately rather than folded into the count: these are edges + // this tool worked out, and a reader should not have to take them on + // the same footing as ones an indexer recorded. + fmt.Fprintf(b, "%s %d of those are derived cross-layer links, drawn dashed\n", comment, derived) + } if sub.Truncated { fmt.Fprintf(b, "%s TRUNCATED at the node limit — raise --limit to see the rest\n", comment) }