From 93ab1de356021fd470cbaf56f599bb9886892282 Mon Sep 17 00:00:00 2001 From: hongye <60014362@kans.cn> Date: Fri, 26 Jun 2026 01:01:15 +0800 Subject: [PATCH 1/3] fix(permissions): normalize MinGW paths in validatePath before resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validatePath` previously used platform-specific `path.isAbsolute` and `path.resolve`. On Windows, this created a sandbox-escape class: - A model emitting `/c/Users/foo/sensitive.txt` (MinGW form, the way Git Bash writes paths) reaches `validatePath`. - `path.isAbsolute('/c/Users/foo/sensitive.txt')` returns true on Windows (treated as a drive-relative absolute path). - `path.resolve('D:\\project', '/c/Users/foo/sensitive.txt')` returns `D:\\c\\Users\\foo\\sensitive.txt` (resolved against the current drive). - The validator compares `D:\\c\\Users\\foo\\sensitive.txt` against the allowed-directories list. If `D:\\` is broadly allowed (e.g. the user's working directory is on D:), validation passes. - Git Bash actually writes to `C:\\Users\\foo\\sensitive.txt` — a completely different filesystem location — bypassing the sandbox. The fix: on Windows, normalize MinGW-style absolute paths (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths (`C:\\Users\\foo`) via `posixPathToWindowsPath` BEFORE the isAbsolute/resolve step. The validator's path space now matches the shell's, so the comparison is against the actual target the shell will write to. `posixPathToWindowsPath` is a no-op for already-Windows paths and relative paths (it just flips slashes), so applying it unconditionally on Windows is safe. Test coverage added in `src/utils/permissions/__tests__/pathValidation.test.ts`: - `/c/...` → `C:\\...` conversion (all 7 cases incl. drive-letter case, bare mount, nested paths) - `/cygdrive/c/...` → `C:\\...` conversion - Already-Windows paths pass through unchanged - Relative paths pass through (just slash direction flipped) - SECURITY regression test: MinGW path that escapes allowed dirs is now correctly denied at the right location - SECURITY regression test: cygdrive variant of the above Verified on Windows 11 + Bun 1.3.14: - `bun run precheck`: 5906/5906 pass (was 5896/5896, +10 new tests) - `bun run build`: succeeds Co-Authored-By: Claude --- .../__tests__/pathValidation.test.ts | 185 ++++++++++++++++++ src/utils/permissions/pathValidation.ts | 25 ++- 2 files changed, 207 insertions(+), 3 deletions(-) create mode 100644 src/utils/permissions/__tests__/pathValidation.test.ts diff --git a/src/utils/permissions/__tests__/pathValidation.test.ts b/src/utils/permissions/__tests__/pathValidation.test.ts new file mode 100644 index 0000000000..169f4131b8 --- /dev/null +++ b/src/utils/permissions/__tests__/pathValidation.test.ts @@ -0,0 +1,185 @@ +import { describe, expect, mock, test } from 'bun:test' +import { logMock } from '../../../../tests/mocks/log' +import { debugMock } from '../../../../tests/mocks/debug' + +// Cut the bootstrap/state dependency chain (mock.module requirement). +mock.module('src/utils/log.ts', logMock) +mock.module('src/utils/debug.ts', debugMock) +mock.module('bun:bundle', () => ({ + feature: (_name: string) => false, +})) + +// MACRO is a build-time define injected by `bun --define` (see +// scripts/dev.ts → -d flags). Without it, `declare const MACRO` references +// in source code resolve to `undefined` at runtime and crash any function +// that touches `MACRO.VERSION` (e.g. `getBundledSkillsRoot` via +// `checkReadableInternalPath`). +// Setting it on globalThis lets the bare `MACRO` identifier resolve at +// runtime in tests. +;(globalThis as unknown as { MACRO: { VERSION: string } }).MACRO = { + VERSION: 'test', +} + +const { validatePath } = await import('../pathValidation.js') +const { getEmptyToolPermissionContext } = await import('../../../Tool.js') + +function makeContext(): ReturnType { + return getEmptyToolPermissionContext() +} + +const isWindows = process.platform === 'win32' +const describeIfWindows = isWindows ? describe : describe.skip + +// ─── MinGW path normalization (Windows) ────────────────────────────────── +// +// These tests pin the fix for a sandbox-escape class: on Windows, the Git +// Bash shell interprets paths like `/c/Users/foo/bar.txt` as `C:\Users\foo\bar.txt` +// (the C: drive). However, the Node `path` module treats such paths as +// drive-relative absolute paths on the current drive, so: +// - path.isAbsolute('/c/Users/foo/bar.txt') === true (on Windows) +// - path.resolve('D:\\project', '/c/Users/foo/bar.txt') +// === 'D:\\c\\Users\\foo\\bar.txt' (on Windows) +// +// That means without normalization, validatePath would compare +// `D:\c\Users\foo\bar.txt` against the allowed-directories list, while +// Git Bash actually writes to `C:\Users\foo\bar.txt` — a completely +// different filesystem location. This is a TOCTOU/sandbox-escape bug. +// +// The fix runs `posixPathToWindowsPath` on Windows before resolution, +// converting `/c/...` and `/cygdrive/c/...` to their `C:\...` form so the +// validator's path space matches the shell's. +describeIfWindows('validatePath MinGW path normalization', () => { + test('converts /c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => { + const result = validatePath( + '/c/Users/foo/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + // resolvedPath is the canonical form the validator (and ultimately the + // shell) operates on. It must be the Windows-style path, not the + // drive-relative form `D:\c\Users\foo\file.txt`. + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('converts /cygdrive/c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => { + const result = validatePath( + '/cygdrive/c/Users/foo/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('uppercases the drive letter', () => { + const result = validatePath( + '/d/work/file.txt', + 'C:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('D:\\work\\file.txt') + }) + + test('preserves Windows paths unchanged', () => { + // An already-Windows path should not be touched. + const result = validatePath( + 'C:\\Users\\foo\\file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\foo\\file.txt', + ) + }) + + test('preserves relative paths (just flips slashes)', () => { + // Relative paths are not MinGW absolute paths; the conversion + // should be a no-op aside from slash direction. The path is then + // resolved against cwd by `validatePath`, which is expected behavior. + const result = validatePath( + 'src/file.txt', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'D:\\project\\src\\file.txt', + ) + }) + + test('handles bare drive mount (no trailing path)', () => { + const result = validatePath('/c', 'D:\\project', makeContext(), 'read') + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\') + }) + + test('handles drive root with trailing slash', () => { + const result = validatePath('/c/', 'D:\\project', makeContext(), 'read') + expect(result.resolvedPath.replace(/\//g, '\\')).toBe('C:\\') + }) + + test('handles deeply nested MinGW paths', () => { + const result = validatePath( + '/c/Users/me/Documents/project/src/index.ts', + 'D:\\project', + makeContext(), + 'read', + ) + expect(result.resolvedPath.replace(/\//g, '\\')).toBe( + 'C:\\Users\\me\\Documents\\project\\src\\index.ts', + ) + }) +}) + +// ─── Sandbox escape regression (Windows) ───────────────────────────────── +// +// This is the bug the MinGW-normalization fix exists to prevent: without +// it, the validator compares `:\c\Users\foo\file.txt` against +// the allowed dirs, while bash writes to `C:\Users\foo\file.txt`. With the +// fix, both sides of the comparison use the same `C:\Users\foo\file.txt` +// location. +// +// We pin this by setting up a context where: +// - cwd is `D:\project` (and D:\project is allowed) +// - `C:\Users\foo` is NOT in any allowed directory +// Then we check that `/c/Users/foo/sensitive.txt` is denied with a +// resolvedPath pointing at C:\Users\foo — proving the validator now sees +// the same path the shell will write to. +describeIfWindows('validatePath sandbox escape regression', () => { + test('MinGW path that escapes allowed dirs is denied at correct location', () => { + // Without the fix, this would resolve to `D:\c\Users\foo\sensitive.txt` + // and (if D:\ is broadly allowed) pass validation, while bash actually + // writes to `C:\Users\foo\sensitive.txt`. With the fix, the validator + // sees the correct path and denies it because C:\Users\foo is not in + // any allowed directory. + const result = validatePath( + '/c/Users/foo/sensitive.txt', + 'D:\\project', + makeContext(), + 'create', + ) + expect(result.allowed).toBe(false) + // The resolvedPath should be at C:\Users\foo — not D:\c\Users\foo. + const normalized = result.resolvedPath.replace(/\//g, '\\') + expect(normalized.startsWith('C:\\Users\\foo')).toBe(true) + expect(normalized.startsWith('D:\\c\\')).toBe(false) + }) + + test('cygdrive path that escapes allowed dirs is denied at correct location', () => { + const result = validatePath( + '/cygdrive/c/Users/foo/sensitive.txt', + 'D:\\project', + makeContext(), + 'create', + ) + expect(result.allowed).toBe(false) + const normalized = result.resolvedPath.replace(/\//g, '\\') + expect(normalized.startsWith('C:\\Users\\foo')).toBe(true) + }) +}) diff --git a/src/utils/permissions/pathValidation.ts b/src/utils/permissions/pathValidation.ts index c8fdf7f8fb..5790aad014 100644 --- a/src/utils/permissions/pathValidation.ts +++ b/src/utils/permissions/pathValidation.ts @@ -3,6 +3,7 @@ import { homedir } from 'os' import { dirname, isAbsolute, resolve } from 'path' import type { ToolPermissionContext } from '../../Tool.js' import { getPlatform } from '../../utils/platform.js' +import { posixPathToWindowsPath } from '../windowsPaths.js' import { getFsImplementation, getPathsForPermissionCheck, @@ -472,9 +473,27 @@ export function validatePath( } // Resolve path - const absolutePath = isAbsolute(cleanPath) - ? cleanPath - : resolve(cwd, cleanPath) + // SECURITY: On Windows, normalize MinGW-style absolute paths + // (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths + // (`C:\Users\foo`) BEFORE the isAbsolute/resolve step below. + // + // Without this, `path.isAbsolute('/c/Users/foo')` returns true on + // Windows (interpreted as a drive-relative absolute path) and + // `path.resolve(cwd, '/c/Users/foo')` produces `:\c\Users\foo` + // — a completely different filesystem location from what Git Bash + // actually writes to (`C:\Users\foo`). The validator would compare + // the wrong path against the allowed-directories list, enabling a + // sandbox escape where `/c/Users/foo/sensitive.txt` passes validation + // when `C:\Users\foo\sensitive.txt` is not in any allowed dir. + // + // `posixPathToWindowsPath` is a no-op for already-Windows paths and + // for relative paths (it just flips slashes), so it's safe to apply + // unconditionally on Windows. + const pathForResolve = + getPlatform() === 'windows' ? posixPathToWindowsPath(cleanPath) : cleanPath + const absolutePath = isAbsolute(pathForResolve) + ? pathForResolve + : resolve(cwd, pathForResolve) const { resolvedPath, isCanonical } = safeResolvePath( getFsImplementation(), absolutePath, From 75c037695a13d7cb1df00ae90941b177b77943be Mon Sep 17 00:00:00 2001 From: hongye <60014362@kans.cn> Date: Fri, 26 Jun 2026 10:51:55 +0800 Subject: [PATCH 2/3] docs(permissions): add JSDoc to address CodeRabbit docstring coverage warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit flagged the PR with "Docstring coverage is 50.00% which is insufficient (threshold 80.00%)". Added JSDoc to: 1. `formatDirectoryList` in pathValidation.ts — the only production function in the changed files that lacked JSDoc. Brief description of the truncation behavior + reference to MAX_DIRS_TO_LIST. 2. Both `describeIfWindows` blocks in pathValidation.test.ts — explain the security rationale of the MinGW-normalization fix and the sandbox-escape regression scenarios. Individual `test()` calls already self-document via their string descriptions. Co-Authored-By: Claude --- .../__tests__/pathValidation.test.ts | 18 ++++++++++++++++++ src/utils/permissions/pathValidation.ts | 6 ++++++ 2 files changed, 24 insertions(+) diff --git a/src/utils/permissions/__tests__/pathValidation.test.ts b/src/utils/permissions/__tests__/pathValidation.test.ts index 169f4131b8..7c9716cae0 100644 --- a/src/utils/permissions/__tests__/pathValidation.test.ts +++ b/src/utils/permissions/__tests__/pathValidation.test.ts @@ -48,6 +48,14 @@ const describeIfWindows = isWindows ? describe : describe.skip // The fix runs `posixPathToWindowsPath` on Windows before resolution, // converting `/c/...` and `/cygdrive/c/...` to their `C:\...` form so the // validator's path space matches the shell's. +/** + * Tests that `validatePath` normalizes MinGW-style absolute paths + * (`/c/Users/foo`, `/cygdrive/c/Users/foo`) to Windows paths + * (`C:\\Users\\foo`) on Windows. Without this, the validator runs in + * Windows host path space while the Git Bash shell runs in MinGW path + * space, leading to a sandbox-escape class — see the comment block + * at the top of this file for the full security rationale. + */ describeIfWindows('validatePath MinGW path normalization', () => { test('converts /c/Users/foo/file.txt to C:\\Users\\foo\\file.txt', () => { const result = validatePath( @@ -151,6 +159,16 @@ describeIfWindows('validatePath MinGW path normalization', () => { // Then we check that `/c/Users/foo/sensitive.txt` is denied with a // resolvedPath pointing at C:\Users\foo — proving the validator now sees // the same path the shell will write to. +/** + * Regression tests for the sandbox-escape class the MinGW-normalization + * fix prevents. Without the fix, a MinGW-style path like + * `/c/Users/foo/sensitive.txt` is resolved (by `path.resolve`) against + * the current drive (`D:\c\Users\foo\sensitive.txt`) and compared to + * the allowed-directories list — while Git Bash actually writes to + * `C:\Users\foo\sensitive.txt`. With the fix, both sides of the + * comparison use the same Windows path so a path the shell will write + * to but isn't in any allowed dir is denied. + */ describeIfWindows('validatePath sandbox escape regression', () => { test('MinGW path that escapes allowed dirs is denied at correct location', () => { // Without the fix, this would resolve to `D:\c\Users\foo\sensitive.txt` diff --git a/src/utils/permissions/pathValidation.ts b/src/utils/permissions/pathValidation.ts index 5790aad014..76cf83b651 100644 --- a/src/utils/permissions/pathValidation.ts +++ b/src/utils/permissions/pathValidation.ts @@ -36,6 +36,12 @@ export type ResolvedPathCheckResult = PathCheckResult & { resolvedPath: string } +/** + * Format a list of working directories for inclusion in permission-prompt + * messages. When the list is short (≤ {@link MAX_DIRS_TO_LIST}) all + * directories are listed; when longer, a truncated form is shown with + * the remaining count. + */ export function formatDirectoryList(directories: string[]): string { const dirCount = directories.length From 562da4c9d5daf210c7a69ba9d678b31bf411efff Mon Sep 17 00:00:00 2001 From: hongye <60014362@kans.cn> Date: Fri, 26 Jun 2026 14:43:40 +0800 Subject: [PATCH 3/3] fix(session-storage): bound getSessionMessages cache to prevent unbounded growth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getSessionMessages` was implemented via `lodash memoize`, which has no size cap. Each unique sessionId adds a `Promise>` entry that is only cleared by `clearSessionMessagesCache()` — and that call is only made from `postCompactCleanup`, which fires once per compaction. In long-running daemon / swarm sessions that spawn many agents (each with its own transcript), the cache grows unbounded for the process lifetime. Per-entry Sets are MUCH larger than the file-path strings the adjacent `existingSessionFiles` cache stores, so the leak is more severe than the one fixed for that cache at `sessionStorage.ts:535`. The fix mirrors the existing `existingSessionFiles` pattern: replace lodash `memoize` with a `Map>>` and evict the oldest entry (FIFO via Map insertion order) when at `MAX_CACHED_SESSION_FILES` capacity. Re-uses the same constant for consistency. Concretely: - `sessionStorage.ts`: refactor `getSessionMessages` to be a regular `async function` backed by a module-level `Map`, with FIFO eviction at `MAX_CACHED_SESSION_FILES`. Export `getSessionMessagesCache()` so `getLastSessionLog`'s priming logic (and tests) can interact with the underlying Map directly. Export `getSessionMessages` itself with a `@internal`-style comment noting it's not part of the public API (callers should keep using `doesMessageExistInSession`). - `src/utils/__tests__/sessionStorage.test.ts` (new): 7 tests covering cache identity, clear behavior, concurrent-call dedup, bounded size after many distinct sessionIds, FIFO eviction order, and refill after clear. Uses real temp directory + `CLAUDE_CONFIG_DIR` env to keep tests hermetic — no `mock.module` pollution. Verified on Windows 11 + Bun 1.3.14: - `bun run precheck`: 5899/5900 pass (the 1 fail is a pre-existing flaky `toolEventObserver` test that times out on Windows runners, unrelated to this change) - 7/7 new tests pass - No regressions in other test files Co-Authored-By: Claude --- src/utils/__tests__/sessionStorage.test.ts | 151 +++++++++++++++++++++ src/utils/sessionStorage.ts | 55 ++++++-- 2 files changed, 193 insertions(+), 13 deletions(-) create mode 100644 src/utils/__tests__/sessionStorage.test.ts diff --git a/src/utils/__tests__/sessionStorage.test.ts b/src/utils/__tests__/sessionStorage.test.ts new file mode 100644 index 0000000000..b0cf13a5f4 --- /dev/null +++ b/src/utils/__tests__/sessionStorage.test.ts @@ -0,0 +1,151 @@ +import { afterEach, beforeEach, describe, expect, test } from 'bun:test' +import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' + +const MAX_CACHED_ENTRIES = 200 // mirrors MAX_CACHED_SESSION_FILES in sessionStorage.ts + +const { + getSessionMessages, + getSessionMessagesCache, + clearSessionMessagesCache, +} = await import('../sessionStorage.js') + +function asUuid(s: string): any { + return s as unknown as any +} + +let tempDir: string +let originalConfigDir: string | undefined + +beforeEach(() => { + tempDir = join( + tmpdir(), + `claude-session-test-${Date.now()}-${Math.random().toString(36).slice(2)}`, + ) + mkdirSync(tempDir, { recursive: true }) + // `getProjectsDir()` returns `${CLAUDE_CONFIG_DIR}/projects`, and + // loadSessionFile reads from `${getProjectsDir()}/${sessionId}.jsonl`. + // Pre-create the projects subdir so writeFileSync doesn't fail. + mkdirSync(join(tempDir, 'projects'), { recursive: true }) + // Pin session-file lookups to a temp dir via CLAUDE_CONFIG_DIR. + // Restoring in afterEach keeps tests hermetic. + originalConfigDir = process.env.CLAUDE_CONFIG_DIR + process.env.CLAUDE_CONFIG_DIR = tempDir +}) + +afterEach(() => { + clearSessionMessagesCache() + if (originalConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR + } else { + process.env.CLAUDE_CONFIG_DIR = originalConfigDir + } + if (tempDir && existsSync(tempDir)) { + rmSync(tempDir, { recursive: true, force: true }) + } +}) + +function sessionFilePath(sessionId: string): string { + // Mirror sessionStorage.ts's path computation: + // getSessionProjectDir() ?? getProjectDir(getOriginalCwd()) + // With CLAUDE_CONFIG_DIR=tempDir and getSessionProjectDir() returning + // null in tests, files live at `${tempDir}/projects/${sessionId}.jsonl`. + return join(tempDir, 'projects', `${sessionId}.jsonl`) +} + +describe('getSessionMessagesCache', () => { + test('returns the same Map instance across calls', () => { + // Cache identity must be stable — `getLastSessionLog` uses + // `getSessionMessagesCache()` directly to prime entries, so a + // different instance each call would break that priming. + expect(getSessionMessagesCache()).toBe(getSessionMessagesCache()) + }) + + test('clearSessionMessagesCache empties a populated cache', async () => { + const cache = getSessionMessagesCache() + writeFileSync(sessionFilePath('id-1'), '') + writeFileSync(sessionFilePath('id-2'), '') + await getSessionMessages(asUuid('id-1')) + await getSessionMessages(asUuid('id-2')) + expect(cache.size).toBeGreaterThan(0) + + clearSessionMessagesCache() + expect(cache.size).toBe(0) + }) + + test('clearSessionMessagesCache is a no-op on empty cache', () => { + const cache = getSessionMessagesCache() + expect(cache.size).toBe(0) + clearSessionMessagesCache() + expect(cache.size).toBe(0) + }) + + test('getSessionMessages dedups concurrent calls for the same sessionId', async () => { + const cache = getSessionMessagesCache() + const id = asUuid('same-id') + writeFileSync(sessionFilePath('same-id'), '') + const [a, b, c] = await Promise.all([ + getSessionMessages(id), + getSessionMessages(id), + getSessionMessages(id), + ]) + expect(a).toBe(b) + expect(b).toBe(c) + expect(cache.size).toBe(1) + }) +}) + +describe('getSessionMessages bounded cache (memory leak fix)', () => { + test('cache size stays at MAX_CACHED_ENTRIES after many distinct sessionIds', async () => { + // Bounded cache — calling getSessionMessages with N distinct + // sessionIds must NOT grow the cache beyond MAX_CACHED_ENTRIES. + // Pre-fix: lodash memoize grew unbounded. Post-fix: Map-based + // cache evicts oldest entry when at capacity. + const cache = getSessionMessagesCache() + const total = MAX_CACHED_ENTRIES * 3 // 600 distinct sessionIds + for (let i = 0; i < total; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + }) + + test('FIFO eviction: oldest entry is removed first', async () => { + // Fill cache to MAX with sequential ids. The first inserted + // (`oldest`) should be evicted on the (MAX+1)th insertion. + const cache = getSessionMessagesCache() + const oldestId = asUuid('id-0') + writeFileSync(sessionFilePath('id-0'), '') + await getSessionMessages(oldestId) + for (let i = 1; i < MAX_CACHED_ENTRIES; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + expect(cache.has(oldestId)).toBe(true) + + writeFileSync(sessionFilePath('id-overflow'), '') + await getSessionMessages(asUuid('id-overflow')) + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + expect(cache.has(oldestId)).toBe(false) + }) + + test('cleared cache can be refilled without leaking entries', async () => { + const cache = getSessionMessagesCache() + for (let i = 0; i < MAX_CACHED_ENTRIES; i++) { + writeFileSync(sessionFilePath(`id-${i}`), '') + await getSessionMessages(asUuid(`id-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + + clearSessionMessagesCache() + expect(cache.size).toBe(0) + + for (let i = 0; i < MAX_CACHED_ENTRIES + 5; i++) { + writeFileSync(sessionFilePath(`refill-${i}`), '') + await getSessionMessages(asUuid(`refill-${i}`)) + } + expect(cache.size).toBe(MAX_CACHED_ENTRIES) + }) +}) diff --git a/src/utils/sessionStorage.ts b/src/utils/sessionStorage.ts index 92e87e5c05..d6da26184a 100644 --- a/src/utils/sessionStorage.ts +++ b/src/utils/sessionStorage.ts @@ -3941,24 +3941,55 @@ async function loadSessionFile(sessionId: UUID): Promise<{ return loadTranscriptFile(sessionFile) } +/** + * Bounded cache for {@link getSessionMessages}. Bounded at + * {@link MAX_CACHED_SESSION_FILES} to prevent unbounded Map growth in + * long-running daemon / swarm sessions that spawn many agents — mirrors + * the existingSessionFiles pattern above. Entries are evicted in FIFO + * order via `Map` insertion order. + */ +const sessionMessagesCache = new Map>>() + /** * Gets message UUIDs for a specific session without loading all sessions. - * Memoized to avoid re-reading the same session file multiple times. + * Cached (bounded) to avoid re-reading the same session file multiple + * times. Concurrent calls for the same `sessionId` share one in-flight + * load promise. + * + * Exported so tests can verify cache eviction behavior directly; not + * intended as a public API — prefer {@link doesMessageExistInSession}. */ -const getSessionMessages = memoize( - async (sessionId: UUID): Promise> => { +export async function getSessionMessages(sessionId: UUID): Promise> { + const existing = sessionMessagesCache.get(sessionId) + if (existing !== undefined) { + return existing + } + // Evict oldest entry when at capacity so the Map stays bounded. + if (sessionMessagesCache.size >= MAX_CACHED_SESSION_FILES) { + const oldestKey = sessionMessagesCache.keys().next().value + if (oldestKey !== undefined) { + sessionMessagesCache.delete(oldestKey) + } + } + const promise = (async () => { const { messages } = await loadSessionFile(sessionId) return new Set(messages.keys()) - }, - (sessionId: UUID) => sessionId, -) + })() + sessionMessagesCache.set(sessionId, promise) + return promise +} + +/** Underlying cache for direct manipulation (priming, clearing, tests). */ +export function getSessionMessagesCache(): Map>> { + return sessionMessagesCache +} /** - * Clear the memoized session messages cache. + * Clear the cached session messages. * Call after compaction when old message UUIDs are no longer valid. */ export function clearSessionMessagesCache(): void { - getSessionMessages.cache.clear?.() + sessionMessagesCache.clear() } /** @@ -3996,11 +4027,9 @@ export async function getLastSessionLog( // Guard: only prime if cache is empty. Mid-session callers (e.g. IssueFeedback) // may call getLastSessionLog on the current session — overwriting a live cache // with a stale disk snapshot would lose unflushed UUIDs and break dedup. - if (!getSessionMessages.cache.has(sessionId)) { - getSessionMessages.cache.set( - sessionId, - Promise.resolve(new Set(messages.keys())), - ) + const messagesCache = getSessionMessagesCache() + if (!messagesCache.has(sessionId)) { + messagesCache.set(sessionId, Promise.resolve(new Set(messages.keys()))) } // Find the most recent non-sidechain message