diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index e4932e234..572e8c783 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -7,10 +7,50 @@ */ import fs from 'fs/promises'; +import { realpathSync } from 'fs'; import path from 'path'; import os from 'os'; import { getInferredRepoName } from './git.js'; +/** + * Normalise a repo path for registry comparison across platforms + * (#664 review feedback from @evander-wang). + * + * Why this exists: `path.resolve` alone is NOT enough for + * cross-platform registry stability. + * - **macOS**: tmpdirs and `/var` are symlinks to `/private/var`. + * A child process that stored `/private/var/folders/.../repo` in + * the registry cannot later be matched by an outer caller that + * supplies the symlink form `/var/folders/.../repo`. `path.resolve` + * does not follow symlinks; `realpathSync.native` does. + * - **Windows**: GitHub runners surface tmpdirs in 8.3 short-name + * form (`RUNNERA~1\...`), but `process.cwd()` often returns the + * long form (`runneradmin\...`). `realpathSync.native` normalises + * both sides to the long-name canonical path. + * + * Fallback behaviour: if the path does not exist on disk (e.g. a user + * passed `gitnexus remove some-alias` and the alias misses every + * registry entry, or the caller is resolving a path that was deleted + * after registration), we return `path.resolve(p)` rather than + * throwing. This preserves the idempotent-on-missing semantics of + * `resolveRegistryEntry` / `remove`. + * + * Backwards compatibility: this function is applied to BOTH the + * caller-supplied input AND each stored `entry.path` at compare time + * inside `resolveRegistryEntry`, so registries written by older + * versions (where `registerRepo` only ran `path.resolve`) still match + * correctly. Newly-written entries are canonicalised at write time too + * so the registry stabilises over analyze/re-analyze cycles. + */ +export const canonicalizePath = (p: string): string => { + const resolved = path.resolve(p); + try { + return realpathSync.native(resolved); + } catch { + return resolved; + } +}; + export interface RepoMeta { repoPath: string; lastCommit: string; @@ -349,12 +389,22 @@ export const registerRepo = async ( meta: RepoMeta, opts?: RegisterRepoOptions, ): Promise => { - const resolved = path.resolve(repoPath); + // Canonicalise the caller's path up-front (#1003 review) — expands + // macOS /var → /private/var and Windows 8.3 → long-name so the + // registry entry stays matchable by `resolveRegistryEntry` regardless + // of which form the caller hands us. + const resolved = canonicalizePath(repoPath); const { storagePath } = getStoragePaths(resolved); const entries = await readRegistry(); const existingIdx = entries.findIndex((e) => { - const a = path.resolve(e.path); + // Canonicalise the STORED entry too so pre-canonicalisation + // registries (written by older versions, or the same version before + // this review fix) still match correctly. `canonicalizePath` falls + // back to `path.resolve` when the path no longer exists on disk, so + // stale entries that have been rm'd externally still resolve to a + // stable key instead of throwing. + const a = canonicalizePath(e.path); const b = resolved; return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; }); @@ -393,7 +443,7 @@ export const registerRepo = async ( (e, i) => i !== existingIdx && e.name.toLowerCase() === name.toLowerCase() && - path.resolve(e.path) !== resolved, + canonicalizePath(e.path) !== resolved, ); if (collidingEntry) { throw new RegistryNameCollisionError(name, collidingEntry.path, resolved); @@ -424,9 +474,16 @@ export const registerRepo = async ( * Called after `gitnexus clean`. */ export const unregisterRepo = async (repoPath: string): Promise => { - const resolved = path.resolve(repoPath); + // Canonicalise BOTH sides so an unregister call issued with the + // symlink form (`/var/folders/.../repo`) still matches an entry + // written with the realpath form (`/private/var/folders/.../repo`), + // and vice versa. Matches the semantics of `registerRepo` and + // `resolveRegistryEntry` post-#1003 review. + const resolved = canonicalizePath(repoPath); const entries = await readRegistry(); - const filtered = entries.filter((e) => path.resolve(e.path) !== resolved); + const matches = (a: string, b: string) => + process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; + const filtered = entries.filter((e) => !matches(canonicalizePath(e.path), resolved)); await writeRegistry(filtered); }; @@ -504,12 +561,19 @@ export class RegistryAmbiguousTargetError extends Error { * `GITNEXUS_HOME`. */ export const resolveRegistryEntry = (entries: RegistryEntry[], target: string): RegistryEntry => { - // Tier 1: path match. Normalise both sides the same way - // `registerRepo` / `unregisterRepo` do. - const resolvedTarget = path.resolve(target); + // Tier 1: path match. Canonicalise BOTH sides so symlink and + // Windows-8.3 quirks don't cause a false miss — e.g. the caller + // passes `/var/folders/.../repo` while the registry has + // `/private/var/folders/.../repo` (both resolve to the same + // `realpath.native`). See `canonicalizePath` for the rationale. + // + // Canonicalising the STORED entry (not just the input) is what gives + // us backward-compat for registries written by versions that only + // ran `path.resolve` — both get canonicalised here at compare time. + const canonicalTarget = canonicalizePath(target); const pathMatch = entries.find((e) => { - const a = path.resolve(e.path); - const b = resolvedTarget; + const a = canonicalizePath(e.path); + const b = canonicalTarget; return process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; }); if (pathMatch) return pathMatch; diff --git a/gitnexus/test/integration/cli-e2e.test.ts b/gitnexus/test/integration/cli-e2e.test.ts index 618320c89..a23293e27 100644 --- a/gitnexus/test/integration/cli-e2e.test.ts +++ b/gitnexus/test/integration/cli-e2e.test.ts @@ -489,8 +489,31 @@ describe('CLI end-to-end', () => { // And path-based remove still works: pass the absolute path of // repoA and it resolves unambiguously. + // + // We pull the path from the registry snapshot rather than + // passing the outer `repoA` variable directly. This is the + // belt-and-suspenders for cross-platform path normalisation + // (#1003 review): the path the registry recorded has already + // gone through the analyze-side canonicalisation (which on + // macOS expands /var → /private/var and on Windows expands 8.3 + // → long-name). Passing that exact string back to `remove` + // guarantees the comparison succeeds even on runners where the + // outer `repoA` is the symlink/short-name form. The code-side + // fix in `canonicalizePath` makes this redundant in practice, + // but the test shouldn't depend on the code fix being perfect + // on every platform — it should prove correctness against the + // registry contract. + const repoAEntry = before.find( + (e: { path: string }) => + path.basename(e.path) === 'dup' && e.path.includes(path.basename(parentA)), + ); + expect( + repoAEntry, + 'repoA entry must exist in registry before path-remove step', + ).toBeDefined(); + const r4 = runCliWithEnv( - ['remove', repoA, '--force'], + ['remove', repoAEntry.path, '--force'], parentA, { GITNEXUS_HOME: gnHome }, 15000, @@ -508,6 +531,8 @@ describe('CLI end-to-end', () => { expect(finalEntries).toHaveLength(1); // The survivor is repoB (its path stays in the registry). expect(path.basename(finalEntries[0].path)).toBe('dup'); + // And it's NOT the one we just removed. + expect(finalEntries[0].path).not.toBe(repoAEntry.path); } finally { fs.rmSync(gnHome, { recursive: true, force: true }); fs.rmSync(parentA, { recursive: true, force: true }); diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 429ecdbc3..0d4e072d1 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -16,6 +16,7 @@ import { registerRepo, listRegisteredRepos, resolveRegistryEntry, + canonicalizePath, RegistryNameCollisionError, RegistryNotFoundError, RegistryAmbiguousTargetError, @@ -605,3 +606,87 @@ describe('resolveRegistryEntry (#664)', () => { expect(hit.name).toBe('website'); }); }); + +// ─── canonicalizePath (#1003 review — @evander-wang / @magyargergo) ── +// +// Shields `registerRepo`, `unregisterRepo`, and `resolveRegistryEntry` +// against cross-platform path-form divergence: macOS symlink expansion +// (/var → /private/var) and Windows 8.3 short-name expansion +// (RUNNERA~1 → runneradmin). The helper also underpins backwards +// compatibility with registries written by versions that only ran +// `path.resolve` — by canonicalising the stored entry at compare time, +// both pre- and post-fix entries converge to the same key. +// +// These tests avoid snapshotting a specific realpath value (that would +// be platform-fragile); instead they assert: +// - canonicalizePath is idempotent (f(f(x)) == f(x)) +// - canonicalizePath falls back cleanly when the path doesn't exist +// - resolveRegistryEntry matches a stored entry even when the target +// and the stored value disagree on one-step normalisation (simulated +// via a fixture that stores the de-canonicalised form of a real +// existing path). + +describe('canonicalizePath (#1003)', () => { + it('is idempotent — canonicalizePath(canonicalizePath(x)) === canonicalizePath(x)', async () => { + // Use the vitest project-root as a known-existing path. `os.tmpdir()` + // would work too but process.cwd() is guaranteed to exist for the + // test runner. + const p = process.cwd(); + const once = canonicalizePath(p); + const twice = canonicalizePath(once); + expect(twice).toBe(once); + }); + + it('falls back to path.resolve when the target does not exist', () => { + // Construct a definitely-nonexistent path under tmpdir. Using + // random-ish segments so we don't collide with anything real. + const ghost = path.join(os.tmpdir(), 'gnx-never-exists-____', 'still-not-there'); + const got = canonicalizePath(ghost); + // Must not throw, must not resolve to something weird — should be + // identical to `path.resolve(ghost)` since realpathSync.native will + // have thrown and we swallowed it. + expect(got).toBe(path.resolve(ghost)); + }); + + it('returns an absolute path for relative input even when the path is missing', () => { + // Relative path that does not exist. Must still be absolute + // (fallback path: path.resolve normalises even non-existent inputs). + const rel = './does-not-exist-zzz-' + Date.now(); + const got = canonicalizePath(rel); + expect(path.isAbsolute(got)).toBe(true); + }); +}); + +describe('resolveRegistryEntry backward-compat with non-canonical stored paths (#1003)', () => { + it('matches a stored entry even when the target was passed in canonical form', async () => { + // Simulate the bug-producing scenario without depending on a real + // symlink/8.3 discrepancy (those are platform-specific and flaky to + // set up in CI). We take a REAL path that exists + // (canonicalizePath-stable), store a known-non-canonical copy of it + // in a fake RegistryEntry, then resolve with the canonical form and + // assert the match. + // + // Construct a non-canonical string that resolves to the same real + // path. `path.join` auto-normalises `.` and trailing separators, so + // we build the string by raw concat to keep it string-unequal to + // `realDir` until `canonicalizePath` runs. + const realDir = process.cwd(); + const nonCanonical = realDir + path.sep + '.'; // e.g. /work/gitnexus/. + // Sanity: these are string-unequal before canonicalisation. + expect(nonCanonical).not.toBe(realDir); + + const entries: RegistryEntry[] = [ + { + name: 'stored-under-noncanonical-form', + path: nonCanonical, + storagePath: path.join(nonCanonical, '.gitnexus'), + indexedAt: '2026-04-20T00:00:00.000Z', + lastCommit: 'deadbee', + }, + ]; + + // Pass the canonical form as the target — resolver must still match. + const hit = resolveRegistryEntry(entries, realDir); + expect(hit).toBe(entries[0]); + }); +});