diff --git a/packages/opencode/src/project/instance.ts b/packages/opencode/src/project/instance.ts index 8cf244cb..4bd87105 100644 --- a/packages/opencode/src/project/instance.ts +++ b/packages/opencode/src/project/instance.ts @@ -104,6 +104,12 @@ export const Instance = { get directory() { return context.use().directory }, + // Like `directory`, but undefined outside an instance context instead of + // throwing — for module state that wants to scope itself by directory but + // may also be touched before an instance is provided. + get directoryOrUndefined() { + return context.tryUse()?.directory + }, get worktree() { return context.use().worktree }, diff --git a/packages/opencode/src/tool/read-state.ts b/packages/opencode/src/tool/read-state.ts index 9c60cfac..6ceedb6e 100644 --- a/packages/opencode/src/tool/read-state.ts +++ b/packages/opencode/src/tool/read-state.ts @@ -3,14 +3,83 @@ import type * as Tool from "./tool" import { SessionCwd } from "./session-cwd" import { AppFileSystem } from "@mimo-ai/shared/filesystem" import { RecoverableError } from "./recoverable" +import { registerDisposer } from "@/effect/instance-registry" +import { Instance } from "@/project/instance" import type { SessionID } from "../session/schema" -// Same normalization both sides of the comparison go through so a Read on -// a relative path lines up with an Edit on the absolute one. +const MAIN_ACTOR_ID = "main" +type ReadContext = Pick + +// Per actor, group read paths by the instance directory that owns them. A +// session can span several directories, and keeping the directory below the +// actor level avoids overwriting marks if an actor ever reads in more than one. +type ActorReads = Map> +const readState = new Map>() + +// A server can host several project instances at once, and disposeInstance() +// runs every disposer with the directory being torn down. Drop only the marks +// owned by that directory; clearing more would wipe other projects' (or, on a +// shared session, other actors') live marks and make their next edit fail +// "has not been read". +registerDisposer(async (directory) => { + for (const [sessionID, actors] of readState) { + for (const [actor, readsByDirectory] of actors) { + readsByDirectory.delete(directory) + if (readsByDirectory.size === 0) actors.delete(actor) + } + if (actors.size === 0) readState.delete(sessionID) + } +}) + function canon(sessionID: SessionID, p: string): string { const abs = path.isAbsolute(p) ? p : path.resolve(SessionCwd.get(sessionID), p) - if (process.platform === "win32") return AppFileSystem.normalizePath(abs).toLowerCase() - return abs + const resolved = AppFileSystem.resolve(abs) + if (process.platform === "win32") return resolved.toLowerCase() + return resolved +} + +function actorID(ctx: ReadContext) { + return ctx.actorID ?? MAIN_ACTOR_ID +} + +function sessionActors(sessionID: SessionID) { + const existing = readState.get(sessionID) + if (existing) return existing + + const next = new Map() + readState.set(sessionID, next) + return next +} + +function actorReads(ctx: ReadContext) { + const actors = sessionActors(ctx.sessionID) + const existing = actors.get(actorID(ctx)) + if (existing) return existing + + const next: ActorReads = new Map() + actors.set(actorID(ctx), next) + return next +} + +export function markFileRead(ctx: ReadContext, targetPath: string): void { + const reads = actorReads(ctx) + const target = canon(ctx.sessionID, targetPath) + const directory = Instance.directoryOrUndefined + const existing = reads.get(directory) + if (existing) { + existing.add(target) + return + } + + reads.set(directory, new Set([target])) +} + +export function clearReadState(sessionID?: SessionID): void { + if (!sessionID) { + readState.clear() + return + } + readState.delete(sessionID) } /** @@ -25,6 +94,7 @@ function canon(sessionID: SessionID, p: string): string { */ export function assertFileRead(ctx: Tool.Context, targetPath: string, toolId: string): void { const target = canon(ctx.sessionID, targetPath) + if ([...(readState.get(ctx.sessionID)?.get(actorID(ctx))?.values() ?? [])].some((paths) => paths.has(target))) return for (const msg of ctx.messages) { for (const part of msg.parts) { diff --git a/packages/opencode/src/tool/read.ts b/packages/opencode/src/tool/read.ts index 026cf323..ab4770d6 100644 --- a/packages/opencode/src/tool/read.ts +++ b/packages/opencode/src/tool/read.ts @@ -12,6 +12,7 @@ import { assertExternalDirectoryEffect } from "./external-directory" import { SessionCwd } from "./session-cwd" import { Instruction } from "../session/instruction" import { isImageAttachment, isPdfAttachment, sniffAttachmentMime } from "@/util/media" +import { markFileRead } from "./read-state" const DEFAULT_READ_LIMIT = 2000 const MAX_LINE_LENGTH = 2000 @@ -183,6 +184,7 @@ export const ReadTool = Tool.define( const start = offset - 1 const sliced = items.slice(start, start + limit) const truncated = start + sliced.length < items.length + markFileRead(ctx, filepath) return { title, @@ -211,6 +213,7 @@ export const ReadTool = Tool.define( if (isImageAttachment(mime) || isPdfAttachment(mime)) { const bytes = yield* fs.readFile(filepath) const msg = isPdfAttachment(mime) ? "PDF read successfully" : "Image read successfully" + markFileRead(ctx, filepath) return { title, output: msg, @@ -263,6 +266,8 @@ export const ReadTool = Tool.define( output += `\n\n\n${loaded.map((item) => item.content).join("\n\n")}\n` } + markFileRead(ctx, filepath) + return { title, output, diff --git a/packages/opencode/src/util/local-context.ts b/packages/opencode/src/util/local-context.ts index c1aef946..c283d58c 100644 --- a/packages/opencode/src/util/local-context.ts +++ b/packages/opencode/src/util/local-context.ts @@ -16,6 +16,9 @@ export function create(name: string) { } return result }, + tryUse() { + return storage.getStore() + }, provide(value: T, fn: () => R) { return storage.run(value, fn) }, diff --git a/packages/opencode/test/tool/edit.test.ts b/packages/opencode/test/tool/edit.test.ts index 88b48bcb..27e54369 100644 --- a/packages/opencode/test/tool/edit.test.ts +++ b/packages/opencode/test/tool/edit.test.ts @@ -14,6 +14,7 @@ import { BusEvent } from "../../src/bus/bus-event" import { Truncate } from "../../src/tool" import { SessionID, MessageID, PartID } from "../../src/session/schema" import type { MessageV2 } from "../../src/session/message-v2" +import { clearReadState, markFileRead } from "../../src/tool/read-state" const baseCtx = { sessionID: SessionID.make("ses_test-edit-session"), @@ -65,6 +66,7 @@ function withRead(filePath: string, ctx: EditCtx = baseCtx): EditCtx { const ctx = baseCtx afterEach(async () => { + clearReadState(ctx.sessionID) await Instance.disposeAll() }) @@ -94,6 +96,8 @@ const resolve = () => const subscribeBus = (def: D, callback: () => unknown) => runtime.runPromise(Bus.Service.use((bus) => bus.subscribeCallback(def, callback))) +const markRead = (filePath: string) => markFileRead(ctx, filePath) + async function onceBus(def: D) { const result = Promise.withResolvers() const unsub = await subscribeBus(def, () => { @@ -198,6 +202,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "existing.txt") await fs.writeFile(filepath, "old content here", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -225,6 +230,9 @@ describe("tool.edit", () => { test("throws error when file does not exist", async () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "nonexistent.txt") + await fs.writeFile(filepath, "old", "utf-8") + markRead(filepath) + await fs.unlink(filepath) await Instance.provide({ directory: tmp.path, @@ -275,6 +283,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "actual content", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -300,6 +309,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "foo bar foo baz foo", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -327,6 +337,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "original", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -362,6 +373,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "line1\nline2\nline3", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -388,6 +400,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "line1\r\nold\r\nline3", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -439,6 +452,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const dirpath = path.join(tmp.path, "adir") await fs.mkdir(dirpath) + markRead(dirpath) await Instance.provide({ directory: tmp.path, @@ -464,6 +478,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "line1\nline2\nline3", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, @@ -539,6 +554,7 @@ describe("tool.edit", () => { fn: async () => { const edit = await resolve() const filePath = path.join(tmp.path, "test.txt") + markRead(filePath) await Effect.runPromise( edit.execute( { @@ -677,6 +693,7 @@ describe("tool.edit", () => { await using tmp = await tmpdir() const filepath = path.join(tmp.path, "file.txt") await fs.writeFile(filepath, "top = 0\nmiddle = keep\nbottom = 0\n", "utf-8") + markRead(filepath) await Instance.provide({ directory: tmp.path, diff --git a/packages/opencode/test/tool/read-state.test.ts b/packages/opencode/test/tool/read-state.test.ts new file mode 100644 index 00000000..4db28452 --- /dev/null +++ b/packages/opencode/test/tool/read-state.test.ts @@ -0,0 +1,197 @@ +import { afterAll, afterEach, describe, expect, test } from "bun:test" +import path from "path" +import { Effect, Layer, ManagedRuntime } from "effect" +import { Agent } from "../../src/agent/agent" +import { Bus } from "../../src/bus" +import { Format } from "../../src/format" +import { LSP } from "../../src/lsp" +import { Instance } from "../../src/project/instance" +import { Instruction } from "../../src/session/instruction" +import { MessageID, SessionID } from "../../src/session/schema" +import { EditTool } from "../../src/tool/edit" +import { ReadTool } from "../../src/tool/read" +import { assertFileRead, clearReadState, markFileRead } from "../../src/tool/read-state" +import { disposeInstance } from "../../src/effect/instance-registry" +import { Truncate } from "../../src/tool" +import { AppFileSystem } from "@mimo-ai/shared/filesystem" +import { tmpdir } from "../fixture/fixture" + +const ctx = { + sessionID: SessionID.make("ses_test-read-state-session"), + messageID: MessageID.make(""), + callID: "", + agent: "build", + abort: AbortSignal.any([]), + messages: [], + metadata: () => Effect.void, + ask: () => Effect.void, +} + +const runtime = ManagedRuntime.make( + Layer.mergeAll( + Agent.defaultLayer, + AppFileSystem.defaultLayer, + Bus.layer, + Format.defaultLayer, + Instruction.defaultLayer, + LSP.defaultLayer, + Truncate.defaultLayer, + ), +) + +afterEach(async () => { + clearReadState() + await Instance.disposeAll() +}) + +afterAll(async () => { + await runtime.dispose() +}) + +const read = (filePath: string) => + runtime.runPromise( + Effect.scoped( + Effect.gen(function* () { + const info = yield* ReadTool + const tool = yield* info.init() + return yield* tool.execute({ file_path: filePath }, ctx) + }), + ), + ) + +const edit = (filePath: string) => + runtime.runPromise( + Effect.gen(function* () { + const info = yield* EditTool + const tool = yield* info.init() + return yield* tool.execute({ file_path: filePath, old_string: "old", new_string: "new" }, ctx) + }), + ) + +describe("tool.read-state", () => { + test("allows edit after the read tool marked the file in the same session", async () => { + await using tmp = await tmpdir() + const filePath = path.join(tmp.path, "file.txt") + await Bun.write(filePath, "old") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + await read("file.txt") + await edit(filePath) + expect(await Bun.file(filePath).text()).toBe("new") + }, + }) + }) + + test("keeps runtime read state scoped to the session", async () => { + await using tmp = await tmpdir() + const filePath = path.join(tmp.path, "file.txt") + await Bun.write(filePath, "old") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + markFileRead(ctx, filePath) + expect(() => + assertFileRead( + { + ...ctx, + sessionID: SessionID.make("ses_test-read-state-other-session"), + }, + filePath, + "edit", + ), + ).toThrow("has not been read") + }, + }) + }) + + test("keeps runtime read state scoped to the actor", async () => { + await using tmp = await tmpdir() + const filePath = path.join(tmp.path, "file.txt") + await Bun.write(filePath, "old") + + await Instance.provide({ + directory: tmp.path, + fn: async () => { + const readerCtx = { ...ctx, actorID: "explore-1" } + markFileRead(readerCtx, filePath) + + expect(() => assertFileRead(readerCtx, filePath, "edit")).not.toThrow() + expect(() => assertFileRead({ ...ctx, actorID: "writer-1" }, filePath, "edit")).toThrow("has not been read") + expect(() => assertFileRead(ctx, filePath, "edit")).toThrow("has not been read") + }, + }) + }) + + test("disposing one instance leaves another instance's read marks intact", async () => { + await using tmpA = await tmpdir() + await using tmpB = await tmpdir() + const fileB = path.join(tmpB.path, "file.txt") + await Bun.write(fileB, "old") + + const ctxB = { ...ctx, sessionID: SessionID.make("ses_test-read-state-dir-b") } + + await Instance.provide({ + directory: tmpB.path, + fn: async () => markFileRead(ctxB, fileB), + }) + + // Tearing down a different project (A) must not wipe B's marks. Absolute + // paths mean assertFileRead needs no instance context here. + await disposeInstance(AppFileSystem.resolve(tmpA.path)) + expect(() => assertFileRead(ctxB, fileB, "edit")).not.toThrow() + + // Tearing down B's own directory does clear B. + await disposeInstance(AppFileSystem.resolve(tmpB.path)) + expect(() => assertFileRead(ctxB, fileB, "edit")).toThrow("has not been read") + }) + + test("scopes disposal per actor when one session spans multiple directories", async () => { + await using parent = await tmpdir() + await using worktree = await tmpdir() + const parentFile = path.join(parent.path, "p.txt") + const worktreeFile = path.join(worktree.path, "w.txt") + await Bun.write(parentFile, "old") + await Bun.write(worktreeFile, "old") + + // Same session, two actors: the parent reads in the main tree while an + // isolated subagent reads in its worktree (workflow/runtime.ts). + const mainCtx = { ...ctx, actorID: "main" } + const subCtx = { ...ctx, actorID: "explore-1" } + + await Instance.provide({ directory: parent.path, fn: async () => markFileRead(mainCtx, parentFile) }) + await Instance.provide({ directory: worktree.path, fn: async () => markFileRead(subCtx, worktreeFile) }) + + // Disposing the worktree clears only the subagent's marks; the parent's survive. + await disposeInstance(AppFileSystem.resolve(worktree.path)) + expect(() => assertFileRead(subCtx, worktreeFile, "edit")).toThrow("has not been read") + expect(() => assertFileRead(mainCtx, parentFile, "edit")).not.toThrow() + + // Disposing the parent then clears the parent actor's marks. + await disposeInstance(AppFileSystem.resolve(parent.path)) + expect(() => assertFileRead(mainCtx, parentFile, "edit")).toThrow("has not been read") + }) + + test("scopes disposal per directory when one actor spans multiple directories", async () => { + await using parent = await tmpdir() + await using worktree = await tmpdir() + const parentFile = path.join(parent.path, "p.txt") + const worktreeFile = path.join(worktree.path, "w.txt") + await Bun.write(parentFile, "old") + await Bun.write(worktreeFile, "old") + + const sharedCtx = { ...ctx, actorID: "shared-actor" } + + await Instance.provide({ directory: parent.path, fn: async () => markFileRead(sharedCtx, parentFile) }) + await Instance.provide({ directory: worktree.path, fn: async () => markFileRead(sharedCtx, worktreeFile) }) + + await disposeInstance(AppFileSystem.resolve(worktree.path)) + expect(() => assertFileRead(sharedCtx, worktreeFile, "edit")).toThrow("has not been read") + expect(() => assertFileRead(sharedCtx, parentFile, "edit")).not.toThrow() + + await disposeInstance(AppFileSystem.resolve(parent.path)) + expect(() => assertFileRead(sharedCtx, parentFile, "edit")).toThrow("has not been read") + }) +})