Commit Graph
6 Commits
Author SHA1 Message Date
Aashish Mahato e5ba5975df Recover working local forge CLIs behind broken PATH launchers
Recover a working local forge CLI when an earlier PATH launcher is broken. Bound executable probes and reuse the verified selection for native operations without replaying authentication or user requests.

Fixes #22975

Co-authored-by: Aashish <145881415+aashish254@users.noreply.github.com>
2026-10-03 20:45:52 -07:00
Neil 1bdb878ac2 perf(preflight): cache WSL CLI probes per distro (#17350)
* perf(preflight): cache WSL CLI probes per distro

A preflight check against a WSL target skipped the cache entirely
(`cacheable = !wslTarget`), so every caller re-spawned up to five
`wsl.exe` probes — git/gh/glab detection plus gh/glab auth, two of them
through login shells — and woke an idle distro each time. Repeated
Landing mounts, pane switches and per-worktree restore each paid the
full set.

Cache per distro so one distro's toolchain never answers for another,
and join concurrent callers onto one probe set instead of letting each
run its own.

The entry expires rather than living for the session like the local
cache does: `isCommandAvailable` collapses every failure into
`installed: false`, so an unreachable distro is indistinguishable from
one with no tooling. Pinning that would report "git not installed"
until relaunch, where the uncached code self-healed. Expiring keeps the
burst collapsed and still lets a transient failure recover.

Propagating unreachable-vs-absent out of the probe layer would allow a
longer-lived entry, but that reaches well past WSL and belongs in its
own change.

* fix(preflight): stop a superseded probe caching its stale result

Review caught two races the first version had.

A forced refresh runs alongside a slower probe already in flight. Both
wrote the cache unconditionally on settle, so the older one landing last
replaced the newer answer — a Re-check could silently return the status
it was asked to replace, for the whole TTL.

The same write also repopulated a cache that `_resetPreflightCache` had
just cleared, which jira/linear call on credential changes: the probe
already out would settle afterwards and restore what was invalidated.

Tag each run and only let it write while it is still the newest for its
key, with an epoch doing the same across a reset. A superseded run still
returns its own answer to its own caller; it just stops becoming the
cached one.

Both tests fail without the guard.
2026-08-30 01:12:22 -07:00
Neil e9e238c883 refactor(wsl): delete the environment-policy layer the reviews kept failing on (#16007)
* refactor(wsl): delete the environment-policy layer the reviews kept failing on

A design council (Opus, Grok, GPT-5.6-Sol) reviewed the merged runner after it
took eleven review rounds to land. All three reached the same conclusion: the
invocation half is sound, the environment/probe half is not, and every round had
been debugging the second one.

The finding that settled it, from Opus: `environmentResolved` had **54
references, all in tests and the runner itself. Not one production reader.** The
safety mechanism the strict default existed for was never wired to anything, so
all 19 degrading sites reported absence with full confidence anyway -- #9725
live at every one, under comments claiming it was handled. Two of those comments
say so out loud; I wrote them.

Root cause, in one line: every knob existed only because a failed probe was
fatal. So it no longer is.

- `allowDegradedEnvironment` and `WslGuestEnvironmentUnavailableError` are gone.
  A missing login PATH is a fact in the result, not an exception. That deletes
  23 opt-outs, six catch-and-remap blocks, the transient/rejected cooldown
  split, `probedWithBudget`, and the 1.5x re-probe heuristic -- none of which
  had a reason to exist once the case stopped throwing.
- `lane` + `allowDegradedEnvironment` collapse into `loginPath: 'none' |
  'preferred'`. 19 of 23 sites passed the opt-out, and two said in comments that
  they did not want the login PATH at all: the flag had become the `'none'` the
  union was missing.
- The `interactive` lane is deleted. It had zero production callers and kept ~30
  lines of fence plumbing alive for tests only.

Net -98 production lines; the runner itself sheds 86 for 38.

Also carries three fixes from the W3 orphan-PR sweep I had not done:
- `WSL_UTF8=1` in the runner. My relay migration deleted the only place setting
  it, so wsl.exe's own error text arrived UTF-16LE and read as NUL-riddled.
  A regression I introduced. Credit: #9010 (Chang-Jin-Lee).
- `GITLAB_HOST` is now named in WSLENV, so a ported self-hosted host actually
  crosses into a distro-routed glab (#12557). Credit: #12558 (makoto-developer).
- The WSL skill-setup command pipes into `sh` instead of `eval "$(...)"`, whose
  nested quoting produced `word unexpected (expecting "in")` (#14292). Credit:
  #14785 (innocarpe).

* fix(wsl): restore the login PATH for the Codex availability lookup

loginPath:'none' on a PATH lookup reports an nvm-installed codex as absent,
which is #9725. A miss without a resolved environment is now 'could not
check', not 'not installed'.

Also hardens the guards that should have caught it:
- bashism ratchet is per-call, not per-file, and fails closed on lexer desync
- blankStringContents handles regex literals (an apostrophe in /'/g desynced
  the lexer, so the scan silently found zero calls)
- windowsHide allowlist 85 -> 80, stale once the lexer parsed those files

Credit: Grok (P0), GPT-Sol (ratchet gaps).

* test(wsl): close the two ratchet gaps that let planted spawns pass

- variable-indirected wsl.exe (`const b = 'wsl.exe'; spawnProcess(b)`) is now
  tracked, so the 5 files recorded only in a comment become real allowlist
  entries. Three actually spawn that way; the other two never spawned wsl.exe
  at all, so the prose record was wrong by three in the hiding direction.
- promisify(renamedAlias) is now resolved, so `const run = promisify(execFile)`
  behind an `execFile as x` import can no longer skip windowsHide.

Each verified by planting the violation, watching it fail, restoring, watching
it pass. Credit: GPT-Sol.

* fix(source-scan): stop the regex-literal reader from eating block comments

At index 0 there is no preceding token, so a file opening with a banner
comment had its `/*` read as a pattern and swallowed to the next slash --
110k characters of preload/index.ts, in the direction that hides offenders.

Measured across the tree, old lexer vs new: worst-case over-blanking drops
from -110564 to -1116 characters, and files that desync drop from 51 to 22.
The remaining extra blanking is regex interiors, which is the intent.

Regression tests for both lexer bugs, each verified to fail with its fix
reverted. The first draft of the comment test did not bind -- it asserted on
text after the swallowed span.

* fix(wsl): restore the unverifiable signal on the two remaining probe sites

Round 2. Three call sites used to throw when the login-PATH probe failed;
the redesign rewired one (Codex) and left two reporting confident absence.

- skill-wsl-provider-detection: the script ends in `|| true`, so a lookup
  without the login PATH exits 0 with empty stdout -- identical to 'nothing
  installed'. Callers skip the ~/.codex and ~/.claude skill roots on an empty
  list, losing an nvm-installed provider's skills.
- wsl-cli-installer: the dead catch is replaced by an explicit check. Its
  `case ":$PATH:"` probe otherwise answers from the distro default PATH and
  Settings states as fact that the CLI is not on PATH. Timeout is checked
  first, since a timed-out run also leaves the environment unresolved.

Also narrows the regex-literal prev-token set. '!', '+', '-', '>' and '}' are
value terminators as often as operators, so postfix `n-- / 2` and JSX
`<A size={14} /> : <B` were read as patterns and their spans blanked -- 13
live JSX spans, and one swallowed execFile call that left no desync behind.
False negatives only risk a desync, and desync fails closed.

Plus: WSL_UTF8 on the probe spawn (#9010 reached the runner, not the probe),
and the allowlist header I shuffled by sorting comments along with entries.

Credit: Grok (both P1s), Opus (lexer false positives).

* docs(wsl): drop the lane comments the redesign made false

The interactive lane is gone, so 'both lanes' and the fenced-stdout note
described code that no longer exists. Also states plainly that
environmentResolved is always true under loginPath:'none' -- the field cannot
rescue a PATH lookup that was mislabelled, which is how #9725 came back.

Credit: Grok.

* fix(wsl): stop piping user scripts into the shell's stdin

The W3 migration moved hooks from `wsl.exe --exec bash -c <script>` to a
script piped into `bash -s`. Anything the script runs that reads stdin then
drains the rest of the script, bash hits EOF and exits 0, and the caller logs
success -- an orca.yaml hook of `ssh -T git@github.com || true` followed by
`pnpm install` silently never installs.

Scripts now travel in argv by default, which is what the pre-migration code
did and what --exec makes safe. `scriptDelivery: 'stdin'` stays for the one
caller that needs it: the hook-relay installer embeds a base64 JS bundle far
past any command-line limit, and reads no stdin.

A runner test already described this exact EOF hazard -- for the login shell,
not for the guest command it was itself creating.

Credit: code review.

* fix(skills): make the unverifiable check unconditional, and stop double-probing

Round 3.

- provider detection threw only on an EMPTY result, so a degraded partial hit
  slipped through: `claude` visible on the default PATH via Windows interop
  plus an nvm-only `codex` returns a plausible ['claude'], and the caller then
  skips the ~/.codex skill roots for a provider that is installed. The
  installer already got this right with an unconditional throw.
- three sites asked for 'preferred' without needing it. The GROK_HOME probe
  runs its own `"$login_shell" -lc`, so the runner's probe was a second login
  shell eating up to half an 8s budget; the two skill scans are
  find/base64/head/printf/stat over $HOME.
- the indirection binder missed `private readonly x = 'wsl.exe'` (the
  modifier was captured as the name), backtick literals, and
  `spawnProcess(this.x)`. Commit 2bbbd99 claimed that gap closed; it now is,
  verified against all three shapes.

Credit: Grok.

* fix(child-process): keep the tail of output whose failure lands last

Two console-flash bugs the ratchet was carrying on its allowlist rather than
catching: daemon-process-inspection execs powershell.exe and the gemini
extractor execs `where gemini`, both console-subsystem, both without
windowsHide (#10488). Allowlist 80 -> 78.

And a migration regression: the hook-relay install used to keep a rolling
tail of stderr (`slice(-MAX)`), while runProcess's maxOutputBytes keeps the
head. A guest install that fails after pages of apt warnings therefore
reported the warnings instead of `mv: Read-only file system`. runProcess
takes retainOutput: 'tail' for output whose meaning is at the end.

Credit: code review.

* test(wsl): close the last two indirection shapes in the binder

`this.binary = 'wsl.exe'` has no declarator keyword, and a helper that just
returns the literal is a spawn one hop away that no regex can follow. The
return case fails closed only when the file also spawns something --
local-windows-terminal-runtime.ts returns the name as terminal metadata and
never spawns, so a blanket rule flagged it wrongly.

Verified against both shapes: planted, failed, restored, passed.

Credit: Opus.

* fix(preflight): stop reporting installed WSL CLIs as absent (#9725)

The last two probe sites that turned an unresolvable login PATH into a
confident negative. The native branch of detectInstalledAgents already
consults install dirs for exactly this reason ('PATH may still be unhydrated
on a cold GUI launch'); the WSL branch had no equivalent, so a cold distro
made an nvm-installed claude/codex read as not installed and told the user to
install a CLI their own terminal runs.

Ports that fallback to the guest: agent detection checks the version-manager
bin dirs for commands the PATH lookup missed, and the preflight command runner
APPENDS them to PATH -- append, never prepend, so a resolved login PATH stays
authoritative and a stale nvm version cannot shadow the real binary.

Tested by executing the generated scripts through /bin/sh against planted
binaries, since the behaviour is shell globbing and [ -x ]. Both the
nvm-discovery and the no-shadowing tests were verified to fail when reverted.

* fix(codex-accounts): hide the console on the legacy active-home migration

execFileSync('wsl.exe') with no windowsHide flashes a conhost and steals
foreground on a GUI-launched Orca (#10488). Sibling WSL spawns got this in
earlier commits; this one only had its quoting rewritten. Allowlist 75 -> 74.

Credit: code review.

* fix(windows): close the shell:true hole that made windowsHide a no-op

I un-allowlisted the gemini extractor after adding `windowsHide: true` to an
`exec()` call. `exec` implies `shell: true`, which this repo's own chokepoint
documents as silently making windowsHide a no-op (#14543) -- so the site still
flashed a conhost while reading as guarded. Now execFile('where.exe', …),
matching the relay sibling that already did it right.

The ratchet could not see that, which is why it passed. It now treats a call
that resolves to exec/execSync, or any `shell: true`, as unguarded regardless
of windowsHide -- including through renamed imports and a renamed promisify.

Also: a script over 8000 chars now falls back to stdin. Windows caps a command
line at 32767 and a user's orca.yaml hook is the one unbounded script Orca
runs (`run-both` concatenates two; a vendored installer is ~15KB), so argv
would fail to spawn outright. Degrading beats failing.

And the binder now sees `let p: string` ... `p = 'wsl.exe'`.

Each verified by planting. Credit: Grok.

* test(wsl): an opaque payload must declare its interpreter

My per-call bashism guard REPLACED the file-wide one, and that was a strict
regression: the real payloads are built in a separate function and passed as a
bare `script,`, so the bashism is never inside the call literal and the
per-call arm cannot fire. Deleting `shell: 'bash'` from skill-discovery-wsl
-- `done < <(find ...)` and `read -r -d ''`, the #14292 signature -- passed on
this branch and failed on main.

Reading through the identifier is guesswork. Requiring the call to name its
shell when the payload is not a literal is not, so seven POSIX call sites now
say `shell: 'sh'` -- no behaviour change, sh was already the default.

Two earlier attempts at this were wrong and are worth recording: a whole-file
BASHISM test blamed codex-accounts/service.ts, which correctly pins bash on its
four inline payloads and correctly leaves printf/mkdir unpinned; and excluding
call text still caught a bash payload belonging to a non-runner execFileSync.

Also: runProcessSync now refuses retainOutput:'tail' instead of silently
keeping the head, and the union docblock no longer describes stdin delivery.

Verified against both of the plants that exposed this. Credit: Opus.

* test(wsl): judge an opaque payload by the file, not by whether shell is set

Round 5. My previous rule -- opaque payload must have `shell:` -- was the
third guard fix in a row that came out weaker than what it replaced:
`shell: 'sh'` on a bash payload satisfied it, which is #14292 with extra
steps. Flipping skill-discovery-wsl's pin from bash to sh shipped green.

Now: strip the text of every call that already names bash, and if a bashism
survives anywhere in the file while a script-carrying call is not bash-pinned,
flag it. Stripping the bash-pinned calls is what keeps codex-accounts clean.

Also closes four ways to hide a call from the collector, each verified by
planting:
- `script: \`${bashism}\`` -- a template literal read as a visible literal
- `runWslProcess({ ...spec })` -- a spread hides script AND shell
- `Object.assign({ a }, { script })` -- the collector took the first `{`, so it
  now takes the whole argument list
- `import { runWslProcess as runWsl }` -- a renamed callee collected nothing,
  and zero calls read as zero violations

Not fixed, recorded instead: a computed `shell:` in the console guard. Matching
any non-false value also flags `shell: spawnConfig.shell`, a pass-through that
is false in every branch, and a false positive there costs an allowlist entry
that disables the guard for a whole correct file.

Credit: Grok.

* fix(preflight): make the guest fallback match the native one it claims to mirror

Three defects in the #9725 fix from earlier today, all found by executing the
generated scripts under real dash rather than reading them.

- $HOME containing a space word-split the unquoted dir list into a relative
  path, so every CLI read as absent -- the exact symptom the fix exists to
  remove. Each entry is quoted now; the nvm entry quotes only its prefix so the
  glob still expands.
- A directory passes `[ -x ]`, so ~/.local/bin/gemini/ was reported as an
  installed CLI that then fails to launch with EISDIR. The PATH half of the
  same script already guarded this, and so does the native twin.
- The header called this the "guest-side twin" of the native fallback while
  omitting four of its directories: volta, asdf, fnm and mise. A WSL user on
  any of those still had #9725 while the same user on native did not -- and
  asdf and mise are named in the motivating comment. The claim is now true.

Credit: Opus.

* test(wsl): mask bash-pinned calls by position, not by String.replace

`rest.replace(text, '')` with a string pattern removes only the FIRST match,
so two identically-written pinned calls left one behind and its bashism then
counted against an unrelated unpinned call in the same file. A body that also
occurred earlier as a substring would blank the wrong region entirely.

The collector now returns ranges and the mask is applied by index. Verified
both directions: two identical pinned bodies plus one unpinned call flags, and
the same file with all three pinned stays clean.

* test(wsl): fail closed on call shapes a regex cannot attribute

Round 6. Rather than widen the pattern again, treat the shapes it cannot
reason about as unreadable.

A regex cannot tell which object a key belongs to, so every round produced
another way to put the pin in one place and the payload in another:
`cond ? {pinned} : {unpinned}`, `{...} as WslSpec`, `Object.assign({a},{b})`.
A call whose SPEC is chosen by a ternary or spread -- one appearing before the
first `{` -- or which carries an `as` assertion is now flagged whenever the
file has a bashism, with no `shell: 'bash'` escape, because the substring test
that would grant the escape is exactly what cannot be trusted on these shapes.

A ternary INSIDE the object is not exotic: claude-accounts/service.ts:977 uses
one to choose a script line in a call that is already pinned, and treating that
as opaque would demand a second pin it already has. Nor is a nested call --
`script: `x ${shellQuote(p)}`` is how every payload here is built, and flagging
it would demand bash on POSIX payloads that must not have it.

Also follows `const run = runWslProcess`, generics and optional chaining, and
counts collected calls against mentions so a shape that slips the pattern reads
as unreadable rather than clean.

I tried the TypeScript parser first, which would remove the class outright.
TypeScript 7 is the native port and exposes no JS compiler API; oxc-parser
works but is transitive, and declaring it surfaced an unmet peer warning.
Recorded here so the next person does not repeat the detour.

Credit: Grok.

* fix(wsl): fish is a PATH lookup, and my lint check could not fail

Two things Opus caught that I had verified wrongly.

`wsl-fish-history-cleanup` passes `program: 'fish'` -- a bare name, so a PATH
lookup by definition, the exact class the earlier rounds hunted. I mapped it to
'none' and then defended that in an audit, because I read
`allowDegradedEnvironment: true` as "does not need the login PATH". It does not
mean that: it means "do not fail when the probe fails". The old call still USED
the login PATH whenever it got one, which is 'preferred'. Under 'none' a fish
from linuxbrew or nix is invisible and the cleanup throws. The truncated
comment left behind when the flag was deleted is finished too.

And `pnpm lint` has been failing on this branch while I reported it clean: I
grepped for `error eslint|error oxlint`, but oxlint prints the rule category
(`error typescript(array-type)`, `error unicorn(prefer-ternary)`). The grep
could not match, so it never failed. Checking the exit code instead surfaced a
third violation hidden behind the first two.

Credit: Opus.

* chore(wsl): clear the round-7 P2s

- Formatting: the branch owned 22 of the tree's 26 oxfmt failures because I
  never ran the formatter. Branch files now own none.
- resolveScriptDelivery was computed twice, in two places that must agree
  about argv shape and stdin payload. Resolved once and threaded through.
- The allowlist header said the list only shrinks while the branch added three
  entries. It grew because the scanner learned to follow a variable-bound
  'wsl.exe'; those three were previously recorded in prose, so the count was
  wrong by three in the direction that hides offenders. The header now says so.
- Two test comments still explained behaviour via the deleted
  allowDegradedEnvironment flag; a stray triple blank line; two adjacent JSDoc
  blocks where only the second attached.

Not taken: platform-guarding addWslEnvKeys. WSLENV is inert off Windows, and
the guard broke a test that asserts the key directly -- more surface than the
tidy is worth, so the reason is recorded at the call site instead.

Credit: Opus.

* test(preflight): plant a fabricated CLI name, not a real one

CI caught what my local run could not: the runner has a real /usr/bin/gh, so
`command -v gh` resolved to it and the planted nvm stub was never reached. The
fallback APPENDS, so that is the code behaving correctly -- the test was
asserting a property of my machine.

Both real-shell suites now plant `orca-fake-cli`, which exists nowhere.
Re-verified the same way as before: with the PATH fallback disabled the test
fails, with it restored it passes.

I declared this branch merge-ready without looking at CI. Local green is not
the gate.

* refactor(wsl): delete two knobs and a duplicated fallback

Elegance pass. The branch had grown from a deletion into a net addition, and
most of the growth was optional axes with one caller each.

- `retainOutput` is gone. One production caller wanted the tail of a 64KiB
  buffer; head-truncation only hurt because of that cap. The caller drops the
  cap, keeps the default, and slices the tail itself -- which is what the live
  relay next door already does. Two mechanisms for one job became one.
- `scriptDelivery` is gone. The size rule was already the whole design:
  argv unless the script is too long for a Windows command line. The option
  existed so a small Orca script could opt into stdin, and no such caller ever
  appeared. Both behaviours stay pinned: a huge script still goes to stdin, an
  ordinary one still leaves the hook's stdin free.
- Agent detection no longer walks the fallback dirs itself. It prepends the
  same PATH prelude the preflight command runner uses and lets the ordinary
  lookup do the work. Its bespoke walk had duplicated the lookup script's
  `! -d` guard -- and had missed it once, which is how a directory read as an
  installed CLI.

All 17 detection tests still pass unchanged, including the $HOME-with-a-space,
directory-is-not-a-CLI, and volta/asdf/fnm/mise cases, so the collapse is
behaviour-preserving rather than assumed to be.

Credit: Grok.

* fix(wsl): never name a path-shaped variable in WSLENV

`buildHostEnv` forwarded every caller-supplied key into WSLENV. wsl.exe
translates path-shaped variables between Windows and Linux form, so a caller
passing PATH would have replaced the guest's own PATH with a translated
Windows one -- silently, and fatally for every lookup after it.

No caller passes PATH today. The point of a chokepoint is that it does not
depend on that staying true.
2026-08-22 20:35:59 -07:00
Neil 5651662494 fix(wsl): migrate 21 call sites onto the WSL runner (#15923)
* fix(wsl): migrate 21 call sites onto the runner, after five review rounds

Rebased onto main now that the runner (#15903) has landed.

21 sites across 15 files move off ad-hoc `execFile('wsl.exe', ...)`. Allowlist
23 -> 16 on the WSL guard; 163 -> 152 on the W1 child_process guard, which moved
as a consequence.

Five review rounds, each finding real defects -- several introduced by the
previous round's fixes:

1. Hooks ran user orca.yaml scripts under dash; probe failure fell back to the
   login shell, reintroducing the ~/.profile stall the runner exists to remove.
2. An unparseable probe was cached permanently, disabling every WSL feature on
   the distro; hooks regressed from "runs degraded" to "fails".
3. Exit 127 had no expiry; a starved 5s probe hard-failed the 10s scan behind
   it; a joiner burned its budget on someone else's probe.
4. The comment stripper blanked live code, so the windowsHide guard walked past
   a real unguarded spawn and reported the file clean; an ownership-probe
   timeout silently deselected the user's Claude account.
5. Verification of the guards themselves.

The recurring finding -- a call answering "is this installed?" on a degraded
PATH -- was eventually fixed structurally rather than per-caller: the runner
refuses an unresolved guest PATH unless the caller opts in. Per-site vigilance
was demonstrably not holding; 3 of 8 sites had already forgotten the analogous
exit-code check.

Remaining 16 files need a runner mode that does not exist: a long-lived
streaming child (OAuth logins, hook relay), a synchronous caller, or a
host-level flag like --status that the guest-command API cannot express.

* fix(wsl): close round 5's P1s -- degrade where PATH was never needed

Round 5 measured the guards by re-executing their algorithms standalone rather
than reading them, and found four things.

P1 -- four skill/plugin paths gained a hard dependency on the login-shell probe
that they never had. They ran under a plain non-login `sh -c` on main, so a
probe failure now breaks WSL skill discovery and install on exactly the distro
the runner was built for: one with a slow `~/.profile`. Worse, the throw escapes
before each site's own error mapping, so the UI gets a raw internal string. They
degrade now, per the rule this branch already wrote down in
`wsl-fish-history-cleanup.ts`.

P1 -- Codex and Claude were asymmetric. Claude's five credential sites degrade;
Codex's were strict, so adding a WSL Codex account failed where adding a Claude
one succeeded. Three of the four are byte-equivalent to Claude sites, and their
scripts read `$HOME`/`$WSL_DISTRO_NAME`, which wsl.exe supplies without a login
shell. `assertWslCodexCliAvailable` stays strict on purpose -- that one really
does answer "is this installed?" (#9725).

P1 -- the ownership-probe timeout fix did not survive the rebase onto main. A
timeout still returned "not owned", which the caller *persists*, clearing the
user's account selection.

P1 -- `blankStringContents` desynced on a nested template literal
(`` `${`x`}` ``), leaving 116 lines of a child_process importer outside the
ratchet, with 27 importers structurally at risk. Now tracks template depth.
Regenerating against the fixed blanker: 70 -> 68 offenders.

Also: the windowsHide vacuity check could not fail while the allowlist alone
exceeded its bound -- the exact defect the sibling guard documents avoiding. It
now names a file that definitely offends.

* fix(wsl): close round 6 -- my blanker fix had traded a false positive for a miss

Round 6 re-derived the guard's answer from a TypeScript AST instead of trusting
the regex, and caught two things.

P1 -- the nested-template fix I shipped in round 5 introduced a worse bug than
the one it closed. Switching to "code mode" inside `${...}` without also
resetting the quote at a newline meant an apostrophe in a regex literal --
`` `'${value.replace(/'/g, "'\\''")}'` `` , which is exactly the shellQuote
shape all over this codebase -- inverted the lexer for the rest of the file.
`claude-accounts/service.ts` went blind from line 96, hiding a REAL unguarded
`spawn` at :1097: the WSL Claude managed-login path, which opens a console and
steals foreground on Windows. Round 5 traded one false positive for one false
negative and I did not notice, because the offender count went down.

The blanker now resets non-backtick quotes at a newline (the rule stripComments
already had) and tracks brace depth per interpolation. The spawn is fixed rather
than allowlisted, and the count is 69 -- the number the AST predicted.

P1 -- the ownership-timeout guard was dead code: it threw into its own `catch`
three lines below, which returned null, which the caller persists as "not owned"
and clears the user's account selection. Now a typed sentinel the catch rethrows.

P2 -- `WslGuestEnvironmentUnavailableError` reached the UI verbatim from the CLI
installer and the Codex availability check. Both mapped.

Method note: I had been regenerating the allowlist with a Python transcription
of the scanner, and the two drifted -- the same two-implementations problem this
workstream keeps finding. The allowlist is now generated by running the shipped
test with an empty list and taking what it reports.

* fix(guards): stop patching the lexer -- make the scanner fail closed instead

Round 7 proved my round-6 fix also did not work, by planting a plainly-named
unguarded `spawn` in `claude-accounts/service.ts` and watching the guard pass
3/3. That is three consecutive attempts at an exact lexer, each shipping a
desync that hid real calls, and each time the offender count went DOWN, which I
read as progress. Round 6's diagnosis was wrong too: the culprit is the
`templates` brace-depth stack, which nothing resets, not quote state.

So stop trying to be exact. `blankStringContentsDesynced` reports when the lexer
lost its bearings, and the guard treats that as an offender. Over-reporting is a
nuisance; under-reporting is a false clean, and a false clean is what let a real
console-flash spawn out of the ratchet twice. The allowlist goes 69 -> 82: the
13 extra are files whose scan cannot be trusted, now named rather than assumed
fine.

The planted violation is now caught.

Also from round 7:
- `SPAWN_CALL` missed promisified and renamed bindings, so `exec('where gemini')`
  (a real Windows cmd.exe spawn) and a detached `shell: true` in
  `cli/runtime/launch.ts` were invisible. Added execAsync/execFileAsync/
  execFileCb/spawnDetached.
- `BASHISM` matched `set -o pipefail` but not `set -euo pipefail`, which is the
  only spelling this tree uses -- so the check could not have caught the #14292
  signature it exists for. Fixed, and it immediately flagged a file; that one
  turned out to be a comment, so the bashism scan now strips comments too.
- The CLI installer error mapping my round-6 commit claimed was "both mapped"
  was never applied -- only the Codex side had been. Now actually mapped.

* fix(guards): close the four holes round 8 found by planting violations

Round 8 stopped reasoning about the guard and planted spawns into it. Four
holes, none of which reading had found:

- `windowsHide: false` **passed**. The check was `args.includes('windowsHide')`,
  a substring test. Now matches `windowsHide: true`.
- A ternary first argument was silently skipped: the method-declaration filter
  `/^\(\s*\w+\s*[:?]/` also matches `exec(useAlt ? 'a' : 'b', …)`. Now requires
  a type after the colon.
- Renamed bindings were not covered, despite the comment I wrote saying they
  were -- I had hardcoded three names. Aliases are now resolved from the import.

Each is verified closed by planting it and watching the guard fail.

`fork` is deliberately still unscanned. Round 8 is right that Node forwards the
option, but `ForkOptions` does not declare it, so the two live sites cannot be
fixed without a cast. Recorded in the verification doc rather than left as a
silent gap, along with two others worth knowing: the allowlist is file-granular,
so its ~18 false-positive entries carry a standing pre-approval for real
regressions in those files and cannot be retired by fixing code; and
`stripComments` has no desync report, so the fail-closed check is only half
applied.

The doc now also says how to verify a guard change: plant a violation. Every
guard fix here that was verified by reading was wrong.

* fix(wsl): stop preflight reporting installed CLIs as absent on a slow distro

Round 9's merge blocker, and the sharpest finding of the whole workstream: the
branch built to close #9725 had reopened it from the other side.

`preflight-wsl-command.ts` was one of five sites without
`allowDegradedEnvironment`, so a guest-PATH probe failure threw. Every consumer
collapses a throw into a verdict: `isCommandAvailable` and `isCommandOnPath`
catch to `false` ("not installed"), `isGhAuthenticated` and `isGlabAuthenticated`
read an empty payload as "not authenticated". So a slow distro made WSL git, gh
and glab read as missing.

Two things made it likely rather than theoretical. The probe took two thirds of
a 5s budget, leaving the command ~1667ms where main gave it the full 5s inside
its own login shell -- a cold WSL VM start routinely lands in that band. And a
probe timeout is cached for 30s with a re-probe threshold of 1.5x the failed
budget, which a 5s caller can never clear, so every preflight command
short-circuited without spawning wsl.exe at all -- and Re-check does not
invalidate the cache.

Fixes: preflight degrades instead of refusing, and the probe is capped at half
the caller's budget and at 4s, so no caller ends up with less time than it had
before the runner existed.

Also fixes a real console flash found on the way: `preflight-command-exec.ts`
spawns git/gh/node through `promisify(execFile)` with no `windowsHide`.

Round 9 also confirmed the credential paths are now *safer* than main: all 11
account sites degrade, every destructive guest operation is still marker-gated,
and main's `getOwnedManagedAuthPath` could disown an account on a 5s timeout --
which this branch turns into a failed launch instead of a destroyed selection.

* fix(wsl): make "Try again" able to succeed, and test the round-9 fix

Round 10 returned MERGE with one residual worth closing first.

A transient probe failure left the null-resolving promise in `inFlight`, so the
only way back was `retryAfter` -- and the 4s probe cap made the 1.5x budget
escape unreachable, because no caller can pass more than 4s. For the full 30s
window the four non-degrading sites returned their error *without spawning
wsl.exe at all*, and each of those errors says "Try again". The advice was
guaranteed to fail.

The entry is now dropped on a transient outcome and an explicit cooldown gate
replaces it, so the window alone decides. The window drops 30s -> 5s: long
enough to stop a stampede, short enough that the user's next click reaches a
distro that has since warmed up.

Round 10 also noted the round-9 fix shipped untested, which was fair. Added: the
probe-budget floor for 5s/8s/10s callers, and preflight's degrade opt-in plus
its stdout/stderr-carrying rejection, which isGhAuthenticated reads off the
caught error as an auth-success fallback.

* test(wsl): make the probe-budget guard actually guard

Round 11 caught that the regression test I added for the probe cap did not
bind: it seeded the guest environment, so the probe resolved in ~0ms and the
assertion read the command leg's timeout instead. Reverting the cap to the old
2/3 split left all three cases green.

Dropping the seed and asserting on the probe leg fixes it -- verified by
reverting the cap and watching all three fail.

A regression guard that cannot fail is the shape that has cost the most in this
workstream: the windowsHide guard silently passed a real unguarded spawn twice
for the same reason.
2026-08-22 05:45:21 -07:00
OrcaWinandOrcaWin 6e8da1df8d fix(wsl): pass guest argv verbatim through --exec (#15039)
* fix(wsl): pass guest argv verbatim through --exec

`wsl.exe <...> -- <argv>` expands `$name` in every argument against the
guest environment before the guest ever runs. It does this even when no
shell is involved, so `-- /usr/bin/printf %s '$HOME'` prints /home/you.

Every WSL invocation went through that preprocessor, so scripts arrived
already rewritten: `awk '{print $2}'` lost its field reference, and a
POSIX script asking for the literal `$HOME` got the expanded path.

`escapeWslShCommandForWindows` tried to compensate by escaping `$`, but
it skipped any `$` preceded by a backslash, so a script containing `\$`
was still corrupted -- and half the call sites never applied it at all.

Route every invocation through `--exec`, which passes argv through
untouched, and delete the escaper. The direct-git path already used
`--exec`, so this is not a new compatibility dependency.

A guard test fails if the `--` form reappears anywhere in the tree.

Net -50 lines of production code.

* test(wsl): drop remaining escaped-dollar assertions

* fix(wsl): cover the --exec migration's blind spots

An audit of every wsl.exe invocation found sites the first pass missed,
including two it actively broke:

- config/scripts/wsl-git-shell-benchmark.mjs imported
  escapeWslShCommandForWindows, which no longer exists, so the script
  threw on startup. Its wslShellArgs helper also still used `--`; the
  file already had an --exec helper, so route both call sites there.
- classifySubprocessCommand unwrapped `wsl.exe <...> -- <binary>` by
  breaking on `--` alone. With every Orca spawn now on --exec it never
  found the guest binary and bucketed all WSL subprocesses as plain
  "wsl", losing the git/gh/glab breakdown. Break on either separator,
  since foreign wsl.exe processes still use `--`.

CliSkillRuntimeSetup builds its setup command as a template literal
rather than an argv array, so no array-shaped search could see it. Its
decoder accepts both separators so commands persisted before this
change still decode.

The guard now scans config/ and tests/ as well as src/, and checks the
command-string spelling alongside the argv one — the two shapes that
have each shipped a regression. It skips comment lines so prose about
the old form stays allowed, and asserts it scanned a plausible file
count so a bad root cannot make it vacuous.

* fix(wsl): restore the guard's multi-line sensitivity

The guard matched line by line, so `'--',\s*'bash'` could not span a
newline -- and every argv array in this repo is formatted one element
per line, which is exactly the shape it exists to catch. Measured
against the pre-migration tree it caught 17 files before and 9 fewer
after. It now strips comment lines and matches the rejoined text, with
a case that pins the multi-line shape so this cannot silently return.

The program list is wider than shells now, which surfaced a false
positive: tmux takes a `--` separator followed by a program too
(`split-window ... -- cat`). Matching is scoped to files that mention
WSL rather than narrowing the list back.

Also:
- Replaced the `sed` regression case, which was vacuous. A backreference
  contains no `$`, so it returned `bac` under both separators and would
  have passed without the fix. The block claimed every case proved the
  bug. Swapped in a positional argument and a shell local, both measured
  to differ -- the positional is the shape `wslUncDirectoryExists` uses,
  where `--` blanked `$1` so every existing directory probed as missing.
- windows-shell-args.test.ts derived its expected argv from
  buildWslExecArgs, the helper under test, so six assertions would still
  pass if it regressed to `--`. Spelled the expectation out.
- Dropped two comments citing the removed `--` behavior as rationale.

---------

Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
2026-08-18 02:12:21 -07:00
Neil 9367169888 refactor(tests): split every oversized test file off the max-lines suppression list (#14728)
* refactor(tests): split oversized test files off the max-lines suppression list

Every `*.test.ts`/`*.spec.ts` that carried an `eslint/oxlint-disable max-lines`
directive is now split into focused, behavior-scoped suites that fit the 800-line
test budget, with shared setup extracted into co-located `*-test-harness.ts` /
`*-test-fixtures.ts` modules (300-line budget). 83 files became ~930; the largest
output is 797 effective lines. `orca-runtime.test.ts` is intentionally untouched.

Test bodies were moved by scripted line-range slicing rather than retyped, so
assertions are byte-identical. The only permitted body edits were mechanical
rebinding where a shared value moved into a harness (e.g. `tmpHome` ->
`homes.tmpHome`).

Registries that enumerate test files were updated in lockstep:
- config/max-lines-baseline.txt: pruned 341 -> 258 entries (all 83 removed).
- config/reliability-gates.jsonc: 33 gates repointed at the split files, with
  assertionRefs split per file where a gate's coverage now spans several.
- .github/workflows/pr.yml: the real-zsh lane now lists the 4 split files that
  actually exercise zsh, so they keep running in the dedicated shell lane.

Also renamed agent-hooks `server-test-fixtures.ts` to `server.test-fixtures.ts`
so the global-fetch call-site audit keeps skipping it, and added `.js` extensions
to the CLI suites' dynamic harness imports (node16 resolution) to unbreak
`build:cli`.

Verification: full suite 52,449 passing vs 52,448 at baseline with zero
assertions lost; `pnpm lint`, `pnpm typecheck`, and `pnpm build:cli` all exit 0;
the terminal-pane e2e spec runs 31/31 headless.

* refactor(tests): split hook-idle arbitration suite that oxfmt pushed over budget

The pre-commit oxfmt pass reflowed pty-connection-hook-idle-arbitration.test.ts
to 811 effective lines, 11 over the test budget. Split the hook-completion side
effect and replacement-agent veto cases into their own suite; both files now sit
well under the cap and the 15 tests are unchanged.

* test: port upstream test changes into the split files after rebase

Rebasing onto main surfaced 27 tests that main had added to files this branch
deleted, plus edits to tests that had already moved. Taking the deletion side of
those modify/delete conflicts would have dropped that coverage silently, so each
upstream change is ported into the split file that now owns the behavior — for
example main's six orchestration mailbox tests land across orchestration-runs,
-send, and -check.

Also repoints `orchestration.notification-mailbox-consistency`, a gate main added
after this branch's gate remap, at those same three split files, and re-prunes
the max-lines baseline against main's (257 entries).

Verified: all 27 upstream test titles present; full suite 52,761 passing with the
only diff vs baseline being 12 tests main itself removed and 3 that moved from
skipped to passing; lint and typecheck exit 0.

* fix(test): flush pending continuations before tearing down terminal test globals

CI shard 5/16 failed on both Node 24 and 26 with `ReferenceError: window is not
defined` from pty-connection.ts, surfacing through
pty-connection-daemon-snapshot-replay.test.ts.

The reattach/settle chains `await` a real promise and then touch `window.api`.
Under fake timers those continuations cannot run, so they only become schedulable
once restoreTerminalTestGlobals() switches back to real timers — which previously
happened immediately before `delete globalThis.window`, so a late continuation
threw and failed the whole file. Flush async ticks in that window instead.

This is latent in the source rather than new: the pre-split 25k-line file kept
running other tests after these, which gave the chains time to settle before
teardown. Splitting the file moved teardown directly behind them.

* fix(test): keep an inert window after terminal test teardown instead of deleting it

The async-tick flush was not enough: the reattach/settle chain can resolve after
teardown regardless of how long we drain, so CI shard 5/16 still failed with
`ReferenceError: window is not defined` from pty-connection.ts.

A real renderer never loses `window`, so deleting it was the artificial part.
Swap in an inert proxy whose properties resolve to callables and whose calls
resolve to undefined, making a late `window.api.pty.*` call a harmless no-op.
The next test replaces it wholesale via installTerminalTestGlobals(), and no test
asserts that `window` is absent.
2026-08-15 00:54:20 -07:00