From 5e3f9d1ba273985ca68038ca27b889513fb5847e Mon Sep 17 00:00:00 2001 From: jaygupta17 Date: Tue, 25 Aug 2026 19:15:35 +0530 Subject: [PATCH] fix(git): harden nested repository discovery - discard an in-flight discovery when the runtime switches, mirroring the isRequestCurrent pattern, so a late completion cannot repopulate the cleared map and suppress a fresh scan - treat a 501 from /api/fs/git-dirs as an explicit 'unsupported' marker instead of a generic failure; the VS Code webview now answers the route with unsupportedWebRouteResponse so non-repo roots show the honest not-a-repository state without a futile Retry --- packages/ui/src/lib/gitApiHttp.ts | 10 ++++ packages/ui/src/stores/useGitStore.test.ts | 54 +++++++++++++++++++++- packages/ui/src/stores/useGitStore.ts | 38 +++++++++++---- packages/vscode/webview/main.tsx | 4 ++ 4 files changed, 97 insertions(+), 9 deletions(-) diff --git a/packages/ui/src/lib/gitApiHttp.ts b/packages/ui/src/lib/gitApiHttp.ts index 05ff58b2..0ca0cc8d 100644 --- a/packages/ui/src/lib/gitApiHttp.ts +++ b/packages/ui/src/lib/gitApiHttp.ts @@ -116,8 +116,18 @@ export async function checkIsGitRepository(directory: string): Promise } } +export class GitDirectoriesUnsupportedError extends Error { + constructor() { + super('Nested git repository discovery is not supported by this runtime'); + this.name = 'GitDirectoriesUnsupportedError'; + } +} + export async function listGitDirectories(root: string): Promise { const response = await runtimeFetch('/api/fs/git-dirs', { query: { path: root } }); + if (response.status === 501) { + throw new GitDirectoriesUnsupportedError(); + } if (!response.ok) { throw new Error(`Failed to list git directories: ${response.statusText}`); } diff --git a/packages/ui/src/stores/useGitStore.test.ts b/packages/ui/src/stores/useGitStore.test.ts index a1dd6902..20296dba 100644 --- a/packages/ui/src/stores/useGitStore.test.ts +++ b/packages/ui/src/stores/useGitStore.test.ts @@ -1,8 +1,22 @@ -import { beforeEach, describe, expect, test } from 'bun:test'; +import { beforeEach, describe, expect, mock, test } from 'bun:test'; import type { GitStatus } from '@/lib/api/types'; import { useGitStore } from './useGitStore'; import { getRuntimeKey } from '@/lib/runtime-switch'; +// The real transport has no server in tests and fails as a generic error. +// Tests that exercise other failure modes swap this implementation; the +// default keeps every pre-existing expectation (generic failure → null). +const listGitDirectoriesControl: { impl: (root: string) => Promise } = { + impl: async () => { + throw new Error('network unavailable'); + }, +}; +class TestGitDirectoriesUnsupportedError extends Error {} +mock.module('@/lib/gitApiHttp', () => ({ + GitDirectoriesUnsupportedError: TestGitDirectoriesUnsupportedError, + listGitDirectories: (root: string) => listGitDirectoriesControl.impl(root), +})); + type Deferred = { promise: Promise; resolve: (value: T) => void; @@ -338,6 +352,9 @@ describe('useGitStore', () => { describe('useGitStore nested repository discovery', () => { beforeEach(() => { + listGitDirectoriesControl.impl = async () => { + throw new Error('network unavailable'); + }; useGitStore.getState().resetForRuntimeSwitch(getRuntimeKey()); }); @@ -379,12 +396,47 @@ describe('useGitStore nested repository discovery', () => { expect(useGitStore.getState().nestedReposByRoot.size).toBe(0); }); + test('discards an in-flight discovery result when the runtime switches', async () => { + const stale = useGitStore.getState().ensureNestedRepos('/root-a'); + useGitStore.getState().resetForRuntimeSwitch('runtime-b'); + await stale; + + // The old runtime's late completion must not repopulate the cleared map. + expect(useGitStore.getState().nestedReposByRoot.has('/root-a')).toBe(false); + + // Discovery started under the new runtime still commits normally. + await useGitStore.getState().ensureNestedRepos('/root-a'); + expect(useGitStore.getState().nestedReposByRoot.get('/root-a')).toBeNull(); + }); + test('marks discovery failure as a failed marker, not an empty success', async () => { await useGitStore.getState().ensureNestedRepos('/root-a'); expect(useGitStore.getState().nestedReposByRoot.get('/root-a')).toBeNull(); }); + test('marks a 501 runtime as unsupported instead of failed', async () => { + listGitDirectoriesControl.impl = async () => { + throw new TestGitDirectoriesUnsupportedError(); + }; + + await useGitStore.getState().ensureNestedRepos('/root-a'); + + expect(useGitStore.getState().nestedReposByRoot.get('/root-a')).toBe('unsupported'); + }); + + test('unsupported does not clobber a previous successful discovery', async () => { + listGitDirectoriesControl.impl = async () => ['/root-a/one']; + await useGitStore.getState().ensureNestedRepos('/root-a'); + + listGitDirectoriesControl.impl = async () => { + throw new TestGitDirectoriesUnsupportedError(); + }; + await useGitStore.getState().ensureNestedRepos('/root-a', { force: true }); + + expect(useGitStore.getState().nestedReposByRoot.get('/root-a')).toEqual(['/root-a/one']); + }); + test('dedupes concurrent discovery runs for the same root', async () => { const first = useGitStore.getState().ensureNestedRepos('/root-a'); const second = useGitStore.getState().ensureNestedRepos('/root-a'); diff --git a/packages/ui/src/stores/useGitStore.ts b/packages/ui/src/stores/useGitStore.ts index 2ec728df..0d1dcf2e 100644 --- a/packages/ui/src/stores/useGitStore.ts +++ b/packages/ui/src/stores/useGitStore.ts @@ -9,7 +9,7 @@ import type { } from '@/lib/api/types'; import { getDeferredSafeStorage } from '@/stores/utils/safeStorage'; import { getRuntimeKey } from '@/lib/runtime-switch'; -import { listGitDirectories } from '@/lib/gitApiHttp'; +import { GitDirectoriesUnsupportedError, listGitDirectories } from '@/lib/gitApiHttp'; const LOG_STALE_THRESHOLD = 10000; const REPO_CHECK_STALE_THRESHOLD = 60_000; @@ -28,6 +28,11 @@ const DIFF_CACHE_MAX_TOTAL_SIZE_BYTES = 20 * 1024 * 1024; // 20MB const DIFF_CACHE_MAX_GLOBAL_ENTRIES = 200; type GitStatusFetchMode = 'full' | 'light'; +// Discovery outcome for a root that is not itself a git repository. The three +// states are mutually exclusive: a repository list (possibly empty), a failed +// scan (`null`), or a runtime without the discovery route (`'unsupported'`). +export type NestedRepoDiscovery = string[] | null | 'unsupported'; + interface DirectoryGitState { isGitRepo: boolean | null; status: GitStatus | null; @@ -81,8 +86,9 @@ interface GitStore { // Nested repository discovery: when the root directory is not itself a git // repository, these hold the discovered repositories and the user's pick. // `nestedReposByRoot` values are `null` when discovery failed — never a - // valid empty result — and absent when discovery has not run yet. - nestedReposByRoot: Map; + // valid empty result — `'unsupported'` when the runtime has no discovery + // route, and absent when discovery has not run yet. + nestedReposByRoot: Map; nestedRepoSelection: Map; ensureNestedRepos: (root: string, options?: { force?: boolean }) => Promise; selectNestedRepo: (root: string, repository: string) => void; @@ -1203,6 +1209,7 @@ export const useGitStore = create()( if (!root) return; const { force = false } = options; const runtimeKey = getRuntimeKey(); + const runtimeGeneration = gitRuntimeGeneration; const key = runtimeDirectoryKey(runtimeKey, root); const current = get().nestedReposByRoot.get(root); if (!force && (current !== undefined || inFlightNestedRepoDiscovery.has(key))) { @@ -1217,16 +1224,30 @@ export const useGitStore = create()( const discovery = (async () => { let repositories: string[] | null = null; + let unsupported = false; try { repositories = await listGitDirectories(root); } catch (error) { - console.error('Failed to discover nested git repositories:', error); + if (error instanceof GitDirectoriesUnsupportedError) { + unsupported = true; + } else { + console.error('Failed to discover nested git repositories:', error); + } repositories = null; } - // A failed retry must not clobber an earlier successful discovery. + // A runtime switch invalidates the discovery: resetForRuntimeSwitch + // already cleared the map, and committing old-runtime data here would + // both leak it and suppress a fresh scan for this root. + if (runtimeKey !== getRuntimeKey() || runtimeGeneration !== gitRuntimeGeneration) return; + const previous = get().nestedReposByRoot.get(root); - const nextValue = repositories ?? previous ?? null; + // An authoritative "unsupported" answer replaces only unknown or + // failed state; like a failed retry, it must not clobber an earlier + // successful discovery. + const nextValue: NestedRepoDiscovery = unsupported + ? (previous ?? 'unsupported') + : (repositories ?? previous ?? null); const next = new Map(get().nestedReposByRoot); next.set(root, nextValue); set({ nestedReposByRoot: next }); @@ -1368,8 +1389,9 @@ export const useEffectiveGitDirectory = (root: string | null) => { }); }; -// `undefined` = discovery not run yet, `null` = discovery failed, otherwise -// the discovered nested repository paths (possibly empty). +// `undefined` = discovery not run yet, `null` = discovery failed, +// `'unsupported'` = the runtime has no discovery route, otherwise the +// discovered nested repository paths (possibly empty). export const useNestedRepos = (root: string | null) => { return useGitStore((state) => { if (!root) return undefined; diff --git a/packages/vscode/webview/main.tsx b/packages/vscode/webview/main.tsx index 3a83a155..a49ba39f 100644 --- a/packages/vscode/webview/main.tsx +++ b/packages/vscode/webview/main.tsx @@ -388,6 +388,10 @@ const handleLocalApiRequest = async (input: RequestInfo | URL, url: URL, init: R return unsupportedWebRouteResponse('Scheduled tasks'); } + if (normalizedPathname === '/api/fs/git-dirs') { + return unsupportedWebRouteResponse('Nested git repository discovery'); + } + if (normalizedPathname === '/api/sessions/snapshot' && method === 'GET') { const activity = await sendBridgeMessage>('api:session-activity:get') .catch(() => ({}));