mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
72befaf360e7303b9948112d4e83167eec2cb34b
633
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  Brennan BensonandMerge Sim
|
72befaf360 |
feat(native-chat): show Claude subagent activity on the shared carrier (#18806)
* feat(native-chat): show Claude subagent activity on the shared carrier Claude's `message:system:task_*` frames are classified `status-chrome` and reach the transcript as nothing at all, so a turn that spawns subagents renders as an idle turn. The journal translator now reads them into the shared subagent-group carrier — no new UI, and the frames stay `status-chrome` so nothing prints a raw opcode row. `local_agent`, `local_workflow` and `local_bash` tasks share that channel and all carry a `tool_use_id`, so `task_type` is the discriminator and a backgrounded `sleep 20` stays out of the roster; `subagent_type` covers releases that predate `task_type`. `skip_transcript` tasks never render, `is_backgrounded` children survive the turn-end sweep, and a resumed task re-announced under a fresh tool id is aliased onto its `task_id` rather than duplicated. A child still reported as working when the turn — or the session — ends becomes `unverifiable`: contact was lost, which is not evidence it exited. * fix(native-chat): stop the Claude subagent roster dropping its own rows The roster published under the same coalescing key it appends the row with, and the sink queue replaces any queued operation sharing a key regardless of kind: once a write was in flight, each new append evicted the pending publish and the next publish evicted that append, so the body never reached the journal and `lastSerialized` had already moved past it. Publish now takes the sink's own slot, as the Codex streams do. A tombstoned row could never come back: the non-batch item-row builder derived its revision from `items` alone, so a re-add was built at revision 1 against a tombstone at 2 and the reducer discarded it forever. It now takes the same `max(items, tombstones)` the batch builder already used — reachable here because an announcement that reveals a `local_bash` task empties and tombstones the group row that a genuine subagent later in the turn reuses. `settleTurn` swept whatever group the key named at the time it ran, so children rostered before any turn key existed were never swept, and a turn whose result never arrives was left working forever. The ending turn's key is now an argument, a superseding turn start settles the turn it replaces, and every turn end also sweeps the outside-turn group. Teardown without an `ended` event, and eviction past the group bound, both lose contact instead of stranding a row at `working`. Label ordinals are a high-water mark now: releasing one on a re-label handed the next child an ordinal that was already on screen. * fix(native-chat): bound subagent-group blocks on every wire that carries one Adding a fifth arm to `NativeChatBlock` made every consumer that assumed four wrong. Two of them ended in `return block`, so they compiled while handing a roster straight through: the mobile RPC sanitizer shipped it unclipped past both mobile char caps, and the legacy transcript import stored an untrusted roster unbounded. Both now clip each label and cap the entry count the way they bound their other blocks. The remaining three sites did not compile at all. The worker transcript payload and the live-session benchmark get real arms rather than casts — a cast would have turned the transcript one into a third silent passthrough inside the wire byte budget — and the CLI worker output renders a roster with its shared summary instead of `[image omitted]`. The mobile sanitizer moves to a sibling module beside the image-block one: the file sat exactly on the max-lines bound, and the block bounds are a self-contained concern with their own caps. Also caps the roster's `subagent_type` label fallback, which reached the journal uncapped, and covers the new block type in the schema audit. * test(native-chat): cover the capped subagent_type label The roster stores the frame's label verbatim, so the cap on the `subagent_type` fallback is the only thing bounding it. * fix(native-chat): type the roster fixture so the suite typechecks The mobile-cap test built its entries with an inferred `state: string`, which is not a `NativeChatSubagentState` — the only typecheck failure on the branch. * fix(native-chat): stop child traffic rostering an id Claude never announced `observeChildActivity` minted a provisional row for any `parent_tool_use_id` outside the excluded set. An id that was never announced is never excluded, so a nested Task, a workflow child, or a grandchild parented to a tool id inside the sidechain each produced a permanently unlabelled `subagent` row that could only ever end `unverifiable`. The bounded exclusion set cannot cover an id no frame ever declared, and in a long session it can forget a genuine exclusion. Track instead whether this CLI announces tasks at all — set by ANY `task_started`, including one the subagent filter rejects. Once it has, an undeclared child is provably not a new subagent, so no row is created. The provisional path now serves only releases that announce no task frames, which is what its comment already said it was for. The label-ordinal test moves to an announcement-driven removal, the scenario that path now actually reaches; it still fails if `remove` releases the ordinal. * fix(native-chat): outrank the tombstone when building one too `buildJournalTombstoneRow` still built its revision from `items` alone, leaving it asymmetric with the item builder. It is correct today only because `upsertItem` clears the tombstone whenever a re-add wins — an invariant that lives in the reducer and was not pinned. Apply the same `Math.max`, and pin the invariant so the reducer cannot drop it silently. * fix(native-chat): stop an unrelated turn end settling an outside-turn child `settleTurn` swept the `outside-turn` group on every turn end, so a child Claude announced while no turn was live — a frame trailing the previous turn's result, or one that arrives before the first turn starts — was marked `unverifiable` by the next, unrelated turn ending. That state is terminal and latches, so the `task_updated: completed` that followed was discarded: loss of contact was recorded as the child's outcome on evidence that was never about that child. A turn end now sweeps exactly the group its key names. `outside-turn` belongs to no turn, so only an end with no key of its own reaches it, and what no turn end reaches `settleSession` does — reliably, since teardown without an `ended` event also routes through it. The cost is a child outside every turn showing `working` a little longer; the alternative prints a wrong outcome that nothing can revise. Also pins that a subagent announced after a task the filter rejected still rosters: the announcement path was never what the child-traffic gate closes. * fix(native-chat): bound a subagent entry's id, not just its label Every site that bounds a `subagent-group` block clipped the label and handed the id through whole. From the Claude producer the id is bounded upstream, but the legacy transcript import reads an untrusted file, so an oversized id survived into the journal and then out to every wire that replays it — 64 entries of it, since only the entry count was capped. Each site now clips the id with the helper it already uses for its other bounded fields: the journal's inline-text bound on import, the transcript payload's metadata clip, and the mobile char cap (renamed, since it is no longer a label-only cap). * fix(native-chat): surface an adverse subagent outcome in the fallback sentence The roster row's plain-text stand-in counted only `working`, so a fan-out whose children all latched `unverifiable` (or `failed`, or `stopped`) rendered as "Ran 3 subagents" — a completion claim. Mobile and paired web have no roster renderer, so that write-time-frozen sentence is the entire row there, and collapsing `unverifiable` into something that reads like success is exactly what the SSH execution boundary forbids. It now appends the worst adverse count, worst-first across failed/stopped/ unverifiable, and shows it even while siblings still work — matching the Codex lane's shared `subagentGroupFallbackText` verbatim so collapsing the two copies later is a deletion, not a behaviour change. Also bounds the provisional entry id. `observeChildActivity` wrote the `parent_tool_use_id` straight into the entry's durable id with no length cap, while the announced path already rejects an over-long id via `claudeTaskId`. Both now share `isBoundedClaudeTaskId`, and the provisional path rejects rather than truncates, as the announced one does. * fix(native-chat): stop a subagent label ordinal and a clipped roster key colliding - claimLabel probes the labels the group actually rendered instead of a per-base counter, so a generated `Audit 2` cannot duplicate a provider's own `Audit 2`. - Bound `NativeChatSubagentEntry.id` with a head plus a digest of the whole id at every site that bounds it. The id is the roster key: a prefix clip merged two distinct children onto one entry. - Correct a stale journal-reducer test comment: tombstone cleanup is a map-state invariant, no longer load-bearing for revision ordering. * fix(claude): merge duplicate unhandled-provider-frame imports * fix(claude): preserve subagent lifecycle and bounded invocation identity --------- Co-authored-by: Merge Sim <sim@local> |
||
 Jinwoo Hong
|
98fdbc4ade | fix(orchestration): file federated worker mail under the coordinator Run (#19542) | ||
 Neil
|
e182930670 |
test: cover input in five simultaneously flooding SSH panes (#19071)
* test: cover keyboard input in five simultaneously flooding SSH panes * test: capture pane focus and buffers on flood input failure * test: capture pane focus and buffers on flood input failure * test: capture pane focus and buffers on flood input failure * test: record replay input loss and application fix dependency * test: record merged replay-input fix in the five-pane flood gate |
||
  
|
aeddfa463d |
perf(renderer): avoid per-second spinner animation events (#19407)
* perf(renderer): avoid per-second spinner animation events * fix(bench): ensure the Electron runtime before bench:spinners The script launches Electron via Playwright but skipped ensure:electron-runtime, which every other Electron-launching bench script runs first. * docs(renderer): scope spinner pixel-tolerance claim to paused-animation checks --------- Co-authored-by: m4air <m4air@m4airs-MacBook-Air.local> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> |
||
|
Neil
|
cbc7bb418c |
fix(ci): read the changed-path list past the first pipe buffer (#19409)
`pr-code-change-scope.mjs` read stdin with `readFileSync(0, 'utf8')`, a single read of fd 0. Once the writer outgrows the 64 KB pipe buffer that read returns early or throws EAGAIN, the script exits 0 having emitted no `name=value` pairs, and `tee -a "$GITHUB_OUTPUT"` records nothing -- so every lane the classifier gates is silently skipped rather than failing loudly. A PR opened long ago carries a stale `pull_request.base.sha`, so the gate's merge-base diff spans the whole base branch. PR #13178 diffed 13,294 files (773 KB) against a base 1,592 commits behind main and lost typecheck, test, static analysis, xterm patch sync, package and e2e to this. Stream stdin instead, matching how the sibling `pr-e2e-source-routing.mjs` already reads the same list in the same workflow. |
||
|
Jinwoo Hong
|
c37413271e |
perf(mobile): open a session with parallel startup RPCs and a pre-warmed terminal engine (#19260)
Startup RPCs now fan out in parallel and the xterm engine pre-warms inside the real terminal frame while they are in flight, so the first pane inherits a warm WebView and an already-measured viewport instead of paying a round trip for it. The pre-warm opens its engine before measuring: web-ready only reports that the bundle loaded, and the WebView answers a measure with null until a terminal exists. It also pre-warms at the user's saved text size, because cell size is what the frame height gets divided by. Host writes such as worktree.activate wait for an evaluated status.get reply. Navigation still fails open when a host cannot answer one, but that fallback no longer reads as a passing compatibility verdict. |
||
 
|
a899f92402 |
feat(windows): enable structured Codex chat on native Windows (#18519)
* feat(native-chat): enable Windows structured sessions
* fix(codex): prove native Windows process identity
* style(codex): format Windows session seam
* fix Windows structured Codex admission
* fix(windows): reprobe missing process identity capability
* fix(windows): decide folder-workspace WSL routing before the click
Review found pathUsesWslUnc exported but unused, and the folder composer
hardcoding worktreeUsesWslPath:false. Together those meant a folder picked
under a \\wsl.localhost\ parent routed to structured chat, then got refused
by the host and fell back AFTER the click -- which defeats the lane's own
design goal that create cannot fail after the click.
The group's parentPath is in scope at submit and the workspace is created
under it, so the parent decides WSL-ness pre-click. Wires pathUsesWslUnc
there and adds tests for the helper, including the unhydrated-store case
that previously threw.
* fix(windows): collapse the gate derivation to one call, restoring max-lines
CI static analysis failed: launch-agent-in-new-tab.ts crossed the 300-line
oxlint ceiling. Adding a max-lines disable is forbidden, so the two gate
derivations collapse into one readWindowsStructuredGateInputs() call --
a store-backed site now adds one line and one import name instead of two.
Better shape anyway: one derivation entry point rather than two reads a
call site must remember to pair.
* fix(windows): engage the legacy fallback when the host THROWS a refusal
Review found a P1 this merge composes: neither parent could reach it. At the
lane head the only structured entry was launch-agent-in-new-tab (full
store-backed WSL check); on main all win32 was refused. The merge enables
win32 in creation flows that pass no projectRuntime, so a WSL folder
workspace, a WSL-configured repo, or a repair-required runtime now routes
structured -- and the host refuses correctly, but by THROWING rather than
returning {ok:false, refusal}.
Callers engage their legacy-terminal fallback on the refusal CLASS, so an
unmapped throw arrives as a generic RPC rejection: no fallback, empty
workspace, error toast, prompt stranded in the launch outbox. Pre-merge the
same action opened a legacy terminal agent.
Map the host's thrown definitive refusals onto the refusal class at the
launch boundary, so every creation flow -- present and future -- degrades to
the legacy terminal instead of stranding. Narrow predicate: unrelated
failures (ECONNRESET, empty message, non-Error) still propagate untouched.
Ablation-proven: removing the mapping reddens the fallback test.
* fix(windows): teach the mobile RPC double the status probe the lane added
CI's first-ever run on this lane caught a pre-existing lane defect. The lane
changed status.get to resolve through
runtime.getStatusAfterWindowsProcessStartTimeProbe(), but never taught the
mobile-surface runtime double about it, so status.get failed for mobile
clients with "not a function". The lane's own test list did not include this
file and the lane had zero CI, so nothing ever ran it.
The real runtime always implements the method; the double omitted it.
* chore: merge current main and regenerate the localization runtime catalog
CI static analysis failed on a stale en-runtime-required.json: main added
onboarding integration-capability keys, and the generated catalog is checked
against the PR MERGE result, not the branch alone -- so it read clean locally
while failing in CI. Merging current main (
|
||
|
Neil
|
314506003a |
fix: retain MSYS shell descendants in their terminal job (#19068)
* fix: retain MSYS shell descendants in their terminal job * test: complete MSYS regression CI registration and teardown contract * fix(windows): deny job breakaway for the whole Cygwin/MSYS shell family The per-PTY job probed only msys-2.0.dll, and only for bash.exe/sh.exe. Cygwin ships the same spawn.cc breakaway logic under cygwin1.dll, and an MSYS2 zsh escapes exactly like its bash does, so both kept the orphan bug. Probe the runtime DLL on the shell's own search path instead of matching shell names: that is the property that decides whether the runtime will ask for CREATE_BREAKAWAY_FROM_JOB, and it drops the name special-casing. * chore(patch): restore the conpty.cc index line The earlier hand-edit dropped it while every sibling section kept one. Recomputed against the real blobs: applying this patch to 7b286d3d yields exactly 4b06d185, so git apply -3 has its fallback back. |
||
|
Jinwoo Hong
|
fb322046e8 |
skills: rewrite and trim the seven non-orchestration guides (#19128)
* skills: rewrite the seven non-orchestration guides to one outcome-first standard
Every guide leads with Result / Done / Safe failure, states conditions instead of case lists, keeps one done bar and one autonomy envelope, and loads references at the point of use via `skills get <topic> --full`. orca-cli drops from 424 to 260 always-loaded lines with three references; orca-per-workspace-env from 794 to 397 with five.
Defects fixed in shipped guides: `emulator camera` (no such command), iOS `permissions` (backend refuses it), Android pane described as in development, `relayGracePeriodSeconds: 0` documented as immediate teardown (it is unbounded), doctor `ok: true` hiding `warn`, an SSH exemplar setting both `jumpHost` and `proxyCommand`, a provisioned-root fetch from `origin`, and the Linear unconfirmed-write rule keyed on four verbs when ten emit it.
The resolver ladder, placeholder rule, and older-binary fallback shared by every installable SKILL.md now come from one skill-stubs/_shared/cli-resolution.md fragment composed by the generator, which also bundles per-guide references into --full. New guards: every ORCA invocation and flag resolves against COMMAND_SPECS, descriptions carry no angle-bracket tokens, reference routing is checked both ways, and an always-loaded size ratchet (300 lines) that guides may leave but never join.
* skills: address review on the SSH recipe and the parity guard
- ssh-host create script: route the bootstrap ssh through the chosen jump host or proxy command, refuse both at once, use StrictHostKeyChecking=accept-new instead of a blind ssh-keyscan append, and pass gh_token/project_root/repo_url/repo_ref to the remote bash via printf %q so a quote in a value cannot break out of the command.
- per-workspace-env envelope: the step-10 workspace test the user asked for is no longer forbidden by the same paragraph.
- linear guides: name the full verb, ORCA linear list-issues.
- parity guard: a prefix reference such as ORCA linear --help or ORCA emulator --webcam now has its flags checked against every command under that prefix; only an exact path or an explicit ... was checked before.
* skills: tighten prose in the seven rewritten guides
Shorter outcome spines, one idea per sentence, no restated rationale after a rule. No rule, command, or pinned phrase changes; 47 net lines fewer across the guides and references.
* skills: route orca-cli and per-workspace-env gates through --reference
Both guides told agents to load --full at a gate because the per-reference
selector did not exist when they were written. Now that main serves
`skills get <topic> --reference references/<file>.md`, load only the
named file and keep --full as the fallback for an older CLI, matching the
orchestration kernel.
* skills: drop outcome-spine boilerplate from the CLI-wrapper guides
The Result/Done/Safe-failure preambles and Next Action closers restated
rules the body already carries. Agents stop fine without them, and for
a CLI wrapper the command surface is the guide. Keeps the one substantive
rule computer-use's Done block added (never report unverified as success)
inside Action Rules. orchestration and per-workspace-env keep theirs:
those are multi-step workflows where the done bar is load-bearing.
(cherry picked from commit
|
||
|
Neil
|
1848855515 |
test: enable software WebGL for Linux CI headful specs (#19001)
* test: enable CI WebGL and route GPU-dependent regressions * test: retain headful atlas cases in terminal rendering goldens * test: reuse golden command in project coverage assertions |
||
|
Neil
|
1e301ab1df |
test: cover native Wayland Hangul in isolated CI (#19174)
* test: exercise native Wayland Hangul in isolated CI session * test: wait for nested compositor socket before selecting IBus * test: align Wayland IBus discovery with GNOME environment filtering * test: assert Wayland launch and register native Hangul evidence |
||
|
Neil
|
2e8fa3fe9b |
test: exercise packaged browser compatibility in scheduled CI (#19157)
* test: exercise packaged browser compatibility in scheduled CI * test: record final packaged workflow participation evidence * test: expose manual packaged revision and simplify executable check * test: reject missing package checksum assertion |
||
|
Neil
|
4120501979 |
perf(store): detect Zustand rerender churn the current audit cannot see (#19059)
* perf(store): detect Zustand rerender churn the current audit cannot see The app-store-performance audit only understood inline selectors passed to a hook imported literally as `useAppStore`, so three shapes went unlinted: - a selector referenced by name (`useAppStore(selectRows)`), including one hoisted below its call site — resolved now via a Program:exit pass - the sibling store hooks (`usePluginPanelsStore` and friends), matched by the use<Name>Store convention on local imports; React's `useSyncExternalStore` matches that shape and is excluded - a fresh reference nested inside a `useShallow` projection, which is the worst case of the three: the comparator runs on every write and can never match, so the memo silently buys nothing `no-nested-fresh-under-shallow` covers the last one. `src` is clean against all four rules today, so this is a ratchet rather than a cleanup. The write side stays undecidable statically — whether a `set()` reallocated for nothing depends on the payload — so it gets a runtime probe instead. withStoreIdentityChurnProbe counts writes that replace a field's reference while its value stays equal, and can name the calling site. Cost when disarmed is one boolean load per write, matching react-commit-cascade-write-probe. * perf(store): scope the churn probe's scan to the write's own keys recordWrite iterated Object.keys of the full post-write state, so the armed cost scaled with the store's top-level field count (hundreds) rather than the size of the write. `set(partial)` merges, so no field outside the partial can have changed. The wrapper now resolves a functional updater itself and iterates the resolved partial's keys. Same function, same argument, called once — there is a test pinning that, since calling it twice would double any work a slice does inside its own updater. A replace write drops absent fields, so that path still scans every field. Disarmed cost is unchanged: one boolean load. * perf(store): follow a selector one hop into its helper Review feedback: both the lint rule and the manual sweep it was checked against only looked at the inline selector body, so neither could see a fresh allocation made inside a helper the selector calls — and delegating to a module-scope helper is the idiomatic shape here. Two methods sharing a blind spot is not corroboration. The two fresh-reference rules now resolve a single hop into a module-scope helper. The predicate used across that hop is deliberately stricter than the inline one: it requires EVERY returned expression to allocate unconditionally, so the common `cache.get(k) ?? buildFresh(state)` identity-caching shape is not flagged. An unresolvable helper is left alone rather than guessed at. Still zero hits across 20,330 files, so this stays a ratchet. * perf(store): keep the churn probe off the shipped write path Review hardening for the churn probe and the widened lint rules. Probe: it no longer resolves a functional updater itself. Zustand keeps sole ownership of when and with what argument an updater runs, so the middleware cannot double-invoke it or hand it a stale state. Object partials still scope the scan to the write's own keys; updater and replace writes fall back to the full field list, which costs one Object.is per untouched field and nothing more, since the deep compare only runs on replaced references. store/index.ts installs the probe only when import.meta.env.DEV or e2eConfig.exposeStore is set, the same gate as __store exposure. Nothing in the app arms it, so a shipped build was paying a wrapper frame per write for a diagnostic it could never read. The cascade probe stays unconditional because crash telemetry arms it in the field. Site capture now skips any *-probe.ts frame; under the real composition the first non-node_modules frame was the cascade probe's wrapper, so every churn was attributed to react-commit-cascade-write-probe.ts:32 instead of the caller. Plugin: named-selector recording is restricted to module scope. A component-local `const selectRows = ...` used to overwrite the entry for a same-named imported selector and flag an unrelated useAppStore(selectRows). The any-branch and every-branch allocation predicates are one function with a flag, the Object.* static list is a Set, and import recording is a single pass. Tests: updater called once with live state, identical-state writes ignored, disarmed path forwards exact arguments without calling get(), full composition with the cascade probe (no drop, no double, correct site), and the module-scope shadowing case for the plugin. |
||
|
Jinwoo Hong
|
b8311d509a |
Revert "skills: rewrite the seven non-orchestration guides to one outcome-first standard (#18724)" (#19126)
This reverts commit
|
||
|
Brennan BensonandMerge Sim
|
1478101342 |
fix(windows): unblock structured native chat by exposing process creation time (#18986)
* fix(windows): guard process creation times
* fix(windows): ask the relay's bare addon for creation times too
The relay addon build now emits creationTimeMs, but the runtime binding
for the bare addon still declared only CommandLine, so a Windows relay
host requested flag 2 and every row came back without a creation time.
That leaves captureWindowsDescendantSnapshot returning null and
verifyWindowsProcessIdentity false forever on those hosts -- the relay
half of the patch was unreachable.
Naming CreationTime in the adapter is safe because the bare addon is a
content-hashed relay artifact: it ships in the same immutable relay
directory as the bundle reading it, so it can never be older than the
code asking for the bit.
Also bound the win32 guard test on our own row, which the addon can
never fail to answer, so an unconverted FILETIME or a 1601-epoch stamp
fails instead of satisfying a bare count.
* fix(windows): make the compiled addon prove its own CreationTime support
CI caught the real defect: the win32 guard test read
isWindowsProcessStartTimeAvailable() as true and then found 0 rows
carrying creationTimeMs. Unlike node-pty, this package publishes a
prebuilt .node at the same build/Release path node-gyp writes to, so
pnpm patches the source tree and leaves that binary alone. A host then
holds a patched lib/index.js -- ProcessDataFlag.CreationTime and all --
over a binary that ignores flag 4, and neither a load check nor a path
check can see the difference.
So the binary now says so itself: addon.cc exports
supportedProcessDataFlags, lib/index.js re-exports it, and
- windows-process-tree-creation-time.cjs asserts it during install,
which is what forces a from-source rebuild. It is shared by the Node
probe in ensure-native-runtime.mjs and the Electron probe in
rebuild-native-deps.mjs, exactly as node-pty-job-ownership.cjs is --
the Electron half matters because that probe decides onlyModules, so
without it the packaged app would ship the stale prebuilt.
- isWindowsProcessStartTimeAvailable() gates on the reported bit, not
the enum. Believing the enum is worse than reporting false: the
descendant snapshot returns null forever and the exit proof latches
unverifiable while structured chat believes it has a reaper.
rebuildNodeRuntimeModules could not actually have rebuilt this package:
the patched binding.gyp includes deps/node-addon-api, which the tarball
does not ship, and node-gyp must run from the physical dir.
Also closes the relay repair path's divergence: repairCreationTimeSources
wrote the C++ but not the buildNode splat or the tree-node typing, and
assertPatchApplied checked neither, so a repaired tree passed as patched
with buildProcessTree silently dropping the field.
The guard test is unchanged.
* fix(windows): keep the process-tree patch LF-only
windows-process-tree-patch-contract.test.mjs requires the patch file to
carry no CR bytes. Regenerating through pnpm patch-commit emitted 199 of
them, because the creation-time change is the first to touch files the
package ships as CRLF (src/process.h, src/process_worker.cc,
src/addon.cc, lib/index.js, lib/index.ts, the typings) -- and #17886's
own hunks over binding.gyp and src/process_commandline.cc carry the rest.
Stripping them is safe and changes nothing the lockfile records: pnpm
hashes patches CRLF-normalized, so the digest stays
e66202cc623996d02040c93449eb9ae353fddadf426cb53202a59ee710ee6fe7 and now
equals the file's plain sha256 too. It also still applies -- verified
against a deleted store entry, not a warm one -- and the precedent was
already there: the previous patch was LF-only and had been patching
those same CRLF files all along.
ensure-native-runtime.test.mjs stages the siblings the script loads at
module scope into its temp project. The import walk added by #17886 sees
`from './x.mjs'` only, so the createRequire'd .cjs siblings still have to
be named, and this PR adds a second one.
---------
Co-authored-by: Merge Sim <sim@local>
|
||
|
Jinwoo Hong
|
59fe8266bd |
fix(orchestration): keep worker lineage across app restart (STA-6366) (#19121)
* fix(orchestration): keep worker lineage across app restart (STA-6366) Terminal handles are minted per process, so after a restart the projected parent (coordinator or creator) named a handle no live row carried and every worker rendered as a top-level row. The projection now resolves the parent from the durable pane keys (runs.coordinator_pane_key, tasks.created_by_pane_key) whenever the stored handle is not one this process minted, re-resolves it to the live handle for that pane, and omits stale handles so they cannot mismatch a row. The creator-pane incarnation gate is untouched: it still decides mutation authority, and display lineage no longer depends on it. Dispatch lookup also passes the pane identity so a worker's own dispatch resolves once its handle is reminted. * test(orchestration): compare lineage without the merged attention field |
||
|
Jinwoo Hong
|
15d0f8aedf |
skills: rewrite the seven non-orchestration guides to one outcome-first standard (#18724)
<!-- orca-pr-loc -->
<!-- Programmatic LoC summary. Do not edit by hand; rewritten on every commit. -->
| | Files | Added | Deleted | Net |
| :--- | ---: | ---: | ---: | ---: |
| Test | 6 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$544 | $\color{#cf222e}{\Huge{\mathbf{−}}}$49 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$495 |
| Prod | 36 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$1719 | $\color{#cf222e}{\Huge{\mathbf{−}}}$1703 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$16 |
<!-- /orca-pr-loc -->
## ELI5
Orca ships eight skill guides that agents read before running the CLI. Seven of them (everything except `orchestration`, which #16904 rewrites) were command catalogs that had drifted from the binary. This PR rewrites them so an agent reads the outcome, the done bar, and the safe-failure rule first, loads reference material only at the step that needs it, and never sees a command or flag the installed CLI does not define.
## What changed
- **Seven guides rewritten** to one standard: outcome spine first (Result / Done / Safe failure), conditions instead of case lists, one done bar, one autonomy envelope, references loaded at the point of use via `skills get <topic> --full`, every runnable invocation spelled `ORCA`. `orca-cli` is 424→260 always-loaded lines with three references (browser, automations, publishing); `orca-per-workspace-env` is 794→397 with five (provider-vercel, ssh-host, docker-ssh, windows-scripts, failure-modes).
- **Defects fixed in shipped guides:** `emulator camera` (no such command), iOS `permissions` (backend refuses it), Android pane described as "in development" (shipped in June), `relayGracePeriodSeconds: 0` documented as immediate teardown (it is unbounded), doctor `ok: true` hiding `warn`, an SSH exemplar setting both `jumpHost` and `proxyCommand`, a provisioned-root fetch from `origin`, the Linear unconfirmed-write rule keyed on four verbs when ten emit it. Linear and emulator descriptions dropped embedded commands and angle-bracket placeholders (651→329, 732→404 chars).
- **Generator bundles references.** `skill-guides/<name>/references/*.md` is appended to `--full`; `skills get` help says compact by default, full with references.
- **Stubs single-authored.** The resolver ladder, placeholder rule, and older-binary fallback shared by all eight installable `SKILL.md` files come from one `skill-stubs/_shared/cli-resolution.md` fragment composed by the generator. Projections were byte-identical before the content fixes.
- **Guards:** every `ORCA <cmd>` and flag in every guide and reference resolves against `COMMAND_SPECS` (this found the camera defect); descriptions ≤1024 chars with no angle-bracket tokens; reference routing checked both directions; an always-loaded size ratchet (300 lines) that guides may leave but never join. `orchestration` (440 lines on main) is recorded as an exception until #16904 lands its kernel.
## Relationship to #16904
Split out of #16904 so that PR carries only the orchestration guide. On main, `terminal send` has no `--wait-submit` / `--retry-request` and the orchestration kernel still carries the resolver ladder and worktree-selector rule, so this branch pins `accepted: true` for handoff receipts and leaves the orchestration pins where main has them. The merge in either direction is mechanical: #16904 rebased on this becomes a one-file `orchestration.md` change plus dropping the two exceptions.
## Standard
Compound Engineering's portable skill-authoring guidance (outcome spine, conditions not cases, pinned fragile commands with an ordered hatch, references at point of use). NVIDIA SkillEvaluator Tier 1 (`schema,pii,license,quality,unicode,lint`) was run on every guide; its deterministic checks pass, its template nudges (Instructions/Examples sections, 50–150 char descriptions) do not apply to Orca's stub architecture and were not applied.
## Testing
- `pnpm typecheck:tsc:cli` clean; `check:code-quality:changed` and `check:react-doctor:changed` 0 findings
- `pnpm verify:bundled-skill-guides` and skill-bundle manifest verify clean
- vitest over `config/scripts`, `src/cli/skill-guide-cli-parity.test.ts`, `src/cli/skills.test.ts`, `src/cli/specs/skills.test.ts`, `src/cli/help.test.ts`, `src/main/skills`: 240 files / 2,019 pass
- Live smoke on the built CLI of every `skills get <topic>` and `--full`, every emulator, linear, and vm verb named in the guides, and every projection's resolver, GNOME warning, and bounded fallback (done on the #16904 branch before the split; the guide bodies are identical here except the send-receipt vocabulary noted above)
## Deferred product decisions
Merging `orca-emulator` and `orca-emulator-android` into one skill with a platform branch; collapsing `linear-tickets` to a guide alias; a `skills get --reference <name>` selector so a gate table can load one file; a fresh-agent routing eval before trimming the `orca-cli` (1,015 chars) and `orchestration` descriptions, whose quoted triggers each fixed a routing misroute.
|
||
|
Jinwoo Hong
|
0c33f58e8a |
fix(ssh-relay): daemon owns the endpoint credential; a losing start never rotates it (#19052)
<!-- orca-pr-loc -->
<!-- Programmatic LoC summary. Do not edit by hand; rewritten on every commit. -->
| | Files | Added | Deleted | Net |
| :--- | ---: | ---: | ---: | ---: |
| Test | 19 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$962 | $\color{#cf222e}{\Huge{\mathbf{−}}}$136 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$826 |
| Prod | 18 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$295 | $\color{#cf222e}{\Huge{\mathbf{−}}}$116 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$179 |
<!-- /orca-pr-loc -->
## Symptom
Live 2026-09-05 (Orca 1.4.198 client, Ubuntu host): both relay processes `kill -STOP`ped for 20 s, then `-CONT`. The client redeployed while the host was frozen. Its fresh daemon lost the socket bind (`Socket path already in use`) but had **already rewritten** `relay-<id>.sock.credential`. The surviving daemon kept its in-memory credential, so every later `--connect` got `Endpoint credential mismatch; closing socket`, then `Grace started … timeoutMs=0 … ptys=1, clients=0` every ~20 s, forever. Only a manual `kill -TERM` cleared it. Receipts: `review-archive/orchestration-v3-pr16904/smoke-receipts-t012b/E16,E17,E18,E24`.
Three independent defects kept the wedge alive; each is fixed at its own seam.
## Fix
**1. The relay daemon owns credential publication (race-free under two concurrent starters).**
`relay-daemon.ts` binds the socket first, then publishes via the new `src/relay/relay-endpoint-credential-publication.ts`: adopt a valid pre-existing file (older clients still pre-write), else mint 32 random bytes and write temp+rename at 0600. A start that loses the bind exits inside `listen()` and never reaches the file. Why this option and not restore-on-loss or a client-side write: the only process that can *prove* ownership is the one whose `listen()` succeeded, and that proof is atomic with the bind. The client-side pre-write (`ssh-relay-endpoint-credential.ts`) and the launch-command `chmod 600`/`icacls` are removed on POSIX and Windows. The racing test also exposed that macOS reports a mid-bind collision as `EEXIST` rather than `EADDRINUSE`; `relay-socket-ownership.ts` now treats both as "held or stale".
**2. The client distinguishes "no daemon" from "daemon present but not answering", and never rewrites.**
A credential refusal is now typed on the wire: the daemon replies `orca-relay-handshake-credential-mismatch` (same frame type, no new opcode) and the bridge exits **43**; `waitForSentinel` maps it to `RelayCredentialMismatchError`, which the takeover treats as handshake-refusal evidence exactly like exit 42. A relay that holds the endpoint but **never refused** (the stalled-host shape: kernel backlog accepts the probe, handshake gets no answer) is now `RelayEndpointUnresponsiveError`, routed to the relay-lost backoff instead of the terminal Reset Relay path. Silence is not a decision (`docs/reference/ssh-execution-boundary.md`).
**2b. Deploy honours the verdict.** The 40 s live run exposed that the `--connect` catch block in `deployAndLaunchRelay` predates the incumbent probe and swallowed both verdicts as "probe failed, launch fresh", so a fresh daemon was still launched over the live one (it lost the bind by luck, which is exactly the collision in the incident). Held and Unresponsive now propagate; the session backs off on Unresponsive and surfaces Reset Relay on Held. Red-first in `ssh-relay-deploy-incumbent-verdict.test.ts`.
**3. The daemon cannot be wedged by a rotated file, because nothing can rotate it.**
The credential lives in the content-hashed relay dir, and after (1) the only writer is the daemon that owns the socket, so the "file changed under a live daemon" state the incident depended on is no longer reachable in-product. The credential is therefore fixed for the daemon's lifetime, as a plain secret should be. A hand-edited file is refused with the typed reply until restored (tested). Startup adoption of a pre-written file applies an owner-only + same-uid rule (review finding): anything else is replaced by a fresh mint. An earlier revision of this PR also re-read the file on mismatch and adopted it; that was removed as unreachable machinery that turned the credential into a per-handshake file-ownership check.
**3b. Fail closed between bind and publication.** A client that arrives after `listen()` resolves but before the credential is set is refused, not admitted as `unproved`. Nothing can be delivered in that window today; the guard makes the boundary structural instead of an event-loop ordering fact. Red-first in `relay-reconnect-listener-credential-gate.test.ts`.
**Wire compat.** New optional handshake reply only; an old `--connect` hits `Unknown handshake type` and exits 1 pre-sentinel, which it already treated as a generic failure. New daemon adopts an old client's pre-written file; new client still passes `--credential-file` so an old daemon reads it as before. Absence of exit 43 is never used as evidence.
**Also.** `terminal create` on a reconnecting SSH host now says what to do instead of a bare `No PTY provider for connection "<id>"` (prefix preserved; the renderer matches it).
## Tests (red first)
- `src/relay/subprocess.test.ts`: two `--detached` starts race one socket + credential file → exactly one reaches the sentinel, loser exits 1 with `Socket path already in use`, file valid + 0600, a `--connect` reading it reaches `relay.status` and reports the winner's pid. Red before (both starters died: daemon required a pre-existing file), green 6/6 after.
- `src/relay/relay-endpoint-credential-publication.test.ts`: mints after bind; adopts a pre-written 0600 file; replaces a pre-written 0644 file with a fresh mint; refuses a stale credential with exit 43 while still serving the real one, and keeps refusing a rewritten file until it is restored.
- `src/relay/relay-reconnect-listener-credential-gate.test.ts`: a client in the bind-to-publish window is refused and never attached; after publication the right credential is accepted and a wrong one refused; a daemon launched without a credential file is not gated. Red without the guard.
- `ssh-relay-deploy-incumbent-verdict.test.ts`: live-but-silent incumbent → `RelayEndpointUnresponsiveError`, refused → `RelayEndpointHeldError`, and in neither case is `--detached` launched; a failed `test -S` probe still launches fresh. Red 2/3 without the deploy change.
- `ssh-relay-deploy-helpers.test.ts` (exit 43), `ssh-relay-endpoint-takeover.test.ts` (refused → Held even with no `lsof`; silent → Unresponsive, nothing unlinked or signalled), `ssh-relay-session-terminal-error.test.ts` (Unresponsive → `onRelayLost`, not terminal). Deploy/namespace/native-deps tests updated to assert the client writes **no** credential.
## Live proof
New `tests/e2e/ssh-docker-relay-stall-credential.spec.ts` (claimed in `run-ssh-docker-e2e.mjs` and PR source routing), two cases: `kill -STOP` every relay pid in the container, send input during the freeze, hold **20 s** (the incident's duration, which races the mux liveness timeout) or **40 s** (past it for sure), `kill -CONT`; assert status back to `connected`, same pty, same daemon pid, same credential inode and content, relay.log did not shrink (a relaunch truncates it) and has zero `Endpoint credential mismatch` / `Socket path already in use` lines, in-stall input delivered at most once.
Run output (local, fixture image `orca-e2e-ssh-relay:3a864c665ba2cefd`, `ORCA_E2E_SSH_DOCKER=1 SKIP_BUILD=1 ORCA_E2E_FORWARD_APP_LOGS=1 … --project electron-headless --workers=1`, head `c2c20fd994`; re-run identically on the final head after the credential-lifetime change, 2 passed (1.7m), same annotations, and the bind-to-publish refusal never fired):
```
✓ keeps the same daemon and credential across a 20s relay freeze (38.3s)
relay-processes-stopped: 2 relay-processes-continued: 2
bridge-pids-before-after: 480 -> 480
socket-clients-accepted-before-after: 1 -> 1
in-stall-input-delivered: 1
✓ backs off and reattaches, never relaunching, across a 40s relay freeze (57.5s)
relay-processes-stopped: 2 relay-processes-continued: 4
bridge-pids-before-after: 480 -> 1202
socket-clients-accepted-before-after: 1 -> 3
in-stall-input-delivered: 1
2 passed (1.6m)
```
Client log in the 40 s case shows the new path end to end: `Relay channel lost … reconnect attempt 1/6` → `Socket probe result: "ALIVE"` → `Socket reconnect failed … Relay failed to start within 10s` → `Relay endpoint incumbent: … verdict=live evidence=accepted-connection holders=unenumerable` → `Failed to re-establish relay … A relay still owns … but did not answer the handshake … Orca will retry` → `reconnect attempt 2/6` → `Reconnected to existing relay via socket`. The 20 s case never left the frozen bridge (same bridge pid, one accept), so it exercises the "silence is not death" side of the same race. The 20 s case passed 6/6 across the session; the 40 s case was red on the prior head (`Socket path already in use` + `Startup failed: listen EADDRINUSE` in relay.log from the swallowed verdict) and is green after 2b. Before the fix the same injection produced a fresh daemon that rewrote the credential and a survivor refusing every client.
The `relay-processes-continued` count exceeds `stopped` in the 40 s case because the timed-out client's `--connect` bridge and the loser-side processes are parked behind the frozen listener when `CONT` runs; they exit on their own once it resumes.
## Gates
`pnpm test src/relay src/main/ssh` 332 files / 3884 tests pass · `pnpm typecheck:tsc:node` clean · `check:code-quality:changed` 0 findings · `check:react-doctor:changed` 0 findings · `pr-e2e-gate-contract.test.mjs` 42 pass · no lint disables or max-lines bumps added.
## Noted, not fixed here
- `terminal list` `orphaned:false` / `terminal close` `ptyKilled:true` for a pane whose relay is gone (`orca-runtime-stop-explicitly-closed-tab-ptys.ts`): different seam, `@ts-nocheck` characterization-covered file.
- On a host with no `lsof`, a stalled relay still cannot be enumerated as the holder; it is now retried rather than declared held, but a relay frozen past the backoff budget still ends in the existing "reconnect manually" banner.
|
||
|
Jinwoo Hong
|
06a607a1d7 |
feat(orchestration): make multi-agent workflows durable (#16904)
<!-- orca-pr-loc -->
<!-- Programmatic LoC summary. Do not edit by hand; rewritten on every commit. -->
| | Files | Added | Deleted | Net |
| :--- | ---: | ---: | ---: | ---: |
| Test | 225 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$21666 | $\color{#cf222e}{\Huge{\mathbf{−}}}$2820 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$18846 |
| Prod | 348 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$17107 | $\color{#cf222e}{\Huge{\mathbf{−}}}$4706 | $\color{#1a7f37}{\Huge{\mathbf{+}}}$12401 |
<!-- /orca-pr-loc -->
## ELI5
Orca now treats orchestration like a durable control plane instead of inferring success from terminal keystrokes. Agents can tell whether a prompt was accepted or a turn started, replay an ambiguous request without sending twice, and recover coordinator mail after a crash. Completed workers can be inspected, released, or retained, and their panes no longer auto-resume as if the work were still running.
## What changed
- **Run receipts** from `run-create/use/current/show/list` are the row without routing plumbing (`home_database`, `coordinator_pane_key`) and without the duplicate `binding` object.
- **`terminal send` receipts are honest and idempotent.** `input_accepted` and `turn_started` are the only stages; `--wait-submit` observes without resending; `--retry-request <uuid>` replays the exact request against the same process incarnation. A transport timeout keeps the retry ID; only a different runtime answering strips it. Value-less or non-UUID `--retry-request` is rejected on the CLI and the SSH shim.
- **Mailbox delivery is committed before wakeup.** Pointer writes are staged in the DB before any PTY byte, replayed once after restart, and never emit a naked Enter. The watermark that parks concurrent deliveries is released with the DB reservation. Restart rescans pointer-pending and `dispatch:` mailboxes.
- **Lifecycle is a guarded transition graph** (`lifecycle-transition.ts`) with a table-driven test over every caller edge. Task reopen/overturn stays in the public contract. A PTY exit during `worker-stop` is the stop succeeding, not a failure.
- **Worker lifecycle CLI:** `worker-start` (`--spec` creates Task + attempt in one call), `worker-show`, `worker-read` (provider transcript first, bounded terminal fallback with a typed reason, local/WSL/SSH), `worker-stop`, `worker-abandon`, `worker-release`, `worker-retain`, `worker-list` (rowid-fenced pagination, fleet liveness, `attention`, literal `nextAction`).
- **Release is an explicit ownership table** (`decideWorkerTerminalRelease`): only an `owned` resource can be settled, the archive is mandatory where reachable, and an owner whose process is proven exited can always get out of `retained` via `archive_status: unavailable`. User-taken-over, external, and transferred panes stay retained.
- **Settled-worker resume fence** (folds in #17651): a settled dispatch whose pane is still open is fenced at settlement, on stop/abandon/exit, and at startup; lifted on release, retain, takeover, and pane reuse.
- **Liveness is `live` / `unverifiable` / `exited` only**, from execution-host evidence. Fleet projection reads the evidence clock, not the relay delivery clock. A host-certified exit outranks the worker's settled state. `unverifiable` never authorizes stop, abandon, retry, or release, in code or in the guide.
- **Federation:** structured reads negotiate by `method_not_found` so every shipped host keeps transcript-first output; exited remote workers are closed before being reported closed; epoch fencing holds across peer restart, downgrade, and pairing rotation; no per-second forced capability probe.
- **Schema v35:** repairs databases stamped v34 by the pre-fix branch (mailbox_handle default, index predicates), drops the write-only `lifecycle_transition_receipts` ledger and five never-read v31 identity columns.
- **Schema v36:** `dispatch:<id>` mailboxes get a real consumer generation on `dispatch_contexts` and `remote_dispatch_attachments`, bumped and fenced in the same transaction on every re-attach (manual inject, worker-start, federated attach). A stale worker whose Dispatch moved to another process now gets `consumer_fenced` instead of silently acking the new worker's Delivery. Run mailboxes already worked this way.
- **Schema v37:** `dispatch_contexts` records its creator (`creator_handle`, `creator_pane_key`), so a coordinator's context-only self-dispatch is bookkeeping rather than a nesting parent; before this, one self-dispatch made every later `worker-start` from that coordinator fail the depth cap. Pre-v37 rows keep counting (fails closed).
- **Dispatch-mailbox ownership is checked, not inferred.** A `check` from a process whose pane no longer holds the Dispatch, or whose last Attempt was abandoned/failed and moved to another terminal, gets `consumer_fenced` instead of an empty inbox that reads as "no mail yet". `--peek`/`--all` stay readable. A paneless caller still gets `stable_pane_required` with the rebind recovery.
- **Liveness certification is stricter:** a `process_exited` stage whose termination reason is `unknown` (a stop that was issued but never observed) projects `unverifiable`, not `exited`. Federated `worker-show` carries the execution host's verdict and host kind instead of a local guess. A live, ready worker with nothing pending has `nextAction: none` rather than pointing at the `worker-show` that produced it.
- **Wire:** `workerShow` keeps `dispatch.task_id` next to `taskId` for shipped CLIs. `ask --json` uses the standard `{ok, result}` envelope like every sibling verb.
- **Migration start-version detection** treats the two v32 recovery columns as versioned. Before this, every shipped database stamped below 32 resolved to the v6 floor and replayed the whole chain (the v23 backfill synthesized 68 phantom retained workers on a real v30 profile). Verified on a copy of a real 62 MB v30 profile: starts at 30, no row delta, integrity ok, 11 ms.
- **Skill guide** rewritten as a ≤200-line kernel plus seven references, to the outcome-first standard (Result / Done / Safe failure first, conditions not case lists, one done bar, references loaded at the point of use). The canonical loop uses `worker-start --spec`, names `worker-list` for completion accounting, documents `--retry-request` / `request-show` / `--wait-submit`, and requires positive evidence before any stall action. The other seven guides get the same treatment in #18724, split out so this PR stays orchestration-only.
- **`rpc/methods/orchestration-*`** (126 flat files) regrouped into `orchestration/{worker,federation,messaging,runs,gates}/`.
## Why
User reports showed the same boundary failures: false `agent_prompt_stalled` causing duplicate sends (#15180), coordinators unable to trust screen scrapes, cold-parked terminals receiving a pointer without the submit, settled workers accumulating as live tabs and auto-resuming after restart, and no way to tell a stalled worker from a working one.
## Linked issues
Fixes #15180. Fixes #17935 (orchestration skill description is 866 characters; a guard now caps every bundled skill at 1,024). Supersedes #17651 (fence folded in). Advances #16660, #16522, #14907, #13047.
## Review record
This PR was reviewed adversarially after revival: eight independent lenses (lifecycle, mailbox, send, worker, federation, transcript, complexity, live ergonomics), each required to prove findings with a failing test. That produced 16 proven blockers, all fixed with red-then-green regression tests, followed by two re-review rounds and a third fix wave that caught 3 regressions introduced by the fixes and 7 fixes that missed their target; all closed. A final pass (five lenses incl. a live built-runtime smoke, then a re-review of the fix wave) found and fixed seven more, chiefly the stale-worker mailbox steal, the self-dispatch depth wedge, and the unproven-exit certification. Three independent Codex (gpt-6-astra) passes followed: the first found nothing new, the second found and fixed 3 defects (task-status reachability, WSL-local host classification, peer-capability epoch), the third found and fixed 6 (production PTY controller never installed settled writes, ambiguous in-flight pointer failures allowed duplicate replay, SSH/relay deadlines cut off a valid `--wait-submit`, stop-vs-exit race during inspection, and two release-recovery paths for vanished or exited terminals). The full record (findings, proof tests, triage, declines with reasons) is archived outside the repo.
**Rework after the live smoke.** A first live cross-host run on the shipped adhoc build (this Mac, a paired Windows host on the same build, a paired Mac on 1.4.195, and an SSH host) found a P1: a running local worker read `unverifiable`/`missing_status` because the fleet snapshot rows lacked the terminal handle the matcher keyed on. A 59-row failure table over every bug fixed during review showed the same two classes recurring: a fact dropped in transit through optional fields, and two authorities for one fact. Two blind designs (Opus, Codex) converged on the same mechanisms, and the scoped tranches landed here with red-then-green seam tests from the real producer to the real consumer, faults injected only at the transport or hook-ingest boundary:
- **Settlement (data-loss class):** one three-valued `WriteSettlement` (`accepted | refused{reason} | unverifiable{reason, bytesHandedToTransport}`) from the SSH multiplexer through daemon client, providers, controller, to pointer staging. No boolean, no rejection-as-third-state. The two silent degrades that fabricated a handoff are deleted; a provider that cannot settle refuses before any effect. Pointer text and Enter share the contract; a partial flush is `unverifiable`, never `refused`.
- **Evidence identity (false-liveness class):** fleet agent-status evidence is a tagged union (`binding: worker | pane | unresolved{reason}`, `clock: observed | delivery`) minted once at ingest, so a hook row captured on one process incarnation can never bind to a later dispatch on the same pane. The matcher's `!worker.paneKey ||` defaults are gone. One host-scope parser replaces two.
- **Small pre-merge items:** `capability_unsupported` from an old peer is no longer relabelled `host_unavailable`; a producer census test asserts every agent-status consumer path projects a pane-only hook row as `live`.
Two ergonomics defects the second live run surfaced on a real database are fixed here too: a pre-v3 dispatch already marked `completed` projected as `outcome_unknown` / `requiresAction: true` forever (three copies of the outcome ladder disagreed on legacy rows; now one resolver, legacy `completed` reads `succeeded` with nothing to act on, legacy `failed` stays actionable on the failure), and an unscoped `worker-list` enumerated the entire database (now defaults to the Run bound to the calling terminal, `--run` overrides, and the receipt's additive `scope` field says which).
A third live round on the shipped adhoc build of `b082443e1f` (same four hosts) plus an unscripted run in the user's own prompt style (a plain Claude Code shell, `/orchestration`, three workers, zero errors, bound-Run default confirmed) found two more branch defects, fixed with red-then-green tests: a worker freshly started on a paired server projected `unverifiable`/`host_indeterminate` with `requiresAction` for ~3 minutes, including after its own `worker_done`, because the host's federation observation returned `missing_liveness_verdict` for any PTY the liveness register had not yet swept (the host now reads a connected pane it owns locally as `live`; disconnected or SSH-scoped panes stay `unverifiable`); and six pre-v3 completed rows still carried an `input` category because settling through the task-status path or `failDispatch` never closed the Dispatch's pending question threads (both paths close them now, and schema v38 closes threads already pending on settled rows). The guide's `worker-start` examples now show `--model sonnet`, since an omitted model inherits the launcher's default.
A Codex adversarial pass on the tranche diff found one real design hole (identity minted at read time instead of ingest, now closed) and two daemon settlement paths that threw instead of settling (fixed). Two `@ts-nocheck` runtime mixins on these paths were extracted into checked modules; the repo-wide `@ts-nocheck` count is unchanged at 171.
Deletions during review: ~1,900 lines (write-only ledger, unread columns, dead v1 archive path, test harnesses shipped in prod, duplicated liveness and state-machine copies, self-capability checks that were compile-time true).
## Testing
- `pnpm typecheck:tsc:node|cli|web` clean
- `pnpm run check:code-quality:changed` 0 findings; `check:react-doctor:changed` 0
- `pnpm verify:bundled-skill-guides`, `verify:skill-bundle-manifest`
- full `pnpm test` on the integrated head: 72,332 pass / 292 skipped; the only failures were three non-PR files (two zsh live-shell suites hit a node-pty spawn-helper ENOENT while a concurrent native rebuild ran, 44/44 in isolation; `release-checkout.unit.test.ts` is a known 30 s load timeout that passes in isolation on `origin/main` too).
- CI on
|
||
|
Neil
|
3be526c5e6 |
test: cover SSH reattach replay and enable deterministic Codex CI (#19106)
* test: cover SSH replay replies and run deterministic Codex restore scenarios * test: register replay probe unit command in reliability gate |
||
|
Neil
|
4d9e963ffd |
test: enable localhost SSH terminal and hook journey in CI (#19097)
* test: run localhost SSH terminal and hooks in CI * test: isolate localhost SSH session fixtures across repetitions * test: route remote agent hook source changes to localhost journey * test: record localhost SSH reliability evidence and remaining gaps * test: route the real SSH session hook authority |
||
|
Neil
|
f5960cec00 |
test: enable Docker SSH browser network route coverage in CI (#19095)
* test: enable Docker SSH browser network route journeys in CI * test: register Docker browser job in token permissions contract * test: declare SSH client dependency and narrow browser fixture routing |
||
|
Neil
|
3631f886a7 |
test: enable direct and client-hosted SSH browser coverage (#19090)
* test: enable direct and client-hosted SSH browser coverage * test: record twelve passing SSH browser journey repetitions * test: distinguish SSH journey evidence from unit runtime budget |
||
|
Neil
|
1d2e00819f |
test: restore SSH bulk-open freeze coverage in headed CI (#19081)
* test: restore SSH bulk-open freeze coverage in headed CI * test: record ten passing headed SSH freeze repetitions * test: record ten passing headed SSH freeze repetitions * test: route changed SSH freeze spec only to its dedicated lane |
||
|
Neil
|
f952f1ac96 |
test: run real WSL terminal launch and paste in PR CI (#19072)
* test: continuously exercise real WSL terminal launch and paste * test: establish live WSL reader before changing default shell * ci: pin WSL kernel installer and participation selectors * ci: route deleted WSL paths and record immutable run evidence * test: require exactly three WSL repetitions in lane contract |
||
|
Brennan BensonandMerge Sim
|
6933fd70d7 |
fix(packaging): ship Claude agent SDK with desktop builds (#19042)
* fix(packaging): include Claude agent SDK at runtime
* test(packaging): cover spaced runtime imports
* fix(packaging): verify every emitted main file for bare runtime imports
The packaged-main verifier read two fixed entry files, but rolldown hoists
modules shared by two entries into out/main/chunks. jsonc-parser is reached
only from a chunk today, so nothing verified it, and the agent-hooks entry
the list names contributes no coverage at all. An import that migrates into
a chunk would silently stop being checked -- the same blindness that let the
missing Claude agent SDK ship.
Scan every out/main/**/*.js entry in the asar instead, keeping the two
required-file assertions as a build-integrity check. Measured against the
shipped 1.4.198 app: 93 entries in 72ms, reporting the absent SDK and
nothing else.
Also tighten the specifier match with a (?<![.\w]) lookbehind. Orca has
three registry methods of its own named require(), two taking a string key,
so a minified registry.require('public-a') otherwise reads as a bare module
specifier and fails packaging with a confusing error -- a risk the wider
file set would have multiplied. The lookbehind drops nothing real: detection
over the shipped bundle is identical with and without it.
* test(packaging): cover the missing packaged main entry assertion
The required-file check had no test, so the refactor that split it out of
the scanning loop could have dropped it silently. Removing the assertion
now fails this case.
* docs(packaging): name the embedded-source-string limit of the main scan
ssh-relay-deploy builds a probe script for the REMOTE host as a string, and
its require("node-pty") / require("@parcel/watcher") survive into
out/main/index.js, where this scan counts them as desktop-main imports. Both
are packaged, so it is benign today, but a remote-only dependency added to
that script would fail desktop packaging with a false message -- and the two
obvious fixes (ship the remote dep, or weaken the guard) are both wrong.
Separating an embedded string from real code needs a parser.
* test(packaging): pin the exact import shape oxc emits for the SDK
The fixture only carried the spaced `import (` variant, so nothing pinned
the form a shipped build actually contains. Use the real emitted shape --
`p??=import(`@anthropic-ai/claude-agent-sdk`)`, no space, backticks, and the
`??=` that precedes it -- and keep the spaced variant on the second entry so
both stay covered.
* fix(packaging): keep the main scan able to see a spread require
The `(?<![.\w])` lookbehind also rejected `[...require("pkg")]`, because the
third dot of a spread satisfies it. That trade is not symmetric: excluding a
member call costs a loud release-build failure if it ever misfires, but
excluding a real specifier is this guard going blind -- the failure mode the
whole verifier exists to prevent. Readmit a dot that ends a spread.
Zero occurrences in the shipped bundle today, so this was latent. The chunk
test's asar mock now also emits directory nodes, because real listPackage does
and extractFile throws on them -- that makes the `.js` anchor's load-bearing
role something the tests can actually catch.
---------
Co-authored-by: Merge Sim <sim@local>
|
||
|
|
6494f2a4f0 |
fix(native-chat): resume a structured chat from Agent Session History (#18933)
* fix(native-chat): resume a structured chat from Agent Session History Clicking Resume on a chat-UI row could only reveal an already-open tab. If the chat had been closed, or this process had never published it, the click re-read an inventory that did not contain it and toasted "Retry in a moment" — advice that could never come true, because nothing republishes an unpublished tab. The legacy `claude --resume` fallback is deliberately refused for structured-owned rows, so the row had no way back at all. `close` already keeps the record and the journal on disk so a session can be attached again, and the hold path already resurrects one in full. What was missing was the tab: `restoreReadableSessions` is latched to run once, at startup, so nothing could ask for a single session later. Adds `agentSession.reveal`. The host looks up its own record, restores the session readable, and republishes the tab through the same call `agentSession.create` uses. Deliberately narrow: - It takes no hold. A provider child exists because a surface asked, and the chat pane asks when it binds. - A journal it cannot read is not a refusal. A chat whose journal predates the SQLite store restores to nothing here, but attach still recovers it, so the tab is published and the pane's hold finishes the job. - Workspace and provider come from the record, never the client, so a session id alone cannot aim the publication at another workspace. Claude and Codex both, by construction: eligibility is `adapterSupportsRecord`, which the router answers from the record's own provider. Gated on a new advertised capability rather than probing for method_not_found, matching agent-session.structured.hold.v1 — absence is visible during negotiation instead of by calling. * fix(native-chat): negotiate reveal against the host that owns the workspace The capability gate read the LOCAL runtime's advertised capabilities while the call went to the host that owns the workspace, which for a paired workspace is a different build. On desktop the renderer and its local host are always the same build, so the gate passed unconditionally and proved nothing about the host being called: an older paired host still received the unknown method and its method_not_found was reported to the user as 'this chat is no longer on this host'. The cache it read also starts empty and resets to empty when status.get fails, so 'not fetched yet' and 'unsupported' were the same value. Gate on the environment that will answer, the way agentSession.close already does, and skip the round trip entirely for a local host. Reveal now reports four outcomes instead of a boolean, so a host that is merely too old is not reported as a chat that is gone, and a host we could not reach keeps the retryable message. Also syncs the localization catalog: the 'gone' key shipped without an en.json entry, which reddens static analysis and verify while typecheck stays green. * fix(native-chat): tell a refused reveal apart from a missing chat The host raises two refusals here and they mean opposite things to a user: it holds no such record, or it holds one no adapter of its own can open. The client collapsed both into 'this chat is no longer on this host', which is a eulogy for a chat still sitting on disk. Read the refusal code, and fold the host-side case in with the too-old host under one honest message, since the remedy for both is the same. Adds the coverage the readiness pass found missing: the host's reveal answer itself (workspace and provider from the record, both refusals, an unreadable journal, a live session), and the activation branches for a host that cannot open the chat and for one that never answered. * fix(native-chat): read a host version block as the host's age, not a lost link The capability probe reaches assertRuntimeStatusCompatible, which throws a runtime_compat_block error. Treating that as unreachable told a user with an out-of-date host to retry, which is the one thing that cannot help. Branch on isRuntimeCompatBlockError the way remote-agent-session-launch already does for the same probe. Also adds the refusal-code case a previous commit claimed and did not deliver: nothing drove a structured_agent_session_unsupported reply through the reveal client, which is the branch that commit existed to add. Corrects a doc comment that reveal made wrong: attach is no longer the only call that builds the host. * fix(native-chat): let a dragged history row reach the same reveal as a click Dropping an Agent Session History row onto a pane activated the tab by id and, on a miss, raised the very toast this PR exists to remove — so the same row answered a click and a drop differently, and the drop kept the advice that can never come true. The structured branch never used the drop pane, so routing it through the shared activation loses nothing and gains the reveal. The helper only ever read one field, so its parameter narrows to that field and the drag payload satisfies it directly. A source ratchet holds both entry points to the reveal-capable path, since a mounted drag harness does not exist for this layer and what regresses is a call site, not a rendering. * fix(native-chat): stop an advisory refresh ending the click, and one click per row Manual QA found the reveal never ran: the inventory refresh that precedes it is an optimization, but its failure returned early with 'not available yet, retry in a moment' — reinstating the dead end this PR removes, one step earlier. A failed refresh now falls through to the reveal, which is the repair and does not need the refresh to have worked. The click can chain a refresh, a capability probe, a reveal and a second refresh, each with its own timeout, while nothing on the row says it is working. A per-session in-flight guard keeps an impatient second click from running the whole sequence again and landing its own toast. Also drops an unreachable owner scope: the snapshot apply discards any worktree whose execution host is not local before it reads one, so naming a remote scope there described a synchronisation that cannot happen. * fix(native-chat): bound the capability probe and stop naming the wrong machine The in-flight guard releases when the activation settles, so an await that never settles holds the row for the life of the process. The capability probe was the one call in the chain not raced against a deadline: on a cache hit it awaits a promise an earlier probe created, which may carry no deadline of its own. Race it like the two calls around it. A version block can name either side — evaluateRuntimeCompat reports client-too-old as well as host-too-old — so a message that blamed the host pointed half of those at the wrong machine. Name the remedy instead of the machine, which is true for every case that reaches it. * chore: remove a scratch repro file committed by mistake It was swept into the previous commit by a broad `git add` while a diagnostic ran in this worktree. It asserts the current renderer-sync defect as expected behaviour, so it would fail the moment that defect is fixed. * fix(native-chat): stop a reveal's own inventory refresh discarding its republished tab Manual QA: the host answered reveal with ok:true and republished the tab, and the chat still did not reopen — only a renderer reload brought it back. The renderer publishes under one epoch string for its whole lifetime, and a frame recorded under a different lineage retires that epoch permanently with nothing to un-retire it. The Resume click asks for an inventory first, and a worktree the host holds no entry for answers with the none/v0 sentinel; the structured path recorded it, retiring the renderer's own epoch, so the tab the reveal published a moment later was dropped. A reload minted a new epoch, which is why reloading appeared to fix it. A frame that carries no publication is not a later publication to fence against. Treat the sentinel and a removal frame as a cursor reset, the way the mainstream session-tabs path already clears its tracking — its comment names this exact hazard: recording that sentinel would retire the host epoch and reject the next live frame. Pre-existing, and it swallows an ordinary new-tab launch on an empty worktree too; the reveal is what turned a silent invisibility into a visible failure. * fix(native-chat): let a retraction prune its rows without retiring the epoch Correcting the previous commit. Skipping a retraction frame outright stopped it pruning the mirrored rows, so a worktree the host no longer publishes would have kept a chat on screen with nothing behind it. Apply the frame as before and clear its cursors instead of recording them, which is what the mainstream session-tabs path does. The unpublished sentinel keeps its cursor now too: it is skipped rather than cleared, so a stale frame arriving late is still fenced. Adds the case the earlier version would have broken. * fix(native-chat): keep the retraction's fences, and fence the reveal's refresh Correcting the retraction handling again. Clearing its cursors was more than the bug needed and cost a guard: the host mints a fresh epoch when it rebuilds a pruned entry, so a republication is never gated by the retained cursor, while dropping it left an inventory response issued before the close free to land afterwards and strand a chat row for a worktree the host no longer publishes. Skip only the recording. The mainstream path keeps its epoch history for the same reason, as a tombstone fence. The test that justified the stronger clearing asserted a host behaviour that does not exist — a rebuilt entry republishing under the renderer's epoch with a restarted counter. It now uses what publishStructuredAgentSessionTab actually mints for a pruned entry, and a new case covers the frame that would strand. Also fences the reveal's inventory refresh on the sync generation, which every other caller that applies an inventory already does: structured chat can be switched off mid-flight, and the answer would otherwise re-seed a row into a renderer that just discarded them. * fix(native-chat): drop the retraction's epoch history, keep its version cursor Third and final shape for this branch, and the only one of the three that holds. Keeping both maps re-poisons the epoch one cycle later: the consumer here is also the publisher, so the history's current is the renderer's own lifetime epoch, and recording the reveal's fresh epoch retires it. The next chat the renderer publishes is then dropped — this bug again, one close later. Deleting both loses the guard that stops a frame issued before the close landing after it and stranding a row nothing republishes. So: clear the history, keep the cursor. The mainstream path keeps its history as a tombstone because there the epochs belong to a remote publisher; that reasoning does not carry to a path that publishes under its own. Each of the three variants now fails a different test. * fix(native-chat): a retraction forgets what is current, not the tombstones The delete lost a fence the cursor cannot replace: the version cursor only compares within a lineage, so a delayed frame from an already-superseded epoch had nothing left to stop it putting a chat row back for a worktree the host no longer publishes. Keeping the record intact had the opposite fault — the renderer's own epoch is the history's current, so the next frame under any other epoch retired it. Clearing only current does neither: noteRetiredValue retires nothing when there is nothing current, and the tombstones stay. Each of the four shapes now fails a different test. * fix(native-chat): narrow the retraction frame through its own type Typecheck caught what the tests could not: `removed` is not on RuntimeMobileSessionTabsResult. The repo already names the shape — RuntimeMobileSessionTabsRemovedResult — so this reads it through a guard rather than the inline cast the mainstream path uses. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Merge Sim <sim@local> |
||
|
OrcaWinandOrca Worker
|
0f27445789 |
fix(build): pin config/relay-assets LF so one release is one relay hash (#19024)
* fix(build): pin config/relay-assets LF so one release is one relay hash * test(build): correct why the negative fixtures exist Review measured it: the first assertion checks the eol attribute via check-attr, not file content, so it fails first without the pin. The fixtures add over-broadness coverage, they do not carry the test. * test(build): key the relay line-ending pin off the manifest, not a directory A path glob proves the directory is non-empty, not that it is still the directory build-relay reads from. Relocating an asset into config/scripts (where only **/*.mjs is pinned) reintroduced the CRLF bug with the suite fully green. RELAY_ARTIFACTS is the right anchor: build-relay refuses to emit an artifact absent from it, so a relocated or new asset cannot slip past. Bundles have no tracked source and drop out with zero hits. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
 Jinjing
|
891ae62df5 |
fix(release): revalidate draft state before patching generated notes (#19019)
* fix(release): revalidate draft state before patching generated notes The release listing is a snapshot taken before generate-notes runs. If the draft is published in that window, the PATCH overwrote a live release body. Re-read the release by id immediately before the update and skip it when the release is no longer a draft. * Handle publication race during draft release notes patch Between the draft status check and the PATCH request, a release can be published. The PATCH succeeds but now modifies published content. Check the PATCH response—if draft=false, publication won; restore the published body and leave generated notes unapplied. * fix(release): only roll back the draft body we actually wrote Re-read the release before the compensating PATCH and skip the rollback when the body no longer matches the notes we patched in, so a body written after our PATCH is not clobbered. |
||
|
Neil
|
bf5f3c2ec4 |
test: cover native X11 Hangul-plus-digit PTY bytes in CI (#19013)
* test: run the native Hangul terminating-digit regression in CI * test: distinguish X11 byte coverage from the manual Wayland repro * test: require native IME engagement proof for the digit case |
||
|
Jinjing
|
bdad20b4c1 |
Support updating existing draft releases when regenerating notes (#19014)
Move release existence check into create-draft-release.mjs. Draft releases are updated via PATCH, published releases are skipped, making the release-cut workflow idempotent. |
||
|
OrcaWinandOrca Worker
|
ec030f1d35 |
fix(windows): sign the NSIS uninstaller via SignPath (#17868)
* fix(windows): sign the NSIS uninstaller via SignPath
`Uninstall Orca.exe` ships NotSigned, and MDE's whole update cluster is
that one file: electron-builder copies it to `old-uninstaller.exe` and
runs it silently during every update.
The cause is narrower than "NSIS generates the uninstaller at install
time". app-builder-lib already builds the uninstaller in its own makensis
pass and calls `packager.signIf(uninstallerPath)` on it before embedding
it (NsisTarget.computeScriptAndSignUninstaller). Orca signs nothing during
electron-builder — SignPath signs afterwards, behind a human approval — so
that hook is a no-op and the file is deleted before CI can reach it.
Use the hook as a relay instead of a signer: the first Windows build
exports the uninstaller, it rides the existing inner-binaries SignPath
request (no third approval wait), and the rebuild-from-signed-tree pass
swaps the signed bytes back in before makensis embeds them.
Every added step is fail-open. A missing export, a SignPath artifact
configuration that does not cover `uninstaller/`, or a relay error costs
only the uninstaller signature — the inner-binary chain and the shipped
installer are unchanged.
* fix(windows): keep the uninstaller relay out of the packed checkout
Review fixes on the uninstaller signing chain.
The export path lived at `${{ github.workspace }}\uninstaller-signing\`.
`files` in the electron-builder config is all-negation, so app-builder
prepends `**/*` and packs whatever is left in the checkout root, and the
build step retries up to three times — attempt 1 wrote the file after
packing, attempts 2 and 3 would have packed an unsigned `.exe` into
app.asar. All seven relay sites move to `runner.temp`, and a contract test
now fails if any of them points back into the checkout.
The uninstaller staging block guarded with `Test-Path` but left `New-Item`
and `Copy-Item` able to throw. That step's outcome gates the upload of
every inner binary, so a locked file there would have cost all of them
their signatures — worse than before the chain existed. It is wrapped in
try/catch, asserted.
Also: test `signWindowsUninstallerViaSignPath` itself (it runs in a step
with no continue-on-error, so its no-throw property is load-bearing) and
the sha1+sha256 double invocation; make the rehearsal verify the
uninstaller the installer actually writes to disk rather than only the
relay receipt, whose digest comparison is equal by construction; correct
the staged-name comment, which asserted a collision that does not
reproduce; count what was reported rather than what was extracted; and
note two traps — a custom sign hook replaces signtool outright, and the
single-env-var relay would race if a second NSIS target or arch is added.
* fix(windows): stop the signing rehearsal failing on its own artefact
The rehearsal is the merge gate for this chain, so it must not be able to
fail on something that is not the thing under test.
It trusted whatever 7-Zip's NSIS handler emitted. That handler produces
partial or garbled output on some NSIS builds, and a truncated extract
would score NotSigned and be reported as "the shipped uninstaller is
unsigned" when nothing was wrong. It now has to reproduce the digest the
sign hook recorded before its output is trusted; otherwise it falls
through to the silent-install route, which is ground truth. A name miss
falls through the same way.
The install route only checked the signature. Comparing the on-disk file
against the receipt is what actually proves the shipped installer embedded
the SignPath-signed bytes — the release job's own comparison is equal by
construction, so this is the only place the claim is really tested.
Also: bound the silent install (a bare `-Wait` on an installer that ever
prompts hangs to the 360-minute job cap) and poll before stopping Orca,
since the oneClick installer launches the app as it finishes and the
process can appear after the installer has already exited.
Two smaller ones: `-ErrorAction Stop` on the staging New-Item/Copy-Item so
the catch above them does not depend on GitHub's $ErrorActionPreference
default; and the relay-path test now counts every occurrence rather than
the first, so a step carrying two paths cannot root one in RUNNER_TEMP and
leave the other bare-relative — the exact shape of the bug it guards.
* test(windows): stop a pre-existing elevate.exe defect masking the gate
The first real rehearsal (run 33484703381) proved the uninstaller relay
works end to end — the 7-Zip route read the embedded uninstaller, the
digest guard did not trip, SignPath accepted the new uninstaller/ zip
entry, and the shipped `Uninstall Orca.exe` came back signed.
It also failed, on `resources\elevate.exe`, for a reason that predates
this PR. app-builder-lib re-copies the pristine cached elevate.exe over
`resources\elevate.exe` on every nsis pack — `AppPackageHelper.packArch`
calls `elevateHelper.copy()` before `buildAppPackage`, and
`CopyElevateHelper.copy` does `copyFile(elevatePath, outFile, false)` then
`signIf(outFile)`, which signs nothing because this build configures no
certificate. The signed copy restored into win-unpacked is clobbered by
the rebuild.
That is not the sign hook displacing a signtool call: with no `sign` hook,
`signFile` already returned false at "no signing info identified", so
nothing was signing elevate.exe before either. release-cut.yml mitigates
it separately by pre-seeding the electron-builder cache; this workflow has
no such step, which is why the clobber is visible here and not there.
Downgrade elevate.exe alone to advisory so it cannot mask the uninstaller
result, and record it in the evidence artifact so downgrading stays
distinguishable from deleting the check. Both uninstaller verdicts stay
fatal, pinned by a contract test that also holds the escape hatch to
exactly one file. The underlying defect gets its own PR — it is a UAC
elevation helper and deserves more scrutiny than a footnote here.
* docs(windows): warn against relaxing the elevate.exe cache guard
The tempting edit, for anyone who finds the rehearsal red on
resources\elevate.exe, is to relax release-cut's `Valid` +
`CN=SignPath Foundation` guard so the cache swap runs under test-signing
and the rehearsal goes green.
That guard is the only thing stopping a test certificate from being seeded
into a cache a real release restores from — both workflows share the key
`electron-builder-win-<lockfile hash>`. Shipping users a binary signed by
"Test certificate for 'Orca agent ide [OSS]'" is worse than shipping it
unsigned, so say so at the place someone would make that edit.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
OrcaWinandOrca Worker
|
8415d53a05 |
fix(release): stop shipping an unsigned elevate.exe on Windows (#18044)
* fix(release): stop shipping an unsigned elevate.exe on Windows The release cut swaps the SignPath-signed elevate.exe into the electron-builder toolset cache so the NSIS rebuild's CopyElevateHelper re-copy becomes a no-op. It searched `<cache>\nsis`, a directory no app-builder-lib layout creates, and `-ErrorAction SilentlyContinue` plus `exit 0` turned that miss into a green step — v1.4.193 and v1.4.194 shipped an unsigned UAC elevation helper. Move the lookup into a script that covers the real layouts (`nsis-3.0.4.1/…`, `nsis@<toolset>/…`, `ELECTRON_BUILDER_NSIS_DIR`), asks app-builder-lib for the authoritative path, and exits non-zero with an ::error:: annotation when it finds nothing. The step stays continue-on-error so the inner-signing chain remains fail-open. * fix(release): make the elevate.exe swap prove it replaced the packed copy Success was "some cached copy was replaced", which a stale release directory carried in by the `electron-builder-win-` prefix restore can satisfy on its own while the bundle the rebuild packs stays unsigned. The app-builder-lib probe returns the exact path CopyElevateHelper will pack, so make that the check and the directory scan the fallback: exit non-zero when the probed copy was not replaced, and annotate a warning when the probe could not run at all, so a green step never quietly means the authoritative check was skipped. Also pin both shebang scripts to LF: `core.autocrlf=true` gives a Windows checkout CRLF, and CRLF plus a shebang breaks vite's transform, so resolve-7za-path.test.mjs currently runs zero tests there. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
OrcaWinandOrca Worker
|
c252d855ac |
fix(windows): resolve npm/pnpm .cmd shims past cmd.exe (#17869)
* fix(windows): resolve npm/pnpm .cmd shims past cmd.exe
A `.cmd` target forces every spawn through `cmd.exe /c` with each argument
caret-escaped, and Microsoft Defender for Endpoint scores a long `cmd.exe /c`
line carrying caret-escaped natural language as obfuscation. `codex.cmd` is
named in the spawn cluster of the MDE incident this addresses.
npm's `cmd-shim` and pnpm's `@zkochan/cmd-shim` generate files whose whole body
is "find node, run this script". Read one, and the spawn can go straight to
`node.exe <script> <args>` — no cmd.exe, no caret escaping. Anything the parser
does not recognise exactly, or whose target cannot be confirmed on disk, keeps
the existing cmd.exe path.
Incidentally fixes a real bug: cmd ends its command at a raw CR/LF whatever the
quote state, so a multi-line agent prompt through a `.cmd` shim had to be
rejected. Resolved shims have no such limit.
* fix(windows): refuse drive-relative shim paths and run the win32 tests in CI
Two blocking findings from review.
A drive-relative path defeated the absolute-path guard:
`win32.isAbsolute('D:evil.js')` is false, but `win32.resolve` reads the drive
letter and lands on `D:\evil.js`, outside the shim directory. cmd would have
built `C:\shim\D:evil.js` and failed; we would have executed the wrong file.
Adding `:` to the unsafe-character set closes it, and the alternate-data-stream
spelling `a.js:zone` with it. It costs no coverage: 84 of the 91 real shims on
this box still resolve, the same seven fall back.
Neither `windows-cmd-shim-resolution.test.ts` nor its `.win32` sibling was in
the Windows package job's file list, so the whole filesystem/resolution half and
the real-spawn equivalence suite ran nowhere. Both are now in
`WINDOWS_PACKAGE_TESTS` and in the pr.yml step.
Also from review: clear `windowsVerbatimArguments` explicitly on the resolved
branch rather than inheriting it, since there is no caller-built command line
there; document the kill switch and the PTY/hook-wrapper scope limits in
docs/reference; and cover drive-relative, BOM, line-ending, casing and `%*`
tampering in the platform-independent half of the tests.
* docs(windows): justify the shim-path colon guard from the filesystem rule
The guard was argued empirically ("none of the 91 shims on this box has one"),
which invites a future reader to relax it for a shim we have not seen. Windows
reserves `:` within a path segment, so a relative path cannot carry one at all:
the only spellings that can are drive-qualified, an alternate data stream, or a
`\?\` device path, and the last is already refused as absolute. That makes a
false refusal impossible rather than unobserved.
* refactor(child-process): move resolveSpawn into its own module
The merge with main pushed run-process.ts one line past the 300-line cap:
both sides grew it. The spawn-argv decision is already a pure, separately
tested unit, so it moves out rather than the cap moving up. run-process.ts
re-exports it, so no caller changes.
* perf(child-process): cache the shim interpreter lookup
The parse cache spared the shim read but not the PATH walk, so a second
resolution of the same .cmd did 0 reads and one statSync per PATH entry --
30 on a 30-entry PATH, synchronous on resolveSpawn, where one dead network
mount blocks the calling thread on every spawn.
Keyed by shim directory AND PATH, since the shim's own rule is
%~dp0\node.exe first then PATH, and a PATH edit between spawns must miss.
Corrects the stat comment, which accounted only for the shim itself.
* fix(child-process): revalidate a cached shim interpreter before using it
The node cache was held for process life and never rechecked, so a cached
node.exe that was later uninstalled -- or dropped from PATH by a version
manager -- was still handed to resolveSpawn, failing the spawn with ENOENT.
An uncached process in the same state returns null and falls back to
cmd.exe successfully, so the cache was strictly worse than no cache.
One statSync on a non-null hit, not one per PATH entry, so the walk this
cache exists to skip is still skipped. The stale-null direction stays
uncorrected on purpose: it only keeps the working cmd.exe fallback. Both
directions are now stated in the comment, along with the known miss for
callers that vary PATH per spawn.
* fix(child-process): honour PATHEXT when resolving the shim interpreter
The doc claimed a node.com/.bat/.cmd on PATH returned null and fell back to
cmd.exe. The scan actually skipped those entries and kept looking for a
node.exe, so PATH=C:\A;C:\B with C:\A\node.com and C:\B\node.exe resolved to
B's node.exe while the shim runs A's node.com -- a different binary, chosen
silently, on the one axis this module must not get wrong.
The scan now follows cmd's rule: first PATH directory holding any PATHEXT
spelling wins, PATHEXT order decides within it, and only an .exe winner is
returned. Anything else gives up and keeps the cmd.exe path, which restores
the strict-subset-of-cmd property everywhere except the documented cwd case.
PATHEXT is read from the child's env and joined into the cache key, since it
now changes the answer. Costs one stat per PATHEXT entry per node-less
directory, paid once per process behind the cache.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
Neil
|
ba86e1c83c |
perf: avoid repeated whitespace scans in diagnostic redaction (#18908)
* perf: scan diagnostic environment lines without repeated whitespace searches * perf(observability): skip redundant terminator scans in environment-line redaction After a successful match ENV_LINE.lastIndex already sits at end of input or a line terminator, and a failed start's whitespace skip plus terminator scan collapse into one LINE_CONTENT exec. Byte-identical output; verified against the pre-change regex over 280k generated inputs. * perf(observability): find environment-line ends without entering the regex engine |
||
|
|
0cbb01ef4b |
fix(security): apply the Windows path-hardening ACL that never ran (#17884)
* fix(security): apply the Windows path-hardening ACL that never ran `buildWindowsRestrictAclArgs` invoked the hardening script as `powershell.exe -Command <script> <path> <sid> <isDir>`. `-Command` does not populate `$args`; it appends the trailing tokens to the command text. The script therefore read `$args[1]` as `$null`, threw `NullArrayIndex` at `$allowedSids[$sidText] = $true` under `$ErrorActionPreference = 'Stop'`, and exited 1. Both callers swallowed that: the async callback was empty and `applySecurePathRestriction` returned `true` regardless, while the sync `catch` returned `false` and nobody logged. Every Windows secure path has been left on its inherited ACL since the ACL was introduced (#5006), and nothing said so. Replace PowerShell with `icacls.exe`, which takes plain argv. That removes the quoting surface entirely rather than escaping it: interpolating a path into the command text would have turned a dead no-op into arbitrary PowerShell on a filesystem path, since `-Command` executes what it appends. It also drops the execution-policy dependency and the `powershell.exe` spawn an EDR flags, and runs ~25x faster than the PowerShell cold start. Hardening is now three passes: `/reset` to purge explicit ACEs that `/inheritance:r` leaves behind, `/inheritance:r` plus a `/grant:r` per allowed SID, then a read-back that checks the DACL is protected and grants only the intended rights. The predecessor's verification block was equally dead, and an apply that is never read back is only half a control. Failures stay non-fatal — non-NTFS volumes, network paths and restricted tokens fail legitimately and must not break startup — but they are no longer invisible: every failure is logged, and a failed async apply now evicts its cache entry so the next call retries instead of trusting a success that never happened. Routing through `runProcess`/`runProcessSync` also retires this file's `node:child_process` allowlist entry. * fix(security): verify the hardened ACL by identity, not by shape Review found the bug class this PR fixes surviving inside the fix. The verify pass checked rule count, absence of the inherited marker, and exact rights — never *who* the rules named. Granting Everyone full control satisfies all three, so hardening reported success on a DACL that handed the credential to every local account, and most of the real-filesystem tests still passed. Verification now reads the descriptor back with `icacls /save`, which emits SDDL with raw SIDs, and compares the principal set exactly. That is also locale-independent by construction: the previous parse read localized account names out of icacls' OEM-codepage stdout, where a non-ASCII path survived by accident rather than by the documented mechanism. SDDL parsing moves to `windows-security-descriptor.ts`. Two further self-inflicted problems, both measured: The post-rename re-harden led with `/reset`, which re-widened a DACL that was already correct — the staged file's protected DACL survives the rename, so the pass had nothing to do but open a window. Polling an external process during a write into a relocated root caught it: the e2ee keypair dropped to `BUILTIN\Users:(RX)` plus `Authenticated Users:(M)` — read *and* write — before tightening again. Hardening now verifies first and returns early when the DACL already reads back correct, which closes the window and cuts the steady state from three spawns to one. Re-measured: 158 samples, one DACL state, zero broad. Evicting the cache on every failed async apply reintroduced #4901. The env store re-hardens on the read path at ~2/s, so on a host where hardening cannot work (FAT32, network path, restricted token) that was two icacls spawns and two warnings a second, forever. Async retries now take a retry floor and a hard per-path attempt cap. The write path keeps retrying unthrottled — it is user-driven, and a failed credential ACL must still be retried on the next write. Also: failures route through a reporter hook that the main process points at the diagnostic tracer, because `console.warn` reaches nothing in a packaged GUI-subsystem build; `writeSecureFile` returns whether hardening took, and the async branch reports `pending` rather than claiming `applied`; a transient `whoami` failure no longer disables hardening for the process lifetime, and the SID is shape-validated; the `/c` guard now covers the synchronous runner too. * fix(security): re-probe hardening instead of latching a transient failure The per-process attempt cap added for the read-path storm was a permanent latch: one AV scan, momentary lock or %TEMP% blip and every later credential write in that session went unhardened, silently, on a host where hardening would now succeed. Same defect class as #17858's computer-use host, and worse here because what stops happening is security hardening on credential files and nothing said so. The retry budget now bounds the *rate*, not the lifetime: at most three attempts per path per minute, re-probing in every later window, forever. The transition is announced in both directions — `throttled` once per window on entry, `recovered` when a rate-limited path hardens again — so a host stuck in the degraded state is diagnosable rather than merely quiet. The reporter type covers both, and the main process ends the `recovered` span successfully rather than failing it. Extracted to secure-path-hardening-retry-budget.ts, which keeps secure-file.ts under its line cap without a max-lines disable. Also confirms the second flagged risk rather than assuming it: a real unwritable %TEMP% is now covered by a test proving verification fails closed, reports at the `verify` stage, and still leaves the ACL applied — so that path loses proof, not protection, and with the lifetime cap gone it can no longer combine into a permanent-off state. * fix(security): verify a directory's whole inheritance flag set The flag check tested only that `OI` was present — never that `CI` was, nor that nothing else was. That was harmless while `/reset` + `/grant` ran on every pass and repaired whatever was there. The verify-first short-circuit made it load-bearing: what verification accepts is now left alone, so a latent under-check went live because a different fix started depending on it. Two directory DACLs passed while being wrong — both protected, three non-inherited full-control rules, correct SIDs, differing from correct only in their flags: (OI)(F) - no CI, so subdirectories are left unprotected (OI)(CI)(IO) - inherit-only, so the directory object itself grants nobody anything; the next writeFileSync into it fails with EPERM, on a directory just cached as hardened Verification now compares the whole flag set, which also rejects IO and NP, and names the offending flags in the failure. Both shapes are planted in real-filesystem regression tests, including an assertion that a write into the repaired directory succeeds and its child inherits. Confirmed both tests fail against the old check and pass against this one. * fix(security): back the hardening retry off exponentially The fixed one-minute window bounded the retry rate but left a standing floor of three attempts per path per minute on a host where hardening can never succeed — FAT32/exFAT, a network path, a redirected profile. That budget is per path and there are several secure files, so the floor multiplied into tens of thousands of icacls spawns a day for work guaranteed to fail. The delay now doubles after each consecutive failure, from a one-minute floor to a thirty-minute ceiling, and the attempt cap is gone entirely: once the backoff elapses the path is re-probed however long it has been failing. A permanently incapable host settles at ~2 attempts/hour. Slowing the backstop costs almost nothing, because it is not the recovery mechanism: the synchronous write path is deliberately unthrottled, so a host that recovers hardens on its very next credential write regardless of what the read-path budget says. The `throttled`/`recovered` reports are unchanged and matter more here, since the quiet periods between probes are now much longer. The curve is pinned in a new unit test against the exported delay function rather than a copy of its constants, covering the doubling, the ceiling holding at 5000 consecutive failures, a 30-day failing path still re-probing, one announcement per degraded episode, and per-path isolation. The integration tests keep only what they uniquely prove: that the read path is wired to the budget, and that a day of failures still re-probes. Confirmed four of these fail against a reinstated lifetime cap. * ci(windows): run the real-icacls DACL suite in CI The win32 suite only self-skips off Windows, so it passed vacuously in every lane. Register it the way the cmd-shim suite is registered. * fix(security): describe the cache's real cost, which is icacls now Both cache comments still justified themselves with PowerShell -- "~1-1.5s" and "a PowerShell spawn every read" -- in the same file whose PR removed PowerShell from this path. The caches are still right, but for different numbers, and the old ones are the kind an engineer would reasonably delete a cache over. The real shape: hardening verifies first and returns early, so an already-correct DACL costs one synchronous icacls spawn and a rewrite costs four (verify, reset, grant, verify). Still worth caching on the read path, which polls at ~2/s. * test(security): make the DACL suite safe to schedule Registering this spec in the Windows lane put it under two rules it had never been measured against. Teardown now goes through `removeTreeSync`, which the lane's boundary test requires, and repairs the DACLs the suite plants on purpose first: those retries only cover transient locks, so a regressed `(OI)(CI)(IO)` repair leaves the root un-removable and `afterAll` throws EPERM. And the no-permission case decides by elevation before it writes anything. `windows-2022` runs elevated, where hardening succeeds: the old branch asserted nothing about denial and instead replaced the `hosts` DACL, then `icacls /reset` -- which is not a restore, it drops the explicit `SYSTEM:(F)` that file ships with. Ephemeral in CI; permanent for a developer running the lane from an elevated shell. Now it asserts or it skips. The probe reads the token integrity SID rather than `icacls /save`, which succeeds unelevated (`BUILTIN\Users:(RX)` carries READ_CONTROL) and would have skipped the case on every machine. * fix(security): measure the hardening latches on a clock that cannot go backwards `mayAttemptHardening` compared wall-clock times, so any backwards step -- an NTP correction, a VM snapshot restore, a user changing the clock -- made the elapsed time negative and held every failing path below its delay until the clock caught up. Measured at the 30-minute ceiling with the clock stepped back a year, the path was refused at +0d, +1d, +30d, +180d and +364d, and re-probed only at +366d. That is the permanent latch the exponential backoff was added to remove, and it contradicts the module's own "bounds the rate without ever bounding the lifetime". The SID lookup's own one-minute window had the identical shape and is worse: a failed lookup makes `planFor` return null, which disables the synchronous *write* path too, so the write-path exemption that recovers the read-path budget cannot recover it. Both now measure elapsed monotonic time, following the repo's existing `monotonicNowMs` spelling. Two things the write path was not doing, both found in the same pass: - A successful synchronous apply now records the outcome. It is exempt from the budget, but it was also invisible to it, so a host that had demonstrably recovered kept the read path backing off for up to 30 minutes and no `recovered` transition ever came from that lane. Only success is recorded; recording failure would put the exempt lane back under the budget. - `writeSecureFile`'s JSDoc now says its boolean covers the file only. The directory harden is fire-and-forget and answers `pending` on Windows regardless, so a `true` says nothing about the directory's ACL. * fix(security): stop the hardening test doubles from faking a no-op Three CI failures on this branch, one failure shape: hardening silently does nothing and the check that should have caught it agrees. The auth critical-path test hand-rolled a `node:child_process` factory with `execFileSync`/`execFile`. The rewritten ACL path goes through `runProcessSync`, i.e. `spawnSync`, which the factory never returned — so every spawn threw into the SID lookup's bare catch, `planFor` returned null, and hardening no-opped. It mocks `child-process/run-process` now, the boundary production code actually calls and the one sibling ACL tests already mock: an export missing there fails loudly by name instead of returning undefined. Its fake icacls writes a real UTF-16LE SDDL file, so the pinned spawn count per write is a property of the ACL path rather than of the double. The test forces `platform='win32'`, so this failed on every platform, Linux CI included. `windowsSystem32Binary` is a production bug, not a test bug: it builds a Windows path with the host `join`, which off-platform yields the mixed `C:\Windows/System32/whoami.exe`. On Windows the two joins agree, which is why it survived; on Linux the SID lookup's whoami match missed and 27 of secure-file's 32 tests exercised a lane that never ran. These are always Windows paths, so `path.win32.join` is what it should have been. The import-boundary pin still read 160 after this branch migrated secure-path-windows-acl.ts off `node:child_process`; the ratchet correctly refuses a pin left above reality. * fix(security): resolve the machine-relative SDDL alias, and stop a denied read destroying the file Path hardening verified the DACL it wrote by comparing the SIDs `icacls /save` reports. SDDL substitutes two-letter aliases for well-known SIDs, and the resolution table could only hold constants -- but `LA` and `LG` name an account by RID inside the *machine's own* SID, so on a box whose user is the built-in Administrator (a CI runner, an Administrator-only install) the current user read back as `LA`, matched nothing, and hardening reported failure for every path. Resolve those two against the machine authority derived from the user SID; without one they stay unresolved and the comparison still fails closed. Three secret stores treated any read failure as "malformed -- regenerate" and overwrote. A hardened file granting a SID this process does not hold reads as EPERM while its directory stays writable, so the overwrite succeeds: renaming over an unreadable file needs FILE_DELETE_CHILD on the parent, not DELETE on the file. That destroyed the E2EE secret key, every paired device's bearer token, and the plugin vault. Distinguish EPERM/EACCES from a parse failure and refuse. Also close the async lane's unhandled rejection: `void p.then(onSettled)` turned a throw from `onSettled` into a dead main process, and the retry budget it calls threw whenever nothing had configured it -- a contract held only by import order. The budget now defaults its own bounds. * test(windows): say which ACEs icacls listed when a planted DACL fails `toHaveLength` reports only a count and vitest elides the array, so three preconditions failing on the CI runner said "expected 3, got 6" and nothing about what the sixth entry was. Name the entries in the failure. * fix(security): stop three more stores overwriting what they were denied Same swallow-default-overwrite shape as the readers already fixed, found by sweeping every store that reads under a hardened root. - plugin-storage-store.ts returned `{}` on any read failure and set()/delete() wrote it back, losing the plugin KV store. It is the secrets store's shape line for line, so the two now behave identically. - relay-revoke-outbox.ts returned [] and save() wrote it, dropping revocations that never reached the relay -- a revoked device stays live. - profile-cloud-session-store.ts mapped an EPERM read onto `decrypt-failed`, which fails the `status === 'found'` guard in clearCloudSessionIfUnchanged and falls through to an rmSync of the account session. A denied read now reports `unreadable`, which licenses nothing; the refresh path bails on it and the auth status surfaces it rather than reporting a bare reconnect. All reuse isPermissionDeniedError. The predicate stays an EPERM/EACCES allow list rather than "ENOENT defaults, everything else throws": these stores are meant to self-heal a truncated or malformed file, and inverting it would turn a corrupt keypair into an app that cannot start. The distinction that matters is "could not read it" versus "read it and it was garbage". * test(windows): plant fixture DACLs that cannot inherit what they did not plant %TEMP% grants [SYSTEM, Administrators, <user>] (OI)(CI)(F) by default, and those propagate into every fixture. Three preconditions read back 4 and 6 ACEs where 3 were planted, and the extras looked like Orca's own hardening because the shape is identical -- on a runner whose user is the built-in Administrator, the inherited trio IS the trio production grants. Combining /inheritance:r with /grant:r leaves the argument order to icacls, and that combined form drops the inherited ACEs on Windows 11 but keeps them as explicit ones on the Windows Server runner. Removing inheritance in its own invocation makes the grant the whole DACL on either host, and the fixture root is de-inherited once up front so nothing propagates in. Rooting the fixtures outside %TEMP% would not have fixed this: any directory inherits from wherever it lives. The fix is to stop inheriting, not to move. No assertion is relaxed -- the counts stay exact. * test(windows): pick a foreign SID that stays foreign on an elevated runner `S-1-5-32-544` is only foreign to a token that is not an administrator. The CI runner is elevated AND logged in as the built-in Administrator, so granting Administrators granted the reader full control: the file stayed readable, and all six preservation assertions went vacuous rather than proving anything. BUILTIN\Guests is resolvable everywhere and no interactive token is a member, so the read is denied on an unelevated developer box and on the runner alike. An unresolvable SID would have been the stronger choice but icacls rejects one with ERROR_NONE_MAPPED (1332). The premise guard is what caught this -- it asserted the file was actually unreadable instead of trusting the grant, and named elevation as the suspect. * fix(security): refuse on any read that never reached the contents, not just a denied one isPermissionDeniedError becomes isUnreadableError, because "permission denied" was never the concept -- "could not read it", as opposed to "read it and it was garbage", is. EBUSY, EMFILE, ENFILE and EIO say exactly as little about a file's contents as EACCES does, and they fell into the branch that regenerates and overwrites. On Windows EBUSY is the likelier of the two: antivirus holding a credential open at the moment of a startup read produces it, which makes it a commoner path to the same permanent loss than the ACL case that motivated the original fix. Still an allow list, deliberately: ENOENT keeps licensing a create, and a parse failure keeps self-healing. The stores are built to recover from a truncated write, and turning that into a refusal would trade a recoverable state for an unrecoverable one on the startup path. Also fixes the regression suite's own premise on an elevated runner: makeUnreadable combined /inheritance:r with /grant:r, and that form keeps %TEMP%'s inherited [SYSTEM, Administrators, user] as explicit ACEs on Windows Server -- so the file stayed readable and all six assertions were vacuous. Same split-the-invocation fix as the ACL suite's planter. * test(windows): skip the preservation suite where a read cannot be denied An elevated token logged in as the built-in Administrator reads straight through a DACL that grants it nothing -- confirmed on the CI runner against both BUILTIN\Administrators and BUILTIN\Guests, and with the grant split into its own icacls invocation so the DACL really was the planted one. On such a host the premise these tests rest on does not hold, and every assertion would pass while proving nothing. So probe once at module scope and skip rather than assert vacuously -- the same trade the ACL suite already makes for its unelevated-only case. The gate stays in the compound `<win32 check> && <flag>` form the win32 lane ratchet detects, so the file stays registered in both lane lists. Coverage is not lost where it counts: isUnreadableError has unit tests that run on every platform and every host, and the stores' refusal is exercised in full on any machine where a denial is reproducible -- which is every developer box. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> |
||
|
OrcaWinandOrca Worker
|
975bbdedcc |
fix(windows): scan ports natively instead of encoded PowerShell (#17861)
* fix(windows): scan ports natively instead of encoded PowerShell Microsoft Defender for Endpoint scored the relay's Windows port scan as suspicious PowerShell plus network discovery (T1049). The command line was `-ExecutionPolicy Bypass -EncodedCommand <base64>` around a Get-NetTCPConnection/Get-Process join -- base64 next to a policy override is the highest-weighted token pair on a PowerShell command line, and netstat only ever ran as its fallback. Invert the chain. `netstat.exe -ano` is now the primary reader and the owning process name comes from the shared native process table, which exists to keep PID lookups off PowerShell. The payload survives only as a last resort, and without the override: execution policy gates script files, never `-Command`, so nothing needed it (verified: `-ExecutionPolicy Restricted -Command` runs). Drop `-p tcp` while inverting: on Windows that protocol name means IPv4 only, so as a primary reader it would have hidden every `[::]` listener the payload used to report. Names arrive as `sshd.exe` from the table and are published as `sshd`, keeping the sshd filter and old clients' rendering intact. Routes both spawns through runProcess, removing the file from the child_process and windowsHide ratchets. * fix(windows): read netstat state by shape and refuse a truncated table Review of the port-scan inversion found two ways the new primary path could be silently wrong, both of which would have kept the flagged PowerShell payload running on exactly the hosts this change targets. `LISTENING` is not in netstat.exe. It lives in System32\<locale>\netstat.exe.mui and MUI selection follows the UI language, so the pinned-locale env in relay-command-env.ts cannot reach it -- a German host prints `ABHOEREN` and the word test parsed zero rows. The zero-listeners guard then read that as a blocked reader and ran `Get-NetTCPConnection` every 12-30s forever, or returned nothing at all where PowerShell is also restricted. Keep the word as the fast path and, when it finds nothing over output that did contain TCP rows, re-read by shape: only a listening socket has no peer. Measured on this host across all four states present (LISTENING 47, ESTABLISHED 49, CLOSE_WAIT 29, TIME_WAIT 213): zero non-listening rows with a zero peer, zero listening rows without one, and the same 47 rows parse after substituting the German state words. Shape stays the fallback because `BOUND` also prints a zero peer. Truncation was invisible: createOutputSink discards overflow, ProcessResult carries no flag, so a capped read still exits 0 and its head still parses. netstat orders IPv4 TCP, then IPv6 TCP, then UDP, so a host with tens of thousands of TIME_WAIT rows would have lost every `[::]` listener -- the exact loss dropping `-p tcp` exists to prevent, and one the zero-listeners guard cannot see. Refuse the read instead. A `truncated` flag on the shared sink would be cleaner and is left as a follow-up rather than widened into this PR. Also: decline to wait on the shared process table once the request is aborted (it takes no signal and must not be cancelled for other callers); note the name lookup as best-effort, since a TTL-cached snapshot can hand a recycled PID its previous owner name; log once on either fall-through, because both are permanent and invisible when wrong; and drop a stderr assertion that any PowerShell autoload banner would redden. Correcting the cost claim in the previous commit: the aggregate win holds with the native addon (netstat 21ms vs the retired payload 860ms at 532 processes), not without it. The addon is optional, the snapshot TTL is 500ms and the scan cadence is 12-30s, so a relay with no active agent pane never warms its own cache and pays ~1.4s cold on the CIM path -- slower than what it replaced. * fix(windows): log the port-scan fall-through on the relay diagnostic stream Checked where this code actually runs before trusting the log. `console.warn` did reach a file, but relayLogLine is the right call and the reasoning is worth recording. `scanWindowsListeningPorts` runs only in the detached relay daemon: relay.ts returns early for --connect and --orca-cli, so PortScanHandler is reached only through runRelayDaemon, and both launchers start it detached with a log file (POSIX `> relay.log 2>&1`, Windows `1>relay.log 2>relay.err.log` via Win32_Process.Create). installRelayLogRotation then wraps both streams into relay.log, which is the file the documented diagnostics tail reads. Verified by installing the real rotation over a temp path and reading the file back. So the line surfaced -- but untimestamped, in a log whose format exists so reconnect flaps can be correlated with the events around them (#7773). relayLogLine is that format and the relay idiom in 41 other places, and "since when has this host been stuck on PowerShell" is most of what this line is for. The test spies on process.stderr to pin the stream and the ISO stamp rather than just asserting something was called, since a fall-through logged somewhere unread is the failure being guarded against. Also fixes a comment that ended its own block early: `relay-*/relay.log` in a doc comment contains `*/`. * fix(windows): keep the dominant zero-peer state when reading a localized netstat Shape alone promoted any zero-peer TCP row, not just listeners. `BOUND` and `CLOSED` print a zero peer too, and on a localized host their state words are exactly as unreadable as the listening one -- so a German host with listeners plus one BOUND socket published a phantom listener. Reachable on an English host too: with zero listeners a lone BOUND row is promoted AND, because the result is then non-empty, it suppresses the blocked-reader fall-through. Group the zero-peer rows by state word and keep only the largest group. A transient BOUND or CLOSED socket cannot outnumber the listeners (51 against 0 on this host), so this removes the class rather than special-casing the words, which would just be the localization bug again. An exact tie keeps every tied group rather than guessing -- no worse than reading shape alone. Verified against real netstat output: injecting a BOUND row into the localized capture leaves the result identical to the English answer (47 rows, no phantom 65001). The new test has teeth -- reverting the grouping fails it and nothing else. Corrects two claims that were slightly wrong: the docblock said shape was the fallback because BOUND prints a zero peer, which described the hazard without saying it was unhandled; and a test comment said an English host "never sees a bound socket", true only when it has at least one readable LISTENING row. Also gates the fall-through log per reason instead of per module, so a host that parses nothing today and truncates tomorrow reports both faults. Same one-shot cost, and the vocabulary is two fixed strings so the set cannot grow. That guard matters more than it looks: --log-file rotates stdout only, so the file stderr can land in is unrotated. * docs(windows): note the direction the zero-peer majority rule can fail in The docblock described the tie case and stopped there, which reads as a complete account of the limits when it is not: a majority rule inverts if the majority is wrong, and enough transient zero-peer sockets would publish the phantoms and drop the real listeners. Someone would reasonably have concluded the rule was safe in both directions. Trigger numbers and the repro stay in the PR discussion; the code only needs the reader to know the rule has a direction, and the hatch (defer to the PowerShell reader, which reads the state word instead of inferring it) since that is the part a future editor would otherwise re-derive. * ci(windows): run the real-netstat port scan suite in CI The win32 suite only self-skips off Windows, so it passed vacuously in every lane. Register it the way the cmd-shim suite is registered. * test(windows): lower both child-process ratchets to the ground this PR took Migrating the port scan off `node:child_process` onto `runProcess` drops `src/relay/windows-port-scan.ts` from both allowlists, so both offender counts fall by one. Each ratchet pins the count from below as well as above, so a pin left above reality fails and re-opens room for the next direct import to land for free. * docs(windows): qualify the no-PowerShell claim on the netstat scan The scan starts no PowerShell of its own, but no released relay carries the optional `windows-process-tree.node` addon (only dev-channel-win-build.yml builds it), so the shared process-table read falls back to a CIM scan that forks one `powershell.exe`. The EDR win is the removal of the `-EncodedCommand` / `-ExecutionPolicy Bypass` shape, not the elimination of PowerShell. Comment-only. * docs(windows): record the identity-reader follow-up and the perf table's addon attachWindowsProcessNames reads only `name`, so it should move to `readWindowsProcessIdentityTable` once #17866 lands -- on that PR's detailed reader it would open per-process handles for a field it discards. The reader does not exist on this branch, so the call stays as-is with the follow-up recorded rather than pulling #17866 in. The process-table perf table's two Toolhelp32 rows assume the optional `windows-process-tree.node` addon. The desktop bundles it; no released relay does, so on an SSH host the CIM row is the operative number. Comment-only. * docs(windows): state the CIM scan as the relay's normal path, not a fallback No released relay carries the optional `windows-process-tree.node` addon -- release-cut.yml has zero references to it and only dev-channel-win-build.yml builds it -- so the PowerShell CIM scan is what every SSH host runs. The call-site docstring read as a conditional fallback standalone. Comment-only. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
|
bfc6a262a7 |
fix(windows): read command lines from the kernel, not each process's PEB (#17886)
* fix(windows): read command lines from the kernel, not each process's PEB MDE incident D scored Orca for suspicious memory activity: the vendored `@vscode/windows-process-tree` recovered every process's command line by opening it with `PROCESS_QUERY_INFORMATION | PROCESS_VM_READ` and chaining three `ReadProcessMemory` calls through the PEB and `RTL_USER_PROCESS_PARAMETERS`. On a 750ms/2s cadence over the whole table that is the credential-dumping primitive, whatever the intent. Windows 8.1 added `NtQueryInformationProcess`'s `ProcessCommandLineInformation` class (60), which returns the same string as a kernel-built `UNICODE_STRING` under `PROCESS_QUERY_LIMITED_INFORMATION` alone. Electron's floor is Windows 10, so every supported OS has it. The PEB reader stays behind a process-wide latch that only `STATUS_INVALID_INFO_CLASS`/`NOT_SUPPORTED`/`NOT_IMPLEMENTED` can set; a pid that merely denied a handle does not re-arm it, because `PROCESS_QUERY_INFORMATION` implicitly grants the limited right and so cannot be obtained where the weaker open already failed. The same hunk drops `PROCESS_VM_READ` from `GetProcessMemoryUsage` and `GetCpuUsage`, which acquired it and never read an address space. Measured on Windows 11 (514 processes), counted in-process by swapping the addon's import table entries for counting stubs, per CommandLine scan: `ReadProcessMemory` 1128 -> 0, desired access 0x0410 -> 0x1000, p50 12.7ms -> 9.3ms. Command lines were byte-identical on every process both readers recovered (376/376, 379/379 across runs), including a 24,068-character argv with quotes, non-ASCII and trailing whitespace, and a WOW64 target. Three processes that refused the old rights granted the new one; none went the other way. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * fix(windows): drop the PEB fallback and detect the unpatched prebuilt Review of #17886 found three ways the reader could still perform, or silently resume, the primitive it exists to remove. The class-missing latch was a permanent, process-wide, one-way downgrade back to the PEB read, and any single target returning STATUS_INVALID_INFO_CLASS / NOT_SUPPORTED / NOT_IMPLEMENTED could trip it. On an EDR-hooked ntdll -- the entire premise of this change -- a hook that does not recognise class 60 would have restored PROCESS_VM_READ plus three ReadProcessMemory per pid per scan for the life of the process, unobservably, on precisely the machines this was written for. The fallback is deleted rather than guarded: GetProcessCommandLine now returns false and leaves the command line empty, which callers already handle, so the addon imports no ReadProcessMemory at all. That absence is what makes the property checkable on the artifact. The published 0.8.0 tarball ships a loadable prebuilt built from unpatched source; it is node-addon-api, so a bare require() accepts it, allowBuilds is false and CI installs with --ignore-scripts, and a rebuild that soft-exits on a Windows file lock leaves it in place. Source-text guards could never see it. windowsProcessTreeAddonReadsProcessMemory() checks the compiled binary instead, and is wired into the install check, the rebuild, and the relay build. The repair itself never worked: `git apply` run inside a work tree prefixes patch paths with the cwd-relative prefix, skips what does not match, and exits 0, so the branch always fell through to its own post-check throw. The package dir is always under the project root, while the fixture that covered it was in %TEMP%, outside any repo. Blinding git with GIT_DIR fixes it, and the test now runs inside a real work tree. Also from review: bounds-check the returned UNICODE_STRING against the allocation (not the size the second query clobbers) and cap the probe so a bogus length cannot bad_alloc a whole scan; test NT_SUCCESS explicitly; value- initialize ProcessInfo, which left `memory` as stack garbage -- measured, 82 processes reported the same bogus working set; and correct a comment in windows-process-table.ts that still described the command line as a PEB read. Re-measured on Windows 11 (543 processes): ReadProcessMemory 1128 -> 0, with the symbol absent from the import table so the IAT hook finds no slot to count; desired access 0x0410 -> 0x1000 on all 543 opens; p50 13.5 -> 12.3ms; 405/405 command lines byte-identical including a 24,087-character quoted non-ASCII argv and a WOW64 target; 3 processes recovered only by the new path, 0 only by the old. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * test(scripts): stage a script's local imports into the native-runtime fixture ensure-native-runtime.mjs gained an import of windows-process-tree-gyp-rebuild.mjs, but the fixture copied only the script itself, so every case in the suite died with ERR_MODULE_NOT_FOUND before reaching its own assertions. copyScriptWithLocalModules already walks a script's co-located imports for exactly this reason -- its own doc comment names this failure -- so use it rather than listing files by hand. The two Windows cases still fail here, on a missing node-pty ConPTY runtime that also fails on main; this only stops a resolution error from standing in front of whatever they were meant to catch. * fix(windows): route a locked stale addon to the Windows file-lock message `pnpm install` with Orca running aborted with a raw EPERM stack. The stale-binary guard -- which deletes an addon that still imports ReadProcessMemory so a skipped rebuild cannot use it -- ran outside the try whose catch classifies Windows file locks, and whose message is literally "Close running Orca/Electron/dev processes for this worktree": exactly this situation. Measured rather than assumed: rmSync against a loaded (memory-mapped) addon throws EPERM, and `force: true` does not help, since it only swallows ENOENT. Cold copies of the same file delete fine. So the delete threw a page before the handler that knows what it means. Moving the guard inside the try is the whole fix; the classifier already matches the EPERM text. The new case runs the real script against a temp project whose stale addon is held open by a live child process, and fails against the old placement with the raw `syscall: 'rm'` stack the report described. * feat(windows): warn once when command-line recovery is refused host-wide Removing the PEB fallback removed a total-defeat vector, but it left a cliff: if NtQueryInformationProcess(ProcessCommandLineInformation) is refused -- a hooked ntdll that does not know class 60 -- every command line comes back empty and agent identity matching silently degrades to image names. The addon still loads and still enumerates, so every health check the app has stays green. A cliff nobody can see is the failure mode this area keeps producing. The querying process is the unambiguous probe. A process can always open itself with PROCESS_QUERY_LIMITED_INFORMATION, so its own command line coming back empty means the query is refused for every process -- not that some target denied a handle, which is normal for roughly a quarter of the table. Keying on our own row rather than a fraction means no threshold to tune and no false positive on a hardened box where most processes deny. One warning per session, gated on the CommandLine flag actually being requested so a future identity-only reader cannot trip it. The suite's own SELF fixture gains a command line for the same reason: a self row without one is the alarm, not a detail. * fix(windows): check the relay's staged addon at load, and answer tri-state Two gaps in the ReadProcessMemory check, both about what it does not see. It only ever looked at node_modules/@vscode/windows-process-tree. A relay host has no node_modules of ours: it loads ./windows-process-tree.node staged beside the bundle. The relay build asserts the symbol on the artifact it produces, but a bundle and the addon beside it redeploy independently, so a host that has not taken a new bundle keeps whatever binary is already there -- and the published prebuilt is node-addon-api, so it binds cleanly and then walks every process's address space. loadWindowsProcessTree now checks that file too and refuses it, falling back to the CIM scan: slower, but not the thing an EDR quarantines a host for. The predicate is duplicated rather than imported, because the config-script copy is install-time tooling that drags in node-gyp and child_process, and this module is bundled into the app and the relay. And it returned false for a binary that is not there. All three callers happened to be safe, but the name read as a safety predicate, so a future caller would take a missing binary as verified. inspectWindowsProcessTreeAddon() now answers clean/unpatched/missing over an explicit binary path -- which is also what lets the relay's staged addon be checked at all -- and each caller states which state it acts on. Both are covered by cases that fail against the old code: without the load-time check the unpatched staged addon is bound and the CIM fallback never runs, and with 'missing' folded back into 'clean' the absence case fails outright. * test(windows): load the addon in beforeAll, not at collection time loadAddon() ran while the file was being collected, so on a Windows checkout with no built addon the require threw before any case existed and took the seven patch-text cases down with it -- cases that read only the patch file and need no binary at all. Verified both ways against a deliberately unresolvable addon path: at collection time vitest reports "no tests" for the file; from beforeAll the seven text cases pass and only the three addon cases go. * fix(deps): normalize the windows-process-tree patch to LF and let pnpm own its hash `pnpm install --frozen-lockfile` failed on this branch on every platform with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, which breaks CI and the release build. Two coupled defects. The patch file was committed with CRLF -- 174 CR bytes, against zero on main -- and `.gitattributes` pins `/config/patches/*.patch -text` precisely so checkout cannot convert it, so those bytes reached every runner. And pnpm hashes a patch **LF-normalized**, so the raw sha256 of a CRLF file is a value pnpm never computes: raw sha256 322965470c05f63d8527f7d8e892ee26ee444136b66b57fd64c362a9f2ff05d1 LF-normalized f8ea245391c94da5770045aeea01fa6de466c2199c6ef46b5b769b398aa9823e The lockfile carried the raw one, at all three sites. It is the only one of the seven patches where the two digests differ, which is why the other six passed. Normalized the patch to LF and took pnpm's own value from `pnpm install --no-frozen-lockfile`; nothing here is hand-computed. With the file LF-only the two interpretations coincide, so the lockfile, the contract test's no-CR assertion and its hash assertion all agree at one number -- and `config/scripts/windows-process-tree-patch-contract.test.mjs`, which was red on this branch for the same reason, is green again. The lockfile diff is exactly the three hash lines. The regression check is the installer, not a digest. Two separate reviews "verified" the shipped hash by recomputing sha256(patchBytes) and matching the lockfile; both were wrong, because both repeated the same wrong assumption about which bytes pnpm hashes. A check that reproduces the original mistake is not independent. So the new case runs `pnpm install --frozen-lockfile --lockfile-only --ignore-scripts` against a copy of the manifest, lockfile and patches, and asserts exit 0 -- verified by deletion: restoring the shipped hash fails it with the exact ERR_PNPM_LOCKFILE_CONFIG_MISMATCH from the branch's package (windows) job. Also corrected the `.gitattributes` comment claiming pnpm hashes patches byte-for-byte. The `-text` setting is right -- `git apply` needs the exact bytes -- but that sentence is the claim that produced the wrong hash twice. * ci(windows): run the process-tree patch suites in CI Both suites only self-skip off Windows, so the binary-level check that the addon carries no ReadProcessMemory passed vacuously in every lane. * fix(windows): force core.autocrlf=input for the patch repair My LF normalization of the windows-process-tree patch broke the `git apply` repair path introduced in this PR. The two are coupled and I checked only one. Those 174 CR bytes were not editor noise. They sat on exactly the pre-image lines and nowhere else -- 107/107 in src/process.cc, 67/67 in src/process_commandline.cc, 0 on every added or context line -- because @vscode/windows-process-tree@0.8.0 ships those two sources as CRLF. Normalizing the patch made its pre-image stop matching the file it is applied against. Measured, reconstructing the true CRLF pre-image from the pre-normalization blob and applying the current LF patch: core.autocrlf plain -c core.autocrlf=input true exit 0 exit 0 input exit 0 exit 0 false exit 1 exit 0 `false` is Git's own built-in default and what "checkout as-is" selects in the Git for Windows installer -- on this box the `true` that hides it comes from the installer's system gitconfig, not from anything in the repo. There the repair throws, ensureWindowsProcessTreeCommandLinePatch reports "still reads the PEB, and repairing it ... failed", isWindowsNativeLockError does not match that text, and `pnpm install` dies with no path forward. Forcing the mode rather than `--ignore-whitespace`: both fix every cell and both leave the applied file fully LF, but `input` relaxes line endings only, so a hunk whose real content drifted is still rejected. The repair rewrites a security-relevant source file; it should stay strict about everything except the thing that is legitimately ambiguous. Not reverting the patch to CRLF: windows-process-tree-patch-contract.test.mjs (pre-existing on main) forbids CR bytes in it, and pnpm computes the same hash either way. LF plus the forced mode is the end state. The suite could not have caught this. The fixture built its pre-image from the patch itself and joined with '\n', so fixture and patch agreed by construction on any encoding -- once again a test that passes without its fix. It now emits the CRLF the real package ships, and the case runs under both autocrlf modes pinned through a temp HOME gitconfig, because the repair blinds git to the repo and so reads global config. Verified by deletion in both directions: with the flag removed the autocrlf=false case fails with the exact "still reads the PEB" dead end while autocrlf=true still passes, and with the fixture back on LF all eight cases pass with no fix present at all. Also corrected the .gitattributes comment I added last commit. It said `git apply` needs the bytes the patch was written against, which is now false -- the pinned bytes are LF and the bytes it was written against are CRLF. That is the same class of confident-and-wrong claim that produced the bad hash twice. * fix(windows): assert the rebuilt addon, and install the patch for real in tests Three follow-ups from review. **The packaged binary had no check.** The relay build asserts its own artifact and ensure-native-runtime asserts what it loads, but nothing looked at the addon copied into the packaged app -- so a rebuild that silently produced the upstream reader shipped. `rebuild-native-deps.mjs` now asserts `clean` on it after `rebuild()`. This is also the caller D4's tri-state was missing: every existing site branches on `=== 'unpatched'`, so `missing` still behaved exactly like `clean` everywhere, which was the thing making it a state rather than a boolean. Here both non-clean states fail, and they fail differently: after a rebuild that reported success, an absent binary is a broken build, not an absence to shrug at. The fake `rebuild()` had to start producing a binary for that to mean anything, so it now emits stand-in bytes and takes `addon: 'clean' | 'unpatched' | 'none'`. Verified by deletion: with the assertion removed both new cases pass. **The frozen-install case could not see a patch at all.** `--lockfile-only` resolves and never applies one, so its coverage stops at hash consistency. Added a case that installs `@vscode/windows-process-tree@0.8.0` for real with the patch and asserts the materialized `src/process_commandline.cc` carries the marker and no longer carries `ReadProcessMemory` -- about 1.5s for the pair. Correcting the brief on that one: it does **not** catch the `git apply` breakage from the previous commit. Measured -- with `-c core.autocrlf=input` removed it passes cleanly, because `pnpm install` uses pnpm's own patch applier and never runs our repair script. What it does catch is a patch pnpm can no longer apply: corrupting one pre-image line fails both cases. The repair path stays covered by the CRLF fixture in rebuild-native-deps-node-pty.test.mjs. Worth recording, since it decides whether the LF normalization was safe at all: pnpm applies the LF patch to the CRLF tarball sources without complaint, and materializes them as LF with the marker present and `ReadProcessMemory` absent. The primary install path was never affected -- only the `git apply` fallback was. **Dead timeout.** The frozen-install case passed `timeoutMs: 300_000` to the spawn while vitest capped the case itself at 30s, so on a cold runner vitest would have killed it first. Both cases now declare the budget they use. * test(windows): route the frozen-install check through the pnpm invocation owner The new patched-dependencies check hand-rolled a PATH walk naming 'pnpm.cmd', which the windows batch shim spawn boundary ratchet rejects: pnpm-cli-invocation already owns that decision for every other script, and its allowlist only shrinks. Reuse resolvePnpmCliInvocation for the command and prefixArgs, and the shared resolveCliCommand for the presence check, so no shim name is spelled here. Its `shell` flag is dropped because runProcessSync refuses it and already drives a shim through the interpreter itself. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> |
||
|
OrcaWinandOrca Worker
|
687a22e1ee |
fix(computer-use): run the Windows runtime as one persistent helper (#17858)
* fix(computer-use): run the Windows runtime as one persistent helper
Microsoft Defender for Endpoint raised multi-stage Execution + Collection
incidents against Orca on Windows ("Screenshots were taken unexpectedly on
this device... Screen capture code was found in a script launched by
powershell.exe", factor "Executes suspicious MSIL code"). The desktop script
provider spawned a fresh powershell.exe per operation, so a single computer-use
session produced a burst of short-lived PIDs and re-emitted runtime.ps1's
inline Add-Type P/Invoke assembly on every click.
runtime.ps1 gains a -Serve mode that loads its assemblies once and then reads
NDJSON requests from stdin, and a new DesktopScriptRuntimeHost owns one
long-lived child: lazy spawn, strict serialization, a 30s per-request timeout,
restart on crash, a 120s idle shutdown, and dispose() on provider teardown. The
one-shot -OperationPath path stays as the fallback, and Linux keeps its python3
bridge unchanged.
Both Windows spawn sites now use -ExecutionPolicy RemoteSigned instead of
Bypass, falling back once to Bypass (and logging) when a Restricted host
refuses the unsigned script.
* fix(computer-use): recover the runtime host instead of latching it off
Review follow-up on the persistent Windows computer-use helper.
A helper that died before producing a line set an unavailable flag nothing ever
cleared, and the client then dropped the host for the life of the session. One
transient bad spawn — a Defender scan, a locked CSC temp directory — silently
restored the per-click powershell.exe burst and per-operation MSIL emission this
work exists to remove, with computer use still working so nothing looked wrong.
Start failures are now retried, then cool down for 60s, then re-probed; the
client keeps the host so it can come back. Repeated post-answer crashes cool
down too, and a single reply no longer clears the failure count.
The one-shot bridge decided its execution-policy retry from a message that fell
back to stdout, so a window title containing "SecurityError" could replay a
non-idempotent operation — a double click, keystroke or paste — and stick the
session on Bypass. The retry now requires empty stdout and a matching stderr.
Serve-mode replies carry an echoed request id. Without one a single stray stdout
line would make every later response answer the previous request, acting on
stale element indexes with no error raised; a mismatch now kills the child.
Non-JSON noise is ignored rather than counted as the helper having answered.
Also: warnings reach the main process over the sidecar's IPC channel rather than
its piped, unread stdio; the child is watched on close rather than exit; dispose
latches so a queued request cannot respawn during teardown; and the host is
split into a serve channel and an availability policy to stay under max-lines.
* fix(computer-use): prove a helper never started before replaying its request
The retry that replaced the permanent-latch bug could deliver unrequested
input. send() re-sent the same request whenever the helper died without
replying, but "no reply came back" is not "the operation did not run":
runtime.ps1 synthesizes the click and only then builds the snapshot, which
allocates a full-window bitmap and walks the UIA tree — a native GDI+/UIA fault
there is uncatchable, and leaves the click already delivered. A deterministic
fault meant three clicks from the host plus a fourth from the one-shot bridge,
surfaced as a single failed operation.
-Serve now writes one {"ready":true} line after its Add-Type work and before
its first read, so "never started" is a fact rather than an inference. A request
is replayed only when the helper died before announcing. A runtime.ps1 that
predates the announcement — reachable through the provider path override — is
covered by an observation-tool allowlist until a ready line proves otherwise.
Host-detected aborts (timeout, desynchronised reply, oversized line) suppress
the exit handler, so they were bypassing failure accounting entirely and a
helper failing that way was respawned once per operation forever. They now
count and are logged.
Also stop charging twice for one outage: entering the cooldown resets the
failure count, so the first death after recovery no longer re-enters a full
cooldown and an interleaved workload cannot be stranded on the one-shot bridge.
* fix(computer-use): ignore a stdin write callback from a torn-down helper
stop() destroys stdin, so a write still queued at teardown calls back with
ERR_STREAM_DESTROYED. The callback carried no channel or request identity and
write() had no closed guard, so it ran abortChannel a second time: stopChannel
no-opped but recordFailure and the warning did not, charging two failures for
one operation and reaching the 3-strike cooldown at half the intended rate.
That feeds the same accounting that keeps a persistently broken helper from
respawning once per operation.
The same root also allowed a late callback landing after a replacement channel
existed to stop that channel and reject a different request with the previous
one's error. Node fires the destroyed-stream callback on the next tick, well
before a new request arrives, so the double-count is the reachable effect;
binding the callback closes both.
write() now drops payloads and error reports once closed, and the host ignores
any report whose channel or request id is no longer current.
* test(computer-use): pin each stale-write guard independently
The channel's closed guard and the host's request-identity check are redundant
by design, and the existing tests only failed when both were absent. Someone
deleting one, believing the other was the covered one, would have got a green
suite and a live regression — the same shape as a test that passes without the
fix it was written for.
Each is now pinned on its own. The channel's half is tested against the channel
directly: after stop() it takes no writes and reports no error from one already
queued, which the host cannot observe because it drops the channel at the same
moment. The host's half is pinned by the case the channel cannot see — a live
channel whose request was already answered, where backpressure delivers a write
callback for a request that is no longer pending.
Removing either guard alone now fails a test. Both carry a comment saying they
are deliberately redundant and separately pinned, so the next reader does not
have to rediscover this from the diff.
* ci(windows): run the computer-use runtime host suite in CI
The win32 suite only self-skips off Windows, so it passed vacuously in
every lane. Register it the way the cmd-shim suite is registered.
* fix(computer-use): time the runtime host cooldown on a monotonic clock
The start-failure cooldown was a wall-clock deadline, so a backwards step —
an NTP correction, a VM snapshot restore, a user changing the clock — left
`remainingCooldown()` returning the cooldown plus the whole step. A one-hour
step measured 3,660,000ms, and ten real minutes later still 3,060,000ms.
Nothing shortens it from there. Only `recordSuccess()` clears the cooldown on
a non-dispose path, and no request can reach a helper to succeed while it
holds, so every `send()` throws `runtime_host_unavailable` first. The host is
built with no `now` override and its lifecycle is a module-level singleton
that shuts down at process exit, so the latch held for the sidecar's life —
computer use kept working via the one-shot bridge while the per-click
powershell.exe burst this host exists to remove came back silently.
Store the instant the cooldown began and compare elapsed monotonic time,
following the two fixes in #17884. The field is `number | null` rather than
sentinel 0 because `performance.now()` legitimately returns 0.
Both new tests leave `now` unset, because the bug was in the default the host
picks and a test that injects a clock cannot see it.
* fix(computer-use): give a queued request its own deadline
The 30s request timeout was armed only in `sendOnce`, once a request reached
a helper. A request behind N timing-out ones therefore waited roughly N times
that with no deadline of its own: bounded, but the caller sees an `await` that
looks hung for minutes and gets no error to act on.
Move the serialization tail into its own class and arm a deadline at enqueue
time. Only the wait is bounded — a request that reaches a helper still gets
its full execution budget, so nothing that used to succeed now fails. An
expired request is dropped rather than sent late: the caller has already been
told it failed, and a click delivered after that is worse than no click.
The tail keeps its never-rejecting shape and chains on the turn rather than on
the raced promise, so a caller giving up early cannot release the next request
while its predecessor is still in flight.
* fix(computer-use): stop reading a locked file as an execution policy block
`UnauthorizedAccess` is the FullyQualifiedErrorId PowerShell reports for a
policy block, and it is also a strict prefix of `UnauthorizedAccessException`,
which .NET raises for any ordinary locked or ACL-denied file. The predicate
matched the token unanchored, so an AV scan holding runtime.ps1 or a locked
CSC temp directory was read as a policy block.
Two consequences, both bad. `escalateExecutionPolicy()` has no path back, so
one false match spent the rest of the session on `-ExecutionPolicy Bypass` —
the exact command line token this stack exists to stop emitting. And on the
one-shot path `isPolicyBlockedStart` re-runs the operation: one-shot mode
writes stdout only after the operation returns, so a crash partway through an
action is indistinguishable from a helper that never started, and the click
lands twice.
Measured on Windows against all three records, which the test carries verbatim
as fixtures:
policy/Restricted FullyQualifiedErrorId: UnauthorizedAccess
policy/RemoteSigned FullyQualifiedErrorId: UnauthorizedAccess
genuine access denied FullyQualifiedErrorId: UnauthorizedAccessException
`\b` is the whole discriminator: between `s` and `E` both sides are word
characters, so no boundary exists there and the exception cannot match.
Dropped two alternatives that measurement showed were wrong. `PSSecurityException`
never appears — the record surfaces through a native-command wrapper and reports
`ParentContainsErrorRecordException`. The prose is wrong three times over: it
differs by policy, it is localized, and PowerShell hard-wraps it mid-sentence.
Anchoring on the `FullyQualifiedErrorId:`/`CategoryInfo:` labels would be more
precise again, but those labels are localized where the values are not, so it
would lose a real block on a non-English host and strand it with no fallback.
Matching the values with word boundaries keeps both directions; a fixture with
translated labels pins it.
The escalation stays sticky. With the predicate correct, it only fires on a
machine that really does block, where re-probing the preferred policy would buy
a guaranteed failed spawn per operation.
* fix(computer-use): route a malformed request back to the request that caused it
`ConvertFrom-Json` throws before `$requestId` is read, so the serve loop
answered an unparseable request with an untagged error. On the client that is
not an error at all: `deliver()` sees no matching id, calls `abortChannel`,
kills the helper and charges a failure — and the helper's own message is
discarded. A parse failure was reported as a stream desync with no trace of
the real cause, and three of them walked into the 60s cooldown behind three
misleading "did not match" messages.
Recover the id from the raw line when the parse fails. No wire change: the
response shape is untouched and `BridgeResponse.requestId` already documents
this echo. It is the same shape the helper already returns for `not_a_tool`,
where the id survives because it is read before the operation runs. Both
mixed pairings degrade safely — a new script with an old client resolves the
error normally, and an old script with a new client still aborts, but now
reports what the helper said.
When the line is mangled past recovering an id, the desync abort is the honest
outcome, so keep it and carry the helper's text into it rather than replacing
it. A line the helper could not tag is usually the only account of the cause.
Proven against the real `runtime.ps1 -Serve`: the host can only write
well-formed JSON, so the parse-failure branch is unreachable through it and
the test drives the channel directly.
* fix(computer-use): keep the Bypass escalation only when Bypass actually works
AppLocker and WDAC constrained language mode raise PSSecurityException under
the same SecurityError category a real execution-policy block uses, so the
predicate matches them - correctly, on the evidence available. But those block
the script at parse time, which `-ExecutionPolicy Bypass` cannot lift. The
escalation was sticky unconditionally, so on a WDAC host we misdiagnosed,
retried, failed again, and then latched: every later command line carried the
most heavily weighted MDE token there is, on exactly the hardened, monitored
enterprise machine that is watching for it.
Treat the escalation as the diagnosis it is. A fallback that cannot start a
helper either disproves it - the policy was not what stopped the first attempt
- so revert to RemoteSigned instead of latching. When Bypass does start a
helper the diagnosis is confirmed and it stays sticky exactly as before, so a
genuinely Restricted machine still never pays a re-probe per operation.
The revert lands inside the outage rather than only at its end, so a
misdiagnosis costs one Bypass command line instead of one per attempt, and an
escalation that never proved itself does not outlive the cooldown that ends
the outage. Deliberately not a permanent "fallback is useless" flag: a Bypass
attempt that failed for a transient reason would then disable the fallback for
the session, which is the same latch in the other direction.
Only `runtime_host_unavailable` proves no helper started, so only that reverts;
a helper that started and then died proves Bypass works. That also makes the
policy branch reachable on a final attempt for the first time, so it now
rejects as unavailable rather than a generic error - that code is what routes
the operation to the one-shot bridge, which carries its own policy fallback,
and without it an all-blocked host would fail operations outright instead of
degrading. The pre-existing "reports itself unavailable when Bypass is also
refused" test pins that.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
OrcaWinandOrca Worker
|
fba90e017c |
fix(windows): copy the daemon host exe verbatim instead of renaming it (MDE T1036) (#17865)
* docs(windows): document the EDR signal surface Six Microsoft Defender for Endpoint incidents fired against Orca 1.4.192 in eight days on one enterprise Windows 11 / Intune tenant. All six were behavioural process-tree scoring, not signature hits; two escalated to multi-stage incidents mapped to ATT&CK Execution and Collection. Add a reference doc mapping each attack-technique-shaped behaviour to the code that produces it and to why it exists: the renamed daemon image (T1036), the per-process PEB read, encoded policy-bypassed PowerShell (T1049), caret-escaped cmd.exe lines, and computer-use screen capture plus runtime-compiled MSIL (T1113). Records that signing is not the gate -- reputation is signer plus hash-keyed prevalence -- and carries the two evidence gaps the report noted. Adds an engineer checklist, deployment guidance for admins (AV path exclusions do not suppress EDR behavioural alerts; an MDE alert suppression rule does), and an explicit pre-deployment warning about computer use. * docs(windows): correct the PowerShell flag inventory and admin paths Review corrections to the EDR posture doc. The "encoded, policy-bypassing PowerShell" list conflated three different shapes and was incomplete. Split it into the three tiers an EDR actually scores differently -- bypass plus encoding, encoding alone, and bypass alone -- and add the sites it missed, including windows-mobile-firewall.ts, which encodes a script and launches it elevated through Start-Process -Verb RunAs. system-fonts.ts (-Command) and desktop-script-provider-bridge.ts (-File) were listed as encoded and are not. Notes that a raw grep under-reports, because the hook sites reach -EncodedCommand through wrapWindowsPowerShellEncodedCommand. Attribute the in-payload Set-ExecutionPolicy move to #16576 rather than to #16003's measurement, which keyed on -WindowStyle Hidden + -EncodedCommand, and record that the launcher's own tradeoff is unverified on a real box. Admin guidance was missing two ways a suppression rule pinned to one full path misses real activity: the .staging-<hex> sibling that exists mid-update, which is when the update-cluster incidents fire, and the userData fallback when LOCALAPPDATA is unset. Also: state the measurement conditions on the process-table timings, note that Hermes has surface even though we have no telemetry for it, note that the uninstaller names are electron-builder-generated and in no repo file, drop a volatile line count, and mark the per-operation computer-use shape as being addressed by an unmerged change. Drops the duplicated AGENTS.md section, keeping the indexed bullet. * docs(windows): reconcile the EDR posture doc with the shipped remediation Three claims in this doc became false once the rest of the Windows EDR set landed, and two told engineers the opposite of what the release does. The process-table section still described one shared snapshot taken with `Memory | CommandLine | CreationTime`, argued that splitting the cache per field set "would restore exactly the fan-out it exists to prevent", and concluded the shape was unfixable because "the information is only in the PEB". The split shipped (identity opens no handle at all), `Memory` is retired, and the command line now comes from the kernel through `ProcessCommandLineInformation` -- `ReadProcessMemory` is absent from the compiled addon and a ratchet asserts it against the import table. An engineer reading the old text would have concluded both fixes were dead ends. The PowerShell site inventories were stale in three of four lists: the port scan went native, every `-ExecutionPolicy Bypass` + `-EncodedCommand` pair was dropped as a measured no-op, and of the unencoded-bypass list only `wsl-cli-scripts.ts` survives. Regenerated against the merged tree, including the sites that reach the flag through `wrapWindowsPowerShellEncodedCommand` and never spell it, which a raw `rg` misses. Incident-evidence sections are left alone: they record what the tenant observed on 1.4.192, not what the code does now. * fix(windows): copy the daemon host exe verbatim instead of renaming it Microsoft Defender for Endpoint flagged `orca-terminal-daemon.exe` as MITRE T1036 (Masquerading): Orca copied its own `Orca.exe` into %LOCALAPPDATA% under a different name, specifically so the NSIS updater's `taskkill /IM Orca.exe` could not match, then ran it detached. Because that process is what every other flagged action was attributed to, the name mismatch acted as a reputation multiplier on unrelated findings. The rename was never what made the daemon survive. In app-builder-lib 26.15.3 the installer's FIND_PROCESS/KILL_PROCESS select processes whose image path is under $INSTDIR; `taskkill /IM` is only the fallback for hosts where PowerShell is missing or blocked. Survival is a property of the path, and %LOCALAPPDATA%\Orca\daemon-host is outside $INSTDIR whatever the file is called. Derive the host exe name from process.execPath so the copy is byte-for-byte, name included — it keeps its Authenticode signature and carries no renamed-image signal. On the no-PowerShell fallback the daemon is now killed with the app and terminals cold-restore, which is the documented pre-relocation outcome the update harness already asserts, not a regression. The uninstall macro no longer needs a distinct name to find the daemon; it kills the app's own image name (plus the legacy name, for hosts left by older builds). Adds docs/reference/windows-daemon-host-relocation.md with the survival contract, the rejected alternatives and their measured costs, and the invariants to keep. * fix(windows): apply daemon-host relocation review corrections Scope the uninstall taskkill to the current user with `/FI "USERNAME eq %USERNAME%"` via cmd.exe, matching upstream's per-user KILL_PROCESS — without it an elevated machine-wide uninstall reaches another logged-on user's session, so the "no collateral" claim in the comment was overstated. Comment the rmSync-before-publish: Windows refuses to delete a running image, so a live daemon already hosted in this version's dir (same-version reinstall, or a dev channel reusing a version) throws and materialization fails open. Doc corrections: - The fallback selector is the full per-user `taskkill /F /IM "<app>.exe" /FI "PID ne $pid" /FI "USERNAME eq %USERNAME%"`, not a bare `taskkill /IM`. - The probe reads `Get-ExecutionPolicy -Scope Process`, not the effective policy, and GPO writes MachinePolicy/UserPolicy — so GPO-managed hosts take the primary path-scoped branch. Narrow the fallback triggers accordingly. - Drop the Authenticode sentence: the old name was equally byte-identical and equally signed, so a filename has no bearing on signature validity. - Name the new update-abort path: the daemon now matches FIND_PROCESS, so on the fallback branch an unkillable host reaches the retry loop's MessageBox /SD IDCANCEL and Quits, aborting a silent update. - Correct the customCheckAppRunning rejection. It is ~6 lines, not a rewrite; it is wrong because forcing the PowerShell branch where PowerShell is absent makes FIND/KILL silently no-op and leaves the real app running with files in use. - Bound the win honestly: OriginalFilename is empty on the shipped binary, so the strongest T1036 indicator never fired, and the residual copy-and-run-detached shape still maps to T1036.005. Reconcile docs/reference/windows-edr-posture.md, which documents the rename as a live finding and would otherwise contradict this change. Content-only edit: markdown under docs/reference/ is not oxfmt-formatted as a matter of practice and nothing in CI gates it, so the file is left consistent with its neighbours. * fix(windows): expand USERNAME in NSIS instead of spawning cmd.exe The uninstall macro routed both taskkills through `"$SYSDIR\cmd.exe" /C` purely so `%USERNAME%` would expand — two extra interpreter spawns on the uninstall path, in a change whose whole point is not adding scored behaviour, and the exact `cmd.exe /c` shape the new AGENTS.md EDR bullet warns about. NSIS reads the variable itself with ReadEnvStr, so the spawns buy nothing. Verified on Windows 11 that the generated command line does what the filter is there for: a copy of cmd.exe running as orca-nonexistent-probe.exe (pid 34244) was terminated by `taskkill /F /IM "orca-nonexistent-probe.exe" /FI "USERNAME eq <user>"` — SUCCESS, exit 0, process gone. Guarded on an empty USERNAME because the degenerate case is silent: taskkill rejects an empty filter value outright ("The search filter cannot be recognized") and kills nothing, which would leave exactly the orphaned daemon this macro exists to reap. `*` is rejected as a filter value too, so there is no branchless spelling. With no USERNAME to scope by it kills unfiltered, as the macro did before the filter was added. Stack stays balanced: three pushes, two nsExec pops, three restores. Also strike the last stale row in windows-edr-posture.md's remediation table. "Copying our own image under a different name" read as outstanding work; it is done by this change, so the row now points at the relocation doc. Same class of staleness as the section reconciled in the previous commit, and git would not have flagged it either. * fix(windows): port the daemon-host uninstall sweep into the live NSIS include The uninstall macro this branch rewrote lived in config/nsis/daemon-host-uninstall.nsh, which main no longer includes: #17906 consolidated every Windows installer hook into config/nsis/orca-installer-hooks.nsh because electron-builder accepts exactly one `nsis.include`. Merged as-is, the rewritten macro would have been dead code while the shipped uninstaller kept running main's stale sweep — `taskkill /F /IM orca-terminal-daemon.exe`, which matches nothing now that the relocated host is a verbatim Orca.exe copy. The RMDir that follows then cannot delete the running image, so a live orphaned daemon and its ~224 MB tree would survive every uninstall. Ported into the live include: the ${APP_EXECUTABLE_FILENAME} kill, the USERNAME filter that keeps an elevated machine-wide uninstall out of another logged-on user's session, and the register save/restore around both. The legacy orca-terminal-daemon.exe kill stays so hosts left by older builds are still reaped. The ratchet that was meant to catch exactly this pinned only the legacy image name, which main's stale macro already satisfied, so it passed both ways. It now asserts the app-exe kill and the USERNAME filter, against comment-stripped script — the prose above the macro names both image names, so a toContain over the raw file proves nothing. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
Neil
|
8ed81ceb8d | perf(tabs): index saved tab order during hydration repair (#18964) | ||
|
Neil
|
56626e7daa |
perf(ssh): reuse and release relay startup buffers (#18953)
* perf(ssh): reuse the searched relay startup prefix * perf(ssh): release startup banners after relay readiness |
||
|
Neil
|
bf073b833e | perf(skills): skip symlink probes beyond discovery depth (#18937) | ||
|
Neil
|
6f28e019b5 | perf(hooks): use native reverse search for transcript lines (#18936) | ||
|
Neil
|
9b76ff9217 | perf(explorer): avoid redundant dotfile path filtering (#18929) | ||
|
Neil
|
fb7b75d55d |
perf(cli): skip feature formatters during help and error startup (#18923)
* perf(cli): load error reporting without feature formatters * test(cli): follow extracted error reporter in import guard * chore(cli): track cli-error.ts in deferral equivalence baseline The equivalence script restores TOUCHED files from the baseline rev to rebuild the pre-deferral CLI. reportCliError/formatCliError moved from format.ts into cli-error.ts, so the baseline arm must also drop cli-error.ts (absent at older revs) or the old tree would still compile against the new module. |
||
|
Neil
|
388e9fb776 | perf: avoid rescanning emitted source in analysis guards (#18920) | ||
|
Neil
|
4e8e14424d | perf: avoid splitting every path during file autocomplete (#18919) | ||
|
Neil
|
4204bdf717 | perf: avoid repeated Quick Open exclusion string allocations (#18916) | ||
|
Neil
|
e9d9d42ccc |
perf: find mobile Markdown placeholder prefixes in one scan (#18914)
* perf: find mobile Markdown placeholder prefixes in one scan * style: format mobile Markdown benchmark guard * docs: explain collision-free Markdown prefix length |