From 1c1130e7cfd3353a196aa98a275f10ec16aa4dc9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Tue, 18 Aug 2026 19:07:34 +0200 Subject: [PATCH 1/2] fix(utils): throw AppError from app-log-files and verified-file guards The symlink, not-a-regular-file, identity-changed and identity-race guards threw plain Error, so they surfaced as UNKNOWN with a misleading hint and tests could only assert them by message. They are now COMMAND_FAILED with a recovery hint (ADR 0010); the affected tests assert through assertThrowsAppError, and the two race guards gain planted-interleaving coverage. Closes #1792 --- src/daemon/__tests__/app-log.test.ts | 14 ++++-- src/utils/__tests__/app-log-files.test.ts | 8 +-- src/utils/__tests__/verified-file.test.ts | 61 +++++++++++++++++++---- src/utils/app-log-files.ts | 17 +++---- src/utils/verified-file.ts | 39 ++++++++++++--- 5 files changed, 106 insertions(+), 33 deletions(-) diff --git a/src/daemon/__tests__/app-log.test.ts b/src/daemon/__tests__/app-log.test.ts index 9d4623cc9..1ae6c5005 100644 --- a/src/daemon/__tests__/app-log.test.ts +++ b/src/daemon/__tests__/app-log.test.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { expect, test } from 'vitest'; +import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { appendAppLogMarker, clearAppLogFiles, getAppLogPathMetadata } from '../app-log.ts'; @@ -37,11 +38,14 @@ test.each(['metadata', 'mark', 'clear'] as const)( // whose identity check reports the path as simply not a regular file. const expectedMessage = operation === 'mark' ? /must not be a symbolic link/ : /must be a regular file/; - expect(() => { - if (operation === 'metadata') getAppLogPathMetadata(outPath); - else if (operation === 'mark') appendAppLogMarker(outPath, 'checkpoint'); - else clearAppLogFiles(outPath); - }).toThrow(expectedMessage); + assertThrowsAppError( + () => { + if (operation === 'metadata') getAppLogPathMetadata(outPath); + else if (operation === 'mark') appendAppLogMarker(outPath, 'checkpoint'); + else clearAppLogFiles(outPath); + }, + { code: 'COMMAND_FAILED', message: expectedMessage }, + ); expect(fs.readFileSync(outsidePath, 'utf8')).toBe('outside'); expect(fs.lstatSync(outPath).isSymbolicLink()).toBe(true); }, diff --git a/src/utils/__tests__/app-log-files.test.ts b/src/utils/__tests__/app-log-files.test.ts index 26910b7b3..5212c2faa 100644 --- a/src/utils/__tests__/app-log-files.test.ts +++ b/src/utils/__tests__/app-log-files.test.ts @@ -1,6 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { expect, test } from 'vitest'; +import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { ensureAppLogPath, rotateAppLogIfNeeded } from '../app-log-files.ts'; @@ -39,9 +40,10 @@ test('rotation rejects a final app.log symlink without touching its target', () fs.writeFileSync(outsidePath, 'outside'); fs.symlinkSync(outsidePath, outPath); - expect(() => rotateAppLogIfNeeded(outPath, { maxBytes: 1, maxRotatedFiles: 1 })).toThrow( - 'symbolic link', - ); + assertThrowsAppError(() => rotateAppLogIfNeeded(outPath, { maxBytes: 1, maxRotatedFiles: 1 }), { + code: 'COMMAND_FAILED', + message: /must not be a symbolic link/, + }); expect(fs.readFileSync(outsidePath, 'utf8')).toBe('outside'); expect(fs.lstatSync(outPath).isSymbolicLink()).toBe(true); }); diff --git a/src/utils/__tests__/verified-file.test.ts b/src/utils/__tests__/verified-file.test.ts index cb765cfd9..2bef1a235 100644 --- a/src/utils/__tests__/verified-file.test.ts +++ b/src/utils/__tests__/verified-file.test.ts @@ -1,7 +1,8 @@ import fs from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { afterEach, expect, test } from 'vitest'; +import { afterEach, expect, test, vi } from 'vitest'; +import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; import { openVerifiedFileForAppend, openVerifiedFileForRead, @@ -11,6 +12,7 @@ import { const roots: string[] = []; afterEach(() => { + vi.restoreAllMocks(); for (const root of roots.splice(0)) fs.rmSync(root, { recursive: true, force: true }); }); @@ -39,19 +41,58 @@ test.each(['read', 'append', 'truncate'] as const)( fs.writeFileSync(outside, 'outside'); fs.symlinkSync(outside, pathname); - expect(() => { - const descriptor = - operation === 'read' - ? openVerifiedFileForRead(pathname) - : operation === 'append' - ? openVerifiedFileForAppend(pathname) - : openVerifiedFileForTruncate(pathname); - if (descriptor !== undefined) fs.closeSync(descriptor); - }).toThrow('regular file'); + assertThrowsAppError( + () => { + const descriptor = + operation === 'read' + ? openVerifiedFileForRead(pathname) + : operation === 'append' + ? openVerifiedFileForAppend(pathname) + : openVerifiedFileForTruncate(pathname); + if (descriptor !== undefined) fs.closeSync(descriptor); + }, + { code: 'COMMAND_FAILED', message: /must be a regular file/ }, + ); expect(fs.readFileSync(outside, 'utf8')).toBe('outside'); }, ); +// The two race guards cannot be reached from the filesystem alone, so the interleaving is +// planted: the path is swapped between the open and its post-open lstat, or the create keeps +// losing to a concurrent creator. Both must surface as typed failures with a recovery hint. +test('reports a typed failure when the file is swapped while it is being opened', () => { + const pathname = fixturePath('swapped'); + const other = `${pathname}.other`; + fs.writeFileSync(pathname, 'first'); + fs.writeFileSync(other, 'second'); + const realLstat = fs.lstatSync; + let lstatCalls = 0; + vi.spyOn(fs, 'lstatSync').mockImplementation(((target: fs.PathLike, options?: unknown) => { + lstatCalls += 1; + // The second lstat is the post-open identity check; answer it with the other file. + const resolved = lstatCalls === 2 ? other : target; + return (realLstat as (path: fs.PathLike, options?: unknown) => fs.Stats)(resolved, options); + }) as typeof fs.lstatSync); + + assertThrowsAppError(() => openVerifiedFileForRead(pathname), { + code: 'COMMAND_FAILED', + message: /identity changed while it was opened/, + }); +}); + +test('reports a typed failure when a create keeps losing the identity race', () => { + const pathname = fixturePath('contended'); + const eexist = Object.assign(new Error('EEXIST: file already exists'), { code: 'EEXIST' }); + vi.spyOn(fs, 'openSync').mockImplementation(() => { + throw eexist; + }); + + assertThrowsAppError(() => openVerifiedFileForAppend(pathname), { + code: 'COMMAND_FAILED', + message: /could not be opened without an identity race/, + }); +}); + test('returns absent for a missing read without creating the file', () => { const pathname = fixturePath('missing'); expect(openVerifiedFileForRead(pathname)).toBeUndefined(); diff --git a/src/utils/app-log-files.ts b/src/utils/app-log-files.ts index b06f9d1bc..c6c02dc42 100644 --- a/src/utils/app-log-files.ts +++ b/src/utils/app-log-files.ts @@ -1,5 +1,7 @@ import fs from 'node:fs'; import path from 'node:path'; +import { AppError } from '@agent-device/kernel/errors'; +import { lstatIfPresent, NOT_REGULAR_FILE_HINT } from './verified-file.ts'; const DEFAULT_MAX_APP_LOG_BYTES = 5 * 1024 * 1024; const DEFAULT_MAX_ROTATED_FILES = 1; @@ -37,16 +39,13 @@ function assertAppLogFileIsNotSymbolicLink(outPath: string): void { } function lstatAppLogFile(outPath: string): fs.Stats | undefined { - try { - const stats = fs.lstatSync(outPath); - if (stats.isSymbolicLink()) { - throw new Error(`App-log file must not be a symbolic link: ${outPath}`); - } - return stats; - } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; - throw error; + const stats = lstatIfPresent(outPath); + if (stats?.isSymbolicLink()) { + throw new AppError('COMMAND_FAILED', `App-log file must not be a symbolic link: ${outPath}`, { + hint: NOT_REGULAR_FILE_HINT, + }); } + return stats; } function positiveIntEnv(raw: string | undefined, fallback: number): number { diff --git a/src/utils/verified-file.ts b/src/utils/verified-file.ts index 60e6ac38e..3d0a23907 100644 --- a/src/utils/verified-file.ts +++ b/src/utils/verified-file.ts @@ -1,4 +1,14 @@ import fs from 'node:fs'; +import { AppError } from '@agent-device/kernel/errors'; + +/** + * Both guards are the same failure mode seen from two sides — something other than a plain + * regular file sits at the final path — so the recovery is shared (ADR 0010 §3). + */ +export const NOT_REGULAR_FILE_HINT = + 'agent-device only reads and writes regular files at this path. Remove the symbolic link or special file there and retry.'; +const CONCURRENT_REPLACEMENT_HINT = + 'Another process replaced the file at this path while it was being opened. Stop the concurrent writer, then retry.'; /** Opens a regular final-path file for verified reads, or returns absent. */ export function openVerifiedFileForRead(pathname: string): number | undefined { @@ -32,7 +42,11 @@ function openVerifiedFile( if (result.status === 'retry') continue; return result.status === 'missing' ? undefined : result.descriptor; } - throw new Error(`Final file could not be opened without an identity race: ${pathname}`); + throw new AppError( + 'COMMAND_FAILED', + `Final file could not be opened without an identity race: ${pathname}`, + { hint: CONCURRENT_REPLACEMENT_HINT }, + ); } type VerifiedOpenAttempt = @@ -100,15 +114,28 @@ function assertOpenedIdentity(pathname: string, descriptor: number, before?: fs. if (before && !sameFile(before, opened)) throw identityChangedError(pathname); } -function identityChangedError(pathname: string): Error { - return new Error(`Final file identity changed while it was opened: ${pathname}`); +function identityChangedError(pathname: string): AppError { + return new AppError( + 'COMMAND_FAILED', + `Final file identity changed while it was opened: ${pathname}`, + { hint: CONCURRENT_REPLACEMENT_HINT }, + ); } function lstatRegularFile(pathname: string): fs.Stats | undefined { + const stats = lstatIfPresent(pathname); + if (stats && !stats.isFile()) { + throw new AppError('COMMAND_FAILED', `Final path must be a regular file: ${pathname}`, { + hint: NOT_REGULAR_FILE_HINT, + }); + } + return stats; +} + +/** `lstat` that treats absence as `undefined`; every other errno propagates. */ +export function lstatIfPresent(pathname: string): fs.Stats | undefined { try { - const stats = fs.lstatSync(pathname); - if (!stats.isFile()) throw new Error(`Final path must be a regular file: ${pathname}`); - return stats; + return fs.lstatSync(pathname); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') return undefined; throw error; From 8bf8695fe985ada854d1b2e64a56ed1c2a9c7e1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pierzcha=C5=82a?= Date: Wed, 19 Aug 2026 13:25:51 +0200 Subject: [PATCH 2/2] test(utils): pin the recovery hints the AppError conversion added MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tightened assertions supplied only code and message, and the helper did not look at the hint, so deleting either hint constant left every targeted test green — vacuous for the half of #1792 that ADR 0010 actually cares about. assertThrowsAppError/assertRejectsAppError now accept a hint, checked against normalizeError's view so a dropped hint surfaces as the misleading per-code default rather than passing, and each guard pins its exact text as a literal (importing the constant would compare it to itself). --- src/__tests__/test-utils/app-error.ts | 59 +++++++++++++---------- src/daemon/__tests__/app-log.test.ts | 6 ++- src/utils/__tests__/app-log-files.test.ts | 5 ++ src/utils/__tests__/verified-file.test.ts | 12 ++++- 4 files changed, 54 insertions(+), 28 deletions(-) diff --git a/src/__tests__/test-utils/app-error.ts b/src/__tests__/test-utils/app-error.ts index 8d7da8d41..bac11a467 100644 --- a/src/__tests__/test-utils/app-error.ts +++ b/src/__tests__/test-utils/app-error.ts @@ -1,43 +1,50 @@ import assert from 'node:assert/strict'; -import { AppError } from '@agent-device/kernel/errors'; +import { AppError, normalizeError } from '@agent-device/kernel/errors'; + +/** + * What an {@link AppError} assertion may pin. `hint` is checked against the + * hint a caller actually reads — `normalizeError`'s, not `details.hint` — so a + * throw site that drops its hint and silently inherits `defaultHintForCode` + * fails the assertion instead of passing on the default (ADR 0010: a hint is + * required wherever the per-code default would mislead). + */ +type ExpectedAppError = { code: string; message?: RegExp; hint?: string | RegExp }; + +function assertAppError(error: unknown, expected: ExpectedAppError): true { + assert.ok( + error instanceof AppError, + `expected AppError, got ${error?.constructor?.name ?? typeof error}: ${String(error)}`, + ); + assert.equal(error.code, expected.code); + if (expected.message) assert.match(error.message, expected.message); + if (expected.hint !== undefined) { + const { hint } = normalizeError(error); + assert.ok(typeof hint === 'string', `expected a hint on ${error.code}, got ${String(hint)}`); + if (typeof expected.hint === 'string') assert.equal(hint, expected.hint); + else assert.match(hint, expected.hint); + } + return true; +} /** * Asserts that `run` rejects with an {@link AppError} carrying `code` and, - * when given, a message matching `message`. Replaces the hand-rolled + * when given, a message matching `message` and the exact `hint` a caller + * reads. Replaces the hand-rolled * `assert.rejects(..., error instanceof AppError + code + match)` validator * repeated across platform tests. */ export async function assertRejectsAppError( run: () => Promise, - expected: { code: string; message?: RegExp }, + expected: ExpectedAppError, ): Promise { - await assert.rejects(run, (error: unknown) => { - assert.ok( - error instanceof AppError, - `expected AppError, got ${error?.constructor?.name ?? typeof error}: ${String(error)}`, - ); - assert.equal(error.code, expected.code); - if (expected.message) assert.match(error.message, expected.message); - return true; - }); + await assert.rejects(run, (error: unknown) => assertAppError(error, expected)); } /** * Synchronous sibling of {@link assertRejectsAppError}: asserts that `fn` * throws an {@link AppError} carrying `code` and, when given, a message - * matching `message`. + * matching `message` and the exact `hint`. */ -export function assertThrowsAppError( - fn: () => unknown, - expected: { code: string; message?: RegExp }, -): void { - assert.throws(fn, (error: unknown) => { - assert.ok( - error instanceof AppError, - `expected AppError, got ${error?.constructor?.name ?? typeof error}: ${String(error)}`, - ); - assert.equal(error.code, expected.code); - if (expected.message) assert.match(error.message, expected.message); - return true; - }); +export function assertThrowsAppError(fn: () => unknown, expected: ExpectedAppError): void { + assert.throws(fn, (error: unknown) => assertAppError(error, expected)); } diff --git a/src/daemon/__tests__/app-log.test.ts b/src/daemon/__tests__/app-log.test.ts index 1ae6c5005..4ae0baea5 100644 --- a/src/daemon/__tests__/app-log.test.ts +++ b/src/daemon/__tests__/app-log.test.ts @@ -5,6 +5,10 @@ import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { appendAppLogMarker, clearAppLogFiles, getAppLogPathMetadata } from '../app-log.ts'; +// Pinned as a literal on purpose — see the note in src/utils/__tests__/verified-file.test.ts. +const NOT_REGULAR_FILE_HINT = + 'agent-device only reads and writes regular files at this path. Remove the symbolic link or special file there and retry.'; + test('marker and clear operations keep app-log file ownership in the daemon', () => { const root = mkdtempForTestSync('agent-device-app-log-files-'); const outPath = path.join(root, 'session', 'app.log'); @@ -44,7 +48,7 @@ test.each(['metadata', 'mark', 'clear'] as const)( else if (operation === 'mark') appendAppLogMarker(outPath, 'checkpoint'); else clearAppLogFiles(outPath); }, - { code: 'COMMAND_FAILED', message: expectedMessage }, + { code: 'COMMAND_FAILED', message: expectedMessage, hint: NOT_REGULAR_FILE_HINT }, ); expect(fs.readFileSync(outsidePath, 'utf8')).toBe('outside'); expect(fs.lstatSync(outPath).isSymbolicLink()).toBe(true); diff --git a/src/utils/__tests__/app-log-files.test.ts b/src/utils/__tests__/app-log-files.test.ts index 5212c2faa..6128ddc61 100644 --- a/src/utils/__tests__/app-log-files.test.ts +++ b/src/utils/__tests__/app-log-files.test.ts @@ -5,6 +5,10 @@ import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; import { mkdtempForTestSync } from '../../__tests__/test-utils/tmp-dir.ts'; import { ensureAppLogPath, rotateAppLogIfNeeded } from '../app-log-files.ts'; +// Pinned as a literal on purpose — see the note in verified-file.test.ts. +const NOT_REGULAR_FILE_HINT = + 'agent-device only reads and writes regular files at this path. Remove the symbolic link or special file there and retry.'; + test('rotateAppLogIfNeeded rotates files and discards the oldest generation', () => { const root = mkdtempForTestSync('agent-device-app-log-rotate-'); const outPath = path.join(root, 'app.log'); @@ -43,6 +47,7 @@ test('rotation rejects a final app.log symlink without touching its target', () assertThrowsAppError(() => rotateAppLogIfNeeded(outPath, { maxBytes: 1, maxRotatedFiles: 1 }), { code: 'COMMAND_FAILED', message: /must not be a symbolic link/, + hint: NOT_REGULAR_FILE_HINT, }); expect(fs.readFileSync(outsidePath, 'utf8')).toBe('outside'); expect(fs.lstatSync(outPath).isSymbolicLink()).toBe(true); diff --git a/src/utils/__tests__/verified-file.test.ts b/src/utils/__tests__/verified-file.test.ts index 2bef1a235..7c8358d08 100644 --- a/src/utils/__tests__/verified-file.test.ts +++ b/src/utils/__tests__/verified-file.test.ts @@ -3,6 +3,14 @@ import os from 'node:os'; import path from 'node:path'; import { afterEach, expect, test, vi } from 'vitest'; import { assertThrowsAppError } from '../../__tests__/test-utils/index.ts'; + +// The recovery hints are pinned as literals, not imported from the module under test: an +// assertion that compares the constant to itself stays green when the constant is deleted or +// reworded, which is the whole behaviour #1792 adds (ADR 0010 — errors say how to recover). +const NOT_REGULAR_FILE_HINT = + 'agent-device only reads and writes regular files at this path. Remove the symbolic link or special file there and retry.'; +const CONCURRENT_REPLACEMENT_HINT = + 'Another process replaced the file at this path while it was being opened. Stop the concurrent writer, then retry.'; import { openVerifiedFileForAppend, openVerifiedFileForRead, @@ -51,7 +59,7 @@ test.each(['read', 'append', 'truncate'] as const)( : openVerifiedFileForTruncate(pathname); if (descriptor !== undefined) fs.closeSync(descriptor); }, - { code: 'COMMAND_FAILED', message: /must be a regular file/ }, + { code: 'COMMAND_FAILED', message: /must be a regular file/, hint: NOT_REGULAR_FILE_HINT }, ); expect(fs.readFileSync(outside, 'utf8')).toBe('outside'); }, @@ -77,6 +85,7 @@ test('reports a typed failure when the file is swapped while it is being opened' assertThrowsAppError(() => openVerifiedFileForRead(pathname), { code: 'COMMAND_FAILED', message: /identity changed while it was opened/, + hint: CONCURRENT_REPLACEMENT_HINT, }); }); @@ -90,6 +99,7 @@ test('reports a typed failure when a create keeps losing the identity race', () assertThrowsAppError(() => openVerifiedFileForAppend(pathname), { code: 'COMMAND_FAILED', message: /could not be opened without an identity race/, + hint: CONCURRENT_REPLACEMENT_HINT, }); });