mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
fa2b8a52c34838038cb3cc1d5274fa8cd74e8765
26
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
fa2b8a52c3 | test(terminal): verify contrast pixels after fresh calculation | ||
 Jinwoo Hong
|
381a3da46f |
feat(build): Route A, the phone's host routes bundled for the web, dark (OTA phase C, C0.7) (#21449)
* refactor(mobile-web): share the bundle manifest assembly with a second builder Manifest assembly and the on-disk write move to writeMobileWebBundleTree, and the helpers the Phase C app builder needs become exports. No behaviour change to the shipped bootstrap bundle. The CRLF guard grows two exemptions it needs once it is pointed at mobile/src: the image and font extensions .gitattributes already pins -text, and the gitignored webview engine modules the postinstall writes. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): web entry for the host route tree, and its two transport siblings The entry mounts app/h on react-native-web through expo-router's own ExpoRoot. It lives inside mobile/ so one React resolves, and supplies RpcClientProvider itself: the route tree starts below the native root layout that owns it. route-manifest.ts is a real typed module whose body the builder replaces -- esbuild has no require.context. A virtual specifier would need an ambient declaration and would leave the entry unchecked. Two .web.* siblings, both listed with a reason in web-overrides.json: the transport substitution point (a placeholder client until C0.4 lands BridgeRpcClient) and the device token store, whose native path imports expo-secure-store, which is {} on web. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(build): build:mobile-web:app, the phone's host routes bundled for the web Same builder shape as the Phase A bootstrap into a separate out/mobile-web-app, with the same manifest and the same two-scratch-build determinism check. Dark: build:mobile-web, packaging and the A2 census are untouched, and C1 is what flips build:release. Six shims, each a named Metro or RN Web gap. Images are emitted as same-origin hashed assets rather than data: URLs, because the shell's CSP sets img-src 'self'; the render check under that exact header is what found it. The script is referenced root-absolute for the same reason a <base> tag cannot be used: the document is served at every route depth and base-uri is 'none'. The budget sits below the contract's per-asset ceiling so growth trips a build rather than a refused asset on a phone. esbuild splitting does not lower it: one entry with only static imports emits one chunk (measured). Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): let React Native Web paint under the shell CSP RN Web 0.21.2 injects its stylesheet at runtime with no nonce support, so style-src 'self' blocks every rule and the page renders unstyled. Measured, not predicted: the render check serves the document under this exact header and reported the violation. 'unsafe-inline' is granted to style-src and nothing else. script-src 'self' holds, which is the directive that decides whether page code can arrive any way other than as a fetched same-origin script. The test now pins that scoping rather than rejecting the token everywhere. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * ci: prove the Route A app bundle on every PR A dedicated job, for the same reason the browser provider has one: it needs mobile/node_modules and a real browser, and the sharded test matrix would pay for both on every shard. It builds the bundle, verifies it, and runs the builder, override-census and render suites. It ships nothing. The mobile_web_app signal is lifted out of should_run the way static_analysis is. A mobile-only diff is desktop-irrelevant and skips every gated job, and that is exactly the diff that changes the page this job builds. Also the C0.6 review follow-up: mobile/package.json and mobile/pnpm-lock.yaml join the installer cache keys in the two workflows that build an installer off a hashFiles key, since beforePack requires out/mobile-web and a mobile-only change must miss those caches rather than reuse a stale build. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): pin the shipped builder against the app builder's own module name The assertion named a specifier that no longer exists, so it held vacuously. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): assert the RN Web style-src grant in the Swift checks The Swift twin of the Kotlin CSP test still required style-src 'self' and no unsafe-inline anywhere, so it trapped on the approved grant. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): make the Route A render check name what each route paints The check asserted only "some html, no errors", which expo-router's Unmatched screen satisfies: pointing HOST_ROUTE at /zzz/not-a-real-prefix stayed green. Each route now asserts content only its own component produces, and the unmatched case asserts the screen positively so the negatives discriminate. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): read the shell CSP past the comments that quote directives Both constants document themselves with // comments containing quoted directive text, which the quoted-string scan picked up as directives. One parser now drops comment lines, and iOS and Android go through it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(build): honour a .web.* route sibling in the app bundle Routes were imported by absolute path with the extension, so esbuild's resolveExtensions never applied and a .web.tsx under app/ was dead code the census still accepted. The manifest now carries a key and a module: the key stays the native filename so the URL does not move, and the module is the web sibling when one exists. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): tie each named shim to the esbuild option that implements it The shim list was asserted against a literal copy of itself, which passes however the build is configured. Each entry now carries an appliesTo that reads its own option, checked against the real options object. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * chore(build): line up the CRLF exemptions, the budget comment, and the job scope The builder loads .gif as a file but neither .gitattributes nor the CRLF scan exempted it, so the blanket eol=lf pin would have rewritten one. A test now keeps the two lists in step. The Phase C byte budget's comment sat on the asset count, and a root package.json edit could change build:mobile-web:app without running the job that proves it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * style(build): satisfy the index-check lint rule in the CSP parser Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * ci: key the installer caches on the mobile page trees too beforePack builds the mobile web bundle into the installer. Today those bytes are Phase A's, which src/** already covers, but once C1 flips the entry to mobile/app a page-only change would hit a cache holding a stale installer. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): skip the bundling tests where mobile dependencies are absent The sharded `test` job collects config/scripts/**/*.test.mjs and installs no mobile dependencies, so the two new suites failed there on "Could not resolve react-native-web". They now skip themselves with a message naming the job that runs them, and that job sets ORCA_MOBILE_WEB_APP_DEPS_REQUIRED so a missing install fails it instead of skipping everything it exists to prove. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(build): scan mobile/packages in the .web.* census The census claimed the app entry never resolves into packages/, but the dictation hook imports @orca/expo-two-way-audio and the built script carries ExpoTwoWayAudioModule.web.ts. That file is now listed with its reason, and planting a .web.* in each scanned tree proves the scan is not passing because a tree happens to be empty. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): assert the route exclusions against a tree that has them mobile/app holds no test, spec or +api file, so the exclusion rule was asserted against a tree it could not fire on. A scratch tree plants one of each; dropping the rule now fails this test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): 404 unknown file paths in the render check's page server The server answered every path with the document, so pointing publicPath at /wrong-prefix still rendered three green routes: the script is fetched from the one prefix that is served. A path naming a file now has to come out of the bundle, which is what the shell's manifest map does. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): cover the app bundle verifier's own checks The verifier had no test. One doctors the buildId, which the packaged assert catches; the other rewrites the tree so every digest still agrees and only the two fresh builds can tell, which is what a stale out/ looks like. Deleting either check now fails a test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * chore(build): tidy the app bundle comments and the job's path prefixes Drops an export nothing read, merges two comments that had drifted apart from the constant they describe, and corrects the claim that the job runs on every PR when it is path-gated. package.json leaves the prefix list because GLOBAL_FORCE_FILES already forces every job on it; mobile/packages/ joins it, since the page resolves a .web.ts out of there. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * style(build): merge the duplicate node:fs/promises import in the census Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): redirect the hybrid shell route on the web page app/h/[hostId]/web.tsx reaches OrcaMobileWebShellView, whose module calls requireNativeViewManager at import. In a browser that throws before React mounts, and the route manifest imports every route statically, so one native route left the whole page blank at every URL. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): fail the render check with the error that stopped the mount The check waited on "#root has children" with Playwright's animation-frame polling, so a route module that threw at import read as a bare 30s timeout naming nothing. It now waits on a mount attribute the entry sets after the router commits, polls on a timer, and races the wait against the first uncaught error so the failure carries it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): answer the favicon the render browser asks for CI resolves the runner's Google Chrome, which requests /favicon.ico; the bundled headless shell does not. The bundle carries no icon, so the server answers 204 rather than turning a browser habit into a console error the render assertions read as a page fault. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(build): settle the render check's uncaught-error race without rejecting The entry throws during goto, before anything awaits the race, so a rejected promise surfaced as an unhandled rejection beside the real failure. The same signal now resolves with the error. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * chore(mobile): list the page transport in the raw request port inventory The placeholder client implements the port, so the boundary test counts it as an unlisted file. It belongs under OWNERS until C0.4's BridgeRpcClient replaces it. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
 Neil
