mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
e65d2dadeb21323e0284b310f763545cc4641dbb
300
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 OrcaWin
|
1ec13cbda2 | Speed up CI dependency and computer E2E setup (#17513) | ||
 Neil
|
e84042572c |
Upgrade xterm to 6.1.0-beta.303 and generate addon patches
* Upgrade xterm to 6.1.0-beta.303 and generate the addon patches
Takes the current xterm beta line: xterm 287 -> 303, addon-webgl 286 -> 299,
addon-serialize 287 -> 300, headless 302, the remaining addons -> 300, and the
same set on mobile. All four packages stamp upstream commit d3e32b3.
The reasons are upstream #6042/#6043/#6055 (a shared glyph atlas no longer
garbles sibling panes on a page merge, clear, or sampler-budget overflow) and
Note that core 303 is not image-addon-only over 302: it carries the buffer perf
work, including the new BufferLineStringCache.
addon-webgl and addon-serialize move into the patch generator
--------------------------------------------------------------
Both were hand-edited minified bundles, which is what the Known Gaps section of
docs/reference/xterm-patch-regeneration.md described. Both reproduce byte for
byte from the pinned commit, so they are now manifest entries generated from a
source patch like @xterm/xterm already was. Their sourcemaps now move with their
bundles; before this they shipped maps whose offsets did not match the code
beside them.
The webgl patch shrinks from a 1.06 MB hand-edited bundle to a 6.6 KB source
patch, because upstream took the invalidation half Orca had backported. What is
left is only what upstream still lacks: the fragment-shader else branch for a
v_texpage past the sampler budget, the clearTexture guard that no-ops once a
merged page holds index 0, spending the merge retry budget before beginFrame
latches the version it saw, and Orca's font-weight probe.
The serialize source patch is byte-for-byte the same fixes as before; upstream
changed nothing in that addon between 287 and 300.
Generator fixes, each of which failed silently
----------------------------------------------
- `--relative` was appended after the `--` separator in CHECKOUT_DIFF_FLAGS, so
git read it as a pathspec and kept repo-root-relative paths, dropping every
source hunk from an addon's patch.
- `git apply` run from a package subdirectory still resolves patch paths from
the repo root, skips every hunk and exits 0. It now runs from the root with
`--directory=<packageDir>`, and a source patch that leaves the checkout
unchanged is a hard failure rather than an empty patch.
- An addon's own `tsgo -p .` has empty files/include and only project
references, so it emits nothing and the addon webpack then fails on a missing
./out/. The root build now runs first.
- versionStampFile is optional; publish.js stamps an addon's package.json, which
overlayBuildOutput never patches.
- On a version bump the lockfile has no entry under the new key yet, so --write
reports the gap instead of aborting mid-run. --check still fails on it.
Adding the two addons pushed the generator and the Electron packaging contract
test over max-lines, so the patch-text helpers move to xterm-patch-text.mjs
(pure text: no checkout, no build) and the vendored-xterm assertions move out of
the packaging contract into xterm-webgl-runtime-contract.test.mjs.
Tests
-----
Four tests asserted upstream bugs that are now fixed, not Orca behaviour:
- xterm-user-scrolling-contract pinned headless and core by version string.
Upstream bumps each package only when its own output changes, so headless 302
and core 303 are the same source. It now asserts they share a commit.
- Five CSI 3 J assertions expected a reader stranded at the top after an erase.
Upstream #6081 clears isUserScrolling there, so the erase releases them to the
bottom instead. Orca's pin still lands them correctly, because its parser
handler observes the erase before xterm's own handler runs.
- The IME transaction test hard-coded the xterm version; it now reads the
installed package, since the point is that bundle, map and version agree.
- The Electron runtime contract asserted Orca's old clearModelGeneration. Shared
atlas invalidation is upstream's now, so it asserts pageLayoutVersion on the
resolved dependency, plus the Orca-only hunks on the patch.
Verified: 66,008 unit tests, mobile's 3,863, the four WebGL atlas e2e specs, and
`regenerate-xterm-patches.mjs --check` in sync on all three packages.
Left alone deliberately: resetAllTerminalWebglAtlases still fans out globally
even though clearTexture now self-heals siblings, and upstream #6068
(WebglAddon.dispose leaks the GL context) is still open.
* Drop the two unused WebGL atlas fan-out exports
resetAllTerminalWebglAtlases and presentAllTerminalPanesWithoutAtlasClear have
no callers, and had none at
|
||
  Brennan BensonandMerge Sim
