Code Review

This pull request refactors the codebase by migrating core functionality to the @opencode-ai/core package, introducing new services for OpenTelemetry observability and npm package management using @npmcli/arborist. The changes include centralized path handling, process metadata utilities, and a significant update to dependency management. Feedback identifies several issues: top-level filesystem side effects in the global module, incorrect package resolution logic for Node.js, and fragile parsing of OpenTelemetry resource attributes. Furthermore, the review suggests avoiding silent error suppression in npm configuration loading and removing unexpected side effects, like lock file deletion, from the package lookup utility.

PR #266 Review — Core consolidation follow-ups

Reviewed the full diff (159 files, +975 / -574) focusing on behavioral regressions, boundary conditions, and PawWork carve-out integrity. Findings below, sorted P0→P3.

P0 — Must fix before merge

1. global.ts top-level side effects on import — xdg ! non-null assertions can silently produce "undefined/pawwork" paths

packages/core/src/global.ts evaluates xdgData!, xdgCache!, xdgConfig!, xdgState! at module top level. On Linux systems or containers where these XDG env vars are unset, xdgData etc. are undefined, and path.join(undefined!, "pawwork") produces "undefined/pawwork" — silently, with no error. The old code had the same ! assertions but they ran inside an Effect Layer where a failure would propagate. Now the bad path is baked into Path.data etc. at import time with no recovery. Both the core global.ts and the opencode local global/index.ts have this issue.

The top-level await Promise.all([fs.mkdir(...)]) and Flock.setGlobal() also execute on every import. If any consumer imports Log from @opencode-ai/core/util/log (which chains to ../global), directory creation and flock wiring happen as a side effect of that import. This is a contract change from the old code where global.ts was side-effect-free at import time.

2. Npm.add() skips loadVirtual() — corrupted package dirs treated as valid

Old packages/opencode/src/npm/index.ts calls arborist.loadVirtual() first to verify tree integrity; only if that fails or the package is absent does it call reify(). This means a partially-installed or corrupted node_modules gets repaired on the next add().

New packages/core/src/npm.ts replaces this with afs.existsSafe(dir) — if the directory exists, it returns the cached entry point immediately without verifying the tree. A corrupted install (e.g. interrupted reify(), deleted node_modules inner files) will be treated as valid, and LSP servers / plugins will fail at spawn time with no self-repair path.

P1 — Should fix before merge (user-facing regression or high risk)

3. emitLspInstallFailedIfApplicable() removed — LSP install failure UI notification is dead

The old npm/index.ts published LSP.Event.InstallFailed to the Bus when an LSP server package failed to install. The packages/opencode/src/lsp/index.ts still defines Event.InstallFailed and still checks err instanceof Npm.InstallFailedError, but the new core npm.ts never publishes this event — it can't, since core has no knowledge of LSP or Bus. The SDK type EventLspServerInstallFailed still exists. The result: LSP install failures will be caught by the cooldown logic in lsp/index.ts, but no InstallFailed Bus event is ever fired, so any UI that renders a toast or notification for this event is dead code.

4. InstallFailedError schema changed — { pkg: string } → { add?: string[], dir: string, cause?: Defect }

packages/opencode/src/lsp/index.ts line checks err instanceof Npm.InstallFailedError — this will still match because the class name is the same. However, any code that reads err.payload.pkg (or err.pkg) will get undefined instead of the package name. The LSP error handler doesn't read the payload today, but the Bus event schema z.object({ pkg: z.string(), error: z.string() }) expects pkg which no longer exists on the error object. If anyone wires up the event again, the Zod validation will fail.

5. serviceName: "opencode" hardcoded in observability.ts — PawWork traces report as "opencode"

packages/core/src/effect/observability.ts resource() returns serviceName: "opencode". PawWork's OTel traces and logs will be attributed to the "opencode" service. This should use Runtime.appName() like the Global carve-out does, so PawWork reports as "pawwork". If this is intentional for upstream compatibility it should be documented in the carve-out list.

6. Dual Global module — cache version check TOCTOU race

