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 <makeittech@users.noreply.github.com>
This commit is contained in:
co-authored by
Serhii Dziupin
parent
09f0c64839
commit
d839c8b0f8
@@ -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.
|
||||
|
||||
@@ -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' });
|
||||
|
||||
@@ -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' });
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user