mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
c72afda498d4d29b69ebad2b5aa6bfea86e199f2
482
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
6f8c5888b3 |
Run Node 26 compatibility daily instead of per PR (#16946)
* Run Node 26 compatibility daily * Update relocated unit workflow contracts |
||
|
Neil
|
2b391652b1 |
fix(terminal): a close the host never heard must survive the reconnect (#16752)
An enterprise user: "Every day I open orca and it opens more tabs daily at a linear scale." Three reports over a week, told on 08-19 that a PR had fixed it, reported twice more after. STA-4658 (P0), GH #12447, #15136, #10342, #9585. One install held 39 zombie tab records. The revived tab's sleeping-agent record still holds the pre-close session id, so it boots `claude --resume <old id>` -- two agents on one transcript. ## The chain, measured Reproduced deterministically in `ssh-lost-kill-tab-resurrection.spec.ts`: close an SSH tab, kill the relay daemon in the container so `pty.kill` rejects with a transport-class error, reconnect. drop 2 resurrected the closed tab <id>: baseline=1 drop1=1 drop2=2 (closed tab returned) drop3=1 The trigger is narrow and had to be measured rather than assumed: killed relay daemon reproduces **6 of 6 runs**; an orderly `ssh.disconnect` **passes**. Only an ungraceful loss -- network partition, host reboot, relay crash, a laptop sleeping mid-session -- strands the close with the RPC rejecting on a transport-class error. Both variants live in the spec behind one `runResurrectionCycles` parameterized solely by the disruption, so the difference is attributable to that single variable. What actually carries the tab back, from the pull path (`workspace.get` -> `getRemoteSnapshot`, `remote-workspace-relay-sync.ts:29`): pullSnapshot rev=3 tabs={repo:["16c4a3e1","06aba6b6"]} pullSnapshot rev=4 tabs={repo:["16c4a3e1","ff72768e"]} <- ff72768e IS the resurrected tab pullSnapshot rev=5 tabs={repo:["16c4a3e1","ff72768e","da21b76c"]} The client uploaded the session containing the tab; the user closed it; the kill RPC rejected so the close never reached the host; the host's snapshot still lists it; the client pulls it back and the merge restores it -- **correctly, by its own rule that the host is authoritative for what it knows.** A pane then mounts, respawns, and takes the recycled pty id. Client-side correlation from the same run, two controls and one positive in one run differing in exactly one variable: | Tab | Close events observed | Resurrected? | |---|---|---| | `6305cc07` | `user` + `pty-exit` | No | | `ed56f66c` | `user` + `pty-exit` | No | | `2036e760` | `user` only | **YES** | ## The fix `src/shared/closed-terminal-tab-tombstones.ts` (99 lines). A client-recorded close is first-party intent and must survive until the host acknowledges it. Per `docs/reference/ssh-execution-boundary.md` the remote verdict is `unverifiable` -- which may not authorise declaring the process dead, but equally must not authorise resurrecting the tab. This is SSH-v3 principle P2, "durable tombstones with a monotonic per-scope revision", reusing the existing `RemoteWorkspaceSnapshot.revision` rather than adding a twelfth per-tab identity field (the codebase carries eleven, 784 refs, that SSH-v3 Phase 3 deletes). - **Recorded** only on `closeReason === 'user'` (`terminal-tab-close.ts:69`). - **Suppresses** a host-sourced tab only when `tabId in tombstones && !currentTabsById.has(tabId)` -- a live local tab always wins, because deleting a live pane is the one outcome the merge exists to avoid. - **Retires** on positive acknowledgement: `!hostKnownTabIds.has(tabId) && hostRevision > observed`. Strictly newer, so a pull already in flight at close time cannot ack a close it predates. - Three never-retire guards: no revision retires nothing; a worktree the snapshot has no row for retires nothing; the first omitting snapshot only stamps the watermark. - TTL (30d) + cap (500) are **backstops** for a target the user never returns to, not the mechanism. - **Client-local only** -- never crosses the wire, so there is no mixed-version exposure. - Suppression is scoped to `replaceWorktreeIds`, which is what makes the live-tab check meaningful. A final whole-map sweep over the assembled `tabsByWorktree` would break that (a live tab is absent from `currentTabsById` outside the scope and would look suppressible); it is deliberately not there, and the comment at the top of the function says so. ## Evidence The load-bearing evidence is an A/B control on one tree, not the oracle's assertion. Flipping `isSuppressedByClose` to `false` -- one character -- reproduces the resurrection on demand: --repeat-each=2: 1) drop 2 resurrected the closed tab ab0e305d-…: baseline=1 drop1=1 drop2=2 2) drop 2 resurrected the closed tab 51533e34-…: baseline=1 drop1=1 drop2=2 2 failed With suppression on: **0 occurrences of "resurrected the closed tab" across five runs plus one independent run by a second agent.** Provenance verified positively, not by mtime: `closedTerminalTabTombstonesByTabId` appears 13x across 3 renderer chunks including `store-Do3KBvRE.js`; for every red control run `mayCreate` appeared 0 times in `out/main/index.js`. At the unit layer, disabling the same predicate: 3 failed | 39 passed. Restored: 42 passed; 287 across the workspace-session, terminal-store, remote-workspace, shared-tombstone and profile suites; 24 in the four tombstone suites. ## The oracle spec: GREEN in the full lane `ssh-lost-kill-tab-resurrection.spec.ts` passes both tests at this commit. Full Docker-SSH lane, clean tree: BUILD_SHA=49bb96e0b4c DIRTY=0 PROVENANCE tombstone=13 hasLocalTabsRow=2 hostAuthority=4 mayCreate=3 14 specs / 20 tests -> 17 passed, 2 failed, 1 skipped (10.7m) [12/20] :178 does not resurrect tabs whose kill was lost to a killed relay daemon PASSED [13/20] :190 does not resurrect tabs closed while the host is disconnected PASSED grep -c "resurrected the closed tab" (whole lane) -> 0 It passes WITHOUT PR 7 in the build (`mayCreate` present, `SshPtyAbsentFromRelayError` absent), so the bug-2 fix below is not required for it. Test 1 fails intermittently in ISOLATED single-spec runs, where a third defect blocks its cycle-2 setup. The resurrection assertion itself has never failed with this fix in place -- the intermittent failure is always a setup failure, never a resurrected tab. A reviewer running the spec alone may see it red; that is not this fix regressing. Three defects sit under STA-3374 and should not be conflated: - Bug 1 -- the closed tab resurrects. Fixed here. - Bug 2 -- `ssh-pty-session-reattach.ts:227-231` rewrites the relay's `PTY "pty-1" not found` into a bare `SSH_SESSION_EXPIRED`, so `isPtyAlreadyGoneError`'s `/PTY ".+" not found/` cannot match and `attachStablePaneOwner:242`'s already-correct fallback never runs. Owned by PR 7 (`nwparker/ssh-07-absent-from-relay`). Not required for the oracle above. - Bug 3 -- after the daemon is killed and the client launches a replacement, the client's OWN SSH transport drops and does not reconnect within 60s: no "delay step 2/9", no handshake failure, nothing. `ssh-connection.ts:1533` only logs on an SSH-level close. Unfixed, its own ticket. This is what makes test 1 intermittent in isolation. Discriminator for bug 3, measured in the isolated runs (the lane above ran without `ORCA_E2E_FORWARD_APP_LOGS=1`, so it was not re-confirmed there): `[ssh-relay] Socket probe result:` reads "DEAD" on every cycle of test 1 (daemon killed, a NEW relay must be launched) and "ALIVE" on every cycle of test 2 (daemon survived). Whenever a new daemon must be launched, the SSH transport drops afterwards and does not recover. An earlier reading blamed `kill.ts:82-84` for skipping `finishPtyShutdown` on a non-already-gone error. That was eliminated by direct test: the implied fix, `markSshRemotePtyLease(…, 'expired')` in that branch, was implemented, changed nothing, and was reverted rather than shipped unproven. Recorded so the path is not re-walked. The `SSH_SESSION_EXPIRED` rejection is real but fires during cycle 1 for the baseline pane, after which cycle 1 completes; the 60s silence begins only after `Relay channel lost ..., triggering reconnect`. The spec is claimed by the Docker-SSH lane, and that lane does not gate merges today. ## Persistence: the tombstone must survive a relaunch `closedTerminalTabTombstonesByTabId` is declared on `WorkspaceSessionState` but was missing from `workspaceSessionStateSchema` (`src/shared/workspace-session-schema.ts`), which is the load boundary for BOTH partitions -- `normalize-loaded-state-collections.ts` for `local` and `workspace-session-partitions.ts` for `ssh:<target>`. Zod strips unknown keys and the write side does not validate, so the map reached disk and was discarded on the next launch. Measured with the repo's own parser: input : closedTerminalTabTombstonesByTabId: { 'tab-1': {...} } ok = true tombstones after parse = undefined That made the fix ineffective in the exact reported scenario: close an SSH tab with the transport down, QUIT, relaunch, reconnect -- the merge runs with an empty map, the host still lists the tab, and it resurrects. "Every day I open orca and it opens more tabs" is a claim about restarts. Neither the green oracle nor the A/B control could see it: both run entirely inside one app process. It also made the 30-day TTL and the 500 cap unreachable. Fixed by adding the field with a `salvagingRecord` matching its sibling `terminalSurfaceTombstonesByPaneKey`, so one malformed entry drops that entry rather than the map. `workspace-session-schema.ts` was one line under its 300-line max-lines limit, so adding the field required room rather than a suppression (the project forbids max-lines disables and per-file bumps). Two value schemas were extracted to modules named after what they contain: `terminal-tab-id-schema.ts` and `terminal-surface-tombstone-schema.ts`. The closed-tab tombstone's own schema is colocated with its type in `closed-terminal-tab-tombstones.ts`, which is where it belongs -- omitting it from the session schema is exactly the drift that caused this bug. `workspace-session-schema-field-coverage.test.ts` is the ratchet. Two sibling tables already pin themselves with `satisfies Record<keyof WorkspaceSessionState, ...>`; this schema had no such guard and is the one that fell behind. The new file adds both halves -- a `satisfies` list that makes a forgotten field a compile error, and a runtime assertion that names it -- plus a `parseWorkspaceSession` round-trip. Without the schema entry: 3 failed. With it: 3 passed. ## A host tab the user never closed could be deleted `tabId in closedTerminalTabTombstonesByTabId` answers true for every `Object.prototype` key even on an EMPTY map, because the map is a plain object from `Object.fromEntries`. A host tab whose id is `toString` was filtered from the reconciled list, blocked from the host-unknown branch, and stripped of its layout and session id. Tab ids are validated only as non-empty and colon-free, and `createTab` honours caller-supplied id hints, so the id is reachable rather than theoretical. This was the only path in either direction that could delete a tab the user never closed. Now `Object.hasOwn`, as the same file already uses elsewhere. Suppression is also scoped structurally: `isSuppressedByClose` compares the tombstone's stored `worktreeId`, which it already carried, so it cannot reach another workspace's tab. The two sweeps that have no worktree in scope (`terminalLayoutsByTabId`, `remoteSessionIdsByTabId`) now consult the set of ids this merge actually suppressed rather than re-deriving a verdict without that scope. The scope comment at the top of the function was also wrong and is corrected. It claimed every use of suppression sits inside `replaceWorktreeIds`; it does not -- the tabs pass walks all of `orderedWorktreeIds` and the two sweeps cover the whole remote maps. What actually makes it safe is that `closeTab` strips the id from every worktree row before recording the tombstone, plus the worktree match above, plus `closeReason === 'user'` being the only writer. Real guarantee, different from the documented one. ## Divergences from open PR #16571 #16571 implements the same concept. Three deliberate changes: 1. It never retires on acknowledgement -- TTL+cap only, so it never converges. Ack retirement added. 2. It crosses the wire and lets a HOST-sourced tombstone delete a LOCAL tab in a final whole-map sweep. After #14361 that is the wrong risk; dropped. This also removes the mixed-version regression its own body flags. 3. Its hydration unions rather than replaces the map -- a union resurrects every tombstone the merge just retired, so it never converges. Its `activeTabId` nulling is also dropped as redundant: `workspace-terminal-hydration.ts:99-105,126-138` already revalidates both pointers against the tab rows it just built, and nulling twice would add a second rule that has to stay in step with the first. ## Can a tab the user did NOT close disappear? No, but the guarantee needs stating precisely. The only writer is `recordClosedTerminalTabTombstone` (`terminal-tab-close.ts:69`), reachable only on `closeReason === 'user'`; suppression additionally requires the tab not be live locally. Reopen (`recently-closed-tabs.ts:122-166`) calls `createTab` and restores cwd/shell/title/color/position, never the old id. **Caveat, stated because the slogan is not literally true:** `createTab` honours a caller-supplied id hint (`terminal-tab-creation.ts:53-65`, used by `useIpcEvents` for host-admitted tabs), so "tab ids are uuids that never recur" does not hold in this codebase. The guarantee rests on the `closeReason === 'user'` writer plus the live-local-tab check, not on id uniqueness. ## Risk Renderer-side, client-local, no wire change. The blast radius is `mergeDirectSshRemoteWorkspaceSession` and the persisted session field. Worst case if the ack logic were wrong in the retiring direction: a tombstone outlives its usefulness and suppresses a host tab whose id the host re-issues -- bounded by the live-local-tab check, the 30d TTL and the 500 cap. Worst case in the other direction is today's behaviour. `profile-project-session-field-disposition.ts` records the new field as `notRepoScoped` / `notTransferred` residue, bounded by the same TTL and cap. ## Verify pnpm test src/shared/closed-terminal-tab-tombstones.test.ts \ src/renderer/src/lib/workspace-session-closed-tab-tombstones.test.ts \ src/renderer/src/store/terminals/terminal-tab-close-tombstone.test.ts \ src/renderer/src/hooks/remote-workspace-session-merge-close-tombstones.test.ts To reproduce the bug this fixes, set `isSuppressedByClose` to `() => false` in `remote-workspace-session-merge.ts` and run `pnpm test:e2e:ssh-docker -- tests/e2e/ssh-lost-kill-tab-resurrection.spec.ts --repeat-each=2`. |
||
|
Neil
|
e06a8667a9 |
fix(terminal): do not seed or resume while the execution host has not answered (#16750)
Two client behaviours read local tab rows as the verdict on what the execution host is running. Before the host answers, "I hold no pane for this" is `unverifiable`, not `exited` -- the collapse `docs/reference/ssh-execution-boundary.md` forbids. Symptom 1, seeding. `worktree-initial-terminal-seeding.ts:47,128` seeds a terminal when `renderableTabCount === 0`. Its only bail-out (`:72-77`) covered the paired-web-runtime flavor -- "while that session is live the host owns terminal creation" -- with no equivalent for direct SSH. So a client that has never held the workspace runs the predicate during the hydration gap and creates a tab from nothing. The snapshot then arrives, the merge rightly keeps the tab it was never told about, and the union uploads as the new host truth. Measured on a fresh client against a host owning 3 tabs: **1 tab created from nothing, 0 of the host's 3 adopted.** (A restart never reaches the predicate -- local state restores the row first -- which is why restart-only repros came back flat.) That guard was also the wrong question. It asked "am I a client of a live paired session?", which a host desktop window answers "no" and a paired client answers "yes", so both seeded -- #15556. Symptom 2, sleeping-agent resume, and the data-corrupting half. `Terminal.tsx:1554` calls `resumeSleepingAgentSessionsForWorktree` twenty lines after the seeding call at `:1529-1534` -- same startup path, same pre-hydration window, and not SSH-gated at all. Seeding produces a spare empty tab; the sweep launches `claude --resume <id>` for a session still running on the remote and still owned by a live pane. Two agent processes writing one transcript; STA-3498 observed five. STA-3500 files exactly this race. Failure is asymmetric: declining to resume is user-recoverable, a duplicate resume corrupts a transcript irreversibly. `workspace-terminal-host-authority.ts` answers the one ownership question both paths ask, in the three-verdict vocabulary the renderer already uses for host terminal inventory (`HostLiveTerminalProbeVerdict`, aliased rather than restated so the two cannot drift): `live` (a remote host owns creation here), `unverifiable` (there is a remote host and it has not answered), `none` (local, or the host answered and holds nothing). Seeding requires `none`; the sweep declines on `unverifiable` without consuming its one-shot, so the agents are not stranded for the session once the verdict lands. Shape notes: - An ownership question, not a client-liveness one -- that is what fixes #15556. - Folder workspaces resolve to `none`: the snapshot replaces exactly `DirectSshTargetScope.gitWorktreeIds`, so a folder's rows are never replaced by the host and waiting for an answer that will never name them would leave it terminal-less for good. - A `conflict` sync phase is `unverifiable`, matching the pair `use-app-session-persistence.ts` already gates uploads on. - Explicit launch work (setup/issue commands) stays ungated -- that is a request to create a terminal now. - `Terminal.tsx` subscribes through a retained selector rather than reading in the effect: the verdict flipping to `none` is what must re-run the passes, and resolution walks the owner catalogs, so recomputing per store write would be the STA-3363 render-path multiplier again. The `unverifiable` verdict is BOUNDED, and must be. `remoteWorkspaceHydratedTargetIds` is add-only in practice -- `markRemoteWorkspaceHydrated` has two production call sites, both on success paths, and `clearRemoteWorkspaceHydrated` has NONE. Four paths return without marking: local-hydration timeout (`remote-workspace-target-sync.ts:136-145`), a null `remoteWorkspace.get` (`:160-169`), a falsy apply token (`:172-185`), and never connecting at all. Without a floor, any of them would leave every git worktree on that target `unverifiable` for the rest of the app session: no initial terminal, no sleeping-agent resume, escapable only by creating a tab by hand. That is strictly worse than the behaviour it replaces -- on main the user got a terminal. So a sync that terminates in `offline` or `error` without ever hydrating resolves `none`: declining to seed is meant to be a wait, not a permanent refusal. `pulling` still declines, and a target that HAS hydrated stays `none` even if a later sync errors. Scope, stated because the doc comment previously overstated it: this gate is first-hydration-per-target, not per-connection-generation. Since nothing clears the flag, a disconnected target that hydrated once reads `none`. It does not cover mid-session reconnect or sleep/resume. The memo's input list is checked for COMPLETENESS, not just membership. `satisfies readonly (keyof State)[]` only proves each listed key exists; a field added to the state and forgotten from the list would type-check while making the memo return a stale verdict -- silent, and it looks like "the gate did not fire". A conditional type now names the missing key at compile time. Deliberately not `const x: Missing[] = []`, which passes regardless because an empty array literal is assignable to every array type. Known limitation, stated rather than hidden: the SEEDING half of this change has no measurable end-to-end effect today, and the branch's own e2e spec says so. `applyDirectSshRemoteWorkspaceSnapshot` calls `markRemoteWorkspaceHydrated` unconditionally AFTER the hydrate calls -- including when they wrote nothing. So in the same tick adoption yields zero, the verdict flips `unverifiable` -> `none`, `Terminal.tsx` re-runs the effect, and it seeds. The gate cannot outlive the failure it guards against, because the same function that fails to adopt is the one that lifts it. `ssh-cold-hydration-gap-tab-seeding.spec.ts:218` is named for what it asserts -- one tab, adopted none -- rather than for the behaviour we want. The fixme at `:293` pins the intended behaviour. Making the seeding half effective needs hydration resolved PER WORKTREE (or a refusal to say `none` when the completed apply's `replaceWorkspaceKeys` did not name this worktree) rather than a per-target "some apply finished" flag. That is deliberately not in this commit. The RESUME half is the valuable half and is unit-proven: it declines while the host is unanswered and wakes the same session once the verdict lands, without consuming its one-shot. Preventing one duplicate `claude --resume` on a live transcript is worth more than preventing one spare tab -- declining to resume is user-recoverable, a duplicate resume corrupts a transcript irreversibly. Before: 7 failed | 3 passed. After: 10 passed; 103 across the seeding, resume, authority and remote-workspace suites. |
||
|
Neil
|
971d987c4b |
ci(e2e): trigger the Docker-SSH lane from SSH source and claim every gated spec (#16746)
The Docker-SSH e2e lane only ran when a PR's changed specs happened to include
`ssh-startup-exec-readiness.spec.ts` or `paired-startup-exec-readiness.spec.ts`.
Editing SSH source itself did not trigger it, and pruning either spec from a
route's list would have silently retired the whole lane. Meanwhile the sharded
lanes set no `ORCA_E2E_SSH_DOCKER`, so every Docker-gated spec skipped itself
while the shard still reported green -- the exact silent-skip shape
`docs/reference/ssh-reconnect-source-recovery.md` blames for four regressions
that reached users.
Separately, the modules that actually own direct-SSH workspace and tab restore
carry no "ssh" in their names, so the `ssh-terminal-source` route never reached
them. Measured on the real script before this change:
printf '%s\n' src/renderer/src/hooks/remote-workspace-session-merge.ts \
src/main/ipc/remote-workspace-snapshot-normalization.ts \
src/renderer/src/lib/worktree-initial-terminal-seeding.ts \
src/shared/remote-workspace-session-projection.ts \
| node config/scripts/pr-e2e-source-routing.mjs
=> []
Three changes, all pinned by the executable gate contract:
- `hasSshSourceChange` derives an `ssh_source_changed` signal from the SSH
routes themselves, plumbed pr.yml -> e2e.yml, so the lane triggers on source
rather than on a spec name surviving in a list. One list, so the two cannot
drift.
- A sibling `ssh-workspace-session-restore` route names the restore seams
(`remote-workspace-*`, `worktree-initial-terminal-seeding`,
`worktree-default-terminal-tabs`, `initial-terminal`) and routes them to the
two restore specs -- a sibling rather than more paths on `ssh-terminal-source`
so a tab-tombstone edit does not run the whole SSH terminal list.
- A new `test:e2e:ssh-docker` runner claims the remaining Docker-gated specs on
the one VM that sets the flag, and the contract now fails by name when any
Docker-gated spec is claimed by no runner. `ssh-docker-relay-perf` and
`ssh-codex-display-artifacts-repro` are recorded exemptions (wall-clock
budgets; needs a real remote codex binary) and the contract asserts each
exemption still corresponds to a real gated spec, so a stale one cannot
quietly excuse a gap. Lane timeout raised 35 -> 60 minutes for the added
serial specs.
The lane's first act was to surface four latent bugs in a spec that had been
silently skipping. `ssh-docker-bulk-open-freeze-repro.spec.ts` is four call sites
out of date against `tests/e2e/helpers/terminal.ts`: `startDockerSshRelayTarget()`
is called with no argument though the helper dereferences `testInfo.workerIndex`
(a 100% failure, not a flake), `execInTerminal` gained a `ptyId` parameter, and
`splitActiveTerminalPane` gained a direction. It was invisible because it ran
nowhere and `typecheck:e2e` is red on main with 240 pre-existing errors, so four
more could not be seen.
The `testInfo` bug is fixed here -- correct on its own, and it removes one real
error from `typecheck:e2e` (240 -> 239). The other three are not, because they
are not argument plumbing: repairing them requires choosing which ptyId to
capture and which split direction to use, and both change what the repro
measures.
The spec is therefore added to the exemption list rather than repaired, for two
independent reasons recorded in the runner: it is a perf oracle, not a
correctness one (`SOFT_FREEZE_LAG_MS=2500` / `HARD_FREEZE_LAG_MS=5000` measured
under a deliberate 5-pane flood on a 420s budget -- the same rule already applied
to `ssh-docker-relay-perf.spec.ts`), and it is known-rotted. Repair is tracked in
stablyai/orca#16764. Applying an existing written rule to a sibling that plainly
meets it is consistency; inventing a new exemption to dodge a red would not be.
Three hardening fixes to the contract itself:
- Runner text is comment-stripped before the claimed-by-a-lane scan. A substring
scan over raw text lets a spec merely *discussed* in a runner comment count as
claimed -- the silent skip this assertion exists to catch, re-entering through
the documentation. Not live today only because the existing comments write the
spec names without their `tests/e2e/` prefix.
- An exempt spec must not be invoked by any runner. `unreachableSpecs`
short-circuits the unclaimed check, so a spec could be documented as exempt
while a runner still ran it -- an exemption that reads as coverage removal but
changes nothing, leaving the lane red for a reason the file says it excluded.
This is not hypothetical: adding the bulk-open exemption without removing it
from the runner's spec list produced exactly that state, and this assertion is
what caught it.
- The Docker-gate detector is now `/ORCA_E2E_SSH_DOCKER\s*[!=]==\s*['"]1['"]/`
rather than one fixed string, so a double-quoted or `!==` spelling can no
longer escape the contract.
`ssh-restart-tab-accumulation.spec.ts` is a new three-cycle restart fence
asserting tab-id set identity, not just the active pane's reclaimed ptyId as
`ssh-cold-activation-restore.spec.ts:241` did. It passes today; it was validated
by a negative control that injected one tab after cycle 1 and correctly failed.
|
||
|
Neil
|
ebe6b559ef | Prime changed native caches before test fanout (#16918) | ||
 Jinjing
|
9e0704b9d4 | fix(ci): prepare node runtime for e2e build | ||
|
Neil
|
350423b7cb |
Speed up PR CI with path skips, native caches, and fewer shards (#16863)
* Speed up PR CI with per-job path skips and native caches Skip git-compat, xterm, packaging, and shell jobs when their inputs are unchanged, reuse the composite install action (including Windows node-pty cache), skip compiling the Windows CLI launcher on a cache hit, and cut the test matrix from 16x2 to 8x2 shards without dropping coverage. * Widen PR job skip prefixes for orcad browser and live shells Chrome session/tab modules and zsh/fish wrapper templates are inputs to required jobs the classifier previously skipped. Include that implementation graph so those jobs still run when the files they load change. * Fix CI cache safety and required gates * Build scriptless Windows addons explicitly * Preserve node-pty Windows support prebuild * Remove duplicated Windows launcher unit lane |
||
|
Jinjing
|
026389a3bc |
Suppress default quit on window-all-closed in DNS probe (#16683)
* Suppress default quit on window-all-closed in DNS probe Destroying the probe window awaits stopLogging, which yields to the event loop long enough for the default window-all-closed exit (non-macOS) to trigger before the result can be written. Preventing this default behavior allows the result to complete and exit cleanly. * Preserve Japanese Skills UI labels; fix test flakiness and deps - Add skipKeyPrefixes filter to Japanese phrase fixes to prevent automatic translation of UI labels (e.g., keep エージェント in Skills components) - Wrap timing-dependent tests in vi.waitFor to eliminate race conditions during reconciliation and scheduler boundary ticks - Complete useEffect dependency arrays to resolve React hook warnings * Fix updater startup scheduling test flakiness Set last update check to 23h ago instead of null to make timing deterministic. The startup check arms its own 24h timer; by pre-setting the last check time, only the result handler's re-arm can produce the expected check 24h later, eliminating race conditions. |
||
|
Neil
|
5631aa00dd |
feat(orcad): items 2–7 — degradation, natives, daemon, ops, deploy (#16398)
* fix(ports): stop joining an undefined resourcesPath on a non-Electron host `resolveWorkerEntryPath` branched on `isPackaged` alone and joined `process.resourcesPath`. orcad reports `isPackaged` true — correctly, it is a production build, and ~15 consumers read it that way to gate HTTPS-only skill downloads and the real CLI name — but `process.resourcesPath` is Electron-only and `undefined` under plain Node. So the packaged branch threw `TypeError [ERR_INVALID_ARG_TYPE]: The "path" argument must be of type string` where a clean "worker unavailable" was the honest outcome. The type said `resourcesPath: string`, which is how it went unnoticed; it is now `string | undefined`, so the compiler carries the fact. A host with no Electron resources tree has no asar to look in, so it falls back to the module directory and lets the caller report a missing worker. Found by the item 1 agent while auditing the same `isPackaged` defect class in the watcher. Verified in both directions: reverting the guard reproduces the TypeError. * feat(orcad): prove node-pty loads before anything requires it Of the two ways node-pty fails, only one is catchable. A missing module throws MODULE_NOT_FOUND. A module built against the wrong libc or Node ABI is refused by the dynamic loader, and in the worst case takes the process down before any handler exists — that is #9902, which crashed the desktop app on Ubuntu 20.04 before a window appeared. There was no libc or ABI precondition anywhere in the tree. So orcad now proves the load in a CHILD process, from main.ts, before anything requires node-pty. Whatever the child does — throw, abort, die on a signal — is data rather than our own death, and the operator gets a sentence naming the host's libc, Node ABI and prebuild slot plus the command to run. Proven-unloadable exits 78 (EX_CONFIG), so a supervisor does not restart an unequippable host forever. A probe that never answered is unverifiable, not blocked: refusing to boot on an inconclusive signal would take down hosts that work. The child dlopens the file node-pty would have chosen, before requiring the package. node-pty's loader walks several directories and rethrows only the LAST error, so a refused binary reads as "Cannot find module ./prebuilds/..." — which sends the operator to install a module that is already there. It also reports through stdout: node echoes the whole -e source above a stack trace, and matching tokens against stderr made the probe's own source text answer for the verdict. Verdicts reach clients as a terminal_unavailable degradation alongside the existing browser_unavailable one, through the same cause-registry shape. degradations[].code is now an open vocabulary; clients already render only `message`. Prebuilds are compiled from PATCHED sources — the patch IS the glibc-floor fix, so an upstream tarball reproduces #9902 — into linux-{x64,arm64}-{glibc,musl} and darwin-{x64,arm64} slots. libc is in the slot name because node-pty's loader falls back to prebuilds/<platform>-<arch> and cannot tell glibc from musl. orcad installs the matching slot at boot, so a host with no compiler serves terminals. The relay's five pure toolchain-diagnosis functions moved to a transport-free module so the Node bundle can reuse them without dragging ssh2 in behind them; the relay keeps its API by re-export. macOS gets `xcode-select --install` rather than the cross-distro apt/dnf/pacman/apk menu, every line of which is wrong there. * test(orcad): pin the node-pty precondition to ground truth, not a prepared host CI's test shard runs `vitest` directly, so `ensure-native-runtime --runtime=node` never prepares node-pty for the Node ABI — `degraded` is the correct verdict there, and asserting 'ok' encoded an environment the shard does not have. Asserting whatever it returned would be vacuous, so the expectation is now derived from an independent require() of node-pty. Verified it still bites: forcing the precondition to always report 'ok' fails the suite. * feat(orcad): run the terminal daemon, and the ops contract around it orcad declared `canRecoverPersistentLocalPtys: () => false` because it did not run the terminal daemon, so every restart, update and rollback SIGKILLed every running terminal — on the host whose selling point is that work survives the client going away. That is the one property `ssh-execution-boundary.md` recommends the peer model for. Item 4 — the daemon: - Port the launch path off electron: `daemon-init.ts`, `daemon-host-relocation.ts` and `observability/logs-directory.ts` now read the `AppEnvironment` port. Relocation additionally asks whether the app root is an asar archive rather than whether the build is packaged, so a Node host answering `isPackaged() === true` no longer walks into an Electron-only NSIS-escape path (same precedent as `parcel-watcher-entry-path.ts`). - `build-orcad.mjs` emits `daemon-entry.js` beside `orcad.js`, scans the forked children's metafiles for electron/node:sqlite, and load-checks the child under plain Node. - orcad spawns and adopts the daemon; shutdown disconnects and never kills it. `canRecoverPersistentLocalPtys` now reads the live provider and is false under degraded routing, where fresh terminals would die with the process. Item 3 — the ops contract (docs/reference/orcad-operations.md): - Bind policy: `--bind`, default loopback, pinned so neither `orca serve`'s wide default nor the connected-device widen can override it, and so a paired client cannot rebind the listener from outside. - Instance lock on the data root before profile load, scoped to the runtime role so it never refuses a restart that a live daemon makes worthwhile. - Supervision: exit codes a supervisor can act on (78 = do not retry), second-signal escalation, a shutdown deadline, and crash-loop containment on daemon respawn. - Health in the readiness payload: build hash, Node ABI, and a PTY self-test that spans both processes — the daemon spawns a real PTY in its own process and the verdict crosses its socket. Both bundle load-checks now assert on exit codes: these bundles are minified onto one line, so Node's uncaught-exception report echoes every string literal in the bundle and the previous message match passed against a bundle that never loaded. * feat(orcad): deploy, activate and roll back a versioned orcad install Plan items 6 and 7 from docs/design/shipping-orcad.html. Install reuses the relay's transaction verbatim — per-version lock, staged SFTP write, .install-complete sentinel, stale-lock recovery — under a parameterized namespace, so orcad-<v>/ sits beside relay-<v>/ permanently (§06). Parameterizing GC is the trap that creates: each model now collects only its own directories, enforced twice (prefix-scoped remote listing plus a local ownership re-check), and a client picks its model from how the host is registered, never from what it finds on disk. Activation is separate from installation, because a versioned directory selects nothing. A candidate is launched, publishes orca_server_ready, and only becomes active if its cross-process health payload passes: right build hash, listening, daemon live, PTY self-test green. A rejected candidate is stopped and the incumbent restarted, so a careful deploy cannot cause the outage it was being careful about. Update and rollback are shaped by the daemon. An update restarts orcad, the daemon outlives it, and the surviving daemon was forked from the outgoing bundle — so live terminals defer the update rather than proceed, and GC pins the active version, the rollback target and the live daemon's bundle. Orca's persisted state carries no schema version, so rollback restores a pre-activation snapshot rather than trusting backward-readability; the point past which it is unsafe is the first terminal created after activation, which the snapshot cannot describe and the surviving daemon still owns. Running the generated shell for real found two bugs the text assertions missed: tar members re-quoted inside a shell variable captured nothing, and kill -0 reports a zombie as alive. * test(orcad): assert the precondition is self-consistent, not environment-shaped The real-host case cannot predict a status: CI's shard runs vitest directly, so node-pty is never built for the Node ABI and 'degraded' is correct there, while a prepared checkout gives 'ok'. The previous attempt used require('node-pty') as ground truth, which resolves the JS wrapper while the native binding loads lazily — it proved strictly less than the precondition checks, and failed CI for exactly that reason. What is invariant on a host with node-pty installed: never 'blocked', and never a degraded verdict carrying an unestablished reason. The injected-input tests keep the logic coverage. * fix(orcad): drop an eslint-disable the rule no longer needs * test(orcad): separate slot placement from the load verdict Both remaining CI failures were the same shape: tests reaching into node_modules for a pty.node that only exists after `ensure-native-runtime --runtime=node`, which CI's shard never runs because it invokes vitest directly. Slot *placement* is the logic worth checking on every host, so it now uses a synthetic payload and asserts the verdict stays honest about not loading. The three assertions that genuinely need a Node-ABI binding are gated on it existing. Verified: breaking slot installation fails both placement tests; with the real pty.node hidden the file is 17 passed / 3 skipped instead of ENOENT. * test(orcad): gate the load-dependent cases on a real load, not on the file existing CI ships a pty.node built for Electron's ABI, so existsSync was true while require still failed — the gate ran exactly the tests that host can never satisfy. It now probes the binding in a child process, so a bad one cannot take the runner down. The self-consistency assertion also allowed too little: 'blocked' is the honest verdict for a corrupt binding, alongside 'ok' on a prepared host and 'degraded' on an unprepared one. What stays invariant is that anything other than 'ok' names an established cause, so a terminal is never declined for a reason nobody worked out. Verified against all three host states: prepared (19 passed), unprepared, and a corrupt binding (17 passed / 3 skipped, no failures). * test(orcad): gate on the whole premise — binding AND spawn-helper CI has a loadable pty.node but no spawn-helper, and a slot without the helper is legitimately 'degraded'. So the previous gate let a test run whose premise ('a complete slot yields ok') that host cannot satisfy. Verified in both states: with the helper present 19 pass; with it removed the load-dependent cases skip (17 passed / 3 skipped) instead of failing. * fix(orcad): preserve degradation types after rebase |
||
|
Neil
|
0096e47850 |
fix(windows): keep windows-process-tree gyp paths absolute under pnpm (#16688)
* fix(windows): keep windows-process-tree gyp paths absolute under pnpm Hourly Windows builds have failed since #16598 at `build-windows-process-tree-relay-addon`: `require('node-addon-api').targets` is cwd-relative, so node-gyp evaluates it from the pnpm store realpath and then loads it from the `node_modules` symlink. That resolves `node_addon_api.gyp` outside the repo. Use `require.resolve` for an absolute path, matching the node-pty patch. * i18n: keep ja skill-filter labels on the catalog's Agent brand #16682 merged with a failing localization catalog: ja used エージェント in three new skill-filter strings, and repair-locale-catalog rewrites those to Agent. Match the rest of ja.json so static analysis can pass. |
||
|
Neil
|
64c992cd56 |
fix(memory): report the Windows number that predicts paging, not just resident pages (#16211) (#16589)
* fix(memory): report Windows commit charge, not just working set (#16211) On Windows the per-process figure was working set — resident pages only. An agent whose pages Windows has trimmed to the pagefile shrinks its working set while still holding the commit that pushes the host into paging, so Resource Manager and `orca diagnostics memory` understated an owned tree by 10-40x (9 codex.exe: 1.4 GB working set, 13.4 GB private) and could not warn before the host was already thrashing. Add committed private bytes as a second, separately-labelled quantity rather than redefining the existing one: - CIM sweep gains one property (PageFileUsage, UInt32 KB); the typeperf fallback gains one counter (\Process(*)\Private Bytes). Both ride the sweep that already runs. - MemorySnapshot gains optional `privateMemory` per app/worktree/session plus `processCommitMetric` and `totalPrivateMemory`. Rule 1 additive optional fields: old clients ignore them, and absence reads as "not measured", never as zero — Unix hosts and older hosts send nothing. - `totalMemory` and `processMemoryMetric` keep their exact meaning, so the "shared pages may repeat" copy stays true; the working-set copy now also says paged-out memory is not counted. - Resource Manager shows "Σ Private" beside "Σ WS", and tints the badge yellow/red once tracked commit passes 60/80% of physical RAM — the same thresholds `usageTextColorClass` already uses for host usage. Tint and tooltip only; no toast, and the badge number is unchanged. The parsers move to windows-process-sample-parsing.ts and the Windows sweep tests to their own file to stay under max-lines. Not migrating the collector to windows-process-table.ts: the native snapshot exposes no commit figure and no CPU times, and truncates WorkingSetSize through a DWORD. Documented in the enumeration reference. * fix(memory): derive the typeperf field cap from the counter list The fallback parser's 8192-field cap was sized for three `\Process(*)` counters. Adding `Private Bytes` cut the parsable process count from ~2730 to ~2047, and overrun is a blackout (`parseTypeperfCsvLine` returns `[]`, so the whole sweep reports nothing) rather than a truncation. The counter list now lives beside the decoder that reads those names back out of the PDH header, and the cap is derived from it. Also collapses the four spellings of "omit privateMemory when unmeasured" in collector.ts onto one `commitField` helper, drops the unread parameter and the never-rendered `columnLabel` from `getResourceCommitMetricCopy`, folds `getCommitPressurePercent` into the only function that called it, and reverts unrelated Prettier churn in the Windows enumeration doc. The commit tint's doc comment no longer claims to predict host paging: it measures Orca's own share of physical RAM. Host commit charge / commit limit stays a follow-up (#16211). |
||
|
Neil
|
19e9ec695b |
perf(windows): ship the native process table to Windows relay hosts (#16598)
* feat(windows): let a relay host bind the native process table directly The CIM fallback from #16550 answers on relay hosts, but it costs a powershell.exe and ~1.4s per scan where the native reader costs ~57ms. It is a parachute, not the destination. Teach the loader a second source: the desktop app keeps resolving the npm package, and a relay host -- which has none of our node_modules -- binds a bare `windows-process-tree.node` staged beside the bundle. The CIM scan stays as the last resort, so a host with neither is unchanged. Bind the addon directly rather than its package wrapper. lib/index.js adds only a queue over getProcessList, and that queue is the wedge this module already defends against: it latches a module-global requestInProgress with no try/catch. We hold our own single-flight and deadline, so going straight to the addon drops the duplicate. Measured on a Windows 11 SSH host with ~1490 processes, running the relay-externals bundle from the deployed relay directory: no addon staged nativeAvailable=false 1247ms (CIM) addon staged nativeAvailable=true 57ms memory restored Degradation was exercised on that host, not just in fakes: a truncated upload, a text file, and a foreign-arch ELF each fall through to the scan rather than throwing, and restoring a good addon recovers. A file that loads but lacks getProcessList is rejected by shape, because binding to it would reject every read forever where falling through still answers. No artifact is staged yet, so this is inert until the packaging change lands: today every relay takes the same CIM path it does now. * build(relay): ship the Windows process-table addon to relay hosts The CIM scan restored correctness on Windows SSH hosts, but it costs a powershell.exe and ~1.4s per read where the native addon costs ~57ms. It was always the floor, not the destination. The addon cannot be npm-installed on a relay host: it carries a binding.gyp, so npm rebuilds from source and the build wants Spectre-mitigated libraries even where MSVC is already present. The binary inside the published tarball loads, but predates our patch and still caps enumeration at 1024 processes -- on a 1486-process host it returned exactly 1024 rows with the querying process among the missing, which reads as unavailable only under load. No published alternative clears the bar either; the one fork with a working prebuild story still carries the same cap. So build it where a compiler exists and ship the result. The build script refuses unpatched source -- checking the source rather than trusting the install, because the Spectre hunk fails loudly while the 1024 hunk fails silently -- and verifies the PE machine field so a cross-build cannot emit host arch for another target. The artifact is optional: hashed when present so a relay carrying it never shares an immutable directory with one that does not, and never probed, since requiring a file only a Windows build machine can produce would make a correct relay read as MISSING and redeploy forever. Builds on any other OS keep using the scan, unchanged. arm64 cross-compiles from the x64 runner but needs the optional MSVC ARM64 toolset, so it stays best-effort: a runner image without that component should cost arm64 relays the fast path, not fail the release the x64 relay is riding on. ORCA_REQUIRE_RELAY_NATIVE_ADDONS is a per-arch list rather than a flag for exactly that reason. * build(relay): require the arm64 process-table addon too The arm64 cross-compile is no longer unproven. On a Windows x64 machine with the MSVC v143 ARM64 build tools component installed, node-gyp --arch=arm64 produces a genuine ARM64 image: x64 machine=0x8664 152064 bytes arm64 machine=0xaa64 139776 bytes So arm64 stops being best-effort and joins x64 in the required list. It was only best-effort because the component is optional and I had not seen it succeed; a runner image without it now fails the build with MSB8020 naming the missing component, and that step runs before the long packaging step so the failure costs seconds rather than twenty minutes. The env var stays a per-arch list rather than reverting to a flag, so a future arch can land best-effort before being promoted the same way. |
||
  Jinwoo HongandJinwoo-H
|
a9781a4118 |
STA-4150: client-hosted remote browser (consolidated) (#15448)
Co-authored-by: Jinwoo-H <jinwoo@stably.ai> |
||
|
Jinjing
|
e361da7fb7 |
Deleting skill (#16357)
* Add skill deletion with cross-platform transaction safety Implements end-to-end skill removal with placement enumeration, dependency guards, and transactional recovery. Covers native, WSL, and remote hosts; users can delete canonical directories and alias placements (symlinked directories or files) in a single atomic batch. Includes UI selection flow, preview, confirmation, and results band. Block reasons (bundled, plugin, unowned, stale) gate deletions that would fail or contradict user intent. * Organize IPC handlers into module subdirectories Move register-core-handlers and skill-delete-ipc-handlers into dedicated subdirectories for improved code organization and to reduce the flat structure in src/main/ipc/. * Make skill deletion recovery transactions idempotent Defer journal cleanup until both staging removal and receipt cleanup succeed, leaving the journal in place for startup to retry if either operation fails. This ensures the recovery process is safe to run multiple times without leaving partially-deleted skills. * Consolidate skill-delete files into dedicated module Reorganize skill deletion functionality into a modular structure under `src/main/skills/skill-delete/` with simplified file names. Remove the redundant `skill-delete-` prefix from file names since they now live in the dedicated directory. Update all import paths throughout the codebase to reflect the new structure, including imports from IPC handlers and RPC methods. * Fix broken import paths and add deletion robustness improvements Import paths using `..//'` were invalid and broken. Replace with explicit module names (`skill-discovery-sources`, `skill-install-filesystem`, etc.) to clarify dependencies. - Bind WSL filesystem methods to preserve `this` context - Keep recovery journal when rollback rename fails, so startup can retry - Skip symlink-based tests on Windows where they cannot run - Only treat ENOENT/ENOTDIR as empty directories; propagate other errors - Fix cross-platform path parent calculation to handle drive roots - Replace shared constant with localized string for user-facing message - Use `runProcess` for WSL integration test instead of bare `execFile` * Add batch limit for skill deletion and improve host availability checkin - Limit concurrent deletions to prevent remote host overload - Add retry logic for capability probing to handle transient unavailability - Add reprobe() method to recheck capability after errors or user refresh - Fix status logic: receipt cleanup is best-effort, completion depends only on content removal - Improve error message for unreachable hosts |
||
|
Neil
|
6faaf3af74 |
fix(lint): match moved code by ordered near-match, not strict contiguity (#16385)
A diagnostic's span often reaches past the block a split moved — most commonly to a hook dependency array, which legitimately grows when closure variables become props. Requiring every line of the span to match contiguously reported the moved body as new. The block must still start at the same line in the base and appear in order, and >=90% of it must be present. Genuinely new code shares neither the anchor nor the ordering. |
||
|
Neil
|
20c1a61401 |
fix(orcad): answer host paths honestly and ship the watcher child (#16369)
orcad's AppEnvironment implemented three of seven AppPathNames and returned the userData directory for the rest — including 'exe', where a data directory is not an executable. Every name now has a Node answer: 'appData' is the platform's per-user application-data root, 'logs' lives inside the data root so a headless deployment stays one removable directory, 'downloads' honours XDG_DOWNLOAD_DIR, and 'exe' is the Node binary. getAppPath() is the directory orcad was launched from rather than cwd, so children resolve against the bundle instead of wherever the supervisor happened to be. The watcher child was the load-bearing consequence: resolveWatcherProcessEntryPath probed for the adjacent entry only when !isPackaged, so orcad resolved a desktop out/main path that no deployment has — and build-orcad never emitted the child anyway. isPackaged stays true (consumers read it as "production, not a dev checkout" and it gates HTTPS-only skill downloads); the resolver now asks whether the app root is an asar archive, which is the question it actually meant. The child ships beside orcad.js, and the build forks it to prove it runs. |
||
|
Neil
|
33c9353f29 |
fix(lint): exempt verbatim-moved code from the changed-lines quality gate (#16359)
A file-splitting refactor makes every line of the new module an added line, so pre-existing lint debt in code that merely moved starts failing the gate. The only way to satisfy it is to edit the moved code, which is what a behavior-preserving refactor must not do. Exempt a diagnostic when its highlighted lines already existed verbatim and contiguous in the base revision. |
||
|
Neil
|
e217fdd10f |
build(orcad): gate orcad's own graph, and prove it loads under plain Node (#16368)
* fix(orcad): close the browser-provider gaps The providers landed without enforced coverage, so a regression in either path would have landed silently. - CI: the external-Chromium integration test was gated on ORCA_BROWSER_EXECUTABLE and nothing ever set it, so it skipped forever. It now runs in its own job against the runner's Chrome and FAILS when Chrome is absent rather than skipping, because an unset variable is exactly how it went uncovered. Timeout raised to 120s: a warm run is ~7s but the first launch against an unseeded profile took 30s and hit Vitest's default, and CI is always that cold case. - Electron provider had no test at all. It is the path anyone with the desktop app hits. - Browser unavailability reported one message for four causes, including telling an operator to set a variable they had already set. Fixes a live defect found while covering it: the runtime advertises browser.tabCreate.known-id.v1 unconditionally, so a web client sends a provisional page id for a page that does not exist yet — and the sidecar's generic requestedPageId branch ran require() on it first and threw. Every known-id create against the Electron provider failed. The adoption logic was already there; only the ordering was wrong. Also updates the workflow-parallelism guard, which correctly caught the new job missing from verify's required-check list, and asserts verify actually reads it. * build(orcad): gate orcad's own graph, and prove it loads under plain Node Two gaps the artifact's own comment asked for. The ratchet measured only orca-runtime + runtime-rpc, but orcad imports ipc/pty directly to install the PTY controller, so its graph is strictly larger. The gate could read zero while the shipped artifact regressed. orcad's entry is now a ratchet entry point, and the baseline stays empty with it included. orcad cannot join plain-node-entry-guard — that is a rollup plugin keyed on electron-vite input names, and orcad is an esbuild artifact. But the half that matters here is the guard's smoke-load: scanning the metafile proves no module NAMES electron, not that the graph resolves under plain Node. A dynamic require, a missing native or a top-level throw all pass the scan and fail at runtime. build-orcad now runs the bundle with a bogus flag and requires the argv rejection that only a fully loaded graph can produce. Verified: a bundle that builds but throws on load fails the gate. |
||
|
Neil
|
f7033e0e70 |
build(windows): drop the packaged node-pty prebuild that can silently replace the patch (#16350)
* build(windows): drop the packaged node-pty prebuild that can silently replace the patch node-pty's loader tries build/Release, then build/Debug, then prebuilds/<platform>-<arch>, and swallows every failure in between. Windows packaging ships both the source build and the prebuild, and only the source build carries Orca's job-object exports (listJobProcessIds, terminateJob, assignCurrentProcessToJob). So an ABI mismatch, a truncated file, or an AV quarantine of build/Release/conpty.node degrades the shipped app to the UNPATCHED prebuild: PTY teardown silently falls back to guessing by PID ancestry, with no error anywhere. That is the failure mode that made #16059 hard to see -- an install that looks fine and quietly cannot own a PTY tree. Removing the fallback turns a silent downgrade into a loud load failure. Scoped narrowly: only win32, and only when the source build is actually present, so a build that legitimately has no build/Release keeps something loadable. macOS and Linux prebuilds are untouched -- they have no patched export to lose. Refs #16059. * fix: delete only the stale conpty fallback, not the whole prebuilds tree Review caught a P0 in the first version of this change, and it was the same defect the PR exists to prevent, pointed at a different target. Orca's own patch removes the `conpty_console_list` and winpty `pty` gyp targets, so a Windows source build emits conpty.node and nothing else. conpty_console_list.node, pty.node, winpty.dll and winpty-agent.exe therefore exist ONLY in prebuilds/. Deleting the tree removed them: - the forked console-list agent throws at require, and its caller resolves null with silent: true, so console-membership probing dies with no log anywhere -- a new silent degradation, in a PR whose thesis is "make it loud"; - node-pty still selects winpty below Windows build 18309, so PTY spawn would fail outright on Server 2019 / Win10 LTSC 2019. Now removes only prebuilds/win32-<arch>/conpty{.node,.pdb}, and only when electronArch matches the host arch -- a cross-arch package copies the host's build/Release, so its presence does not mean it matches the target, and deleting the target-arch prebuild would remove the only loadable binary. The old fixture wrote just conpty.node, so it could not see any of this. It now seeds a realistic prebuilds directory, and four tests assert each sibling survives; all four fail against the broad delete. Credit: review counsel. |
||
|
Neil
|
09048c63d4 |
feat(orcad): add headless browser providers (#16193)
* feat(orcad): add headless browser providers * fix(orcad): merge the duplicate runtime-browser type import |
||
|
Neil
|
2d500278b4 |
build(windows): refuse unpatched node-pty prebuilds
Merged after clean CI, Windows packaging verification, and readiness review. |
||
|
Jinwoo Hong
|
c618ec7393 | test(reliability): protect recent P0 regression invariants (#16163) | ||
|
Jinwoo Hong
|
fe6f942d1f | Fix Ubuntu release dependency lockfile gate (STA-5109) (#16055) | ||
|
Neil
|
838f5bfb75 |
fix(secrets): tell Linux users when their secrets are only obfuscated (#16033)
On Linux with no keyring, Electron falls back to the `basic_text` backend, which "encrypts" with a hardcoded password. `isEncryptionAvailable()` returns true for it, so Orca reported those secrets as sealed. They are not. The obvious fix — returning false for basic_text — is wrong and would have been a credential regression: `decryptWithStatus()` skips decryption entirely when encryption is unavailable, so every already-stored secret would read back empty. Sealing genuinely works on basic_text and must keep working. So capability and trust are now separate questions. `isEncryptionAvailable()` still answers "can this host seal and unseal", and `describeProtectionGap()` (renamed from `describeUnavailable`) answers "is my data actually protected", covering both no-sealing and weak-sealing. That method had no production caller — the port documented a promise nothing kept. `reportSecretProtectionGap()` now reads it at startup. A user-visible surface is follow-up; this at least stops the silence. Adds a bootstrap wiring guard over all nine host port installs. The no-op defaults are correct for a renderer-less host and silently wrong for the desktop, and a dropped or reordered install fails no existing test. Verified in both directions: it fails when an install is removed, and when one moves after the runtime is constructed. |
||
|
Neil
|
03fcfdfb92 |
feat(orcad): boot the Orca runtime on plain Node (#15968)
* refactor(host): resolve the app root through the port in fork-reachable modules
`parcel-watcher-entry-path.ts` and `session-scanner-service-entry-path.ts` read the
app root via `require('electron').app` inside a try/catch that already returns null
when Electron is absent. They were therefore correct under plain Node at runtime and
only failed the *static* text check — which is real, not pedantic: the comment in
`ports/port-scan-command-client.ts:19` records that the plain-node-entry-guard fails
on that literal text, try/catch or not.
`hasAppEnvironment() ? getAppEnvironment() : null` gives the identical "no app root
here" answer without the text. That restores `hasAppEnvironment`, which an earlier
commit in this stack deleted as unused — it now has the caller it was waiting for.
Ratchet baseline 27 → 25.
Verified: 74 files / 458 tests; `pnpm typecheck` clean; `oxlint` clean.
* feat(orcad): boot the Orca runtime on plain Node
Closes the last two Electron couplings and makes `orcad` a working artifact:
a 4.43 MB Node bundle that boots, pairs, registers a repo, creates a real git
worktree and round-trips a PTY — with zero `require("electron")`.
Ratchet 2 -> 0, so `config/runtime-electron-baseline.txt` is now empty and its
test asserts exactly that: any reachable electron import is a regression.
- speech: inject the service factories, so importing ModelManager for its type
no longer drags Electron's streaming net.request into the graph
- filesystem-watcher: add a WorktreeWatcherRemoval port. Every entry in those
maps arrives through an ipcMain handler carrying a renderer sender, so a host
with no renderer has nothing to close, restore or forget — the inert default
is what the desktop code does against empty maps, not a stub hiding work
- user-data-path / profile-storage-paths: resolve userData through
AppEnvironment. These surfaced only once orcad pulled the store in
Both host ports now anchor to a realm-global symbol. `vi.resetModules()` gives
the re-imported graph a fresh module copy, so a binding installed before the
reset silently read back as uninstalled.
The acceptance smoke drives both hosts through one code path (`--target
orcad|electron`) and seeds its own git repo, so it is hermetic and asserts the
same contract of each. Wired into PR CI.
* test(smoke): remove the seeded workspace container, not just the worktree
* test(smoke): surface the server's stderr when it dies before ready
* fix(smoke): build node-pty for Node before booting orcad in CI
* fix(smoke): drive the CLI built from this checkout, not one on PATH
* docs(ratchet): say the baseline must stay empty, not merely shrink
* build(orcad): externalize only the native modules actually in the graph
|
||
|
Neil
|
f975035809 |
refactor(ipc): split preflight and SSH registry out of the ipcMain modules (#15927)
* refactor(preflight): split agent detection out of the ipcMain registration
First of the IPC extractions the revised design requires. `src/main/ipc/preflight.ts`
mixed 285 lines of agent/tool detection with 35 lines of `ipcMain.handle`
registration, and the runtime calls that detection during normal operation
(`orca-runtime.ts:573`, plus the preflight RPC methods). So the runtime dragged
`ipcMain` into its graph to reach pure logic.
Detection moves to `src/main/preflight/agent-detection.ts` — named for what it
contains, per AGENTS.md. `ipc/preflight.ts` keeps only the handler registration and
re-exports the domain module so existing importers are unaffected. The runtime and
its RPC methods now import the domain module directly.
Ratchet baseline 36 → 35: `src/main/ipc/preflight.ts` is no longer reachable from
the runtime. The gate detected the improvement and refused to pass until the
baseline tightened, which is the behaviour it was built for.
Verified: 2 files / 1,187 tests pass across every suite touching preflight;
`pnpm typecheck` clean; `oxlint` clean.
* refactor(ssh): split the SSH target registry out of the ipcMain module
Second IPC extraction, and by far the biggest win: this removes **eight** modules
from the runtime's Electron graph, taking the ratchet baseline 35 → 27.
The runtime needed five thin accessors from `src/main/ipc/ssh.ts` —
`connectRegisteredSshTarget`, `getRegisteredSshState`, `listRegisteredSshTargets`,
`listRegisteredRemovedSshTargetLabels`, `getActiveMultiplexer`. Each is a one-line
read over module-level state. Importing them dragged in `ipcMain`, `powerMonitor`
and a `BrowserWindow` accessor — and, transitively, `ipc/pty.ts` (8,031 lines),
`ssh-browse`, `ssh-passphrase`, `ssh-relay-deploy`, `ssh-remote-cli-host-passthrough`,
`wsl-hook-relay-launch` and `user-data-path`.
`src/main/ssh/ssh-target-registry.ts` now holds that state plus its accessors.
`registerSshHandlers` populates it; the runtime reads it. The indirection is kept
deliberately: SSH providers register after construction and may reconnect, so
callers must resolve the current generation rather than freeze one.
`ipc/ssh.ts` re-exports all five, so non-test importers are unaffected.
`connectRegisteredSshTarget` still throws `ssh_handlers_not_registered` when no
handler layer registered — a headless host must fail loudly rather than report a
target as unreachable, which would read as `exited` (see ssh-execution-boundary.md).
Verified: 9 files / 59 tests across the ssh, automations and trust-preset suites;
orca-runtime.test.ts 1,183 pass; `pnpm typecheck` clean; `oxlint` clean.
* refactor(host): resolve the app root through the port in fork-reachable modules
`parcel-watcher-entry-path.ts` and `session-scanner-service-entry-path.ts` read the
app root via `require('electron').app` inside a try/catch that already returns null
when Electron is absent. They were therefore correct under plain Node at runtime and
only failed the *static* text check — which is real, not pedantic: the comment in
`ports/port-scan-command-client.ts:19` records that the plain-node-entry-guard fails
on that literal text, try/catch or not.
`hasAppEnvironment() ? getAppEnvironment() : null` gives the identical "no app root
here" answer without the text. That restores `hasAppEnvironment`, which an earlier
commit in this stack deleted as unused — it now has the caller it was waiting for.
Ratchet baseline 27 → 25.
Verified: 74 files / 458 tests; `pnpm typecheck` clean; `oxlint` clean.
* test(ssh): mock the SSH target registry alongside the ipc/ssh mock
Thirty-eight suites mocked `vi.mock('./ssh')` for `getActiveMultiplexer`. That
factory went inert when production started importing the accessor from
`../ssh/ssh-target-registry`, so the real module loaded and the assertions drifted.
Adds a companion registry mock returning the same stub, plus a
`sshTargetRegistryModuleMock` builder beside the existing `sshModuleMock` so the
shared harness stays one place. No assertion changed.
Found by a full-suite run: the targeted ssh/runtime suites were green while
30 tests in ipc/worktrees and ipc/repos were not.
* refactor(runtime): read app paths and the packaged flag through the port
`orca-runtime.ts` is the last module in its own graph that imports `electron`
directly. Nineteen of its uses were `app.getPath` (12) and `app.isPackaged` (7) —
exactly what the AppEnvironment port already covers.
Also removes a dead `const { app } = require('electron')` inside
`getOrchestrationDb`. It was left unused once the path came from the port, and it
is precisely the dynamic-require pattern `plain-node-entry-guard.ts` exists to
catch, sitting in the runtime's own constructor path.
What still binds `orca-runtime.ts` to Electron is now three sites, not nineteen:
`new Notification(...)` (one), `BrowserWindow.fromId` (one), and the
`ipcMain.on('terminal:tabCreateReply')` renderer round-trip — which is the browser
tab path, and the same one that would hang a headless host for ten seconds.
Two suites drove `electronMocks.app.isPackaged` directly; they now install a fake
AppEnvironment reading the same mutable field, so their per-test toggles work
unchanged and no assertion moved.
Verified: 376 files / 4,717 tests across src/main/runtime; typecheck and oxlint clean.
* test(serve): add the built-artifact terminal round-trip acceptance smoke
"The server started" proves almost nothing. Terminal creation dispatches into
OrcaRuntimeService, and without an installed headless PTY controller that path
falls through to a renderer reply that never arrives and times out after ten
seconds. A boot probe, a port bind, and a `host.platform` call all pass against a
server whose terminals are dead — which is exactly the gap the design doc's own
boot proof was retracted for.
This boots the BUILT `out/main/index.js --serve`, parses its ready payload, pairs a
real client over the advertised endpoint, lists worktrees, creates a terminal, runs
a command through the PTY, asserts the output comes back, and asserts clean
shutdown. It drives nothing but the public pairing + RPC surface, so the same
script is the acceptance gate a future Node-only backend must pass unchanged.
The sentinel invokes `process.execPath` rather than `echo`, because the shell
differs per platform and node does not.
Verified both directions: passes against the real server, and fails with an
actionable message when the command produces no output — a smoke that cannot fail
is worthless.
* fix(ssh): fail loudly when the multiplexer resolver was never installed
`getActiveMultiplexer` resolves through a resolver that `ipc/ssh.ts` installs at
module scope. A process that never loads the SSH layer — which is the whole point
of the Node-only backend — would get `undefined` from every call.
`undefined` already means something specific here: "not connected". So a missing
resolver and a disconnected target were indistinguishable, and a host with no SSH
layer would quietly report every target as not connected. That is the
unverifiable-reported-as-exited conflation `docs/reference/ssh-execution-boundary.md`
exists to prevent — the doc is explicit that absence of contact is never evidence
of absence of the thing.
A missing resolver is a wiring error, not a connection state, so it throws, matching
what `connectRegisteredSshTarget` already does for unregistered handlers.
Verified: 432 files / 4,759 tests across ipc, ssh, preflight, automations and trust
presets; typecheck and oxlint clean.
* refactor(pty): stop faking a BrowserWindow for the headless PTY path
`registerHeadlessPtyRuntime` passed `registerPtyHandlers` a stub object cast to
`BrowserWindow` whose `isDestroyed()` returned true and whose `webContents.send`
was a no-op — a window-shaped thing that lied about being a window, purely to
satisfy the type. Adversarial review named it as the same "looks fine, silently
returns a lie" pattern this codebase rejects elsewhere, and it is the shape that
keeps `electron` on a path that otherwise needs none.
`registerPtyHandlers` now takes `BrowserWindow | null`. An absent renderer is
semantically identical to a destroyed one — all 42 call sites already guarded on
`isDestroyed()` and skipped — so `src/main/ipc/pty-renderer-surface.ts` states that
directly: `isRendererGone`, `sendToRenderer`, `rendererWebContents`. The compound
`isDestroyed() || webContents.isDestroyed()` guards collapse into one predicate.
`isPtyWriteEventFromMainWindow` becomes null-tolerant and fails closed: with no
renderer no sender can legitimately match, so every write is rejected. Those
handlers cannot fire headless today, but failing closed is the right answer if that
ever changes.
This is the precondition for installing a PTY controller without Electron, which is
what a Node-only backend needs and what `terminal.create` actually calls.
Verified: 129 files / 2,473 tests across ipc/pty, providers and orca-runtime; the
built-artifact acceptance smoke still passes end-to-end (boot → pair →
terminal.create → sentinel → close), which is the check that matters most here
since this changes the headless PTY path itself; typecheck and oxlint clean.
* refactor(pty): read app paths and the packaged flag through the port
Follows the fake-window removal. `ipc/pty.ts` had nine `app.*` reads — all
`getPath`, `getVersion` or `isPackaged` — which the AppEnvironment port already
covers. The `BrowserWindow` import was also dead after the null-window change.
What still binds this file to Electron is now `ipcMain` (75 uses, all handler
registration) and `powerMonitor` (2). That is a clean statement of the remaining
job: split logic from registration, the same shape already applied to preflight
and the SSH registry.
Test wiring: the shared `pty-ipc-suite-environment` beforeEach installs a fake
AppEnvironment that reads through the existing `vi.mock('electron')` app object
rather than freezing values — suites toggle `app.isPackaged` mid-test to exercise
dev-mode spawn paths, so the port has to observe the same mutable field. One edit
in the shared harness covers every pty suite.
Verified: 128 files / 1,290 tests across ipc/pty and providers; the built-artifact
acceptance smoke passes; typecheck and oxlint clean; ratchet unchanged at 25.
* refactor(pty): inject the ipcMain surface so the PTY module loads without Electron
This closes the round-3 blocker: "the doc never says how orcad installs
setPtyController without Electron."
`registerPtyHandlers` owns the `RuntimePtyController` that `terminal.create`
actually spawns through — the thing a Node backend needs and cannot get from the
provider thunks. The module was otherwise host-agnostic already; the only thing
pinning 8,031 lines to Electron was a static `ipcMain` / `powerMonitor` import used
purely to register renderer handlers that no headless host will ever receive.
`src/main/ipc/pty-host-bindings.ts` makes those surfaces settable, defaulting to
no-ops. Unlike AppEnvironment and SecretStore, the default does NOT throw: a host
with no renderer legitimately has nothing to register against, so not registering
handlers nobody can call is correct rather than a hidden downgrade. The desktop
installs the real objects in `attach-main-window-services` before its handlers run.
Also converts the remaining electron import to a top-level `import type`. oxlint's
`no-import-type-side-effects` caught that inline `type` specifiers still leave a
side-effect import — precisely the "type-only is not enough if esbuild still emits
require('electron')" trap a reviewer flagged.
**`src/main/ipc/pty.ts` now bundles with zero `require("electron")`.** A Node entry
can call `registerPtyHandlers(null, runtime, …)` and get a working PTY controller.
Verified: 128 files / 1,290 tests across ipc/pty and providers; the built-artifact
acceptance smoke passes end-to-end — which is the check that matters, since this
changes how every PTY handler registers; typecheck and oxlint clean.
* fix(pty-bindings): drop two unused eslint-disable directives
CI runs oxlint with unused-disable reporting; the two
`@typescript-eslint/no-explicit-any` suppressions I added were never triggered by
any enabled rule, so they failed static analysis as dead directives. The `any[]`
rest args stay — they mirror electron's own IpcMain signature, and narrowing them
would reject the real object at the desktop call site.
Verified with the exact CI invocation: `oxlint --format github` reports 0 warnings,
0 errors across the repo.
* fix(pty): install the host bindings per process, not per window
A real regression my own change introduced, caught by the SSH docker E2E
(`paired-startup-exec-readiness` — "recovers startup exec through a headed paired
desktop owner"). It reproduced on rerun, so it was not a flake.
`setPtyHostBindings` was called inside `attachMainWindowServices`, i.e. when a
window attaches. But `registerHeadlessPtyRuntime` (index.ts:3163) calls
`registerPtyHandlers` on the serve path *before* any window exists — so those
handlers registered against the no-op default and never reached the real `ipcMain`.
A paired desktop owner then attached to a runtime whose PTY handlers were wired to
nothing.
The bindings describe the *host*, not the *window*: an Electron main process always
has `ipcMain`, whether or not a window is open. Installing them beside
`setAppEnvironment`/`setSecretStore` at the top of bootstrap fixes both paths.
Verified: 128 files / 1,290 tests; the built-artifact acceptance smoke passes;
typecheck clean; `oxlint --format github` (the exact CI invocation) reports 0/0.
* feat(orcad): de-electron the runtime core and add the Node entry + build gate
**`src/main/runtime/orca-runtime.ts` — 41,048 lines — no longer imports electron.**
Its last three sites go through `runtime-desktop-surface.ts`: a native notification,
the authoritative-window lookup, and the one `ipcMain` channel used by the
renderer-backed tab-create fallback. All three are unreachable without a renderer —
`createTerminal` already takes the background branch when no window exists (#10333) —
so a Node host installs none and the runtime relays notifications to paired clients,
which is the better destination anyway. Ratchet 25 → 24.
Adds `src/main/orcad/orcad-entry.ts`: Node host adapters plus a `startOrcad` that
constructs the runtime, installs the PTY controller via `registerPtyHandlers(null, …)`,
and serves RPC. It sets two defaults the constructor gets wrong for a headless host —
`canRecoverPersistentLocalPtys: false` (no daemon here) and
`getDesktopWindowStatus: 'blocked'` (a Node host can never be promoted to a desktop
window, which is what `'openable'` claims).
Adds `config/scripts/build-orcad.mjs`, which **currently fails, on purpose**: 25
modules still import electron (browser and speech clusters, plugins, jira/proxy,
filesystem-watcher, and four `require('electron').app` one-liners). It names them.
Two bugs found while building it, both worth recording:
- The first bundle looked clean and was not. `electron` was bundleable, so esbuild
rewrote the metafile `path` to the resolved file under node_modules and a check for
`path === 'electron'` passed while the package was in the bundle — it failed at
runtime with electron's own installer message. The check now reads `original`, and
electron is marked external so a residual import fails loudly instead.
- `jsonc-parser`'s UMD build breaks the bundle at load; aliased to its ESM entry, the
same fix `build-relay.mjs` already carries.
Verified: desktop unchanged — the built-artifact acceptance smoke passes, runtime/pty/
provider suites green, typecheck clean, `oxlint --format github` 0/0.
* refactor(host): drop the last two require('electron') app lookups
`computer/sidecar-client.ts` and `ports/port-scan-command-client.ts` read the app
root through `require('electron').app` inside a try/catch. Both were already correct
under plain Node at runtime — they return null when it throws — but the literal text
fails the plain-Node entry guard regardless, which is why port-scan carried a comment
warning it must never become reachable from a fork entry.
Reading the AppEnvironment port gives the identical "no app root here" answer without
the text, so that warning is now obsolete and the comment says so.
Ratchet 24 → 22. Every remaining entry is a real coupling: the browser cluster (15,
which variant B does not ship), speech (2), plugins (2), and jira/proxy-settings (2,
needing an HttpClient port for Chromium session partitions).
Verified: 25 files / 209 tests; acceptance smoke passes; typecheck and
`oxlint --format github` clean.
* docs(orcad): record that the ratchet under-counts orcad's graph
The ratchet reports 22 electron importers; the orcad build reports 23. The extra is
agent-hooks/wsl-hook-relay-launch.ts, and the cause is a gap in the gate rather than
a rounding error: the ratchet measures what orca-runtime + runtime-rpc reach, while
orcad's entry also imports ipc/pty directly to install the PTY controller.
Once orcad ships it must become a ratchet entry point, or the two numbers drift and
the gate quietly stops covering the artifact it exists for.
* refactor(runtime): inject the browser commands factory
Drops 14 modules from the runtime's Electron graph in one change — the whole Chromium
browser cluster. Ratchet 22 → 8.
`OrcaRuntimeService` constructed `RuntimeBrowserCommands` as a field initializer, and
that construction is what pulled in `BrowserWindow`, `session`, `webContents` and the
cookie jars. Importing the class for its *type* is free; only building it costs.
So the class import becomes `import type`, and the instance comes from
`runtime-browser-commands-factory.ts`. The desktop installs the real factory at the
Electron entry. **All ~80 existing `this.browserCommands.*.bind(...)` delegations are
untouched** — a review round specifically warned that rewriting those was the
expensive, risky part, and this avoids it entirely.
With no factory installed, browser commands reject per call with `browser_unavailable`
rather than resolving to a stub that silently succeeds. The runtime already filters
browser capabilities out of `getStatus()` when no backend exists, so clients do not
offer the affordance in the first place.
Also corrects a stale comment in `pty-renderer-surface.ts` that still described the
fake window as present tense; it was deleted two commits ago.
Verified: 451 files / 5,513 tests across `src/main/browser` and `src/main/runtime` —
the entire browser automation suite; the built-artifact acceptance smoke passes;
`pnpm typecheck` and `oxlint --format github` clean.
* refactor(host): extract the plugin client list and port two app lookups
Ratchet 8 → 5.
- `listPluginsForClients` moves to `src/main/plugins/plugin-client-list.ts`. It needed
only three `plugins/*` helpers, none of them Electron — it was colocated with
`ipcMain.handle` registrations, so the runtime's `plugins.list` RPC dragged all of
Electron in to call a function that reads a lockfile. Same shape as preflight.
Dropping it also releases `ipc/plugin-marketplaces.ts`.
- `agent-hooks/wsl-hook-relay-launch.ts` and `speech/stt-service.ts` read `getAppPath`
and `isPackaged` through the AppEnvironment port.
The five that remain are all genuinely Chromium and need the HttpClient port or a
watcher split, not another mechanical swap: `browser/cdp-bridge` (webContents),
`ipc/filesystem-watcher` (ipcMain), `jira/authenticated-request` and
`network/proxy-settings` (net + session partitions), `speech/model-manager`
(`net.request`, which honors app proxy settings that Node https does not — replacing
it is a behaviour change, not a rename).
Verified: 219 files / 1,922 tests across plugins, speech, agent-hooks and the runtime
RPC methods; the built-artifact acceptance smoke passes; typecheck and
`oxlint --format github` clean.
* refactor(network): resolve the default proxy session lazily
Ratchet 5 → 4.
`proxy-settings.ts` needed exactly one Electron value: `session.defaultSession`, as
the fallback when a caller does not pass `options.proxySession`. Callers could already
inject a session; only the default was hard-wired. It now comes from a settable
resolver, so the module loads under plain Node.
**A resolver rather than a Session, because a Session eagerly throws.** The first
attempt installed `session.defaultSession` directly in pre-ready bootstrap and broke
startup outright — `TypeError: Session can only be received when app is ready`. The
acceptance smoke caught it before commit. Deferring to first use is always after ready.
Behaviour with no session is not a degradation: there is no Chromium proxy config to
discover, so `resolveProxy` is skipped and the environment variables become the whole
answer rather than a fallback. Applying rules to a session that does not exist is
likewise skipped; settings are still honoured because outbound requests read the env.
This reaches past Jira — a review round noted `ensureElectronProxyFromEnvironment` is
also on the Claude HTTP path via `oauth-refresh.ts` and `rate-limits/claude-fetcher.ts`.
Verified: 48 files / 526 tests across network, jira and rate-limits; the
built-artifact acceptance smoke passes; typecheck and `oxlint --format github` clean.
* fix(index): merge the duplicate proxy-settings import
CI's code-quality lint (`oxlint --config config/oxlint-code-quality-native-plugins.json
--deny-warnings`) flags a module imported twice in one file. My earlier insertion added
a second `./network/proxy-settings` import beside the existing one.
Verified with CI's exact invocation: exit 0.
* refactor(network): add the HttpClient port and lift BrowserError out of cdp-bridge
Ratchet 4 → 2.
Two unrelated couplings, both of the same shape — a small thing living inside a
Chromium-heavy file.
`BrowserError` is a seven-line error class with no dependencies, but it lived in
`browser/cdp-bridge.ts`, which imports `webContents`. The runtime catches that type on
paths with nothing to do with CDP, so one import kept a Node host from loading the
runtime at all. Moved to `browser/browser-error.ts`; cdp-bridge re-exports it.
`jira/authenticated-request.ts` fetches through `net.fetch` and reads
`session.defaultSession`. `network/http-client.ts` makes both settable. This one is a
**named port rather than a silent fallback, because the fallback is not transparent**:
Electron's net follows Chromium session/proxy state, avoids undici's stale keep-alive
sockets after a VPN path change, and sends a Chrome user agent that Jira's XSRF check
depends on. A Node host gets `globalThis.fetch`, reads proxy config from the
environment, and sends Node's user agent. That difference is documented at the port.
`session.defaultSession` is read per call, not captured at install — it throws before
the app is ready, which is the mistake the previous commit made and the acceptance
smoke caught.
Test wiring: `jira/client.test.ts` installs the port *inside* `loadClientModule`, after
its `vi.resetModules()`, since the reset gives the module a fresh singleton.
Verified: 461 files / 5,616 tests across jira, browser, network and runtime; the
built-artifact acceptance smoke passes; typecheck, `oxlint --format github` and the
code-quality lint with `--deny-warnings` all clean.
* fix(http-client): register the Node fetch fallback with the call-site audit
`global-fetch-call-site-audit.test.ts` guards every global-fetch use, because the
global runs on undici where an unread response body can crash the whole process
(orca#8695). The HttpClient port's Node fallback is a new such call site and was
unregistered — the guard caught it in a full-suite run.
Registered with the reasoning, and the port's doc comment now states the body-safety
contract explicitly: it hands the Response straight to its caller and never inspects
it, so the consume/cancel obligation stays exactly where it already was — with the
caller, unchanged from when they called Electron's net directly.
Two comments elsewhere mentioned the global by name and tripped the line scan as false
positives; reworded to describe the behaviour rather than name the API.
Verified: audit passes; typecheck and `oxlint --format github` clean.
* fix(app-environment): read hasAppEnvironment through the realm slot
|
||
|
Neil
|
cbea7530b4 |
build(runtime): gate new Electron imports reachable from the Orca runtime (#15919)
* build(runtime): gate new Electron imports reachable from the Orca runtime
The runtime is meant to become host-agnostic so it can also run on plain Node,
but nothing enforced that. `orca-runtime.ts` reaches dozens of modules that
import `electron`, and the count grows silently: the import that breaks
portability is usually several hops away, so no reviewer sees the edge.
Add a reachability ratchet, modelled on the existing max-lines one. It bundles
the runtime and its RPC server with esbuild, reads the metafile for every module
importing `electron`, and diffs that against a checked-in baseline. A new module
fails; a removed one forces the baseline to tighten. The list may only shrink.
A per-file lint rule cannot do this — the point is precisely the transitive
edges — so this runs as a build gate in `pnpm lint`.
Baseline starts at 36, down from 50 before the SecretStore and AppEnvironment
ports landed, which is the migration made measurable.
Verified: gate passes clean, fails with an actionable message when an `electron`
import is added to a runtime module, and passes again when reverted.
* fix(runtime-ratchet): resolve paths from the script, not the caller's cwd
Run from anywhere but the repo root, the gate died with an unhandled ENOENT stack
instead of a usable message. It failed closed, so it was never unsafe — just
undebuggable. Anchor ROOT to import.meta.dirname and pass absWorkingDir to esbuild
so metafile keys stay repo-relative.
* ci(runtime-ratchet): actually run the gate in CI
The ratchet was wired into the `lint` npm script, but CI's static-analysis job
runs the individual checks rather than `pnpm lint`, so the gate would never have
fired on a PR — it would have looked enforced while enforcing nothing.
Runs on ubuntu-latest alongside the max-lines ratchet, so the checked-in baseline
is only ever produced by one platform.
* fix(runtime-ratchet): mark native addons external so CI can run the gate
ssh2's optional cpu-features dep points at a prebuilt .node that only exists
where a build toolchain has run. Loading it made the gate pass locally and
hard-fail on CI with 'Could not resolve ../build/Release/cpufeatures.node'.
The gate only reads the import graph, never the addon, so resolve every .node to
an external stub instead. Verified by hiding the local prebuild — which is CI's
state — and re-running: still 36 entries, exit 0.
* fix(runtime-ratchet): stop the gate failing open on Windows
The entry guard compared import.meta.url against a `file://${process.argv[1]}`
template. On Windows argv[1] is a native path (C:\repo\...) while import.meta.url
is file:///C:/repo/..., so they never match: main() never ran and `pnpm lint`
exited 0 on Windows without bundling, reading the baseline, or enforcing anything.
Use pathToFileURL, which is the idiom check-max-lines-ratchet.mjs:225 already uses.
CI runs this on ubuntu so enforcement was never actually lost, but a Windows
developer got a green gate that checked nothing.
|
||
|
Neil
|
0bbc6c80e8 |
refactor(host): route app paths and version through an AppEnvironment port (#16019)
* refactor(host): route app paths and version through an AppEnvironment port
`app.getPath('userData')` is the single largest Electron coupling in the main
process — 37 call sites — and it is one of the things stopping the Orca runtime
from booting on plain Node. Give it the same treatment as SecretStore.
- `src/shared/app-environment.ts` — the port plus a settable registry, covering
the members the runtime's module graph actually reads: paths, app path,
version, packaged flag, shutdown hook, exit, and Chromium process metrics.
`getAppEnvironment()` throws until installed, for the same reason the secret
store does: a silent default resolves `userData` to the wrong directory and the
caller writes real state there before anyone notices. No `node:` imports,
because `src/shared/**` is in the web build graph.
- `src/main/host/electron-app-environment.ts` — the desktop adapter, a
pass-through to `electron.app`.
- 9 modules migrated: telemetry, opencode/mimo/pi hook services,
terminal-history-paths, terminal-scrollback-snapshots, cli-installer,
clipboard-image-temp-file, memory/collector.
Deliberately NOT migrated: `src/main/browser/**`. That cluster is Chromium-
adjacent by nature — cookie jars, download destinations, offscreen pages — and a
Node backend does not ship it at all, so porting it buys nothing and churns
heavily-mocked suites. Also left alone for now: the call sites that additionally
touch `app.asar` path literals or `app.setName`, which need more than a
mechanical swap.
`getAppMetrics` stays on the port rather than being injected because
memory/collector.ts is its only caller and reads it from module scope; a Node
host returns [], having no Chromium processes to measure.
Test wiring: the secret-store setup file becomes `vitest-host-ports-setup.ts` and
installs both ports, exporting `fakeAppEnvironment`/`installFakeAppEnvironment`
so suites needing one specific member state only that instead of restating all
seven — which is boilerplate, and had pushed one suite past the max-lines budget.
Verified: 159 files / 1651 tests pass across every touched area; `tsc` clean on
both the node and web projects; `oxlint` clean.
* fix(typecheck): list the vitest host-ports setup in the node project
Three suites import `installFakeAppEnvironment` from config/scripts, but that
directory is outside tsconfig.node.json's include list, so composite typecheck
failed with TS6307. Listing the one file matches how this config already pins
individual files it needs.
Local `tsc --composite false` does not reproduce this — only `pnpm typecheck`
does, which is what CI runs.
* refactor(host): drop two unused AppEnvironment exports
hasAppEnvironment() and resetAppEnvironmentForTests() had zero callers. The
secret-store equivalents are used, so these were mirror-symmetry rather than
need; add them back when something actually needs them.
* test(terminal-history): install the AppEnvironment fake instead of mocking electron
These three suites mocked `electron.app.getPath` to point at a fixture dir. The
production module now reads the port, so the mock was inert and the global test
default's temp dir won — which broke the WSL path assertions and every deletion
count.
Found by a full-suite run, not by the targeted checks around the migrated modules,
which is the argument for running the whole suite on a refactor this wide.
* test(host-ports): remove the per-environment temp dir on teardown
The setup allocated a mkdtemp directory at module scope, which vitest evaluates
once per test *environment* — one per test file, not one per worker. Nothing
removed them, so a full 6,000-file run left thousands behind.
Proven: with an isolated TMPDIR, a three-file run previously added directories and
now leaves zero.
* fix(app-environment): anchor the installed environment to a realm global
Same reason as the SecretStore: vi.resetModules() rebuilds the module registry,
and an environment installed before the reset read back as uninstalled.
|
||
 erish
|
063b804298 |
fix(i18n): match CheckRunJobs succeeded to its sibling count-label register (#16013)
succeeded rendered as casual declarative 성공했다 ('it succeeded') next to
skipped's polite 건너뛰었습니다 and pending's noun-phrase 보류 중, in a summary
that joins all three after a count: '3 성공했다 · 1 건너뛰었습니다'. Machine
translation read succeeded as a finished sentence instead of the noun label
the other two siblings use. Switch to 성공 and pin it in the key-override
file so the catalog regen script can't revert it; #15875 fixed five other
keys in the same family but its hardcoded regression map didn't cover this
one.
|
||
|
Neil
|
d07ce15cff | refactor(host): route secret storage through a SecretStore port (#15916) | ||
|
Neil
|
990b23611e | fix(i18n): correct five Korean strings that changed meaning in machine translation (#15875) | ||
 Neilandpoorpaper
|
9e335e9a37 |
fix(skills): normalize contributor-facing paths in the skill guide generator (#15861)
Co-authored-by: poorpaper <poorpaperdesire@gmail.com> |
||
|
Neil
|
2b1254d681 |
fix(windows): own PTY process trees with job objects (#15755)
* fix(windows): own PTY process trees with job objects Teardown used to answer 'is this tree mine, and how do I kill it?' by scraping the process table, walking parent pids back to Orca, and running taskkill /T /F only if the walk said yes. Every step is a guess, and the code said so itself: windows-pty-root-identity.ts:35 already named the fix -- 'an inherited handle / Job Object'. The guesses fail in the ways users report. A pid walk cannot survive pid reuse, so teardown refused whenever it could not prove ownership, and a refused kill is an orphaned agent tree holding the worktree directory open (#9045, #10475, #10087). A descendant that reparented is invisible to the walk. The scrape itself could be blocked by policy, which read as 'no evidence'. node-pty now creates a job object per ConPTY and assigns the shell under CREATE_SUSPENDED, before it can spawn anything -- assigning afterwards leaves a window in which a fast child escapes. Termination is one TerminateJobObject; liveness is QueryInformationJobObject. Verified on Windows 11 against a shell whose grandchild was spawned detached: job membership came back [shell, grandchild] and one call killed both. Neither a parent-pid walk nor GetConsoleProcessList sees that grandchild -- it leaves the console and reparents, which is exactly the claude.exe/node.exe/cmd.exe orphan in #9045. KILL_ON_JOB_CLOSE means a daemon that dies without unwinding no longer strands shells (#9195, #10415). The job is the daemon's, not the app's, so an app-main crash still leaves sessions alive -- the guarantee win-crash-survival-e2e asserts. Both entry points report unavailable rather than a false success when a pty has no job: an outer job without BREAKAWAY_OK can refuse the assignment, and a pty from an older build has none. Reading 'we could not tell' as 'already dead' is the original bug, so the old probe stays as the fallback. * test(windows): pin job ownership against a real detached grandchild The unit tests pin the contract; this pins what the contract is for. A grandchild spawned detached leaves the pane's console and reparents, so GetConsoleProcessList and a parent-pid walk both miss it -- that is the process that outlived its pane and held the worktree directory open. Includes a guard that this build actually has job support, so a node-pty rebuilt from unpatched sources fails loudly instead of letting every assertion pass vacuously. * fix(windows): correct the job liveness contract to what Windows actually does I claimed an emptied tree would report [] and that this was the evidence a stale registry entry lacks (#15549). Running it on Windows 11 showed otherwise: node-pty drops its handle record and closes the job when the shell exits, so a dead tree reports null. Null therefore means unverifiable in the sense of docs/reference/ssh-execution-boundary.md -- no job support, not a ConPTY, or no longer tracked -- and is never evidence that processes died. A caller reading it as proof of death would have been right by accident after a normal exit and wrong on a host that refused the assignment. What the API does add is descendant liveness for a tree that is still tracked, including children that detached from the console. * fix(windows): stop a clean shell exit from reaping backgrounded processes Measured on Windows 11: with JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE on the per-PTY job, releasing the handle when the shell exits also killed whatever the user had backgrounded. Typing 'exit' in a pane reaped a detached server that survived before this patch. That is a behaviour change nobody asked for. The approved change was that killing the terminal daemon reaps its shells -- not that a clean exit reaps your background job. The job's purpose is to make an EXPLICIT teardown exact, which TerminateJobObject still does. Reaping a dead daemon's shells now needs the daemon-level job the design called for: the daemon assigns itself, children inherit membership, and its closure on daemon death reaps them without touching clean-exit semantics. Not in this PR; noted in the reference doc. * test(windows): pin that a clean exit leaves backgrounded work alone The counterpart to the tree-kill test. Without it, re-adding JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE would look like a tightening rather than the regression it is. * fix(windows): stop a winpty pty id from matching a ConPTY job winpty.cc and conpty.cc each mint their 'pty' id from an independent counter, and windowsPtyAgent stores both in the same _pty field. So a winpty-backed terminal's id can collide with a live ConPTY baton -- and closing that pane would have terminated an unrelated pane's entire process tree. Both job entry points now take the shell pid and the native side refuses unless GetProcessId(hShell) matches, which makes the id unforgeable. Two more from the same read-through: - ResumeThread's failure was ignored. A shell left suspended is a pane that never prints and never exits, which is far harder to diagnose than a failed spawn; it now cleans up and throws. - handle->hJob was assigned before LoadConptyDll, which can throw. A baton carrying a job but never reaching SetupExitCallback has nothing left to close it, so the assignment moved down beside hShell. * docs(windows): record the unsynchronised node-pty baton table Pre-existing upstream -- the exit thread erases while the main thread reads -- but terminatePtyJob adds an instance of it, so it belongs in writing rather than in someone's head. * fix(windows): close four gaps found in review BREAKAWAY. The per-PTY job set no limits, so a child asking for CREATE_BREAKAWAY_FROM_JOB was refused with ERROR_ACCESS_DENIED. Installers, msiexec and some updater and service-control paths spawn that way deliberately -- they worked before this patch and would have failed only inside an Orca terminal, which is the worst shape a bug report can take. JOB_OBJECT_LIMIT_BREAKAWAY_OK restores it; a child still has to ask, so ordinary descendants stay owned. EMPTY IS NOT UNAVAILABLE. The native reader returns an empty list -- not an error -- when CreateToolhelp32Snapshot fails, which is what an EDR hook or a restricted token produces. Callers read that as 'nothing is running' and teardown concludes a live PTY root is already gone. The snapshot must contain the querying process; nothing else is unfalsifiable, and one predicate catches empty, truncated and permission-filtered tables alike. NO DEADLINE. Replacing execFile dropped its 3s timeout. The vendored reader latches a module-global while a request is in flight and clears it only after draining its callbacks, with no try/catch -- so one wedge leaves every later call queued behind a promise that never settles, and the process table is dead for the life of the app. The bound is back. GUESSED IMAGE PATH. executablePath was derived from the first space-delimited token, which reads 'C:\Program' out of an unquoted 'C:\Program Files\nodejs\node.exe ...'. Wrong evidence is worse than none, and the only consumer already had the full path in , so the field is gone rather than repaired. Also: remove_pty_baton no longer sits inside assert(), which NDEBUG would compile away along with the call, and the job accessors hold a lock across lookup and use -- handle values are recycled, so an unguarded read could pass the shell-pid check against an unrelated process and terminate the wrong job. * fix(windows): apply the job lock once per accessor The patch script matched a string its own replacement still contained, so PtyTerminateJob got two lock_guards named guard and PtyListJobProcessIds got none. MSVC caught it: error C2374 redefinition. * test(windows): pin that a child can still break away from the job Verified on Windows 11: 'start /b' writes its marker and no access-denied appears. Without JOB_OBJECT_LIMIT_BREAKAWAY_OK this fails, and it fails only inside an Orca terminal -- so the failure would look like Orca corrupting unrelated software rather than like a job-object change. * fix(windows): stop the ownership guard from reading a closing handle The guard called GetProcessId(hShell) to prove identity, but the exit watcher closes hShell on another thread -- so the guard could read a closed handle, and under strict handle checks that is fatal rather than merely wrong. Worse, it widened the gap between validating hJob and using it from two instructions to a kernel round-trip, and handle values recycle: the likeliest occupant of a freshly recycled value in this process is another pane's job. The pid never needed a handle. It is captured at spawn and compared as a DWORD, so the guard touches no handle at all, and hShell is now closed inside the same lock as hJob. Also from review: - reject CR/LF in a cmd argument. cmd ends the command at a raw line break whatever the quote state, so there is no escape for it; encoding one anyway truncates the argument and can leave the remainder to run as a command. Agent prompts are this encoder's motivating input. - ask the process table only for the fields a caller needs. Memory and CommandLine each cost an OpenProcess per process, inline, for every process on the box -- and the 1024 bound is patched out. Ancestry reads now skip both. - corpus gains the degenerate quote-only and two-quote arguments. - PtyListJobProcessIds' docblock still taught the empty-list contract that was corrected on the TS side, and now records that the ConPTY console host is never a job member. - drop a write to NumberOfAssignedProcesses, which is output-only. - pty_baton::hShell is initialised; ownsShell was only safe because && short-circuited ahead of it. The backgrounded-child test is rescoped: 'start /b' uses CREATE_NEW_CONSOLE, not CREATE_BREAKAWAY_FROM_JOB, so it proves job membership does not block backgrounding -- not that BREAKAWAY_OK works. That flag rests on the Win32 contract, and I have said so rather than letting the test imply coverage it does not have. * fix(windows): bound retries after the process table wedges The 3s deadline stops a caller hanging, but the timed-out call leaves its callback in the vendored module's queue -- and that queue drains only when the latched request completes, which in this wedge never happens. Retrying at the caller's poll rate would add a closure per tick forever. A 30s cooldown bounds it to one probe, and a late callback clears the cooldown because it proves the reader recovered. Also pins the deadlock invariant in the patch: the exit thread's lock must close before tsfn.BlockingCall, because that waits on the JS thread and the JS thread can be waiting on the same mutex inside PtyTerminateJob. Correct today by scoping; a comment so a later refactor does not widen it. * revert(windows): drop the field-selection API, which cannot pay off I added it for a real perf finding -- Memory and CommandLine each cost an OpenProcess per process -- and then never wired a caller, so the claim that ancestry reads skip them was wrong. Wiring it would have been worse than leaving it dead. The only ancestry consumer is the teardown identity probe, which needs a snapshot that started AFTER it asked, for pid-recycle detection. Bypassing the shared reader to get narrow fields would let that request join a scan already in flight -- trading a correctness guarantee for milliseconds. Field selection only pays off if callers can ask for less, and they cannot: one shared snapshot serves every caller so a 32-wide teardown collapses into a single scan, which means it has to carry every field. The reasoning now lives next to the flags instead of in a dead export. * fix(process): three P1s from review — a crash vector and two wedge bugs STDIN EPIPE COULD TAKE DOWN THE MAIN PROCESS. A child that exits without reading makes the queued write fail with EPIPE, and an unhandled error on a stream is an uncaught exception. The child's own error listener does not cover its stdin stream, so runProcess({ input }) against a short-lived child was a crash, not a failed call. THE COOLDOWN LEAKED A BATCH PER CYCLE INSTEAD OF BOUNDING IT. At expiry every concurrent caller passed the check before any of them re-armed it, so each enqueued a callback into the still-latched native queue and each cycle leaked another batch. The cooldown is now re-armed BEFORE probing, so exactly one caller gets through. A SYNCHRONOUS THROW LEFT ITS DEADLINE RUNNING. The timer was declared inside the try, so catch could not clear it; it fired later and wedged a reader that had already recovered. Hoisted and cleared, and wedge state now carries a generation so a request that lost its deadline cannot mutate it on behalf of the one that replaced it. Found by review once the prompts were short enough for the reviewer to finish -- the previous two rounds died on prompt length. * fix(process): stop a stream error from crashing the main process Same class as the stdin EPIPE finding, two instances further on: stdout and stderr had data listeners and no error listeners, and an unhandled error on a stream is an uncaught exception. Scoped to runProcess, which owns the child outright. spawnProcess hands the streams to its caller, and a blanket handler there defeats callers that track and remove their own listeners -- the SSH ProxyCommand transport does exactly that, and its cleanup test caught the attempt. Documented on spawnProcess so the boundary is explicit rather than inferred. * fix(windows): validate the ConPTY DLL before creating the process LoadConptyDll throws when conpty.dll is missing -- a real state, and one this branch hit during development. It ran after CreateProcessW and ResumeThread but before the baton and the exit watcher were installed, so a throw leaked the job, process and thread handles and left an untracked shell tree running. Once per attempt, so a broken install accumulates orphan shells on every retry. Resolving the DLL first costs nothing and leaves exactly two throws after creation: the CreateProcessW failure, where nothing exists yet, and the resume failure, which already cleans up after itself. This also closes the same leak for hProcess and hThread, which predates the job work. * feat(windows): add the daemon-level job the design called for The plan specified two nested jobs and I built one. That gap is why dropping KILL_ON_JOB_CLOSE from the per-PTY job cost the approved guarantee that a dead daemon reaps its shells -- I had one job trying to answer two questions, and the two answers conflict. They are separate jobs. The per-PTY job answers 'kill exactly this pane's tree, now', and cannot be kill-on-close because its handle is released when the shell exits, which would reap whatever the user backgrounded. The daemon assigns itself to a second job that IS kill-on-close; its handle is released only when the daemon dies. Children inherit membership, so every pty is covered and the per-PTY jobs nest inside it. Daemon, never app: an app-main crash must still leave sessions alive, which win-crash-survival-e2e asserts. Both jobs carry BREAKAWAY_OK, or a child asking to break away is refused at whichever level lacks it. Restores #9195 and #10415, which I withdrew from this PR earlier. * docs(windows): record what the host job does not cover An app-hosted PTY gets a per-PTY job but no crash reaping, because the alternative is a kill-on-close job on the app -- which is precisely what the crash-survival guarantee forbids. * ci(windows): run the win32 suites in the PR windows job Both were skip-on-non-win32 and had only ever run on one machine I drive by hand -- which went unreachable at exactly the moment I needed to verify the percent-escaping fix. Verification that depends on one box is not verification. The job already builds node-pty from patched source and already runs a useConptyDll test, so the ConPTY runtime files are in place by this step. This also makes the encoder a gate: the corpus is the only thing standing between an agent prompt and a mangled argv, and it now runs against real cmd.exe on every PR. * fix(deps): refresh the lockfile for the current patch hashes pnpm records a hash per patched dependency, and I regenerated both patches repeatedly across the review rounds without refreshing the lockfile. Every local run used --frozen-lockfile's looser sibling, so nothing caught it until CI did: ERR_PNPM_LOCKFILE_CONFIG_MISMATCH Cannot proceed with the frozen installation. The current "patchedDependencies" configuration doesn't match the value found in the lockfile Verified with pnpm install --frozen-lockfile locally this time. * ci(windows): build node-pty from source before the win32 suites CI proved the encoder fix on real cmd.exe -- 26/26 -- and in the same run proved the job suite had been testing an unpatched binary. node-pty prefers its upstream prebuild, which does not contain this patch, so every job-object export was absent and isPtyJobOwnershipAvailable() was false. That guard is why the failure was loud rather than a vacuous pass, and it is the reason the assertion exists. Packaging was never affected: rebuild-native-deps.mjs already builds node-pty from source for Electron and restores the ConPTY runtime files. The gap was the node-runtime test environment only. Not changing requiresPatchedNodePtySourceBuild's win32 exemption here. Its premise -- that the patch is Unix-only -- is now false, but lifting it also needs pnpm rebuild to force a source build, and I cannot validate that on macOS and Linux from here. Recorded as a follow-up instead of changed blind. * test(windows): gate the host-job guarantee in CI The daemon-level job had one hand-run proof and no automated coverage -- the same shape of gap that let an unpatched node-pty go unnoticed until CI caught it. It needs a real second process, because the assertion is about what happens when that process is force-killed: a host in a kill-on-close job must strand neither its pty nor a grandchild spawned detached, which is the process a parent-pid walk cannot see. Runs in the Windows PR job alongside the per-pty and encoder suites, so both halves of the two-job design are now gated rather than asserted. * fix(windows): serialise host-job creation Two callers racing PtyAssignCurrentProcessToJob would each create a job, put the process in both, and leak the first handle -- and the handle is what keeps a kill-on-close job alive, so a leaked one is never released. 'Only JS calls it' is not a guarantee: a worker thread with its own N-API env shares these statics. Also records the ordering requirement it depends on. AssignProcessToJobObject adds only the named process; children inherit membership, but a pty that already exists does not join retroactively and would not be reaped. The daemon assigns at startup, before the ConPTY warmup and before any session, which is correct today and now stated rather than implied. * fix(daemon): keep the host job off the startup path Assigning the host job at daemon startup resolves the node-pty native module, which loads the ConPTY addon -- and paying that before the endpoint is published delayed readiness enough that daemon-boot-smoke failed on windows-latest, deterministically. windows-conpty-warmup already carries the comment for this exact hazard ('setImmediate keeps the ready/handshake path ahead of the warm-up') and I put an eager load in front of it anyway. Moved to the pty spawn path, which already pays ConPTY cost, and memoised. Children inherit job membership, so assigning immediately before the first spawn still covers every pty -- and nothing can spawn one before the endpoint exists. |
||
|
Neil
|
057fbfcffc |
perf(windows): read the process table natively instead of forking PowerShell (#15749)
* perf(windows): read the process table natively instead of forking PowerShell Seven independent readers each forked powershell.exe to run Get-CimInstance Win32_Process, with a wmic fallback that Windows 11 24H2 has removed. On a domain-joined host with PowerShell Transcription enabled by policy, one of them running every ~2s recorded ~289GB across 1.4 million files (#15209). The same scan cost ~700ms and ran per pane (#15036), and a Group Policy or AV block turned it into 'unavailable', which callers read as 'no evidence' -- which is how a PTY tree survives its own teardown (#9045, #10475). A Toolhelp32 snapshot answers the same question with no child process. Measured on Windows 11 with 1050 processes, p50/p95: pid+ppid+name 15.9 / 17.5 ms +memory +command line 30.6 / 33.7 ms Get-CimInstance 706 / 723 ms Two upstream defects needed patching, both found by running it on real hardware. The binding requires Spectre-mitigated libraries our agents do not carry (node-pty is patched the same way). And enumeration stopped after 1024 processes: on a host with 1051 the module returned exactly 1024, and the querying process was itself among the 27 missing -- a truncated snapshot silently hides the descendants teardown is looking for, which is the failure this whole change exists to remove. Migrated: the foreground/descendant reader (the #15209 scraper and the teardown identity gate) and the port scanner's PID attribution. NOT migrated: the memory collector and three identity probes, which need Win32_Process.CreationDate and have no native equivalent. Start time is a proxy for identity anyway; an inherited job handle is the real answer, so those belong with the job-object work rather than here. Packaging follows the windows-native-registry contract exactly: optional, absent from onlyBuiltDependencies so macOS/Linux never run node-gyp, win32-only in the packaged runtime. Asserted by the existing contract test, which also stops pinning a whole source literal that only tested its own formatting. * chore(process): ratchet the child_process allowlist down windows-foreground-process-rows.ts no longer spawns anything, so its allowlist line is stale. The guard fails on a stale entry as well as a new one, precisely so a migrated file cannot keep a slot open and hide the next regression in the same path. * fix(ports): import the process-table reader the scanner uses Missing import: the migration replaced the PowerShell call but the new symbol was never imported, so tsc failed. Vitest transpiles without typechecking, which is why the port-scanner suite stayed green. * fix(deps): sync this branch's lockfile with its patch set Same class as the fix on the tip branch: pnpm records a hash per patched dependency, and this branch introduces the windows-process-tree patch without its lockfile entry matching. Every job here failed at install with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH. Verified with --frozen-lockfile, which is what CI runs and what my local runs were not. * test(relay): drive the relay's Windows fixtures from the native snapshot Two relay cases fed a PowerShell CIM payload through a mocked execFile. That reader is gone, so both failed -- deterministically, on every PR run for this branch and the one above it. I did not catch it because my own verification sweep was 'src/main src/shared config/scripts' and never included src/relay. The relay is a first-class consumer of the process table; leaving it out of the sweep is how a deterministic failure survived six review rounds. |
||
|
Jinwoo Hong
|
a0578641e9 | fix(ci): restrict release test token permissions (STA-4970) (#15675) | ||
 NeilandBrennan
|
c92f394cde |
fix(pty): delete the reply-withholding scheduler (#15578)
* fix(pty): answer a terminal colour query in its own turn Root-cause follow-up to #15559, which stopped a CPR overtaking a deferred colour reply but left the deferral itself in place. Orca answers terminal queries by writing to the PTY master, which a line discipline in ECHO copies straight back out as junk on a cooked prompt (#12112). The guard was to withhold the write until an `stty` subprocess proved ECHO clear — and forking is what forced the decision to be async. Any deferral, however short, lets a reply written later in the same turn overtake this one, so the async probe was the bug's root cause. Read the bit synchronously instead. Linux and the BSDs redirect a master's mode ioctls to the slave, so a `tcgetattr` on the master fd node-pty already owns answers for the slave with no fork: measured 0.26us against 2403us for the subprocess. With a verdict available inline, a querying program that already cleared ECHO — every raw-mode prober, including the colour probe behind the `gh auth login` report — is answered in its own turn and can never be reordered. The deferral stays for the genuinely cooked case, and the ordering guarantee stays underneath it: hosts whose node-pty predates this patch get no sync probe and fall back to the deferred path, which mixed client/host versions make a live production path. Reply routing is all-or-nothing: a payload needing neither containment nor ordering stays on the host's own path, so a CPR answered during shell startup cannot pass the daemon's post-ready flush gate and splice into the buffered startup command. Native side is fail-safe: a kernel that did not redirect would answer from the master's own termios, whose ECHO defaults set, so the degraded verdict is "echoing" — never a false "quiet". The JS half ships in the pnpm patch while the binding needs a source build, so ORCA_REQUIRE_NODE_PTY_ECHO_STATE=1 makes CI fail rather than silently skip when it is handed an upstream prebuild. Co-authored-by: Brennan <brennanb2025@users.noreply.github.com> * fix(pty): keep the flush ordered under synchronous re-entry Three defects found in external review of the reply-ordering work. node-pty delivers onData inside the master write, so a query can be answered while the queue is mid-flush. `flushPendingWrites` spliced the array off before writing, so that reply saw an empty queue, took the same-turn path, and landed ahead of entries the loop had not written yet — reproduced as 01, 99, 02, 03. It now shifts one entry at a time so a re-entrant reply queues behind the rest, bounded by the length at entry so a re-entrant push cannot spin the loop. An overflow flush can re-enter as far as teardown. `answer` did not re-check `closed` afterwards, so it queued behind a closed delivery, returned true, and the reply was never written and never reported. The payload router's ownership comment overstated its guarantee. The `any` semantics are deliberate — returning false after a constituent was already written would have the caller re-write the whole payload and duplicate it into the child's stdin — so the residual mixed-failure drop is now documented rather than implied away. * fix(pty): delete the reply-withholding scheduler Orca answered a terminal query by withholding the write until a probe proved the slave's ECHO bit was clear. That was the wrong mechanism, and it is now gone: replies are written in the caller's turn and their echo is contained on the output side, where it always was. Withholding never removed an echo. The wait was bounded and always ended in a write, so the output-side projections were doing the work the whole time — including the readline rewrite, which happens with the tty already raw and which therefore no reading of the ECHO bit can predict. What withholding did add was an asynchronous write path, and that is what let one reply overtake another and land in the next program's stdin (#15559), what produced a re-entrancy inversion inside its own flush, and what four rounds of regressions have lived in. The last thing it covered was the verbatim echo of a `stty -echoctl` tty. That shape is now projected directly. It starts with ESC, so it is matched only when complete and never held as a partial: holding it would take a bare trailing ESC from the query parser and an expired hold would release it raw, so a query torn at its own ESC would never be answered. Complete-match-only is what makes the shape safe to project at all. Measured on a real pty: a cooked-mode master write is both echoed AND delivered — ECHO copies the bytes without consuming them from the slave's input queue, so a program arming raw mode with TCSANOW/TCSADRAIN (libuv's setRawMode, hence every Node agent) still reads them. Only a TCSAFLUSH switcher discards it, which it does on every terminal, none of which gates a reply on termios state. Deletes the pending-write queue, the async stty probe, the poll budget and probe rate limit, the deadline-driven flush, and the answer/ answerInOrder split. Replies now leave in call order by construction. No packaging, native or CI surface is touched. * test(pty): restore stty-probe coverage and pin the duplicate-query retry Archaeology on how withholding got here, and what its tests were really protecting. Deleting the ECHO probe took four tests with it that were not about the probe at all: they cover createSttyProbe, which the shell-readiness line-editor probe still uses — in-flight sharing, the per-platform stty flag, and transient-versus-permanent failure latching. Restored against the line-editor probe, which is now their only caller. Also pins the property that answers the one case an immediate write cannot serve. A program that queries while cooked and then arms raw mode with TCSAFLUSH discards the reply with the rest of its input queue. Nothing can prevent that from the terminal side, and no terminal tries. What matters is that such a program re-queries after its own timeout: the ingress declines to answer an already-answered slot but forwards the duplicate downstream, so the renderer's emulator answers the retry, by which point the program is raw. The retry path is the recovery, not withholding. * ci(pty): keep the fish real-PTY test in the shell-contracts lane only Reverting pr.yml to main dropped the exclusion for the fish query-reply test, which this branch keeps, so it would have run in the sharded lane as well. Restores it to the shell-contracts include list and the shard exclude list, and drops the parallelism expectations for the deleted cooked-querier suite and the echo-state env guard. --------- Co-authored-by: Brennan <brennanb2025@users.noreply.github.com> |
||
 OrcaWin
|
471bc9d8ce | Ship the WSL transcript helper with the Windows relay (STA-4831) (#15529) | ||
|
Neil
|
9d06b3ba93 |
ci: stop docs-only commits from starting the skill-roundtrip matrix (#15474)
A cancelled Skill update round trip on a README merge painted main red because push to main had no path filter. Share the PR path list on push, and skip expensive PR Checks when every changed file is docs. |
||
|
Jinjing
|
acbcb477a1 |
Auto e2e tests autofix scheduled ci 1h run 1 20260818T2143 (#15379)
* fix: update E2E tests for API changes and selector robustness - Improve source control file locator specificity to avoid flakiness - Fix board test to use correct worktree ID attribute - Update removeWorktree calls to pass host ID parameter - Simplify git status polling with timeout expectation * fix: increase packaged-watchdog launch timeout and await git-status rows Extract hardcoded 15s launch timeout to a 30s constant for better reliability under load. E2E test now waits for all git-status rows to render before asserting absence of status messages, preventing flaky passes when the list is still loading. |
||
|
Neil
|
c72a4eecdd |
refactor(shell): collapse the zsh wrapper to one .zshenv and a precmd hook (STA-4786) (#15391)
* refactor(shell): collapse the zsh wrapper to one .zshenv and a precmd hook (STA-4786)
Orca needs to run code after the user's own zsh startup files. It bought that by
keeping ZDOTDIR pointed at its own wrapper dir for the whole of startup and
sourcing each user file by hand -- four generated files per transport, with a
fake ZDOTDIR live while /etc/zshrc ran. That single decision is the root of a
whole bug family:
- /etc/zshrc assigns HISTFILE=${ZDOTDIR:-$HOME}/.zsh_history unconditionally, so
history landed inside Orca's own dir (#11044), and an epilogue had to repair it.
- zsh's sourcehome() ignores ZDOTDIR once the shell is in sh/ksh emulation, so a
user .zshenv or .zprofile ending in `emulate sh` hid every later wrapper file.
The emulation degrade blocks and their forked $(emulate) probes exist for that.
- One wrapper dir shared by two installed builds could mix files from both, so
every generated file had to redefine the helpers it called.
- The baked generation-time ZDOTDIR literal is unusable when a Windows-generated
wrapper is sourced inside WSL via /mnt/c (#8003), so the runtime path had to be
re-derived from %x.
The wrapper now hands ZDOTDIR back on its first lines and defers Orca's work to a
precmd hook that runs at the first prompt -- after .zprofile, /etc/zshrc, .zshrc
and .zlogin, every one of which zsh reads from the user's own directory exactly
as in an unwrapped shell. Each bug above stops being reachable rather than being
repaired, and their machinery goes with them: eight of thirteen exported blocks in
shell-templates.ts, both drifted discovery bodies (unifying them closes the
"reconciling the two is a follow-up" note the file carried), and the relay's
separate ORCA_USER_ZDOTDIR shape. Generated zsh drops from 819 lines across
twelve files to 143 in three.
Two things the design has to get right, both found by running it rather than
reasoning about it:
- Every function is defined ABOVE the source of the user's .zshenv. A user file
ending in `emulate sh` puts the rest of the wrapper under sh parsing rules, and
the first prototype died there with `parse error near '\n'` -- silently, leaving
the pane unwrapped. Function bodies are parsed at definition time.
- ORCA_ORIG_ZDOTDIR is vetted, not trusted. The launch config only sets it when it
resolved a usable dir, but a pane inherits its parent's environment too, so a
stale value from an older build can arrive on its own and would point ZDOTDIR
back at a wrapper dir. The ownership check Node applies now also runs in the
shell, where that route is visible.
Orca also stops inventing a ZDOTDIR: where the user has none, ORCA_ORIG_ZDOTDIR is
absent and the pane ends with ZDOTDIR unset, as an unwrapped login zsh does.
Verified on real zsh over a real PTY -- necessary, because a precmd hook never runs
in a shell started with -c, so the existing `zsh -i -c` probes could not have
exercised this design at all. src/main/zsh-startup-hook-pty-harness.ts drives the
shell to a prompt and reports through a file rather than stdout, which a PTY echoes.
* test(shell): cover the relay variant of the zsh hook in a real shell
The relay writes its own variant -- no OSC 133, remote CLI bin dir instead of the
agent-teams shim -- and it had no live coverage. It used to carry a second ZDOTDIR
shape as well, which is how it drifted from the desktop template in the first
place; now the spec flags are the only difference, and this pins that.
* fix(shell): rebase the single-file hook onto content-addressed wrapper trees
#15285 landed content-addressed wrapper roots and a per-transport fileset module
while this branch was in flight. The fileset modules are now the single place the
tree is described, so 'only .zshenv' is stated once per transport and the
required-paths check follows from it rather than repeating the list.
* test(shell): point the mixed-build proof at the relay, the one fixed wrapper path
#15285 content-addressed the desktop and daemon trees, so two builds can no
longer write the same directory there and the scenario this file covers became
unreachable on those paths. The relay still writes a fixed ~/.orca-relay/
shell-ready, so that is where the hazard survives and where the proof belongs.
* fix(test): make the zsh PTY harness survive a startup that stops to ask
Two CI-only failures, both from driving a real PTY where the old probes drove a
pipe:
- A host whose global zshrc runs `compinit` over directories it considers
insecure stops startup and ASKS. A pipe-backed `zsh -i -c` never saw the
question; a PTY sits at it until the timeout. The harness now answers it.
ZSH_DISABLE_COMPFIX does not help -- that is an oh-my-zsh convention and plain
compinit ignores it, which I confirmed by reproducing the prompt locally.
- The PS1 line was typed at t=0, so on such a host the question consumed it as
its answer. The harness now waits for the shell to fall quiet first, which
also stops a slow prompt framework racing the same write.
Also merges a duplicate vitest import the native code-quality audit flagged.
* fix(test): stop the live-shell assertions assuming macOS host behaviour
Two of them hardcoded what my machine does rather than what Orca owes:
- LINEINIT was pinned to 'none'. A host whose global zsh config installs its own
zle-line-init widget has one either way; the contract is that it looks the same
wrapped as unwrapped, which the assertion beside it already states.
- The dropped-precmd_functions case asserted HISTFILE was no longer the scoped
path. Whether the scoped value survives at all is the host's call: macOS
/etc/zshrc overwrites HISTFILE so it does not, and a host with no such
assignment keeps whatever the spawn env set. Now compared against an unwrapped
pane given the same env, which is the real contract on both.
Also notes, where the emulation cases live, that they only discriminate on a host
whose system zshrc clobbers HISTFILE -- on CI's Ubuntu the load-bearing assertion
is ORCA_HISTFILE having been consumed.
* test(shell): re-pin the fixes the four-file wrapper was built for
Archaeology over the removed blocks: each existed for a bug, so each needs the
bug shown to be unreachable rather than just the code gone. Six restored or added,
each naming the change that introduced the behaviour.
- #8003, twice: the wrapper sourced from a relocated root, and from a non-ASCII
(token-range) one. The old file baked its generation-time path in and had to
re-derive the runtime one from %x to avoid using it; this one bakes nothing.
Both runs assert ORCA_SHELL_FEATURES came back consumed, so 'the user's .zshrc
loaded' cannot pass on a pane that never read the wrapper at all.
- #4667: user startup files must see their OWN ZDOTDIR while they run, or plugin
and theme lookups resolve into Orca's dir. The old wrapper swapped ZDOTDIR
around each source; this one never takes it away, and the values now have to
match an unwrapped pane's.
- #1947: a user .zshenv that returns early.
- #15258: an inherited ZDOTDIR that is an Orca wrapper dir must be refused. CI
proved this route is live -- the launch config only sets ORCA_ORIG_ZDOTDIR when
it resolved a usable dir, but a pane inherits its parent's environment too.
- #11044/#11146: a nested Orca inherits neither cross-process channel and no
ZDOTDIR of Orca's, which is what makes #11044's plain shape unreachable rather
than repaired. Verified the child-env probe detects a real leak before trusting
it to report the absence of one.
|
||
|
Neil
|
fdd4091ebd | fix(hooks): isolate lint-staged backups per worktree (#15388) | ||
|
Neil
|
cb95582cea | feat(release): build unsigned Windows artifacts for the dev channels (#15465) | ||
 Brennan Benson
|
15d2e31777 |
ci: bound the shell-contracts apt install so a stalled mirror cannot wedge the run (#15532)
* ci: bound the shell-contracts apt install so a stalled mirror cannot wedge the run shell contracts wedges intermittently. It is not a lock, not a prompt, and not the PR under test - it is download throughput with no wall-clock bound. Measured on a passing run: apt-get update fetched 11.4 MB of index in 40s, then apt-get install fetched 8.9 MB of packages at 65 kB/s taking 2m17s, while the shell-contract tests the job exists to run took 14s. apt applies no wall-clock bound to a stalled mirror, so when throughput drops below that already-poor baseline the step runs indefinitely - observed at 12+ minutes and climbing while every other job had passed. The job also had no timeout-minutes, so it inherited GitHub's 6h default, and an in-progress required check holds the whole run open and blocks rerun --failed. Bounds both layers: timeout-minutes on the job (a passing run is ~4m30s), and Acquire timeouts plus retries in apt.conf.d so a dead mirror fails fast while a transient blip still passes. Written to apt.conf.d rather than onto the command lines because pr-workflow-parallelism.test.mjs parses those invocations. Does not make the job faster; the 3m38s of download is untouched. Scoping the index refresh to just the PPA risks installing against a stale base index and wants its own evidence. * ci: bound the apt commands by wall clock, not per-connection timeouts The first attempt at this set Acquire timeouts of 30s with 3 retries. That made the wedge worse and the job's own timeout-minutes proved it: on this PR the install step ran 14m26s and was killed by the 15 minute bound. The log shows why. Acquire timeouts are per-connection, so a dead mirror costs timeout x retries x every index file: 30s x 3 across roughly ten index files is ~15 minutes, which is what was observed. The azure archive mirror returned Ign for every suite, apt fell back to archive.ubuntu.com, and that connection then produced zero bytes for 14m26s. So per-connection bounds cannot bound this step; only a wall-clock bound can. Wraps both apt invocations in `timeout`, and drops Acquire::Retries to 1 so a dead mirror fails once instead of multiplying. The update is already tolerant by design, so bounding it just caps what a dead mirror costs before the install runs against whatever index exists. Also drops DPkg::Lock::Timeout: no lock contention was ever observed in these logs, and an option added on speculation is not worth carrying. |
||
 OrcaWinandOrcaWin
|
4ec6bbf588 |
Kill hung WSL transcript filesystem operations via child process with route quarantine (#15381)
* fix(native-chat): kill hung WSL operations via child process
Stalled UNC file operations hold libuv permits even after the gate
timeout expires, blocking Chat tab recovery. Two stalled operations
fill both permits and freeze all WSL access until restart.
Fork file I/O for UNC paths into a separate child process. On deadline
expiry, kill the process to force the hung syscall to exit. This frees
the permit for the affected tab's next read. Temporarily quarantine the
stalled route to avoid retry storms.
* chore: drop internal review artifact from the repo root
* fix(native-chat): harden the WSL transcript fs sidecar
Review follow-ups on the sidecar isolation change:
- Only the deadline may abort running gate work. The sole waiter's
same-duration timeout fired first, killed healthy children on caller
abandonment, and settled the task before the deadline could quarantine
a stalled route - leaving the back-off dead for every dedupe:false op.
- Resolve the fork entry from out/main/chunks too: the resolver compiles
into a shared chunk, and the scanner service child has no
process.resourcesPath, so packaged WSL vault scans threw entry-not-found
(masked as an empty tree).
- Allowlist the fork env instead of spreading process.env; ambient
NODE_OPTIONS would halt or --require code into every child.
- Wrap transport faults (spawn failure, child death) in
WslTranscriptFsError('unavailable') so discovery reports them as scan
issues instead of misreading them as missing paths or empty trees.
- Gate the vitest in-process fallback on the vitest worker global so a
leaked VITEST=true cannot revert production to in-process UNC syscalls.
- Reap idle sidecar processes after 60s instead of holding them for the
app session.
- Split 'open' into its own protocol union member so the reusable-call
Exclude actually strips it from the pooled-process API.
- Guard kill('SIGKILL') against the teardown race where an exiting child
emits an unlistened 'error', and dispatch reads by handle kind before
path spelling.
* fix(native-chat): probe stalled WSL routes instead of a fixed quarantine
Remaining review follow-ups:
- Escalating route quarantine: first strike lifts after 5s so a distro
that was cold-booting when its op hit the deadline recovers on the
next poll (~35s total instead of ~90s); repeat stalls double the
back-off toward the prior 2x-timeout cap, and any settle the deadline
did not force clears the strikes. Queued same-route tasks fail fast
at quarantine instead of stranding one waiter deadline per file in
sequential scans.
- Single request implementation: the vitest in-process fallback now runs
the child's own dispatcher (WslTranscriptFsProcessOperations + decode),
so unit suites exercise exactly what the forked process executes and
the per-call-site fallback closures are gone. Dirent fixtures gained
the full kind-flag set the serializer reads.
- Dropped the production-dead per-route close queue; UNC FileHandles
(test fallback only) mirror the process-handle close contract.
- Error class, messages, and factories move to wsl-transcript-fs-error
(re-exported from the gate) to keep the gate under the lines budget.
* fix(native-chat): harden WSL transcript fs with route quarantine strike
Extract quarantine logic into a dedicated module with strike decay: stalls older
than 5 minutes restart from base back-off, and concurrent-lane timeouts count as
one incident. Allow joining live in-flight tasks on quarantined routes (they cost
no new I/O). Preserve quarantine across transport faults (child death). Handle
file shrinking during tail reads by detecting short reads and returning empty.
Defer file closes that arrive mid-read instead of refusing, preventing slot
leaks. Separate process slot and boundary-finding concerns into focused modules.
* fix(native-chat): enforce route quarantine windows and isolate lanes per
A late result arriving after the deadline was incorrectly lifting the route
quarantine, allowing subsequent work to start before the back-off period
expired. Now late results are correctly recognized as stale and never cut
the quarantine short.
Process work is now isolated per (route, priority) lane so a scan stall
cannot block exact reads on the same distro. Each lane gets its own client
and process pool; late results and handle faults stay scoped to their lane.
Tests now fake performance.now() alongside timers (the quarantine clock
depends on it) and wait for the full back-off window to expire rather than
advancing by 0. Gate state is reset between test cases since late releases
never lift the quarantine.
---------
Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
|
||
|
Neil
|
487e43d619 | refactor(test): use __fixtures__ for wrapper snapshots and teach the LoC bot (#15365) | ||
|
Neil
|
a3edabcd7b |
fix(package): keep cached dev Electron bundles out of app.asar (#15359)
`files` is an all-negation list, so electron-builder's default `**/*` packs anything without an explicit `!` entry. out/electron-dev holds `pnpm dev`'s per-branch Electron.app copies (~270MB each), so packaging on a machine that has run dev bundled them all. CI never creates the directory, so releases were never affected. |
||
|
Neil
|
a3da91b10a | fix(dev): stop caching unsigned bundles and reclaim stale dev copies (#15247) | ||
|
Neil
|
66a5e5d245 |
fix(shell): repair worktree HISTFILE in plain zsh panes via one positive feature channel (#15258)
* fix(shell): repair worktree HISTFILE in plain zsh panes via one positive feature channel
A plain zsh pane — no startup command, no agent overlay — was never wrapped, so
Orca's HISTFILE repair never ran in it. macOS `/etc/zshrc` assigns
`HISTFILE=${ZDOTDIR:-$HOME}/.zsh_history` with no check-before-set and runs
before any file Orca controls, so per-worktree history was a silent no-op for
every ordinary pane on the primary platform.
Wrapping those panes needs a way to say which wrapper features a shell should
turn on. That channel is one exported variable, ORCA_SHELL_FEATURES, carrying a
comma-separated positive allowlist from a closed set (history, markers, ready,
identity, overlay). The wrapper .zshenv reads it into a plain, non-exported
array and unsets it in its first executable lines, before the user's own
.zshenv — so the selection survives .zshenv -> .zprofile -> .zshrc -> .zlogin in
this process but physically cannot reach a child. There is no negative or
suppression variable anywhere; an absent or inherited value can only ever mean
fewer features. ORCA_HISTFILE is consumed and destroyed the same way, which
removes the root cause of #11146 instead of patching it.
All order-sensitive wrapper work now lives in one `__orca_shell_epilogue`
defined in .zshenv and invoked exactly once, from .zshrc for a non-login shell
and .zlogin for a login shell, with each feature an independent guard.
Selection is a pure function of spawn env and launch intent, so a pane wrapped
only for history gets no OSC 133 and is observably identical to the unwrapped
pane it used to be.
Generation is now fail-closed: wrapper files are written to a temp name and
renamed, every required path is verified non-empty, and ZDOTDIR is only set when
that holds. Previously a failed write still pointed ZDOTDIR at an empty dir and
the user silently lost their entire zsh config.
Orca also recognised only its own `*/shell-ready/zsh` dir shape when deciding
what the user's ZDOTDIR was, so being launched from any other terminal that had
hijacked ZDOTDIR captured that as the user's config dir. Ownership is now
established positively — a stamped marker file, or Orca's own dir shape for
wrappers written by older builds — and an inherited ZDOTDIR holding no zsh
startup file is ignored. No vendor is detected by name.
* fix(shell): make the zsh epilogue option-proof and stop history widening relay wrapping
Review follow-ups on the feature-channel PR.
- `emulate -L zsh` as the epilogue's first statement. It runs after the user's
own config, so `setopt no_unset` made the precmd_functions append a fatal
error that returned from the whole function (no ready widget, ZDOTDIR left at
Orca's wrapper dir), and `setopt ksh_arrays` made the 1-based feature
subscript drop whichever feature is listed first.
- The `/etc/zshrc` HISTFILE repair is no longer behind the `history` guard: it
undoes damage Orca's own ZDOTDIR caused, so it must also run for a shell that
re-enters the wrapper after the allowlist was consumed.
- The relay keeps its own wrapping gate. Its .zshenv resolves the user's config
dir from a ZDOTDIR Orca has already overwritten, so wrapping a remote pane
just for `history` cost a relocated-ZDOTDIR user their whole shell config.
- A failed primary spawn no longer leaks the primary shell's launch env
(wrapper ZDOTDIR + feature channel) into an unwrapped fallback pane.
* fix(shell): drop the relay wrapping gate and stop HISTFILE inheriting across Orca instances
The relay-specific gate added last round rested on a false premise:
main's hasOverlayRestoreEnv already included ORCA_REMOTE_CLI_BIN_DIR, and
ssh-pty-spawn-env sets that on every SSH pane whose session has a CLI
bridge — so ordinary remote zsh panes were already wrapped. The gate only
bit where remoteCliBridgeEnv is null (a host too old to report its
platform), where it silently dropped that pane's worktree history. All
three transports now share the features.length rule.
HISTFILE stays exported, so a newly wrapped pane handed the worktree
history path to every child, including a nested Orca whose panes then all
hit injectHistoryEnv's check-before-set and appended into the launching
worktree's file. Same class as the fish_history fix in #15195: recognise a
path Orca minted and drop it before the check, on the desktop, daemon and
relay injection paths and both history-disabled branches.
Also: run the epilogue from the wrapper .zshrc when zsh is in sh/ksh
emulation, since sourcehome() then reads $HOME/.zlogin and the wrapper's
.zlogin never runs; track fallback launch-env keys per attempt rather than
once from the primary; and guard the cross-file epilogue call so a wrapper
dir shared by two builds degrades quietly.
* fix(shell): make every wrapper file self-sufficient and retire the deleted marker vars from tests
- .zprofile/.zshrc/.zlogin each define __orca_resolve_user_config_dir. They
called it on line 2 while only .zshenv defined it, so a wrapper dir written by
two concurrently installed builds printed three "command not found" and
skipped the user's entire zsh config. New live-shell test covers it.
- Retarget every remaining ORCA_SHELL_READY_MARKER/ORCA_SHELL_STARTUP_IDENTITY
reference onto ORCA_SHELL_FEATURES, or delete it where the key is now dead.
- isOrcaMintedHistFile requires a leading '/', so a relative path of the same
shape stays the user's.
- Drop an unused no-control-regex disable, and register the two real-zsh suites
in the dedicated shell-contracts lane.
* fix(shell): stop the zsh wrapper colliding on REPLY and degrade under sh emulation
Widening wrapping from overlay/startup panes to every zsh pane turned three
latent wrapper defects into user-visible ones.
- The config-dir resolver used `REPLY`, zsh's shared scratch global, as its
out-parameter. `typeset -r REPLY` in a user config made the wrapper's first
executable assignment fatal, `typeset -i REPLY` silently resolved every path
to 0; both left HISTFILE inside Orca's wrapper dir. It now writes an
Orca-private `_orca_resolved_config_dir`, declared `typeset -g` so the
rename introduces no `warn_create_global` noise. A new rule test fails on any
generated wrapper file that writes a global outside Orca's namespace.
- The daemon dropped an inherited HISTFILE but never an inherited
ORCA_HISTFILE, which now both wraps a pane the client scoped nothing for and
re-exports another worktree's history path. The relay had the same gap on its
isolation-off and revive paths. Both now mirror the desktop.
- A user .zshenv or .zprofile ending in `emulate sh` makes zsh ignore ZDOTDIR,
so no later wrapper file is read and the epilogue never runs. Nothing can
repair HISTFILE from there, so the wrapper now detects the emulation and
hands the pane back unwrapped instead of leaving history somewhere invisible.
Also `typeset -g __orca_in_command` so the OSC 133 preexec hook prints no
warning under `setopt warn_create_global`.
|
||
|
OrcaWinandOrcaWin
|
6e8da1df8d |
fix(wsl): pass guest argv verbatim through --exec (#15039)
* fix(wsl): pass guest argv verbatim through --exec
`wsl.exe <...> -- <argv>` expands `$name` in every argument against the
guest environment before the guest ever runs. It does this even when no
shell is involved, so `-- /usr/bin/printf %s '$HOME'` prints /home/you.
Every WSL invocation went through that preprocessor, so scripts arrived
already rewritten: `awk '{print $2}'` lost its field reference, and a
POSIX script asking for the literal `$HOME` got the expanded path.
`escapeWslShCommandForWindows` tried to compensate by escaping `$`, but
it skipped any `$` preceded by a backslash, so a script containing `\$`
was still corrupted -- and half the call sites never applied it at all.
Route every invocation through `--exec`, which passes argv through
untouched, and delete the escaper. The direct-git path already used
`--exec`, so this is not a new compatibility dependency.
A guard test fails if the `--` form reappears anywhere in the tree.
Net -50 lines of production code.
* test(wsl): drop remaining escaped-dollar assertions
* fix(wsl): cover the --exec migration's blind spots
An audit of every wsl.exe invocation found sites the first pass missed,
including two it actively broke:
- config/scripts/wsl-git-shell-benchmark.mjs imported
escapeWslShCommandForWindows, which no longer exists, so the script
threw on startup. Its wslShellArgs helper also still used `--`; the
file already had an --exec helper, so route both call sites there.
- classifySubprocessCommand unwrapped `wsl.exe <...> -- <binary>` by
breaking on `--` alone. With every Orca spawn now on --exec it never
found the guest binary and bucketed all WSL subprocesses as plain
"wsl", losing the git/gh/glab breakdown. Break on either separator,
since foreign wsl.exe processes still use `--`.
CliSkillRuntimeSetup builds its setup command as a template literal
rather than an argv array, so no array-shaped search could see it. Its
decoder accepts both separators so commands persisted before this
change still decode.
The guard now scans config/ and tests/ as well as src/, and checks the
command-string spelling alongside the argv one — the two shapes that
have each shipped a regression. It skips comment lines so prose about
the old form stays allowed, and asserts it scanned a plausible file
count so a bad root cannot make it vacuous.
* fix(wsl): restore the guard's multi-line sensitivity
The guard matched line by line, so `'--',\s*'bash'` could not span a
newline -- and every argv array in this repo is formatted one element
per line, which is exactly the shape it exists to catch. Measured
against the pre-migration tree it caught 17 files before and 9 fewer
after. It now strips comment lines and matches the rejoined text, with
a case that pins the multi-line shape so this cannot silently return.
The program list is wider than shells now, which surfaced a false
positive: tmux takes a `--` separator followed by a program too
(`split-window ... -- cat`). Matching is scoped to files that mention
WSL rather than narrowing the list back.
Also:
- Replaced the `sed` regression case, which was vacuous. A backreference
contains no `$`, so it returned `bac` under both separators and would
have passed without the fix. The block claimed every case proved the
bug. Swapped in a positional argument and a shell local, both measured
to differ -- the positional is the shape `wslUncDirectoryExists` uses,
where `--` blanked `$1` so every existing directory probed as missing.
- windows-shell-args.test.ts derived its expected argv from
buildWslExecArgs, the helper under test, so six assertions would still
pass if it regressed to `--`. Spelled the expectation out.
- Dropped two comments citing the removed `--` behavior as rationale.
---------
Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
|
||
|
Brennan Benson
|
fc8b92e507 |
docs(computer): explain screenshot file requirements (#15054)
* docs(computer): clarify screenshot output requirements * fix(cli): do not advertise an unshipped --probe flag The capabilities help line referenced --probe, which does not exist yet; it ships in a later change. Advertising it here would be false until then. * fix(cli): align computer-use screenshot guidance * docs(computer): document inline screenshot fallback * docs(computer): keep screenshot summary accurate * docs(computer): keep screenshot guidance general |