From 36e079e83ca512ec5810e67dfea2feca9c88fd98 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Sun, 23 Aug 2026 18:40:45 -0500 Subject: [PATCH] refactor: move reflection state behind storage (#1205) --- .../service-filesystem-boundary.json | 8 +- docs/testing/critical-path-coverage.json | 1 + .../reflection-state-repository.test.ts | 112 ++++++++++++++++++ server/src/services/reflection-service.ts | 49 +++----- server/src/storage/index.ts | 5 + .../storage/reflection-state-repository.ts | 84 +++++++++++++ 6 files changed, 218 insertions(+), 41 deletions(-) create mode 100644 server/src/__tests__/reflection-state-repository.test.ts create mode 100644 server/src/storage/reflection-state-repository.ts diff --git a/docs/architecture/service-filesystem-boundary.json b/docs/architecture/service-filesystem-boundary.json index 9253337c..ca8b0c15 100644 --- a/docs/architecture/service-filesystem-boundary.json +++ b/docs/architecture/service-filesystem-boundary.json @@ -1,6 +1,6 @@ { "schemaVersion": 1, - "maximumEntries": 39, + "maximumEntries": 38, "entries": [ { "path": "server/src/services/agent-health-service.ts", @@ -140,12 +140,6 @@ "owner": "#1187", "rationale": "Operational evidence storage migration is tracked in issue #1187." }, - { - "path": "server/src/services/reflection-service.ts", - "category": "authoritative-persistence", - "owner": "#1186", - "rationale": "Coordination-state storage migration is tracked in issue #1186." - }, { "path": "server/src/services/run-session-share-service.ts", "category": "authoritative-persistence", diff --git a/docs/testing/critical-path-coverage.json b/docs/testing/critical-path-coverage.json index f9e92970..bc3a1e1d 100644 --- a/docs/testing/critical-path-coverage.json +++ b/docs/testing/critical-path-coverage.json @@ -50,6 +50,7 @@ "src/__tests__/provider-runtime-manifest-service.test.ts", "src/__tests__/provider-task-envelope-renderer.test.ts", "src/__tests__/reflection-extraction-job-service.test.ts", + "src/__tests__/reflection-state-repository.test.ts", "src/__tests__/runtime-paths.test.ts", "src/__tests__/scheduler-state-repository.test.ts", "src/__tests__/scheduled-deliverables-repository.test.ts", diff --git a/server/src/__tests__/reflection-state-repository.test.ts b/server/src/__tests__/reflection-state-repository.test.ts new file mode 100644 index 00000000..c2cdf106 --- /dev/null +++ b/server/src/__tests__/reflection-state-repository.test.ts @@ -0,0 +1,112 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { lstat, mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises'; +import path from 'node:path'; +import type { ReflectionCandidate } from '@veritas-kanban/shared'; +import { FileReflectionStateRepository } from '../storage/reflection-state-repository.js'; + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, lstat: vi.fn(actual.lstat) }; +}); + +function candidate(id: string, summary = id): ReflectionCandidate { + return { + id, + status: 'pending', + category: 'team', + promotionTarget: 'memory', + confidence: 0.5, + source: { kind: 'user-correction' }, + summary, + previousApproach: 'old', + correction: 'new', + nextAttempt: 'retry', + evidence: [], + tags: [], + duplicateKey: id, + duplicateCount: 1, + appliedTargets: [], + redaction: { redacted: false, notes: [] }, + createdAt: '2026-08-23T00:00:00.000Z', + updatedAt: '2026-08-23T00:00:00.000Z', + }; +} + +describe('FileReflectionStateRepository', () => { + let root: string; + let storageDir: string; + let repository: FileReflectionStateRepository; + + beforeEach(async () => { + root = await mkdtemp(path.join(process.cwd(), '.veritas-reflection-state-')); + storageDir = path.join(root, 'reflections'); + repository = new FileReflectionStateRepository(storageDir); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + it('reads missing state and atomically replaces normalized state', async () => { + await expect(repository.read()).resolves.toBeNull(); + const state = { + version: 1 as const, + candidates: [candidate('one')], + updatedAt: '2026-08-23T00:00:00.000Z', + }; + await repository.write(state); + await expect(repository.read()).resolves.toEqual(state); + await repository.write({ ...state, candidates: [candidate('two')] }); + await expect(repository.read()).resolves.toEqual({ + ...state, + candidates: [candidate('two')], + }); + }); + + it('rejects symbolic links, changed files, and non-file paths', async () => { + await mkdir(storageDir, { recursive: true }); + const stateFile = path.join(storageDir, 'candidates.json'); + const target = path.join(root, 'outside.json'); + await writeFile(target, JSON.stringify({ version: 1, candidates: [] }), 'utf8'); + await symlink(target, stateFile); + await expect(repository.read()).rejects.toThrow(/symbolic link/i); + + await rm(stateFile); + await writeFile(stateFile, JSON.stringify({ version: 1, candidates: [] }), 'utf8'); + const actual = await vi.importActual('node:fs/promises'); + vi.mocked(lstat).mockImplementationOnce(async (filePath) => { + const stats = await actual.lstat(filePath); + return Object.assign(Object.create(Object.getPrototypeOf(stats)), stats, { + ino: stats.ino + 1, + }); + }); + await expect(repository.read()).rejects.toThrow(/changed file/i); + + await rm(stateFile); + await mkdir(stateFile); + await expect(repository.read()).rejects.toThrow(/bounded regular file/i); + }); + + it('rejects symbolic-link directories and oversized state', async () => { + const realDirectory = path.join(root, 'real-reflections'); + const linkedDirectory = path.join(root, 'linked-reflections'); + await mkdir(realDirectory); + await symlink(realDirectory, linkedDirectory, 'dir'); + const linkedRepository = new FileReflectionStateRepository(linkedDirectory); + await expect( + linkedRepository.write({ + version: 1, + candidates: [], + updatedAt: '2026-08-23T00:00:00.000Z', + }) + ).rejects.toThrow(/regular directory/i); + + await expect( + repository.write({ + version: 1, + candidates: [candidate('large', 'x'.repeat(16 * 1024 * 1024))], + updatedAt: '2026-08-23T00:00:00.000Z', + }) + ).rejects.toThrow(/16 MiB/i); + }); +}); diff --git a/server/src/services/reflection-service.ts b/server/src/services/reflection-service.ts index 1545c0a7..30ae87c3 100644 --- a/server/src/services/reflection-service.ts +++ b/server/src/services/reflection-service.ts @@ -1,4 +1,3 @@ -import fs from 'fs/promises'; import path from 'path'; import { nanoid } from 'nanoid'; import type { @@ -22,12 +21,16 @@ import type { TaskEnvelopeMemoryReference, } from '@veritas-kanban/shared'; import { auditLog, type AuditEvent } from './audit-service.js'; -import { withFileLock } from './file-lock.js'; import { getTaskService } from './task-service.js'; import { ConflictError, NotFoundError } from '../middleware/error-handler.js'; -import { ensureWithinBase, stripHtml, validatePathSegment } from '../utils/sanitize.js'; +import { stripHtml, validatePathSegment } from '../utils/sanitize.js'; import { getRuntimeDir } from '../utils/paths.js'; import { createLogger } from '../lib/logger.js'; +import { + FileReflectionStateRepository, + type ReflectionState, + type ReflectionStateRepository, +} from '../storage/reflection-state-repository.js'; import { getReflectionPromotionAdapterRegistry, type ReflectionPromotionAdapterRegistry, @@ -43,12 +46,6 @@ const MAX_SOURCE_EVENT_IDS = 20; const MAX_RETRIEVALS_PER_CANDIDATE = 100; const DEFAULT_RETRIEVAL_LIMIT = 8; -interface ReflectionState { - version: 1; - candidates: ReflectionCandidate[]; - updatedAt: string; -} - export interface ReflectionListFilters { status?: ReflectionCandidateStatus; category?: ReflectionCandidateCategory; @@ -76,6 +73,7 @@ export interface ReflectionTaskService { export interface ReflectionServiceOptions { storageDir?: string; + stateRepository?: ReflectionStateRepository; persist?: boolean; audit?: (event: AuditEvent) => Promise; taskService?: ReflectionTaskService; @@ -143,7 +141,7 @@ function lessonEntry( } export class ReflectionService { - private readonly storageDir: string; + private readonly stateRepository: ReflectionStateRepository; private readonly persist: boolean; private readonly audit: (event: AuditEvent) => Promise; private readonly taskService: ReflectionTaskService; @@ -152,7 +150,8 @@ export class ReflectionService { private state: ReflectionState = this.emptyState(); constructor(options: ReflectionServiceOptions = {}) { - this.storageDir = options.storageDir ?? path.join(getRuntimeDir(), 'reflections'); + const storageDir = options.storageDir ?? path.join(getRuntimeDir(), 'reflections'); + this.stateRepository = options.stateRepository ?? new FileReflectionStateRepository(storageDir); this.persist = options.persist ?? process.env.VITEST !== 'true'; this.audit = options.audit ?? auditLog; this.taskService = options.taskService ?? getTaskService(); @@ -664,20 +663,11 @@ export class ReflectionService { return; } - await fs.mkdir(this.storageDir, { recursive: true }); - try { - const raw = await fs.readFile(this.statePath, 'utf-8'); - const parsed = JSON.parse(raw) as Partial; - this.state = { - version: 1, - candidates: Array.isArray(parsed.candidates) - ? (parsed.candidates as ReflectionCandidate[]) - : [], - updatedAt: typeof parsed.updatedAt === 'string' ? parsed.updatedAt : nowIso(), - }; + const state = await this.stateRepository.read(); + if (state) { + this.state = state; this.refreshDuplicateCounts(); - } catch (err) { - if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } else { this.state = this.emptyState(); } this.loaded = true; @@ -686,16 +676,7 @@ export class ReflectionService { private async saveState(): Promise { this.state.updatedAt = nowIso(); if (!this.persist) return; - await fs.mkdir(this.storageDir, { recursive: true }); - await withFileLock(this.statePath, async () => { - await fs.writeFile(this.statePath, JSON.stringify(this.state, null, 2), 'utf-8'); - }); - } - - private get statePath(): string { - const filePath = path.join(this.storageDir, 'candidates.json'); - ensureWithinBase(this.storageDir, filePath); - return filePath; + await this.stateRepository.write(this.state); } private emptyState(): ReflectionState { diff --git a/server/src/storage/index.ts b/server/src/storage/index.ts index c4ca2278..cdab2a19 100644 --- a/server/src/storage/index.ts +++ b/server/src/storage/index.ts @@ -55,6 +55,11 @@ export { type SchedulerState, type SchedulerStateRepository, } from './scheduler-state-repository.js'; +export { + FileReflectionStateRepository, + type ReflectionState, + type ReflectionStateRepository, +} from './reflection-state-repository.js'; export { FileScheduledDeliverablesStore } from './scheduled-deliverables-repository.js'; export { FileBroadcastRepository } from './broadcast-repository.js'; export { diff --git a/server/src/storage/reflection-state-repository.ts b/server/src/storage/reflection-state-repository.ts new file mode 100644 index 00000000..a8df9c5c --- /dev/null +++ b/server/src/storage/reflection-state-repository.ts @@ -0,0 +1,84 @@ +import { constants } from 'node:fs'; +import { lstat, mkdir, open } from 'node:fs/promises'; +import path from 'node:path'; +import type { ReflectionCandidate } from '@veritas-kanban/shared'; +import { withFileLock } from '../services/file-lock.js'; +import { ensureWithinBase } from '../utils/sanitize.js'; +import { atomicWriteFile } from './fs-helpers.js'; + +const MAX_REFLECTION_STATE_BYTES = 16 * 1024 * 1024; + +export interface ReflectionState { + version: 1; + candidates: ReflectionCandidate[]; + updatedAt: string; +} + +export interface ReflectionStateRepository { + read(): Promise; + write(state: ReflectionState): Promise; +} + +function normalizeState(parsed: Partial): ReflectionState { + return { + version: 1, + candidates: Array.isArray(parsed.candidates) + ? (parsed.candidates as ReflectionCandidate[]) + : [], + updatedAt: typeof parsed.updatedAt === 'string' ? parsed.updatedAt : new Date().toISOString(), + }; +} + +export class FileReflectionStateRepository implements ReflectionStateRepository { + private readonly storageDir: string; + private readonly stateFile: string; + + constructor(storageDir: string) { + this.storageDir = path.resolve(storageDir); + this.stateFile = ensureWithinBase( + this.storageDir, + path.join(this.storageDir, 'candidates.json') + ); + } + + async read(): Promise { + let handle: Awaited> | undefined; + try { + handle = await open(this.stateFile, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + const [pathStats, stats] = await Promise.all([lstat(this.stateFile), handle.stat()]); + if ( + pathStats.isSymbolicLink() || + pathStats.dev !== stats.dev || + pathStats.ino !== stats.ino + ) { + throw new Error('Reflection state must not use a symbolic link or changed file'); + } + if (!stats.isFile() || stats.size > MAX_REFLECTION_STATE_BYTES) { + throw new Error('Reflection state must use a bounded regular file'); + } + return normalizeState(JSON.parse(await handle.readFile({ encoding: 'utf8' }))); + } catch (error) { + const errorCode = (error as NodeJS.ErrnoException).code; + if (errorCode === 'ENOENT') return null; + if (errorCode === 'ELOOP') { + throw new Error('Reflection state must not use a symbolic link', { cause: error }); + } + throw error; + } finally { + await handle?.close(); + } + } + + async write(state: ReflectionState): Promise { + await mkdir(this.storageDir, { recursive: true, mode: 0o700 }); + const directoryStats = await lstat(this.storageDir); + if (!directoryStats.isDirectory() || directoryStats.isSymbolicLink()) { + throw new Error('Reflection state path must use a regular directory'); + } + const content = JSON.stringify(state, null, 2); + if (Buffer.byteLength(content, 'utf8') > MAX_REFLECTION_STATE_BYTES) { + throw new Error('Reflection state exceeds the 16 MiB storage limit'); + } + await withFileLock(this.stateFile, () => atomicWriteFile(this.stateFile, content, 'utf8')); + } +}