mirror of
https://github.com/stablyai/orca.git
synced 2026-09-21 16:02:20 +00:00
adhoc-build-sort-fix-moved
321
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
e8a7be4ce2 |
fix(omp): recover retired pane status with validated restart authority
Merged after fresh run 35448889017 passed all required checks, including static analysis, typecheck, package jobs, all test shards, changed E2E, Docker SSH E2E, and verify. |
||
|
Neil
|
3ba7cb4de9 |
feat(diagnostics): trace terminal startup delivery phases
Merge fully verified: all required CI checks pass. This lands bounded startup timing instrumentation for the open Windows OMP first-paint investigation in #19333; it does not claim the latency fix itself. |
||
|
Neil
|
b0ec11f5b0 | fix(omp): redact credential references before status transport (#21673) | ||
|
Neil
|
ff934256ae |
fix(omp): retain recorded transcript paths when resuming (#20634)
Based on the resume-locator proposal in stablyai/orca#16276 by @CodeHourra. Retains UUID-based ownership and existing reattach behavior. |
||
|
Neil
|
605a4ef868 |
fix(omp): start new tasks without auto-resuming old sessions (#20622)
* wip(omp): prove fresh settings overlay without redirecting storage * fix(omp): guard fresh launches with execution-host settings * fix(omp): preserve unmodelled shell launch commands * test(omp): consolidate shell fixture path import * preserve fresh OMP launch status * test: cover preserved OMP launch status * fix: recognize wrapped fresh OMP launches * chore(ci): refresh validation against fixed main baseline * fix(omp): recognize generated fresh launch guards across shells * fix(omp): preserve draft status and clear prefill across Unix shells * fix(omp): run cmd draft cleanup after either guard branch * fix(omp): launch drafts safely with nounset enabled * fix(omp): select draft shell without parser diagnostics * test(omp): await relay environment augmentation |
||
 Neilandstevelliu
|
ea02d90704 |
fix(omp): answer startup Kitty queries before renderer handoff (#20620)
* fix(omp): answer startup Kitty queries before renderer handoff Forward actual renderer capability through local and remote spawn. Preserve source ranges and following keyboard mode pushes, and retain independent ConPTY color authority. Refs #17081. Secondary review: #17082. Co-authored-by: stevelliu <stevelliu@tencent.com> * test(omp): cover fragmented keyboard modes and ConPTY handoff * fix: preserve keyboard startup intent without terminal colors * fix: negotiate keyboard support for host-authoritative agent launches * fix: keep terminal creation within line budget * fix(omp): negotiate keyboard support for paired web launches * test: remove obsolete message type import after main integration * fix: validate paired launch results and retry incomplete SSH test snapshots * fix(omp): negotiate keyboard support for background paired launches --------- Co-authored-by: stevelliu <stevelliu@tencent.com> |
||
|
Neil
|
9841f4f790 |
fix(omp): keep child sessions from reporting into the parent pane (#20611)
* fix(omp): fence pane status to the root session manager * test(omp): preserve root preview and recovery through child hooks * test(omp): exercise status ownership through actual runtime runner * fix(omp): honor runtime subagent provenance when available * test(omp): avoid writes to the read-only hook status view * fix(omp): preserve child status ownership provenance * fix(omp): normalize child transcript paths across platforms * fix(omp): clean status handler rebase * fix(omp): keep prefill inside session ownership fence |
||
|
Neil
|
a913f1b457 |
fix(omp): resolve transcripts from active XDG and profile roots (#20633)
* fix(omp): resolve XDG session root fallback * fix(omp): share profile-aware transcript root resolution * test: verify actual OMP XDG transcript discovery and reading * test(omp): satisfy reader smoke loop lint * test(omp): resolve vault fixture paths without redundant cache * fix(omp): refuse normalized Windows drive-relative roots |
||
|
Neil
|
2bf538a4e1 |
fix(runtime): detect a same-size terminal artifact swap the granted stat cannot see (#21436)
* fix(runtime): detect a same-size terminal artifact swap the granted stat cannot see A local terminal-artifact grant pinned the file as `dev:ino:nlink:size:mtimeMs`. On Linux every one of those can survive an unlink+recreate: ext4 reuses the just-freed inode (measured: 100% of the time), nlink and size are unchanged for a same-size replacement, and the mtime clock is tick-quantized to 1ms, so a swap inside one tick produces a byte-identical identity string. The grant then served the attacker's bytes as if nothing had changed. Local grants now also pin a sha256 of the artifact's content, taken from the same handle as the stat so nothing can swap the file between them, and every local read, preview and write re-checks it before returning or committing content. The stat identity string itself is unchanged: the relay recomputes it verbatim to honour `expectedStatIdentity`, so its format is a wire contract. Remote grants keep the stat-only check and are untouched. This is also the mechanism behind the intermittent `orca-runtime-files-terminal-artifact-io.test.ts` failure on `rejects stale absolute terminal artifact previews before returning changed content`: it replaces an 8-byte artifact with 8 different bytes, so whenever the two writes shared a 1ms tick the product genuinely could not tell them apart. * docs(runtime): record what the terminal artifact grant checks do not close The digest makes the same-size swap detectable; it does not make the sequence atomic. A reader arriving at the access module would reasonably assume otherwise, so write down the measured limits of the stat identity, why the identity string cannot change, and the four windows that stay open — the write path's surviving rename() gap above all. |
||
     
|
09073086a8 |
feat(terminal): inline images via @xterm/addon-image (perf-first) (#19512)
* feat(terminal): inline images via @xterm/addon-image, perf-first Add opt-in inline terminal images (SIXEL, iTerm2 IIP, Kitty graphics) through @xterm/addon-image, designed to keep idle terminals unaffected. Performance: - The addon (base64-inlined wasm decoders + protocol handlers) loads off the boot critical path via a deferred loader that mirrors the WebGL addon: primed after first paint only when the setting is on, read back synchronously at attach, with a 3-attempt cap so a transient failure never disables images for the session and a missing chunk never refetches per pane. renderer-boot-graph guards against eager import. - enableSizeReports:false so the addon never sets windowOptions and double-answers Orca's own CSI 14t/16t responder. - Perf-tuned decode/storage limits (storageLimit, sixel/iip/kitty size caps) in one place. Correctness: - Orca's DA1 handler wins over the addon's (last-registered-first), and the default DA1 response never advertised Sixel (;4), so DA1-detecting tools (chafa, img2sixel, viu, timg) never emitted it. The winning handler now appends ;4 while the setting is on, resolved per query so a live toggle changes the next DA1; idempotent against the ConPTY response that already lists it. - ORCA_IMAGE_PROTOCOL=kitty is exported to spawned shells (local, daemon, relay/SSH) and forwarded across the WSL boundary, so image-capable agents can pick an encoder. Unknown image sequences are swallowed by xterm when the addon is detached, so this never garbles output. - Settings toggle (default on) gates rendering and DA1 advertisement. Cross-checked against community PRs #7775, #11706, and #19201 at the end; credited below. Co-authored-by: s546126 <s546126@users.noreply.github.com> Co-authored-by: XRX193 <XRX193@users.noreply.github.com> Co-authored-by: lmsh7 <lmsh7@users.noreply.github.com> * fix(terminal): bound inline image memory and classify Kitty replies * fix(terminal): bound image decode and release image resources on cleanup * fix(terminal): address image addon review feedback * test(terminal): stub setPaneInlineImagesEnabled in appearance manager fakes * fix(terminal): evict unplaced kitty payloads before displayed images Byte-budget eviction dropped the oldest transmitted blob regardless of placement, so a new upload could erase a visible image while abandoned blobs still held budget. Unplaced payloads now go first and displayed ones only when that is not enough. The incoming image is always stored, so an oversized one overshoots the cap by one payload instead of being dropped after the protocol already acked OK. * fix(terminal): gate DA1 Sixel on real addon attachment; claim SSH image spec in CI - DA1 advertised Sixel from the setting alone, so a pane whose lazy addon chunk was still loading (or had failed all three attempts) told feature-detecting tools to emit DCS that nothing could render. Track the attached decoder per terminal and require it before setting the ;4 bit. - tests/e2e/terminal-inline-images-ssh.spec.ts was Docker-gated but claimed by no lane runner, so pr-e2e-gate-contract failed and the spec would have self-skipped green forever. - Reject non-positive PNG IHDR dimensions before decode: they are parsed with signed shifts, so a dimension >= 0x80000000 came back negative and slipped past the pixel-limit comparison. - One resolveTerminalInlineImagesEnabled() for the default-on setting; the four call sites mixed '?? true' with '!== false', which disagree on null. - One readInlineImageResources() walk of the addon internals instead of two copies that could drift against the patched dependency. - Isolate the deferred-attach drain per pane; make the zoom-invariance and backing-storage e2e assertions fail when the feature is dead. * refactor(terminal): one lazy xterm addon loader for webgl and image terminal-image-addon-loader was a structural clone of the webgl one — same memo, attempt cap, and .then(ok,err)-clears-memo recovery. Both now wrap createLazyXtermAddonLoader; each keeps its literal import() specifier so the bundler still splits the chunk (verified against a fresh build: addon-image stays out of the boot graph). * refactor(terminal): name openTerminal's addon flags; pin image addon limits Two adjacent optional booleans could be swapped without a type error once inline images added the second one. * docs(terminal): state the real per-pane image ceiling; drop test ordering dependency storageLimit:32 reads like the pane's budget but keys three pools — decoded pixels, retained encoded Kitty blobs, and pending WASM decoders — so the worst case is ~98 MB per pane with no cross-pane governor. Say so at the constant. pane-inline-images.test.ts's deferred case needed to run first; it now takes a fresh module instead, and the rest prime in beforeAll. Verified by running the file with that test moved last. * fix(terminal): satisfy rebased static analysis gate * fix(terminal): complete casting gate cleanup * fix(terminal): recover failed image addon loads * fix(terminal): bound image decoder allocations --------- Co-authored-by: m4air <m4air@m4airs-MacBook-Air.local> Co-authored-by: s546126 <s546126@users.noreply.github.com> Co-authored-by: XRX193 <XRX193@users.noreply.github.com> Co-authored-by: lmsh7 <lmsh7@users.noreply.github.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> Co-authored-by: Neil <neil@stably.ai> |
||
|
Neil
|
a61119ceb0 |
refactor(runtime): name the four answers a host probe can give (#21207)
The renderer expressed every non-answer as one nullable `status`, so a probe in flight, a probe that failed, a host that refused us and a retired pairing all reached readers as the same `null` -- and readers spent that `null` on decisions of very different weight, including destructive ones. `RuntimeHostContact` names the four. Nothing changes yet: the connection-state derivation is rewritten on top of it and a 384-case parity table asserts the result is identical to a frozen copy of the old one on every combination of verification, transport, retired, answered and remote-control state. |
||
|
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. |
||
|
Neil
|
6c3b97b950 |
fix(mobile): a scope refusal is not a missing method on the Relay pairing probes (#19952)
* fix(mobile): a scope refusal is not a missing method on the Relay pairing probes
The desktop's mobile allowlist gate runs before its RPC dispatcher, so a method an
older desktop predates is absent from both and the phone is answered `forbidden`,
never `method_not_found`. Keying the "too old for Relay, stay on LAN" fallback on
`method_not_found` alone therefore never fired against the exact desktop it exists
for: first-time pairing threw instead of committing a LAN host.
`isPairingRelayRpcUnavailable` accepts both codes at the three pairing probe sites.
It is pairing-scoped on purpose - `isMethodNotFoundRefusal` has four other consumers
that must keep reading `forbidden` as a refusal, not as absence.
The main-side test pins the claim the fallback rests on: the dispatcher really does
answer `forbidden` to a mobile-scoped device and `method_not_found` to a runtime one,
and this build allowlists both probes, so `forbidden` on either can only mean an
older desktop.
* fix(mobile): leave a breadcrumb when a desktop refuses relay pairing
The LAN fallback now commits a host instead of throwing, so the refusal code
was the only record of why a phone ended up without a relay endpoint and
nothing wrote it down. Log it on the path that swallows it.
Narrow `isPairingRelayRpcUnavailable` to the two codes it matches rather than
to `RpcFailure`: a plain failure guard would collapse the *false* branch to
`RpcSuccess`, which a refusal carrying any other code still reaches.
Rename the `'method-not-found'` sentinel in the direct-upgrade reader, which
stopped describing what it covers, and correct two comments that named a
`method_not_found` mechanism the desktop cannot produce for these methods:
both probes have been allowlisted and registered by the same commit since
Relay landed, and an unwired pairing provider answers `runtime_error`.
* docs(wire): record that the mobile surface refuses by scope, not by absence
Two comments cited this page for "a scope refusal is not a missing method" and
the page did not say it — the only nearby statement says the opposite, because
it describes the runtime-scoped surface, where the dispatcher does answer
`method_not_found`. The allowlist gate makes the mobile surface the exception,
and the harness does not run that surface, so this note is the only record.
* docs(mobile): name the pairing site the scope refusal actually reached
The comments and the wire-compat note said this fixed first-time QR pairing.
It cannot: the `relay` block on the pairing offer, both RPC handlers and both
allowlist entries all landed in
|
||
 Jinwoo Hong
|
a28085adbf |
refactor(mobile): checked reply readers for the source-control domain (step 7 pilot) (#20950)
* test(mobile): ratchet the 201 unchecked RPC reply readers Step 4 moved every call-site cast into an RpcOperation's `read`, but 201 of those readers still answer `compatible: true` for any payload: `rpcUncheckedPayloadReader` (163), `rpcReadUnchecked` (26 outside its own module) and `rpcUncheckedMemberReader` (12), across 42 files. The cast moved; it did not become true. Held as data with an AST boundary test, shaped on the raw-request-port ratchet: a file that is not listed fails, a listed file that no longer has one fails, and a count that rises fails. Only a call counts, so an import is not a reader and prose never is. No behaviour change: this commit adds a list and a test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * feat(mobile): validate the source-control domain's RPC replies at arrival Replaces all 17 unchecked readers in mobile/src/source-control/ with `rpcResultVariant(variant, schema)`, so a malformed reply is an `RpcIncompatibleReplyError` naming the operation instead of a TypeError three frames downstream. The inventory drops 201 -> 184 and the five source-control operations files leave it entirely. This is a behaviour change, scoped to malformed replies. Six reply-matrix goldens move; every named-scenario golden and every `normal` partition is byte-identical, which is the parity claim. Schemas live one module per reply domain, beside the operations that read them: git-status, git-compare, git-history, hosted-review and worktree-metadata. A member is required only where a consumer reads it unguarded, and each schema records the consumer line that justifies it. Nothing is `.strict()`; every reply a consumer publishes verbatim keeps `z.looseObject` so an undeclared host member still passes through. Six replies have no reader anywhere in mobile and get `z.unknown()`, which is the honest schema for them, not a holdout. Three readers stay total by construction, because their contract is that an unreadable reply is a value rather than an error: the `git.status` projection (a null status three screens route on), the `session.tabs.list` reveal (a null list means poll again) and the generated commit message (a screen's copy, never a decode error in a text field). They gain the salvage report, not a verdict. Consumers take the schema's output type, so `MobileGitStatusResult` and the branch-compare aliases now name what mobile reads rather than the desktop aggregate, and seven call-site casts are gone. Three requirements came from the goldens, not from the host types: `git.history` sends `timestamp: null`, `hostedReview.getCreationEligibility` sends a `reviewLookupOutcome` the shared union does not list, and the `git.status` projection writes an absent member as a present `undefined`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the six source-control reply-matrix goldens step 7 moves Six goldens, all on malformed partitions. Every named-scenario golden and every `normal` partition is unchanged, which is the parity claim for this step. git.history-read / git.history#1 result-absent, result-null, inner-ok-missing, inner-false-string-error, inner-false-object-error: the load rejected with a TypeError reading 'items' or 'map' off undefined/null; it now rejects with `incompatible_reply: git.history-page (git.history)`. hostedReview.eligibility + create-intent / hostedReview.getCreationEligibility result-absent, result-null, inner-ok-*: the fetch fulfilled with the error envelope itself, re-typed as an eligibility and published into the compose prefill; it now rejects, and both callers already route that to the same "eligibility unavailable" state a null answer produced. hostedReview.create-chain + create-intent / hostedReview.create result-absent, result-null, inner-ok-missing, inner-false-object-error: the create form showed the raw TypeError text "Cannot read properties of undefined (reading 'ok')"; it now shows the incompatible-reply message. Every header digest is unchanged -- baseline, recorder, adapter, scenario and lockfile all match -- so the diff is the behaviour and nothing else. Recorded from this branch into a scratch directory and copied in, because there is no scoped honest alternative: scripts/rpc-recording.mts refuses to run unless the product tree equals the pinned baseline, and the README's remedy for an intended behaviour change is to repin, which rewrites the `baseline` header of all 667 goldens. So these six now carry a pin whose tree no longer produces them. That is a real gap in the oracle's design for behaviour changes, not a detail of this step, and it needs a decision before this lands. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): pin the four reply-schema properties the goldens found Each of these cost a reply-matrix golden while writing the source-control schemas, and none of them follows from reading the consumers or the host types: a newer host's undeclared members must still decode, `git.history` sends `timestamp: null`, `hostedReview.getCreationEligibility` sends a `reviewLookupOutcome` the shared union does not list, and the `git.status` projection writes an absent member as a present `undefined`. The `.strict()` case is the one worth stating twice: at the top level it rejects the reply, and on the entry it drops the row, which shows a dirty worktree an empty Changes list. The fifth test pins the salvage report that makes such a drop visible instead of silent. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): give an unreadable reply a message a user can read `RpcIncompatibleReplyError` put `incompatible_reply: <op> (<method>)` in `message`, and `message` is what the screens hand to a toast. Step 7 is the first change that can reach this error at all, so the token would have shipped to users as its own error copy. Fixed at the boundary rather than per site: `message` is now plain copy, and the machine token moved to `code` (`incompatible_reply`) and `name` (`RpcIncompatibleReplyError`), both readable by callers. The cross-bundle fallback in `isRpcIncompatibleReplyError` matched on the old message prefix, so it now matches on `name`, which a foreign copy of the module still carries. No existing test pinned the old text. Two new ones pin the copy, the token and the foreign-copy match. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the recording baseline to this branch and re-record Commit |
||
|
Jinwoo Hong
|
b997fcc77a |
fix(session-search): try phrase and AND routes for prose queries before OR (#20754)
* fix(session-search): try phrase and AND routes for prose queries before OR An exact sentence pasted out of a transcript was not returned. The route ladder only ran the phrase and AND rungs for a literal-looking query, so prose fell straight to OR, where the sentence's common words filled the candidate limit with recent sessions and the old session holding the sentence never reached ranking. The planner now carries a `phrase` candidate: the query's tokens in order with stop words kept, which is what the sentence is actually indexed as. The ladder runs phrase then AND over those tokens for every query of two or more tokens. A one-token query still takes the rung only when it looked literal. `incomplete` is reported by the rung that answered rather than accumulated across every rung tried. * fix(session-search): mark a snippet with the route that retrieved it A phrase hit was highlighted with the OR expression over the stop-word stripped terms, so an exact sentence rendered as scattered bold words with its stop words plain. The snippet now uses the expression the route matched by: one run for a phrase, every typed word for AND, the terms for OR. * fix(session-search): repair a prose phrase without dropping its stop words Typo repair re-planned the query from `plan.body`, which prose has already had its stop words removed from. `relay is droppng frames` therefore came back as the plan for `relay dropping frames`, and the phrase rung searched for a sentence nobody wrote: the transcript holds `relay is dropping frames`, so the exact match fell through to AND. The repair now maps over `plan.phrase`, the tokens as typed, and re-plans from those. Only terms the body holds are offered to the corrector, so a stop word is still never repaired, and the re-plan recomputes the body from the corrected sentence exactly as before. |
||
|
Jinwoo Hong
|
3e5eb0329a |
feat(cli): orca search over the agent session index (#20514)
* feat(cli): orca search over the agent session index `orca search <query>` calls PR 5's `aiVault.searchSessions` over the CLI's existing runtime RPC, against the host `--environment` / `--pairing-code` selects and no other. `orca search --index-status` calls `aiVault.searchStatus`. It is the proof the contract works with no panel. Every flag maps onto a contract field and nothing else: `--scope`, `--fresh`, `--limit`, `--cursor`, repeatable `--agent` and `--path`, `--since`, `--sort`, `--debug`, `--json`. No fan-out, no merged output, no `--host`. One command rather than a `search status` subcommand: the query is a bare positional, so `orca search status` could not be told apart from searching for the word "status". `--status` is unavailable because `orchestration task-list --status <state>` already owns the name as a valued flag. No new runtime capability. PR 5 decided an explicit `method_not_found` refusal maps to `unavailable/no-service`, so reusing `createSessionSearchClient` gives an old host a plain "this host runs no session search service" answer at exit 0 instead of a raw JSON-RPC error. `CommandSpec.repeatableFlags` scopes repeatability per command, because `--agent` must repeat for search and stay single-valued for `worktree create`. `help.ts` sat exactly at max-lines, so `skills-command-flag-help.ts` becomes `command-scoped-flag-help.ts` carrying both tables at the same call-site size. * refactor(cli): drop the search type assertions main's casting gate now rejects Main gained a `consistent-type-assertions: never` scan in the changed-code gate after this branch was cut, and it reported twelve assertions in the new files. The four in the argument parser were avoidable. `readEnum` now keeps the value `find` returns, which already carries the narrow type, and the agent filter goes through an `isAiVaultAgent` predicate over a `Set<string>` instead of widening the agent tuple. The test now narrows the printed envelope by shape and re-reads the printed result through `AiVaultSearchResponseSchema`, so the JSON assertions are checked rather than claimed, and the flag table is typed so its callback needs no cast. One assertion is left, for the structural fake client, with the SAFETY rationale AGENTS.md requires. * fix(cli): sanitize host strings and scope pre-command repeatable flags Route every host-supplied string the search formatter prints through the escape stripper, and resolve the repeatable-flag set from the command tokens ahead when a flag sits before the command. * refactor(cli): resolve repeatable flag rules once per command * fix(cli): clarify session search availability and SSH scope * feat(cli): hide orca search until the settings toggle ships `orca search` stays dispatchable but leaves every discovery surface: root help, group help, unknown-command suggestions, and `agent-context --json`. `buildAgentContext` did not filter hidden specs, so it also stops leaking the hidden `terminal stop`. |
||
|
Jinwoo Hong
|
dec0e2cd56 |
feat(session-history): add local search settings and index controls (#20582)
* feat(session-history): add local search settings and index controls * Use shared local host identifier for session index status * feat(settings): live index status, enable confirm, advanced delete * fix(settings): let Button and Collapsible own their spacing and type |
||
 Brennan Benson
|
291b4ddd6f |
feat(agent-status): route structured sessions through canonical ownership (#20718)
* feat(agent-status): route structured status through canonical ownership and fence child lifetimes Restacked onto the canonical store and child-work contract. Completing that restack drops the `reopenStructuredParent` mutation flag this change had carried, along with its contract field, its codec branch, and its single call site in structured ingest, which passed a hardcoded `true`. The flag was a narrow escape hatch from the absolute `tombstones.has(...)` rule that governed parent upserts in this branch's original base. The canonical store replaces that rule with a revision envelope, because a bounded store compacts tombstones away and a presence-based guard silently stops fencing once one is evicted. With the envelope deciding the outcome, the escape hatch has nothing left to escape from, so removing it changes no production behaviour. `agent-status-store-reopen.test.ts` is rewritten against the envelope: the reopen case now pins that an unflagged republication succeeds while replay from before the reopen stays fenced even after the parent tombstone is compacted away, and the second case pins where the guard genuinely bites — a republication inside the removing mutation itself, for every subject kind. * fix(agent-status): re-admit unchanged structured owners after teardown * fix(agent-status): clear anti-slop object-param and Reflect.apply findings - agent-status-store-byte-budget.ts: type the byte-budget helper's record parameter as the union of what its call sites actually pass (the snapshot header plus each store entity record) instead of the broad `object`. - server-structured-canonical-status.test.ts: replace `Reflect.apply` with a typed, explicitly-bound call that models a caller at an untyped boundary omitting the trusted owner subject. * docs(agent-status): drop the 2A progress doc from docs/reference docs/reference/ holds implementation detail, not rollout progress. The canonical-boundary notes move to the effort's working directory; the agent-status-store status section keeps the boundary statement and loses the now-dangling link. * fix(agent-status): mint the canonical epoch on first use, not at construction The hook server's canonical store was built in an instance-member initializer, so constructing AgentHookServer — which happens at import time for the module singleton — demanded a live randomUUID. Any importer that stubs node:crypto threw 'Invalid agent status store epoch' before a single test ran. The store is now created on first canonical access and reset by dropping it, so construction owes nothing to a crypto implementation and the epoch still rotates per authority incarnation. * fix(agent-status): drop the orphaned snapshot budget and a duplicated pane guard Two leftovers from the canonical-store routing change: agent-status-store-snapshot-budget.ts lost its only caller when the store state switched to agentStatusStoreFitsByteBudget. Nothing in the repo imports it now, so the module goes with the caller it existed for. The replacement is not a straight copy: it only memoises a record's measured size once the record is frozen, so a still-mutable record can no longer return a stale byte count. persistedStructuredWorkerPaneKeyIsValid repeated its public-pane-key rejection verbatim three lines below the first one. The tests covering that rejection pass on the first occurrence alone, so the second decided nothing and only obscured which predicate was load-bearing. * fix(agent-status): stop a failed structured publish from latching as owned Three defects found reviewing the structured routing path. combinedStatusEntries defaulted a missing listing order to 0, but the counter it compares against starts at 1, so any unordered row sorted above every ordered one. Unknown order now sorts last. The owner map recorded a session as owned before the sink ran. A publish that threw therefore left matchesLocation reporting an owned location for a row that was never written, and the unchanged-projection path — the only thing that would re-offer it — stopped. The address still has to survive a throw so teardown can forget a row that did land, so the two facts are now separate: the address is recorded up front, and only a publish that returned marks the row as landed. The reopen test claimed the revision envelope rather than the tombstone fences a stale replay. It cannot tell: transport consecutiveness, the parent-revision validator and the tombstone guard each refuse that replay alone, and ablating any two leaves the test green. It now asserts the outcome and says so. |
||
|
Neil
|
13ba649c22 |
fix(terminal): let a runtime-created Windows terminal BE the requested shell (#20825)
* fix(terminal): let a runtime-created Windows terminal BE the requested shell
`orca terminal create --environment <windows-host> --command 'cmd.exe'` never
created a cmd terminal. `--command` is text the provider TYPES into whatever
shell it spawned, so the PTY stayed the host's default shell with cmd running
inside it. Captured on `awin`, whose default is Git Bash:
$ orca terminal create --environment awin --command 'cmd.exe' --json
$ orca terminal send --environment awin --terminal term_10656cf7... \
--text exit --enter
$ orca terminal read --environment awin --terminal term_10656cf7... --screen
neil@awin MINGW64 ~/orca/orca ((30f820708f...))
$ cmd.exe
Microsoft Windows [Version 10.0.26200.9445]
C:\Users\neil\orca\orca>exit
neil@awin MINGW64 ~/orca/orca ((30f820708f...))
$
The handle is alive the whole time and `terminal list` shows one healthy
terminal, because the PTY never changed — so the only symptom is that the
caller's terminal is now a shell it never asked for, and every later `send` is
quoted for the wrong one. On `win-lowspec` (default pwsh) the same create lands
cmd inside PowerShell.
Root cause
----------
There are two spawn preflights and they are twins:
- `src/main/ipc/pty/ipc/spawn-preflight.ts` — renderer/IPC spawns, i.e. a
terminal tab opened in the app.
- `src/main/ipc/pty/runtime/spawn-preflight.ts` — runtime spawns: the CLI's
`terminal.create`, headless `orca serve`, and every paired remote
environment.
Only the IPC twin read the caller's requested shell. The runtime twin passed a
literal `requestedShellOverride: undefined`, so a runtime-created terminal on
Windows could only ever be the host default. Everything downstream of that
point — `spawn-options`, the daemon, `resolvePtyShellOverride` in the relay,
`local-pty-launch-plan` — already honoured `shellOverride`; nothing upstream
could supply one.
Change
------
- Thread `shellOverride` through the runtime lane: `RuntimePtySpawnArgs` ->
runtime `spawn-preflight` -> `RuntimePtyController.spawn` ->
`TerminalCreateOptions` -> the `terminal.create` RPC's new `shell` param ->
`orca terminal create --shell`.
- Thread it through the renderer-backed lane too (`createDesktopTerminal` ->
`terminal:requestTabCreate` -> `store.createTab`), so `--shell --focus` is not
silently dropped on a local Windows app.
- An agent launch quotes its startup command for the shell it will actually run
in, so a requested shell now owns the startup-shell family instead of the
global `terminalWindowsShell` setting.
- Lift the relay's `ALLOWED_WINDOWS_SHELL_OVERRIDES` into
`isSupportedWindowsShellOverride` in `src/shared/windows-terminal-shell.ts`
(membership unchanged) so the CLI, the zod param schema, and the relay refuse
the same names. `--shell` therefore cannot carry a path or a command line into
`pty.spawn`; only allowlisted bare shell names pass.
- Gate on `TERMINAL_CREATE_SHELL_SELECTION_RUNTIME_CAPABILITY`. An older host
strips the unknown `shell` param and answers with a healthy terminal running
its default shell — a reply indistinguishable from success — so the CLI
refuses before creating anything rather than creating the wrong shell quietly.
`--shell` stays Windows-only; macOS and Linux hosts spawn the login shell and
the relay drops the value off win32 rather than honouring it half-way. A WSL
project runtime still outranks it, unchanged.
Tests
-----
- `pty-spawn-shell-override-parity.test.ts` pins both preflights against the
exact drift that caused this (verified failing with the fix reverted).
- `createTerminal` passes `shellOverride` to `ptyController.spawn` with no
startup command.
- CLI: sends `shell`, refuses a shell the host cannot spawn, and refuses a host
without the capability — in both refusals without making the round trip.
- Allowlist and `terminal.create` schema accept/refuse cases, including paths
and appended arguments.
* fix(terminal): refuse a requested shell the execution host cannot apply
The first commit made `--shell` reach the spawn, but only a LOCAL win32
execution host applies it: `spawn-options` gates the override on
`process.platform === 'win32' && !args.connectionId`. So `--shell cmd.exe`
against an SSH-routed worktree, or against a macOS/Linux host, still returned a
healthy terminal running that host's default shell — the same
indistinguishable-from-success reply the capability gate exists to prevent, one
layer down.
Refuse instead, before anything spawns. The check sits at the top of
`resolveAgentTerminalCreateOptions`, which every create lane funnels through, so
neither lane has to remember it; the desktop lane additionally refuses a
worktree-less create, which has no execution host to resolve a shell on.
An SSH host's platform and installed shells are not visible to this runtime, and
a POSIX host has no Windows shell to pick. Neither can honour the request, and
saying so is the whole point of the flag.
Docs and the CLI spec now say "refused", not "ignored".
* fix(terminal): refuse a shell that contradicts the project execution runtime
`resolveLocalWindowsTerminalRuntimeOptions` does not merely rank the project's
execution runtime above a per-terminal pick -- it REWRITES the pick, in both
directions, and says nothing:
- a WSL project forces `wsl.exe`, discarding `--shell cmd.exe`;
- a Windows-host project discards a WSL name and falls back to `COMSPEC`
(`getHostShellForProjectRuntime`), so `--shell wsl.exe` spawns cmd. That is
the common case, not an edge: `resolveProjectExecutionRuntime` resolves
`windows-host` for every project that is not WSL, while a repo belonging to no
project honours `wsl.exe` -- so the same flag behaved differently depending on
whether the repo was in a project.
Either rewrite returns a healthy terminal running a shell the caller did not ask
for, which is the failure `--shell` exists to remove.
It also split an agent launch's quoting from the shell that receives it. The
previous commit made the startup-shell family follow the REQUESTED shell, so
`--shell wsl.exe --command codex` on a Windows-host project typed POSIX-quoted
launch args into cmd. Refusing the contradiction removes that case rather than
papering over it.
Refuse instead, alongside the SSH and non-Windows refusals, from the same
`resolveAgentTerminalCreateOptions` seam every create lane funnels through.
Also from review:
- the allowlist test looped the list against itself; spell the members out.
- the runtime spec case claimed to prove the pty's shell when it asserts the
controller received the field; name it for what it checks.
Reported by an adversarial review of the branch.
* fix(terminal): canonicalize --shell and refuse a WSL-path rewrite
Review of the --shell create path turned up two ways the terminal could
still end up being a shell the caller never asked for -- the exact failure
--shell exists to remove.
Bare and mixed-case spellings passed the allowlist but reached consumers
that exact-match the canonical name: resolveWindowsShellStartupFamily
classified `cmd` as the PowerShell family, resolveWindowsShellLaunchArgs
fell through to empty shellArgs (no `chcp 65001`, no OSC 133 bootstrap that
Windows foreground status depends on), and resolveWindowsGitBashShellPath
compares case-sensitively so `Git-Bash` spawned a literal `Git-Bash`.
The allowlist is now one canonical-name map and terminal.create canonicalizes
on parse, so the spawn path only ever sees `.exe` spellings. `pwsh` and
`powershell` stay distinct binaries.
A `\\wsl$\<distro>\...` cwd made the providers force wsl.exe regardless of
the request, and terminalShellOverrideRefusal only inspected the project
runtime -- undefined for a folder workspace with no project. Refuse on the
resolved cwd and the workspace path, judging what the PTY actually gets.
Also: the capability gate reported an unreachable host as too old rather
than unavailable; the SSH CLI shim dropped capabilities from status, so
--shell there blamed the host version instead of naming SSH; and --shell
had no help entry, rendering bare in `orca terminal create --help`. Adding
that entry crossed help.ts's max-lines cap, so the flag table moved to
flag-help-text.ts rather than suppressing the rule.
Adds a behavioural test for the runtime preflight (the one-line fix was
pinned only by a source-text scan), plus coverage for the startup-command
quoting family, the no-workspace refusal, and the WSL-path refusal.
* fix(build): keep tests out of the RPC params catalog bundle
The catalog walk under methods/ already skips *.test.ts, but the contract
directory glob took every .ts. terminal-create-shell-param.test.ts is the
first test to live there, so the bundle pulled vitest into a CJS build and
the generator threw on require(). Same exclusion, same reason.
|
||
|
Neil
|
20794ee785 |
ci: keep the baseline build off the compatibility matrix lanes (#20733)
The compatibility gate started the pinned 2.25.5 source build inside the same step that runs the three measured lanes, so `make -j$(nproc)` competed with two container lanes whose wall clock is container starts, not Git. A boundary case that costs ~1.5s stretched past Vitest's 30s timeout and failed the job. Build the binary in its own step before the matrix, and pull both images before any lane starts so a lazy pull cannot stall whichever test its sibling is timing. |
||
|
Neil
|
41e42beab4 |
fix(worktrees): safely remove prunable git-file registrations (#20617)
Preserve checkout files and the named branch when removing a positively attested malformed Git-file registration. Reject file/symlink targets in deferred directory deletion. Verified exact head with 75 focused tests including actual Git malformation, preserved marker/file bytes and branch HEAD. Independent review and complete product CI passed. WSL routing is covered by unit tests; direct SSH fails safely without local recovery. Fixes #17316 |
||
|
NeilandBrennan Benson
|
389d672dab |
fix(omp): preserve saved conversation names in session history (#20636)
Co-authored-by: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> |
||
|
Jinwoo Hong
|
c853e10e0c |
fix(rpc): validate provider-specific fields in TaskProviderIdentity (#20284)
* fix(rpc): validate task provider identity fields Validate provider-specific field types while preserving nullable scopes and unknown identity fields. Record the producer census and pin validation with regression tests. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(rpc): reject a blank GitHub owner or repo normalizeTaskProviderIdentity treats a blank owner or repo as no identity at all, but the schema accepted '' and whitespace-only, so the two disagreed about the same payload. Refined rather than trimmed: trimming would rewrite the parsed value and change what the handler receives. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(rpc): re-measure the identity evidence counts The blank-field commit added seven tests, so the recorded 74/16/58 described the commit before it. Re-ran both: 81 tests, and the discriminant-only mutation now gives 17 failures / 64 passes. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(rpc): correct the remaining stale gate count The blank-field commit moved the full-RPC total too; 2,463 was the count before it. Re-ran: 278 files, 2,470 passed, one skipped. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Jinwoo Hong
|
22f56f7c2a |
fix(runtime): reject malformed file Base64 padding (#20283)
Require padded file-write payloads to end on a Base64 quartet boundary. Cover both RPC methods and padded final upload chunks, and document client compatibility evidence. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Jinwoo Hong
|
1f7655f3e3 |
feat(ai-vault-search): public session search contract and transports (#20277)
* feat(ai-vault-search): define public contract and service seam * feat(ai-vault-search): add IPC runtime relay and web transports * fix(ai-vault-search): register search IPC at the core handler site ai-vault.ts was two lines over the 300-line max-lines limit; the search handlers belong with the other register*Handlers calls anyway. * fix(ai-vault-search): withhold degraded-root paths from relay status Status carried local filesystem paths over the relay while hits redact theirs. redactStatusForTransport applies the same policy at the same boundary: relay callers keep each root's reason and the array length as the count, so the type only makes root optional. * fix(ai-vault-search): close diagnostic path leak and remove test casts * feat(ai-vault-search): carry an execution host id and per-host outcomes on hits * feat(ai-vault-search): route desktop search by execution host scope, including runtimes * feat(preload): accept an execution host scope on session search * feat(web): answer only for the paired runtime on session search * docs(ai-vault-search): describe execution-host routing and the all-hosts merge * test(ai-vault-search): cover every host scope, the all-hosts merge and wire compat * fix(ai-vault-search): resume every host mid-page so a merged page never drops a hit * fix(ai-vault-search): decode the merged cursor with a schema instead of casts CI's type-aware audit refuses type assertions; a zod record validates the per-host entries and yields the typed map without one. * refactor(ai-vault-search): defer cross-host merged search |
||
|
Neil
|
e86cba888b |
build: reduce native dependency installs to the host platform (#20420)
* Reduce native dependency installs to the host platform * Remove install policy documentation * Guard cross-arch packaging and scope release installs to the runner electron-builder only logs a warning for a missing extraResources source, so a host-only install silently shipped a foreign-arch slice without its natives — `pnpm build:mac` on Apple Silicon produced an x64 DMG with no sherpa-onnx-darwin-x64 and no @parcel/watcher-darwin-x64. The previous beforePack hook covered only win32. - Add assertPackagedNativeVariantsInstalled, an arch-aware check over the target's sherpa-onnx, @parcel/watcher, and (on Windows) node-gyp addons. beforePack now runs it for every platform, with remedies split: another architecture comes from install:release, the os:win32 addons need a Windows host. - Drop --os from the release installs. Every packaging job already runs on a runner whose OS matches its target, so only the macOS lanes need extra breadth, and only on CPU for their x64+arm64 config. Windows and Linux packaging return to a plain host-only install. - Add --frozen-lockfile to install:release so a bare run cannot rewrite the lockfile. - Restore the install policy reference doc and the CONTRIBUTING note, plus the rationale comments dropped from the runtime contract test. - Gate the packaging-closure assertions on whether the Windows addons are installed rather than on the host OS, so a cross-arch install exercises them off Windows too. - Make the workflow contract test read `run:` steps as well as retry-action commands, and enforce host-only scoping on the non-macOS packaging lanes. - Remove the unreferenced install measurement script; its numbers live in the policy doc. * Track the install policy doc and index it from AGENTS.md docs/** is ignored behind a per-file allow-list, so the new reference doc was only committed via git add -f and future edits would be skipped. Add it to the allow-list and give it an AGENTS.md entry like every other tracked reference doc, so the host-only install rule is discoverable before someone packages a second architecture. * Route Windows-lane removals through the retrying helper Adding these four specs to the PR Windows lane pulled them into the windows-lane-tree-removal-boundary ratchet, which failed on 20 raw recursive removals. On Windows a bare rmSync races a handle the OS has not released, throwing EPERM after the assertions already passed and reporting a green test as a lane failure. * Adapt the packaging guard to the vendored Windows registry addon main vendored windows-native-registry as the workspace package @orca/windows-registry (#20438). A workspace link resolves on every host, so including it in the installed-Windows-addons checks proved nothing. @vscode/windows-process-tree is the only os: win32 npm addon left, so it alone decides whether the win32 resource plan resolves. |
||
|
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
|
56fcb544e0 |
fix(browser): move cookie scoping off psl's stale suffix list (#20421)
* fix(browser): move cookie scoping off psl's stale suffix list psl@1.15.0 is its latest release and ships a Dec-2024 snapshot of the public suffix list. Measured against the current upstream list, it fails to recognise 600 of 10,030 suffixes; tldts misses 2. That gap is a cookie-isolation bug. psl does not know `api.br` is a suffix, so it falls back to the `br` rule and maps foo.api.br, bar.api.br and example.api.br all onto the single family `api.br`. Unrelated registrants then share a removal scope, and a replace-mode import for one clears the others' cookies. The same holds for seg.ar, co.az, gov.cz and ~597 more. tldts is called with allowPrivateDomains, without which the PSL's PRIVATE section is ignored and every *.github.io / *.s3.amazonaws.com / *.vercel.app tenant collapses into one family — 21 of 49 probed hosts changed family under the default. The new test pins that boundary. One deliberate behaviour change: hosts under `.local` (not in the PSL) were their own family under psl, which returned an all-null parse for them; they now resolve to the two-label boundary (app.orca.local -> orca.local), matching what Chromium treats as the registrable domain. * fix(build): bundle tldts into the main process like psl was psl sat in BUNDLED_MAIN_DEPENDENCIES, so it was inlined into the main bundle rather than externalized and copied into resources/node_modules. Swapping the dependency without moving that entry left a bare tldts import that afterPack's runtime-closure check rejects. * fix(build): point the output contract at tldts and drop the psl shim The contract test still asserted psl was in BUNDLED_MAIN_DEPENDENCIES, so it failed once the entry became tldts. src/types/psl.ts declared a module that no longer resolves; tldts ships its own types. * test(browser): pin the suffix boundaries the tldts swap moved Three semantic changes shipped untested: - `.local` is unlisted, and the libraries disagreed on what that means. psl returned an all-null parse so every `*.orca.local` host was its own family; tldts stops at `orca.local`. The consequence is wider than the family name — importDomainAncestors now yields the shared parent, so a replace-mode import of one host clears non-host-only cookies every sibling shares. - psl's snapshot had `compute.amazonaws.com` as a literal PRIVATE suffix; the current list only carries the wildcard, so the bare host is ICANN now. - The renderer's `psl.isValid` gate had no direct test at all — nothing imported the module from a test. Also drops comments that explained a boundary in terms of psl's internals. One was wrong under tldts: bracketed IPv6 does not reach an error branch, it parses with the brackets stripped and falls through the unlisted path. |
||
|
Jinwoo Hong
|
fa3d29fa11 |
feat(ai-vault-search): session search query engine over the index (#20029)
* feat(ai-vault-search): give the index a generation a page cursor can be fenced by
A search page is a slice of one ranked list, so a cursor only means anything
against the snapshot that produced it. The store now keeps a monotone
generation in `meta`, bumped by every mutation that can change which rows a
read returns, and starts a new one on open so a write whose bump never landed
cannot leave a cursor pointing at content that is already gone.
Schema version 2 adds the two tables the query layer needs: `messages_vocab`
(fts5vocab over messages_fts, the typo repair's whole dictionary) and
`search_log`. Both are pure additions and the index is a cache, so a version-1
file is dropped and rebuilt exactly as any other mismatch is.
* feat(ai-vault-search): plan a query the way the index tokenized it
The planner unfolds the tokenizer contract instead of asking SQLite: same
boundaries as `unicode61 tokenchars '_.-/+'`, pinned against real fts5vocab
output so a query can be planned without a round trip. It decides the route
ladder's first rung (literal shape), strips stop words from prose but never
from a literal, and fans an identifier out into its pieces for the OR fallback.
Typo repair uses the index's own vocabulary as its dictionary, so it can never
suggest a term the index does not hold. Its exact-match probe now joins
`visible_messages`: `messages_vocab` is a view over the FTS b-tree and still
lists staged and tombstoned terms, and the PR 2 read ratchet is right to
demand the join.
`splitAiVaultSearchQuery` is the index's reading of repo: / path:. It keeps
operator case, which the panel folds and cwd_key must not; a census test pins
the two parsers to the same answer about what is an operator until PR 7 moves
the panel onto this one.
* feat(ai-vault-search): narrow a search the way the sidebar keys a folder
Every caller-supplied narrowing in one place, so retrieval, the operator-only
page and the session load cannot drift apart: agents, an updated-at floor, the
retention cutoff, scope paths, and the repo: / path: operators.
Scope keying goes through PR 2's `cwdKey`, which is the sidebar's
`folderGroupKey` without its prefix, rather than the original branch's second
spelling. That drops the branch's WSL distro qualification, which PR 2 removed
on purpose, and it makes the filesystem root a key that already ends in a
separator, so the child-prefix range is built from the key rather than by
appending one; `//` sorts below every real child and would scope the root to
nothing.
Engine types land here too, under src/main and not src/shared: nothing in this
PR is a wire type, and PR 5 lifts what a caller may receive.
* feat(ai-vault-search): rank, page and answer a session search
`SessionSearchEngine.search()` over the PR 2 store: route ladder (phrase, AND,
typo repair, OR), BM25 weights per corpus, one hit per session, fork folding,
and a page.
- `scope` picks the corpus and the engine never second-guesses it.
`conversation` is user and assistant turns; `all` adds tool output and the
identifier shadow column. Switching corpus while typing is PR 7's policy; an
engine that widened on a miss would make a result impossible to reproduce
from its own request.
- Pagination is an offset into one ranked list, fenced by the index generation
and by a hash of everything that changes the ranking. A cursor from another
generation or another query is refused with a typed error rather than
silently re-run. Ranking breaks every tie by session id, because a cursor
indexes into that order and retrieval does not promise one.
- Snippets and source presence are paid for by the page, not the list. A
snippet past the per-hit ceiling is cut on a code point, never between `[[`
and its `]]`, and flagged on the hit.
- Source presence is read from the `files` table. No stat on the query path,
and no `missing`: only a proven deletion may claim one, and this read cannot
prove it.
- `SESSION_SEARCH_CANDIDATE_LIMIT_DEFAULT` is an option, not a constant, and
the result says when the limit was what cut the answer.
- The engine is the only caller of `store.warm()`, on first search.
Library only: no IPC, no settings, no Electron, nothing constructs it in
production.
* perf(ai-vault-search): measure what a query costs and what the candidate limit buys
Two corpora, because they answer different questions. The 10.5 MB / 40-session
corpus is what the scope split costs a reader: conversation is about 1.6x
faster at p50 and 3.4x at p95 than the full corpus, which is the argument for
the second FTS table being the one a keystroke can afford.
The candidate limit needs more sessions than the limit before it costs
anything, so it is swept over 2,500 one-turn transcripts with every session
matching. Limits are interleaved sample by sample: run back to back, the first
configuration pays for every page the OS cache had not seen and the ordering
alone moved p95 further than the limit did.
The doc says plainly what these numbers do not cover. They are cost, not
relevance; the MRR figures quoted beside the BM25 weights and the identifier
shadow column come from a shoot-out over real transcripts and cannot be
reproduced from this repository.
* refactor(ai-vault-search): key a page once per search, and report reachable pages honestly
The page key was hashed twice per search, once to decode the incoming cursor
and once to mint the outgoing one. The benchmark's reachable-page figure was
clamped against a constant that could never bind; it is the limit over the page
size and nothing else.
* test(ai-vault-search): pin the two engine seams nothing was holding
The warm wiring and the query-length cap were both written and neither was
observable. A spy pins that a search is what warms the store, and the cap is
pinned through the one input the planner's own term limit does not already
bound: a single enormous token, where the cut is what decides whether the term
matches the indexed one at all.
* fix(ai-vault-search): fence the generation against writers this process cannot see
The generation was cached in memory and moved only on this store's own writes,
and the bump itself was a read-then-write outside any transaction. Two handles
on one file is the normal case once PR 3 lands: the indexer writes in the
scanner child while an engine reads elsewhere. A reader would see that writer's
deletions while its own generation stood still, honour a stale cursor, and skip
a session; two writers could mint one generation for two snapshots.
`generation` now reads `meta` on every call, and the bump is a single
`ON CONFLICT DO UPDATE ... + 1` inside the transaction that makes the change.
That closes the crash hole the bump-on-open existed for, so opening a store no
longer invalidates anyone's cursor.
Only a change to what a read returns counts. Retiring a path the index never
held hides nothing, and the row deletes that drain a tombstone take away rows
that were already invisible: bumping there would refuse a cursor every 256 rows
and leave pagination unusable for as long as indexing ran.
Also drops redaction from the query log, following PR 2's decision to store
transcript content as written; the module it called no longer exists.
* fix(ai-vault-search): answer from a version-1 index, and keep a repaired literal whole
Two smaller findings.
An engine can be handed a connection to a version-1 file that another handle is
still answering from, and this PR is what first makes that reachable. It now
probes once for the two tables version 2 added and names what it cannot serve
on every result, instead of throwing at the first query that reaches for a
vocabulary that is not there. The route ladder simply skips its repair rung.
Which process may unlink and rebuild the index is PR 3b's decision and is not
solved here.
Typo repair re-planned the corrected query from scratch, and a corrected
spelling can read as prose even when what was typed was a literal:
`parseJsonn(the, data)` has the punctuation, `parsejson the data` does not, so
the re-plan dropped `the` as a stop word. The repaired query searched for less
than was asked and `repairedTerms` reported a body nobody typed. The re-plan is
now told what the original decided, because a repair changes spellings, not the
query's character.
`repairedTerms` is documented as the whole body the repaired plan ran, with the
index's own case-folded spelling for the terms it corrected.
* fix(ai-vault-search): make repo: and path: mean one thing in the list and the index
The engine said `repo:` and `path:` a second time, in SQL, and SQL cannot say
them. LIKE folds ASCII and nothing else, so `path:CAFÉ` missed `café`; the
engine searched `cwd_key` while the panel searches the working directory and
the transcript path, so `path:jsonl` matched every session in the panel and
none in the index; and the engine compared one path segment where the panel
compares the last two, so `repo:orca/session-search` missed. All four
reproduce in both directions.
So there is one definition now, not two that resemble each other.
`matchesAiVaultQueryOperators` moves into the shared filter module beside the
panel that already owned the semantics, the panel calls it, and the engine
applies it over the rows it retrieved. SQL keeps only what it can express
exactly: the `cwd_key` prefix range for `scopePaths`.
`parseVaultQuery` now parses through `splitAiVaultSearchQuery`, so one parser
decides what an operator is. Its existing tests pass unchanged; four degenerate
shapes do answer differently and are pinned as decisions rather than left to be
discovered.
Two consequences worth stating. The operators are conjunctive now, because that
is what the panel has always done, where the engine had been ORing within a
key. And the operator-only page walks newest sessions in bounded pages applying
the predicate, rather than taking one cut of the newest N and filtering it,
which would have answered `repo:x` with nothing on a busy index.
* docs(ai-vault-search): re-measure the query benchmark after the operator change
repo: and path: moved out of SQL, so the operator-only row measures something
different now and the note that it is a range seek was wrong. The rest of the
table is re-measured on an idle machine: the previous run's p95 column was
mostly contention, which is why conversation looked 3.4x faster at p95 rather
than the 1.7x it actually is.
Adds what this PR does not settle: which process may unlink and rebuild the
index is PR 3b's, and PR 4 is only the first thing that makes reading it
reachable.
* fix(ai-vault-search): stop reporting a search that gave up as a search that finished
The operator-only walk stops at a scan ceiling as well as at a full candidate
set, and only the first of those reached the result. A query whose one match
sat past the ceiling came back with no hits and truncated.candidates false,
which is the engine claiming there is nothing to find when what happened is
that it stopped looking. Retrieval now says why it stopped, because it is the
only layer that knows, and the count it used to return could not distinguish
the two cases.
The cursor fence stays as it is: any published read moves the generation, so an
outstanding cursor is refused, and that is what F11 asked for. What was wrong
was the claim next to the row-delete skip that pagination stays usable through
indexing. It does not, and the engine now says so. The rejection carries the
generation the cursor was minted in and the one the index is at, so a caller
can tell a moved index from a bad cursor and re-issue page one without showing
anyone an error.
The capability probe was nearly dead code, since every store opens through a
function that rebuilds a stale file. It is not dead, because two handles can be
open on one file, so the claim is corrected rather than the probe deleted. It
now runs per search: a verdict cached in the constructor is wrong in both
directions once another handle rebuilds the index.
Also says why the row-delete loop may skip the bump: those messages keep
batch_id NULL and stay in visible_messages, so what makes them unreachable is
their session's tombstone, and the read ratchet is what keeps every reader
joining the view that applies it.
* fix(ai-vault-search): restore the panel's reading of a quote that does not end a word
Unifying the two parsers changed panel behaviour on nine of twenty probed
shapes, not the three previously pinned. Six of the nine were regressions, all
from one rule: the shared parser refused a quoted span whose closing quote was
not followed by a space, so `"a b"c` and `repo:"a"b` became single terms
carrying their own quote characters, which match nothing.
The rule was justified as what stops the apostrophes in `it's a repo:orca
thing's` from swallowing the operator between them. It is not: a span only ever
opens at a token start, and the quote in `it's` is not at one. Dropping the
rule restores all six shapes to what the panel has always done and leaves that
protection intact.
Three changes remain and are kept because the old answer was worse in each: an
operator with an empty quoted value is dropped rather than filtering on `""`
and silently emptying the list, and a bare pair of quotes reads as an empty
term rather than as the two characters. Each is pinned with a test that says
which behaviour it is and why.
* fix(ai-vault-search): trim operator values, and report a query the engine had to cut
Three lows.
`repo:" "` survived as a term and matched no label, silently emptying the
list, which is the exact defect the empty-value drop exists to prevent wearing
different clothes. Operator values are trimmed, and a whitespace-only one drops
like an empty one. The substring matcher's copy of a free-text term is trimmed
too, so `" "` reads as the empty term already does; the span kept for FTS is
still the query verbatim.
Two caps upstream of retrieval fired silently: the planner searches at most 48
terms, and the engine cuts the query at 512 characters. A 56-term query whose
only match was the 56th came back with no hits and nothing truncated, which
claims there is nothing to find. `truncated.query` now says when either fired,
alongside the candidate and snippet flags that already did.
Every cursor refusal now carries the generation the index is at, which the
engine knows before it looks at the cursor, and the generation the cursor
claimed wherever that survived parsing. The doc says exactly when each is
present instead of leaving absence unexplained.
* refactor(ai-vault-search): read the tables the simplified index writes, and own the fence
PR 2 deleted the visibility views, the staging tables and the store's
generation, so this reads `sessions` and `messages` directly and carries the
three schema objects only a query needs — the vocabulary, the query log, and
the triggers that move the generation — as its own extension over the store's
schema.
The fence is now three triggers on `files`, because every transaction the store
opens that can change what a search returns writes that table and nothing else
does; retention's orphan drain is the one write path that touches neither, and
it is the one that must not bump. The triggers live in the file, so a writer in
another process moves the generation without knowing a reader exists.
A message row can now outlive its session row until the drain reaches it, so
the snippet read joins `sessions` and the typo repair asks for a live posting
instead of trusting the vocabulary's document count.
The engine takes a connection rather than a store: PR 2's store keeps its
connection private, and which process may open or rebuild the index file is
PR 3b's decision, not a query engine's.
* perf(ai-vault-search): price the second FTS table, and re-measure without warmup
Open decision 3. A column filter over `messages_fts` returns the identical
rowid set as `conversation_fts` — checked here per query rather than assumed —
so the table exists for latency alone. On a 105 MB corpus at both ends of the
tool-output band, the column-filtered form costs 1.16-1.42x at p95, against a
bar of 2x, so the recommendation is to delete it.
The shoot-out writes its own corpus because the answer turns on the one
property the shared generator fixes: how much of a transcript is tool output.
Half the tokens in that output are words the conversation also uses, which is
deliberately generous to the table under question.
The doc records the number that argues the other way. PR 2 priced the table at
about a quarter of the index on a corpus whose tool output is 56% of its
message text; on a tool-heavy one it is 6.7-11%, because `messages_fts` grows
with the tool text and the second table does not.
Page warmup is not re-added. The measurement behind it was on a 4 GB index,
removing it moves this corpus by less than the run-to-run spread, and a
cancellable background pass needs a lifecycle a query library does not have.
* test(ai-vault-search): pin that an append moves the generation a cursor is fenced by
* refactor(ai-vault-search): answer the conversation scope with a column filter
PR 2 deleted `conversation_fts` on the strength of this PR's shoot-out, so the
scope is a column filter over the one FTS table now. `ftsTableFor` is gone; a
scope is a pair of `scopedExpression` and `scopedWeights`, and the table name
no longer travels through the engine, the snippet builder or a hit.
The filter is parenthesised, and that is the whole of it: `{cols}: (a AND b)`
binds both terms, while `{cols}: a AND b` binds only the first and searches
tool output for the rest. A test drives an AND whose second term lives only in
tool output through both scopes.
The snippet keeps one guard, not two. Its column list and its expression were
each hiding the other's mistakes — a tool-only row was unreachable through
either — so the list is the same four columns for every scope and the scoped
expression is what makes a conversation snippet impossible to draw out of tool
output. Dropping it now leaks that row, which a test catches.
One behaviour the deleted table did not have, pinned rather than wished away:
bm25 normalises by the whole row's length and has no per-column length, so two
rows with identical prose score differently when one also holds tool output.
The rowid set is unchanged; the order within it can move.
Re-measured on the shipping schema. The conversation scope is 1.2-1.4x faster
than `all` at every rung, and the index is 57 MB rather than about 150 MB at
93% tool output, because a tool row is now capped at 3,072 characters.
* fix(ai-vault-search): repair a spelling inside the scope that will answer it
Typo repair read `messages_vocab` and probed `messages_fts` with no column
filter, so tool output decided whether a conversation-scoped query was
repaired, in both directions. A tool row carrying the misspelling made the
query look correctly spelled and suppressed the repair; a tool row carrying a
rare word became the suggestion, naming in `repairedTerms` a string from a
column the scope will never show. Both reproduced against a control index that
differs by exactly that one row.
The vocabulary proposes and a scoped count disposes. fts5vocab is per table and
cannot be column-filtered, so every decision that reaches the plan — already
spelled right, eligible, and which of two equally close candidates wins — now
comes from a `messages_fts MATCH` under the same filter retrieval uses, joined
to `sessions`.
That also takes the vocabulary's `doc` out of the ranking, which is the half of
the drain defect that belongs here: `doc` counts rows whose session a purge has
already cut loose, so reclaiming them changed which word a query was repaired
to. Candidates are ordered by term now, because the ordering decides which of
them survive the scan limit, and ties on similarity go to the more common word
counted live rather than to the vocabulary's number.
The cost is one bounded count per candidate examined, at most eight per prefix,
and only for a term the scope has no posting for at all.
* fix(ai-vault-search): fence the rows a purge reclaims after it cuts a session loose
Retention's second half deletes from `messages` alone and touched neither
`files` nor `sessions`, so it moved no generation. The argument was that those
rows answer nothing, which was true of retrieval and not of the engine: the
typo repair's dictionary is a view over the FTS b-tree and listed them, so a
drain running between two pages swapped the repair under a cursor that was
still honoured, and a search that had answered stopped answering.
The commit before this one fixes that at its source by counting live rows. It
does not make the drain provably inert — the vocabulary still decides which
candidates survive its scan limit, and reclaiming a term's last row moves where
that limit cuts — so the fence is what covers the rest.
A fourth trigger, on `messages`, with a `WHEN` clause that is the whole reason
it is affordable: a replace and a `removeFile` delete a session's rows while
its `sessions` row still stands, so neither fires, and both already bump
through `files`. Only the drain deletes a row whose session is gone.
The price is named rather than avoided: a cursor outstanding while a purge runs
is now refused once per batch, which `SessionSearchCursorError` reports as
`stale-generation` so a caller re-issues page one. The test that pinned the old
contract is replaced by one for the new one, and by one proving a replace still
does not fire it.
* fix(ai-vault-search): tell a highlight from a transcript that contains brackets
The snippet builder asked each of a row's four columns for a marked snippet and
showed the first whose text contained `[[`. Transcripts contain `[[`: a bash
`if [[ -f … ]]`, numpy's `[[1, 2], [3, 4]]`. A row matching only in tool output
was shown its user turn instead, with nothing highlighted in it, and the
any-column fallback an identifier-only match depends on was unreachable behind
the same collision.
Whether a column matched is now the difference between two renderings of the
same text: `snippet(…, MARK, MARK, …)` beside `snippet(…, '', '', …)`. Content
cannot forge a difference between those two, because it is the same content
either way.
The marks FTS5 inserts are private-use code points, rewritten to the public
`[[` and `]]` once, at the end. That is for the other decision that has to tell
a mark from content: the truncation refuses to cut between an open mark and its
close, and a transcript's own bracket used to move that cut.
* fix(ai-vault-search): cut a query on a code point and bind ids in batches
Two small ones from the review's not-routed list.
`query.slice(0, 512)` can land between the halves of a surrogate pair, leaving
a lone half that matches nothing and that a caller cannot echo back. The reader
already has `sliceAtCodeUnitLimit` for exactly this.
`loadSessions` bound one parameter per candidate id in a single statement. The
list is as long as the candidate limit, the tuning doc invites a host to raise
that limit, and SQLite's `SQLITE_MAX_VARIABLE_NUMBER` is 999 on builds older
than 3.32 — so one settings change away from `too many SQL variables`. Read in
batches of 500, leaving room for the filter's own bound values.
* docs(ai-vault-search): price the repair rung, and record what is left open
Typo repair is the one rung whose cost tracks the vocabulary rather than the
result, and it only runs for a term the scope has no posting for. Measured over
1.6 M distinct terms: 10 ms for one unknown term, 387 ms for a 480-character
query of thirty-nine of them.
The scoped-count fix made that cheaper rather than dearer, from 737 ms, because
ordering the vocabulary scan by term drops the sort `doc DESC` needed and the
counts it adds are at most eight bounded probes per prefix. A cap on unknown
terms per query is a follow-up in the split plan, with the five other items the
final review raised and did not route.
* test(ai-vault-search): make each snippet mark mechanism answer for itself
Two mechanisms landed together and hid each other: choosing a column by
comparing a marked rendering against an unmarked one, and marking with
private-use code points instead of `[[`. Either alone fixed the bracket repro,
so neither had a mutation against it — the same masking the snippet's two
column guards had a round ago.
They do different jobs, so both stay and each gets the test that needs it. A
transcript holding a private-use code point of its own is what the comparison
is for; agent output carries Nerd Font glyphs from that block. A snippet past
the character ceiling with a bracket after its last real mark is what the
private-use marks are for, because the truncation has to find that mark by
searching the text.
The two one-line fixes get honest framing rather than a mutation neither can
have. A lone surrogate is not a token character, so the planner drops it either
way and the safe cut is hygiene. And no SQLite this stack runs refuses 1,100
bound ids — 32,766 has been the floor since 3.32 — so the batch is about
owning the ceiling here rather than rescuing a reachable failure.
* refactor(ai-vault-search): keep the scope's expression with the other expressions
Making the typo repair ask its questions in the search's own scope put an
import from retrieval into it, and retrieval already owns the repair — a cycle
the native audit catches. `scopedExpression` belongs beside `phraseExpression`,
`andExpression` and `orExpression` anyway: it builds a MATCH expression, and
two callers now need it. The bm25 weights stay in retrieval, where the SQL that
uses them is.
* docs(ai-vault-search): say which delete paths fire the orphan-reclaim trigger after a replace cuts loose
* test(ai-vault-search): make the trigger-restore test exercise the trigger it drops
* fix(ai-vault-search): keep a scope nothing could key from widening the search
* fix(ai-vault-search): read a fractional or negative cursor generation as malformed
* fix(ai-vault-search): highlight only the marks FTS5 inserted, not the text's own
* test(ai-vault-search): compare the whole query, so a quoted operator value survives
* fix(ai-vault-search): filter routes and fence page reads; simplify query engine
* refactor(ai-vault-search): narrow retrieval API and clarify page rejection
|
||
|
Neil
|
9c918b93a6 | fix(deps): take Electron 43.7.0 for the glibc environ use-after-free (#20089) | ||
 Brennan BensonandMerge Sim
|
da5d555259 |
refactor(agent-status): delete the runtime's retained row store (PR 1b) (#19785)
* docs(agent-status): plan PR 1b at file level Names the five RuntimeAgentRowStore call sites and what each becomes, why terminalHandle has to be stamped before the store can go, and the one intended behavior change. * feat(agent-status): stamp the pane terminal handle on hook-server rows The runtime's retained row store carried the pty binding two readers need. Put that fact on the row that already owns the pane instead, resolved through the same lookup the renderer-facing IPC boundary runs, so the two surfaces cannot disagree about which terminal a pane is. Carried forward when a later write resolves no handle (only main's OSC parse can), and never persisted: a handle belongs to the runtime that issued it. * refactor(agent-status): route the session-tabs republish off the store `retain()` was not only a duplicate store: its boolean return was the signal that republished `session.tabs` for a status-only transition, which no title change covers (#7970). `hook-status-session-tabs-invalidation.ts` already mirrors that change set plus hook restore provenance, so route the signal off the store rather than keep a second comparator. Adds the status-drop arm a user dismissal emits, which the pane-clear fan-out deliberately skips — now load-bearing, because a dismissed row leaves the listing at once. Installed on both hosts. orcad had neither the OSC producer nor this signal, so its runtime observed agent status and published it nowhere; deleting the retained copy without wiring it would list no PTY agents there at all. * refactor(agent-status): delete the runtime's duplicate retained row store `RuntimeAgentRowStore` held the same payload the hook server already holds, so the same pane could legitimately read differently in the sidebar, in `worktree ps`, and on the phone. Both of its readers move onto the store's snapshot in `runtime-hook-agent-row-selection.ts`, and `collectRuntimeWorktreePtyAgentSources` loses the retained-versus-hook reconciliation that only existed because two stores could disagree. `ConnectedPtyEvidence` trades its flat pty-id set for `ptyIdByTerminalHandle`, which is how a row still resolves the connected PTY behind it — the working-terminal rollup's match key, and the last rescue for a row whose pane binding a controller incarnation nulled under it. The one intended behavior change: a row the user dismisses on the desktop leaves `worktree ps` and mobile at once instead of lingering until the pty exits. One store means one dismissal. The suites written against the retained store are rewired to a real AgentHookServer rather than deleted, so each still asserts the listing behavior it named. * docs(agent-status): record what PR 1b landed Past tense, plus two corrections to the plan: `terminalHandle` is not the pty id (they are different identifiers, and the explicit-status reader was already comparing against a real handle), and the legacy numeric pane key is a consequence the plan did not name. * fix(agent-status): harden single-store lifecycle * fix(agent-status): preserve mobile terminal rejoin * fix(agent-status): preserve unverifiable remote rows * fix(agent-status): own PTY row lifecycle in hook server * fix(agent-status): preserve state and renew freshness * fix(agent-status): ignore freshness for dismissed identity rows * fix(agent-status): fence orcad observed identities * fix(orcad): always release daemon adapter on cleanup * fix(agent-status): cover remint and headless lifecycle edges * fix agent status identity recovery gaps * fix(agent-status): suppress duplicate child-only row mutation * test(runtime): preserve hook store wiring in transcript harness --------- Co-authored-by: Merge Sim <sim@local> |
||
|
Neil
|
6bb2b0c6d7 |
test(runtime): capture real Antigravity transcripts — the detector is inverted on live output (#19983)
* test(runtime): capture real agent PTY transcripts before rewriting Antigravity readiness
Antigravity readiness has been written five times against a five-line screen
typed from memory. There is no Antigravity transcript in this repository, so
every attempt was a guess tested against another guess. This adds the recorder,
the protocol and the fixture-driven suite so the sixth attempt can be written
against evidence, and changes no detector logic.
- config/scripts/capture-agent-pty-transcript.mjs records a live agent session
through a real PTY, escapes and wrapping intact. Ctrl-] is consumed by the
recorder and never forwarded, which is the only way to end a capture while a
dialog still owns the screen.
- config/scripts/pty-transcript-secret-scan.mjs finds account identifiers and
credentials, redacts them with same-length placeholders so wrapping survives,
and recognises its own placeholders so a scrubbed file verifies clean.
- src/main/runtime/antigravity-readiness-transcripts.test.ts asserts a verdict
per transcript and skips by name until the transcripts land, with a
doc-coverage ratchet and a guard that a fixture contains escape bytes.
The escape-byte guard exists because the three cursor-agent fixtures carry a
comment claiming they were captured verbatim through Orca, yet contain zero ESC
bytes and zero carriage returns. That comment is corrected here to say what
those files are; the fixtures and the rules built on them are untouched.
* test(runtime): capture real Antigravity transcripts, and pin what they prove
`agy` 1.1.25 turned out to be installed, so the transcripts this scaffold was
built for now exist. Six are recorded from live sessions and committed; the
rest are named as skipped, because reaching them would mean signing the
operator out or deleting their config.
The captures invert the story. On real output the shipped detector refuses a
genuinely ready screen and accepts a live `/model` picker:
- Antigravity paints a block-glyph logo down the left, so the model row never
starts a line. `startsWith('gemini', trimmedStart)` cannot match a real ready
screen, on any account or model. Stripping the logo flips the same screen to
ready, which means a decorative glyph decides readiness today.
- The `/model` picker prints `Gemini 3.x Flash` one per line, at line start, and
a bare `>` composer sits earlier in the tail. Both halves of the rule are
satisfied while a dialog owns the screen.
- For an API-key user the identity row reads `Gemini API key` — no `@`, no
domain — and `AGY_CLI_HIDE_ACCOUNT_INFO=1` removes the row entirely. The
account-row requirement of attempts 4 and 5 can never pass for those users.
- The banner is printed once and never reprinted after a dialog is dismissed, so
`headerIndex` cannot be the ordering anchor.
Four suite cases are pinned as KNOWN DEFECT: they assert what the detector does
so CI stays honest instead of permanently red, and flip to failing the moment
someone fixes it. No detector logic changed.
The recorder gains `--send "<ms>:<text>"` because a dialog capture has to be
driven and an unattended run has no TTY, and the scrub scanner gains a UUID rule
because agy prints a resumable conversation id on exit.
* test(runtime): capture agy mid-turn, and make the scan file reviewable
Answers the busy-frame question a P1 review raised against attempt six, with
two new captures from a live turn.
At the frame level the review is right: a busy frame parks the caret with the
same bytes as an idle one, `CR ESC[2A ESC[2C`, and the only differing row —
`esc to cancel` versus `? for shortcuts` — is erased by that park.
At the retained-tail level it does not reproduce. Each spinner tick is its own
repaint with its own `CR ESC[2A`, two rows higher than the frame's, which
splices the composer away: a live turn's tail ends on `⣟ Generating...`, with
no bare caret to match. A constructed input that keeps the park and edits only
the status text is not faithful, because a live turn has a spinner row
repainting below the composer.
The residual is the gap between a frame park and the next tick, where the tail
does end on the bare caret. Quiescence-gated paths are safe there because ticks
keep arriving; text-only paths are not, and for those the capture supports one
clause: a braille glyph on the last visible line means working. That predicate
already exists here for cursor-agent and should be reused, scoped to the last
line — a first-run transcript prints `⠾ Signing in...` during startup.
Also in this commit, from the same review:
- pty-transcript-secret-scan.mjs held raw 0x00-0x1f bytes in a character class,
so the one file gating real PTY data into history was binary to git and
unreviewable in a diff. It now tests codepoints, which the formatter cannot
fold back into control bytes.
- Pin `src/main/runtime/__fixtures__/*.txt` as -text. A Windows checkout would
otherwise normalise line endings and rewrite the CR bytes that make these
files evidence.
The recorder now stops appending at the stop moment rather than through
shutdown: an agent repaints an idle frame on its way out, which was overwriting
the mid-turn state the capture existed to record.
* test(tooling): allowlist the transcript scan test in the batch-shim ratchet
pty-transcript-secret-scan.test.mjs asserts that the capture recorder routes
an 'agy.cmd' shim through cmd.exe, so the shim literal it names is the
assertion, not a spawn. Fits the existing assert-on-shim-files category.
|
||
|
Jinwoo Hong
|
f31bfa8fb7 |
Revert "feat(ai-vault-search): session search query engine over the index (#19750)" (#20023)
This reverts commit
|
||
|
Jinwoo Hong
|
cf7408a37f |
feat(ai-vault-search): session search query engine over the index (#19750)
* feat(ai-vault-search): give the index a generation a page cursor can be fenced by
A search page is a slice of one ranked list, so a cursor only means anything
against the snapshot that produced it. The store now keeps a monotone
generation in `meta`, bumped by every mutation that can change which rows a
read returns, and starts a new one on open so a write whose bump never landed
cannot leave a cursor pointing at content that is already gone.
Schema version 2 adds the two tables the query layer needs: `messages_vocab`
(fts5vocab over messages_fts, the typo repair's whole dictionary) and
`search_log`. Both are pure additions and the index is a cache, so a version-1
file is dropped and rebuilt exactly as any other mismatch is.
* feat(ai-vault-search): plan a query the way the index tokenized it
The planner unfolds the tokenizer contract instead of asking SQLite: same
boundaries as `unicode61 tokenchars '_.-/+'`, pinned against real fts5vocab
output so a query can be planned without a round trip. It decides the route
ladder's first rung (literal shape), strips stop words from prose but never
from a literal, and fans an identifier out into its pieces for the OR fallback.
Typo repair uses the index's own vocabulary as its dictionary, so it can never
suggest a term the index does not hold. Its exact-match probe now joins
`visible_messages`: `messages_vocab` is a view over the FTS b-tree and still
lists staged and tombstoned terms, and the PR 2 read ratchet is right to
demand the join.
`splitAiVaultSearchQuery` is the index's reading of repo: / path:. It keeps
operator case, which the panel folds and cwd_key must not; a census test pins
the two parsers to the same answer about what is an operator until PR 7 moves
the panel onto this one.
* feat(ai-vault-search): narrow a search the way the sidebar keys a folder
Every caller-supplied narrowing in one place, so retrieval, the operator-only
page and the session load cannot drift apart: agents, an updated-at floor, the
retention cutoff, scope paths, and the repo: / path: operators.
Scope keying goes through PR 2's `cwdKey`, which is the sidebar's
`folderGroupKey` without its prefix, rather than the original branch's second
spelling. That drops the branch's WSL distro qualification, which PR 2 removed
on purpose, and it makes the filesystem root a key that already ends in a
separator, so the child-prefix range is built from the key rather than by
appending one; `//` sorts below every real child and would scope the root to
nothing.
Engine types land here too, under src/main and not src/shared: nothing in this
PR is a wire type, and PR 5 lifts what a caller may receive.
* feat(ai-vault-search): rank, page and answer a session search
`SessionSearchEngine.search()` over the PR 2 store: route ladder (phrase, AND,
typo repair, OR), BM25 weights per corpus, one hit per session, fork folding,
and a page.
- `scope` picks the corpus and the engine never second-guesses it.
`conversation` is user and assistant turns; `all` adds tool output and the
identifier shadow column. Switching corpus while typing is PR 7's policy; an
engine that widened on a miss would make a result impossible to reproduce
from its own request.
- Pagination is an offset into one ranked list, fenced by the index generation
and by a hash of everything that changes the ranking. A cursor from another
generation or another query is refused with a typed error rather than
silently re-run. Ranking breaks every tie by session id, because a cursor
indexes into that order and retrieval does not promise one.
- Snippets and source presence are paid for by the page, not the list. A
snippet past the per-hit ceiling is cut on a code point, never between `[[`
and its `]]`, and flagged on the hit.
- Source presence is read from the `files` table. No stat on the query path,
and no `missing`: only a proven deletion may claim one, and this read cannot
prove it.
- `SESSION_SEARCH_CANDIDATE_LIMIT_DEFAULT` is an option, not a constant, and
the result says when the limit was what cut the answer.
- The engine is the only caller of `store.warm()`, on first search.
Library only: no IPC, no settings, no Electron, nothing constructs it in
production.
* perf(ai-vault-search): measure what a query costs and what the candidate limit buys
Two corpora, because they answer different questions. The 10.5 MB / 40-session
corpus is what the scope split costs a reader: conversation is about 1.6x
faster at p50 and 3.4x at p95 than the full corpus, which is the argument for
the second FTS table being the one a keystroke can afford.
The candidate limit needs more sessions than the limit before it costs
anything, so it is swept over 2,500 one-turn transcripts with every session
matching. Limits are interleaved sample by sample: run back to back, the first
configuration pays for every page the OS cache had not seen and the ordering
alone moved p95 further than the limit did.
The doc says plainly what these numbers do not cover. They are cost, not
relevance; the MRR figures quoted beside the BM25 weights and the identifier
shadow column come from a shoot-out over real transcripts and cannot be
reproduced from this repository.
* refactor(ai-vault-search): key a page once per search, and report reachable pages honestly
The page key was hashed twice per search, once to decode the incoming cursor
and once to mint the outgoing one. The benchmark's reachable-page figure was
clamped against a constant that could never bind; it is the limit over the page
size and nothing else.
* test(ai-vault-search): pin the two engine seams nothing was holding
The warm wiring and the query-length cap were both written and neither was
observable. A spy pins that a search is what warms the store, and the cap is
pinned through the one input the planner's own term limit does not already
bound: a single enormous token, where the cut is what decides whether the term
matches the indexed one at all.
* fix(ai-vault-search): fence the generation against writers this process cannot see
The generation was cached in memory and moved only on this store's own writes,
and the bump itself was a read-then-write outside any transaction. Two handles
on one file is the normal case once PR 3 lands: the indexer writes in the
scanner child while an engine reads elsewhere. A reader would see that writer's
deletions while its own generation stood still, honour a stale cursor, and skip
a session; two writers could mint one generation for two snapshots.
`generation` now reads `meta` on every call, and the bump is a single
`ON CONFLICT DO UPDATE ... + 1` inside the transaction that makes the change.
That closes the crash hole the bump-on-open existed for, so opening a store no
longer invalidates anyone's cursor.
Only a change to what a read returns counts. Retiring a path the index never
held hides nothing, and the row deletes that drain a tombstone take away rows
that were already invisible: bumping there would refuse a cursor every 256 rows
and leave pagination unusable for as long as indexing ran.
Also drops redaction from the query log, following PR 2's decision to store
transcript content as written; the module it called no longer exists.
* fix(ai-vault-search): answer from a version-1 index, and keep a repaired literal whole
Two smaller findings.
An engine can be handed a connection to a version-1 file that another handle is
still answering from, and this PR is what first makes that reachable. It now
probes once for the two tables version 2 added and names what it cannot serve
on every result, instead of throwing at the first query that reaches for a
vocabulary that is not there. The route ladder simply skips its repair rung.
Which process may unlink and rebuild the index is PR 3b's decision and is not
solved here.
Typo repair re-planned the corrected query from scratch, and a corrected
spelling can read as prose even when what was typed was a literal:
`parseJsonn(the, data)` has the punctuation, `parsejson the data` does not, so
the re-plan dropped `the` as a stop word. The repaired query searched for less
than was asked and `repairedTerms` reported a body nobody typed. The re-plan is
now told what the original decided, because a repair changes spellings, not the
query's character.
`repairedTerms` is documented as the whole body the repaired plan ran, with the
index's own case-folded spelling for the terms it corrected.
* fix(ai-vault-search): make repo: and path: mean one thing in the list and the index
The engine said `repo:` and `path:` a second time, in SQL, and SQL cannot say
them. LIKE folds ASCII and nothing else, so `path:CAFÉ` missed `café`; the
engine searched `cwd_key` while the panel searches the working directory and
the transcript path, so `path:jsonl` matched every session in the panel and
none in the index; and the engine compared one path segment where the panel
compares the last two, so `repo:orca/session-search` missed. All four
reproduce in both directions.
So there is one definition now, not two that resemble each other.
`matchesAiVaultQueryOperators` moves into the shared filter module beside the
panel that already owned the semantics, the panel calls it, and the engine
applies it over the rows it retrieved. SQL keeps only what it can express
exactly: the `cwd_key` prefix range for `scopePaths`.
`parseVaultQuery` now parses through `splitAiVaultSearchQuery`, so one parser
decides what an operator is. Its existing tests pass unchanged; four degenerate
shapes do answer differently and are pinned as decisions rather than left to be
discovered.
Two consequences worth stating. The operators are conjunctive now, because that
is what the panel has always done, where the engine had been ORing within a
key. And the operator-only page walks newest sessions in bounded pages applying
the predicate, rather than taking one cut of the newest N and filtering it,
which would have answered `repo:x` with nothing on a busy index.
* docs(ai-vault-search): re-measure the query benchmark after the operator change
repo: and path: moved out of SQL, so the operator-only row measures something
different now and the note that it is a range seek was wrong. The rest of the
table is re-measured on an idle machine: the previous run's p95 column was
mostly contention, which is why conversation looked 3.4x faster at p95 rather
than the 1.7x it actually is.
Adds what this PR does not settle: which process may unlink and rebuild the
index is PR 3b's, and PR 4 is only the first thing that makes reading it
reachable.
* fix(ai-vault-search): stop reporting a search that gave up as a search that finished
The operator-only walk stops at a scan ceiling as well as at a full candidate
set, and only the first of those reached the result. A query whose one match
sat past the ceiling came back with no hits and truncated.candidates false,
which is the engine claiming there is nothing to find when what happened is
that it stopped looking. Retrieval now says why it stopped, because it is the
only layer that knows, and the count it used to return could not distinguish
the two cases.
The cursor fence stays as it is: any published read moves the generation, so an
outstanding cursor is refused, and that is what F11 asked for. What was wrong
was the claim next to the row-delete skip that pagination stays usable through
indexing. It does not, and the engine now says so. The rejection carries the
generation the cursor was minted in and the one the index is at, so a caller
can tell a moved index from a bad cursor and re-issue page one without showing
anyone an error.
The capability probe was nearly dead code, since every store opens through a
function that rebuilds a stale file. It is not dead, because two handles can be
open on one file, so the claim is corrected rather than the probe deleted. It
now runs per search: a verdict cached in the constructor is wrong in both
directions once another handle rebuilds the index.
Also says why the row-delete loop may skip the bump: those messages keep
batch_id NULL and stay in visible_messages, so what makes them unreachable is
their session's tombstone, and the read ratchet is what keeps every reader
joining the view that applies it.
* fix(ai-vault-search): restore the panel's reading of a quote that does not end a word
Unifying the two parsers changed panel behaviour on nine of twenty probed
shapes, not the three previously pinned. Six of the nine were regressions, all
from one rule: the shared parser refused a quoted span whose closing quote was
not followed by a space, so `"a b"c` and `repo:"a"b` became single terms
carrying their own quote characters, which match nothing.
The rule was justified as what stops the apostrophes in `it's a repo:orca
thing's` from swallowing the operator between them. It is not: a span only ever
opens at a token start, and the quote in `it's` is not at one. Dropping the
rule restores all six shapes to what the panel has always done and leaves that
protection intact.
Three changes remain and are kept because the old answer was worse in each: an
operator with an empty quoted value is dropped rather than filtering on `""`
and silently emptying the list, and a bare pair of quotes reads as an empty
term rather than as the two characters. Each is pinned with a test that says
which behaviour it is and why.
* fix(ai-vault-search): trim operator values, and report a query the engine had to cut
Three lows.
`repo:" "` survived as a term and matched no label, silently emptying the
list, which is the exact defect the empty-value drop exists to prevent wearing
different clothes. Operator values are trimmed, and a whitespace-only one drops
like an empty one. The substring matcher's copy of a free-text term is trimmed
too, so `" "` reads as the empty term already does; the span kept for FTS is
still the query verbatim.
Two caps upstream of retrieval fired silently: the planner searches at most 48
terms, and the engine cuts the query at 512 characters. A 56-term query whose
only match was the 56th came back with no hits and nothing truncated, which
claims there is nothing to find. `truncated.query` now says when either fired,
alongside the candidate and snippet flags that already did.
Every cursor refusal now carries the generation the index is at, which the
engine knows before it looks at the cursor, and the generation the cursor
claimed wherever that survived parsing. The doc says exactly when each is
present instead of leaving absence unexplained.
* refactor(ai-vault-search): read the tables the simplified index writes, and own the fence
PR 2 deleted the visibility views, the staging tables and the store's
generation, so this reads `sessions` and `messages` directly and carries the
three schema objects only a query needs — the vocabulary, the query log, and
the triggers that move the generation — as its own extension over the store's
schema.
The fence is now three triggers on `files`, because every transaction the store
opens that can change what a search returns writes that table and nothing else
does; retention's orphan drain is the one write path that touches neither, and
it is the one that must not bump. The triggers live in the file, so a writer in
another process moves the generation without knowing a reader exists.
A message row can now outlive its session row until the drain reaches it, so
the snippet read joins `sessions` and the typo repair asks for a live posting
instead of trusting the vocabulary's document count.
The engine takes a connection rather than a store: PR 2's store keeps its
connection private, and which process may open or rebuild the index file is
PR 3b's decision, not a query engine's.
* perf(ai-vault-search): price the second FTS table, and re-measure without warmup
Open decision 3. A column filter over `messages_fts` returns the identical
rowid set as `conversation_fts` — checked here per query rather than assumed —
so the table exists for latency alone. On a 105 MB corpus at both ends of the
tool-output band, the column-filtered form costs 1.16-1.42x at p95, against a
bar of 2x, so the recommendation is to delete it.
The shoot-out writes its own corpus because the answer turns on the one
property the shared generator fixes: how much of a transcript is tool output.
Half the tokens in that output are words the conversation also uses, which is
deliberately generous to the table under question.
The doc records the number that argues the other way. PR 2 priced the table at
about a quarter of the index on a corpus whose tool output is 56% of its
message text; on a tool-heavy one it is 6.7-11%, because `messages_fts` grows
with the tool text and the second table does not.
Page warmup is not re-added. The measurement behind it was on a 4 GB index,
removing it moves this corpus by less than the run-to-run spread, and a
cancellable background pass needs a lifecycle a query library does not have.
* test(ai-vault-search): pin that an append moves the generation a cursor is fenced by
* refactor(ai-vault-search): answer the conversation scope with a column filter
PR 2 deleted `conversation_fts` on the strength of this PR's shoot-out, so the
scope is a column filter over the one FTS table now. `ftsTableFor` is gone; a
scope is a pair of `scopedExpression` and `scopedWeights`, and the table name
no longer travels through the engine, the snippet builder or a hit.
The filter is parenthesised, and that is the whole of it: `{cols}: (a AND b)`
binds both terms, while `{cols}: a AND b` binds only the first and searches
tool output for the rest. A test drives an AND whose second term lives only in
tool output through both scopes.
The snippet keeps one guard, not two. Its column list and its expression were
each hiding the other's mistakes — a tool-only row was unreachable through
either — so the list is the same four columns for every scope and the scoped
expression is what makes a conversation snippet impossible to draw out of tool
output. Dropping it now leaks that row, which a test catches.
One behaviour the deleted table did not have, pinned rather than wished away:
bm25 normalises by the whole row's length and has no per-column length, so two
rows with identical prose score differently when one also holds tool output.
The rowid set is unchanged; the order within it can move.
Re-measured on the shipping schema. The conversation scope is 1.2-1.4x faster
than `all` at every rung, and the index is 57 MB rather than about 150 MB at
93% tool output, because a tool row is now capped at 3,072 characters.
* fix(ai-vault-search): repair a spelling inside the scope that will answer it
Typo repair read `messages_vocab` and probed `messages_fts` with no column
filter, so tool output decided whether a conversation-scoped query was
repaired, in both directions. A tool row carrying the misspelling made the
query look correctly spelled and suppressed the repair; a tool row carrying a
rare word became the suggestion, naming in `repairedTerms` a string from a
column the scope will never show. Both reproduced against a control index that
differs by exactly that one row.
The vocabulary proposes and a scoped count disposes. fts5vocab is per table and
cannot be column-filtered, so every decision that reaches the plan — already
spelled right, eligible, and which of two equally close candidates wins — now
comes from a `messages_fts MATCH` under the same filter retrieval uses, joined
to `sessions`.
That also takes the vocabulary's `doc` out of the ranking, which is the half of
the drain defect that belongs here: `doc` counts rows whose session a purge has
already cut loose, so reclaiming them changed which word a query was repaired
to. Candidates are ordered by term now, because the ordering decides which of
them survive the scan limit, and ties on similarity go to the more common word
counted live rather than to the vocabulary's number.
The cost is one bounded count per candidate examined, at most eight per prefix,
and only for a term the scope has no posting for at all.
* fix(ai-vault-search): fence the rows a purge reclaims after it cuts a session loose
Retention's second half deletes from `messages` alone and touched neither
`files` nor `sessions`, so it moved no generation. The argument was that those
rows answer nothing, which was true of retrieval and not of the engine: the
typo repair's dictionary is a view over the FTS b-tree and listed them, so a
drain running between two pages swapped the repair under a cursor that was
still honoured, and a search that had answered stopped answering.
The commit before this one fixes that at its source by counting live rows. It
does not make the drain provably inert — the vocabulary still decides which
candidates survive its scan limit, and reclaiming a term's last row moves where
that limit cuts — so the fence is what covers the rest.
A fourth trigger, on `messages`, with a `WHEN` clause that is the whole reason
it is affordable: a replace and a `removeFile` delete a session's rows while
its `sessions` row still stands, so neither fires, and both already bump
through `files`. Only the drain deletes a row whose session is gone.
The price is named rather than avoided: a cursor outstanding while a purge runs
is now refused once per batch, which `SessionSearchCursorError` reports as
`stale-generation` so a caller re-issues page one. The test that pinned the old
contract is replaced by one for the new one, and by one proving a replace still
does not fire it.
* fix(ai-vault-search): tell a highlight from a transcript that contains brackets
The snippet builder asked each of a row's four columns for a marked snippet and
showed the first whose text contained `[[`. Transcripts contain `[[`: a bash
`if [[ -f … ]]`, numpy's `[[1, 2], [3, 4]]`. A row matching only in tool output
was shown its user turn instead, with nothing highlighted in it, and the
any-column fallback an identifier-only match depends on was unreachable behind
the same collision.
Whether a column matched is now the difference between two renderings of the
same text: `snippet(…, MARK, MARK, …)` beside `snippet(…, '', '', …)`. Content
cannot forge a difference between those two, because it is the same content
either way.
The marks FTS5 inserts are private-use code points, rewritten to the public
`[[` and `]]` once, at the end. That is for the other decision that has to tell
a mark from content: the truncation refuses to cut between an open mark and its
close, and a transcript's own bracket used to move that cut.
* fix(ai-vault-search): cut a query on a code point and bind ids in batches
Two small ones from the review's not-routed list.
`query.slice(0, 512)` can land between the halves of a surrogate pair, leaving
a lone half that matches nothing and that a caller cannot echo back. The reader
already has `sliceAtCodeUnitLimit` for exactly this.
`loadSessions` bound one parameter per candidate id in a single statement. The
list is as long as the candidate limit, the tuning doc invites a host to raise
that limit, and SQLite's `SQLITE_MAX_VARIABLE_NUMBER` is 999 on builds older
than 3.32 — so one settings change away from `too many SQL variables`. Read in
batches of 500, leaving room for the filter's own bound values.
* docs(ai-vault-search): price the repair rung, and record what is left open
Typo repair is the one rung whose cost tracks the vocabulary rather than the
result, and it only runs for a term the scope has no posting for. Measured over
1.6 M distinct terms: 10 ms for one unknown term, 387 ms for a 480-character
query of thirty-nine of them.
The scoped-count fix made that cheaper rather than dearer, from 737 ms, because
ordering the vocabulary scan by term drops the sort `doc DESC` needed and the
counts it adds are at most eight bounded probes per prefix. A cap on unknown
terms per query is a follow-up in the split plan, with the five other items the
final review raised and did not route.
* test(ai-vault-search): make each snippet mark mechanism answer for itself
Two mechanisms landed together and hid each other: choosing a column by
comparing a marked rendering against an unmarked one, and marking with
private-use code points instead of `[[`. Either alone fixed the bracket repro,
so neither had a mutation against it — the same masking the snippet's two
column guards had a round ago.
They do different jobs, so both stay and each gets the test that needs it. A
transcript holding a private-use code point of its own is what the comparison
is for; agent output carries Nerd Font glyphs from that block. A snippet past
the character ceiling with a bracket after its last real mark is what the
private-use marks are for, because the truncation has to find that mark by
searching the text.
The two one-line fixes get honest framing rather than a mutation neither can
have. A lone surrogate is not a token character, so the planner drops it either
way and the safe cut is hygiene. And no SQLite this stack runs refuses 1,100
bound ids — 32,766 has been the floor since 3.32 — so the batch is about
owning the ceiling here rather than rescuing a reachable failure.
* refactor(ai-vault-search): keep the scope's expression with the other expressions
Making the typo repair ask its questions in the search's own scope put an
import from retrieval into it, and retrieval already owns the repair — a cycle
the native audit catches. `scopedExpression` belongs beside `phraseExpression`,
`andExpression` and `orExpression` anyway: it builds a MATCH expression, and
two callers now need it. The bm25 weights stay in retrieval, where the SQL that
uses them is.
* docs(ai-vault-search): say which delete paths fire the orphan-reclaim trigger after a replace cuts loose
|
||
|
Jinwoo Hong
|
74cc9b5039 |
feat(desktop): native mobile push integration (2/3) (#19935)
* feat(desktop): integrate native mobile push delivery and lifecycle * fix(desktop): preserve notification replay policy and review invariants * fix(desktop): correct notification locale namespace and auto-ack tests |
||
|
Brennan BensonandMerge Sim
|
2626e2eca4 |
Make the structured turn lifecycle row durable so completed durations survive (#19695)
* Make the structured turn lifecycle row durable so completed durations survive A structured-chat turn used to end by tombstoning its running lifecycle item, which threw away the only durable record of when the turn ended. Completed "Worked for" labels therefore depended on the renderer having observed the turn finish, and vanished on reopen. The lifecycle item is now revised in place, never tombstoned: - running, with startedAt, at the provider's turn start - completed or interrupted, with completedAt, at the provider's terminal frame, a user stop, or a child exit the host observed - unverifiable, with no end, when a cold acquire finds a running row from a generation whose exit nobody observed Both timestamps are the execution host's clock at receipt, captured before the deferred sink, so the completed value is identical on every client and needs no client clock. Codex history restore uses the provider's own second-granular endpoints for turns that predate this change. Desktop and mobile read settled durations off the journal through one shared selector, and anchor the live counter on the host start with the client's local receipt so a skewed client clock never leaks into the label. Locally observed durations remain the fallback for hosts that still tombstone. Timestamps live inside the existing turnLifecycle field, which old clients strip, and every working-state consumer keys on state === 'running', so no capability negotiation is needed. * native-chat: avoid stale working status on settled turns * test: align settled turn status expectations * Name settled lifecycle rows by their terminal state An interrupted or unverifiable turn must not read as completed for any consumer that renders status text raw. One shared helper builds the text for both providers from the lifecycle state. * test: deduplicate turn lifecycle suites Each behavior keeps one test; duplicated harnesses and restated cases go. * Key lifecycle rows to their user item and record the provider's measured duration A lifecycle row now names the user item that opened the turn by its provider key, so clients attribute timing explicitly and fall back to journal order only for rows from older hosts. A provider-initiated turn with no prompt can no longer claim the previous prompt's duration. When the provider measures the turn itself (Codex turn.durationMs, Claude result.duration_ms) the terminal row records it and clients prefer it over the host interval, so a turn shows the same number live and after a history restore. Host receipt times remain the live-counter anchor and the fallback. * Record a turn as a first-class journal item The turn record is now its own item kind rather than a status row carrying a lifecycle field: no text to misuse, and the fold matches the durable turn record other systems keep. Rows that carry it are stamped journal schema v3; every other row stays v2, so an older host keeps reading them and latches read-only at the first v3 row instead of truncating the epoch. Clients that predate the item would paint an unknown kind as a text bubble, so the host publishes the legacy status form to any client that does not advertise agent-session.turn-item.v1, through the same per-client seam background tasks use. The downgrade is transitional and goes once no supported release lacks the capability. The shared projection now renders unknown item kinds as nothing, so later kinds need no gate. One shared reader handles both forms for old journals and old hosts. * Preserve observed turn end across settlement retries * Retain turn attribution for loaded chat history * Preserve Codex exit receipt across close retries * Register completed turn duration reliability gate * Keep earlier turns through a Codex rewind and count a mid-turn attach from the real start Findings from an independent adversarial review of the typed turn record: - A Codex rewind adopted the provider's item list as the new epoch, and the provider never returns the host's own turn rows, so every duration before the rewind point vanished. The host's turn rows are now spliced back beside the item each followed, and recovery no longer expects the provider to prove rows it never owned. - The epoch row was stamped with the current schema version, so an older host latched read-only at row 1 of every new session, defeating the mixed version design. It carries no body and stays at v2; a stored-row test now reads SQLite directly, because the reader upcasts every row on read. - A send Codex folds into a running turn shares the opening prompt's provider key, and the alias map credited the duration to the later prompt. The earliest submission naming a key now wins. - The live counter anchored on first sight, so a client attaching mid-turn counted from zero. Published frames now carry the host's clock, the reducer keeps the last sample with its local receipt time, and both clients anchor on how long the host says the turn has run. * Correct turn duration gate assertion reference * Respect authoritative unknown native chat duration * Preserve unverifiable timing across older host upgrade * Record final completed turn duration reliability evidence * Fix the CI failures the merge left behind - A merged import list named the same module twice, which the native code quality plugin fails on. - A running turn is now reported by the host with no duration, so the settled map carries an explicit null for it; the hook test still expected the entry to be absent. - main gave the older-page action a cursor with a head-trim guard, so the retention test's epoch-only action no longer typechecks; it now passes an unbounded sequence, which is what the old shape meant. - The roster comparator moved into the extracted module, leaving its import unused in the reducer. * Split two files back under the line cap after the merge Merging main put both one effective line over 300, and the cap forbids a disable or a shave. The wire module's refusal vocabulary moves to its own file and is re-exported, so its consumers are untouched; the host's four thin mutation delegates move next to the functions they call. * Advertise the turn-item capability on every client transport Local IPC and mobile advertised it; the remote and web transports did not, so a desktop paired to a remote host, the CLI, and web silently ran on the legacy carrier forever and the canonical row was never exercised there. The renderer that paints it is the same build on every transport. * Update the web auth-frame expectation for the new capability --------- Co-authored-by: Merge Sim <sim@local> |
||
|
Brennan BensonandMerge Sim
|
ebb1acfa37 |
refactor(agent-status): publish structured sessions into the hook server store (#19683)
* refactor(agent-status): publish structured sessions into the hook server store Structured (native chat) sessions have no PTY and no hook script, so their status never reached the hook server's store; #19217 gave `worktree ps` its own adapter over the structured feed instead. The feed now writes every projection into that store through a status sink the runtime wires, drops the row when the host closes the session, and `worktree ps` reads the one snapshot like every other agent. Rows carry a `structuredHost` marker and the journal clock; they are never persisted to last-status.json, and the main process does not forward them to the renderer yet, whose feed bridge still owns them until it is retired. Design and the two follow-ups: docs/reference/agent-status-store.md. * chore: drop stray @pnpm/exe lockfile entry An unrelated local pnpm run added @pnpm/exe as a packageManagerDependency with no package.json change, so CI's --frozen-lockfile install failed before any job ran. * docs(agent-status): describe the step that actually landed The design record claimed PR 1 deletes RuntimeAgentRowStore, drops the retained-versus-hook reconciliation, stamps terminalHandle on OSC rows, and tags rows with a source field of 'structured-host'. None of that is true of the shipped code: the retained store and its reconciliation are still in place, and the row field is structuredHost: 'held' | 'owned'. AGENTS.md points every future contributor here before they touch agent status, so split the roadmap into the 1a that landed and the 1b that has not, and name the fields the code actually writes. * fix(agent-status): pair session removal with the status-row forget A session dropped from the host's map without an explicit forget left its row in the store forever: `structuredHostOwned` bypasses the staleness check, so a failed re-attach (the Claude rewind path reaches one) stranded a permanently working agent in `worktree ps` and on mobile with no UI able to clear it. Deletion and forget are now one operation both callers route through. * fix(agent-status): give orcad the store worktree ps reads from `orcad` constructed its runtime with neither `getAgentStatusSnapshot` nor `structuredAgentStatusSink`, so once `worktree ps` sourced rows only from that snapshot the headless host published nowhere and listed nothing. The hook server's store is a module singleton whose import tree never reaches Electron, and its file paths come from `start()`, which orcad never calls. * fix(agent-status): drop a structured row without a renderer clear `dropStructuredStatus` went through `clearPaneState`, which fans a pane clear out to the renderer for a pane key the renderer's own feed bridge still writes - so 'exactly one writer per pane key' held for writes and not for deletes. `dropStatusEntry` routes through the status-drop tap instead, and skips the resume-identity remnant: a structured session has no pane to resume into, and every null-status publish would otherwise re-mint one. * test(agent-status): pin both half-migration structured-row filters Neither the `agentStatus:getSnapshot` filter nor the main-window listener's had a single assertion, so deleting either — the first step of PR 2 — was green everywhere. Also covers the perf skip and the drop's lack of a renderer clear. * docs(agent-status): correct three statements this PR made false The sink JSDoc claimed only tests construct a host without one; `orcad` did. The doc argued a structured row needs no tab mirror 'because headless serve has no renderer', reasoning about exactly the topology the wiring had not reached. The deleted runtime adapter's warning that the pane key must be the DERIVED one - never a bearer handle or minted worker key - was lost with it. * test(agent-status): declare orcad in the hook-row producer census Wiring the hook store into the orcad runtime added a production site that hands hook rows to a consumer, which the census ratchet pins deliberately. --------- Co-authored-by: Merge Sim <sim@local> |
||
|
aeddfa463d |
perf(renderer): avoid per-second spinner animation events (#19407)
* perf(renderer): avoid per-second spinner animation events * fix(bench): ensure the Electron runtime before bench:spinners The script launches Electron via Playwright but skipped ensure:electron-runtime, which every other Electron-launching bench script runs first. * docs(renderer): scope spinner pixel-tolerance claim to paused-animation checks --------- Co-authored-by: m4air <m4air@m4airs-MacBook-Air.local> Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com> |
||
 Jinjing
|
9fed61e5c2 |
Persist agents sidebar search visibility as pairing-local preference (#19313)
* Persist agents sidebar search field visibility as pairing-local preferen - Add `agentsShowSearch` to workspace UI state with default on - Include in pairing-local fields so preference syncs across clients - Convert search from menu action to checkbox menu item for explicit toggle - Update activity thread options menu to reflect checkbox state - Add localization strings across all supported languages - Update RPC schemas and preference persistence layer - Includes readiness validation reports confirming feature is clean * rm review * fix documentation |
||
|
Jinwoo Hong
|
33af0af4ea |
fix(relay): stop rejecting the near region on a cold first probe (#19233)
* fix(relay): stop rejecting the near region on a cold first probe The region probe counted the process's first /health request, which pays TCP and TLS setup, as a latency sample. The resulting spread rejected the near region on essentially every cold run, leaving the far region as the sole survivor and pinning US desktops to Asia cells for 24 hours. Discard a warm-up probe per origin, compare regions by minimum latency, and keep the spread check only for a genuinely flapping path. A region now wins only against a measured competitor; a rejected or unmeasurable peer sends no hint, which is remembered for an hour so a reconnect does not re-probe. An origin that fails its warm-up is dropped before the sampling rounds, so an unreachable region costs one probe timeout instead of four. After a control socket registers, probe the cell we landed on once per process, and delete the cache only when it names a region other than the best measured one and that cell is more than 3x slower -- a far cell under a correct cache is the director declining the hint, and re-measuring would return the same answer. * test(relay): audit the region probe's global fetch call site |
||
|
Neil
|
314506003a |
fix: retain MSYS shell descendants in their terminal job (#19068)
* fix: retain MSYS shell descendants in their terminal job * test: complete MSYS regression CI registration and teardown contract * fix(windows): deny job breakaway for the whole Cygwin/MSYS shell family The per-PTY job probed only msys-2.0.dll, and only for bash.exe/sh.exe. Cygwin ships the same spawn.cc breakaway logic under cygwin1.dll, and an MSYS2 zsh escapes exactly like its bash does, so both kept the orphan bug. Probe the runtime DLL on the shell's own search path instead of matching shell names: that is the property that decides whether the runtime will ask for CREATE_BREAKAWAY_FROM_JOB, and it drops the name special-casing. * chore(patch): restore the conpty.cc index line The earlier hand-edit dropped it while every sibling section kept one. Recomputed against the real blobs: applying this patch to 7b286d3d yields exactly 4b06d185, so git apply -3 has its fallback back. |
||
|
Jinwoo Hong
|
d53cbed43f |
revert: hold mobile push feature for user testing (#19203)
Reverts
|
||
|
Jinwoo Hong
|
3160b54c69 |
feat: real background push notifications for the mobile app (#8129) (#18554)
* feat(cloud): add the mobile push gateway and its contract package (#8129) A small open-source service that holds the APNs key and FCM credentials and sends background push to paired phones on the desktop's behalf. Hosts authenticate with a box challenge and HMAC proof on their pairing key, the same shape the relay uses, so signed-in and accountless desktops share one path. Tokens are stored; alert text is held only for the coalescing window. The contract doc in docs/reference is the source of truth for every wire shape. The interop test runs the real desktop answerer against a real gateway-issued challenge so transcript drift fails in CI. * feat(push): register phones and send background push from the desktop (#8129) Adds the notifications.remote-push.v1 capability, the registerPush and unregisterPush RPCs on the mobile allowlist, a gateway client with a cached session and 401 re-auth, a durable unregister outbox, and a dispatcher that offers every mobile notification to the gateway after the socket fan-out. The dispatcher is fire-and-forget with one retry and drops registrations the gateway reports dead. Puts agentState on the mobile frame and fixes the #4375 wording so a working agent is never announced as finished. The relay host-proof code moves onto a shared envelope module with no behaviour change. * feat(mobile): background push registration, receive, and settings (#8129) Fetches the native APNs or FCM token, registers it with every paired host that advertises the capability, and re-registers on token change. Foreground pushes are suppressed inside handleNotification against the same seen set the socket path uses, so nothing shows twice. Taps route by host fingerprint. One Background notifications switch, off by default, with the disclaimer and needs-input / finished sub-switches; hidden until a paired desktop is new enough. Adds google-services.json and the expo-notifications plugin. * chore(cloud): Terraform and deploy workflow for the push gateway (#8129) Declares the Cloud Run service, runtime account, secrets, and orca_push database behind push_gateway_enabled, true only in production. The deploy workflow is gated like the relay's, deploys with no traffic, probes /ready and a validate-only FCM send, then shifts traffic. It runs as the shared production deploy account because the Cloud SQL rollout lease grant is foundation-owned; its extra authority is three bindings on the push service. docs/push-gateway.md carries the import commands for the resources created by hand and the APNs key rotation procedure. * docs: describe background notifications on the phone (#8129) * docs: check in the mobile push contract (#8129) Seven committed files cite it as the source of truth for every wire shape; docs/reference is allowlisted per file, so add the entry. * test(push): replay one checked-in host-proof vector on both sides (#8129) Cloud Verify installs only the cloud workspace, so the gateway suite cannot import the desktop answerer. Replace the cross-workspace import with a fixed challenge vector generated from the contract package; the gateway fixture and the desktop answerer each replay it and must produce the same HMAC. A transcript drift on either side now fails in that side's own suite. * fix(cloud): open the push gateway with invoker_iam_disabled, not an allUsers binding (#8129) The production domain-restricted-sharing policy rejects an allUsers run.invoker member, which the runbook anticipated. Opt the service out of invoker IAM the way the relay director already does; the host proof is the authentication either way. * docs(cloud): the push.onorca.dev record exists and is hand-managed (#8129) * fix(push): close review findings in the gateway (#8129) - Quota reservation takes a per-host advisory lock; READ COMMITTED admitted a whole burst past the cap (80/80 without, 60/80 with, against Postgres 16). - Challenge issuance no longer writes push_hosts; the row lands on proof verification. Stale hosts prune after 30 days. Per-IP token bucket on the two unauthenticated routes. - Streaming body limit via hono bodyLimit; a chunked body bypassed the Content-Length check. - registrationIds deduped in the schema; per-host device cap of 64; list bounded to its schema. - Gateway-side challenge TTL is the specified 10 s, not 40 s. - APNs stream settles on close as well as end/error. * fix(push): close review findings in the desktop client (#8129) - A gateway registration the registry cannot persist is enqueued for delete instead of leaking a live token. - Unregister outbox re-reads pending per pass, honours enqueues during a drain, and retries with backoff instead of waiting for the next launch. - Dispatcher batches registrations by 20 rather than starving the rest. - 401 compare-and-clear; a 401 after re-auth is unreachable; refused handshakes and 429s are cached briefly instead of re-handshaking per event. - Service is stopped on quit. * fix(mobile): close review findings in push registration and receive (#8129) - Consent generation guards a register that finishes after the switch went off; the host is re-queued for unregister instead of recorded live. - Foreground pushes seed the watermark before adopting the epoch, so a push on a never-connected session cannot wipe a valid watermark. - aps-environment follows the build via app.config.js; the iOS release workflow sets it to production. A bare plugin entry wrote development. - Pushes the OS showed while closed are marked seen before catch-up replay. - Token null result is not cached; failed capability probes are retried and never block an unregister; coalesced summaries are shown but not marked. - Unresolvable fingerprint routes nowhere and is suppressed in foreground. - Android channel ensured at boot; capability hook diffs clients by identity. * fix(cloud): harden the push deploy workflow and size the gateway to the budget (#8129) - Roll traffic back on a failed post-shift check; delete a candidate that never took traffic; retry the origin probe and the FCM probe. - Assert Terraform-owned scaling instead of mutating it from the workflow. - Build before taking the Cloud SQL rollout lease. - Declare the database pool in Terraform (2 per instance, max 2 instances) and add the gateway to the connection budget; the previous default put the shared instance 65 connections over its ceiling. - State plainly that the shared deploy identity's relay authority is inherited. * fix(push): read the runtime from shared state at push startup (#8129) Threading the runtime through launchDesktopMode put the launch module one line over the 300-line lint budget after the rebase. * fix(push): key the unauthenticated rate limit on the hop Cloud Run wrote (#8129) Cloud Run appends the connecting peer to x-forwarded-for; the limiter read the left-most value, which the caller controls, so a forged first hop earned a fresh bucket per request. * fix(push): close the final security review findings in the gateway and infra (#8129) - app.onError logs only the error name and answers a bare 500; hono's default handler printed the whole error, and a pg error carries the row in detail - a second per-IP bucket (240/min) runs ahead of the bearer lookup on every authenticated route, so forged bearers cannot spend the two-connection pool - one live session per host: minting deletes the host's earlier row - device-less hosts are pruned after 1 h, not 30 d; any keypair mints one free - notificationId is printable ASCII, since it becomes the APNs collapse header - the impersonated FCM probe token is masked in the workflow log - prevent_destroy on the Apple secrets and the orca_push database * fix(push): close the final security review findings in the desktop client (#8129) - fetch never follows a redirect: a 307 would replay the host proof and the phone's token to whatever origin the redirect named - registerPush params are strict and the paired identity is spread last - a per-device bucket (10/min) bounds a phone looping registerPush, which costs a gateway write and a synchronous registry write each time * fix(mobile): close the final security review findings in push receive (#8129) - a push with no epoch can no longer claim a seq-derived dedup key, in the foreground or from the tray; a forged seq:N could otherwise swallow the real bell at that seq - a provider-delivered push with no host catalog, or no fingerprint at all, stays unrouted instead of falling back to the hostId its raw data carries * docs(push): record the ip buckets, session and host retention, and the token-ownership limit (#8129) * fix(push): apply the schema on an untimed pool and retry statement-timeout aborts (#8129) Ports the relay's #18722 pattern to the gateway: DDL runs on a one-connection pool with statement_timeout 0 that is closed before the serving pool opens, and SQLSTATE 57014 joins the bounded transaction retry path. * fix: harden mobile push delivery and deployment recovery * feat: align mobile notification preferences with desktop delivery * fix: accept variable-length APNs device tokens * fix: deduplicate native APNs and background socket notifications |
||
|
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
|
fc5fa16870 |
perf(windows): split the process table into two flag sets (#17866)
* perf(windows): split the process table into two flag sets
MDE flags "suspicious memory activity" on the process-table reader: it
opened a handle into every process on the box and read each one's PEB on
a repeating cadence. Two changes narrow that.
Drop `Memory` outright. It cost a second
OpenProcess(PROCESS_QUERY_INFORMATION | PROCESS_VM_READ) plus
GetProcessMemoryInfo per process, and nothing reads a working set off
this table -- the Resource Manager runs its own sweep, and the addon
stores WorkingSetSize into a DWORD so anything above 4 GB wraps.
Split the rest in two. `readWindowsProcessIdentityTable[Fresh]` is a
bare Toolhelp32 walk with zero per-process handles, and returns
`WindowsProcessIdentityRow`, which has no `command` to read.
`readWindowsProcessTable[Fresh]` keeps the command line for the callers
that match on it. PTY root identity and the owner start-time probe move
to the cheap reader; agent recognition, port attribution, codex turn
processes and structured-TUI matching all genuinely need the command
line and stay.
Two independently single-flighted caches, never one per caller: the
fan-out this module prevents is one scan per caller, and each reader
still serves every caller wanting its flag set. The wedge gate and the
3s deadline stay shared, because both readers call the same addon and
one wedged read latches its one `requestInProgress`. With no binding
there is only the 1.4s PowerShell scan to run, so the identity view
rides the detailed snapshot rather than forking a second one.
Measured on Windows 11, 492 processes (p50/p95): identity 6.3/7.0 ms,
detailed 12.3/13.4 ms, previous memory+commandLine 13.1/14.1 ms.
* fix(windows): serialize native process-table reads across flag sets
The two flag-set readers could both be in flight at once, and the
vendored wrapper does not tolerate that. `getRawProcessList` pushes the
callback onto one list and calls the addon only when no request is in
progress, so a second concurrent caller's `flags` are DISCARDED and it
is handed the first caller's rows. Measured against the real addon:
identity issued first, both callers got the same array, 0 of 541 rows
with a command line. A detailed read overlapping an identity read
therefore returned a table with every command line empty, which agent
recognition reads as "no agent" -- silently, and only under concurrency.
Nothing already here excluded that. Each snapshot cache single-flights
only within itself, and the wedge set latches only after a read misses
its 3s deadline, so through the healthy ~12ms of a scan neither reader
excluded the other. Overlap is the normal state: panes poll detailed at
750ms while a teardown takes identity snapshots.
`nativeReadGate` admits one native read at a time across both flag sets.
It also fixes the relay path, where `adaptAddon` has no queue at all and
two simultaneous CreateToolhelp32Snapshot calls are the crash the
vendor's queue exists to prevent. Every link settles, so a wedged read
never strands a waiter; the waiter re-checks the wedge and rejects. With
one call outstanding, retention stays bounded at one callback rather
than one per reader.
Also from review:
- The CIM fallback now belongs to the detailed flag set alone, and the
identity view projects that snapshot through `toIdentityRow`, so an
identity row carries no command line on a no-binding host either.
- The concurrency test modelled the wrapper's coalescing queue, which
the previous synchronous mock could not express; verified failing
without the gate and passing with it.
- `agent-session-process-identity-probe` early-returns when the
creation-time flag is unavailable, which no shipped addon build
provides, instead of scanning the table to produce null.
- Corrected the cost framing: Memory took an OpenProcess(...|VM_READ)
it never read through, so dropping it halves per-process handle opens
and leaves the PEB/ReadProcessMemory telemetry unchanged.
* test(windows): keep read exclusion across resets and flag each field
Two review follow-ups, both about tests passing for the wrong reason.
`resetNativeReaderState` replaced the read gate with a resolved promise,
so waiters still holding the old chain ran beside reads queued on the
new one. Reachable only from the `__set*ForTests` hooks, which is what
makes it worth fixing: it hands a suite two concurrent calls into its
own mock addon -- the exact condition the concurrency tests exist to
detect. Chain onto the gate instead; every link settles within the
deadline, so the bounded wait that costs is the right trade.
The coalescing mock shaped every field off the CommandLine bit, so an
identity read that did request CreationTime got `creationTimeMs`
stripped. The identity-side assertion was then only `!('command' in
row)`, which a correctly flagged read and a coalesced one satisfy
equally: a future regression losing identity flags under concurrency
would have kept the case green. Gate each field on its own bit and
assert `creationTimeMs` positively, inside the helper both orderings
share.
Concurrency assertions move to a new bare-addon mock. The coalescing
mock's own latch means it can never report more than one call in
flight, so measuring exclusion there proved nothing; the bare addon has
no queue -- like `adaptAddon` on a relay, where re-entering
CreateToolhelp32Snapshot is a real crash -- and makes re-entry visible.
Verified by deletion: restoring `nativeReadGate = Promise.resolve()`
fails the reset case with `expected 2 to be 1`, and restoring the
single-bit mock fails both overlap orderings on `creationTimeMs`.
* docs(windows): count the third test defect in the list that names them
The section opened "Two defects have now shipped", numbered two, then
described the third in its closing paragraph -- a list that reads as a
complete account while quietly omitting one, which is the exact failure
the section exists to warn about. Say three and number it, and note that
the third arrived inside the fix for the first two.
Also record why the creationTimeMs and flags-array assertions are not
redundant, in the doc and beside the assertions: the flags array catches
a read served another flag set's rows, the positional creationTimeMs
check catches field shaping (identity dropping CreationTime, or
toIdentityRow not forwarding it). Neither sees the other's failure.
* docs(windows): stop describing a PEB read this release removed
Every comment here that justified the flag split in terms of PEB reads became
false when the command-line reader moved to the kernel. Left alone, the
enumeration doc contradicted itself inside one file: the flag-set section
described three chained `ReadProcessMemory` calls per process while the
sections below it explained that the addon contains no such primitive and has
no PEB fallback.
The measurement is now attributed rather than merged. Dropping `Memory` halved
the per-process handle opens and nothing else -- both handles carried
`PROCESS_VM_READ` at the time -- and it was replacing the PEB walk that took
`PROCESS_VM_READ` and `ReadProcessMemory` out of the addon. Neither change
substitutes for the other, which is worth keeping straight: the split's
remaining value is the handle itself, not the memory access.
Also adds `relay/windows-port-scan.ts` to the caller table, the one caller this
effort introduced, and records that it reads only pid/name through the detailed
reader -- free while a pane is polling, not free on a headless relay.
* test(windows): pin the fresh links path against the identity TTL cache
The identity and detailed tables are separate snapshot readers with
independent TTLs, so the detailed path's existing freshness guard says
nothing about the ancestry walk's. Cover the identity reader on its own.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
OrcaWinandOrca Worker
|
c252d855ac |
fix(windows): resolve npm/pnpm .cmd shims past cmd.exe (#17869)
* fix(windows): resolve npm/pnpm .cmd shims past cmd.exe
A `.cmd` target forces every spawn through `cmd.exe /c` with each argument
caret-escaped, and Microsoft Defender for Endpoint scores a long `cmd.exe /c`
line carrying caret-escaped natural language as obfuscation. `codex.cmd` is
named in the spawn cluster of the MDE incident this addresses.
npm's `cmd-shim` and pnpm's `@zkochan/cmd-shim` generate files whose whole body
is "find node, run this script". Read one, and the spawn can go straight to
`node.exe <script> <args>` — no cmd.exe, no caret escaping. Anything the parser
does not recognise exactly, or whose target cannot be confirmed on disk, keeps
the existing cmd.exe path.
Incidentally fixes a real bug: cmd ends its command at a raw CR/LF whatever the
quote state, so a multi-line agent prompt through a `.cmd` shim had to be
rejected. Resolved shims have no such limit.
* fix(windows): refuse drive-relative shim paths and run the win32 tests in CI
Two blocking findings from review.
A drive-relative path defeated the absolute-path guard:
`win32.isAbsolute('D:evil.js')` is false, but `win32.resolve` reads the drive
letter and lands on `D:\evil.js`, outside the shim directory. cmd would have
built `C:\shim\D:evil.js` and failed; we would have executed the wrong file.
Adding `:` to the unsafe-character set closes it, and the alternate-data-stream
spelling `a.js:zone` with it. It costs no coverage: 84 of the 91 real shims on
this box still resolve, the same seven fall back.
Neither `windows-cmd-shim-resolution.test.ts` nor its `.win32` sibling was in
the Windows package job's file list, so the whole filesystem/resolution half and
the real-spawn equivalence suite ran nowhere. Both are now in
`WINDOWS_PACKAGE_TESTS` and in the pr.yml step.
Also from review: clear `windowsVerbatimArguments` explicitly on the resolved
branch rather than inheriting it, since there is no caller-built command line
there; document the kill switch and the PTY/hook-wrapper scope limits in
docs/reference; and cover drive-relative, BOM, line-ending, casing and `%*`
tampering in the platform-independent half of the tests.
* docs(windows): justify the shim-path colon guard from the filesystem rule
The guard was argued empirically ("none of the 91 shims on this box has one"),
which invites a future reader to relax it for a shim we have not seen. Windows
reserves `:` within a path segment, so a relative path cannot carry one at all:
the only spellings that can are drive-qualified, an alternate data stream, or a
`\?\` device path, and the last is already refused as absolute. That makes a
false refusal impossible rather than unobserved.
* refactor(child-process): move resolveSpawn into its own module
The merge with main pushed run-process.ts one line past the 300-line cap:
both sides grew it. The spawn-argv decision is already a pure, separately
tested unit, so it moves out rather than the cap moving up. run-process.ts
re-exports it, so no caller changes.
* perf(child-process): cache the shim interpreter lookup
The parse cache spared the shim read but not the PATH walk, so a second
resolution of the same .cmd did 0 reads and one statSync per PATH entry --
30 on a 30-entry PATH, synchronous on resolveSpawn, where one dead network
mount blocks the calling thread on every spawn.
Keyed by shim directory AND PATH, since the shim's own rule is
%~dp0\node.exe first then PATH, and a PATH edit between spawns must miss.
Corrects the stat comment, which accounted only for the shim itself.
* fix(child-process): revalidate a cached shim interpreter before using it
The node cache was held for process life and never rechecked, so a cached
node.exe that was later uninstalled -- or dropped from PATH by a version
manager -- was still handed to resolveSpawn, failing the spawn with ENOENT.
An uncached process in the same state returns null and falls back to
cmd.exe successfully, so the cache was strictly worse than no cache.
One statSync on a non-null hit, not one per PATH entry, so the walk this
cache exists to skip is still skipped. The stale-null direction stays
uncorrected on purpose: it only keeps the working cmd.exe fallback. Both
directions are now stated in the comment, along with the known miss for
callers that vary PATH per spawn.
* fix(child-process): honour PATHEXT when resolving the shim interpreter
The doc claimed a node.com/.bat/.cmd on PATH returned null and fell back to
cmd.exe. The scan actually skipped those entries and kept looking for a
node.exe, so PATH=C:\A;C:\B with C:\A\node.com and C:\B\node.exe resolved to
B's node.exe while the shim runs A's node.com -- a different binary, chosen
silently, on the one axis this module must not get wrong.
The scan now follows cmd's rule: first PATH directory holding any PATHEXT
spelling wins, PATHEXT order decides within it, and only an .exe winner is
returned. Anything else gives up and keeps the cmd.exe path, which restores
the strict-subset-of-cmd property everywhere except the documented cwd case.
PATHEXT is read from the child's env and joined into the cache key, since it
now changes the answer. Costs one stat per PATHEXT entry per node-less
directory, paid once per process behind the cache.
---------
Co-authored-by: Orca Worker <orca-worker@localhost>
|
||
|
|
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
|
fba90e017c |
fix(windows): copy the daemon host exe verbatim instead of renaming it (MDE T1036) (#17865)
* docs(windows): document the EDR signal surface Six Microsoft Defender for Endpoint incidents fired against Orca 1.4.192 in eight days on one enterprise Windows 11 / Intune tenant. All six were behavioural process-tree scoring, not signature hits; two escalated to multi-stage incidents mapped to ATT&CK Execution and Collection. Add a reference doc mapping each attack-technique-shaped behaviour to the code that produces it and to why it exists: the renamed daemon image (T1036), the per-process PEB read, encoded policy-bypassed PowerShell (T1049), caret-escaped cmd.exe lines, and computer-use screen capture plus runtime-compiled MSIL (T1113). Records that signing is not the gate -- reputation is signer plus hash-keyed prevalence -- and carries the two evidence gaps the report noted. Adds an engineer checklist, deployment guidance for admins (AV path exclusions do not suppress EDR behavioural alerts; an MDE alert suppression rule does), and an explicit pre-deployment warning about computer use. * docs(windows): correct the PowerShell flag inventory and admin paths Review corrections to the EDR posture doc. The "encoded, policy-bypassing PowerShell" list conflated three different shapes and was incomplete. Split it into the three tiers an EDR actually scores differently -- bypass plus encoding, encoding alone, and bypass alone -- and add the sites it missed, including windows-mobile-firewall.ts, which encodes a script and launches it elevated through Start-Process -Verb RunAs. system-fonts.ts (-Command) and desktop-script-provider-bridge.ts (-File) were listed as encoded and are not. Notes that a raw grep under-reports, because the hook sites reach -EncodedCommand through wrapWindowsPowerShellEncodedCommand. Attribute the in-payload Set-ExecutionPolicy move to #16576 rather than to #16003's measurement, which keyed on -WindowStyle Hidden + -EncodedCommand, and record that the launcher's own tradeoff is unverified on a real box. Admin guidance was missing two ways a suppression rule pinned to one full path misses real activity: the .staging-<hex> sibling that exists mid-update, which is when the update-cluster incidents fire, and the userData fallback when LOCALAPPDATA is unset. Also: state the measurement conditions on the process-table timings, note that Hermes has surface even though we have no telemetry for it, note that the uninstaller names are electron-builder-generated and in no repo file, drop a volatile line count, and mark the per-operation computer-use shape as being addressed by an unmerged change. Drops the duplicated AGENTS.md section, keeping the indexed bullet. * docs(windows): reconcile the EDR posture doc with the shipped remediation Three claims in this doc became false once the rest of the Windows EDR set landed, and two told engineers the opposite of what the release does. The process-table section still described one shared snapshot taken with `Memory | CommandLine | CreationTime`, argued that splitting the cache per field set "would restore exactly the fan-out it exists to prevent", and concluded the shape was unfixable because "the information is only in the PEB". The split shipped (identity opens no handle at all), `Memory` is retired, and the command line now comes from the kernel through `ProcessCommandLineInformation` -- `ReadProcessMemory` is absent from the compiled addon and a ratchet asserts it against the import table. An engineer reading the old text would have concluded both fixes were dead ends. The PowerShell site inventories were stale in three of four lists: the port scan went native, every `-ExecutionPolicy Bypass` + `-EncodedCommand` pair was dropped as a measured no-op, and of the unencoded-bypass list only `wsl-cli-scripts.ts` survives. Regenerated against the merged tree, including the sites that reach the flag through `wrapWindowsPowerShellEncodedCommand` and never spell it, which a raw `rg` misses. Incident-evidence sections are left alone: they record what the tenant observed on 1.4.192, not what the code does now. * fix(windows): copy the daemon host exe verbatim instead of renaming it Microsoft Defender for Endpoint flagged `orca-terminal-daemon.exe` as MITRE T1036 (Masquerading): Orca copied its own `Orca.exe` into %LOCALAPPDATA% under a different name, specifically so the NSIS updater's `taskkill /IM Orca.exe` could not match, then ran it detached. Because that process is what every other flagged action was attributed to, the name mismatch acted as a reputation multiplier on unrelated findings. The rename was never what made the daemon survive. In app-builder-lib 26.15.3 the installer's FIND_PROCESS/KILL_PROCESS select processes whose image path is under $INSTDIR; `taskkill /IM` is only the fallback for hosts where PowerShell is missing or blocked. Survival is a property of the path, and %LOCALAPPDATA%\Orca\daemon-host is outside $INSTDIR whatever the file is called. Derive the host exe name from process.execPath so the copy is byte-for-byte, name included — it keeps its Authenticode signature and carries no renamed-image signal. On the no-PowerShell fallback the daemon is now killed with the app and terminals cold-restore, which is the documented pre-relocation outcome the update harness already asserts, not a regression. The uninstall macro no longer needs a distinct name to find the daemon; it kills the app's own image name (plus the legacy name, for hosts left by older builds). Adds docs/reference/windows-daemon-host-relocation.md with the survival contract, the rejected alternatives and their measured costs, and the invariants to keep. * fix(windows): apply daemon-host relocation review corrections Scope the uninstall taskkill to the current user with `/FI "USERNAME eq %USERNAME%"` via cmd.exe, matching upstream's per-user KILL_PROCESS — without it an elevated machine-wide uninstall reaches another logged-on user's session, so the "no collateral" claim in the comment was overstated. Comment the rmSync-before-publish: Windows refuses to delete a running image, so a live daemon already hosted in this version's dir (same-version reinstall, or a dev channel reusing a version) throws and materialization fails open. Doc corrections: - The fallback selector is the full per-user `taskkill /F /IM "<app>.exe" /FI "PID ne $pid" /FI "USERNAME eq %USERNAME%"`, not a bare `taskkill /IM`. - The probe reads `Get-ExecutionPolicy -Scope Process`, not the effective policy, and GPO writes MachinePolicy/UserPolicy — so GPO-managed hosts take the primary path-scoped branch. Narrow the fallback triggers accordingly. - Drop the Authenticode sentence: the old name was equally byte-identical and equally signed, so a filename has no bearing on signature validity. - Name the new update-abort path: the daemon now matches FIND_PROCESS, so on the fallback branch an unkillable host reaches the retry loop's MessageBox /SD IDCANCEL and Quits, aborting a silent update. - Correct the customCheckAppRunning rejection. It is ~6 lines, not a rewrite; it is wrong because forcing the PowerShell branch where PowerShell is absent makes FIND/KILL silently no-op and leaves the real app running with files in use. - Bound the win honestly: OriginalFilename is empty on the shipped binary, so the strongest T1036 indicator never fired, and the residual copy-and-run-detached shape still maps to T1036.005. Reconcile docs/reference/windows-edr-posture.md, which documents the rename as a live finding and would otherwise contradict this change. Content-only edit: markdown under docs/reference/ is not oxfmt-formatted as a matter of practice and nothing in CI gates it, so the file is left consistent with its neighbours. * fix(windows): expand USERNAME in NSIS instead of spawning cmd.exe The uninstall macro routed both taskkills through `"$SYSDIR\cmd.exe" /C` purely so `%USERNAME%` would expand — two extra interpreter spawns on the uninstall path, in a change whose whole point is not adding scored behaviour, and the exact `cmd.exe /c` shape the new AGENTS.md EDR bullet warns about. NSIS reads the variable itself with ReadEnvStr, so the spawns buy nothing. Verified on Windows 11 that the generated command line does what the filter is there for: a copy of cmd.exe running as orca-nonexistent-probe.exe (pid 34244) was terminated by `taskkill /F /IM "orca-nonexistent-probe.exe" /FI "USERNAME eq <user>"` — SUCCESS, exit 0, process gone. Guarded on an empty USERNAME because the degenerate case is silent: taskkill rejects an empty filter value outright ("The search filter cannot be recognized") and kills nothing, which would leave exactly the orphaned daemon this macro exists to reap. `*` is rejected as a filter value too, so there is no branchless spelling. With no USERNAME to scope by it kills unfiltered, as the macro did before the filter was added. Stack stays balanced: three pushes, two nsExec pops, three restores. Also strike the last stale row in windows-edr-posture.md's remediation table. "Copying our own image under a different name" read as outstanding work; it is done by this change, so the row now points at the relocation doc. Same class of staleness as the section reconciled in the previous commit, and git would not have flagged it either. * fix(windows): port the daemon-host uninstall sweep into the live NSIS include The uninstall macro this branch rewrote lived in config/nsis/daemon-host-uninstall.nsh, which main no longer includes: #17906 consolidated every Windows installer hook into config/nsis/orca-installer-hooks.nsh because electron-builder accepts exactly one `nsis.include`. Merged as-is, the rewritten macro would have been dead code while the shipped uninstaller kept running main's stale sweep — `taskkill /F /IM orca-terminal-daemon.exe`, which matches nothing now that the relocated host is a verbatim Orca.exe copy. The RMDir that follows then cannot delete the running image, so a live orphaned daemon and its ~224 MB tree would survive every uninstall. Ported into the live include: the ${APP_EXECUTABLE_FILENAME} kill, the USERNAME filter that keeps an elevated machine-wide uninstall out of another logged-on user's session, and the register save/restore around both. The legacy orca-terminal-daemon.exe kill stays so hosts left by older builds are still reaped. The ratchet that was meant to catch exactly this pinned only the legacy image name, which main's stale macro already satisfied, so it passed both ways. It now asserts the app-exe kill and the USERNAME filter, against comment-stripped script — the prose above the macro names both image names, so a toContain over the raw file proves nothing. --------- Co-authored-by: Orca Worker <orca-worker@localhost> |
||
|
Neil
|
6031c19e9f |
ci: reduce dependency, checkout, and test deadline overhead (#18968)
* ci: reduce dependency, checkout, and test deadline overhead * ci: avoid generic E2E jobs for native-only IME changes |
||
|
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> |