From fa8f0c98dae876c8a2854226c17b6cc79f917210 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 15:42:30 -0700 Subject: [PATCH 1/6] refactor([issue-3194]): own AI-CLI child-env composition in one helper Eight spawn sites each rebuilt the same child-env tuple by hand: provider.envVars over process.env, the OpenCode declared-models map, the PWD pin, the CLAUDECODE strip, and (for agents) the pm2 guard shim. Three separate fixes had already paid an N-file sweep to keep them in step -- the OpenCode models map (#2190/#2243), the CLAUDECODE strip, and the PWD pin (#3193) -- with a missed site failing silently every time. buildCliChildEnv() in server/lib/cliChildEnv.js now owns the composition. Precedence is preserved per site rather than flattened: two distinct slots, 'before' (under provider.envVars, for forgeTokenEnv/ claudeSettingsEnv so an explicit provider GH_TOKEN still wins) and 'extra' (over it, for a PTY's TERM/COLORTERM). cliChildEnv.test.js asserts the composed order one case per call site. Routing askService and visionCli through the builder also brings them the OpenCode declared-models map for the first time, fixing #2190 at two more sites: both inject --model, so an Ollama-backed OpenCode provider used for Ask or vision captioning was rejecting it. A discovery-based guard replaces the per-fix grep list: any file that strips CLAUDECODE, or spreads provider.envVars over process.env into a cwd-bearing spawn, must route through the builder or be explicitly exempt with a reason. spawnCwd.test.js's pin scan now counts buildCliChildEnv as a pin so the collapsed sites stay covered. --- server/cos-runner/index.js | 17 +- server/cos-runner/index.test.js | 9 +- server/lib/README.md | 1 + server/lib/cliChildEnv.js | 112 +++++++++++ server/lib/cliChildEnv.test.js | 296 ++++++++++++++++++++++++++++ server/lib/cliProviderRun.js | 27 ++- server/lib/index.js | 1 + server/lib/spawnCwd.test.js | 6 +- server/lib/tuiPromptRunner.js | 29 ++- server/lib/tuiUsageScrape.js | 24 +-- server/services/agentCliSpawning.js | 28 +-- server/services/askService.js | 10 +- server/services/runner.js | 27 ++- server/services/visionCli.js | 21 +- 14 files changed, 519 insertions(+), 89 deletions(-) create mode 100644 server/lib/cliChildEnv.js create mode 100644 server/lib/cliChildEnv.test.js diff --git a/server/cos-runner/index.js b/server/cos-runner/index.js index e51468c517..ca1a3d7bd2 100644 --- a/server/cos-runner/index.js +++ b/server/cos-runner/index.js @@ -17,7 +17,7 @@ import http from 'http'; import { Server as SocketServer } from 'socket.io'; import { ensureDir, PATHS, sleep } from '../lib/fileUtils.js'; import { prepareCliSpawn, killProcessTree } from '../lib/bufferedSpawn.js'; -import { withSpawnCwdEnv } from '../lib/spawnCwd.js'; +import { buildCliChildEnv } from '../lib/cliChildEnv.js'; import { prepareCliPrompt } from '../lib/cliProviderArgs.js'; import { createCodexStderrFormatter } from '../lib/codexCliOutput.js'; import { createStreamJsonParser } from './streamJsonParser.js'; @@ -188,10 +188,17 @@ app.post('/spawn', async (req, res) => { // Ensure workspacePath is valid const cwd = workspacePath && typeof workspacePath === 'string' ? workspacePath : ROOT_DIR; - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). This is the path the - // bug was reported through: the log line above named the app's workspace - // correctly while every OpenCode agent still ran in the PortOS folder. - const childEnv = (() => { const e = withSpawnCwdEnv({ ...process.env, ...envVars }, cwd); delete e.CLAUDECODE; return e; })(); + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. The runner receives the provider's + // envVars over HTTP rather than a provider record, so it wraps them into the + // `provider` slot; with no `command` in the payload the OpenCode layer is a + // no-op and the result is exactly `{ ...process.env, ...envVars }` as before. + // The PWD pin is the path #3193 was reported through: the log line above named + // the app's workspace correctly while every OpenCode agent still ran in the + // PortOS folder. No `guard`, preserving today's behavior: this runner is a + // separate process that does not carry the pm2 shim (only the in-process + // agent-spawning paths opt in). + const childEnv = buildCliChildEnv({ provider: { envVars }, cwd }); // Resolve a bare npm-installed CLI (opencode/codex/claude/… — a .cmd/.bat // shim on Windows) to its real path and wrap a shim as `cmd.exe /c ` so diff --git a/server/cos-runner/index.test.js b/server/cos-runner/index.test.js index 01b1e22abe..d43d45fe58 100644 --- a/server/cos-runner/index.test.js +++ b/server/cos-runner/index.test.js @@ -34,10 +34,11 @@ describe('cos-runner spawn — Windows CLI shim resolve+wrap (#2243)', () => { }); it('resolves the command against the child env (childEnv) so a provider PATH override is honored', () => { - // childEnv (process.env + provider envVars, CLAUDECODE stripped) is built - // BEFORE the resolve so PATH resolution sees the child's PATH, and is reused - // as the spawn env — matching the working server/services/runner.js path. - const childEnvIdx = RUNNER_SRC.indexOf('const childEnv = (() =>'); + // childEnv (process.env + provider envVars, CLAUDECODE stripped, PWD pinned + // — composed by the shared buildCliChildEnv) is built BEFORE the resolve so + // PATH resolution sees the child's PATH, and is reused as the spawn env — + // matching the working server/services/runner.js path. + const childEnvIdx = RUNNER_SRC.indexOf('const childEnv = buildCliChildEnv('); const prepareIdx = RUNNER_SRC.indexOf('prepareCliSpawn(command, deliveredArgs, childEnv)'); expect(childEnvIdx, 'childEnv must be defined').toBeGreaterThan(-1); expect(prepareIdx, 'prepareCliSpawn must run against childEnv').toBeGreaterThan(-1); diff --git a/server/lib/README.md b/server/lib/README.md index 3bd8e91b2f..0344f9a447 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -105,6 +105,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `usageRange.js` | `resolveUsageRange({ period, from, to })` — pure period→inclusive-YYYY-MM-DD range resolution for the usage cost report (explicit dates win; `all` unbounded; default 7d). | | `providerTranscriptUsage.js` | Parsers for the real per-message token counts the coding CLIs write to disk (0 tokens to read) — `parseClaudeTranscript` (`~/.claude/projects//*.jsonl`), `parseCodexRollout` (`~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl`), `claudeProjectSlug`, `totalTranscriptTokens`. Both de-duplicate a format hazard that otherwise inflates counts badly: Claude repeats one response across several lines sharing a `message.id`, and Codex's `total_token_usage` is cumulative and repeated. Each parser returns per-model buckets (`byModel`) plus the message keys it counted (`countedKeys`), and accepts an `exclude` set — that is what stops two overlapping PortOS runs from both billing the same messages. Tolerant of truncated (mid-write) files; consumed by `services/usageReconciler.js`. | | `opencodeConfig.js` | OpenCode config builder — `buildOpencodeEnvVars(provider, model)` builds dynamic `OPENCODE_CONFIG_CONTENT` declaring the models map under `provider.ollama.models` (bare ids) for Ollama-backed OpenCode providers. Fixes --model rejection. | +| `cliChildEnv.js` | `buildCliChildEnv({ baseEnv, before, provider, model, cwd, extra, guard })` — the one place the AI-CLI child environment is composed. Layers `baseEnv → before → provider.envVars → buildOpencodeEnvVars → extra`, pins `PWD` to the spawn cwd, strips `CLAUDECODE`, and (with `guard: true`) prepends the pm2 guard shim onto the final `PATH`. Replaces the hand-rolled copy that lived at all five spawn sites (`services/runner.js`, `cliProviderRun.js`, `services/agentCliSpawning.js`, `tuiPromptRunner.js`, `cos-runner/index.js`) and made every env-level fix an N-file sweep (#3194). The two slots are not interchangeable: `before` sits UNDER `provider.envVars` (forgeTokenEnv/claudeSettingsEnv, so a provider override still wins), `extra` sits OVER it (TERM/COLORTERM for a PTY) — `cliChildEnv.test.js` asserts the composed order per call site. | | `cliProviderArgs.js` | Per-CLI argv conventions (`buildCliArgs`) for stdin prompt delivery — dependency-light extraction from runner.js so out-of-process callers (autofixer) can import it. | | `cliProviderRun.js` | One-shot CLI provider invocation (`pickCliProvider` + `runCliProviderPrompt`) — lightweight path for the autofixer + calendar MCP sync to honor the configured provider/model. | | `grok.js` | xAI Grok Build (`grok`) provider helpers — id/endpoint constants (`GROK_API_ID`/`GROK_CLI_ID`/`GROK_TUI_ID`/`GROK_API_ENDPOINT`), `isGrokCommand`/`isGrokCliProvider`/`isGrokTuiProvider` predicates, `ensureGrokHeadlessArgs`/`ensureGrokTuiArgs` argv builders (grok reads its prompt from `--prompt-file /dev/stdin`, not raw stdin; model selection uses the `GROK_CONFIGURED_DEFAULT` sentinel in `providerModels.js` so PortOS omits `--model` like Antigravity), and `prepareGrokPromptFile` (Windows temp-file delivery fallback). | diff --git a/server/lib/cliChildEnv.js b/server/lib/cliChildEnv.js new file mode 100644 index 0000000000..6c648651d6 --- /dev/null +++ b/server/lib/cliChildEnv.js @@ -0,0 +1,112 @@ +/** + * Child-environment composition for every AI-CLI spawn. + * + * Five spawn sites — `services/runner.js` (Run Prompt), `lib/cliProviderRun.js` + * (fire-and-collect), `services/agentCliSpawning.js` (CoS agents), + * `lib/tuiPromptRunner.js` (PTY TUI runs), and `cos-runner/index.js` + * (`POST /spawn`) — each independently rebuilt the same env tuple: + * + * { ...baseEnv, ...providerEnv, ...opencodeEnv } + * → pin PWD to the spawn cwd + * → strip CLAUDECODE + * → prepend the pm2 guard shim (agent-spawning sites only) + * + * Three separate fixes each paid an N-file sweep to keep those five in step: + * the OpenCode declared-models map (#2190/#2243), the `CLAUDECODE` strip, and + * the `PWD` pin (#3193). Every time, the guard against missing a site was + * "a reviewer noticed" or a source-grep test. This module owns the composition + * so the next env-level concern is a one-file change (#3194). + * + * **Precedence is the load-bearing part.** The sites do NOT all layer their + * extras the same way, and flattening them into one order would silently change + * which value wins: + * + * - `agentCliSpawning` puts `forgeTokenEnv`/`claudeSettingsEnv` BEFORE + * `provider.envVars`, so an explicit provider `GH_TOKEN` still overrides the + * repo-owner-pinned one. + * - `tuiPromptRunner` puts `TERM`/`COLORTERM` AFTER everything, so the PTY + * always gets a truecolor terminal regardless of provider config. + * + * Hence two distinct slots — `before` (lower precedence than the provider) and + * `extra` (higher) — rather than one `extra`. `cliChildEnv.test.js` asserts the + * exact composed order for each of the five call sites. + * + * Deliberately NOT in scope: a `spawnAiCli({ command, args, cwd, env })` wrapper + * that would also own cwd resolution and the Windows `.cmd` shim. The eight + * spawn shapes differ too much (child_process vs node-pty, stdio variants, + * kill-tree wiring) to fold in the same change. `lib/aiToolkit/runner.js` is + * also out of scope: it is vendored and must not import out to other PortOS + * modules (see `lib/aiToolkit/CLAUDE.md`), so it keeps its inline pin. + */ + +import { withSpawnCwdEnv } from './spawnCwd.js'; +import { buildOpencodeEnvVars } from './opencodeConfig.js'; +import { agentGuardEnv } from './agentGuard/index.js'; + +/** + * Compose the environment an AI-CLI child process is spawned with. + * + * Layering, lowest precedence first: + * `baseEnv` → `before` → `provider.envVars` → OpenCode env → `extra` + * then `PWD` is pinned to `cwd`, `CLAUDECODE` is stripped, and — only when + * `guard` is true — the pm2 guard shim is prepended to the final `PATH`. + * + * @param {object} options + * @param {NodeJS.ProcessEnv|object} [options.baseEnv=process.env] - env to start from. + * Callers that must not leak host credentials to an autonomous agent (e.g. the + * autofixer) pass a sanitized allowlist here; every later layer still overlays it. + * @param {object|null} [options.before] - env spread AFTER `baseEnv` but BEFORE + * `provider.envVars`, so a provider-configured value still wins over it + * (forgeTokenEnv, claudeSettingsEnv). + * @param {{command?:string, envVars?:object, models?:string[], defaultModel?:string|null, ollamaBacked?:boolean}|null} [options.provider] + * - the provider record. `provider.envVars` is layered in, and the provider's + * command/ollamaBacked flags decide whether an OpenCode config is built. + * @param {string|null} [options.model] - the model being run this invocation, used + * to build the OpenCode declared-models map. Pass `provider.defaultModel` when + * the site has no per-call model. + * @param {string|undefined|null} options.cwd - the directory being handed to + * `spawn`/`pty.spawn`; `PWD` is pinned to it (see `withSpawnCwdEnv`, #3193). + * @param {object|null} [options.extra] - env spread LAST, so it overrides every + * other layer including `provider.envVars` (TERM/COLORTERM for a PTY). + * @param {boolean} [options.guard=false] - prepend the pm2 guard shim onto the + * final `PATH` so an unrestricted agent can't `pm2 kill` the shared daemon. + * Only the two agent-spawning sites opt in; the Run Prompt / fire-and-collect + * paths keep today's unguarded behavior. + * @returns {object} a fresh env object safe to hand straight to `spawn` + */ +export function buildCliChildEnv({ + baseEnv = process.env, + before = null, + provider = null, + model = null, + cwd, + extra = null, + guard = false, +} = {}) { + // buildOpencodeEnvVars rebuilds OPENCODE_CONFIG_CONTENT with a declared models + // map for OpenCode Ollama providers (an empty object for everyone else) so the + // injected `--model ollama/` isn't rejected as "not valid" — see #2190. + // It comes after provider.envVars to override the provider's STATIC + // OPENCODE_CONFIG_CONTENT, which it was built from. + const env = withSpawnCwdEnv( + { + ...baseEnv, + ...(before || {}), + ...(provider?.envVars || {}), + ...buildOpencodeEnvVars(provider, model), + ...(extra || {}), + }, + cwd, + ); + + // CLAUDECODE is set when PortOS itself runs inside Claude Code; passing it + // through would make a spawned Claude CLI think it's nested inside the parent + // session. + delete env.CLAUDECODE; + + // Must be applied to the FINAL PATH (after every override above), or a + // provider-configured PATH would drop the shim back off the front. + if (guard) Object.assign(env, agentGuardEnv(env)); + + return env; +} diff --git a/server/lib/cliChildEnv.test.js b/server/lib/cliChildEnv.test.js new file mode 100644 index 0000000000..d3277dd4fd --- /dev/null +++ b/server/lib/cliChildEnv.test.js @@ -0,0 +1,296 @@ +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync } from 'fs'; +import { join, relative } from 'path'; +import { fileURLToPath } from 'url'; +import { buildCliChildEnv } from './cliChildEnv.js'; +import { AGENT_GUARD_BIN } from './agentGuard/index.js'; + +// An OpenCode provider that IS ollama-backed — the only shape for which +// buildOpencodeEnvVars returns anything. Everyone else gets `{}`, which is why +// the OpenCode layer is invisible at the other call sites. +const OLLAMA_OPENCODE = { + command: 'opencode', + ollamaBacked: true, + models: ['qwen2.5:7b'], + defaultModel: 'qwen2.5:7b', + envVars: { OPENCODE_CONFIG_CONTENT: '{"permission":"deny"}', API_KEY: 'from-provider' }, +}; + +const declaredModels = (env) => Object.keys(JSON.parse(env.OPENCODE_CONFIG_CONTENT).provider.ollama.models); + +describe('buildCliChildEnv — layering', () => { + it('layers baseEnv < before < provider.envVars < extra', () => { + const env = buildCliChildEnv({ + baseEnv: { WHO: 'base', FROM_BASE: '1' }, + before: { WHO: 'before', FROM_BEFORE: '1' }, + provider: { envVars: { WHO: 'provider', FROM_PROVIDER: '1' } }, + extra: { WHO: 'extra', FROM_EXTRA: '1' }, + }); + expect(env.WHO).toBe('extra'); + // Every layer still contributes its own non-conflicting keys. + expect(env).toMatchObject({ FROM_BASE: '1', FROM_BEFORE: '1', FROM_PROVIDER: '1', FROM_EXTRA: '1' }); + }); + + // The `before` slot exists specifically so agentCliSpawning's forgeTokenEnv + // can be overridden by an explicit provider credential. Collapsing it into + // `extra` would silently flip which GH_TOKEN the agent's `gh pr create` uses. + it('lets provider.envVars beat `before` but lose to `extra`', () => { + const env = buildCliChildEnv({ + baseEnv: {}, + before: { GH_TOKEN: 'repo-owner-pinned' }, + provider: { envVars: { GH_TOKEN: 'provider-explicit', TERM: 'dumb' } }, + extra: { TERM: 'xterm-256color' }, + }); + expect(env.GH_TOKEN).toBe('provider-explicit'); + expect(env.TERM).toBe('xterm-256color'); + }); + + // The OpenCode map is built FROM provider.envVars.OPENCODE_CONFIG_CONTENT, so + // it must land after it — otherwise the static value the map was derived from + // wins and `--model ollama/` is rejected again (#2190). + it('overrides the provider static OPENCODE_CONFIG_CONTENT with the declared-models map', () => { + const env = buildCliChildEnv({ baseEnv: {}, provider: OLLAMA_OPENCODE, model: 'llama3.1:8b' }); + expect(declaredModels(env).sort()).toEqual(['llama3.1:8b', 'qwen2.5:7b']); + // Non-conflicting provider vars survive. + expect(env.API_KEY).toBe('from-provider'); + // The stored base is merged, not clobbered. + expect(JSON.parse(env.OPENCODE_CONFIG_CONTENT).permission).toBe('deny'); + }); + + it('is a no-op OpenCode layer for a non-OpenCode provider', () => { + const env = buildCliChildEnv({ baseEnv: {}, provider: { command: 'claude', envVars: { A: '1' } }, model: 'opus' }); + expect(env.OPENCODE_CONFIG_CONTENT).toBeUndefined(); + expect(env.A).toBe('1'); + }); + + it('tolerates an absent provider / before / extra', () => { + expect(buildCliChildEnv({ baseEnv: { A: '1' } })).toEqual({ A: '1' }); + expect(buildCliChildEnv({ baseEnv: { A: '1' }, provider: null, before: null, extra: null })).toEqual({ A: '1' }); + }); + + it('defaults baseEnv to process.env', () => { + expect(buildCliChildEnv({ cwd: undefined }).PATH).toBe(process.env.PATH); + }); + + it('copies rather than mutating the caller env', () => { + const baseEnv = { A: '1' }; + const env = buildCliChildEnv({ baseEnv, extra: { B: '2' } }); + expect(baseEnv).toEqual({ A: '1' }); + expect(env).not.toBe(baseEnv); + }); +}); + +describe('buildCliChildEnv — PWD pin and CLAUDECODE strip', () => { + it('pins PWD to the spawn cwd, overriding a stale inherited value (#3193)', () => { + const env = buildCliChildEnv({ baseEnv: { PWD: '/repos/PortOS' }, cwd: '/repos/my-app' }); + expect(env.PWD).toBe('/repos/my-app'); + }); + + it('leaves the inherited PWD alone when no cwd is passed', () => { + expect(buildCliChildEnv({ baseEnv: { PWD: '/repos/PortOS' } }).PWD).toBe('/repos/PortOS'); + }); + + // The pin runs LAST, over the composed object — so a provider that sets its + // own PWD cannot re-point the child at the wrong repo. + it('pins PWD over a provider-supplied PWD', () => { + const env = buildCliChildEnv({ + baseEnv: {}, provider: { envVars: { PWD: '/somewhere/else' } }, cwd: '/repos/my-app', + }); + expect(env.PWD).toBe('/repos/my-app'); + }); + + it('strips CLAUDECODE from every layer that could supply it', () => { + const env = buildCliChildEnv({ + baseEnv: { CLAUDECODE: '1' }, + before: { CLAUDECODE: '1' }, + provider: { envVars: { CLAUDECODE: '1' } }, + extra: { CLAUDECODE: '1' }, + }); + expect(env.CLAUDECODE).toBeUndefined(); + }); +}); + +describe('buildCliChildEnv — pm2 guard', () => { + it('leaves PATH untouched without `guard` (Run Prompt / fire-and-collect paths)', () => { + const env = buildCliChildEnv({ baseEnv: { PATH: '/usr/bin' } }); + expect(env.PATH).toBe('/usr/bin'); + expect(env.PORTOS_REAL_PM2).toBeUndefined(); + }); + + // Load-bearing: the shim must sit on the FINAL PATH. Prepending it before the + // provider's override would let a `--dangerously-skip-permissions` agent reach + // the real pm2 and `pm2 kill` the shared daemon. + it('prepends the guard shim onto the PATH a provider override produced', () => { + const env = buildCliChildEnv({ + baseEnv: { PATH: '/usr/bin' }, + provider: { envVars: { PATH: '/provider/bin' } }, + guard: true, + }); + expect(env.PATH.startsWith(`${AGENT_GUARD_BIN}`)).toBe(true); + expect(env.PATH).toContain('/provider/bin'); + expect(env.PATH).not.toContain('/usr/bin'); + }); +}); + +// One case per real call site, asserting the precedence THAT site depends on. +// The sites do not all layer the same way, so a single "extra wins" rule would +// have silently changed two of them — these pin the actual contracts. +describe('buildCliChildEnv — per-call-site composition', () => { + it('runner.js / cliProviderRun.js: provider.envVars over baseEnv, guarded only for the runner', () => { + const args = { baseEnv: { A: 'base', PATH: '/usr/bin' }, provider: { envVars: { A: 'provider' } }, cwd: '/w' }; + expect(buildCliChildEnv({ ...args, guard: true }).A).toBe('provider'); + expect(buildCliChildEnv({ ...args, guard: true }).PATH).toContain(AGENT_GUARD_BIN); + // The fire-and-collect path is not an agent — it must stay unguarded. + expect(buildCliChildEnv(args).PATH).toBe('/usr/bin'); + }); + + it('cliProviderRun.js: honors a sanitized baseEnv instead of process.env', () => { + // The autofixer passes an allowlist so host credentials never reach the CLI. + const env = buildCliChildEnv({ baseEnv: { PATH: '/usr/bin' }, provider: { envVars: {} }, cwd: '/w' }); + expect(env.HOME).toBeUndefined(); + expect(env.PATH).toBe('/usr/bin'); + }); + + it('agentCliSpawning.js: forgeToken/claudeSettings sit UNDER provider.envVars', () => { + const env = buildCliChildEnv({ + baseEnv: { PATH: '/usr/bin' }, + before: { GH_TOKEN: 'forge', AWS_PROFILE: 'from-settings' }, + provider: { envVars: { GH_TOKEN: 'provider' } }, + model: 'opus', + cwd: '/w', + guard: true, + }); + expect(env.GH_TOKEN).toBe('provider'); // explicit provider credential wins + expect(env.AWS_PROFILE).toBe('from-settings'); // non-conflicting settings survive + expect(env.PWD).toBe('/w'); + expect(env.PATH).toContain(AGENT_GUARD_BIN); + }); + + it('tuiPromptRunner.js / tuiUsageScrape.js: TERM/COLORTERM beat provider.envVars', () => { + const env = buildCliChildEnv({ + baseEnv: { TERM: 'dumb' }, + provider: { envVars: { TERM: 'vt100', COLORTERM: '' } }, + cwd: '/sandbox', + extra: { TERM: 'xterm-256color', COLORTERM: 'truecolor' }, + }); + expect(env.TERM).toBe('xterm-256color'); + expect(env.COLORTERM).toBe('truecolor'); + expect(env.PWD).toBe('/sandbox'); + // A PTY prompt run is not an agent — no shim. + expect(env.PORTOS_REAL_PM2).toBeUndefined(); + }); + + it('cos-runner/index.js: request envVars over process.env, no OpenCode layer, unguarded', () => { + // The runner receives loose envVars over HTTP with no `command` to classify. + const env = buildCliChildEnv({ + baseEnv: { A: 'base', PATH: '/usr/bin', CLAUDECODE: '1' }, + provider: { envVars: { A: 'request' } }, + cwd: '/workspace', + }); + expect(env).toEqual({ A: 'request', PATH: '/usr/bin', PWD: '/workspace' }); + }); + + it('askService.js: no cwd means no PWD is invented', () => { + const env = buildCliChildEnv({ baseEnv: { PATH: '/usr/bin' }, provider: { envVars: {} }, model: 'opus' }); + expect(env.PWD).toBeUndefined(); + }); + + it('visionCli.js: the image dir is pinned as PWD so the CLI can find the file', () => { + const env = buildCliChildEnv({ + baseEnv: { PWD: '/repos/PortOS' }, provider: OLLAMA_OPENCODE, model: 'llava:7b', cwd: '/tmp/portos-vision-x', + }); + expect(env.PWD).toBe('/tmp/portos-vision-x'); + // And the vision model is declared, so `--model ollama/llava:7b` is accepted. + expect(declaredModels(env)).toContain('llava:7b'); + }); +}); + +// Source invariant. The whole point of this module is that the next env-level +// concern is a ONE-file change — which only holds if no spawn site quietly +// rebuilds the tuple by hand again. Three separate fixes (the OpenCode models +// map, the CLAUDECODE strip, the PWD pin) each had to sweep every site because +// nothing failed when one was missed. +// +// Deliberately DISCOVERS the offenders rather than listing the known call sites: +// an allowlist of "these files must call the builder" passes the day someone +// adds a ninth spawn site, which is exactly when the guard needs to fire. +describe('no spawn site rebuilds the CLI child env by hand', () => { + const SERVER_DIR = fileURLToPath(new URL('..', import.meta.url)); + + // Files allowed to compose the tuple themselves, each with the reason. + const EXEMPT = new Map([ + ['lib/cliChildEnv.js', 'this module IS the shared composer'], + ['lib/aiToolkit/runner.js', 'vendored toolkit — must not import out to other PortOS modules'], + ]); + + const collectSources = (dir) => readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + if (entry.name === 'node_modules' || entry.name.startsWith('.')) return []; + const abs = join(dir, entry.name); + if (entry.isDirectory()) return collectSources(abs); + if (!entry.name.endsWith('.js') || entry.name.endsWith('.test.js')) return []; + return [relative(SERVER_DIR, abs)]; + }); + + // Two independent markers, because either one alone has a blind spot: a new + // site could strip CLAUDECODE without spreading provider.envVars, or spread + // provider.envVars while forgetting the strip (which is itself the bug). + // + // Marker A — the CLAUDECODE strip. Crisp: every path that runs a coding CLI + // strips it, and nothing else in the tree does. + const STRIPS_CLAUDECODE = /delete\s+[A-Za-z_$][\w$]*\.CLAUDECODE\b/; + + // Marker B — a provider-env-over-base composition that is handed to a child + // running in a workspace. The `cwd` requirement is what separates a real spawn + // env from the two shapes that merge the same objects for a different purpose + // and correctly need none of this: a model-id LOOKUP env + // (`resolveBedrockCliModel({ env })`, `resolveWindowsExecutable(…, searchEnv)`) + // and a capability PROBE (`agy models`, `--version`) — neither runs a model, + // writes files, or has a workspace to be misrouted into. + // + // A window rather than a parser: the composition is always one object literal. + // KNOWN BLIND SPOT, stated rather than papered over: a real spawn env built + // with NO cwd (askService's Ask path is the one such shape today) is invisible + // to this marker. Marker A still catches it as long as the new site strips + // CLAUDECODE — and a site that does not strip it has the very bug marker B + // was never going to be able to name. + const SPREADS_PROVIDER_ENV_INTO_SPAWN = (src) => { + for (const m of src.matchAll(/\.\.\.\s*(?:process\.env|baseEnv)\b/g)) { + // Wide enough to span the comment blocks that sit between the composition + // and the spawn call it feeds (the vendored toolkit runner has ~500 chars + // of them). Over-matching costs one EXEMPT line; under-matching silently + // reopens the N-file sweep, so the bias is deliberate. + const window = src.slice(m.index, m.index + 900); + if (/\.\.\.\s*[A-Za-z_$][\w$]*\??\.envVars\b/.test(window) && /\bcwd\s*[:,]/.test(window)) return true; + } + return false; + }; + + const offenders = () => collectSources(SERVER_DIR).filter((rel) => { + if (EXEMPT.has(rel)) return false; + const src = readFileSync(join(SERVER_DIR, rel), 'utf8'); + return STRIPS_CLAUDECODE.test(src) || SPREADS_PROVIDER_ENV_INTO_SPAWN(src); + }); + + it('finds the exempt files (guard is not vacuous)', () => { + // If the markers stop matching even the composer itself, the scan broke and + // the assertion below would pass for the wrong reason. + for (const rel of EXEMPT.keys()) { + const src = readFileSync(join(SERVER_DIR, rel), 'utf8'); + expect( + STRIPS_CLAUDECODE.test(src) || SPREADS_PROVIDER_ENV_INTO_SPAWN(src), + `${rel} no longer matches either marker — the scan broke`, + ).toBe(true); + } + }); + + it('every AI-CLI spawn composes its child env through buildCliChildEnv', () => { + expect( + offenders(), + 'These files build an AI-CLI child environment by hand instead of calling ' + + 'buildCliChildEnv (server/lib/cliChildEnv.js). That is what made the OpenCode ' + + 'models map (#2190), the CLAUDECODE strip, and the PWD pin (#3193) each cost an ' + + 'N-file sweep, with a missed site failing silently. Route the spawn through the ' + + 'shared builder, or add the file to EXEMPT above with the reason it must not.', + ).toEqual([]); + }); +}); diff --git a/server/lib/cliProviderRun.js b/server/lib/cliProviderRun.js index 3c099fb397..dae9d49a48 100644 --- a/server/lib/cliProviderRun.js +++ b/server/lib/cliProviderRun.js @@ -18,9 +18,8 @@ import { spawn } from 'child_process'; import { buildCliArgs, prepareCliPrompt } from './cliProviderArgs.js'; -import { buildOpencodeEnvVars } from './opencodeConfig.js'; import { killProcessTree, resolveWindowsExecutable, prepareWindowsSafeSpawn } from './bufferedSpawn.js'; -import { withSpawnCwdEnv } from './spawnCwd.js'; +import { buildCliChildEnv } from './cliChildEnv.js'; /** * Resolve which CLI provider + model a feature should use from the providers @@ -102,19 +101,19 @@ export function runCliProviderPrompt(args = {}) { let stderr = ''; let settled = false; - // CLAUDECODE is deleted from the child env so a nested invocation doesn't - // think it's running inside the parent Claude Code session. - // buildOpencodeEnvVars rebuilds OPENCODE_CONFIG_CONTENT with a declared - // models map for OpenCode Ollama providers (empty/no-op otherwise) so the - // injected `--model ollama/` isn't rejected as "not valid" — see - // issue-2190. effectiveProvider carries the per-call model as defaultModel. - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. No `guard`: this is the + // fire-and-collect path, not an autonomous agent, so it keeps the unguarded + // PATH it has always had. effectiveProvider carries the per-call model as + // defaultModel, which is what the OpenCode declared-models map is built from + // (its envVars are provider's, unchanged). const effectiveCwd = cwd || process.cwd(); - const childEnv = withSpawnCwdEnv( - { ...baseEnv, ...provider.envVars, ...buildOpencodeEnvVars(effectiveProvider, effectiveProvider.defaultModel) }, - effectiveCwd, - ); - delete childEnv.CLAUDECODE; + const childEnv = buildCliChildEnv({ + baseEnv, + provider: effectiveProvider, + model: effectiveProvider.defaultModel, + cwd: effectiveCwd, + }); // npm-installed CLI providers are .cmd/.bat shims on Windows; resolve+wrap // (cmd.exe /c) instead of enabling a shell — shell:true + an args array diff --git a/server/lib/index.js b/server/lib/index.js index 34f93ddcfa..c173dfed8f 100644 --- a/server/lib/index.js +++ b/server/lib/index.js @@ -85,6 +85,7 @@ export * from './ansiStrip.js'; // ANTIGRAVITY_CONFIGURED_DEFAULT, so a flat `export *` would trip the // barrel's duplicate-identifier collision check. export * as antigravity from './antigravity.js'; +export * from './cliChildEnv.js'; export * from './cliProviderArgs.js'; export * from './cliProviderRun.js'; export * from './codexAssistantExtract.js'; diff --git a/server/lib/spawnCwd.test.js b/server/lib/spawnCwd.test.js index f153a633ee..2b3443245a 100644 --- a/server/lib/spawnCwd.test.js +++ b/server/lib/spawnCwd.test.js @@ -323,9 +323,13 @@ describe('every cwd-passing spawn pins PWD', () => { // Count pins rather than merely asserting one exists: a file that already // pins its first spawn would otherwise pass forever, even after a second, // unpinned cwd-spawn is added to it. + // Two markers count as a pin: the low-level helper, and buildCliChildEnv — + // the shared AI-CLI env composer, which calls withSpawnCwdEnv internally + // (#3194). Counting the composer here is what lets the five AI-CLI spawn + // sites collapse to one call each without going dark to this guard. const pins = INLINE_PIN.has(rel) ? (src.match(/childEnv\.PWD\s*=/g) || []).length - : (src.match(/withSpawnCwdEnv\(/g) || []).length; + : (src.match(/(?:withSpawnCwdEnv|buildCliChildEnv)\(/g) || []).length; if (pins < sites) unpinned.push(`${rel} (${sites} cwd-passing spawn(s), ${pins} pin(s))`); } expect( diff --git a/server/lib/tuiPromptRunner.js b/server/lib/tuiPromptRunner.js index e20b98977e..ad42ecab0b 100644 --- a/server/lib/tuiPromptRunner.js +++ b/server/lib/tuiPromptRunner.js @@ -57,8 +57,7 @@ import { buildTuiInvocation, detectMissingTuiBinary, } from './tuiHandshake.js'; -import { buildOpencodeEnvVars } from './opencodeConfig.js'; -import { withSpawnCwdEnv } from './spawnCwd.js'; +import { buildCliChildEnv } from './cliChildEnv.js'; // One-shot defaults that don't apply to the long-running agent path: // - hard run cap (5 min vs unbounded for agents) @@ -171,20 +170,18 @@ ${prompt}`; // unstripped) is counted the same way the stripped post-paste buffer is. const promptMarkerCount = countPasteMarkers(stripAnsi(wrappedPrompt)); - // CLAUDECODE is set when PortOS itself runs inside Claude Code; passing it - // through to a spawned Claude Code TUI would make the child think it's - // nested. Other AI spawn paths (runner.js, agentCliSpawning.js) strip it - // for the same reason. - // For OpenCode Ollama providers, build dynamic OPENCODE_CONFIG_CONTENT with - // the models map so --model is accepted (the static env var lacked this). - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). This PTY runs the - // CLI directly, so there is no login shell to rewrite PWD for us. - const opencodeEnv = buildOpencodeEnvVars(provider, provider.defaultModel); - const childEnv = withSpawnCwdEnv( - { ...process.env, ...(provider.envVars || {}), ...opencodeEnv, TERM: 'xterm-256color', COLORTERM: 'truecolor' }, - workingDir, - ); - delete childEnv.CLAUDECODE; + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. The PWD pin matters here because + // this PTY runs the CLI directly, so there is no login shell to rewrite PWD + // for us. TERM/COLORTERM go in `extra` so they override any provider setting — + // the PTY is always a truecolor xterm regardless of provider config. No + // `guard`: this is a Run Prompt TUI, not an autonomous agent. + const childEnv = buildCliChildEnv({ + provider, + model: provider.defaultModel, + cwd: workingDir, + extra: { TERM: 'xterm-256color', COLORTERM: 'truecolor' }, + }); let ptyProcess; try { diff --git a/server/lib/tuiUsageScrape.js b/server/lib/tuiUsageScrape.js index 0f5512875c..1c3114bac7 100644 --- a/server/lib/tuiUsageScrape.js +++ b/server/lib/tuiUsageScrape.js @@ -4,7 +4,7 @@ import { tmpdir } from 'os'; import { join } from 'path'; import { stripAnsi } from './ansiStrip.js'; import { sleep } from './fileUtils.js'; -import { withSpawnCwdEnv } from './spawnCwd.js'; +import { buildCliChildEnv } from './cliChildEnv.js'; /** * Drive an interactive coding-agent TUI (Antigravity `agy`, Grok Build `grok`) @@ -96,16 +96,18 @@ export async function scrapeTuiUsage({ mkdirSync(sandboxDir, { recursive: true }); - // Provider envVars (auth/config) merged over process.env, then the PTY hints. - // CLAUDECODE leaks when PortOS itself runs under Claude Code; strip it so a - // spawned TUI doesn't think it's nested (mirrors tuiPromptRunner.js). - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). Keeps the scrape in - // the throwaway sandbox instead of opening the PortOS checkout as a project. - const env = withSpawnCwdEnv( - { ...process.env, ...extraEnv, TERM: 'xterm-256color', COLORTERM: 'truecolor' }, - sandboxDir, - ); - delete env.CLAUDECODE; + // Shared composition (provider envVars + PWD pin + CLAUDECODE strip) — see + // buildCliChildEnv. `extraEnv` is the provider's auth/config env, handed in + // loose rather than as a provider record; with no command to classify, the + // OpenCode layer is a no-op here. TERM/COLORTERM go in `extra` so the PTY is + // always a truecolor xterm regardless of provider config (mirrors + // tuiPromptRunner.js). The PWD pin keeps the scrape in the throwaway sandbox + // instead of opening the PortOS checkout as a project. + const env = buildCliChildEnv({ + provider: { envVars: extraEnv }, + cwd: sandboxDir, + extra: { TERM: 'xterm-256color', COLORTERM: 'truecolor' }, + }); let pty; try { diff --git a/server/services/agentCliSpawning.js b/server/services/agentCliSpawning.js index 84f306f940..092b32d43e 100644 --- a/server/services/agentCliSpawning.js +++ b/server/services/agentCliSpawning.js @@ -30,11 +30,9 @@ import { prepareCliPrompt } from '../lib/cliProviderArgs.js'; import { isGrokCommand, ensureGrokHeadlessArgs } from '../lib/grok.js'; import { isKimiCommand, ensureKimiHeadlessArgs } from '../lib/kimi.js'; import { resolveCliModel, buildEffortArgs, buildCodexStartupArgs, resolveBedrockCliModel, prefixOpencodeModel, hasModelFlag, isOpencodeCommand, applyLeanClaudeArgs, providerSuppliesGithubToken } from '../lib/providerModels.js'; -import { agentGuardEnv } from '../lib/agentGuard/index.js'; import { resolveForgeTokenEnv } from './git.js'; -import { buildOpencodeEnvVars } from '../lib/opencodeConfig.js'; import { prepareCliSpawn, killProcessTree } from '../lib/bufferedSpawn.js'; -import { withSpawnCwdEnv } from '../lib/spawnCwd.js'; +import { buildCliChildEnv } from '../lib/cliChildEnv.js'; import { resolvePrCompletion } from '../lib/prDisposition.js'; const AGENTS_DIR = PATHS.cosAgents; @@ -483,17 +481,19 @@ export async function spawnDirectly({ providerSuppliesGithubToken(provider) ? Promise.resolve({}) : resolveForgeTokenEnv(cwd), ]); - // For OpenCode Ollama providers, build dynamic OPENCODE_CONFIG_CONTENT with - // the models map so --model is accepted (the static env var lacked this). - const opencodeEnv = buildOpencodeEnvVars(provider, model); - - // The pm2 shim must be prepended onto the FINAL PATH (after any - // provider.envVars override) so a `--dangerously-skip-permissions` agent - // can't `pm2 kill` the shared daemon. opencodeEnv comes LAST to override - // the static OPENCODE_CONFIG_CONTENT in provider.envVars; forgeTokenEnv sits - // before provider.envVars so an explicit provider GH_TOKEN override still wins. - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). - const childEnv = (() => { const e = withSpawnCwdEnv({ ...process.env, ...forgeTokenEnv, ...claudeSettingsEnv, ...provider.envVars, ...opencodeEnv }, cwd); delete e.CLAUDECODE; Object.assign(e, agentGuardEnv(e)); return e; })(); + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. forgeTokenEnv/claudeSettingsEnv go + // in `before` so they sit UNDER provider.envVars and an explicit provider + // GH_TOKEN override still wins. `guard: true` prepends the pm2 shim onto the + // final PATH so a `--dangerously-skip-permissions` agent can't `pm2 kill` the + // shared daemon. + const childEnv = buildCliChildEnv({ + before: { ...forgeTokenEnv, ...claudeSettingsEnv }, + provider, + model, + cwd, + guard: true, + }); // Resolve a bare npm-installed CLI (a .cmd/.bat shim on Windows) to its real // path and wrap a shim as `cmd.exe /c ` so spawn() under shell:false diff --git a/server/services/askService.js b/server/services/askService.js index 9bbc207c10..45faa46e0d 100644 --- a/server/services/askService.js +++ b/server/services/askService.js @@ -36,6 +36,7 @@ import { ensureAntigravityPrintArgs, isAntigravityCliProvider } from '../lib/ant import { isGrokCommand, ensureGrokHeadlessArgs } from '../lib/grok.js'; import { prepareCliPrompt } from '../lib/cliProviderArgs.js'; import { prepareCliSpawn } from '../lib/bufferedSpawn.js'; +import { buildCliChildEnv } from '../lib/cliChildEnv.js'; import { ensureProviderReady as ensureOllamaProviderReady } from './ollamaManager.js'; import { evaluateSecretEndpoint } from '../lib/aiToolkit/internal/endpointGuard.js'; @@ -603,7 +604,14 @@ async function* streamCompletion(provider, model, prompt, signal) { // --print VALUE (agy doesn't read stdin); grok's /dev/stdin sentinel via stdin // (POSIX) / temp file (Windows); everyone else via stdin (writePromptToStdin). const { args: deliveredArgs, useStdin: writePromptToStdin, cleanup: cleanupPromptFile } = prepareCliPrompt(provider.command, args, prompt); - const childEnv = (() => { const e = { ...process.env, ...provider.envVars }; delete e.CLAUDECODE; return e; })(); + // Shared composition (provider.envVars + OpenCode models map + CLAUDECODE + // strip) — see buildCliChildEnv. No cwd is passed to spawn below, so there is + // no PWD to pin; no `guard` — Ask is a one-shot answer, not an agent. Routing + // through the shared builder also brings the OpenCode declared-models map here + // for the first time, so the `--model` this path injects above isn't rejected + // by an Ollama-backed OpenCode provider (the #2190 fix, previously applied + // only at the runner/agent sites). + const childEnv = buildCliChildEnv({ provider, model: cliModel }); // Resolve a bare npm-installed CLI (a .cmd/.bat shim on Windows) to its real // path and wrap it as `cmd.exe /c ` so spawn() under shell:false can // launch it — a bare shim name ENOENTs otherwise. No-op off Windows. Mirrors diff --git a/server/services/runner.js b/server/services/runner.js index 57f5829fb2..821b51b602 100644 --- a/server/services/runner.js +++ b/server/services/runner.js @@ -6,11 +6,10 @@ import { spawn } from 'child_process'; import { writeFile, readFile } from 'fs/promises'; import { join } from 'path'; import { atomicWrite, ensureDir, tryReadFile, PATHS } from '../lib/fileUtils.js'; -import { resolveSpawnCwd, withSpawnCwdEnv } from '../lib/spawnCwd.js'; +import { resolveSpawnCwd } from '../lib/spawnCwd.js'; import { hasModelFlag, extractBakedModel } from '../lib/providerModels.js'; -import { buildOpencodeEnvVars } from '../lib/opencodeConfig.js'; import { buildCliArgs, prepareCliPrompt } from '../lib/cliProviderArgs.js'; -import { agentGuardEnv } from '../lib/agentGuard/index.js'; +import { buildCliChildEnv } from '../lib/cliChildEnv.js'; import { createImmediateFallbackSignalDetector } from '../lib/aiToolkit/errorDetection.js'; import { killProcessTree, resolveWindowsExecutable, prepareWindowsSafeSpawn } from '../lib/bufferedSpawn.js'; import { @@ -261,18 +260,16 @@ export async function executeCliRun({ runId, provider, prompt, workspacePath, on const { args, useStdin, cleanup: cleanupPromptFile } = prepareCliPrompt(provider.command, builtArgs, prompt); console.log(`🚀 Executing CLI: ${provider.command} (${prompt.length} chars via ${useStdin ? 'stdin' : 'argv'})`); - // Prepend the pm2 shim (agentGuardEnv) onto the final PATH so an unrestricted - // agent can't `pm2 kill` the shared daemon. See server/lib/agentGuard. - // buildOpencodeEnvVars rebuilds OPENCODE_CONFIG_CONTENT with a declared models - // map for OpenCode Ollama providers (empty/no-op otherwise) so the injected - // `--model ollama/` isn't rejected as "not valid" — see issue-2190. - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). - const childEnv = withSpawnCwdEnv( - { ...process.env, ...provider.envVars, ...buildOpencodeEnvVars(provider, provider.defaultModel) }, - effectiveCwd, - ); - delete childEnv.CLAUDECODE; - Object.assign(childEnv, agentGuardEnv(childEnv)); + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. `guard: true` prepends the pm2 + // shim onto the final PATH so an unrestricted agent can't `pm2 kill` the + // shared daemon. + const childEnv = buildCliChildEnv({ + provider, + model: provider.defaultModel, + cwd: effectiveCwd, + guard: true, + }); // See the executeCliRun docblock above for why this is a resolve+wrap, not // a shell:true. Resolved against `childEnv` (not bare process.env) so a diff --git a/server/services/visionCli.js b/server/services/visionCli.js index 7b1af653be..65850d8c15 100644 --- a/server/services/visionCli.js +++ b/server/services/visionCli.js @@ -31,7 +31,7 @@ import { buildCliArgs, prepareCliPrompt } from '../lib/cliProviderArgs.js'; import { resolveCliModel, isCodexProvider, buildCodexStartupArgs } from '../lib/providerModels.js'; import { extractCodexAssistant, extractCodexAssistantTail } from '../lib/codexAssistantExtract.js'; import { killProcessTree, resolveWindowsExecutable, prepareWindowsSafeSpawn } from '../lib/bufferedSpawn.js'; -import { withSpawnCwdEnv } from '../lib/spawnCwd.js'; +import { buildCliChildEnv } from '../lib/cliChildEnv.js'; const CLI_VISION_TIMEOUT_MS = 120000; const IMAGE_BASENAME = 'vision-input.png'; @@ -128,13 +128,18 @@ export async function describeImageViaCli({ const { args: deliveredArgs, useStdin: writePromptToStdin, cleanup } = prepareCliPrompt(command, args, stdin); cleanupPromptFile = cleanup; - // Pin PWD to the spawn cwd — see withSpawnCwdEnv (#3193). Load-bearing here: - // the non-codex branch above tells the CLI the image is "in the current - // directory", and the vision provider is user-configurable — so on OpenCode - // (which resolves its project root from PWD) a stale value both hides - // vision-input.png and turns the CLI loose in the PortOS checkout. - const childEnv = withSpawnCwdEnv({ ...process.env, ...provider?.envVars }, cwd); - delete childEnv.CLAUDECODE; + // Shared composition (provider.envVars + OpenCode models map + PWD pin + + // CLAUDECODE strip) — see buildCliChildEnv. The PWD pin is load-bearing + // here: the non-codex branch above tells the CLI the image is "in the + // current directory", and the vision provider is user-configurable — so on + // OpenCode (which resolves its project root from PWD) a stale value both + // hides vision-input.png and turns the CLI loose in the PortOS checkout. + // Routing through the shared builder also brings the OpenCode declared-models + // map here for the first time, so the `--model` buildCliVisionInvocation + // injects isn't rejected by an Ollama-backed OpenCode provider (the #2190 + // fix, previously applied only at the runner/agent sites). No `guard` — + // vision is a one-shot describe, not an agent. + const childEnv = buildCliChildEnv({ provider, model: visionModel, cwd }); // npm-installed CLI providers are .cmd/.bat shims on Windows; resolve+wrap // (cmd.exe /c) instead of enabling a shell. This matters even more here From 3eaed52c9b81b19bf862d940a330a7afe42b17e4 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 15:42:56 -0700 Subject: [PATCH 2/6] docs([issue-3194]): log issue #3194 to changelog --- .changelog/NEXT.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index d76cb8df7e..daf0af9c8c 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -33,3 +33,11 @@ - **Chief of Staff feedback now captures why an agent helped or missed the mark.** Add detail to positive, negative, or neutral ratings so future CoS learning has the context needed to improve. - **Branch reconciler prioritizes recognized work branches.** The in-flight set handed to the coordinator agent is now ordered by branch prefix — `claim/`, `cos/`, `next/`, `feature/`, `fix/`, and other conventional prefixes are reconciled ahead of unrecognized/ad-hoc branches — so a bounded run spends its budget on real deliverables first. Both `/` and `-` separators match. + +## AI providers + +- **[issue-3194] Ask and image captioning now work on a local OpenCode provider.** If you pointed Ask Yourself or vision captioning at an OpenCode provider backed by Ollama, the run failed with the model reported as "not valid" — OpenCode has to be told which local models exist before it will accept one, and only the agent and Run Prompt paths were doing that. Every path that starts a coding CLI now declares them the same way, so the model you picked is accepted wherever you use it. + +## Internal + +- **[issue-3194] Consolidated the AI-CLI child-environment composition.** Eight spawn sites each rebuilt the same environment tuple by hand (provider env vars, the OpenCode declared-models map, the working-directory pin, the nested-session strip, and the pm2 guard shim for agents), so every environment-level fix had to sweep all eight and a missed site failed silently. A single `buildCliChildEnv` helper now owns it, preserving each site's original precedence, and a discovery-based test fails when a new spawn site composes the environment by hand instead. From 4a98bba4278e388f6081d2f9997a2097eb3cd3c6 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 15:58:32 -0700 Subject: [PATCH 3/6] refactor([issue-3194]): extract composeProviderEnv and close the two delta sites The quality pass found the consolidation stopped two sites short of its own claim. agentLifecycle's spawnViaRunner payload and agentTuiSpawning's shell-session overlay build the same forgeTokenEnv -> provider.envVars -> opencodeEnv layering by hand, and neither was visible to the new guard: both compose a DELTA that someone else bases and spawns, so there is no process.env spread and no CLAUDECODE strip to key on. The evidence they are the recurrence site was already in the tree -- a comment at spawnViaRunner records that the #2243/#2190 sweep missed that exact line. composeProviderEnv() now owns the ordered layers; buildCliChildEnv calls it and adds the base env, PWD pin, strip, and guard. Both delta sites route through it. Guard fixes: - Marker B keys on a provider.envVars spread reaching a child process rather than on a process.env base, so it sees delta composers too. The base-env anchor had already been widened once for a parameter-name variant. It also now accepts the parenthesized spread shape, which two real sites use and the earlier pattern read as clean. Window sized empirically: at 1500 the only files flagged across server/ are the two exempt ones, and every pre-refactor hand-rolled site is caught. - spawnCwd.test.js counts buildCliChildEnv as a PWD pin only when the call actually passes a cwd. It is an optional key there, so a bare call next to a cwd-bearing spawn would otherwise satisfy the #3193 guard while reintroducing the bug. - The three agentLifecycle source-regex tests that pinned the old spread literal now assert the composer wiring; the layer ORDER is asserted against the real function in cliChildEnv.test.js. Also: shared collectServerSources/readServerSource in testHelper.js (the two overlapping guards had byte-identical tree walkers and must agree on what a source file is), dropped the fabricated pseudo provider records at cos-runner/tuiUsageScrape in favor of the before slot, dropped three redundant model args (buildOpencodeEnvVars already unions defaultModel), and corrected the stale site count in the docblock and catalog README. Not taken: folding the inline resolveWindowsExecutable + prepareWindowsSafeSpawn pairs into prepareCliSpawn. It is a real duplicate the helper's own docblock invites, but the #1865 tests inject a fake resolveWindowsExecutable and prepareCliSpawn calls the real one internally -- the swap costs that seam at three sites. Worth its own change with the test migration. --- server/cos-runner/index.js | 18 ++-- server/lib/README.md | 2 +- server/lib/cliChildEnv.js | 136 ++++++++++++++----------- server/lib/cliChildEnv.test.js | 128 ++++++++++++++--------- server/lib/cliProviderRun.js | 1 - server/lib/spawnCwd.test.js | 52 ++++++---- server/lib/testHelper.js | 34 +++++++ server/lib/tuiPromptRunner.js | 1 - server/lib/tuiUsageScrape.js | 14 ++- server/services/agentLifecycle.js | 23 +++-- server/services/agentLifecycle.test.js | 47 +++++---- server/services/agentTuiSpawning.js | 27 ++--- server/services/runner.js | 1 - 13 files changed, 293 insertions(+), 191 deletions(-) diff --git a/server/cos-runner/index.js b/server/cos-runner/index.js index ca1a3d7bd2..894740a22a 100644 --- a/server/cos-runner/index.js +++ b/server/cos-runner/index.js @@ -188,17 +188,13 @@ app.post('/spawn', async (req, res) => { // Ensure workspacePath is valid const cwd = workspacePath && typeof workspacePath === 'string' ? workspacePath : ROOT_DIR; - // Shared composition (provider.envVars + OpenCode models map + PWD pin + - // CLAUDECODE strip) — see buildCliChildEnv. The runner receives the provider's - // envVars over HTTP rather than a provider record, so it wraps them into the - // `provider` slot; with no `command` in the payload the OpenCode layer is a - // no-op and the result is exactly `{ ...process.env, ...envVars }` as before. - // The PWD pin is the path #3193 was reported through: the log line above named - // the app's workspace correctly while every OpenCode agent still ran in the - // PortOS folder. No `guard`, preserving today's behavior: this runner is a - // separate process that does not carry the pm2 shim (only the in-process - // agent-spawning paths opt in). - const childEnv = buildCliChildEnv({ provider: { envVars }, cwd }); + // The env arrives already composed — agentLifecycle built it with + // composeProviderEnv (provider.envVars + the OpenCode models map) and POSTed it + // — so this side only needs the base, the PWD pin, and the CLAUDECODE strip. + // The pin is the path #3193 was reported through: the log line above named the + // app's workspace correctly while every OpenCode agent still ran in the PortOS + // folder. No `guard` — this separate process has never carried the pm2 shim. + const childEnv = buildCliChildEnv({ before: envVars, cwd }); // Resolve a bare npm-installed CLI (opencode/codex/claude/… — a .cmd/.bat // shim on Windows) to its real path and wrap a shim as `cmd.exe /c ` so diff --git a/server/lib/README.md b/server/lib/README.md index 0344f9a447..0ccacfa3c9 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -105,7 +105,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `usageRange.js` | `resolveUsageRange({ period, from, to })` — pure period→inclusive-YYYY-MM-DD range resolution for the usage cost report (explicit dates win; `all` unbounded; default 7d). | | `providerTranscriptUsage.js` | Parsers for the real per-message token counts the coding CLIs write to disk (0 tokens to read) — `parseClaudeTranscript` (`~/.claude/projects//*.jsonl`), `parseCodexRollout` (`~/.codex/sessions/YYYY/MM/DD/rollout-*.jsonl`), `claudeProjectSlug`, `totalTranscriptTokens`. Both de-duplicate a format hazard that otherwise inflates counts badly: Claude repeats one response across several lines sharing a `message.id`, and Codex's `total_token_usage` is cumulative and repeated. Each parser returns per-model buckets (`byModel`) plus the message keys it counted (`countedKeys`), and accepts an `exclude` set — that is what stops two overlapping PortOS runs from both billing the same messages. Tolerant of truncated (mid-write) files; consumed by `services/usageReconciler.js`. | | `opencodeConfig.js` | OpenCode config builder — `buildOpencodeEnvVars(provider, model)` builds dynamic `OPENCODE_CONFIG_CONTENT` declaring the models map under `provider.ollama.models` (bare ids) for Ollama-backed OpenCode providers. Fixes --model rejection. | -| `cliChildEnv.js` | `buildCliChildEnv({ baseEnv, before, provider, model, cwd, extra, guard })` — the one place the AI-CLI child environment is composed. Layers `baseEnv → before → provider.envVars → buildOpencodeEnvVars → extra`, pins `PWD` to the spawn cwd, strips `CLAUDECODE`, and (with `guard: true`) prepends the pm2 guard shim onto the final `PATH`. Replaces the hand-rolled copy that lived at all five spawn sites (`services/runner.js`, `cliProviderRun.js`, `services/agentCliSpawning.js`, `tuiPromptRunner.js`, `cos-runner/index.js`) and made every env-level fix an N-file sweep (#3194). The two slots are not interchangeable: `before` sits UNDER `provider.envVars` (forgeTokenEnv/claudeSettingsEnv, so a provider override still wins), `extra` sits OVER it (TERM/COLORTERM for a PTY) — `cliChildEnv.test.js` asserts the composed order per call site. | +| `cliChildEnv.js` | The one place the AI-CLI child environment is composed, replacing the hand-rolled copy every spawn site carried — which made each env-level fix an N-file sweep (#3194). `buildCliChildEnv({ baseEnv, before, provider, model, cwd, extra, guard })` returns a COMPLETE env for `spawn`: layers `baseEnv → before → provider.envVars → buildOpencodeEnvVars → extra`, pins `PWD` to `cwd`, strips `CLAUDECODE`, and (with `guard: true`) prepends the pm2 guard shim onto the final `PATH`. `composeProviderEnv({ before, provider, model, extra })` returns just the ordered provider layers, for sites that build a DELTA someone else bases and spawns (the CoS runner payload, a shell-session overlay). The two slots are not interchangeable: `before` sits UNDER `provider.envVars` (forgeTokenEnv/claudeSettingsEnv, so a provider override still wins), `extra` sits OVER it (TERM/COLORTERM for a PTY). `cliChildEnv.test.js` asserts the composed order per call site and **discovers** any new site that hand-rolls the tuple instead of calling these — so the call-site list stays in the test, not in prose here. | | `cliProviderArgs.js` | Per-CLI argv conventions (`buildCliArgs`) for stdin prompt delivery — dependency-light extraction from runner.js so out-of-process callers (autofixer) can import it. | | `cliProviderRun.js` | One-shot CLI provider invocation (`pickCliProvider` + `runCliProviderPrompt`) — lightweight path for the autofixer + calendar MCP sync to honor the configured provider/model. | | `grok.js` | xAI Grok Build (`grok`) provider helpers — id/endpoint constants (`GROK_API_ID`/`GROK_CLI_ID`/`GROK_TUI_ID`/`GROK_API_ENDPOINT`), `isGrokCommand`/`isGrokCliProvider`/`isGrokTuiProvider` predicates, `ensureGrokHeadlessArgs`/`ensureGrokTuiArgs` argv builders (grok reads its prompt from `--prompt-file /dev/stdin`, not raw stdin; model selection uses the `GROK_CONFIGURED_DEFAULT` sentinel in `providerModels.js` so PortOS omits `--model` like Antigravity), and `prepareGrokPromptFile` (Windows temp-file delivery fallback). | diff --git a/server/lib/cliChildEnv.js b/server/lib/cliChildEnv.js index 6c648651d6..216b671b9e 100644 --- a/server/lib/cliChildEnv.js +++ b/server/lib/cliChildEnv.js @@ -1,42 +1,43 @@ /** * Child-environment composition for every AI-CLI spawn. * - * Five spawn sites — `services/runner.js` (Run Prompt), `lib/cliProviderRun.js` - * (fire-and-collect), `services/agentCliSpawning.js` (CoS agents), - * `lib/tuiPromptRunner.js` (PTY TUI runs), and `cos-runner/index.js` - * (`POST /spawn`) — each independently rebuilt the same env tuple: + * Ten sites independently rebuilt the same env layering, and three separate + * fixes each paid an N-file sweep to keep them in step: the OpenCode + * declared-models map (#2190/#2243), the `CLAUDECODE` strip, and the `PWD` pin + * (#3193). Every time, the guard against missing a site was "a reviewer noticed" + * or a source-grep test — and one of those sweeps did miss a site (see the + * comment at `services/agentLifecycle.js#spawnViaRunner`). This module owns the + * composition so the next env-level concern is a one-file change (#3194). * - * { ...baseEnv, ...providerEnv, ...opencodeEnv } - * → pin PWD to the spawn cwd - * → strip CLAUDECODE - * → prepend the pm2 guard shim (agent-spawning sites only) + * Two entry points, because the sites split into two shapes: * - * Three separate fixes each paid an N-file sweep to keep those five in step: - * the OpenCode declared-models map (#2190/#2243), the `CLAUDECODE` strip, and - * the `PWD` pin (#3193). Every time, the guard against missing a site was - * "a reviewer noticed" or a source-grep test. This module owns the composition - * so the next env-level concern is a one-file change (#3194). + * - `buildCliChildEnv` — a COMPLETE child env, handed straight to `spawn`. + * - `composeProviderEnv` — just the ordered provider layers, for the sites + * that produce a DELTA someone else bases and spawns (the CoS runner + * payload, a shell session's env overlay). * - * **Precedence is the load-bearing part.** The sites do NOT all layer their - * extras the same way, and flattening them into one order would silently change - * which value wins: + * The layer ORDER is the load-bearing part, and the sites do not all layer the + * same way — flattening them into one order would silently change which value + * wins: * - * - `agentCliSpawning` puts `forgeTokenEnv`/`claudeSettingsEnv` BEFORE + * - the agent spawners put `forgeTokenEnv`/`claudeSettingsEnv` BEFORE * `provider.envVars`, so an explicit provider `GH_TOKEN` still overrides the - * repo-owner-pinned one. - * - `tuiPromptRunner` puts `TERM`/`COLORTERM` AFTER everything, so the PTY - * always gets a truecolor terminal regardless of provider config. + * repo-owner-pinned one; + * - the PTY runners put `TERM`/`COLORTERM` AFTER everything, so the terminal + * is always truecolor regardless of provider config. * * Hence two distinct slots — `before` (lower precedence than the provider) and - * `extra` (higher) — rather than one `extra`. `cliChildEnv.test.js` asserts the - * exact composed order for each of the five call sites. + * `extra` (higher) — rather than one. `cliChildEnv.test.js` asserts the composed + * order for each call site, and fails when a new site hand-rolls it instead. * * Deliberately NOT in scope: a `spawnAiCli({ command, args, cwd, env })` wrapper - * that would also own cwd resolution and the Windows `.cmd` shim. The eight - * spawn shapes differ too much (child_process vs node-pty, stdio variants, - * kill-tree wiring) to fold in the same change. `lib/aiToolkit/runner.js` is - * also out of scope: it is vendored and must not import out to other PortOS - * modules (see `lib/aiToolkit/CLAUDE.md`), so it keeps its inline pin. + * that would also own cwd resolution and the Windows `.cmd` shim. The spawn + * shapes differ too much (child_process vs node-pty vs an HTTP hand-off to the + * CoS runner, stdio variants, kill-tree wiring) to fold in the same change. + * `lib/aiToolkit/runner.js` is also out of scope: it is vendored and must not + * import out to other PortOS modules (see `lib/aiToolkit/CLAUDE.md`), and its + * spawn is dormant under PortOS's `setCliRunner` override, so it keeps its + * inline copy. */ import { withSpawnCwdEnv } from './spawnCwd.js'; @@ -44,33 +45,60 @@ import { buildOpencodeEnvVars } from './opencodeConfig.js'; import { agentGuardEnv } from './agentGuard/index.js'; /** - * Compose the environment an AI-CLI child process is spawned with. + * The ordered provider env layers, without a base env, PWD pin, or strip. * - * Layering, lowest precedence first: - * `baseEnv` → `before` → `provider.envVars` → OpenCode env → `extra` - * then `PWD` is pinned to `cwd`, `CLAUDECODE` is stripped, and — only when - * `guard` is true — the pm2 guard shim is prepended to the final `PATH`. + * Use this for a site that builds a DELTA — an overlay another layer bases and + * spawns (`spawnAgentViaRunner`'s `envVars` payload, `createShellSession`'s + * `env`). Sites that spawn directly want `buildCliChildEnv` instead. * * @param {object} options - * @param {NodeJS.ProcessEnv|object} [options.baseEnv=process.env] - env to start from. - * Callers that must not leak host credentials to an autonomous agent (e.g. the - * autofixer) pass a sanitized allowlist here; every later layer still overlays it. - * @param {object|null} [options.before] - env spread AFTER `baseEnv` but BEFORE - * `provider.envVars`, so a provider-configured value still wins over it - * (forgeTokenEnv, claudeSettingsEnv). + * @param {object|null} [options.before] - layered first, so `provider.envVars` + * overrides it (forgeTokenEnv, claudeSettingsEnv). * @param {{command?:string, envVars?:object, models?:string[], defaultModel?:string|null, ollamaBacked?:boolean}|null} [options.provider] - * - the provider record. `provider.envVars` is layered in, and the provider's - * command/ollamaBacked flags decide whether an OpenCode config is built. - * @param {string|null} [options.model] - the model being run this invocation, used - * to build the OpenCode declared-models map. Pass `provider.defaultModel` when - * the site has no per-call model. + * @param {string|null} [options.model] - the model being run this invocation, + * unioned into the OpenCode declared-models map. Omit when the site has no + * per-call model — `provider.defaultModel` is always declared regardless. + * @param {object|null} [options.extra] - layered last, so it overrides every + * other layer including `provider.envVars` (TERM/COLORTERM for a PTY). + * @returns {object} a fresh object holding only these layers + */ +export function composeProviderEnv({ before = null, provider = null, model = null, extra = null } = {}) { + return { + ...(before || {}), + ...(provider?.envVars || {}), + // Rebuilds OPENCODE_CONFIG_CONTENT with a declared models map for OpenCode + // Ollama providers (an empty object for everyone else) so the injected + // `--model ollama/` isn't rejected as "not valid" — see #2190. It lands + // after provider.envVars to override the provider's STATIC + // OPENCODE_CONFIG_CONTENT, which it was built from. + ...buildOpencodeEnvVars(provider, model), + ...(extra || {}), + }; +} + +/** + * Compose the complete environment an AI-CLI child process is spawned with. + * + * Layering, lowest precedence first: `baseEnv` → the `composeProviderEnv` layers + * → then `PWD` pinned to `cwd`, `CLAUDECODE` stripped, and — only when `guard` + * is true — the pm2 guard shim prepended to the final `PATH`. + * + * @param {object} options + * @param {NodeJS.ProcessEnv|object} [options.baseEnv=process.env] - env to start + * from. Callers that must not leak host credentials to an autonomous agent + * (e.g. the autofixer) pass a sanitized allowlist; every later layer still + * overlays it. + * @param {object|null} [options.before] - see `composeProviderEnv`. + * @param {object|null} [options.provider] - see `composeProviderEnv`. + * @param {string|null} [options.model] - see `composeProviderEnv`. * @param {string|undefined|null} options.cwd - the directory being handed to * `spawn`/`pty.spawn`; `PWD` is pinned to it (see `withSpawnCwdEnv`, #3193). - * @param {object|null} [options.extra] - env spread LAST, so it overrides every - * other layer including `provider.envVars` (TERM/COLORTERM for a PTY). + * Pass it whenever the spawn names a cwd — omitting it silently reopens #3193, + * which is why `spawnCwd.test.js` only counts a call carrying `cwd` as a pin. + * @param {object|null} [options.extra] - see `composeProviderEnv`. * @param {boolean} [options.guard=false] - prepend the pm2 guard shim onto the * final `PATH` so an unrestricted agent can't `pm2 kill` the shared daemon. - * Only the two agent-spawning sites opt in; the Run Prompt / fire-and-collect + * Only the agent-spawning sites opt in; the Run Prompt / fire-and-collect * paths keep today's unguarded behavior. * @returns {object} a fresh env object safe to hand straight to `spawn` */ @@ -83,25 +111,13 @@ export function buildCliChildEnv({ extra = null, guard = false, } = {}) { - // buildOpencodeEnvVars rebuilds OPENCODE_CONFIG_CONTENT with a declared models - // map for OpenCode Ollama providers (an empty object for everyone else) so the - // injected `--model ollama/` isn't rejected as "not valid" — see #2190. - // It comes after provider.envVars to override the provider's STATIC - // OPENCODE_CONFIG_CONTENT, which it was built from. const env = withSpawnCwdEnv( - { - ...baseEnv, - ...(before || {}), - ...(provider?.envVars || {}), - ...buildOpencodeEnvVars(provider, model), - ...(extra || {}), - }, + { ...baseEnv, ...composeProviderEnv({ before, provider, model, extra }) }, cwd, ); // CLAUDECODE is set when PortOS itself runs inside Claude Code; passing it - // through would make a spawned Claude CLI think it's nested inside the parent - // session. + // through would make a spawned Claude CLI think it's nested in that session. delete env.CLAUDECODE; // Must be applied to the FINAL PATH (after every override above), or a diff --git a/server/lib/cliChildEnv.test.js b/server/lib/cliChildEnv.test.js index d3277dd4fd..c9e224d18b 100644 --- a/server/lib/cliChildEnv.test.js +++ b/server/lib/cliChildEnv.test.js @@ -1,9 +1,7 @@ import { describe, it, expect } from 'vitest'; -import { readFileSync, readdirSync } from 'fs'; -import { join, relative } from 'path'; -import { fileURLToPath } from 'url'; -import { buildCliChildEnv } from './cliChildEnv.js'; +import { buildCliChildEnv, composeProviderEnv } from './cliChildEnv.js'; import { AGENT_GUARD_BIN } from './agentGuard/index.js'; +import { collectServerSources, readServerSource } from './testHelper.js'; // An OpenCode provider that IS ollama-backed — the only shape for which // buildOpencodeEnvVars returns anything. Everyone else gets `{}`, which is why @@ -64,12 +62,11 @@ describe('buildCliChildEnv — layering', () => { }); it('tolerates an absent provider / before / extra', () => { - expect(buildCliChildEnv({ baseEnv: { A: '1' } })).toEqual({ A: '1' }); expect(buildCliChildEnv({ baseEnv: { A: '1' }, provider: null, before: null, extra: null })).toEqual({ A: '1' }); }); it('defaults baseEnv to process.env', () => { - expect(buildCliChildEnv({ cwd: undefined }).PATH).toBe(process.env.PATH); + expect(buildCliChildEnv().PATH).toBe(process.env.PATH); }); it('copies rather than mutating the caller env', () => { @@ -132,6 +129,44 @@ describe('buildCliChildEnv — pm2 guard', () => { }); }); +// composeProviderEnv is the same layering WITHOUT a base env, PWD pin, or strip +// — for the two sites that build a DELTA someone else bases and spawns. Those +// are exactly the sites an earlier draft of the guard could not see, and where +// the OpenCode sweep was missed once before. +describe('composeProviderEnv — delta for sites that do not spawn directly', () => { + it('emits only the provider layers, with no base env, PWD, or strip', () => { + const delta = composeProviderEnv({ + before: { GH_TOKEN: 'forge' }, + provider: { envVars: { GH_TOKEN: 'provider', CLAUDECODE: '1' } }, + }); + // No process.env keys leak in — the consumer supplies the base. + expect(delta).toEqual({ GH_TOKEN: 'provider', CLAUDECODE: '1' }); + // And no PWD is invented for a caller that has no cwd to pin. + expect(delta.PWD).toBeUndefined(); + }); + + it('keeps the same layer order buildCliChildEnv uses', () => { + expect(composeProviderEnv({ + before: { K: 'before' }, + provider: { envVars: { K: 'provider' } }, + extra: { K: 'extra' }, + }).K).toBe('extra'); + }); + + it('declares the OpenCode models map for the runner payload (#2243/#2190)', () => { + // agentLifecycle hands this to the cos-runner over HTTP, which has no + // provider record of its own — so the map has to be baked in HERE or the + // runner-spawned agent rejects its own --model. + const delta = composeProviderEnv({ provider: OLLAMA_OPENCODE, model: 'llama3.1:8b' }); + expect(declaredModels(delta).sort()).toEqual(['llama3.1:8b', 'qwen2.5:7b']); + }); + + it('is what buildCliChildEnv layers over its base env', () => { + const layers = { before: { A: '1' }, provider: { envVars: { B: '2' } }, extra: { C: '3' } }; + expect(buildCliChildEnv({ baseEnv: {}, ...layers })).toEqual(composeProviderEnv(layers)); + }); +}); + // One case per real call site, asserting the precedence THAT site depends on. // The sites do not all layer the same way, so a single "extra wins" rule would // have silently changed two of them — these pin the actual contracts. @@ -145,10 +180,10 @@ describe('buildCliChildEnv — per-call-site composition', () => { }); it('cliProviderRun.js: honors a sanitized baseEnv instead of process.env', () => { - // The autofixer passes an allowlist so host credentials never reach the CLI. + // The autofixer passes an allowlist so host credentials never reach the CLI — + // so the builder must not smuggle process.env back in under it. const env = buildCliChildEnv({ baseEnv: { PATH: '/usr/bin' }, provider: { envVars: {} }, cwd: '/w' }); - expect(env.HOME).toBeUndefined(); - expect(env.PATH).toBe('/usr/bin'); + expect(Object.keys(env).sort()).toEqual(['PATH', 'PWD']); }); it('agentCliSpawning.js: forgeToken/claudeSettings sit UNDER provider.envVars', () => { @@ -215,22 +250,15 @@ describe('buildCliChildEnv — per-call-site composition', () => { // an allowlist of "these files must call the builder" passes the day someone // adds a ninth spawn site, which is exactly when the guard needs to fire. describe('no spawn site rebuilds the CLI child env by hand', () => { - const SERVER_DIR = fileURLToPath(new URL('..', import.meta.url)); - // Files allowed to compose the tuple themselves, each with the reason. const EXEMPT = new Map([ ['lib/cliChildEnv.js', 'this module IS the shared composer'], - ['lib/aiToolkit/runner.js', 'vendored toolkit — must not import out to other PortOS modules'], + // Worth stating both halves: the import constraint is why it cannot call the + // composer, and the dormancy is why its missing CLAUDECODE strip / OpenCode + // map is not a live PortOS gap someone needs to chase. + ['lib/aiToolkit/runner.js', 'vendored toolkit — must not import out to other PortOS modules, and its spawn is dormant under PortOS\'s setCliRunner override'], ]); - const collectSources = (dir) => readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { - if (entry.name === 'node_modules' || entry.name.startsWith('.')) return []; - const abs = join(dir, entry.name); - if (entry.isDirectory()) return collectSources(abs); - if (!entry.name.endsWith('.js') || entry.name.endsWith('.test.js')) return []; - return [relative(SERVER_DIR, abs)]; - }); - // Two independent markers, because either one alone has a blind spot: a new // site could strip CLAUDECODE without spreading provider.envVars, or spread // provider.envVars while forgetting the strip (which is itself the bug). @@ -239,45 +267,51 @@ describe('no spawn site rebuilds the CLI child env by hand', () => { // strips it, and nothing else in the tree does. const STRIPS_CLAUDECODE = /delete\s+[A-Za-z_$][\w$]*\.CLAUDECODE\b/; - // Marker B — a provider-env-over-base composition that is handed to a child - // running in a workspace. The `cwd` requirement is what separates a real spawn - // env from the two shapes that merge the same objects for a different purpose - // and correctly need none of this: a model-id LOOKUP env - // (`resolveBedrockCliModel({ env })`, `resolveWindowsExecutable(…, searchEnv)`) - // and a capability PROBE (`agy models`, `--version`) — neither runs a model, - // writes files, or has a workspace to be misrouted into. + // Marker B — a `provider.envVars` spread that reaches a child process. // - // A window rather than a parser: the composition is always one object literal. - // KNOWN BLIND SPOT, stated rather than papered over: a real spawn env built - // with NO cwd (askService's Ask path is the one such shape today) is invisible - // to this marker. Marker A still catches it as long as the new site strips - // CLAUDECODE — and a site that does not strip it has the very bug marker B - // was never going to be able to name. + // Keyed on the spread + the handoff, NOT on a `...process.env` base. An + // earlier draft anchored on the base env and had to be widened once already + // (for cliProviderRun's `baseEnv` parameter name) — and it still could not see + // the two sites that compose a DELTA someone else bases: agentLifecycle's + // runner payload and agentTuiSpawning's shell overlay, which is exactly where + // the OpenCode sweep was missed once before. Requiring only the spread and a + // nearby handoff catches both shapes. + // + // The handoff requirement is what separates a real child env from the two + // shapes that merge the same objects for a different purpose and correctly + // need none of this: a model-id LOOKUP env (`resolveBedrockCliModel({ env })`, + // `resolveWindowsExecutable(…, searchEnv)`) and a capability PROBE + // (`agy models`, `--version`) — neither runs a model, writes files, nor has a + // workspace to be misrouted into. + // + // A window rather than a parser, sized from the real tree: the widest real gap + // is ~1400 chars (agentTuiSpawning's `env:` overlay sits that far below its + // `createShellSession(`, behind a long comment block), so the back-window is + // 1500. Verified empirically — at 1500 the only files flagged across all of + // `server/` are the two EXEMPT ones, and every pre-refactor hand-rolled site + // is caught. Over-matching costs one EXEMPT line; under-matching silently + // reopens the N-file sweep, so the bias is deliberate. + // + // The optional `(` in the spread pattern matters: `...(provider.envVars || {})` + // is the shape two sites use, and a pattern without it reads them as clean. + const HANDS_OFF_TO_CHILD = /\b(?:spawn|ptySpawn|spawnImpl|pty\.spawn|createShellSession|spawnAgentViaRunner)\s*\(|\benvVars:/; const SPREADS_PROVIDER_ENV_INTO_SPAWN = (src) => { - for (const m of src.matchAll(/\.\.\.\s*(?:process\.env|baseEnv)\b/g)) { - // Wide enough to span the comment blocks that sit between the composition - // and the spawn call it feeds (the vendored toolkit runner has ~500 chars - // of them). Over-matching costs one EXEMPT line; under-matching silently - // reopens the N-file sweep, so the bias is deliberate. - const window = src.slice(m.index, m.index + 900); - if (/\.\.\.\s*[A-Za-z_$][\w$]*\??\.envVars\b/.test(window) && /\bcwd\s*[:,]/.test(window)) return true; + for (const m of src.matchAll(/\.\.\.\s*\(?\s*[A-Za-z_$][\w$]*\??\.envVars\b/g)) { + if (HANDS_OFF_TO_CHILD.test(src.slice(Math.max(0, m.index - 1500), m.index + 900))) return true; } return false; }; - const offenders = () => collectSources(SERVER_DIR).filter((rel) => { - if (EXEMPT.has(rel)) return false; - const src = readFileSync(join(SERVER_DIR, rel), 'utf8'); - return STRIPS_CLAUDECODE.test(src) || SPREADS_PROVIDER_ENV_INTO_SPAWN(src); - }); + const isOffender = (src) => STRIPS_CLAUDECODE.test(src) || SPREADS_PROVIDER_ENV_INTO_SPAWN(src); + const offenders = () => collectServerSources() + .filter((rel) => !EXEMPT.has(rel) && isOffender(readServerSource(rel))); it('finds the exempt files (guard is not vacuous)', () => { // If the markers stop matching even the composer itself, the scan broke and // the assertion below would pass for the wrong reason. for (const rel of EXEMPT.keys()) { - const src = readFileSync(join(SERVER_DIR, rel), 'utf8'); expect( - STRIPS_CLAUDECODE.test(src) || SPREADS_PROVIDER_ENV_INTO_SPAWN(src), + isOffender(readServerSource(rel)), `${rel} no longer matches either marker — the scan broke`, ).toBe(true); } diff --git a/server/lib/cliProviderRun.js b/server/lib/cliProviderRun.js index dae9d49a48..2bbdc44b1b 100644 --- a/server/lib/cliProviderRun.js +++ b/server/lib/cliProviderRun.js @@ -111,7 +111,6 @@ export function runCliProviderPrompt(args = {}) { const childEnv = buildCliChildEnv({ baseEnv, provider: effectiveProvider, - model: effectiveProvider.defaultModel, cwd: effectiveCwd, }); diff --git a/server/lib/spawnCwd.test.js b/server/lib/spawnCwd.test.js index 2b3443245a..d96d1442a4 100644 --- a/server/lib/spawnCwd.test.js +++ b/server/lib/spawnCwd.test.js @@ -1,9 +1,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import { mkdtempSync, rmSync, writeFileSync, readFileSync, readdirSync } from 'fs'; +import { mkdtempSync, rmSync, writeFileSync } from 'fs'; import { tmpdir, homedir } from 'os'; -import { join, relative } from 'path'; -import { fileURLToPath } from 'url'; +import { join } from 'path'; import { resolveSpawnCwd, withSpawnCwdEnv } from './spawnCwd.js'; +import { collectServerSources, readServerSource } from './testHelper.js'; describe('resolveSpawnCwd', () => { let dir; @@ -198,8 +198,7 @@ describe('withSpawnCwdEnv', () => { // cwd-passing spawn fails the suite until it is either pinned or explicitly // exempted below with a reason. describe('every cwd-passing spawn pins PWD', () => { - const SERVER_DIR = fileURLToPath(new URL('..', import.meta.url)); - + // Files whose cwd-passing spawn does NOT need the pin, each with the reason it // is safe. Anything not listed here must pin. The common reason is "this spawns // a tool that resolves paths from its real cwd, never from PWD" — git, gh, @@ -240,14 +239,19 @@ describe('every cwd-passing spawn pins PWD', () => { const SPAWN_CALL_RE = /\b(?:spawn|ptySpawn|spawnImpl|pty\.spawn|bufferedSpawn|execFile|execAsync)\s*\(/g; - /** Recursively collect non-test .js files under `dir`, as server-relative paths. */ - const collectSources = (dir) => readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { - if (entry.name === 'node_modules' || entry.name.startsWith('.')) return []; - const abs = join(dir, entry.name); - if (entry.isDirectory()) return collectSources(abs); - if (!entry.name.endsWith('.js') || entry.name.endsWith('.test.js')) return []; - return [relative(SERVER_DIR, abs)]; - }); + /** + * Count `buildCliChildEnv({ … cwd … })` calls — the shared AI-CLI env composer + * (#3194), which pins PWD internally. Only a call that actually PASSES a cwd + * counts: `cwd` is an optional key there, so a bare `buildCliChildEnv({ provider })` + * next to a `spawn(cmd, args, { cwd })` would otherwise satisfy this guard while + * reintroducing #3193 verbatim. `withSpawnCwdEnv(x, cwd)` can't be called without + * one, so requiring the key keeps the two markers equally strong. + * + * A window rather than a brace match, because an argument object nests + * (`before: { … }`) and `[^}]*` would stop at the inner brace. + */ + const countCwdBearingComposerCalls = (src) => [...src.matchAll(/\bbuildCliChildEnv\s*\(/g)] + .filter((m) => /\bcwd\s*[:,}]/.test(src.slice(m.index, m.index + 400))).length; // A spawn is in scope when its options carry a `cwd` — either written inline // (`spawn(cmd, args, { cwd, ... })`) or FORWARDED from a caller-supplied @@ -291,7 +295,7 @@ describe('every cwd-passing spawn pins PWD', () => { }; it('discovers the cwd-passing spawn sites (guard is not vacuous)', () => { - const found = collectSources(SERVER_DIR).filter((rel) => spawnSitesInScope(readFileSync(join(SERVER_DIR, rel), 'utf8')) > 0); + const found = collectServerSources().filter((rel) => spawnSitesInScope(readServerSource(rel)) > 0); // If this ever collapses toward zero the scan broke (a rename, a new spawn // wrapper) and every assertion below would pass for the wrong reason. expect(found.length, 'expected the scan to still find the known spawn sites').toBeGreaterThan(10); @@ -307,15 +311,15 @@ describe('every cwd-passing spawn pins PWD', () => { // A stale entry is worse than no entry: it silently pre-approves whatever // cwd-passing spawn is added to that file later. it('has no dead EXEMPT / DELEGATES / INLINE_PIN entries', () => { - const inScope = new Set(collectSources(SERVER_DIR).filter((rel) => spawnSitesInScope(readFileSync(join(SERVER_DIR, rel), 'utf8')) > 0)); + const inScope = new Set(collectServerSources().filter((rel) => spawnSitesInScope(readServerSource(rel)) > 0)); const dead = [...EXEMPT.keys(), ...DELEGATES.keys(), ...INLINE_PIN.keys()].filter((rel) => !inScope.has(rel)); expect(dead, 'these entries name files the scan no longer finds — delete them').toEqual([]); }); it('every cwd-passing spawn either pins PWD or is explicitly exempt', () => { const unpinned = []; - for (const rel of collectSources(SERVER_DIR)) { - const src = readFileSync(join(SERVER_DIR, rel), 'utf8'); + for (const rel of collectServerSources()) { + const src = readServerSource(rel); const sites = spawnSitesInScope(src); if (sites === 0) continue; if (EXEMPT.has(rel)) continue; @@ -325,11 +329,19 @@ describe('every cwd-passing spawn pins PWD', () => { // unpinned cwd-spawn is added to it. // Two markers count as a pin: the low-level helper, and buildCliChildEnv — // the shared AI-CLI env composer, which calls withSpawnCwdEnv internally - // (#3194). Counting the composer here is what lets the five AI-CLI spawn - // sites collapse to one call each without going dark to this guard. + // (#3194). Counting the composer here is what lets the AI-CLI spawn sites + // collapse to one call each without going dark to this guard. + // + // The composer's marker REQUIRES a `cwd` in the call. `withSpawnCwdEnv(x, cwd)` + // cannot be called without one, but `buildCliChildEnv({ … })` takes `cwd` as an + // optional key — so a bare `buildCliChildEnv({ provider })` followed by a + // `spawn(cmd, args, { cwd })` would otherwise satisfy this guard while + // reintroducing #3193 verbatim. Requiring the key keeps the two markers + // equally strong. (askService.js legitimately calls it with no cwd — it also + // spawns with no cwd, so it is not in scope here at all.) const pins = INLINE_PIN.has(rel) ? (src.match(/childEnv\.PWD\s*=/g) || []).length - : (src.match(/(?:withSpawnCwdEnv|buildCliChildEnv)\(/g) || []).length; + : (src.match(/withSpawnCwdEnv\(/g) || []).length + countCwdBearingComposerCalls(src); if (pins < sites) unpinned.push(`${rel} (${sites} cwd-passing spawn(s), ${pins} pin(s))`); } expect( diff --git a/server/lib/testHelper.js b/server/lib/testHelper.js index d2b5d881e1..16396cc263 100644 --- a/server/lib/testHelper.js +++ b/server/lib/testHelper.js @@ -5,6 +5,12 @@ */ import { createServer } from 'http'; +import { readdirSync, readFileSync } from 'fs'; +import { join, relative } from 'path'; +import { fileURLToPath } from 'url'; + +/** The `server/` root — the scan root for the source-guard helpers below. */ +export const SERVER_DIR = fileURLToPath(new URL('..', import.meta.url)); function startServer(app) { return new Promise((resolve, reject) => { @@ -132,3 +138,31 @@ export function mockJsonResponse(body, { ok = true, status = 200 } = {}) { export function mockTextResponse(body = '', { ok = true, status = 200 } = {}) { return { ok, status, text: async () => body }; } + +/** + * Every non-test `.js` file under `server/`, as server-relative paths. + * + * Shared by the source-scanning guard suites — `spawnCwd.test.js` (every + * cwd-passing spawn pins PWD, #3193) and `cliChildEnv.test.js` (every AI-CLI + * spawn composes its env through the shared builder, #3194). Those two guards + * deliberately overlap, so they must agree on what "a source file" is: a change + * to the ignore rules here (a new extension, a skipped directory) has to apply + * to both, or one guard silently stops covering files the other still checks. + * + * @param {string} [dir] - directory to walk (defaults to the `server/` root) + * @returns {string[]} paths relative to `server/`, e.g. `services/runner.js` + */ +export function collectServerSources(dir = SERVER_DIR) { + return readdirSync(dir, { withFileTypes: true }).flatMap((entry) => { + if (entry.name === 'node_modules' || entry.name.startsWith('.')) return []; + const abs = join(dir, entry.name); + if (entry.isDirectory()) return collectServerSources(abs); + if (!entry.name.endsWith('.js') || entry.name.endsWith('.test.js')) return []; + return [relative(SERVER_DIR, abs)]; + }); +} + +/** Read a source file named by a `collectServerSources()` path. */ +export function readServerSource(rel) { + return readFileSync(join(SERVER_DIR, rel), 'utf8'); +} diff --git a/server/lib/tuiPromptRunner.js b/server/lib/tuiPromptRunner.js index ad42ecab0b..5940c199d3 100644 --- a/server/lib/tuiPromptRunner.js +++ b/server/lib/tuiPromptRunner.js @@ -178,7 +178,6 @@ ${prompt}`; // `guard`: this is a Run Prompt TUI, not an autonomous agent. const childEnv = buildCliChildEnv({ provider, - model: provider.defaultModel, cwd: workingDir, extra: { TERM: 'xterm-256color', COLORTERM: 'truecolor' }, }); diff --git a/server/lib/tuiUsageScrape.js b/server/lib/tuiUsageScrape.js index 1c3114bac7..70a631d7d4 100644 --- a/server/lib/tuiUsageScrape.js +++ b/server/lib/tuiUsageScrape.js @@ -96,15 +96,13 @@ export async function scrapeTuiUsage({ mkdirSync(sandboxDir, { recursive: true }); - // Shared composition (provider envVars + PWD pin + CLAUDECODE strip) — see - // buildCliChildEnv. `extraEnv` is the provider's auth/config env, handed in - // loose rather than as a provider record; with no command to classify, the - // OpenCode layer is a no-op here. TERM/COLORTERM go in `extra` so the PTY is - // always a truecolor xterm regardless of provider config (mirrors - // tuiPromptRunner.js). The PWD pin keeps the scrape in the throwaway sandbox - // instead of opening the PortOS checkout as a project. + // TERM/COLORTERM go in `extra` so the PTY is always a truecolor xterm + // regardless of the provider's own env (mirrors tuiPromptRunner.js). The PWD + // pin keeps the scrape in the throwaway sandbox instead of opening the PortOS + // checkout as a project. No provider record here — this drives a slash command, + // not a model, so there is nothing for the OpenCode layer to declare. const env = buildCliChildEnv({ - provider: { envVars: extraEnv }, + before: extraEnv, cwd: sandboxDir, extra: { TERM: 'xterm-256color', COLORTERM: 'truecolor' }, }); diff --git a/server/services/agentLifecycle.js b/server/services/agentLifecycle.js index 73bc053d13..bf50d3fe45 100644 --- a/server/services/agentLifecycle.js +++ b/server/services/agentLifecycle.js @@ -38,7 +38,7 @@ import { analyzeAgentFailure } from './agentErrorAnalysis.js'; import { createAgentRun, checkForTaskCommit } from './agentRunTracking.js'; import { buildAgentPrompt, getAppWorkspace } from './agentPromptBuilder.js'; import { isOllamaClaudeProvider, isClaudeCommand, providerSuppliesGithubToken } from '../lib/providerModels.js'; -import { buildOpencodeEnvVars } from '../lib/opencodeConfig.js'; +import { composeProviderEnv } from '../lib/cliChildEnv.js'; import { PROVIDER_TYPES } from '../lib/aiToolkit/constants.js'; import { buildCliSpawnConfig, isClaudeCliProvider, isTuiProvider, getClaudeSettingsEnv, spawnDirectly } from './agentCliSpawning.js'; import { buildTuiSpawnConfig, spawnTuiAgent } from './agentTuiSpawning.js'; @@ -687,22 +687,23 @@ export async function spawnViaRunner(agentId, task, opts) { providerSuppliesGithubToken(provider) ? Promise.resolve({}) : resolveForgeTokenEnv(workspacePath), ]); - // For OpenCode Ollama providers, build dynamic OPENCODE_CONFIG_CONTENT with the - // models map so the injected `--model ollama/` is accepted (empty/no-op - // otherwise). The direct-spawn path (agentCliSpawning.spawnDirectly) and the - // "Run Prompt" path (server/services/runner.js) already do this; the runner - // path omitted it, so an OpenCode Ollama CoS task on the runner would reject - // the model even after the spawn itself succeeds. See issue #2243 / #2190. - const opencodeEnv = buildOpencodeEnvVars(provider, model); - const result = await spawnAgentViaRunner({ agentId, taskId: task.id, prompt, workspacePath, model, - // forgeTokenEnv before provider.envVars so an explicit provider override wins. - envVars: { ...forgeTokenEnv, ...claudeSettingsEnv, ...provider.envVars, ...opencodeEnv }, + // A DELTA, not a full env — the cos-runner bases it on its own process.env + // and does the PWD pin / CLAUDECODE strip. composeProviderEnv owns the layer + // order: forgeTokenEnv before provider.envVars so an explicit provider + // override wins, and the OpenCode declared-models map after it so the + // injected `--model ollama/` is accepted (#2243/#2190 — this path was + // the site that sweep originally missed). + envVars: composeProviderEnv({ + before: { ...forgeTokenEnv, ...claudeSettingsEnv }, + provider, + model, + }), cliCommand: cliConfig.command, cliArgs: cliConfig.args }); diff --git a/server/services/agentLifecycle.test.js b/server/services/agentLifecycle.test.js index d8b34a6e32..1bd9e06700 100644 --- a/server/services/agentLifecycle.test.js +++ b/server/services/agentLifecycle.test.js @@ -441,31 +441,44 @@ describe('runAgentSpawn source — instance provenance + claim ordering (#1563)' }); }); +// These used to be three source-regex assertions pinning a hand-spread env +// literal (`...forgeTokenEnv, ...provider.envVars, ...opencodeEnv`) inside +// spawnViaRunner. The layering now lives in `lib/cliChildEnv.js#composeProviderEnv`, +// where `cliChildEnv.test.js` asserts the ORDER against the real function instead +// of against source text — which is the point of #3194: a grep-shaped guard was +// what let this exact site miss the #2243/#2190 sweep in the first place. +// +// What is still worth pinning HERE is the wiring: that spawnViaRunner routes +// through the shared composer and feeds it the right inputs. describe('agentLifecycle — runner OpenCode Ollama env (#2243 / #2190)', () => { - it('source: imports buildOpencodeEnvVars from opencodeConfig', () => { + const runnerBody = () => { + const fnStart = AGENT_LIFECYCLE_SRC.indexOf('export async function spawnViaRunner'); + expect(fnStart, 'spawnViaRunner must exist').toBeGreaterThan(-1); + return AGENT_LIFECYCLE_SRC.slice(fnStart, fnStart + 4000); + }; + + it('source: composes the runner envVars through the shared composeProviderEnv', () => { expect(AGENT_LIFECYCLE_SRC).toMatch( - /import\s*\{\s*buildOpencodeEnvVars\s*\}\s*from\s*'\.\.\/lib\/opencodeConfig\.js';/ + /import\s*\{\s*composeProviderEnv\s*\}\s*from\s*'\.\.\/lib\/cliChildEnv\.js';/ ); + // The payload must BE the composed env — not a literal that re-spreads it, + // which is how the layer order drifted from the other spawn sites before. + expect(runnerBody()).toMatch(/envVars:\s*composeProviderEnv\(\{/); }); - it('source: spawnViaRunner merges buildOpencodeEnvVars into the runner envVars so --model ollama/ is accepted', () => { - const fnStart = AGENT_LIFECYCLE_SRC.indexOf('export async function spawnViaRunner'); - expect(fnStart, 'spawnViaRunner must exist').toBeGreaterThan(-1); - const fnBody = AGENT_LIFECYCLE_SRC.slice(fnStart, fnStart + 4000); - const buildIdx = fnBody.indexOf('buildOpencodeEnvVars(provider, model)'); - expect(buildIdx, 'must build the opencode env from provider+model').toBeGreaterThan(-1); - expect(fnBody).toMatch(/envVars:\s*\{[^}]*\.\.\.opencodeEnv[^}]*\}/); - expect(fnBody.indexOf('...opencodeEnv'), 'opencodeEnv must be spread AFTER provider.envVars so it overrides the static config') - .toBeGreaterThan(fnBody.indexOf('...provider.envVars')); + it('source: feeds the composer the provider + per-call model so --model ollama/ is accepted', () => { + const fnBody = runnerBody(); + const call = fnBody.slice(fnBody.indexOf('composeProviderEnv({'), fnBody.indexOf('composeProviderEnv({') + 300); + expect(call, 'the OpenCode declared-models map is built from provider+model').toContain('provider,'); + expect(call).toContain('model,'); }); - it("source: spawnViaRunner pins GH_TOKEN via resolveForgeTokenEnv so the runner-spawned agent's `gh` uses the repo-owner account", () => { - const fnStart = AGENT_LIFECYCLE_SRC.indexOf('export async function spawnViaRunner'); - const fnBody = AGENT_LIFECYCLE_SRC.slice(fnStart, fnStart + 4000); + it("source: pins GH_TOKEN via resolveForgeTokenEnv so the runner-spawned agent's `gh` uses the repo-owner account", () => { + const fnBody = runnerBody(); expect(fnBody).toContain('resolveForgeTokenEnv(workspacePath)'); - expect(fnBody).toMatch(/envVars:\s*\{[^}]*\.\.\.forgeTokenEnv[^}]*\}/); - expect(fnBody.indexOf('...forgeTokenEnv'), 'forgeTokenEnv must be spread BEFORE provider.envVars') - .toBeLessThan(fnBody.indexOf('...provider.envVars')); + // `before` is the slot that sits UNDER provider.envVars, so an explicit + // provider GH_TOKEN still wins — passing it as `extra` would invert that. + expect(fnBody).toMatch(/before:\s*\{[^}]*\.\.\.forgeTokenEnv[^}]*\}/); }); }); diff --git a/server/services/agentTuiSpawning.js b/server/services/agentTuiSpawning.js index 45894a11fe..1a2ab881d7 100644 --- a/server/services/agentTuiSpawning.js +++ b/server/services/agentTuiSpawning.js @@ -66,7 +66,7 @@ import { isPasteConfirmed, } from '../lib/tuiHandshake.js'; import { agentGuardEnv } from '../lib/agentGuard/index.js'; -import { buildOpencodeEnvVars } from '../lib/opencodeConfig.js'; +import { composeProviderEnv } from '../lib/cliChildEnv.js'; import { execFile } from 'child_process'; // Agent-specific timing/lifecycle constants (not shared with the one-shot @@ -103,9 +103,6 @@ const DONE_POLL_INTERVAL_MS = 2000; * to bail out via its `finish` path. */ export function createAgentTuiSession({ agentId, provider, model, tuiConfig, cwd, forgeTokenEnv = {}, onData, onExit, onInitialCommandSent }) { - // For OpenCode Ollama providers, build dynamic OPENCODE_CONFIG_CONTENT with - // the models map so --model is accepted (the static env var lacked this). - const opencodeEnv = buildOpencodeEnvVars(provider, model); const sessionId = shellService.createShellSession(null, { cwd, kind: 'agent-tui', @@ -122,15 +119,19 @@ export function createAgentTuiSession({ agentId, provider, model, tuiConfig, cwd // claude's input-readiness only after this so the readiness probe's own // shell activity can't prematurely open the paste gate. onInitialCommandSent, - // agentGuardEnv() prepends the pm2 shim to the agent session's PATH (and - // points it at the real pm2). Spread LAST so it wins over any provider PATH. - // Only AI agent sessions get this — the user's own Shell page does not. - // opencodeEnv comes after provider.envVars to override the static config. - // forgeTokenEnv is threaded in explicitly because buildSafeEnv strips - // GH_TOKEN from the inherited env (see resolveForgeTokenEnv); it goes before - // provider.envVars so an explicit provider GH_TOKEN override still wins, - // matching the direct-CLI and runner spawn paths. - env: { ...forgeTokenEnv, ...(provider.envVars || {}), ...opencodeEnv, ...agentGuardEnv() }, + // A DELTA, not a full env — buildSafeEnv inside createShellSession supplies + // the base and shell.js does the PWD pin. composeProviderEnv owns the layer + // order (forgeTokenEnv before provider.envVars so an explicit provider + // GH_TOKEN still wins; the OpenCode declared-models map after it, overriding + // the static config). forgeTokenEnv has to be threaded in explicitly because + // buildSafeEnv strips GH_TOKEN from the inherited env (resolveForgeTokenEnv). + // + // agentGuardEnv() is spread last so the pm2 shim wins over any provider PATH. + // It reads PATH from process.env rather than the composed env — correct here + // and NOT what buildCliChildEnv's `guard` does, because this is an overlay + // whose real base env is assembled downstream. Only AI agent sessions get + // the shim; the user's own Shell page does not. + env: { ...composeProviderEnv({ before: forgeTokenEnv, provider, model }), ...agentGuardEnv() }, onData, onExit, }); diff --git a/server/services/runner.js b/server/services/runner.js index 821b51b602..ae3f85426c 100644 --- a/server/services/runner.js +++ b/server/services/runner.js @@ -266,7 +266,6 @@ export async function executeCliRun({ runId, provider, prompt, workspacePath, on // shared daemon. const childEnv = buildCliChildEnv({ provider, - model: provider.defaultModel, cwd: effectiveCwd, guard: true, }); From bd55e60e49939ec7477c922c50356784fd84d8ea Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 15:58:44 -0700 Subject: [PATCH 4/6] docs([issue-3194]): correct the consolidated-site count in the changelog --- .changelog/NEXT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/NEXT.md b/.changelog/NEXT.md index daf0af9c8c..d1403d4d4e 100644 --- a/.changelog/NEXT.md +++ b/.changelog/NEXT.md @@ -40,4 +40,4 @@ ## Internal -- **[issue-3194] Consolidated the AI-CLI child-environment composition.** Eight spawn sites each rebuilt the same environment tuple by hand (provider env vars, the OpenCode declared-models map, the working-directory pin, the nested-session strip, and the pm2 guard shim for agents), so every environment-level fix had to sweep all eight and a missed site failed silently. A single `buildCliChildEnv` helper now owns it, preserving each site's original precedence, and a discovery-based test fails when a new spawn site composes the environment by hand instead. +- **[issue-3194] Consolidated the AI-CLI child-environment composition.** Ten spawn sites each rebuilt the same environment tuple by hand (provider env vars, the OpenCode declared-models map, the working-directory pin, the nested-session strip, and the pm2 guard shim for agents), so every environment-level fix had to sweep all ten and a missed site failed silently — which is exactly what happened to the OpenCode fix once already. `buildCliChildEnv` now owns the full child env and `composeProviderEnv` the ordered layers for sites that hand off a partial env, both preserving each site's original precedence. A discovery-based test fails when a new spawn site composes the environment by hand instead of routing through them. From 1775336240800974152ae54a1e713cf93166f630 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 16:00:24 -0700 Subject: [PATCH 5/6] docs([issue-3194]): document the shared source-scan helpers in the lib catalog --- server/lib/README.md | 2 +- server/lib/spawnCwd.test.js | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/server/lib/README.md b/server/lib/README.md index 0ccacfa3c9..5b5a5b557f 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -312,4 +312,4 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `mirrorParity.js` | Source-comparison primitives for the `*.mirror.test.js` server↔client parity tests: `stripCommentsAndNormalize` (so per-side commentary may diverge but logic may not), `extractDeclaration(src, name)` (balanced `{}`/`()`/`[]` walk over `function` / `async function` / `const`), and `compareDeclaration(serverSrc, clientSrc, name)`. Use these instead of hand-rolling a brace-walker per mirror. Pure — no `vitest` import — so callers own the assertions. | | `mockPathsDataRoot.js` | Shared Vitest helpers for `PATHS.data → temp dir` and no-peer record creation guards. | | `settingsTestUtil.js` | `bindSettingsFile(dataRoot)` → `writeSettingsFile`/`mergeSettingsFile`: direct settings.json disk writes that also drop the `getSettings()` read cache (dynamic-import reset) so a stale cache can't survive a bypass-`save()` write. | -| `testHelper.js` | Test helpers: `request()` (supertest-style HTTP) + `mockJsonResponse`/`mockTextResponse` (fetch `Response` mocks read via `.text()`). | +| `testHelper.js` | Test helpers: `request()` (supertest-style HTTP) + `mockJsonResponse`/`mockTextResponse` (fetch `Response` mocks read via `.text()`), plus the source-scan pair `collectServerSources()` / `readServerSource(rel)` (and `SERVER_DIR`) used by the whole-tree guard suites — `spawnCwd.test.js` (#3193) and `cliChildEnv.test.js` (#3194). Those guards overlap deliberately, so they share one definition of "a source file"; change the ignore rules here and both move together. | diff --git a/server/lib/spawnCwd.test.js b/server/lib/spawnCwd.test.js index d96d1442a4..676100d3f6 100644 --- a/server/lib/spawnCwd.test.js +++ b/server/lib/spawnCwd.test.js @@ -198,7 +198,6 @@ describe('withSpawnCwdEnv', () => { // cwd-passing spawn fails the suite until it is either pinned or explicitly // exempted below with a reason. describe('every cwd-passing spawn pins PWD', () => { - // Files whose cwd-passing spawn does NOT need the pin, each with the reason it // is safe. Anything not listed here must pin. The common reason is "this spawns // a tool that resolves paths from its real cwd, never from PWD" — git, gh, From ec3ea2bcad07c6d578f999214cbe810a344bc58d Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Tue, 28 Jul 2026 16:10:32 -0700 Subject: [PATCH 6/6] fix([issue-3194]) address review (claude): composer PWD-pin marker read its surroundings, not the call countCwdBearingComposerCalls tested a fixed 400-char window from the call site. The statement after the composer is nearly always the spawn, whose options carry cwd -- so the window matched the SPAWN's cwd and counted an unpinned buildCliChildEnv({ provider, model }) as a pin. That shape is the natural way to write a new site, and it would have reopened #3193 with the whole suite green. Replaced with a balanced-paren walk over the call's own argument list, plus regression tests pinning both directions (unpinned call beside a cwd-passing spawn = 0 pins; nested before: { ... } neither ends the walk early nor lets it run into the next statement). Also corrected the cos-runner case in cliChildEnv.test.js to mirror the real call (before: envVars, no provider record) and assert the baked-in upstream OPENCODE_CONFIG_CONTENT survives untouched. --- server/lib/cliChildEnv.test.js | 14 ++++++-- server/lib/spawnCwd.test.js | 59 +++++++++++++++++++++++++++++++--- 2 files changed, 66 insertions(+), 7 deletions(-) diff --git a/server/lib/cliChildEnv.test.js b/server/lib/cliChildEnv.test.js index c9e224d18b..4d37941978 100644 --- a/server/lib/cliChildEnv.test.js +++ b/server/lib/cliChildEnv.test.js @@ -216,13 +216,21 @@ describe('buildCliChildEnv — per-call-site composition', () => { }); it('cos-runner/index.js: request envVars over process.env, no OpenCode layer, unguarded', () => { - // The runner receives loose envVars over HTTP with no `command` to classify. + // Mirrors the real call exactly — `before: envVars`, no provider record. The + // runner receives an ALREADY-composed delta over HTTP (agentLifecycle built it + // with composeProviderEnv), so this side must add only the base, the pin, and + // the strip; inventing an OpenCode config here would clobber the baked-in one. const env = buildCliChildEnv({ baseEnv: { A: 'base', PATH: '/usr/bin', CLAUDECODE: '1' }, - provider: { envVars: { A: 'request' } }, + before: { A: 'request', OPENCODE_CONFIG_CONTENT: '{"baked":"upstream"}' }, cwd: '/workspace', }); - expect(env).toEqual({ A: 'request', PATH: '/usr/bin', PWD: '/workspace' }); + expect(env).toEqual({ + A: 'request', + OPENCODE_CONFIG_CONTENT: '{"baked":"upstream"}', + PATH: '/usr/bin', + PWD: '/workspace', + }); }); it('askService.js: no cwd means no PWD is invented', () => { diff --git a/server/lib/spawnCwd.test.js b/server/lib/spawnCwd.test.js index 676100d3f6..86f4ed7d16 100644 --- a/server/lib/spawnCwd.test.js +++ b/server/lib/spawnCwd.test.js @@ -246,11 +246,33 @@ describe('every cwd-passing spawn pins PWD', () => { * reintroducing #3193 verbatim. `withSpawnCwdEnv(x, cwd)` can't be called without * one, so requiring the key keeps the two markers equally strong. * - * A window rather than a brace match, because an argument object nests - * (`before: { … }`) and `[^}]*` would stop at the inner brace. + * This reads the call's OWN argument list via a balanced-paren walk — NOT a + * fixed character window. A window is wrong here in the exact shape that + * matters: the statement after the composer is nearly always the spawn, whose + * options carry `cwd`, so a window sees the SPAWN's cwd and passes an unpinned + * composer call. That is not hypothetical — it is the natural way to write a + * new site, and it silently reopens #3193 with every test still green. */ - const countCwdBearingComposerCalls = (src) => [...src.matchAll(/\bbuildCliChildEnv\s*\(/g)] - .filter((m) => /\bcwd\s*[:,}]/.test(src.slice(m.index, m.index + 400))).length; + const countCwdBearingComposerCalls = (src) => { + let count = 0; + for (const m of src.matchAll(/\bbuildCliChildEnv\s*\(/g)) { + // Walk from the call's open paren to its matching close paren, so nested + // object/array/call braces inside the argument (e.g. `before: { … }`) are + // spanned but the argument list is never overrun. + let depth = 0; + let end = src.length; + for (let i = src.indexOf('(', m.index); i < src.length; i += 1) { + const ch = src[i]; + if (ch === '(' || ch === '{' || ch === '[') depth += 1; + else if (ch === ')' || ch === '}' || ch === ']') { + depth -= 1; + if (depth === 0) { end = i; break; } + } + } + if (/\bcwd\s*[:,}]/.test(src.slice(m.index, end))) count += 1; + } + return count; + }; // A spawn is in scope when its options carry a `cwd` — either written inline // (`spawn(cmd, args, { cwd, ... })`) or FORWARDED from a caller-supplied @@ -293,6 +315,35 @@ describe('every cwd-passing spawn pins PWD', () => { return count; }; + // The composer marker must read the COMPOSER call, not its surroundings. A + // fixed-width window failed exactly here: the statement after the composer is + // nearly always the spawn, whose options carry `cwd`, so the window matched the + // SPAWN's cwd and counted an unpinned composer call as pinned — passing this + // whole suite while the child inherited a stale PWD (#3193 reopened, silently). + it('does not count a composer call that omits cwd, even next to a cwd-passing spawn', () => { + const unpinned = [ + 'const childEnv = buildCliChildEnv({ provider, model });', + 'const child = spawn(command, args, { cwd, env: childEnv });', + ].join('\n'); + expect(spawnSitesInScope(unpinned), 'the spawn must still be in scope').toBe(1); + expect( + countCwdBearingComposerCalls(unpinned), + 'a buildCliChildEnv call with no cwd must NOT count as a PWD pin', + ).toBe(0); + }); + + it('counts a composer call that passes cwd, including across a nested argument', () => { + expect(countCwdBearingComposerCalls('buildCliChildEnv({ provider, cwd });')).toBe(1); + // A nested object in an earlier slot must not end the argument-list walk. + expect(countCwdBearingComposerCalls( + 'buildCliChildEnv({\n before: { ...forgeTokenEnv, ...claudeSettingsEnv },\n provider,\n cwd,\n guard: true,\n});', + )).toBe(1); + // ...and a nested object must not let the walk run on into the NEXT statement. + expect(countCwdBearingComposerCalls( + 'buildCliChildEnv({ before: { A: 1 } });\nspawn(c, a, { cwd });', + )).toBe(0); + }); + it('discovers the cwd-passing spawn sites (guard is not vacuous)', () => { const found = collectServerSources().filter((rel) => spawnSitesInScope(readServerSource(rel)) > 0); // If this ever collapses toward zero the scan broke (a rename, a new spawn