refactor([issue-3194]): own the AI-CLI child-env composition in one module - #3196
Merged
Conversation
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.
…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.
…ad 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ten AI-CLI spawn sites each rebuilt the same child-environment tuple by hand —
provider.envVarsoverprocess.env, the OpenCode declared-models map, thePWDpin, theCLAUDECODEstrip, and (for agents) the pm2 guard shim onPATH. Three separate fixes had already paid an N-file sweep to keep them in step: the OpenCode models map (#2190/#2243), theCLAUDECODEstrip, and thePWDpin (#3193). Each time the only guard against missing a site was "a reviewer noticed" or a source-grep test — and one sweep did miss a site, which a comment atspawnViaRunnerstill records.server/lib/cliChildEnv.jsnow owns the composition, via two entry points because the sites split into two shapes:buildCliChildEnv(...)— a complete env handed straight tospawn.composeProviderEnv(...)— just the ordered provider layers, for the sites that emit a delta someone else bases and spawns (the CoS runner HTTP payload, a shell-session env overlay). These two were invisible to a base-env-anchored guard, and are exactly where the OpenCode sweep was missed before.Precedence is preserved per site, not flattened. The sites do not all layer their extras the same way, so there are two distinct slots rather than one:
beforesits underprovider.envVars(so an explicit providerGH_TOKENstill beats the repo-owner-pinned one),extrasits over it (so a PTY always gets truecolorTERMregardless of provider config). Collapsing them would have silently flipped both.Behavior changes
Routing
askServiceandvisionClithrough the shared builder brings them the OpenCode declared-models map for the first time. Both inject--model, so an Ollama-backed OpenCode provider used for Ask or vision captioning was rejecting it as "not valid" — the #2190 fix, previously applied only at the runner/agent sites. Every other site is byte-identical to before.The guard
A discovery-based test replaces the per-fix grep list: any file that strips
CLAUDECODE, or spreadsprovider.envVarsinto a child process, must route through the shared builder or be explicitly exempt with a reason. It was tuned against the real tree — verified to flag every pre-refactor hand-rolled site (including both delta sites) while leaving the model-id lookups and capability probes alone, which legitimately need none of this.spawnCwd.test.js's existing PWD-pin scan now countsbuildCliChildEnvas a pin, but only when the call passescwd— it is an optional key there, unlikewithSpawnCwdEnv(x, cwd), so a bare call next to a cwd-bearing spawn would otherwise satisfy the #3193 guard while reintroducing the bug.Not in scope
A
spawnAiCli({command, args, cwd, env})wrapper that would also own cwd and the Windows.cmdshim: the spawn shapes differ too much (child_process vs node-pty vs an HTTP hand-off, stdio variants, kill-tree wiring).lib/aiToolkit/runner.jskeeps its inline copy — it is vendored and must not import out to other PortOS modules, and its spawn is dormant under thesetCliRunneroverride.Folding the three inline
resolveWindowsExecutable+prepareWindowsSafeSpawnpairs intoprepareCliSpawnwas attempted and reverted: it is a real duplicate the helper's own docblock invites, but the #1865 tests inject a fakeresolveWindowsExecutableandprepareCliSpawncalls the real one internally, so the swap costs that seam at three sites. Worth its own change with the test migration.Closes #3194
Test plan
cd server && npm test— 23,243 passed, 210 skipped, 0 failures.server/lib/cliChildEnv.test.js: layering, PWD pin,CLAUDECODEstrip, pm2 guard,composeProviderEnvdelta semantics, and one case per call site pinning the precedence that site depends on.before: { … }neither ends the argument walk early nor lets it run into the next statement.claude(one real finding on the pin marker, fixed) and a localollamapass (no findings).