Browse Source

fix(util): no filesystem side effects at global module load (#41619)

Kit Langton 6 days ago
parent
commit
6afff5a55f

+ 0 - 16
packages/core/test/global.test.ts

@@ -1,16 +0,0 @@
-import { describe, expect, test } from "bun:test"
-import fs from "fs/promises"
-import os from "os"
-import path from "path"
-import { Global } from "@opencode-ai/util/global"
-
-describe("global paths", () => {
-  test("tmp path is the canonical system temp directory", async () => {
-    expect(Global.Path.tmp).toBe(await fs.realpath(path.join(os.tmpdir(), "opencode")))
-    expect(Global.make().tmp).toBe(Global.Path.tmp)
-  })
-
-  test("tmp path is created on module load", async () => {
-    expect((await fs.stat(Global.Path.tmp)).isDirectory()).toBe(true)
-  })
-})

+ 7 - 5
packages/core/test/instruction-discovery.test.ts

@@ -1,6 +1,7 @@
 import { describe, expect } from "bun:test"
 import { Effect, Layer } from "effect"
 import fs from "fs/promises"
+import os from "os"
 import path from "path"
 import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder"
 import { LayerNode } from "@opencode-ai/util/effect/layer-node"
@@ -15,6 +16,7 @@ import { testEffect } from "./lib/effect"
 import { readInitial, readUpdate, state } from "./lib/instructions"
 
 const it = testEffect(Layer.empty)
+const testConfig = path.join(os.tmpdir(), "opencode-instruction-discovery-test")
 
 const instructionLayer = (input: {
   config: string
@@ -147,7 +149,7 @@ describe("InstructionDiscovery", () => {
         Effect.flatMap((service) => service.load()),
         Effect.provide(
           instructionLayer({
-            config: "/global",
+            config: testConfig,
             filesystemLayer: failingFS,
             locationServiceLayer: Layer.succeed(
               Location.Service,
@@ -183,7 +185,7 @@ describe("InstructionDiscovery", () => {
         Effect.flatMap((service) => service.load()),
         Effect.provide(
           instructionLayer({
-            config: "/global",
+            config: testConfig,
             filesystemLayer: racingFS,
             locationServiceLayer: Layer.succeed(
               Location.Service,
@@ -222,7 +224,7 @@ describe("InstructionDiscovery", () => {
         Effect.flatMap((service) => service.load()),
         Effect.provide(
           instructionLayer({
-            config: "/global",
+            config: testConfig,
             filesystemLayer: observingFS,
             locationServiceLayer: Layer.succeed(
               Location.Service,
@@ -250,7 +252,7 @@ describe("InstructionDiscovery", () => {
         Effect.flatMap((service) => service.load()),
         Effect.provide(
           instructionLayer({
-            config: "/global",
+            config: testConfig,
             project: false,
             filesystemLayer: Layer.effect(
               FSUtil.Service,
@@ -277,7 +279,7 @@ describe("InstructionDiscovery", () => {
         Effect.flatMap((service) => service.load()),
         Effect.provide(
           instructionLayer({
-            config: "/global",
+            config: testConfig,
             filesystemLayer: Layer.effect(
               FSUtil.Service,
               FSUtil.Service.pipe(

+ 4 - 2
packages/core/test/instructions/builtins.test.ts

@@ -1,4 +1,5 @@
 import { describe, expect } from "bun:test"
+import os from "os"
 import { Effect, Layer } from "effect"
 import * as TestClock from "effect/testing/TestClock"
 import { AppNodeBuilder } from "@opencode-ai/core/effect/app-node-builder"
@@ -16,6 +17,7 @@ const directory = AbsolutePath.make(FSUtil.resolve("/repo/packages/core"))
 const projectDirectory = AbsolutePath.make(FSUtil.resolve("/repo"))
 const timestamp = Date.parse("2026-06-03T12:00:00.000Z")
 const sessionID = SessionSchema.ID.make("ses_builtin_test")
+const temporary = os.tmpdir()
 const localDate = (time: number) => new Date(time).toDateString()
 const locationLayer = Layer.succeed(
   Location.Service,
@@ -29,7 +31,7 @@ const locationLayer = Layer.succeed(
 const it = testEffect(
   AppNodeBuilder.build(InstructionBuiltIns.node, [
     [Location.node, locationLayer],
-    [Global.node, Global.layerWith({ config: "/global", tmp: "/temporary" })],
+    [Global.node, Global.layerWith({ config: temporary, tmp: temporary })],
   ]),
 )
 
@@ -49,7 +51,7 @@ describe("InstructionBuiltIns", () => {
           `  Workspace root folder: ${projectDirectory}`,
           "  Is directory a git repo: yes",
           `  Platform: ${process.platform}`,
-          "  Use /temporary for temporary work outside the workspace; it already exists and is pre-approved for external directory access.",
+          `  Use ${temporary} for temporary work outside the workspace; it already exists and is pre-approved for external directory access.`,
           "</env>",
           "",
           `Today's date: ${localDate(timestamp)}`,

+ 20 - 20
packages/util/src/global.ts

@@ -1,5 +1,5 @@
 import path from "path"
-import fs from "fs/promises"
+import fs from "fs"
 import { xdgData, xdgCache, xdgConfig, xdgState } from "xdg-basedir"
 import os from "os"
 import { Context, Effect, Layer } from "effect"
@@ -13,8 +13,6 @@ const config = path.join(xdgConfig!, app)
 const state = path.join(xdgState!, app)
 const tmp = path.join(os.tmpdir(), app)
 
-await fs.mkdir(tmp, { recursive: true })
-
 const paths = {
   get home() {
     return process.env.OPENCODE_TEST_HOME ?? os.homedir()
@@ -26,22 +24,13 @@ const paths = {
   cache,
   config,
   state,
-  tmp: await fs.realpath(tmp),
+  tmp,
 }
 
 export const Path = paths
 
 Flock.setGlobal({ state })
 
-await Promise.all([
-  fs.mkdir(Path.data, { recursive: true }),
-  fs.mkdir(Path.config, { recursive: true }),
-  fs.mkdir(Path.state, { recursive: true }),
-  fs.mkdir(Path.log, { recursive: true }),
-  fs.mkdir(Path.bin, { recursive: true }),
-  fs.mkdir(Path.repos, { recursive: true }),
-])
-
 export class Service extends Context.Service<Service, Interface>()("@opencode/Global") {}
 
 export interface Interface {
@@ -57,13 +46,14 @@ export interface Interface {
 }
 
 export function make(input: Partial<Interface> = {}): Interface {
+  // The acquired service canonicalizes default tmp; use it instead of Path.tmp for path comparisons.
   return {
     home: Path.home,
     data: Path.data,
     cache: Path.cache,
     config: Path.config,
     state: Path.state,
-    tmp: Path.tmp,
+    tmp: input.tmp ?? Path.tmp,
     bin: Path.bin,
     log: Path.log,
     repos: Path.repos,
@@ -71,17 +61,27 @@ export function make(input: Partial<Interface> = {}): Interface {
   }
 }
 
+const acquire = (input: Partial<Interface>) =>
+  Effect.gen(function* () {
+    const service = Service.of(make(input))
+    yield* Effect.promise(() =>
+      Promise.all(
+        [service.data, service.config, service.state, service.log, service.bin, service.repos, service.tmp].map(
+          (directory) => fs.promises.mkdir(directory, { recursive: true }),
+        ),
+      ),
+    )
+    const canonicalTmp = yield* Effect.promise(() => fs.promises.realpath(service.tmp))
+    return Service.of({ ...service, tmp: input.tmp ?? canonicalTmp })
+  })
+
 const layer = Layer.effect(
   Service,
-  Effect.sync(() => Service.of(make({ config: process.env.OPENCODE_CONFIG_DIR ?? Path.config }))),
+  Effect.suspend(() => acquire({ config: process.env.OPENCODE_CONFIG_DIR ?? Path.config })),
 )
 
 export const node = makeGlobalNode({ service: Service, layer: layer, deps: [] })
 
-export const layerWith = (input: Partial<Interface>) =>
-  Layer.effect(
-    Service,
-    Effect.sync(() => Service.of(make(input))),
-  )
+export const layerWith = (input: Partial<Interface>) => Layer.effect(Service, acquire(input))
 
 export * as Global from "./global.js"

+ 6 - 2
packages/util/src/observability/logging.ts

@@ -1,4 +1,4 @@
-import { Formatter, Logger, type LogLevel } from "effect"
+import { Effect, FileSystem, Formatter, Logger, type LogLevel } from "effect"
 import path from "path"
 import { Global } from "../global.js"
 import { runID } from "./shared.js"
@@ -53,7 +53,11 @@ export function file(local = true, channel = "local") {
 
 export function fileLogger(target = file(), id: string = runID) {
   // Do not set batchWindow to 0; it causes high idle CPU usage.
-  return Logger.toFile(formatter(id), target, { flag: "a" })
+  return Effect.gen(function* () {
+    const fs = yield* FileSystem.FileSystem
+    yield* fs.makeDirectory(path.dirname(target), { recursive: true })
+    return yield* Logger.toFile(formatter(id), target, { flag: "a" })
+  })
 }
 
 const stderrLogger = Logger.make((options) => process.stderr.write(formatter().log(options) + "\n"))

+ 93 - 0
packages/util/test/global.test.ts

@@ -0,0 +1,93 @@
+import { describe, expect, test } from "bun:test"
+import fs from "fs"
+import os from "os"
+import path from "path"
+import { pathToFileURL } from "url"
+import { Context, Effect, Layer } from "effect"
+import { Global } from "../src/global.js"
+
+describe("global", () => {
+  test("importing the module does not create directories", () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "opencode-global-import-"))
+    const directories = ["data", "cache", "config", "state", "tmp"].map((directory) => path.join(root, directory))
+    const module = pathToFileURL(path.join(import.meta.dir, "../src/global.ts")).href
+    const result = Bun.spawnSync({
+      cmd: [process.execPath, "-e", `const { Global } = await import(${JSON.stringify(module)}); void Global.Path.tmp`],
+      env: {
+        ...process.env,
+        XDG_DATA_HOME: directories[0],
+        XDG_CACHE_HOME: directories[1],
+        XDG_CONFIG_HOME: directories[2],
+        XDG_STATE_HOME: directories[3],
+        TMPDIR: directories[4],
+      },
+      stderr: "pipe",
+    })
+
+    expect(result.exitCode, result.stderr.toString()).toBe(0)
+    directories.forEach((directory) => expect(fs.existsSync(path.join(directory, "opencode"))).toBe(false))
+    fs.rmSync(root, { recursive: true, force: true })
+  })
+
+  test("building layerWith creates service directories and preserves an explicit tmp", async () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "opencode-global-layer-"))
+    const directories = {
+      data: path.join(root, "data"),
+      config: path.join(root, "config"),
+      state: path.join(root, "state"),
+      log: path.join(root, "log"),
+      bin: path.join(root, "bin"),
+      repos: path.join(root, "repos"),
+      tmp: path.join(root, "nested", "..", "tmp"),
+    }
+
+    const context = await Effect.runPromise(Effect.scoped(Layer.build(Global.layerWith(directories))))
+
+    Object.values(directories).forEach((directory) => expect(fs.statSync(directory).isDirectory()).toBe(true))
+    expect(Context.get(context, Global.Service).tmp).toBe(directories.tmp)
+    fs.rmSync(root, { recursive: true, force: true })
+  })
+
+  test("building a layer with default tmp creates and canonicalizes it", () => {
+    const root = fs.mkdtempSync(path.join(os.tmpdir(), "opencode-global-layer-"))
+    const directories = ["data", "cache", "config", "state", "tmp"].map((directory) => path.join(root, directory))
+    const module = pathToFileURL(path.join(import.meta.dir, "../src/global.ts")).href
+    const result = Bun.spawnSync({
+      cmd: [
+        process.execPath,
+        "-e",
+        `
+          import { Context, Effect, Layer } from "effect"
+          const { Global } = await import(${JSON.stringify(module)})
+          const context = await Effect.runPromise(Effect.scoped(Layer.build(Global.layerWith({}))))
+          process.stdout.write(Context.get(context, Global.Service).tmp)
+        `,
+      ],
+      cwd: path.join(import.meta.dir, ".."),
+      env: {
+        ...process.env,
+        XDG_DATA_HOME: directories[0],
+        XDG_CACHE_HOME: directories[1],
+        XDG_CONFIG_HOME: directories[2],
+        XDG_STATE_HOME: directories[3],
+        TMPDIR: directories[4],
+      },
+      stdout: "pipe",
+      stderr: "pipe",
+    })
+
+    expect(result.exitCode, result.stderr.toString()).toBe(0)
+    expect(result.stdout.toString()).toBe(fs.realpathSync(path.join(directories[4], "opencode")))
+    const created = [
+      path.join(directories[0], "opencode"),
+      path.join(directories[1], "opencode", "bin"),
+      path.join(directories[2], "opencode"),
+      path.join(directories[3], "opencode"),
+      path.join(directories[0], "opencode", "log"),
+      path.join(directories[0], "opencode", "repos"),
+      path.join(directories[4], "opencode"),
+    ]
+    created.forEach((directory) => expect(fs.statSync(directory).isDirectory()).toBe(true))
+    fs.rmSync(root, { recursive: true, force: true })
+  })
+})