perf(cli): stop one-shot runs from booting the CLI twice - #12602

Review at 5019fa2 (author is myself — this COMMENT is the record; a non-author maintainer vote is needed to clear REVIEW_REQUIRED).

No blocking findings. All 7 open threads are the bot's Suggestion-level notes from today, and I verified each against the head rather than adopting them:

• R1-1 / R1-8 (test-worthiness): real — the environmentChangedSinceBoot wiring has no test witness, and the disabled-cache test never exercises the status === ENABLED term. Worth one small test each in a follow-up.
• R1-2 (hasLoadedEnvironmentValues under-reports): real but narrow — only when CLOUD_SHELL=true AND a .env carries the exact keys the cloud-shell helper writes first (e.g. GOOGLE_CLOUD_PROJECT). The relaunch skip then misses a file-injected value. Edge enough to fix in a follow-up rather than gate this PR.
• R1-3 / R1-4 (guard duplicated; design-doc invariant now stale): real — three copies of the compile-cache publish guard exist now, and acp-compile-cache-propagation.md's "only mutation point" line is falsified by the second launcher site. Cheap follow-up: hoist the guard + fix the doc sentence.
• R1-5 (identical-image skip inside the execve gate): noted — win32/os400 don't reach execve at all, so moving the skip platform-wide needs a separate look at what relaunch means there; not a defect in this PR's scope.
• R1-6 (launcher writes resolved NODE_COMPILE_CACHE into the child env): real semantic note — a file-sourced NODE_COMPILE_CACHE can no longer win in the child because the key arrives pre-set. The written value is the correct default, and overriding it via .env is a rare corner; acceptable for this PR, worth a doc line.

CI is green except review-pr (queued, bot infrastructure). Threads stay open for author disposition rather than resolved-by-me.

No blocking findings.
Approval blockers: none.

Tier: Standard — perf optimization, in-place continuation skips the second CLI boot when a relaunch would produce an identical image.

Checked:

• Tracking correctness: dotEnvSourcedKeys and settingsEnvSourcedKeys are populated inside loadEnvironment() only when a key is actually written to process.env (behind isEffectivelyUnset). hasLoadedEnvironmentValues() correctly returns true whenever any value was injected by .env or settings.env. preResolveHomeEnvOverrides intentionally does not contribute to these sets — HOME bootstrap vars are set before any module loads, so the continuing process already holds the right state and a fresh image would read the same values.
• In-place condition (relaunch.ts:99–106): fires only when additionalNodeArgs.length === 0 && additionalScriptArgs.length === 0 && !options?.environmentChangedSinceBoot — the three conditions required for a truly identical image. QWEN_CODE_NO_RELAUNCH is set to true on in-place continuation, matching what the replaced image would have received. childEnv entries are deliberately not applied (correct: provenance info is already in-memory; privateAcpChildEnv is only relevant when replaceProcess is false, i.e. not on this path).
• Fallback-to-spawn test (relaunch.test.ts:270–279): correctly updated to use ['--max-old-space-size=4096'] so the execve path is exercised rather than the new in-place short-circuit.
• Compile-cache forwarding (cli-entry.js): module.enableCompileCache?.() with three guards — not already inherited (!env['NODE_COMPILE_CACHE']), status is ENABLED, directory is present. Tests cover the newly-enabled case, an inherited NODE_COMPILE_CACHE, and NODE_DISABLE_COMPILE_CACHE (mock returns { status: 3 }, no directory set).
• Design doc (EN + zh-CN): in-place continuation paragraph added at matching positions in both files.

