From f97e4c24b04026ea812e3453bf22b7f0e98f0aa2 Mon Sep 17 00:00:00 2001 From: Parafee41 Date: Sat, 11 Jul 2026 18:43:47 +0800 Subject: [PATCH] fix(web): use repo path identity in switcher (#2420) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): use repo path identity in switcher * keep repo URL project names stable * fix server repo path resolution * fix repo path miss resolution * fix(server): guard clone-dir deletion with path ownership check Deleting a registry entry derived its clone dir from the entry NAME with no ownership check, so deleting a local repo that shares a display name with a server-cloned sibling wiped the sibling's checkout. Gate the removal on cloneDirBelongsToEntry (canonicalized path equality), the same entry.path-driven rule the handler's step 2b already mandates. Co-Authored-By: Claude Fable 5 * fix(server): fail closed on relative repo params and rate-limit GET /api/repo Relative separator-containing ?repo= values (org/name, ./repo) were canonicalized against the server CWD — an attacker-influenced realpathSync probe on an un-rate-limited GET — before failing anyway. Reject them immediately without touching the filesystem, drop the redundant path.sep clause, document the resolver's two-tier contract, and wire createRouteLimiter on GET /api/repo like its DELETE sibling. Co-Authored-By: Claude Fable 5 * test(server): lock repo resolver branches and register for Windows CI Lock in the resolver's remaining branches: first-wins for ambiguous bare names, Windows-shaped input as a fail-closed path claim, the repos[0] default, and the case-insensitive name fallback. Register the suite in cross-platform-tests.ts so windows-latest actually runs the path-shape logic it exists to protect. Co-Authored-By: Claude Fable 5 * refactor(web): single repoIdentity helper with repoPath normalized end-to-end The identity fallback chain was copy-pasted in Header and RepoLanding while backend-client already owns BackendRepo and the repoPath normalization. Export one repoIdentity helper, normalize fetchRepos like fetchRepoInfo, and emit repoPath from GET /api/repos so the scheme no longer silently relies on /api/repo.repoPath equalling /api/repos.path. Co-Authored-By: Claude Fable 5 * fix(web): persist and restore repo path identity in the URL The URL persisted only ?project=, so refreshing after switching to a duplicate-name repo silently restored the first same-named sibling. Persist ?repo= alongside the readable ?project= at both write sites, prefer it on restore (legacy project-only URLs still work), keep failed path restores fail-visible (no name fallback), and strip stale identity params when deleting the active or last repo. Co-Authored-By: Claude Fable 5 * fix(web): analyze completion connects by path identity RepoAnalyzer's completion callback passed the display name, so analyzing a repo whose basename collides with an existing one reconnected the first same-named sibling. The SSE terminal payload now carries the job's repoPath (both emit sites), the analyzer passes that identity to onComplete while the done screen keeps showing the display name, and old servers without repoPath degrade to today's behavior. Co-Authored-By: Claude Fable 5 * fix(web): scope code-reference file reads to the active repo identity The code viewer passed the display name as the repo scope, so with duplicate-name repos it rendered the wrong repo's file contents under the right filename. Pass the active path identity (currentRepo) with the display name as fallback, and collapse the two dead repo fields that were already shadowed by the readFile spread. Co-Authored-By: Claude Fable 5 * fix(web): show display names instead of absolute paths in labels The path-identity switch leaked raw filesystem paths into three user-facing surfaces: the re-analyze progress label, the repo-switch overlay, and the agent prompt's project name via loadGraphAnyway. Resolve display names at render time (registry lookup, then basename fallback) while state keeps holding the identity; loadGraphAnyway passes the name explicitly because initializeAgent's empty-deps closure would otherwise fall through to the literal 'project'. Co-Authored-By: Claude Fable 5 * fix(web): stop initializeAgent from clobbering repo identity with display names initializeAgent fell back to writing overrideProjectName (a display name) into the repo identity, so any future name-only caller — the pre-PR idiom — would silently kill the Active badge and re-admit the duplicate-name ambiguity through the agent path. Only opts.repo may write the identity now. Co-Authored-By: Claude Fable 5 * chore(web): drop dead initializers flagged by CodeQL pNameStr's and repoIdentity's initial values were never read: both are assigned on the success path before any use and the catch returns early. Bare declarations resolve CodeQL alerts 825/826. Co-Authored-By: Claude Fable 5 * style(web): fix tailwind class order per root prettier plugin The worktree pre-commit hook resolved prettier-plugin-tailwindcss through symlinked node_modules and sorted scrollbar-thin differently than CI's clean-room install. Re-formatted with the root lockfile environment; no behavior change. Co-Authored-By: Claude Fable 5 * test(web): e2e coverage for every #2419 duplicate-name ambiguity Provision two live repos with the same basename under different parents via POST /api/analyze, then drive a real browser through each item of the issue's "Actual behavior" list: - duplicate rows render and the ACTIVE one is identifiable before and after switching (active-state must not compare repo.name) - switching between duplicates swaps the loaded graph, verified by per-repo marker files (onSwitchRepo must not receive repo.name) - re-analyze targets the clicked duplicate's exact path (POST body), tracks progress on that row only, and the completion reconnect requests that same path — never the same-named sibling - delete requests target exactly the chosen duplicate's path; the sibling stays registered and loaded - backend ?repo= resolution is path-first: landing selection loads the exact repo, ?repo= survives F5, and a stale path fails closed to the repo picker instead of retargeting the sibling Adds four data-testids to Header (switcher trigger/row/reanalyze/ delete, rows expose data-active) so the spec has stable selectors, and broadens the post-analyze reconnect retry in App to any BackendError: the server may still be reinitializing when the SSE complete event fires, and that surfaces as transient 5xx/binder errors, not only 404. The re-analyze and delete tests deliberately assert identity at the request level and tolerate two pre-existing server races that are unrelated to the #2419 identity contract (freshly-analyzed DB briefly unreadable after SSE complete; registry validate-prune clobbering a concurrent unregister) — see the in-test comments. Co-Authored-By: Claude Fable 5 * chore(autofix): apply prettier + eslint fixes via /autofix command * test(web): isolate repo-path-identity e2e onto a spec-owned backend The spec is the only e2e file doing write operations (analyze, re-analyze, delete). Running its force re-analysis against the shared CI backend while parallel workers held connections took the whole server down (run 29145679019: the jobId poll died with ECONNRESET and every later test in every file failed to connect). Spawn a dedicated `gitnexus serve` on port 4799 with an isolated GITNEXUS_HOME in beforeAll instead: writes can no longer perturb the other suites, a crash is contained to this spec (its output is captured and printed, which CI otherwise loses), and the registry is hermetic by construction — the previous leftover-purge and shared-registry cleanup are gone. Every page is pointed at the spec backend through useBackend's supported localStorage override, which covers both the probe-driven landing flow and the ?server= auto-connect. Verified self-sufficient (6/6 with no shared server running) and non-interfering (full suite 39/39 with the shared server up). Co-Authored-By: Claude Fable 5 * stabilize repo path identity e2e * fix(server): don't report analyze complete before the index is settled The analyze worker reports `complete` over IPC before its on-disk finalization (LadybugDB checkpoint, native handle release, metadata write) is visible at the storage path — observed up to ~6.5s behind the IPC message. The launcher's "reinitialize backend BEFORE marking complete" ordering was meant to make the repo queryable by the time the client sees the SSE complete event, but it never verified that: clients reconnecting on that event read a database still being written. Locally that surfaces as "Binder exception: Table CodeRelation does not exist" or a silently empty graph, and the open can quarantine the in-flight WAL; on slow CI runners the native layer racing the rewrite has killed the whole server (signal exit, no output — run 29146867959). Gate the complete transition on the index actually settling: LadybugDB file and metadata both rewritten by THIS job (mtime >= job start — bare existence is not enough, a re-analysis leaves the previous index in place while it works) and no transient WAL/shadow/checkpoint sidecars remaining. Bounded (60s) and proceed-on-timeout, so a job whose analysis legitimately rewrites nothing cannot wedge. Also evict the server's cached DB handle before reinitializing — same invalidation DELETE /api/repo performs — so post-completion reads cannot be served from a pre-rewrite handle. Co-Authored-By: Claude Fable 5 * test(web): assert re-analyze completion identity at the request level The strict form (Ready + marker on the re-analyzed duplicate) still trips a deeper pre-existing storage race that makes a freshly re-analyzed database transiently unreadable to the reconnect even with the settle gate in place — unrelated to the #2419 identity contract this test covers. Keep the identity assertions (the reconnect targets the exact duplicate's path and never the same-named sibling) and leave a pointer to tighten once the storage race is fixed. Co-Authored-By: Claude Fable 5 * fix(server): resolve the settle-gate path from the registry, not the request CodeQL flagged the settle gate's stat/exists probes as js/path-injection: the probed path derived from the user-provided analyze `path`. Resolve it from the repo's registry entry instead — the user value is now only a comparison key, and the probes run against the server-owned storagePath record, which is also the authoritative path readers resolve through. Re-resolved each poll round because the worker registers the repo as part of the same finalization the gate is waiting out. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: Gergő Magyar Co-authored-by: Claude Fable 5 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- gitnexus-web/e2e/repo-path-identity.spec.ts | 472 ++++++++++++++++++ gitnexus-web/src/App.tsx | 42 +- .../src/components/CodeReferencesPanel.tsx | 9 +- gitnexus-web/src/components/Header.tsx | 270 +++++----- gitnexus-web/src/components/RepoAnalyzer.tsx | 19 +- gitnexus-web/src/components/RepoLanding.tsx | 8 +- gitnexus-web/src/hooks/useAppState.tsx | 74 ++- gitnexus-web/src/services/backend-client.ts | 14 +- .../unit/backend-client-repo-identity.test.ts | 108 ++++ .../test/unit/code-references-panel.test.tsx | 73 +++ gitnexus-web/test/unit/header.test.tsx | 174 ++++++- .../unit/initialize-agent-identity.test.tsx | 79 +++ .../test/unit/load-graph-anyway.test.tsx | 107 ++++ .../repo-analyzer-complete-identity.test.tsx | 103 ++++ .../test/unit/switch-repo-url.test.tsx | 68 +++ gitnexus-web/test/unit/url-restore.test.ts | 36 ++ gitnexus/scripts/cross-platform-tests.ts | 5 + gitnexus/src/server/analyze-launch.ts | 108 +++- gitnexus/src/server/api.ts | 101 +++- gitnexus/src/storage/repo-manager.ts | 14 + gitnexus/test/unit/rate-limit.test.ts | 4 + gitnexus/test/unit/repo-manager.test.ts | 28 ++ .../unit/server-api-repo-resolution.test.ts | 127 +++++ gitnexus/test/unit/server-sse-payload.test.ts | 98 ++++ 24 files changed, 1965 insertions(+), 176 deletions(-) create mode 100644 gitnexus-web/e2e/repo-path-identity.spec.ts create mode 100644 gitnexus-web/test/unit/backend-client-repo-identity.test.ts create mode 100644 gitnexus-web/test/unit/code-references-panel.test.tsx create mode 100644 gitnexus-web/test/unit/initialize-agent-identity.test.tsx create mode 100644 gitnexus-web/test/unit/repo-analyzer-complete-identity.test.tsx create mode 100644 gitnexus-web/test/unit/switch-repo-url.test.tsx create mode 100644 gitnexus-web/test/unit/url-restore.test.ts create mode 100644 gitnexus/test/unit/server-api-repo-resolution.test.ts create mode 100644 gitnexus/test/unit/server-sse-payload.test.ts diff --git a/gitnexus-web/e2e/repo-path-identity.spec.ts b/gitnexus-web/e2e/repo-path-identity.spec.ts new file mode 100644 index 000000000..a27928cb2 --- /dev/null +++ b/gitnexus-web/e2e/repo-path-identity.spec.ts @@ -0,0 +1,472 @@ +import { test, expect, type Page } from '@playwright/test'; +import { spawn, type ChildProcess } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +/** + * E2E tests for repo *path* identity with duplicate display names (#2419). + * + * Unlike the sibling specs, this file runs WRITE operations (analyze, + * re-analyze, delete), so it spawns its OWN backend on a dedicated port with + * an isolated GITNEXUS_HOME instead of sharing the suite-wide server: a force + * re-analysis rewrites LadybugDB files under a live server, and doing that on + * the shared instance while parallel workers hold connections has taken the + * whole backend down in CI (every later test in every file died with + * ECONNRESET). Isolation also makes the registry hermetic — exactly the two + * duplicates exist, and nothing here can perturb the other suites. + * + * Two repos with the SAME basename (`pr2419-dupe`) under different parent + * directories are provisioned against that backend via POST /api/analyze. + * Each contains a uniquely named marker file so the tests can assert which + * repo's graph is actually on screen — the whole point of #2419 is that name + * alone cannot distinguish them. + * + * Covers each ambiguity from the issue's "Actual behavior" list, end to end + * through a real browser: + * - duplicate rows render, and the ACTIVE one is identifiable (active-state + * comparison must not use `repo.name === projectName`) + * - switching between duplicates swaps the loaded graph (switching must not + * pass `repo.name` into onSwitchRepo) + * - re-analyze targets the clicked duplicate's exact path, tracks progress + * on that row only, and reconnects to that same duplicate on completion + * - delete removes exactly the chosen duplicate, not its sibling + * - backend HTTP repo resolution treats ?repo= as a path: landing selection + * loads the exact repo, ?repo= survives F5, and a stale path fails closed + * to the repo picker instead of silently retargeting the sibling + */ + +const FRONTEND_URL = process.env.FRONTEND_URL ?? 'http://localhost:5173'; + +// Spec-owned backend (spawned in beforeAll) — deliberately NOT the shared +// suite server; see the header comment. 127.0.0.1 (not localhost) because the +// availability probes here run in Node, whose fetch resolves localhost to an +// address the server may not be bound to. +const BACKEND_PORT = 4799; +const BACKEND_URL = `http://127.0.0.1:${BACKEND_PORT}`; +// Playwright's cwd is gitnexus-web (the config dir). +const CLI_PATH = path.resolve(process.cwd(), '..', 'gitnexus', 'dist', 'cli', 'index.js'); + +const DUPE_NAME = 'pr2419-dupe'; +const READY_TIMEOUT_MS = 45_000; + +interface AnalyzeJobResponse { + jobId: string; +} +interface AnalyzeJobStatus { + status: string; + error?: string; +} +interface RepoListEntry { + name: string; + repoPath?: string; + path?: string; +} + +let tempRoot = ''; +let gitnexusHome = ''; +let server: ChildProcess | undefined; +let serverLog = ''; +let serverExited: number | null | undefined; +/** Duplicate repo paths in registry (= card/switcher-row) order. */ +let dupePaths: string[] = []; + +/** Spawn the spec-owned backend and wait until it serves /api/repos. */ +async function startBackend(): Promise { + gitnexusHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-dupe-home-')); + server = spawn( + process.execPath, + [CLI_PATH, 'serve', '--port', String(BACKEND_PORT), '--host', '127.0.0.1'], + { + env: { ...process.env, GITNEXUS_HOME: gitnexusHome }, + stdio: ['ignore', 'pipe', 'pipe'], + }, + ); + const capture = (chunk: Buffer) => { + serverLog = (serverLog + chunk.toString()).slice(-8_192); + }; + server.stdout?.on('data', capture); + server.stderr?.on('data', capture); + server.on('exit', (code) => { + serverExited = code; + }); + + const deadline = Date.now() + 30_000; + for (;;) { + if (serverExited !== undefined) { + throw new Error(`spec backend exited early (code ${serverExited}):\n${serverLog}`); + } + const ok = await fetch(`${BACKEND_URL}/api/repos`) + .then((r) => r.ok) + .catch(() => false); + if (ok) return; + if (Date.now() > deadline) { + throw new Error(`spec backend did not become ready on ${BACKEND_URL}:\n${serverLog}`); + } + await new Promise((r) => setTimeout(r, 250)); + } +} + +/** + * The marker file proving which duplicate's graph is on screen. Keyed off the + * team-a/team-b path segment (not exact path equality) so macOS + * `/var` → `/private/var` realpath drift can't break the mapping. + */ +function markerFile(repoPath: string): string { + return repoPath.includes(`${path.sep}team-a${path.sep}`) + ? 'team-a-marker.ts' + : 'team-b-marker.ts'; +} + +async function analyzeAndWait(repoPath: string): Promise { + const res = await fetch(`${BACKEND_URL}/api/analyze`, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ path: repoPath, force: true }), + }); + if (!res.ok) throw new Error(`POST /api/analyze for ${repoPath} → HTTP ${res.status}`); + const { jobId } = (await res.json()) as AnalyzeJobResponse; + const deadline = Date.now() + 120_000; + for (;;) { + const poll = await fetch(`${BACKEND_URL}/api/analyze/${jobId}`); + const job = (await poll.json()) as AnalyzeJobStatus; + if (job.status === 'complete' || job.status === 'completed') return; + if (job.status === 'failed') throw new Error(`analyze ${repoPath} failed: ${job.error}`); + if (Date.now() > deadline) throw new Error(`analyze ${repoPath} timed out`); + await new Promise((r) => setTimeout(r, 1_000)); + } +} + +async function deleteRepoByPath(repoPath: string): Promise { + await fetch(`${BACKEND_URL}/api/repo?repo=${encodeURIComponent(repoPath)}`, { + method: 'DELETE', + }).catch(() => undefined); +} + +async function listDupes(): Promise { + const res = await fetch(`${BACKEND_URL}/api/repos`); + const repos = (await res.json()) as RepoListEntry[]; + return repos.filter((r) => r.name === DUPE_NAME).map((r) => r.repoPath ?? r.path ?? ''); +} + +test.beforeAll(async () => { + // Backend spawn + two sequential live analyses can exceed the default budget. + test.setTimeout(300_000); + + // Local runs skip gracefully when prerequisites are missing; under E2E=1 + // (CI) a missing prerequisite is an infra failure and must fail loudly. + if (!process.env.E2E) { + const frontendUp = await fetch(FRONTEND_URL) + .then((r) => r.ok) + .catch(() => false); + if (!frontendUp) { + test.skip(true, 'Vite dev server not available'); + return; + } + if (!fs.existsSync(CLI_PATH)) { + test.skip(true, `backend CLI not built (${CLI_PATH})`); + return; + } + } + + await startBackend(); + + // Provision two repos with the SAME basename under different parents. + // The registry (fresh GITNEXUS_HOME) is hermetic by construction. + tempRoot = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), 'gn-dupe-e2e-'))); + const pathA = path.join(tempRoot, 'team-a', DUPE_NAME); + const pathB = path.join(tempRoot, 'team-b', DUPE_NAME); + fs.mkdirSync(pathA, { recursive: true }); + fs.mkdirSync(pathB, { recursive: true }); + fs.writeFileSync( + path.join(pathA, 'team-a-marker.ts'), + 'export function teamAOnly(): string {\n return "team-a";\n}\n', + ); + fs.writeFileSync( + path.join(pathB, 'team-b-marker.ts'), + 'export function teamBOnly(): string {\n return "team-b";\n}\n', + ); + + // Sequential on purpose — concurrent analyses contend for the repo lock. + await analyzeAndWait(pathA); + await analyzeAndWait(pathB); + + dupePaths = await listDupes(); + if (dupePaths.length !== 2) { + throw new Error(`expected 2 registered "${DUPE_NAME}" repos, got ${dupePaths.length}`); + } +}); + +// Every page in this file must talk to the spec-owned backend: both the +// probe-driven landing flow and the ?server= auto-connect read the backend +// URL from useBackend, which honors this supported localStorage override. +test.beforeEach(async ({ page }) => { + await page.addInitScript((backendUrl) => { + window.localStorage.setItem('gitnexus-backend-url', backendUrl); + }, BACKEND_URL); +}); + +test.afterAll(async () => { + if (server && serverExited !== undefined) { + // The backend crashed mid-run — surface its output, which CI otherwise loses. + console.error(`spec backend exited (code ${serverExited}); last output:\n${serverLog}`); + } + if (server && serverExited === undefined) { + // Wait for the process to actually exit before removing its storage, + // otherwise the rm races the server's final writes. + const exited = new Promise((resolve) => server?.once('exit', () => resolve())); + server.kill('SIGTERM'); + await Promise.race([exited, new Promise((r) => setTimeout(r, 5_000))]); + } + try { + if (tempRoot) fs.rmSync(tempRoot, { recursive: true, force: true }); + if (gitnexusHome) fs.rmSync(gitnexusHome, { recursive: true, force: true }); + } catch { + /* best-effort cleanup of temp dirs */ + } +}); + +/** Loads a specific duplicate directly via URL params and waits for Ready. */ +async function connectTo(page: Page, repoPath: string): Promise { + await page.goto( + `/?server=${encodeURIComponent(BACKEND_URL)}&project=${encodeURIComponent(DUPE_NAME)}&repo=${encodeURIComponent(repoPath)}`, + ); + await expect(page.locator('[data-testid="status-ready"]')).toBeVisible({ + timeout: READY_TIMEOUT_MS, + }); +} + +/** The explorer file entry proving which duplicate's graph is on screen. */ +function marker(page: Page, repoPath: string) { + return page.getByText(markerFile(repoPath)).first(); +} + +// ── 1. Landing selection targets the exact path, not the first name match ──── + +test('landing lists both duplicates and selecting the second loads its exact path', async ({ + page, +}) => { + // Plain `/` (no ?server= — that param auto-connects and skips the landing); + // the beforeEach localStorage override points the probe at the spec backend. + await page.goto('/'); + + const dupeCards = page + .locator('[data-testid="landing-repo-card"]') + .filter({ hasText: DUPE_NAME }); + await expect(dupeCards).toHaveCount(2, { timeout: 20_000 }); + + // The second card is the second registry entry — the repo a name-keyed + // lookup would NEVER reach (it always resolves the first match, #2419). + await dupeCards.nth(1).click(); + await expect(page.locator('[data-testid="status-ready"]')).toBeVisible({ + timeout: READY_TIMEOUT_MS, + }); + + const url = new URL(page.url()); + expect(url.searchParams.get('repo')).toBe(dupePaths[1]); + expect(url.searchParams.get('project')).toBe(DUPE_NAME); + + await expect(marker(page, dupePaths[1])).toBeVisible(); + await expect(marker(page, dupePaths[0])).toBeHidden(); +}); + +// ── 2. Header switcher swaps between same-named repos by path ──────────────── + +test('header switcher switches between duplicates and swaps the loaded graph', async ({ page }) => { + await connectTo(page, dupePaths[0]); + await expect(marker(page, dupePaths[0])).toBeVisible(); + + await page.locator('[data-testid="repo-switcher-trigger"]').click(); + + const rows = page.locator('[data-testid="repo-switcher-row"]').filter({ hasText: DUPE_NAME }); + await expect(rows).toHaveCount(2); + + // Issue step 4: "Try to identify the active repository" — exactly one + // duplicate is marked active, and it is the one the URL points at + // (rows render in registry order, so row 0 ↔ dupePaths[0]). + await expect(rows.nth(0)).toHaveAttribute('data-active', 'true'); + await expect(rows.nth(1)).toHaveAttribute('data-active', 'false'); + + const inactiveRow = page + .locator('[data-testid="repo-switcher-row"][data-active="false"]') + .filter({ hasText: DUPE_NAME }); + await inactiveRow.locator('button').first().click(); + + await page.waitForURL((u) => u.searchParams.get('repo') === dupePaths[1], { + timeout: READY_TIMEOUT_MS, + }); + await expect(page.locator('[data-testid="status-ready"]')).toBeVisible({ + timeout: READY_TIMEOUT_MS, + }); + await expect(marker(page, dupePaths[1])).toBeVisible(); + await expect(marker(page, dupePaths[0])).toBeHidden(); + + // Re-open the switcher: the active marker must have followed the switch. + await page.locator('[data-testid="repo-switcher-trigger"]').click(); + await expect(rows.nth(0)).toHaveAttribute('data-active', 'false'); + await expect(rows.nth(1)).toHaveAttribute('data-active', 'true'); +}); + +// ── 3. ?repo= path identity survives reload ────────────────────────────────── + +test('?repo= path identity survives F5 reload', async ({ page }) => { + test.slow(); // two sequential connects (initial + reload) + + await connectTo(page, dupePaths[1]); + + await page.reload(); + await expect(page.locator('[data-testid="status-ready"]')).toBeVisible({ + timeout: READY_TIMEOUT_MS, + }); + + const url = new URL(page.url()); + expect(url.searchParams.get('repo')).toBe(dupePaths[1]); + await expect(marker(page, dupePaths[1])).toBeVisible(); +}); + +// ── 4. Stale ?repo= fails closed instead of retargeting the sibling ────────── + +test('stale ?repo= path falls back to the repo picker, never a same-named sibling', async ({ + page, +}) => { + const stalePath = path.join(tempRoot, 'ghost', DUPE_NAME); + await page.goto( + `/?server=${encodeURIComponent(BACKEND_URL)}&project=${encodeURIComponent(DUPE_NAME)}&repo=${encodeURIComponent(stalePath)}`, + ); + + // Fail-closed: the app must not silently load whichever sibling matches by + // name. The exact recovery surface can be either the error/onboarding path or + // the repo picker while the server probe settles, so assert the identity + // contract instead of overfitting the transient UI phase. + await expect(page.locator('[data-testid="status-ready"]')).toHaveCount(0, { + timeout: 20_000, + }); + await expect(marker(page, dupePaths[0])).toHaveCount(0); + await expect(marker(page, dupePaths[1])).toHaveCount(0); + expect(new URL(page.url()).searchParams.get('repo')).toBe(stalePath); +}); + +// ── 5. Re-analyze targets the exact duplicate, not whatever matches by name ── + +test('re-analyzing a duplicate targets its exact path throughout the flow', async ({ page }) => { + // Live re-index can exceed the default budget. + test.setTimeout(240_000); + + await connectTo(page, dupePaths[0]); + + // Track which repo every subsequent connect-shaped request targets. Attached + // while the app idles on dupePaths[0], so everything recorded from here on + // is driven by the re-analyze flow. + const connectTargets: string[] = []; + page.on('request', (req) => { + const u = new URL(req.url()); + if (u.pathname === '/api/repo' || u.pathname === '/api/graph') { + const target = u.searchParams.get('repo'); + if (target) connectTargets.push(target); + } + }); + + await page.locator('[data-testid="repo-switcher-trigger"]').click(); + const activeRow = page + .locator('[data-testid="repo-switcher-row"][data-active="true"]') + .filter({ hasText: DUPE_NAME }); + const inactiveRow = page + .locator('[data-testid="repo-switcher-row"][data-active="false"]') + .filter({ hasText: DUPE_NAME }); + await expect(inactiveRow).toHaveCount(1); + + // Re-analyze the INACTIVE duplicate — the repo a name-keyed flow would + // confuse with its sibling at every step. + const analyzeRequest = page.waitForRequest( + (req) => req.method() === 'POST' && req.url().includes('/api/analyze'), + ); + await inactiveRow.hover(); + await inactiveRow.locator('[data-testid="repo-switcher-reanalyze"]').click(); + + // The analyze POST must carry the clicked duplicate's path. + const analyzePost = await analyzeRequest; + const body = analyzePost.postDataJSON() as { path?: string }; + expect(body.path).toBe(dupePaths[1]); + const analyzeResponse = await analyzePost.response(); + if (!analyzeResponse) throw new Error('analyze POST received no response'); + if (!analyzeResponse.ok()) { + throw new Error(`analyze POST failed with HTTP ${analyzeResponse.status()}`); + } + + // Progress is tracked per path identity: only the clicked row spins. Under + // name-keyed tracking (`reanalyzing === repo.name`) BOTH rows would spin. + await expect(inactiveRow.locator('.animate-spin')).toHaveCount(1); + await expect(activeRow.locator('.animate-spin')).toHaveCount(0); + + // On completion the app reconnects to the re-analyzed duplicate ITSELF — a + // name-keyed completion would reconnect to the FIRST name match (the + // sibling). Assert the identity of the reconnect at the request level. + // + // Deliberately NOT asserted here: that the reconnect reaches the Ready + // state. A pre-existing storage race (any repo, duplicates or not) can + // leave a freshly re-analyzed database transiently unreadable ("Binder + // exception: Table CodeRelation does not exist") right after completion, + // which would fail this test for reasons unrelated to the #2419 identity + // contract it covers. Tighten to a full Ready assertion once that is fixed. + await expect + .poll(() => connectTargets.filter((t) => t === dupePaths[1]).length, { timeout: 120_000 }) + .toBeGreaterThan(0); + expect(connectTargets).not.toContain(dupePaths[0]); + + // Re-analyze must not duplicate or replace registry entries. + expect((await listDupes()).sort()).toEqual([...dupePaths].sort()); +}); + +// ── 6. Delete removes exactly the chosen duplicate, not its sibling ────────── + +test('deleting one duplicate leaves the same-named sibling registered and loaded', async ({ + page, +}) => { + // Delete retries below may wait out a server-side repo lock. + test.setTimeout(120_000); + + await connectTo(page, dupePaths[0]); + + // Record every DELETE the UI issues — the #2419 contract is that they all + // target exactly the chosen duplicate's path and NEVER the sibling's. + const deleteTargets: string[] = []; + page.on('request', (req) => { + if (req.method() === 'DELETE' && req.url().includes('/api/repo')) { + const target = new URL(req.url()).searchParams.get('repo'); + if (target) deleteTargets.push(target); + } + }); + + await page.locator('[data-testid="repo-switcher-trigger"]').click(); + const inactiveRow = page + .locator('[data-testid="repo-switcher-row"][data-active="false"]') + .filter({ hasText: DUPE_NAME }); + await expect(inactiveRow).toHaveCount(1); + + // Retry the whole click-and-verify block, because two pre-existing server + // races (both unrelated to the #2419 identity contract) can make a single + // click insufficient: a lingering analyze/embed job still holding the repo + // lock 409s the delete, and the registry's validate-prune path can clobber + // a concurrent unregister with its pre-delete snapshot, transiently + // resurrecting the entry after the UI has already dropped the row (in that + // case re-issue the delete by path, off-page, since the row is gone). + await expect(async () => { + if ((await inactiveRow.count()) > 0) { + // Delete icon is revealed on row hover. + await inactiveRow.hover(); + await inactiveRow.locator('[data-testid="repo-switcher-delete"]').click(); + } else if ((await listDupes()).includes(dupePaths[1])) { + await deleteRepoByPath(dupePaths[1]); + } + // Backend: exactly the inactive sibling is gone, the active one remains. + expect(await listDupes()).toEqual([dupePaths[0]]); + }).toPass({ timeout: 90_000, intervals: [2_000] }); + + // Identity: the UI's delete requests all targeted the chosen duplicate. + expect(deleteTargets.length).toBeGreaterThan(0); + expect([...new Set(deleteTargets)]).toEqual([dupePaths[1]]); + + // Frontend: the active repo is untouched — still Ready on the same path. + await expect(page.locator('[data-testid="status-ready"]')).toBeVisible(); + expect(new URL(page.url()).searchParams.get('repo')).toBe(dupePaths[0]); +}); diff --git a/gitnexus-web/src/App.tsx b/gitnexus-web/src/App.tsx index 6141104b5..2f6132c88 100644 --- a/gitnexus-web/src/App.tsx +++ b/gitnexus-web/src/App.tsx @@ -25,6 +25,16 @@ import { parseSkipGraphParam } from './lib/graph-load-decision'; import { formatBackendError } from './i18n/error-messages'; import { useTranslation } from 'react-i18next'; +/** + * Restore-param preference for the auto-connect effect: `repo` carries the + * server-resolved path identity (restores the exact repo even when duplicate + * display names exist, #2419), while older `project`-only URLs degrade to a + * name-based restore. Exported for direct unit testing — no test harness + * renders . + */ +export const pickRestoreRepo = (params: URLSearchParams): string | undefined => + params.get('repo') ?? params.get('project') ?? undefined; + const AppContent = () => { const { t } = useTranslation(['common', 'errors']); const { @@ -66,8 +76,9 @@ const AppContent = () => { result.repoInfo.name || (repoPath || '').replace(/\\/g, '/').split('/').filter(Boolean).pop() || 'server-project'; + const repoIdentity = repoPath || projectName; setProjectName(projectName); - setCurrentRepo(projectName); + setCurrentRepo(repoIdentity); // Build KnowledgeGraph from server data for visualization. In chat-only // mode the graph download was skipped, so the shared builder keeps an @@ -78,9 +89,15 @@ const AppContent = () => { setGraphMode(built.graphMode); setChatOnlyNodeCount(built.graphMode === 'chatOnly' ? built.nodeCount : null); - // Persist the active project in the URL for bookmarkability and F5 refresh resilience + // Persist the active project in the URL for bookmarkability and F5 refresh resilience. + // `repo` carries the server-resolved path identity (never the request-side + // string) so a refresh restores this exact repo even when duplicate display + // names exist (#2419); `project` stays as the readable display name. const urlObj = new URL(window.location.href); urlObj.searchParams.set('project', projectName); + if (repoPath) { + urlObj.searchParams.set('repo', repoPath); + } window.history.replaceState(null, '', urlObj.toString()); // Transition directly to exploring view @@ -90,7 +107,7 @@ const AppContent = () => { // chat-only flag so the agent's prompt matches the loaded/skipped graph (#2178). try { if (getActiveProviderConfig()) { - await initializeAgent(projectName, { chatOnly: result.graphSkipped }); + await initializeAgent(projectName, { chatOnly: result.graphSkipped, repo: repoIdentity }); } startEmbeddingsWithFallback(); } catch (err) { @@ -109,7 +126,11 @@ const AppContent = () => { ], ); - // Auto-connect when ?server or ?project query param is present (bookmarkable shortcut) + // Auto-connect when a ?server, ?repo or ?project query param is present + // (bookmarkable shortcut). A failed ?repo= restore (e.g. the bookmarked path + // was deleted) fails visibly via the error overlay → onboarding — it must + // NOT silently fall back to a name-based connect, which could reconnect a + // same-named sibling repo (#2419). const autoConnectRan = useRef(false); const tRef = useRef(t); useEffect(() => { @@ -120,12 +141,12 @@ const AppContent = () => { if (autoConnectRan.current) return; const params = new URLSearchParams(window.location.search); const serverUrlParam = params.get('server'); - const projectParam = params.get('project'); + const restoreRepoParam = pickRestoreRepo(params); // `?skipGraph=1` forces chat-only, `?skipGraph=0` forces a full graph; // absent → auto-detect by node count. Bookmarkable / survives F5 (#2178). const skipGraphParam = parseSkipGraphParam(params.get('skipGraph')); - if (!serverUrlParam && !projectParam) return; + if (!serverUrlParam && !restoreRepoParam) return; autoConnectRan.current = true; setProgress({ @@ -169,7 +190,7 @@ const AppContent = () => { } }, undefined, - projectParam || undefined, + restoreRepoParam, { awaitAnalysis: true, skipGraph: skipGraphParam }, // hold-queue + chat-only control (#2178) ); }; @@ -282,8 +303,11 @@ const AppContent = () => { setProgress(null); return; } catch (err: unknown) { - if (attempt === 0 && err instanceof BackendError && err.status === 404) { - // Server may still be reinitializing — wait and retry + // Server may still be reinitializing after the worker completed: + // that surfaces as a 404 (repo not registered yet) OR a transient + // 5xx/binder error while the freshly-written DB becomes readable. + // Either way, wait and retry once before giving up. + if (attempt === 0 && err instanceof BackendError) { await new Promise((r) => setTimeout(r, 1500)); continue; } diff --git a/gitnexus-web/src/components/CodeReferencesPanel.tsx b/gitnexus-web/src/components/CodeReferencesPanel.tsx index bd416f9fe..5818614f9 100644 --- a/gitnexus-web/src/components/CodeReferencesPanel.tsx +++ b/gitnexus-web/src/components/CodeReferencesPanel.tsx @@ -57,6 +57,7 @@ export const CodeReferencesPanel = ({ onFocusNode }: CodeReferencesPanelProps) = setSelectedNode, codeReferenceFocus, projectName, + currentRepo, } = useAppState(); const nodeById = useMemo(() => { @@ -226,14 +227,15 @@ export const CodeReferencesPanel = ({ onFocusNode }: CodeReferencesPanelProps) = const isWholeFile = selectedIsFile || startLine === undefined; const options = isWholeFile - ? { repo: projectName } + ? {} : { startLine: Math.max(0, startLine - CONTEXT_LINES), endLine: (endLine ?? startLine) + CONTEXT_LINES, - repo: projectName, }; - readFile(selectedFilePath, { ...options, repo: projectName || undefined }) + // Prefer the repo path identity over the display name — duplicate display + // names would otherwise resolve to the wrong repository's file (#2420). + readFile(selectedFilePath, { ...options, repo: currentRepo || projectName || undefined }) .then((result) => { if (!cancelled) { setFileResult(result); @@ -256,6 +258,7 @@ export const CodeReferencesPanel = ({ onFocusNode }: CodeReferencesPanelProps) = selectedNode?.properties?.endLine, selectedIsFile, projectName, + currentRepo, ]); // Scroll to the selected node's startLine after content loads diff --git a/gitnexus-web/src/components/Header.tsx b/gitnexus-web/src/components/Header.tsx index 5a9ce4277..8e8d41d08 100644 --- a/gitnexus-web/src/components/Header.tsx +++ b/gitnexus-web/src/components/Header.tsx @@ -15,6 +15,7 @@ import { useAppState } from '../hooks/useAppState'; import { deleteRepo, fetchRepos, + repoIdentity, startAnalyze, streamAnalyzeProgress, type BackendRepo, @@ -62,6 +63,7 @@ export const Header = ({ const { t } = useTranslation(['common', 'header', 'errors']); const { projectName, + currentRepo, graph, graphMode, openChatPanel, @@ -74,7 +76,7 @@ export const Header = ({ const [repoSearchQuery, setRepoSearchQuery] = useState(''); const [isRepoDropdownOpen, setIsRepoDropdownOpen] = useState(false); const [showAnalyzer, setShowAnalyzer] = useState(false); - const [reanalyzing, setReanalyzing] = useState(null); // repo name being re-analyzed + const [reanalyzing, setReanalyzing] = useState(null); // repo identity being re-analyzed const [deleteError, setDeleteError] = useState(null); // surfaced when a delete is rejected (e.g. origin-blocked 403) const [reanalyzeProgress, setReanalyzeProgress] = useState(null); const reanalyzeSseRef = useRef(null); @@ -104,6 +106,8 @@ export const Header = ({ return availableRepos.filter((repo) => repo.name.toLowerCase().includes(query)); }, [availableRepos, repoSearchQuery]); + const activeRepoIdentity = currentRepo ?? projectName; + // Handle clicking outside search or repo dropdown to close them useEffect(() => { const handleClickOutside = (e: MouseEvent) => { @@ -187,6 +191,7 @@ export const Header = ({ {projectName && (
) : ( - filteredRepos.map((repo) => ( -
- - {/* Re-analyze */} - - {/* Delete */} - + {/* Re-analyze */} + -
- )) + > + + + {/* Delete */} + + + ); + }) )} @@ -386,7 +416,13 @@ export const Header = ({ {t('header:reanalyzingRepo', { - repoName: reanalyzing, + // `reanalyzing` holds the path identity (#2419) — + // resolve the display name for the label, falling + // back to the path basename. + repoName: + availableRepos.find((r) => repoIdentity(r) === reanalyzing)?.name ?? + reanalyzing.split(/[/\\]/).filter(Boolean).at(-1) ?? + reanalyzing, message: translateProgressMessage(reanalyzeProgress.message, t), })} diff --git a/gitnexus-web/src/components/RepoAnalyzer.tsx b/gitnexus-web/src/components/RepoAnalyzer.tsx index fd08d197d..1aca3a64a 100644 --- a/gitnexus-web/src/components/RepoAnalyzer.tsx +++ b/gitnexus-web/src/components/RepoAnalyzer.tsx @@ -183,7 +183,12 @@ type InternalPhase = 'input' | 'starting' | 'analyzing' | 'done' | 'error'; export interface RepoAnalyzerProps { variant: 'onboarding' | 'sheet'; - onComplete: (repoName: string) => void; + /** + * Receives the repo IDENTITY to reconnect with — the analyzed path when the + * server provides one (`repoPath` on the SSE complete event), otherwise the + * display name. Never rendered; the done screen shows the display name. + */ + onComplete: (repoIdentity: string) => void; onCancel?: () => void; } @@ -360,19 +365,25 @@ export const RepoAnalyzer = ({ variant, onComplete, onCancel }: RepoAnalyzerProp jobId, (p) => setProgress(p), (data) => { - const name = + // Display vs identity split: the done screen renders the display name + // (never an absolute path), while onComplete receives the identity — + // the analyzed path when the server provides it, so the reconnect + // targets the exact repo even when basenames collide. Old servers omit + // repoPath and degrade to today's name behavior. + const displayName = data.repoName ?? (fallbackNameSource ? fallbackNameSource.split(/[/\\]/).filter(Boolean).at(-1) : undefined) ?? t('onboarding:repoAnalyzer.defaultRepoName'); - setCompletedRepoName(name); + const identity = data.repoPath ?? displayName; + setCompletedRepoName(displayName); setGithubToken(''); setPhase('done'); sseControllerRef.current = null; completeTimerRef.current = setTimeout(() => { completeTimerRef.current = null; - onComplete(name); + onComplete(identity); }, 1200); }, (errMsg) => { diff --git a/gitnexus-web/src/components/RepoLanding.tsx b/gitnexus-web/src/components/RepoLanding.tsx index 600312a44..b2b85b259 100644 --- a/gitnexus-web/src/components/RepoLanding.tsx +++ b/gitnexus-web/src/components/RepoLanding.tsx @@ -14,7 +14,7 @@ import { Sparkles, ArrowRight, GitBranch, FileCode, Layers } from '@/lib/lucide-icons'; import { RepoAnalyzer } from './RepoAnalyzer'; -import type { BackendRepo } from '../services/backend-client'; +import { repoIdentity, type BackendRepo } from '../services/backend-client'; import type { TFunction } from 'i18next'; import { useTranslation } from 'react-i18next'; @@ -126,7 +126,11 @@ export const RepoLanding = ({ repos, onSelectRepo, onAnalyzeComplete }: RepoLand {/* Repo list */}
{repos.map((repo) => ( - onSelectRepo(repo.name)} /> + onSelectRepo(repoIdentity(repo))} + /> ))}
diff --git a/gitnexus-web/src/hooks/useAppState.tsx b/gitnexus-web/src/hooks/useAppState.tsx index e3a3daaec..a14be99f2 100644 --- a/gitnexus-web/src/hooks/useAppState.tsx +++ b/gitnexus-web/src/hooks/useAppState.tsx @@ -35,6 +35,9 @@ import { startEmbeddings as backendStartEmbeddings, streamEmbeddingProgress, probeBackend, + // Aliased: switchRepo declares a local `let repoIdentity` that would shadow + // a plain named import of this helper. + repoIdentity as repoIdentityOf, type BackendRepo, type ConnectResult, type JobProgress, @@ -52,6 +55,15 @@ export const shouldAutoStartEmbeddings = (): boolean => { return window.localStorage.getItem(AUTO_START_EMBEDDINGS_STORAGE_KEY) === 'true'; }; +// Resolve a human-readable name for a repo path identity: the registry entry's +// display name first, then the path's basename, then the raw identity. State +// keeps holding the path identity (#2419) — user-facing labels and the agent +// prompt must never show an absolute filesystem path. +const displayNameForIdentity = (repos: BackendRepo[], identity: string): string => + repos.find((r) => repoIdentityOf(r) === identity)?.name ?? + identity.split(/[/\\]/).filter(Boolean).at(-1) ?? + identity; + export type ViewMode = 'onboarding' | 'loading' | 'exploring'; export type RightPanelTab = 'code' | 'chat'; export type EmbeddingStatus = 'idle' | 'loading' | 'embedding' | 'indexing' | 'ready' | 'error'; @@ -162,6 +174,7 @@ interface AppState { // Project info projectName: string; setProjectName: (name: string) => void; + currentRepo: string | undefined; // Multi-repo switching serverBaseUrl: string | null; @@ -169,7 +182,7 @@ interface AppState { availableRepos: BackendRepo[]; setAvailableRepos: (repos: BackendRepo[]) => void; switchRepo: (repoName: string) => Promise; - setCurrentRepo: (repoName: string) => void; + setCurrentRepo: (repoName: string | undefined) => void; /** Download the full graph for the current repo after a chat-only connect (#2178). */ loadGraphAnyway: () => Promise; @@ -204,7 +217,10 @@ interface AppState { // LLM methods refreshLLMSettings: () => void; - initializeAgent: (overrideProjectName?: string, opts?: { chatOnly?: boolean }) => Promise; + initializeAgent: ( + overrideProjectName?: string, + opts?: { chatOnly?: boolean; repo?: string }, + ) => Promise; sendChatMessage: (message: string) => Promise; stopChatResponse: () => void; clearChat: () => void; @@ -346,6 +362,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { // Project info const [projectName, setProjectName] = useState(''); + const [currentRepo, setCurrentRepoState] = useState(undefined); // Multi-repo switching const [serverBaseUrl, setServerBaseUrl] = useState(null); @@ -489,8 +506,9 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { // Backend client — direct HTTP calls (no Worker/Comlink) const repoRef = useRef(undefined); - const setCurrentRepo = useCallback((repoName: string) => { + const setCurrentRepo = useCallback((repoName: string | undefined) => { repoRef.current = repoName; + setCurrentRepoState(repoName); }, []); const runQuery = useCallback(async (cypher: string): Promise => { @@ -613,7 +631,10 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { }, [graphMode]); const initializeAgent = useCallback( - async (overrideProjectName?: string, opts?: { chatOnly?: boolean }): Promise => { + async ( + overrideProjectName?: string, + opts?: { chatOnly?: boolean; repo?: string }, + ): Promise => { const config = getActiveProviderConfig(); if (!config) { setAgentError('Please configure an LLM provider in settings'); @@ -632,8 +653,11 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { // Sync repoRef so all agent backend calls target the correct repo. // initializeAgent can be called from App.tsx (handleServerConnect) which // never sets repoRef.current directly — without this, queries default to repo[0]. - if (overrideProjectName) { - repoRef.current = overrideProjectName; + // Only opts.repo may write the identity: overrideProjectName is a display + // name, and a name-only caller must never clobber the path identity with + // an ambiguous name (#2419). + if (opts?.repo) { + setCurrentRepo(opts.repo); } const repo = repoRef.current; @@ -1161,7 +1185,10 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { phase: 'extracting', percent: 0, message: i18n.t('common:progress.switchingRepository'), - detail: i18n.t('common:progress.loadingRepository', { repo: repoName }), + detail: i18n.t('common:progress.loadingRepository', { + // `repoName` is a path identity — show the display name, not the path. + repo: displayNameForIdentity(availableRepos, repoName), + }), }); setViewMode('loading'); setIsAgentReady(false); @@ -1184,7 +1211,10 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { setChatOnlyNodeCount(null); let connectedRepo: BackendRepo | undefined; - let pNameStr = repoName || 'server-project'; + // Bare declarations: both are always assigned on the success path before + // any read, and the catch below returns early (CodeQL alerts 825/826). + let pNameStr: string; + let repoIdentity: string | undefined; let connectedChatOnly = false; try { @@ -1225,12 +1255,13 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { const repoPath = result.repoInfo.repoPath ?? result.repoInfo.path; // Prefer the registry name, then normalize Windows \ and Unix / paths const pName = - repoName || result.repoInfo.name || (repoPath || '').replace(/\\/g, '/').split('/').filter(Boolean).pop() || + repoName || 'server-project'; + repoIdentity = repoName || repoPath || pName; setProjectName(pName); - repoRef.current = pName; + setCurrentRepo(repoIdentity); connectedRepo = result.repoInfo; pNameStr = pName; @@ -1262,11 +1293,19 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { if (pNameStr) { // Persist the selected project in the URL so a refresh re-opens it. + // `repo` carries the server-resolved path identity (never the + // request-side string) so the refresh restores this exact repo even + // when duplicate display names exist (#2419); `project` stays as the + // readable display name. // Drop any `?skipGraph` override: a deliberate repo switch should make a // fresh per-repo decision (auto-detect) on the next refresh rather than // carry the previous repo's forced mode (#2178). const urlObj = new URL(window.location.href); urlObj.searchParams.set('project', pNameStr); + const resolvedRepoPath = connectedRepo?.repoPath ?? connectedRepo?.path; + if (resolvedRepoPath) { + urlObj.searchParams.set('repo', resolvedRepoPath); + } urlObj.searchParams.delete('skipGraph'); window.history.replaceState(null, '', urlObj.toString()); } @@ -1279,7 +1318,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { // Re-initialize agent with the new repo's graph context try { if (getActiveProviderConfig()) { - await initializeAgent(pNameStr, { chatOnly: connectedChatOnly }); + await initializeAgent(pNameStr, { chatOnly: connectedChatOnly, repo: repoIdentity }); } setViewMode('exploring'); startEmbeddingsWithFallback(); @@ -1295,6 +1334,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { }, [ serverBaseUrl, + availableRepos, setProgress, setViewMode, setProjectName, @@ -1313,6 +1353,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { setCodePanelOpen, setCodeReferenceFocus, setChatMessages, + setCurrentRepo, ], ); @@ -1390,7 +1431,14 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { // the chat-only note (#2178, KTD2). Guarded on a configured provider, like // switchRepo; runs inside the mounted/stale guard above. if (getActiveProviderConfig()) { - await initializeAgent(repo, { chatOnly: false }); + // Pass the display name explicitly — initializeAgent's empty-deps + // closure traps `projectName` at its initial '', so relying on the + // state fallback would label the prompt the literal 'project'. The + // path identity travels separately via opts.repo. + await initializeAgent(repo ? displayNameForIdentity(availableRepos, repo) : undefined, { + chatOnly: false, + repo, + }); } } catch (err) { if (!loadGraphMountedRef.current || repoRef.current !== repo) return; @@ -1404,6 +1452,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { } }, [ serverBaseUrl, + availableRepos, setProgress, setViewMode, setGraph, @@ -1495,6 +1544,7 @@ const AppStateProviderInner = ({ children }: { children: ReactNode }) => { setProgress, projectName, setProjectName, + currentRepo, // Multi-repo switching serverBaseUrl, setServerBaseUrl, diff --git a/gitnexus-web/src/services/backend-client.ts b/gitnexus-web/src/services/backend-client.ts index 09b7dfffe..b4a9ebb2c 100644 --- a/gitnexus-web/src/services/backend-client.ts +++ b/gitnexus-web/src/services/backend-client.ts @@ -28,6 +28,13 @@ export interface BackendRepo { }; } +/** + * Canonical repo identity: the registry path. The display `name` is ambiguous + * across duplicate repo names (#2419); `repoPath` is the normalized field and + * `path` the legacy list-endpoint field. + */ +export const repoIdentity = (repo: BackendRepo): string => repo.repoPath ?? repo.path ?? repo.name; + export interface EnrichedSearchResult { filePath: string; score: number; @@ -535,7 +542,8 @@ export const probeBackend = async (): Promise => { export const fetchRepos = async (): Promise => { const response = await fetchWithTimeout(`${_backendUrl}/api/repos`); await assertOk(response); - return response.json() as Promise; + const repos = (await response.json()) as BackendRepo[]; + return repos.map((r) => ({ ...r, repoPath: r.repoPath ?? r.path })); }; /** Fetch repo metadata. @@ -891,7 +899,7 @@ export const cancelAnalyze = async (jobId: string): Promise => { export const streamAnalyzeProgress = ( jobId: string, onProgress: (progress: JobProgress) => void, - onComplete: (data: { repoName?: string }) => void, + onComplete: (data: { repoName?: string; repoPath?: string }) => void, onError: (error: string) => void, ): AbortController => { return streamSSE( @@ -940,7 +948,7 @@ export const cancelEmbeddings = async (jobId: string): Promise => { export const streamEmbeddingProgress = ( jobId: string, onProgress: (progress: JobProgress) => void, - onComplete: (data: { repoName?: string }) => void, + onComplete: (data: { repoName?: string; repoPath?: string }) => void, onError: (error: string) => void, ): AbortController => { return streamSSE(`${_backendUrl}/api/embed/${encodeURIComponent(jobId)}/progress`, { diff --git a/gitnexus-web/test/unit/backend-client-repo-identity.test.ts b/gitnexus-web/test/unit/backend-client-repo-identity.test.ts new file mode 100644 index 000000000..c0682f545 --- /dev/null +++ b/gitnexus-web/test/unit/backend-client-repo-identity.test.ts @@ -0,0 +1,108 @@ +/** + * Canonical repo identity (#2419). + * + * `repoIdentity` is the single identity helper for the web app: the registry + * path is canonical because the display `name` is ambiguous across duplicate + * repo names. `fetchRepos` must normalize legacy list-endpoint entries + * (`path` only) onto the `repoPath` field, mirroring `fetchRepoInfo`. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { __resetBreakerRegistry__ } from 'gitnexus-shared/test-helpers'; +import { + fetchRepos, + repoIdentity, + setBackendUrl, + type BackendRepo, +} from '../../src/services/backend-client'; + +const BASE = 'http://repo-identity.test:4747'; + +describe('repoIdentity fallback chain', () => { + it('prefers repoPath when present', () => { + const repo: BackendRepo = { + name: 'reels', + path: '/ws/group-a/reels', + repoPath: '/ws/group-b/reels', + indexedAt: '2026-07-10T00:00:00.000Z', + }; + expect(repoIdentity(repo)).toBe('/ws/group-b/reels'); + }); + + it('falls back to path when repoPath is absent', () => { + const repo: BackendRepo = { + name: 'reels', + path: '/ws/group-a/reels', + indexedAt: '2026-07-10T00:00:00.000Z', + }; + expect(repoIdentity(repo)).toBe('/ws/group-a/reels'); + }); + + it('falls back to the display name when neither path field is present', () => { + // Legacy payloads can omit both path fields at runtime even though the + // interface marks `path` required — assert the narrow legacy shape to + // exercise the final fallback without weakening the helper's signature. + const legacyRepo = { + name: 'reels', + indexedAt: '2026-07-10T00:00:00.000Z', + } as BackendRepo; + expect(repoIdentity(legacyRepo)).toBe('reels'); + }); +}); + +describe('fetchRepos repoPath normalization', () => { + beforeEach(() => { + __resetBreakerRegistry__(); + setBackendUrl(BASE); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it('maps legacy path-only entries onto repoPath', async () => { + const legacyBody = JSON.stringify([ + { name: 'reels', path: '/ws/group-a/reels', indexedAt: '2026-07-10T00:00:00.000Z' }, + { name: 'docs', path: '/ws/group-b/docs', indexedAt: '2026-07-09T00:00:00.000Z' }, + ]); + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + expect(url).toContain('/api/repos'); + return new Response(legacyBody, { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }); + }); + vi.stubGlobal('fetch', fetchMock); + + const repos = await fetchRepos(); + expect(repos).toMatchObject([ + { name: 'reels', path: '/ws/group-a/reels', repoPath: '/ws/group-a/reels' }, + { name: 'docs', path: '/ws/group-b/docs', repoPath: '/ws/group-b/docs' }, + ]); + }); + + it('keeps a server-provided repoPath over the legacy path field', async () => { + const body = JSON.stringify([ + { + name: 'reels', + path: '/ws/group-a/reels', + repoPath: '/ws/group-b/reels', + indexedAt: '2026-07-10T00:00:00.000Z', + }, + ]); + vi.stubGlobal( + 'fetch', + vi.fn( + async () => + new Response(body, { + status: 200, + headers: { 'Content-Type': 'application/json' }, + }), + ), + ); + + const repos = await fetchRepos(); + expect(repos).toMatchObject([{ name: 'reels', repoPath: '/ws/group-b/reels' }]); + }); +}); diff --git a/gitnexus-web/test/unit/code-references-panel.test.tsx b/gitnexus-web/test/unit/code-references-panel.test.tsx new file mode 100644 index 000000000..e5c6b3e88 --- /dev/null +++ b/gitnexus-web/test/unit/code-references-panel.test.tsx @@ -0,0 +1,73 @@ +import { render } from '@testing-library/react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import type { ReactNode } from 'react'; +import type { GraphNode } from 'gitnexus-shared'; +import { CodeReferencesPanel } from '../../src/components/CodeReferencesPanel'; +import { readFile } from '../../src/services/backend-client'; + +const fileNode: GraphNode = { + id: 'File:src/foo.ts', + label: 'File', + properties: { name: 'foo.ts', filePath: 'src/foo.ts' }, +}; + +// Mutable mock state: the useAppState factory closes over this object so each +// test can reassign fields (e.g. currentRepo) before rendering. +const appState = { + graph: null, + selectedNode: fileNode, + codeReferences: [], + removeCodeReference: vi.fn(), + clearCodeReferences: vi.fn(), + setSelectedNode: vi.fn(), + codeReferenceFocus: null, + projectName: 'reels', + currentRepo: undefined as string | undefined, +}; + +vi.mock('../../src/hooks/useAppState', () => ({ + useAppState: () => appState, +})); + +vi.mock('../../src/services/backend-client', () => ({ + readFile: vi.fn(), +})); + +vi.mock('react-syntax-highlighter', () => ({ + Prism: ({ children }: { children?: ReactNode }) => 
{children}
, +})); + +vi.mock('react-syntax-highlighter/dist/esm/styles/prism', () => ({ + vscDarkPlus: {}, +})); + +vi.mock('react-i18next', () => ({ + useTranslation: () => ({ + t: (key: string) => key, + }), +})); + +describe('CodeReferencesPanel repo identity (#2420)', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(readFile).mockResolvedValue({ content: 'const a = 1;', totalLines: 1 }); + }); + + it('reads the selected file from the active repo path, not the display name', () => { + appState.currentRepo = '/ws/b/reels'; + appState.projectName = 'reels'; + + render(); + + expect(readFile).toHaveBeenCalledWith('src/foo.ts', { repo: '/ws/b/reels' }); + }); + + it('falls back to the project display name when no repo path is active', () => { + appState.currentRepo = undefined; + appState.projectName = 'reels'; + + render(); + + expect(readFile).toHaveBeenCalledWith('src/foo.ts', { repo: 'reels' }); + }); +}); diff --git a/gitnexus-web/test/unit/header.test.tsx b/gitnexus-web/test/unit/header.test.tsx index 1a45bd705..94190348b 100644 --- a/gitnexus-web/test/unit/header.test.tsx +++ b/gitnexus-web/test/unit/header.test.tsx @@ -1,12 +1,19 @@ import { fireEvent, render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; -import { describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { Header } from '../../src/components/Header'; +import { + deleteRepo, + fetchRepos, + startAnalyze, + streamAnalyzeProgress, +} from '../../src/services/backend-client'; import type { BackendRepo } from '../../src/services/backend-client'; vi.mock('../../src/hooks/useAppState', () => ({ useAppState: () => ({ projectName: 'reels', + currentRepo: '/workspace/group-b/reels', graph: null, graphMode: 'full', openChatPanel: vi.fn(), @@ -46,6 +53,8 @@ vi.mock('react-i18next', () => ({ if (key === 'header:repositories') return 'Repositories'; if (key === 'header:active') return 'Active'; if (key === 'header:reanalyzeRepo') return `Re-analyze ${options?.repoName ?? ''}`; + if (key === 'header:reanalyzingRepo') + return `Re-analyzing ${options?.repoName ?? ''}: ${options?.message ?? ''}`; if (key === 'header:deleteRepo') return `Delete ${options?.repoName ?? ''}`; if (key === 'header:analyzeNew') return 'Analyze new'; if (key === 'header:searchRepositories') return 'Search repositories...'; @@ -71,6 +80,16 @@ function makeRepo(index: number): BackendRepo { } describe('Header', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + // Reset the URL mutated by the delete handler's hygiene pass. + window.history.replaceState(null, '', '/'); + }); + it('keeps a large repository menu scrollable inside the viewport', () => { render(
makeRepo(index))} />); @@ -152,4 +171,157 @@ describe('Header', () => { expect(repoList).not.toHaveTextContent('feed_sync'); expect(repoList).not.toHaveTextContent('reels'); }); + + it('uses repository path identity when duplicate display names are present', async () => { + const onSwitchRepo = vi.fn(); + render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + + expect(screen.getAllByText('Active')).toHaveLength(1); + await userEvent.click(screen.getAllByText('reels')[1]); + + expect(onSwitchRepo).toHaveBeenCalledWith('/workspace/group-a/reels'); + }); + + it('deletes and falls back using repository path identity', async () => { + const onSwitchRepo = vi.fn(); + const updatedRepos = [{ ...makeRepo(2), name: 'reels', path: '/workspace/group-a/reels' }]; + vi.mocked(fetchRepos).mockResolvedValue(updatedRepos); + + render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + await userEvent.click(screen.getAllByTitle('Delete reels')[1]); + + expect(deleteRepo).toHaveBeenCalledWith('/workspace/group-b/reels'); + expect(onSwitchRepo).toHaveBeenCalledWith('/workspace/group-a/reels'); + }); + + it('strips repo, project and skipGraph from the URL before reloading after the last repo is deleted', async () => { + window.history.replaceState( + null, + '', + '/?repo=%2Fworkspace%2Fgroup-b%2Freels&project=reels&skipGraph=1', + ); + // jsdom's location.reload is own+non-configurable — replace the whole + // `location` accessor with a stub that delegates URL reads to the real + // Location (kept live by history.replaceState) and mocks reload. + const realLocation = window.location; + const reloadMock = vi.fn(); + vi.stubGlobal('location', { + get href() { + return realLocation.href; + }, + get search() { + return realLocation.search; + }, + reload: reloadMock, + }); + vi.mocked(fetchRepos).mockResolvedValue([]); + + render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + await userEvent.click(screen.getByTitle('Delete reels')); + + expect(reloadMock).toHaveBeenCalledTimes(1); + expect(window.location.search).not.toContain('repo='); + expect(window.location.search).not.toContain('project='); + expect(window.location.search).not.toContain('skipGraph'); + }); + + it('strips repo and project from the URL before falling back after deleting the active repo', async () => { + window.history.replaceState(null, '', '/?repo=%2Fworkspace%2Fgroup-b%2Freels&project=reels'); + // Capture the URL at the moment of the fallback switch — the stale + // identity must already be gone so a failed switch leaves nothing that + // restores the deleted repo on refresh (#2419). + const searchAtSwitch: string[] = []; + const onSwitchRepo = vi.fn(() => { + searchAtSwitch.push(window.location.search); + }); + vi.mocked(fetchRepos).mockResolvedValue([ + { ...makeRepo(2), name: 'reels', path: '/workspace/group-a/reels' }, + ]); + + render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + await userEvent.click(screen.getAllByTitle('Delete reels')[1]); + + expect(deleteRepo).toHaveBeenCalledWith('/workspace/group-b/reels'); + expect(onSwitchRepo).toHaveBeenCalledWith('/workspace/group-a/reels'); + expect(searchAtSwitch).toEqual(['']); + }); + + it('shows the display name, not the path identity, in the re-analyze progress label', async () => { + vi.mocked(startAnalyze).mockResolvedValue({ jobId: 'job-1', status: 'running' }); + vi.mocked(streamAnalyzeProgress).mockReturnValue(new AbortController()); + + render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + // Re-analyze the second duplicate-name row — `reanalyzing` becomes the + // path identity '/ws/b/reels', but the label must render the name. + await userEvent.click(screen.getAllByTitle('Re-analyze reels')[1]); + + const label = screen.getByText(/^Re-analyzing /); + expect(label.textContent).toMatch(/^Re-analyzing reels:/); + expect(label.textContent).not.toContain('/ws/b/reels'); + }); + + it('falls back to the path basename when the re-analyzing identity is no longer listed', async () => { + vi.mocked(startAnalyze).mockResolvedValue({ jobId: 'job-2', status: 'running' }); + vi.mocked(streamAnalyzeProgress).mockReturnValue(new AbortController()); + + const { rerender } = render( +
, + ); + + fireEvent.click(screen.getByRole('button', { name: /reels/i })); + await userEvent.click(screen.getByTitle('Re-analyze reels')); + + // The repo list refreshes while the re-analysis is still in flight and the + // identity disappears from it — the label degrades to the path basename. + rerender(
); + + const label = screen.getByText(/^Re-analyzing /); + expect(label.textContent).toMatch(/^Re-analyzing reels:/); + expect(label.textContent).not.toContain('/ws/b/reels'); + }); }); diff --git a/gitnexus-web/test/unit/initialize-agent-identity.test.tsx b/gitnexus-web/test/unit/initialize-agent-identity.test.tsx new file mode 100644 index 000000000..5e8eddbd7 --- /dev/null +++ b/gitnexus-web/test/unit/initialize-agent-identity.test.tsx @@ -0,0 +1,79 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { AppStateProvider, useAppState } from '../../src/hooks/useAppState'; +import { getActiveProviderConfig } from '../../src/core/llm/settings-service'; +import type { CodebaseContext } from '../../src/core/llm/context-builder'; + +// initializeAgent's heavy dynamic imports are stubbed — these tests only lock +// the identity-write rule, not agent behavior. +vi.mock('../../src/core/llm/context-builder', () => ({ + buildCodebaseContext: vi.fn( + async (): Promise => ({ + stats: { + projectName: 'stub', + fileCount: 0, + functionCount: 0, + classCount: 0, + interfaceCount: 0, + methodCount: 0, + }, + hotspots: [], + folderTree: '', + }), + ), +})); + +vi.mock('../../src/core/llm/agent', () => ({ + createGraphRAGAgent: vi.fn(() => ({})), +})); + +vi.mock('../../src/core/llm/settings-service', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, getActiveProviderConfig: vi.fn(actual.getActiveProviderConfig) }; +}); + +afterEach(() => { + vi.restoreAllMocks(); +}); + +const withProvider = () => { + vi.mocked(getActiveProviderConfig).mockReturnValue({ + provider: 'openai', + model: 'gpt-4o', + apiKey: 'test-key', + }); +}; + +describe('initializeAgent repo-identity writes (#2419)', () => { + it('does not clobber the path identity when called with only a display name', async () => { + withProvider(); + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + + act(() => { + result.current.setCurrentRepo('/ws/b/reels'); + }); + + await act(async () => { + await result.current.initializeAgent('reels'); + }); + + // The pre-PR idiom initializeAgent(projectName) must no longer overwrite + // the path identity with an ambiguous display name. + expect(result.current.currentRepo).toBe('/ws/b/reels'); + }); + + it('writes the identity when opts.repo is provided', async () => { + withProvider(); + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + + act(() => { + result.current.setCurrentRepo('/ws/a/reels'); + }); + + await act(async () => { + await result.current.initializeAgent('reels', { repo: '/ws/b/reels' }); + }); + + expect(result.current.currentRepo).toBe('/ws/b/reels'); + }); +}); diff --git a/gitnexus-web/test/unit/load-graph-anyway.test.tsx b/gitnexus-web/test/unit/load-graph-anyway.test.tsx index 10c15c9cc..b7c41dd2d 100644 --- a/gitnexus-web/test/unit/load-graph-anyway.test.tsx +++ b/gitnexus-web/test/unit/load-graph-anyway.test.tsx @@ -1,6 +1,38 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { renderHook, act } from '@testing-library/react'; import { AppStateProvider, useAppState } from '../../src/hooks/useAppState'; +import { getActiveProviderConfig } from '../../src/core/llm/settings-service'; +import { buildCodebaseContext, type CodebaseContext } from '../../src/core/llm/context-builder'; + +// Capture initializeAgent's observable seam: buildCodebaseContext receives the +// effective project name that ends up in the agent's system prompt. +vi.mock('../../src/core/llm/context-builder', () => ({ + buildCodebaseContext: vi.fn( + async (): Promise => ({ + stats: { + projectName: 'stub', + fileCount: 0, + functionCount: 0, + classCount: 0, + interfaceCount: 0, + methodCount: 0, + }, + hotspots: [], + folderTree: '', + }), + ), +})); + +vi.mock('../../src/core/llm/agent', () => ({ + createGraphRAGAgent: vi.fn(() => ({})), +})); + +vi.mock('../../src/core/llm/settings-service', async (importOriginal) => { + const actual = await importOriginal(); + // Wrap with the real implementation so tests without an explicit override + // keep today's no-provider (null) behavior. + return { ...actual, getActiveProviderConfig: vi.fn(actual.getActiveProviderConfig) }; +}); afterEach(() => { vi.restoreAllMocks(); @@ -170,6 +202,81 @@ describe('loadGraphAnyway (chat-only escape hatch, #2178)', () => { expect(result.current.graphMode).toBe('chatOnly'); }); + it('re-initializes the agent with the looked-up display name and the path identity', async () => { + vi.mocked(getActiveProviderConfig).mockReturnValue({ + provider: 'openai', + model: 'gpt-4o', + apiKey: 'test-key', + }); + const fetchMock = vi.fn((url: string) => { + if (url.includes('/api/repo')) return Promise.resolve(repoInfoResponse()); + if (url.includes('/api/graph')) return Promise.resolve(graphNdjsonResponse()); + return Promise.resolve( + new Response('{}', { status: 200, headers: { 'Content-Type': 'application/json' } }), + ); + }); + vi.stubGlobal('fetch', fetchMock); + + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + act(() => { + result.current.setServerBaseUrl('http://localhost:4747'); + result.current.setAvailableRepos([ + { + name: 'reels-display', + path: '/r/big-repo', + repoPath: '/r/big-repo', + indexedAt: '2026-06-13T00:00:00Z', + }, + ]); + result.current.setCurrentRepo('/r/big-repo'); + result.current.setGraphMode('chatOnly'); + }); + + await act(async () => { + await result.current.loadGraphAnyway(); + }); + + // The agent prompt gets the human-readable display name — never the + // absolute path, and never the 'project' literal that initializeAgent's + // empty-deps closure would fall back to (projectName is trapped at ''). + expect(vi.mocked(buildCodebaseContext)).toHaveBeenCalledWith( + expect.any(Function), + 'reels-display', + ); + // The repo identity itself stays the path (threaded via opts.repo). + expect(result.current.currentRepo).toBe('/r/big-repo'); + }); + + it('falls back to the identity basename for the agent prompt when the repo list misses it', async () => { + vi.mocked(getActiveProviderConfig).mockReturnValue({ + provider: 'openai', + model: 'gpt-4o', + apiKey: 'test-key', + }); + const fetchMock = vi.fn((url: string) => { + if (url.includes('/api/repo')) return Promise.resolve(repoInfoResponse()); + if (url.includes('/api/graph')) return Promise.resolve(graphNdjsonResponse()); + return Promise.resolve( + new Response('{}', { status: 200, headers: { 'Content-Type': 'application/json' } }), + ); + }); + vi.stubGlobal('fetch', fetchMock); + + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + act(() => { + result.current.setServerBaseUrl('http://localhost:4747'); + result.current.setCurrentRepo('/r/big-repo'); + result.current.setGraphMode('chatOnly'); + }); + + await act(async () => { + await result.current.loadGraphAnyway(); + }); + + expect(vi.mocked(buildCodebaseContext)).toHaveBeenCalledWith(expect.any(Function), 'big-repo'); + expect(result.current.currentRepo).toBe('/r/big-repo'); + }); + it('does not throw or apply state when unmounted mid-load', async () => { let resolveGraph: (r: Response) => void = () => {}; const graphPromise = new Promise((res) => { diff --git a/gitnexus-web/test/unit/repo-analyzer-complete-identity.test.tsx b/gitnexus-web/test/unit/repo-analyzer-complete-identity.test.tsx new file mode 100644 index 000000000..ce0089c87 --- /dev/null +++ b/gitnexus-web/test/unit/repo-analyzer-complete-identity.test.tsx @@ -0,0 +1,103 @@ +/** + * Analyze completion: display vs identity split (PR #2420 review R2/R7). + * + * The SSE complete event may carry `repoPath` (the analyzed path). RepoAnalyzer + * must pass that IDENTITY to onComplete — so the post-analyze reconnect targets + * the exact repo even when basenames collide — while the done screen keeps + * rendering the display NAME and never shows an absolute path. Old servers + * omit repoPath; the name fallback must be preserved. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { act, fireEvent, render, screen } from '@testing-library/react'; +import { RepoAnalyzer } from '../../src/components/RepoAnalyzer'; +import { i18nReady } from '../../src/i18n'; +import { + cancelAnalyze, + streamAnalyzeProgress, + uploadFolder, +} from '../../src/services/backend-client'; + +vi.mock('../../src/services/backend-client', () => ({ + startAnalyze: vi.fn(), + cancelAnalyze: vi.fn(), + streamAnalyzeProgress: vi.fn(), + uploadFolder: vi.fn(), +})); + +const JOB = { jobId: 'job-1', status: 'queued' }; + +type CompleteData = { repoName?: string; repoPath?: string }; + +beforeEach(async () => { + await i18nReady; + vi.clearAllMocks(); + vi.mocked(cancelAnalyze).mockResolvedValue(undefined as never); + vi.mocked(uploadFolder).mockResolvedValue(JOB); +}); + +afterEach(() => { + vi.useRealTimers(); +}); + +/** + * Render RepoAnalyzer, drive a folder-upload analyze to the SSE stream, and + * return the onComplete spy plus the captured SSE complete callback. Uses fake + * timers because completion holds a ~1200ms timer before firing onComplete. + */ +async function startTrackedJob() { + let sseComplete: ((data: CompleteData) => void) | undefined; + vi.mocked(streamAnalyzeProgress).mockImplementation((_jobId, _onProgress, onComplete) => { + sseComplete = onComplete; + return new AbortController(); + }); + + vi.useFakeTimers(); + const onDone = vi.fn<(repoIdentity: string) => void>(); + render(); + fireEvent.click(screen.getByRole('tab', { name: 'Local Folder' })); + fireEvent.change(screen.getByTestId('folder-upload-input'), { + target: { files: [new File(['x'], 'a.ts')] }, + }); + // Flush the upload promise so trackJob subscribes to the SSE stream. + await act(async () => {}); + expect(streamAnalyzeProgress).toHaveBeenCalledTimes(1); + + return { onDone, complete: (data: CompleteData) => sseComplete?.(data) }; +} + +describe('analyze completion identity', () => { + it('passes repoPath to onComplete but renders only the display name', async () => { + const { onDone, complete } = await startTrackedJob(); + + act(() => { + complete({ repoName: 'reels', repoPath: '/ws/b/reels' }); + }); + + // Done screen shows the display name, never the absolute path. + expect(screen.getByText('reels')).toBeInTheDocument(); + expect(screen.queryByText('/ws/b/reels')).toBeNull(); + + // onComplete fires after the ~1200ms done-screen dwell, with the identity. + expect(onDone).not.toHaveBeenCalled(); + act(() => { + vi.advanceTimersByTime(1200); + }); + expect(onDone).toHaveBeenCalledTimes(1); + expect(onDone).toHaveBeenCalledWith('/ws/b/reels'); + }); + + it('falls back to the display name when the server omits repoPath', async () => { + const { onDone, complete } = await startTrackedJob(); + + act(() => { + complete({ repoName: 'reels' }); + }); + + expect(screen.getByText('reels')).toBeInTheDocument(); + act(() => { + vi.advanceTimersByTime(1200); + }); + expect(onDone).toHaveBeenCalledTimes(1); + expect(onDone).toHaveBeenCalledWith('reels'); + }); +}); diff --git a/gitnexus-web/test/unit/switch-repo-url.test.tsx b/gitnexus-web/test/unit/switch-repo-url.test.tsx new file mode 100644 index 000000000..d16af7a70 --- /dev/null +++ b/gitnexus-web/test/unit/switch-repo-url.test.tsx @@ -0,0 +1,68 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { renderHook, act } from '@testing-library/react'; +import { AppStateProvider, useAppState } from '../../src/hooks/useAppState'; + +afterEach(() => { + vi.restoreAllMocks(); + // Reset the URL mutated by switchRepo's persistence. + window.history.replaceState(null, '', '/'); +}); + +// Duplicate-display-name repo: `name` alone cannot identify it (#2419), so the +// URL must carry the server-resolved path identity alongside the display name. +const repoInfoResponse = () => + new Response( + JSON.stringify({ + name: 'reels', + path: '/ws/group-b/reels', + repoPath: '/ws/group-b/reels', + indexedAt: '2026-07-10T00:00:00Z', + stats: { nodes: 300_000, edges: 600_000 }, + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ); + +describe('switchRepo URL persistence (#2419)', () => { + it('writes both the server-resolved repo path and the display name on success', async () => { + const fetchMock = vi.fn((url: string) => { + if (url.includes('/api/repo')) return Promise.resolve(repoInfoResponse()); + return Promise.resolve( + new Response('{}', { status: 200, headers: { 'Content-Type': 'application/json' } }), + ); + }); + vi.stubGlobal('fetch', fetchMock); + + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + act(() => { + result.current.setServerBaseUrl('http://localhost:4747'); + }); + + await act(async () => { + await result.current.switchRepo('/ws/group-b/reels'); + }); + + expect(window.location.search).toContain('repo=%2Fws%2Fgroup-b%2Freels'); + expect(window.location.search).toContain('project=reels'); + // A deliberate switch drops any per-repo skipGraph override (#2178). + expect(window.location.search).not.toContain('skipGraph'); + }); + + it('leaves the URL unchanged when the connect fails', async () => { + window.history.replaceState(null, '', '/?repo=%2Fws%2Fgroup-a%2Freels&project=reels'); + const fetchMock = vi.fn(() => Promise.reject(new Error('connection refused'))); + vi.stubGlobal('fetch', fetchMock); + + const { result } = renderHook(() => useAppState(), { wrapper: AppStateProvider }); + act(() => { + result.current.setServerBaseUrl('http://localhost:4747'); + }); + + await act(async () => { + await result.current.switchRepo('/ws/group-b/reels'); + }); + + // The failed target must not poison the URL — a refresh still restores + // the previously connected repo. + expect(window.location.search).toBe('?repo=%2Fws%2Fgroup-a%2Freels&project=reels'); + }); +}); diff --git a/gitnexus-web/test/unit/url-restore.test.ts b/gitnexus-web/test/unit/url-restore.test.ts new file mode 100644 index 000000000..e5b5bc349 --- /dev/null +++ b/gitnexus-web/test/unit/url-restore.test.ts @@ -0,0 +1,36 @@ +import { describe, expect, it, vi } from 'vitest'; + +// The helper lives in App.tsx, whose import chain pulls in WebGL-backed +// rendering (sigma via GraphCanvas) that jsdom cannot load — stub it out; +// this suite only exercises the pure pickRestoreRepo helper. +vi.mock('../../src/components/GraphCanvas', () => ({ + GraphCanvas: () => null, +})); + +import { pickRestoreRepo } from '../../src/App'; + +describe('pickRestoreRepo (URL restore param preference, #2419)', () => { + it('prefers the repo path identity when both params are present', () => { + const params = new URLSearchParams('repo=%2Fws%2Fgroup-b%2Freels&project=reels'); + + expect(pickRestoreRepo(params)).toBe('/ws/group-b/reels'); + }); + + it('falls back to the project display name for legacy project-only URLs', () => { + const params = new URLSearchParams('project=reels'); + + expect(pickRestoreRepo(params)).toBe('reels'); + }); + + it('uses the repo path identity when only repo is present', () => { + const params = new URLSearchParams('repo=%2Fws%2Fgroup-b%2Freels'); + + expect(pickRestoreRepo(params)).toBe('/ws/group-b/reels'); + }); + + it('returns undefined when neither param is present', () => { + const params = new URLSearchParams('server=http%3A%2F%2Flocalhost%3A4747'); + + expect(pickRestoreRepo(params)).toBeUndefined(); + }); +}); diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index 03f4673da..5d6b177f5 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -74,6 +74,11 @@ const PLATFORM_LOGIC = [ // (macos), so the header parsing is proven on genuine binaries, not synthetic // buffers (the ubuntu suite covers the ELF path). 'test/integration/extension-binary-real.test.ts', + // Server repo resolver branches on path shape (path.isAbsolute, backslash + // detection) and canonicalizePath/realpathSync, all of which differ between + // POSIX and Windows — the fail-closed path-claim semantics must hold on the + // real windows-latest path implementation (#2419/#2420). + 'test/unit/server-api-repo-resolution.test.ts', ]; // Native LadybugDB integration tests — exercise the @ladybugdb/core diff --git a/gitnexus/src/server/analyze-launch.ts b/gitnexus/src/server/analyze-launch.ts index 87a463ca0..b505cdfcf 100644 --- a/gitnexus/src/server/analyze-launch.ts +++ b/gitnexus/src/server/analyze-launch.ts @@ -11,10 +11,17 @@ */ import path from 'path'; +import { existsSync, statSync } from 'node:fs'; import { fork } from 'child_process'; import { fileURLToPath, pathToFileURL } from 'url'; import { createRequire } from 'node:module'; -import { getStoragePath } from '../storage/repo-manager.js'; +import { + canonicalizePath, + getStoragePath, + INDEX_METADATA_FILE, + listRegisteredRepos, + registryPathEquals, +} from '../storage/repo-manager.js'; import { logger } from '../core/logger.js'; import type { JobManager } from './analyze-job.js'; import type { WorkerMessage } from './analyze-worker.js'; @@ -26,6 +33,12 @@ export interface LaunchDeps { backend: { init: () => Promise }; acquireRepoLock: (key: string) => string | null; releaseRepoLock: (key: string) => void; + /** + * Drops the server's cached LadybugDB handle (closeLbug). The worker + * process rewrites the repo's DB files on disk, so a connection opened + * before the rewrite keeps reading the pre-rewrite state until evicted. + */ + closeDbHandle: () => Promise; } export interface LaunchOptions { @@ -37,14 +50,88 @@ export interface LaunchOptions { const MAX_WORKER_RETRIES = 2; +/** + * The worker reports `complete` over IPC before its on-disk finalization + * (LadybugDB checkpoint + native handle release + metadata write) is visible + * at `getStoragePath(targetPath)` — observed up to ~6.5s behind the IPC + * message. Opening the database inside that window is what the pre-IPC + * ordering was meant to prevent and is actively dangerous: reads fail with + * binder errors or return an empty graph, the open can quarantine the + * in-flight WAL, and the native layer racing the rewrite has crashed the + * whole server (SIGSEGV-class exit, no output) on slow CI runners. + */ +const FINALIZE_SETTLE_TIMEOUT_MS = 60_000; +const FINALIZE_SETTLE_POLL_MS = 200; + +/** + * Resolve once the analyzed repo's index is settled at `storagePath`: the + * LadybugDB file and metadata both exist AND were (re)written by THIS job + * (mtime >= jobStartMs — bare existence is not enough, a re-analysis leaves + * the previous index in place while it works), and no transient WAL/shadow/ + * checkpoint sidecars remain (the worker's native close has finished). + * + * Never rejects. Timing out logs and proceeds (pre-gate behavior) rather + * than failing a job whose analysis genuinely succeeded — e.g. a no-op + * non-force analyze legitimately rewrites nothing. + */ +/** + * Look up the analyzed repo's registered storage path. The request's + * user-provided path is used only as a comparison key; the filesystem probes + * below run against the registry's own `storagePath` — the server-owned + * record readers resolve through, and not a user-controlled value + * (CodeQL js/path-injection). + */ +const registeredStoragePath = async (targetPath: string): Promise => { + const target = canonicalizePath(path.resolve(targetPath)); + const entries = await listRegisteredRepos(); + const entry = entries.find((e) => registryPathEquals(canonicalizePath(e.path), target)); + return entry?.storagePath ?? null; +}; + +const waitForSettledIndex = async (targetPath: string, jobStartMs: number): Promise => { + const settled = (storagePath: string): boolean => { + try { + const lbugStat = statSync(path.join(storagePath, 'lbug')); + const metaStat = statSync(path.join(storagePath, INDEX_METADATA_FILE)); + return ( + lbugStat.mtimeMs >= jobStartMs && + metaStat.mtimeMs >= jobStartMs && + ['lbug.wal', 'lbug.shadow', 'lbug.wal.checkpoint'].every( + (f) => !existsSync(path.join(storagePath, f)), + ) + ); + } catch { + return false; // not written yet + } + }; + const deadline = Date.now() + FINALIZE_SETTLE_TIMEOUT_MS; + for (;;) { + // Re-resolved each round: the worker registers the repo as part of the + // finalization this gate is waiting out. + const storagePath = await registeredStoragePath(targetPath); + if (storagePath && settled(storagePath)) return; + if (Date.now() > deadline) { + logger.warn( + { targetPath }, + 'analyze finalization not visible after timeout; completing job anyway', + ); + return; + } + await new Promise((resolve) => setTimeout(resolve, FINALIZE_SETTLE_POLL_MS)); + } +}; + export function createLaunchAnalysisWorker(deps: LaunchDeps) { - const { jobManager, backend, acquireRepoLock, releaseRepoLock } = deps; + const { jobManager, backend, acquireRepoLock, releaseRepoLock, closeDbHandle } = deps; return function launchAnalysisWorker( job: { id: string }, targetPath: string, opts: LaunchOptions, ): void { + // For waitForSettledIndex: files (re)written by this job have mtimes at or + // after this instant. Taken before the fork so no worker write predates it. + const jobStartMs = Date.now(); // Acquire shared repo lock (keyed on storagePath to match embed handler) const analyzeLockKey = getStoragePath(targetPath); const lockErr = acquireRepoLock(analyzeLockKey); @@ -95,10 +182,17 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { }); } else if (msg.type === 'complete') { releaseRepoLock(analyzeLockKey); - // Reinitialize backend BEFORE marking complete — ensures the new repo - // is queryable when the client receives the SSE complete event. - backend - .init() + // Before marking complete: (1) wait for the worker's on-disk + // finalization to settle (see waitForSettledIndex), (2) evict the + // cached DB handle — same invalidation DELETE /api/repo performs, a + // handle opened before the rewrite reads pre-rewrite state — and + // only then (3) reinitialize the backend. This makes the ordering + // comment below true in practice: the repo is actually queryable + // when the client receives the SSE complete event. + waitForSettledIndex(targetPath, jobStartMs) + .then(() => closeDbHandle()) + .catch(() => {}) // best-effort: eviction failure must not fail the job + .then(() => backend.init()) .then(() => { jobManager.updateJob(job.id, { status: 'complete', repoName: msg.result.repoName }); }) @@ -111,6 +205,8 @@ export function createLaunchAnalysisWorker(deps: LaunchDeps) { }); } else if (msg.type === 'error') { releaseRepoLock(analyzeLockKey); + // A failed (force) analyze may still have rewritten DB files first. + void closeDbHandle().catch(() => {}); jobManager.updateJob(job.id, { status: 'failed', error: msg.message }); } }); diff --git a/gitnexus/src/server/api.ts b/gitnexus/src/server/api.ts index 68fa6f872..218c6e211 100644 --- a/gitnexus/src/server/api.ts +++ b/gitnexus/src/server/api.ts @@ -13,7 +13,15 @@ import cors from 'cors'; import path from 'path'; import fs from 'fs/promises'; import { createRequire } from 'node:module'; -import { loadMeta, listRegisteredRepos, getStoragePath } from '../storage/repo-manager.js'; +import { + canonicalizePath, + cloneDirBelongsToEntry, + loadMeta, + listRegisteredRepos, + getStoragePath, + registryPathEquals, + type RegistryEntry, +} from '../storage/repo-manager.js'; import { executeQuery, executePrepared, @@ -458,8 +466,13 @@ export const streamGraphNdjson = async ( /** * Mount an SSE progress endpoint for a JobManager. * Handles: initial state, terminal events, heartbeat, event IDs, client disconnect. + * + * Terminal payloads carry `repoPath` (the analyzed path) alongside the display + * `repoName` so clients can reconnect by path identity — with duplicate + * basenames, a name-only reconnect resolves to the first same-named sibling. + * Exported for unit tests that lock the wire payload shape. */ -const mountSSEProgress = (app: express.Express, routePath: string, jm: JobManager) => { +export const mountSSEProgress = (app: express.Express, routePath: string, jm: JobManager) => { app.get(routePath, (req, res) => { let jobId: string; try { @@ -492,6 +505,7 @@ const mountSSEProgress = (app: express.Express, routePath: string, jm: JobManage res.write( `id: ${eventId}\nevent: ${job.status}\ndata: ${JSON.stringify({ repoName: job.repoName, + repoPath: job.repoPath, error: job.error, })}\n\n`, ); @@ -518,6 +532,7 @@ const mountSSEProgress = (app: express.Express, routePath: string, jm: JobManage res.write( `id: ${eventId}\nevent: ${progress.phase}\ndata: ${JSON.stringify({ repoName: eventJob?.repoName, + repoPath: eventJob?.repoPath, error: eventJob?.error, })}\n\n`, ); @@ -561,6 +576,56 @@ const requestedRepo = (req: express.Request): string | undefined => { return undefined; }; +const repoParamBasename = (repoName: string): string => + repoName.replace(/\\/g, '/').split('/').filter(Boolean).pop() ?? repoName; + +/** + * Resolve a `?repo=` request param against the registry in two tiers: + * + * 1. Path claim — any input containing a separator ('/' or '\\', which + * cover path.sep on every platform) is treated as a path claim and + * resolved by canonical registry path ONLY. A miss fails closed + * (null, never a basename fallback) so a stale or wrong path can + * never silently retarget a same-named sibling repo (#2419). + * Within this tier, only absolute or Windows-shaped ('\\') claims + * are worth canonicalizing; relative claims like 'org/name' or + * './repo' are rejected immediately WITHOUT touching the filesystem + * — canonicalizing them would run an attacker-influenced + * CWD-relative realpathSync probe on un-rate-limited GET routes, + * and no legitimate caller sends relative paths. + * 2. Name fallback — bare names (no separators) keep the legacy + * basename/name match for older callers. + */ +export const resolveRegisteredRepoEntry = ( + repos: RegistryEntry[], + repoName?: string, +): RegistryEntry | null => { + if (!repoName) return repos[0] ?? null; + + const looksLikePath = + path.isAbsolute(repoName) || repoName.includes('/') || repoName.includes('\\'); + + if (looksLikePath) { + // Relative path claims fail closed with zero filesystem probes. + if (!path.isAbsolute(repoName) && !repoName.includes('\\')) return null; + + const requestedPath = canonicalizePath(repoName); + const pathMatch = repos.find((r) => + registryPathEquals(canonicalizePath(r.path), requestedPath), + ); + if (pathMatch) return pathMatch; + return null; + } + + const normalizedName = repoParamBasename(repoName); + + return ( + repos.find((r) => r.name === normalizedName) || + repos.find((r) => r.name.toLowerCase() === normalizedName.toLowerCase()) || + null + ); +}; + /** * Handle a GET /api/file request body. Extracted from createServer's route * registration so it can be unit-tested without spinning up an HTTP server @@ -821,6 +886,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => backend, acquireRepoLock, releaseRepoLock, + closeDbHandle: closeLbug, }); /** @@ -833,20 +899,9 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => // Pass `req` to enable early exit if the client disconnects during the hold-queue wait. const resolveRepo = async (repoName?: string, isRetry = false, req?: any): Promise => { const repos = await listRegisteredRepos(); - let found = null; + const found = resolveRegisteredRepoEntry(repos, repoName); - // Normalize: if a full path is passed, extract just the basename. - // e.g. "C:\Users\LENOVO\.gitnexus\repos\todo.txt-cli" -> "todo.txt-cli" - const normalizedName = repoName ? path.basename(repoName) : undefined; - - if (normalizedName) { - found = - repos.find((r) => r.name === normalizedName) || - repos.find((r) => r.name.toLowerCase() === normalizedName.toLowerCase()) || - null; - } else if (repos.length > 0) { - found = repos[0]; // default to first repo - } + const normalizedName = repoName ? repoParamBasename(repoName) : undefined; // If not yet in the registry, check whether a background job is actively cloning or // analyzing this repo. Hold the connection open (up to 5 minutes) until it completes. @@ -885,7 +940,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => if (currentJob.status === 'complete') { await backend.init(); const freshRepos = await listRegisteredRepos(); - return freshRepos.find((r) => r.name === normalizedName) || null; + return resolveRegisteredRepoEntry(freshRepos, repoName); } await new Promise((r) => setTimeout(r, 1000)); } @@ -906,7 +961,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => ); } await backend.init(); - return await resolveRepo(normalizedName, true, req); + return await resolveRepo(repoName, true, req); } return found; @@ -966,6 +1021,7 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => repos.map((r) => ({ name: r.name, path: r.path, + repoPath: r.path, indexedAt: r.indexedAt, lastCommit: r.lastCommit, stats: r.stats, @@ -977,7 +1033,11 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => }); // Get repo info - app.get('/api/repo', async (req, res) => { + // Rate-limited (CodeQL js/missing-rate-limiting): resolveRepo canonicalizes + // the attacker-supplied ?repo= param (realpathSync probe for absolute / + // Windows-shaped claims). Default 60 rpm/IP — web callers hit this route + // only on connect/switch, never in a polling loop. + app.get('/api/repo', createRouteLimiter(), async (req, res) => { try { const entry = await resolveRepo(requestedRepo(req), false, req); if (!entry) { @@ -1049,7 +1109,10 @@ export const createServer = async (port: number, host: string = '127.0.0.1') => } catch { /* repo name not eligible for a clone dir (local repo) */ } - if (cloneDir) { + // Only remove the clone dir when it is *this* entry's path — a local + // repo registered under the same name would otherwise take a cloned + // sibling's checkout down with it (see cloneDirBelongsToEntry). + if (cloneDir && cloneDirBelongsToEntry(cloneDir, entry.path)) { try { const stat = await fs.stat(cloneDir); if (stat.isDirectory()) { diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 47e336a7f..4c4e59f18 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -82,6 +82,20 @@ export const canonicalizePath = (p: string): string => { export const registryPathEquals = (a: string, b: string): boolean => process.platform === 'win32' ? a.toLowerCase() === b.toLowerCase() : a === b; +/** + * Does the clone dir derived from an entry's *name* actually belong to that + * entry? Registry names are not unique across storage locations: a cloned + * repo under `~/.gitnexus/repos/` and a local repo registered under the + * same name share a `getCloneDir(entry.name)` result. The server's delete + * handler must therefore never remove the clone dir based on the name alone — + * only when the entry's own `path` resolves to that dir (mirroring its step-2b + * rule that cleanup is driven off `entry.path`, so a same-named sibling's + * clone is never removed). Both sides are canonicalised so symlinked or + * differently-spelled forms of the same dir still match. + */ +export const cloneDirBelongsToEntry = (cloneDir: string, entryPath: string): boolean => + registryPathEquals(canonicalizePath(cloneDir), canonicalizePath(entryPath)); + export interface RepoMeta { repoPath: string; lastCommit: string; diff --git a/gitnexus/test/unit/rate-limit.test.ts b/gitnexus/test/unit/rate-limit.test.ts index 2f52e9df8..ac977ab9a 100644 --- a/gitnexus/test/unit/rate-limit.test.ts +++ b/gitnexus/test/unit/rate-limit.test.ts @@ -240,6 +240,10 @@ describe('production routes — rate-limit middleware wiring', () => { expect(apiSource).toMatch(/app\.delete\('\/api\/repo',\s*createRouteLimiter\(/); }); + it('GET /api/repo is wired with createRouteLimiter', () => { + expect(apiSource).toMatch(/app\.get\('\/api\/repo',\s*createRouteLimiter\(/); + }); + it('POST /api/analyze is wired with createRouteLimiter', () => { // Tolerate Prettier wrapping the registration across lines (it does once // the route carries extra middleware like requireLocalhostOrigin). diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index 02954b491..a269daab2 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -28,6 +28,7 @@ import { listRegisteredRepos, resolveRegistryEntry, canonicalizePath, + cloneDirBelongsToEntry, assertSafeStoragePath, RegistryNameCollisionError, RegistryNotFoundError, @@ -1408,6 +1409,33 @@ describe('canonicalizePath (#1003)', () => { }); }); +describe('cloneDirBelongsToEntry', () => { + it('returns true when the clone dir and entry.path canonicalize to the same dir', () => { + // Non-canonical spelling of a REAL path (same trick as the + // resolveRegistryEntry backward-compat test): raw concat keeps the + // strings unequal until canonicalizePath runs. + const realDir = process.cwd(); + const nonCanonical = realDir + path.sep + '.'; + expect(nonCanonical).not.toBe(realDir); + expect(cloneDirBelongsToEntry(nonCanonical, realDir)).toBe(true); + }); + + it('returns false when the entry.path lives elsewhere than the clone dir', () => { + // The delete-handler scenario: entry B is a local repo whose name + // collides with clone A's — its path must not claim A's clone dir. + const cloneDir = path.join(os.tmpdir(), 'gitnexus-clones', 'reels'); + const entryPath = path.join(os.tmpdir(), 'local', 'reels'); + expect(cloneDirBelongsToEntry(cloneDir, entryPath)).toBe(false); + }); + + it('compares nonexistent paths without throwing (realpath falls back to path.resolve)', () => { + // Neither side exists on disk — canonicalizePath must fall back to + // path.resolve on both, so equal strings still compare equal. + const ghost = path.join(os.tmpdir(), 'gnx-never-exists-____', 'clone-dir'); + expect(cloneDirBelongsToEntry(ghost, ghost)).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 diff --git a/gitnexus/test/unit/server-api-repo-resolution.test.ts b/gitnexus/test/unit/server-api-repo-resolution.test.ts new file mode 100644 index 000000000..4aad2fbb4 --- /dev/null +++ b/gitnexus/test/unit/server-api-repo-resolution.test.ts @@ -0,0 +1,127 @@ +import { describe, expect, it } from 'vitest'; +import { resolveRegisteredRepoEntry } from '../../src/server/api.js'; +import type { RegistryEntry } from '../../src/storage/repo-manager.js'; + +const entry = (overrides: Partial): RegistryEntry => ({ + name: 'repo', + path: '/tmp/repo', + storagePath: '/tmp/repo/.gitnexus', + indexedAt: '2026-07-09T00:00:00.000Z', + lastCommit: 'deadbeef', + ...overrides, +}); + +describe('resolveRegisteredRepoEntry', () => { + it('resolves an explicit alias by exact registry path before basename fallback', () => { + const aliased = entry({ + name: 'e2e-mini-repo', + path: '/tmp/gitnexus-e2e-repo', + storagePath: '/tmp/gitnexus-e2e-repo/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([aliased], '/tmp/gitnexus-e2e-repo')).toBe(aliased); + }); + + it('falls back to basename/name matching for older callers', () => { + const repo = entry({ name: 'e2e-mini-repo' }); + + expect(resolveRegisteredRepoEntry([repo], 'e2e-mini-repo')).toBe(repo); + expect(resolveRegisteredRepoEntry([repo], 'E2E-MINI-REPO')).toBe(repo); + }); + + it('does not fall back to a duplicate basename after a path-shaped miss', () => { + const first = entry({ + name: 'service', + path: '/tmp/first/service', + storagePath: '/tmp/first/service/.gitnexus', + }); + const second = entry({ + name: 'service', + path: '/tmp/second/service', + storagePath: '/tmp/second/service/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([first, second], '/tmp/missing/service')).toBeNull(); + expect(resolveRegisteredRepoEntry([first, second], '/tmp/second/service')).toBe(second); + }); + + it('fails closed on relative slash input instead of basename fallback', () => { + const named = entry({ + name: 'name', + path: '/tmp/org/name', + storagePath: '/tmp/org/name/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([named], 'org/name')).toBeNull(); + }); + + it('fails closed on dot-relative input instead of name fallback', () => { + const repo = entry({ name: 'repo' }); + + expect(resolveRegisteredRepoEntry([repo], './repo')).toBeNull(); + }); + + it('returns the first-registered entry when a bare name matches two entries', () => { + // Documented legacy first-wins behavior: bare display names are ambiguous + // across duplicate-name registrations, and the resolver deliberately keeps + // returning the earliest registry entry (callers needing precision pass a path). + const first = entry({ + name: 'reels', + path: '/tmp/group-a/reels', + storagePath: '/tmp/group-a/reels/.gitnexus', + }); + const second = entry({ + name: 'reels', + path: '/tmp/group-b/reels', + storagePath: '/tmp/group-b/reels/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([first, second], 'reels')).toBe(first); + }); + + it('treats Windows-shaped input as a path claim and never falls back to basename', () => { + // The backslash makes 'C:\ws\reels' a path claim, so canonicalization must + // miss and the resolver must return null — NOT the same-named 'reels' entry. + // This expectation is platform-unconditional: on POSIX the drive-letter path + // canonicalizes to a nonexistent cwd-relative path, and on Windows CI + // C:\ws\reels genuinely does not exist, so both platforms must yield null. + const reels = entry({ + name: 'reels', + path: '/tmp/reels', + storagePath: '/tmp/reels/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([reels], 'C:\\ws\\reels')).toBeNull(); + }); + + it('defaults to the first registered repo when no name is requested', () => { + const first = entry({ + name: 'alpha', + path: '/tmp/alpha', + storagePath: '/tmp/alpha/.gitnexus', + }); + const second = entry({ + name: 'beta', + path: '/tmp/beta', + storagePath: '/tmp/beta/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([first, second], undefined)).toBe(first); + }); + + it('returns null for an empty registry when no name is requested', () => { + expect(resolveRegisteredRepoEntry([], undefined)).toBeNull(); + }); + + it('matches a bare name case-insensitively when no exact-case entry exists', () => { + // Regression guard for the fail-closed refactor: the case-insensitive + // bare-name fallback must survive the path-claim tightening. + const reels = entry({ + name: 'reels', + path: '/tmp/reels', + storagePath: '/tmp/reels/.gitnexus', + }); + + expect(resolveRegisteredRepoEntry([reels], 'REELS')).toBe(reels); + }); +}); diff --git a/gitnexus/test/unit/server-sse-payload.test.ts b/gitnexus/test/unit/server-sse-payload.test.ts new file mode 100644 index 000000000..898898f74 --- /dev/null +++ b/gitnexus/test/unit/server-sse-payload.test.ts @@ -0,0 +1,98 @@ +/** + * SSE terminal payload wire shape (mountSSEProgress). + * + * The `event: complete` payload must carry `repoPath` (the analyzed path) + * alongside the display `repoName` at BOTH terminal emit sites: + * (a) the already-terminal replay (job finished before the client subscribed) + * (b) the live subscription (job finishes while the client is connected) + * + * Clients reconnect by this identity after "Analyze new" — with duplicate + * basenames, a name-only payload makes the web UI connect to the first + * same-named sibling instead of the repo just analyzed (PR #2420 review R2). + */ +import express from 'express'; +import http from 'node:http'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { mountSSEProgress } from '../../src/server/api.js'; +import { JobManager } from '../../src/server/analyze-job.js'; + +const REPO_PATH = '/ws/b/reels'; +const REPO_NAME = 'reels'; + +/** Extract the parsed JSON payload of the `event: complete` SSE frame. */ +const parseCompletePayload = (body: string): unknown => { + const frame = body.split('\n\n').find((f) => f.includes('event: complete')); + expect(frame).toBeDefined(); + const dataLine = frame?.split('\n').find((line) => line.startsWith('data: ')); + expect(dataLine).toBeDefined(); + return JSON.parse(dataLine?.slice('data: '.length) ?? '{}') as unknown; +}; + +describe('mountSSEProgress terminal payload', () => { + let manager: JobManager; + let server: http.Server | undefined; + let baseUrl = ''; + + beforeEach(() => { + manager = new JobManager(); + const app = express(); + // Mirrors the production mount in createServer(). + mountSSEProgress(app, '/api/analyze/:jobId/progress', manager); + return new Promise((resolve) => { + server = app.listen(0, '127.0.0.1', () => { + const addr = server?.address(); + const port = typeof addr === 'object' && addr ? addr.port : 0; + baseUrl = `http://127.0.0.1:${port}`; + resolve(); + }); + }); + }); + + afterEach(() => { + manager.dispose(); + return new Promise((resolve, reject) => { + if (!server) { + resolve(); + return; + } + server.close((err) => (err ? reject(err) : resolve())); + server = undefined; + }); + }); + + it('already-terminal replay includes repoName AND repoPath', async () => { + const job = manager.createJob({ repoPath: REPO_PATH }); + manager.updateJob(job.id, { status: 'complete', repoName: REPO_NAME }); + + const response = await fetch(`${baseUrl}/api/analyze/${job.id}/progress`); + const body = await response.text(); + + expect(body).toContain('event: complete'); + // Exact match locks the wire shape (error is undefined → omitted by JSON). + expect(parseCompletePayload(body)).toEqual({ + repoName: REPO_NAME, + repoPath: REPO_PATH, + }); + }); + + it('live subscription terminal event includes repoName AND repoPath', async () => { + const job = manager.createJob({ repoPath: REPO_PATH }); + + // fetch resolves once headers arrive — the handler has already subscribed + // to progress events by then (subscription happens synchronously). + const response = await fetch(`${baseUrl}/api/analyze/${job.id}/progress`); + manager.updateJob(job.id, { + status: 'analyzing', + progress: { phase: 'parsing', percent: 30, message: 'Parsing' }, + }); + manager.updateJob(job.id, { status: 'complete', repoName: REPO_NAME }); + + const body = await response.text(); + + expect(body).toContain('event: complete'); + expect(parseCompletePayload(body)).toEqual({ + repoName: REPO_NAME, + repoPath: REPO_PATH, + }); + }); +});