* 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>
* 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
* 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.
* 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>
* fix(terminal): retire captured remote handles when pending panes close
A restored pane can hold a scoped `remote:<environment>@@<handle>` layout
binding while `remote.attach()` is still waiting for `terminal.resolvePane`.
The transport's `getPtyId()` is null, so an explicit split close passed null to
`closeWebRuntimeTerminal`, dropped the binding and destroyed only the viewer.
The host terminal stayed connected.
Only an exact scoped handle whose environment matches the owning workspace's
runtime authorizes the close. The provider helper captures the pairing
revision, runs its existing compatibility check, then rechecks pairing and
ownership immediately before dispatch.
Rebased onto main after #21001 was squash-merged. The previous head was a merge
commit that carried its own conflict-resolution content -- the runtime branch in
`terminal-pane-close-admission.ts` and the restored `it.each([false, true])`
parameter -- which a plain rebase drops along with the merge. Rebuilt from the
recorded net diff instead and verified byte-identical at 15 files,
906 insertions, 41 deletions.
* test(memory): rebase the pending runtime-close proof onto the squashed base
`fix.patch` recorded a baseline taken against #21001's pre-squash branch tip.
Squash-merging #21001 replaced that tip with a single commit, so the recorded
hunks no longer reverse-applied and `reproduce.mjs` aborted with
`Source changed: use-terminal-pane-close-actions.ts` -- confirmed by running it
before regenerating rather than assuming the rebase alone would fix it.
Regenerated against `main` and re-run: 5 pass / 10 fail before, 15 pass / 0
fail after, exit 0, and every `results.json` hash recomputed from the run
rather than hand-edited.
---------
Co-authored-by: m4air <m4air@Mac.localdomain>
Co-authored-by: Neil <neil@stably.ai>
* fix: avoid retaining foreign SSH file frames before metadata
* test(ssh): exercise empty metadata through the streaming mux fixture
* fix(ssh): fail the file read when beforeResolve never runs
Moving the metadata install from .then() to beforeResolve moved it from a
mandatory callback to an optional one, and handleResponse clears the request
timer before beforeResolve runs. That left "response fulfilled, metadata never
installed" with no deadline: the read never settled, holding its notification
and dispose closures until mux disposal. Before this PR the same state failed
after the 60s inactivity deadline.
Unreachable with the concrete mux, which calls resolve on the line after
beforeResolve, but the hook is optional in the type and nothing enforces the
pairing. The guard is a no-op on every real path: empty, missing streamId,
cap-exceeded and alloc-failure all settle first, and the success path sets
metadataReady.
Found during review of #21167; raised at
https://github.com/stablyai/orca/pull/21167#issuecomment-5726058832
---------
Co-authored-by: m4air <m4air@Mac.localdomain>
Co-authored-by: Claude <noreply@anthropic.com>
* fix(terminal): retire explicitly closed pending split connections
* test(memory): keep pending split proof compatible with formatted source
* fix(terminal): confirm pending split retirement before stopping work
* fix(terminal): restore the pending split-close gates CI checks
Three CI gates were red on this branch and all three were this branch's own.
The hook-order parity snapshot did not count the `confirmedCloseRef` this
branch adds to `use-terminal-pane-close-actions.ts`. Dumping the flattened
order against clean `main` shows exactly one added `useRef` at position 148
and no reordering, so the count moves 211 -> 212 and the digest with it.
`pending-split-close-test-fixture.ts` is Vitest support code, but it sits
outside the `*.test` / `*.spec` / `tests` globs that already switch
`anti-slop/no-module-mocking` off, so the gate failed on all twelve of its
`vi.mock` calls. It carries a file-scoped disable with the reason, matching
`work-item-search-test-harness.ts`.
`fix.patch` still described the pre-confirmation shape of the close hook, so
`reproduce.mjs` aborted with `Source changed` and the cited ablation could not
run at this head. Regenerated against the committed sources; the harness again
reports 10 pass / 14 fail before and 24 pass / 0 fail after.
Merges `main` rather than rebasing: #21005 is stacked on this branch.
---------
Co-authored-by: m4air <m4air@Mac.localdomain>
Co-authored-by: Neil <neil@stably.ai>
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.
* fix(chat): release provider children after lost resume holds
* test: load audit fixtures as modules and verify combined mobile payload
---------
Co-authored-by: m4air <m4air@Mac.localdomain>
* 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.
* 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 77b154d5dd, so a desktop old enough to refuse
the probe also omits the offer block, and that flow commits a LAN host without
ever probing. The site that reached is `upgradeDirectMobileRelay`, which
re-probes every LAN-only host on reconnect: the refusal threw into the
controller's swallowing catch, so the write-once journal it had just written —
and the pending resume secret in it — was never retired.
Also drop two overclaims: the phone's Files and Git fallbacks have read both
codes since they shipped, so this is settled practice rather than a new rule,
and the reason the allowlisted-but-unregistered case cannot ship is
mobile-rpc-allowlist.test.ts, not a convention about what lands together.
* 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 adeb5f9531 recorded the six moved goldens into a scratch directory and
copied them back, which left them pinned to `e7206f62`, a tree that no longer
produces them. That is the one claim the `baseline` header exists to make, so
this replaces it with the README's remedy done in full.
`baseline` is now f741b2ea82, the last commit on
this branch that touches a fenced path, so the recording fence passes in place
and every golden is pinned to the tree that produced it. All 667 were
re-recorded through `scripts/rpc-recording.mts --record`; none were hand-edited.
Decoding every value pool against the branch point b8d4cde09f sorts the corpus
into 661 header-only moves where `baseline` is the only key that moved, 6 whose
body moved as well, 0 added and 0 deleted. The 6 are the disclosed step-7 delta,
unchanged at 69 moved observation fields across malformed reply partitions, plus
the readable incompatible-reply copy from f741b2ea82. No `normal` partition and
no named-scenario golden moved.
`scenarioSha256` hashes the derived scenarios, not the manifest, so the repin
moves no other header key; the README section this adds records that, the
scratch-copy failure mode, and the follow-up repin main needs after a squash
merge.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): narrow the incompatible-reply error by instanceof, not by cast
The two new tests in f741b2ea82 read the error through `as` casts, which the
changed-code casting gate rejects. An `instanceof` guard narrows the same value
and checks the class at the same time.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the branch tip and re-record
71d8c6a1e2 touched a fenced path (`mobile/src`), so the pin from 5f3f184fdf no
longer named the tree that produces these goldens. The fence compares the whole
of `mobile/src`, and a test file is inside it, so the pin follows the last commit
that touches a fenced path rather than the commit whose behaviour moved.
Re-recorded all 667 in place through `scripts/rpc-recording.mts --record`.
Decoding every value pool against the branch point b8d4cde09f still gives 661
header-only moves with `baseline` the only moved key, 6 body moves, 0 added and
0 deleted; the six and their 69 moved observation fields are unchanged.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): record the four source-control reads that had no oracle
git.status (host payload), git.branchCompare, git.commitCompare and
git.branchDiff were migrated to checked readers with no recording observing
them, so a required member a host omits would have surfaced only in production.
Three families mount the owners rather than the senders, because each reply is
only visible in what the owner then publishes: the Changes screen's loader hook
(git.status, and the base-ref chain and git.branchCompare it triggers), the
history list screen (git.history and the per-commit git.commitCompare), and the
committed-diff opener hook (git.branchDiff). Ten goldens: three pilot recordings
and seven reply matrices.
Two adapter capabilities this needed. An inert FlatList never calls `renderItem`,
so the history adapter renders one row through the screen's own callback, both to
reach the handler that expands a commit and to read the file list back; without
that the commit-compare reply changes nothing observable. And `lowlight` joins
`react` and `zod` as a real library rather than a refusing proxy, because the
branch diff highlights on its success arm before the preview reaches state, so
the shipped text arm was otherwise unrecordable. No golden recorded its absence,
so only `recorderSha256` moves.
Recording the same scenarios against 4b0009d414, the pre-refactor tree, is the
before column. Decoding every value pool across the two gives 11 body moves and
666 header-only, 0 added, 0 deleted: the 6 already disclosed, plus the 5 new
matrices at 63 moved observation fields. What moved is the point. A malformed
git.status used to leave Changes `ready` over the malformed payload and go on to
fetch a branch compare; it now says the host sent a reply it could not read. An
absent git.branchDiff result used to put "Cannot read properties of undefined
(reading 'kind')" on the screen. An unreadable git.commitCompare used to spin the
expanded commit forever; it now says "No file changes".
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the merge commit and re-record
The merge is the last commit touching a fenced path, so it is the only tree
the recorder's fence can match. Every golden moves `baseline` and picks up
main's `recorderSha256` from #20920; the six the checked readers changed are
the only bodies that move against main.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* docs(mobile): note the merge-commit pin and unwrap the recipe's record command
`format:check` from `mobile/` caught the wrapped inline command the recipe
had been carrying since it landed; pointing at the command above removes the
duplicate and the wrap together.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile): open the source-control reply enums so a newer host's arm degrades
A closed `z.enum` in a reply schema is a version claim, and it refused replies
every declared reader could have rendered: a `git.branchCompare` summary status
of 'shallow-base' failed the whole Changes compare, a 'codeberg' provider failed
the whole eligibility, and a 'typechange' entry status dropped the row. Main
passed all three through.
`openEnum` in zod-salvage declares the arm set open: an unrecognised arm reads as
a member the consumers already handle, while absence and a non-string stay fatal.
Not `.catch()`, which would swallow those two as well.
`area` stays closed and says why: every arm grants stage, unstage or commit, so
there is no member to degrade to that would not offer an action against a row
this build cannot place. Main rendered such a row in no section either.
Also drops two claims the code does not back. Nothing reads the salvage report,
so the two comments promising a dropped entry "arrives as salvage.droppedPaths"
are gone, and `hostKind` on the non-text diff arm had no reader.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* docs: write down the open-enum rule and the header keys a branch moves
Rule 4 in the wire-compatibility page, beside the three rules it belongs with:
an enum arm set is a wire surface, unknown arms degrade rather than reject, and
leaving one closed is a decision to state where the schema is declared.
The recorder recipe's step 4 said `baseline` would be the only moved header key,
which is only true of a branch that never touched the recorder. It now names the
three digests a branch's own edits move, so a reader recognises a clean result.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile): stop the recorder's own timeout killing a full re-record
The corpus records in ~110s warm and 160s under load, against a 120s budget, so
a full re-record was killed roughly half the time. A killed run wrote a partial
reporter banner and exited 1, which reads as a failing scenario rather than as a
run that never finished — it cost two investigations here. The budget is now ten
minutes, and a killed run says so.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the open-enum commit and re-record
`baseline` is the only header key that moves and no golden body moves: no matrix
partition scripts an unknown enum arm, so the corpus cannot see this change. The
eight schema unit tests are its only oracle.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile): stop an unresolvable eligibility claiming the branch is not ready
Both fallback prefills set `canCreate: false`, which is a determination nobody
made. It short-circuits getMobilePrCreateBlockMessage before reviewLookupOutcome
is read, so a malformed, refused or rejected eligibility told the user "This
branch is not ready for a pull request yet." instead of asking them to retry.
Dropping it leaves `canCreate` undefined, which is what "unproven" means here.
Only a host that determined `canCreate: false` still gets the blocked copy.
`area` now degrades to absent rather than staying closed. Dropping the row also
dropped it from the unresolved-conflict gate, which grants create on a conflicted
worktree; absent withholds stage, unstage and commit while keeping the row, since
every area reader is an equality check. Its four consumers narrow explicitly: the
diff-review queue filters unplaceable rows, the opener withholds the route, and
the commit-failure prompt pins 'staged' where its own filter already did.
`git.branchCompare` entries are nullish, matching the `?? []` its consumers use.
Deletions: `MobileGitStatusProjection` and `uncheckedReaderCount` lose `export`,
the boundary test drops its dead inventory self-file (the AST counter finds zero
calls there, only prose), and `isRpcIncompatibleReplyError` is gone — it had no
caller in mobile, desktop or e2e.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* style(mobile): formatting and a thrown rejection in the round-2 tests
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the round-2 tip and re-record
The round-2 eligibility fix is a behaviour change, so the corpus has to be
re-recorded at a pin that includes it. Four goldens move body: the two
create-intent eligibility matrices on every non-normal partition, and the two
prefill scenarios that lose the fallback's `canCreate: false`.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the main merge and re-record
The merge is now the last commit touching a fenced path, so the corpus has to
carry its sha. No body moves against the pre-merge corpus: main's engine change
shifts `recorderSha256` on every golden and nothing else, and main's fifteen
step-6 goldens re-record byte-identical.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): admit the three unchecked readers #20954 landed
The ratchet is a ceiling against this branch adding readers, not a claim about
what main may land. #20954 brought `notification-stream-closed`,
`native-chat-session-page` and `terminal-buffer-cleared`, so the merge has to
raise those lines and say where they came from.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the recording baseline to the inventory commit and re-record
The ratchet inventory is a fenced path, so admitting #20954's three readers
moved the fence head again. Baseline only; no body moves against the merge
re-record.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile): send the host's own provider token back instead of a fallback
`provider` is not a member mobile only reads. The eligibility reply names it and
the create call returns it, so `openEnum(..., 'unsupported')` did not soften a
reading — it rewrote the bytes, and a host that had just named `codeberg` refused
its own provider as unsupported. The action-sheet Create path has no provider
gate, so nothing caught it.
Passes the token through as a string from the reply to the create params. The
allow-list that decides whether mobile may create stays supportsHostedReviewCreation(),
which already answers no for a token this build does not know; its parameter
widens to `string`, since answering for an unknown token is the whole job. The
worktree-link switch gains a default, which also fixes an older hole: an
unrecognised provider used to fall out of the switch as `undefined` params.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): pin the provider pass-through in the corpus
Repins to the provider fix and records `sc-create-intent-unlisted-provider`,
whose eligibility reply names `codeberg` and whose recorded `hostedReview.create`
params carry it back unchanged. Restoring the old enum fallback fails that
golden on `Request params mismatch: hostedReview.create#1` and nothing else.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
Neil
OrcaWin
github-actions[bot]
Jinwoo Hong