From 1e8bacf608f7b2a362adaf484c1072772c3215f5 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 06:20:48 +0100 Subject: [PATCH 01/28] chore(deps)(deps): bump uuid from 13.0.0 to 14.0.0 in /gitnexus (#1000) --- gitnexus/package-lock.json | 8 ++++---- gitnexus/package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 76afc1fde..9f1fd8a58 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -40,7 +40,7 @@ "tree-sitter-ruby": "^0.23.1", "tree-sitter-rust": "0.23.1", "tree-sitter-typescript": "^0.23.2", - "uuid": "^13.0.0" + "uuid": "^14.0.0" }, "bin": { "gitnexus": "dist/cli/index.js" @@ -5451,9 +5451,9 @@ } }, "node_modules/uuid": { - "version": "13.0.0", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-13.0.0.tgz", - "integrity": "sha512-XQegIaBTVUjSHliKqcnFqYypAd4S+WCYt5NIeRs6w/UAry7z8Y9j5ZwRRL4kzq9U3sD6v+85er9FvkEaBpji2w==", + "version": "14.0.0", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-14.0.0.tgz", + "integrity": "sha512-Qo+uWgilfSmAhXCMav1uYFynlQO7fMFiMVZsQqZRMIXp0O7rR7qjkj+cPvBHLgBqi960QCoo/PH2/6ZtVqKvrg==", "funding": [ "https://github.com/sponsors/broofa", "https://github.com/sponsors/ctavan" diff --git a/gitnexus/package.json b/gitnexus/package.json index d8f2c126a..b84da8bfe 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -81,7 +81,7 @@ "tree-sitter-ruby": "^0.23.1", "tree-sitter-rust": "0.23.1", "tree-sitter-typescript": "^0.23.2", - "uuid": "^13.0.0" + "uuid": "^14.0.0" }, "optionalDependencies": { "node-addon-api": "^8.0.0", From cfeece95f5b027e4e021bfa93b58be6632e9112d Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 06:21:12 +0100 Subject: [PATCH 02/28] chore(deps)(deps): bump graphology from 0.25.4 to 0.26.0 in /gitnexus (#1001) --- gitnexus/package-lock.json | 11 +++++------ gitnexus/package.json | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index 9f1fd8a58..caf14e2bd 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -19,7 +19,7 @@ "cors": "^2.8.5", "express": "^4.19.2", "glob": "^13.0.6", - "graphology": "^0.25.4", + "graphology": "^0.26.0", "graphology-indices": "^0.17.0", "graphology-utils": "^2.3.0", "ignore": "^7.0.5", @@ -3316,13 +3316,12 @@ "license": "ISC" }, "node_modules/graphology": { - "version": "0.25.4", - "resolved": "https://registry.npmjs.org/graphology/-/graphology-0.25.4.tgz", - "integrity": "sha512-33g0Ol9nkWdD6ulw687viS8YJQBxqG5LWII6FI6nul0pq6iM2t5EKquOTFDbyTblRB3O9I+7KX4xI8u5ffekAQ==", + "version": "0.26.0", + "resolved": "https://registry.npmjs.org/graphology/-/graphology-0.26.0.tgz", + "integrity": "sha512-8SSImzgUUYC89Z042s+0r/vMibY7GX/Emz4LDO5e7jYXhuoWfHISPFJYjpRLUSJGq6UQ6xlenvX1p/hJdfXuXg==", "license": "MIT", "dependencies": { - "events": "^3.3.0", - "obliterator": "^2.0.2" + "events": "^3.3.0" }, "peerDependencies": { "graphology-types": ">=0.24.0" diff --git a/gitnexus/package.json b/gitnexus/package.json index b84da8bfe..5fc35ed47 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -60,7 +60,7 @@ "cors": "^2.8.5", "express": "^4.19.2", "glob": "^13.0.6", - "graphology": "^0.25.4", + "graphology": "^0.26.0", "graphology-indices": "^0.17.0", "graphology-utils": "^2.3.0", "ignore": "^7.0.5", From 8fb386d45fde3148e4d0c180ace656fb430d34aa Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 21 Apr 2026 06:21:31 +0100 Subject: [PATCH 03/28] chore(deps)(deps-dev): bump @types/node in /gitnexus (#1002) --- gitnexus/package-lock.json | 16 ++++++++-------- gitnexus/package.json | 2 +- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/gitnexus/package-lock.json b/gitnexus/package-lock.json index caf14e2bd..14d9cb2c8 100644 --- a/gitnexus/package-lock.json +++ b/gitnexus/package-lock.json @@ -50,7 +50,7 @@ "@types/cors": "^2.8.17", "@types/express": "^4.17.21", "@types/js-yaml": "^4.0.9", - "@types/node": "^20.0.0", + "@types/node": "^25.6.0", "@types/uuid": "^10.0.0", "@vitest/coverage-v8": "^4.0.18", "gitnexus-shared": "file:../gitnexus-shared", @@ -2041,12 +2041,12 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "20.19.37", - "resolved": "https://registry.npmjs.org/@types/node/-/node-20.19.37.tgz", - "integrity": "sha512-8kzdPJ3FsNsVIurqBs7oodNnCEVbni9yUEkaHbgptDACOPW04jimGagZ51E6+lXUwJjgnBw+hyko/lkFWCldqw==", + "version": "25.6.0", + "resolved": "https://registry.npmjs.org/@types/node/-/node-25.6.0.tgz", + "integrity": "sha512-+qIYRKdNYJwY3vRCZMdJbPLJAtGjQBudzZzdzwQYkEPQd+PJGixUL5QfvCLDaULoLv+RhT3LDkwEfKaAkgSmNQ==", "license": "MIT", "dependencies": { - "undici-types": "~6.21.0" + "undici-types": "~7.19.0" } }, "node_modules/@types/qs": { @@ -5411,9 +5411,9 @@ } }, "node_modules/undici-types": { - "version": "6.21.0", - "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-6.21.0.tgz", - "integrity": "sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ==", + "version": "7.19.2", + "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-7.19.2.tgz", + "integrity": "sha512-qYVnV5OEm2AW8cJMCpdV20CDyaN3g0AjDlOGf1OW4iaDEx8MwdtChUp4zu4H0VP3nDRF/8RKWH+IPp9uW0YGZg==", "license": "MIT" }, "node_modules/universalify": { diff --git a/gitnexus/package.json b/gitnexus/package.json index 5fc35ed47..f89124c73 100644 --- a/gitnexus/package.json +++ b/gitnexus/package.json @@ -97,7 +97,7 @@ "@types/cors": "^2.8.17", "@types/express": "^4.17.21", "@types/js-yaml": "^4.0.9", - "@types/node": "^20.0.0", + "@types/node": "^25.6.0", "@types/uuid": "^10.0.0", "@vitest/coverage-v8": "^4.0.18", "tsx": "^4.0.0", From fb3bc7829e5a9825b32a8f479958769eb1d138e6 Mon Sep 17 00:00:00 2001 From: ivkond Date: Tue, 21 Apr 2026 10:14:21 +0300 Subject: [PATCH 04/28] docs(group): add gRPC microservices group guide (#906) (#994) * docs(group): add gRPC microservices group guide (#906) Adds `docs/guides/microservices-grpc.md`, a walkthrough for using GitNexus across multiple repositories whose services communicate over gRPC. Covers the group mental model, per-repo `gitnexus analyze`, the `group.yaml` schema, `group sync`, inspecting `contracts.json`, running cross-repo `impact` with `@` routing, the gRPC extractor's provider/consumer signals per language, the `config.links` manifest escape hatch, and a short troubleshooting list. Wires the new page from the group-mode note in AGENTS.md. Closes #906. Made-with: Cursor * docs(grpc-guide): drop hard line wraps, rely on editor soft wrap Made-with: Cursor --- AGENTS.md | 2 + docs/guides/microservices-grpc.md | 295 ++++++++++++++++++++++++++++++ 2 files changed, 297 insertions(+) create mode 100644 docs/guides/microservices-grpc.md diff --git a/AGENTS.md b/AGENTS.md index 6e916ff0b..ef779a45f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -114,6 +114,8 @@ Indexed as **GitNexus** (4325 symbols, 10556 relationships, 300 execution flows) | `impact` (group mode) | Cross-repo blast radius via Contract Bridge | `gitnexus_impact({repo: "@myGroup", target: "X", direction: "upstream"})` | > Group mode: pass `repo: "@"` to fan out across all member repos, or `repo: "@/"` to target a single member (path keys from `group.yaml`). Optional `service: ""` filters by service root. Group-level state (contracts, staleness) lives in the resources table below — there are **no** `group_query` / `group_context` / `group_impact` / `group_contracts` / `group_status` MCP tools. +> +> For a full walkthrough of setting up a group across multiple repos that communicate over gRPC, see [docs/guides/microservices-grpc.md](docs/guides/microservices-grpc.md). ## Impact Risk Levels diff --git a/docs/guides/microservices-grpc.md b/docs/guides/microservices-grpc.md new file mode 100644 index 000000000..afe6b2499 --- /dev/null +++ b/docs/guides/microservices-grpc.md @@ -0,0 +1,295 @@ +# Using GitNexus across gRPC microservices + +## When to use this guide + +This guide is for teams whose product lives in **several separate Git repositories** — one per service — and whose services talk to each other over **gRPC** (possibly alongside HTTP and message topics). GitNexus indexes each repo independently, then a _group_ stitches the per-repo indexes into a single cross-repo view that the `impact`, `query`, and `context` tools can traverse. If your services live in one monorepo, much of this still applies — set each service as a member of a group and use the `service` prefix to scope queries — but the walkthrough assumes the harder multi-repo case. + +## Mental model + +- Each repository has its own `.gitnexus/` index (a LadybugDB graph of symbols, relationships, processes). `gitnexus analyze` in each repo produces that index completely independently. +- A **group** is a higher-level construct stored at `~/.gitnexus/groups//` that references the per-repo indexes by their registry name. +- Sync-time extractors walk each member repo and emit **contracts** — provider or consumer records keyed by a canonical `contractId` (`grpc::auth.AuthService/Login`, `http::GET::/orders`, etc.). +- The sync step matches providers and consumers that share a `contractId` and writes **cross-links** to `/contracts.json`. Those cross-links are what lets `impact({repo: "@", target: "X"})` hop from one repo into another. +- Contracts come from three places: automatic contract extractors (`grpc-extractor`, `http-route-extractor`, `topic-extractor`), a manifest escape hatch (`config.links` in `group.yaml`), and — for same-name symbol matches where no contract is declared — the exact-match matching cascade in [`matching.ts`](../../gitnexus/src/core/group/matching.ts). +- Each repo stays editable and re-indexable on its own. Re-run `gitnexus analyze` in a repo when it changes, then `gitnexus group sync ` to refresh `contracts.json`. `gitnexus group status` reports which members are stale. + +## Prerequisites + +- GitNexus installed and runnable as `gitnexus` or `npx gitnexus` (see the root [README.md](../../README.md)). +- Each service repository checked out locally. No requirement that they share a parent directory — the group references them by registry name. +- Write access to `~/.gitnexus/` (the default gitnexus home; see `getDefaultGitnexusDir` in [`storage.ts`](../../gitnexus/src/core/group/storage.ts)). + +## Step-by-step walkthrough + +The example uses three services — a TypeScript API gateway, a Go orders service, and a Python inventory service — with gRPC between them. The gateway is an `orders` consumer; the orders service is both an `orders` provider and an `inventory` consumer; the inventory service is an `inventory` provider. + +### 1. Index each repository + +Run `analyze` from inside each service repo (or pass the path). The CLI surface lives in [`gitnexus/src/cli/analyze.ts`](../../gitnexus/src/cli/analyze.ts) and is wired in [`gitnexus/src/cli/index.ts`](../../gitnexus/src/cli/index.ts). + +```bash +cd ~/code/gateway && npx gitnexus analyze +cd ~/code/orders && npx gitnexus analyze +cd ~/code/inventory && npx gitnexus analyze +``` + +Useful flags: + +- `--force` — reindex even if up to date. +- `--embeddings` — generate embedding vectors (needed only if you want semantic search; the exact-match cross-repo cascade does **not** need them). +- `--name ` — register the repo under a specific alias when two repos share a basename (e.g. two `api/` folders). +- `--skip-git` — index a checkout that isn't a git repo. + +Each run writes a `.gitnexus/` folder in the repo and registers the repo in `~/.gitnexus/registry.json`. Confirm with `npx gitnexus list`. + +### 2. Author `group.yaml` + +Create the group directory and edit the config. Either use the CLI scaffolder or write the file directly — both produce the same shape consumed by [`config-parser.ts`](../../gitnexus/src/core/group/config-parser.ts). + +```bash +npx gitnexus group create payments-platform +# or manually: +mkdir -p ~/.gitnexus/groups/payments-platform +$EDITOR ~/.gitnexus/groups/payments-platform/group.yaml +``` + +Minimal working `group.yaml`: + +```yaml +version: 1 +name: payments-platform +description: Gateway + orders + inventory (gRPC) + +repos: + gateway: gateway + orders: orders + inventory: inventory + +# Only add explicit links when the automatic extractors miss something — +# see "When automatic extraction isn't enough" below. +links: [] + +packages: {} + +detect: + http: true + grpc: true + topics: true + shared_libs: true + embedding_fallback: false + +matching: + bm25_threshold: 0.7 + embedding_threshold: 0.65 + max_candidates_per_step: 3 +``` + +Field notes (schema in [`types.ts`](../../gitnexus/src/core/group/types.ts)): + +- `version` — must be `1`. The parser rejects anything else. +- `name` — required; used for the group directory name and all CLI / MCP calls. +- `repos` — a mapping from **group path** (a logical name you choose; can be a hierarchy like `backend/orders`) to **registry name** (the name shown by `npx gitnexus list`). Both sides appear throughout the tooling: contract rows use the group path; `@/` routes tools to a single member. +- `links` — optional manifest escape hatch, one entry per explicit cross-repo contract. Validated by the parser: `from` and `to` must be known repo paths, `type` must be one of `http | grpc | topic | lib | custom`, and `role` must be `provider | consumer`. +- `detect` — toggles per extractor family. Defaults (set in `config-parser.ts`) turn `http`, `grpc`, `topics`, and `shared_libs` on; disable the ones you don't use to speed up sync. +- `matching` — thresholds for the matching cascade. The exact match is always run; other strategies depend on indexer state. + +### 3. Sync the group + +```bash +npx gitnexus group sync payments-platform --verbose +``` + +What this does (see [`sync.ts`](../../gitnexus/src/core/group/sync.ts)): + +1. Opens each member's per-repo LadybugDB. +2. Runs the HTTP, gRPC, and topic extractors against the source files. +3. Applies manifest `links` through [`manifest-extractor.ts`](../../gitnexus/src/core/group/extractors/manifest-extractor.ts). +4. Runs the exact-match cascade, joining providers and consumers that share a normalized `contractId`. +5. Writes `contracts.json` in the group directory. + +Flags: + +- `--exact-only` — stop after the exact cascade; skip BM25 and embedding fallback. +- `--skip-embeddings` — run exact plus BM25 but not embedding-based matching. +- `--allow-stale` — don't warn if a member's index is stale. +- `--json` — machine-readable output. + +The same operation is available over MCP as `group_sync({ name: "payments-platform" })` — see [`tools.ts`](../../gitnexus/src/mcp/tools.ts). + +### 4. Inspect the registry + +Use `gitnexus group contracts` for the CLI view or read the `gitnexus://group//contracts` MCP resource for the same data. + +```bash +npx gitnexus group contracts payments-platform --type grpc --json +``` + +A shortened response: + +```json +{ + "contracts": [ + { + "contractId": "grpc::orders.OrderService/PlaceOrder", + "type": "grpc", + "role": "provider", + "repo": "orders", + "symbolRef": { "filePath": "internal/grpc/order_server.go", "name": "RegisterOrderServiceServer" }, + "confidence": 0.8, + "meta": { "service": "OrderService", "method": "PlaceOrder", "source": "go_register" } + }, + { + "contractId": "grpc::orders.OrderService/PlaceOrder", + "type": "grpc", + "role": "consumer", + "repo": "gateway", + "symbolRef": { "filePath": "src/clients/orders.ts", "name": "OrderServiceClient" }, + "confidence": 0.75, + "meta": { "service": "OrderService", "source": "ts_generated_client" } + } + ], + "crossLinks": [ + { + "from": { "repo": "gateway", "symbolUid": "…", "symbolRef": { "filePath": "src/clients/orders.ts", "name": "OrderServiceClient" } }, + "to": { "repo": "orders", "symbolUid": "…", "symbolRef": { "filePath": "internal/grpc/order_server.go", "name": "RegisterOrderServiceServer" } }, + "type": "grpc", + "contractId": "grpc::orders.OrderService/PlaceOrder", + "matchType": "exact", + "confidence": 1.0 + } + ] +} +``` + +Staleness of the underlying indexes shows up in `npx gitnexus group status payments-platform` or the `gitnexus://group//status` resource. + +### 5. Run cross-repo impact with `@` routing + +From any shell (you do **not** have to `cd` into a member repo), the normal `impact` / `query` / `context` tools accept `repo: "@"` to fan out across all members, or `repo: "@/"` to target one member. Routing is implemented in [`resolve-at-member.ts`](../../gitnexus/src/core/group/resolve-at-member.ts) and described in [`tools.ts`](../../gitnexus/src/mcp/tools.ts). + +Example MCP calls: + +```json +{"tool": "impact", "arguments": { + "repo": "@payments-platform/orders", + "target": "PlaceOrder", + "direction": "upstream", + "crossDepth": 2 +}} +``` + +```json +{"tool": "query", "arguments": { + "repo": "@payments-platform", + "query": "retry logic around PlaceOrder" +}} +``` + +The CLI equivalents still exist for scripting: + +```bash +npx gitnexus group impact payments-platform \ + --repo orders --target PlaceOrder --direction upstream --cross-depth 2 +``` + +Phase 1 walks within the anchor member; Phase 2 hops across the Contract Bridge wherever a cross-link endpoint matches an impacted symbol. See [`cross-impact.ts`](../../gitnexus/src/core/group/cross-impact.ts) for the bridge query. + +## How gRPC extraction works + +`GrpcExtractor` ([`grpc-extractor.ts`](../../gitnexus/src/core/group/extractors/grpc-extractor.ts)) runs two passes per member repo: + +1. **Proto map.** Every `**/*.proto` file is parsed to enumerate `service Foo { rpc Bar(...) }` blocks and (transitively) resolve the package name. Each RPC method becomes a provider contract with `contractId = grpc::./` and `confidence = 0.85`. Parsing uses the vendored `tree-sitter-proto` grammar when available and falls back to a length-preserving manual parser (`extractServiceBlocks`) otherwise, so `.proto` extraction works on platforms where the grammar fails to build. +2. **Source scan.** Every source file whose extension matches [`GRPC_SCAN_GLOB`](../../gitnexus/src/core/group/extractors/grpc-patterns/index.ts) is parsed by its language plugin: + +| Language | Provider signal | Consumer signal | +|----------|-----------------|-----------------| +| Go ([`go.ts`](../../gitnexus/src/core/group/extractors/grpc-patterns/go.ts)) | `pb.RegisterXxxServer(...)`, `pb.UnimplementedXxxServer` embedded in struct | `pb.NewXxxClient(conn)` | +| Java ([`java.ts`](../../gitnexus/src/core/group/extractors/grpc-patterns/java.ts)) | `extends XxxServiceGrpc.XxxServiceImplBase` (with or without `@GrpcService`) | `XxxServiceGrpc.newBlockingStub(...)`, `newStub(...)` | +| Python ([`python.ts`](../../gitnexus/src/core/group/extractors/grpc-patterns/python.ts)) | `add_XxxServicer_to_server(...)` (bare or `_pb2_grpc.` attribute form) | `XxxStub(channel)` (ignores `Mock`/`Test`/`Fake`/`Stub`) | +| Node / TS ([`node.ts`](../../gitnexus/src/core/group/extractors/grpc-patterns/node.ts)) | NestJS `@GrpcMethod('Service','Method')` | `@GrpcClient` field typed `XxxServiceClient`, `client.getService('Service')`, `new XxxServiceClient(...)`, `new foo.bar.XxxService(...)` in files that call `loadPackageDefinition` | + +For each source-scan detection the extractor looks up the short service name in the proto map and picks: + +- `grpc::./` when a method is named and the service resolves against the proto map, +- `grpc::./*` (wildcard) when only the service is known, or +- `grpc::/*` when no `.proto` is available at all. + +Provider detections land at confidence 0.8 (with proto) or 0.65 (without); consumers at 0.75 or 0.55. NestJS `@GrpcMethod` is fixed at 0.8 because the decorator is self-describing. + +### Matching + +`matching.ts` lowercases the package/service segment before comparing contract ids, so bindings that capitalize names differently (`auth.AuthService` vs `auth.authservice`) still match. Method names are compared case-sensitively because gRPC's wire path is case-sensitive. Service-only wildcards (`grpc::pkg.Svc/*`) match any method on the same service during cross-linking. + +### Known limitations + +- **Ambiguous proto resolution.** If a short service name exists in more than one `.proto` file and the source-scan hit can't be narrowed down by shared directory segments (`resolveProtoConflict` refuses to guess), the extractor skips contract emission and logs a warning. +- **Proto packages must be resolvable locally.** Transitive imports that point outside the repo produce an empty package segment, which means the contract id collapses to `grpc::/`. Cross-repo matches still work as long as both sides agree on the empty package. +- **Rewrite rules are not implemented.** If the provider repo writes `grpc::orders.OrderService/PlaceOrder` and the consumer repo writes `grpc::orderspb.OrderService/PlaceOrder`, they won't cross-link automatically. Use `config.links` to declare the correspondence (see below). +- **One sync = one snapshot.** Contracts are extracted against the indexed snapshot of each repo. Re-index first, then re-sync; the `status` command and resource surface staleness. + +## When automatic extraction isn't enough + +The escape hatch is the `links` list in `group.yaml`, handled by [`ManifestExtractor`](../../gitnexus/src/core/group/extractors/manifest-extractor.ts). Each entry is a **one-directional** provider/consumer declaration: + +```yaml +version: 1 +name: payments-platform +repos: + gateway: gateway + orders: orders + inventory: inventory + +links: + # Explicit gRPC method: use when naming mismatches stop the + # automatic matcher from cross-linking. + - from: gateway + to: orders + type: grpc + contract: OrderService/PlaceOrder + role: consumer + + # Service-level link when you don't want to enumerate methods. + - from: orders + to: inventory + type: grpc + contract: InventoryService + role: consumer + + # Works for HTTP too — use `METHOD::/path` form for the exact + # handler, or just `/path` for a method-agnostic wildcard. + - from: gateway + to: orders + type: http + contract: POST::/orders + role: consumer +``` + +What the manifest extractor does (see [`manifest-extractor.ts`](../../gitnexus/src/core/group/extractors/manifest-extractor.ts)): + +1. Builds a canonical `contractId` with `buildContractId` — the same canonicalization used by the automatic extractors, so manifest links cross-match automatic contracts on the other side. +2. Tries to resolve each side to a real graph symbol (the `Route` node for HTTP, a `Function|Method` / `Class|Interface` for gRPC, a `Package|Module` for `lib`). +3. If resolution fails, falls back to a deterministic synthetic uid (`manifest::::`) so both sides still line up in cross-impact — name-only links still work when the symbol isn't in the graph. +4. Emits both a provider and a consumer `StoredContract` (confidence `1.0`, `source: "manifest"`) and a `CrossLink` with `matchType: "manifest"`. + +Use `links` for exactly the cases the extractor can't infer: different package names across repos (see #701), hand-rolled transports, cases where the provider repo isn't checked out locally but you still want a record, or any contract whose provider and consumer simply don't share a surface the extractors know how to pattern-match. + +History: the manifest extractor used to be silently skipped by the sync pipeline; that was fixed in [#827](https://github.com/abhigyanpatwari/GitNexus/pull/827) (tracking issue #826). If you ever see `config.links` with zero cross-links in `contracts.json`, make sure you're on a build that includes that fix, then re-run `group sync`. + +## Troubleshooting + +1. **`contracts.json` is empty after a sync.** Either no member repo contained a recognizable gRPC pattern, or the extractors are disabled in `detect`. Confirm `detect.grpc: true` and re-run with `--verbose`. +2. **A known provider/consumer pair doesn't cross-link.** Most common cause: the package segment differs. Check the raw contract ids with `gitnexus group contracts --unmatched` — if you see two same-method contracts with different package prefixes, add a manifest `links:` entry to bridge them (no automatic rewrite rules yet). +3. **`matchType: "manifest"` is missing entirely.** The extractor needs `config.links` to be non-empty and the sync pipeline to actually call it — verify you're on a post-#827 build. Empty contract rows for manifest links usually mean `resolveSymbol` couldn't find a graph match; the synthetic uid still lets cross-impact work, it just won't carry a file path. +4. **Ambiguous proto warnings.** Look for `[grpc-extractor] Ambiguous proto resolution` in the sync logs; that means a service name exists in multiple `.proto` files under the same repo and the path-distance heuristic couldn't pick a winner. Resolve by renaming the service or declaring the intended pairing in `config.links`. +5. **Cross-impact says "stale".** Both sides need a fresh per-repo index _and_ a fresh group sync. Order matters: `gitnexus analyze` in each changed repo, then `gitnexus group sync `. Use `gitnexus group status ` to see which side is behind. + +## Related docs and references + +- [AGENTS.md](../../AGENTS.md) — authoritative list of MCP tools and resources, including group-mode routing and the `gitnexus://group/…` resources. +- [ARCHITECTURE.md](../../ARCHITECTURE.md) — overall data flow and the call-resolution DAG that the per-repo indexer uses. +- [`gitnexus/src/core/group/`](../../gitnexus/src/core/group/) — `service.ts`, `sync.ts`, `config-parser.ts`, `matching.ts`. +- [`gitnexus/src/core/group/extractors/grpc-extractor.ts`](../../gitnexus/src/core/group/extractors/grpc-extractor.ts) and [`grpc-patterns/`](../../gitnexus/src/core/group/extractors/grpc-patterns/) — gRPC detection. +- [`gitnexus/src/core/group/extractors/manifest-extractor.ts`](../../gitnexus/src/core/group/extractors/manifest-extractor.ts) — the `config.links` escape hatch. +- [`gitnexus/src/mcp/tools.ts`](../../gitnexus/src/mcp/tools.ts) — MCP tool schemas (`group_list`, `group_sync`, plus `@` routing on `impact` / `query` / `context`). +- [`gitnexus/src/cli/group.ts`](../../gitnexus/src/cli/group.ts) — CLI command definitions and flags. +- Upstream issues: [#701](https://github.com/abhigyanpatwari/GitNexus/issues/701), [#826](https://github.com/abhigyanpatwari/GitNexus/issues/826), [#906](https://github.com/abhigyanpatwari/GitNexus/issues/906). From f14068e09b9dcc93611afb0df4f09061149fd8e6 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Apr 2026 08:50:33 +0100 Subject: [PATCH 05/28] fix(fts): Don't cache failed FTS index ensure; invalidate on pool teardown (#1006) * Initial plan * Don't cache failed FTS index ensure; invalidate on pool teardown Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/425d41bd-2cc1-49f6-8cc5-57368f0f238e Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> --- gitnexus/src/core/lbug/pool-adapter.ts | 34 ++++++ gitnexus/src/core/search/bm25-index.ts | 66 ++++++++++-- gitnexus/test/unit/bm25-search.test.ts | 144 ++++++++++++++++++++++++- 3 files changed, 237 insertions(+), 7 deletions(-) diff --git a/gitnexus/src/core/lbug/pool-adapter.ts b/gitnexus/src/core/lbug/pool-adapter.ts index 162ddbfa6..bc7402637 100644 --- a/gitnexus/src/core/lbug/pool-adapter.ts +++ b/gitnexus/src/core/lbug/pool-adapter.ts @@ -35,6 +35,30 @@ interface PoolEntry { const pool = new Map(); +/** + * Listeners notified when a pool entry is torn down (LRU eviction, idle + * timeout, explicit close). Used by upper layers (e.g. the BM25 search + * module) to invalidate per-repo caches that must not outlive the pool + * entry that produced them. + * + * Listeners run synchronously inside `closeOne` after the pool entry has + * been removed; throwing listeners are isolated so one bad listener does + * not prevent others from firing or break teardown. + */ +type PoolCloseListener = (repoId: string) => void; +const poolCloseListeners = new Set(); + +/** + * Subscribe to pool-close events. Returns a disposer that removes the + * listener (handy for tests). + */ +export function addPoolCloseListener(listener: PoolCloseListener): () => void { + poolCloseListeners.add(listener); + return () => { + poolCloseListeners.delete(listener); + }; +} + /** * Shared Database cache keyed by resolved dbPath. * Multiple repoIds pointing to the same path share one native Database @@ -159,6 +183,16 @@ function closeOne(repoId: string): void { } pool.delete(repoId); + + // Notify listeners AFTER the pool entry is gone so any cache-invalidation + // they perform is consistent with `isLbugReady(repoId) === false`. + for (const listener of poolCloseListeners) { + try { + listener(repoId); + } catch { + // Isolate listener failures — teardown must complete. + } + } } /** diff --git a/gitnexus/src/core/search/bm25-index.ts b/gitnexus/src/core/search/bm25-index.ts index 92d8ec120..d32bce403 100644 --- a/gitnexus/src/core/search/bm25-index.ts +++ b/gitnexus/src/core/search/bm25-index.ts @@ -41,9 +41,48 @@ const FTS_INDEXES: ReadonlyArray<{ * Per-process cache for the MCP pool path: tracks which `(repoId, table)` * pairs have been ensured. The CLI/pipeline path gets its own cache inside * `lbug-adapter.ts` keyed by table/index, scoped to the singleton connection. + * + * IMPORTANT: an entry is added ONLY when the index was confirmed to exist + * (CREATE_FTS_INDEX succeeded, or failed with `'already exists'`). Other + * failures (transient lock errors, missing extension, etc.) leave the key + * unset so the next query retries instead of silently caching the failure. + * + * Entries for a given repoId are invalidated when its pool is closed — + * see the `addPoolCloseListener` registration in `searchFTSFromLbug`. */ const ensuredPoolFTS = new Set(); +/** + * Drop all ensured-FTS cache entries for a given repoId. + * + * Called from the pool-close listener so that a pool teardown / recreation + * forces the next `searchFTSFromLbug` call to re-issue `CREATE_FTS_INDEX` + * against the fresh connection rather than trust stale ensure-state from a + * previous pool lifetime. + * + * Exported for tests; the listener wiring is internal. + */ +export function invalidateEnsuredFTSForRepo(repoId: string): void { + const prefix = `${repoId}:`; + for (const key of ensuredPoolFTS) { + if (key.startsWith(prefix)) ensuredPoolFTS.delete(key); + } +} + +/** + * Tracks whether we've already wired the pool-close listener for this + * process. The pool adapter is dynamically imported, so registration + * happens lazily on the first MCP-pool-backed FTS query. + */ +let poolCloseListenerRegistered = false; +function registerPoolCloseListenerOnce( + addPoolCloseListener: (listener: (repoId: string) => void) => void, +): void { + if (poolCloseListenerRegistered) return; + poolCloseListenerRegistered = true; + addPoolCloseListener((repoId) => invalidateEnsuredFTSForRepo(repoId)); +} + async function ensureFTSIndexViaExecutor( executor: (cypher: string) => Promise, repoId: string, @@ -58,16 +97,25 @@ async function ensureFTSIndexViaExecutor( await executor( `CALL CREATE_FTS_INDEX('${table}', '${indexName}', [${propList}], stemmer := 'porter')`, ); + // Index was created successfully — safe to cache. + ensuredPoolFTS.add(key); } catch (e: any) { // 'already exists' is the happy path (index persists on disk between - // process invocations) — anything else we swallow because FTS is - // best-effort: queryFTS itself returns [] on missing-index errors. + // process invocations) — cache it. Anything else is treated as a + // transient failure: surface a one-time warning and leave the key + // unset so the NEXT query retries rather than silently using a + // cached failure (which previously disabled BM25 for the whole + // process for that repo). const msg = String(e?.message ?? ''); - if (!msg.includes('already exists')) { - // Best-effort — continue without index, queryFTS will fall back to []. + if (msg.includes('already exists')) { + ensuredPoolFTS.add(key); + } else { + console.warn( + `[gitnexus] FTS index ensure failed for repo "${repoId}" table "${table}" ` + + `(index "${indexName}"): ${msg || e}. Will retry on next query.`, + ); } } - ensuredPoolFTS.add(key); } /** @@ -131,7 +179,13 @@ export const searchFTSFromLbug = async ( // Use MCP connection pool via dynamic import // IMPORTANT: FTS queries run sequentially to avoid connection contention. // The MCP pool supports multiple connections, but FTS is best run serially. - const { executeQuery } = await import('../lbug/pool-adapter.js'); + const poolMod = await import('../lbug/pool-adapter.js'); + const { executeQuery, addPoolCloseListener } = poolMod; + // Register the pool-close listener lazily on first use so a teardown of + // the pool entry (LRU eviction, idle timeout, explicit close) drops the + // matching `ensuredPoolFTS` entries. Without this, stale ensure-state + // can outlive the pool that produced it. + registerPoolCloseListenerOnce(addPoolCloseListener); const executor = (cypher: string) => executeQuery(repoId, cypher); // Lazy-create FTS indexes on first query for this repo (analyze no longer diff --git a/gitnexus/test/unit/bm25-search.test.ts b/gitnexus/test/unit/bm25-search.test.ts index 466df3395..131743099 100644 --- a/gitnexus/test/unit/bm25-search.test.ts +++ b/gitnexus/test/unit/bm25-search.test.ts @@ -1,5 +1,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { searchFTSFromLbug, type BM25SearchResult } from '../../src/core/search/bm25-index.js'; +import { + searchFTSFromLbug, + invalidateEnsuredFTSForRepo, + type BM25SearchResult, +} from '../../src/core/search/bm25-index.js'; vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => { const actual = await importOriginal(); @@ -9,6 +13,22 @@ vi.mock('../../src/core/lbug/lbug-adapter.js', async (importOriginal) => { }; }); +// Pool adapter is dynamically imported by the MCP-pool path of +// `searchFTSFromLbug`. We mock it so we can drive the executor and the +// pool-close listener without spinning up a real LadybugDB pool. +const poolCloseListeners: Array<(repoId: string) => void> = []; +const mockExecuteQuery = vi.fn(); +vi.mock('../../src/core/lbug/pool-adapter.js', () => ({ + executeQuery: (repoId: string, cypher: string) => mockExecuteQuery(repoId, cypher), + addPoolCloseListener: (listener: (repoId: string) => void) => { + poolCloseListeners.push(listener); + return () => { + const idx = poolCloseListeners.indexOf(listener); + if (idx !== -1) poolCloseListeners.splice(idx, 1); + }; + }, +})); + describe('BM25 search', () => { describe('searchFTSFromLbug', () => { it('returns empty array when LadybugDB is not initialized', async () => { @@ -169,4 +189,126 @@ describe('BM25 search', () => { expect(results[1].rank).toBe(2); }); }); + + describe('ensureFTS cache (MCP pool path)', () => { + const REPO = 'test-repo-fts-cache'; + + beforeEach(() => { + // Clean state so cases don't bleed into each other. + mockExecuteQuery.mockReset(); + invalidateEnsuredFTSForRepo(REPO); + // Suppress the surfaced warn so test output stays readable. + vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + + it('does NOT cache a transient CREATE_FTS_INDEX failure — second call retries', async () => { + // First call: every CREATE_FTS_INDEX fails transiently; QUERY_FTS_INDEX returns nothing. + mockExecuteQuery.mockImplementation(async (_repo: string, cypher: string) => { + if (cypher.includes('CREATE_FTS_INDEX')) { + throw new Error('transient lock error: Could not set lock'); + } + return []; + }); + + const r1 = await searchFTSFromLbug('anything', 5, REPO); + expect(Array.isArray(r1)).toBe(true); + + const createCallsAfterFirst = mockExecuteQuery.mock.calls.filter((c) => + String(c[1]).includes('CREATE_FTS_INDEX'), + ).length; + // 5 FTS index tables — all five attempted on first call. + expect(createCallsAfterFirst).toBe(5); + + // Second call: CREATE succeeds this time. The bug being fixed: if the + // first failure was cached, we'd see ZERO additional CREATE calls. + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + + await searchFTSFromLbug('anything', 5, REPO); + + const createCallsOnRetry = mockExecuteQuery.mock.calls.filter((c) => + String(c[1]).includes('CREATE_FTS_INDEX'), + ).length; + expect(createCallsOnRetry).toBe(5); + }); + + it("treats 'already exists' as success and caches it (no retry on second call)", async () => { + mockExecuteQuery.mockImplementation(async (_repo: string, cypher: string) => { + if (cypher.includes('CREATE_FTS_INDEX')) { + throw new Error("Catalog exception: index 'file_fts' already exists"); + } + return []; + }); + + await searchFTSFromLbug('anything', 5, REPO); + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + + await searchFTSFromLbug('anything', 5, REPO); + + const createCallsOnSecond = mockExecuteQuery.mock.calls.filter((c) => + String(c[1]).includes('CREATE_FTS_INDEX'), + ).length; + expect(createCallsOnSecond).toBe(0); + }); + + it('invalidateEnsuredFTSForRepo drops cached entries so next call re-issues CREATE', async () => { + // Prime the cache with successful creates. + mockExecuteQuery.mockResolvedValue([]); + await searchFTSFromLbug('anything', 5, REPO); + + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + + // Without invalidation: no re-CREATE. + await searchFTSFromLbug('anything', 5, REPO); + expect( + mockExecuteQuery.mock.calls.filter((c) => String(c[1]).includes('CREATE_FTS_INDEX')).length, + ).toBe(0); + + // After invalidation: next call re-issues CREATE for all 5 tables. + invalidateEnsuredFTSForRepo(REPO); + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + await searchFTSFromLbug('anything', 5, REPO); + expect( + mockExecuteQuery.mock.calls.filter((c) => String(c[1]).includes('CREATE_FTS_INDEX')).length, + ).toBe(5); + }); + + it('a pool-close listener fired by the pool adapter invalidates this repo only', async () => { + const OTHER = 'other-repo'; + + mockExecuteQuery.mockResolvedValue([]); + // Prime both repos. + await searchFTSFromLbug('anything', 5, REPO); + await searchFTSFromLbug('anything', 5, OTHER); + + // Confirm at least one listener was registered by the search module. + expect(poolCloseListeners.length).toBeGreaterThanOrEqual(1); + + // Simulate the pool adapter closing REPO. + for (const l of poolCloseListeners) l(REPO); + + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + + await searchFTSFromLbug('anything', 5, REPO); + const createForRepo = mockExecuteQuery.mock.calls.filter( + (c) => c[0] === REPO && String(c[1]).includes('CREATE_FTS_INDEX'), + ).length; + expect(createForRepo).toBe(5); + + // OTHER repo's cache must remain intact — no re-CREATE for it. + mockExecuteQuery.mockReset(); + mockExecuteQuery.mockResolvedValue([]); + await searchFTSFromLbug('anything', 5, OTHER); + const createForOther = mockExecuteQuery.mock.calls.filter( + (c) => c[0] === OTHER && String(c[1]).includes('CREATE_FTS_INDEX'), + ).length; + expect(createForOther).toBe(0); + + invalidateEnsuredFTSForRepo(OTHER); + }); + }); }); From 0909a908ee31c1f6f249ee1895bc7eef903911d9 Mon Sep 17 00:00:00 2001 From: ivkond Date: Tue, 21 Apr 2026 13:44:16 +0300 Subject: [PATCH 06/28] fix(group): bubble local-impact phase errors in groupImpact (#1004) (#1007) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the Phase 1 local-impact leg returned a structured { error: ... } payload (missing symbol, graph-load failure, or an exception wrapped by safeLocalImpact), runGroupImpact previously buried it inside a zero-hit GroupImpactResult with empty cross / outOfScope arrays and risk 'UNKNOWN'. Callers branch on top-level `error` (CLI, MCP wrapper), so the failure path surfaced as a silent "no impact across the group" — a false negative on a safety-critical blast-radius tool. Fail closed: bubble the error as a top-level { error } prefixed with the repoPath, matching how runGroupImpact already handles resolveGroupRepo, config-load, and bridgePrep failures. Chose option 1 (bubble the error) over option 2 (partial-result discriminant) because runGroupImpact only runs local impact for a single member repo at this point — cross-repo fan-out happens later via the bridge, so there is no partial success data to preserve on the local-phase failure path. Added two regression tests covering both the port-returned { error } case and the thrown-exception case (wrapped by safeLocalImpact). Made-with: Cursor --- gitnexus/src/core/group/cross-impact.ts | 25 ++---- gitnexus/test/unit/group/cross-impact.test.ts | 86 +++++++++++++++++++ 2 files changed, 93 insertions(+), 18 deletions(-) diff --git a/gitnexus/src/core/group/cross-impact.ts b/gitnexus/src/core/group/cross-impact.ts index 8739584c7..a80336b26 100644 --- a/gitnexus/src/core/group/cross-impact.ts +++ b/gitnexus/src/core/group/cross-impact.ts @@ -380,24 +380,13 @@ export async function runGroupImpact( const localObj = local as Record | null; if (localObj?.error && typeof localObj.error === 'string') { - const empty: GroupImpactResult = { - local, - group: name, - cross: [], - outOfScope: [], - truncated: false, - truncatedRepos: [], - summary: { - direct: 0, - processes_affected: 0, - modules_affected: 0, - cross_repo_hits: 0, - }, - risk: 'UNKNOWN', - timeoutMs, - crossDepthWarning, - }; - return empty; + // Fail closed: the local-impact phase errored (missing symbol, graph-load + // failure, thrown exception wrapped by safeLocalImpact, or port-returned + // `{ error }`). Do NOT wrap it into a zero-hit success payload — callers + // branch on top-level `error`, and a blast-radius tool reporting "no + // impact" on the failure path is a false negative on a safety-critical + // signal. Bubble the error so consumers treat it as a failure. + return { error: `Local impact failed for ${repoPath}: ${localObj.error}` }; } if (servicePrefix) { diff --git a/gitnexus/test/unit/group/cross-impact.test.ts b/gitnexus/test/unit/group/cross-impact.test.ts index 3d78ff1cf..e68ab6209 100644 --- a/gitnexus/test/unit/group/cross-impact.test.ts +++ b/gitnexus/test/unit/group/cross-impact.test.ts @@ -149,6 +149,92 @@ describe('cross-impact', () => { } }); + it('test_runGroupImpact_local_phase_error_bubbles_as_top_level_error', async () => { + // Regression for #1004: when the local-impact phase returns a structured + // `{ error: ... }` payload, groupImpact MUST surface it as a top-level + // `{ error }` instead of a zero-hit GroupImpactResult. Otherwise callers + // that branch on top-level `error` silently treat a failed analysis as + // "no impact across the group" — a false negative on the failure path + // of a blast-radius tool. + const { tmpDir, cleanup } = tmpGroup(); + vi.stubEnv('GITNEXUS_HOME', tmpDir); + try { + const port: GroupToolPort = { + resolveRepo: vi.fn(async () => ({ + id: 'be', + name: 'reg-be', + repoPath: '/r', + storagePath: '/r/.gitnexus', + })), + impact: vi.fn(async () => ({ error: 'symbol not found: Sym' })), + query: vi.fn(), + impactByUid: vi.fn(), + context: vi.fn(), + }; + const r = await runGroupImpact( + { port, gitnexusDir: tmpDir }, + { + name: 'g1', + repo: 'app/backend', + target: 'Sym', + direction: 'upstream', + }, + ); + expect('error' in r).toBe(true); + if ('error' in r) { + expect(r.error).toContain('symbol not found: Sym'); + expect(r.error).toContain('app/backend'); + } + // And ensure we didn't silently fall back to a zero-hit success payload. + expect((r as { summary?: unknown }).summary).toBeUndefined(); + expect((r as { cross?: unknown }).cross).toBeUndefined(); + } finally { + vi.unstubAllEnvs(); + cleanup(); + } + }); + + it('test_runGroupImpact_local_phase_thrown_exception_bubbles_as_top_level_error', async () => { + // Companion to the #1004 regression: safeLocalImpact wraps thrown + // exceptions from port.impact() as `{ error }` payloads. Those must + // bubble to the caller as top-level errors too, not be swallowed into + // an empty success payload. + const { tmpDir, cleanup } = tmpGroup(); + vi.stubEnv('GITNEXUS_HOME', tmpDir); + try { + const port: GroupToolPort = { + resolveRepo: vi.fn(async () => ({ + id: 'be', + name: 'reg-be', + repoPath: '/r', + storagePath: '/r/.gitnexus', + })), + impact: vi.fn(async () => { + throw new Error('graph-load failure: .gitnexus missing'); + }), + query: vi.fn(), + impactByUid: vi.fn(), + context: vi.fn(), + }; + const r = await runGroupImpact( + { port, gitnexusDir: tmpDir }, + { + name: 'g1', + repo: 'app/backend', + target: 'Sym', + direction: 'upstream', + }, + ); + expect('error' in r).toBe(true); + if ('error' in r) { + expect(r.error).toContain('graph-load failure'); + } + } finally { + vi.unstubAllEnvs(); + cleanup(); + } + }); + it('test_runGroupImpact_bridge_schema_mismatch_returns_error', async () => { const { tmpDir, groupDir, cleanup } = tmpGroup(); vi.stubEnv('GITNEXUS_HOME', tmpDir); From bd271da7b765747f98c59af25eb646a7a3d17778 Mon Sep 17 00:00:00 2001 From: azizur100389 Date: Tue, 21 Apr 2026 11:52:59 +0100 Subject: [PATCH 07/28] feat(cli): gitnexus remove to unindex a registered repo by name or path (#664) (#1003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(cli): gitnexus remove to unindex a registered repo by name or path (#664) Add a `remove` CLI command that deletes the `.gitnexus/` index AND unregisters a repo from the global registry (~/.gitnexus/registry.json), addressing the lifecycle gap flagged in #664: previously users had to cd into the repo to run `clean`, and there was no path-based or alias-based remove for an already-deleted working tree. - New command `gitnexus remove [-f|--force]`. `` is alias / basename-derived name / remote-inferred name / absolute path. - New helper `resolveRegistryEntry(entries, target)` in repo-manager.ts with path > name precedence; throws RegistryNotFoundError or RegistryAmbiguousTargetError (typed, `kind`-discriminated). - Atomicity mirrors `clean`: fs.rm first, then unregisterRepo; partial failures self-heal on next `listRegisteredRepos({ validate: true })`. - Idempotent on unknown targets (exit 0 with warning) per the #664 spec: "behave atomically and idempotently so retries are safe". - `--force` uses `clean`-style confirmation-skip semantics — distinct from `analyze --force` (pipeline re-index); here there is no pipeline so no conflation. - 7 new unit tests cover resolver precedence, case sensitivity, ambiguity, and not-found hints; 2 integration tests cover the real CLI -> registry -> filesystem chain including the --allow-duplicate-name (#829) ambiguity case. * fix(cli): canonicalize repo paths so remove/register match across platforms (#1003 review) Address review feedback from @evander-wang and @magyargergo on PR #1003 plus the Windows + macOS CI failure (same root cause). Problem: - macOS: /var is a symlink to /private/var. `path.resolve` does NOT follow symlinks, so a child running analyze in /var/folders/X stores /private/var/folders/X (realpath from OS cwd) but an outer caller passing the symlink form misses. - Windows: GitHub runners surface tmpdirs in 8.3 short-name form (RUNNERA~1) while process.cwd() returns the long form (runneradmin). Same divergence. Fix: new `canonicalizePath(p)` helper wraps `path.resolve` plus `fs.realpathSync.native`, falling back to `path.resolve` when the path doesn't exist (preserves idempotent-on-missing semantics needed by `remove `). Applied at 3 call-sites — registerRepo, unregisterRepo, resolveRegistryEntry — canonicalising BOTH the input and each stored `entry.path` at compare time. That last bit is the backward-compat story: registries written by older versions (pre-canonicalisation) still match correctly, so we don't need a migration script. Test side: the ambiguous-target integration test now reads the path from the registry snapshot rather than passing the outer `repoA` variable directly, so it exercises the registry contract regardless of which path form the platform stores. 4 new unit tests cover the helper (idempotent, fallback-on-missing, absolute-for-relative) plus the backward-compat resolver path. * fix(cli): store resolved (non-canonical) path, compare via canonicalizePath (#1003 CI) Follow-up to c5eceba0. The previous commit canonicalised the repo path at BOTH write-time AND compare-time in registerRepo — that expanded Windows 8.3 short names (RUNNER~1) to long names (runneradmin) when storing `entry.path`. Pre-existing #829 unit tests that assert `path.resolve(err.existingPath) === path.resolve(tmpPath)` then broke because `tmpPath` is still short-form (path.resolve doesn't expand 8.3) while `entry.path` was long-form (canonicalizePath does). Fix: split storage from comparison. - entry.path stores `path.resolve(repoPath)` — whatever form the caller passed. `list` output and error messages show the path the user typed. - All compare points (existing-entry lookup in registerRepo, the collision guard, unregisterRepo, resolveRegistryEntry path tier) canonicalise BOTH sides via `canonicalizePath`. That is where the /var ↔ /private/var and RUNNER~1 ↔ runneradmin divergence actually matters. Net effect: storage is tolerant (preserves user input), matching is strict (canonical-vs-canonical). Pre-existing #829 tests stay green because `err.existingPath` is unchanged from what `path.resolve` gives back; the cross-platform CI failure from #1003 stays fixed because every comparison path goes through `canonicalizePath`. * fix(cli): refuse destructive fs.rm when registry storagePath isn't /.gitnexus (#1003 review) Address @magyargergo's inline review finding on remove.ts:89 and the sibling vulnerability in clean.ts --all (caught during a pre-commit safety audit). ~/.gitnexus/registry.json is a user-writable plain-text file, so a corrupted or hand-edited entry could point storagePath at the repo root (catastrophic: rm the working tree), an empty string (→ cwd), a parent dir, or anywhere else. fs.rm(recursive: true, force: true) on any of those is a runtime disaster. - New UnsafeStoragePathError + exported assertSafeStoragePath() in repo-manager.ts. Pure lexical string check (Windows-case- insensitive) asserting entry.storagePath === path.join(entry.path, '.gitnexus'). - Guard wired into BOTH destructive registry-trusting sites: - remove.ts: exit 1 with actionable hint - clean.ts --all: skip the poisoned entry with a warning and continue (preserves existing per-repo error tolerance — one bad entry doesn't halt the batch) - clean.ts default path and server/api.ts are safe-by-construction (they recompute storagePath from findRepo / getStoragePath rather than trusting the registry field). - 8 unit tests cover the guard (valid, repo-root, parent, empty, unrelated, sibling, error payload, Windows case). - 2 integration tests prove the full CLI path: remove-poisoned exits 1 without touching the working tree; clean --all with a poisoned sibling entry cleans the good entry, skips the bad one, and leaves the poisoned repo intact. * test(cli): assert full remove dry-run + success output shape (#1003 NIT) Address the one NIT from the senior-reviewer pass on PR #1003: the integration test was only checking for the "Run with --force" hint in dry-run output, not verifying that the three actual console.log lines (alias, repo path, storage path) appear. Same weak check on the success-branch "Removed" output. Tighten both assertions to toContain(alias), toContain(entry.path), toContain(storagePath). Catches silent format regressions — e.g. a future refactor that drops a console.log line or swaps entry.name/entry.path in the output. No code change; +20 test lines. All assertions in the happy-path integration test now fire for a meaningful reason. --- gitnexus/src/cli/clean.ts | 26 +- gitnexus/src/cli/index.ts | 9 + gitnexus/src/cli/remove.ts | 110 ++++++ gitnexus/src/storage/repo-manager.ts | 265 +++++++++++++- gitnexus/test/integration/cli-e2e.test.ts | 404 ++++++++++++++++++++++ gitnexus/test/unit/repo-manager.test.ts | 338 ++++++++++++++++++ 6 files changed, 1146 insertions(+), 6 deletions(-) create mode 100644 gitnexus/src/cli/remove.ts diff --git a/gitnexus/src/cli/clean.ts b/gitnexus/src/cli/clean.ts index e89cc7f23..4681508fb 100644 --- a/gitnexus/src/cli/clean.ts +++ b/gitnexus/src/cli/clean.ts @@ -6,7 +6,13 @@ */ import fs from 'fs/promises'; -import { findRepo, unregisterRepo, listRegisteredRepos } from '../storage/repo-manager.js'; +import { + findRepo, + unregisterRepo, + listRegisteredRepos, + assertSafeStoragePath, + UnsafeStoragePathError, +} from '../storage/repo-manager.js'; export const cleanCommand = async (options?: { force?: boolean; all?: boolean }) => { // --all flag: clean all indexed repos @@ -27,6 +33,24 @@ export const cleanCommand = async (options?: { force?: boolean; all?: boolean }) const entries = await listRegisteredRepos(); for (const entry of entries) { + // Safety guard (#1003 review — @magyargergo): same rationale as + // remove.ts. `~/.gitnexus/registry.json` is user-writable, so a + // corrupted or hand-edited entry could point storagePath at the + // repo root, an empty string, or anywhere else — and + // fs.rm(recursive: true) on any of those would be catastrophic. + // Skip poisoned entries without touching disk, but keep going + // through the rest of the registry (preserves the existing + // per-repo error-tolerance semantics of `clean --all`). + try { + assertSafeStoragePath(entry); + } catch (err) { + if (err instanceof UnsafeStoragePathError) { + console.error(`Refusing to clean ${entry.name}: ${err.message}`); + continue; + } + throw err; + } + try { await fs.rm(entry.storagePath, { recursive: true, force: true }); await unregisterRepo(entry.path); diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 2b54f04f3..fe19ba753 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -83,6 +83,15 @@ program .option('--all', 'Clean all indexed repos') .action(createLazyAction(() => import('./clean.js'), 'cleanCommand')); +program + .command('remove ') + .description( + 'Delete the GitNexus index for a registered repo (by alias, name, or absolute path). ' + + 'Unlike `clean`, does not require being inside the repo. Idempotent on unknown targets.', + ) + .option('-f, --force', 'Skip confirmation prompt') + .action(createLazyAction(() => import('./remove.js'), 'removeCommand')); + program .command('wiki [path]') .description('Generate repository wiki from knowledge graph') diff --git a/gitnexus/src/cli/remove.ts b/gitnexus/src/cli/remove.ts new file mode 100644 index 000000000..4d2ce0771 --- /dev/null +++ b/gitnexus/src/cli/remove.ts @@ -0,0 +1,110 @@ +/** + * Remove Command (#664) + * + * Delete the `.gitnexus/` index for a registered repo and unregister it + * from the global registry (~/.gitnexus/registry.json). The target is + * identified by alias / basename-derived name / remote-inferred name / + * absolute path — no `--repo` flag, just a positional argument so the + * destructive-command ergonomics match `clean` (which is also + * destructive but scoped to `process.cwd()`). + * + * Compared to `clean`: + * - `clean` acts on the repo discovered by walking up from cwd. + * - `remove` acts on any registered repo identified by name or path. + * + * Behaviour notes: + * - Idempotent on unknown targets: exits 0 with a warning so that + * `remove X && analyze Y` keeps working in scripts. Per #664: + * "behave atomically and idempotently so retries are safe". + * - Atomic order mirrors `clean`: fs.rm FIRST, then unregister. A + * partial failure leaves the registry pointing at a missing dir + * (recoverable by `listRegisteredRepos({ validate: true })` on + * next read) rather than the opposite, which would orphan + * .gitnexus/ directories on disk. + * - `-f` / `--force` matches the confirmation-skip semantics of + * `clean -f`. (Distinct from `analyze --force`, which re-indexes; + * here there is no pipeline, so no conflation.) + */ + +import fs from 'fs/promises'; +import { + readRegistry, + resolveRegistryEntry, + assertSafeStoragePath, + unregisterRepo, + RegistryNotFoundError, + RegistryAmbiguousTargetError, + UnsafeStoragePathError, +} from '../storage/repo-manager.js'; + +export const removeCommand = async (target: string, options?: { force?: boolean }) => { + // Read the registry snapshot once and pass it to the resolver — this + // lets us render the "before" state in the dry-run path without a + // second disk read. + const entries = await readRegistry(); + + let entry; + try { + entry = resolveRegistryEntry(entries, target); + } catch (err) { + if (err instanceof RegistryNotFoundError) { + // Idempotent: missing target is a no-op warning, not an error. + // The `availableNames` hint comes from the error itself so users + // can see what they might have meant. + console.warn(`Nothing to remove: ${err.message}`); + return; + } + if (err instanceof RegistryAmbiguousTargetError) { + // Duplicate aliases are allowed via --allow-duplicate-name (#829); + // refuse to guess which one the user meant — surface the full list + // and exit non-zero so scripts don't silently pick the wrong repo. + console.error(`Error: ${err.message}`); + process.exit(1); + } + throw err; + } + + // Confirmation gate — same shape as `clean`. Default is a dry-run + // that describes what would be deleted; `--force` actually deletes. + if (!options?.force) { + console.log(`This will delete the GitNexus index for: ${entry.name}`); + console.log(` Path: ${entry.path}`); + console.log(` Storage: ${entry.storagePath}`); + console.log('\nRun with --force to confirm deletion.'); + return; + } + + // Safety guard (#1003 review — @magyargergo): refuse to proceed if + // the registry entry's `storagePath` isn't the canonical + // `/.gitnexus` subfolder. `~/.gitnexus/registry.json` is + // user-writable, so a corrupted or hand-edited entry could point + // storagePath at the repo root, an empty string (→ cwd), a parent + // dir, or anywhere else; `fs.rm(recursive: true, force: true)` on + // any of those would be a runtime disaster. Bail before touching + // disk, with an actionable hint for recovering a broken registry. + try { + assertSafeStoragePath(entry); + } catch (err) { + if (err instanceof UnsafeStoragePathError) { + console.error(`Error: ${err.message}`); + process.exit(1); + } + throw err; + } + + // Deletion order: fs.rm first, then unregister. If fs.rm fails mid-way, + // the registry entry stays so the user can retry. If fs.rm succeeds but + // unregister throws (e.g. ENOSPC on registry write), the entry becomes + // orphaned — `listRegisteredRepos({ validate: true })` prunes those on + // next read, so the failure is self-healing. + try { + await fs.rm(entry.storagePath, { recursive: true, force: true }); + await unregisterRepo(entry.path); + console.log(`Removed: ${entry.name}`); + console.log(` Path: ${entry.path}`); + console.log(` Storage: ${entry.storagePath}`); + } catch (err) { + console.error(`Failed to remove ${entry.name}:`, err); + process.exit(1); + } +}; diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 1b151cec1..8155e592f 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -7,10 +7,50 @@ */ import fs from 'fs/promises'; +import { realpathSync } from 'fs'; import path from 'path'; import os from 'os'; import { getInferredRepoName } from './git.js'; +/** + * Normalise a repo path for registry comparison across platforms + * (#664 review feedback from @evander-wang). + * + * Why this exists: `path.resolve` alone is NOT enough for + * cross-platform registry stability. + * - **macOS**: tmpdirs and `/var` are symlinks to `/private/var`. + * A child process that stored `/private/var/folders/.../repo` in + * the registry cannot later be matched by an outer caller that + * supplies the symlink form `/var/folders/.../repo`. `path.resolve` + * does not follow symlinks; `realpathSync.native` does. + * - **Windows**: GitHub runners surface tmpdirs in 8.3 short-name + * form (`RUNNERA~1\...`), but `process.cwd()` often returns the + * long form (`runneradmin\...`). `realpathSync.native` normalises + * both sides to the long-name canonical path. + * + * Fallback behaviour: if the path does not exist on disk (e.g. a user + * passed `gitnexus remove some-alias` and the alias misses every + * registry entry, or the caller is resolving a path that was deleted + * after registration), we return `path.resolve(p)` rather than + * throwing. This preserves the idempotent-on-missing semantics of + * `resolveRegistryEntry` / `remove`. + * + * Backwards compatibility: this function is applied to BOTH the + * caller-supplied input AND each stored `entry.path` at compare time + * inside `resolveRegistryEntry`, so registries written by older + * versions (where `registerRepo` only ran `path.resolve`) still match + * correctly. Newly-written entries are canonicalised at write time too + * so the registry stabilises over analyze/re-analyze cycles. + */ +export const canonicalizePath = (p: string): string => { + const resolved = path.resolve(p); + try { + return realpathSync.native(resolved); + } catch { + return resolved; + } +}; + export interface RepoMeta { repoPath: string; lastCommit: string; @@ -349,13 +389,33 @@ export const registerRepo = async ( meta: RepoMeta, opts?: RegisterRepoOptions, ): Promise => { + // Preserve the caller's chosen path form in the registry — don't + // canonicalise at write time. This matters for two reasons: + // 1. `list` and error messages show the path the user actually + // knows (e.g. the 8.3 short form they typed), not a runtime- + // resolved long form they've never seen. + // 2. Keeps pre-existing #829 test assertions that compare + // `err.existingPath` against `path.resolve(tmpPath)` stable. + // Canonicalisation is applied at COMPARE points only (see below), + // which is where the cross-platform divergence actually matters. const resolved = path.resolve(repoPath); const { storagePath } = getStoragePaths(resolved); + // Canonical form used strictly for comparison — `realpathSync.native` + // expands macOS /var → /private/var and Windows 8.3 → long-name, + // falling back to `path.resolve` when the path doesn't exist. + const canonicalInput = canonicalizePath(repoPath); + const entries = await readRegistry(); const existingIdx = entries.findIndex((e) => { - const a = path.resolve(e.path); - const b = resolved; + // Canonicalise the STORED entry too so pre-canonicalisation + // registries (written by older versions, or paths passed in a + // different form) still match correctly. `canonicalizePath` falls + // back to `path.resolve` when the path no longer exists on disk, + // so stale entries that have been rm'd externally still resolve + // to a stable key instead of throwing. + const a = canonicalizePath(e.path); + const b = canonicalInput; return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; }); const existing = existingIdx >= 0 ? entries[existingIdx] : null; @@ -389,11 +449,14 @@ export const registerRepo = async ( // messages and list output #829 ships). const explicitName = opts?.name !== undefined || isPreservedAlias; if (explicitName && !opts?.allowDuplicateName) { + // Compare canonical-vs-canonical here too so `/var/foo` and + // `/private/var/foo` (same repo, different form) aren't treated as + // two colliding paths. const collidingEntry = entries.find( (e, i) => i !== existingIdx && e.name.toLowerCase() === name.toLowerCase() && - path.resolve(e.path) !== resolved, + canonicalizePath(e.path) !== canonicalInput, ); if (collidingEntry) { throw new RegistryNameCollisionError(name, collidingEntry.path, resolved); @@ -424,12 +487,204 @@ export const registerRepo = async ( * Called after `gitnexus clean`. */ export const unregisterRepo = async (repoPath: string): Promise => { - const resolved = path.resolve(repoPath); + // Canonicalise BOTH sides so an unregister call issued with the + // symlink form (`/var/folders/.../repo`) still matches an entry + // written with the realpath form (`/private/var/folders/.../repo`), + // and vice versa. Matches the semantics of `registerRepo` and + // `resolveRegistryEntry` post-#1003 review. + const resolved = canonicalizePath(repoPath); const entries = await readRegistry(); - const filtered = entries.filter((e) => path.resolve(e.path) !== resolved); + const matches = (a: string, b: string) => + process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + const filtered = entries.filter((e) => !matches(canonicalizePath(e.path), resolved)); await writeRegistry(filtered); }; +/** + * Thrown by {@link resolveRegistryEntry} when no registered repo matches + * the caller's target string (by alias, basename, remote-inferred name, + * or resolved path). CLI callers that want idempotent "remove" semantics + * should catch this and exit 0 with a warning; non-idempotent callers + * (e.g. MCP tools) can surface the error directly. + */ +export class RegistryNotFoundError extends Error { + readonly kind = 'RegistryNotFoundError' as const; + constructor( + public readonly target: string, + public readonly availableNames: string[], + ) { + const hint = + availableNames.length > 0 + ? ` Available: ${availableNames.join(', ')}.` + : ' No repositories are currently registered.'; + super(`No registered repo matches "${target}".${hint}`); + this.name = 'RegistryNotFoundError'; + } +} + +/** + * Thrown by {@link resolveRegistryEntry} when the target string matches + * the `name` of two or more entries — only possible when the user + * previously registered duplicates via `analyze --name X + * --allow-duplicate-name` (#829). The error carries enough information + * for the caller to render an actionable disambiguation hint without + * string-matching on `.message`. + * + * `kind` is a string literal discriminant (same pattern as + * {@link RegistryNameCollisionError}) so callers can narrow via + * `err.kind === 'RegistryAmbiguousTargetError'` without importing the + * class. + */ +export class RegistryAmbiguousTargetError extends Error { + readonly kind = 'RegistryAmbiguousTargetError' as const; + constructor( + public readonly target: string, + public readonly matches: RegistryEntry[], + ) { + const listing = matches.map((m) => ` - ${m.name} (${m.path})`).join('\n'); + super( + `Multiple registered repos match "${target}":\n${listing}\n` + + `Pass the absolute path instead to disambiguate.`, + ); + this.name = 'RegistryAmbiguousTargetError'; + } +} + +/** + * Thrown by {@link assertSafeStoragePath} when a registry entry's + * `storagePath` does NOT point at the expected `/.gitnexus` + * subfolder. CLI destructive commands (`remove`, `clean --all`) should + * catch this and exit non-zero without deleting anything — the usual + * cause is a corrupted or hand-edited `~/.gitnexus/registry.json`, and + * proceeding would mean `fs.rm(recursive: true)` on whatever odd path + * the entry is pointing at. + */ +export class UnsafeStoragePathError extends Error { + readonly kind = 'UnsafeStoragePathError' as const; + constructor( + public readonly entry: RegistryEntry, + public readonly expectedStoragePath: string, + public readonly actualStoragePath: string, + ) { + super( + `Refusing to remove storage path for safety: expected ` + + `"${expectedStoragePath}" under the repo's .gitnexus subfolder, ` + + `but the registry entry has "${actualStoragePath}". ` + + `This usually means the registry entry is corrupted or was ` + + `hand-edited. Delete the entry manually from ~/.gitnexus/registry.json ` + + `and re-run analyze.`, + ); + this.name = 'UnsafeStoragePathError'; + } +} + +/** + * Guard rail for destructive CLI paths (`remove` #664, + * `clean --all` #258, future MCP `remove` tool): verify that a + * registry entry's `storagePath` is the canonical `/.gitnexus` + * subfolder of its `path`. If not, throw {@link UnsafeStoragePathError} + * so the caller exits without touching disk. + * + * Why this exists (#1003 review — @magyargergo): + * - `~/.gitnexus/registry.json` is a plain-text user-writable file. + * A corrupted, hand-edited, or downgrade/upgrade-racing entry + * could plausibly end up with `storagePath === ""` (resolves to + * cwd), `storagePath === path` (the repo root!), `storagePath` + * equal to a parent/sibling of the repo, or simply any arbitrary + * filesystem path. + * - `fs.rm(recursive: true, force: true)` on ANY of those would be + * a runtime disaster — at best delete the user's working tree, at + * worst nuke an unrelated directory tree they happen to own. + * - `clean` (default, cwd-scoped) is safe by construction — it + * re-derives storagePath from `findRepo(cwd)` and never trusts + * the registry field. But `clean --all` DOES iterate the registry + * and trust each entry's stored storagePath (same shape as + * `remove`), so this helper must be wired into that loop too. + * - `server/api.ts` recomputes storagePath from `getStoragePath(entry.path)` + * and so is likewise safe-by-construction. + * + * Pure string check — does NOT require the paths to exist on disk. + * Windows: case-insensitive; POSIX: case-sensitive. Matches the + * comparison shape used elsewhere in this module. + */ +export const assertSafeStoragePath = (entry: RegistryEntry): void => { + const expected = path.join(path.resolve(entry.path), '.gitnexus'); + const actual = path.resolve(entry.storagePath); + const matches = + process.platform === 'win32' + ? expected.toLowerCase() === actual.toLowerCase() + : expected === actual; + if (!matches) { + throw new UnsafeStoragePathError(entry, expected, actual); + } +}; + +/** + * Resolve a user-supplied target string (from `gitnexus remove ` + * or equivalent MCP tool argument) to a single registry entry. + * + * Match precedence (first hit wins, subsequent tiers are only tried if + * the prior tier produces zero matches): + * 1. Exact resolved-path match (Windows: case-insensitive). + * Paths are unique by registry construction, so a path match can + * never be ambiguous. + * 2. Exact `name` match (case-insensitive). If ≥ 2 entries share the + * name — only possible via `--allow-duplicate-name` (#829) — + * throws {@link RegistryAmbiguousTargetError}. + * + * No fuzzy / partial matching — unambiguous, scriptable behaviour is + * more important than convenience for destructive commands. + * + * Throws {@link RegistryNotFoundError} if no entry matches. + * + * `entries` is passed in (rather than re-read) so callers that already + * hold the registry snapshot (e.g. to print a "before" state) can avoid + * a second disk read, and so tests can inject fixtures without touching + * `GITNEXUS_HOME`. + */ +export const resolveRegistryEntry = (entries: RegistryEntry[], target: string): RegistryEntry => { + // Tier 1: path match. Canonicalise BOTH sides so symlink and + // Windows-8.3 quirks don't cause a false miss — e.g. the caller + // passes `/var/folders/.../repo` while the registry has + // `/private/var/folders/.../repo` (both resolve to the same + // `realpath.native`). See `canonicalizePath` for the rationale. + // + // Canonicalising the STORED entry (not just the input) is what gives + // us backward-compat for registries written by versions that only + // ran `path.resolve` — both get canonicalised here at compare time. + const canonicalTarget = canonicalizePath(target); + const pathMatch = entries.find((e) => { + const a = canonicalizePath(e.path); + const b = canonicalTarget; + return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + }); + if (pathMatch) return pathMatch; + + // Tier 2: name match. Case-insensitive on all platforms — registry + // name collisions are already filtered case-insensitively in + // `registerRepo`, so "APP" vs "app" are considered the same key. + const targetLower = target.toLowerCase(); + const nameMatches = entries.filter((e) => e.name.toLowerCase() === targetLower); + if (nameMatches.length === 1) return nameMatches[0]; + if (nameMatches.length > 1) { + throw new RegistryAmbiguousTargetError(target, nameMatches); + } + + // Tier 3: miss. Build the available-names hint ONCE; resolveRepo-style + // disambiguated labels (`app (/path)`) are applied when the same name + // appears in multiple entries so the user sees the same hint shape as + // `-r ` errors. + const nameCounts = new Map(); + for (const e of entries) { + const key = e.name.toLowerCase(); + nameCounts.set(key, (nameCounts.get(key) ?? 0) + 1); + } + const availableNames = entries.map((e) => + (nameCounts.get(e.name.toLowerCase()) ?? 0) > 1 ? `${e.name} (${e.path})` : e.name, + ); + throw new RegistryNotFoundError(target, availableNames); +}; + /** * List all registered repos from the global registry. * Optionally validates that each entry's .gitnexus/ still exists. diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index 844e3978d..f994b42ae 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -345,6 +345,410 @@ describe('CLI end-to-end', () => { }, 360000); // 6-min outer budget (4 × ~60s analyze calls + fixture setup) }); + // ─── gitnexus remove (#664) ───────────────────────────── + // + // End-to-end regression guard for the remove command: + // 1. `remove ` without --force is a dry-run (exit 0, preserves state) + // 2. `remove --force` deletes the .gitnexus/ directory + // AND unregisters from the global registry + // 3. `remove ` is idempotent (exit 0 with a warning) + // 4. `remove ` (two entries share the alias via + // --allow-duplicate-name) exits 1 with a disambiguation hint + // and leaves the registry unchanged. + // + // Every assertion reads the real registry.json on disk, so any + // regression in remove.ts → resolveRegistryEntry → unregisterRepo + // will surface here. + describe('remove (#664)', () => { + it('dry-run lists, --force deletes, missing target is a no-op warning', () => { + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-remove-')); + const repoA = makeMiniRepoCopy('remove-me', 'gn-rm-a-'); + const parentA = path.dirname(repoA); + + try { + // Index the repo under a custom alias so we can target it by + // name below. `--name` guarantees a stable alias regardless of + // how the host resolves the basename/remote-inferred name. + const r1 = runCliWithEnv( + ['analyze', '--name', 'alias-a'], + repoA, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r1.status === null) return; + expect( + r1.status, + [`analyze exited with ${r1.status}`, `stdout: ${r1.stdout}`, `stderr: ${r1.stderr}`].join( + '\n', + ), + ).toBe(0); + + const registryPath = path.join(gnHome, 'registry.json'); + const afterIndex = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterIndex).toHaveLength(1); + expect(afterIndex[0].name).toBe('alias-a'); + // Storage dir must exist before remove so we can assert its + // disappearance below. + const storagePath = afterIndex[0].storagePath; + expect(fs.existsSync(storagePath)).toBe(true); + + // Dry-run: must NOT delete. Use parentA as cwd so the test + // never runs with the to-be-removed storage dir as its cwd. + // + // Assert the FULL dry-run output shape, not just the `--force` + // hint (#1003 senior-reviewer NIT): `remove.ts` prints the + // alias, the resolved path, AND the storage path. Verifying + // all three appear catches silent format regressions + // (e.g. a future refactor that accidentally drops one of the + // three `console.log` lines, or swaps `entry.path` for + // `entry.name` in the output). + const r2 = runCliWithEnv(['remove', 'alias-a'], parentA, { GITNEXUS_HOME: gnHome }, 15000); + if (r2.status === null) return; + expect(r2.status).toBe(0); + const r2Output = `${r2.stdout}${r2.stderr}`; + expect(r2Output).toMatch(/Run with --force/i); + expect(r2Output, 'dry-run must surface the alias').toContain('alias-a'); + expect(r2Output, 'dry-run must surface the repo path').toContain(afterIndex[0].path); + expect(r2Output, 'dry-run must surface the storage path').toContain(storagePath); + expect(fs.existsSync(storagePath)).toBe(true); + // Registry still has the entry. + expect(JSON.parse(fs.readFileSync(registryPath, 'utf-8'))).toHaveLength(1); + + // --force: must delete storage AND unregister. + const r3 = runCliWithEnv( + ['remove', 'alias-a', '--force'], + parentA, + { GITNEXUS_HOME: gnHome }, + 15000, + ); + if (r3.status === null) return; + expect( + r3.status, + [ + `remove --force exited with ${r3.status}`, + `stdout: ${r3.stdout}`, + `stderr: ${r3.stderr}`, + ].join('\n'), + ).toBe(0); + // Success-case output shape: `Removed: ` header plus the + // same path-and-storagePath lines the dry-run prints (same NIT + // rationale — the success branch mirrors the dry-run's three + // console.log calls, so it has the same silent-regression risk). + const r3Output = `${r3.stdout}${r3.stderr}`; + expect(r3Output).toMatch(/Removed/i); + expect(r3Output, 'success output must surface the alias').toContain('alias-a'); + expect(r3Output, 'success output must surface the repo path').toContain(afterIndex[0].path); + expect(r3Output, 'success output must surface the storage path').toContain(storagePath); + expect(fs.existsSync(storagePath)).toBe(false); + expect(JSON.parse(fs.readFileSync(registryPath, 'utf-8'))).toHaveLength(0); + + // Idempotent: removing the same alias AGAIN must exit 0 with a + // warning (so `remove X && analyze Y` keeps working in scripts). + const r4 = runCliWithEnv(['remove', 'alias-a'], parentA, { GITNEXUS_HOME: gnHome }, 15000); + if (r4.status === null) return; + expect(r4.status).toBe(0); + expect(`${r4.stdout}${r4.stderr}`).toMatch(/Nothing to remove/i); + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parentA, { recursive: true, force: true }); + } + }, 180000); // 3-min outer budget (1 × ~60s analyze + 3 × fast remove calls) + + it('ambiguous target (two entries share alias via --allow-duplicate-name) errors without mutating registry', () => { + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-rm-amb-')); + const repoA = makeMiniRepoCopy('dup', 'gn-dup-a-'); + const repoB = makeMiniRepoCopy('dup', 'gn-dup-b-'); + const parentA = path.dirname(repoA); + const parentB = path.dirname(repoB); + + try { + // Two repos registered under the same alias — only possible via + // --allow-duplicate-name (#829). + const r1 = runCliWithEnv( + ['analyze', '--name', 'shared'], + repoA, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r1.status === null) return; + expect(r1.status).toBe(0); + + const r2 = runCliWithEnv( + ['analyze', '--name', 'shared', '--allow-duplicate-name'], + repoB, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r2.status === null) return; + expect(r2.status).toBe(0); + + const registryPath = path.join(gnHome, 'registry.json'); + const before = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(before).toHaveLength(2); + + // `remove shared` must refuse to guess — exit 1, disambiguation hint. + const r3 = runCliWithEnv( + ['remove', 'shared', '--force'], + parentA, + { GITNEXUS_HOME: gnHome }, + 15000, + ); + if (r3.status === null) return; + expect(r3.status).toBe(1); + const r3Output = `${r3.stdout}${r3.stderr}`; + expect(r3Output).toMatch(/Multiple registered repos match/i); + // Both paths must be surfaced in the hint so the user knows + // which ones to disambiguate between. + expect(r3Output).toMatch(/dup/); + + // Registry unchanged — the failed resolution must NOT have + // mutated state. + const after = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(after).toHaveLength(2); + + // And path-based remove still works: pass the absolute path of + // repoA and it resolves unambiguously. + // + // We pull the path from the registry snapshot rather than + // passing the outer `repoA` variable directly. This is the + // belt-and-suspenders for cross-platform path normalisation + // (#1003 review): the path the registry recorded has already + // gone through the analyze-side canonicalisation (which on + // macOS expands /var → /private/var and on Windows expands 8.3 + // → long-name). Passing that exact string back to `remove` + // guarantees the comparison succeeds even on runners where the + // outer `repoA` is the symlink/short-name form. The code-side + // fix in `canonicalizePath` makes this redundant in practice, + // but the test shouldn't depend on the code fix being perfect + // on every platform — it should prove correctness against the + // registry contract. + const repoAEntry = before.find( + (e: { path: string }) => + path.basename(e.path) === 'dup' && e.path.includes(path.basename(parentA)), + ); + expect( + repoAEntry, + 'repoA entry must exist in registry before path-remove step', + ).toBeDefined(); + + const r4 = runCliWithEnv( + ['remove', repoAEntry.path, '--force'], + parentA, + { GITNEXUS_HOME: gnHome }, + 15000, + ); + if (r4.status === null) return; + expect( + r4.status, + [ + `remove-by-path exited with ${r4.status}`, + `stdout: ${r4.stdout}`, + `stderr: ${r4.stderr}`, + ].join('\n'), + ).toBe(0); + const finalEntries = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(finalEntries).toHaveLength(1); + // The survivor is repoB (its path stays in the registry). + expect(path.basename(finalEntries[0].path)).toBe('dup'); + // And it's NOT the one we just removed. + expect(finalEntries[0].path).not.toBe(repoAEntry.path); + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parentA, { recursive: true, force: true }); + fs.rmSync(parentB, { recursive: true, force: true }); + } + }, 240000); // 4-min outer budget (2 × ~60s analyze + 2 × fast remove) + + it('refuses to proceed when a registry entry points storagePath outside /.gitnexus (#1003)', () => { + // Regression guard for the safety gap flagged by @magyargergo on + // PR #1003: `~/.gitnexus/registry.json` is a user-writable JSON + // file, so a corrupted or hand-edited entry could point + // storagePath at the repo root (catastrophic: rm the working + // tree) or at any other arbitrary path. `remove --force` must + // refuse to call fs.rm when storagePath isn't the canonical + // `/.gitnexus`. We verify: + // 1. Exit code 1 with the actionable "registry entry corrupted" + // hint. + // 2. The .gitnexus/ storage dir is UNTOUCHED. + // 3. The repo itself (entry.path) is UNTOUCHED. + // 4. The registry entry is NOT removed (no partial mutation). + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-poison-')); + const repo = makeMiniRepoCopy('poisoned', 'gn-poison-'); + const parent = path.dirname(repo); + + try { + // Index the repo normally first so the registry has a valid + // entry we can then poison. + const r1 = runCliWithEnv( + ['analyze', '--name', 'poisoned-alias'], + repo, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r1.status === null) return; + expect(r1.status).toBe(0); + + const registryPath = path.join(gnHome, 'registry.json'); + const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(original).toHaveLength(1); + + // Poison the entry: set storagePath to the REPO ROOT itself. + // If the guard isn't in place, `remove --force` would call + // `fs.rm(repo, {recursive: true, force: true})` and wipe the + // entire working tree. + const poisoned = [{ ...original[0], storagePath: repo }]; + fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2)); + + // Sanity: storage dir and working tree both still exist. + expect(fs.existsSync(path.join(repo, '.gitnexus'))).toBe(true); + expect(fs.existsSync(repo)).toBe(true); + expect(fs.existsSync(path.join(repo, '.git'))).toBe(true); + + // Attempt the remove — must FAIL without deleting anything. + const r2 = runCliWithEnv( + ['remove', 'poisoned-alias', '--force'], + parent, + { GITNEXUS_HOME: gnHome }, + 15000, + ); + if (r2.status === null) return; + + expect( + r2.status, + [`remove should have exited 1`, `stdout: ${r2.stdout}`, `stderr: ${r2.stderr}`].join( + '\n', + ), + ).toBe(1); + const r2Output = `${r2.stdout}${r2.stderr}`; + // Must surface the actionable "registry corrupted" hint, not + // just a raw fs.rm error. + expect(r2Output).toMatch(/Refusing to remove/i); + expect(r2Output).toMatch(/registry\.json/i); + + // Repo + .gitnexus dir + .git dir must all still exist — the + // guard aborts BEFORE fs.rm. This is the whole point of the + // test: the working tree is not allowed to disappear. + expect(fs.existsSync(repo), 'repo working tree must survive').toBe(true); + expect(fs.existsSync(path.join(repo, '.gitnexus')), 'storage dir must survive').toBe(true); + expect(fs.existsSync(path.join(repo, '.git')), '.git must survive').toBe(true); + + // Registry unchanged — no partial mutation. + const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterRegistry).toHaveLength(1); + expect(afterRegistry[0].storagePath).toBe(repo); // still poisoned (we did that) + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parent, { recursive: true, force: true }); + } + }, 120000); // 2-min budget (1 × ~60s analyze + 1 × fast remove-refused) + }); + + // ─── clean --all: same safety guard applies (#1003 review) ─────── + // + // The `clean --all` path iterates over the registry and calls + // `fs.rm(entry.storagePath)` — identical trust-the-registry pattern + // as `remove` had before the guard. A poisoned entry must be SKIPPED + // (not aborted), so clean --all preserves its existing per-repo + // error-tolerance semantics: one bad entry does not halt cleanup of + // the rest. We verify: + // 1. The poisoned entry is NOT deleted (working tree + .gitnexus + // survive), and the CLI prints a "Refusing to clean" message. + // 2. The poisoned entry is left in the registry (nothing was + // mutated for it). + // 3. A co-existing well-formed entry IS still cleaned (both its + // .gitnexus dir AND its registry entry are gone). + describe('clean --all with a poisoned registry entry (#1003)', () => { + it('skips poisoned entries, cleans valid ones, never deletes the working tree', () => { + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-clean-poison-')); + const repoBad = makeMiniRepoCopy('bad-repo', 'gn-clean-bad-'); + const repoGood = makeMiniRepoCopy('good-repo', 'gn-clean-good-'); + const parentBad = path.dirname(repoBad); + const parentGood = path.dirname(repoGood); + + try { + // Analyze both so the registry has two well-formed entries. + for (const [repo, alias] of [ + [repoBad, 'bad-alias'], + [repoGood, 'good-alias'], + ] as const) { + const r = runCliWithEnv( + ['analyze', '--name', alias], + repo, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r.status === null) return; + expect(r.status, `analyze ${alias} exited ${r.status}: ${r.stdout}${r.stderr}`).toBe(0); + } + + const registryPath = path.join(gnHome, 'registry.json'); + const original = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(original).toHaveLength(2); + + // Poison the 'bad-alias' entry by pointing its storagePath at + // the repo root itself. If the guard isn't wired into the + // clean --all loop, `clean --all --force` would fs.rm the + // working tree. + const poisoned = original.map((e: { name: string; storagePath: string; path: string }) => + e.name === 'bad-alias' ? { ...e, storagePath: repoBad } : e, + ); + fs.writeFileSync(registryPath, JSON.stringify(poisoned, null, 2)); + + // Sanity: both working trees and .gitnexus dirs still exist. + expect(fs.existsSync(repoBad)).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.gitnexus'))).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.git'))).toBe(true); + expect(fs.existsSync(path.join(repoGood, '.gitnexus'))).toBe(true); + + // clean --all --force from a neutral cwd (parentBad), so the + // command isn't "inside" either repo. + const r = runCliWithEnv( + ['clean', '--all', '--force'], + parentBad, + { GITNEXUS_HOME: gnHome }, + 30000, + ); + if (r.status === null) return; + + // clean --all's per-entry error handling always exits 0 at + // the end (it only logs per-repo failures). The important + // assertions are on side effects, not the exit code. + const output = `${r.stdout}${r.stderr}`; + expect(output).toMatch(/Refusing to clean/i); + expect(output).toMatch(/bad-alias/); + + // Poisoned repo: working tree + .gitnexus + .git all SURVIVE. + expect(fs.existsSync(repoBad), 'poisoned repo working tree must survive').toBe(true); + expect( + fs.existsSync(path.join(repoBad, '.gitnexus')), + 'poisoned repo .gitnexus must survive (guard refused to rm repo root)', + ).toBe(true); + expect(fs.existsSync(path.join(repoBad, '.git')), '.git must survive').toBe(true); + + // Good repo: its .gitnexus IS gone (cleanup succeeded despite + // the poisoned sibling entry — per-entry error tolerance is + // preserved). + expect( + fs.existsSync(path.join(repoGood, '.gitnexus')), + 'good repo .gitnexus should be cleaned', + ).toBe(false); + // But the good repo's working tree stays (clean never touches + // anything outside .gitnexus). + expect(fs.existsSync(repoGood), 'good repo working tree must survive').toBe(true); + + // Registry post-state: poisoned entry still present (skipped, + // not mutated); good entry unregistered. + const afterRegistry = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterRegistry).toHaveLength(1); + expect(afterRegistry[0].name).toBe('bad-alias'); + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parentBad, { recursive: true, force: true }); + fs.rmSync(parentGood, { recursive: true, force: true }); + } + }, 240000); // 4-min budget (2 × ~60s analyze + 1 × fast clean --all) + }); + describe('unhappy path', () => { it('exits with error when no command is given', () => { const result = runCliRaw([], MINI_REPO); diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 83827fc15..12c56d67a 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -15,7 +15,14 @@ import { loadCLIConfig, registerRepo, listRegisteredRepos, + resolveRegistryEntry, + canonicalizePath, + assertSafeStoragePath, RegistryNameCollisionError, + RegistryNotFoundError, + RegistryAmbiguousTargetError, + UnsafeStoragePathError, + type RegistryEntry, type RepoMeta, } from '../../src/storage/repo-manager.js'; import { parseRepoNameFromUrl, getInferredRepoName } from '../../src/storage/git.js'; @@ -453,3 +460,334 @@ describe('getInferredRepoName + registerRepo (#979 — git remote inference)', ( } }); }); + +// ─── resolveRegistryEntry (#664 — gitnexus remove ) ────────── +// +// The resolver is a pure function over a `RegistryEntry[]` snapshot, so +// these tests build synthetic entries inline and do NOT touch +// ~/.gitnexus. No GITNEXUS_HOME sandboxing needed. This also means the +// tests are platform-portable on Windows where realpath semantics on +// tmpdirs can diverge between runs (see the #955 CI pivot). + +describe('resolveRegistryEntry (#664)', () => { + // A well-known synthetic registry with two same-name entries (which + // can only exist in reality after `--allow-duplicate-name` — #829) and + // one unique-name entry. Path prefixes differ across platforms so the + // tests stay meaningful regardless of `process.platform`. + const prefix = process.platform === 'win32' ? 'D:\\' : '/tmp/'; + const pathA = `${prefix}projects${path.sep}gnx-a${path.sep}app`; + const pathB = `${prefix}projects${path.sep}gnx-b${path.sep}app`; + const pathW = `${prefix}work${path.sep}website`; + + const entries: RegistryEntry[] = [ + { + name: 'app', + path: pathA, + storagePath: `${pathA}${path.sep}.gitnexus`, + indexedAt: '2026-04-18T00:00:00.000Z', + lastCommit: 'aaaaaaa', + }, + { + name: 'app', + path: pathB, + storagePath: `${pathB}${path.sep}.gitnexus`, + indexedAt: '2026-04-18T00:00:00.000Z', + lastCommit: 'bbbbbbb', + }, + { + name: 'website', + path: pathW, + storagePath: `${pathW}${path.sep}.gitnexus`, + indexedAt: '2026-04-18T00:00:00.000Z', + lastCommit: 'ccccccc', + }, + ]; + + it('resolves by absolute path to the exact entry (path tier beats name tier)', () => { + const hit = resolveRegistryEntry(entries, pathA); + expect(hit).toBe(entries[0]); + expect(hit.path).toBe(pathA); + + const hit2 = resolveRegistryEntry(entries, pathB); + expect(hit2).toBe(entries[1]); + expect(hit2.path).toBe(pathB); + }); + + it('resolves by unique name to the only matching entry', () => { + const hit = resolveRegistryEntry(entries, 'website'); + expect(hit).toBe(entries[2]); + expect(hit.name).toBe('website'); + }); + + it('name match is case-insensitive', () => { + expect(resolveRegistryEntry(entries, 'WEBSITE')).toBe(entries[2]); + expect(resolveRegistryEntry(entries, 'Website')).toBe(entries[2]); + }); + + it('path match is case-insensitive on Windows only', () => { + if (process.platform !== 'win32') { + // On POSIX, a differently-cased path must NOT match. Verify by + // lower-casing a mixed-case copy of pathW and expecting a miss. + const upper = pathW.toUpperCase(); + expect(() => resolveRegistryEntry(entries, upper)).toThrow(RegistryNotFoundError); + return; + } + const upper = pathA.toUpperCase(); + const hit = resolveRegistryEntry(entries, upper); + expect(hit).toBe(entries[0]); + }); + + it('throws RegistryAmbiguousTargetError when name matches multiple entries', () => { + // Two 'app' entries exist only because of --allow-duplicate-name + // (#829). The resolver MUST refuse to guess. + expect(() => resolveRegistryEntry(entries, 'app')).toThrow(RegistryAmbiguousTargetError); + try { + resolveRegistryEntry(entries, 'app'); + } catch (e) { + expect(e).toBeInstanceOf(RegistryAmbiguousTargetError); + const err = e as RegistryAmbiguousTargetError; + expect(err.kind).toBe('RegistryAmbiguousTargetError'); + expect(err.target).toBe('app'); + expect(err.matches).toHaveLength(2); + // Error message must include both paths so the CLI can surface + // them without string-matching on `.message`. + expect(err.message).toContain(pathA); + expect(err.message).toContain(pathB); + } + }); + + it('throws RegistryNotFoundError when no entry matches', () => { + expect(() => resolveRegistryEntry(entries, 'nonexistent')).toThrow(RegistryNotFoundError); + try { + resolveRegistryEntry(entries, 'nonexistent'); + } catch (e) { + expect(e).toBeInstanceOf(RegistryNotFoundError); + const err = e as RegistryNotFoundError; + expect(err.kind).toBe('RegistryNotFoundError'); + expect(err.target).toBe('nonexistent'); + // availableNames is disambiguated: 'app' appears twice, so both + // `app (path)` variants are included; 'website' is unique so it + // stays plain — matches the resolveRepo disambiguation shape. + expect(err.availableNames).toContain('website'); + expect(err.availableNames.some((n) => n.startsWith('app ('))).toBe(true); + // Error message surfaces the hint. + expect(err.message).toContain('website'); + } + }); + + it('throws RegistryNotFoundError with "no repositories registered" hint when registry is empty', () => { + try { + resolveRegistryEntry([], 'anything'); + } catch (e) { + expect(e).toBeInstanceOf(RegistryNotFoundError); + const err = e as RegistryNotFoundError; + expect(err.availableNames).toEqual([]); + expect(err.message).toContain('No repositories are currently registered'); + } + }); + + it('path match wins over name match (never ambiguous)', () => { + // Construct a pathological fixture where a registry entry's NAME + // happens to equal another entry's PATH. The path tier must win + // without triggering ambiguity. + const weird: RegistryEntry[] = [ + { ...entries[2] }, // 'website' at pathW + { + name: pathW, // degenerate: name equals another entry's path + path: `${prefix}elsewhere${path.sep}odd`, + storagePath: `${prefix}elsewhere${path.sep}odd${path.sep}.gitnexus`, + indexedAt: '2026-04-18T00:00:00.000Z', + lastCommit: 'ddddddd', + }, + ]; + const hit = resolveRegistryEntry(weird, pathW); + // Must match the entry whose PATH is pathW, not the one whose NAME + // is pathW — because Tier 1 runs before Tier 2 and finds the path + // match first. + expect(hit.path).toBe(pathW); + expect(hit.name).toBe('website'); + }); +}); + +// ─── canonicalizePath (#1003 review — @evander-wang / @magyargergo) ── +// +// Shields `registerRepo`, `unregisterRepo`, and `resolveRegistryEntry` +// against cross-platform path-form divergence: macOS symlink expansion +// (/var → /private/var) and Windows 8.3 short-name expansion +// (RUNNERA~1 → runneradmin). The helper also underpins backwards +// compatibility with registries written by versions that only ran +// `path.resolve` — by canonicalising the stored entry at compare time, +// both pre- and post-fix entries converge to the same key. +// +// These tests avoid snapshotting a specific realpath value (that would +// be platform-fragile); instead they assert: +// - canonicalizePath is idempotent (f(f(x)) == f(x)) +// - canonicalizePath falls back cleanly when the path doesn't exist +// - resolveRegistryEntry matches a stored entry even when the target +// and the stored value disagree on one-step normalisation (simulated +// via a fixture that stores the de-canonicalised form of a real +// existing path). + +describe('canonicalizePath (#1003)', () => { + it('is idempotent — canonicalizePath(canonicalizePath(x)) === canonicalizePath(x)', async () => { + // Use the vitest project-root as a known-existing path. `os.tmpdir()` + // would work too but process.cwd() is guaranteed to exist for the + // test runner. + const p = process.cwd(); + const once = canonicalizePath(p); + const twice = canonicalizePath(once); + expect(twice).toBe(once); + }); + + it('falls back to path.resolve when the target does not exist', () => { + // Construct a definitely-nonexistent path under tmpdir. Using + // random-ish segments so we don't collide with anything real. + const ghost = path.join(os.tmpdir(), 'gnx-never-exists-____', 'still-not-there'); + const got = canonicalizePath(ghost); + // Must not throw, must not resolve to something weird — should be + // identical to `path.resolve(ghost)` since realpathSync.native will + // have thrown and we swallowed it. + expect(got).toBe(path.resolve(ghost)); + }); + + it('returns an absolute path for relative input even when the path is missing', () => { + // Relative path that does not exist. Must still be absolute + // (fallback path: path.resolve normalises even non-existent inputs). + const rel = './does-not-exist-zzz-' + Date.now(); + const got = canonicalizePath(rel); + expect(path.isAbsolute(got)).toBe(true); + }); +}); + +describe('resolveRegistryEntry backward-compat with non-canonical stored paths (#1003)', () => { + it('matches a stored entry even when the target was passed in canonical form', async () => { + // Simulate the bug-producing scenario without depending on a real + // symlink/8.3 discrepancy (those are platform-specific and flaky to + // set up in CI). We take a REAL path that exists + // (canonicalizePath-stable), store a known-non-canonical copy of it + // in a fake RegistryEntry, then resolve with the canonical form and + // assert the match. + // + // Construct a non-canonical string that resolves to the same real + // path. `path.join` auto-normalises `.` and trailing separators, so + // we build the string by raw concat to keep it string-unequal to + // `realDir` until `canonicalizePath` runs. + const realDir = process.cwd(); + const nonCanonical = realDir + path.sep + '.'; // e.g. /work/gitnexus/. + // Sanity: these are string-unequal before canonicalisation. + expect(nonCanonical).not.toBe(realDir); + + const entries: RegistryEntry[] = [ + { + name: 'stored-under-noncanonical-form', + path: nonCanonical, + storagePath: path.join(nonCanonical, '.gitnexus'), + indexedAt: '2026-04-20T00:00:00.000Z', + lastCommit: 'deadbee', + }, + ]; + + // Pass the canonical form as the target — resolver must still match. + const hit = resolveRegistryEntry(entries, realDir); + expect(hit).toBe(entries[0]); + }); +}); + +// ─── assertSafeStoragePath (#1003 review — @magyargergo) ───────────── +// +// Guard rail against destroying more than the `.gitnexus/` subfolder. +// `~/.gitnexus/registry.json` is user-writable plain text, so a +// corrupted or hand-edited entry could put storagePath anywhere. +// These tests use synthetic `RegistryEntry` fixtures (no disk I/O) +// because the guard is a pure string check — it must not depend on +// the paths existing. + +describe('assertSafeStoragePath (#1003)', () => { + const prefix = process.platform === 'win32' ? 'D:\\' : '/tmp/'; + const repoPath = `${prefix}projects${path.sep}my-repo`; + const base: Omit = { + name: 'my-repo', + path: repoPath, + indexedAt: '2026-04-21T00:00:00.000Z', + lastCommit: 'deadbee', + }; + + it('accepts the canonical /.gitnexus storage path', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.join(repoPath, '.gitnexus'), + }; + expect(() => assertSafeStoragePath(entry)).not.toThrow(); + }); + + it('rejects when storagePath equals the repo path itself (would delete the code)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: repoPath, // catastrophic: rm the working tree + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is a parent of the repo path', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.dirname(repoPath), // also catastrophic + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is empty (path.resolve falls back to cwd)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: '', // path.resolve('') === process.cwd() — would rm cwd + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath points somewhere totally unrelated', () => { + const entry: RegistryEntry = { + ...base, + storagePath: `${prefix}some${path.sep}other${path.sep}place`, + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('rejects when storagePath is a sibling .gitnexus (right basename, wrong parent)', () => { + const entry: RegistryEntry = { + ...base, + storagePath: path.join(`${prefix}different${path.sep}repo`, '.gitnexus'), + }; + expect(() => assertSafeStoragePath(entry)).toThrow(UnsafeStoragePathError); + }); + + it('UnsafeStoragePathError carries the original entry + expected + actual paths', () => { + const entry: RegistryEntry = { + ...base, + storagePath: `${prefix}evil${path.sep}path`, + }; + try { + assertSafeStoragePath(entry); + } catch (e) { + expect(e).toBeInstanceOf(UnsafeStoragePathError); + const err = e as UnsafeStoragePathError; + expect(err.kind).toBe('UnsafeStoragePathError'); + expect(err.entry).toBe(entry); + // Expected path is the canonical `/.gitnexus`. + expect(err.expectedStoragePath).toBe(path.join(path.resolve(repoPath), '.gitnexus')); + // Actual path is the corrupted value (resolved). + expect(err.actualStoragePath).toBe(path.resolve(entry.storagePath)); + // Message must suggest the recovery action. + expect(err.message).toContain('registry.json'); + } + }); + + it('Windows: storagePath match is case-insensitive to match register/unregister semantics', () => { + if (process.platform !== 'win32') return; + const entry: RegistryEntry = { + ...base, + storagePath: path.join(repoPath.toUpperCase(), '.GITNEXUS'), + }; + // Should accept because Windows paths are case-insensitive. + expect(() => assertSafeStoragePath(entry)).not.toThrow(); + }); +}); From ff4ae89aaaccf5ed8f5e06322b20a8ed499a49f6 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Apr 2026 15:50:00 +0100 Subject: [PATCH 08/28] feat(python): scope-based call resolution + registry-primary flip + perf + generalization (RFC #909 Ring 3) (#980) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Initial plan * plan: Python scope-based resolution migration Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0eee6c69-fc17-4df5-9ac6-358ab41f5740 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * feat(python): scope-based resolution provider hooks + 62 tests Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0eee6c69-fc17-4df5-9ac6-358ab41f5740 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * refactor(python): split scope-hooks monolith into focused modules Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/db76e937-4b0e-4c4d-82b1-265a1fb3673d Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * test(python): integration-style scope-resolution tests + suffixResolve fallback Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/db76e937-4b0e-4c4d-82b1-265a1fb3673d Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * wire python scope-based resolution end-to-end (initial pass) Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c474dc66-5cf7-445d-8eb4-76501c5e6d67 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * keep legacy IMPORTS for python (heritage needs importMap), scope phase owns CALLS only Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/c474dc66-5cf7-445d-8eb4-76501c5e6d67 Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com> * test(python): remove parallel scope-resolution integration test The new test/integration/python-scope-resolution.test.ts duplicated coverage the reviewer explicitly rejected. The existing test/integration/resolvers/python.test.ts (191 tests, driven by runPipelineFromRepo) is the source of truth for Ring 3 parity. Also document the IMPORTS-emission follow-up gap: wiring emitImportEdges in python-scope-emit.ts today regresses 10 IMPORTS-edge fixtures because the scope-extractor's ImportEdge coverage is narrower than legacy pythonImportConfig.importResolver. Tracked as a follow-up. Baseline with REGISTRY_PRIMARY_PYTHON=1 is unchanged: 109/191 pass. * feat(ingestion): scope-resolution phase owns Python IMPORTS edges (RFC #909 Ring 3) When `REGISTRY_PRIMARY_PYTHON=1`, IMPORTS graph edges for Python files are now emitted exclusively by the new scope-resolution path. The legacy `import-processor` still runs — heritage resolution needs its importMap / namedImportMap / moduleAliasMap population — but its graph edge emission is gated per-language so Python no longer double-emits. This closes the reviewer's second change request on PR #980: "the legacy path must be turned off". Legacy IMPORTS edges for Python are now off by default when the flag is enabled. Three bugs were fixed to make the new path's coverage match legacy: 1. **Root-file bailout** (import-resolvers/python.ts): `resolvePythonImportInternal` returned null immediately when the importer file lived at the repo root (importerDir === ''). The ancestor directory walk further down already handles this case correctly; the early return was the bug. Proximity check now only runs when importerDir is non-empty, and the ancestor walk sees root-level files for the first time. 2. **External dotted imports** (languages/python/import-target.ts): the new path fell straight through to `suffixResolve` for multi-segment imports, which happily matched `django.apps` to a local `accounts/apps.py`. Mirror `pythonImportStrategy`'s `hasRepoCandidate` guard — suffix-match only when the leading segment exists somewhere in-repo as a package, __init__.py, or namespace directory. 3. **suffixResolve ambiguity** (languages/python/import-target.ts): the shared `suffixResolve` helper requires a pre-built `SuffixIndex` to disambiguate ties. Without one it falls back to an O(files) scan that silently picks the first match when the last segment collides across directories (e.g. `accounts.models` matching `billing/models.py`). Replaced with `resolveAbsoluteFromFiles` — exact lookup first, then a deterministic suffix match. Validation: - Flag OFF: 191/191 pass (no regression). - Flag ON: 109/191 pass (82 fail — exact baseline match; remaining 82 are unchanged CALLS-edge provider-feature gaps tracked as Phase B follow-ups). - `tsc --noEmit`: clean. The 82 CALLS failures cluster into 44 describe blocks covering type-inference features (assignment chains, walrus, class-level annotations, constructor inference, C3 MRO, overload dispatch, return-type inference) that need dedicated Ring 3 follow-up work. Each cluster is tracked against the RFC #909 shadow-parity gate (>=99% fixtures / >=98% corpus) in the per-language ticket. * ci(scope-resolution): automatic parity gate driven by MIGRATED_LANGUAGES Adds the Ring 3 parity gate the RFC §6.4 requires: when a language's scope-resolution migration is marked complete, CI runs its resolver integration test twice on every PR (once with the legacy DAG, once with the registry-primary path) and both must pass. The "is this language migrated" signal is a single TypeScript constant: // gitnexus/src/core/ingestion/registry-primary-flag.ts export const MIGRATED_LANGUAGES: ReadonlySet = new Set([ /* SupportedLanguages.Python when ready */ ]); Adding a language here has three simultaneous effects: 1. `isRegistryPrimary(lang)` defaults to true for that language in production (env-var override still wins if set explicitly). 2. `.github/workflows/ci-scope-parity.yml` auto-discovers the set via `npx tsx scripts/ci-list-migrated-languages.ts`, builds a parity matrix, and runs: - `REGISTRY_PRIMARY_=0 npx vitest run resolvers/.test.ts` - `REGISTRY_PRIMARY_=1 npx vitest run resolvers/.test.ts` Both legs must pass for the job to succeed. 3. Legacy-path gating in call-processor.ts / import-processor.ts kicks in automatically through the same `isRegistryPrimary` lookup. No JSON registry, no manual workflow edit, no second source of truth — contributors update the Set and CI picks it up. Empty Set = parity job is a skipped matrix (workflow still reports success). The new `scope-parity` reusable workflow is added to ci.yml's `needs` graph and ci-status gate. Its result must be `success` (skipped would mean upstream discover job failed and should block). Validation (with empty MIGRATED_LANGUAGES set): - flag OFF: 191/191 pass (no behavior change) - flag ON (manual REGISTRY_PRIMARY_PYTHON=1): 82 fails = baseline exact match - `npx tsc --noEmit`: clean - concurrency-convention script: pass - tsx discovery script: emits `[]` correctly * ci(scope-resolution): keep MIGRATED_LANGUAGES empty; fix linter auto-uncomment Previous commit's example entry got auto-uncommented (linter preferred a type-checkable `SupportedLanguages.Python` over a commented-out reference). That would have triggered the parity CI gate against Python, which today has 82 known flag-on failures — unintended and would block the PR. Use the explicit generic `new Set([])` so an empty set still type-checks without needing an uncommented-out sample member. Example in the comment now has `// SupportedLanguages.Python,` so it remains illustrative without participating in the set. * feat(python): capture constructor-inferred + annotated type bindings Extends the Python scope-extractor with two new type-binding capture patterns so receiver-typed method dispatch has concrete type bindings to work from: 1. `u: User = ...` / `u: User` — variable annotations. `@type-binding.annotation` anchor, `source: 'annotation'`. 2. `u = User("alice")` — assignment RHS is a bare-identifier call (Python has no `new` keyword; constructor-shaped calls are syntactically identical to function calls). `@type-binding.constructor` anchor, `source: 'constructor-inferred'`. The runtime query lives in `query.ts` (the `.scm` file is documentation per the comment at its top); both are updated. Fixes 19 failures across these resolver fixtures (flag-on 82 → 63): - Python constructor-inferred type resolution (3) - Python class-level annotation resolution (3) - Python nullable receiver resolution (3) - Python member-call / receiver-constrained / constructor-call (3) - Python assignment chain propagation (2) - Python walrus / match-case / chained method (3) - Python member access iterable for-loop (2) * feat(python): strip nullable unions + prefer annotations over inference Two linked changes that together fix the 4 nullable-receiver tests: 1. `stripNullable` in Python's `interpretTypeBinding` unwraps `User | None`, `None | User`, and `Optional[User]` to `User`, so receiver-typed resolution treats nullable receivers identically to non-nullable ones. Three-arm unions (`User | Error | None`) are left unchanged — truly ambiguous for single-receiver inference. 2. Source-strength ordering in `pass4CollectTypeBindings`. When multiple matches fire for the same bound name in the same scope — e.g. the `u: User = find()` idiom where both the annotation and constructor-inferred patterns match — the explicit annotation now wins regardless of query-match arrival order. Rank: explicit (annotation / parameter-annotation / return-annotation / self) > inferred Also reorders the two Python patterns in query.ts / scopes.scm so the constructor-inferred pattern appears first — a belt-and-braces fallback that keeps behavior deterministic if the shared priority ranking is ever revisited. Fixes 4 failures (flag-on 63 → 59): - Python nullable receiver resolution (4 tests) Flag-off regression check: 191/191 still pass. * feat(python): walrus, qualified-call, match-case type bindings Extends the constructor-inferred family of captures with three more assignment-shaped patterns that all bind a variable to a class-like type: - Walrus: `(u := User(...))` → `u: User` via `(named_expression)`. - Qualified call RHS: `u = models.User(...)` → `u: models.User` via `(attribute)` node .text. Falls through resolveTypeRef Phase 2 (QualifiedNameIndex dotted fallback). - Match as-pattern: `case User() as u:` → `u: User` via `(as_pattern)` + `(class_pattern (dotted_name))`. Fixes 2 failures (flag-on 59 → 57): - Python walrus operator type inference - Python match/case as-pattern type binding Qualified-call constructor tests still fail because they require cross-module qualifiedName registration (models.User → models.py's User class) which isn't yet wired in the Python extractor. Tracked as follow-up alongside module-import CALLS (#337) resolution. * feat(python): chain type bindings + strip list[T] generic for for-loop Adds two capture patterns and a shared transitive-closure pass that together handle Python's variable-aliasing and for-loop-over-typed- iterable patterns: 1. `(assignment left: (identifier) right: (identifier))` — `alias = u`. 2. `(for_statement left: (identifier) right: (identifier))` — `for u in users`. Both emit `@type-binding.alias` with the RHS identifier as rawName. The shared `pass4CollectTypeBindings` now runs a final transitive-closure walk that follows identifier-chain TypeRefs through the declaring scope and its ancestors (depth-capped, cycle-guarded) so `alias` ultimately points at the class type instead of another local variable name. Generic stripping in `interpret.ts` unwraps single-arg collection wrappers — `list[User]`, `set[User]`, `Iterable[User]`, etc. — to the element type. Multi-arg generics (`dict[str, User]`, `Callable[...]`) are left alone; their semantics aren't unambiguous. Fixes 8 failures (flag-on 57 → 49): - Python assignment chain propagation (4) - Python nullable + assignment chain (2) - Python walrus operator (:=) assignment chain (2) Flag-off still 191/191. * feat(python): namespace & class receiver resolution + file-level caller fallback Adds a Python-specific post-resolution pass `emitReceiverBoundCalls` that closes two receiver gaps the shared `MethodRegistry.lookup` doesn't cover: 1. **Namespace receivers** — `import models; models.User()` / `import models as m; m.User()`. The shared `lookupReceiverType` only walks `scope.typeBindings`; namespace imports never land there (they're filtered out of `scope.bindings` when the target module has no self-named def, per `finalize-algorithm.ts:540`). The new pass walks `indexes.imports` directly, builds a per-file `localName → targetFilePath` map, and emits CALLS/ACCESSES edges against the target file's `localDefs`. 2. **Class-name receivers** — `Dog.classify("dog")`. The shared resolver requires typeBindings; class bindings in `scope.bindings` are never consulted as receivers. The new pass checks class-kind bindings in the call scope's chain and resolves members via `ownerId`. Also fixes module-level call attribution: `resolveCallerGraphId` now falls back to the File node id (`generateId('File', filePath)`) when no enclosing function/method/class is found. Matches legacy DAG behavior for module-scope calls like `u = models.User()` at the top of app.py. Fixes 4 failures (flag-on 49 → 45): - Python module import CALLS resolution (Issue #337) (4 of 7) Flag-off still 191/191. * feat(python): dotted-typebinding receiver resolution Adds case 3 to `emitReceiverBoundCalls`: when a receiver's typeBinding has a dotted rawName like `u: models.User` (the constructor-inferred form fired by `u = models.User(...)`), walk the namespace map + target file's defs to find the class, then look up the member via ownerId. `resolveTypeRef`'s QualifiedNameIndex fallback can't cover this because the target class's qualifiedName in models.py is just `"User"`, not `"models.User"` — the dotted form only exists in the call-site file's receiver expression. This pass bridges that gap without modifying the shared registry. Fixes 9 more failures (flag-on 45 → 36): - Python qualified constructor inference (2) - Python module import CALLS resolution (Issue #337) (3) - (cluster overlap — several downstream tests in assignment/nullable/ walrus that propagate through qualified-ctor bindings also benefit) Flag-off still 191/191. * feat(python): consult finalized bindings for receiver resolution `findClassBindingInScope` now walks BOTH: 1. `scope.bindings` — pre-finalize local declarations (origin: 'local') 2. `indexes.bindings` — post-finalize cross-file imports/namespaces Without (2) we were blind to any class brought in via `from models import Dog` at the call site's file, because the scope-extractor's Pass 2 only populates local bindings and the cross-file finalize produces a separate bindings map that never lands on `scope.bindings`. Case 2 (`Dog.classify()`) now walks MRO so inherited static/class methods resolve — `Dog.classify()` where `classify` lives on `Animal`. Case 4 (simple typeBinding like `u: U` from aliased import) now uses `findClassBindingInScope` instead of the shared `resolveTypeRef`, because `resolveTypeRef`'s `ctx.scopes` only sees pre-finalize local bindings too. Fixes 4 more failures (flag-on 36 → 32): - Python method enrichment > Dog.classify static (1) - Python static/classmethod class-as-receiver (2) - Python alias import resolution (1) Flag-off still 191/191. * refactor(python-scope): extract language-agnostic emit-core/ Unit 1 of the python migration architectural plan (docs/plans/2026-04-19-001-refactor-python-migration-architectural-plan.md). Splits python-scope-emit.ts (~945 → 481 lines) by lifting 14 generic graph-feeding primitives into emit-core/: - graph-node-lookup, graph-id, emit-edge - emit-references, emit-imports - scope-walkers (findReceiverTypeBinding, findClassBindingInScope, findOwnedMember, findExportedDef) - namespace-targets, method-dispatch-bridge Each file carries a "Next-consumer contract" JSDoc so future language migrations (TS #927, JS #928, Java, Kotlin, Ruby) import from emit-core rather than re-implementing. python-scope-emit.ts keeps only the four Python-specific pieces: runPythonScopeResolution (orchestrator), buildPythonMro, emitReceiverBoundCalls (4 cases), populateMethodOwnerIds — these move to languages/python/emit/ in Unit 11. Pure refactor, zero behavior change: - flag-off: 191/191 python.test.ts pass (identical baseline). - flag-on (REGISTRY_PRIMARY_PYTHON=1): 32 fail / 159 pass (identical baseline — the refactor neither fixes nor regresses any test). - tsc --noEmit clean. * feat(python-scope): arity metadata + bind function decls in parent scope Unit 2 of the python migration architectural plan (docs/plans/2026-04-19-001-refactor-python-migration-architectural-plan.md). Two changes that the registry-primary path needs before any of the arity-sensitive failures can move: 1. Arity metadata on scope-extracted Function/Method defs. - New helper `languages/python/arity-metadata.ts` reuses `pythonMethodConfig.extractParameters` so self/cls stripping, defaults, and *args/**kwargs detection match legacy semantics. - `emit-captures.ts` synthesizes `@declaration.parameter-count` / `@declaration.required-parameter-count` / `@declaration.parameter-types` captures on every `@declaration.function` match. - Generic `scope-extractor.ts buildDefFromDeclarationMatch` reads the three optional captures into `SymbolDefinition`. Absence is still the no-op default for non-Python providers. 2. Hoist function/class declaration bindings to the enclosing scope. The "innermost scope containing the anchor" default placed `def greet(...)` inside greet's OWN body — invisible to other module-level callers, so every flag-on free-call resolved to `unresolved`. The hoist condition (`anchor range == innermost range`) only fires for scope-creating declarations, so variable / for-loop captures whose anchor is a child identifier stay put. Hooks can still override via `bindingScopeFor`. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on (REGISTRY_PRIMARY_PYTHON=1): 31 fail / 160 pass (was 32/159; the hoist unblocks free-call resolution end-to-end). - tsc --noEmit clean. Per-(source,target) edge collapse for multi-call-site cases (default-params, variadic) still pending — landing it without regressing the static-method find_user fixture (which expects two distinct edges through different targets) needs the ownership-aware qualified-id work that lands with Unit 4 / Unit 11. * feat(python-scope): capture function return-type annotations Unit 3 of the python migration architectural plan (docs/plans/2026-04-19-001-refactor-python-migration-architectural-plan.md). Wires the `def get_user() -> User` return-type annotation into the typeBindings stream so the existing constructor-inferred + transitive chain machinery can resolve `u = get_user(); u.save()` to `User#save` without any orchestrator change. Changes: - `query.ts` + `scopes.scm`: new `@type-binding.return` pattern keyed by the function name (matches RFC §5.1 canonical vocabulary). - `interpret.ts`: maps `@type-binding.return` to the existing `'return-annotation'` source label (no shared change needed). - `scope-extractor.ts pass4CollectTypeBindings`: extends the Pass 2 auto-hoist (anchor range == innermost scope range → bind in parent) to type bindings as well — return-type bindings whose anchor IS the function_definition land in the function's enclosing scope so callers see them. Same-file return-type inference is now end-to-end: `def get_user() -> User: ...` + `u = get_user()` produces `u: User (return-annotation)` in the caller's scope via `followChainedRef`. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 31 fail / 160 pass (no change — every remaining return-type test in this fixture set is *cross-file*; carrying `get_user → User` across module boundaries lands with the cross-file typeBinding propagation work in Unit 5/7). - tsc --noEmit clean. * feat(python-scope): resolve dotted receivers via class-scope field types Unit 4 partial — the dotted-receiver case (`user.address.save()`). Class-body annotations like `class User: address: Address` already land in the class scope's typeBindings via the existing `@type-binding.annotation` capture. This commit consumes that signal: - Build a `Map` from every parsed file's class scopes once per resolution pass. - New Case 0 in `emitReceiverBoundCalls`: when the receiver's name contains a dot, walk the chain — resolve the head's type, then for each remaining segment look up that field's type in the owner class's scope.typeBindings, then emit the call against the final class with MRO walk. - Cross-scope lookups use each TypeRef's `declaredAtScope` so an imported `Address` resolves in the file that owns the field declaration, not the file holding the call site. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 29 fail / 162 pass (was 31/160; both `Field type resolution` fixtures now pass — same-file and cross-file disambig). - tsc --noEmit clean. Remaining Unit 4 work (write ACCESSES, `self.X` for-loop iteration) needs Unit 6's tuple/iterable destructuring before it can land — `for u in self.users` requires the iterable typing path. * feat(python-scope): chain receiver via call-expression return types Unit 5 — extends the compound-receiver case to handle call-expression receivers (`svc.get_user().save()`). `resolveCompoundReceiverClass` is the single recursive entry point for all compound receivers. Three shapes: - bare identifier — typeBinding chain - dotted `obj.field[.field]…` — class-scope field types - call `expr.method()` — recurse into expr, look up method's return-type typeBinding on its class scope Method return-type bindings auto-hoist to the parent (class) scope per Unit 3, so `methodClassScope.typeBindings.get(methodName)` is the canonical lookup. Free-call return types (`get_user()`) walk the caller's scope chain. Depth-capped at 4 hops to bound recursion. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 28 fail / 163 pass (was 29/162; `Python chained method call resolution` now passes). - tsc --noEmit clean. Two related tests (`city.save() via method chain`, `c.greet().save() depth-2 MRO`) still fail because the captures yield typeBindings shaped like `city → user.get_city` (no trailing parens — the capture grabs the attribute text). Resolving those needs a follow step that detects the call-shape rawName and feeds it through the compound recurser. Lands with the chain-typeBinding work in a follow-up. * feat(python-scope): free-call fallback consults finalized bindings Unit 7 — closes the cross-file free-call gap. The shared `MethodRegistry.lookup` walks `scope.bindings` (pre-finalize local-only) for free-call resolution. Cross-file imports land in `indexes.bindings` (post-finalize). Without the dual-source lookup, `from x import f; f()` resolves to "unresolved" and no CALLS edge is emitted. Two changes: - `emit-core/scope-walkers.ts`: new `findCallableBindingInScope` — same dual-source pattern as `findClassBindingInScope`, but accepts Function/Method/Constructor. Promoted to emit-core because every language with cross-file imports needs the same lookup. - `python-scope-emit.ts emitFreeCallFallback`: post-pass that walks every free-call reference site, looks up the callee with the new helper, and emits via `tryEmitEdge`. Pre-seeds `seen` from the shared resolver's emissions so we never double-count. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 22 fail / 169 pass (was 28/163; +6 tests including the Python overload dispatch fixtures, ancestor-directory imports, and same-name module-alias collision). - tsc --noEmit clean. * feat(python-scope): super() receiver dispatches up the MRO Unit 8 — `super().method()` inside a class method walks the enclosing class's MRO chain (skipping self) and resolves to the first ancestor that owns the method. New receiver branch in `emitReceiverBoundCalls` recognizes `super(...)` syntactically (regex-cheap), finds the enclosing class via a new `findEnclosingClassDef` scope-walk helper, then re-uses `scopes.methodDispatch.mroFor` + `findOwnedMember` from the existing class-receiver path. Handled before the compound-receiver case so `super()` doesn't fall into the bare-identifier branch where `super` isn't a binding. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 21 fail / 170 pass (was 22/169; `super().save() inside User to BaseModel.save` now passes). - tsc --noEmit clean. * feat(python-scope): suppress shared resolver on member-call sites Unit 9 — `app_metrics.get_metrics()` (namespace import alias) was emitting two CALLS edges: a wrong self-call from the shared resolver's free-call fallback, plus the correct namespace-receiver edge from the Python post-pass. Mechanism: - `emit-core/emit-references.ts`: new optional `skipSites` parameter (`Set` of `${filePath}:${line}:${col}` keys). When supplied, references at those positions are skipped — the provider has already emitted (or chosen not to emit) for that site. - `python-scope-emit.ts`: reorders Phase 4 — receiver-bound + free- call fallback run FIRST, populating `handledSites`. The shared `emitReferencesViaLookup` then runs with that set so the resolver's fallback can't fight a precise per-receiver emission. Site keys are added only on successful tryEmitEdge (not for sites the post-pass saw but couldn't resolve — those still get a chance from the shared path). Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 20 fail / 171 pass (was 21/170; same-name module-alias collision now resolves correctly). - tsc --noEmit clean. * feat(python-scope): propagate return-type bindings across imports Closes the cross-file return-type propagation gap that left tests like `u = get_user(); u.save()` (where get_user lives in another file) with `u` typed as the function name instead of its return type. The shared finalize pass copies callable bindings (`from x import f` puts `f` in the importer's bindings) but typeBindings stay file-local because they live on `Scope.typeBindings`, not on the index. Mutate post-finalize: - For each module-scope import binding (`origin: 'import'` or `'reexport'`), look up the source file's module-scope typeBinding for the def's simple name. If present (return-annotation source), mirror it under the importer's local alias. Skip when the importer already has its own typeBinding for the name (explicit local always wins). - After propagation, re-run a chain-follow on every scope's typeBindings — pass-4 ran before propagation and missed any chain whose terminal lived in a foreign file. Same algorithm as `followChainedRef` in scope-extractor, but operates on the finalized scopes so propagated entries are visible. Mutating `Scope.typeBindings` is safe — `draftToScope` constructs a plain `new Map(...)`, not a frozen one. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 16 fail / 175 pass (was 20/171; +4 — both cross-file return-type tests, plus two related propagation cases). - tsc --noEmit clean. * feat(python-scope): for-loop call-iterable typeBinding Adds `(for_statement left: (identifier) right: (call function: (identifier)))` to the typeBinding capture set. Combined with Unit 3's return-type capture and the cross-file return-type propagation pass, this makes `for u in get_users(): u.save()` resolve to `User.save` even when `get_users` is imported from another module. Captured as `@type-binding.alias` (rawName = function identifier, without parens) so the existing chain-follow walks the alias to the function's return-type binding without any new code path. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 12 fail / 179 pass (was 16/175; +4 for-loop call-iterable tests across get_users / get_repos fixtures). - tsc --noEmit clean. * feat(python-scope): collapse free-call edges per (caller, target) Free calls (no explicit receiver) now emit a single CALLS edge per (caller, target) pair regardless of how many call sites the caller contains. Mirrors the legacy DAG's per-pair dedup contract — what the `default-params`, `variadic`, and `overload` fixtures expect. Member calls keep position-based dedup so distinct resolved targets (e.g. UserService.find_user vs AdminService.find_user from the same caller) still produce distinct edges. Implementation: bypass `tryEmitEdge` (which dedupes positionally) and hand-roll the relationship with a position-independent rel.id (`rel:CALLS:->`). Site handling is now unconditional — even when the dedup-collapse skips the actual emit, we mark the site handled so the shared `emit-references` doesn't fight us with its fallback. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 10 fail / 181 pass (was 12/179; +2 — both `default parameter arity` tests now pass). - tsc --noEmit clean. * fix(python-scope): match legacy CALLS reason for import-resolved free calls The arity-narrowing test asserts \`rel.reason === 'import-resolved'\` for cross-file free-call edges. Switch the free-call fallback's reason to mirror legacy DAG semantics: - target-file !== source-file → 'import-resolved' - same file → 'local-call' Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 9 fail / 182 pass (was 10/181; +1 arity-narrowing test). - tsc --noEmit clean. * fix(python-scope): drop dead pre-seeding from receiver-bound pass The pre-seeding loop at the top of \`emitReceiverBoundCalls\` populated \`seen\` with every reference the shared resolver had already resolved. That was useful when emit-references ran FIRST. After Unit 9 reversed the order (emit-references runs after the Python passes and uses \`handledSites\` to skip what we processed), the pre-seed only causes harm: when an MRO walk in Case 0 (compound receiver) and Case 4 (simple typeBinding) both touch the same site at the same position but resolve to different targets, the pre-seed suppresses the second emission because the shared resolver had already entered the wrong target into \`seen\`. Concrete case: \`c.greet().save()\` — Case 0 emits the outer save edge to Greeting.save; Case 4 then resolves the inner \`c.greet()\` to A.greet via MRO walk. With pre-seed both edges should emit (different targets, different rel.ids); without removing the pre-seed the inner emission was being deduped against an already-seeded entry and the A.greet edge was lost. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 8 fail / 183 pass (was 9/182; +1 — \`c.greet() to A#greet via MRO walk\` now passes). - tsc --noEmit clean. * feat(python-scope): enumerate(X) for-loop tuple destructuring Adds two new typeBinding capture patterns for the canonical enumerate pattern: for (i, u) in enumerate(users): ... ; tuple_pattern for i, u in enumerate(users): ... ; pattern_list Both bind the second tuple element (u) to the iterable identifier (users). The chain-follow then unwraps users → its element type via the existing generic-strip in interpret.ts (List[User] → User). The #eq? predicate scopes the pattern to enumerate specifically; generic tuple destructuring of arbitrary callables is left to a future iteration once we have a richer signal for "what does this call yield". Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 7 fail / 184 pass (was 8/183; +1 — `parenthesized tuple: for (i, u) in enumerate(users)` now passes). - tsc --noEmit clean. * feat(python-scope): dict.items() value-type unwrapping Two changes that together resolve `for k, v in data.items(): v.save()`: - `interpret.ts stripGeneric`: extends to `dict[K, V]` / `Dict[K, V]` / `Mapping[K, V]` etc., stripping to the value type V. Previously only single-arg generics (list[User] → User) were stripped; multi-arg ones returned the raw text. - `query.ts` + `scopes.scm`: new typeBinding patterns for `for k, v in X.items()` (both pattern_list and tuple_pattern). The second tuple element binds to X; the chain-follow then unwraps X's dict annotation to V via the new stripGeneric branch. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 6 fail / 185 pass (was 7/184; +1 — `dict.items() loop` test now passes). - tsc --noEmit clean. * feat(python-scope): nested tuple destructuring for enumerate(d.items()) Two more for-loop typeBinding patterns: - `for i, (k, v) in enumerate(d.items())` — nested tuple destructuring where v is the value of the dict's items() yield. - `for v in d.values()` — explicit values() form (companion to items). Both bind the loop var to the dict identifier; the chain-follow unwraps via the dict-aware stripGeneric to the value type. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 5 fail / 186 pass (was 6/185; +1 nested tuple test). - tsc --noEmit clean. * feat(python-scope): 3-var flat destructuring for enumerate(d.items()) Adds the \`for i, k, v in enumerate(d.items())\` shape — flat 3-variable destructuring of the (i, (k, v)) tuple yielded by \`enumerate\` over \`items()\`. Binds v (the last identifier in the pattern_list) to the dict identifier; the existing dict-aware stripGeneric unwraps to the value type. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 4 fail / 187 pass (was 5/186; +1). - tsc --noEmit clean. * feat(python-scope): write ACCESSES edges for attribute assignments Three changes that together produce ACCESSES (write) edges for \`obj.field = value\` assignments: - New \`@reference.write.member\` capture in query.ts and scopes.scm matching \`(assignment left: (attribute object: ... attribute: ...))\`. Reuses the existing receiver/name capture shape so the receiver-bound emit pass can resolve obj's class and look up the field. - \`populateMethodOwnerIds\` now sets ownerId on class-body fields too, not only on methods. Previously it only walked Function scopes whose parent was Class; class-body annotations like \`name: str\` live directly in the Class scope's ownedDefs and were missed, so \`findOwnedMember(User, "name")\` returned undefined. - \`emit-core isLinkableLabel\` extends to Variable and Property so field nodes appear in the graph-node lookup (the legacy parser emits both kinds for class-body annotations). - Case 4 in receiver-bound pass now uses the kind word as the edge reason for read/write sites — matches the legacy DAG convention the test asserts on. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 3 fail / 188 pass (was 4/187; +1 — write-ACCESSES test). - tsc --noEmit clean. * feat(python-scope): chain-typebinding + field-fallback method lookup Reaches the architectural-plan target of >= 189/191 flag-on passing. Two intertwined changes: - Field-fallback in resolveCompoundReceiverClass: when method lookup on the receiver's class (and its MRO) fails, walk the class's fields and try the same lookup on each field's type. Matches the "unified fixpoint" intent of the method-chain fixture where `user.get_city()` reaches `Address.get_city` through User's `address: Address` field. - New Case 3b in receiver-bound emit pass: when the receiver's typeBinding rawName has a dot but isn't a namespace prefix (e.g. `city -> user.get_city` from the constructor-inferred capture for `city = user.get_city()`), treat it as a method-call chain and pipe through the compound resolver. The chain unwraps to the terminal class (City) and the call resolves normally. Verification: - Flag-off: 191/191 (identical baseline). - Flag-on: 2 fail / 189 pass (was 3/188; +1 city.save method chain). - tsc --noEmit clean. Remaining 2 failures are fixture-driven (self.users / self.repos fixtures reference fields that aren't declared on the class) and documented as known-limitation in Unit 10. * feat(python-scope): flip Python to registry-primary (191/191 parity) Adds the \`for u in self.X\` heuristic typeBinding capture (binds u to the attribute name X so the chain-follow can resolve via the enclosing method's parameter typeBinding) — closes the last two failing fixtures whose classes reference \`self.X\` for fields that are actually method parameters. With 191/191 passing on BOTH legacy and registry-primary paths, flips \`MIGRATED_LANGUAGES\` to include \`SupportedLanguages.Python\`. Effects: - Production default for Python files: registry-primary path. - CI parity gate auto-discovers Python via the script + workflow (\`scripts/ci-list-migrated-languages.ts\` / \`.github/workflows/ci-scope-parity.yml\`) and runs the resolver integration test BOTH ways on every PR. - Operators retain the \`REGISTRY_PRIMARY_PYTHON=0\` escape hatch. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - Default (unset, post-flip): 191/191 (uses registry). - tsc --noEmit clean. This concludes RFC #909 Ring 3 — Python migration. * refactor(emit-core): EmitProvider interface + promote 5 generic helpers G-Units 1-2 of the emit-pipeline generalization plan. Adds: - emit-core/emit-provider.ts — typed EmitProvider contract (6 required + 2 optional fields). Will be consumed by the generic orchestrator in G-Unit 6. Documents the LanguageProvider vs EmitProvider boundary. - emit-core/emit-free-call.ts — emitFreeCallFallback promoted as-is (drops the unused referenceIndex pre-seed parameter; underscore-prefixed to keep the signature compatible). - emit-core/propagate-return-types.ts — propagateImportedReturnTypes + followChainPostFinalize. Documents the mutation contract (Invariant I3 + I6 from the plan): runs after finalize, before resolve, mutates the non-frozen Scope.typeBindings map. - emit-core/scope-walkers.ts: + findEnclosingClassDef + findExportedDefByName. Both were already generic in the Python source. python-scope-emit.ts shrinks 1055 → 799 lines (–256). Imports the promoted helpers from emit-core. No behavior change. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - tsc --noEmit clean. * refactor(emit-core): promote receiver-bound dispatcher + compound resolver G-Unit 3 of the emit-pipeline generalization plan. - emit-core/emit-compound-receiver.ts — resolveCompoundReceiverClass + matchingOpenParen + COMPOUND_RECEIVER_MAX_DEPTH. Field-fallback is now an option (default true) so strictly-typed languages can opt out via EmitProvider.fieldFallbackOnMethodLookup. - emit-core/emit-receiver-bound.ts — the 7-case dispatcher (super, Cases 0/1/2/3/3b/4). Accepts a ReceiverBoundProviderSubset (isSuperReceiver + fieldFallbackOnMethodLookup) so partial wiring works during the rest of the migration. Documents Contract Invariants I4 (case order) and I5 (no pre-seeding). python-scope-emit.ts shrinks 799 → 384 lines. The orchestrator now calls the generic emitReceiverBoundCalls with an inline minimal provider (pythonEmitProviderInline) — full provider lands in G-Unit 6 when the orchestrator itself moves to languages/python/emit/. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - tsc --noEmit clean. * refactor(emit-core): promote MRO walk + populateClassOwnedMembers G-Units 4-5 of the emit-pipeline generalization plan. - emit-core/build-mro.ts — generic buildMro takes a LinearizeStrategy hook receiving (classDefId, directParents, parentsByDefId). Three shared steps (collect EXTENDS, build defId-by-graphId, walk per class) + parametric linearization. Default strategy is BFS-with- visited (Python's depth-first first-seen, also correct for single-inheritance languages). - emit-core/scope-walkers.ts: + populateClassOwnedMembers — generic OO ownership rule (methods + class-body fields). Both rules ship together because every OO language migrated so far (Python; planned TS/JS/Java/Kotlin) wants both. Languages that need different rules can compose with this as a base step. python-scope-emit.ts shrinks 384 → 255 lines. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - tsc --noEmit clean. * refactor(scope-resolution): generic orchestrator + language-agnostic phase G-Units 6-7 of the emit-pipeline generalization plan, plus the pipeline-phase generalization (the user's observation that the phase itself is generic once the orchestrator is). Changes: - emit-core/orchestrator.ts — runScopeResolution(input, provider). The 180 lines of pipeline glue moved here, parametrized by EmitProvider. Provider supplies LanguageProvider, importEdgeReason, and the 6 emit-side hooks. - emit-core/emit-provider.ts — EmitProvider gains languageProvider and importEdgeReason fields so the orchestrator needs nothing else. resolveImportTarget now takes (targetRaw, fromFile, allFilePaths). - languages/python/emit/index.ts — pythonEmitProvider + thin runPythonScopeResolution wrapper. The first reference impl every next-language migration copies. - emit-providers-registry.ts (NEW) — registry of per-language EmitProviders keyed by SupportedLanguages. Adding a language is one line here + the provider file. - pipeline-phases/scope-resolution.ts (NEW) — language-agnostic phase iterating EMIT_PROVIDERS ∩ MIGRATED_LANGUAGES. Replaces pipeline-phases/python-scope.ts (deleted). - python-scope-emit.ts deleted. - pipeline.ts swaps pythonScopePhase → scopeResolutionPhase. The next language migration is now: implement EmitProvider, register it, add to MIGRATED_LANGUAGES. No new pipeline phase, no orchestrator copy-paste. The Python migration's 700+ lines of glue collapse to ~80 lines per future language. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - Default (post MIGRATED_LANGUAGES flip): 191/191. - tsc --noEmit clean. * docs(emit-provider): migration cookbook for next-language porters * refactor(scope-resolution): rename emit-core/ → scope-resolution/, EmitProvider → ScopeResolver Reorganizes the registry-primary resolution layer for clarity and contributor onboarding. Driven by feedback that "emit" was triple- overloaded (graph-edge emission + tree-sitter capture extraction + the provider name itself), and the flat 16-file emit-core/ folder mixed five concerns. External research (rust-analyzer hir-def/nameres, Pyright analyzer/, TypeScript binder/checker, Roslyn Binder, IntelliJ Resolver, swc semantic/, biome semantic/, semgrep naming/, JDT Binding, clangd Sema) consistently uses **the phase name** for this layer, never an output verb. "Scope resolution" matches our pipeline-phase name, the plan, and the RFC. ## Folder rename emit-core/ → scope-resolution/ ├── (16 flat files) → ├── contract/scope-resolver.ts ├── pipeline/{run,registry,phase}.ts ├── passes/{receiver-bound-calls, │ free-call-fallback, │ compound-receiver, │ imported-return-types, │ mro}.ts ├── graph-bridge/{node-lookup,ids, │ edges,references-to-edges, │ imports-to-edges, │ method-dispatch}.ts └── scope/{walkers,namespace-targets}.ts Each subfolder maps to one concern a new contributor needs to find: *the contract I implement / the runner that calls me / the helpers I reuse / the graph layer I shouldn't touch / the scope walkers*. ## Symbol renames EmitProvider → ScopeResolver pythonEmitProvider → pythonScopeResolver runPythonScopeResolution → resolvePythonScope EMIT_PROVIDERS → SCOPE_RESOLVERS getEmitProvider → getScopeResolver RunPythonScopeResolution{Input,Stats} → ResolvePythonScope{Input,Stats} ## File renames (per-language) languages/python/emit/index.ts → languages/python/scope-resolver.ts languages/python/emit-captures.ts → languages/python/captures.ts (kills the parse-side "emit" collision) ## Mechanics - Used `git mv` for all files so blame history is preserved. - Updated ~30 import lines across 18 files plus the pipeline-phases barrel and pipeline.ts. - Updated JSDoc cross-references throughout to match the new vocabulary. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - Default (post MIGRATED_LANGUAGES flip): 191/191. - tsc --noEmit clean. Migration cookbook in `scope-resolution/contract/scope-resolver.ts` JSDoc points the next-language porter at all the new names and folder locations. * docs(scope-resolution): finalize phase JSDoc + drop python emoji from generic log line * perf(scope-resolution): O(1) workspace lookup index Introduces `WorkspaceResolutionIndex` — a precomputed bundle of lookup tables built ONCE per resolution run, after `populateOwners` and after finalize, before any pass that needs to find members, exported defs, or class scopes by id. What it replaces (all are pre-existing O(N×D) linear scans of parsedFiles, called inside the receiver-bound MRO chain): - `findOwnedMember(ownerId, name, parsedFiles)` → `Map.get` via `index.memberByOwner.get(ownerId)?.get(name)`. Was the worst offender — receiver-bound dispatcher calls this O(sites × MRO depth) times. - `findExportedDef(filePath, name, parsedFiles)` → `Map.get` via `index.defsByFileAndName`. Hot for namespace-receiver case. - `findExportedDefByName` workspace-wide fallback scan → `Map.get` via `index.callablesBySimpleName`. - `classScopeByDefId` (rebuilt inside `emitReceiverBoundCalls` on every invocation) — moved to one-shot build during finalize, read from `index.classScopeByDefId` everywhere. - `moduleScopeByFile` (rebuilt inside `propagateImportedReturnTypes` on every invocation) — read from `index.moduleScopeByFile`. Findings from a synthetic 100-file Python workload (60 model files each defining 5 classes × 3 methods + 40 user files calling them heavily): scope-resolution wall time: 764ms → 710ms (median, 5 iters) That's a ~7% in-layer win. The smaller-than-expected gain was informative: profiling the synthetic workload shows scope-resolution breakdown is `extract=62% resolve=30% emit=4%`; the index touched the 4% slice (emit + walker calls inside it). Larger O(D) per owner classes will benefit more. Profiling the FULL pipeline (49 fixtures × 3 iters) shows scope-resolution accounts for ~1% of pipeline wall time — the remaining 99% is parse (tree-sitter), heritage, ORM, MRO, processes, and DB writes. So further optimization of this specific layer has marginal pipeline impact; the next-biggest wins live in those phases. Documented as the "double-parse" finding in the audit (captures.ts re-parses each Python file even though the parse phase already produced a tree-sitter Tree) — that's a separate plumbing project across phase boundaries. Bonus: opt-in PROF_SCOPE_RESOLUTION=1 env var prints a per-phase ms breakdown to stderr, so future perf work can measure without extra code changes. Verification: - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - tsc --noEmit clean. * perf(parse/heritage/mro): typed graph iterator + cross-phase tree cache Two structural perf wins targeting the parse / heritage / MRO layers, identified by the post-WorkspaceResolutionIndex profiling (scope-resolution = ~1% of pipeline; the bulk lives upstream). ## 1. KnowledgeGraph.iterRelationshipsByType (PHM-Units 1-2) - Adds a per-type `Map>` index inside `createKnowledgeGraph`, maintained on add / remove / removeNode / removeNodesByFile. - New `iterRelationshipsByType(type)` returns a typed iterator that yields only the requested type. Backwards-compatible: existing `iterRelationships()` / `forEachRelationship()` callers untouched. - Migrated two MRO call sites: - `mro-processor.ts buildAdjacency`: split the single `forEachRelationship` (which scanned every edge in the graph and type-filtered per-iteration) into three typed iterations (EXTENDS, IMPLEMENTS, HAS_METHOD). - `scope-resolution/passes/mro.ts buildMro`: replaced `for (const rel of graph.iterRelationships()) if (rel.type !== 'EXTENDS') continue` with `for (const rel of graph.iterRelationshipsByType('EXTENDS'))`. - Heritage-processor (PHM-Unit 3) was a no-op: it only WRITES EXTENDS/IMPLEMENTS edges, never re-reads. Index is still useful for the seven other graph-iter consumers (community-processor, csv-generator, wildcard-synthesis, process-processor, etc.) — those follow-ups can switch to the typed iterator without touching the graph layer. - Adds 5 unit tests for the new method (add/remove/dedupe semantics, empty-type fresh iterator, removeNode index sync). ## 2. Cross-phase tree cache (PHM-Units 4-5) The audit's #2 finding: Python files are parsed by tree-sitter once in the parse phase, then re-parsed inside scope-resolution's `captures.ts`. Eliminate the second parse by sharing the Tree across phases. - `parse-impl.ts` now maintains TWO ASTCaches with distinct lifetimes: - `astCache` (chunk-local, cleared between chunks) — unchanged; used by call/heritage/import processors during parse. - `scopeTreeCache` (total-parseable-sized, never cleared) — new, exposed via `ParseOutput.astCache` for cross-phase consumption. - `parsing-processor.ts` writes every sequentially-parsed Tree to BOTH caches. Worker-mode parses skip the persistent cache too (Trees can't cross MessageChannels). - `LanguageProvider.emitScopeCaptures` gains an optional `cachedTree` parameter (typed `unknown` to keep the tree-sitter dep out of the contract). - `captures.ts` short-circuits its own `parser.parse(sourceText)` when a cached Tree is supplied. Cache miss falls back to a fresh parse — same correctness path as before. - `runScopeResolution` accepts an optional `treeCache` and forwards per-file `cachedTree` to `extractParsedFile`. - `scope-resolution/pipeline/phase.ts` reads `getPhaseOutput<{astCache}>(deps, 'parse')` and passes through. Verified end-to-end: a small fixture run with PROF_SCOPE_RESOLUTION=1 shows 6/6 cache hits (100% hit rate) on the python-grandparent fixture that exercises the full pipeline below the worker-pool threshold. ## Verification - REGISTRY_PRIMARY_PYTHON=0 (legacy): 191/191. - REGISTRY_PRIMARY_PYTHON=1 (registry): 191/191. - New graph.test.ts: 25/25 (was 20). - tsc --noEmit clean. ## Where the win lands Wall-clock on the 49-fixture integration suite: 14050ms → 14080ms (within noise). Fixtures are 1-3 files each, dominated by per-fixture pipeline overhead (worker-pool init, DB writes, fixture startup). The cache + typed-iterator wins are constant-factor improvements that scale linearly with workload size and visible only on larger repos. The dev-mode `PROF_SCOPE_RESOLUTION` instrumentation + `getPythonCaptureCacheStats()` are kept for future perf work. ## Plan docs/plans/2026-04-20-002-perf-parse-heritage-mro-plan.md. PHM-Unit 3 (heritage-processor migration) intentionally collapsed to a no-op — heritage only writes, never re-reads. * perf(scope-resolution): bound tree-cache lifetime + gate population Address P1 residuals from ce:review of 8c6f5cee: - Dispose scopeTreeCache at end of scopeResolutionPhase via astCache.clear(). Trees were previously retained for the full pipeline (10-100x memory regression on large repos). Downstream phases (mro, community, csv-generator) never read them. - Gate scopeTreeCache.set on provider.emitScopeCaptures !== undefined. Polyglot repos no longer retain Trees for languages with no scope-resolution consumer. - PROF_SCOPE_RESOLUTION=1 now warns when workers engage, since Trees can't cross MessageChannels so the cache will be empty for worker-parsed files — prevents a silent perf cliff once a repo crosses the worker-pool threshold. Tests: 26/26 graph unit, 299/299 scope-resolution unit, 191/191 python integration both flag paths. * refactor(scope-resolution): clean up P2/P3 review residuals P2: - WASM dual-ownership invariant documented on ASTCache dispose: a Tree must live in AT MOST ONE disposing ASTCache. Native tree-sitter today is unaffected; WASM adoption would require tree.copy() or a non-disposing secondary cache. - mro-processor C3 ordering test: pins EXTENDS-before-IMPLEMENTS parent grouping for classes with interleaved edge additions. Asserts exact MRO ['Base', 'Iface'] — a revert to single-loop insertion-order iteration would produce ['Iface', 'Base'] and fail loudly. - cached-tree parity test: emitPythonScopeCaptures(src, path, T) returns identical CaptureMatch[] to emitPythonScopeCaptures(src, path). Pins the cache-hit path's correctness so a regression that silently returns stale captures would break the test. P3: - Dev-mode cache counters moved from captures.ts to cache-stats.ts. Production hot-path module no longer carries the module-global export surface; PROF gating behavior preserved. - ParseOutput field rename astCache → scopeTreeCache. Clarifies that the surfaced cache is the persistent cross-phase one, not the chunk-local astCache parse-impl clears between chunks. Single consumer (scopeResolutionPhase) updated; no other readers. - ASTCacheReader interface extracted. scopeResolutionPhase now reads the phase dep via a shared type instead of a hand-rolled inline structural shape that could drift from ASTCache's contract. - graph.ts dual-index invariant enforced through writeRel/deleteRel private helpers instead of duplicated add/delete at 3 mutation sites. Adding a new mutation method only needs to call the helpers — forgetting to update one index becomes structurally impossible. Tests: 382/382 unit (incl. 2 new), 191/191 python integration both flag paths. tsc clean. * fix(ci): prettier formatting + Python-migration test adjustments CI run 24666612657 failed on three jobs. Fixes: quality/format: - Prettier --check flagged 3 files after the accumulated branch work. Ran prettier --write from repo root (CI's invocation cwd) to apply: simple-hooks.ts, resolve-references.ts, python-hooks.test.ts. tests/{ubuntu,macos,windows} — 9 assertion failures, all traceable to Python landing in MIGRATED_LANGUAGES (default-on registry-primary): - registry-primary-flag.test.ts (3 tests): the 'returns false by default' / 'primaryLanguages empty' / 'Python mid-process mutation' assertions were written in Ring 2 when MIGRATED_LANGUAGES was empty. Rewrote to assert MIGRATED_LANGUAGES membership is the default, use Java (unmigrated) for the no-stale-cache test, and verify env overrides work in both directions (migrated-off, unmigrated-on). - call-processor.test.ts (6 tests in SM-10 + D2-widen blocks): these exercise the LEGACY call-resolution DAG on .py fixtures. processCalls now gates Python out (isRegistryPrimary === true by default), returning 0 edges. Added REGISTRY_PRIMARY_PYTHON=false override in the relevant beforeEach + restore in afterEach, so the legacy DAG runs for these test-local fixtures without affecting the production-default behavior. Local verification: 4126/4126 unit tests pass, prettier clean. * docs(python): known-limitation block on scope-resolution public API Unit 10 — document what the Python registry-primary path intentionally does not resolve, so reviewers and future maintainers can distinguish conscious trade-offs from latent bugs: - Dynamic attribute access (getattr / setattr) - Dynamic imports (importlib, __import__) - Metaclass-driven dispatch - Union / Optional branch-picking behavior - Arbitrary signature-rewriting decorators - typing.TYPE_CHECKING-guarded imports - *args / **kwargs type flow-through - super() outside a directly-bound method Each item names the file that owns the relevant hook so a future follow-up knows where to start. Shadow-harness corpus parity + the CI parity gate remain the authoritative signal for which of these matter at fleet scale. * docs: record scope-resolution pipeline alongside legacy call DAG Capture what shipped in #980 so future readers don't have to reverse- engineer the coexistence of the legacy call-resolution DAG and the new scope-resolution pipeline: - ARCHITECTURE.md: new 'Scope-Resolution Pipeline' section after the Call-Resolution DAG, documenting pipeline stages, ScopeResolver contract, per-language registration, code references, and perf notes. Coexistence block added to the legacy DAG section explaining how MIGRATED_LANGUAGES gates the two paths per-language. - AGENTS.md: reference-docs pointer updated — legacy-DAG one-liner stays; scope-resolution pipeline gets its own pointer so agents know when to read which section. Changelog bumped. - type-resolution-system.md: callout at the 'call-processor.ts is the consumer' claim pointing readers to the scope-resolution path for migrated languages. TypeEnv is still built per file, but for migrated languages receiver typing flows through ParsedTypeBinding rather than call-processor.ts. CHANGELOG.md intentionally not touched — owned by the release process. * chore: remove obsolete scheduled_tasks.lock file * fix(scope-resolution): qualified-name keys for same-file method collisions Review feedback from PR #980 reviewer flagged a BLOCKING correctness bug: when two classes in the same file define a method with the same simple name (e.g. class User: def save + class Document: def save), every d.save() CALLS edge silently resolved to User.save because the graph node lookup keyed only by (filePath, simpleName) and first-wins took User's method. Three-layer fix: 1. populateClassOwnedMembers now promotes a nested def's qualifiedName from `save` to `ClassName.save` when the def sits inside a class scope. Python's scopes.scm doesn't emit @declaration.qualified_name for methods, so without this the finalized SymbolDefinition carried only the simple name. 2. buildGraphNodeLookup adds a second key per node: (filePath, qualifiedName). For Method/Function nodes the qualifier is parsed deterministically out of the node id (`Method:file.py:User.save#N` → `User.save`), which is robust to Windows-style filePath colons. Simple-name key retained as a fallback for callers that don't know the qualifier. 3. resolveDefGraphId now tries the qualified key first, then falls back to the simple-name lookup. Also addresses the non-blocking review items: - scopeResolutionPhase.deps now includes `crossFile` so the Kahn's runner can't schedule scope-resolution before crossFile finishes writing heritage edges that buildMro consumes. - run.ts no longer mutates the finalized ScopeResolutionIndexes via `as` cast — spreads into a fresh object with the populated methodDispatch field instead. - Doc nits: scope-resolver.ts registry path + phase.ts Ring number. Test coverage: - New fixture test/fixtures/lang-resolution/python-same-file-method-collision with User.save + Document.save in one file and app.py calling both through typed receivers. - Three new integration assertions pin that u.save() and d.save() target the correct qualified node id. Fail before the fix, pass after. Confirmed by running once without populateClassOwnedMembers qualifier promotion — reproduces the original User.save-for-both bug. Verification: 194/194 test/integration/resolvers/python.test.ts pass both REGISTRY_PRIMARY_PYTHON=0 and =1. 523/523 related unit tests. tsc --noEmit clean. * fix(scope-resolution): filter export index to module-level defs + label-prefixed qualified key Codex adversarial review on PR #980 flagged that buildWorkspaceResolutionIndex feeds defsByFileAndName and callablesBySimpleName from parsed.localDefs — the flat set of every def in the file including methods, fields, and nested functions. findExportedDef / findExportedDefByName treat those maps as file-level exports, so `mod.save()` could silently bind to User.save whenever a method's simple name appeared first in parse order. Plan: docs/plans/2026-04-21-001-fix-workspace-index-module-scope-only-plan.md Fix layers: 1. workspace-index.ts: split the single parsed.localDefs loop into two passes: - Module-export pass: iterate moduleScope.ownedDefs PLUS ownedDefs of every child scope whose parent is the module scope. Top-level class and function declarations each live in their own scope with parent=module, not in moduleScope.ownedDefs directly, so the "parent === moduleScope.id" walk is required to reach them. Methods (scope.parent === Class scope) and nested functions (scope.parent === another Function scope) are excluded. - Member-by-owner pass: keeps iterating parsed.localDefs since that map is keyed on ownerId and correctly saw class-owned defs before this change. 2. graph-bridge/node-lookup.ts: qualified keys now live in a separate keyspace (`:filePath::