Both packages/core/src/global.ts and packages/opencode/src/global/index.ts independently run the cache version check (read version → if mismatch, readdir + rm all cache contents → write new version). When both modules are loaded in the same process (which happens — core is imported by e.g. @opencode-ai/core/util/log, and opencode's local global/index.ts is imported by ~12 still-unmigrated files), these two checks race. If the cache version is being bumped, one module could delete files the other is reading, or write the version file before the other finishes cleaning. Low frequency (only fires on version bumps) but can corrupt cache state.

P2 — Should fix soon (maintainability / correctness)

7. Windows Bun workaround removed — __FAKE_PLATFORM__ = "linux" gone

Old npm/index.ts sets process.env.__FAKE_PLATFORM__ = "linux" before importing Arborist on Windows, because "Bun on Windows does not support the UV_FS_O_FILEMAP flag used by tar." New npm.ts does a plain import("@npmcli/arborist") without this workaround. If PawWork supports Windows + Bun (the project is a macOS + Windows desktop product per AGENTS.md), npm package installation may fail on Windows.

8. NpmConfig.load() silently swallows all errors → returns {}

packages/core/src/npm-config.ts catches all exceptions in load() and returns an empty object. This means authentication errors (bad .npmrc tokens), misconfigured registries, or corrupt config files are silently ignored. The caller proceeds with default registry + no auth, producing confusing "401 Unauthorized" errors downstream instead of an upfront clear failure.

9. Npm.outdated() removed with no replacement

The old Npm.outdated(pkg, cachedVersion) function checked the npm registry for newer versions. While no current code calls it, it was a public export of the Npm namespace. Any future code that needs version checking will need to re-implement this.

10. Inconsistent import styles for CrossSpawnSpawner and EffectLogger

Some files use import * as CrossSpawnSpawner from "@opencode-ai/core/cross-spawn-spawner" while others use import { CrossSpawnSpawner } from "...". Both produce the same runtime result because the module uses export * as CrossSpawnSpawner from "./cross-spawn-spawner", but the inconsistency is a maintenance hazard. Same for EffectLogger — import * as EffectLogger vs import { EffectLogger }. Recommend standardizing on { CrossSpawnSpawner } / { EffectLogger } since those are the explicit named namespace exports.

11. install() writable check silently masks permission errors

New npm.ts install() checks canWrite and returns early if false. Old code attempted the operation regardless. If a directory exists but is read-only (common in CI, Docker, or restricted environments), the install silently does nothing instead of surfacing the permission problem. The caller gets no error and no indication that dependencies weren't installed.

P3 — Nice to have / low risk

12. types.d.ts removal drops Bun global type declaration

packages/core/src/types.d.ts had declare var Bun: ... | undefined. If @types/bun isn't configured for the core package, other files referencing the Bun global may get implicit any. The new npm.ts uses typeof Bun !== "undefined" which works at runtime, but type safety is reduced.

13. Npm.which() behavioral change — returns undefined instead of falling back to first binary

Old which() with no bin parameter: if the requested binary isn't found, falls through to add() and returns whatever pick() resolves. New which() with bin parameter: if the specific bin isn't in .bin/, returns Option.none(). This is actually better behavior (explicit failure vs silent fallback), but is a behavioral change that could surprise callers.

14. Half-migrated Global imports — 12 opencode source files still import from local ../global

Files like src/storage/db.ts, src/snapshot/index.ts, src/lsp/server.ts, etc. still import Global from the local opencode module rather than @opencode-ai/core/global. This means both Global modules are loaded in every process, persisting the dual-module risks (#6, #10). Completing the migration and deleting the local global/index.ts (or making it re-export from core) would eliminate the entire class of dual-Global issues.

15. mergeWithExisting: false in observability logs is a behavioral change from old loggerMergeWithExisting: true

Old packages/opencode/src/effect/observability.ts used loggerMergeWithExisting: true. New code uses mergeWithExisting: false. The new code explicitly composes EffectLogger.logger + OtlpLogger.make() so both are present, but if any downstream code added custom loggers to the Effect runtime independently, they'd be dropped when OTel is enabled. The non-OTel path is unaffected.

16. setGlobalContextManager is a global singleton mutation in traces()

packages/core/src/effect/observability.ts calls context.setGlobalContextManager(mgr). This cannot be undone and will throw or silently overwrite if called more than once. In test environments that import this module multiple times or across test isolation boundaries, this could cause failures.

PR #266 Review — Supplemental findings (round 2)

Deeper inspection of barrel files, dead code, cross-module dependency chains, and boundary conditions missed in the first pass.

P1 — Should fix before merge

17. resource() hardcodes ensureProcessMetadata("main") — worker processes get wrong OTel process role

packages/core/src/effect/observability.ts:281 calls ensureProcessMetadata("main") every time resource() is evaluated. If a worker process sets OPENCODE_PROCESS_ROLE = "worker" via ensureProcessRole("worker") BEFORE the observability layer is built for the first time, the ??= operator will leave "worker" intact and the role is correct. But if the observability layer is built first (e.g., during app startup), ??= will set the env var to "main" permanently, and the worker's OTel data will be incorrectly labeled as opencode.process_role: main.

The fix: either don't call ensureProcessMetadata in resource() (use the env var directly with a fallback), or pass a role parameter through the layer construction chain.

P2 — Should fix soon

18. packages/opencode/src/effect/oltp.ts is dead code — duplicate Observability namespace ghost

After packages/opencode/src/effect/observability.ts was deleted (replaced by core), the sibling file oltp.ts still exists in opencode with its own export namespace Observability. No code imports from it anymore — both the barrel (packages/opencode/src/effect/index.ts) and run-service.ts now import from @opencode-ai/core/effect/observability. If any future code accidentally imports Observability from @/effect/oltp, it would get a stale, different implementation with no trace support and the old loggerMergeWithExisting: true behavior.

Recommendation: delete packages/opencode/src/effect/oltp.ts.

19. sanitize() is a no-op on non-Windows — no path traversal guard for .. in package names

packages/core/src/npm.ts:sanitize() returns the input unchanged on macOS/Linux. The directory() function does path.join(global.cache, "packages", sanitize(pkg)). On non-Windows, a crafted package name containing path traversal sequences (e.g., ../../etc passed as the pkg argument) would escape the cache directory. While npa() and Arborist validate package names against npm registry rules, the internal directory() function doesn't enforce this. A second defense-in-depth guard (e.g., path.basename(pkg) === pkg check) would be safer.

20. packages/core/src/util/log.ts triggers core global.ts top-level side effects on import

The core log.ts does import * as Global from "../global" at module scope. Since core/global.ts has top-level await (directory creation + cache version check), importing Log from @opencode-ai/core/util/log now creates directories as a side effect. This is a contract change from the old packages/opencode/src/util/log.ts which imported Global from the old opencode global that also had top-level directory creation — so this was already a side-effecting import. However, the new import chain is longer (Log → core/global → Flock → Hash, including the Flock.setGlobal() call), and the xdg ! non-null assertions in core/global can now fail silently if xdgData is undefined.

21. packages/opencode/src/effect/index.ts barrel and oltp.ts export conflicting Observability namespaces

// barrel: packages/opencode/src/effect/index.ts
export { Observability } from "@opencode-ai/core/effect/observability"  // has traces + new setup

// dead file: packages/opencode/src/effect/oltp.ts
export namespace Observability { ... }  // old Otlp.layerJson, no traces

TypeScript won't complain about this since nothing imports from oltp.ts. But if the barrel is the only import path, it'll always resolve to core. Risk: a search-and-replace or IDE auto-import could inadvertently wire up the dead oltp.ts Observability.

P3 — Nice to have

22. loggerExportInterval: Duration.seconds(1) removed — log batching behavior may change

The old packages/opencode/src/effect/observability.ts used Otlp.layerJson({ loggerExportInterval: Duration.seconds(1), ... }). The new core observability.ts uses OtlpLogger.make({ url, resource, headers }) without an explicit export interval. The default interval for OtlpLogger may differ (likely 5s or 30s depending on the Effect version). This could change OTel log delivery latency for PawWork telemetry.

23. resource() not memoized — called every time logs() is invoked

logs() creates a new Logger.layer with OtlpLogger.make({ url, resource: resource(), headers }). Each call to resource() re-computes the attributes object (including ensureProcessMetadata, OTEL_RESOURCE_ATTRIBUTES parsing, and the spread). While ensureProcessMetadata is idempotent (??=), the OTEL_RESOURCE_ATTRIBUTES parsing and object creation happen fresh each time. This is low-impact (happens once per runtime construction) but needless work.

24. packages/core/package.json has "bin": { "opencode": "./bin/opencode" } — wrong for a library package

The core package exports "bin": { "opencode": "./bin/opencode" }. Since @opencode-ai/core is a dependency library (not a directly-installed binary), this bin entry has no effect unless someone npm install -g @opencode-ai/core or another package hoists it. Harmless but misleading — if this bin entry is intended for the opencode package, it should be in packages/opencode/package.json instead.

25. ensureProcessMetadata("main") hardcoded in observability.ts but opencode-process.ts API expects a fallback parameter

ensureProcessMetadata(fallback) accepts "main" | "worker". The observability code hardcodes "main". If the observability layer is used in a worker process where OPENCODE_PROCESS_ROLE is already "worker", the ??= operator preserves the existing value and the hardcoded "main" is just a fallback. However, the fallback parameter name implies it should match the caller's context — if a worker builds the layer, the semantic fallback should be "worker", not "main". Currently this is fine because the observability layer is only built in the main process, but it's fragile documentation.

26. @effect/opentelemetry catalog entry added to root but @opentelemetry/context-async-hooks is a direct dependency in core/package.json

The root package.json adds "@effect/opentelemetry": "4.0.0-beta.46" to the catalog. The core package.json lists @opentelemetry/context-async-hooks: 2.6.1 as a direct dependency (not catalog). This is fine since it's a transitive peer, but if this dependency is moved to the catalog in the future, the version pin needs to stay synchronized.

27. Self-referential export * as Global from "./global" in core/global.ts could confuse TypeScript autocompletion

The pattern creates Global.Global.Global... in IntelliSense. For developers working in the core package, autocomplete will show the recursive self-reference as a valid path. Purely cosmetic but worth standardizing — either use export namespace Global { ... } (old style) or rename the re-export label.

PR #266 Inline Review — P0–P3 summary

10 inline comments posted across 3 files covering critical issues. Full summary below.

Inline comments posted

packages/core/src/effect/observability.ts (6 comments)

• L26: P1 ensureProcessMetadata("main") hardcoded — worker process role mislabel
• L44: P1 serviceName: "opencode" hardcoded — PawWork traces misattributed
• L61: P2 loggerExportInterval removed — log batching behavior change
• L67: P2 mergeWithExisting: false — behavioral change from old code

packages/core/src/npm.ts (6 comments)

• L14: P1 InstallFailedError schema change breaks Bus event validation
• L43: P2 sanitize() no-op on non-Windows — no path traversal guard
• L82: P2 __FAKE_PLATFORM__ workaround removed — Windows+Bun may fail
• L117: P2 loadVirtual() removed — corrupted packages not self-repaired
• L139: P2 canWrite early return silently masks permission errors

packages/core/src/npm-config.ts (1 comment)

• L13: P2 NpmConfig.load() silently swallows all errors

Additional findings not covered by inline comments

• P0: global.ts xdg ! non-null assertions produce "undefined/pawwork" paths when XDG vars are unset
• P0: Top-level await in global.ts triggers FS side effects on import — changes module border contract
• P1: emitLspInstallFailedIfApplicable() removed — LSP install failure UI notification is dead code
• P1: Dual Global module cache version check TOCTOU race when both core and opencode globals load
• P2: packages/opencode/src/effect/oltp.ts is dead code with conflicting Observability namespace
• P3: Inconsistent * as vs { } import styles for CrossSpawnSpawner/EffectLogger
• P3: Core package.json carries "bin": { "opencode": ... } — wrong package for a binary

Quick action items

Review comments focused on current CI blocking regressions and PawWork boundary behavior.

Review Summary

This PR moves Global, log, flag, cross-spawn-spawner, npm, and observability into @opencode-ai/core. The mechanical import rewrites look correct, and the carve-outs for PawWork branding are well-documented. Below are inline-style comments organized by severity.

[P0] packages/core/src/global.ts — top-level await in module scope

The new global.ts performs top-level await Promise.all([fs.mkdir(...)]) and calls Flock.setGlobal({ state }) at import time. This is a significant behavior change from the previous Layer.effect-based lazy initialization.

• Boundary case: In test environments where OPENCODE_TEST_HOME is set after the module is first imported (e.g., via vitest/bun test preload patterns), the paths will already be computed against the real home directory. The previous Layer.effect deferred this until Effect.runSync or similar.
• Regression risk: Any test that imports @opencode-ai/core/global transitively before setting OPENCODE_TEST_HOME will pollute the real ~/.config/pawwork/ or ~/.cache/pawwork/ directories.
• Suggested fix: Move the eager mkdir and Flock.setGlobal into a Global.init() function that callers invoke explicitly, or keep them inside the Layer.effect so initialization is lazy and test-controllable.

[P0] packages/core/src/npm.ts — Npm.which swallows all errors silently

export async function which(...args) {
  const resolved = await runPromise((svc) => svc.which(...args))
  return Option.getOrUndefined(resolved)
}

The upstream which() implementation returns Option.none() on any failure (including InstallFailedError, EffectFlock.LockError, file system errors, etc.). The PR description acknowledges this and adds npmWhich() in lsp/server.ts to re-emit InstallFailedError.

• Boundary case: Non-LSP callers of Npm.which (e.g., Formatter in packages/opencode/src/format/formatter.ts, Provider in provider/provider.ts) now lose all failure signals. Previously, Npm.which in the old packages/opencode/src/npm/index.ts would throw on add() failure, letting callers handle it.
• Regression risk: Formatter or provider resolution may silently fail without any log or user feedback. The npmWhich wrapper only covers LSP servers.
• Suggested fix: Either restore error propagation in Npm.which, or audit all non-LSP callers to ensure silent failure is acceptable.

[P1] packages/core/src/npm.ts — loadArborist mutates process.env globally

if (process.platform === "win32") {
  process.env.__FAKE_PLATFORM__ = "linux"
}

This is set at module load time inside loadArborist(), which is called lazily. However, once set, it persists for the entire process.

• Boundary case: If PawWork later needs to run actual tar operations on Windows (e.g., packaging, extracting user archives), __FAKE_PLATFORM__=linux may cause incorrect behavior in those tar instances.
• Regression risk: Global env mutation is hard to trace. If another dependency also uses tar and expects Windows behavior, it will be silently broken.
• Suggested fix: Consider scoping this to the Arborist child process only, or documenting it prominently as a known process-wide side effect.

[P1] packages/core/src/effect/observability.ts — serviceName: Runtime.appName() but opencode.client attribute remains

The carve-out changes serviceName from hardcoded "opencode" to Runtime.appName() (which returns "pawwork"). However, the OTel resource attributes still include "opencode.client", "opencode.process_role", "opencode.run_id".

• Boundary case: If PawWork ever sends telemetry to an upstream opencode collector, the serviceName will say "pawwork" but the attributes will still say "opencode". This is inconsistent branding.
• Regression risk: Low for functionality, but medium for telemetry attribution confusion.
• Suggested fix: Either rename the attribute keys to pawwork.* or add a comment explaining why they remain opencode.* (e.g., upstream schema compatibility).

[P1] packages/core/src/npm.ts — InstallFailedError schema change breaks external consumers

The error shape changed from { pkg: string } to { add: string[], dir: string, cause: Schema.Defect }. The PR updates the SDK types and OpenAPI snapshot, which is good.

• Boundary case: Any external SDK consumer (third-party integrations, scripts) that parsed properties.pkg will break. The OpenAPI spec change is a breaking API change.
• Regression risk: If there are downstream consumers outside this repo (e.g., cloud control plane, mobile app), they need coordinated updates.
• Suggested fix: Add a note to the PR description or a migration doc about the breaking SDK change. Consider whether pkg should be kept as a deprecated alias for backward compatibility.

[P2] packages/core/src/global.ts — xdg-basedir modules may return undefined

const data = path.join(xdgData!, app)

The code uses non-null assertion (!) on xdgData, xdgCache, etc. In exotic environments (Windows without WSL, containers without XDG vars, certain CI images), these can return undefined.

• Boundary case: On Windows or minimal Docker containers, xdg-basedir may return undefined for some paths, causing path.join(undefined, "pawwork") to throw.
• Regression risk: Crash on startup in unsupported environments. The old code had the same issue, so this is not a new regression, but the move to core makes it harder to patch locally.
• Suggested fix: Add fallback values (e.g., path.join(os.homedir(), ".local", "share")) or at least a runtime check with a clear error message.

[P2] packages/core/src/npm.ts — resolveEntryPoint uses import.meta.resolve with Bun-only guard

const resolved = typeof Bun !== "undefined" ? import.meta.resolve(name, dir) : import.meta.resolve(dir)

• Boundary case: In Node.js (non-Bun) environments, import.meta.resolve(dir) resolves the directory itself, not the package name. This is likely incorrect — it should resolve name with dir as the parent.
• Regression risk: If PawWork ever runs on Node.js (e.g., for debugging, testing, or future portability), resolveEntryPoint will return wrong paths.
• Suggested fix: Use require.resolve(name, { paths: [dir] }) for Node.js, or document that this module is Bun-only.

[P2] packages/core/src/npm-config.ts — imports from @npmcli/config/lib/definitions/index.js

import { definitions, flatten, nerfDarts, shorthands } from "@npmcli/config/lib/definitions/index.js"

This imports from a deep internal path that npm does not guarantee as stable API.

• Boundary case: A future npm patch release could move or rename this file, breaking the build.
• Regression risk: Build breakage on bun install when npm updates. The // @ts-expect-error comment acknowledges this, but it doesn't prevent runtime breakage.
• Suggested fix: Pin @npmcli/config to an exact version in package.json (already done: 10.8.1), and add a CI check or dependabot rule to block automatic minor/patch bumps for this package.

[P2] packages/core/src/util/opencode-process.ts — env var names still say OPENCODE

export const OPENCODE_RUN_ID = "OPENCODE_RUN_ID"
export const OPENCODE_PROCESS_ROLE = "OPENCODE_PROCESS_ROLE"

These are now in @opencode-ai/core but the env var names are still OPENCODE_*. This is inconsistent with the PawWork carve-out philosophy.

• Boundary case: If a user runs both opencode and pawwork on the same machine, these env vars will collide.
• Regression risk: Very low, but it’s a branding inconsistency that will need to be carved out in every future sync.
• Suggested fix: Add these to the permanent carve-out list and rename to PAWWORK_RUN_ID / PAWWORK_PROCESS_ROLE, or document why they must stay OPENCODE_*.

[P2] packages/core/test/npm.test.ts — test uses Bun.write and fs.stat but no cleanup on failure

await Bun.write(path.join(tmp.path, ".npmrc"), "omit=dev\n")

The tmpdir helper uses Symbol.asyncDispose, which is good. But if Bun.write or Npm.install throws, the using block should still dispose. However, Bun.write is not wrapped in try/finally.

• Boundary case: Not a real issue because using handles disposal, but Bun.write may leave partial files if interrupted.
• Regression risk: Minimal — test-only.
• Suggested fix: None critical, but consider using fs.writeFile for consistency with the rest of the test suite (the cross-spawn test already switched from Bun.write to fs.writeFile).

[P3] packages/core/src/npm.ts — Npm.install early-return on !canWrite without logging

if (!canWrite) return

If the target directory is not writable, Npm.install silently returns without installing anything and without logging.

• Boundary case: Read-only project directories (e.g., mounted volumes, protected workspaces).
• Regression risk: Users may wonder why dependencies are missing with no error.
• Suggested fix: Add a log.warn or similar before the early return.

[P3] packages/core/package.json — overrides: { "drizzle-orm": "catalog:" } unexplained

The new overrides field in packages/core/package.json pins drizzle-orm to the catalog version. core does not appear to depend on drizzle-orm directly.

• Boundary case: This may be a transitive override to resolve a version conflict elsewhere in the workspace.
• Regression risk: If the override is unnecessary, it adds maintenance burden. If it is necessary, it should be documented.
• Suggested fix: Add a comment in the PR or a package.json comment explaining why this override exists.

[P3] packages/core/src/effect/observability.ts — AsyncLocalStorageContextManager registered globally without cleanup

const mgr = new AsyncLocalStorageContextManager()
mgr.enable()
context.setGlobalContextManager(mgr)

This is called inside the traces() async function, which is invoked once when the observability layer is built. There is no mgr.disable() or cleanup on process exit.

• Boundary case: In test suites that create and tear down multiple runtimes, the global context manager may accumulate state or interfere with other tests.
• Regression risk: Test flakiness in observability-enabled test runs.
• Suggested fix: Add a cleanup hook or document that observability layer should not be rebuilt in tests.

[P3] packages/app/src/context/global-sync.tsx — add?.[0] fallback may show empty string

pkg: properties?.add?.[0] ?? "",

If add is an empty array, add?.[0] is undefined, so the toast description will show an empty package name.

• Boundary case: InstallFailedError constructed with add: [] (theoretically possible via the schema since add is optional).
• Regression risk: User sees "Failed to install " with no package name.
• Suggested fix: Use properties?.add?.[0] ?? properties?.dir ?? "unknown" for a better fallback.

[P3] packages/core/src/npm.ts — reify uses Effect.withSpan but add is not sanitized in span attributes

Effect.withSpan("Npm.reify", { attributes: input })

If input.add contains a scoped package with a registry token (e.g., @acme/pkg@npm:registry.example.com/pkg?authToken=secret), the token will be emitted into the OTel span.

• Boundary case: Private registry URLs with embedded credentials.
• Regression risk: Credential leakage in telemetry.
• Suggested fix: Sanitize or redact add entries before putting them in span attributes.

[P3] packages/opencode/src/lsp/server.ts — npmWhich catches Npm.InstallFailedError but re-throws other errors

if (err instanceof Npm.InstallFailedError) { ... }
throw err

If Npm.add throws a non-InstallFailedError (e.g., EffectFlock.LockError, unexpected runtime error), it bubbles up and may crash the LSP server initialization.

• Boundary case: File system lock contention or disk-full errors.
• Regression risk: LSP server crash instead of graceful degradation.
• Suggested fix: Consider catching all errors in npmWhich, logging them, and returning undefined to match the "which returns undefined on failure" contract.

Review Summary

This PR moves Global, log, flag, cross-spawn-spawner, npm, and observability into @opencode-ai/core. The mechanical import rewrites look correct, and the carve-outs for PawWork branding are well-documented. Below are inline-style comments organized by severity.

[P0] packages/core/src/global.ts — top-level await in module scope

The new global.ts performs top-level await Promise.all([fs.mkdir(...)]) and calls Flock.setGlobal({ state }) at import time. This is a significant behavior change from the previous Layer.effect-based lazy initialization.

• Boundary case: In test environments where OPENCODE_TEST_HOME is set after the module is first imported (e.g., via vitest/bun test preload patterns), the paths will already be computed against the real home directory. The previous Layer.effect deferred this until Effect.runSync or similar.
• Regression risk: Any test that imports @opencode-ai/core/global transitively before setting OPENCODE_TEST_HOME will pollute the real ~/.config/pawwork/ or ~/.cache/pawwork/ directories.
• Suggested fix: Move the eager mkdir and Flock.setGlobal into a Global.init() function that callers invoke explicitly, or keep them inside the Layer.effect so initialization is lazy and test-controllable.

[P0] packages/core/src/npm.ts — Npm.which swallows all errors silently

export async function which(...args) {
  const resolved = await runPromise((svc) => svc.which(...args))
  return Option.getOrUndefined(resolved)
}

The upstream which() implementation returns Option.none() on any failure (including InstallFailedError, EffectFlock.LockError, file system errors, etc.). The PR description acknowledges this and adds npmWhich() in lsp/server.ts to re-emit InstallFailedError.

• Boundary case: Non-LSP callers of Npm.which (e.g., Formatter in packages/opencode/src/format/formatter.ts, Provider in provider/provider.ts) now lose all failure signals. Previously, Npm.which in the old packages/opencode/src/npm/index.ts would throw on add() failure, letting callers handle it.
• Regression risk: Formatter or provider resolution may silently fail without any log or user feedback. The npmWhich wrapper only covers LSP servers.
• Suggested fix: Either restore error propagation in Npm.which, or audit all non-LSP callers to ensure silent failure is acceptable.

[P1] packages/core/src/npm.ts — loadArborist mutates process.env globally

if (process.platform === "win32") {
  process.env.__FAKE_PLATFORM__ = "linux"
}

This is set at module load time inside loadArborist(), which is called lazily. However, once set, it persists for the entire process.

• Boundary case: If PawWork later needs to run actual tar operations on Windows (e.g., packaging, extracting user archives), __FAKE_PLATFORM__=linux may cause incorrect behavior in those tar instances.
• Regression risk: Global env mutation is hard to trace. If another dependency also uses tar and expects Windows behavior, it will be silently broken.
• Suggested fix: Consider scoping this to the Arborist child process only, or documenting it prominently as a known process-wide side effect.

[P1] packages/core/src/effect/observability.ts — serviceName: Runtime.appName() but opencode.client attribute remains

The carve-out changes serviceName from hardcoded "opencode" to Runtime.appName() (which returns "pawwork"). However, the OTel resource attributes still include "opencode.client", "opencode.process_role", "opencode.run_id".

• Boundary case: If PawWork ever sends telemetry to an upstream opencode collector, the serviceName will say "pawwork" but the attributes will still say "opencode". This is inconsistent branding.
• Regression risk: Low for functionality, but medium for telemetry attribution confusion.
• Suggested fix: Either rename the attribute keys to pawwork.* or add a comment explaining why they remain opencode.* (e.g., upstream schema compatibility).

[P1] packages/core/src/npm.ts — InstallFailedError schema change breaks external consumers

The error shape changed from { pkg: string } to { add: string[], dir: string, cause: Schema.Defect }. The PR updates the SDK types and OpenAPI snapshot, which is good.

• Boundary case: Any external SDK consumer (third-party integrations, scripts) that parsed properties.pkg will break. The OpenAPI spec change is a breaking API change.
• Regression risk: If there are downstream consumers outside this repo (e.g., cloud control plane, mobile app), they need coordinated updates.
• Suggested fix: Add a note to the PR description or a migration doc about the breaking SDK change. Consider whether pkg should be kept as a deprecated alias for backward compatibility.

[P2] packages/core/src/global.ts — xdg-basedir modules may return undefined

const data = path.join(xdgData!, app)

The code uses non-null assertion (!) on xdgData, xdgCache, etc. In exotic environments (Windows without WSL, containers without XDG vars, certain CI images), these can return undefined.

• Boundary case: On Windows or minimal Docker containers, xdg-basedir may return undefined for some paths, causing path.join(undefined, "pawwork") to throw.
• Regression risk: Crash on startup in unsupported environments. The old code had the same issue, so this is not a new regression, but the move to core makes it harder to patch locally.
• Suggested fix: Add fallback values (e.g., path.join(os.homedir(), ".local", "share")) or at least a runtime check with a clear error message.

[P2] packages/core/src/npm.ts — resolveEntryPoint uses import.meta.resolve with Bun-only guard

const resolved = typeof Bun !== "undefined" ? import.meta.resolve(name, dir) : import.meta.resolve(dir)

• Boundary case: In Node.js (non-Bun) environments, import.meta.resolve(dir) resolves the directory itself, not the package name. This is likely incorrect — it should resolve name with dir as the parent.
• Regression risk: If PawWork ever runs on Node.js (e.g., for debugging, testing, or future portability), resolveEntryPoint will return wrong paths.
• Suggested fix: Use require.resolve(name, { paths: [dir] }) for Node.js, or document that this module is Bun-only.

[P2] packages/core/src/npm-config.ts — imports from @npmcli/config/lib/definitions/index.js

import { definitions, flatten, nerfDarts, shorthands } from "@npmcli/config/lib/definitions/index.js"

This imports from a deep internal path that npm does not guarantee as stable API.

• Boundary case: A future npm patch release could move or rename this file, breaking the build.
• Regression risk: Build breakage on bun install when npm updates. The // @ts-expect-error comment acknowledges this, but it doesn't prevent runtime breakage.
• Suggested fix: Pin @npmcli/config to an exact version in package.json (already done: 10.8.1), and add a CI check or dependabot rule to block automatic minor/patch bumps for this package.

[P2] packages/core/src/util/opencode-process.ts — env var names still say OPENCODE

export const OPENCODE_RUN_ID = "OPENCODE_RUN_ID"
export const OPENCODE_PROCESS_ROLE = "OPENCODE_PROCESS_ROLE"

These are now in @opencode-ai/core but the env var names are still OPENCODE_*. This is inconsistent with the PawWork carve-out philosophy.

• Boundary case: If a user runs both opencode and pawwork on the same machine, these env vars will collide.
• Regression risk: Very low, but it's a branding inconsistency that will need to be carved out in every future sync.
• Suggested fix: Add these to the permanent carve-out list and rename to PAWWORK_RUN_ID / PAWWORK_PROCESS_ROLE, or document why they must stay OPENCODE_*.

[P2] packages/core/test/npm.test.ts — test uses Bun.write and fs.stat but no cleanup on failure

await Bun.write(path.join(tmp.path, ".npmrc"), "omit=dev\n")

The tmpdir helper uses Symbol.asyncDispose, which is good. But if Bun.write or Npm.install throws, the using block should still dispose. However, Bun.write is not wrapped in try/finally.

• Boundary case: Not a real issue because using handles disposal, but Bun.write may leave partial files if interrupted.
• Regression risk: Minimal — test-only.
• Suggested fix: None critical, but consider using fs.writeFile for consistency with the rest of the test suite (the cross-spawn test already switched from Bun.write to fs.writeFile).

[P3] packages/core/src/npm.ts — Npm.install early-return on !canWrite without logging

if (!canWrite) return

If the target directory is not writable, Npm.install silently returns without installing anything and without logging.

• Boundary case: Read-only project directories (e.g., mounted volumes, protected workspaces).
• Regression risk: Users may wonder why dependencies are missing with no error.
• Suggested fix: Add a log.warn or similar before the early return.

[P3] packages/core/package.json — overrides: { "drizzle-orm": "catalog:" } unexplained

The new overrides field in packages/core/package.json pins drizzle-orm to the catalog version. core does not appear to depend on drizzle-orm directly.

• Boundary case: This may be a transitive override to resolve a version conflict elsewhere in the workspace.
• Regression risk: If the override is unnecessary, it adds maintenance burden. If it is necessary, it should be documented.
• Suggested fix: Add a comment in the PR or a package.json comment explaining why this override exists.

[P3] packages/core/src/effect/observability.ts — AsyncLocalStorageContextManager registered globally without cleanup

const mgr = new AsyncLocalStorageContextManager()
mgr.enable()
context.setGlobalContextManager(mgr)

This is called inside the traces() async function, which is invoked once when the observability layer is built. There is no mgr.disable() or cleanup on process exit.

• Boundary case: In test suites that create and tear down multiple runtimes, the global context manager may accumulate state or interfere with other tests.
• Regression risk: Test flakiness in observability-enabled test runs.
• Suggested fix: Add a cleanup hook or document that observability layer should not be rebuilt in tests.

[P3] packages/app/src/context/global-sync.tsx — add?.[0] fallback may show empty string

pkg: properties?.add?.[0] ?? "",

If add is an empty array, add?.[0] is undefined, so the toast description will show an empty package name.

• Boundary case: InstallFailedError constructed with add: [] (theoretically possible via the schema since add is optional).
• Regression risk: User sees "Failed to install " with no package name.
• Suggested fix: Use properties?.add?.[0] ?? properties?.dir ?? "unknown" for a better fallback.

[P3] packages/core/src/npm.ts — reify uses Effect.withSpan but add is not sanitized in span attributes

Effect.withSpan("Npm.reify", { attributes: input })

If input.add contains a scoped package with a registry token (e.g., @acme/pkg@npm:registry.example.com/pkg?authToken=secret), the token will be emitted into the OTel span.

• Boundary case: Private registry URLs with embedded credentials.
• Regression risk: Credential leakage in telemetry.
• Suggested fix: Sanitize or redact add entries before putting them in span attributes.

[P3] packages/opencode/src/lsp/server.ts — npmWhich catches Npm.InstallFailedError but re-throws other errors

if (err instanceof Npm.InstallFailedError) { ... }
throw err

If Npm.add throws a non-InstallFailedError (e.g., EffectFlock.LockError, unexpected runtime error), it bubbles up and may crash the LSP server initialization.

• Boundary case: File system lock contention or disk-full errors.
• Regression risk: LSP server crash instead of graceful degradation.
• Suggested fix: Consider catching all errors in npmWhich, logging them, and returning undefined to match the "which returns undefined on failure" contract.