mirror of https://github.com/nexu-io/open-design.git synced 2026-05-31 19:04:39 +07:00

fix(daemon, packaged): unbreak GUI-launched agent detection on minimal PATHs (#442 ) (#614 )

* fix(daemon, packaged): unbreak GUI-launched agent detection on minimal PATHs (#442)

GUI-launched daemons (Finder/Dock on macOS, .desktop on Linux) inherit a
stripped PATH from launchd / the desktop session and don't read the
user's interactive shell rc files, so any CLI installed via `npm i -g`
under a sudo-free prefix like ~/.npm-global was silently undetected.

Two layers maintained their own copies of the user-toolchain bin list
(`apps/daemon/src/agents.ts:userToolchainDirs` for the resolver,
`apps/packaged/src/sidecars.ts:resolvePackagedPathEnv` for the packaged
sidecar PATH builder) and had already drifted on `~/.asdf/shims` and
`~/Library/pnpm`. Adding ~/.npm-global to one side would have
preserved the same anti-pattern.

Extracts `wellKnownUserToolchainBins` into @open-design/platform as the
single source of truth, has both layers consume it, and extends the
list to cover ~/.npm-global/bin, ~/.npm-packages/bin, plus
$NPM_CONFIG_PREFIX/bin / $npm_config_prefix/bin for users with a
non-standard prefix. New vitest coverage in the platform package and
a regression test in apps/daemon/tests/agents.test.ts modelled on the
existing mise case.

Verified end-to-end: under PATH=/usr/bin:/bin:/usr/sbin:/sbin (the
launchd default a `.app` actually inherits), `resolveAgentExecutable`
now returns ~/.npm-global/bin/gemini instead of null.

* fix(daemon): isolate OD_AGENT_HOME resolution from $NPM_CONFIG_PREFIX leakage

Address review feedback on PR #614:

- mrcfps spotted that the daemon wrapper called wellKnownUserToolchainBins
  without passing `env`, so the helper read its default process.env. A
  developer or CI runner with NPM_CONFIG_PREFIX / npm_config_prefix
  exported would inject that real <prefix>/bin into resolveOnPath() even
  while the OD_AGENT_HOME hook pointed home at a temp fixture, making
  agent-detection tests environment-dependent. Reproduced locally: with
  OD_AGENT_HOME=<tmp> + NPM_CONFIG_PREFIX=/Users/me/.npm-global,
  resolveAgentExecutable({ bin: 'codex' }) returned the real machine's
  binary instead of null. Wrapper now passes `env: {}` whenever
  homeOverride is set, alongside the existing includeSystemBins gate.

- lefarcen suggested also handling whitespace-only NPM_CONFIG_PREFIX
  values (e.g. NPM_CONFIG_PREFIX=" ") so the helper does not emit a
  bogus "<whitespace>/bin" entry. Added a .trim() check before
  appending.

- lefarcen also suggested a comment pointer from the daemon wrapper to
  the platform helper so readers don't have to grep. Added the
  reference inline.

Coverage:
- packages/platform/tests/index.test.ts: new whitespace-prefix case.
- apps/daemon/tests/agents.test.ts: new env-isolation regression
  asserting that OD_AGENT_HOME + NPM_CONFIG_PREFIX cannot leak the
  real prefix bin into the sandbox.

* test(daemon): preserve $NPM_CONFIG_PREFIX across the env-isolation case (#614)

Address mrcfps's second-round review on PR #614: the env-isolation
regression sets `process.env.NPM_CONFIG_PREFIX = realPrefix` in its
body and then unconditionally `delete`s it in `finally`. On a developer
machine or CI runner that already exported `NPM_CONFIG_PREFIX`, that
mutates the worker-wide env for every later test, making downstream
env-sensitive assertions order-dependent.

Move the save/restore into the file's existing afterEach hook (mirroring
the OD_AGENT_HOME / OD_DAEMON_URL / OD_TOOL_TOKEN pattern) and drop the
in-test `delete`. Same coverage, no worker-state mutation.

* fix(platform): prioritise $NPM_CONFIG_PREFIX over the conventional npm guesses (#614)

Address mrcfps's third-round review on PR #614: when the user has
explicitly configured a prefix via $NPM_CONFIG_PREFIX (or
$npm_config_prefix), that's where `npm i -g` puts the *current*
binaries. The conventional guesses ~/.npm-global / ~/.npm-packages
often hold *stale* installs from an older prefix the user has since
rewritten — searching the env-driven prefix first matches npm's own
resolution order (env > .npmrc > default) and gives "explicit beats
convention" semantics.

Move the env-driven push above the conventional `dirs.push(.npm-global,
.npm-packages)`. Add a vitest case in the platform package that asserts
$NPM_CONFIG_PREFIX/bin's index in the result is strictly less than
~/.npm-global/bin's and ~/.npm-packages/bin's.

`resolveOnPath()` and the packaged PATH builder both preserve insertion
order, so first hit wins and the new ordering propagates to both
layers.

* fix(platform): lift $NPM_CONFIG_PREFIX above every conventional bin (#614)

Address mrcfps's fourth-round review on PR #614: the previous fix only
moved $NPM_CONFIG_PREFIX/bin ahead of ~/.npm-global / ~/.npm-packages,
but ~/.local/bin still appeared earlier in the array. Under a minimal
GUI-launch PATH a stale agent in ~/.local/bin (also a shared dumping
ground for pip --user / cargo install / hand-built binaries) could
outrank the user's *current* explicit npm prefix.

Move the env-driven push to the head of `dirs` so the explicit prefix
wins over every conventional location below — ~/.local/bin included.
Matches npm's own resolution order (env > .npmrc > default) across the
whole list, not just the npm-prefix block.

Tightened the existing order test to assert `explicitIdx === 0` and
that ~/.local/bin's index is strictly greater than the explicit
prefix's index, so a future drift would fail loudly.

2026-05-06 20:35:19 +08:00

2.5 KiB

Raw Blame History

packages/AGENTS.md

Follow the root AGENTS.md first. This file only records module-level boundaries for packages/.

Package responsibilities

packages/contracts: web/daemon app contract layer. Keep it pure TypeScript; it must not depend on Next.js, Express, Node filesystem/process APIs, browser APIs, SQLite, daemon internals, or the sidecar control-plane protocol.
packages/sidecar-proto: Open Design sidecar business protocol. Owns app/mode/source constants, namespace validation, stamp descriptor/fields/flags, IPC message schema, status shapes, error semantics, and default product path constants.
packages/sidecar: generic sidecar runtime primitives. Includes bootstrap, IPC transport, path/runtime resolution, launch env, and JSON runtime file helpers; it must not hard-code Open Design app keys or IPC business messages.
packages/platform: generic OS process primitives. Includes stamp serialization, command parsing, process matching/search, and well-known user-toolchain bin discovery; it must consume the sidecar-proto descriptor and must not hard-code --od-stamp-* details. The toolchain helper is the single source of truth shared by the daemon agent resolver (apps/daemon/src/agents.ts) and the packaged sidecar PATH builder (apps/packaged/src/sidecars.ts) so neither layer can drift the search list.

Removed directories

packages/shared has been removed; do not restore it.
For new shared types, choose the boundary first: web/daemon app DTOs go in contracts; sidecar control-plane protocol goes in sidecar-proto; generic runtime code goes in sidecar; generic OS/process code goes in platform.

Boundary checklist

Package tests live in each package's tests/ directory, sibling to src/; keep src/ source-only and do not add new *.test.ts or *.test.tsx files under src/.
Do not move runtime validation/schema enforcement into contracts prematurely; current contracts define the typed target shape only.
Do not let app packages depend directly on sidecar control-plane details.
Do not hard-code Open Design app/source/mode constants in sidecar or platform.
Keep stamp fields limited to five: app, mode, namespace, ipc, and source.

Common package commands

pnpm --filter @open-design/contracts typecheck
pnpm --filter @open-design/sidecar-proto typecheck
pnpm --filter @open-design/sidecar-proto test
pnpm --filter @open-design/sidecar typecheck
pnpm --filter @open-design/sidecar test
pnpm --filter @open-design/platform typecheck
pnpm --filter @open-design/platform test

2.5 KiB Raw Blame History