From d839c8b0f865e35a16819bf12bdac845190c1071 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 31 Jul 2026 09:53:09 +0000 Subject: [PATCH] fix(git): pin simple-git to project path for session discovery simple-git without baseDir inherits process.cwd(), so launching OpenChamber from a neutral directory (e.g. $HOME) and opening a git project elsewhere produced repeated "not a git repository" status errors and could abort project/session enumeration. Always require an explicit baseDir, soft-handle non-repo GitErrors on status/check routes, and cover non-git, foreign-cwd, and nested-repo cases. Co-authored-by: Serhii Dziupin --- packages/web/server/lib/git/DOCUMENTATION.md | 7 +- packages/web/server/lib/git/routes.js | 58 ++++++++++---- packages/web/server/lib/git/routes.test.js | 75 ++++++++++++++++++- packages/web/server/lib/git/service.js | 46 ++++++++++-- packages/web/server/lib/git/service.test.js | 79 ++++++++++++++++++++ 5 files changed, 239 insertions(+), 26 deletions(-) diff --git a/packages/web/server/lib/git/DOCUMENTATION.md b/packages/web/server/lib/git/DOCUMENTATION.md index 2af4f877..cf9bbdf8 100644 --- a/packages/web/server/lib/git/DOCUMENTATION.md +++ b/packages/web/server/lib/git/DOCUMENTATION.md @@ -135,7 +135,7 @@ The following functions are internal helpers used by exported functions: ### Adding a New Git Operation 1. Add the function to `packages/web/server/lib/git/service.js`. 2. Export the function if it's part of the public API. -3. Use `createGit(directory)` to get a simple-git instance with the correct environment. +3. Use `createGit(directory)` to get a simple-git instance with the correct environment. `directory` is required (`baseDir`); never omit it so commands cannot inherit `process.cwd()`. 4. Use `runGitCommand(cwd, args)` for direct git command execution with better error handling. 5. Use `runGitCommandOrThrow(cwd, args, fallbackMessage)` for commands that must succeed. 6. Return consistent error messages; use `parseGitErrorText(error)` to extract meaningful git errors. @@ -146,6 +146,11 @@ The following functions are internal helpers used by exported functions: - On Windows, paths are converted to MSYS format (`C:/path` → `/c/path`). - SSH_AUTH_SOCK is automatically resolved via `resolveSshAuthSock` (checks GPG agent, gpgconf). +### Working directory (simple-git) +- Repository operations always pass an explicit `baseDir` (the opened project/directory path) into simple-git. Omitting `baseDir` would default to `process.cwd()`, which breaks when the server was launched from a neutral directory (e.g. `$HOME`) while the opened project lives elsewhere. +- Global identity reads use the user home directory as `baseDir` (they do not need a repository). +- A `GitError` / non-repository result from status or check must not abort project/session enumeration: routes return a soft non-repo payload and log a warning. + ### Worktree Naming - Worktree names are slugified via `slugWorktreeName`. - Random names use adjectives/nouns from `OPENCODE_ADJECTIVES` and `OPENCODE_NOUNS` lists. diff --git a/packages/web/server/lib/git/routes.js b/packages/web/server/lib/git/routes.js index 25fd5bba..bbcc9e2f 100644 --- a/packages/web/server/lib/git/routes.js +++ b/packages/web/server/lib/git/routes.js @@ -7,6 +7,36 @@ export function registerGitRoutes(app) { return gitLibraries; }; + const resolveDirectoryQuery = (value) => { + const raw = Array.isArray(value) ? value[0] : value; + if (typeof raw !== 'string') { + return null; + } + const trimmed = raw.trim(); + return trimmed || null; + }; + + const extractGitErrorText = (error) => { + const message = typeof error?.message === 'string' ? error.message : ''; + const stderr = typeof error?.stderr === 'string' ? error.stderr : ''; + const stdout = typeof error?.stdout === 'string' ? error.stdout : ''; + const fallback = !message && error != null ? String(error) : ''; + return [message, stderr, stdout, fallback] + .map((value) => String(value || '').trim()) + .filter(Boolean) + .join('\n'); + }; + + const isNonRepoGitError = (error) => /not a git repository/i.test(extractGitErrorText(error)); + + const nonRepoStatusPayload = () => ({ + isGitRepository: false, + files: [], + branch: null, + ahead: 0, + behind: 0, + }); + app.get('/api/git/identities', async (req, res) => { const { getProfiles } = await getGitLibraries(); try { @@ -79,7 +109,7 @@ export function registerGitRoutes(app) { app.get('/api/git/check', async (req, res) => { const { isGitRepository } = await getGitLibraries(); try { - const directory = req.query.directory; + const directory = resolveDirectoryQuery(req.query.directory); if (!directory) { return res.status(400).json({ error: 'directory parameter is required' }); } @@ -87,6 +117,10 @@ export function registerGitRoutes(app) { const isRepo = await isGitRepository(directory); res.json({ isGitRepository: isRepo }); } catch (error) { + if (isNonRepoGitError(error)) { + console.warn('Git check treated non-repository path as not a git repo:', extractGitErrorText(error)); + return res.json({ isGitRepository: false }); + } console.error('Failed to check git repository:', error); res.status(500).json({ error: 'Failed to check git repository' }); } @@ -188,34 +222,26 @@ export function registerGitRoutes(app) { app.get('/api/git/status', async (req, res) => { const { getStatus, isGitRepository } = await getGitLibraries(); - const extractGitErrorText = (error) => { - const message = typeof error?.message === 'string' ? error.message : ''; - const stderr = typeof error?.stderr === 'string' ? error.stderr : ''; - const stdout = typeof error?.stdout === 'string' ? error.stdout : ''; - return [message, stderr, stdout] - .map((value) => String(value || '').trim()) - .filter(Boolean) - .join('\n'); - }; - try { - const directory = req.query.directory; + const directory = resolveDirectoryQuery(req.query.directory); if (!directory) { return res.status(400).json({ error: 'directory parameter is required' }); } const isRepo = await isGitRepository(directory); if (!isRepo) { - return res.json({ isGitRepository: false, files: [], branch: null, ahead: 0, behind: 0 }); + return res.json(nonRepoStatusPayload()); } const mode = req.query.mode === 'light' ? 'light' : undefined; const status = await getStatus(directory, { mode }); res.json(status); } catch (error) { - const errorText = extractGitErrorText(error); - if (/not a git repository/i.test(errorText)) { - return res.json({ isGitRepository: false, files: [], branch: null, ahead: 0, behind: 0 }); + // Non-repo / GitError must not abort callers that enumerate projects or + // sessions (e.g. sidebar discovery). Log a warning and continue. + if (isNonRepoGitError(error)) { + console.warn('Git status skipped for non-repository path:', extractGitErrorText(error)); + return res.json(nonRepoStatusPayload()); } console.error('Failed to get git status:', error); res.status(500).json({ error: error.message || 'Failed to get git status' }); diff --git a/packages/web/server/lib/git/routes.test.js b/packages/web/server/lib/git/routes.test.js index 0c0597c8..ac6e3309 100644 --- a/packages/web/server/lib/git/routes.test.js +++ b/packages/web/server/lib/git/routes.test.js @@ -3,11 +3,15 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; const gitLibraries = { stageFiles: vi.fn(), unstageFiles: vi.fn(), + isGitRepository: vi.fn(), + getStatus: vi.fn(), }; vi.mock('./index.js', () => ({ stageFiles: gitLibraries.stageFiles, unstageFiles: gitLibraries.unstageFiles, + isGitRepository: gitLibraries.isGitRepository, + getStatus: gitLibraries.getStatus, })); const { registerGitRoutes } = await import('./routes.js'); @@ -47,7 +51,6 @@ const createMockResponse = () => { }, json(payload) { body = payload; - return this; }, get statusCode() { return statusCode; @@ -62,6 +65,8 @@ describe('git routes index mutations', () => { beforeEach(() => { gitLibraries.stageFiles.mockReset(); gitLibraries.unstageFiles.mockReset(); + gitLibraries.isGitRepository.mockReset(); + gitLibraries.getStatus.mockReset(); }); it('accepts legacy stage path payloads', async () => { @@ -135,3 +140,71 @@ describe('git routes index mutations', () => { expect(gitLibraries.stageFiles).not.toHaveBeenCalled(); }); }); + +describe('git routes status discovery', () => { + beforeEach(() => { + gitLibraries.isGitRepository.mockReset(); + gitLibraries.getStatus.mockReset(); + }); + + it('returns a soft non-repo payload for non-git folders', async () => { + gitLibraries.isGitRepository.mockResolvedValue(false); + const { app, getRoute } = createRouteRegistry(); + registerGitRoutes(app); + const response = createMockResponse(); + + await getRoute('GET', '/api/git/status')( + { query: { directory: '/tmp/not-a-repo' } }, + response, + ); + + expect(response.statusCode).toBe(200); + expect(response.body).toEqual({ + isGitRepository: false, + files: [], + branch: null, + ahead: 0, + behind: 0, + }); + expect(gitLibraries.getStatus).not.toHaveBeenCalled(); + }); + + it('does not abort when getStatus throws a non-repo GitError', async () => { + gitLibraries.isGitRepository.mockResolvedValue(true); + gitLibraries.getStatus.mockRejectedValue( + Object.assign(new Error('fatal: not a git repository (or any of the parent directories): .git'), { + task: { commands: ['status'] }, + }), + ); + const { app, getRoute } = createRouteRegistry(); + registerGitRoutes(app); + const response = createMockResponse(); + + await getRoute('GET', '/api/git/status')( + { query: { directory: '/opened/project' } }, + response, + ); + + expect(response.statusCode).toBe(200); + expect(response.body).toMatchObject({ isGitRepository: false }); + expect(gitLibraries.getStatus).toHaveBeenCalledWith('/opened/project', { mode: undefined }); + }); + + it('uses the opened project path from query arrays without falling back to cwd', async () => { + gitLibraries.isGitRepository.mockResolvedValue(true); + gitLibraries.getStatus.mockResolvedValue({ current: 'main', files: [], isClean: true, ahead: 0, behind: 0 }); + const { app, getRoute } = createRouteRegistry(); + registerGitRoutes(app); + const response = createMockResponse(); + + await getRoute('GET', '/api/git/status')( + { query: { directory: ['/opened/git-project', '/ignored'] } }, + response, + ); + + expect(response.statusCode).toBe(200); + expect(gitLibraries.isGitRepository).toHaveBeenCalledWith('/opened/git-project'); + expect(gitLibraries.getStatus).toHaveBeenCalledWith('/opened/git-project', { mode: undefined }); + expect(response.body).toMatchObject({ current: 'main' }); + }); +}); diff --git a/packages/web/server/lib/git/service.js b/packages/web/server/lib/git/service.js index a23f0cea..ce20ec81 100644 --- a/packages/web/server/lib/git/service.js +++ b/packages/web/server/lib/git/service.js @@ -357,11 +357,17 @@ const createGit = async (directory) => { const binary = getGitBinary(); const hasCustomBinary = typeof binary === 'string' && binary.trim() && binary !== 'git' && binary !== 'git.exe'; const unsafe = hasCustomBinary ? { allowUnsafeCustomBinary: true } : undefined; - if (!directory) { - return createSimpleGit({ env, spawnOptions, binary, unsafe }); + // Always pin simple-git to an explicit working directory. Omitting baseDir + // makes simple-git use process.cwd(), which breaks when the OpenChamber + // server was launched from a neutral directory (e.g. $HOME) and the opened + // project lives elsewhere — session/project discovery then sees spurious + // "not a git repository" errors and can abort enumeration. + const baseDir = normalizeDirectoryPath(directory); + if (typeof baseDir !== 'string' || !baseDir.trim()) { + throw new Error('Git directory is required'); } return createSimpleGit({ - baseDir: normalizeDirectoryPath(directory), + baseDir, env, spawnOptions, binary, @@ -369,6 +375,10 @@ const createGit = async (directory) => { }); }; +// Global config reads do not need a repository; use the home directory as a +// stable baseDir so we never accidentally inherit process.cwd(). +const createGitForGlobalConfig = async () => createGit(os.homedir()); + const normalizeDirectoryPath = (value) => { if (typeof value !== 'string') { return value; @@ -469,6 +479,9 @@ const resolveGitRepositoryRoot = async (directoryPath, git) => { const createRepositoryGitContext = async (directory) => { const directoryPath = normalizeDirectoryPath(directory); + if (typeof directoryPath !== 'string' || !directoryPath.trim()) { + throw new Error('Git directory is required'); + } const directoryGit = await createGit(directoryPath); const repoRoot = await resolveGitRepositoryRoot(directoryPath, directoryGit); const git = path.resolve(directoryPath) === repoRoot ? directoryGit : await createGit(repoRoot); @@ -764,7 +777,11 @@ const parseGitErrorText = (error) => { const stderr = typeof error?.stderr === 'string' ? error.stderr : ''; const stdout = typeof error?.stdout === 'string' ? error.stdout : ''; const message = typeof error?.message === 'string' ? error.message : ''; - return [stderr, stdout, message] + // Some runtimes (notably Bun + simple-git GitError) surface the fatal text + // primarily via message/toString; keep String(error) as a last resort so + // "not a git repository" matching never misses and aborts callers. + const fallback = !message && error != null ? String(error) : ''; + return [stderr, stdout, message, fallback] .map((chunk) => String(chunk || '').trim()) .filter(Boolean) .join('\n') @@ -1939,7 +1956,7 @@ export async function isGitRepository(directory) { } export async function getGlobalIdentity() { - const git = await createGit(); + const git = await createGitForGlobalConfig(); try { const userName = await git.getConfig('user.name', 'global').catch(() => null); @@ -2059,9 +2076,19 @@ export async function setLocalIdentity(directory, profile) { export async function getStatus(directory, options = {}) { const lightMode = options.mode === 'light'; + const normalizedDirectory = normalizeDirectoryPath(directory); + if (typeof normalizedDirectory !== 'string' || !normalizedDirectory.trim()) { + throw new Error('directory is required'); + } try { - const { directoryPath, repoRoot, git } = await createRepositoryGitContext(directory); + // Prefer an explicit non-repo check before simple-git status so a missing + // repository never depends on process.cwd() or an opaque GitError shape. + if (!(await isGitRepository(normalizedDirectory))) { + throw new Error('fatal: not a git repository (or any of the parent directories): .git'); + } + + const { directoryPath, repoRoot, git } = await createRepositoryGitContext(normalizedDirectory); // Use -uall to show all untracked files individually, not just directories const status = await git.status(['-uall']); @@ -2315,9 +2342,12 @@ export async function getStatus(directory, options = {}) { rebaseInProgress, }; } catch (error) { - if (!isNotGitRepositoryError(error) && !isMissingDirectoryError(error)) { - console.error('Failed to get Git status:', error); + if (isNotGitRepositoryError(error) || isMissingDirectoryError(error)) { + // Re-throw a plain Error so route/session callers can match reliably and + // continue enumerating other projects instead of treating GitError as 500. + throw new Error('fatal: not a git repository (or any of the parent directories): .git'); } + console.error('Failed to get Git status:', error); throw error; } } diff --git a/packages/web/server/lib/git/service.test.js b/packages/web/server/lib/git/service.test.js index 0e480b8e..21fbbb3e 100644 --- a/packages/web/server/lib/git/service.test.js +++ b/packages/web/server/lib/git/service.test.js @@ -11,6 +11,7 @@ import { createWorktree, getWorktreeBootstrapStatus, getStatus, + isGitRepository, populateWorktreeWithLockRecovery, removeWorktree, resolvePrimaryWorktreeRoot, @@ -282,6 +283,84 @@ describe('getStatus', () => { await expect(getStatus(repo)).resolves.toMatchObject({ current: 'main' }); }); + + it('rejects a non-git folder without using process.cwd()', async () => { + if (!canRunGit()) return; + + const nonGit = createTempDir(); + const previousCwd = process.cwd(); + process.chdir(nonGit); + try { + await expect(getStatus(nonGit)).rejects.toThrow(/not a git repository/i); + } finally { + process.chdir(previousCwd); + } + }); + + it('reads status for a git repo when process.cwd() is elsewhere', async () => { + if (!canRunGit()) return; + + const repo = createTempDir(); + const neutralCwd = createTempDir(); + runGit(repo, ['init', '-b', 'main']); + runGit(repo, ['config', 'user.email', 'test@example.com']); + runGit(repo, ['config', 'user.name', 'Test User']); + fs.writeFileSync(path.join(repo, 'README.md'), '# Test\n'); + runGit(repo, ['add', 'README.md']); + runGit(repo, ['commit', '-m', 'Initial commit']); + + const previousCwd = process.cwd(); + process.chdir(neutralCwd); + try { + await expect(getStatus(repo)).resolves.toMatchObject({ current: 'main', isClean: true }); + await expect(isGitRepository(repo)).resolves.toBe(true); + await expect(isGitRepository(neutralCwd)).resolves.toBe(false); + } finally { + process.chdir(previousCwd); + } + }); + + it('supports a folder with nested git repositories from a foreign cwd', async () => { + if (!canRunGit()) return; + + const parent = createTempDir(); + const nested = path.join(parent, 'nested'); + const neutralCwd = createTempDir(); + fs.mkdirSync(nested, { recursive: true }); + + runGit(parent, ['init', '-b', 'main']); + runGit(parent, ['config', 'user.email', 'test@example.com']); + runGit(parent, ['config', 'user.name', 'Test User']); + fs.writeFileSync(path.join(parent, 'README.md'), '# Parent\n'); + runGit(parent, ['add', 'README.md']); + runGit(parent, ['commit', '-m', 'Parent commit']); + + runGit(nested, ['init', '-b', 'feature']); + runGit(nested, ['config', 'user.email', 'test@example.com']); + runGit(nested, ['config', 'user.name', 'Test User']); + fs.writeFileSync(path.join(nested, 'nested.txt'), 'nested\n'); + runGit(nested, ['add', 'nested.txt']); + runGit(nested, ['commit', '-m', 'Nested commit']); + + const previousCwd = process.cwd(); + process.chdir(neutralCwd); + try { + await expect(getStatus(parent)).resolves.toMatchObject({ current: 'main' }); + await expect(getStatus(nested)).resolves.toMatchObject({ current: 'feature' }); + // Enumeration must continue when one path is not a repo. + const results = await Promise.allSettled([ + getStatus(parent), + getStatus(neutralCwd), + getStatus(nested), + ]); + expect(results[0].status).toBe('fulfilled'); + expect(results[1].status).toBe('rejected'); + expect(results[1].reason?.message || String(results[1].reason)).toMatch(/not a git repository/i); + expect(results[2].status).toBe('fulfilled'); + } finally { + process.chdir(previousCwd); + } + }); }); // ---------------------------------------------------------------------------