feat(cli): analyze --name <alias> + duplicate-name guard for the repo registry#955
Conversation
… registry
The global registry at ~/.gitnexus/registry.json keys indexed repos by
`name`, derived from path.basename(). Two different projects whose
top-level folders share a basename (e.g. both have an `app/` dir) both
get `"name": "app"`, which makes:
- `-r app` on impact/context/query ambiguous (picks whichever the
resolver hits first)
- the `Available: app, backend, website` error hint actively
misleading
- `gitnexus list` output identical for two different repos
Issue abhigyanpatwari#829 (from doc-l2) has clean reproduction steps and a four-item
wish list. This PR addresses the three-and-a-half items that have
meaningful code surface; item 3 (accept absolute paths on -r) was
already functionally supported in resolveRepoFromCache and is now
locked in by the new test coverage below.
Changes:
* repo-manager.ts: extend registerRepo with optional
`{ name, force }` options, a new RegistryNameCollisionError class,
and a precedence rule:
explicit --name > preserved existing alias > path.basename
Preservation means re-running `analyze` on a path without --name
keeps the alias set by an earlier --name run. The collision guard
only fires when the caller EXPLICITLY asks for a name that's taken
(via --name or a preserved alias) — basename collisions keep
registering silently, preserving behaviour for users who don't know
about --name yet.
* cli/index.ts: add `--name <alias>` option to `analyze`.
* cli/analyze.ts: thread `name` to runFullAnalysis as `registryName`,
catch RegistryNameCollisionError with an actionable error message
pointing at the conflicting path.
* core/run-analyze.ts: extend AnalyzeOptions.registryName, forward
to registerRepo({ name: registryName }).
* cli/list.ts: collision-aware header format. Unique-name entries
render identically; colliding entries gain ` (<path>)` suffix.
* mcp/local/local-backend.ts: the `Available: …` hint in resolveRepo
now annotates colliding names with their paths so the caller can
actually pick the right one. Applied to both not-found and
multiple-repos-no-param error branches.
Tests: 6 new it() blocks in test/unit/repo-manager.test.ts covering:
1. { name: 'alias' } stores the alias instead of basename
2. Re-register without name preserves an existing alias
3. Re-register with a different name overrides the previous alias
4. Collision with another path throws RegistryNameCollisionError
and leaves the registry unmodified
5. { force: true } allows the duplicate to coexist
6. Backward-compat: basename collisions without --name still
register silently (users unaware of --name see no break)
Each test isolates the registry via GITNEXUS_HOME pointing at a
per-test tmpdir, so tests don't touch the developer's real
~/.gitnexus/registry.json.
Backward compat:
- registry.json schema unchanged. `name` is still a string; aliased
entries look identical on disk to basename entries, only the
precedence logic distinguishes them at runtime. Old registries
load unchanged.
- registerRepo third parameter is optional. Existing call sites
(none outside run-analyze.ts) compile unchanged.
- Error-message and list-output changes are additive — only the
colliding-name case gains the path suffix, single-name cases are
byte-identical to pre-abhigyanpatwari#829.
- Users who never pass --name see zero behaviour change.
Scope declined for v1:
- Strict mode (reject all basename collisions without --force)
would break existing workflows. The issue's wording is ambiguous;
sticking with opt-in behaviour and test abhigyanpatwari#6 locks that in. Flip to
strict is one conditional change if reviewer prefers.
- --as as a second spelling of --name (mentioned in the issue).
- Rename path-hash suffix for list (full path is clearer for now).
Verification:
npx vitest run test/unit/repo-manager.test.ts -> 15 pass (9+6)
npx vitest run test/unit/calltool-dispatch.test.ts -> 65 pass
npm run test:unit -> 3784 pass
(2 pre-existing env failures in git-utils.test.ts unchanged)
npx tsc --noEmit -> clean
Closes abhigyanpatwari#829
|
@azizur100389 is attempting to deploy a commit to the NexusCore Team on Vercel. A member of the Team first needs to authorize it. |
CI Report✅ All checks passed Pipeline Status
Test Results
✅ All 6792 tests passed 97 test(s) skipped — expand for details
Code CoverageTests
📋 View full run · Generated by CI |
|
@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:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 4m 39s —— View job PR Review —
|
| Check | Status |
|---|---|
| No secrets / machine-specific paths | ✅ |
| No blind find-and-replace renames | ✅ |
| PR title follows conventional-commit | ✅ feat(cli): ... |
| PolyForm license metadata untouched | ✅ |
| HIGH/CRITICAL impact left unaddressed | N/A — no graph/schema changes |
What I could not fully verify
- End-to-end CLI behavior with
--force --name <collision>: the test suite doesn't cover therunFullAnalysis→registerRepopath with both options set; I'm relying on static code analysis to confirm the passthrough is absent. - Windows case-insensitive path normalization by
path.resolve: whether Windowspath.resolvenormalizes case such that the asymmetry in the collision guard is harmless in practice. - Behavior when a preserved-alias re-registration collides mid-session (user A aliased a repo, user B registers with the same alias on the same machine): correct per spec but the error message could mention the preserved-alias context to avoid user confusion.
Final verdict
Blocking item (priority order):
run-analyze.ts:322— addforce: options.forceto theregisterRepocall. The error messageanalyze.ts:321tells usersgitnexus analyze --forcebypasses the collision guard, but the flag is never forwarded. Users following the documented workaround will hit the same error again. Fix is one line.
The alias storage logic, collision guard, MCP improvements, backward-compat behavior, and test coverage are all correct and well-designed. The NIT on Error.name shadowing is worth addressing in a follow-up but is not merge-blocking.
Addresses the two findings from the @claude senior review on PR abhigyanpatwari#955. BLOCKING — forward --force to registerRepo: The CLI error handler told users to re-run with `gitnexus analyze --force` to bypass the name-collision guard, but the flag was never threaded through runFullAnalysis -> registerRepo. A user hitting the collision who followed the documented workaround would hit the same error again, because AnalyzeOptions.force only controlled pipeline re-indexing, not the registry guard. Fix: pass `force: options.force` alongside `name: options.registryName` in the registerRepo call at core/run-analyze.ts. The two flags are semantically different (re-index vs. allow-duplicate) but the CLI intentionally reuses one --force switch for both; the missing passthrough was the gap. NIT — RegistryNameCollisionError.name shadowed Error.prototype.name: The constructor parameter `public readonly name: string` created an instance property that shadowed the inherited `Error.name` class-name identifier. Downstream code doing `err.name === 'RegistryNameCollisionError'` would fail; the test at repo-manager.test.ts asserted `err.name === 'shared'` which locked in the wrong behaviour. Fix: rename the constructor parameter to `registryName` and expose it as `err.registryName`. Explicitly set `this.name` to the class name so `Error.prototype.name` semantics are preserved for type checks. Update analyze.ts catch block and the test assertion. Added a companion assertion that locks in `err.name === 'RegistryNameCollisionError'` so future regressions are caught. Verification: npx vitest run test/unit/repo-manager.test.ts -> 15 pass npx tsc --noEmit -> clean Not in scope for this commit: The reviewer suggested an integration-style test that exercises runFullAnalysis -> registerRepo end-to-end via a mocked registerRepo to lock in the force passthrough. Adding that would require mocking the ingestion pipeline stubs (git, LadybugDB, KuzuDB migration, FTS) which is a larger scaffolding lift than the 1-line bug fix. Left as a follow-up if demand surfaces.
|
Thanks for the review — both findings addressed in BLOCKING —
|
|
@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:
Your task has 2 phases, in this exact order: PHASE 1 — DEFINE THE BAR PHASE 2 — REVIEW THE PR AGAINST THAT BAR CONTEXT TO LOAD FIRST
Additional context:
PRIMARY OBJECTIVE Do not optimize for completeness at the expense of signal. REVIEW RULES
For each finding, assign one severity:
REPO-SPECIFIC REVIEW CHECKLIST 1. Correctness & functional completenessCheck:
2. Code clarity & clean codeCheck:
3. Test coverage & change safetyEvaluate against TESTING.md:
4. PerformanceInspect for:
5. Operational riskCheck:
6. MaintainabilityCheck:
NON-NEGOTIABLES TO VERIFY
OUTPUT FORMAT Production-ready bar for this PRGive a concise repo-specific definition in 5–10 bullets. FindingsFor each finding, use this structure exactly: [BLOCKING|NON-BLOCKING|NIT] Short title
Only include findings that are real and supported.
What I could not fully verifyList any behavior you could not prove from the diff, tests, or available context. Final verdictChoose exactly one:
If verdict is not ready, list the blocking items in priority order. IMPORTANT REVIEW BEHAVIOR
|
|
Claude finished @magyargergo's task in 2m 31s —— View job PR Review —
|
| Check | Status |
|---|---|
| No secrets / machine-specific paths | ✅ |
| No blind find-and-replace renames | ✅ |
| PR title follows conventional-commit | ✅ feat(cli): ... |
| PolyForm license metadata untouched | ✅ |
| HIGH/CRITICAL impact unaddressed | N/A — no graph/schema changes |
What I could not fully verify
- End-to-end
--forcebehavior from CLI throughrunFullAnalysis→registerRepo: static analysis confirms the passthrough is wired; no integration test exercises the full CLI path. --name <same-as-basename>aliasing behavior:hasCustomAliasreturnsfalsewhen the supplied--nameequals the directory basename, so the alias is not persisted across re-analyses. This may be intentional (Option B) but is an undocumented edge case.
Final verdict
✅ Ready to merge — no blocking issues.
The two previously flagged issues (BLOCKING: force not forwarded; NIT: Error.name shadowing) are correctly addressed in ded8b94. The NON-BLOCKING finding (--skills inadvertently bypasses the collision guard) is real but limited in impact and requires an unusual flag combination to trigger — acceptable as a follow-up.
|
@azizur100389 it's required to provide integration tests PRs like yours! Please add proper integration test |
…ce + fix --skills bypass Addresses two open items on PR abhigyanpatwari#955 raised by the senior review round 2 and by @magyargergo's follow-up comment: 1. magyargergo: "it's required to provide integration tests PRs like yours! Please add proper integration test." (taken on board — going forward every PR on this class of surface will ship with an integration test, not just unit tests.) 2. @claude review round 2 NON-BLOCKING: `--skills` inadvertently bypassed the collision guard because pipeline-force and registry-force were conflated. Integration test (test/integration/cli-e2e.test.ts): End-to-end spawn-based test exercises the full CLI -> runFullAnalysis -> registerRepo chain across four steps: 1. `gitnexus analyze --name shared` on repo A -> succeeds; registry entry created with name "shared" 2. `gitnexus analyze --name shared` on repo B -> exits 1 with "Registry name collision" / "already used" on stderr 3. `gitnexus analyze --name shared --force` on repo B -> succeeds; registry now has both entries under name "shared" (regression guard for the round-1 BLOCKING --force passthrough bug) 4. `gitnexus analyze --name shared --skills` (no --force) on repo C -> exits 1 with collision error; registry still has only A + B (regression guard for the round-2 --skills bypass) Each step inspects registry.json directly, so the test locks in both functional behavior AND the passthrough wiring. Isolated GITNEXUS_HOME per-run so the developer's real ~/.gitnexus is never touched. Two new helpers (runCliWithEnv, makeMiniRepoCopy) reuse existing CLI-spawn + mini-repo-fixture infrastructure. Outer budget: 6 minutes (4 x ~60s analyze + fixture setup). All steps honor the CI-timeout `status === null` tolerance pattern. --skills bypass fix (analyze.ts + run-analyze.ts): Pre-fix: analyze.ts passed `force: options?.force || options?.skills` to runFullAnalysis, and run-analyze.ts forwarded that OR'd bit to registerRepo. Consequence: `analyze --name taken --skills` silently bypassed the collision guard without the user passing --force. Fix: split the two concerns. AnalyzeOptions.force keeps pipeline-re-index semantics (still OR'd with --skills; skills needs a fresh pipelineResult). New AnalyzeOptions.registryForce is populated ONLY from the real --force flag; run-analyze.ts passes registryForce to registerRepo. Inline comments at both call sites document the split so future flag additions don't re-conflate. Verification: npx vitest run test/integration/cli-e2e.test.ts -t "analyze --name" -> 1 pass (70s, all 4 steps exercised against real CLI) npx vitest run test/unit/repo-manager.test.ts -> 15 pass npm run test:unit -> 3778 pass, 2 pre-existing git-utils env failures unchanged npx tsc --noEmit -> clean Pre-commit hook (lint-staged + prettier + tsc) -> clean
|
Thanks @magyargergo — you were right to push on this. I should have shipped the integration test with the original PR rather than proposing it as a follow-up. Going forward I'll default to shipping an integration test alongside any PR that touches CLI / MCP / storage surfaces; unit tests alone don't catch wiring bugs like the Commit
Verification: integration test passes (70s, 4 steps), 15 unit tests pass, 3778 full-suite pass, |
|
Please make sure the CI is healthy |
…anpatwari#955 CI) macOS and windows-latest CI failed on the new integration test because of platform-specific path resolution asymmetries: * macOS: os.tmpdir() returns /var/folders/... but spawnSync child processes resolve the symlink to /private/var/folders/... when reading process.cwd() — so registerRepo stores the realpath while the test's path.resolve(repoA) kept the symlink form. Expected: /var/folders/tb/.../collide-app Actual: /private/var/folders/tb/.../collide-app * Windows: os.tmpdir() can return the 8.3 short-name form (C:\Users\RUNNER~1\...) while the spawned CLI process sees the long form (C:\Users\runneradmin\...). Expected: C:\Users\RUNNER~1\...\collide-app Actual: C:\Users\runneradmin\...\collide-app * Ubuntu passed because /tmp is not symlinked on GitHub runners. Fix: introduce a local `canonical(p)` helper that calls fs.realpathSync(), which normalizes both macOS symlinks AND Windows 8.3 short names to the canonical form. Applied to all three path assertions in the integration test. No source changes — the fix is in the test harness only. Verification: npx vitest run test/integration/cli-e2e.test.ts -t "analyze --name" -> 1 pass locally (76s on Windows worktree, same platform that failed in CI before the fix) npx tsc --noEmit -> clean Pre-commit hook -> clean
|
Thanks @magyargergo — CI failure diagnosed and fix pushed in Root cause: platform-specific tmpdir path asymmetry in the new integration test:
Fix: canonicalise both sides of each path-equality assertion via Honest verification statement:
|
| // Registry-collision bypass — only the explicit --force flag. | ||
| // Keeping this separate from `force` above means --skills (and any | ||
| // future flag that triggers pipeline re-run) does NOT accidentally | ||
| // bypass the RegistryNameCollisionError guard. See #829 review | ||
| // round 2. | ||
| registryForce: options?.force, |
There was a problem hiding this comment.
I'm not sure if this is the right direction! Since, --force re-indexes the repo and users may not want to do that. I'd recommend using a different option from the --force.
…ate-name (abhigyanpatwari#955) Per review feedback on abhigyanpatwari#955: --force re-indexes the repo, which users may not want when they just need to register a duplicate alias. - New CLI flag: --allow-duplicate-name (registry bypass only, no re-index) - Renamed RegisterRepoOptions.force -> allowDuplicateName end-to-end - run-analyze.ts keeps options.force for pipeline re-index decisions; registry path now reads options.allowDuplicateName independently - Collision error hint updated to suggest --allow-duplicate-name - Integration test step 4 now probes --skills (not --force) as the "pipeline-rerun must not silently bypass the guard" case
|
@magyargergo — good catch, you're right. Pushed
End-to-end rename across the PR: Integration test update: step 3 previously called Local verification: Thanks for the review — the split reads much cleaner. |
| // list output this PR also ships). | ||
| const explicitName = opts?.name !== undefined || (existing && hasCustomAlias(existing)); | ||
| if (explicitName && !opts?.allowDuplicateName) { | ||
| const collision = entries.find( |
There was a problem hiding this comment.
| const collision = entries.find( | |
| const collision = entries.some( |
I think, it's more appropriate.
There was a problem hiding this comment.
Thanks — fair signal on the noise. I kept find rather than some on purpose: line 355 passes the matched entry's path into the RegistryNameCollisionError constructor so the user sees exactly which existing path already owns the alias (and can disambiguate with -r <path>). With some, we'd only get a boolean and would need a second pass over entries to build that error — two scans for one check.
Worth flagging that the informative "already used by /path/to/X" message is a deliberate feature of this PR, not incidental: issue #829 explicitly called out the misleading "Available: app, backend, website" hint as one of the bugs to fix, so losing "by which path" in the error would undermine the fix.
To address what I think was the real reviewer-signal (the variable name reading boolean-ish): in 553ee0e I renamed collision → collidingEntry. Same semantics, but the name now makes it obvious the value is the matched entry, not a yes/no, which is why .find() is the operator we want here.
| * the default basename-derived registry `name`. Persisted — subsequent | ||
| * re-analyses of the same path without `--name` preserve the alias. | ||
| */ | ||
| name?: string; |
There was a problem hiding this comment.
Do I understand correctly I can make multiple analysis on one repo and has multiple aliases with this option?
There was a problem hiding this comment.
Good question — no. One repo path always maps to exactly one registry entry, and therefore exactly one alias.
Concretely, registerRepo upserts by resolved path: entries.findIndex(...) on gitnexus/src/storage/repo-manager.ts:329 locates the existing entry for this path, and the write branch at gitnexus/src/storage/repo-manager.ts:368 either updates that slot in place or pushes a new one — never both. So running analyze --name Y on a repo that was previously --name X replaces X with Y rather than adding a second entry. That invariant is locked in by the re-registerRepo with a different name overrides the previous alias test at repo-manager.test.ts:196.
--allow-duplicate-name operates in the orthogonal direction: it permits two different paths to share one alias (rendering -r <alias> ambiguous between those two paths, at which point the user falls back to -r <path>, which already works).
In 553ee0e I expanded the JSDoc on this field to spell that scope out explicitly — "cross-path alias sharing only · one path → one entry always · --name Y on the same path overwrites --name X" — so the next reader doesn't have to reconstruct it from the code.
abhigyanpatwari#955 review) Address inline review comments from @magyargergo: 1. Rename `collision` -> `collidingEntry` in registerRepo's duplicate guard. Zero behaviour change; signals the value is the registry entry object (consumed as `collidingEntry.path` when constructing RegistryNameCollisionError), which is precisely why `.find()` is the right operator rather than `.some()`. 2. Expand the JSDoc on RegisterRepoOptions.allowDuplicateName to explicitly state the flag's scope: it permits TWO DIFFERENT repo paths to share one alias; one repo path always has exactly one registry entry / one alias (registerRepo upserts by resolved path). Re-analyzing the same path with `--name Y` overwrites a previous `--name X` — already locked in by the "re-registerRepo with a different name overrides the previous alias" unit test.
|
@claude does this PR resolves the issue ticket as it promises? |
|
Claude finished @magyargergo's task in 1m 24s —— View job Does this PR resolve issue #829?Tasks:
SummaryYes — the PR fully resolves the three problems described in issue #829. Here's a point-by-point verification against each bug and the stated fixes: Issue #829 — Three root problemsProblem 1: ✅ Fixed. The PR adds Problem 2: ✅ Fixed. Unique-name entries render byte-identically to before this PR. Problem 3: ✅ Fixed. Bonus item (already worked, regression-tested): ✅ Confirmed. Key design decisions — verified correct
Test coverage
VerdictThe PR delivers exactly what #829 asked for: a |
|
Thank you for this quality work! Please keep up your great contribution coming. :) |
|
Thanks, Gergő — really appreciated. Will send more! |
2 participants
Closes #829.
Problem
The global registry at `~/.gitnexus/registry.json` keys indexed repos by `name` derived from `path.basename()`. Two projects whose top-level folders share a basename (e.g. both have an `app/` dir) both get `"name": "app"`, which makes:
Reproduction from issue #829:
```bash
mkdir -p /tmp/gnx-a/app /tmp/gnx-b/app
cd /tmp/gnx-a/app && git init -q && echo "export const x = 1" > index.ts && git add . && git commit -qm init
npx gitnexus analyze
cd /tmp/gnx-b/app && git init -q && echo "export const y = 2" > index.ts && git add . && git commit -qm init
npx gitnexus analyze
npx gitnexus list # Both entries show as name: "app"
npx gitnexus impact x -r app # Which "app" does this target?
```
What's added
`analyze --name ` — override the registry `name` at index time. Persisted. Survives subsequent re-analyses of the same path without `--name` (preserved-alias precedence).
Duplicate-name guard — if another path already uses the requested name, `analyze` aborts with a `RegistryNameCollisionError` that names the conflicting path. The guard only fires when the user explicitly asks for a name that's taken (via `--name` or a preserved alias). Basename collisions still register silently so users unaware of `--name` see no behaviour change.
`--force` — allows intentional same-name coexistence. The colliding entries get disambiguated in list output + resolver errors (below) rather than at the storage layer.
Collision-aware error messages in `resolveRepo` (both "not-found" and "multiple-repos-no-param" branches). Two entries named `app` now produce:
```
Available: app (/tmp/gnx-a/app), app (/tmp/gnx-b/app), backend
```
instead of the pre-#829 misleading `Available: app, app, backend`.
Collision-aware `gitnexus list` output — colliding entries gain a `name (path)` header so they're visually distinct. Unique-name entries render byte-identically to pre-#829.
Item 3 from the issue — `-r` accepts absolute path
Already works in resolveRepoFromCache via `path.resolve(repoParam)` + exact match. This PR adds no new code for it, but the new tests exercise the case for regression coverage.
Ranking / precedence
When `registerRepo` resolves the final `name`:
Behaviour decision flagged for reviewer
The issue's wording "Reject duplicate names on registration unless `--force` or `--name` is supplied" is ambiguous between:
I went with Option B because Option A is a behaviour break. If you'd rather strict, it's a one-line conditional flip and one test update. Happy to follow reviewer preference.
Files changed
Tests
6 new unit tests in `repo-manager.test.ts`, each isolating the registry via `GITNEXUS_HOME` pointing at a per-test tmpdir:
Backward compat
Scope declined
Test plan