From 8b2d8018bc860c68632e8d0bed8092a77352cab3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9on=20Simmons?= Date: Thu, 14 May 2026 12:26:34 -0400 Subject: [PATCH] fix(cli): tolerate read-only workspace in `ensureGitNexusIgnored` (#1549) (#1550) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(cli): tolerate read-only workspace in ensureGitNexusIgnored The documented Docker workflow mounts the host workspace at /workspace:ro and runs `gitnexus index /workspace/` against an index produced by a prior host-side `analyze`. Since PR #1248 ("keep GitNexus ignores inside .gitnexus") the index command has called `ensureGitNexusIgnored`, which unconditionally writes `/.gitnexus/.gitignore` and `/.git/info/exclude` — both fail with EROFS on the :ro bind mount even though the host already wrote the correct file during `analyze`. Two complementary changes: 1. Idempotent fast path. Read the existing .gitnexus/.gitignore content first; if it already matches the desired value (`*\n`), skip the write entirely. This is the common case for the Docker workflow and avoids touching the FS at all. 2. EROFS/EACCES tolerance. When a write is genuinely needed but the FS refuses it, log a structured warning via the existing pino logger and continue. `registerRepo` runs before `ensureGitNexusIgnored` in `indexCommand`, so the global-registry write is already committed when we get here — letting the gitignore-write failure propagate leaves the user with a registered-but-error-exited command. Three new unit tests pin the behaviour: - idempotent re-call leaves mtime untouched - ENOENT-then-correct path on a writable parent succeeds - :ro parent (simulated via chmod 0o555) does not throw, on the already-correct fast path and on the cold-create path Existing tests (61) still pass. Closes #1549. * test(storage): cover read-only ignore paths and tolerate EPERM (#1550) - Add isReadOnlyFilesystemError helper including EPERM alongside EROFS/EACCES for ensureGitNexusIgnored and ensureGitInfoExclude (Windows parity with lbug-config / bridge-db patterns). - Skip chmod-based read-only tests on win32 and uid 0; assert logger.warn on POSIX chmod denial for missing .gitignore. - Add repo-manager-ensure-ignore-readonly.test.ts with vi.mock fs/promises delegating writeFile so EROFS/EACCES/EPERM rejections are asserted with structured log path and message for both .gitignore and .git/info/exclude. Co-authored-by: Cursor * chore(autofix): apply prettier + eslint fixes via /autofix command --------- Co-authored-by: Gergő Magyar Co-authored-by: Cursor Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> --- gitnexus/src/storage/repo-manager.ts | 51 +++++++- ...epo-manager-ensure-ignore-readonly.test.ts | 113 ++++++++++++++++++ gitnexus/test/unit/repo-manager.test.ts | 72 ++++++++++- 3 files changed, 230 insertions(+), 6 deletions(-) create mode 100644 gitnexus/test/unit/repo-manager-ensure-ignore-readonly.test.ts diff --git a/gitnexus/src/storage/repo-manager.ts b/gitnexus/src/storage/repo-manager.ts index 456a6c143..44446e0ec 100644 --- a/gitnexus/src/storage/repo-manager.ts +++ b/gitnexus/src/storage/repo-manager.ts @@ -11,6 +11,7 @@ import { realpathSync } from 'fs'; import path from 'path'; import os from 'os'; import { getInferredRepoName, resolveRepoIdentityRoot } from './git.js'; +import { logger } from '../core/logger.js'; /** * Normalise a repo path for registry comparison across platforms @@ -281,14 +282,44 @@ export const findRepo = async (startPath: string): Promise = return null; }; +function isReadOnlyFilesystemError(err: unknown): boolean { + const code = (err as NodeJS.ErrnoException)?.code; + return code === 'EROFS' || code === 'EACCES' || code === 'EPERM'; +} + /** * Keep generated index files ignored without modifying the user's root .gitignore. */ export const ensureGitNexusIgnored = async (repoPath: string): Promise => { const gitignorePath = path.join(getStoragePath(repoPath), '.gitignore'); + const desired = '*\n'; - await fs.mkdir(path.dirname(gitignorePath), { recursive: true }); - await fs.writeFile(gitignorePath, '*\n', 'utf-8'); + // Idempotent fast path: skip the write entirely when the file already has + // the expected content. Lets this run cleanly on read-only mounts (e.g. + // the documented Docker workflow with WORKSPACE_DIR bound :ro) when an + // earlier `analyze` already created the file. See issue #1549. + try { + if ((await fs.readFile(gitignorePath, 'utf-8')) === desired) { + await ensureGitInfoExclude(repoPath); + return; + } + } catch (err: any) { + if (err?.code !== 'ENOENT') throw err; + } + + try { + await fs.mkdir(path.dirname(gitignorePath), { recursive: true }); + await fs.writeFile(gitignorePath, desired, 'utf-8'); + } catch (err: any) { + if (isReadOnlyFilesystemError(err)) { + logger.warn( + { path: gitignorePath, code: err.code }, + 'GitNexus storage filesystem is not writable; skipping .gitnexus/.gitignore. Generated files may appear as untracked in this repo locally.', + ); + } else { + throw err; + } + } await ensureGitInfoExclude(repoPath); }; @@ -304,8 +335,6 @@ const ensureGitInfoExclude = async (repoPath: string): Promise => { return; } - await fs.mkdir(path.dirname(excludePath), { recursive: true }); - let content = ''; try { content = await fs.readFile(excludePath, 'utf-8'); @@ -320,7 +349,19 @@ const ensureGitInfoExclude = async (repoPath: string): Promise => { if (excludes.includes(GITNEXUS_DIR) || excludes.includes(GITNEXUS_EXCLUDE_ENTRY)) return; const separator = content.length === 0 || content.endsWith('\n') ? '' : '\n'; - await fs.writeFile(excludePath, `${content}${separator}${GITNEXUS_EXCLUDE_ENTRY}\n`, 'utf-8'); + try { + await fs.mkdir(path.dirname(excludePath), { recursive: true }); + await fs.writeFile(excludePath, `${content}${separator}${GITNEXUS_EXCLUDE_ENTRY}\n`, 'utf-8'); + } catch (err: any) { + if (isReadOnlyFilesystemError(err)) { + logger.warn( + { path: excludePath, code: err.code }, + 'GitNexus storage filesystem is not writable; skipping .git/info/exclude update. .gitnexus/ may appear as untracked in `git status` locally.', + ); + } else { + throw err; + } + } }; // ─── Global Registry (~/.gitnexus/registry.json) ─────────────────────── diff --git a/gitnexus/test/unit/repo-manager-ensure-ignore-readonly.test.ts b/gitnexus/test/unit/repo-manager-ensure-ignore-readonly.test.ts new file mode 100644 index 000000000..8e49a01f3 --- /dev/null +++ b/gitnexus/test/unit/repo-manager-ensure-ignore-readonly.test.ts @@ -0,0 +1,113 @@ +/** + * Read-only / permission-denied write paths for ensureGitNexusIgnored (#1549, PR #1550). + * Separate from repo-manager.test.ts: Vitest cannot vi.spyOn ESM namespace exports of + * fs/promises; a delegating vi.mock is required for cross-platform mock rejects. + */ +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import path from 'path'; + +const fswCtx = vi.hoisted(() => ({ + writeFileMock: vi.fn(), + realWrite: null as ((...args: unknown[]) => Promise) | null, +})); + +vi.mock('fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + const d = actual.default; + fswCtx.realWrite = d.writeFile.bind(d); + fswCtx.writeFileMock.mockImplementation((...args) => fswCtx.realWrite!(...args)); + return { + default: new Proxy(d, { + get(target, prop) { + if (prop === 'writeFile') return fswCtx.writeFileMock; + const v = Reflect.get(target, prop, target) as unknown; + return typeof v === 'function' ? (v as (...args: unknown[]) => unknown).bind(target) : v; + }, + }), + }; +}); + +import fs from 'fs/promises'; +import { ensureGitNexusIgnored } from '../../src/storage/repo-manager.js'; +import { _captureLogger } from '../../src/core/logger.js'; +import { createTempDir } from '../helpers/test-db.js'; + +const samePath = (a: string, b: string) => path.normalize(a) === path.normalize(b); + +describe('ensureGitNexusIgnored — mocked writeFile (EROFS / EACCES / EPERM)', () => { + let tmpRepo: Awaited>; + + beforeEach(async () => { + tmpRepo = await createTempDir('gitnexus-ro-ignore-mock-'); + fswCtx.writeFileMock.mockClear(); + fswCtx.writeFileMock.mockImplementation((...args) => fswCtx.realWrite!(...args)); + }); + + afterEach(async () => { + await tmpRepo.cleanup(); + }); + + it.each(['EROFS', 'EACCES', 'EPERM'] as const)( + 'tolerates %s on .git/info/exclude write and logs a warn', + async (code) => { + const gitignorePath = path.join(tmpRepo.dbPath, '.gitnexus', '.gitignore'); + await fs.mkdir(path.dirname(gitignorePath), { recursive: true }); + await fs.writeFile(gitignorePath, '*\n', 'utf-8'); + + const excludePath = path.join(tmpRepo.dbPath, '.git', 'info', 'exclude'); + await fs.mkdir(path.dirname(excludePath), { recursive: true }); + await fs.writeFile(excludePath, '# empty\n', 'utf-8'); + + const cap = _captureLogger(); + fswCtx.writeFileMock.mockRejectedValueOnce(Object.assign(new Error('mock ro'), { code })); + + try { + await expect(ensureGitNexusIgnored(tmpRepo.dbPath)).resolves.not.toThrow(); + expect(fswCtx.writeFileMock).toHaveBeenCalled(); + expect( + cap + .records() + .some( + (r) => + r.level === 40 && + r.code === code && + typeof r.path === 'string' && + samePath(String(r.path), excludePath) && + String(r.msg ?? '').includes('.git/info/exclude'), + ), + ).toBe(true); + } finally { + cap.restore(); + } + }, + ); + + it.each(['EROFS', 'EACCES', 'EPERM'] as const)( + 'tolerates %s on .gitnexus/.gitignore write and logs a warn', + async (code) => { + const cap = _captureLogger(); + const gitignorePath = path.join(tmpRepo.dbPath, '.gitnexus', '.gitignore'); + + fswCtx.writeFileMock.mockRejectedValueOnce(Object.assign(new Error('mock ro'), { code })); + + try { + await expect(ensureGitNexusIgnored(tmpRepo.dbPath)).resolves.not.toThrow(); + expect(fswCtx.writeFileMock).toHaveBeenCalled(); + expect( + cap + .records() + .some( + (r) => + r.level === 40 && + r.code === code && + typeof r.path === 'string' && + samePath(String(r.path), gitignorePath) && + String(r.msg ?? '').includes('.gitnexus/.gitignore'), + ), + ).toBe(true); + } finally { + cap.restore(); + } + }, + ); +}); diff --git a/gitnexus/test/unit/repo-manager.test.ts b/gitnexus/test/unit/repo-manager.test.ts index bbd0fd50e..d3dd65a27 100644 --- a/gitnexus/test/unit/repo-manager.test.ts +++ b/gitnexus/test/unit/repo-manager.test.ts @@ -4,10 +4,11 @@ * Tests: getStoragePath, getStoragePaths, readRegistry, registerRepo, unregisterRepo * Covers hardening fixes #29 (API key file permissions) and #30 (case-insensitive paths on Windows) */ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import path from 'path'; import os from 'os'; import fs from 'fs/promises'; +import { _captureLogger } from '../../src/core/logger.js'; import { getStoragePath, getStoragePaths, @@ -73,6 +74,7 @@ describe('ensureGitNexusIgnored (#1233)', () => { }); afterEach(async () => { + vi.restoreAllMocks(); await tmpRepo.cleanup(); }); @@ -139,6 +141,74 @@ describe('ensureGitNexusIgnored (#1233)', () => { }); expect(status).toBe(''); }); + + // ─ Read-only workspace tolerance (#1549) ──────────────────────────── + // The documented Docker workflow mounts the host workspace at /workspace:ro + // and runs `gitnexus index /workspace/`. The host has already created + // the .gitnexus dir during a prior `analyze`, so the gitignore file already + // exists with the correct content — there's no real work to do. The tests + // below pin two pieces of behaviour that make that workflow work: + // (a) the function short-circuits when the file is already correct + // (no write attempt, no mtime bump); + // (b) when a write *is* needed but the FS is not writable + // (EROFS / EACCES / EPERM), the function logs and continues instead of + // throwing — so the caller's `registerRepo` work stays committed. + + it('does not re-write .gitnexus/.gitignore when it already has the desired content', async () => { + await ensureGitNexusIgnored(tmpRepo.dbPath); + const gitignorePath = path.join(tmpRepo.dbPath, '.gitnexus', '.gitignore'); + const before = await fs.stat(gitignorePath); + + await new Promise((resolve) => setTimeout(resolve, 25)); + + await ensureGitNexusIgnored(tmpRepo.dbPath); + + const after = await fs.stat(gitignorePath); + expect(after.mtimeMs).toBe(before.mtimeMs); + }); + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'does not throw when .gitnexus/.gitignore is already correct and the storage dir is read-only', + async () => { + await ensureGitNexusIgnored(tmpRepo.dbPath); + const storagePath = path.join(tmpRepo.dbPath, '.gitnexus'); + + await fs.chmod(storagePath, 0o555); + try { + await expect(ensureGitNexusIgnored(tmpRepo.dbPath)).resolves.not.toThrow(); + } finally { + await fs.chmod(storagePath, 0o755); + } + }, + ); + + it.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'warns and continues when the storage dir is read-only and the file does not yet exist', + async () => { + const storagePath = path.join(tmpRepo.dbPath, '.gitnexus'); + await fs.mkdir(storagePath, { recursive: true }); + await fs.chmod(storagePath, 0o555); + + const cap = _captureLogger(); + try { + await expect(ensureGitNexusIgnored(tmpRepo.dbPath)).resolves.not.toThrow(); + expect( + cap + .records() + .some( + (r) => + r.level === 40 && + (r.code === 'EACCES' || r.code === 'EPERM') && + String(r.msg ?? '').includes('.gitnexus/.gitignore') && + String(r.path ?? '').includes('.gitnexus'), + ), + ).toBe(true); + } finally { + cap.restore(); + await fs.chmod(storagePath, 0o755); + } + }, + ); }); // ─── readRegistry ────────────────────────────────────────────────────