Refactor npm config handling (#24565)

6 files changed, 164 insertions, 83 deletions

diff --git a/packages/core/package.json b/packages/core/package.json
index d18acf3c7..546dc576c 100644
--- a/packages/core/package.json
+++ b/packages/core/package.json
@@ -7,6 +7,7 @@
   "private": true,
   "scripts": {
     "test": "bun test",
+    "test:ci": "mkdir -p .artifacts/unit && bun test --timeout 30000 --reporter=junit --reporter-outfile=.artifacts/unit/junit.xml",
     "typecheck": "tsgo --noEmit"
   },
   "bin": {
diff --git a/packages/core/src/npm-config.ts b/packages/core/src/npm-config.ts
new file mode 100644
index 000000000..896bb8487
--- /dev/null
+++ b/packages/core/src/npm-config.ts
@@ -0,0 +1,40 @@
+export * as NpmConfig from "./npm-config"
+
+import { fileURLToPath } from "url"
+// @ts-expect-error npm does not publish types for this internal config API.
+import Config from "@npmcli/config"
+// @ts-expect-error npm does not publish types for this internal config API.
+import { definitions, flatten, nerfDarts, shorthands } from "@npmcli/config/lib/definitions/index.js"
+import { Effect } from "effect"
+
+const npmPath = fileURLToPath(new URL("..", import.meta.url))
+
+export const load = (dir: string) =>
+  Effect.tryPromise({
+    try: async () => {
+      const config = new Config({
+        npmPath,
+        cwd: dir,
+        env: { ...process.env },
+        argv: [process.execPath, process.execPath],
+        execPath: process.execPath,
+        platform: process.platform,
+        definitions,
+        flatten,
+        nerfDarts,
+        shorthands,
+        warn: false,
+      })
+      await config.load()
+      return config.flat as Record<string, unknown>
+    },
+    catch: (cause) => cause,
+  }).pipe(Effect.orElseSucceed(() => ({}) as Record<string, unknown>))
+
+export const registry = (dir: string) =>
+  load(dir).pipe(
+    Effect.map((config) => {
+      const registry = typeof config.registry === "string" ? config.registry : "https://registry.npmjs.org"
+      return registry.endsWith("/") ? registry.slice(0, -1) : registry
+    }),
+  )
diff --git a/packages/core/src/npm.ts b/packages/core/src/npm.ts
index a52e0a9a5..92e404276 100644
--- a/packages/core/src/npm.ts
+++ b/packages/core/src/npm.ts
@@ -1,22 +1,14 @@
 export * as Npm from "./npm"
 
 import path from "path"
-import { fileURLToPath } from "url"
 import npa from "npm-package-arg"
-import semver from "semver"
-// @ts-expect-error npm does not publish types for this internal config API.
-import Config from "@npmcli/config"
-// @ts-expect-error npm does not publish types for this internal config API.
-import { definitions, flatten, nerfDarts, shorthands } from "@npmcli/config/lib/definitions/index.js"
-import { Effect, Schema, Context, Layer, Option, FileSystem, Stream } from "effect"
+import { Effect, Schema, Context, Layer, Option, FileSystem } from "effect"
 import { NodeFileSystem } from "@effect/platform-node"
 import { AppFileSystem } from "./filesystem"
 import { Global } from "./global"
 import { EffectFlock } from "./util/effect-flock"
 import { makeRuntime } from "./effect/runtime"
-import { ChildProcess, ChildProcessSpawner } from "effect/unstable/process"
-
-import { CrossSpawnSpawner } from "./cross-spawn-spawner"
+import { NpmConfig } from "./npm-config"
 
 export class InstallFailedError extends Schema.TaggedErrorClass<InstallFailedError>()("NpmInstallFailedError", {
   add: Schema.Array(Schema.String).pipe(Schema.optional),
@@ -40,46 +32,18 @@ export interface Interface {
       }[]
     },
   ) => Effect.Effect<void, EffectFlock.LockError | InstallFailedError>
-  readonly outdated: (pkg: string, cachedVersion: string) => Effect.Effect<boolean>
   readonly which: (pkg: string, bin?: string) => Effect.Effect<Option.Option<string>>
 }
 
 export class Service extends Context.Service<Service, Interface>()("@opencode/Npm") {}
 
 const illegal = process.platform === "win32" ? new Set(["<", ">", ":", '"', "|", "?", "*"]) : undefined
-const npmPath = fileURLToPath(new URL("..", import.meta.url))
 
 export function sanitize(pkg: string) {
   if (!illegal) return pkg
   return Array.from(pkg, (char) => (illegal.has(char) || char.charCodeAt(0) < 32 ? "_" : char)).join("")
 }
 
-const loadOptions = (dir: string) =>
-  Effect.tryPromise({
-    try: async () => {
-      const config = new Config({
-        npmPath,
-        cwd: dir,
-        env: { ...process.env },
-        argv: [process.execPath, process.execPath],
-        execPath: process.execPath,
-        platform: process.platform,
-        definitions,
-        flatten,
-        nerfDarts,
-        shorthands,
-        warn: false,
-      })
-      await config.load()
-      return config.flat
-    },
-    catch: (cause) =>
-      new InstallFailedError({
-        cause,
-        dir,
-      }),
-  })
-
 const resolveEntryPoint = (name: string, dir: string): EntryPoint => {
   let entrypoint: Option.Option<string>
   try {
@@ -110,39 +74,13 @@ export const layer = Layer.effect(
     const global = yield* Global.Service
     const fs = yield* FileSystem.FileSystem
     const flock = yield* EffectFlock.Service
-    const spawner = yield* ChildProcessSpawner.ChildProcessSpawner
     const directory = (pkg: string) => path.join(global.cache, "packages", sanitize(pkg))
-    const runView = Effect.fnUntraced(function* (cmd: string[]) {
-      const handle = yield* spawner.spawn(
-        ChildProcess.make(cmd[0], cmd.slice(1), {
-          extendEnv: true,
-        }),
-      )
-      const [stdout, stderr] = yield* Effect.all(
-        [Stream.mkString(Stream.decodeText(handle.stdout)), Stream.mkString(Stream.decodeText(handle.stderr))],
-        { concurrency: 2 },
-      )
-      const code = yield* handle.exitCode
-      if (code !== 0 || !stdout.trim()) {
-        return yield* Effect.fail(stderr || stdout || `Failed to run ${cmd.join(" ")}`)
-      }
-      return yield* Schema.decodeUnknownEffect(Schema.fromJsonString(Schema.String))(stdout)
-    }, Effect.scoped)
-    const viewLatestVersion = Effect.fnUntraced(function* (pkg: string) {
-      return yield* runView(["npm", "view", pkg, "dist-tags.latest", "--json"]).pipe(
-        Effect.catch(() =>
-          runView(["pnpm", "view", pkg, "dist-tags.latest", "--json"]).pipe(
-            Effect.catch(() => runView(["bun", "pm", "view", pkg, "dist-tags.latest", "--json"])),
-          ),
-        ),
-      )
-    })
     const reify = (input: { dir: string; add?: string[] }) =>
       Effect.gen(function* () {
         yield* flock.acquire(`npm-install:${input.dir}`)
         const { Arborist } = yield* Effect.promise(() => import("@npmcli/arborist"))
         const add = input.add ?? []
-        const npmOptions = yield* loadOptions(input.dir)
+        const npmOptions = yield* NpmConfig.load(input.dir)
         const arborist = new Arborist({
           ...npmOptions,
           path: input.dir,
@@ -172,18 +110,6 @@ export const layer = Layer.effect(
         }),
       )
 
-    const outdated = Effect.fn("Npm.outdated")(function* (pkg: string, cachedVersion: string) {
-      const latestVersion = yield* viewLatestVersion(pkg).pipe(Effect.option)
-      if (Option.isNone(latestVersion)) {
-        return false
-      }
-
-      const range = /[\s^~*xX<>|=]/.test(cachedVersion)
-      if (range) return !semver.satisfies(latestVersion.value, cachedVersion)
-
-      return semver.lt(cachedVersion, latestVersion.value)
-    })
-
     const add = Effect.fn("Npm.add")(function* (pkg: string) {
       const dir = directory(pkg)
       const name = (() => {
@@ -309,7 +235,6 @@ export const layer = Layer.effect(
     return Service.of({
       add,
       install,
-      outdated,
       which,
     })
   }),
@@ -320,7 +245,6 @@ export const defaultLayer = layer.pipe(
   Layer.provide(AppFileSystem.layer),
   Layer.provide(Global.layer),
   Layer.provide(NodeFileSystem.layer),
-  Layer.provide(CrossSpawnSpawner.defaultLayer),
 )
 
 const { runPromise } = makeRuntime(Service, defaultLayer)
@@ -337,10 +261,6 @@ export async function add(...args: Parameters<Interface["add"]>) {
   }
 }
 
-export async function outdated(...args: Parameters<Interface["outdated"]>) {
-  return runPromise((svc) => svc.outdated(...args))
-}
-
 export async function which(...args: Parameters<Interface["which"]>) {
   const resolved = await runPromise((svc) => svc.which(...args))
   return Option.getOrUndefined(resolved)
diff --git a/packages/core/test/fixture/tmpdir.ts b/packages/core/test/fixture/tmpdir.ts
new file mode 100644
index 000000000..950b1401b
--- /dev/null
+++ b/packages/core/test/fixture/tmpdir.ts
@@ -0,0 +1,13 @@
+import fs from "fs/promises"
+import { tmpdir as osTmpdir } from "os"
+import path from "path"
+
+export const tmpdir = async () => {
+  const dir = await fs.mkdtemp(path.join(osTmpdir(), "opencode-core-test-"))
+  return {
+    path: dir,
+    async [Symbol.asyncDispose]() {
+      await fs.rm(dir, { recursive: true, force: true })
+    },
+  }
+}
diff --git a/packages/core/test/npm-config.test.ts b/packages/core/test/npm-config.test.ts
new file mode 100644
index 000000000..895b35db5
--- /dev/null
+++ b/packages/core/test/npm-config.test.ts
@@ -0,0 +1,51 @@
+import path from "path"
+import { describe, expect, test } from "bun:test"
+import { Effect } from "effect"
+import { NpmConfig } from "@opencode-ai/core/npm-config"
+import { tmpdir } from "./fixture/tmpdir"
+
+describe("NpmConfig.load", () => {
+  test("reads registry from project .npmrc", async () => {
+    await using tmp = await tmpdir()
+    await Bun.write(path.join(tmp.path, ".npmrc"), "registry=https://registry.example.test/\n")
+
+    const config = await Effect.runPromise(NpmConfig.load(tmp.path))
+
+    expect(config.registry).toBe("https://registry.example.test/")
+  })
+
+  test("reads scoped registries from project .npmrc", async () => {
+    await using tmp = await tmpdir()
+    await Bun.write(path.join(tmp.path, ".npmrc"), "@acme:registry=https://npm.acme.test/\n")
+
+    const config = await Effect.runPromise(NpmConfig.load(tmp.path))
+
+    expect(config["@acme:registry"]).toBe("https://npm.acme.test/")
+  })
+
+  test("flattens boolean and list options", async () => {
+    await using tmp = await tmpdir()
+    await Bun.write(path.join(tmp.path, ".npmrc"), "ignore-scripts=true\nomit[]=dev\nomit[]=optional\n")
+
+    const config = await Effect.runPromise(NpmConfig.load(tmp.path))
+
+    expect(config.ignoreScripts).toBe(true)
+    expect(config.omit).toEqual(["dev", "optional"])
+  })
+})
+
+describe("NpmConfig.registry", () => {
+  test("normalizes configured registry without trailing slash", async () => {
+    await using tmp = await tmpdir()
+    await Bun.write(path.join(tmp.path, ".npmrc"), "registry=https://registry.example.test/\n")
+
+    await expect(Effect.runPromise(NpmConfig.registry(tmp.path))).resolves.toBe("https://registry.example.test")
+  })
+
+  test("leaves configured registry without trailing slash unchanged", async () => {
+    await using tmp = await tmpdir()
+    await Bun.write(path.join(tmp.path, ".npmrc"), "registry=https://registry.example.test\n")
+
+    await expect(Effect.runPromise(NpmConfig.registry(tmp.path))).resolves.toBe("https://registry.example.test")
+  })
+})
diff --git a/packages/core/test/npm.test.ts b/packages/core/test/npm.test.ts
new file mode 100644
index 000000000..3e94a0869
--- /dev/null
+++ b/packages/core/test/npm.test.ts
@@ -0,0 +1,56 @@
+import fs from "fs/promises"
+import path from "path"
+import { describe, expect, test } from "bun:test"
+import { Npm } from "@opencode-ai/core/npm"
+import { tmpdir } from "./fixture/tmpdir"
+
+const win = process.platform === "win32"
+
+const writePackage = (dir: string, pkg: Record<string, unknown>) =>
+  Bun.write(
+    path.join(dir, "package.json"),
+    JSON.stringify({
+      version: "1.0.0",
+      ...pkg,
+    }),
+  )
+
+describe("Npm.sanitize", () => {
+  test("keeps normal scoped package specs unchanged", () => {
+    expect(Npm.sanitize("@opencode/acme")).toBe("@opencode/acme")
+    expect(Npm.sanitize("@opencode/[email protected]")).toBe("@opencode/[email protected]")
+    expect(Npm.sanitize("prettier")).toBe("prettier")
+  })
+
+  test("handles git https specs", () => {
+    const spec = "acme@git+https://github.com/opencode/acme.git"
+    const expected = win ? "acme@git+https_//github.com/opencode/acme.git" : spec
+    expect(Npm.sanitize(spec)).toBe(expected)
+  })
+})
+
+describe("Npm.install", () => {
+  test("respects omit from project .npmrc", async () => {
+    await using tmp = await tmpdir()
+
+    await writePackage(tmp.path, {
+      name: "fixture",
+      dependencies: {
+        "prod-pkg": "file:./prod-pkg",
+      },
+      devDependencies: {
+        "dev-pkg": "file:./dev-pkg",
+      },
+    })
+    await Bun.write(path.join(tmp.path, ".npmrc"), "omit=dev\n")
+    await fs.mkdir(path.join(tmp.path, "prod-pkg"))
+    await fs.mkdir(path.join(tmp.path, "dev-pkg"))
+    await writePackage(path.join(tmp.path, "prod-pkg"), { name: "prod-pkg" })
+    await writePackage(path.join(tmp.path, "dev-pkg"), { name: "dev-pkg" })
+
+    await Npm.install(tmp.path)
+
+    await expect(fs.stat(path.join(tmp.path, "node_modules", "prod-pkg"))).resolves.toBeDefined()
+    await expect(fs.stat(path.join(tmp.path, "node_modules", "dev-pkg"))).rejects.toThrow()
+  })
+})