Suggestions noted (non-blocking, consistent with prior reviewer's Suggestion classification):

• R1-2: hasLoadedEnvironmentValues() under-reports on Cloud Shell: setUpCloudShellEnvironmentInEnv (environment.ts:682) writes GOOGLE_CLOUD_PROJECT before the tracking loop, so a .env supplying only that key returns false. No import-time read of that key was identified, so no live wrong outcome — but the JSDoc's completeness claim is violated.
• R1-5: In-place skip sits inside the execve/win32 gate; the new design-doc paragraph says "every one-shot run" but Windows / IBM i don't benefit. Doc overclaims.
• R1-6: Launcher writes final NODE_COMPILE_CACHE into the spawned child's boot env, pre-empting the CLI precedence layer. An operator's settings.env compile-cache path is silently overridden on spawned runs, though honoured on the serve fast path. Author acknowledged as a documented gap.

Cross-check: Author and qqqys both classified all seven prior suggestions as non-blocking. I agree with that classification.

Not covered: macOS and Windows end-to-end timing (no host); rung 3 not applicable (no terminal-dependent behavior changed).

Reviewed with AI assistance.

Critical-only review at 5019fa27, base d0cd622a.

Verdict: APPROVE — no Critical. Seven Suggestions are unresolved and I do not treat any of them as gating; two of them point at claims I checked independently because they sit on the wire that makes the skip safe, and both hold.

The skip is gated on the three things that would make a fresh image necessary

if (
  additionalNodeArgs.length === 0 &&
  additionalScriptArgs.length === 0 &&
  !options?.environmentChangedSinceBoot
) {
  process.env['QWEN_CODE_NO_RELAUNCH'] = 'true';
  return;
}

A replacement image is only needed when the new process would differ from this one, and the guard tests exactly the three ways it could: new Node arguments (the caller passes memoryArgs, so a heap-calibrated run still execs), new script arguments, and an environment that changed after this process's modules and Node itself read it. QWEN_CODE_NO_RELAUNCH is not a new signal — llm.tsx already reads it at lines 188 and 522, the latter alongside SANDBOX — so publishing it here puts this path on the same footing as the ones that already skip relaunching.

The predicate under-reports, and that is safe — verified rather than assumed

One unresolved thread says hasLoadedEnvironmentValues() contradicts its own JSDoc, because a .env-supplied value can reach process.env without entering either tracking set. That is factually correct, so I checked whether any of those untracked values is one that requires a fresh image — which is the only way under-reporting could cost something.

The tracked tiers are the two that matter for this question: the .env tier at lines 694-703 and the settings.env tier at 732-740 both apply the no-override rule and both add the key to their set, so anything they inject is reported. The untracked paths are two, and neither needs a relaunch:

• readHomeEnvInto, reached from preResolveHomeEnvOverrides, injects the home bootstrap keys — and its entire purpose, per its own docblock, is to resolve QWEN_HOME and QWEN_RUNTIME_DIR before any settings or storage path is read, precisely so the current process does not snapshot legacy paths. The values are consumed in-process, by design, ahead of the readers. A fresh image would add nothing.
• setUpCloudShellEnvironment writes GOOGLE_CLOUD_PROJECT in place. It is read at runtime by application code, not by Node at startup, so this process already has it.

Neither is a variable Node itself reads during bootstrap — NODE_EXTRA_CA_CERTS is the shape that would matter, and that arrives through the tracked tiers. So the JSDoc over-claims its own scope, which is worth tightening, and the behaviour is correct.

The related thread about the escape hatch having no test is also fair: environmentChangedSinceBoot: hasLoadedEnvironmentValues() at the single production call site, and the predicate it calls, are asserted nowhere, so deleting the line would keep every suite green while silently reintroducing the relaunch for env-changed runs. I confirmed the line is present at this head and that the predicate answers correctly for the tiers that matter, so the uncovered path contains no defect today — which under a Critical-only standard makes it a coverage gap rather than a blocker. It is the one test I would add first.

Skipping the execve does not strand the afterSpawn hook

The caller passes afterSpawn: clearCorruptionEnvVars, which deletes ENV_CORRUPTED_PATH and ENV_WAS_RECOVERED. On the skip path that hook never runs, so I checked whether the markers ending up retained is wrong. It is not: in the relaunch path the child inherits them and the parent clears them after spawning, so the process doing the real work holds them in both cases. Continuing in place makes this process that process. The outcomes match.

The compile-cache addition is correct, with two recorded costs

The launcher's non-fast-path branch now enables the compile cache and writes the resolved directory into the child's boot environment, guarded on !env['NODE_COMPILE_CACHE'], status === ENABLED and a non-empty directory — so it cannot publish a half-initialised value, and it respects NODE_DISABLE_COMPILE_CACHE because enableCompileCache() returns a non-ENABLED status in that case. Two unresolved threads about it are accurate and neither is blocking: the guard is a near-duplicate of the one already in this file's in-process fast path, differing in three of ten lines; and the design doc's recorded invariant that the production fast path is the only mutation point is now false, so acp-compile-cache-propagation.md:66 needs updating. The precedence thread is real too — because loadEnvironment is no-override for both tiers, a NODE_COMPILE_CACHE supplied by a .env file can no longer win over the launcher's resolved directory. That is a user-visible precedence change on a regenerable performance knob, not a correctness or data-loss defect, so it does not gate this merge, but it is the one I would not leave undocumented.

The last thread notes that the identical-image skip sits inside the execve / non-win32 / non-os400 gate while the added design-doc text speaks of every one-shot run, so Windows still double-boots. Worth aligning the doc to the gate; the gate itself is the right place for the decision, since execve is what makes continuing in place equivalent.

CI

15 checks pass and 27 are skipped at this head, with route still pending and nothing failing. Test (ubuntu-latest, Node 22.x), Lint & Static and the cli-entry suite that covers the launcher all completed green. No review carries a changes-requested state.