mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 16:02:43 +00:00
Neil
e9e238c883
* 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.