refactor([issue-3194]): own the AI-CLI child-env composition in one module

.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.** 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.

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,13 @@ 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; })();
+  // 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 <path>` so

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);

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/<cwd-slug>/*.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` | 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). |
@@ -311,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. |

server/lib/cliChildEnv.js

@@ -0,0 +1,128 @@
+/**
+ * Child-environment composition for every AI-CLI spawn.
+ *
+ * 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).
+ *
+ * Two entry points, because the sites split into two shapes:
+ *
+ *   - `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).
+ *
+ * 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:
+ *
+ *   - the agent spawners put `forgeTokenEnv`/`claudeSettingsEnv` BEFORE
+ *     `provider.envVars`, so an explicit provider `GH_TOKEN` still overrides the
+ *     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. `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 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';
+import { buildOpencodeEnvVars } from './opencodeConfig.js';
+import { agentGuardEnv } from './agentGuard/index.js';
+
+/**
+ * The ordered provider env layers, without a base env, PWD pin, or strip.
+ *
+ * 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 {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]
+ * @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/<id>` 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).
+ *   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 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,
+} = {}) {
+  const env = withSpawnCwdEnv(
+    { ...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 in that 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;
+}