|
585b4086d3 |
test(codex): pin Codex read-repair with a real-binary contract check (#17300)
* test(codex): pin Codex read-repair with a real-binary contract check
Orca's session index-heal depends on a Codex behavior: a `thread/read` of an
unindexed rollout performs a read-repair that inserts the `threads` row. All 55
existing heal tests drive a stub app-server and assert "healed" as "the call did
not error", so if Codex ever dropped the repair they would all stay green while
the subsystem went silently inert.
Adds a real-binary contract check built to the same shape as the Git binary
compatibility contract (src/shared/git-binary-compatibility.test.ts): env-gated
test file, version asserted against the binary, dedicated path-filtered PR job.
Pins only the four arms ablation established Orca relies on:
- a read of an unindexed rollout inserts the state row
- a session with no read inserts nothing (the negative control that makes the
insert causal rather than incidental)
- re-reading an indexed thread inserts nothing
- an archived thread stays archived rather than being resurrected
Written against codex-cli 0.150.1. The job sets ORCA_CODEX_CONTRACT_REQUIRED=1
so a missing or failed CLI install fails red instead of silently skipping.
Existing heal tests are unchanged.
* test(codex): register the contract job in the verify aggregate contract
`pr-workflow-parallelism.test.mjs` pins `verify.needs` exactly, so adding the
job to pr.yml without updating that list failed the shard. Adds the entry, and
adds a workflow contract test mirroring `git-binary-compatibility-workflow.test.mjs`:
- the pinned CODEX_CLI_VERSION is the single source for both the npm install
and the runtime version assertion, so the two cannot drift apart
- the install prefix and the binary path the test is pointed at are the same tree
- ORCA_CODEX_CONTRACT_REQUIRED=1 is set, so a failed install fails red rather
than turning the job into a green no-op
Removing the REQUIRED env from pr.yml reddens the new test, confirming it is live.
* test(codex): make binary version guard exact and bounded
* ci(codex): cover index-heal transport dependencies
* test(ci): pin Codex contract dependency coverage
* test(codex): align contract watchdog with child deadlines
* test(codex): cover three-session contract watchdog
* fix(codex): add sqlite sync-database to index-heal scope
---------
Co-authored-by: Merge Sim <sim@local>
|
||
|
Neil
|
7b467bd0a6 |
ci: gate PRs on a real input method, and prove the lane engaged one (#17365)
* ci: gate PRs on a real input method, and prove the lane engaged one No job on the PR gate has ever run a real input method. pr.yml and e2e.yml are ubuntu-latest with CDP `Input.imeSetComposition`, which is a synthetic composition; the only job that drives ibus-hangul through xdotool is terminal-ime-e2e.yml, and it is schedule + dispatch only. A PR could turn the real-IME path red and merge green. Route IME source to that lane from pr.yml through the existing pr-e2e-source-routing mechanism, so it runs on IME-touching PRs and nothing else. The lane stays out of verify.needs — advisory, like `e2e` — because its reliability is known only from nightly main runs. Deliberately no continue-on-error: that reports green and hides the signal. The harness fails open in ways that all look like success: Playwright reports a skipped test as a pass, so an unset ORCA_E2E_NATIVE_IBUS_HANGUL, a renamed test, or a session with no engine all exit 0 having exercised nothing. The specs now append an engagement receipt only after observing real composition events, and the runner requires one per expected test before the lane may report success. Also drop the native spec from changed-e2e: it was already routed there by its own filename, where it self-skips for want of an ibus session and reported that skip as coverage. * ci: let the real-IME step report even when the synthetic step failed |
||
 Jinwoo Hong
|
252dbd60ea |
fix(terminal): restore lossy initial remote snapshots (#17113)
* fix(terminal): restore lossy initial remote snapshots * test(terminal): strengthen lossy snapshot causal oracle |
||
|
Neil
|
63ff0a515d |
Prime native cache before E2E fanout (#17280)
* Prime E2E native cache before fanout * Update E2E permission contract |
||
|
Neil
|
b17f60d744 | build: upgrade to pnpm 12 (#17156) | ||
|
Neil
|
51ed7d4f67 |
Pin pnpm and rebalance scheduled E2E (#17133)
* Pin pnpm and rebalance scheduled E2E * Give scheduled E2E failure headroom |
||
|
Neil
|
92ab618a11 |
Repair scheduled computer-use CI (#17122)
* Repair scheduled computer-use CI * Make Calculator E2E Windows-version neutral * Handle classic Calculator accessibility panes * Update Calculator E2E source contract |
||
|
Brennan Benson
|
6bef6d2727 |
fix(ci): stop the Linux Electron probe step from starving its own probes (#17071)
* fix(ci): stop the Linux Electron probe step from starving its own probes The package job's "Test Linux Electron lifecycle boundary" step ran five Electron probe files under Vitest's default file parallelism, so four full Electron stacks competed for a 4-vCPU runner. Each probe carries its own in-process deadline (20s for WebRTC, 25s for H3), and every observed failure was one of those deadlines expiring: exit code 2, "no result", with every sibling file in the same run slower than its own green maximum. Run the step with --no-file-parallelism so each probe owns the runner, and stop each probe nesting a private `xvfb-run --auto-servernum` X server inside the step's own xvfb-run: reuse an inherited DISPLAY, and only own one when there is none (shards, local dev), which leaves those lanes unchanged. Also set each Docker-SSH E2E step's Playwright output aside before the next step starts, because Playwright empties test-results/ on every run and only the last lane's traces survived to the artifact. * fix(ci): route the persisted-worker probe through the same display resolver |
||
|
Brennan Benson
|
fd9125ea8c |
feat(native-chat): Codex structured native chat restructure (#16729)
* feat(native-chat): port structured Codex sessions from restructure-recovery Rebuilds the desktop structured native-chat implementation from brennanb2025/native-chat-restructure-recovery (tip 4e31c08db3) on top of current main as a single commit, scoped to the local Codex path. Ported: - Structured agent-session core: durable record store + single-writer lease, canonical journal, agent-session wire host/attach/eviction/subscribers, `agentSession.*` RPC surface (registered via ALL_RPC_METHODS; host-side mobile allowlist included for wire compat), pty write gate, transcript additions, and the Codex app-server adapter/launch resolution. - Renderer: NativeChatStructuredSession view/composer stack, structured launch path with the single-flight guard, local structured session tabs sync, activation gate + structured inventory (read-only `agentSession.handoffStatus` probe), agent-session tabs in the tab strip, AI-vault structured session activation, and the settings pane with the parent Experimental Chat UI toggle plus the nested "Use updated structured native chat" toggle. New sessions require both flags, agent codex, no prompt, and a local non-WSL, non-Windows-host execution host (structured-native-chat-availability). - Fixes 72c013cea6 (verified Codex launch recovery), 8ddbaf5e3d (defer native terminal view switching affordances), and 4e31c08db3 (release the launch gate after a visibility retry) with their regression tests, including the third-launch-after-retry guard case. - Cross-version agent-session wire test + CI lane, packaging entries (proper-lockfile, agent-tooling asar excludes), and the wire-compat doc section. Deliberately not ported: mobile/ changes, the Claude structured runtime (only the claude-transcript-branch-proof and claude-structured-owner-identity leaf modules remain, backing the kept TUI-recovery arms), the terminal↔chat adoption/handoff flow (`agentSession.adoptTerminal`/`requestHandoff`, the handoff request engine, TUI adoption machinery, orca-runtime adoption methods), renderer switching affordances and their dead leftovers, the hook/subagent-status refactor cluster, and unrelated branch changes. The crash-during-acquisition recovery path (restart handoff adjudication, restore/reverse re-acquire, lease schema handoff keys) is kept because every plain direct launch depends on it; a trimmed handoff coordinator exposes only status/restore/close. Branch edits that targeted files main has since split (ipc/pty.ts, worktrees.ts, rpc/methods/terminal.ts, useIpcEvents, pty-connection, store/slices/terminals.ts, runtime-types, web preload) were re-applied to the split modules, preserving main's newer logic (Windows CIM fallback, browser tab close rework, cold-restore resume flow, dispatcher threading). Known seam: the mobile clipboard image-provenance CONSUMER gate ships (agentSession.send refuses unproven mobile image refs with agent_session_image_untrusted) but the producer hunk in rpc/methods/clipboard.ts stays with the unported mobile cluster, so mobile image sends into structured chat fail closed until that side ports. * fix(native-chat): trust only authenticated local image uploads * fix(build): preserve Windows process-tree patch application * test(windows): include process creation time in addon fixture * fix(build): run windows-process-tree node-gyp from the physical package dir gyp expands the node-addon-api dependency by probing node, whose cwd resolves to the package's physical directory in the store, so the emitted target is a store-relative ../../../../node-addon-api@... hop. gyp then resolves that hop against the rebuild cwd; from the node_modules symlink/junction it escapes the store and configure fails with "node_addon_api.gyp not found" (run 32999886072). Rebuild from realpath(package dir) so both bases agree, matching how the package manager itself runs native install scripts. The regression test replays gyp's expansion+resolution against the planned cwd and fails without the fix. * fix(native-chat): keep chat tabs visible through terminal closes and empty-worktree launches Two proven blockers in the native Codex tab contract: closeTerminalTab pre-empted the canonical unified close. With one terminal left it deactivated the worktree on a terminal/editor/browser-only check, blanking a workspace that still held a renderable agent-session tab; with two or more it pre-picked a successor from terminal entities only, re-stamping the group active before closeUnifiedTab's MRU/neighbor repair could land on the chat tab. Successor choice now defers to the unified contract whenever the terminal has a unified row, and deactivation is gated on the unified renderable count (matching leaveWorktreeIfEmpty), with the legacy pre-pick kept only for terminals without a unified row. A structured session created on an empty worktree was published into the host's headless group while preserveLocalLayout froze the local layout, leaving the tab in store but permanently off screen. A preserveLocalLayout owner now always takes client-owned placement — repairing a rendered leaf whose group record is missing, or materializing a rendered group on a truly empty worktree — and applies the client-derived layout repair while still rejecting host-authored layout. Regression tests drive the real store through closeTerminalTab (git worktree and folder workspace) and the real snapshot applier for the empty-worktree adoption states; all fail without the fixes. * fix(native-chat): close stale turns and retry rejected sends * fix(native-chat): retire hosted rows on structured tab activation * fix(native-chat): preserve rpc defaults across main merge * chore: format remote wire compatibility guide * test(native-chat): cover retry after unconfirmed send * fix(native-chat): reload outbox on session switch * docs(settings): disclose structured chat platform limits * fix(native-chat): await Codex launch-home preparation * fix(codex): align child-process allowlist with async trust bridge * test(identity): update inventory for tab surface refactor * fix(windows): preserve process-tree CRLF patch sources * fix(native-chat): anchor an unmatched chat echo where it was sent (#16117) * fix(native-chat): anchor an unmatched chat echo where it was sent The reported symptom was old user messages replaying below every new turn, so the conversation read as scrambled. The cause was not that the echo failed to match a transcript row. Claude consumes a mid-turn send through a `queued_command` attachment and writes no `type:"user"` record for it, so some echoes can never match, and no amount of matching will change that. The cause was WHERE an unmatched echo rendered: buildMobileNativeChatTransientData appended every pending item after the entire transcript, so it re-read below each turn that landed afterwards. Render each echo directly after the transcript row it was sent against, using the baseline the send already captures. An unmatched echo is then at worst a duplicate in the right position rather than a scrambled one, and it stays visible. Echoes sharing an anchor keep send order; a send with no baseline, or one whose anchor folding dropped, still falls back to the tail. Deliberately NOT fixed by deleting the echo. Inferring from send ordering that an echo can never match, then removing it, loses the user's own text for a message the agent did receive, and it cannot fire in the common case anyway - measured drain groups are 1,017 of size 1 against 55 larger. It also escalates an existing gap: the count pass has no baseline-tail guard, unlike the glue pass, while `messages` is a 40-row window that head-trims, resets on reconnect and grows at the front on loadEarlier, so a false landing there would license deleting a DIFFERENT outstanding message. That count-pass gap is real and left for a separate change; anchoring makes its worst case a duplicate in place rather than a scrambled conversation. * fix(native-chat): preserve folded echo anchors * fix(native-chat): preserve forward-folded echo anchors * fix(native-chat): keep leading folded echoes in place * fix(workspace-cleanup): show git status for every row (#16690) * fix(native-chat): refuse structured chat on every Windows execution path canUseStructuredNativeChat only refused win32 when a project runtime resolved, so folder-workspace keys (and other keys with no project runtime) failed open into structured chat on Windows. Fail closed on win32 unconditionally after the host check, matching the settings copy: local macOS/Linux only; Windows/WSL/SSH stay on terminal chat. * fix(native-chat): restore runtime refusals behind the win32 gate |
||
|
Jinwoo Hong
|
86b770e448 |
fix(release): trust Linux floor workspace (#16988)
* fix(release): trust Linux floor workspace * test(release): ratchet workspace trust scope |
||
 Jinjing
|
c4b39295c1 |
style: format codebase (#16935)
* style: format codebase * style: format codebase * refactor: extract skill install dialog footer and content Extract footer and content sections from SkillInstallDialog and SkillInstallManagementDialog into separate components for improved maintainability and clarity of component responsibilities. |
||
|
Jinwoo Hong
|
59515beb70 |
fix(release): recover immutable patch validation gates (#16984)
* fix(release): recover immutable patch validation gates * test(e2e): locate wrapped terminal file links * test(e2e): keep sibling file links on one terminal row |
||
|
Neil
|
6f8c5888b3 |
Run Node 26 compatibility daily instead of per PR (#16946)
* Run Node 26 compatibility daily * Update relocated unit workflow contracts |
||
|
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 |
||
|
Brennan Benson
|
928d306b53 |
fix(agent-hooks): stop the Windows hook launcher spelling the AV-denied flag pair (STA-5237) (#16739)
* fix(agent-hooks): stop the Windows hook launcher spelling the AV-denied flag pair (STA-5237) `-WindowStyle Hidden` + `-EncodedCommand` is denied at CreateProcess by Kaspersky on Windows 11, whatever the payload decodes to. Bash reports it as `Permission denied` and every managed hook event fails, so agent status never arrives; the parent shell also briefly cannot spawn anything afterwards, so a denied hook can take the user's next command down with it. Measured on the reporting host (#16003), with a harmless `exit 0` payload: -NoProfile -ExecutionPolicy Bypass -WindowStyle Hidden -EncodedCommand 126 -NoProfile -WindowStyle Hidden -EncodedCommand 126 -WindowStyle Hidden -EncodedCommand 126 -NoProfile -EncodedCommand 0 (5/5) #16576 removed `-ExecutionPolicy Bypass`, which is the one flag of the three NOT in the signature, so hooks kept failing after that fix. The pair that has to stop being spelled is `-WindowStyle Hidden` + `-EncodedCommand`. Because the change is to the shared switch constant, it covers every site that spells the denied pair in one edit: Claude via `wrapWindowsPowerShellEncodedCommand`, gemini/cursor/droid/command-code/copilot via `wrapWindowsHookCommand`, the `runtime-home-hook-command` unsafe-HOME fallback, and the spaced-path fallback for codex/grok/devin/antigravity. Only a flag is removed, so parser and payload compatibility is unchanged for every executor: the string is still a PowerShell command line, still one self-contained token, still base64-shielded. The tradeoff, recorded rather than hidden: `-WindowStyle Hidden` was the shipped fix for #14815 (+#14828, #15117, #15447, #15767), and this removes it. Its suppression was never measured — #14825 confirmed it visually, #16576's author stated it "remains unverified on a real box", and #15506's author argued it cannot help a `.cmd` child with no console to inherit. The console is allocated by the parent chain, not by this command line. A live window measurement is still outstanding and is called out in the PR. Also adds `windows-hook-payload-delivery.test.ts` to the PR CI Windows leg, which had never run it. * test(agent-hooks): keep launcher token out of source grep |
||
|
Brennan Benson
|
f352e3e27d |
fix(cursor): emit Cursor-contract JSON from managed hooks
Merge rebased conflict repair after exact-head tests, typecheck, lint, format, and all required GitHub checks passed. |
||
|
Neil
|
d1a11b3299 |
fix(pty): match the echo shapes a real tty actually produces (#16542)
Reply echo suppression modelled two echo shapes from the spec rather than from
a tty. Captured under node-pty against real bash, at a readline prompt and
under `read`:
- Readline mangles CSI replies, not just OSC: `ESC [ ?` becomes BEL and the
residue echoes. The projection was gated on an OSC introducer, so a private
DSR echo was never matched at a readline prompt. This is the reachable one:
a mode-2031 theme push (`CSI ?997;1n`) left latched by an exited TUI paints
`997;1n` on a bash prompt (#9993's scenario).
- ECHOCTL carets EVERY control, not just ESC. A BEL-terminated OSC reply
echoes as `^G`, but the needle kept a literal BEL — a string no tty
produces. Hardening only: every in-tree OSC reply is ST-terminated
(terminal-osc-color-reply.ts:112, xterm's own reply), so the changed byte
is unreachable except from a foreign or older emulator.
Why this is not the CSI projection #13160 review dropped: that one was the
identity (`replaceAll('\x1b]', …)` is a no-op on a CSI reply), so it was
ESC-led and 500ms-held bare-ESC tails away from the query parser. This one is
BEL-led. The rule is now asserted for every shape rather than implied by the
gate: holdPartial iff the needle does not start with ESC.
The readline branch is keyed on the private-DSR grammar with a non-empty
parameter list, plus a floor on needle length. The containment grammar admits
`CSI ? n`, and `answerLiveQueryReply` takes client-supplied bytes on the relay
path, so a peer could otherwise arm a two-byte `BEL n` needle and delete the
first bell-then-`n` in ordinary output. #61c65151129 proved this system can eat
real output when a needle outlives its budget; a length floor is cheap.
Live coverage: pty-reply-echo-shapes.node-pty.test.ts writes a reply to a real
bash master and feeds back what it echoes, so a shell or libc change fails the
suite instead of silently disarming suppression. Registered in the
shell-contracts lane. The transcript tests and the caretEcho helpers that
encoded the same ESC-only assumption are corrected alongside.
Suppression is display-only. This does not change what reaches the child's
stdin — the reply is written to the master either way, in call order.
|
||
|
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. |
||
|
Neil
|
7c2b5a2334 | ci: rename cross-platform adhoc workflow (#16536) | ||
 Jinwoo HongandJinwoo-H
|
a9781a4118 |
STA-4150: client-hosted remote browser (consolidated) (#15448)
Co-authored-by: Jinwoo-H <jinwoo@stably.ai> |
||
|
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
|
beb1d45198 |
test(windows): run PTY IPC suites in Windows lane
Merged after clean CI, Windows PTY IPC validation, and readiness review. |
||
|
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) | ||
|
Jinjing
|
f5fd7303ab |
test(e2e): cover tab-bar agent launches on Windows and WSL (#16110)
* test(e2e): gate the tab-bar agent launcher on Windows shells and WSL The `+` menu agent launcher had no golden coverage in the Windows lane, so a Windows-only break anywhere in its chain (detection row, startup-plan build, tab create, PTY spawn, startup-command injection) could ship unnoticed. Adds a golden spec that launches a stub agent from the menu and asserts the agent's own banner reached the pane — a tab that spawned a bare shell instead is indistinguishable at the store/tab layer. Runs two agents everywhere, and on Windows also PowerShell, cmd, Git Bash and a WSL project runtime. * test(e2e): track WSL stub agent staging state for precise cleanup Refactor `stageWslGoldenStubAgent` to track which artifacts it creates during setup, then only remove those artifacts during cleanup. This prevents the test from destructively removing pre-existing symlinks or state from previous runs, improving test isolation and idempotency. * test(e2e): track WSL stub agent staging state for precise cleanup - Back up and restore pre-existing stub agents to avoid destroying them - Simplify verbose test comments to match project style guidelines * test(e2e): serialize WSL stub agent setup with distributed lock - Add mkdir-based lock to prevent concurrent staging invocations - Reclaim stale locks after 10 minutes to recover from crashes - Track lock ownership in stage state for safe cleanup * test(e2e): track WSL stub agent staging state for precise cleanup Track which stubs this test helper stages by writing a marker file, then only remove stubs during stale-lock recovery if we created them. Prevents cleanup from removing stubs left by other processes. |
||
|
Jinwoo Hong
|
0a613d5fed | test(e2e): stabilize paired Quick Open large-tree coverage (#16058) | ||
|
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
|
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.
|
||
 Neilandterry-li-hm
|
5f2fbd862e |
fix(pty): wrap codex without expanding or destroying a user alias (#15873)
Co-authored-by: terry-li-hm <12233004+terry-li-hm@users.noreply.github.com> |
||
|
Neil
|
5651662494 |
fix(wsl): migrate 21 call sites onto the WSL runner (#15923)
* fix(wsl): migrate 21 call sites onto the runner, after five review rounds Rebased onto main now that the runner (#15903) has landed. 21 sites across 15 files move off ad-hoc `execFile('wsl.exe', ...)`. Allowlist 23 -> 16 on the WSL guard; 163 -> 152 on the W1 child_process guard, which moved as a consequence. Five review rounds, each finding real defects -- several introduced by the previous round's fixes: 1. Hooks ran user orca.yaml scripts under dash; probe failure fell back to the login shell, reintroducing the ~/.profile stall the runner exists to remove. 2. An unparseable probe was cached permanently, disabling every WSL feature on the distro; hooks regressed from "runs degraded" to "fails". 3. Exit 127 had no expiry; a starved 5s probe hard-failed the 10s scan behind it; a joiner burned its budget on someone else's probe. 4. The comment stripper blanked live code, so the windowsHide guard walked past a real unguarded spawn and reported the file clean; an ownership-probe timeout silently deselected the user's Claude account. 5. Verification of the guards themselves. The recurring finding -- a call answering "is this installed?" on a degraded PATH -- was eventually fixed structurally rather than per-caller: the runner refuses an unresolved guest PATH unless the caller opts in. Per-site vigilance was demonstrably not holding; 3 of 8 sites had already forgotten the analogous exit-code check. Remaining 16 files need a runner mode that does not exist: a long-lived streaming child (OAuth logins, hook relay), a synchronous caller, or a host-level flag like --status that the guest-command API cannot express. * fix(wsl): close round 5's P1s -- degrade where PATH was never needed Round 5 measured the guards by re-executing their algorithms standalone rather than reading them, and found four things. P1 -- four skill/plugin paths gained a hard dependency on the login-shell probe that they never had. They ran under a plain non-login `sh -c` on main, so a probe failure now breaks WSL skill discovery and install on exactly the distro the runner was built for: one with a slow `~/.profile`. Worse, the throw escapes before each site's own error mapping, so the UI gets a raw internal string. They degrade now, per the rule this branch already wrote down in `wsl-fish-history-cleanup.ts`. P1 -- Codex and Claude were asymmetric. Claude's five credential sites degrade; Codex's were strict, so adding a WSL Codex account failed where adding a Claude one succeeded. Three of the four are byte-equivalent to Claude sites, and their scripts read `$HOME`/`$WSL_DISTRO_NAME`, which wsl.exe supplies without a login shell. `assertWslCodexCliAvailable` stays strict on purpose -- that one really does answer "is this installed?" (#9725). P1 -- the ownership-probe timeout fix did not survive the rebase onto main. A timeout still returned "not owned", which the caller *persists*, clearing the user's account selection. P1 -- `blankStringContents` desynced on a nested template literal (`` `${`x`}` ``), leaving 116 lines of a child_process importer outside the ratchet, with 27 importers structurally at risk. Now tracks template depth. Regenerating against the fixed blanker: 70 -> 68 offenders. Also: the windowsHide vacuity check could not fail while the allowlist alone exceeded its bound -- the exact defect the sibling guard documents avoiding. It now names a file that definitely offends. * fix(wsl): close round 6 -- my blanker fix had traded a false positive for a miss Round 6 re-derived the guard's answer from a TypeScript AST instead of trusting the regex, and caught two things. P1 -- the nested-template fix I shipped in round 5 introduced a worse bug than the one it closed. Switching to "code mode" inside `${...}` without also resetting the quote at a newline meant an apostrophe in a regex literal -- `` `'${value.replace(/'/g, "'\\''")}'` `` , which is exactly the shellQuote shape all over this codebase -- inverted the lexer for the rest of the file. `claude-accounts/service.ts` went blind from line 96, hiding a REAL unguarded `spawn` at :1097: the WSL Claude managed-login path, which opens a console and steals foreground on Windows. Round 5 traded one false positive for one false negative and I did not notice, because the offender count went down. The blanker now resets non-backtick quotes at a newline (the rule stripComments already had) and tracks brace depth per interpolation. The spawn is fixed rather than allowlisted, and the count is 69 -- the number the AST predicted. P1 -- the ownership-timeout guard was dead code: it threw into its own `catch` three lines below, which returned null, which the caller persists as "not owned" and clears the user's account selection. Now a typed sentinel the catch rethrows. P2 -- `WslGuestEnvironmentUnavailableError` reached the UI verbatim from the CLI installer and the Codex availability check. Both mapped. Method note: I had been regenerating the allowlist with a Python transcription of the scanner, and the two drifted -- the same two-implementations problem this workstream keeps finding. The allowlist is now generated by running the shipped test with an empty list and taking what it reports. * fix(guards): stop patching the lexer -- make the scanner fail closed instead Round 7 proved my round-6 fix also did not work, by planting a plainly-named unguarded `spawn` in `claude-accounts/service.ts` and watching the guard pass 3/3. That is three consecutive attempts at an exact lexer, each shipping a desync that hid real calls, and each time the offender count went DOWN, which I read as progress. Round 6's diagnosis was wrong too: the culprit is the `templates` brace-depth stack, which nothing resets, not quote state. So stop trying to be exact. `blankStringContentsDesynced` reports when the lexer lost its bearings, and the guard treats that as an offender. Over-reporting is a nuisance; under-reporting is a false clean, and a false clean is what let a real console-flash spawn out of the ratchet twice. The allowlist goes 69 -> 82: the 13 extra are files whose scan cannot be trusted, now named rather than assumed fine. The planted violation is now caught. Also from round 7: - `SPAWN_CALL` missed promisified and renamed bindings, so `exec('where gemini')` (a real Windows cmd.exe spawn) and a detached `shell: true` in `cli/runtime/launch.ts` were invisible. Added execAsync/execFileAsync/ execFileCb/spawnDetached. - `BASHISM` matched `set -o pipefail` but not `set -euo pipefail`, which is the only spelling this tree uses -- so the check could not have caught the #14292 signature it exists for. Fixed, and it immediately flagged a file; that one turned out to be a comment, so the bashism scan now strips comments too. - The CLI installer error mapping my round-6 commit claimed was "both mapped" was never applied -- only the Codex side had been. Now actually mapped. * fix(guards): close the four holes round 8 found by planting violations Round 8 stopped reasoning about the guard and planted spawns into it. Four holes, none of which reading had found: - `windowsHide: false` **passed**. The check was `args.includes('windowsHide')`, a substring test. Now matches `windowsHide: true`. - A ternary first argument was silently skipped: the method-declaration filter `/^\(\s*\w+\s*[:?]/` also matches `exec(useAlt ? 'a' : 'b', …)`. Now requires a type after the colon. - Renamed bindings were not covered, despite the comment I wrote saying they were -- I had hardcoded three names. Aliases are now resolved from the import. Each is verified closed by planting it and watching the guard fail. `fork` is deliberately still unscanned. Round 8 is right that Node forwards the option, but `ForkOptions` does not declare it, so the two live sites cannot be fixed without a cast. Recorded in the verification doc rather than left as a silent gap, along with two others worth knowing: the allowlist is file-granular, so its ~18 false-positive entries carry a standing pre-approval for real regressions in those files and cannot be retired by fixing code; and `stripComments` has no desync report, so the fail-closed check is only half applied. The doc now also says how to verify a guard change: plant a violation. Every guard fix here that was verified by reading was wrong. * fix(wsl): stop preflight reporting installed CLIs as absent on a slow distro Round 9's merge blocker, and the sharpest finding of the whole workstream: the branch built to close #9725 had reopened it from the other side. `preflight-wsl-command.ts` was one of five sites without `allowDegradedEnvironment`, so a guest-PATH probe failure threw. Every consumer collapses a throw into a verdict: `isCommandAvailable` and `isCommandOnPath` catch to `false` ("not installed"), `isGhAuthenticated` and `isGlabAuthenticated` read an empty payload as "not authenticated". So a slow distro made WSL git, gh and glab read as missing. Two things made it likely rather than theoretical. The probe took two thirds of a 5s budget, leaving the command ~1667ms where main gave it the full 5s inside its own login shell -- a cold WSL VM start routinely lands in that band. And a probe timeout is cached for 30s with a re-probe threshold of 1.5x the failed budget, which a 5s caller can never clear, so every preflight command short-circuited without spawning wsl.exe at all -- and Re-check does not invalidate the cache. Fixes: preflight degrades instead of refusing, and the probe is capped at half the caller's budget and at 4s, so no caller ends up with less time than it had before the runner existed. Also fixes a real console flash found on the way: `preflight-command-exec.ts` spawns git/gh/node through `promisify(execFile)` with no `windowsHide`. Round 9 also confirmed the credential paths are now *safer* than main: all 11 account sites degrade, every destructive guest operation is still marker-gated, and main's `getOwnedManagedAuthPath` could disown an account on a 5s timeout -- which this branch turns into a failed launch instead of a destroyed selection. * fix(wsl): make "Try again" able to succeed, and test the round-9 fix Round 10 returned MERGE with one residual worth closing first. A transient probe failure left the null-resolving promise in `inFlight`, so the only way back was `retryAfter` -- and the 4s probe cap made the 1.5x budget escape unreachable, because no caller can pass more than 4s. For the full 30s window the four non-degrading sites returned their error *without spawning wsl.exe at all*, and each of those errors says "Try again". The advice was guaranteed to fail. The entry is now dropped on a transient outcome and an explicit cooldown gate replaces it, so the window alone decides. The window drops 30s -> 5s: long enough to stop a stampede, short enough that the user's next click reaches a distro that has since warmed up. Round 10 also noted the round-9 fix shipped untested, which was fair. Added: the probe-budget floor for 5s/8s/10s callers, and preflight's degrade opt-in plus its stdout/stderr-carrying rejection, which isGhAuthenticated reads off the caught error as an auth-success fallback. * test(wsl): make the probe-budget guard actually guard Round 11 caught that the regression test I added for the probe cap did not bind: it seeded the guest environment, so the probe resolved in ~0ms and the assertion read the command leg's timeout instead. Reverting the cap to the old 2/3 split left all three cases green. Dropping the seed and asserting on the probe leg fixes it -- verified by reverting the cap and watching all three fail. A regression guard that cannot fail is the shape that has cost the most in this workstream: the windowsHide guard silently passed a real unguarded spawn twice for the same reason. |
||
 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. |
||
|
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> |
||
|
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. |
||
|
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
|
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. |
||
|
Neil
|
ef788c80c7 |
test(terminal): assert wide-char buffer content across repaints (#15280)
* test(terminal): assert wide-char buffer content across repaints The first reproduction for the Korean duplication asserted on the byte stream a pty emits, and it passes on real Windows on both ConPTY backends. The reporter's evidence says that is the wrong surface: their copied text pastes doubled, so the corruption is in the buffer, and in one run the command echo is doubled while that same command's output is clean - text that was correct when it left the shell and went wrong while being placed on screen. A redraw landing on the wrong cells emits perfectly legitimate bytes, so no stream assertion can see it. These assert buffer content instead, against a cell-level model of a grid where one glyph spans two cells. The model shares no code with the emulator, but its rules were chosen to match observed behaviour, so it is a regression detector rather than a first-principles oracle - the header says so rather than overclaiming. The largest case is a cursor positioned onto the second cell of a two-cell glyph, then erase-to-end-of-line, then rewrite, swept across every row and column at widths 8 to 44. That is the only place a wide character can be half addressed. Also covered: a whole row re-emitted over wide characters, which is the shape a console redraw takes; snapshot round-trip of a half-addressed buffer; and reflow across 148 width pairs, which is the resize the reporter uses as a workaround. Nothing reproduces. The emulator blanks the orphaned half correctly at every width and column, and reflow is lossless. The two pty specs run in the existing Windows packaging job rather than a new lane. A dedicated runner cost roughly four minutes, almost all of it checkout and a native rebuild, to run thirteen seconds of tests, and Windows minutes bill at double - the packaging job already installs the same dependencies and already runs a Windows test step. No production code changes. An earlier revision added an environment variable to select the system ConPTY, and it is dropped: its only effect would have been to let a user disable the fix for an earlier duplication bug, it logged nothing so a support bundle could not confirm it took effect, and the tests drive the backend directly without it. Refs #15192 * test(terminal): share the wide-glyph predicate instead of copying its regex The duplication detector carried its own copy of the range the grid model already exports, so the two could drift and only one would be updated. |
||
|
Jinwoo Hong
|
a77a2f93f7 | fix(remote): search Quick Open paths on the host (#15158) | ||
|
Brennan Benson
|
53bd956ef6 |
ci(e2e): keep the relay version markers in the e2e build artifact (#15303)
build-relay.mjs writes each relay's marker as out/relay/<platform>/.version, and upload-artifact excludes dotfiles unless include-hidden-files is set. The markers were therefore stripped from e2e-build-out, so every consumer that actually starts a relay failed with: Orca's local relay build is missing its version marker at out/relay/linux-x64/.version This stayed latent because the sharded e2e lane never sets ORCA_E2E_SSH_DOCKER, so its SSH specs skip instead of touching the relay. The changed-specs lane does set it whenever a changed spec needs Docker SSH, which is why the failure only appears on PRs that touch SSH-adjacent code. |
||
|
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`.
|
||
|
Neil
|
4b2c901b66 |
test(terminal): pin that the CJK block is the preedit overlay, not the cursor (#15242)
* test(terminal): pin that the CJK block is the preedit overlay, not the cursor A report described the cursor sitting on a wide character's first cell and hiding its right half, with cursor style and opacity settings ignored. Neither defect reproduces. Replaying the reporter's own captured byte stream leaves the cursor at column 11, exactly where the application asked, with correct wide and continuation cells. A block cursor also cannot hide half a glyph: it inverts the cell and the syllable renders inside the cursor span. The black block is the IME preedit overlay. macOS 2-set Korean keeps the trailing syllable composing until a terminator, so it sits in an opaque absolutely-positioned box over the grid rather than in the buffer. That box took stock upstream colours, black on white. It explains what no cursor theory can: the block appears at the composing cursor cell, no cursor option reaches it, it is identical with GPU acceleration off since it is a DOM node above both renderers, Latin never triggers it because Latin opens no composition, and Enter clears it because Enter commits the composition. Already fixed by the overlay theming in #15014, which landed a day after the reported release, so the fix ships in the next one. Tests only, no production change. Two pin the negative results so the cursor explanation cannot be re-derived, and one pins the actual mechanism at end of row, beside the existing mid-line arm. Separately confirmed and not fixed here: the WebGL renderer drops the cursor colour's alpha, so terminal cursor opacity genuinely does nothing for a block cursor, which is the default style on the default renderer. That is in the webgl addon rather than in xterm or in our code. Refs #12729 * test(terminal): make the cursor precedence assertion real and measure the overlay Review of the first pass found one assertion that could not fail. It set options.cursorStyle and then read decPrivateModes.cursorStyle, which are separate fields with separate storage, so it pinned that writing one does not clobber the other. Deleting the precedence expression from both renderers left it green. It now asserts the rendered cursor class: the option style renders, a DECSCUSR overrides it, and the reset hands control back. That fails if the precedence is removed. The overlay's rendered width is the one measurement in the report that argues against our explanation, and no test here could reach it, because the unit environment performs no layout. Adds an end-of-row browser arm beside the existing mid-line one, asserting a single composing Hangul syllable spans about two cells. That settles whether the block the reporter measured at one cell can be this overlay. Also scopes two DOM queries to the test container rather than the document, and attaches the render listener before writing so a missed render fails instead of hanging to timeout. Records in the file header what it does not establish: composing the opacity into the theme is not the same as it reaching the screen, since the webgl renderer drops the cursor colour's alpha for a block cursor. Refs #12729 |
||
|
Neil
|
442b46f020 |
ci(e2e): install a CJK font on the e2e runners (#15259)
The runners have no font covering Hangul, Han or Kana, so any spec that asserts how CJK text renders is measuring tofu rather than the glyph. That is not hypothetical. An end-of-row preedit spec added for #12729 measured the composition overlay at 1.02 cells against an 8.43px grid and failed its "wider than one cell" assertion. The overlay is shrink-to-fit with no width of its own, so it tracks the glyph's advance rather than the two cells the grid reserves for a wide character. With no Korean font that advance is one cell, and the assertion cannot distinguish a real result from a missing font - which is exactly the question that spec exists to answer. fonts-noto-cjk covers all three scripts and is added to both jobs that execute specs, the sharded suite and the changed-spec job, plus the ssh docker lane so the three stay consistent. This does not make any spec pass on its own. It makes the CJK ones mean something. |
||
|
Jinjing
|
7a695c70f1 |
test(e2e): harden triaged CI failures (#14656)
* test(e2e): harden triaged failures
* test(e2e): ship relay bundle to reusable shards
* test(e2e): tolerate expected IPC closures in daemon shutdown
A normal client exit can close the IPC channel before the finish ack
lands. Distinguish this from real failures by checking error codes,
only throwing if forced cleanup occurred or the error is not an IPC
closure.
* rm doc
* test(e2e): return termination status from legacy close handler
- terminateLegacyCloseClient now returns a discriminated union indicating
whether the process had already exited ('already-exited') or termination
was actually attempted ('termination-attempted')
- Allows finishLegacyCloseClient to only set forcedCleanup when termination
was genuinely needed, not when the process exited cleanly on its own
* test(e2e): fix dispatch contract and voice mic locator
Point the release E2E contract at the renamed build step, and assert the
relabeled microphone through the Voice pane combobox even when Radix
leaves the listbox open.
* test(e2e): add contract test for relay artifact dispatch
Validate that the relay artifact built in CI is properly uploaded,
downloaded, and passed via ORCA_RELAY_PATH to E2E test runs.
* Distinguish between terminated and already-exited processes
Detect when processes have already exited instead of always reporting
termination success. Return booleans from cleanup functions to indicate
whether they actually signalled a process, catch tree-capture failures
when the root process exits before recording completes, and use these
signals to return accurate exit status from termination handlers.
* test(e2e): stabilize file creation and voice microphone tests
Use stable locators (aria-autocomplete, named triggers) and add retry
logic to handle file scans and device events that can interfere with
listbox state. Increase timeouts to allow async operations to complete.
* Add retry logic for transient GitHub API errors in PR body updates
GitHub API occasionally returns transient 5xx errors. Retry up to 3 times
with exponential backoff (1s, 2s, 4s) to improve reliability during
temporary service disruptions. Export updatePullRequest and add sleepImpl
parameter for test injection.
* Add tab search result retention during typing
Keep search results on screen while the deferred query catches up with
the live query. Re-validates results against the current input without
dropping rows prematurely, ensuring the user can select from what they see.
* Add proper types to tab search mock
Replace `unknown` with concrete types (`OpenTabSearchResult`,
`OpenTabSearchEntries`, `SearchableWorkspaceTab`) and use type guards
for discriminated unions to improve test type safety.
|