From 04ae8c29eb5fe01666bbade3eb2536e66ce9532c Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Sun, 21 Jun 2026 10:03:59 +0000 Subject: [PATCH] fix(analyze): don't take the up-to-date fast path for an unregistered repo (#2264) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A prior 'analyze --name X' that hit a registry name collision writes meta.json (meta-save runs before registerRepo) but fails before registering — leaving the index up-to-date but UNREGISTERED. A later 'analyze --name X --allow-duplicate-name' then matched the up-to-date gate and early-returned WITHOUT registering, so the repo stayed invisible to list_repos/MCP and the CLI's assertAnalysisFinalized rejected it. --allow-duplicate-name could never heal it. This was latent on main, masked by the very close-hang this PR fixes: the lingering process pushed the cli-e2e #829 step-3 analyze past its 60s spawn timeout (status===null → the test's vacuous early-return). With the hang gone the analyze exits promptly, exit 1 surfaces, and the bug becomes deterministic on all platforms. Fix: the up-to-date fast path now short-circuits only when the repo is actually registered (new isRepoRegistered helper, sharing assertAnalysisFinalized's exact canonical/case-folded membership check). An indexed-but-unregistered repo falls through to the pipeline, which registers it honoring allowDuplicateName. Already registered repos keep the fast path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JBJomjoTdBV2eveDVq4JMm --- gitnexus/src/core/run-analyze.ts | 11 +++++++- gitnexus/src/storage/repo-manager.ts | 25 +++++++++++++------ .../repo-manager-finalize-invariant.test.ts | 14 +++++++++++ 3 files changed, 41 insertions(+), 9 deletions(-) diff --git a/gitnexus/src/core/run-analyze.ts b/gitnexus/src/core/run-analyze.ts index 0649bce08..cf8196acb 100644 --- a/gitnexus/src/core/run-analyze.ts +++ b/gitnexus/src/core/run-analyze.ts @@ -42,6 +42,7 @@ import { loadMeta, ensureGitNexusIgnored, registerRepo, + isRepoRegistered, cleanupOldKuzuFiles, INCREMENTAL_SCHEMA_VERSION, type RepoMeta, @@ -777,7 +778,15 @@ export async function runFullAnalysis( return true; // conservative on git failure } })(); - if (!dirty) { + // Only short-circuit when this repo is actually REGISTERED. A prior run + // can write meta.json and then fail before registerRepo (e.g. a rejected + // --name collision), leaving the index up-to-date but UNREGISTERED. Taking + // the fast path there returns an unregistered repo that the CLI's + // assertAnalysisFinalized rejects — and `--allow-duplicate-name` could + // never heal it (it would keep hitting this early-return). Fall through to + // the pipeline so it gets registered (honoring allowDuplicateName); already + // registered repos keep the fast path unchanged (#2264). + if (!dirty && (await isRepoRegistered(repoPath))) { await ensureGitNexusIgnored(repoPath); return { // `resolveRepoIdentityRoot` collapses worktree roots to the diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 2a63d7fb5..abe3e3141 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -978,6 +978,22 @@ export class AnalysisNotFinalizedError extends Error { } } +/** + * True when the global registry already contains an entry whose canonical path + * matches `repoPath`. Uses the same canonical, case-folded (Windows) comparison + * as {@link assertAnalysisFinalized} so "is it registered?" answers identically + * at the analyze fast-path gate and at the finalize assertion. Pure read. + */ +export const isRepoRegistered = async (repoPath: string): Promise => { + const entries = await readRegistry(); + const canonicalInput = canonicalizePath(path.resolve(repoPath)); + const isWin = process.platform === 'win32'; + return entries.some((e) => { + const a = canonicalizePath(e.path); + return isWin ? a.toLowerCase() === canonicalInput.toLowerCase() : a === canonicalInput; + }); +}; + /** * Verify that a successful `analyze` call actually produced an indexed, * registered repo on disk. Two checks, both strictly required: @@ -1002,14 +1018,7 @@ export const assertAnalysisFinalized = async (repoPath: string): Promise = throw new AnalysisNotFinalizedError(resolved, storagePath, 'meta', getGlobalRegistryPath()); } - const entries = await readRegistry(); - const canonicalInput = canonicalizePath(resolved); - const isWin = process.platform === 'win32'; - const found = entries.some((e) => { - const a = canonicalizePath(e.path); - return isWin ? a.toLowerCase() === canonicalInput.toLowerCase() : a === canonicalInput; - }); - if (!found) { + if (!(await isRepoRegistered(resolved))) { throw new AnalysisNotFinalizedError( resolved, storagePath, diff --git a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts index c4472a6de..d0e217365 100644 --- a/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts +++ b/gitnexus/test/unit/repo-manager-finalize-invariant.test.ts @@ -18,6 +18,7 @@ import fs from 'fs/promises'; import { AnalysisNotFinalizedError, assertAnalysisFinalized, + isRepoRegistered, registerRepo, saveMeta, getStoragePaths, @@ -128,4 +129,17 @@ describe('assertAnalysisFinalized (#1169)', () => { const variant = process.platform === 'win32' ? tmpRepo.dbPath.toUpperCase() : tmpRepo.dbPath; // POSIX is case-sensitive; assertion uses canonical form await expect(assertAnalysisFinalized(variant)).resolves.toBeUndefined(); }); + + // isRepoRegistered backs the analyze up-to-date fast-path gate (#2264): the + // fast path must NOT short-circuit a repo that is indexed-but-unregistered + // (e.g. a prior --name collision wrote meta.json then failed before + // registerRepo), otherwise --allow-duplicate-name could never heal it. + it('isRepoRegistered is false when the repo has no registry entry', async () => { + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(false); + }); + + it('isRepoRegistered is true once a matching entry is written', async () => { + await registerRepo(tmpRepo.dbPath, meta); + expect(await isRepoRegistered(tmpRepo.dbPath)).toBe(true); + }); });