From 99c774c7fa7968b27b619d1e7666f50216ade637 Mon Sep 17 00:00:00 2001 From: ci-belphegor <324126930+ci-belphegor@users.noreply.github.com> Date: Fri, 11 Sep 2026 07:33:18 -0400 Subject: [PATCH] refactor(kernel-platforms): fs promises-only Remove `existsSync` from the fs capability, its config field, and the synchronous operation wrapper it was the only user of. Narrowed methods always forward through `E()` and every derived guard is `M.callWhen`, so a synchronous method cannot survive narrowing. Co-Authored-By: Claude Opus 5 --- .../src/capabilities/fs/browser.test.ts | 10 --- .../src/capabilities/fs/browser.ts | 1 - .../src/capabilities/fs/nodejs.test.ts | 47 +----------- .../src/capabilities/fs/nodejs.ts | 3 +- .../src/capabilities/fs/shared.test.ts | 73 +------------------ .../src/capabilities/fs/shared.ts | 41 +---------- .../src/capabilities/fs/types.test.ts | 15 ---- .../src/capabilities/fs/types.ts | 5 +- packages/ocap-kernel/src/types.test.ts | 2 +- 9 files changed, 8 insertions(+), 189 deletions(-) diff --git a/packages/kernel-platforms/src/capabilities/fs/browser.test.ts b/packages/kernel-platforms/src/capabilities/fs/browser.test.ts index d536206a17..d66d768e54 100644 --- a/packages/kernel-platforms/src/capabilities/fs/browser.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/browser.test.ts @@ -5,14 +5,6 @@ import type { FsConfig } from './types.ts'; describe('fs browser capability', () => { describe('capabilityFactory', () => { - it('existsSync returns false', () => { - const config: FsConfig = { rootDir: '/root', existsSync: true }; - const capability = capabilityFactory(config); - - // eslint-disable-next-line n/no-sync - expect(capability.existsSync?.('/path')).toBe(false); - }); - it.each([ { name: 'promises.readFile', @@ -26,7 +18,6 @@ describe('fs browser capability', () => { name: 'all operations', config: { rootDir: '/root', - existsSync: true, promises: { readFile: true, access: true, @@ -43,7 +34,6 @@ describe('fs browser capability', () => { const config: FsConfig = { rootDir: '/root' }; const capability = capabilityFactory(config); - expect(capability).not.toHaveProperty('existsSync'); expect(capability).not.toHaveProperty('promises'); }); }); diff --git a/packages/kernel-platforms/src/capabilities/fs/browser.ts b/packages/kernel-platforms/src/capabilities/fs/browser.ts index b7e4c169de..a3c453b4dd 100644 --- a/packages/kernel-platforms/src/capabilities/fs/browser.ts +++ b/packages/kernel-platforms/src/capabilities/fs/browser.ts @@ -5,7 +5,6 @@ const notImplemented = (name: string): never => { }; export const { configStruct, capabilityFactory } = makeFsSpecification({ - makeExistsSync: () => () => false, promises: { makeReadFile: () => notImplemented('readFile'), makeAccess: () => notImplemented('access'), diff --git a/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts b/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts index 87efa1a386..011b78c3ad 100644 --- a/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/nodejs.test.ts @@ -1,4 +1,4 @@ -import { existsSync, lstatSync, Stats } from 'node:fs'; +import { lstatSync, Stats } from 'node:fs'; import fs from 'node:fs/promises'; import { relative } from 'node:path'; import { describe, expect, it, vi, beforeEach } from 'vitest'; @@ -20,7 +20,6 @@ vi.mock('node:fs/promises', () => ({ // Mock fs vi.mock('node:fs', () => ({ - existsSync: vi.fn(), lstatSync: vi.fn(), })); @@ -140,50 +139,6 @@ describe('fs nodejs capability', () => { }); describe('capabilityFactory', () => { - describe('existsSync operation', () => { - it('returns true for existing file', () => { - vi.mocked(existsSync).mockReturnValue(true); - - const config: FsConfig = { rootDir: '/root', existsSync: true }; - const capability = capabilityFactory(config); - - const result = capability.existsSync?.('/root/file.txt'); - expect(existsSync).toHaveBeenCalledWith('/root/file.txt'); - expect(result).toBe(true); - }); - - it.each([ - { - name: 'outside root', - relativeReturn: '../../outside/file.txt', - isSymlink: false, - testPath: '/outside/file.txt', - expectedError: 'Path /outside/file.txt is outside allowed root /root', - }, - { - name: 'symlink', - relativeReturn: '/root/file.txt', - isSymlink: true, - testPath: '/root/file.txt', - expectedError: 'Symlinks are prohibited: /root/file.txt', - }, - ])( - 'throws error for path $name', - ({ relativeReturn, isSymlink, testPath, expectedError }) => { - createMockRelative(relativeReturn); - createMockLstatSync(isSymlink); - - const config: FsConfig = { rootDir: '/root', existsSync: true }; - const capability = capabilityFactory(config); - - expect(() => capability.existsSync?.(testPath)).toThrow( - expectedError, - ); - expect(existsSync).not.toHaveBeenCalled(); - }, - ); - }); - describe.each([ { operation: 'readFile', diff --git a/packages/kernel-platforms/src/capabilities/fs/nodejs.ts b/packages/kernel-platforms/src/capabilities/fs/nodejs.ts index 1815968ccb..f04f32cb84 100644 --- a/packages/kernel-platforms/src/capabilities/fs/nodejs.ts +++ b/packages/kernel-platforms/src/capabilities/fs/nodejs.ts @@ -1,4 +1,4 @@ -import { existsSync, lstatSync } from 'node:fs'; +import { lstatSync } from 'node:fs'; import fs from 'node:fs/promises'; import { relative } from 'node:path'; @@ -54,7 +54,6 @@ const makeNodejsPathCaveat = (rootDir: string): SyncPathCaveat => { }; export const { configStruct, capabilityFactory } = makeFsSpecification({ - makeExistsSync: () => existsSync, promises: { makeReadFile: () => fs.readFile, makeAccess: () => fs.access, diff --git a/packages/kernel-platforms/src/capabilities/fs/shared.test.ts b/packages/kernel-platforms/src/capabilities/fs/shared.test.ts index de4bc0f42a..b96833ee83 100644 --- a/packages/kernel-platforms/src/capabilities/fs/shared.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/shared.test.ts @@ -1,11 +1,7 @@ import { describe, expect, it, vi } from 'vitest'; -import { - makeCaveatedFsOperation, - makeCaveatedSyncFsOperation, - makeFsSpecification, -} from './shared.ts'; -import type { ReadFile, Access, ExistsSync, SyncPathCaveat } from './types.ts'; +import { makeCaveatedFsOperation, makeFsSpecification } from './shared.ts'; +import type { ReadFile, Access, SyncPathCaveat } from './types.ts'; describe('makeCaveatedFsOperation', () => { it('applies caveat before operation', async () => { @@ -57,64 +53,14 @@ describe('makeCaveatedFsOperation', () => { }); }); -describe('makeCaveatedSyncFsOperation', () => { - it('applies caveat before operation', () => { - const mockOperation = vi.fn().mockReturnValue('result'); - const mockCaveat = vi.fn().mockReturnValue(undefined); - - const caveatedOperation = makeCaveatedSyncFsOperation( - mockOperation, - mockCaveat, - ); - - const result = caveatedOperation('/path', 'arg2', 'arg3'); - - expect(mockCaveat).toHaveBeenCalledWith('/path'); - expect(mockOperation).toHaveBeenCalledWith('/path', 'arg2', 'arg3'); - expect(result).toBe('result'); - }); - - it('throws on caveat rejection', () => { - const mockOperation = vi.fn(); - const mockCaveat = vi.fn().mockImplementation(() => { - throw new Error('Path not allowed'); - }); - - const caveatedOperation = makeCaveatedSyncFsOperation( - mockOperation, - mockCaveat, - ); - - expect(() => caveatedOperation('/path')).toThrow('Path not allowed'); - expect(mockCaveat).toHaveBeenCalledWith('/path'); - expect(mockOperation).not.toHaveBeenCalled(); - }); - - it('handles void operations', () => { - const mockOperation = vi.fn().mockReturnValue(undefined); - const mockCaveat = vi.fn().mockReturnValue(undefined); - - const caveatedOperation = makeCaveatedSyncFsOperation( - mockOperation, - mockCaveat, - ); - - expect(caveatedOperation('/path')).toBeUndefined(); - expect(mockCaveat).toHaveBeenCalledWith('/path'); - expect(mockOperation).toHaveBeenCalledWith('/path'); - }); -}); - describe('makeFsSpecification', () => { const createMockSpecification = () => { const mockReadFile: ReadFile = vi.fn(); const mockAccess: Access = vi.fn(); - const mockExistsSync: ExistsSync = vi.fn(); const mockPathCaveat: SyncPathCaveat = vi.fn(); return { specification: makeFsSpecification({ - makeExistsSync: () => mockExistsSync, promises: { makeReadFile: () => mockReadFile, makeAccess: () => mockAccess, @@ -123,7 +69,6 @@ describe('makeFsSpecification', () => { }), mockReadFile, mockAccess, - mockExistsSync, mockPathCaveat, }; }; @@ -135,21 +80,11 @@ describe('makeFsSpecification', () => { expect(specification).toHaveProperty('capabilityFactory'); }); - it('creates capability with existsSync', () => { - const { specification } = createMockSpecification(); - const config = { rootDir: '/root', existsSync: true }; - const capability = specification.capabilityFactory(config); - - expect(capability).toHaveProperty('existsSync'); - expect(capability).not.toHaveProperty('promises'); - }); - it('creates capability with promises.readFile', () => { const { specification } = createMockSpecification(); const config = { rootDir: '/root', promises: { readFile: true } }; const capability = specification.capabilityFactory(config); - expect(capability).not.toHaveProperty('existsSync'); expect(capability).toHaveProperty('promises'); expect(capability.promises).toHaveProperty('readFile'); expect(capability.promises).not.toHaveProperty('access'); @@ -160,7 +95,6 @@ describe('makeFsSpecification', () => { const config = { rootDir: '/root', promises: { access: true } }; const capability = specification.capabilityFactory(config); - expect(capability).not.toHaveProperty('existsSync'); expect(capability).toHaveProperty('promises'); expect(capability.promises).not.toHaveProperty('readFile'); expect(capability.promises).toHaveProperty('access'); @@ -170,7 +104,6 @@ describe('makeFsSpecification', () => { const { specification } = createMockSpecification(); const config = { rootDir: '/root', - existsSync: true, promises: { readFile: true, access: true, @@ -178,7 +111,6 @@ describe('makeFsSpecification', () => { }; const capability = specification.capabilityFactory(config); - expect(capability).toHaveProperty('existsSync'); expect(capability).toHaveProperty('promises'); expect(capability.promises).toHaveProperty('readFile'); expect(capability.promises).toHaveProperty('access'); @@ -189,7 +121,6 @@ describe('makeFsSpecification', () => { const config = { rootDir: '/root' }; const capability = specification.capabilityFactory(config); - expect(capability).not.toHaveProperty('existsSync'); expect(capability).not.toHaveProperty('promises'); }); }); diff --git a/packages/kernel-platforms/src/capabilities/fs/shared.ts b/packages/kernel-platforms/src/capabilities/fs/shared.ts index e9b490c43d..9082251ccb 100644 --- a/packages/kernel-platforms/src/capabilities/fs/shared.ts +++ b/packages/kernel-platforms/src/capabilities/fs/shared.ts @@ -3,7 +3,6 @@ import type { SyncPathCaveat, ReadFile, Access, - ExistsSync, FsConfig, FsCapability, } from './types.ts'; @@ -11,7 +10,7 @@ import { fsConfigStruct } from './types.ts'; import { makeCapabilitySpecification } from '../../specification.ts'; /** - * Cross-platform FS operation wrapper with validation (async version) + * Cross-platform FS operation wrapper with validation * * @param operation - The underlying operation to wrap * @param syncPathCaveat - The caveat to apply to path arguments @@ -36,37 +35,11 @@ export const makeCaveatedFsOperation = < }) as Operation; }; -/** - * Cross-platform synchronous FS operation wrapper with validation - * - * @param operation - The underlying synchronous operation to wrap - * @param syncPathCaveat - The caveat to apply to path arguments - * @returns The operation restricted by the provided caveat - */ -export const makeCaveatedSyncFsOperation = < - Operation extends (...args: never[]) => unknown, ->( - operation: Operation, - syncPathCaveat: SyncPathCaveat, -): Operation => { - return harden((...args: Parameters) => { - try { - // Assuming first argument is always the path - syncPathCaveat(args[0] as unknown as PathLike); - } catch (cause) { - const message = cause instanceof Error ? cause.message : 'Caveat failed'; - throw new Error(`fs.${operation.name}: ${message}`, { cause }); - } - return operation(...args); - }) as Operation; -}; - /* eslint-disable @typescript-eslint/explicit-function-return-type */ /** * Cross-platform FS capability specification factory * * @param config - The configuration for the capability specification - * @param config.makeExistsSync - The factory returning an existsSync operation * @param config.promises - Object containing promise-based operation factories * @param config.promises.makeReadFile - The factory returning a read file operation * @param config.promises.makeAccess - The factory returning an access operation @@ -74,11 +47,9 @@ export const makeCaveatedSyncFsOperation = < * @returns The capability specification */ export const makeFsSpecification = ({ - makeExistsSync, promises, makePathCaveat, }: { - makeExistsSync: () => ExistsSync; promises: { makeReadFile: () => ReadFile; makeAccess: () => Access; @@ -90,19 +61,11 @@ export const makeFsSpecification = ({ (config: FsConfig): FsCapability => { // The construction of this capability left ad-hoc until additional // requirements dictate additional structure. - const { rootDir, existsSync, promises: promisesConfig } = config; + const { rootDir, promises: promisesConfig } = config; const caveat = makePathCaveat(rootDir); const toExport: FsCapability = {}; - if (existsSync) { - toExport.existsSync = makeCaveatedSyncFsOperation( - // eslint-disable-next-line n/no-sync - makeExistsSync(), - caveat, - ); - } - if (promisesConfig) { const promisesObj: FsCapability['promises'] = {}; diff --git a/packages/kernel-platforms/src/capabilities/fs/types.test.ts b/packages/kernel-platforms/src/capabilities/fs/types.test.ts index 762f1106d8..c0deec222d 100644 --- a/packages/kernel-platforms/src/capabilities/fs/types.test.ts +++ b/packages/kernel-platforms/src/capabilities/fs/types.test.ts @@ -8,10 +8,6 @@ describe('fs types', () => { describe('fsConfigStruct', () => { it.each([ { name: 'minimal config with rootDir', config: { rootDir: '/root' } }, - { - name: 'config with rootDir and existsSync enabled', - config: { rootDir: '/root', existsSync: true }, - }, { name: 'config with rootDir and promises.readFile enabled', config: { rootDir: '/root', promises: { readFile: true } }, @@ -24,7 +20,6 @@ describe('fs types', () => { name: 'config with all operations enabled', config: { rootDir: '/root', - existsSync: true, promises: { readFile: true, access: true, @@ -35,7 +30,6 @@ describe('fs types', () => { name: 'config with some operations disabled', config: { rootDir: '/root', - existsSync: false, promises: { readFile: true, access: false, @@ -50,10 +44,6 @@ describe('fs types', () => { it.each([ { name: 'config without rootDir', config: {} }, { name: 'config with non-string rootDir', config: { rootDir: 123 } }, - { - name: 'config with non-boolean existsSync', - config: { rootDir: '/root', existsSync: 'true' }, - }, { name: 'config with non-boolean promises.readFile', config: { rootDir: '/root', promises: { readFile: 'true' } }, @@ -77,15 +67,12 @@ describe('fs types', () => { const validated = fsConfigStruct.create(config); expect(validated.rootDir).toBe('/root'); - // eslint-disable-next-line n/no-sync - expect(validated.existsSync).toBeUndefined(); expect(validated.promises).toBeUndefined(); }); it('preserves boolean values', () => { const config: FsConfig = { rootDir: '/root', - existsSync: true, promises: { readFile: false, access: true, @@ -94,8 +81,6 @@ describe('fs types', () => { const validated = fsConfigStruct.create(config); expect(validated.rootDir).toBe('/root'); - // eslint-disable-next-line n/no-sync - expect(validated.existsSync).toBe(true); expect(validated.promises?.readFile).toBe(false); expect(validated.promises?.access).toBe(true); }); diff --git a/packages/kernel-platforms/src/capabilities/fs/types.ts b/packages/kernel-platforms/src/capabilities/fs/types.ts index 60aeafd162..8a4d2d2842 100644 --- a/packages/kernel-platforms/src/capabilities/fs/types.ts +++ b/packages/kernel-platforms/src/capabilities/fs/types.ts @@ -1,6 +1,6 @@ import { exactOptional, object, string, boolean } from '@metamask/superstruct'; import type { Infer } from '@metamask/superstruct'; -import type { PathLike, existsSync } from 'node:fs'; +import type { PathLike } from 'node:fs'; import type { readFile, access } from 'node:fs/promises'; export type { PathLike }; @@ -12,11 +12,9 @@ export type SyncPathCaveat = (path: PathLike) => void; export type ReadFile = typeof readFile; export type Access = typeof access; -export type ExistsSync = typeof existsSync; export const fsConfigStruct = object({ rootDir: string(), - existsSync: exactOptional(boolean()), promises: exactOptional( object({ readFile: exactOptional(boolean()), @@ -26,7 +24,6 @@ export const fsConfigStruct = object({ }); export type FsCapability = Partial<{ - existsSync: ExistsSync; promises: Partial<{ readFile: ReadFile; access: Access; diff --git a/packages/ocap-kernel/src/types.test.ts b/packages/ocap-kernel/src/types.test.ts index 9590ea580e..cb2ddc05bb 100644 --- a/packages/ocap-kernel/src/types.test.ts +++ b/packages/ocap-kernel/src/types.test.ts @@ -135,7 +135,7 @@ describe('isVatConfig', () => { creationOptions: { foo: 'bar' }, parameters: { baz: 123 }, platformConfig: { - fs: { rootDir: '/tmp', existsSync: true }, + fs: { rootDir: '/tmp', promises: { readFile: true } }, }, }, expected: true,