From a06c6e31144b339b0f7adffb7650aeb74d84affc Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 13:04:52 +0100 Subject: [PATCH 1/6] feat(cli): analyze --name + duplicate-name guard for the repo registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 ` 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 ` ()` 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-#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 #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 #829 --- gitnexus/src/cli/analyze.ts | 30 +++++- gitnexus/src/cli/index.ts | 5 + gitnexus/src/cli/list.ts | 13 ++- gitnexus/src/core/run-analyze.ts | 8 +- gitnexus/src/mcp/local/local-backend.ts | 20 +++- gitnexus/src/storage/repo-manager.ts | 98 ++++++++++++++++- gitnexus/test/unit/repo-manager.test.ts | 134 ++++++++++++++++++++++++ 7 files changed, 296 insertions(+), 12 deletions(-) diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 1e75ea6753..240f367aa9 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -13,7 +13,11 @@ import { execFileSync } from 'child_process'; import v8 from 'v8'; import cliProgress from 'cli-progress'; import { closeLbug } from '../core/lbug/lbug-adapter.js'; -import { getStoragePaths, getGlobalRegistryPath } from '../storage/repo-manager.js'; +import { + getStoragePaths, + getGlobalRegistryPath, + RegistryNameCollisionError, +} from '../storage/repo-manager.js'; import { getGitRoot, hasGitDir } from '../storage/git.js'; import { runFullAnalysis } from '../core/run-analyze.js'; import fs from 'fs/promises'; @@ -59,6 +63,13 @@ export interface AnalyzeOptions { noStats?: boolean; /** Index the folder even when no .git directory is present. */ skipGit?: boolean; + /** + * Override the default basename-derived registry `name` with a + * user-supplied alias (#829). Disambiguates repos whose paths share a + * basename. Persisted — subsequent re-analyses of the same path without + * `--name` preserve the alias. + */ + name?: string; } export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOptions) => { @@ -191,6 +202,7 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption skipGit: options?.skipGit, skipAgentsMd: options?.skipAgentsMd, noStats: options?.noStats, + registryName: options?.name, }, { onProgress: (_phase, percent, message) => { @@ -298,6 +310,22 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption bar.stop(); const msg = err.message || String(err); + + // Registry name-collision from --name (#829) — surface as an + // actionable error rather than a generic stack-trace. + if (err instanceof RegistryNameCollisionError) { + console.error(`\n Registry name collision:\n`); + console.error(` "${err.name}" is already used by "${err.existingPath}".\n`); + console.error(` Options:`); + console.error(` • Pick a different alias: gitnexus analyze --name `); + console.error( + ` • Force the duplicate: gitnexus analyze --force (leaves "-r ${err.name}" ambiguous)`, + ); + console.error(''); + process.exitCode = 1; + return; + } + console.error(`\n Analysis failed: ${msg}\n`); // Provide helpful guidance for known failure modes diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 02581ae478..093c2382c6 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -28,6 +28,11 @@ program .option('--skip-agents-md', 'Skip updating the gitnexus section in AGENTS.md and CLAUDE.md') .option('--no-stats', 'Omit volatile file/symbol counts from AGENTS.md and CLAUDE.md') .option('--skip-git', 'Index a folder without requiring a .git directory') + .option( + '--name ', + 'Register this repo under a custom name in ~/.gitnexus/registry.json ' + + '(disambiguates repos whose paths share a basename, e.g. two different .../app folders)', + ) .option('-v, --verbose', 'Enable verbose ingestion warnings (default: false)') .addHelpText( 'after', diff --git a/gitnexus/src/cli/list.ts b/gitnexus/src/cli/list.ts index 722c8ad596..5da9a86f0e 100644 --- a/gitnexus/src/cli/list.ts +++ b/gitnexus/src/cli/list.ts @@ -17,12 +17,23 @@ export const listCommand = async () => { console.log(`\n Indexed Repositories (${entries.length})\n`); + // Count occurrences of each name so colliding entries can be + // disambiguated in the header (#829). Unique-name entries render + // identically to pre-#829 output; only collisions gain a suffix. + const nameCounts = new Map(); + for (const e of entries) { + const key = e.name.toLowerCase(); + nameCounts.set(key, (nameCounts.get(key) ?? 0) + 1); + } + for (const entry of entries) { const indexedDate = new Date(entry.indexedAt).toLocaleString(); const stats = entry.stats || {}; const commitShort = entry.lastCommit?.slice(0, 7) || 'unknown'; + const hasCollision = (nameCounts.get(entry.name.toLowerCase()) ?? 0) > 1; + const header = hasCollision ? `${entry.name} (${entry.path})` : entry.name; - console.log(` ${entry.name}`); + console.log(` ${header}`); console.log(` Path: ${entry.path}`); console.log(` Indexed: ${indexedDate}`); console.log(` Commit: ${commitShort}`); diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 910624f5ff..69fda87c19 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -53,6 +53,12 @@ export interface AnalyzeOptions { skipAgentsMd?: boolean; /** Omit volatile symbol/relationship counts from AGENTS.md and CLAUDE.md. */ noStats?: boolean; + /** + * User-provided alias for the registry `name` (#829). When set, + * forwarded to `registerRepo` so the indexed repo is stored under + * this alias instead of the path-derived basename. + */ + registryName?: string; } export interface AnalyzeResult { @@ -313,7 +319,7 @@ export async function runFullAnalysis( }, }; await saveMeta(storagePath, meta); - await registerRepo(repoPath, meta); + await registerRepo(repoPath, meta, { name: options.registryName }); // Only attempt to update .gitignore when a .git directory is present. if (hasGitDir(repoPath)) { diff --git a/gitnexus/src/mcp/local/local-backend.ts b/gitnexus/src/mcp/local/local-backend.ts index fe63ee1a29..ca63ab001f 100644 --- a/gitnexus/src/mcp/local/local-backend.ts +++ b/gitnexus/src/mcp/local/local-backend.ts @@ -319,13 +319,25 @@ export class LocalBackend { if (this.repos.size === 0) { throw new Error('No indexed repositories. Run: gitnexus analyze'); } + + // Build a disambiguated "Available: …" list (#829). When two handles + // share a name, annotate each colliding label with its path so the + // caller can actually pick the right one. Single-name entries render + // identically to pre-#829 output. + const nameCounts = new Map(); + for (const h of this.repos.values()) { + const key = h.name.toLowerCase(); + nameCounts.set(key, (nameCounts.get(key) ?? 0) + 1); + } + const labels = [...this.repos.values()].map((h) => + (nameCounts.get(h.name.toLowerCase()) ?? 0) > 1 ? `${h.name} (${h.repoPath})` : h.name, + ); + if (repoParam) { - const names = [...this.repos.values()].map((h) => h.name); - throw new Error(`Repository "${repoParam}" not found. Available: ${names.join(', ')}`); + throw new Error(`Repository "${repoParam}" not found. Available: ${labels.join(', ')}`); } - const names = [...this.repos.values()].map((h) => h.name); throw new Error( - `Multiple repositories indexed. Specify which one with the "repo" parameter. Available: ${names.join(', ')}`, + `Multiple repositories indexed. Specify which one with the "repo" parameter. Available: ${labels.join(', ')}`, ); } diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index b233d44fb8..3cc756a6bc 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -244,21 +244,109 @@ const writeRegistry = async (entries: RegistryEntry[]): Promise => { await fs.writeFile(getGlobalRegistryPath(), JSON.stringify(entries, null, 2), 'utf-8'); }; +/** + * Options for {@link registerRepo}. All optional — callers without any + * disambiguation requirement can keep calling `registerRepo(path, meta)` + * unchanged. + */ +export interface RegisterRepoOptions { + /** + * User-provided alias from `analyze --name ` (#829). Overrides + * the default basename-derived registry `name`. Persisted — subsequent + * re-analyses of the same path without `--name` preserve the alias. + */ + name?: string; + /** + * Allow registration even when another path already uses this name. + * Required for intentional same-name coexistence (two different + * same-basename `app` repos, for instance) without passing `--name`. + */ + force?: boolean; +} + +/** + * Thrown by {@link registerRepo} when a requested name is already in + * use by a DIFFERENT path. The CLI layer surfaces this as an actionable + * error instead of relying on `.message` string-matching. + */ +export class RegistryNameCollisionError extends Error { + readonly kind = 'RegistryNameCollisionError' as const; + constructor( + public readonly name: string, + public readonly existingPath: string, + public readonly requestedPath: string, + ) { + super( + `Registry name "${name}" is already used by "${existingPath}".\n` + + `Pass --name to register "${requestedPath}" under a different name, ` + + `or --force to allow both paths under the same name (leaves -r ambiguous for these two).`, + ); + this.name = name; + } +} + +/** Returns true when a previously-registered entry's `name` differs from + * `path.basename(entry.path)` — i.e. a user explicitly aliased it via + * `analyze --name ` on a prior run. Used to preserve the alias + * across re-analyses that omit `--name`. */ +const hasCustomAlias = (entry: RegistryEntry): boolean => { + return entry.name !== path.basename(path.resolve(entry.path)); +}; + /** * Register (add or update) a repo in the global registry. * Called after `gitnexus analyze` completes. + * + * Name resolution precedence (#829): + * 1. explicit `opts.name` (from `analyze --name `) + * 2. preserved alias on an existing entry for this path + * 3. `path.basename(repoPath)` (the original default) + * + * Duplicate-name guard: if another path already uses the resolved + * `name`, throw {@link RegistryNameCollisionError} unless `opts.force` + * is set. The guard ONLY fires when the user explicitly passed a + * `name`; un-aliased basename collisions continue to register silently + * so existing users who don't know about `--name` see no behaviour + * change. */ -export const registerRepo = async (repoPath: string, meta: RepoMeta): Promise => { +export const registerRepo = async ( + repoPath: string, + meta: RepoMeta, + opts?: RegisterRepoOptions, +): Promise => { const resolved = path.resolve(repoPath); - const name = path.basename(resolved); const { storagePath } = getStoragePaths(resolved); const entries = await readRegistry(); - const existing = entries.findIndex((e) => { + const existingIdx = entries.findIndex((e) => { const a = path.resolve(e.path); const b = resolved; return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; }); + const existing = existingIdx >= 0 ? entries[existingIdx] : null; + + // Precedence: explicit --name > preserved alias > basename. + const name = + opts?.name ?? (existing && hasCustomAlias(existing) ? existing.name : path.basename(resolved)); + + // Duplicate-name guard: only fire when the user EXPLICITLY asked for + // this name (via opts.name or a preserved alias). Unqualified basename + // collisions are preserved for backward-compat — they still register, + // and the user sees the ambiguity at `-r` / `list` resolution time + // (which is already improved by the disambiguated error messages and + // list output this PR also ships). + const explicitName = opts?.name !== undefined || (existing && hasCustomAlias(existing)); + if (explicitName && !opts?.force) { + const collision = entries.find( + (e, i) => + i !== existingIdx && + e.name.toLowerCase() === name.toLowerCase() && + path.resolve(e.path) !== resolved, + ); + if (collision) { + throw new RegistryNameCollisionError(name, collision.path, resolved); + } + } const entry: RegistryEntry = { name, @@ -269,8 +357,8 @@ export const registerRepo = async (repoPath: string, meta: RepoMeta): Promise= 0) { - entries[existing] = entry; + if (existingIdx >= 0) { + entries[existingIdx] = entry; } else { entries.push(entry); } diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 08afceaff8..66d91b3b7e 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -13,6 +13,10 @@ import { getStoragePaths, readRegistry, loadCLIConfig, + registerRepo, + listRegisteredRepos, + RegistryNameCollisionError, + type RepoMeta, } from '../../src/storage/repo-manager.js'; import { createTempDir } from '../helpers/test-db.js'; @@ -133,3 +137,133 @@ describe('API key file permissions', () => { expect(source).toContain("process.platform !== 'win32'"); }); }); + +// ─── analyze --name + duplicate-name guard (#829) ──────────── +// +// Each test isolates the global registry by pointing GITNEXUS_HOME at a +// per-test tmpdir. `getGlobalDir()` honors that env var, so registerRepo +// writes/reads a sandboxed registry.json without touching the user's +// real ~/.gitnexus. + +describe('registerRepo name override + collision guard (#829)', () => { + let tmpHome: Awaited>; + let tmpRepoA: Awaited>; + let tmpRepoB: Awaited>; + let savedGitnexusHome: string | undefined; + + const meta: RepoMeta = { + repoPath: '', + lastCommit: 'abc1234', + indexedAt: '2026-04-18T12:00:00.000Z', + stats: { files: 1, nodes: 1 }, + }; + + beforeEach(async () => { + tmpHome = await createTempDir('gitnexus-registry-home-'); + tmpRepoA = await createTempDir('gitnexus-repo-a-'); + tmpRepoB = await createTempDir('gitnexus-repo-b-'); + savedGitnexusHome = process.env.GITNEXUS_HOME; + process.env.GITNEXUS_HOME = tmpHome.dbPath; + }); + + afterEach(async () => { + if (savedGitnexusHome === undefined) delete process.env.GITNEXUS_HOME; + else process.env.GITNEXUS_HOME = savedGitnexusHome; + await tmpHome.cleanup(); + await tmpRepoA.cleanup(); + await tmpRepoB.cleanup(); + }); + + it('registerRepo({ name: "alias" }) stores the alias instead of basename', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'custom-alias' }); + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(1); + expect(entries[0].name).toBe('custom-alias'); + expect(entries[0].name).not.toBe(path.basename(tmpRepoA.dbPath)); + }); + + it('re-registerRepo on same path without name preserves an existing alias', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'custom-alias' }); + // Second call with no opts should keep the alias, not revert to basename. + await registerRepo(tmpRepoA.dbPath, meta); + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(1); + expect(entries[0].name).toBe('custom-alias'); + }); + + it('re-registerRepo with a different name overrides the previous alias', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'old-alias' }); + await registerRepo(tmpRepoA.dbPath, meta, { name: 'new-alias' }); + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(1); + expect(entries[0].name).toBe('new-alias'); + }); + + it('registerRepo throws RegistryNameCollisionError when another path uses the name', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'shared' }); + + await expect(registerRepo(tmpRepoB.dbPath, meta, { name: 'shared' })).rejects.toBeInstanceOf( + RegistryNameCollisionError, + ); + + // And the colliding entry in the error carries enough info for the + // CLI layer to surface an actionable message without string-matching. + try { + await registerRepo(tmpRepoB.dbPath, meta, { name: 'shared' }); + } catch (e) { + expect(e).toBeInstanceOf(RegistryNameCollisionError); + const err = e as RegistryNameCollisionError; + expect(err.name).toBe('shared'); + expect(path.resolve(err.existingPath)).toBe(path.resolve(tmpRepoA.dbPath)); + expect(path.resolve(err.requestedPath)).toBe(path.resolve(tmpRepoB.dbPath)); + } + + // Registry still only has the first entry — the failed call didn't + // corrupt state. + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(1); + expect(entries[0].name).toBe('shared'); + }); + + it('registerRepo({ name, force: true }) allows the duplicate to coexist', async () => { + await registerRepo(tmpRepoA.dbPath, meta, { name: 'shared' }); + await registerRepo(tmpRepoB.dbPath, meta, { name: 'shared', force: true }); + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(2); + expect(entries.every((e) => e.name === 'shared')).toBe(true); + // Both paths are stored distinctly — the collision is surfaced to the + // user via resolveRepo / list output, not hidden at the storage layer. + const paths = entries.map((e) => path.resolve(e.path)).sort(); + expect(paths).toEqual([path.resolve(tmpRepoA.dbPath), path.resolve(tmpRepoB.dbPath)].sort()); + }); + + it('basename collisions without an explicit --name still register silently (backward-compat)', async () => { + // Create two sibling dirs whose basenames collide. Neither caller + // passes { name }, so the guard must NOT fire — this preserves the + // pre-#829 behaviour for users who don't know about --name yet. + const parentA = await createTempDir('gitnexus-collide-parent-a-'); + const parentB = await createTempDir('gitnexus-collide-parent-b-'); + const sharedBasename = 'app'; + const pathA = path.join(parentA.dbPath, sharedBasename); + const pathB = path.join(parentB.dbPath, sharedBasename); + await fs.mkdir(pathA, { recursive: true }); + await fs.mkdir(pathB, { recursive: true }); + + try { + await registerRepo(pathA, meta); + await registerRepo(pathB, meta); // must NOT throw + + const entries = await listRegisteredRepos(); + expect(entries).toHaveLength(2); + expect(entries[0].name).toBe(sharedBasename); + expect(entries[1].name).toBe(sharedBasename); + } finally { + await parentA.cleanup(); + await parentB.cleanup(); + } + }); +}); From ded8b94c844b771139fcf9b2d099560bbab6ccd7 Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 16:29:24 +0100 Subject: [PATCH 2/6] fix(cli): wire --force through to registerRepo + rename collision field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the two findings from the @claude senior review on PR #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. --- gitnexus/src/cli/analyze.ts | 4 ++-- gitnexus/src/core/run-analyze.ts | 10 +++++++++- gitnexus/src/storage/repo-manager.ts | 12 +++++++++--- gitnexus/test/unit/repo-manager.test.ts | 6 +++++- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 240f367aa9..4dee98215a 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -315,11 +315,11 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption // actionable error rather than a generic stack-trace. if (err instanceof RegistryNameCollisionError) { console.error(`\n Registry name collision:\n`); - console.error(` "${err.name}" is already used by "${err.existingPath}".\n`); + console.error(` "${err.registryName}" is already used by "${err.existingPath}".\n`); console.error(` Options:`); console.error(` • Pick a different alias: gitnexus analyze --name `); console.error( - ` • Force the duplicate: gitnexus analyze --force (leaves "-r ${err.name}" ambiguous)`, + ` • Force the duplicate: gitnexus analyze --force (leaves "-r ${err.registryName}" ambiguous)`, ); console.error(''); process.exitCode = 1; diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 69fda87c19..df6687131c 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -319,7 +319,15 @@ export async function runFullAnalysis( }, }; await saveMeta(storagePath, meta); - await registerRepo(repoPath, meta, { name: options.registryName }); + // Forward both `name` (the --name alias) and `force` (bypass the + // name-collision guard when the user explicitly opts in). The error + // message in analyze.ts instructs the user to re-run with --force, + // so the flag MUST reach registerRepo or the documented workaround + // would hit the same error again (#829 review feedback). + await registerRepo(repoPath, meta, { + name: options.registryName, + force: options.force, + }); // Only attempt to update .gitignore when a .git directory is present. if (hasGitDir(repoPath)) { diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 3cc756a6bc..aa9014b542 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -268,20 +268,26 @@ export interface RegisterRepoOptions { * Thrown by {@link registerRepo} when a requested name is already in * use by a DIFFERENT path. The CLI layer surfaces this as an actionable * error instead of relying on `.message` string-matching. + * + * The colliding alias is exposed as `err.registryName` (not `err.name`). + * `err.name` keeps its inherited `Error.prototype.name` semantics (the + * class name) so downstream code can do the usual `err.name === + * 'RegistryNameCollisionError'` checks; use the `kind` discriminant or + * `instanceof RegistryNameCollisionError` for type-safe narrowing. */ export class RegistryNameCollisionError extends Error { readonly kind = 'RegistryNameCollisionError' as const; constructor( - public readonly name: string, + public readonly registryName: string, public readonly existingPath: string, public readonly requestedPath: string, ) { super( - `Registry name "${name}" is already used by "${existingPath}".\n` + + `Registry name "${registryName}" is already used by "${existingPath}".\n` + `Pass --name to register "${requestedPath}" under a different name, ` + `or --force to allow both paths under the same name (leaves -r ambiguous for these two).`, ); - this.name = name; + this.name = 'RegistryNameCollisionError'; } } diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 66d91b3b7e..6562980581 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -216,7 +216,11 @@ describe('registerRepo name override + collision guard (#829)', () => { } catch (e) { expect(e).toBeInstanceOf(RegistryNameCollisionError); const err = e as RegistryNameCollisionError; - expect(err.name).toBe('shared'); + // err.registryName carries the colliding alias (exposed as its own + // field so err.name retains the inherited Error.prototype.name + // semantics for downstream `err.name === '…Error'` checks). + expect(err.registryName).toBe('shared'); + expect(err.name).toBe('RegistryNameCollisionError'); expect(path.resolve(err.existingPath)).toBe(path.resolve(tmpRepoA.dbPath)); expect(path.resolve(err.requestedPath)).toBe(path.resolve(tmpRepoB.dbPath)); } From 36d66259d6782d8cc7ac400630115903c820fda5 Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 18:37:52 +0100 Subject: [PATCH 3/6] test(cli): end-to-end integration coverage for analyze --name + --force + fix --skills bypass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two open items on PR #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 --- gitnexus/src/cli/analyze.ts | 9 ++ gitnexus/src/core/run-analyze.ts | 31 +++- gitnexus/test/integration/cli-e2e.test.ts | 176 ++++++++++++++++++++++ 3 files changed, 210 insertions(+), 6 deletions(-) diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 4dee98215a..3c528c2ad6 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -197,12 +197,21 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption const result = await runFullAnalysis( repoPath, { + // Pipeline re-index — OR'd with --skills because skill generation + // needs a fresh pipelineResult. This is intentional and has no + // bearing on the registry collision guard (see registryForce below). force: options?.force || options?.skills, embeddings: options?.embeddings, skipGit: options?.skipGit, skipAgentsMd: options?.skipAgentsMd, noStats: options?.noStats, registryName: options?.name, + // 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, }, { onProgress: (_phase, percent, message) => { diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index df6687131c..b2cbaef9ad 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -46,6 +46,12 @@ export interface AnalyzeCallbacks { } export interface AnalyzeOptions { + /** + * Force a full re-index of the pipeline. Callers may OR this with + * other flags that imply re-analysis (e.g. `--skills`), so the value + * here is the PIPELINE-force signal, NOT the registry-collision + * bypass. See `registryForce` below. + */ force?: boolean; embeddings?: boolean; skipGit?: boolean; @@ -59,6 +65,16 @@ export interface AnalyzeOptions { * this alias instead of the path-derived basename. */ registryName?: string; + /** + * Bypass the `RegistryNameCollisionError` guard and allow two paths + * to register under the same `name` (#829). Semantically distinct + * from `force` above — the pipeline may be force-re-indexed for + * many reasons (`--skills`, future flags), but registry-collision + * bypass requires the user to have explicitly passed `--force`. + * Conflating the two (as an earlier draft of this feature did) + * meant `--skills` silently bypassed the collision guard. + */ + registryForce?: boolean; } export interface AnalyzeResult { @@ -319,14 +335,17 @@ export async function runFullAnalysis( }, }; await saveMeta(storagePath, meta); - // Forward both `name` (the --name alias) and `force` (bypass the - // name-collision guard when the user explicitly opts in). The error - // message in analyze.ts instructs the user to re-run with --force, - // so the flag MUST reach registerRepo or the documented workaround - // would hit the same error again (#829 review feedback). + // Forward the --name alias AND the registry-specific force bit. + // Note: `options.registryForce` is distinct from `options.force` + // (the pipeline re-index signal). The CLI only sets registryForce + // when the user explicitly passes --force; --skills (which sets + // force for pipeline re-analysis) does NOT set registryForce, so + // --name + --skills without --force correctly hits the collision + // guard. See #829 review round 2 for the --skills bypass regression + // this split prevents. await registerRepo(repoPath, meta, { name: options.registryName, - force: options.force, + force: options.registryForce, }); // Only attempt to update .gitignore when a .git directory is present. diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index abdd436a4d..c711fe0c2b 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -110,6 +110,55 @@ function runCliRaw(extraArgs: string[], cwd: string, timeoutMs = 15000) { }); } +/** + * Like runCliRaw but accepts extra env vars. Used by tests that need to + * isolate the global registry via GITNEXUS_HOME so they don't touch the + * developer / CI agent's real ~/.gitnexus/registry.json (#829). + */ +function runCliWithEnv( + extraArgs: string[], + cwd: string, + extraEnv: Record, + timeoutMs = 15000, +) { + return spawnSync(process.execPath, ['--import', tsxImportUrl, cliEntry, ...extraArgs], { + cwd, + encoding: 'utf8', + timeout: timeoutMs, + stdio: ['pipe', 'pipe', 'pipe'], + env: { + ...process.env, + NODE_OPTIONS: `${process.env.NODE_OPTIONS || ''} --max-old-space-size=8192`.trim(), + ...extraEnv, + }, + }); +} + +/** + * Create a fresh git-initialised throwaway repo at `/` + * and return its path. Used for tests that need multiple repos whose + * basenames intentionally collide (#829 reproduction). + */ +function makeMiniRepoCopy(basename: string, prefix: string): string { + const parent = fs.mkdtempSync(path.join(os.tmpdir(), prefix)); + const repo = path.join(parent, basename); + fs.cpSync(FIXTURE_SRC, repo, { recursive: true }); + spawnSync('git', ['init'], { cwd: repo, stdio: 'pipe' }); + spawnSync('git', ['add', '-A'], { cwd: repo, stdio: 'pipe' }); + spawnSync('git', ['commit', '-m', 'initial commit'], { + cwd: repo, + stdio: 'pipe', + env: { + ...process.env, + GIT_AUTHOR_NAME: 'test', + GIT_AUTHOR_EMAIL: 'test@test', + GIT_COMMITTER_NAME: 'test', + GIT_COMMITTER_EMAIL: 'test@test', + }, + }); + return repo; +} + describe('CLI end-to-end', () => { it('status command exits cleanly', () => { const result = runCli('status', MINI_REPO); @@ -144,6 +193,133 @@ describe('CLI end-to-end', () => { expect(fs.statSync(gitnexusDir).isDirectory()).toBe(true); }); + // ─── analyze --name + --force (#829) ─────────────────────── + // + // End-to-end regression guard for the name-collision feature: + // 1. `analyze --name X` persists the alias to ~/.gitnexus/registry.json + // 2. A second `analyze --name X` on a DIFFERENT path is rejected with + // a collision error (exit code 1, "already used" in output) + // 3. A second `analyze --name X --force` bypasses the guard and both + // entries coexist in registry.json + // + // The third assertion is the regression guard for the blocking bug + // caught in the first round of review: `--force` was documented in the + // error hint but not forwarded to registerRepo, so following the + // documented workaround produced the same error. This test invokes + // the real CLI → runFullAnalysis → registerRepo chain, so the + // passthrough bug cannot slip back in silently. + describe('analyze --name and --force (#829)', () => { + it('--name alias stores; collision rejects; --force bypasses', () => { + // Isolate the global registry so this test never touches the + // developer's real ~/.gitnexus. + const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-')); + + // Two mini-repo copies whose basenames intentionally collide. + const repoA = makeMiniRepoCopy('collide-app', 'gn-collide-a-'); + const repoB = makeMiniRepoCopy('collide-app', 'gn-collide-b-'); + const parentA = path.dirname(repoA); + const parentB = path.dirname(repoB); + + try { + // Step 1: analyze repoA with --name shared → registry entry created. + const r1 = runCliWithEnv( + ['analyze', '--name', 'shared'], + repoA, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r1.status === null) return; // CI timeout tolerance + expect( + r1.status, + [`step 1 exited with ${r1.status}`, `stdout: ${r1.stdout}`, `stderr: ${r1.stderr}`].join( + '\n', + ), + ).toBe(0); + + const registryPath = path.join(gnHome, 'registry.json'); + const afterStep1 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(Array.isArray(afterStep1)).toBe(true); + expect(afterStep1).toHaveLength(1); + expect(afterStep1[0].name).toBe('shared'); + expect(path.resolve(afterStep1[0].path)).toBe(path.resolve(repoA)); + + // Step 2: analyze repoB with the SAME --name → collision error. + const r2 = runCliWithEnv( + ['analyze', '--name', 'shared'], + repoB, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r2.status === null) return; + expect(r2.status).toBe(1); + const r2Output = `${r2.stdout}${r2.stderr}`; + expect(r2Output).toMatch(/Registry name collision|already used/i); + + // Registry still has just the first entry — step 2 must not have + // silently added, overwritten, or corrupted anything. + const afterStep2 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterStep2).toHaveLength(1); + expect(path.resolve(afterStep2[0].path)).toBe(path.resolve(repoA)); + + // Step 3: REGRESSION GUARD for the missing --force passthrough bug. + // Same args as step 2 but with --force → must succeed, and the + // registry must now contain BOTH entries under name "shared". + const r3 = runCliWithEnv( + ['analyze', '--name', 'shared', '--force'], + repoB, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r3.status === null) return; + expect( + r3.status, + [ + `step 3 (--force bypass) exited with ${r3.status}`, + `stdout: ${r3.stdout}`, + `stderr: ${r3.stderr}`, + ].join('\n'), + ).toBe(0); + + const afterStep3 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterStep3).toHaveLength(2); + expect(afterStep3.every((e: { name: string }) => e.name === 'shared')).toBe(true); + const paths = afterStep3.map((e: { path: string }) => path.resolve(e.path)).sort(); + expect(paths).toEqual([path.resolve(repoA), path.resolve(repoB)].sort()); + + // Step 4: REGRESSION GUARD for the #955 review-round-2 finding. + // Create a THIRD repo with the same basename and try to register + // it via `--name shared --skills` — NO --force. This must still + // hit the collision guard even though --skills OR's with force at + // the pipeline level. If future refactors accidentally re-conflate + // pipeline-force and registry-force, this test fails. + const repoC = makeMiniRepoCopy('collide-app', 'gn-collide-c-'); + const parentC = path.dirname(repoC); + try { + const r4 = runCliWithEnv( + ['analyze', '--name', 'shared', '--skills'], + repoC, + { GITNEXUS_HOME: gnHome }, + 60000, + ); + if (r4.status === null) return; + expect(r4.status).toBe(1); + const r4Output = `${r4.stdout}${r4.stderr}`; + expect(r4Output).toMatch(/Registry name collision|already used/i); + + // Registry unchanged — still only A + B under "shared". + const afterStep4 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); + expect(afterStep4).toHaveLength(2); + } finally { + fs.rmSync(parentC, { recursive: true, force: true }); + } + } finally { + fs.rmSync(gnHome, { recursive: true, force: true }); + fs.rmSync(parentA, { recursive: true, force: true }); + fs.rmSync(parentB, { recursive: true, force: true }); + } + }, 360000); // 6-min outer budget (4 × ~60s analyze calls + fixture setup) + }); + describe('unhappy path', () => { it('exits with error when no command is given', () => { const result = runCliRaw([], MINI_REPO); From 76431bf263a642d68627683280f2f5736f9c476f Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 18:58:12 +0100 Subject: [PATCH 4/6] fix(test): canonicalize tmp paths for cross-platform equality (#955 CI) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- gitnexus/test/integration/cli-e2e.test.ts | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index c711fe0c2b..2623233076 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -209,6 +209,16 @@ describe('CLI end-to-end', () => { // the real CLI → runFullAnalysis → registerRepo chain, so the // passthrough bug cannot slip back in silently. describe('analyze --name and --force (#829)', () => { + // Canonicalize paths for cross-platform equality checks. On macOS + // os.tmpdir() returns /var/folders/... but spawnSync child processes + // see the symlink-resolved /private/var/folders/... form, so the + // registry stores the realpath. On 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\...). + // fs.realpathSync() canonicalizes both so assertions don't break on + // platform-specific path quirks. + const canonical = (p: string): string => fs.realpathSync(p); + it('--name alias stores; collision rejects; --force bypasses', () => { // Isolate the global registry so this test never touches the // developer's real ~/.gitnexus. @@ -241,7 +251,7 @@ describe('CLI end-to-end', () => { expect(Array.isArray(afterStep1)).toBe(true); expect(afterStep1).toHaveLength(1); expect(afterStep1[0].name).toBe('shared'); - expect(path.resolve(afterStep1[0].path)).toBe(path.resolve(repoA)); + expect(canonical(afterStep1[0].path)).toBe(canonical(repoA)); // Step 2: analyze repoB with the SAME --name → collision error. const r2 = runCliWithEnv( @@ -259,7 +269,7 @@ describe('CLI end-to-end', () => { // silently added, overwritten, or corrupted anything. const afterStep2 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); expect(afterStep2).toHaveLength(1); - expect(path.resolve(afterStep2[0].path)).toBe(path.resolve(repoA)); + expect(canonical(afterStep2[0].path)).toBe(canonical(repoA)); // Step 3: REGRESSION GUARD for the missing --force passthrough bug. // Same args as step 2 but with --force → must succeed, and the @@ -283,8 +293,8 @@ describe('CLI end-to-end', () => { const afterStep3 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); expect(afterStep3).toHaveLength(2); expect(afterStep3.every((e: { name: string }) => e.name === 'shared')).toBe(true); - const paths = afterStep3.map((e: { path: string }) => path.resolve(e.path)).sort(); - expect(paths).toEqual([path.resolve(repoA), path.resolve(repoB)].sort()); + const paths = afterStep3.map((e: { path: string }) => canonical(e.path)).sort(); + expect(paths).toEqual([canonical(repoA), canonical(repoB)].sort()); // Step 4: REGRESSION GUARD for the #955 review-round-2 finding. // Create a THIRD repo with the same basename and try to register From 915271cb4795f878c65fdf518c9b64f83101eb36 Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 19:42:15 +0100 Subject: [PATCH 5/6] refactor(cli): split --force from registry bypass; add --allow-duplicate-name (#955) Per review feedback on #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 --- gitnexus/src/cli/analyze.ts | 25 ++++--- gitnexus/src/cli/index.ts | 5 ++ gitnexus/src/core/run-analyze.ts | 30 ++++---- gitnexus/src/storage/repo-manager.ts | 18 +++-- gitnexus/test/integration/cli-e2e.test.ts | 91 +++++++++++++---------- gitnexus/test/unit/repo-manager.test.ts | 4 +- 6 files changed, 99 insertions(+), 74 deletions(-) diff --git a/gitnexus/src/cli/analyze.ts b/gitnexus/src/cli/analyze.ts index 3c528c2ad6..46cedc4349 100644 --- a/gitnexus/src/cli/analyze.ts +++ b/gitnexus/src/cli/analyze.ts @@ -70,6 +70,14 @@ export interface AnalyzeOptions { * `--name` preserve the alias. */ name?: string; + /** + * Allow registration even when another path already uses the same + * `--name` alias (#829). Intentionally a distinct flag from `--force` + * because the user may want to coexist under the same name WITHOUT + * paying the cost of a pipeline re-index. Maps to registerRepo's + * `allowDuplicateName` option end-to-end. + */ + allowDuplicateName?: boolean; } export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOptions) => { @@ -198,20 +206,19 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption repoPath, { // Pipeline re-index — OR'd with --skills because skill generation - // needs a fresh pipelineResult. This is intentional and has no - // bearing on the registry collision guard (see registryForce below). + // needs a fresh pipelineResult. Has no bearing on the registry + // collision guard (see allowDuplicateName below). force: options?.force || options?.skills, embeddings: options?.embeddings, skipGit: options?.skipGit, skipAgentsMd: options?.skipAgentsMd, noStats: options?.noStats, registryName: options?.name, - // 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, + // Registry-collision bypass — its own CLI flag, intentionally NOT + // overloading --force. A user who hits the collision guard should + // be able to accept the duplicate name without also paying the + // cost of a full pipeline re-index. See #829 review round 2. + allowDuplicateName: options?.allowDuplicateName, }, { onProgress: (_phase, percent, message) => { @@ -328,7 +335,7 @@ export const analyzeCommand = async (inputPath?: string, options?: AnalyzeOption console.error(` Options:`); console.error(` • Pick a different alias: gitnexus analyze --name `); console.error( - ` • Force the duplicate: gitnexus analyze --force (leaves "-r ${err.registryName}" ambiguous)`, + ` • Allow the duplicate: gitnexus analyze --allow-duplicate-name (leaves "-r ${err.registryName}" ambiguous)`, ); console.error(''); process.exitCode = 1; diff --git a/gitnexus/src/cli/index.ts b/gitnexus/src/cli/index.ts index 093c2382c6..dca5983e0c 100644 --- a/gitnexus/src/cli/index.ts +++ b/gitnexus/src/cli/index.ts @@ -33,6 +33,11 @@ program 'Register this repo under a custom name in ~/.gitnexus/registry.json ' + '(disambiguates repos whose paths share a basename, e.g. two different .../app folders)', ) + .option( + '--allow-duplicate-name', + 'Register this repo even if another path already uses the same --name alias. ' + + 'Leaves `-r ` ambiguous for the two paths; use -r to disambiguate.', + ) .option('-v, --verbose', 'Enable verbose ingestion warnings (default: false)') .addHelpText( 'after', diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index b2cbaef9ad..b17fb8e573 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -50,7 +50,7 @@ export interface AnalyzeOptions { * Force a full re-index of the pipeline. Callers may OR this with * other flags that imply re-analysis (e.g. `--skills`), so the value * here is the PIPELINE-force signal, NOT the registry-collision - * bypass. See `registryForce` below. + * bypass. See `allowDuplicateName` below. */ force?: boolean; embeddings?: boolean; @@ -67,14 +67,13 @@ export interface AnalyzeOptions { registryName?: string; /** * Bypass the `RegistryNameCollisionError` guard and allow two paths - * to register under the same `name` (#829). Semantically distinct - * from `force` above — the pipeline may be force-re-indexed for - * many reasons (`--skills`, future flags), but registry-collision - * bypass requires the user to have explicitly passed `--force`. - * Conflating the two (as an earlier draft of this feature did) - * meant `--skills` silently bypassed the collision guard. + * to register under the same `name` (#829). Controlled by the + * dedicated `--allow-duplicate-name` CLI flag, intentionally + * independent from `--force` — users who hit the collision guard + * should be able to accept the duplicate without paying the cost + * of a pipeline re-index. */ - registryForce?: boolean; + allowDuplicateName?: boolean; } export interface AnalyzeResult { @@ -335,17 +334,14 @@ export async function runFullAnalysis( }, }; await saveMeta(storagePath, meta); - // Forward the --name alias AND the registry-specific force bit. - // Note: `options.registryForce` is distinct from `options.force` - // (the pipeline re-index signal). The CLI only sets registryForce - // when the user explicitly passes --force; --skills (which sets - // force for pipeline re-analysis) does NOT set registryForce, so - // --name + --skills without --force correctly hits the collision - // guard. See #829 review round 2 for the --skills bypass regression - // this split prevents. + // Forward the --name alias and the registry-collision bypass bit. + // `allowDuplicateName` is its own concern — independent from the + // pipeline `force` above. The CLI maps it from + // `--allow-duplicate-name` only; `--force` and `--skills` both + // trigger pipeline re-run but never bypass the registry guard. await registerRepo(repoPath, meta, { name: options.registryName, - force: options.registryForce, + allowDuplicateName: options.allowDuplicateName, }); // Only attempt to update .gitignore when a .git directory is present. diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index aa9014b542..6dbaab3feb 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -257,11 +257,13 @@ export interface RegisterRepoOptions { */ name?: string; /** - * Allow registration even when another path already uses this name. - * Required for intentional same-name coexistence (two different - * same-basename `app` repos, for instance) without passing `--name`. + * Allow registration even when another path already uses this name + * (#829). Mapped from the `--allow-duplicate-name` CLI flag — + * intentionally a distinct flag from `--force` (which only triggers + * pipeline re-index) because a user accepting a duplicate name should + * not be forced to also re-run the full pipeline. */ - force?: boolean; + allowDuplicateName?: boolean; } /** @@ -285,7 +287,7 @@ export class RegistryNameCollisionError extends Error { super( `Registry name "${registryName}" is already used by "${existingPath}".\n` + `Pass --name to register "${requestedPath}" under a different name, ` + - `or --force to allow both paths under the same name (leaves -r ambiguous for these two).`, + `or --allow-duplicate-name to allow both paths under the same name (leaves -r ambiguous for these two).`, ); this.name = 'RegistryNameCollisionError'; } @@ -309,8 +311,8 @@ const hasCustomAlias = (entry: RegistryEntry): boolean => { * 3. `path.basename(repoPath)` (the original default) * * Duplicate-name guard: if another path already uses the resolved - * `name`, throw {@link RegistryNameCollisionError} unless `opts.force` - * is set. The guard ONLY fires when the user explicitly passed a + * `name`, throw {@link RegistryNameCollisionError} unless + * `opts.allowDuplicateName` is set. The guard ONLY fires when the user explicitly passed a * `name`; un-aliased basename collisions continue to register silently * so existing users who don't know about `--name` see no behaviour * change. @@ -342,7 +344,7 @@ export const registerRepo = async ( // (which is already improved by the disambiguated error messages and // list output this PR also ships). const explicitName = opts?.name !== undefined || (existing && hasCustomAlias(existing)); - if (explicitName && !opts?.force) { + if (explicitName && !opts?.allowDuplicateName) { const collision = entries.find( (e, i) => i !== existingIdx && diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index 2623233076..844e3978d6 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -193,33 +193,37 @@ describe('CLI end-to-end', () => { expect(fs.statSync(gitnexusDir).isDirectory()).toBe(true); }); - // ─── analyze --name + --force (#829) ─────────────────────── + // ─── analyze --name + --allow-duplicate-name (#829) ────── // // End-to-end regression guard for the name-collision feature: // 1. `analyze --name X` persists the alias to ~/.gitnexus/registry.json // 2. A second `analyze --name X` on a DIFFERENT path is rejected with // a collision error (exit code 1, "already used" in output) - // 3. A second `analyze --name X --force` bypasses the guard and both - // entries coexist in registry.json + // 3. `analyze --name X --allow-duplicate-name` bypasses the guard; + // both entries coexist in registry.json + // 4. Pipeline-re-index flags (e.g. --skills) WITHOUT + // --allow-duplicate-name must STILL hit the collision guard — + // the bypass must stay gated on its dedicated flag so it isn't + // silently triggered by unrelated pipeline signals + // (review round 2/3 design decision). // - // The third assertion is the regression guard for the blocking bug - // caught in the first round of review: `--force` was documented in the - // error hint but not forwarded to registerRepo, so following the - // documented workaround produced the same error. This test invokes - // the real CLI → runFullAnalysis → registerRepo chain, so the - // passthrough bug cannot slip back in silently. - describe('analyze --name and --force (#829)', () => { - // Canonicalize paths for cross-platform equality checks. On macOS - // os.tmpdir() returns /var/folders/... but spawnSync child processes - // see the symlink-resolved /private/var/folders/... form, so the - // registry stores the realpath. On 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\...). - // fs.realpathSync() canonicalizes both so assertions don't break on - // platform-specific path quirks. - const canonical = (p: string): string => fs.realpathSync(p); - - it('--name alias stores; collision rejects; --force bypasses', () => { + // This test invokes the real CLI → runFullAnalysis → registerRepo + // chain, so any wiring regression fails here. + describe('analyze --name and --allow-duplicate-name (#829)', () => { + // Path-equality assertions across CLI spawn boundaries are fragile + // cross-platform: + // - macOS: os.tmpdir() returns /var/folders/...; child processes + // resolve the symlink to /private/var/folders/... + // - Windows: os.tmpdir() on GitHub runners returns 8.3 short-name + // form (C:\Users\RUNNER~1\...); the child sees the long form + // (C:\Users\runneradmin\...). fs.realpathSync does NOT reliably + // expand 8.3 to long form. + // Rather than fight the platform-path quagmire, we assert STRUCTURAL + // properties: entry count, alias value, path basename, path + // distinctness. That covers the behavior this test is here to + // protect without depending on exact-string path equality. + + it('--name alias stores; collision rejects; --allow-duplicate-name bypasses', () => { // Isolate the global registry so this test never touches the // developer's real ~/.gitnexus. const gnHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-home-')); @@ -251,7 +255,7 @@ describe('CLI end-to-end', () => { expect(Array.isArray(afterStep1)).toBe(true); expect(afterStep1).toHaveLength(1); expect(afterStep1[0].name).toBe('shared'); - expect(canonical(afterStep1[0].path)).toBe(canonical(repoA)); + expect(path.basename(afterStep1[0].path)).toBe('collide-app'); // Step 2: analyze repoB with the SAME --name → collision error. const r2 = runCliWithEnv( @@ -269,13 +273,16 @@ describe('CLI end-to-end', () => { // silently added, overwritten, or corrupted anything. const afterStep2 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); expect(afterStep2).toHaveLength(1); - expect(canonical(afterStep2[0].path)).toBe(canonical(repoA)); - - // Step 3: REGRESSION GUARD for the missing --force passthrough bug. - // Same args as step 2 but with --force → must succeed, and the - // registry must now contain BOTH entries under name "shared". + // Registry still has only the step-1 entry — the failed call + // must not have silently added, overwritten, or corrupted state. + expect(afterStep2[0].path).toBe(afterStep1[0].path); + + // Step 3: REGRESSION GUARD for the missing collision-bypass wire + // (originally a --force passthrough bug; per review round 3 the + // bypass moved to its own --allow-duplicate-name flag to avoid + // conflating it with pipeline re-index). const r3 = runCliWithEnv( - ['analyze', '--name', 'shared', '--force'], + ['analyze', '--name', 'shared', '--allow-duplicate-name'], repoB, { GITNEXUS_HOME: gnHome }, 60000, @@ -284,7 +291,7 @@ describe('CLI end-to-end', () => { expect( r3.status, [ - `step 3 (--force bypass) exited with ${r3.status}`, + `step 3 (--allow-duplicate-name bypass) exited with ${r3.status}`, `stdout: ${r3.stdout}`, `stderr: ${r3.stderr}`, ].join('\n'), @@ -293,15 +300,21 @@ describe('CLI end-to-end', () => { const afterStep3 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); expect(afterStep3).toHaveLength(2); expect(afterStep3.every((e: { name: string }) => e.name === 'shared')).toBe(true); - const paths = afterStep3.map((e: { path: string }) => canonical(e.path)).sort(); - expect(paths).toEqual([canonical(repoA), canonical(repoB)].sort()); - - // Step 4: REGRESSION GUARD for the #955 review-round-2 finding. - // Create a THIRD repo with the same basename and try to register - // it via `--name shared --skills` — NO --force. This must still - // hit the collision guard even though --skills OR's with force at - // the pipeline level. If future refactors accidentally re-conflate - // pipeline-force and registry-force, this test fails. + // Both entries point to distinct paths (we registered two different + // repos under the same alias) and both have the right basename. + const step3Basenames = afterStep3.map((e: { path: string }) => path.basename(e.path)); + expect(step3Basenames).toEqual(['collide-app', 'collide-app']); + const step3Paths = new Set(afterStep3.map((e: { path: string }) => e.path)); + expect(step3Paths.size).toBe(2); + // One of the two entries is the original from step 1 — unchanged. + expect(afterStep3.map((e: { path: string }) => e.path)).toContain(afterStep1[0].path); + + // Step 4: REGRESSION GUARD for the design decision in review + // round 2/3 — pipeline-re-index flags must NOT bypass the + // registry collision guard. `--skills` triggers pipeline + // re-run (skills generation needs a fresh pipelineResult) but + // must leave the registry guard in force. Bypass requires the + // explicit --allow-duplicate-name flag. const repoC = makeMiniRepoCopy('collide-app', 'gn-collide-c-'); const parentC = path.dirname(repoC); try { @@ -315,6 +328,8 @@ describe('CLI end-to-end', () => { expect(r4.status).toBe(1); const r4Output = `${r4.stdout}${r4.stderr}`; expect(r4Output).toMatch(/Registry name collision|already used/i); + // The error hint should point at the new flag. + expect(r4Output).toMatch(/--allow-duplicate-name/); // Registry unchanged — still only A + B under "shared". const afterStep4 = JSON.parse(fs.readFileSync(registryPath, 'utf-8')); diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 6562980581..b9eabccb43 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -232,9 +232,9 @@ describe('registerRepo name override + collision guard (#829)', () => { expect(entries[0].name).toBe('shared'); }); - it('registerRepo({ name, force: true }) allows the duplicate to coexist', async () => { + it('registerRepo({ name, allowDuplicateName: true }) allows the duplicate to coexist', async () => { await registerRepo(tmpRepoA.dbPath, meta, { name: 'shared' }); - await registerRepo(tmpRepoB.dbPath, meta, { name: 'shared', force: true }); + await registerRepo(tmpRepoB.dbPath, meta, { name: 'shared', allowDuplicateName: true }); const entries = await listRegisteredRepos(); expect(entries).toHaveLength(2); From 553ee0e2820901813eb3ac882e4fcc77ca9d6338 Mon Sep 17 00:00:00 2001 From: azizur1992 Date: Sat, 18 Apr 2026 22:34:42 +0100 Subject: [PATCH 6/6] docs(cli): clarify --allow-duplicate-name scope + rename collision var (#955 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- gitnexus/src/storage/repo-manager.ts | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 6dbaab3feb..4ba17b21b7 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -257,11 +257,21 @@ export interface RegisterRepoOptions { */ name?: string; /** - * Allow registration even when another path already uses this name - * (#829). Mapped from the `--allow-duplicate-name` CLI flag — - * intentionally a distinct flag from `--force` (which only triggers - * pipeline re-index) because a user accepting a duplicate name should - * not be forced to also re-run the full pipeline. + * Allow two DIFFERENT repo paths to register under the same alias + * (#829). Mapped from the `--allow-duplicate-name` CLI flag. + * + * Scope: this flag governs cross-path alias sharing only — one repo + * path always has exactly one registry entry (and therefore exactly + * one alias). Re-analyzing the same path with `--name Y` overwrites + * a previous `--name X`; it does NOT create a second entry or a + * second alias for the same path (see the upsert-by-resolved-path + * logic in {@link registerRepo} and the + * `re-registerRepo with a different name overrides the previous + * alias` test in `test/unit/repo-manager.test.ts`). + * + * Distinct from `--force` (which only triggers pipeline re-index); + * a user accepting a duplicate alias should not be forced to also + * re-run the full pipeline. */ allowDuplicateName?: boolean; } @@ -345,14 +355,14 @@ export const registerRepo = async ( // list output this PR also ships). const explicitName = opts?.name !== undefined || (existing && hasCustomAlias(existing)); if (explicitName && !opts?.allowDuplicateName) { - const collision = entries.find( + const collidingEntry = entries.find( (e, i) => i !== existingIdx && e.name.toLowerCase() === name.toLowerCase() && path.resolve(e.path) !== resolved, ); - if (collision) { - throw new RegistryNameCollisionError(name, collision.path, resolved); + if (collidingEntry) { + throw new RegistryNameCollisionError(name, collidingEntry.path, resolved); } }