|
ea01cd0ccd |
fix(windows): reject a node-pty addon that predates the MSYS breakaway denial (#20047)
* docs(windows): record the measured MSYS job-breakaway mechanism The per-PTY job already denies JOB_OBJECT_LIMIT_BREAKAWAY_OK for Cygwin/MSYS shells (#19068), but nothing records why, and a conpty.node built before that commit fails windows-msys-job.win32.test.ts in a way that reads as a source defect. Measured on a real Windows 11 host: both the plain and the exec- replacement Git Bash shapes leak, the escape is the MSYS runtime's own spawn/exec (fork keeps membership), and a single-variable A/B on usesCygwinRuntime flips the result 0/2 -> 4/4. Also names the gap the failure hid behind: node-pty-job-ownership.cjs asserts symbol presence, which cannot distinguish patch revisions. * fix(windows): reject a node-pty addon that predates the MSYS breakaway denial The native-runtime gate asserted only that terminateJob, listJobProcessIds and assignCurrentProcessToJob were exported. All three predate the Cygwin/MSYS breakaway denial, so an addon built before it passes every gate, isPtyJobOwnershipAvailable() returns true, and windows-pty-job.win32.test.ts passes 6/6 -- while every Git Bash child is created outside its pane's job and survives terminatePtyJob. Read the resolved .node and require the wide msys-2.0.dll literal that usesCygwinRuntime holds, the way stagedRelayAddonIsUnpatched() already tells a patched windows-process-tree addon from a published one. An addon the caller cannot name is refused rather than skipped: a gate that cannot see its subject is not a gate. Verified against real binaries on a Windows 11 host: the shared checkout's pre-#19068 build errors, a build from current patched source passes, a missing path errors. Also closes the cross-host packaging skip. The export half has to load the addon so it cannot run when the packaging host is not the target, which is how a Windows release built elsewhere could ship this. The marker is a file read and needs neither; an unrecognised layout warns rather than fails a release that was packaging fine. * fix(windows): check the MSYS breakaway denial on the rebuild path too The Electron probe carried the marker check, but it lives inside probeElectronNativeModules, which returns early whenever the Electron package binary is unusable. Covered by another path is not this path checks -- and the defect this whole change closes was a gate that looked like it checked. Reading the binary needs neither a loadable Electron nor an executable target arch, so assert it after the rebuild, beside the windows-process-tree assertion that exists for the same reason: this is the addon copied into the packaged app. Absent warns (a cross-platform rebuild need not leave a win32 addon on this disk); present and unmarked is fatal. The fixtures now write a real addon file, because the gate reads the binary it was told about rather than trusting the exports. Verified against the two real binaries measured on the Windows host: the pre-#19068 build fails this path, the build from current patched source passes. * fix(windows): check the marker on every ConPTY path the packaged app can load The packaged marker check read one hard-coded path, `build/Release/conpty.node`, and warned when it was absent. `loadNativeModule` tries `build/Release`, then `build/Debug`, then `prebuilds/win32-<arch>`, swallowing each failure, and `prunePackagedNodePty` drops the published prebuild only when a same-arch `build/Release` exists to replace it. So the two packages the check was added for were the two it could not see: - cross-host: no host but Windows can build conpty.node, so there is no `build/Release` and the prebuild is what ships. The check warned and returned. - cross-arch: `build/Release` is the packaging host's own arch, patched and marked, so the check printed OK -- while the target app cannot load it and falls through to the unmarked prebuild underneath. Measured, not assumed: both published Windows prebuilds in the node-pty tarball contain neither `msys-2.0.dll` nor `cygwin1.dll` in any encoding. They are the binary that leaks every MSYS pane child out of its job. It now sweeps every candidate present for the *target* arch and refuses a package with no candidate at all, which is a package with no ConPTY backend rather than a layout to shrug at. It runs for every Windows slice instead of only the branch the export check skips, so deleting the export check cannot silently take it too. A stale source build keeps the rebuild advice; the prebuild gets the advice that actually works, which is to package the slice on a Windows host of that arch. Also: the marker constant was re-typed in four places and was tied to the C++ literal that produces it by nothing at all, so editing the patch would have left a gate that fails every correctly rebuilt addon and tells the developer to do the one thing that cannot help. The fixtures now take the constant from the gate, and a test asserts the patch still adds `L"msys-2.0.dll"` to conpty.cc. And the rebuild path treated a missing addon as a warning even on the host that will run the install, where node-pty would fall through to that same prebuild. The verdict is now a value, so it is tested without a platform gate. * fix(windows): resolve the packaged ConPTY the way its loader does Sweeping every candidate and demanding the marker on all of them was wrong in the one case it was meant to make safe. `beforeBuild` runs `rebuild-native-deps.mjs --platform=win32 --arch=<target>`, so a cross-arch slice normally does get a patched `build/Release` for the target; `prunePackagedNodePty` keeps the prebuild anyway because its guard is `electronArch === process.arch` rather than the arch of the binary. That package is correct and its leftover prebuild is never reached, and the sweep failed it -- telling whoever ran it to package on a Windows arm64 host, which is both the wrong remedy and one no runner here can offer. Presence cannot separate that package from the one whose cross-arch rebuild quietly emitted the host's architecture, because the only difference is the arch of `build/Release`. So the gate now resolves the addon the way `loadNativeModule` does -- first candidate whose PE `IMAGE_FILE_HEADER.Machine` matches the target, walking root-then-lib for each layout in node-pty's own order -- and checks the marker on the one that will actually run. A package with no candidate, or none of the target's architecture, is refused: it has no ConPTY backend either way, and the second is exactly what a silently host-arch cross-build looks like. The PE machine reader already existed, privately, in the relay addon builder that needed the same "a cross-build cannot silently emit host arch" guarantee. It is now shared rather than copied. Two seams were unreachable from anything but Windows, so nothing tested them: - the afterPack hook's win32 block was an inline if/else that only a source-text assertion could inspect, and that assertion could not tell the difference between the check running and the check being wrapped in `try {} catch {}`. It is now `verifyPackagedWindowsNodePty`, and "the marker check runs even where the export check cannot" is four spied assertions instead of a string match. - the rebuild path's verdict read `process` directly, so the branch that fires only on the host being rebuilt for was dead on every other host. It now takes the host as arguments, and the fs checks, the warning and the failure are all exercised from macOS. Fixtures write a real PE header rather than `MZ fake addon`, since the gate now reads one. The machine table is pinned to the documented IMAGE_FILE_MACHINE values, because every fixture builds its header from that table and a table wrong in both entries would otherwise agree with itself. * fix(windows): say why the packaged ConPTY fell back, not just that it did The previous commit resolved the addon by architecture but still had one message for every way the resolution could land on the published prebuild. Those ways want opposite remedies, and the one it printed was the remedy the commit before it had just called wrong: - no source build in the package at all — the slice has to be built somewhere that can build node-pty for the target arch. - a source build that is there but is the packaging host's architecture, because the cross-arch rebuild did not honour `--arch` — re-running that rebuild is the fix, and "package on a Windows arm64 host" is neither necessary nor possible. The second is the common one, since node-pty publishes a prebuild for both Windows arches and prune keeps the target's on every cross-arch package. So the old text fired mostly on the case it described least. It now reports which source builds were skipped and the machine field each carried, and names the rebuild command. "Nothing the target can load" had the same problem in reverse: a zero-length or truncated `conpty.node` got a cross-architecture diagnosis. Every candidate is now named with what was actually read, including "not a PE image". The rebuild path asserts the architecture too. A rebuild that ignored `--arch` was otherwise only visible at packaging, two steps from the command that fixes it. Arches with no known machine value are left unjudged rather than guessed at. Two things the extraction broke or nearly broke, both found by mutation: - the shared PE reader answers `null` where the relay builder's private copy returned a number, which would have turned its "node-gyp ignored --arch" error into a `TypeError`. Both callers now go through `describePeMachine`. - the rebuild fixtures stage a script's co-located modules by walking its imports, and the walker only understood `from '...'` — so the gate's new `require('./windows-pe-machine.cjs')` was left behind and every subprocess test failed with a resolution error, which is the exact failure its own comment warns about. It now follows `require` and bare side-effect `import` as well, and has tests; the fixture stages the gate by walking it rather than by naming one file. Fixtures write real PE headers through one shared builder instead of three hand-rolled ones. * fix(windows): run the node-pty addon gates on the Windows job that can `rebuild-native-deps-node-pty.test.mjs` carries four `skipIf(platform !== 'win32')` tests. The full suite runs on ubuntu, and the Windows PR job runs an explicit file list that never named this file -- so those tests were skipped on Linux and never reached anywhere else. Three of them predate this branch. The Windows job is added the four node-pty addon suites plus the module-walker one; the comment above that list already says why it is the right place, which is that the addon assertions only hold once natives have been rebuilt. Running the path-joining suites there also covers the separator this gate's candidate list is built from. The rest is round-three review: - the rebuild-time arch assertion told a reader "node-gyp did not honour --arch" about a file that was not a PE image at all, which is a truncated or quarantined artifact and a different command to run. The two now read differently, and neither claims the other's cause. Same fix the packaged gate had one commit ago, in the place that had not had it yet. - the missing-addon error said node-pty "would load" a prebuild without checking it is there. It says "fall through to" now, which is true either way. - `isLoadableByArch` had no caller left once the packaged gate started needing the raw machine field for its message. Removed rather than kept warm. - each candidate's header is read once instead of up to three times. - the module walker's comment claimed every shape that reaches a co-located module; it does not follow `projectRequire`/`requireLocal`, and it must not -- those specifiers resolve against the project root, so following one stages the wrong path and the copy fails. Proven by trying: widening the pattern to require-shaped names broke nine tests on `projectRequire('./config/scripts/...')`. The comment now says what it follows and why it stops there. - a new test resolved a file URL with `.pathname`, which keeps the drive-letter slash on Windows -- the very job this commit adds it to. * docs(windows): put the superseded export-only gate in the past tense It describes what used to pass a broken addon, so present tense reads as a description of the gate the same document then explains replacing it. * fix(windows): repair what running the node-pty suites on Windows exposed Putting these files on the Windows job turned four assertions red on the first run. Three of them were in tests that carried `skipIf(platform !== 'win32')` and had therefore never executed anywhere, on any branch. - `writeFakeElectronRebuild` emitted the `windows-process-tree` addon a real rebuild leaves but never node-pty's, so every Windows test of the rebuild path ran against a tree no real rebuild can produce: node-pty "rebuilt" with nothing in `build/Release`. The new same-host check reads that state correctly and said so. The fake rebuild now writes `build/Release/conpty.node` when it was asked to rebuild node-pty for win32, with the marker and the target machine. - `mkTempProject` never staged `windows-process-tree-creation-time.cjs`. The rebuild script reaches it through `projectRequire`, which resolves against the project root, so the module walker cannot follow it and must not try. Staged by name, with a comment saying which of the two it is. Without it the windows-process-tree probe failed to load its own checker and the module joined `modulesToRebuild`, which is the second and third red assertion. - the two `nodePtyAddonPath` cases compared against a literal POSIX string. `resolve` returns a drive letter and backslashes on Windows, so they could only ever pass off it. Built from segments now, which still pins the `..` traversal that is the point of the test. Verified on macOS: ensure-native-runtime-job-ownership, verify-packaged-node-pty-job-ownership, windows-pe-machine, script-module-dependencies, rebuild-native-deps-node-pty, rebuild-native-deps, rebuild-native-deps-windows-process-tree, ensure-native-runtime -- 109 passed, 6 skipped. The 6 are the Windows-gated rebuild tests, which is the job this change is aimed at; Windows CI is the arbiter. * fix(windows): give the packaged fallback a third verdict, for a file that is no image The packaged gate had two remedies for landing on the published prebuild and picked between them on `!prebuilt`, which puts a truncated, empty or quarantined `build/Release/conpty.node` in the cross-arch bucket: "the source build beside it is the wrong architecture ... re-run with --arch". It is not the wrong architecture, it is not an architecture, and `--arch` is not the command. The rebuild-path gate was split for exactly this a commit ago; this is the same split in the place that had not had it. Also from review of the settled state: - the stale-source-build branch ended in a call that happened to throw, so a reader could not see it was terminal and the file was read twice to get there. The verdict is now an Error the caller throws, built once from the read it already did, and shared with `assertCygwinBreakawayDenied` rather than copied. - four injection seams had no consumer in production or in tests (`deniesBreakaway`, `peMachine`, and `exists`/`peMachine` on the rebuild verdict). An unused seam is a way for the tested path and the real one to drift apart; the tests drive both with real files. Removed. - the loader table existed in a docblock and in the reference doc, already disagreeing about row four. The docblock cites the doc now. - `peImage` stamped machine `0x0000` for an arch it had no value for, because `writeUInt16LE(undefined)` coerces to zero. A fixture that quietly invents the field the gates read is the same species of silent lie the gates exist to catch; it throws, and a test holds it to that. - a test named for refusing an unreadable candidate asserted only that something threw. Renamed to what it proves. * fix(windows): make the rebuild fixtures represent a tree that can exist Second round of what running these suites on Windows exposed. The module the walker could not stage is now staged, so the probe reached its own checker and the real reasons surfaced: - `writeFakeWindowsProcessTree` exported `{}`. The creation-time gate reads `supportedProcessDataFlags` off the addon and calls its absence "the tarball prebuilt, not a build of the patched source" — correctly. The fixture predates that gate and, being Windows-only, never met it. The healthy fake now reports the flag, taken from the gate's own constant. Two tests were failing on this, the second only because the module then joined `modulesToRebuild`. - `rebuilds a loadable ConPTY native that lacks Orca job ownership` asked for a node-pty rebuild in a tree where node-pty had none of the payload its package ships. It gets `writeFakeNodePtyConptyPayload` like its two siblings. I also tried making the fake rebuild emit `build/Release/conpty.node` the way a real one does, and backed it out: `restoreNodePtyWindowsConptyRuntime` keys off that file and then reads `third_party/conpty`, so emitting it in a tree without the package payload turns one honest gap into an ENOENT two steps away. The payload fixture is where "node-pty has its addon" belongs. macOS: ensure-native-runtime-job-ownership, verify-packaged-node-pty-job-ownership, windows-pe-machine, script-module-dependencies, rebuild-native-deps-node-pty, rebuild-native-deps, rebuild-native-deps-windows-process-tree, ensure-native-runtime — 112 passed, 6 skipped. The 6 are the Windows-gated rebuild tests; Windows CI is the arbiter and is why they are on that job now. * fix(windows): register the node-pty addon suites in the scope list too Putting the five suites in the Windows lane's vitest argv gets them run once the job starts; `WINDOWS_PACKAGE_TESTS` in `pr-code-change-scope.mjs` is what decides whether the job starts at all. Only the argv was updated, so a PR touching just `rebuild-native-deps-node-pty.test.mjs` would not have started the Windows job, and its four Windows-only cases — including the same-host-absent one added here — would have run on no machine for that PR. Exactly the shape of gap this branch is about. Both lists now name all five, and `windows-pe-machine`, `windows-pe-image-fixture` and `script-module-dependencies` join `NATIVE_RUNTIME_PREFIXES` so a change to the modules themselves starts it too. `win32-test-lane-registration.test.mjs` exists to catch precisely this and did not, because its matcher only recognises suite-level gates (`describe.runIf` / `describe.skipIf`) and a `.win32.` filename. These tests gate per `it`. Widening it is not this branch's change to make: about thirty files across the repo carry per-`it` Windows gates and are unregistered, so the ratchet would move far beyond node-pty. Flagged rather than done. Message repairs from the same review: - the non-PE arm of the rebuild-time arch error read "... is not a PE image, so nothing can load it, so node-pty would fall back ...". The shared consequence clause already opens with ", so". - the no-source-build packaging error ended "Package this Windows slice on such a host", which is wrong advice for the case where the host IS such a host and the rebuild simply left nothing — reachable when the artifact is removed before prune runs. It now names both readings and points at the beforeBuild output. - the relay-addon builder blamed `--arch` for a build output that is not a PE at all, the same guess the node-pty gate was taught to stop making. - the patch-drift assertion was a bare `toBe(true)`, so a real drift read as "expected false to be true". It now names the two things that can have drifted and what happens until they agree. |
||
  Brennan BensonandMerge Sim
|
97aa5ff19b |
fix(mobile): open native chat when a new worktree launches a default agent (#19850)
* refactor(agent-launch): make the launch-mode decision surface-neutral
`decideWorkerStartMode` was the only shared answer to "structured chat session
or terminal agent?", but it lived in an orchestration-named module and spoke
orchestration's vocabulary, so the other launch surfaces could not call it.
Move the decision to `main/agent-launch/agent-launch-mode` unchanged and leave
`orchestration-worker-start-mode` as the adapter that supplies the noun.
A worker is not a special kind of launch; it is the same launch with a dispatch
attached. Naming the receipt's subject is the only thing orchestration actually
contributed, so that is the only thing the adapter keeps: "worker" in both
sentences, plus the `--terminal` wording, which reads as nonsense anywhere a
`--terminal` flag does not exist. Both are pinned, because they are asserted.
No behavior change. The receipts are byte-identical for every reachable case,
proven by running the new pin against both implementations.
Also pins the wording, which nothing was holding. The existing suites assert
`toContain` fragments ('terminal agent', 'cannot create') and the CLI suite
asserts a receipt handed to it by a mock rather than one this code produced;
all six files stayed green against a deliberately corrupted vocabulary. A
dispatch receipt is the only place a structured-to-terminal downgrade explains
itself, so the whole sentence is the contract, not a fragment of it.
* feat(agent-launch): add the launch intent and the one executor that runs it
The sequencing around the launch decision was duplicated per surface, and the
duplicate is where the bug lives. A new worktree was created agent-first, so
its startup terminal WAS the agent and the structured branch below it could
never be reached — every new-worktree launch was a PTY regardless of the user's
default. Orchestration fixed that for itself in #19431; mobile and the CLI
still have it.
`executeAgentLaunch` inverts the order once, for everyone. When the preference
is structured the worktree is created with NO startup agent, the executing host
is then asked whether it can host a session for the workspace that now exists,
and only then is a surface created. The host verdict cannot be hoisted above
creation: `agentSession.createSupport` only answers for a workspace it can
resolve, which is why the decision stays in two halves.
Agent-first creation is deliberately preserved for PTY launches — it is what
sequences the agent's startup command behind the setup runner, so wait-for-setup
comes for free there.
What actually differs per surface is only how a surface is built (an
orchestration worker's session takes a dispatch hold and a mailbox a plain
launch must not take), so that is injected as a factory rather than branched on.
The intent also strips the reserved agent fields from a migrated create payload:
a caller moving off `worktree.create` passes its existing params, and a stale
`startupAgent` in there would re-create the very path this replaces.
Tests assert order and arguments, not just the resulting mode. Reintroducing
agent-first creation reddens 4 of 11.
* feat(agent-launch): expose the launch executor as the agent.launch RPC
Adds `agent.launch` — one host-side method that decides structured-vs-terminal and
creates the surface — wired to the real runtime factories: `createManagedWorktree`
for the workspace, forking on `startupAgent` exactly as the orchestration worker
path does; `createStructuredAgentSessionForWorktree` for a chat session; and
`createTerminal` for a PTY agent. Allowlisted for mobile, which is the surface the
routing gap was reported on.
`worktree.create` is untouched. Its `startupAgent` keeps meaning "spawn a PTY agent"
verbatim, because it answers with `agentTerminalHandle` only on that path: a host
that quietly routed it to a structured session would hand every older client a
response with no handle and no error. All new behaviour sits behind
`agent.launch.v1`, which the host now advertises and a remote client must negotiate,
so a client that does not gets today's behaviour unchanged.
* feat(mobile): route workspace creates through agent.launch
Picking an agent on the mobile create sheet always produced a terminal, even
when the user's default was native chat, because all three create paths put
`startupAgent` on `worktree.create`. That means "create the worktree
agent-first", so its startup terminal IS the agent and the structured branch
below it is unreachable — while the same phone's in-workspace "+" button opened
a chat.
The blank, branch and new-branch creates now send the same payload through
`agent.launch` and let the host settle the surface. `worktree.create` is
untouched, and a host that does not advertise `agent.launch.v1` (read from the
existing `status.get` probe) keeps today's path exactly.
Work-item creates stay on `worktree.create`: they pre-fill the issue/PR URL as
an unsent `startupDraft`, which a structured session cannot hold yet, so routing
them would submit the URL as a first turn.
* fix(agent-launch): drop the deleted draft-prompt blocker from the reason map
main removed the draft-prompt blocker in #19681 (a structured session now holds
an unsent draft), so the exhaustive Record no longer typechecks.
* chore(agent-launch): carry a SAFETY rationale on the agent placement cast
The type-assertion gate landed after this branch's base, so the new file's
copy of the worker-start cast is now a changed-code finding.
* chore(agent-launch): carry agent.launch through main's RPC typing and casting gates
The typed-method contract, the generated params catalog and the
`assertionStyle: never` casting scan all landed after this branch's base.
- AGENT_LAUNCH_METHODS kept an `RpcMethod[]` annotation, which widened its
method name to `string` and broke assignability; every sibling infers instead.
- `agent.launch` binds a schema under src/main, so it joins the catalog's
RPC_METHODS_WITHOUT_SHARED_PARAMS and the parity gate's hand-listed twin.
- The now-typed methods make most test casts unnecessary; the few that remain
carry the line-specific SAFETY rationale the casting gate requires.
* test(mobile): supply the agent-launch fixture the create-submit recording needs
The golden RPC recordings landed upstream while this branch was out, so they
first met agent.launch here. Three things had to happen, and only one of them is
a fixture bump.
1. workspace-settings-mounts.ts mounts useNewWorkspaceCreateSubmit against a
fixture model that throws on any member it was not given. This PR added a
required getAgentLaunchSupport, so the submit aborted with "Missing model
fixture" before it ever issued the create, and three cleanup checkpoints
vanished. That read like a product regression and was not one. Supplying the
member restores the recording byte-for-byte; it is pinned false for the same
reason the cutover probe is, so the baseline stays on worktree.create.
2. Editing that adapter moves adapterSha256 for the twelve settings goldens it
mounts. Their recordings are unchanged - header only, by design: the digest
is per-golden so editing a module fails exactly the goldens that mounted it.
3. Five goldens changed behaviourally, and both changes are this PR's:
the capability probe now reports agentLaunch, and a create whose reply
carries no worktree returns "Failed to create workspace" instead of throwing
a TypeError off an unguarded result.worktree read. The launch route needs
that guard, since a receipt can arrive without a worktreeId.
* refactor(mobile): decode the launch receipt instead of asserting its shape
The changed-code quality gate refuses type assertions, and the eight it flagged
were worth removing rather than suppressing.
The production one was the point. readAgentLaunchCreateOutcome asserted the RPC
payload into Partial<AgentLaunchResult> and then runtime-checked it anyway, so
the assertion bought nothing and claimed a contract the host had not proven. It
now narrows with `in` and validates each hop, which is the same nullability
question readCreateResult already answers on the sibling path - a launch receipt
can legitimately arrive without a worktreeId. AgentLaunchCreateOutcome ties
worktreeId to the shared contract so a change there fails this reader's
typecheck rather than passing a differently-typed field through.
The test fakes claimed a whole RpcClient via `as unknown as RpcClient` while
implementing one member. They now build a typed literal, matching the pattern in
use-mobile-structured-agent-options.test.ts. The read sites cast params and then
read one field; they now assert the payload with toMatchObject, which removes
the cast and pins more of the shape than the cast did.
Also pins the warning passthrough, which nothing covered: a terminal launch that
seats the workspace but cannot start the pty reports why, and the absent, blank,
non-string and structured-surface cases report nothing. Writing that test caught
a real drop I had introduced in the reader.
* ci(mobile): re-run Mobile Checks when a shared capability changes
Mobile Checks is path-filtered to mobile/**, but mobile imports the negotiated
capability names straight from src/shared/protocol-version.ts and records the
whole capability read verbatim in its goldens. So a capability added desktop-side
rewrites a mobile fixture while never triggering the suite that would catch it.
That is what happened here: #19849 introduced agent.launch.v1 and Mobile Checks
never ran on it. Verified at the run level rather than by check name - the
window-free check-runs API on
|
||
|
Brennan Benson
|
170ebce1f2 |
fix(ci): run static analysis for every tree the repo-wide audits scan (#20918)
A mobile-only diff is desktop-irrelevant, so should_run was false and every PR check skipped -- including the audits that do lint mobile/. The violation then landed on main and failed the same gate on every later PR's merge ref. Derive the trigger from the audit commands' own scan roots so the two cannot drift. |
||
|
Neil
|
d62328aa4d |
fix(codex): remove redundant Windows hook launcher for Unicode profiles (#20952)
* fix(codex): reuse the Windows hook shell for Unicode profile paths * test(codex): register Unicode hook tests in Windows CI * test(codex): pin trust hash replacement during Windows upgrade * test(codex): retry transient Windows teardown locks |
||
|
Neil
|
5127d1eb3b |
refactor(windows): vendor the registry addon as @orca/windows-registry (#20438)
* refactor(windows): vendor the registry addon as @orca/windows-registry windows-native-registry@3.2.2 was last published in 2023 by a single maintainer. Orca called two of its exports, both read-only, so the whole dependency is replaced by a local N-API addon under native/. The vendored addon is read-only by construction: setValue, createKey and deleteKey are gone, so RegDeleteTreeW no longer ships in the app. Two upstream defects are also fixed rather than carried over — the name/data scratch buffers were file-scope statics that concurrent reads would scribble over, and createKey/deleteKey called .c_str() on a temporary. Build wiring keeps the existing shape: still an optionalDependency gated to win32, still excluded from pnpm's allowBuilds so only Orca's own Windows rebuild runs node-gyp for it, still copied into the packaged resources. The CI native caches now key on the vendored sources so an addon.cc edit cannot restore a stale .node. * test(windows): check the vendored registry addon against reg.exe The addon is vendored source, so no upstream release proves it still decodes values the way Orca's PATH readers expect. reg.exe is the only independent oracle on the box. * ci(windows): register the registry addon test on the Windows runner A Windows-gated file self-skips on ubuntu, so without both registrations it reports success while running on no machine at all. * fix(build): link the registry addon as a workspace package, not file: As a `file:` dependency pnpm re-resolved and re-linked the package on every install, including `--frozen-lockfile` (measured: "added 1" on a repeat no-op install). That virtual-store churn ran concurrently with node-gyp reading the same tree and cost @vscode/windows-process-tree its binding.gyp mid-rebuild, failing package (windows) whenever the native cache hit and only that module needed building. The linux packaging job hit the same race from the other side, as a pnpm staging move failure. A workspace link resolves once and leaves the store alone; repeat installs are now 55ms no-ops. native/windows-registry is listed explicitly so `packages:` still does not auto-discover mobile/. * fix(build): stop tracking node-gyp output for the vendored addon The build/ tree is generated per host and ABI; the committed copy was macOS-specific gyp scaffolding from a local build and would have shipped stale Makefiles to every checkout. * chore: ignore the vendored addon's node-gyp bin output too node-gyp also emits bin/<platform>-<abi>/ beside build/; both are per-host generated output that must never be committed. |
||
|
Neil
|
cbc7bb418c |
fix(ci): read the changed-path list past the first pipe buffer (#19409)
`pr-code-change-scope.mjs` read stdin with `readFileSync(0, 'utf8')`, a single read of fd 0. Once the writer outgrows the 64 KB pipe buffer that read returns early or throws EAGAIN, the script exits 0 having emitted no `name=value` pairs, and `tee -a "$GITHUB_OUTPUT"` records nothing -- so every lane the classifier gates is silently skipped rather than failing loudly. A PR opened long ago carries a stale `pull_request.base.sha`, so the gate's merge-base diff spans the whole base branch. PR #13178 diffed 13,294 files (773 KB) against a base 1,592 commits behind main and lost typecheck, test, static analysis, xterm patch sync, package and e2e to this. Stream stdin instead, matching how the sibling `pr-e2e-source-routing.mjs` already reads the same list in the same workflow. |
||
|
Neil
|
314506003a |
fix: retain MSYS shell descendants in their terminal job (#19068)
* fix: retain MSYS shell descendants in their terminal job * test: complete MSYS regression CI registration and teardown contract * fix(windows): deny job breakaway for the whole Cygwin/MSYS shell family The per-PTY job probed only msys-2.0.dll, and only for bash.exe/sh.exe. Cygwin ships the same spawn.cc breakaway logic under cygwin1.dll, and an MSYS2 zsh escapes exactly like its bash does, so both kept the orphan bug. Probe the runtime DLL on the shell's own search path instead of matching shell names: that is the property that decides whether the runtime will ask for CREATE_BREAKAWAY_FROM_JOB, and it drops the name special-casing. * chore(patch): restore the conpty.cc index line The earlier hand-edit dropped it while every sibling section kept one. Recomputed against the real blobs: applying this patch to 7b286d3d yields exactly 4b06d185, so git apply -3 has its fallback back. |
||
|
Brennan BensonandMerge Sim
|
1478101342 |
fix(windows): unblock structured native chat by exposing process creation time (#18986)
* fix(windows): guard process creation times
* fix(windows): ask the relay's bare addon for creation times too
The relay addon build now emits creationTimeMs, but the runtime binding
for the bare addon still declared only CommandLine, so a Windows relay
host requested flag 2 and every row came back without a creation time.
That leaves captureWindowsDescendantSnapshot returning null and
verifyWindowsProcessIdentity false forever on those hosts -- the relay
half of the patch was unreachable.
Naming CreationTime in the adapter is safe because the bare addon is a
content-hashed relay artifact: it ships in the same immutable relay
directory as the bundle reading it, so it can never be older than the
code asking for the bit.
Also bound the win32 guard test on our own row, which the addon can
never fail to answer, so an unconverted FILETIME or a 1601-epoch stamp
fails instead of satisfying a bare count.
* fix(windows): make the compiled addon prove its own CreationTime support
CI caught the real defect: the win32 guard test read
isWindowsProcessStartTimeAvailable() as true and then found 0 rows
carrying creationTimeMs. Unlike node-pty, this package publishes a
prebuilt .node at the same build/Release path node-gyp writes to, so
pnpm patches the source tree and leaves that binary alone. A host then
holds a patched lib/index.js -- ProcessDataFlag.CreationTime and all --
over a binary that ignores flag 4, and neither a load check nor a path
check can see the difference.
So the binary now says so itself: addon.cc exports
supportedProcessDataFlags, lib/index.js re-exports it, and
- windows-process-tree-creation-time.cjs asserts it during install,
which is what forces a from-source rebuild. It is shared by the Node
probe in ensure-native-runtime.mjs and the Electron probe in
rebuild-native-deps.mjs, exactly as node-pty-job-ownership.cjs is --
the Electron half matters because that probe decides onlyModules, so
without it the packaged app would ship the stale prebuilt.
- isWindowsProcessStartTimeAvailable() gates on the reported bit, not
the enum. Believing the enum is worse than reporting false: the
descendant snapshot returns null forever and the exit proof latches
unverifiable while structured chat believes it has a reaper.
rebuildNodeRuntimeModules could not actually have rebuilt this package:
the patched binding.gyp includes deps/node-addon-api, which the tarball
does not ship, and node-gyp must run from the physical dir.
Also closes the relay repair path's divergence: repairCreationTimeSources
wrote the C++ but not the buildNode splat or the tree-node typing, and
assertPatchApplied checked neither, so a repaired tree passed as patched
with buildProcessTree silently dropping the field.
The guard test is unchanged.
* fix(windows): keep the process-tree patch LF-only
windows-process-tree-patch-contract.test.mjs requires the patch file to
carry no CR bytes. Regenerating through pnpm patch-commit emitted 199 of
them, because the creation-time change is the first to touch files the
package ships as CRLF (src/process.h, src/process_worker.cc,
src/addon.cc, lib/index.js, lib/index.ts, the typings) -- and #17886's
own hunks over binding.gyp and src/process_commandline.cc carry the rest.
Stripping them is safe and changes nothing the lockfile records: pnpm
hashes patches CRLF-normalized, so the digest stays
e66202cc623996d02040c93449eb9ae353fddadf426cb53202a59ee710ee6fe7 and now
equals the file's plain sha256 too. It also still applies -- verified
against a deleted store entry, not a warm one -- and the precedent was
already there: the previous patch was LF-only and had been patching
those same CRLF files all along.
ensure-native-runtime.test.mjs stages the siblings the script loads at
module scope into its temp project. The import walk added by #17886 sees
`from './x.mjs'` only, so the createRequire'd .cjs siblings still have to
be named, and this PR adds a second one.
---------
Co-authored-by: Merge Sim <sim@local>
|
||
  OrcaWinandOrca Worker
|
c252d855ac |
fix(windows): resolve npm/pnpm .cmd shims past cmd.exe (#17869)
* fix(windows): resolve npm/pnpm .cmd shims past cmd.exe
A `.cmd` target forces every spawn through `cmd.exe /c` with each argument
caret-escaped, and Microsoft Defender for Endpoint scores a long `cmd.exe /c`
line carrying caret-escaped natural language as obfuscation. `codex.cmd` is
named in the spawn cluster of the MDE incident this addresses.
npm's `cmd-shim` and pnpm's `@zkochan/cmd-shim` generate files whose whole body
is "find node, run this script". Read one, and the spawn can go straight to
`node.exe <script> <args>` — no cmd.exe, no caret escaping. Anything the parser
does not recognise exactly, or whose target cannot be confirmed on disk, keeps
the existing cmd.exe path.
Incidentally fixes a real bug: cmd ends its command at a raw CR/LF whatever the
quote state, so a multi-line agent prompt through a `.cmd` shim had to be
rejected. Resolved shims have no such limit.
* fix(windows): refuse drive-relative shim paths and run the win32 tests in CI
Two blocking findings from review.
A drive-relative path defeated the absolute-path guard:
`win32.isAbsolute('D:evil.js')` is false, but `win32.resolve` reads the drive
letter and lands on `D:\evil.js`, outside the shim directory. cmd would have
built `C:\shim\D:evil.js` and failed; we would have executed the wrong file.
Adding `:` to the unsafe-character set closes it, and the alternate-data-stream
spelling `a.js:zone` with it. It costs no coverage: 84 of the 91 real shims on
this box still resolve, the same seven fall back.
Neither `windows-cmd-shim-resolution.test.ts` nor its `.win32` sibling was in
the Windows package job's file list, so the whole filesystem/resolution half and
the real-spawn equivalence suite ran nowhere. Both are now in
`WINDOWS_PACKAGE_TESTS` and in the pr.yml step.
Also from review: clear `windowsVerbatimArguments` explicitly on the resolved
branch rather than inheriting it, since there is no caller-built command line
there; document the kill switch and the PTY/hook-wrapper scope limits in
docs/reference; and cover drive-relative, BOM, line-ending, casing and `%*`
tampering in the platform-independent half of the tests.
* docs(windows): justify the shim-path colon guard from the filesystem rule
The guard was argued empirically ("none of the 91 shims on this box has one"),
which invites a future reader to relax it for a shim we have not seen. Windows
reserves `:` within a path segment, so a relative path cannot carry one at all:
the only spellings that can are drive-qualified, an alternate data stream, or a
`\?\` device path, and the last is already refused as absolute. That makes a
false refusal impossible rather than unobserved.
* refactor(child-process): move resolveSpawn into its own module
The merge with main pushed run-process.ts one line past the 300-line cap:
both sides grew it. The spawn-argv decision is already a pure, separately
tested unit, so it moves out rather than the cap moving up. run-process.ts
re-exports it, so no caller changes.
* perf(child-process): cache the shim interpreter lookup
The parse cache spared the shim read but not the PATH walk, so a second
resolution of the same .cmd did 0 reads and one statSync per PATH entry --
30 on a 30-entry PATH, synchronous on resolveSpawn, where one dead network
mount blocks the calling thread on every spawn.
Keyed by shim directory AND PATH, since the shim's own rule is
%~dp0\node.exe first then PATH, and a PATH edit between spawns must miss.
Corrects the stat comment, which accounted only for the shim itself.
* fix(child-process): revalidate a cached shim interpreter before using it
The node cache was held for process life and never rechecked, so a cached
node.exe that was later uninstalled -- or dropped from PATH by a version
manager -- was still handed to resolveSpawn, failing the spawn with ENOENT.
An uncached process in the same state returns null and falls back to
cmd.exe successfully, so the cache was strictly worse than no cache.
One statSync on a non-null hit, not one per PATH entry, so the walk this
cache exists to skip is still skipped. The stale-null direction stays
uncorrected on purpose: it only keeps the working cmd.exe fallback. Both
directions are now stated in the comment, along with the known miss for
callers that vary PATH per spawn.
* fix(child-process): honour PATHEXT when resolving the shim interpreter
The doc claimed a node.com/.bat/.cmd on PATH returned null and fell back to
cmd.exe. The scan actually skipped those entries and kept looking for a
node.exe, so PATH=C:\A;C:\B with C:\A\node.com and C:\B\node.exe resolved to
B's node.exe while the shim runs A's node.com -- a different binary, chosen
silently, on the one axis this module must not get wrong.
The scan now follows cmd's rule: first PATH directory holding any PATHEXT
spelling wins, PATHEXT order decides within it, and only an .exe winner is
returned. Anything else gives up and keeps the cmd.exe path, which restores
the strict-subset-of-cmd property everywhere except the documented cwd case.
PATHEXT is read from the child's env and joined into the cache key, since it
now changes the answer. Costs one stat per PATHEXT entry per node-less
directory, paid once per process behind the cache.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
|
0cbb01ef4b |
fix(security): apply the Windows path-hardening ACL that never ran (#17884)
* fix(security): apply the Windows path-hardening ACL that never ran `buildWindowsRestrictAclArgs` invoked the hardening script as `powershell.exe -Command <script> <path> <sid> <isDir>`. `-Command` does not populate `$args`; it appends the trailing tokens to the command text. The script therefore read `$args[1]` as `$null`, threw `NullArrayIndex` at `$allowedSids[$sidText] = $true` under `$ErrorActionPreference = 'Stop'`, and exited 1. Both callers swallowed that: the async callback was empty and `applySecurePathRestriction` returned `true` regardless, while the sync `catch` returned `false` and nobody logged. Every Windows secure path has been left on its inherited ACL since the ACL was introduced (#5006), and nothing said so. Replace PowerShell with `icacls.exe`, which takes plain argv. That removes the quoting surface entirely rather than escaping it: interpolating a path into the command text would have turned a dead no-op into arbitrary PowerShell on a filesystem path, since `-Command` executes what it appends. It also drops the execution-policy dependency and the `powershell.exe` spawn an EDR flags, and runs ~25x faster than the PowerShell cold start. Hardening is now three passes: `/reset` to purge explicit ACEs that `/inheritance:r` leaves behind, `/inheritance:r` plus a `/grant:r` per allowed SID, then a read-back that checks the DACL is protected and grants only the intended rights. The predecessor's verification block was equally dead, and an apply that is never read back is only half a control. Failures stay non-fatal — non-NTFS volumes, network paths and restricted tokens fail legitimately and must not break startup — but they are no longer invisible: every failure is logged, and a failed async apply now evicts its cache entry so the next call retries instead of trusting a success that never happened. Routing through `runProcess`/`runProcessSync` also retires this file's `node:child_process` allowlist entry. * fix(security): verify the hardened ACL by identity, not by shape Review found the bug class this PR fixes surviving inside the fix. The verify pass checked rule count, absence of the inherited marker, and exact rights — never *who* the rules named. Granting Everyone full control satisfies all three, so hardening reported success on a DACL that handed the credential to every local account, and most of the real-filesystem tests still passed. Verification now reads the descriptor back with `icacls /save`, which emits SDDL with raw SIDs, and compares the principal set exactly. That is also locale-independent by construction: the previous parse read localized account names out of icacls' OEM-codepage stdout, where a non-ASCII path survived by accident rather than by the documented mechanism. SDDL parsing moves to `windows-security-descriptor.ts`. Two further self-inflicted problems, both measured: The post-rename re-harden led with `/reset`, which re-widened a DACL that was already correct — the staged file's protected DACL survives the rename, so the pass had nothing to do but open a window. Polling an external process during a write into a relocated root caught it: the e2ee keypair dropped to `BUILTIN\Users:(RX)` plus `Authenticated Users:(M)` — read *and* write — before tightening again. Hardening now verifies first and returns early when the DACL already reads back correct, which closes the window and cuts the steady state from three spawns to one. Re-measured: 158 samples, one DACL state, zero broad. Evicting the cache on every failed async apply reintroduced #4901. The env store re-hardens on the read path at ~2/s, so on a host where hardening cannot work (FAT32, network path, restricted token) that was two icacls spawns and two warnings a second, forever. Async retries now take a retry floor and a hard per-path attempt cap. The write path keeps retrying unthrottled — it is user-driven, and a failed credential ACL must still be retried on the next write. Also: failures route through a reporter hook that the main process points at the diagnostic tracer, because `console.warn` reaches nothing in a packaged GUI-subsystem build; `writeSecureFile` returns whether hardening took, and the async branch reports `pending` rather than claiming `applied`; a transient `whoami` failure no longer disables hardening for the process lifetime, and the SID is shape-validated; the `/c` guard now covers the synchronous runner too. * fix(security): re-probe hardening instead of latching a transient failure The per-process attempt cap added for the read-path storm was a permanent latch: one AV scan, momentary lock or %TEMP% blip and every later credential write in that session went unhardened, silently, on a host where hardening would now succeed. Same defect class as #17858's computer-use host, and worse here because what stops happening is security hardening on credential files and nothing said so. The retry budget now bounds the *rate*, not the lifetime: at most three attempts per path per minute, re-probing in every later window, forever. The transition is announced in both directions — `throttled` once per window on entry, `recovered` when a rate-limited path hardens again — so a host stuck in the degraded state is diagnosable rather than merely quiet. The reporter type covers both, and the main process ends the `recovered` span successfully rather than failing it. Extracted to secure-path-hardening-retry-budget.ts, which keeps secure-file.ts under its line cap without a max-lines disable. Also confirms the second flagged risk rather than assuming it: a real unwritable %TEMP% is now covered by a test proving verification fails closed, reports at the `verify` stage, and still leaves the ACL applied — so that path loses proof, not protection, and with the lifetime cap gone it can no longer combine into a permanent-off state. * fix(security): verify a directory's whole inheritance flag set The flag check tested only that `OI` was present — never that `CI` was, nor that nothing else was. That was harmless while `/reset` + `/grant` ran on every pass and repaired whatever was there. The verify-first short-circuit made it load-bearing: what verification accepts is now left alone, so a latent under-check went live because a different fix started depending on it. Two directory DACLs passed while being wrong — both protected, three non-inherited full-control rules, correct SIDs, differing from correct only in their flags: (OI)(F) - no CI, so subdirectories are left unprotected (OI)(CI)(IO) - inherit-only, so the directory object itself grants nobody anything; the next writeFileSync into it fails with EPERM, on a directory just cached as hardened Verification now compares the whole flag set, which also rejects IO and NP, and names the offending flags in the failure. Both shapes are planted in real-filesystem regression tests, including an assertion that a write into the repaired directory succeeds and its child inherits. Confirmed both tests fail against the old check and pass against this one. * fix(security): back the hardening retry off exponentially The fixed one-minute window bounded the retry rate but left a standing floor of three attempts per path per minute on a host where hardening can never succeed — FAT32/exFAT, a network path, a redirected profile. That budget is per path and there are several secure files, so the floor multiplied into tens of thousands of icacls spawns a day for work guaranteed to fail. The delay now doubles after each consecutive failure, from a one-minute floor to a thirty-minute ceiling, and the attempt cap is gone entirely: once the backoff elapses the path is re-probed however long it has been failing. A permanently incapable host settles at ~2 attempts/hour. Slowing the backstop costs almost nothing, because it is not the recovery mechanism: the synchronous write path is deliberately unthrottled, so a host that recovers hardens on its very next credential write regardless of what the read-path budget says. The `throttled`/`recovered` reports are unchanged and matter more here, since the quiet periods between probes are now much longer. The curve is pinned in a new unit test against the exported delay function rather than a copy of its constants, covering the doubling, the ceiling holding at 5000 consecutive failures, a 30-day failing path still re-probing, one announcement per degraded episode, and per-path isolation. The integration tests keep only what they uniquely prove: that the read path is wired to the budget, and that a day of failures still re-probes. Confirmed four of these fail against a reinstated lifetime cap. * ci(windows): run the real-icacls DACL suite in CI The win32 suite only self-skips off Windows, so it passed vacuously in every lane. Register it the way the cmd-shim suite is registered. * fix(security): describe the cache's real cost, which is icacls now Both cache comments still justified themselves with PowerShell -- "~1-1.5s" and "a PowerShell spawn every read" -- in the same file whose PR removed PowerShell from this path. The caches are still right, but for different numbers, and the old ones are the kind an engineer would reasonably delete a cache over. The real shape: hardening verifies first and returns early, so an already-correct DACL costs one synchronous icacls spawn and a rewrite costs four (verify, reset, grant, verify). Still worth caching on the read path, which polls at ~2/s. * test(security): make the DACL suite safe to schedule Registering this spec in the Windows lane put it under two rules it had never been measured against. Teardown now goes through `removeTreeSync`, which the lane's boundary test requires, and repairs the DACLs the suite plants on purpose first: those retries only cover transient locks, so a regressed `(OI)(CI)(IO)` repair leaves the root un-removable and `afterAll` throws EPERM. And the no-permission case decides by elevation before it writes anything. `windows-2022` runs elevated, where hardening succeeds: the old branch asserted nothing about denial and instead replaced the `hosts` DACL, then `icacls /reset` -- which is not a restore, it drops the explicit `SYSTEM:(F)` that file ships with. Ephemeral in CI; permanent for a developer running the lane from an elevated shell. Now it asserts or it skips. The probe reads the token integrity SID rather than `icacls /save`, which succeeds unelevated (`BUILTIN\Users:(RX)` carries READ_CONTROL) and would have skipped the case on every machine. * fix(security): measure the hardening latches on a clock that cannot go backwards `mayAttemptHardening` compared wall-clock times, so any backwards step -- an NTP correction, a VM snapshot restore, a user changing the clock -- made the elapsed time negative and held every failing path below its delay until the clock caught up. Measured at the 30-minute ceiling with the clock stepped back a year, the path was refused at +0d, +1d, +30d, +180d and +364d, and re-probed only at +366d. That is the permanent latch the exponential backoff was added to remove, and it contradicts the module's own "bounds the rate without ever bounding the lifetime". The SID lookup's own one-minute window had the identical shape and is worse: a failed lookup makes `planFor` return null, which disables the synchronous *write* path too, so the write-path exemption that recovers the read-path budget cannot recover it. Both now measure elapsed monotonic time, following the repo's existing `monotonicNowMs` spelling. Two things the write path was not doing, both found in the same pass: - A successful synchronous apply now records the outcome. It is exempt from the budget, but it was also invisible to it, so a host that had demonstrably recovered kept the read path backing off for up to 30 minutes and no `recovered` transition ever came from that lane. Only success is recorded; recording failure would put the exempt lane back under the budget. - `writeSecureFile`'s JSDoc now says its boolean covers the file only. The directory harden is fire-and-forget and answers `pending` on Windows regardless, so a `true` says nothing about the directory's ACL. * fix(security): stop the hardening test doubles from faking a no-op Three CI failures on this branch, one failure shape: hardening silently does nothing and the check that should have caught it agrees. The auth critical-path test hand-rolled a `node:child_process` factory with `execFileSync`/`execFile`. The rewritten ACL path goes through `runProcessSync`, i.e. `spawnSync`, which the factory never returned — so every spawn threw into the SID lookup's bare catch, `planFor` returned null, and hardening no-opped. It mocks `child-process/run-process` now, the boundary production code actually calls and the one sibling ACL tests already mock: an export missing there fails loudly by name instead of returning undefined. Its fake icacls writes a real UTF-16LE SDDL file, so the pinned spawn count per write is a property of the ACL path rather than of the double. The test forces `platform='win32'`, so this failed on every platform, Linux CI included. `windowsSystem32Binary` is a production bug, not a test bug: it builds a Windows path with the host `join`, which off-platform yields the mixed `C:\Windows/System32/whoami.exe`. On Windows the two joins agree, which is why it survived; on Linux the SID lookup's whoami match missed and 27 of secure-file's 32 tests exercised a lane that never ran. These are always Windows paths, so `path.win32.join` is what it should have been. The import-boundary pin still read 160 after this branch migrated secure-path-windows-acl.ts off `node:child_process`; the ratchet correctly refuses a pin left above reality. * fix(security): resolve the machine-relative SDDL alias, and stop a denied read destroying the file Path hardening verified the DACL it wrote by comparing the SIDs `icacls /save` reports. SDDL substitutes two-letter aliases for well-known SIDs, and the resolution table could only hold constants -- but `LA` and `LG` name an account by RID inside the *machine's own* SID, so on a box whose user is the built-in Administrator (a CI runner, an Administrator-only install) the current user read back as `LA`, matched nothing, and hardening reported failure for every path. Resolve those two against the machine authority derived from the user SID; without one they stay unresolved and the comparison still fails closed. Three secret stores treated any read failure as "malformed -- regenerate" and overwrote. A hardened file granting a SID this process does not hold reads as EPERM while its directory stays writable, so the overwrite succeeds: renaming over an unreadable file needs FILE_DELETE_CHILD on the parent, not DELETE on the file. That destroyed the E2EE secret key, every paired device's bearer token, and the plugin vault. Distinguish EPERM/EACCES from a parse failure and refuse. Also close the async lane's unhandled rejection: `void p.then(onSettled)` turned a throw from `onSettled` into a dead main process, and the retry budget it calls threw whenever nothing had configured it -- a contract held only by import order. The budget now defaults its own bounds. * test(windows): say which ACEs icacls listed when a planted DACL fails `toHaveLength` reports only a count and vitest elides the array, so three preconditions failing on the CI runner said "expected 3, got 6" and nothing about what the sixth entry was. Name the entries in the failure. * fix(security): stop three more stores overwriting what they were denied Same swallow-default-overwrite shape as the readers already fixed, found by sweeping every store that reads under a hardened root. - plugin-storage-store.ts returned `{}` on any read failure and set()/delete() wrote it back, losing the plugin KV store. It is the secrets store's shape line for line, so the two now behave identically. - relay-revoke-outbox.ts returned [] and save() wrote it, dropping revocations that never reached the relay -- a revoked device stays live. - profile-cloud-session-store.ts mapped an EPERM read onto `decrypt-failed`, which fails the `status === 'found'` guard in clearCloudSessionIfUnchanged and falls through to an rmSync of the account session. A denied read now reports `unreadable`, which licenses nothing; the refresh path bails on it and the auth status surfaces it rather than reporting a bare reconnect. All reuse isPermissionDeniedError. The predicate stays an EPERM/EACCES allow list rather than "ENOENT defaults, everything else throws": these stores are meant to self-heal a truncated or malformed file, and inverting it would turn a corrupt keypair into an app that cannot start. The distinction that matters is "could not read it" versus "read it and it was garbage". * test(windows): plant fixture DACLs that cannot inherit what they did not plant %TEMP% grants [SYSTEM, Administrators, <user>] (OI)(CI)(F) by default, and those propagate into every fixture. Three preconditions read back 4 and 6 ACEs where 3 were planted, and the extras looked like Orca's own hardening because the shape is identical -- on a runner whose user is the built-in Administrator, the inherited trio IS the trio production grants. Combining /inheritance:r with /grant:r leaves the argument order to icacls, and that combined form drops the inherited ACEs on Windows 11 but keeps them as explicit ones on the Windows Server runner. Removing inheritance in its own invocation makes the grant the whole DACL on either host, and the fixture root is de-inherited once up front so nothing propagates in. Rooting the fixtures outside %TEMP% would not have fixed this: any directory inherits from wherever it lives. The fix is to stop inheriting, not to move. No assertion is relaxed -- the counts stay exact. * test(windows): pick a foreign SID that stays foreign on an elevated runner `S-1-5-32-544` is only foreign to a token that is not an administrator. The CI runner is elevated AND logged in as the built-in Administrator, so granting Administrators granted the reader full control: the file stayed readable, and all six preservation assertions went vacuous rather than proving anything. BUILTIN\Guests is resolvable everywhere and no interactive token is a member, so the read is denied on an unelevated developer box and on the runner alike. An unresolvable SID would have been the stronger choice but icacls rejects one with ERROR_NONE_MAPPED (1332). The premise guard is what caught this -- it asserted the file was actually unreadable instead of trusting the grant, and named elevation as the suspect. * fix(security): refuse on any read that never reached the contents, not just a denied one isPermissionDeniedError becomes isUnreadableError, because "permission denied" was never the concept -- "could not read it", as opposed to "read it and it was garbage", is. EBUSY, EMFILE, ENFILE and EIO say exactly as little about a file's contents as EACCES does, and they fell into the branch that regenerates and overwrites. On Windows EBUSY is the likelier of the two: antivirus holding a credential open at the moment of a startup read produces it, which makes it a commoner path to the same permanent loss than the ACL case that motivated the original fix. Still an allow list, deliberately: ENOENT keeps licensing a create, and a parse failure keeps self-healing. The stores are built to recover from a truncated write, and turning that into a refusal would trade a recoverable state for an unrecoverable one on the startup path. Also fixes the regression suite's own premise on an elevated runner: makeUnreadable combined /inheritance:r with /grant:r, and that form keeps %TEMP%'s inherited [SYSTEM, Administrators, user] as explicit ACEs on Windows Server -- so the file stayed readable and all six assertions were vacuous. Same split-the-invocation fix as the ACL suite's planter. * test(windows): skip the preservation suite where a read cannot be denied An elevated token logged in as the built-in Administrator reads straight through a DACL that grants it nothing -- confirmed on the CI runner against both BUILTIN\Administrators and BUILTIN\Guests, and with the grant split into its own icacls invocation so the DACL really was the planted one. On such a host the premise these tests rest on does not hold, and every assertion would pass while proving nothing. So probe once at module scope and skip rather than assert vacuously -- the same trade the ACL suite already makes for its unelevated-only case. The gate stays in the compound `<win32 check> && <flag>` form the win32 lane ratchet detects, so the file stays registered in both lane lists. Coverage is not lost where it counts: isUnreadableError has unit tests that run on every platform and every host, and the stores' refusal is exercised in full on any machine where a denial is reproducible -- which is every developer box. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> |
||
|
OrcaWinandOrca Worker
|
975bbdedcc |
fix(windows): scan ports natively instead of encoded PowerShell (#17861)
* fix(windows): scan ports natively instead of encoded PowerShell Microsoft Defender for Endpoint scored the relay's Windows port scan as suspicious PowerShell plus network discovery (T1049). The command line was `-ExecutionPolicy Bypass -EncodedCommand <base64>` around a Get-NetTCPConnection/Get-Process join -- base64 next to a policy override is the highest-weighted token pair on a PowerShell command line, and netstat only ever ran as its fallback. Invert the chain. `netstat.exe -ano` is now the primary reader and the owning process name comes from the shared native process table, which exists to keep PID lookups off PowerShell. The payload survives only as a last resort, and without the override: execution policy gates script files, never `-Command`, so nothing needed it (verified: `-ExecutionPolicy Restricted -Command` runs). Drop `-p tcp` while inverting: on Windows that protocol name means IPv4 only, so as a primary reader it would have hidden every `[::]` listener the payload used to report. Names arrive as `sshd.exe` from the table and are published as `sshd`, keeping the sshd filter and old clients' rendering intact. Routes both spawns through runProcess, removing the file from the child_process and windowsHide ratchets. * fix(windows): read netstat state by shape and refuse a truncated table Review of the port-scan inversion found two ways the new primary path could be silently wrong, both of which would have kept the flagged PowerShell payload running on exactly the hosts this change targets. `LISTENING` is not in netstat.exe. It lives in System32\<locale>\netstat.exe.mui and MUI selection follows the UI language, so the pinned-locale env in relay-command-env.ts cannot reach it -- a German host prints `ABHOEREN` and the word test parsed zero rows. The zero-listeners guard then read that as a blocked reader and ran `Get-NetTCPConnection` every 12-30s forever, or returned nothing at all where PowerShell is also restricted. Keep the word as the fast path and, when it finds nothing over output that did contain TCP rows, re-read by shape: only a listening socket has no peer. Measured on this host across all four states present (LISTENING 47, ESTABLISHED 49, CLOSE_WAIT 29, TIME_WAIT 213): zero non-listening rows with a zero peer, zero listening rows without one, and the same 47 rows parse after substituting the German state words. Shape stays the fallback because `BOUND` also prints a zero peer. Truncation was invisible: createOutputSink discards overflow, ProcessResult carries no flag, so a capped read still exits 0 and its head still parses. netstat orders IPv4 TCP, then IPv6 TCP, then UDP, so a host with tens of thousands of TIME_WAIT rows would have lost every `[::]` listener -- the exact loss dropping `-p tcp` exists to prevent, and one the zero-listeners guard cannot see. Refuse the read instead. A `truncated` flag on the shared sink would be cleaner and is left as a follow-up rather than widened into this PR. Also: decline to wait on the shared process table once the request is aborted (it takes no signal and must not be cancelled for other callers); note the name lookup as best-effort, since a TTL-cached snapshot can hand a recycled PID its previous owner name; log once on either fall-through, because both are permanent and invisible when wrong; and drop a stderr assertion that any PowerShell autoload banner would redden. Correcting the cost claim in the previous commit: the aggregate win holds with the native addon (netstat 21ms vs the retired payload 860ms at 532 processes), not without it. The addon is optional, the snapshot TTL is 500ms and the scan cadence is 12-30s, so a relay with no active agent pane never warms its own cache and pays ~1.4s cold on the CIM path -- slower than what it replaced. * fix(windows): log the port-scan fall-through on the relay diagnostic stream Checked where this code actually runs before trusting the log. `console.warn` did reach a file, but relayLogLine is the right call and the reasoning is worth recording. `scanWindowsListeningPorts` runs only in the detached relay daemon: relay.ts returns early for --connect and --orca-cli, so PortScanHandler is reached only through runRelayDaemon, and both launchers start it detached with a log file (POSIX `> relay.log 2>&1`, Windows `1>relay.log 2>relay.err.log` via Win32_Process.Create). installRelayLogRotation then wraps both streams into relay.log, which is the file the documented diagnostics tail reads. Verified by installing the real rotation over a temp path and reading the file back. So the line surfaced -- but untimestamped, in a log whose format exists so reconnect flaps can be correlated with the events around them (#7773). relayLogLine is that format and the relay idiom in 41 other places, and "since when has this host been stuck on PowerShell" is most of what this line is for. The test spies on process.stderr to pin the stream and the ISO stamp rather than just asserting something was called, since a fall-through logged somewhere unread is the failure being guarded against. Also fixes a comment that ended its own block early: `relay-*/relay.log` in a doc comment contains `*/`. * fix(windows): keep the dominant zero-peer state when reading a localized netstat Shape alone promoted any zero-peer TCP row, not just listeners. `BOUND` and `CLOSED` print a zero peer too, and on a localized host their state words are exactly as unreadable as the listening one -- so a German host with listeners plus one BOUND socket published a phantom listener. Reachable on an English host too: with zero listeners a lone BOUND row is promoted AND, because the result is then non-empty, it suppresses the blocked-reader fall-through. Group the zero-peer rows by state word and keep only the largest group. A transient BOUND or CLOSED socket cannot outnumber the listeners (51 against 0 on this host), so this removes the class rather than special-casing the words, which would just be the localization bug again. An exact tie keeps every tied group rather than guessing -- no worse than reading shape alone. Verified against real netstat output: injecting a BOUND row into the localized capture leaves the result identical to the English answer (47 rows, no phantom 65001). The new test has teeth -- reverting the grouping fails it and nothing else. Corrects two claims that were slightly wrong: the docblock said shape was the fallback because BOUND prints a zero peer, which described the hazard without saying it was unhandled; and a test comment said an English host "never sees a bound socket", true only when it has at least one readable LISTENING row. Also gates the fall-through log per reason instead of per module, so a host that parses nothing today and truncates tomorrow reports both faults. Same one-shot cost, and the vocabulary is two fixed strings so the set cannot grow. That guard matters more than it looks: --log-file rotates stdout only, so the file stderr can land in is unrotated. * docs(windows): note the direction the zero-peer majority rule can fail in The docblock described the tie case and stopped there, which reads as a complete account of the limits when it is not: a majority rule inverts if the majority is wrong, and enough transient zero-peer sockets would publish the phantoms and drop the real listeners. Someone would reasonably have concluded the rule was safe in both directions. Trigger numbers and the repro stay in the PR discussion; the code only needs the reader to know the rule has a direction, and the hatch (defer to the PowerShell reader, which reads the state word instead of inferring it) since that is the part a future editor would otherwise re-derive. * ci(windows): run the real-netstat port scan suite in CI The win32 suite only self-skips off Windows, so it passed vacuously in every lane. Register it the way the cmd-shim suite is registered. * test(windows): lower both child-process ratchets to the ground this PR took Migrating the port scan off `node:child_process` onto `runProcess` drops `src/relay/windows-port-scan.ts` from both allowlists, so both offender counts fall by one. Each ratchet pins the count from below as well as above, so a pin left above reality fails and re-opens room for the next direct import to land for free. * docs(windows): qualify the no-PowerShell claim on the netstat scan The scan starts no PowerShell of its own, but no released relay carries the optional `windows-process-tree.node` addon (only dev-channel-win-build.yml builds it), so the shared process-table read falls back to a CIM scan that forks one `powershell.exe`. The EDR win is the removal of the `-EncodedCommand` / `-ExecutionPolicy Bypass` shape, not the elimination of PowerShell. Comment-only. * docs(windows): record the identity-reader follow-up and the perf table's addon attachWindowsProcessNames reads only `name`, so it should move to `readWindowsProcessIdentityTable` once #17866 lands -- on that PR's detailed reader it would open per-process handles for a field it discards. The reader does not exist on this branch, so the call stays as-is with the follow-up recorded rather than pulling #17866 in. The process-table perf table's two Toolhelp32 rows assume the optional `windows-process-tree.node` addon. The desktop bundles it; no released relay does, so on an SSH host the CIM row is the operative number. Comment-only. * docs(windows): state the CIM scan as the relay's normal path, not a fallback No released relay carries the optional `windows-process-tree.node` addon -- release-cut.yml has zero references to it and only dev-channel-win-build.yml builds it -- so the PowerShell CIM scan is what every SSH host runs. The call-site docstring read as a conditional fallback standalone. Comment-only. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
|
bfc6a262a7 |
fix(windows): read command lines from the kernel, not each process's PEB (#17886)
* fix(windows): read command lines from the kernel, not each process's PEB MDE incident D scored Orca for suspicious memory activity: the vendored `@vscode/windows-process-tree` recovered every process's command line by opening it with `PROCESS_QUERY_INFORMATION | PROCESS_VM_READ` and chaining three `ReadProcessMemory` calls through the PEB and `RTL_USER_PROCESS_PARAMETERS`. On a 750ms/2s cadence over the whole table that is the credential-dumping primitive, whatever the intent. Windows 8.1 added `NtQueryInformationProcess`'s `ProcessCommandLineInformation` class (60), which returns the same string as a kernel-built `UNICODE_STRING` under `PROCESS_QUERY_LIMITED_INFORMATION` alone. Electron's floor is Windows 10, so every supported OS has it. The PEB reader stays behind a process-wide latch that only `STATUS_INVALID_INFO_CLASS`/`NOT_SUPPORTED`/`NOT_IMPLEMENTED` can set; a pid that merely denied a handle does not re-arm it, because `PROCESS_QUERY_INFORMATION` implicitly grants the limited right and so cannot be obtained where the weaker open already failed. The same hunk drops `PROCESS_VM_READ` from `GetProcessMemoryUsage` and `GetCpuUsage`, which acquired it and never read an address space. Measured on Windows 11 (514 processes), counted in-process by swapping the addon's import table entries for counting stubs, per CommandLine scan: `ReadProcessMemory` 1128 -> 0, desired access 0x0410 -> 0x1000, p50 12.7ms -> 9.3ms. Command lines were byte-identical on every process both readers recovered (376/376, 379/379 across runs), including a 24,068-character argv with quotes, non-ASCII and trailing whitespace, and a WOW64 target. Three processes that refused the old rights granted the new one; none went the other way. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * fix(windows): drop the PEB fallback and detect the unpatched prebuilt Review of #17886 found three ways the reader could still perform, or silently resume, the primitive it exists to remove. The class-missing latch was a permanent, process-wide, one-way downgrade back to the PEB read, and any single target returning STATUS_INVALID_INFO_CLASS / NOT_SUPPORTED / NOT_IMPLEMENTED could trip it. On an EDR-hooked ntdll -- the entire premise of this change -- a hook that does not recognise class 60 would have restored PROCESS_VM_READ plus three ReadProcessMemory per pid per scan for the life of the process, unobservably, on precisely the machines this was written for. The fallback is deleted rather than guarded: GetProcessCommandLine now returns false and leaves the command line empty, which callers already handle, so the addon imports no ReadProcessMemory at all. That absence is what makes the property checkable on the artifact. The published 0.8.0 tarball ships a loadable prebuilt built from unpatched source; it is node-addon-api, so a bare require() accepts it, allowBuilds is false and CI installs with --ignore-scripts, and a rebuild that soft-exits on a Windows file lock leaves it in place. Source-text guards could never see it. windowsProcessTreeAddonReadsProcessMemory() checks the compiled binary instead, and is wired into the install check, the rebuild, and the relay build. The repair itself never worked: `git apply` run inside a work tree prefixes patch paths with the cwd-relative prefix, skips what does not match, and exits 0, so the branch always fell through to its own post-check throw. The package dir is always under the project root, while the fixture that covered it was in %TEMP%, outside any repo. Blinding git with GIT_DIR fixes it, and the test now runs inside a real work tree. Also from review: bounds-check the returned UNICODE_STRING against the allocation (not the size the second query clobbers) and cap the probe so a bogus length cannot bad_alloc a whole scan; test NT_SUCCESS explicitly; value- initialize ProcessInfo, which left `memory` as stack garbage -- measured, 82 processes reported the same bogus working set; and correct a comment in windows-process-table.ts that still described the command line as a PEB read. Re-measured on Windows 11 (543 processes): ReadProcessMemory 1128 -> 0, with the symbol absent from the import table so the IAT hook finds no slot to count; desired access 0x0410 -> 0x1000 on all 543 opens; p50 13.5 -> 12.3ms; 405/405 command lines byte-identical including a 24,087-character quoted non-ASCII argv and a WOW64 target; 3 processes recovered only by the new path, 0 only by the old. * chore(deps): refresh the windows-process-tree patch hash in the lockfile * test(scripts): stage a script's local imports into the native-runtime fixture ensure-native-runtime.mjs gained an import of windows-process-tree-gyp-rebuild.mjs, but the fixture copied only the script itself, so every case in the suite died with ERR_MODULE_NOT_FOUND before reaching its own assertions. copyScriptWithLocalModules already walks a script's co-located imports for exactly this reason -- its own doc comment names this failure -- so use it rather than listing files by hand. The two Windows cases still fail here, on a missing node-pty ConPTY runtime that also fails on main; this only stops a resolution error from standing in front of whatever they were meant to catch. * fix(windows): route a locked stale addon to the Windows file-lock message `pnpm install` with Orca running aborted with a raw EPERM stack. The stale-binary guard -- which deletes an addon that still imports ReadProcessMemory so a skipped rebuild cannot use it -- ran outside the try whose catch classifies Windows file locks, and whose message is literally "Close running Orca/Electron/dev processes for this worktree": exactly this situation. Measured rather than assumed: rmSync against a loaded (memory-mapped) addon throws EPERM, and `force: true` does not help, since it only swallows ENOENT. Cold copies of the same file delete fine. So the delete threw a page before the handler that knows what it means. Moving the guard inside the try is the whole fix; the classifier already matches the EPERM text. The new case runs the real script against a temp project whose stale addon is held open by a live child process, and fails against the old placement with the raw `syscall: 'rm'` stack the report described. * feat(windows): warn once when command-line recovery is refused host-wide Removing the PEB fallback removed a total-defeat vector, but it left a cliff: if NtQueryInformationProcess(ProcessCommandLineInformation) is refused -- a hooked ntdll that does not know class 60 -- every command line comes back empty and agent identity matching silently degrades to image names. The addon still loads and still enumerates, so every health check the app has stays green. A cliff nobody can see is the failure mode this area keeps producing. The querying process is the unambiguous probe. A process can always open itself with PROCESS_QUERY_LIMITED_INFORMATION, so its own command line coming back empty means the query is refused for every process -- not that some target denied a handle, which is normal for roughly a quarter of the table. Keying on our own row rather than a fraction means no threshold to tune and no false positive on a hardened box where most processes deny. One warning per session, gated on the CommandLine flag actually being requested so a future identity-only reader cannot trip it. The suite's own SELF fixture gains a command line for the same reason: a self row without one is the alarm, not a detail. * fix(windows): check the relay's staged addon at load, and answer tri-state Two gaps in the ReadProcessMemory check, both about what it does not see. It only ever looked at node_modules/@vscode/windows-process-tree. A relay host has no node_modules of ours: it loads ./windows-process-tree.node staged beside the bundle. The relay build asserts the symbol on the artifact it produces, but a bundle and the addon beside it redeploy independently, so a host that has not taken a new bundle keeps whatever binary is already there -- and the published prebuilt is node-addon-api, so it binds cleanly and then walks every process's address space. loadWindowsProcessTree now checks that file too and refuses it, falling back to the CIM scan: slower, but not the thing an EDR quarantines a host for. The predicate is duplicated rather than imported, because the config-script copy is install-time tooling that drags in node-gyp and child_process, and this module is bundled into the app and the relay. And it returned false for a binary that is not there. All three callers happened to be safe, but the name read as a safety predicate, so a future caller would take a missing binary as verified. inspectWindowsProcessTreeAddon() now answers clean/unpatched/missing over an explicit binary path -- which is also what lets the relay's staged addon be checked at all -- and each caller states which state it acts on. Both are covered by cases that fail against the old code: without the load-time check the unpatched staged addon is bound and the CIM fallback never runs, and with 'missing' folded back into 'clean' the absence case fails outright. * test(windows): load the addon in beforeAll, not at collection time loadAddon() ran while the file was being collected, so on a Windows checkout with no built addon the require threw before any case existed and took the seven patch-text cases down with it -- cases that read only the patch file and need no binary at all. Verified both ways against a deliberately unresolvable addon path: at collection time vitest reports "no tests" for the file; from beforeAll the seven text cases pass and only the three addon cases go. * fix(deps): normalize the windows-process-tree patch to LF and let pnpm own its hash `pnpm install --frozen-lockfile` failed on this branch on every platform with ERR_PNPM_LOCKFILE_CONFIG_MISMATCH, which breaks CI and the release build. Two coupled defects. The patch file was committed with CRLF -- 174 CR bytes, against zero on main -- and `.gitattributes` pins `/config/patches/*.patch -text` precisely so checkout cannot convert it, so those bytes reached every runner. And pnpm hashes a patch **LF-normalized**, so the raw sha256 of a CRLF file is a value pnpm never computes: raw sha256 322965470c05f63d8527f7d8e892ee26ee444136b66b57fd64c362a9f2ff05d1 LF-normalized f8ea245391c94da5770045aeea01fa6de466c2199c6ef46b5b769b398aa9823e The lockfile carried the raw one, at all three sites. It is the only one of the seven patches where the two digests differ, which is why the other six passed. Normalized the patch to LF and took pnpm's own value from `pnpm install --no-frozen-lockfile`; nothing here is hand-computed. With the file LF-only the two interpretations coincide, so the lockfile, the contract test's no-CR assertion and its hash assertion all agree at one number -- and `config/scripts/windows-process-tree-patch-contract.test.mjs`, which was red on this branch for the same reason, is green again. The lockfile diff is exactly the three hash lines. The regression check is the installer, not a digest. Two separate reviews "verified" the shipped hash by recomputing sha256(patchBytes) and matching the lockfile; both were wrong, because both repeated the same wrong assumption about which bytes pnpm hashes. A check that reproduces the original mistake is not independent. So the new case runs `pnpm install --frozen-lockfile --lockfile-only --ignore-scripts` against a copy of the manifest, lockfile and patches, and asserts exit 0 -- verified by deletion: restoring the shipped hash fails it with the exact ERR_PNPM_LOCKFILE_CONFIG_MISMATCH from the branch's package (windows) job. Also corrected the `.gitattributes` comment claiming pnpm hashes patches byte-for-byte. The `-text` setting is right -- `git apply` needs the exact bytes -- but that sentence is the claim that produced the wrong hash twice. * ci(windows): run the process-tree patch suites in CI Both suites only self-skip off Windows, so the binary-level check that the addon carries no ReadProcessMemory passed vacuously in every lane. * fix(windows): force core.autocrlf=input for the patch repair My LF normalization of the windows-process-tree patch broke the `git apply` repair path introduced in this PR. The two are coupled and I checked only one. Those 174 CR bytes were not editor noise. They sat on exactly the pre-image lines and nowhere else -- 107/107 in src/process.cc, 67/67 in src/process_commandline.cc, 0 on every added or context line -- because @vscode/windows-process-tree@0.8.0 ships those two sources as CRLF. Normalizing the patch made its pre-image stop matching the file it is applied against. Measured, reconstructing the true CRLF pre-image from the pre-normalization blob and applying the current LF patch: core.autocrlf plain -c core.autocrlf=input true exit 0 exit 0 input exit 0 exit 0 false exit 1 exit 0 `false` is Git's own built-in default and what "checkout as-is" selects in the Git for Windows installer -- on this box the `true` that hides it comes from the installer's system gitconfig, not from anything in the repo. There the repair throws, ensureWindowsProcessTreeCommandLinePatch reports "still reads the PEB, and repairing it ... failed", isWindowsNativeLockError does not match that text, and `pnpm install` dies with no path forward. Forcing the mode rather than `--ignore-whitespace`: both fix every cell and both leave the applied file fully LF, but `input` relaxes line endings only, so a hunk whose real content drifted is still rejected. The repair rewrites a security-relevant source file; it should stay strict about everything except the thing that is legitimately ambiguous. Not reverting the patch to CRLF: windows-process-tree-patch-contract.test.mjs (pre-existing on main) forbids CR bytes in it, and pnpm computes the same hash either way. LF plus the forced mode is the end state. The suite could not have caught this. The fixture built its pre-image from the patch itself and joined with '\n', so fixture and patch agreed by construction on any encoding -- once again a test that passes without its fix. It now emits the CRLF the real package ships, and the case runs under both autocrlf modes pinned through a temp HOME gitconfig, because the repair blinds git to the repo and so reads global config. Verified by deletion in both directions: with the flag removed the autocrlf=false case fails with the exact "still reads the PEB" dead end while autocrlf=true still passes, and with the fixture back on LF all eight cases pass with no fix present at all. Also corrected the .gitattributes comment I added last commit. It said `git apply` needs the bytes the patch was written against, which is now false -- the pinned bytes are LF and the bytes it was written against are CRLF. That is the same class of confident-and-wrong claim that produced the bad hash twice. * fix(windows): assert the rebuilt addon, and install the patch for real in tests Three follow-ups from review. **The packaged binary had no check.** The relay build asserts its own artifact and ensure-native-runtime asserts what it loads, but nothing looked at the addon copied into the packaged app -- so a rebuild that silently produced the upstream reader shipped. `rebuild-native-deps.mjs` now asserts `clean` on it after `rebuild()`. This is also the caller D4's tri-state was missing: every existing site branches on `=== 'unpatched'`, so `missing` still behaved exactly like `clean` everywhere, which was the thing making it a state rather than a boolean. Here both non-clean states fail, and they fail differently: after a rebuild that reported success, an absent binary is a broken build, not an absence to shrug at. The fake `rebuild()` had to start producing a binary for that to mean anything, so it now emits stand-in bytes and takes `addon: 'clean' | 'unpatched' | 'none'`. Verified by deletion: with the assertion removed both new cases pass. **The frozen-install case could not see a patch at all.** `--lockfile-only` resolves and never applies one, so its coverage stops at hash consistency. Added a case that installs `@vscode/windows-process-tree@0.8.0` for real with the patch and asserts the materialized `src/process_commandline.cc` carries the marker and no longer carries `ReadProcessMemory` -- about 1.5s for the pair. Correcting the brief on that one: it does **not** catch the `git apply` breakage from the previous commit. Measured -- with `-c core.autocrlf=input` removed it passes cleanly, because `pnpm install` uses pnpm's own patch applier and never runs our repair script. What it does catch is a patch pnpm can no longer apply: corrupting one pre-image line fails both cases. The repair path stays covered by the CRLF fixture in rebuild-native-deps-node-pty.test.mjs. Worth recording, since it decides whether the LF normalization was safe at all: pnpm applies the LF patch to the CRLF tarball sources without complaint, and materializes them as LF with the marker present and `ReadProcessMemory` absent. The primary install path was never affected -- only the `git apply` fallback was. **Dead timeout.** The frozen-install case passed `timeoutMs: 300_000` to the spawn while vitest capped the case itself at 30s, so on a cold runner vitest would have killed it first. Both cases now declare the budget they use. * test(windows): route the frozen-install check through the pnpm invocation owner The new patched-dependencies check hand-rolled a PATH walk naming 'pnpm.cmd', which the windows batch shim spawn boundary ratchet rejects: pnpm-cli-invocation already owns that decision for every other script, and its allowlist only shrinks. Reuse resolvePnpmCliInvocation for the command and prefixArgs, and the shared resolveCliCommand for the presence check, so no shim name is spelled here. Its `shell` flag is dropped because runProcessSync refuses it and already drives a shim through the interpreter itself. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> |
||
|
OrcaWinandOrca Worker
|
687a22e1ee |
fix(computer-use): run the Windows runtime as one persistent helper (#17858)
* fix(computer-use): run the Windows runtime as one persistent helper
Microsoft Defender for Endpoint raised multi-stage Execution + Collection
incidents against Orca on Windows ("Screenshots were taken unexpectedly on
this device... Screen capture code was found in a script launched by
powershell.exe", factor "Executes suspicious MSIL code"). The desktop script
provider spawned a fresh powershell.exe per operation, so a single computer-use
session produced a burst of short-lived PIDs and re-emitted runtime.ps1's
inline Add-Type P/Invoke assembly on every click.
runtime.ps1 gains a -Serve mode that loads its assemblies once and then reads
NDJSON requests from stdin, and a new DesktopScriptRuntimeHost owns one
long-lived child: lazy spawn, strict serialization, a 30s per-request timeout,
restart on crash, a 120s idle shutdown, and dispose() on provider teardown. The
one-shot -OperationPath path stays as the fallback, and Linux keeps its python3
bridge unchanged.
Both Windows spawn sites now use -ExecutionPolicy RemoteSigned instead of
Bypass, falling back once to Bypass (and logging) when a Restricted host
refuses the unsigned script.
* fix(computer-use): recover the runtime host instead of latching it off
Review follow-up on the persistent Windows computer-use helper.
A helper that died before producing a line set an unavailable flag nothing ever
cleared, and the client then dropped the host for the life of the session. One
transient bad spawn — a Defender scan, a locked CSC temp directory — silently
restored the per-click powershell.exe burst and per-operation MSIL emission this
work exists to remove, with computer use still working so nothing looked wrong.
Start failures are now retried, then cool down for 60s, then re-probed; the
client keeps the host so it can come back. Repeated post-answer crashes cool
down too, and a single reply no longer clears the failure count.
The one-shot bridge decided its execution-policy retry from a message that fell
back to stdout, so a window title containing "SecurityError" could replay a
non-idempotent operation — a double click, keystroke or paste — and stick the
session on Bypass. The retry now requires empty stdout and a matching stderr.
Serve-mode replies carry an echoed request id. Without one a single stray stdout
line would make every later response answer the previous request, acting on
stale element indexes with no error raised; a mismatch now kills the child.
Non-JSON noise is ignored rather than counted as the helper having answered.
Also: warnings reach the main process over the sidecar's IPC channel rather than
its piped, unread stdio; the child is watched on close rather than exit; dispose
latches so a queued request cannot respawn during teardown; and the host is
split into a serve channel and an availability policy to stay under max-lines.
* fix(computer-use): prove a helper never started before replaying its request
The retry that replaced the permanent-latch bug could deliver unrequested
input. send() re-sent the same request whenever the helper died without
replying, but "no reply came back" is not "the operation did not run":
runtime.ps1 synthesizes the click and only then builds the snapshot, which
allocates a full-window bitmap and walks the UIA tree — a native GDI+/UIA fault
there is uncatchable, and leaves the click already delivered. A deterministic
fault meant three clicks from the host plus a fourth from the one-shot bridge,
surfaced as a single failed operation.
-Serve now writes one {"ready":true} line after its Add-Type work and before
its first read, so "never started" is a fact rather than an inference. A request
is replayed only when the helper died before announcing. A runtime.ps1 that
predates the announcement — reachable through the provider path override — is
covered by an observation-tool allowlist until a ready line proves otherwise.
Host-detected aborts (timeout, desynchronised reply, oversized line) suppress
the exit handler, so they were bypassing failure accounting entirely and a
helper failing that way was respawned once per operation forever. They now
count and are logged.
Also stop charging twice for one outage: entering the cooldown resets the
failure count, so the first death after recovery no longer re-enters a full
cooldown and an interleaved workload cannot be stranded on the one-shot bridge.
* fix(computer-use): ignore a stdin write callback from a torn-down helper
stop() destroys stdin, so a write still queued at teardown calls back with
ERR_STREAM_DESTROYED. The callback carried no channel or request identity and
write() had no closed guard, so it ran abortChannel a second time: stopChannel
no-opped but recordFailure and the warning did not, charging two failures for
one operation and reaching the 3-strike cooldown at half the intended rate.
That feeds the same accounting that keeps a persistently broken helper from
respawning once per operation.
The same root also allowed a late callback landing after a replacement channel
existed to stop that channel and reject a different request with the previous
one's error. Node fires the destroyed-stream callback on the next tick, well
before a new request arrives, so the double-count is the reachable effect;
binding the callback closes both.
write() now drops payloads and error reports once closed, and the host ignores
any report whose channel or request id is no longer current.
* test(computer-use): pin each stale-write guard independently
The channel's closed guard and the host's request-identity check are redundant
by design, and the existing tests only failed when both were absent. Someone
deleting one, believing the other was the covered one, would have got a green
suite and a live regression — the same shape as a test that passes without the
fix it was written for.
Each is now pinned on its own. The channel's half is tested against the channel
directly: after stop() it takes no writes and reports no error from one already
queued, which the host cannot observe because it drops the channel at the same
moment. The host's half is pinned by the case the channel cannot see — a live
channel whose request was already answered, where backpressure delivers a write
callback for a request that is no longer pending.
Removing either guard alone now fails a test. Both carry a comment saying they
are deliberately redundant and separately pinned, so the next reader does not
have to rediscover this from the diff.
* ci(windows): run the computer-use runtime host suite in CI
The win32 suite only self-skips off Windows, so it passed vacuously in
every lane. Register it the way the cmd-shim suite is registered.
* fix(computer-use): time the runtime host cooldown on a monotonic clock
The start-failure cooldown was a wall-clock deadline, so a backwards step —
an NTP correction, a VM snapshot restore, a user changing the clock — left
`remainingCooldown()` returning the cooldown plus the whole step. A one-hour
step measured 3,660,000ms, and ten real minutes later still 3,060,000ms.
Nothing shortens it from there. Only `recordSuccess()` clears the cooldown on
a non-dispose path, and no request can reach a helper to succeed while it
holds, so every `send()` throws `runtime_host_unavailable` first. The host is
built with no `now` override and its lifecycle is a module-level singleton
that shuts down at process exit, so the latch held for the sidecar's life —
computer use kept working via the one-shot bridge while the per-click
powershell.exe burst this host exists to remove came back silently.
Store the instant the cooldown began and compare elapsed monotonic time,
following the two fixes in #17884. The field is `number | null` rather than
sentinel 0 because `performance.now()` legitimately returns 0.
Both new tests leave `now` unset, because the bug was in the default the host
picks and a test that injects a clock cannot see it.
* fix(computer-use): give a queued request its own deadline
The 30s request timeout was armed only in `sendOnce`, once a request reached
a helper. A request behind N timing-out ones therefore waited roughly N times
that with no deadline of its own: bounded, but the caller sees an `await` that
looks hung for minutes and gets no error to act on.
Move the serialization tail into its own class and arm a deadline at enqueue
time. Only the wait is bounded — a request that reaches a helper still gets
its full execution budget, so nothing that used to succeed now fails. An
expired request is dropped rather than sent late: the caller has already been
told it failed, and a click delivered after that is worse than no click.
The tail keeps its never-rejecting shape and chains on the turn rather than on
the raced promise, so a caller giving up early cannot release the next request
while its predecessor is still in flight.
* fix(computer-use): stop reading a locked file as an execution policy block
`UnauthorizedAccess` is the FullyQualifiedErrorId PowerShell reports for a
policy block, and it is also a strict prefix of `UnauthorizedAccessException`,
which .NET raises for any ordinary locked or ACL-denied file. The predicate
matched the token unanchored, so an AV scan holding runtime.ps1 or a locked
CSC temp directory was read as a policy block.
Two consequences, both bad. `escalateExecutionPolicy()` has no path back, so
one false match spent the rest of the session on `-ExecutionPolicy Bypass` —
the exact command line token this stack exists to stop emitting. And on the
one-shot path `isPolicyBlockedStart` re-runs the operation: one-shot mode
writes stdout only after the operation returns, so a crash partway through an
action is indistinguishable from a helper that never started, and the click
lands twice.
Measured on Windows against all three records, which the test carries verbatim
as fixtures:
policy/Restricted FullyQualifiedErrorId: UnauthorizedAccess
policy/RemoteSigned FullyQualifiedErrorId: UnauthorizedAccess
genuine access denied FullyQualifiedErrorId: UnauthorizedAccessException
`\b` is the whole discriminator: between `s` and `E` both sides are word
characters, so no boundary exists there and the exception cannot match.
Dropped two alternatives that measurement showed were wrong. `PSSecurityException`
never appears — the record surfaces through a native-command wrapper and reports
`ParentContainsErrorRecordException`. The prose is wrong three times over: it
differs by policy, it is localized, and PowerShell hard-wraps it mid-sentence.
Anchoring on the `FullyQualifiedErrorId:`/`CategoryInfo:` labels would be more
precise again, but those labels are localized where the values are not, so it
would lose a real block on a non-English host and strand it with no fallback.
Matching the values with word boundaries keeps both directions; a fixture with
translated labels pins it.
The escalation stays sticky. With the predicate correct, it only fires on a
machine that really does block, where re-probing the preferred policy would buy
a guaranteed failed spawn per operation.
* fix(computer-use): route a malformed request back to the request that caused it
`ConvertFrom-Json` throws before `$requestId` is read, so the serve loop
answered an unparseable request with an untagged error. On the client that is
not an error at all: `deliver()` sees no matching id, calls `abortChannel`,
kills the helper and charges a failure — and the helper's own message is
discarded. A parse failure was reported as a stream desync with no trace of
the real cause, and three of them walked into the 60s cooldown behind three
misleading "did not match" messages.
Recover the id from the raw line when the parse fails. No wire change: the
response shape is untouched and `BridgeResponse.requestId` already documents
this echo. It is the same shape the helper already returns for `not_a_tool`,
where the id survives because it is read before the operation runs. Both
mixed pairings degrade safely — a new script with an old client resolves the
error normally, and an old script with a new client still aborts, but now
reports what the helper said.
When the line is mangled past recovering an id, the desync abort is the honest
outcome, so keep it and carry the helper's text into it rather than replacing
it. A line the helper could not tag is usually the only account of the cause.
Proven against the real `runtime.ps1 -Serve`: the host can only write
well-formed JSON, so the parse-failure branch is unreachable through it and
the test drives the channel directly.
* fix(computer-use): keep the Bypass escalation only when Bypass actually works
AppLocker and WDAC constrained language mode raise PSSecurityException under
the same SecurityError category a real execution-policy block uses, so the
predicate matches them - correctly, on the evidence available. But those block
the script at parse time, which `-ExecutionPolicy Bypass` cannot lift. The
escalation was sticky unconditionally, so on a WDAC host we misdiagnosed,
retried, failed again, and then latched: every later command line carried the
most heavily weighted MDE token there is, on exactly the hardened, monitored
enterprise machine that is watching for it.
Treat the escalation as the diagnosis it is. A fallback that cannot start a
helper either disproves it - the policy was not what stopped the first attempt
- so revert to RemoteSigned instead of latching. When Bypass does start a
helper the diagnosis is confirmed and it stays sticky exactly as before, so a
genuinely Restricted machine still never pays a re-probe per operation.
The revert lands inside the outage rather than only at its end, so a
misdiagnosis costs one Bypass command line instead of one per attempt, and an
escalation that never proved itself does not outlive the cooldown that ends
the outage. Deliberately not a permanent "fallback is useless" flag: a Bypass
attempt that failed for a transient reason would then disable the fallback for
the session, which is the same latch in the other direction.
Only `runtime_host_unavailable` proves no helper started, so only that reverts;
a helper that started and then died proves Bypass works. That also makes the
policy branch reachable on a final attempt for the first time, so it now
rejects as unavailable rather than a generic error - that code is what routes
the operation to the one-shot bridge, which carries its own policy fallback,
and without it an all-blocked host would fail operations outright instead of
degrading. The pre-existing "reports itself unavailable when Bypass is also
refused" test pins that.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
OrcaWinandOrca Worker
|
b6ca8dad99 |
fix(hooks): register the Claude hook script directly on Windows (#18875) (#18905)
* fix(hooks): register the Claude hook script directly on Windows (#18875) The Windows Claude Code lifecycle hook was registered as `powershell.exe -NoProfile -EncodedCommand <...>` whose entire decoded payload was a `Test-Path` and a call to `~/.orca/agent-hooks/claude-hook.cmd`. Every hook event paid a full PowerShell start-up to reach a script that exits at its first `ORCA_PANE_KEY` guard, so sessions outside Orca paid it to do nothing. Register the script path itself instead, with `|| echo {}` for the neutral-JSON-when-missing contract (#14818). Measured on Windows 11, invoked as Claude Code invokes it (`printf payload | bash -c -l "<command>"`): idle (n=12) baseline 177ms | before 471ms | after 213ms 10-way conc (n=40) -- | before 656ms | after 296ms p95 under load -- | before 696ms | after 337ms It also drops an interpreter from the chain the hook's timeout kill must tear down. Killing the hook does not kill its PowerShell grandchild, which still holds the stdout handle the agent reads to EOF -- measured, EOF arrived 352ms AFTER the kill, when the orphan exited by itself. msys2 creates children suspended and resumes them after, so a kill landing in that window strands one that never exits and EOF never comes; that is the reported frozen session. The encoded launcher stays as the fallback for profile paths the shells cannot carry bare (space, `%`, `^`, `&`, non-ASCII) and for hosts where Git Bash is not resolvable, because PowerShell 5.1 rejects `||`. Every other agent's hook is untouched, as is the remote/SSH path. Not adopted from the report: `cmd.exe /d /c <path>` (MSYS rewrites the `/c` under Git Bash -- measured, the invocation fails), and raising the 10s timeout (the orphan survives the kill regardless; the fast path puts the hook 30x under the budget so the kill effectively stops firing). * fix(build): list the new hook launcher modules in the CLI tsconfig project config/tsconfig.cli.json enumerates its files explicitly, so the two new imports reached by src/main/claude/hook-settings.ts failed tc:cli with TS6307. src/main/git-bash.ts pulls in only node:fs, node:path and a shared constant, so it adds nothing heavy to the CLI project. * fix(hooks): address review of the direct Windows Claude hook launcher - Make the Windows hook suites host-independent. A box with a cmd.exe AutoRun (HKCU\...\Command Processor\AutoRun) failed them at HEAD too: the tests redirect USERPROFILE, the AutoRun target vanishes, and MSYS spawns a .cmd without /d so AutoRun runs and lands on the hook's stderr. Seed an empty target, including under the deliberately-absent profile. - Note in managed-hook-stdin-lifecycle why the "missing managed script" case no longer exercises the fallback for the direct shape (it carries an absolute path, so a redirected profile changes nothing); that path is covered live in windows-direct-cmd-hook-command.test.ts. - Keep the direct shape off UNC profiles: WINDOWS_CMD_SAFE_PATH admits them, but //server/share/... is not a command cmd.exe reliably starts. - Correct the comments: `|| echo {}` also fires when cmd.exe itself exits non-zero (failing AutoRun), printing {} twice. The encoded launcher exited 1 on that same box, so neither shape is clean there. - Test the contract that replaced runtime %USERPROFILE% resolution (STA-3348): a stale absolute path reports not_installed and is rewritten on install. - Record the standing unmeasured assumption in windows-edr-posture.md: `||` does not parse in Windows PowerShell 5.1, so a compat consumer that hosts hook strings there would fail closed. Measure before widening to another agent. - Trim the launcher comments per AGENTS.md; the numbers live in the doc. * test(win32): register the new Windows-gated hook test in the CI lane win32-test-lane-registration guards against exactly this: a Windows-gated file that self-skips on ubuntu and reports success, so it runs on no machine. The new windows-direct-cmd-hook-command.test.ts needs both entries — WINDOWS_PACKAGE_TESTS decides whether package_windows runs for a diff, and the workflow argv decides whether the file runs once that job started. * test(win32): remove the hook temp tree through the retrying helper windows-lane-tree-removal-boundary scans exactly the specs in the Windows CI lane, so registering windows-direct-cmd-hook-command.test.ts subjected it to the rule: cmd.exe and bash have just exited in that tree, and a raw recursive rm throws EPERM on Windows while their handles drain, turning a green spec into a lane failure. Use removeTreeSync, which carries the repo's maxRetries policy. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
Neil
|
0a821e5bc8 |
fix(crash-reporting): make the own-Chromium gate a real choke point, and stop a refusal leaking the root (#18459)
* fix(crash-reporting): make the own-Chromium gate a real choke point
Round-3 review found the guard was not the choke point its own comments
claimed: six pid-addressed `taskkill /pid <pid> /t /f` families in main were
ungated and uninstrumented, so the stale-pid shape stayed producible and a
`selfInitiatedTreeKillCount: 0` could read as exculpatory when it was not.
- Gate the remaining main-process families: the git command-runner abort, the
notebook-cell and automation-precheck timeouts.
- Turn the `src/shared` seam into the gate itself (`process-tree-kill-gate`), so
the runProcess choke point, the codex app-server deadline kill and the
ephemeral-VM recipe kill ask the same decision. Those three are compiled into
the CLI/relay too and cannot import main; main installs the guard at preflight.
- Ratchet (`main-process-tree-kill-gate.test.ts`): a new pid-addressed taskkill
in main that skips the gate fails, and the allowlist entries must still exist.
- Give pid-addressed kills eviction priority in the 32-entry ring: 32 routine
`win-pty-job` teardowns from a window-close burst no longer evict the one
entry that discriminates a self-kill from an external one.
- Correct the coverage doc, which described the uninstrumented Windows sites as
POSIX `process.kill(-pid)` group kills and omitted the git and codex paths.
* fix(crash-reporting): keep a refused tree-kill from leaking the root it owns
A refusal must block the pid-addressed tree walk, not the termination. Five of
the six gated sites returned on refusal with no fallback, so a refused
`taskkill /pid /t /f` left git.exe, a timed-out notebook cell, an automation
precheck or an ephemeral-VM recipe running while the caller reported it stopped.
The root kill is addressed by the child handle, which cannot reach the recycled
pid the refusal is about, so it stays correct and required on that path.
Also fixes the ring eviction the scope preference introduced: with the ring
saturated by pid-addressed kills, the only non-pid-addressed entry is the one
just pushed, so the splice evicted itself and the detail came back `{}` --
byte-identical to the external-kill arm, in the window-close case the guard
exists for. Eviction now excludes the newest entry and falls back to FIFO.
Tests: refusal now asserts the root kill at all six sites, and the ring covers
the saturated-pid ordering as well as round 3's group-burst ordering.
* fix(crash-reporting): stop a refused tree-kill leaking the commit-message agent, and count call sites
Two round-5 blocking findings, both open on main and on both branches.
`killSourceControlAgentProcess` had no root-kill fallback on its win32 arm: the
taskkill was the only termination, so once the own-Chromium gate could refuse it
the promise resolved having killed nothing. Both callers do
`terminationComplete ??= killSourceControlAgentProcess(child)` and then release
the managed-home lock on that promise, so a refusal left the local Codex/Claude
commit-message agent running while the caller reported it stopped -- the
lock-contention failure the taskkill was added for. Same fix as the six sibling
sites: the handle-addressed root kill cannot reach the recycled pid the refusal
is about, so it stays correct and required on that path.
The ratchet was file-granular, not call-site granular: one gate mention anywhere
in a file exempted every taskkill in it, which left the six files that now ask
the gate ratchet-blind -- the inverse of what it is for. It now counts `/pid`
call sites against gate admissions per file, so a second ungated kill inside an
existing family fails. Keying on the `/pid` argument rather than a quoted
`taskkill` also catches a kill whose program name comes from a constant. The
three comments that claimed more than the old scan enforced now state the rule
and its two remaining blind spots.
Also: the recording in `admitSelfInitiatedTreeKill` is now wrapped the way the
`admitProcessTreeKill` seam already wraps it, with the refusal decision taken
before anything that can throw so a diagnostics failure cannot flip it; and
`orca-chromium-process-pids` documents the false-positive direction (a stale
`getAppMetrics()` entry plus pid reuse refuses a live unrelated child), which is
the mechanism the root-kill fallback exists to bound.
Tests: refusal now asserts the root kill at all seven sites; the ratchet asserts
call-site counting and the constant-program form.
* test(crash-reporting): run the own-Chromium gate against real Windows trees
Nothing on this branch had ever executed on Windows. The unit tests pin the
gate's decision against a mocked taskkill, which cannot show that the decision
does anything to a real process: that `/T /F` reaps a detached grandchild, that
a refusal leaves that tree standing, or that the handle-addressed root kill the
refusal path falls back to reaps the root while orphaning descendants.
Adds a win32-gated live test covering all four, registered in both the
`package_windows` CI lane and `WINDOWS_PACKAGE_TESTS` as
`win32-test-lane-registration` requires.
Also completes the coverage doc's "never instrumented" list, which omitted the
macOS keyboard-input-source probe's POSIX group kill in `ipc/app.ts`.
* fix(crash-reporting): pin the commit-message root kill on the Windows arm
The first Windows run of this branch found nine failures the macOS suite
cannot see: `commit-message-text-generation-test-harness` asserts
`expect(child.kill).not.toHaveBeenCalled()` on `process.platform === 'win32'`,
which is the contract the previous commit deliberately replaced — and it
branches on the real platform, so it is dead code everywhere CI runs today.
The harness now asserts the handle-addressed root kill on every platform. On
win32 it lands after the tree walk, so the expectation waits rather than reading
one tick early, and its ten call sites await it. Red against the pre-fix arm at
all seven sites; the production code is unchanged.
* test(crash-reporting): remove the Windows lane marker tree through the retrying helper
The new win32 spec teardown used a raw rmSync, which the windows-lane-tree-removal
boundary ratchet rejects — and which is exactly the EPERM the ratchet exists to
prevent, since this spec's marker directory is written by processes it has just
force-killed.
* fix(crash-reporting): only refuse pid-addressed tree walks, disclose the handle-less codex site
The own-Chromium gate refused the POSIX process-group arm of
signalProcessTree as well, which was new macOS/Linux behaviour: a stale
getAppMetrics() entry plus pid reuse would orphan a group that main reaps
today. A POSIX group only holds what Orca put in it, so the refusal is now
scoped to win-taskkill-tree and the POSIX arm is recorded and admitted like
the other group kills in main. That also drops the synchronous
getAppMetrics() read from every POSIX termination.
codex-turn-added-roots kills roots found by a table walk, so a refusal has
no handle to fall back to. Pin that the refusal is visible - crumb written,
turn reported as not cancelled - rather than fixing what cannot be fixed.
* test(crash-reporting): detach the Windows survival fixture and observe real spawns
|
||
|
|
f36c03e84a |
fix(windows): make the install-dir ACL repair rescue the launch it runs in (#18361)
* fix(windows): repair the poisoned install-dir ACL before the window, not after The install-dir LPAC ACL poison (electron/electron#51761) still costs every affected machine at least one crash: the probe that detects it is setImmediate-deferred and answers 0.9-3.0s in, while createMainWindow runs synchronously in the same frame and its renderer dies at init 48-1373ms later. - Persist the poison verdict the moment the probe reports it, and await the repair (bounded at 20s) before any window is created on a launch that already carries the marker. - Do not engage the GPU safe-graphics fallback while the install-dir ACL verdict is poisoned or still outstanding. Safe graphics does not rescue a poisoned tree, and --in-process-gpu removes the GPU child, erasing the sibling-death evidence that identifies the shape (4 field reports landed in 'misc' this way). - Clear the safe-graphics marker once the repair lands, so a repaired machine stops launching software-rendered for the rest of that build. - Give the repair marker a bounded retry budget: it was written on failure and matched regardless of outcome, so one transient failure pinned a machine to 'marker-hit' for the life of that version. * test(windows): pin the install-dir ACL repair against the real icacls binary * fix(windows): stop the install-DACL verdict from outliving the evidence Adversarial review round 1. Five blocking findings, all addressed. 1. gpu-lifecycle guard had only a source grep (green with the polarity inverted). The stated justification -- that gpu-lifecycle's import graph cannot be driven in-process -- was wrong: mocking `electron` plus `@electron-toolkit/utils` imports it fine. Replaced with gpu-lifecycle-install-dir-acl-guard.test.ts, which drives the real handleGpuChildCrash against a stub tracker. All four cases go red when the guard is flipped to `if (!isInstallDirAclSuspect())`. 2. A clean probe verdict retired the on-disk marker but not the in-memory `poison` verdict, so a machine the probe just proved healthy kept suppressing the GPU safe-graphics fallback and kept the dialog accusing the install folder -- permanently, since a `status:'failed'` probe deliberately keeps the marker. A positive clean reading now latches `installDirReadClean`, drops the verdict, and outranks a repair result that lands after it (a 'failed' from a repair with nothing left to fix must not re-accuse). 'repaired' is kept: it is not a contradiction and it is what tells the user to reload. 3. `noteWindowsInstallDirAclProbePending()` ran on every `openMainWindow` while the probe is once-per-process, so every tray/second-instance reopen armed a 15s window in which `recordGpuCrash` was never called at all -- on healthy machines. `probeWindowsInstallDirAcl` now reports whether THIS call dispatched, and only a dispatch arms the grace window. 4. The pre-window ordering guarantee was defeatable and untested. `focusExistingMainWindow` opens a window whenever there is none and the app is ready -- true for the whole 20s gate, which is exactly when a user double-clicks the shortcut again. Added a `canOpenWindow` seam (same 'pending' semantics as the existing `!app.isReady()` case) wired to `isBlockingInstallDirAclRepairInFlight()`, plus windows-install-dir-acl-startup-wiring.test.ts pinning the await ahead of both window-creation paths and both new call sites. 5. windows-install-dir-acl-repair.win32.test.ts was absent from the pr.yml win32 allowlist, so it ran nowhere. Added. Also from the non-blocking list: - The repair no longer clears a `userConfirmed: true` safe-graphics marker; "keep safe graphics" is a user choice, not Orca's automatic latch. - `repairWindowsInstallDirPackageAcl` now reports its dispatch too, so a second entry into the gate resolves immediately instead of eating the full 20s budget waiting on an `onDone` that is never coming. - The gate is wrapped in try/catch/finally, matching the contract the probe documents as mandatory for anything upstream of window creation. Rebutted, not applied: - "Gate should be conditioned on app.isPackaged." A dev launch only carries the poison marker if a dev launch actually probed that tree and found the signature, in which case the dev renderer is dying the same way and the repair is exactly what is needed. The adjacent `isPackaged` check guards a packaged-only early-window optimisation, not a correctness boundary. - "Fold the poison marker into the repair marker's `outcome`." They answer different questions with different lifetimes. The repair marker is a retry budget (`attempts >= 3` disables the repair for that version) and is never cleared; the poison marker is cleared by a successful repair and by a clean probe. A `'pending'` outcome written before the attempt would bump `attempts`, so three launches killed mid-repair would permanently disable a repair that never once ran icacls to completion. * fix(windows): keep counting GPU crashes while the install-DACL verdict is pending Adversarial review round 2. Both blocking findings addressed. 1. handleGpuChildCrash early-returned on isInstallDirAclSuspect() BEFORE recordGpuCrash, so the crash left no trace in the 30s rolling window. The suspect window is armed on every win32 non-serve launch, and the field bundles put it at 0.8-1.7s after main_window_created on hosts whose DACL is clean (matchesPoisonSignature=false) -- squarely inside the 2.1-6.2s bad-driver bursts this repo already pinned in gpu-crash-fallback-field-sessions.test.ts. A healthy machine with a failing driver could lose an entire coalesced burst and never engage safe graphics. The crash is now always recorded; only the engagement consults the verdict, and it waits for the verdict rather than acting on the suspicion (waitForInstallDirAclVerdict, resolved by the probe's onDone or by the existing 15s grace, whichever lands first). Deviation from the review's suggested shape, deliberately: awaiting the verdict before persisting anything reintroduces the exact race gpu-fallback-engagement.ts documents -- Chromium aborts the whole browser process on the 6th GPU crash, ~1.3s after the 3rd, which is less than the probe takes to answer. So the unconfirmed marker is written up front and withdrawn if the verdict comes back poisoned. A machine killed mid-wait still comes back software-rendered, and its marker is unconfirmed, which is the state the repair's own clear already retires. gpu-lifecycle-install-dir-acl-guard.test.ts now drives the real GpuCrashFallbackTracker and the real engagement path (the restart prompt firing is the signal) instead of a stub tracker, and covers the case the previous suite could not express: a burst that lands entirely inside the pending window still engages once the probe reports clean. Four reverts go red -- restoring the pre-record guard (2 tests), dropping the wait, dropping the post-wait re-check, and dropping the pre-wait marker write (2 tests). 2. The round-1 evidence block quoted commits, a test name and pass counts that no longer exist, and its real-icacls Windows run predated the commit that rewrote the gate. Re-run at this commit; counts and the live-Windows result are restated in the handoff rather than carried forward. Also from the non-blocking list: - 'marker-hit' conflated "already repaired" with "retry budget spent", because hasMarkerFor matches outcome === 'repaired' too. The result now carries alreadyRepaired, and the recovery maps that to stage 'repaired' -- so a launch killed between a successful repair and its marker clear no longer tells the user the folder needs an administrator, no longer latches isInstallDirAclSuspect() for the session, and does retire the poison marker. Not applied, with reasoning: - "clearGpuFallbackMarker narrowed to userConfirmed === false leaves the target population software-rendered after a repair." The summary was overstated and is corrected, but the narrowing stands: a userConfirmed marker now requires a clean DACL verdict, because the restart prompt that writes it is exactly what the gate above withholds while the install is a suspect. The population this family targets can no longer reach confirmMarker while poisoned. - "writeInstallDirAclPoisonMarker re-stamps on a budget-exhausted machine forever." True, but on that machine the tree really is still poisoned and the gate resolves immediately ('skipped', no icacls spawn, no 20s wait), so the marker is telling the truth. Retiring it would be wrong; only a clean probe reading should. * fix(windows): register the real-icacls spec and stop its teardown racing icacls Two ratchets were red: - windows-lane-tree-removal-boundary: the win32 spec's afterAll used raw rmSync on a tree two icacls.exe children had just rewritten DACLs on, which is the EPERM race removeTreeSync exists for. - win32-test-lane-registration: the spec was in the pr.yml argv but not in WINDOWS_PACKAGE_TESTS, so a future diff touching only test files would not select package_windows and the spec would self-skip on ubuntu and report success. * fix(windows): re-arm the GPU fallback latch when the install-DACL verdict withholds it recordGpuCrash reports the threshold crossing exactly once and latches `engaged`. handleGpuChildCrash consumes that report before consulting the DACL verdict, and installDirAclClearsGpuFallback then discards it — so nothing could ever engage safe graphics again in that process. A machine whose tree the repair fixes and whose driver is genuinely broken stayed hardware-accelerated through an unbounded crash loop, with no prompt and no marker. disengage() releases only the one-shot latch; the crash window is untouched, so a real driver burst is still never erased. Test is RED without the re-arm. * fix(windows): keep the safe-graphics marker while an install-DACL repair is in flight The gate dispatches a repair without arming the probe clock, so waitForInstallDirAclVerdict() returns immediately and the withdrawal deleted the marker inside Chromium's FATAL window (crash 6 lands ~1.3s after crash 3, well inside the 20s gate). The process then died mid-repair, spent no attempt, and relaunched hardware accelerated into the same gate — spawning the same GPU children, FATALing again, forever. Hold the marker while poison.stage is 'pending' so that launch comes back software rendered and the next gate runs to completion. Still not engaged this launch, so --in-process-gpu does not erase the sibling-death evidence. A terminal verdict has no next step to rescue, so it still withdraws. Both new tests are RED without the retention. * fix(windows): stop a repaired marker outranking a fresh poison verdict The probe reads the install DACL and finds it poisoned; `startRepair` dispatches; `markerHitFor` sees a repair marker recording `outcome: 'repaired'` for the same installDir+appVersion and reports `alreadyRepaired`, which the recovery module maps to stage 'repaired'. So the launch that just proved the tree poisoned runs no icacls, deletes the poison marker that arms the next launch's pre-window gate, clears the suspect flag so `--in-process-gpu` can engage on a tree safe graphics cannot rescue, and tells the user "Orca repaired the permissions." Reachable whenever the tree is re-poisoned after one successful repair of the same version, and whenever a repair reports success without clearing the tree — the silent icacls no-op this module exists to document. A DACL reading taken this launch now outranks the marker: `probeConfirmedPoisoned` stops `outcome: 'repaired'` short-circuiting the repair. The attempt budget still bounds it, so an unrepairable tree does not re-spawn icacls forever. The pre-window gate does not set the flag — it acts on a marker from an earlier launch, not on evidence of its own, so a recorded repair still outranks it there. Also drives the GPU-fallback re-arm test through a repair that actually completes 'repaired', rather than a later clean probe, which is the route the review exercised. * fix(windows): make the pre-window ACL gate act on the poison evidence it fired on The gate fired on a poison marker — an earlier launch's DACL reading that nothing has retired — but withheld `probeConfirmedPoisoned` from the repair, so a repair marker recording an older success still short-circuited it. On the three-launch shape the gate exists for (repair succeeds; tree is re-poisoned; the next launch's probe records the poison but dies before writing its repair marker) the gate ran no icacls, deleted the poison marker that arms every later gate, un-suspected the tree so --in-process-gpu could engage, and told the user "Orca repaired the permissions." `applyInstallDirAclProbeVerdict` then swallowed that launch's own reading behind `if (poison) return`. Both callers of `startRepair` hold outstanding poison evidence, so the flag is now unconditional (renamed `poisonEvidenceOutstanding`) and `marker-hit` means only that the attempt budget is spent. The probe guard is narrowed to an in-flight gate repair: a reading taken after the gate finished re-arms the poison marker and downgrades a claimed repair. Also: withholding safe graphics now ends with the repair budget. A machine whose attempts are spent while the signature persists was denied safe graphics on every launch for the life of that appVersion — and had its marker deleted each time — including the healthy installs the probe's flag-blind ACE match over-matches, where the driver really is broken. Non-blocking, same lane: re-read `isQuitting` after the up-to-15s verdict wait, and skip the recovered-launch prompt when the ACL gate retired the marker read before whenReady. * fix(windows): stop a timed-out gate repair outranking a later poison reading The gate's 20s budget expires while icacls runs on under its own 120s cap, so the probe can read the tree poisoned while that repair is still in flight. Its success claim then deleted the poison marker, un-suspected the tree and told the user their permissions were fixed. The reading is now latched and outranks it. * fix(windows): stop a gate repair claim pre-empting this launch's probe reading Round-7 adversarial findings, both driven against the real modules: - isInstallDirAclSuspect returned false the moment the pre-window gate set stage 'repaired', short-circuiting ahead of the probe-pending grace check. The GPU children die 48-1373ms after window creation while the probe answers 0.9-3.0s in, so an icacls that silently no-opped (exit 0, tree untouched) opened exactly that interval to --in-process-gpu on a still-poisoned tree - and a 'keep safe graphics' answer then pinned a userConfirmed marker no later repair may clear, with the poison marker already deleted so no later launch gates. The claim now stays provisional until this launch's probe corroborates it or the grace window lapses. - A probe reading that disproves a 'repaired' claim re-armed the poison marker but never restored the unconfirmed safe-graphics marker the claim had cleared, so the next launch relaunched hardware-accelerated into the re-armed gate. The clear is now captured and handed back on disproof. * test(windows): pin the nested and update-inherited grants against real icacls The live spec asserted the grant landed on the root-level module file only. It now also pins that the flagless /T pass reaches a nested file carrying its own protected DACL (the shape app.asar.unpacked and node_modules have), and that a file written after the repair inherits the (OI)(CI) root grant - the stated reason that grant form exists. * fix(windows): keep the recovered-launch prompt silent while the tree is the suspect Round-8 fresh-eyes finding, driven against the real modules: the prompt re-read the marker the pre-window gate may have retired, but never consulted isInstallDirAclSuspect() - so after a FAILED gate (tree still a live suspect, window blank behind the 10s reveal fallback, Keep as both defaultId and cancelId) a 'keep it' answer pinned a userConfirmed marker no later repair may clear, on the exact victim class the repair cannot help. The guard now covers both gate outcomes; staying silent leaves the marker unconfirmed, which a successful repair still retires. --------- Co-authored-by: Orca Worker <orca-worker@localhost> Co-authored-by: OrcaWin <alpha-eng@stably.ai> |
||
|
Brennan BensonandMerge Sim
|
98e77ef1a7 |
feat(mobile): structured native Codex chat (#18074)
* feat(mobile): finalize structured native Codex chat * fix(mobile): close structured chat lifecycle gaps * wip(mobile): fence stale structured inventory and bound operation-id retention Fence local structured-session inventory and subscription responses with a sync generation so a toggle-off clear, reconnect restore, or retry cannot apply a mirror from a superseded instance. Bound mobile ambiguous operation-ID retention at 128 with unmount cleanup. Staged on the reconcile branch only: the sync module is now 312 lines and needs a real split before this can reach the PR head. * fix(ci): split the structured session-tabs sync and give static analysis mobile types The local structured session-tabs sync module outgrew the 300-line cap once it took on generation fencing, so split it along its real seams instead of raising the cap: the generation/cursor fence, snapshot projection, snapshot apply, inventory refresh, and the subscription loop. The original path stays as a barrel so no importer moves. Repoint the host-session-mirror settle census at the apply module, which owns two receipts now — the snapshot it mirrors in, and the toggle-off teardown that retracts what it published. The teardown receipt is named rather than anonymous so the pin says which direction it settles. The changed-code quality gate lints mobile files and resolves their types from mobile/node_modules, but mobile is a separate pnpm project that the root install never populates, so every mobile type degraded to an `error` type and the gate reported phantom findings. Install mobile dependencies in static analysis when the diff touches mobile, gated on a new classifier output. * fix(mobile): let a slow capability handshake still reach connected The mobile capability update is an advisory whose result is discarded, yet an unanswered one was fatal while an explicit rejection was tolerated. A 5s timeout on the direct client force-closed the socket, and on the relay path it failed `confirmResume` before `connected` was ever published, so a consistently slow link redialled forever. Both paths now share one helper that settles every ambiguous outcome (timeout, mid-flight drop) like a rejection and rejects only when the frame never reached the wire — the one case nothing else recovers from, since the socket's own desync force-close is gated on already being connected. The generation guard still keeps a replaced session from connecting. Retained structured-session operation ids were capped at 128 with oldest-first eviction, but every retained id belongs to a send whose outcome is unknown, so eviction turned a user's retry into a second message on the host. Bound the map by expiry against the id's own embedded timestamp instead, mirroring the host's operation ledger, so no id is released while the host would still honour it. Also give the mobile CI install the root install's lockfile drift guard (mobile's lockfile carries patchedDependencies a silent rewrite would drop), gate mobile_dependencies on should_run, and key the pnpm store cache on both lockfiles. * refactor(mobile): extract the relay pending-request registry The merge composed two independently-sized changes — this branch's capability handshake settle and main's dial-stage tracking — pushing the relay session file to 304 lines against a 300 cap. Neither side broke it alone. Move the in-flight request registry (id generation, tracking, settlement, and reject-all with its delivery-ambiguity marking) into RelayPendingRequests, matching the existing collaborator pattern alongside RelayDialStageTracker and RpcSessionLivenessWatchdog. No behavior change. --------- Co-authored-by: Merge Sim <sim@local> |
||
|
Jinwoo Hong
|
3eec77c11a |
chore(cloud): add the relay fence broker, ops console, Terraform root, scripts, and 24 cloud-* workflows (#18413)
Phase 6 of the relay split: the relay's deploy/operate surface moves under cloud/ with 24 cloud-* workflows gated on ORCA_CLOUD_OPERATIONS_ENABLED, the Cloud SQL rollout lease action, the relay Terraform root (dual-accept identities for both repositories), scripts, docs, CODEOWNERS, and a terraform validate job in Cloud Verify. |
||
|
Neil
|
f37d2fec97 |
fix(linux): land the reviewed Linux packaging stack on main (#18100)
* fix(linux): give the CLI one entrypoint by extracting the AppImage once
* refactor(linux): trim AppImage CLI registration seams
* test(cli): assert registration lock serialization
* fix(linux): fence AppImage terminal shim mounts
* fix(linux): accept extracted AppImage runtimes with APPDIR only
* docs(linux): make headless AppImage extraction runnable
* refactor(linux): import bundled launcher directly
* fix(linux): reclaim superseded AppImage payloads and packaged symlinks
Pruning removed 3215 of 3216 files from a superseded generation and always
stranded resources/app.asar, leaking ~105 MB per version update. Electron's
asar shim reports a *.asar file as a directory, so the recursive remove tried
to rmdir a real file and failed with ENOTEMPTY; the .catch(() => {}) hid it.
Reproduced end to end on Ubuntu 24.04: 519M -> 623M across one update, and
519M again once the payload is actually reclaimed.
removeExtractedAppImagePayload holds process.noAsar for the removal, counted
so overlapping removals cannot hand the shim back early, and the prune site
now warns with the path instead of swallowing the rejection. All three
removal sites use it -- staging cleanup and displaced roots leaked the same
way.
Also reclaim symlinks left by a packaged deb/rpm install, which the
extracted-cache-only rule turned into a hard conflict on a deb -> AppImage
migration, and name the remedy in the conflict error.
* fix(linux): bound the CLI registration lock wait
`retries: 1000` caps the attempt count, not elapsed time, so at up to 1s per
attempt an IPC-driven registration could hang ~16 minutes against a wedged
holder with no feedback.
A legitimate holder is bounded by the extraction timeout, so wait that plus
slack and then fail with a message naming the lock file, rather than hanging.
`maxRetryTime` is forwarded verbatim to the `retry` package by proper-lockfile.
* fix(linux): stop re-extracting the AppImage on inode metadata churn
The extracted-payload cache key hashed ctime alongside dev/ino/size/mtime.
ctime moves on any inode metadata write -- `chmod +x`, which every AppImage
user is told to run, plus `chown`, an ACL or SELinux relabel, and a backup
restore -- none of which alter a byte of the payload.
Measured on Ubuntu 24.04: `chmod +x` leaves dev, ino, size and mtime
identical and moves ctime alone, so the key changed and the next launch paid
a full ~519 MB re-extraction and a multi-second stall to rebuild a payload it
already had, then pruned the old generation.
Key on content identity instead. An in-place content change moves mtime and
almost always size; a replacement moves the inode. The existing
replace-in-place test still passes.
* fix(linux): stop CLI commands from falling through to Chromium startup
* refactor(cli): remove redundant command membership check
* test(cli): cover command-named project selectors
* fix(cli): redirect the open-url command before startup
* test(linux): cover AUR serve wrapper flags
* fix(linux): tighten CLI launch detection
* fix(linux): respect CLI flag value boundaries
* fix(linux): strip injected Chromium switches from CLI args
* fix(linux): report a missing display instead of dying in uv_close
* refactor(linux): read display locks without a preflight race
* fix(linux): preserve unverified external displays
* chore: format reliability gate manifest
* test(packaging): split runtime resource checks
* fix(linux): fail serve when no display is available
* fix(linux): do not treat a lockless X socket as a dead display
An X server writes its lock beside its socket and both survive a crash
(verified against Xvfb under SIGKILL), so a socket with no lock was never
left by a crashed server. It is an endpoint published from elsewhere: a
container bind-mounting only /tmp/.X11-unix, WSLg, or a foreign PID
namespace. Declaring those dead made the desktop gate exit(1) on displays
that work, with no workaround, and the serve gate refuse to start.
Liveness now splits by ownership. A foreign DISPLAY trusts a lockless
socket; Orca's own :99 does not, because removeStaleDisplayArtifacts
unlinks the lock before the socket and so manufactures that state itself --
adopting it would resurrect the orphan-socket bug and stop the cleanup from
self-healing. The stale-lock rejection is unchanged.
Also correct four doc statements this behaviour falsified.
* fix(linux): fail closed when a stale socket blocks the Xvfb rebind
Readiness only checked that /tmp/.X11-unix/X99 exists. A stale socket we
could not unlink still exists after our own Xvfb refused to bind, so Orca set
DISPLAY to a dead server and Chromium died in Ozone init.
Measured on Ubuntu 24.04 against the pre-fix build: with a leftover :99
socket and no lock, serve exits 139 (SIGSEGV), the socket inode is unchanged
before and after, and no lock is recreated -- it neither cleaned up nor
respawned. To a user that is a crash, not a misconfiguration.
This is reachable in the documented topology, where orca-xvfb.service has no
User= and runs as root while serve runs as User=orca: /tmp is sticky, so the
orca uid cannot unlink a root-owned socket, rmSync fails, and Xvfb exits with
the display already active.
Readiness now requires the display to actually be live -- our socket plus a
lock naming a running process -- so the same state reports an unusable
display and exits 1 with the existing diagnosis.
* fix(linux): recognise abstract X sockets and inherited Wayland fds
Two display setups this gate could not prove were refused outright, and on the
desktop path that is app.exit(1) with no workaround.
An X server may bind only the abstract namespace (`@/tmp/.X11-unix/X0`), which
leaves no filesystem socket to stat. Abstract addresses are kernel-owned and
vanish the moment the owner exits, so an entry in /proc/net/unix is proof of a
live server -- no lock file needed and no stale entry possible. Verified on
Ubuntu 24.04, where 139 such addresses were present.
WAYLAND_SOCKET is an already-connected fd handed over by the compositor, so
there is no path to stat and WAYLAND_DISPLAY may be unset entirely. Its
presence is the display.
Both are consulted only after the filesystem-socket check fails, so no
existing verdict changes.
* fix(linux): never treat Orca's own display number as a foreign endpoint
Recognising a lockless X socket as live is correct for an endpoint published
from elsewhere -- a container bind mount, WSLg -- because an X server writes
its lock beside its socket and both survive a crash. It is wrong for
VIRTUAL_DISPLAY_NUMBER, because Orca's own teardown unlinks the lock before
the socket and so manufactures that exact state.
The managed branch was already strict, but a caller that sets DISPLAY=:99
explicitly takes the foreign path and skipped it, accepting a dead display
left by Orca's own interrupted cleanup. Route the managed number through the
strict probe on both paths.
Found by an adversarial audit of the asymmetry introduced earlier in this
branch; the documented systemd topology is unaffected because its Xvfb writes
a real lock.
* test(linux): add a packaged-artifact contract for the CLI launch paths
* test(linux): avoid buffered serve readiness detection
* test(linux): signal AppImage serve owner directly
* test(linux): tolerate readiness timeout boundary
* test(linux): add startup margin to shutdown oracle
* ci(linux): give package contracts timeout headroom
* fix(ci): route all Linux packaging contract changes
* test(linux): poll shutdown readiness without tail leaks
* test(linux): bound shutdown cleanup grace
* test(linux): assert on CLI output, not the harness's own control lines
run-cli-case.sh echoes `RESULT status=N case=<name>`, and the two cases named
*-skills asserted `expectOutput: 'skills'`. That substring was satisfied by
the case name in the harness's own line, so 2 of 8 cases asserted nothing
about the command -- gutting `skills` entirely would still have gone green.
Control lines are now excluded before matching, and both cases assert the
rendered help header, which only real help output produces. Verified on an
Ubuntu 24.04 host: 8/8 still pass against a stack-tip AppImage.
Also register the gate in reliability-gates.jsonc, which #15085 added a CI
Docker gate without. Red/green is recorded from a stock release AppImage
failing 4 of 8, three of them at status 133 (SIGTRAP).
* fix(linux): require static AppImage runtimes (#17319)
* test(linux): reject a wrong-architecture native binary at packaging time
Cross-building the arm64 slice on an x64 host silently packed an x86-64
`pty.node` -- the rebuild logged "Forcing native rebuild for linux-arm64" and
shipped the host's binary anyway. Every gate here inspects symbol versions,
which are perfectly valid on the wrong architecture, so nothing noticed.
Observed on a Raspberry Pi 5: the packaged app loaded, then failed with
"Failed to load native module: pty.node", and the launch contract reported
3 of 8 cases crashed rather than naming the cause. Swapping in the aarch64
`pty.node` took the same build to 8/8.
Compare ELF `e_machine` against the slice being packaged and fail with the
offending path. Checked before the glibc pass, because a wrong-architecture
binary's symbol versions are valid but meaningless and would send the reader
down the wrong path.
Release CI builds arm64 on a native runner, so this guards local and future
cross-builds rather than a shipped artifact.
* test(linux): judge per-arch vendored binaries against their own path
The first CI run of the architecture gate failed the x64 package job on
`@parcel/watcher-linux-arm64-glibc/watcher.node`. That binary is arm64 on
purpose: the package ships every architecture and its loader picks the match,
so its presence in an x64 build is correct.
Judge a binary against the architecture its own path names, falling back to
the slice when the path names none. That keeps the case this gate exists for
-- `bin/linux-arm64-*/node-pty.node` holding an x86-64 binary, which is what
shipped to a Raspberry Pi 5 -- while letting multi-arch dependencies through.
Dry-run over the real dependency tree flags nothing for either target arch.
* fix(linux): move deb/rpm update installation outside Orca (#17318)
* fix(linux): complete deb/rpm package metadata
* fix(linux): preserve CLI link during package upgrades
* docs(linux): document local RPM build prerequisites
* fix(linux): move deb/rpm update installation outside Orca
* fix(updater): preserve Linux recovery across stale events
* fix(updater): fence stale downloaded events by active target
* fix(updater): preserve active Linux package recovery
* test(linux): keep workflow order assertion in scope
* test(updater): assert stale recovery stays silent
* fix(updater): preserve Linux package recovery after checks
* refactor(updater): keep Linux marker message with status
* fix(linux): describe the right manual update path for deb/rpm hosts
A remote host installed from .deb or .rpm now reports
manual-service-update-required, and the guidance told the operator to
"update through the service manager that starts this server" -- which is
correct for unsupported-headless-serve but wrong for a package install,
where nothing about the remedy involves the service manager.
Say both, keyed on how the host was installed.
* docs(linux): document orcad update restart safety
* docs(linux): scope restart census omissions
* docs(linux): use absolute service CLI launcher
* fix(serve): validate in-process serve options before startup (#17683)
* fix(linux): stop offering updates a distro-managed install cannot apply (#17918)
Closes #17702.
The resources/package-type marker is authoritative but never checked against
the host, so any repackager that unpacks Orca's .deb -- AUR, Nix, a container
rebuild -- inherits `deb` verbatim. Install feasibility was then computed
after a ~165 MB download, so those users got check -> download -> a card
promising an install command -> a dead end.
Validate the marker against the host: a deb/rpm marker with no matching
package manager in the trusted directories means a package manager owns this
install. This reuses the exact lists and resolver that
buildLinuxPackageInstallCommand already loops over, so a false positive is
impossible by construction -- any host flagged here would have failed with
no-package-manager after the download anyway. The gate only moves that
verdict earlier. Verified across Debian 12, Ubuntu 24.04, Arch, Fedora 40 and
openSUSE Leap: no false positive on a real deb host, correct on every
repackaging host.
The release is still reported, because the user does want to know 1.4.194
exists and to update through their distro; only the download path is closed.
`externallyManaged` is an additive optional field on the existing `available`
status, so older paired clients decode it unchanged. downloadUpdate() refuses
authoritatively, since main owns this verdict rather than the card, and
unwinds any pinned-build state first -- a Linux pinned jump resolves to
'release', and stranding isPinnedBuildActive would silently kill every
background check for the rest of the process.
Note the fix the issue suggests cannot work: electron-updater builds a
PacmanUpdater whose doDownloadUpdate looks for a .pacman asset Orca does not
publish, then dereferences undefined.
* style(cli): restore prettier wrapping on install error copy
* test(linux): re-pin the child-process ratchets and the batch-shim allowlist after the merge
|
||
|
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>
|
||
|
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 |
||
|
Neil
|
ebe6b559ef | Prime changed native caches before test fanout (#16918) | ||
|
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 |
||
|
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. |