From 38e7d2302c8718e95c0ad01dde2e172582be5e9d Mon Sep 17 00:00:00 2001 From: Kit Langton Date: Thu, 6 Aug 2026 18:12:44 -0400 Subject: [PATCH] refactor(core): trim hosted read round trips and share graph utilities --- packages/core/src/file-mutation.ts | 9 +- packages/core/src/filesystem.ts | 6 +- packages/core/src/tool/plugin/read.ts | 7 +- packages/core/src/tool/read-filesystem.ts | 107 ++++++++++-------- packages/core/src/workspace/environment.ts | 7 ++ .../core/test/workspace-graph-guard.test.ts | 40 +++---- packages/server/src/handlers/session.ts | 12 +- packages/util/src/effect/layer-node.ts | 6 +- 8 files changed, 102 insertions(+), 92 deletions(-) diff --git a/packages/core/src/file-mutation.ts b/packages/core/src/file-mutation.ts index 381b8d26d8..05b5ea370e 100644 --- a/packages/core/src/file-mutation.ts +++ b/packages/core/src/file-mutation.ts @@ -218,20 +218,19 @@ const hostedLayer = Layer.effect( Service, Effect.gen(function* () { const env = yield* WorkspaceEnvironment.Service - const mapFileSystemError = (method: string) => (error: WorkspaceEnvironment.Error) => - new FSUtil.FileSystemError({ method, cause: error }) return make({ readOptional: (path) => env.files.read(path).pipe( Effect.catchTag("WorkspaceEnvironment.NotFoundError", () => Effect.succeed(undefined)), - Effect.mapError(mapFileSystemError("read")), + Effect.mapError(WorkspaceEnvironment.toFileSystemError("read")), ), isDirectory: (path) => env.files.stat(path).pipe( Effect.map((info) => info.type === "Directory"), Effect.catch(() => Effect.succeed(false)), ), - write: (path, content) => env.files.write(path, content).pipe(Effect.mapError(mapFileSystemError("write"))), + write: (path, content) => + env.files.write(path, content).pipe(Effect.mapError(WorkspaceEnvironment.toFileSystemError("write"))), remove: (path) => env.files .remove(path) @@ -239,7 +238,7 @@ const hostedLayer = Layer.effect( Effect.mapError((error) => error._tag === "WorkspaceEnvironment.NotFoundError" ? new NotFoundError({ path }) - : mapFileSystemError("remove")(error), + : WorkspaceEnvironment.toFileSystemError("remove")(error), ), ), }) diff --git a/packages/core/src/filesystem.ts b/packages/core/src/filesystem.ts index a483ccdbd5..8004799602 100644 --- a/packages/core/src/filesystem.ts +++ b/packages/core/src/filesystem.ts @@ -111,6 +111,10 @@ const mapGrepError = (error: unknown) => export const Event = FileSystem.Event +/** Directories first, then lexicographic; shared by every entry listing. */ +export const compareEntries = (a: Entry, b: Entry) => + a.type === b.type ? a.path.localeCompare(b.path) : a.type === "directory" ? -1 : 1 + export interface Interface { readonly read: (input: ReadInput) => Effect.Effect<{ readonly content: Uint8Array; readonly mime: string }> readonly list: (input?: ListInput) => Effect.Effect @@ -224,7 +228,7 @@ const makeEntries = (location: Location.Interface, root: string, backend: EntryB }), ] }) - .sort((a, b) => (a.type === b.type ? a.path.localeCompare(b.path) : a.type === "directory" ? -1 : 1)) + .sort(compareEntries) }), } } diff --git a/packages/core/src/tool/plugin/read.ts b/packages/core/src/tool/plugin/read.ts index de7e3fc485..0910485546 100644 --- a/packages/core/src/tool/plugin/read.ts +++ b/packages/core/src/tool/plugin/read.ts @@ -88,10 +88,9 @@ export const Plugin = { // is discovered); for a file it starts at the file's dirname. External reads are // skipped, and discovery failures never fail the read. yield* Effect.gen(function* () { - if (target.externalDirectory !== undefined) return - // Discovery walks the host filesystem; hosted instruction - // discovery needs an environment-backed walk first. - if (location.workspaceID !== undefined) return + // External reads skip discovery, and so do hosted Locations: + // the walk reads the host filesystem. + if (external !== undefined || location.workspaceID !== undefined) return const resolved = yield* fs.resolve(target.canonical) const root = yield* fs.resolve(location.directory) // up() searches its stop directory, so the Location-root AGENTS.md (already diff --git a/packages/core/src/tool/read-filesystem.ts b/packages/core/src/tool/read-filesystem.ts index c4519213f5..63503d9f29 100644 --- a/packages/core/src/tool/read-filesystem.ts +++ b/packages/core/src/tool/read-filesystem.ts @@ -111,6 +111,8 @@ export interface Interface { export class Service extends Context.Service()("@opencode/ReadToolFileSystem") {} +type FileType = WorkspaceEnvironment.FileInfo["type"] + /** The filesystem surface `read` consumes; FSUtil satisfies it structurally. */ export interface ReadSource { readonly realPath: (path: string) => Effect.Effect @@ -119,10 +121,7 @@ export interface ReadSource { options: { readonly flag: "r" }, ) => Effect.Effect< { - readonly stat: Effect.Effect< - { readonly type: WorkspaceEnvironment.FileInfo["type"]; readonly size: bigint | number }, - FSUtil.Error - > + readonly stat: Effect.Effect<{ readonly type: FileType; readonly size: bigint | number }, FSUtil.Error> readonly readAlloc: (bytes: number) => Effect.Effect, FSUtil.Error> }, FSUtil.Error, @@ -191,11 +190,7 @@ const decodeChunk = (resource: string, decoder: TextDecoder, bytes: Uint8Array) bytes.includes(0) ? Effect.fail(new BinaryFileError({ resource })) : decodeUtf8(resource, decoder, bytes) export const inspect = Effect.fn("ReadTool.inspect")(function* ( - fs: { - readonly stat: ( - path: string, - ) => Effect.Effect<{ readonly type: WorkspaceEnvironment.FileInfo["type"] }, FSUtil.Error> - }, + fs: { readonly stat: (path: string) => Effect.Effect<{ readonly type: FileType }, FSUtil.Error> }, input: string, ) { const info = yield* fs.stat(input) @@ -238,10 +233,7 @@ export const read = Effect.fn("ReadTool.read")(function* ( type: "file" as const, uri: pathToFileURL(real).href, name: path.basename(real), - content: Buffer.concat( - chunks.map((chunk) => Buffer.from(chunk)), - total, - ).toString("base64"), + content: Buffer.concat(chunks, total).toString("base64"), encoding: "base64" as const, mime, } @@ -362,8 +354,6 @@ export const read = Effect.fn("ReadTool.read")(function* ( export const list = Effect.fn("ReadTool.list")(function* (fs: FSUtil.Interface, input: string, page: PageInput = {}) { const real = yield* fs.realPath(input) const items = yield* fs.readDirectoryEntries(real) - const offset = page.offset || 1 - const limit = Math.min(page.limit || MAX_READ_LINES, MAX_READ_LINES) const entries = yield* Effect.forEach( items, (item) => @@ -381,16 +371,16 @@ export const list = Effect.fn("ReadTool.list")(function* (fs: FSUtil.Interface, }), { concurrency: 16 }, ) - const visible = entries.filter((item): item is FileSystem.Entry => item !== undefined) - return pageEntries(visible, { offset, limit }) + return sortAndPage( + entries.filter((item): item is FileSystem.Entry => item !== undefined), + page, + ) }) -const pageEntries = (entries: FileSystem.Entry[], page: PageInput) => { +const sortAndPage = (entries: FileSystem.Entry[], page: PageInput) => { const offset = page.offset || 1 const limit = Math.min(page.limit || MAX_READ_LINES, MAX_READ_LINES) - const visible = [...entries].sort((a, b) => - a.type === b.type ? a.path.localeCompare(b.path) : a.type === "directory" ? -1 : 1, - ) + const visible = entries.sort(FileSystem.compareEntries) const selected = visible.slice(offset - 1, offset - 1 + limit) const truncated = offset - 1 + selected.length < visible.length return new ListPage({ @@ -425,36 +415,55 @@ const hostedLayer = Layer.effect( // Absence surfaces as a platform NotFound so the read tool's missing-file // handling stays backend-agnostic; other failures become filesystem errors. - const mapEnv = + const mapError = (method: string) => (error: WorkspaceEnvironment.Error | WorkspaceEnvironment.NotFoundError): FSUtil.Error => error._tag === "WorkspaceEnvironment.NotFoundError" ? systemError({ _tag: "NotFound", module: "FileSystem", method, pathOrDescriptor: error.path, cause: error }) - : new FSUtil.FileSystemError({ method, cause: error }) + : WorkspaceEnvironment.toFileSystemError(method)(error) + + const stats = { stat: (target: string) => env.files.stat(target).pipe(Effect.mapError(mapError("stat"))) } const source: ReadSource = { - realPath: (target) => env.files.realPath(target).pipe(Effect.mapError(mapEnv("realPath"))), + // Paths arrive canonical from LocationMutation.resolve, so identity + // avoids a provider canonicalization round trip per read. + realPath: Effect.succeed, + // The environment has no ranged read, so each read call transfers the + // whole file once and pages in memory. Happy-path opens are that one + // transfer; only the failure path stats to classify a non-file target, + // which the shared read reports as PathKindError. + // TODO: Bound and page large hosted reads once environments expose file + // size and ranged reads. open: (target) => - Effect.gen(function* () { - const info = yield* env.files.stat(target).pipe(Effect.mapError(mapEnv("stat"))) - if (info.type !== "File") + env.files.read(target).pipe( + Effect.map((bytes) => { + let cursor = 0 return { - stat: Effect.succeed({ type: info.type, size: 0 }), - readAlloc: () => Effect.succeed(Option.none()), + stat: Effect.succeed({ type: "File" as const, size: bytes.length }), + readAlloc: (size: number) => + Effect.sync(() => { + if (cursor >= bytes.length) return Option.none() + const chunk = bytes.subarray(cursor, Math.min(cursor + size, bytes.length)) + cursor += chunk.length + return Option.some(chunk) + }), } - const bytes = yield* env.files.read(target).pipe(Effect.mapError(mapEnv("read"))) - let cursor = 0 - return { - stat: Effect.succeed({ type: info.type, size: bytes.length }), - readAlloc: (size: number) => - Effect.sync(() => { - if (cursor >= bytes.length) return Option.none() - const chunk = bytes.subarray(cursor, Math.min(cursor + size, bytes.length)) - cursor += chunk.length - return Option.some(chunk) - }), - } - }), + }), + Effect.catchTag("WorkspaceEnvironment.Error", (error) => + stats.stat(target).pipe( + Effect.catch(() => Effect.fail(mapError("read")(error))), + Effect.flatMap((info) => + info.type === "File" + ? Effect.fail(mapError("read")(error)) + : Effect.succeed({ + stat: Effect.succeed({ type: info.type, size: 0 }), + readAlloc: () => Effect.succeed(Option.none()), + }), + ), + ), + ), + Effect.catchTag("WorkspaceEnvironment.NotFoundError", (error) => Effect.fail(mapError("read")(error))), + ), } // Matches the local list semantics: symlinks resolve and escape-filter @@ -471,9 +480,8 @@ const hostedLayer = Layer.effect( return undefined }) - const hostedList = Effect.fn("ReadTool.list")(function* (input: string, page: PageInput = {}) { - const real = yield* source.realPath(input) - const items = yield* env.files.list(real).pipe(Effect.mapError(mapEnv("list"))) + const list = Effect.fn("ReadTool.list")(function* (input: string, page: PageInput = {}) { + const items = yield* env.files.list(input).pipe(Effect.mapError(mapError("list"))) const entries = yield* Effect.forEach( items, (item) => @@ -482,7 +490,7 @@ const hostedLayer = Layer.effect( item.type === "file" || item.type === "directory" ? item.type : item.type === "symlink" - ? yield* resolveSymlink(real, item.name) + ? yield* resolveSymlink(input, item.name) : undefined if (!type) return return FileSystem.Entry.make({ @@ -492,17 +500,16 @@ const hostedLayer = Layer.effect( }), { concurrency: 4 }, ) - return pageEntries( + return sortAndPage( entries.filter((item): item is FileSystem.Entry => item !== undefined), page, ) }) return Service.of({ - inspect: (target) => - inspect({ stat: (statTarget) => env.files.stat(statTarget).pipe(Effect.mapError(mapEnv("stat"))) }, target), + inspect: (target) => inspect(stats, target), read: (target, resource, page) => read(source, target, resource, page), - list: (target, page) => hostedList(target, page), + list, }) }), ) diff --git a/packages/core/src/workspace/environment.ts b/packages/core/src/workspace/environment.ts index 64e8f8588f..2a9a6b6f14 100644 --- a/packages/core/src/workspace/environment.ts +++ b/packages/core/src/workspace/environment.ts @@ -1,6 +1,7 @@ export * as WorkspaceEnvironment from "./environment" import { Context, Effect, FileSystem, Layer, Schema } from "effect" +import { FSUtil } from "@opencode-ai/util/fs-util" import type { ChildProcessSpawner } from "effect/unstable/process/ChildProcessSpawner" import { makeLocationNode, tags } from "@opencode-ai/util/effect/app-node" import { LayerNode } from "@opencode-ai/util/effect/layer-node" @@ -18,6 +19,12 @@ export class NotFoundError extends Schema.TaggedErrorClass()("Wor path: Schema.String, }) {} +/** Translate environment failures into the host filesystem error vocabulary. */ +export const toFileSystemError = + (method: string) => + (cause: Error | NotFoundError): FSUtil.Error => + new FSUtil.FileSystemError({ method, cause }) + /** * Wrap one driver promise, translating the driver's not-found signal into the * environment error vocabulary so drivers and fakes never construct it ad hoc. diff --git a/packages/core/test/workspace-graph-guard.test.ts b/packages/core/test/workspace-graph-guard.test.ts index a80cacdc3d..e5839f431c 100644 --- a/packages/core/test/workspace-graph-guard.test.ts +++ b/packages/core/test/workspace-graph-guard.test.ts @@ -41,36 +41,30 @@ const allowed: Record = { describe("hosted location graph", () => { test("location-scoped services do not use host primitives without justification", () => { - const ref = Location.Ref.make({ - directory: AbsolutePath.make("/workspace"), - workspaceID: Workspace.ID.make("wrk_graphguard000000000000000"), - }) + const workspaceID = Workspace.ID.make("wrk_graphguard000000000000000") + const ref = Location.Ref.make({ directory: AbsolutePath.make("/workspace"), workspaceID }) // Replacement targets may be bare layers in general; every hosted // replacement is a node today, and bare-layer targets have no walkable // dependencies anyway. - const replaced = new Map, LayerNode.Node>( - hostedReplacements(ref, ref.workspaceID!).flatMap(([from, to]) => - "dependencies" in to ? [[from, to] as const] : [], - ), + const replaced = new Map( + hostedReplacements(ref, workspaceID).flatMap(([from, to]) => (LayerNode.isNode(to) ? [[from, to] as const] : [])), ) - const resolve = (node: LayerNode.Node) => replaced.get(node) ?? node + const resolve = (node: LayerNode.AnyNode) => replaced.get(node) ?? node - const hostPrimitives = new Set>([FSUtil.node, AppProcess.node]) - const visited = new Set>() + const hostPrimitives = new Set([FSUtil.node, AppProcess.node]) const offenders = new Map() - const queue: LayerNode.Node[] = [locationServices] - while (queue.length > 0) { - const node = resolve(queue.pop()!) - if (visited.has(node)) continue - visited.add(node) - for (const dependency of node.dependencies) { - const target = resolve(dependency) - if (hostPrimitives.has(target) && node.tag === Node.tags.values.location) { - offenders.set(node.name, [...(offenders.get(node.name) ?? []), target.name]) + LayerNode.walk( + locationServices, + (node, context) => { + for (const dependency of node.dependencies) { + if (hostPrimitives.has(resolve(dependency)) && node.tag === Node.tags.values.location) { + offenders.set(node.name, [...(offenders.get(node.name) ?? []), resolve(dependency).name]) + } + context.visit(dependency) } - queue.push(target) - } - } + }, + { resolve }, + ) const unjustified = [...offenders.entries()] .filter(([name]) => !(name in allowed)) diff --git a/packages/server/src/handlers/session.ts b/packages/server/src/handlers/session.ts index c173ec5853..97ffe552c7 100644 --- a/packages/server/src/handlers/session.ts +++ b/packages/server/src/handlers/session.ts @@ -82,12 +82,12 @@ export const SessionHandler = HttpApiBuilder.group(Api, "server.session", (handl location: ctx.payload.location ?? { directory: AbsolutePath.make(process.cwd()) }, }) .pipe( - Effect.catchTag("Workspace.NotFoundError", (error) => - Effect.fail(new InvalidRequestError({ message: `Workspace not found: ${error.id}` })), - ), - Effect.catchTag("Session.WorkspaceDirectoryError", (error) => - Effect.fail(new InvalidRequestError({ message: error.message })), - ), + Effect.catchTags({ + "Workspace.NotFoundError": (error) => + Effect.fail(new InvalidRequestError({ message: `Workspace not found: ${error.id}` })), + "Session.WorkspaceDirectoryError": (error) => + Effect.fail(new InvalidRequestError({ message: error.message })), + }), Effect.orDie, ), } diff --git a/packages/util/src/effect/layer-node.ts b/packages/util/src/effect/layer-node.ts index 57234c436a..1261aa6fc5 100644 --- a/packages/util/src/effect/layer-node.ts +++ b/packages/util/src/effect/layer-node.ts @@ -1,6 +1,6 @@ import { Brand, Context, Layer } from "effect" -type AnyNode = Node +export type AnyNode = Node type RuntimeLayer = Layer.Layer type NodeList = readonly [] | readonly [Item, ...Item[]] export type Output = [Item] extends [never] ? never : Item extends Node ? A : never @@ -155,7 +155,7 @@ function nodeMakeIdentity(node: AnyNode): NodeIdentity { return { name: node.name } } -function isNode(input: Layer.Any | AnyNode): input is AnyNode { +export function isNode(input: Layer.Any | AnyNode): input is AnyNode { return "kind" in input && "dependencies" in input } @@ -168,7 +168,7 @@ type VisitContext = { readonly visit: (node: AnyNode) => Result } -function walk( +export function walk( root: AnyNode, visit: Visit, options: {