From f5e361ab8fef61cbebbc6163259be7e5b31f7ab9 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Sun, 23 Aug 2026 18:07:20 -0500 Subject: [PATCH] refactor: move error analyses behind storage (#1200) --- .../service-filesystem-boundary.json | 8 +- docs/testing/critical-path-coverage.json | 1 + .../error-analysis-repository.test.ts | 140 +++++++++++++++++ server/src/services/error-learning-service.ts | 148 +++++++----------- .../src/storage/error-analysis-repository.ts | 135 ++++++++++++++++ server/src/storage/index.ts | 5 + 6 files changed, 342 insertions(+), 95 deletions(-) create mode 100644 server/src/__tests__/error-analysis-repository.test.ts create mode 100644 server/src/storage/error-analysis-repository.ts diff --git a/docs/architecture/service-filesystem-boundary.json b/docs/architecture/service-filesystem-boundary.json index 9afd382b..4c38807b 100644 --- a/docs/architecture/service-filesystem-boundary.json +++ b/docs/architecture/service-filesystem-boundary.json @@ -1,6 +1,6 @@ { "schemaVersion": 1, - "maximumEntries": 44, + "maximumEntries": 43, "entries": [ { "path": "server/src/services/agent-health-service.ts", @@ -62,12 +62,6 @@ "owner": "#1188", "rationale": "Managed-content storage migration is tracked in issue #1188." }, - { - "path": "server/src/services/error-learning-service.ts", - "category": "authoritative-persistence", - "owner": "#1186", - "rationale": "Coordination-state storage migration is tracked in issue #1186." - }, { "path": "server/src/services/external-tracker-service.ts", "category": "authoritative-persistence", diff --git a/docs/testing/critical-path-coverage.json b/docs/testing/critical-path-coverage.json index c75fde9f..36505b11 100644 --- a/docs/testing/critical-path-coverage.json +++ b/docs/testing/critical-path-coverage.json @@ -30,6 +30,7 @@ "src/__tests__/credential-broker-service.test.ts", "src/__tests__/delegation-repository.test.ts", "src/__tests__/enforcement.test.ts", + "src/__tests__/error-analysis-repository.test.ts", "src/__tests__/filesystem-sandbox-service.test.ts", "src/__tests__/harness-support-profile-schemas.test.ts", "src/__tests__/hermes-provider.test.ts", diff --git a/server/src/__tests__/error-analysis-repository.test.ts b/server/src/__tests__/error-analysis-repository.test.ts new file mode 100644 index 00000000..76b651db --- /dev/null +++ b/server/src/__tests__/error-analysis-repository.test.ts @@ -0,0 +1,140 @@ +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 { ErrorLearningService, type ErrorAnalysis } from '../services/error-learning-service.js'; +import { + FileErrorAnalysisRepository, + InMemoryErrorAnalysisRepository, +} from '../storage/error-analysis-repository.js'; + +vi.mock('node:fs/promises', async (importOriginal) => { + const actual = await importOriginal(); + return { ...actual, lstat: vi.fn(actual.lstat) }; +}); + +function analysis(id: string): ErrorAnalysis { + return { + id, + context: { errorMessage: `Failure ${id}`, occurredAt: '2026-08-23T20:00:00.000Z' }, + rootCause: '', + summary: `Failure ${id}`, + severity: 'medium', + optionsConsidered: [], + chosenFix: '', + preventionSteps: [], + tags: [], + relatedTasks: [], + isRepeat: false, + previousOccurrences: [], + analyzedAt: '2026-08-23T20:00:00.000Z', + }; +} + +describe('FileErrorAnalysisRepository', () => { + let root: string; + let runtimeDir: string; + let repository: FileErrorAnalysisRepository; + + beforeEach(async () => { + root = await mkdtemp(path.join(process.cwd(), '.veritas-error-analysis-')); + runtimeDir = path.join(root, 'runtime'); + repository = new FileErrorAnalysisRepository(runtimeDir, []); + }); + + afterEach(async () => { + await rm(root, { recursive: true, force: true }); + }); + + it('reads defaults and serializes concurrent updates', async () => { + await expect(repository.read()).resolves.toEqual([]); + await Promise.all([ + repository.update((analyses) => [...analyses, analysis('one')]), + repository.update((analyses) => [...analyses, analysis('two')]), + ]); + expect((await repository.read()).map(({ id }) => id)).toEqual( + expect.arrayContaining(['one', 'two']) + ); + }); + + it('migrates legacy data and tolerates malformed or non-array JSON', async () => { + const legacyDir = path.join(root, 'legacy'); + await mkdir(legacyDir); + await writeFile( + path.join(legacyDir, 'error-analyses.json'), + JSON.stringify([analysis('legacy')]), + 'utf8' + ); + const migratingRepository = new FileErrorAnalysisRepository(runtimeDir, [legacyDir]); + await expect(migratingRepository.read()).resolves.toEqual([analysis('legacy')]); + + await writeFile(path.join(runtimeDir, 'error-analyses.json'), '{broken', 'utf8'); + await expect(repository.read()).resolves.toEqual([]); + await writeFile(path.join(runtimeDir, 'error-analyses.json'), '{}', 'utf8'); + await expect(repository.read()).resolves.toEqual([]); + }); + + it('rejects symbolic links, changed files, and non-file paths', async () => { + await mkdir(runtimeDir, { recursive: true }); + const target = path.join(root, 'outside.json'); + await writeFile(target, '[]', 'utf8'); + await symlink(target, path.join(runtimeDir, 'error-analyses.json')); + await expect(repository.read()).rejects.toThrow(/symbolic link/i); + + await rm(path.join(runtimeDir, 'error-analyses.json')); + await writeFile(path.join(runtimeDir, 'error-analyses.json'), '[]', '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(path.join(runtimeDir, 'error-analyses.json')); + await mkdir(path.join(runtimeDir, 'error-analyses.json')); + 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-runtime'); + const linkedDirectory = path.join(root, 'linked-runtime'); + await mkdir(realDirectory); + await symlink(realDirectory, linkedDirectory, 'dir'); + const linkedRepository = new FileErrorAnalysisRepository(linkedDirectory, []); + await expect(linkedRepository.update(() => [analysis('unsafe')])).rejects.toThrow( + /regular directory/i + ); + + await expect( + repository.update(() => [{ ...analysis('large'), summary: 'x'.repeat(16 * 1024 * 1024) }]) + ).rejects.toThrow(/16 MiB/i); + }); +}); + +describe('ErrorLearningService storage integration', () => { + it('creates, detects repeats, updates, lists, searches, and summarizes analyses', async () => { + const repository = new InMemoryErrorAnalysisRepository(); + await repository.update(() => []); + const service = new ErrorLearningService(repository); + const first = await service.submitError({ errorMessage: 'Build failed with timeout' }); + const second = await service.submitError({ errorMessage: 'Build failed with timeout again' }); + expect(second.isRepeat).toBe(true); + + const updated = await service.updateAnalysis(first.id, { + rootCause: 'Dependency unavailable', + preventionSteps: ['Retry with backoff'], + }); + expect(updated?.rootCause).toBe('Dependency unavailable'); + await expect(service.getAnalysis(first.id)).resolves.toMatchObject({ + rootCause: 'Dependency unavailable', + }); + await expect(service.listAnalyses({ severity: 'medium' })).resolves.toHaveLength(2); + await expect(service.searchSimilar('dependency unavailable')).resolves.toHaveLength(1); + await expect(service.getStats()).resolves.toMatchObject({ + totalAnalyses: 2, + bySeverity: { medium: 2 }, + repeatRate: 0.5, + }); + }); +}); diff --git a/server/src/services/error-learning-service.ts b/server/src/services/error-learning-service.ts index 258bc7ab..ddb1f4e1 100644 --- a/server/src/services/error-learning-service.ts +++ b/server/src/services/error-learning-service.ts @@ -14,13 +14,10 @@ import { getTaskService } from './task-service.js'; import { createLogger } from '../lib/logger.js'; -import * as fs from 'node:fs/promises'; -import * as path from 'node:path'; -import { getLegacyRuntimeDirs, getRuntimeDir } from '../utils/paths.js'; -import { migrateLegacyFiles } from '../utils/migrate-legacy-files.js'; -const DATA_DIR = getRuntimeDir(); -const LEGACY_DATA_DIRS = getLegacyRuntimeDirs(); -let migrationChecked = false; +import { + FileErrorAnalysisRepository, + type ErrorAnalysisRepository, +} from '../storage/error-analysis-repository.js'; const log = createLogger('error-learning'); @@ -102,38 +99,10 @@ export interface ErrorLearningStats { // ─── Service ───────────────────────────────────────────────────── -class ErrorLearningService { - private analyses: ErrorAnalysis[] = []; - private loaded = false; - - private get storagePath(): string { - return path.join(DATA_DIR, 'error-analyses.json'); - } - - private async ensureLoaded(): Promise { - if (!migrationChecked) { - migrationChecked = true; - await migrateLegacyFiles( - LEGACY_DATA_DIRS, - DATA_DIR, - ['error-analyses.json'], - 'error analysis' - ); - } - - if (this.loaded) return; - try { - const data = await fs.readFile(this.storagePath, 'utf-8'); - this.analyses = JSON.parse(data); - } catch { - this.analyses = []; - } - this.loaded = true; - } - - private async save(): Promise { - await fs.writeFile(this.storagePath, JSON.stringify(this.analyses, null, 2)); - } +export class ErrorLearningService { + constructor( + private readonly repository: ErrorAnalysisRepository = new FileErrorAnalysisRepository() + ) {} /** * Submit an error for analysis. Returns a structured analysis. @@ -143,33 +112,29 @@ class ErrorLearningService { * or that can be completed via the API. */ async submitError(context: ErrorContext): Promise { - await this.ensureLoaded(); - - // Check for previous similar errors - const previousOccurrences = this.findSimilarErrors(context); - - const analysis: ErrorAnalysis = { - id: `err_${Date.now()}_${Math.random().toString(36).slice(2, 6)}`, - context: { - ...context, - occurredAt: context.occurredAt || new Date().toISOString(), - }, - rootCause: '', // To be filled by analyzing agent - summary: `Error in ${context.taskId || 'unknown task'}: ${context.errorMessage.slice(0, 200)}`, - severity: this.estimateSeverity(context), - optionsConsidered: [], - chosenFix: '', - preventionSteps: [], - tags: this.autoTag(context), - relatedTasks: context.taskId ? [context.taskId] : [], - isRepeat: previousOccurrences.length > 0, - previousOccurrences: previousOccurrences.map((a) => a.id), - analyzedAt: new Date().toISOString(), - analyzedBy: context.agent, - }; - - this.analyses.push(analysis); - await this.save(); + const analysis = await this.repository.mutate((analyses) => { + const previousOccurrences = this.findSimilarErrors(analyses, context); + const created: ErrorAnalysis = { + id: `err_${Date.now()}_${Math.random().toString(36).slice(2, 6)}`, + context: { + ...context, + occurredAt: context.occurredAt || new Date().toISOString(), + }, + rootCause: '', + summary: `Error in ${context.taskId || 'unknown task'}: ${context.errorMessage.slice(0, 200)}`, + severity: this.estimateSeverity(context), + optionsConsidered: [], + chosenFix: '', + preventionSteps: [], + tags: this.autoTag(context), + relatedTasks: context.taskId ? [context.taskId] : [], + isRepeat: previousOccurrences.length > 0, + previousOccurrences: previousOccurrences.map((candidate) => candidate.id), + analyzedAt: new Date().toISOString(), + analyzedBy: context.agent, + }; + return { analyses: [...analyses, created], result: created }; + }); // If linked to a task, update the task's lessonsLearned if (context.taskId) { @@ -204,14 +169,22 @@ class ErrorLearningService { > > ): Promise { - await this.ensureLoaded(); - - const analysis = this.analyses.find((a) => a.id === id); + const analysis = await this.repository.mutate((analyses) => { + const index = analyses.findIndex((candidate) => candidate.id === id); + if (index === -1) { + return { analyses, result: null as ErrorAnalysis | null }; + } + const updated: ErrorAnalysis = { + ...analyses[index], + ...update, + analyzedAt: new Date().toISOString(), + }; + const next = [...analyses]; + next[index] = updated; + return { analyses: next, result: updated as ErrorAnalysis | null }; + }); if (!analysis) return null; - Object.assign(analysis, update, { analyzedAt: new Date().toISOString() }); - await this.save(); - // Update linked task if (analysis.relatedTasks.length > 0) { await this.linkToTask(analysis.relatedTasks[0], analysis); @@ -225,8 +198,8 @@ class ErrorLearningService { * Get a specific analysis. */ async getAnalysis(id: string): Promise { - await this.ensureLoaded(); - return this.analyses.find((a) => a.id === id) || null; + const analyses = await this.repository.read(); + return analyses.find((analysis) => analysis.id === id) || null; } /** @@ -239,12 +212,11 @@ class ErrorLearningService { agent?: string; limit?: number; }): Promise { - await this.ensureLoaded(); - - let results = [...this.analyses]; + let results = [...(await this.repository.read())]; if (filters?.taskId) { - results = results.filter((a) => a.relatedTasks.includes(filters.taskId!)); + const taskId = filters.taskId; + results = results.filter((analysis) => analysis.relatedTasks.includes(taskId)); } if (filters?.errorType) { results = results.filter((a) => a.context.errorType === filters.errorType); @@ -272,13 +244,13 @@ class ErrorLearningService { * Get aggregate statistics about error patterns. */ async getStats(): Promise { - await this.ensureLoaded(); + const analyses = await this.repository.read(); const byType: Record = {}; const bySeverity: Record = {}; const preventionCounts: Record = {}; - for (const analysis of this.analyses) { + for (const analysis of analyses) { // By type const type = analysis.context.errorType || 'unknown'; byType[type] = (byType[type] || 0) + 1; @@ -292,15 +264,15 @@ class ErrorLearningService { } } - const repeats = this.analyses.filter((a) => a.isRepeat).length; - const repeatRate = this.analyses.length > 0 ? repeats / this.analyses.length : 0; + const repeats = analyses.filter((analysis) => analysis.isRepeat).length; + const repeatRate = analyses.length > 0 ? repeats / analyses.length : 0; const topPreventionSteps = Object.entries(preventionCounts) .map(([step, count]) => ({ step, count })) .sort((a, b) => b.count - a.count) .slice(0, 10); - const recentAnalyses = this.analyses + const recentAnalyses = analyses .slice(-5) .reverse() .map((a) => ({ @@ -311,7 +283,7 @@ class ErrorLearningService { })); return { - totalAnalyses: this.analyses.length, + totalAnalyses: analyses.length, byType, bySeverity, repeatRate: Math.round(repeatRate * 100) / 100, @@ -325,7 +297,7 @@ class ErrorLearningService { * Useful for agents to check "have we seen this before?" */ async searchSimilar(errorMessage: string, limit = 5): Promise { - await this.ensureLoaded(); + const analyses = await this.repository.read(); // Simple keyword matching — could be replaced with embeddings const keywords = errorMessage @@ -333,7 +305,7 @@ class ErrorLearningService { .split(/\s+/) .filter((w) => w.length > 3); - const scored = this.analyses.map((analysis) => { + const scored = analyses.map((analysis) => { const text = `${analysis.context.errorMessage} ${analysis.rootCause} ${analysis.summary}`.toLowerCase(); const matches = keywords.filter((kw) => text.includes(kw)).length; @@ -349,9 +321,9 @@ class ErrorLearningService { // ─── Private Helpers ───────────────────────────────────────── - private findSimilarErrors(context: ErrorContext): ErrorAnalysis[] { + private findSimilarErrors(analyses: ErrorAnalysis[], context: ErrorContext): ErrorAnalysis[] { const msg = context.errorMessage.toLowerCase(); - return this.analyses.filter((a) => { + return analyses.filter((a) => { const existingMsg = a.context.errorMessage.toLowerCase(); // Check for significant overlap const words = msg.split(/\s+/).filter((w) => w.length > 3); diff --git a/server/src/storage/error-analysis-repository.ts b/server/src/storage/error-analysis-repository.ts new file mode 100644 index 00000000..ce875c9a --- /dev/null +++ b/server/src/storage/error-analysis-repository.ts @@ -0,0 +1,135 @@ +import { constants } from 'node:fs'; +import { lstat, mkdir, open } from 'node:fs/promises'; +import path from 'node:path'; +import type { ErrorAnalysis } from '../services/error-learning-service.js'; +import { withFileLock } from '../services/file-lock.js'; +import { migrateLegacyFiles } from '../utils/migrate-legacy-files.js'; +import { getLegacyRuntimeDirs, getRuntimeDir } from '../utils/paths.js'; +import { ensureWithinBase } from '../utils/sanitize.js'; +import { atomicWriteFile } from './fs-helpers.js'; + +const MAX_ERROR_ANALYSES_BYTES = 16 * 1024 * 1024; + +export interface ErrorAnalysisRepository { + read(): Promise; + update(updater: (analyses: ErrorAnalysis[]) => ErrorAnalysis[]): Promise; + mutate( + updater: (analyses: ErrorAnalysis[]) => { analyses: ErrorAnalysis[]; result: T } + ): Promise; +} + +export class FileErrorAnalysisRepository implements ErrorAnalysisRepository { + private readonly runtimeDir: string; + private readonly analysisFile: string; + private migrationChecked = false; + + constructor( + runtimeDir = getRuntimeDir(), + private readonly legacyRuntimeDirs: readonly string[] = getLegacyRuntimeDirs() + ) { + this.runtimeDir = path.resolve(runtimeDir); + this.analysisFile = ensureWithinBase( + this.runtimeDir, + path.join(this.runtimeDir, 'error-analyses.json') + ); + } + + async read(): Promise { + await this.ensureMigrated(); + return this.readFile(); + } + + async update(updater: (analyses: ErrorAnalysis[]) => ErrorAnalysis[]): Promise { + return this.mutate((analyses) => { + const updated = updater(analyses); + return { analyses: updated, result: updated }; + }); + } + + async mutate( + updater: (analyses: ErrorAnalysis[]) => { analyses: ErrorAnalysis[]; result: T } + ): Promise { + await this.ensureMigrated(); + await this.prepareDirectory(); + return withFileLock(this.analysisFile, async () => { + const { analyses, result } = updater(await this.readFile()); + const content = JSON.stringify(analyses, null, 2); + if (Buffer.byteLength(content, 'utf8') > MAX_ERROR_ANALYSES_BYTES) { + throw new Error('Error analyses exceed the 16 MiB storage limit'); + } + await atomicWriteFile(this.analysisFile, content, 'utf8'); + return result; + }); + } + + private async ensureMigrated(): Promise { + if (this.migrationChecked) return; + this.migrationChecked = true; + await migrateLegacyFiles( + this.legacyRuntimeDirs, + this.runtimeDir, + ['error-analyses.json'], + 'error analysis' + ); + } + + private async prepareDirectory(): Promise { + await mkdir(this.runtimeDir, { recursive: true, mode: 0o700 }); + const stats = await lstat(this.runtimeDir); + if (!stats.isDirectory() || stats.isSymbolicLink()) { + throw new Error('Error analysis storage path must use a regular directory'); + } + } + + private async readFile(): Promise { + let handle: Awaited> | undefined; + try { + handle = await open(this.analysisFile, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + const [pathStats, stats] = await Promise.all([lstat(this.analysisFile), handle.stat()]); + if ( + pathStats.isSymbolicLink() || + pathStats.dev !== stats.dev || + pathStats.ino !== stats.ino + ) { + throw new Error('Error analyses must not use a symbolic link or changed file'); + } + if (!stats.isFile() || stats.size > MAX_ERROR_ANALYSES_BYTES) { + throw new Error('Error analyses must use a bounded regular file'); + } + const parsed: unknown = JSON.parse(await handle.readFile({ encoding: 'utf8' })); + return Array.isArray(parsed) ? (parsed as ErrorAnalysis[]) : []; + } catch (error) { + const errorCode = (error as NodeJS.ErrnoException).code; + if (errorCode === 'ENOENT' || error instanceof SyntaxError) return []; + if (errorCode === 'ELOOP') { + throw new Error('Error analyses must not use a symbolic link', { cause: error }); + } + throw error; + } finally { + await handle?.close(); + } + } +} + +export class InMemoryErrorAnalysisRepository implements ErrorAnalysisRepository { + private analyses: ErrorAnalysis[] = []; + + async read(): Promise { + return this.analyses; + } + + async update(updater: (analyses: ErrorAnalysis[]) => ErrorAnalysis[]): Promise { + return this.mutate((analyses) => { + const updated = updater(analyses); + return { analyses: updated, result: updated }; + }); + } + + async mutate( + updater: (analyses: ErrorAnalysis[]) => { analyses: ErrorAnalysis[]; result: T } + ): Promise { + const mutation = updater(this.analyses); + this.analyses = mutation.analyses; + return mutation.result; + } +} diff --git a/server/src/storage/index.ts b/server/src/storage/index.ts index 2854412e..f06bd450 100644 --- a/server/src/storage/index.ts +++ b/server/src/storage/index.ts @@ -50,6 +50,11 @@ export { type CeremonyStateRepository, } from './ceremony-state-repository.js'; export { FileDelegationRepository, type DelegationRepository } from './delegation-repository.js'; +export { + FileErrorAnalysisRepository, + InMemoryErrorAnalysisRepository, + type ErrorAnalysisRepository, +} from './error-analysis-repository.js'; export { LocalConflictWorkspaceRepository, type ConflictWorkspaceRepository,