Skip to content

Commit c796b9a

Browse files
authored
refactor(effect): move read tool onto defineEffect (#21016)
1 parent 6ea108a commit c796b9a

File tree

6 files changed

+822
-812
lines changed

6 files changed

+822
-812
lines changed

packages/opencode/specs/effect-migration.md

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -235,11 +235,27 @@ Once individual tools are effectified, change `Tool.Info` (`tool/tool.ts`) so `i
235235
2. Update `Tool.define()` factory to work with Effects
236236
3. Update `SessionPrompt` to `yield*` tool results instead of `await`ing
237237

238+
### Tool migration details
239+
240+
Until the tool interface itself returns `Effect`, use this transitional pattern for migrated tools:
241+
242+
- `Tool.defineEffect(...)` should `yield*` the services the tool depends on and close over them in the returned tool definition.
243+
- Keep the bridge at the Promise boundary only. Prefer a single `Effect.runPromise(...)` in the temporary `async execute(...)` implementation, and move the inner logic into `Effect.fn(...)` helpers instead of scattering `runPromise` islands through the tool body.
244+
- If a tool starts requiring new services, wire them into `ToolRegistry.defaultLayer` so production callers resolve the same dependencies as tests.
245+
246+
Tool tests should use the existing Effect helpers in `packages/opencode/test/lib/effect.ts`:
247+
248+
- Use `testEffect(...)` / `it.live(...)` instead of creating fake local wrappers around effectful tools.
249+
- Yield the real tool export, then initialize it: `const info = yield* ReadTool`, `const tool = yield* Effect.promise(() => info.init())`.
250+
- Run tests inside a real instance with `provideTmpdirInstance(...)` or `provideInstance(tmpdirScoped(...))` so instance-scoped services resolve exactly as they do in production.
251+
252+
This keeps migrated tool tests aligned with the production service graph today, and makes the eventual `Tool.Info``Effect` cleanup mostly mechanical later.
253+
238254
Individual tools, ordered by value:
239255

240256
- [ ] `apply_patch.ts` — HIGH: multi-step orchestration, error accumulation, Bus events
241257
- [ ] `bash.ts` — HIGH: shell orchestration, quoting, timeout handling, output capture
242-
- [ ] `read.ts` — HIGH: streaming I/O, readline, binary detection → FileSystem + Stream
258+
- [x] `read.ts` — HIGH: streaming I/O, readline, binary detection → FileSystem + Stream
243259
- [ ] `edit.ts` — HIGH: multi-step diff/format/publish pipeline, FileWatcher lock
244260
- [ ] `grep.ts` — MEDIUM: spawns ripgrep → ChildProcessSpawner, timeout handling
245261
- [ ] `write.ts` — MEDIUM: permission checks, diagnostics polling, Bus events

packages/opencode/src/filesystem/index.ts

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,13 +188,23 @@ export namespace AppFileSystem {
188188

189189
export function normalizePath(p: string): string {
190190
if (process.platform !== "win32") return p
191+
const resolved = pathResolve(windowsPath(p))
191192
try {
192-
return realpathSync.native(p)
193+
return realpathSync.native(resolved)
193194
} catch {
194-
return p
195+
return resolved
195196
}
196197
}
197198

199+
export function normalizePathPattern(p: string): string {
200+
if (process.platform !== "win32") return p
201+
if (p === "*") return p
202+
const match = p.match(/^(.*)[\\/]\*$/)
203+
if (!match) return normalizePath(p)
204+
const dir = /^[A-Za-z]:$/.test(match[1]) ? match[1] + "\\" : match[1]
205+
return join(normalizePath(dir), "*")
206+
}
207+
198208
export function resolve(p: string): string {
199209
const resolved = pathResolve(windowsPath(p))
200210
try {

packages/opencode/src/tool/external-directory.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import path from "path"
2+
import { Effect } from "effect"
23
import type { Tool } from "./tool"
34
import { Instance } from "../project/instance"
4-
import { Filesystem } from "@/util/filesystem"
5+
import { AppFileSystem } from "../filesystem"
56

67
type Kind = "file" | "directory"
78

@@ -15,14 +16,14 @@ export async function assertExternalDirectory(ctx: Tool.Context, target?: string
1516

1617
if (options?.bypass) return
1718

18-
const full = process.platform === "win32" ? Filesystem.normalizePath(target) : target
19+
const full = process.platform === "win32" ? AppFileSystem.normalizePath(target) : target
1920
if (Instance.containsPath(full)) return
2021

2122
const kind = options?.kind ?? "file"
2223
const dir = kind === "directory" ? full : path.dirname(full)
2324
const glob =
2425
process.platform === "win32"
25-
? Filesystem.normalizePathPattern(path.join(dir, "*"))
26+
? AppFileSystem.normalizePathPattern(path.join(dir, "*"))
2627
: path.join(dir, "*").replaceAll("\\", "/")
2728

2829
await ctx.ask({
@@ -35,3 +36,11 @@ export async function assertExternalDirectory(ctx: Tool.Context, target?: string
3536
},
3637
})
3738
}
39+
40+
export const assertExternalDirectoryEffect = Effect.fn("Tool.assertExternalDirectory")(function* (
41+
ctx: Tool.Context,
42+
target?: string,
43+
options?: Options,
44+
) {
45+
yield* Effect.promise(() => assertExternalDirectory(ctx, target, options))
46+
})

0 commit comments

Comments
 (0)