Skip to content

feat: cross-repo impact analysis (#794) — @repo MCP routing + group resources#984

Merged
magyargergo merged 13 commits into
abhigyanpatwari:mainfrom
ivkond:feat/issue-794-cross-repo-impact
Apr 20, 2026
Merged

feat: cross-repo impact analysis (#794) — @repo MCP routing + group resources#984
magyargergo merged 13 commits into
abhigyanpatwari:mainfrom
ivkond:feat/issue-794-cross-repo-impact

Conversation

@ivkond

@ivkond ivkond commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #794. Implements the cross-repo impact analysis surface defined in the linked issue and the refreshed RFC at docs/superpowers/specs/2026-03-31-cross-index-impact-design.md.

The MCP surface is now group-aware without introducing parallel group_* tools:

  • impact, query, context accept repo: "@<groupName>" (optionally "@<groupName>/<memberPath>") and an optional service parameter for monorepo-boundary filtering.
  • Group-scoped state is exposed via two read-only resources:
    • gitnexus://group/{name}/contracts
    • gitnexus://group/{name}/status
  • The previously-planned group_impact, group_contracts, group_query, group_status MCP tools are intentionally not introduced — group_list and group_sync remain.

Merge as Squash and merge. Phase 4 (tools.ts cleanup), Phase 5 (@repo routing in local-backend.ts), and Phase 6 (group resources) live on the same branch and must land atomically — there is no feature flag and no half-state to guard.

What changed

  • Phase 1 + 2 — Cross-impact engine. gitnexus/src/core/group/cross-impact.ts implements the two-phase traversal (local impact walk → cross-repo fan-out via bridge.lbug Cypher) with UID-based symbol resolution (impactByUid) to suppress false positives on common symbol names. Types in gitnexus/src/core/group/types.ts (GroupImpactResult, GroupContextResult).
  • Phase 3 — GroupService + CLI. GroupService.{groupImpact,groupContext,groupQuery,groupContracts,groupStatus,groupList} plus gitnexus group impact (gitnexus/src/cli/group.ts). safeParseMeta makes the contract registry resilient to corrupt rows.
  • Phase 4 — MCP tool cleanup. gitnexus/src/mcp/tools.ts removes group_contracts, group_query, group_status and extends impact/query/context descriptions for @<groupName> + service.
  • Phase 5 — @repo routing. gitnexus/src/mcp/local/local-backend.ts dispatches repo: "@..." calls on impact/query/context to GroupService, stripping the @ prefix before delegation and forwarding service.
  • Phase 6 — Group resources. gitnexus/src/mcp/resources.ts and LocalBackend register gitnexus://group/{name}/contracts and gitnexus://group/{name}/status and route reads through GroupService.
  • Phase 7 — Docs. RFC aligned with the [group] Cross-repo impact analysis, CLI and MCP tool (split 5/5 of #606) #794 MCP model (migration table, resources table, AI-context guidance). gitnexus/src/cli/ai-context.ts updated to advertise the new surface.

Tests

New / updated test files (all green locally on main baseline + this branch):

  • gitnexus/test/unit/group/cross-impact.test.ts
  • gitnexus/test/unit/group/service.test.ts
  • gitnexus/test/unit/group/group-service-group-mode.test.ts
  • gitnexus/test/unit/group/group-tools.test.ts
  • gitnexus/test/integration/group/group-impact.test.ts
  • gitnexus/test/integration/group/group-cli.test.ts
  • gitnexus/test/unit/tools.test.ts (adjusted for removed/extended tools)
  • gitnexus/test/unit/resources.test.ts (new group resources)
  • gitnexus/test/unit/mcp/group-repo-routing.test.ts (includes table-driven Issue [group] Cross-repo impact analysis, CLI and MCP tool (split 5/5 of #606) #794 manual smoke checklist (automated) covering impact / query / context with repo: "@myproduct" + service: "app/backend")

Test plan

  • npx tsc --noEmit — clean
  • npx vitest run test/unit/group/cross-impact.test.ts --pool=forks
  • npx vitest run test/unit/group/service.test.ts --pool=forks
  • npx vitest run test/unit/group/group-service-group-mode.test.ts --pool=forks
  • npx vitest run test/integration/group/group-impact.test.ts --pool=forks
  • npx vitest run test/integration/group/group-cli.test.ts --pool=forks
  • npx vitest run test/unit/group/group-tools.test.ts --pool=forks
  • npx vitest run test/unit/tools.test.ts --pool=forks
  • npx vitest run test/unit/resources.test.ts --pool=forks
  • npx vitest run test/unit/mcp/group-repo-routing.test.ts --pool=forks

Local Final-verification snapshot: 9 files passed, 111 tests passed, tsc clean.

Migration notes for MCP consumers

Old / planned New
group_impact { name, target, service? } impact { repo: "@<name>", target, service? }
group_query { name, query, service? } query { repo: "@<name>", query, service? }
group_context { name, target, service? } context { repo: "@<name>", target, service? }
group_contracts { name } resource gitnexus://group/{name}/contracts
group_status { name } resource gitnexus://group/{name}/status
group_list, group_sync unchanged

ivkond added 7 commits April 16, 2026 00:49
Expand MCP dispatch model, contracts (GroupImpactResult, GroupContextResult),
service semantics, timeout/safeLocalImpact, resolveGroupRepo placement,
safeParseMeta behavior, Phase 3.3a/3.3b test split, parseUri rules, Phase 5
mapping, risks, and final verification commands.

Made-with: Cursor
- Extend GroupImpactResult with timeoutMs, truncationReason, crossDepthWarning
- Add GroupContextResult and per-repo context aggregation
- Implement runGroupImpact (local impact + bridge Cypher fan-out) in cross-impact.ts
- Resilient contracts.json parsing with skippedCorrupt metadata
- Extend GroupToolPort with context; wire LocalBackend
- Unit tests for cross-impact and GroupService

Made-with: Cursor
…ool cleanup

Phase 3: Add integration tests for groupImpact and CLI group impact; add unit
tests for GroupService group-mode API and LocalBackend @group routing.

Phase 4: Remove MCP tools group_contracts, group_query, and group_status;
extend impact, query, and context schemas and descriptions for @group repo
syntax, optional service, and numeric bounds aligned with validation.

Phase 5: Route impact/query/context in LocalBackend.callTool when repo starts
with @ before resolveRepo; narrow handleGroupTool to group_list and group_sync;
add resolveAtGroupMemberRepoPath helper and groupQuery service path filtering.

Made-with: Cursor
- Add subgroupExact to groupQuery/groupContext so @group/member does not
  prefix-match nested member paths (Codex/OpenCode HIGH).
- Build explicit param payloads in callToolAtGroupRepo; map context tool
  symbol name to target so group name is not overwritten (correctness).
- Narrow MCP tool copy: service applies in @ group mode only; align text.
- Refresh ai-context cross-repo section (remove retired group_* tools).
- Extend unit tests for exact filter, routing errors, removed tools, MCP name.

Made-with: Cursor
- Register `gitnexus://group/{name}/contracts` and
  `gitnexus://group/{name}/status` MCP resources backed by GroupService
  (resources.ts + LocalBackend resource handlers).
- Cover the new resources with unit tests in test/unit/resources.test.ts.
- Convert the Phase 5.4 manual smoke into a table-driven test in
  test/unit/mcp/group-repo-routing.test.ts: `impact` / `query` / `context`
  with `repo: "@myProduct"` + `service: "app/backend"` route to the
  matching GroupService method and the leading `@` is stripped before
  delegation.
- Ignore local agent scratch dirs (.tmp/, .agents/) so review prompts
  and generated skill mirrors never leak into a commit.

Refs #794

Made-with: Cursor
…CP surface

- Mark Phases 1–7 in docs/superpowers/plans/2026-04-16-issue-794-implementation.md
  as complete, fold in the Issue #794 traceability map, and record the
  Final verification snapshot (tsc clean + 9 vitest files / 111 tests).
- Update docs/superpowers/specs/2026-03-31-cross-index-impact-design.md
  to match the Issue #794 MCP model:
    * `group_impact` / `group_contracts` / `group_query` / `group_status`
      are NOT MCP tools.
    * `impact` / `query` / `context` accept `repo: "@<groupName>"` and
      optional `service` for monorepo boundary filtering.
    * Group state is exposed via `gitnexus://group/{name}/contracts`
      and `gitnexus://group/{name}/status` resources.
    * Add MCP migration table and refresh AI-context guidance.
- Restate execution handoff: Phases 4 + 5 + 6 share one branch and must
  ship as a single squash-merge to avoid a half-migrated MCP surface.

Closes #794

Made-with: Cursor
@vercel

vercel Bot commented Apr 19, 2026

Copy link
Copy Markdown

@ivkond is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

Sync with upstream — picks up RFC #909 Ring 1/Ring 2 (scope-resolution
SHARED + ingestion PKG), MCP context/impact disambiguation hints (#888),
ai-context CLAUDE.md dedup (#904), Docker/release pipeline (#848 / #978
/ #983), `analyze --name <alias>` (#955) and the v1.6.2 release line.

Conflicts and how they were resolved:

- .gitignore — kept both upstream's `docker/.env` and our local
  `.tmp/` + `.agents/` ignore block.
- gitnexus/src/mcp/local/local-backend.ts — kept the new
  `resolveAtGroupMemberRepoPath` import alongside upstream's
  `collectBestChunks` / embedding / `PhaseTimer` imports.

Auto-merged but verified manually:

- gitnexus/src/mcp/tools.ts — Phase 4 cleanup intact
  (no `group_contracts` / `group_query` / `group_status` /
  `group_impact` tools), Phase 4/5 `repo: "@<groupName>"` + `service`
  descriptions still present on `impact` / `query` / `context`.
- gitnexus/src/cli/ai-context.ts — Phase 7 guidance kept; agents are
  still pointed at `impact` / `query` / `context` with `@<groupName>`.

Verification after merge:
- `npx tsc --noEmit` (in gitnexus/) — clean (after rebuilding
  `gitnexus-shared` with `npm run build` for new RFC #909 exports).
- `npx vitest run --pool=forks` over the Issue #794 Final-verification
  block — 9 files passed / 111 tests passed.

Refs #794

Made-with: Cursor
@ivkond

ivkond commented Apr 19, 2026

Copy link
Copy Markdown
Contributor Author

@magyargergo when you have a moment, could you take a look at this one?

It closes #794 (cross-repo impact analysis) and is the final piece of that workstream — impact / query / context now accept repo: "@<groupName>" and group state moves to gitnexus://group/{name}/{contracts,status} resources. Just merged latest main, so it should apply cleanly. Thanks!

@github-actions

github-actions Bot commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

CI Report

All checks passed

Pipeline Status

Stage Status Details
✅ Typecheck success tsc --noEmit
✅ Tests success unit tests, 3 platforms
✅ E2E success gitnexus-web changes only

Test Results

Tests Passed Failed Skipped Duration
6981 6883 0 98 625s

✅ All 6883 tests passed

98 test(s) skipped — expand for details
  • withTestLbugDB(lock-retry) > withLbugDb retry behaviour > throws after max retry attempts
  • Swift MethodExtractor > isTypeDeclaration > recognizes class_declaration
  • Swift MethodExtractor > isTypeDeclaration > recognizes protocol_declaration
  • Swift MethodExtractor > isTypeDeclaration > rejects import_declaration
  • Swift MethodExtractor > visibility > extracts public method
  • Swift MethodExtractor > visibility > extracts private method
  • Swift MethodExtractor > visibility > defaults to internal when no modifier
  • Swift MethodExtractor > protocol methods > marks protocol method as abstract
  • Swift MethodExtractor > static and class methods > detects static func as isStatic
  • Swift MethodExtractor > static and class methods > detects class func as isStatic
  • Swift MethodExtractor > parameters > extracts parameters with types and default values
  • Swift MethodExtractor > return type > extracts return type from -> annotation
  • Swift MethodExtractor > annotations > extracts @objc attribute
  • Swift MethodExtractor > isFinal > detects final func
  • Swift MethodExtractor > isFinal > is false when not final
  • Swift MethodExtractor > isAsync > detects async func
  • Swift MethodExtractor > isOverride > detects override method
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift explicit init inference uses lookupClassByName
  • buildTypeEnv > constructor inference (Tier 1 fallback) > lookupClassByName regression coverage > Swift lookupClassByName regression coverage > Swift cross-file constructor inference does not bind plain functions
  • buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature
  • Swift constructor-inferred type resolution > detects User and Repo classes, both with save methods
  • Swift constructor-inferred type resolution > resolves user.save() to Models/User.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > resolves repo.save() to Models/Repo.swift via constructor-inferred type
  • Swift constructor-inferred type resolution > emits exactly 2 save() CALLS edges (one per receiver type)
  • Swift self resolution > detects User and Repo classes, each with a save function
  • Swift self resolution > resolves self.save() inside User.process to User.save, not Repo.save
  • Swift parent resolution > detects BaseModel and User classes plus Serializable protocol
  • Swift parent resolution > emits EXTENDS edge: User → BaseModel
  • Swift parent resolution > emits IMPLEMENTS edge: User → Serializable (protocol conformance)
  • Swift cross-file User.init() inference > resolves user.save() via User.init(name:) inference
  • Swift cross-file User.init() inference > resolves user.greet() via User.init(name:) inference
  • Swift return type inference > detects User class and getUser function
  • Swift return type inference > detects save function on User (Swift class methods are Function nodes)
  • Swift return type inference > resolves user.save() to User#save via return type of getUser() -> User
  • Swift return-type inference via function return type > resolves user.save() to User#save via return type of getUser()
  • Swift return-type inference via function return type > user.save() does NOT resolve to Repo#save
  • Swift return-type inference via function return type > resolves repo.save() to Repo#save via return type of getRepo()
  • Swift implicit imports (cross-file visibility) > detects UserService class in Models.swift
  • Swift implicit imports (cross-file visibility) > resolves UserService() constructor call across files (no explicit import)
  • Swift implicit imports (cross-file visibility) > resolves service.fetchUser() member call across files
  • Swift implicit imports (cross-file visibility) > creates IMPORTS edges between files in the same module
  • Swift extension deduplication > detects Product class
  • Swift extension deduplication > resolves Product() constructor despite extension creating duplicate class node
  • Swift extension deduplication > resolves product.save() to Product.swift (primary definition)
  • Swift constructor call fallback (no new keyword) > resolves OCRService() as constructor call across files
  • Swift constructor call fallback (no new keyword) > resolves ocr.recognize() member call via constructor-inferred type
  • Swift export visibility (internal vs private) > resolves PublicService() constructor across files
  • Swift export visibility (internal vs private) > resolves internalHelper() across files (internal = module-scoped)
  • Swift if let / guard let binding resolution > detects User and Repo classes
  • Swift if let / guard let binding resolution > resolves user.save() inside if-let to User#save
  • Swift if let / guard let binding resolution > resolves repo.save() inside guard-let to Repo#save
  • Swift if let / guard let binding resolution > user.save() in if-let does NOT resolve to Repo#save
  • Swift await / try expression unwrapping > resolves user.save() via await fetchUser() return type
  • Swift await / try expression unwrapping > resolves repo.save() via try parseRepo() return type
  • Swift await / try expression unwrapping > detects fetchUser and parseRepo as functions
  • Swift for-in loop element type inference > detects User and Repo classes
  • Swift for-in loop element type inference > creates implicit import edges between files
  • Swift field-type resolution > detects classes and their properties
  • Swift field-type resolution > emits HAS_PROPERTY edges from class to field
  • Swift field-type resolution > resolves field-chain call user.address.save() → Address#save
  • Swift field-type resolution > emits ACCESSES edges for field reads in chains
  • Swift field-type resolution > populates field metadata (visibility, declaredType) on Property nodes
  • Swift call-result binding > resolves call-result-bound method call user.save() → User#save
  • Swift call-result binding > getUser() is present as a defined function
  • Swift call-result binding > emits processUser -> getUser CALLS edge for let-assigned free function call
  • Swift method enrichment > detects Animal protocol and Dog class
  • Swift method enrichment > emits IMPLEMENTS edge Dog -> Animal
  • Swift method enrichment > emits HAS_METHOD edges for Dog methods
  • Swift method enrichment > marks protocol Animal.speak as isAbstract
  • Swift method enrichment > marks Dog.speak as NOT isAbstract
  • Swift method enrichment > marks breathe as isFinal
  • Swift method enrichment > marks classify as isStatic
  • Swift method enrichment > captures @objc annotation on breathe
  • Swift method enrichment > populates parameterTypes for classify(_ name: String)
  • Swift method enrichment > records parameterCount for classify
  • Swift method enrichment > records returnType for speak
  • Swift method enrichment > resolves dog.speak() CALLS edge
  • Swift method enrichment > resolves Dog.classify("dog") CALLS edge
  • Swift abstract dispatch > detects Repository protocol and SqlRepository class
  • Swift abstract dispatch > emits IMPLEMENTS edge SqlRepository -> Repository
  • Swift abstract dispatch > emits HAS_METHOD edges for Repository.find and Repository.save
  • Swift abstract dispatch > emits HAS_METHOD edges for SqlRepository.find and SqlRepository.save
  • Swift abstract dispatch > marks base Repository.find as isAbstract
  • Swift abstract dispatch > marks base Repository.save as isAbstract
  • Swift abstract dispatch > marks concrete SqlRepository.find as NOT isAbstract
  • Swift abstract dispatch > resolves repo.find(id: 42) CALLS edge
  • Swift abstract dispatch > resolves repo.save(entity: user) CALLS edge
  • Swift abstract dispatch > populates parameterTypes for Repository.find
  • Swift abstract dispatch > populates parameterTypes for Repository.save
  • Swift abstract dispatch > records returnType for SqlRepository.find
  • Swift abstract dispatch > emits METHOD_IMPLEMENTS edges from SqlRepository methods → Repository protocol methods
  • Swift overloaded method disambiguation > detects 2 distinct find Method nodes on SqlRepository
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edges for both find overloads
  • Swift overloaded method disambiguation > emits METHOD_IMPLEMENTS edge for save
  • Swift overloaded method disambiguation > emits exactly 3 METHOD_IMPLEMENTS edges total
  • Swift Child extends Parent — inherited method resolution (SM-9) > detects Parent and Child classes
  • Swift Child extends Parent — inherited method resolution (SM-9) > resolves c.parentMethod() to Parent.parentMethod via first-wins MRO walk

Code Coverage

Tests

Metric Coverage Covered Base Delta Status
Statements 73.51% 18781/25547 73.67% 📉 -0.2 🔴 ██████████████░░░░░░
Branches 62.26% 11960/19208 62.5% 📉 -0.2 🔴 ████████████░░░░░░░░
Functions 78.72% 1809/2298 78.61% 📈 +0.1 🟢 ███████████████░░░░░
Lines 76.16% 17037/22368 76.26% 📉 -0.1 🔴 ███████████████░░░░░

📋 View full run · Generated by CI

Fixes the `quality / format` CI job. `npx prettier --check .` reported
29 files repo-wide; this commit reformats only the 9 files that this PR
authored/modified:

- gitnexus/src/cli/group.ts
- gitnexus/src/core/group/{cross-impact,service}.ts
- gitnexus/src/mcp/{tools,resources}.ts
- gitnexus/src/mcp/local/local-backend.ts
- gitnexus/test/unit/group/{cross-impact,service}.test.ts
- gitnexus/test/unit/mcp/group-repo-routing.test.ts

The remaining 20 prettier offenders are pre-existing on `main`
(`.claude-plugin/marketplace.json`, `.mcp.json`, several
`gitnexus-claude-plugin/**/*.json`, `gitnexus-cursor-integration/hooks/hooks.json`,
`gitnexus/test/unit/{ast-cache,embedder,lazy-action,pipeline-exports}.test.ts`,
`gitnexus/test/utils/hook-test-helpers.ts`, `gitnexus/tsconfig.test.json`,
`gitnexus-web/tsconfig.node.json`, `.github/FUNDING.yml`) and out of
scope for this PR.

Re-verified after format:
- `npx tsc --noEmit` (in gitnexus/) — clean.
- `npx vitest run --pool=forks` over the #794 verification block —
  9 files / 111 tests passed.

Refs #794

Made-with: Cursor
@magyargergo

Copy link
Copy Markdown
Collaborator

@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review.

You are reviewing a PR in the GitNexus monorepo:

  • gitnexus/ → CLI + MCP
  • gitnexus-web/
  • gitnexus-shared/

Your task has 2 phases, in this exact order:

PHASE 1 — DEFINE THE BAR
Before reviewing the diff, establish a concise repo-specific definition of “production-ready” for GitNexus, based only on the repo docs and the affected area.
Keep this definition practical and reviewable. Do not invent standards that are not grounded in the repo.

PHASE 2 — REVIEW THE PR AGAINST THAT BAR
Review the actual diff only after defining the bar.
Stay tightly scoped to the changed code and its direct consequences.


CONTEXT TO LOAD FIRST
Read these before reviewing:

  • AGENTS.md
  • GUARDRAILS.md
  • CONTRIBUTING.md
  • TESTING.md
  • ARCHITECTURE.md

Additional context:


PRIMARY OBJECTIVE
Decide whether this PR is safe, correct, maintainable, and operationally acceptable to merge into production for GitNexus.

Do not optimize for completeness at the expense of signal.
Do not pad the review.
Do not propose unrelated refactors.
Do not restate the PR description unless needed for verification.


REVIEW RULES

  • Every finding must be grounded in specific evidence from the diff or directly relevant surrounding code.
  • Every finding must include path:line.
  • If you make a behavioral claim, cite the code that proves it.
  • If you make a performance claim, explain the mechanism.
  • If something cannot be verified from the diff alone, explicitly say so.
  • Distinguish clearly between:
    • verified issue
    • plausible risk
    • unverified concern
  • Avoid vague wording like “might be better” or “could be improved” unless you explain exactly why.
  • Keep the review focused on this PR’s scope only.

For each finding, assign one severity:

  • BLOCKING → must be fixed before merge
  • NON-BLOCKING → valid issue, but merge may still be acceptable
  • NIT → stylistic/minor, not merge-relevant

REPO-SPECIFIC REVIEW CHECKLIST
Use these exact headings.

1. Correctness & functional completeness

Check:

  • Does the implementation actually satisfy the PR claim?
    • ManifestExtractor is truly invoked
    • config.links produces non-zero cross-links where expected
  • Resolver contracts are preserved:
    • resolveSymbol remains exact-match
    • label-scoped Cypher remains correct per contract type
    • flag any regression toward fuzzy or unscoped matching
  • Graph schema integrity is preserved:
    • no silent changes to node labels
    • no silent changes to edge types
    • no silent changes to ID generation (generateId)
  • Call out any missing wiring, partial integration, dead branch, or mismatch between tests and runtime behavior

2. Code clarity & clean code

Check:

  • naming quality
  • local cohesion
  • dead code
  • unnecessary abstraction
  • hidden control flow
  • confusing indirection
  • adherence to repo conventions:
    • direct imports from gitnexus-shared
    • no barrel re-export regressions
    • no // removed comments
    • no unused re-exports
  • no drive-by refactors outside stated scope per CONTRIBUTING.md and GUARDRAILS.md § Scope

3. Test coverage & change safety

Evaluate against TESTING.md:

  • Are there unit tests under gitnexus/test/unit/ covering the newly wired path?
  • Is there a regression guard for 0-link → N-link behavior?
  • Are assertions meaningful rather than tautological?
  • Are fixtures realistic for manifest inputs?
  • If memoization/cache was introduced, is there a test proving hit/miss behavior and correctness?
  • Is there evidence the expected validation path would pass for staged gitnexus/ files?
    • tsc --noEmit
    • vitest run --project default
      If not verifiable, say exactly what is missing.

4. Performance

Inspect for:

  • hot-path overhead in ingestion/group sync
  • excess allocations per manifest entry
  • redundant Cypher round-trips
  • missed batching or missed parallelism (Promise.all) where it materially matters
  • O(n²) or repeated lookup patterns on large repos
  • memoization tradeoffs:
    • correctness
    • invalidation
    • bounded vs unbounded memory growth
      Do not speculate casually; explain the mechanism and likely impact.

5. Operational risk

Check:

  • Windows/cross-platform safety:
    • stream lifecycle
    • FD/file handle lifecycle
    • path separator assumptions
    • anything resembling prior ENOTEMPTY-style lifecycle regressions
  • LadybugDB single-writer invariant is preserved
  • Embeddings preservation:
    • no silent breakage of --embeddings
    • .gitnexus/meta.json.stats.embeddings not silently zeroed by changed paths
  • MCP contracts remain compatible:
    • group_*
    • query
    • context
    • impact
    • detect_changes
    • rename
    • cypher
      Flag any schema or contract break without migration note
  • staleness behavior still triggers correctly (gitnexus/src/mcp/staleness.ts)
  • rollback safety:
    • can this PR be reverted safely without re-analyze?
    • if not, explain why

6. Maintainability

Check:

  • Does the change respect Pipeline Phase DAG boundaries?
  • Is cross-phase coupling being introduced?
  • Are manifest-extractor entry points discoverable from group/sync.ts?
  • Would the next contributor know where to extend this?
  • Is shared logic placed appropriately:
    • in gitnexus-shared/
    • or gitnexus/src/core/**/utils.ts
    • and not duplicated between CLI and web bridge (server/api.ts)?

NON-NEGOTIABLES TO VERIFY
From GUARDRAILS.md / CONTRIBUTING.md:

  • no secrets, tokens, or machine-specific paths committed
  • no blind find-and-replace renames
  • if a symbol rename occurred, rename MCP tool or equivalent careful review was used
  • upstream impact was considered for modified shared symbols
  • no HIGH/CRITICAL impact left unaddressed
  • PR title follows conventional commit style + [area] Short description
  • PolyForm Noncommercial license headers/metadata untouched

OUTPUT FORMAT

Production-ready bar for this PR

Give a concise repo-specific definition in 5–10 bullets.

Findings

For each finding, use this structure exactly:

[BLOCKING|NON-BLOCKING|NIT] Short title

  • Evidence: path:line, path:line
  • Why it matters: ...
  • Recommendation: ...
  • Confidence: High / Medium / Low

Only include findings that are real and supported.
If there are no findings in a section, say:

  • No issues found in this area based on the diff and reviewed context.

What I could not fully verify

List any behavior you could not prove from the diff, tests, or available context.

Final verdict

Choose exactly one:

  • ✅ Ready to merge — no blocking issues.
  • ⚠️ Merge after addressing blocking items.
  • ❌ Not ready — fundamental concerns.

If verdict is not ready, list the blocking items in priority order.


IMPORTANT REVIEW BEHAVIOR

  • Be strict on correctness and contracts.
  • Be conservative about operational risk.
  • Be explicit about uncertainty.
  • Do not suggest unrelated refactors.
  • Do not turn nits into blockers.
  • Do not reward intent; review actual implementation.

@github-actions

github-actions Bot commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 5m 4s —— View job


PR Review: feat/cross-repo impact analysis (#794)

Tasks:

  • Load repo context (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
  • Define production-ready bar (Phase 1)
  • Read diff and key changed files (Phase 2)
  • Evaluate all 6 review checklist areas
  • Write final verdict

Production-ready bar for this PR

Based on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md and the affected area (gitnexus/src/core/group/ + MCP layer):

  1. Resolver contracts preservedresolveSymbol remains exact-match; no regression toward fuzzy matching; Cypher queries remain label-scoped.
  2. MCP contract stability — existing impact, query, context, group_list, group_sync tools continue working for single-repo usage; removed tools produce descriptive errors, not silent failures; new resources are additive.
  3. Graph schema integrity — no silent changes to node labels, edge types, or generateId logic.
  4. Test coverage — unit tests under gitnexus/test/unit/ for all new paths; integration smoke tests; tsc --noEmit and full vitest suite green in CI.
  5. LadybugDB single-writer invariant — bridge DB opened read-only only; no new write paths that could race with analyze.
  6. No secrets, no machine paths, no blind renames — per GUARDRAILS.md non-negotiables.
  7. Documentation updated for changed CLI/MCP contracts — per CONTRIBUTING.md pre-PR checklist.
  8. Cross-platform safety — path handling must work on Windows (CI runs cross-platform per ci-tests.yml).
  9. Rollback safety — revert must not require schema migration or re-analyze.
  10. Embeddings not silently broken — no changes to analyze pipeline or embed paths.

Findings

1. Correctness & functional completeness

@ routing gate correctly intercepts before repo resolution (local-backend.ts:506-510). Confirmed: callTool checks p.repo.startsWith('@') before calling resolveRepo, so group mode never accidentally resolves a group name against the single-repo registry.

resolveAtGroupMemberRepoPath defaults to lexicographically first key (resolve-at-member.ts:30). For impact, the local walk runs against exactly one member repo; for query and context, all repos are iterated (no subgroup set). This asymmetry is documented in tools.ts descriptions but may surprise consumers who expect @myproduct to be symmetric across tools. Behavior is consistent with the RFC design. No correctness defect — flagging for awareness.

Bridge Cypher queries use named fields with positional fallback (cross-impact.ts:291-303): rowToNeighbor reads r.neighborRepo ?? r[0] ?? ''. The RETURN clause order matches the fallback index order (verified by reading both CY_NEIGHBORS queries). Correct.

UID-based dedup before fan-out (cross-impact.ts:486-487): seen.has(key) where key = neighborRepo + neighborUid + contractId prevents duplicate CrossRepoImpact entries. Correct.

Graph schema integrity: No changes to node labels, edge types, or generateId. Verified — this PR adds no pipeline phases, no graph writers. Clean.

  • No issues found in graph schema or resolver contracts.

2. Code clarity & clean code

NON-BLOCKING: repoInSubgroup is duplicated between modules

  • Evidence: cross-impact.ts:73-77, service.ts:76-81
  • Why it matters: The service.ts version adds an exact parameter; the cross-impact.ts version is a strict subset. Future edits to the logic risk diverging silently.
  • Recommendation: Extract to group-path-utils.ts (the module already owns fileMatchesServicePrefix and normalizeServicePrefix) and import in both places. Not merge-blocking given the current implementations are functionally correct.
  • Confidence: High

NON-BLOCKING: groupStatus dynamic-imports already-static built-ins

  • Evidence: service.ts:444-445const fsp = await import('node:fs/promises') and const pathMod = await import('node:path') inside the method body, while both are statically imported at the file top (import fsp from 'node:fs/promises' on line 6, import path from 'node:path' on line 7).
  • Why it matters: The dynamic imports are redundant; module caching prevents real overhead, but this is inconsistent with the rest of the file and confusing to readers. The static imports at the top are shadowed inside the method, which could cause a linter warning if no-shadow is enabled.
  • Recommendation: Remove the dynamic imports in groupStatus and use the file-level fsp / path bindings.
  • Confidence: High

NIT: _crossDepth tracing is opaque

  • Evidence: cross-impact.ts:319 discards crossDepth: _crossDepth with an underscore prefix; local-backend.ts:2891 still passes crossDepth into impactArgs. Validation in validateGroupImpactParams computes a crossDepthWarning but multi-hop traversal beyond depth 1 is silently a no-op.
  • Why it matters: Callers who pass crossDepth > 1 receive a warning string but may not realize the parameter has zero effect on the fan-out. The constant MAX_SUPPORTED_CROSS_DEPTH = 1 at cross-impact.ts:25 makes this clear to source readers but not to API consumers.
  • Recommendation: Add the warning to the tool description in tools.ts (e.g., "Currently only depth 1 is supported; higher values are accepted and return a warning."). NIT severity — does not block.
  • Confidence: High

No drive-by refactors, no barrel re-export regressions, no // removed comments, no blind renames found.


3. Test coverage & change safety

CI: 6860/6860 tests passed, tsc --noEmit clean. Coverage: statements −0.2%, branches −0.3% from base. Slight delta is within acceptable range for a net-new feature.

New test files confirm:

  • cross-impact.test.ts — validates params, uid collection, service-prefix filtering, timeout path, service mismatch short-circuit.
  • group-repo-routing.test.ts — table-driven smoke for impact/query/context with @groupName and @groupName/memberPath; verifies service forwarding, empty-service rejection, unknown-group error, removed-tool error messages.
  • resources.test.ts — round-trip URI parsing for gitnexus://group/{name}/contracts (with query params) and gitnexus://group/{name}/status; dispatch to backend methods verified.

Missing regression guard: There is no test asserting that groupImpact with uids.length === 0 (local impact returned no symbols) skips the bridge DB entirely and returns cross_repo_hits: 0. The code path at cross-impact.ts:424-444 does this correctly, but the test suite doesn't exercise it with an explicit assertion on the bridge-not-opened path. This is a plausible risk, not a verified gap (the no-uid early return is covered implicitly by the service mock tests, but the bridge-open guard is not separately exercised).

Timeout double-budget: safeLocalImpact gets timeoutMs budget for Phase 1; Phase 2 fan-out also starts a fresh deadline = Date.now() + timeoutMs (cross-impact.ts:468). Total wall time can reach 2 × timeoutMs. No test asserts the total-duration ceiling. Not blocking, but worth noting for operational SLAs.


4. Performance

Sequential repo iteration in groupContext and groupQuery (service.ts:330, service.ts:388): Both methods use for...of loops over Object.entries(config.repos) with await inside. For a group with N repos, this is N sequential DB round-trips. Promise.all would parallelize these and is used in comparable patterns elsewhere in the codebase. For small groups (typical use case) this is acceptable; for groups with 10+ repos, latency adds up.

  • Severity: NON-BLOCKING. The sequential pattern is simple and safe; parallelism would require care around error isolation (already handled per-repo with try/catch). This is a known-acceptable trade-off for v1 of this feature.

collectImpactSymbolUids iterates byDepth with nested loops (cross-impact.ts:234-246): O(symbols) across all depth levels. Expected to be bounded by maxDepth × items_per_depth. No O(n²) risk.

Bridge query returns all neighbors in one round-trip: The two Cypher queries (CY_NEIGHBORS_UPSTREAM, CY_NEIGHBORS_DOWNSTREAM) fetch all matching rows at once before the per-neighbor fan-out loop. This is correct batching.

groupQuery RRF scoring uses per-repo rank only (service.ts:406): Score is 1 / (idx + 1 + 60) where idx is position within a single repo's results. This gives identical scores to rank-1 results from all repos (no cross-repo discrimination). For the initial implementation this is acceptable; a true merged-rank RRF would require a second pass.

  • Severity: NIT.

5. Operational risk

NON-BLOCKING: AGENTS.md and ARCHITECTURE.md document removed MCP tools

  • Evidence: AGENTS.md:110-113 still lists group_query, group_contracts, group_status in the Tools Quick Reference table. ARCHITECTURE.md:45-48 lists all three in the MCP Tools table.
  • Why it matters: CONTRIBUTING.md pre-PR checklist explicitly requires "Documentation updated if behavior or public CLI/MCP contract changes." These files are the canonical reference for AI agents working in this repo. Any agent that reads AGENTS.md and calls group_contracts or group_status will receive an unhandled throw (not a structured { error } response), which may propagate as a protocol-level error in the MCP server.
  • Recommendation: Update AGENTS.md and ARCHITECTURE.md to replace the three removed tool entries with the new @<groupName> routing pattern and the two new resource URIs. The migration table in the PR description is the right content; it should be reflected in the checked-in docs. Fix this →
  • Confidence: High

LadybugDB single-writer: Bridge DB is opened read-only via openBridgeDbReadOnly (cross-impact.ts:281). The ensureBridgeReady function uses closeBridgeDb in the finally block (cross-impact.ts:530-532). Single-writer invariant preserved. ✅

Windows/cross-platform: fileMatchesServicePrefix (group-path-utils.ts:12-19) uses filePath.startsWith(prefix + '/') with literal /. On Windows, filePath may use \ depending on the parser. The comment says "POSIX-style" but doesn't normalize. CI runs cross-platform (ci-tests.yml). If tree-sitter emits Windows-style paths on Windows, service prefix filtering silently returns no matches (all files excluded). The failing mode is graceful (empty results, not crash), so this is plausible risk rather than verified bug.

  • Severity: NON-BLOCKING.

Embeddings: This PR touches no analyze pipeline code, no lbug-adapter, no embedding paths. stats.embeddings cannot be silently zeroed by these changes. ✅

Staleness: groupStatus correctly calls checkStaleness (service.ts:455). staleness.ts is untouched. ✅

Rollback safety: No schema migrations, no database structure changes, no lockfile changes. Revert is safe and does not require re-analyze. ✅


6. Maintainability

Pipeline Phase DAG: This PR adds no new pipeline phase. The group/cross-impact machinery operates at query time (Phase 5/6 in the PR's own numbering, not the ingestion DAG). Pipeline boundary is preserved. ✅

Cross-phase coupling: None introduced. The GroupService and LocalBackend use the GroupToolPort interface for injection. The bridge DB is separate from the main lbug. ✅

Extension points: GroupService is the single place to extend group behavior; resources.ts parsing is cleanly separated from dispatch. The next contributor can find all group resource logic at readGroupContractsResource / readGroupStatusResource in local-backend.ts.

Shared logic placement: group-path-utils.ts is appropriate for service-prefix utilities. The duplicate repoInSubgroup (Finding 2 above) is the only placement concern.


Non-negotiables

  • ✅ No secrets, tokens, or machine paths committed
  • ✅ No blind find-and-replace renames (rename MCP not used; no symbol renames in this PR)
  • ✅ No HIGH/CRITICAL impact warnings surfaced by the diff
  • ✅ PR title follows conventional commit format (feat: …)
  • ⚠️ AGENTS.md and ARCHITECTURE.md not updated for removed MCP tools (CONTRIBUTING.md pre-PR requirement)
  • ✅ PolyForm Noncommercial license metadata untouched

What I could not fully verify

  • Bridge Cypher correctness end-to-end: I verified that the RETURN clause column order matches the rowToNeighbor positional fallback indices, but cannot execute the Cypher against a live bridge.lbug to confirm query semantics.
  • BRIDGE_SCHEMA_VERSION bump correctness: Whether the version constant was incremented from any prior value — bridge-schema.ts was not in the changed-files list, so the version is pre-existing.
  • checkStaleness behavior on non-git repos: The groupStatus method calls checkStaleness(repoObj.repoPath, meta.lastCommit) but checkStaleness is not in the diff. Cannot confirm edge-case behavior without reading the full implementation.
  • openBridgeDbReadOnly null return: ensureBridgeReady returns an error if openBridgeDbReadOnly returns null (cross-impact.ts:281-285), but the conditions under which it returns null (vs throwing) are internal to bridge-db.ts, which is not in the diff.
  • Integration test coverage for Phase 2 fan-out with a real bridge.lbug: The integration tests in group-impact.test.ts use mocked ports; end-to-end bridge traversal is not covered by the automated suite.

Final verdict

⚠️ Merge after addressing blocking items.

The implementation is sound, correctly wired, and well-tested for its scope. CI is clean. The single item to address before merge:

1. (Required per CONTRIBUTING.md) Update AGENTS.md and ARCHITECTURE.md to remove group_contracts, group_query, group_status from the tools tables and document the @<groupName> routing + new resource URIs in their place. The migration table in the PR body is the right content.

The NON-BLOCKING items (duplicate repoInSubgroup, dynamic imports in groupStatus, double-budget timeout) are safe to address in follow-up issues.

@magyargergo

Copy link
Copy Markdown
Collaborator

@ivkond can you please look into this?

ivkond added 2 commits April 19, 2026 17:30
- Extract `repoInSubgroup` to `group-path-utils.ts` so `cross-impact.ts`
  and `service.ts` share a single implementation; the file-prefix
  helpers now also accept Windows-style backslashes by normalizing to
  POSIX before comparison (NB#1, NB#5).
- `groupStatus` no longer redundantly `await import()`s `node:fs/promises`
  and `node:path` — the static imports at the top of `service.ts` are
  reused, removing the local-shadow risk (NB#2).
- `groupContext` and `groupQuery` now fan their per-repo work out via
  `Promise.all` instead of a sequential `for...of` await loop. Errors
  remain isolated per repo and the result order matches the filtered
  member list (NB#3).
- `runGroupImpact` now derives a single shared `deadline` from the
  caller-supplied `timeoutMs` before Phase 1, and Phase 2's fan-out
  reuses it instead of starting a fresh budget — total wall-clock can
  no longer exceed `timeoutMs` (NB#4).
- New unit suite `test/unit/group/group-path-utils.test.ts` covers the
  shared helpers, including Windows-path normalization.

Made-with: Cursor
…ting

The PR #984 reviewer flagged that both top-level docs still listed the
removed `group_query`, `group_contracts`, and `group_status` MCP tools.
Bring them in line with the actual MCP surface introduced by #794:

- `AGENTS.md` Tools Quick Reference now lists `query`/`context`/`impact`
  in group mode (`repo: "@<groupName>"`) and drops the three removed
  group_* tools. Adds the two new resources
  (`gitnexus://group/{name}/contracts` + `/status`) to the Resources
  table and a 1.5.0 changelog entry.
- `ARCHITECTURE.md` MCP tools table is trimmed to `group_list` and
  `group_sync`, with a paragraph + resource table describing the
  group-aware behaviour of `query`/`context`/`impact` and the bridge
  fan-out for cross-repo impact. "Where to change what" now points at
  `src/core/group/` for cross-repo work.

Made-with: Cursor
@magyargergo

Copy link
Copy Markdown
Collaborator

@ivkond please make sure the CI i heatlhy

Move the 5 `CREATE_FTS_INDEX` calls out of the analyze pipeline and into
lazy first-use initialisation, restoring CI headroom against the 30 s
e2e test budget.

Why
---
LadybugDB's `CREATE_FTS_INDEX` costs ~440 ms per call regardless of
table size — so on a 7-file mini-repo the 5 indexes (File, Function,
Class, Method, Interface) added ~2.2 s of fixed overhead, ~3× that on
slower Windows CI runners. This made `analyze` sit at ~18.7 s out of a
30 s budget on Windows: a normal runner spike was enough to push the
`analyze command runs pipeline on mini-repo` e2e test (cli-e2e.test.ts:175)
into a timeout flake (PR #984 run 24631480424).

How
---
* `lbug-adapter.ts`: add `ensureFTSIndex(table, indexName, props)` with
  an in-process `Set` cache (`ensuredFTSIndexes`). First call per
  `(table, indexName)` pays the LadybugDB cost; subsequent calls are a
  Set lookup. Cache is cleared by `closeLbug` so re-init starts fresh.
* `bm25-index.ts`: introduce a single `FTS_INDEXES` schema constant,
  call `ensureFTSIndex` (single-process) or `ensureFTSIndexViaExecutor`
  (MCP pool, with its own per-repo `ensuredPoolFTS` cache) before each
  query. Indexes that already exist on disk just re-register cheaply
  through the cache; failures are best-effort and fall back to `[]`.
* `run-analyze.ts`: drop the eager FTS phase block; replace with a
  comment explaining the new lazy strategy.
* `CHANGELOG.md`: add Performance entry under Unreleased.

Effect (mini-repo, Windows, warm runs)
--------------------------------------
| Run        | Before  | After   | Δ       |
|------------|---------|---------|---------|
| Cold       | 10.55 s | 4.37 s  | -58 %   |
| Warm       |  6.37 s | 4.00 s  | -37 %   |

CI projection (Windows, ~3× ratio): 18.7 s → ~11.7 s, doubling the
margin against the 30 s test budget from +60 % to +156 %.

Verification
------------
* `tsc --noEmit` clean.
* All 42 FTS-dependent integration tests pass:
  `search-core`, `search-pool`, `lbug-core-adapter`, `bm25-search`,
  `augmentation`.
* `cli-e2e.test.ts` `analyze` test passes locally; two unrelated
  pre-existing failures in `wiki`/`status` (outdated error-message
  expectations) reproduce on the prior commit and are out of scope.

Refs: #794, #984
Made-with: Cursor
@ivkond

ivkond commented Apr 19, 2026

Copy link
Copy Markdown
Contributor Author

@magyargergo, non blocking items addressed in c829f2a + ac69a08; out-of-scope perf follow-up in 5aef74c after the Windows CI flake.

Review (c829f2a + ac69a08)

  • NB#1/Embeddings pipeline #5repoInSubgroup consolidated into core/group/group-path-utils.ts (shared by cross-impact.ts + service.ts), normalises Windows backslashes to POSIX, covered by test/unit/group/group-path-utils.test.ts.
  • NB#2 — dropped redundant await import() in groupStatus; reuses static imports.
  • NB#3groupContext / groupQuery fan out per-repo via Promise.all, errors isolated, order preserved.
  • NB#4runGroupImpact derives one shared deadline from timeoutMs for both phases; total wall-clock can no longer exceed it.
  • Docs: AGENTS.md / ARCHITECTURE.md aligned with the actual @<group> MCP surface (query/context/impact + the two new resources; removed group_query / group_contracts / group_status).

Perf flake fix (5aef74c) — out of scope, happy to split

tests / windows-latest timed out on ac69a08 (run 24631480424). The analyze e2e test sat at ~18.7 s out of a 30 s budget — a normal runner spike pushed it over. Profiling pinned ~2.2 s of fixed CREATE_FTS_INDEX overhead in runFullAnalysis (5 calls × ~440 ms in LadybugDB regardless of table size).

Moved FTS creation to lazy first-use init via a new ensureFTSIndex(table, indexName, props) helper.

Why it's safe:

  • No public API removed — createFTSIndex still exported, just no longer called eagerly.
  • Idempotent: LadybugDB persists indexes to disk; ensureFTSIndex swallows already exists and an in-process Set cache makes it a no-op for the rest of the session. Cleared by closeLbug.
  • Same failure mode — both paths catch and let queryFTS fall back to [], like the old try { ... } catch {} block.
  • MCP pool uses a separate cache keyed by ${repoId}:${table}:${indexName} — no cross-repo bleed.
  • Cost moves, doesn't disappear: first query/context per session pays ~2.2 s once; net win because most sessions don't need FTS at all.

Effect (mini-repo, Windows warm): analyze 6.37 s → 4.00 s (-37 %). CI projection 18.7 s → ~11.7 s, budget margin +60 % → +156 %.

Verified: tsc --noEmit clean; all 42 FTS-dependent integration tests pass (search-core, search-pool, lbug-core-adapter, bm25-search, augmentation) — they exercise the new lazy path end-to-end. cli-e2e analyze passes locally; two unrelated pre-existing wiki/status failures reproduce on prior commits and are out of scope.

PTAL — and let me know if you'd prefer 5aef74c as a separate PR.

@magyargergo

Copy link
Copy Markdown
Collaborator

@claude Act as a senior reviewer for GitNexus. Your job is to determine whether this PR is production-ready for this repo, not to give a generic code review.

You are reviewing a PR in the GitNexus monorepo:

  • gitnexus/ → CLI + MCP
  • gitnexus-web/
  • gitnexus-shared/

Your task has 2 phases, in this exact order:

PHASE 1 — DEFINE THE BAR
Before reviewing the diff, establish a concise repo-specific definition of “production-ready” for GitNexus, based only on the repo docs and the affected area.
Keep this definition practical and reviewable. Do not invent standards that are not grounded in the repo.

PHASE 2 — REVIEW THE PR AGAINST THAT BAR
Review the actual diff only after defining the bar.
Stay tightly scoped to the changed code and its direct consequences.


CONTEXT TO LOAD FIRST
Read these before reviewing:

  • AGENTS.md
  • GUARDRAILS.md
  • CONTRIBUTING.md
  • TESTING.md
  • ARCHITECTURE.md

Additional context:


PRIMARY OBJECTIVE
Decide whether this PR is safe, correct, maintainable, and operationally acceptable to merge into production for GitNexus.

Do not optimize for completeness at the expense of signal.
Do not pad the review.
Do not propose unrelated refactors.
Do not restate the PR description unless needed for verification.


REVIEW RULES

  • Every finding must be grounded in specific evidence from the diff or directly relevant surrounding code.
  • Every finding must include path:line.
  • If you make a behavioral claim, cite the code that proves it.
  • If you make a performance claim, explain the mechanism.
  • If something cannot be verified from the diff alone, explicitly say so.
  • Distinguish clearly between:
    • verified issue
    • plausible risk
    • unverified concern
  • Avoid vague wording like “might be better” or “could be improved” unless you explain exactly why.
  • Keep the review focused on this PR’s scope only.

For each finding, assign one severity:

  • BLOCKING → must be fixed before merge
  • NON-BLOCKING → valid issue, but merge may still be acceptable
  • NIT → stylistic/minor, not merge-relevant

REPO-SPECIFIC REVIEW CHECKLIST
Use these exact headings.

1. Correctness & functional completeness

Check:

  • Does the implementation actually satisfy the PR claim?
    • ManifestExtractor is truly invoked
    • config.links produces non-zero cross-links where expected
  • Resolver contracts are preserved:
    • resolveSymbol remains exact-match
    • label-scoped Cypher remains correct per contract type
    • flag any regression toward fuzzy or unscoped matching
  • Graph schema integrity is preserved:
    • no silent changes to node labels
    • no silent changes to edge types
    • no silent changes to ID generation (generateId)
  • Call out any missing wiring, partial integration, dead branch, or mismatch between tests and runtime behavior

2. Code clarity & clean code

Check:

  • naming quality
  • local cohesion
  • dead code
  • unnecessary abstraction
  • hidden control flow
  • confusing indirection
  • adherence to repo conventions:
    • direct imports from gitnexus-shared
    • no barrel re-export regressions
    • no // removed comments
    • no unused re-exports
  • no drive-by refactors outside stated scope per CONTRIBUTING.md and GUARDRAILS.md § Scope

3. Test coverage & change safety

Evaluate against TESTING.md:

  • Are there unit tests under gitnexus/test/unit/ covering the newly wired path?
  • Is there a regression guard for 0-link → N-link behavior?
  • Are assertions meaningful rather than tautological?
  • Are fixtures realistic for manifest inputs?
  • If memoization/cache was introduced, is there a test proving hit/miss behavior and correctness?
  • Is there evidence the expected validation path would pass for staged gitnexus/ files?
    • tsc --noEmit
    • vitest run --project default
      If not verifiable, say exactly what is missing.

4. Performance

Inspect for:

  • hot-path overhead in ingestion/group sync
  • excess allocations per manifest entry
  • redundant Cypher round-trips
  • missed batching or missed parallelism (Promise.all) where it materially matters
  • O(n²) or repeated lookup patterns on large repos
  • memoization tradeoffs:
    • correctness
    • invalidation
    • bounded vs unbounded memory growth
      Do not speculate casually; explain the mechanism and likely impact.

5. Operational risk

Check:

  • Windows/cross-platform safety:
    • stream lifecycle
    • FD/file handle lifecycle
    • path separator assumptions
    • anything resembling prior ENOTEMPTY-style lifecycle regressions
  • LadybugDB single-writer invariant is preserved
  • Embeddings preservation:
    • no silent breakage of --embeddings
    • .gitnexus/meta.json.stats.embeddings not silently zeroed by changed paths
  • MCP contracts remain compatible:
    • group_*
    • query
    • context
    • impact
    • detect_changes
    • rename
    • cypher
      Flag any schema or contract break without migration note
  • staleness behavior still triggers correctly (gitnexus/src/mcp/staleness.ts)
  • rollback safety:
    • can this PR be reverted safely without re-analyze?
    • if not, explain why

6. Maintainability

Check:

  • Does the change respect Pipeline Phase DAG boundaries?
  • Is cross-phase coupling being introduced?
  • Are manifest-extractor entry points discoverable from group/sync.ts?
  • Would the next contributor know where to extend this?
  • Is shared logic placed appropriately:
    • in gitnexus-shared/
    • or gitnexus/src/core/**/utils.ts
    • and not duplicated between CLI and web bridge (server/api.ts)?

NON-NEGOTIABLES TO VERIFY
From GUARDRAILS.md / CONTRIBUTING.md:

  • no secrets, tokens, or machine-specific paths committed
  • no blind find-and-replace renames
  • if a symbol rename occurred, rename MCP tool or equivalent careful review was used
  • upstream impact was considered for modified shared symbols
  • no HIGH/CRITICAL impact left unaddressed
  • PR title follows conventional commit style + [area] Short description
  • PolyForm Noncommercial license headers/metadata untouched

OUTPUT FORMAT

Production-ready bar for this PR

Give a concise repo-specific definition in 5–10 bullets.

Findings

For each finding, use this structure exactly:

[BLOCKING|NON-BLOCKING|NIT] Short title

  • Evidence: path:line, path:line
  • Why it matters: ...
  • Recommendation: ...
  • Confidence: High / Medium / Low

Only include findings that are real and supported.
If there are no findings in a section, say:

  • No issues found in this area based on the diff and reviewed context.

What I could not fully verify

List any behavior you could not prove from the diff, tests, or available context.

Final verdict

Choose exactly one:

  • ✅ Ready to merge — no blocking issues.
  • ⚠️ Merge after addressing blocking items.
  • ❌ Not ready — fundamental concerns.

If verdict is not ready, list the blocking items in priority order.


IMPORTANT REVIEW BEHAVIOR

  • Be strict on correctness and contracts.
  • Be conservative about operational risk.
  • Be explicit about uncertainty.
  • Do not suggest unrelated refactors.
  • Do not turn nits into blockers.
  • Do not reward intent; review actual implementation.

@github-actions

github-actions Bot commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Claude finished @magyargergo's task in 4m 32s —— View job


PR Review: feat/cross-repo impact analysis (#794)

Tasks:

  • Load repo context (AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md)
  • Define production-ready bar (Phase 1)
  • Read diff and key changed files (Phase 2)
  • Evaluate all 6 review checklist areas
  • Write final verdict

Production-ready bar for this PR

Based on AGENTS.md, GUARDRAILS.md, CONTRIBUTING.md, TESTING.md, ARCHITECTURE.md, and the affected area (gitnexus/src/core/group/, gitnexus/src/mcp/, gitnexus/src/core/search/):

  1. Resolver contracts preserved — Cypher queries remain label-scoped and use exact UID matching; no regression toward fuzzy or cross-label resolution.
  2. MCP contract stabilityimpact, query, context, group_list, group_sync continue working for single-repo; removed tools produce a descriptive error, not a silent failure; new resources are additive.
  3. Graph schema integrity — no silent changes to node labels, edge types, or generateId.
  4. Test coverage — unit tests under gitnexus/test/unit/ for all new paths; tsc --noEmit and full vitest suite green in CI.
  5. LadybugDB single-writer invariant — bridge DB opened read-only only; no new write paths that could race with analyze.
  6. No secrets, no machine paths, no blind renames — per GUARDRAILS.md.
  7. Documentation updated — AGENTS.md and ARCHITECTURE.md reflect the actual MCP surface (per CONTRIBUTING.md pre-PR checklist).
  8. Cross-platform safety — path handling works on Windows.
  9. Rollback safety — revert must not require schema migration or re-analyze.
  10. Previous review items addressed — all BLOCKING and NON-BLOCKING items from the first pass must be resolved before this PR can merge.

Findings

1. Correctness & functional completeness

@ routing gate (local-backend.ts:505-511): callTool correctly checks p.repo.startsWith('@') before resolveRepo, so group mode never accidentally resolves a group name against the single-repo registry. ✅

Cypher queries are label-scoped and exact-match (cross-impact.ts:34-60): Both CY_NEIGHBORS_UPSTREAM and CY_NEIGHBORS_DOWNSTREAM filter on provider.repo, provider.symbolUid IN $uids, and provider.role. No fuzzy or unscoped matching. ✅

rowToNeighbor positional fallback (cross-impact.ts:289-302): Fallback indices (r[0]r[6]) match the RETURN clause column order in both Cypher queries. Correct.

repoInSubgroup consolidation: Previously duplicated between cross-impact.ts and service.ts. Now consolidated in group-path-utils.ts:36 and imported in both. Previous NB#1 addressed. ✅

Shared deadline (cross-impact.ts:350): deadline = Date.now() + Math.max(0, timeoutMs) is computed once and used for both Phase 1 (via safeLocalImpact) and the Phase 2 loop. Total wall-clock cannot exceed timeoutMs. Previous NB#4 addressed. ✅

uids.length === 0 early return (cross-impact.ts:427-447): When local impact returns no symbols, ensureBridgeReady is never called and cross_repo_hits: 0 is returned. Behavior is correct. No test explicitly asserts the bridge-not-opened path — the early return is implicitly covered by mock-based tests, but the bridge-open guard is not isolated. This is an unverified gap, not a bug.

Graph schema integrity: No changes to node labels, edge types, or generateId. ✅

  • No blocking issues in correctness.

2. Code clarity & clean code

NON-BLOCKING: groupStatus still iterates repos sequentially

  • Evidence: service.ts:452-476for (const [repoPath, registryName] of Object.entries(config.repos)) { ... await this.port.resolveRepo(...) }
  • Why it matters: groupContext (service.ts:332) and groupQuery (service.ts:395) were parallelized via Promise.all in response to the prior review (NB#3). groupStatus was not. For a group with N repos this is N sequential DB round-trips + staleness checks. groupStatus is not on a hot path (it's a diagnostic resource, not called per-tool-use), so this is acceptable for v1 but inconsistent with the rest of the file.
  • Recommendation: Safe to leave as-is; consider Promise.all in a follow-up for consistency.
  • Confidence: High

NIT: _crossDepth is silently ignored — tool description doesn't say so

  • Evidence: cross-impact.ts:317 destructures crossDepth: _crossDepth; cross-impact.ts:29 defines MAX_SUPPORTED_CROSS_DEPTH = 1. The warning string is returned in the response payload, but callers who don't read it may keep sending crossDepth > 1 expecting multi-hop traversal.
  • Why it matters: The tools.ts description for impact does not mention the depth-1 limitation for group mode.
  • Recommendation: Add "Currently only crossDepth 1 is supported; higher values are accepted and return a warning field." to the impact tool description in tools.ts. Fix this →
  • Confidence: High

No // removed comments, no barrel re-export regressions, no drive-by refactors found.


3. Test coverage & change safety

CI (run 24631480424, prior to the review-fix commits): 6866/6866 tests passed, tsc --noEmit clean. 98 skipped tests are pre-existing Swift tests. Coverage delta: statements −0.4%, branches −0.4% — within acceptable range for net-new functionality.

New / modified test files confirm coverage of:

  • cross-impact.test.ts — param validation, service-prefix filtering, timeout path, bridge schema mismatch.
  • group-repo-routing.test.ts — table-driven smoke (impact/query/context with @groupName + service), service forwarding, unknown group/member, removed tool errors.
  • resources.test.ts — URI round-trip parsing for gitnexus://group/{name}/contracts and gitnexus://group/{name}/status, dispatch verification.
  • group-path-utils.test.ts — Windows backslash normalization, subgroup matching.
  • group-service-group-mode.test.ts — groupContext / groupQuery Promise.all fan-out with per-repo error isolation.

FTS lazy-init cache (bm25-index.ts:44): ensuredPoolFTS is a module-level Set keyed by ${repoId}:${table}:${indexName}. It is never cleared by closeLbug (which only clears ensuredFTSIndexes in lbug-adapter.ts:157 — the singleton path's cache). If the pool maintains an open connection to repoId and gitnexus analyze recreates the DB in the same MCP server process, the Set would still hold the old key and CREATE_FTS_INDEX would not be called for the new DB — BM25 queries would silently return [] until the MCP server process restarts. This is a plausible risk (concurrent MCP + analyze is discouraged by the single-writer invariant, which is the main guard here), but worth documenting. The 42 FTS integration tests (5aef74c) verify the lazy path against the fresh-DB case, not the post-analyze reconnect case.

  • Confidence: Medium (cannot verify pool reconnection behavior without pool-adapter.ts)

4. Performance

Promise.all for groupContext and groupQuery (service.ts:332, service.ts:395): Both now fan out concurrently. Errors are isolated per-repo. Previous NB#3 addressed. ✅

FTS lazy-init cost (5aef74c): analyze no longer pays the ~440 ms × 5 LadybugDB CREATE_FTS_INDEX overhead. Cost moves to first query/context call in a session. This is a net win for CI and small repos — the Windows e2e budget issue that triggered the change is resolved.

collectImpactSymbolUids complexity (cross-impact.ts:219-245): O(depth × items_per_depth), bounded by maxDepth × results_per_depth. No O(n²) risk.

Bridge query batching (cross-impact.ts:459-462): Single round-trip fetches all neighbor rows before per-neighbor fan-out. Correct.

groupStatus sequential loop: As noted in Finding 2 — not on a hot path, acceptable for v1.

  • No blocking performance concerns.

5. Operational risk

handleGroupTool throws for removed tools (local-backend.ts:2850-2852):

  • Evidence: default: throw new Error('Unknown group tool: ...') — throws, does not return { error: "..." }.
  • Why it matters: Throwing propagates to the MCP SDK's transport layer, which converts it to a JSON-RPC error response (not a tool result). This is a different error shape than the { error: "..." } return used by other tools. Tests verify rejects.toThrow() at group-repo-routing.test.ts:167-182, consistent with the implementation. The MCP SDK wraps the thrown error into a protocol-level error, which is acceptable — but clients that distinguish tool errors (returned as results) from protocol errors (thrown) will see different behavior for removed tools vs. never-existed tools.
  • Recommendation: This is a deliberate choice and consistent with "Unknown tool" behavior in other backends. Acceptable for v1 of the removal surface.
  • Confidence: High (behavior is verified and documented in tests; MCP SDK handles the throw)
  • Severity: NIT

Windows path safety (group-path-utils.ts:10-12): toPosix() normalizes \ to / before all prefix comparisons. Applied in normalizeServicePrefix, fileMatchesServicePrefix, and repoInSubgroup. Previous NB#5 addressed. ✅

LadybugDB single-writer: Bridge DB opened read-only via openBridgeDbReadOnly (cross-impact.ts:279). closeBridgeDb in finally block (cross-impact.ts:532). Invariant preserved. ✅

Embeddings: No analyze pipeline code touched. stats.embeddings cannot be silently zeroed. ✅

Staleness: groupStatus calls checkStaleness(repoObj.repoPath, meta.lastCommit) (service.ts:460). staleness.ts untouched. ✅

Rollback safety: No schema migrations, no DB structure changes, no lockfile changes. Revert is safe without re-analyze. ✅


6. Maintainability

Pipeline Phase DAG: Not violated. Group tools operate at query time (no ingestion phases added). ✅

Cross-phase coupling: None introduced. GroupService and LocalBackend use the GroupToolPort interface. Bridge DB is separate from main lbug. ✅

Extension points: GroupService is the single location for group behavior. Resource parsing is in resources.ts / LocalBackend (readGroupContractsResource / readGroupStatusResource). ✅

group-path-utils.ts correctly owns service-prefix and subgroup utilities; both cross-impact.ts and service.ts import from it. ✅


Non-negotiables

Check Status
No secrets, tokens, or machine paths committed
No blind find-and-replace renames
No HIGH/CRITICAL impact left unaddressed
PR title follows conventional commit format (feat: …)
PolyForm Noncommercial license metadata untouched
AGENTS.md updated for removed/changed MCP surface ✅ (ac69a08)
ARCHITECTURE.md updated for removed/changed MCP surface ✅ (ac69a08)

All previous review's BLOCKING item (doc update) and NON-BLOCKING items (NB#1–#5) have been addressed in c829f2a + ac69a08. The 5aef74c perf commit is in-scope because it was required to fix the Windows CI timeout regression introduced by this PR's group-mode changes.


What I could not fully verify

  • CI for post-review commits (c829f2a, ac69a08, 5aef74c): CI is currently in progress (run/24637816347). The earlier run (24631480424) was clean; these commits address review feedback and an unrelated FTS timing issue, so breakage is low probability — but green CI should be confirmed before merge.
  • Pool reconnect behavior after analyze: Cannot verify whether pool-adapter.ts clears or reinitializes the connection when the underlying DB is recreated, which affects whether the ensuredPoolFTS stale-cache scenario is reachable.
  • Bridge Cypher semantics end-to-end: Verified column order matches rowToNeighbor fallback indices, but cannot execute against a live bridge.lbug to confirm query plan correctness.
  • openBridgeDbReadOnly null conditions: The conditions under which it returns null vs. throws are internal to bridge-db.ts, which is not in this diff.

Final verdict

Ready to merge — no blocking issues, contingent on CI passing for the latest commits.

The previous review's single BLOCKING item (doc update required by CONTRIBUTING.md) was addressed in ac69a08. All five NON-BLOCKING items were also resolved. The implementation is correct, well-tested, and operationally conservative. The remaining findings are either NITs or unverifiable without pool internals.

Confirm before merging:

  1. CI passes for branch HEAD (5aef74c) — currently in progress.

View CI run (in progress) ·

@magyargergo

Copy link
Copy Markdown
Collaborator

@ivkond before I merge your PR in, please address this comment and also remove the other docs from you changes. 🙏

image

Addresses @magyargergo's review feedback on PR #984: these superpowers
planning artefacts are internal to our workflow and should not live in
the public repo.

Removed:
- docs/superpowers/plans/2026-04-16-issue-794-implementation.md
- docs/superpowers/specs/2026-03-31-cross-index-impact-design.md

Made-with: Cursor
@magyargergo

Copy link
Copy Markdown
Collaborator

Thanks for getting this through and for your patience!

Feel free to join to our community on Discord!

@magyargergo magyargergo merged commit 0096663 into abhigyanpatwari:main Apr 20, 2026
13 of 14 checks passed
@ivkond ivkond deleted the feat/issue-794-cross-repo-impact branch April 20, 2026 16:00
@magyargergo magyargergo mentioned this pull request Apr 24, 2026
4 tasks
github714801013 pushed a commit to github714801013/GitNexus that referenced this pull request Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[group] Cross-repo impact analysis, CLI and MCP tool (split 5/5 of #606)

2 participants