diff --git a/.husky/pre-push b/.husky/pre-push index e9c26c1..795493c 100755 --- a/.husky/pre-push +++ b/.husky/pre-push @@ -26,9 +26,15 @@ if [ "$has_code_push" = false ]; then fi # run_quiet: silent on success, full output on failure +# NOTE: husky runs this script under `sh -e` (errexit). `out=$("$@" 2>&1)` alone +# inherits the wrapped command's exit code, so a failing command kills the +# script on that assignment line before `status=$?`/`echo "$out"` ever run — +# swallowing the diagnostic output entirely. `|| status=$?` keeps the failing +# command from being the last element of the statement, so errexit does not +# fire here, while `$?` is still captured accurately. run_quiet() { - out=$("$@" 2>&1) - status=$? + status=0 + out=$("$@" 2>&1) || status=$? if [ $status -ne 0 ]; then echo "$out" fi diff --git a/packages/core/src/paths.integration.test.ts b/packages/core/src/paths.integration.test.ts index bba3ccb..0f036ff 100644 --- a/packages/core/src/paths.integration.test.ts +++ b/packages/core/src/paths.integration.test.ts @@ -18,6 +18,10 @@ describe('getRealPathFromSession (mocked)', () => { vi.clearAllMocks() }) + // Flaky under load: the dynamic re-import below occasionally exceeds vitest's + // default 5000ms test timeout on a cold module cache (observed independent of + // this test's own logic, on unmodified main). Raise the timeout rather than + // changing behavior. it('returns null for non-existent project', async () => { vi.mocked(fs.readdirSync).mockImplementation(() => { throw new Error('ENOENT') @@ -27,7 +31,7 @@ describe('getRealPathFromSession (mocked)', () => { const { getRealPathFromSession } = await import('./paths.js') const result = getRealPathFromSession('-nonexistent-project') expect(result).toBeNull() - }) + }, 15000) it('returns real cwd from session file when cwd matches folder name', async () => { const mockCwd = '/home/user/example.com' diff --git a/packages/core/src/session/search.test.ts b/packages/core/src/session/search.test.ts index b852b81..3c5b8fc 100644 --- a/packages/core/src/session/search.test.ts +++ b/packages/core/src/session/search.test.ts @@ -4,6 +4,16 @@ import * as path from 'node:path' import * as os from 'node:os' import { Effect } from 'effect' +vi.mock('node:fs/promises', async () => { + const actual = await vi.importActual('node:fs/promises') + const readdirMock = vi.fn(actual.readdir) + return { + ...actual, + readdir: readdirMock, + default: { ...actual, readdir: readdirMock }, + } +}) + vi.mock('../paths.js', async () => { const actual = await vi.importActual('../paths.js') return { @@ -135,6 +145,10 @@ describe('searchSessions', () => { let tempDir: string beforeEach(async () => { + // Restore readdir to the real implementation; individual tests may override. + const actual = await vi.importActual('node:fs/promises') + vi.mocked(fs.readdir).mockImplementation(actual.readdir as typeof fs.readdir) + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'search-test-')) vi.mocked(getSessionsDir).mockReturnValue(tempDir) }) @@ -260,4 +274,147 @@ describe('searchSessions', () => { const results = await Effect.runPromise(searchSessions('needle')) expect(results).toEqual([]) }) + + // searchProjectContent TOCTOU guard — a project's folder may vanish between + // listProjects() returning it and searchProjectContent's own readdir (cross-PC + // sync, manual deletion). Mirrors listProjects' own guard, see Issue #103. + describe('searchProjectContent TOCTOU safety', () => { + function makeEnoent(p: string): NodeJS.ErrnoException { + const err = new Error( + `ENOENT: no such file or directory, scandir '${p}'` + ) as NodeJS.ErrnoException + err.code = 'ENOENT' + return err + } + + it("skips a project whose folder vanishes mid-content-search, keeping other projects' results", async () => { + const projGone = '-Users-test-search-gone' + const projKeep = '-Users-test-search-keep' + await writeSessionWithDistinctTitleAndContent( + projGone, + 'sess-gone', + 'plain title', + 'this has NEEDLE inside' + ) + await writeSessionWithDistinctTitleAndContent( + projKeep, + 'sess-keep', + 'plain title', + 'this also has NEEDLE inside' + ) + + // listProjects() also readdirs each project folder (for sessionCount) before + // searchProjectContent gets a turn. Succeed on that first call so projGone + // survives into targetProjects, then fail with ENOENT on the *next* readdir + // for the same path — simulating the folder vanishing between the two reads + // (the actual TOCTOU window this guard protects, not a same-instant miss). + const actual = await vi.importActual('node:fs/promises') + let goneCallCount = 0 + vi.mocked(fs.readdir).mockImplementation(((p: unknown, opts: unknown) => { + if (typeof p === 'string' && p.endsWith(projGone)) { + goneCallCount += 1 + if (goneCallCount > 1) { + return Promise.reject(makeEnoent(p)) + } + } + return (actual.readdir as typeof fs.readdir)( + p as Parameters[0], + opts as Parameters[1] + ) + }) as typeof fs.readdir) + + // Before fix: the whole searchSessions Effect would die on projGone's ENOENT. + // After fix: projGone is skipped, projKeep's content match still returned. + const results = await Effect.runPromise(searchSessions('needle', { searchContent: true })) + const ids = results.map((r) => r.sessionId) + expect(ids).toContain('sess-keep') + expect(ids).not.toContain('sess-gone') + }) + + it('propagates a non-ENOENT readdir error (EACCES) instead of silently skipping', async () => { + const projDenied = '-Users-test-search-eacces' + await writeSessionWithDistinctTitleAndContent( + projDenied, + 'sess-denied', + 'plain title', + 'this has NEEDLE inside' + ) + + // Same call-count trick as above, but this project sees *three* readdir + // calls before searchProjectContent's own turn: (1) listProjects' own + // readdir (for sessionCount), (2) Phase 1 title search's listSessions() + // readdir, (3) searchProjectContent's own readdir. Let the first two + // succeed so projDenied reaches Phase 2, then fail with EACCES + // specifically on call 3 — isolating this test to *searchProjectContent's* + // non-ENOENT passthrough. (Failing at call 2 instead would have the + // title-search phase's own catchAll re-throw the EACCES and abort + // Effect.all before Phase 2 is ever entered — see the dedicated + // "searchSessions title-search TOCTOU safety" block below, which + // exercises exactly that call-2 case.) + const actual = await vi.importActual('node:fs/promises') + let deniedCallCount = 0 + vi.mocked(fs.readdir).mockImplementation(((p: unknown, opts: unknown) => { + if (typeof p === 'string' && p.endsWith(projDenied)) { + deniedCallCount += 1 + if (deniedCallCount > 2) { + const err = new Error( + `EACCES: permission denied, scandir '${p}'` + ) as NodeJS.ErrnoException + err.code = 'EACCES' + return Promise.reject(err) + } + } + return (actual.readdir as typeof fs.readdir)( + p as Parameters[0], + opts as Parameters[1] + ) + }) as typeof fs.readdir) + + await expect( + Effect.runPromise(searchSessions('needle', { searchContent: true })) + ).rejects.toThrow() + }) + }) + + // Phase 1 (title search) TOCTOU guard — a project's folder may vanish + // between listProjects() returning it and listSessions()'s own readdir. + // listSessions() intentionally throws on a missing project (see the guard's + // comment at its Effect.catchAll call site in search.ts), so non-ENOENT + // errors (EACCES, etc.) must still propagate here too, distinct from the + // ENOENT/ENOTDIR skip case. This coverage was previously only accidental + // (the searchProjectContent EACCES test above used to fail one readdir call + // too early and exercise this path instead of its own) — now isolated into + // its own test so it isn't lost if the call-count fix above ever changes. + describe('searchSessions title-search TOCTOU safety', () => { + it('propagates a non-ENOENT readdir error (EACCES) from the title-search phase', async () => { + const projDenied = '-Users-test-search-title-eacces' + await writeSessionFile(projDenied, 'sess-denied', 'plain title') + + // listProjects() readdirs each project folder once (for sessionCount); + // let that succeed so projDenied reaches Phase 1, then fail with EACCES + // specifically on listSessions' own readdir (call 2) — isolating this + // test to the title-search guard, before Phase 2 (searchProjectContent) + // is ever entered. + const actual = await vi.importActual('node:fs/promises') + let deniedCallCount = 0 + vi.mocked(fs.readdir).mockImplementation(((p: unknown, opts: unknown) => { + if (typeof p === 'string' && p.endsWith(projDenied)) { + deniedCallCount += 1 + if (deniedCallCount > 1) { + const err = new Error( + `EACCES: permission denied, scandir '${p}'` + ) as NodeJS.ErrnoException + err.code = 'EACCES' + return Promise.reject(err) + } + } + return (actual.readdir as typeof fs.readdir)( + p as Parameters[0], + opts as Parameters[1] + ) + }) as typeof fs.readdir) + + await expect(Effect.runPromise(searchSessions('needle'))).rejects.toThrow() + }) + }) }) diff --git a/packages/core/src/session/search.ts b/packages/core/src/session/search.ts index a0b7bcc..1b58cb4 100644 --- a/packages/core/src/session/search.ts +++ b/packages/core/src/session/search.ts @@ -5,11 +5,19 @@ import { Effect, pipe } from 'effect' import * as fs from 'node:fs/promises' import * as path from 'node:path' import { getSessionsDir } from '../paths.js' -import { extractTextContent, extractTitle, tryParseJsonLine } from '../utils.js' +import { + extractTextContent, + extractTitle, + isMissingFolderError, + tryParseJsonLine, +} from '../utils.js' +import { createLogger } from '../logger.js' import { listProjects } from './projects.js' import { listSessions } from './crud.js' import type { Message, SearchResult, Project } from '../types.js' +const log = createLogger('search') + // Pure function: extract snippet around match export const extractSnippet = (text: string, matchIndex: number, queryLength: number): string => { const start = Math.max(0, matchIndex - 50) @@ -74,7 +82,10 @@ const searchSessionContent = ( const searchProjectContent = (project: Project, queryLower: string, alreadyFoundIds: Set) => Effect.gen(function* () { const projectPath = path.join(getSessionsDir(), project.name) - const files = yield* Effect.tryPromise(() => fs.readdir(projectPath)) + const files = yield* Effect.tryPromise({ + try: () => fs.readdir(projectPath), + catch: (error) => error as NodeJS.ErrnoException, + }) const sessionFiles = files.filter((f) => f.endsWith('.jsonl') && !f.startsWith('agent-')) // Filter out already found sessions and create search effects @@ -90,7 +101,19 @@ const searchProjectContent = (project: Project, queryLower: string, alreadyFound const results = yield* Effect.all(searchEffects, { concurrency: 10 }) return results.filter((r): r is NonNullable => r !== null) - }) + }).pipe( + // Guard against TOCTOU: a project's folder may vanish between listProjects() + // returning it and this per-project readdir (cross-PC sync, manual deletion). + // Narrow to ENOENT/ENOTDIR so unrelated I/O failures (EACCES, EIO, etc.) still + // propagate. Mirrors listProjects' own guard — see Issue #103. + Effect.catchAll((error) => { + if (!isMissingFolderError(error)) { + return Effect.fail(error) + } + log.debug(`searchProjectContent: skipping missing project ${project.name}`, error) + return Effect.succeed([] as SearchResult[]) + }) + ) // Pattern to detect potential session ID queries (hex chars and hyphens, 8+ chars) const SESSION_ID_PATTERN = /^[a-f0-9][a-f0-9-]{7,}$/i @@ -196,7 +219,26 @@ export const searchSessions = ( timestamp: session.updatedAt, }) satisfies SearchResult ) - ) + ), + // Guard against TOCTOU: `project` just came from listProjects(), but its + // folder may vanish before this per-project listSessions() call (cross-PC + // sync, manual deletion). listSessions() itself intentionally throws on a + // missing project (callers passing an arbitrary/typo'd name should see an + // error — see packages/mcp's "should throw error for non-existent project" + // test), so the guard lives here at the call site, scoped to this + // already-enumerated aggregation context only. Narrow to ENOENT/ENOTDIR so + // unrelated I/O failures (EACCES, EIO, etc.) still propagate. Mirrors + // listProjects' own guard — see Issue #103. + Effect.catchAll((error) => { + if (!isMissingFolderError(error)) { + return Effect.fail(error) + } + log.debug( + `searchSessions: skipping missing project ${project.name} in title search`, + error + ) + return Effect.succeed([] as SearchResult[]) + }) ) ) diff --git a/packages/core/src/utils.ts b/packages/core/src/utils.ts index 3931008..fa61cd2 100644 --- a/packages/core/src/utils.ts +++ b/packages/core/src/utils.ts @@ -33,10 +33,20 @@ export class FileWriteError extends Data.TaggedError('FileWriteError')<{ * (ENOENT, ENOTDIR). Used to narrow TOCTOU-race recovery so unrelated * I/O failures (EACCES, EIO, EMFILE, EBUSY, etc.) still propagate. * + * Unwraps Effect's `UnknownException` (the wrapper `Effect.tryPromise(fn)` + * produces when called without an explicit `{ try, catch }` mapping — the + * original `NodeJS.ErrnoException` ends up on `.error`/`.cause` instead of + * the top level) so callers don't need to know which `tryPromise` form the + * effect they're guarding happens to use. + * * See Issue #103 + .claude/rules/async-io.md. */ export const isMissingFolderError = (error: unknown): boolean => { - const code = (error as NodeJS.ErrnoException | null | undefined)?.code + const codeOf = (e: unknown) => (e as NodeJS.ErrnoException | null | undefined)?.code + const code = + codeOf(error) ?? + codeOf((error as { error?: unknown } | null | undefined)?.error) ?? + codeOf((error as { cause?: unknown } | null | undefined)?.cause) return code === 'ENOENT' || code === 'ENOTDIR' }