mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 00:03:52 +00:00
924a54167cd85e30a40be1a0510efd556eb82d3a
6792
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Neil
|
924a54167c |
fix(ssh): stop refusing hosts that ssh itself connects to
Two product decisions, both taken deliberately after a review priced their blast radius, and both moving us from stricter-than-ssh to matching it. CERTIFICATE-AUTHORITY HOSTS NO LONGER FAIL. The point of an SSH CA is that the client holds ONE line — very often `@cert-authority *` — instead of per-host entries. That line matches every candidate, so for a Teleport / Vault-SSH / Smallstep / in-house-CA user EVERY target failed, not just CA-signed ones, including on-demand runtime VMs, and StrictHostKeyChecking=no did not help. The documented escape was an environment variable, which an Electron app launched from the Dock or Start Menu never sees. Meanwhile OpenSSH, verified live, treats a CA-covered host presenting a plain key as first contact and connects: ssh2 cannot validate certificates at all, so refusing bought nothing ssh was not already giving up. The residual risk is real and accepted — for a CA-protected host we take a plain key we cannot tie to the CA — and the ca-only outcome is carried through the decision so it stays visible in the log. AN UNREADABLE known_hosts NO LONGER REFUSES EVERYTHING. Any non-ENOENT read error on any configured file rejected every unknown host, with a message blaming the system SSH configuration, which was not what happened. The common trigger is not exotic: a Windows OneDrive Known Folder Move placeholder while offline fails with a cloud-file error, not ENOENT. It was also asymmetric with our own store, which degrades an unreadable file to "nothing trusted" and connects. We now connect as ssh does — it warns and treats the host as unknown — but record NOTHING, so a first contact we could not check never becomes durable trust. That second half is the reason the first is acceptable, so it is pinned end to end with the store actually bound. Which meant splitting verificationSourcesIncomplete back apart. It had been one flag for two claims that now diverge: "a site policy may exist that we cannot read" still refuses, "a file we could not open may contradict this" does not. Merging them was what made the second inherit a strictness only the first justified. Both still lose to evidence we DID read: a mismatch, a revoked key, or an explicit StrictHostKeyChecking still refuse in either state. pnpm lint clean; 1,549 SSH tests pass. |
||
|
Neil
|
1c92304690 |
fix(ssh): only claim the site config was suppressed when ssh -G actually ran
sshGArgsForHost reports which arguments WOULD be used, not what happened. It returns the -F form whenever ~/.ssh/config exists and os.homedir() diverges from the passwd home, so a machine with no usable ssh at all — Windows without OpenSSH, a restricted sandbox, a timed-out probe — was judged by whether it happens to have a ~/.ssh/config, and rejected every unknown host permanently if it did. The same broken machine WITHOUT one stayed fully permissive, which is the tell: the flag is a claim about a config file we could not read, and when ssh never ran there is no such claim to make. Narrow but total where it lands: anything that sets HOME explicitly (wrapper scripts, sudo -E, devcontainers), macOS mobile and network accounts, and the E2E isolation this branch was written for. Found by an availability review. |
||
|
Neil
|
d84645f818 |
fix(ssh): look a HostKeyAlias up the way ssh does — without the port
HostKeyAlias suppresses the port entirely. Verified against OpenSSH 10.2p1 on port 2225 with HostKeyAlias=myalias: an entry keyed `myalias` authenticates, and one keyed `[myalias]:2225` gives "No ED25519 host key is known for myalias". We built [['[alias]:port'], ['alias']] and consulted a form ssh never writes. On its own that was a stale-entry false alarm. The previous commit made it worse: now that the first pass decides as soon as it finds any entry for the host, a leftover `[alias]:2225` line STOPS the bare lookup ssh actually performs — so the one population D2 cites HostKeyAlias for, bastions tunnelled through localhost:port, would get a hard failure on a host ssh connects to. So resolveKnownHostsLookupHost reports whether the name came from the alias, not just what it is, and that flag reaches both the matcher and the algorithm ordering. Returning the name alone is what made the bug invisible: the caller had no way to know it was holding something that must not be bracketed. Found by a parity review. 1,543 SSH tests pass. |
||
|
Neil
|
b54d078465 |
fix(ssh): read ssh2's host key algorithm list instead of copying it
ssh2 builds DEFAULT_SERVER_HOST_KEY at load time and prepends ssh-ed25519 only when a RUNTIME PROBE succeeds — it signs and verifies with a fixed Ed25519 key. On a build where that probe fails, ssh-ed25519 is absent from ssh2's SUPPORTED list too, and generateAlgorithmList throws `Unsupported algorithm: ssh-ed25519` from inside client.connect. That throw matches no retry classifier and no transport-fallback classifier, so it is permanent — and because we only set `algorithms` for hosts we already know, it would fire on trusted hosts while new ones kept working. A copied list cannot be merely stale here; it can be wrong. So it is read from ssh2 now, which also removes the drift risk the previous commit could only report. ssh2 is external in the main bundle and the bundle is CJS, so the deep path resolves at runtime from packaged node_modules. The copy stays as a fallback in case a future ssh2 moves the file — losing the proposal order degrades the type-scoping guarantee, but refusing to connect at all is worse. The test that used to pin the copy against ssh2 now pins the fallback, which is the only part that can still drift. Found by an availability review. pnpm lint clean; 1,537 SSH tests pass. |
||
|
Neil
|
38a1a7858a |
fix(ssh): name a ssh-keygen -R target that actually removes the entry
Verified against OpenSSH 10.2p1: with both `[h.example]:2222` and `h.example` on file, `ssh-keygen -R h.example` removes only the bare line and leaves the bracketed one — and there is no port flag, `-R host -p 2222` is "Too many arguments". An off-port target is keyed `[host]:port` in known_hosts, so the command we printed removed nothing: the user runs it, reconnects, and meets the identical failure with no indication of why. The message now names the bracketed form, quoted because the brackets are shell glob characters, whenever the port is not 22. Found by a parity review. |
||
|
Neil
|
1236ba11fb |
fix(ssh): refuse the known_hosts lines ssh itself refuses to parse
Three ways a line could be trusted by us and invisible to the user's own ssh — or, worse, raise a CHANGED alarm from an entry ssh drops. Buffer.from does not fail on bad base64, it SKIPS invalid characters, so `<key>!!!` and a blob with `@@` spliced into it both decoded to the correct key and matched. Verified live against OpenSSH 10.2p1 on 127.0.0.1:2224: the unmodified control reached authentication and all three malformed variants produced "No ED25519 host key is known". A re-encode-and-compare makes us agree. `<key>AAAA` is the interesting one, and the reason the first fix was not enough: 68 characters plus 4 is still legal base64, and the algorithm header still reads ssh-ed25519, so neither the base64 check nor the existing header check sees anything wrong. ssh parses the whole key structure. We decoded 54 bytes where an ed25519 key is 51 and reported `mismatch` — a man-in-the-middle warning caused by a typo in a file ssh silently ignores. So the blob is now walked as what it is: a run of length-prefixed fields that must consume it exactly. Algorithm-agnostic on purpose, so a key type we do not model is checked as well as one we do. It also rejects a length prefix that overruns the buffer, which readHostKeyType only checked for the first field. And ssh's extract_salt demands exactly one SHA1 digest — "expected salt len 20, got 16" — where we accepted any non-empty salt. A short salt is still a usable HMAC key for us, so a hand-crafted line could match for us and be a parse error for ssh. ssh-keygen -H always writes 20 bytes, so refusing loses no real entry. Worth recording that ssh-keygen -F cannot answer any of this: it matches host names and prints lines without ever decoding the key, so it reports "found" for all four blobs. The real client was the only instrument that worked. Found by a parity review; the base64 finding as reported was right about the behaviour and wrong about the mechanism for the padded case, which is what led to the structural check. 1,533 SSH tests pass. |
||
|
Neil
|
ef08403f80 |
fix(ssh): stop the fallback pass accepting a changed key, and refusing a new one
One wrong loop scope, two opposite errors, both reproduced against a live OpenSSH 10.2p1 client and an ed25519-only sshd on 127.0.0.1:2223. ACCEPTING A CHANGED KEY. ssh runs the bare-host fallback only when the port-qualified lookup matched no plain entry of ANY key type. We ran it unless pass 0 produced a match or a SAME-TYPE mismatch. So with an off-port RSA entry plus a bare, correct ed25519 line — an ordinary shape, an old off-port entry beside one written by a port-22 connect — ssh printed IDENTIFICATION HAS CHANGED and refused, with no "checking without port identifier" in -v because the fallback never ran, while we reached the bare line and returned `match`. REFUSING A NEW ONE. sawKnownHostOtherType and sawCertAuthority were declared outside the pass loop, so an entry found only on the fallback pass could set them. A bare ssh-rsa entry, dialed on a non-default port against an ed25519-only server, made ssh add the host and connect — plain first contact — where we returned unknown-type-known-host and hard-failed. That is Gitea/Forgejo, dev containers, Gerrit, Vagrant: an off-port service on a host already in known_hosts. So the flags are per-pass now, and pass 0 decides as soon as it finds any plain entry for the host. Which of the two rejections it reports only picks the message; ssh calls both HOST_CHANGED. Also drops the type check from the match test: byte equality already implies the types agree, because the blob carries its own algorithm name and parsing rejects any line whose declared type disagrees with it. Reverting either half of the scope fix fails exactly the two new tests. 1,526 SSH tests pass. |
||
|
Neil
|
b48c467978 |
fix(ssh): honour StrictHostKeyChecking, which had never once been read correctly
`ssh -G` does not echo the value the user wrote. StrictHostKeyChecking is rendered through fmt_multistate_int, which prints the first entry of multistate_strict_hostkey, and that table lists true/false before yes/no: yes -> true | no -> false | off -> false | accept-new -> accept-new | ask -> ask Verified against OpenSSH 10.2p1 from both a config file and -o. Not 10.2-specific; the table ordering is old. The decision function tested only 'yes'/'always' and 'no'/'off' — spellings that cannot arrive. So `StrictHostKeyChecking yes` fell through to the default branch and we accepted AND PERSISTED a host the user's config explicitly says to refuse. That is the worst outcome this feature can produce, and it was the behaviour for every strict user from the first commit. `no`/`off` landed there too, breaking the documented "lax settings never persist" invariant. Every unit test passed throughout, because they fed the function 'yes' — the value a human writes, not the one that reaches the code. My ssh -G parity test did capture real output, but I happened to configure accept-new, one of only two values that round-trip unchanged. The new table is keyed on configured value -> what ssh -G actually prints, and asserts both reach the same verdict, so the question "is this the spelling that arrives?" cannot be assumed again. Found by a parity review against a live OpenSSH client. |
||
|
Neil
|
2a9b57cf4c |
style(ssh): satisfy the code-quality lints in the two new test files
A string concatenation that should be a template literal, and an inline import() type annotation that should be a type-only namespace import — erased before vi.mock's hoisted factory runs, so the mock is unaffected. pnpm lint is clean. |
||
|
Neil
|
56cc790c7d |
test(ssh): check the matcher against a live OpenSSH client, not against my beliefs
Every other test in the parser file states what I believe ssh does. These state what it did: an OpenSSH 10.2p1 client against a real sshd on 127.0.0.1:2222, with the client's own verdict recorded from its output, and ssh-keygen -H's own salt and hash pinned as a vector. Two assumptions the design leans on were worth more than an argument. THE FALLBACK PASS MAY NOT REPORT A CHANGE. We look up `[host]:port` first and retry the bare host, and only the first pass may answer `mismatch`. With StrictHostKeyChecking=accept-new, a bare line holding a DIFFERENT key, dialed on 2222, ssh connected and appended a new `[127.0.0.1]:2222` line — no IDENTIFICATION HAS CHANGED banner. It read that as first contact. Had we reported a change there we would refuse hosts ssh connects to happily, and the wrongness would have been invisible: refusing looks like the cautious choice. AND THE TYPE-SCOPING REJECTION IS NOT AN INVENTION. known_hosts holding an ssh-rsa key while the server offers ed25519 makes ssh print IDENTIFICATION HAS CHANGED and refuse. So unknown-type-known-host is neither stricter nor laxer than ssh — treating it as first contact, which is what a naive type-scoped lookup does, is the laxer mistake. That second result also fixes the message. ssh is blocked too, so `ssh-keygen -R <host>` is the remedy that unblocks both, and we were naming it only for a same-type mismatch — leaving this case with a diagnosis and no way out. Named now when known_hosts is the source that disagrees, and still not named when it is our own store, which ssh-keygen would not touch. 1,516 SSH tests pass. |
||
|
Neil
|
62eb47a6ae |
refactor(ssh): give the store one matcher instead of two
The connect path had its own copy of the store comparison, because ssh2's verifier decides synchronously and cannot await the file, so records are preloaded. That copy is precisely where the type downgrade came from: it answered only match/mismatch/unknown, so a record of a different type for the same endpoint read as first contact and a host learned on first contact could be impersonated by presenting another key type. Fixing it left two implementations that have to agree forever, which is the same bug waiting to happen. matchTrustedHostKeys is now the single pure matcher; isTrusted is a load plus a call to it, and the connect path calls it directly on preloaded records. Same for the key types that feed the algorithm ordering, which the connect path was also filtering by hand. The two copies had in fact already drifted: the connect path lower-cased the query host where the write trims AND lower-cases. Not reachable today — the host is trimmed before it reaches there, which I confirmed by mutating the normalisation and watching the wiring test pass anyway. So this is not a bug fix, and the wiring-level test I first wrote for it proved nothing and is gone. The unit test that replaces it drives the matcher directly, where the input is mine to control, and it does fail when the normalisation diverges. Also adds an equivalence test across all six outcomes between the preloaded and awaited paths, so the two can never answer differently again. Restores the local name siteConfigSuppressed for the `-F` check; it was renamed along with the decision input, but at that site it really does mean only the one thing, and the union with the unreadable-file count happens one line later. 1,509 SSH tests pass. |
||
|
Neil
|
088c058c9b |
fix(terminal): show why the SSH connection failed, not just that it did
The reconnect overlay took only a status, so every failure rendered the same sentence: "The SSH connection to devbox failed. Connect again to continue this terminal session." A refused host key and a network timeout were indistinguishable there, and the host key message — the only place that names the remedy, down to `ssh-keygen -R <host>` — reached no terminal user at all. The state carried it the whole way; the overlay simply never asked for it. Adds it as a second line rather than replacing the sentence. The sentence says what to do, the detail says what happened, and keeping both means an errno failure does not lose its guidance to make room for "connect ETIMEDOUT". Wrapped, for the same reason as the settings card: the remedy is at the end. Suppressed for a removed target, which already explains itself and can never reconnect — a stale connection error underneath would contradict it. The new selector mirrors selectRuntimeAwareSshStatus branch for branch, including the unreachable-environment and un-hydrated-bucket nulls, so the pair cannot disagree about which source they read and a detail is never shown next to a status it did not come from. Known and NOT addressed here: "Connect again" is still the wrong advice for a decision that will never change. Telling those apart needs a typed reason on the wire rather than a string, which is a remote-wire-compatibility decision; it is recorded in the STA-4319 follow-ups. Pre-existing on this branch and untouched: 2 failures in terminal-ime-xterm-resumed-preedit-visibility. |
||
|
Neil
|
ecc9df766e |
fix(settings): stop truncating the SSH connection error to one line
The host key messages are written to be actionable — a mismatch ends in "Run: ssh-keygen -R <host>", which is the remedy that also unblocks ssh and git. The only place in the renderer that displays an SSH connection error clamped it to a single line with `truncate` and carried no title attribute, so the remedy was unreachable, not even on hover. The careful wording reached a CSS ellipsis. Wraps instead, with [overflow-wrap:anywhere] because a long hostname offers no break opportunity and would overflow the column on its own. The paragraph only renders on failure, so the extra height costs nothing in the normal case. CORRECTION to the four preceding commits: they each claimed 3 pre-existing failures in ssh-connection-sftp-namespace.test.ts. That was my error — I had been running `npx vitest` without `--config config/vitest.config.ts`, so the project's setupFiles and execArgv were absent. Under the real config those 3 pass, and have throughout. The whole SSH suite is green: 1,504 passed, 13 skipped. Still failing on this branch and unrelated to it (different subsystems, no file overlap): 5 in terminal-snapshot-osc8-roundtrip and 2 in browser-cookie-import. |
||
|
Neil
|
fd277bb0cd |
test(ssh): cover the wire from an accepted key to a record on disk
Nothing covered the store end to end. Every connection test runs with it unwired — a real path, since it degrades to known_hosts only — so an accepted first-contact key was never observed becoming a record, and the record was never observed being believed on the next connection. Without that wire the store is dead weight: an unknown host connects every time and is never learned. Its own file for two reasons. initSshHostKeyStoreFile binds module-level state for the rest of the process, and binding it makes the connect prelude do real disk I/O — which the shared suite cannot absorb, because its reconnect tests drive the clock with fake timers and an fs round trip does not complete inside an advanced tick. Adding these there turned 12 unrelated tests red. Covers: the record is written; it is read back as a match; the same key is not recorded twice; a DIFFERENT key for a host we recorded ourselves is refused (the store's entire security value, and a case known_hosts cannot catch since it has never heard of the host); and an on-demand runtime records nothing. Stubbing rememberHostKey to a no-op fails four of the five. |
||
|
Neil
|
f2070df6db |
test(ssh): pin the parser against real ssh -G output, and accept-new against ask
Two gaps the audit named. The ssh -G fixtures were all hand-written, which means they encode what I expect ssh to print. This one is verbatim OpenSSH_10.2p1 output for a Host block using HostName, HostKeyAlias, StrictHostKeyChecking accept-new, two UserKnownHostsFile paths and a non-default port. The format detail that matters: each file list arrives space-separated on ONE line, so reading it as a single path would consult nothing for anyone with more than one file configured. And accept-new was untested despite being a real OpenSSH value. It currently behaves identically to ask, which is exactly right while no dialog exists and is what lets the defence ship without a modal — so the equivalence is now pinned, and Phase 2 has to break it deliberately rather than discover it. Plus a guard that accept-new never falls into the strict branch. |
||
|
Neil
|
7c788020e5 |
test(ssh): pin the ephemeral carve-out where it is actually observable
The first version of this test asserted an on-demand runtime connects on first contact, which every target does — it would have passed with the carve-out deleted. Pointing HOME at a home whose known_hosts exists and will not open makes the two cases diverge: a normal target is refused there, an on-demand runtime is not. Removing the carve-out now fails three tests instead of none. Also covers the wiring for the unreadable-source refusal itself, which until now was only pinned at the decision level. |
||
|
Neil
|
03264f2d82 |
fix(ssh): let an ephemeral runtime outrank sources we could not read
Three things, all about the same question: when we cannot see everything that decides a host key, what does that actually license us to refuse? 1. ON-DEMAND RUNTIMES WERE REFUSED FOR A POLICY THEY COULD NEVER SATISFY. A machine provisioned a minute ago cannot be in known_hosts, by construction — which is why it has a carve-out at all. But the carve-out sat BELOW the incomplete-sources check, so anyone whose HOME diverges from their passwd home (sandboxes, our own E2E isolation) took the `-F` path, and every on-demand runtime connection was refused, pointing at a config file the user cannot fix. Refusing there buys nothing. No policy, seen or unseen, is satisfiable by a host that did not exist yesterday; the trust comes from the provisioning channel. So the carve-out now outranks it. An EXPLICIT StrictHostKeyChecking=yes still wins over both — that one we can read, and the user asked for it. 2. THE FLAG WAS NAMED FOR ONE OF ITS TWO MEANINGS. `siteConfigSuppressed` started as "-F hid /etc/ssh/ssh_config" and had since grown "a known_hosts file exists and would not open" — which is not a site config, and reading the name in the decision function told you nothing about why an unreadable file landed there. It is `verificationSourcesIncomplete` now: we could not see something that decides this, so do not extend NEW trust. A host we already know still connects, because a match is decided before this is reached, and that is now pinned by name. 3. THE COPIED ssh2 ALGORITHM LIST HAD NO DRIFT ALARM. We reorder ssh2's default host-key proposal, which meant hand-copying a list ssh2 exports only from a deep internal path. ssh2 throws `Unsupported algorithm` on anything outside its supported list, so drift does not degrade — every target stops connecting, before a socket opens, with a message about an algorithm the user never chose. Worth knowing that the list is also built conditionally on ed25519 support. Kept as a literal rather than a deep import, since silently adopting a new proposal order is the wrong default — the order is what makes type-scoped matching safe, so a change deserves review. A test now compares it against ssh2's real constant and checks every entry is one ssh2 accepts. Moved next to the ordering function it feeds, and out from between the import statements. 1,488 SSH tests pass. The 3 in ssh-connection-sftp-namespace.test.ts are pre-existing on origin/main. |
||
|
Neil
|
6dcfcdface |
fix(ssh): stop treating an absent known_hosts as a source we failed to read
The previous commit's "no readable source is not first contact" guard was right about the danger and wrong about how to detect it, and the version that shipped would have refused every connection a new profile ever makes. A file that does not exist and a file that refuses to open both arrive as a rejected readFile, and I counted them the same way. They are opposites. An absent known_hosts is the normal state — ssh creates it on its own first connect, and an Orca profile that has never connected has none — and it is real evidence that no host is known. A file that exists and will not open is evidence withheld: the entry that would have said "this key changed" may be sitting in it. The default list is the reason this was fatal rather than obscure. It always names known_hosts2, which essentially never exists, so on a machine with a normal known_hosts the count was 1-of-2 and everything worked; on a fresh profile it was 0-of-2 and every connect failed with "the system SSH configuration could not be read" — a message about a file the user does not have and an error they cannot act on. The suite passed only because the machine running it happens to have a known_hosts. Pointing HOME at an empty directory fails 62 connection tests, which is what the new wiring test does. So the count is now unreadableFileCount: files that exist and could not be read, which is the condition the guard was always trying to express. Any one of them takes the strict path; an absent or empty file takes none. The store clause is gone with it — a store hit already returns accept before this is consulted, so it never changed an outcome. Tests: absent, empty, permission-denied, directory, and all-parsed at the source; first contact with no known_hosts at all at the wiring level. 1,482 SSH tests pass. The 3 failures in ssh-connection-sftp-namespace.test.ts are pre-existing on origin/main and untouched here. |
||
|
Neil
|
5839ac2d84 |
fix(ssh): stop offering credentials to a host we just refused
A refused host key ended the handshake and then fell into the credential ladder, because ssh2 reports a denied key as a generic auth failure and the passphrase branch is eligible on message shape alone whenever an encrypted identity file is configured. So the sequence was: decide this host may not be who it claims to be, then ask the user for their passphrase and hand it to it. Failing that, prompt for a password. Failing that, retry over the system ssh binary, which for a disagreement with our own store rather than known_hosts would simply connect. That inverts the point of checking at all. A denied key is now final for the attempt: recognised by type before any fallback runs, and again inside the agent-fallback retry, which re-runs the handshake and so can be the attempt that denies. Two things had to change for that to hold. The rejection is now a HostKeyVerificationError rather than a rebuilt Error, so the connect path recognises it by type. Substring matching would have worked today and quietly stopped working the first time a reason string was reworded — and these strings are already worded around the auth-error classifier, so they are exactly the kind that get edited. And the verifier now reports the denials that skipped the policy: an unreadable key blob and an internal failure both denied without calling onDecision, so the connect path saw only ssh2's generic failure and walked the ladder. That was the actual path the new test hit first. The report carries no fingerprint, since there is no host key to identify, and the connection no longer overwrites the fingerprint it holds with an empty one — the relay keys install-lock isolation on that value. The reconnect ladder also refuses to retry it. Its classifier is otherwise substring-driven, so a reason containing "connection reset" would have been retried until the ladder gave up, burying the reason under nine attempts. Tests: no credential prompt after a refusal, an encrypted key configured so the passphrase branch is eligible; refused reports as 'error', not 'auth-failed', which would invite the user to re-enter credentials that are not the problem; no retry; both bypassing denials report; a throwing listener still denies rather than hanging the handshake. Also drops three test files a bisect resurrected from before the revert that deleted them. 172 tests pass across the three touched files; typecheck and lint clean. Pre-existing on origin/main and untouched here: 3 failures in ssh-connection-sftp-namespace.test.ts. |
||
|
Neil
|
11b2f93cf8 |
fix(ssh): close two downgrades the implementation review found
Both were in my wiring, not the design, and two reviewers found the first independently. 1. OUR STORE WAS TYPE-DOWNGRADABLE. The inline lookup filtered by key type first and could only answer match/mismatch/unknown, so a record of a DIFFERENT type for the same endpoint read as "unknown". A host learned on first contact — ed25519, since ssh2 proposes it first — and absent from known_hosts could then be impersonated by presenting RSA: both sources say unknown, so accept-and-remember, silently. That is exactly the downgrade D3 says the design cannot ship without, applied to the records we create ourselves. The store's own isTrusted already computed the right answer and had no production caller. Stored types now also feed the algorithm ordering, without which the guard is only half present. 2. WE KEYED ON THE ORCA LABEL, NOT THE DIALED HOST. "ssh -G" echoes its own argument back as its hostname field when no Host block matches, so for a manual target that field IS the Orca label — the one name D2 forbids keying on, and one ssh never wrote. We consulted no entries at all, so an impersonated host read as first contact. Now keys on the dialed host, which buildConnectConfig has already resolved through HostName, with HostKeyAlias still winning. That inverted an existing test rather than deleting it: "uses the resolved hostname, never the Orca label" encoded an assumption disproved against OpenSSH 10.2p1, so it is renamed and reversed with the reason recorded in the test. 3. NO READABLE SOURCE IS NOT FIRST CONTACT. Every known_hosts file failing to read was indistinguishable from "this host is unknown", so a changed key would be accepted the one time we could not check. The loader now reports how many files it could read, and zero readable sources with an empty store takes the strict path instead of recording trust. 4. A superseded attempt's rejection could replace the live attempt's error, and substituting a new Error drops ssh2's code, so a transient ECONNRESET would stop being classified as retryable. The rejection is now local to its attempt. 5. displayHost was the Orca label, so a mismatch could print "ssh-keygen -R <label>" — a remedy that removes nothing. Also adds the tests that would have caught 1 and 2, the stale-attempt denial, and IPv6 literals, which the design moved into scope and nothing covered. 1,469 SSH tests pass. |
||
|
Neil
|
0f37005de0 |
fix(ssh): bind the host key store to the active profile at startup
Without this the store reports nothing trusted on every launch. Safe — known_hosts still decides, and a host trusted only by us degrades to first contact and is re-recorded — but it silently discarded our own accept records, so the store the design calls for was not actually in use. Bound beside the profile Store, since it is a sidecar of the same data file. Also records why the paired-web carve-out the migration review asked for is NOT implemented in Phase 1, rather than leaving it looking forgotten. That carve-out exists to stop a web client waiting out the 120s prompt timeout — a hang only reachable if a prompt exists, and Phase 1 has none, which the decision function pins with a test asserting it never returns 'prompt'. An RPC connect therefore behaves exactly like a local one. Adding a fail-fast path now would introduce a failure mode for a hang that cannot occur; it becomes load-bearing when the dialog lands and is listed under Phase 2. Noted there for whoever builds Phase 2: runtime/rpc/methods/ssh.ts already swallows the specific error and rethrows a generic one, so the host-key reason will not reach a web user without a change there too. Full unit suite: 52,352 pass. The 8 failures are the known environment baseline (5 osc8, 2 IME) plus one browser-cookie suite-ordering flake that passes in isolation — none in src/main/ssh, and none related to this change. |
||
|
Neil
|
a1331d2602 |
fix(ssh): verify host keys instead of accepting every one (STA-4319)
The actual fix. ssh-connection's verifier recorded a fingerprint and returned
true, so every ssh2 connection accepted every host key — no known_hosts consult,
no change detection. It now consults the user's known_hosts plus our own store
and refuses a changed, revoked or unverifiable key.
Phase 1 by design: no dialog. Unknown hosts are accepted and recorded
(accept-new semantics), because startup restore opens every previously-active
target at once, ephemeral VM targets present a new key each launch, and
paired-web connects run on the host desktop where a prompt would appear on
someone else's screen. The MITM defence lands now; the prompt is Phase 2.
Also sets algorithms.serverHostKey to lead with the types already known for the
host. Without it the type-scoped matching is a downgrade — an attacker who cannot
forge the key on file just presents another type and turns a hard failure into
first contact. Verified from ssh2's kex.js that the client list decides.
Denial replaces ssh2's generic handshake error with the specific reason, because
the reconnect ladder cannot distinguish a generic failure from a transient fault
and would retry forever against a decision that will never change.
An unreadable trust store degrades to known_hosts only rather than failing the
connect: a changed key is still refused, and a host trusted only by us falls back
to first contact and is re-recorded, reaching the same decision.
The ssh2 mock now uses the callback form and aborts the handshake on denial. As
written it called hostVerifier(key) with one argument and ignored the result, so
it would have passed against a verifier that never decides — flagged in the
design as a mock that had to change, not a test to quietly rewrite. Two new tests
pin the wiring rather than the module: an unidentifiable blob is refused, and a
well-formed key is accepted.
Note for review: commit
|
||
|
Neil
|
43ed05509b |
fix(ssh): promote every RSA signature algorithm for a known ssh-rsa key
A known_hosts entry names the KEY type, which is not the negotiated ALGORITHM name. One ssh-rsa key is offered as rsa-sha2-512, rsa-sha2-256 or ssh-rsa depending on the signature algorithm, so matching the literal name only would leave a host we know by RSA ordered behind ed25519 — precisely the ordering this function exists to prevent, and precisely the population (RSA-era known_hosts entries) it was written for. Verified from ssh2's own negotiation while wiring this: kex.js iterates the CLIENT list and takes the first entry the server also offers, so client order does decide, as RFC 4253 says. ssh2's default order leads with ed25519 and places the RSA algorithms fifth through seventh. |
||
|
Neil
|
2d2a0880ba |
feat(ssh): build the host key verifier and the algorithm order that makes it safe
Still not wired into the handshake — that lands next. This is the piece that turns a decision into an ssh2 callback, plus the half of the design that is easy to forget because it lives in a different config field. The verifier MUST be a plain function returning undefined. ssh2 does 'const ret = verifier(key, verify); if (ret !== undefined) verify(ret)', so an async function returns a Promise — neither undefined nor falsy — and ssh2 accepts the key immediately while ignoring whatever the callback later decides. Making this async would silently restore exactly the accept-everything behaviour the module exists to remove, so a test asserts the return value is undefined. orderServerHostKeyAlgorithms is what makes type-scoped matching safe rather than a downgrade. RFC 4253 gives the client's algorithm order priority, so leading with the types we already hold for a host denies a server the choice of presenting some other type to convert a hard failure into first contact. Without it, an attacker who cannot forge the key on file just offers a different algorithm. Revoked entries never contribute to that order. Also fails closed on two paths that would otherwise hang or over-trust: a key whose own length-prefixed header cannot be read is refused rather than reasoned about, and a throw from any dependency denies, because ssh2 may not catch an exception raised inside the verifier and the handshake would hang instead of failing. 18 tests. Includes the two negative cases that matter — first-contact keys are recorded, but keys we already know, rejected keys, ephemeral runtime targets and a lax StrictHostKeyChecking are not. |
||
|
Neil
|
b5e27ab7b7 |
feat(ssh): decide what to do with a presented host key
The policy half of STA-4319, kept separate from the ssh2 wiring so it is testable without a handshake and injected rather than importing its sources, so a test states its own trust state instead of writing files. Phase 1 ships no dialog — a test asserts the decision is never 'prompt'. Startup restore opens every previously-active target at once, ephemeral VM targets would ask every launch, and paired-web connects run on the host desktop where the dialog would appear on someone else's screen. Ordering that matters: revocation outranks everything including StrictHostKeyChecking=no, because a revoked key is a statement that this key is known-bad rather than merely unrecognised. known_hosts is named before our own store on a change, because its remedy (ssh-keygen -R) is the one that also unblocks ssh and git — pointing at a remedy that cannot work is worse than none. Two carve-outs with reasons: an ephemeral runtime target accepts WITHOUT recording, since a fresh VM presents a new key every launch and a stored record would accumulate per launch and eventually read as a spurious change; and when ssh -G ran on the HOME-divergent path that suppresses /etc/ssh/ssh_config, an unknown host is denied, because a site-wide policy may forbid it and being laxer than ssh is the one outcome that is never acceptable. Rejection text deliberately avoids 'authentication failed' and 'permission denied': the reconnect ladder classifies on those substrings, so a denial phrased that way is retried forever against a decision that will never change. Pinned by a test. |
||
|
Neil
|
bee19b083f |
feat(ssh): parse and match OpenSSH known_hosts
The matcher half of STA-4319. No behaviour change yet — nothing calls this.
Hand-rolled because no maintained JS implementation exists, and written against
behaviour observed from OpenSSH 10.2p1 rather than inferred from the man page.
Three of those behaviours a reasonable reading gets wrong:
- A non-default port is TWO ordered lookups, not one candidate set: '[host]:port'
first, then bare host ('checking without port identifier' in ssh -v). The
fallback pass can only yield match or unknown — OpenSSH downgrades a wrong key
there rather than reporting a change. Collapse them and anyone holding a bare
line who connects off-port gets a spurious first-contact result; treat the
fallback as authoritative and they get a false change-of-key alarm.
- Revocation resolves in its own pass so the verdict cannot depend on line order.
Verified both orderings.
- A cert-authority line never matches a plain host key; it only validates
certificates. A normal line alongside it still decides.
Mismatch is scoped to the same key type, and a host known by a DIFFERENT type
returns unknown-type-known-host rather than plain unknown — an attacker who
cannot forge the key on file must not get a friendly first-contact result by
presenting another type. That outcome is only half the defence; the other half
(leading serverHostKey with known types) lands with the wiring.
47 tests from vectors executed against real sshd, including ssh-keygen -H hashed
entries. Each of six mutations reddens it: collapsing the passes, letting the
fallback report mismatch, dropping type scoping, resolving revocation in line
order, honouring an unrecognised marker, and skipping the blob/type agreement
check.
|
||
 Jinjing
|
cb42b60849 |
fix(linear): label Start workspace and add Open on Linear (#14492)
* fix(linear): label Start workspace and add Open on Linear The issue page header used three unlabeled icons. Match the GitHub issue header: copy stays quiet, Open on Linear is an external-link control, and Start workspace is a labeled primary button. * Address PR review feedback (#14492) - Make the header source contract ignore Start workspace formatting |
||
|
Neil
|
87bafb5e6a |
fix(terminal): ground the emulator to the snapshot's baseline after a byte gap (#14374)
A hidden-delivery byte gap can strand more than the SGR pen, and the reset #14241 added to the split alt-screen replay is undone before any content is painted: xterm answers `?1049l` with restoreCursor(), which reloads the pen, all four G-set designations, GL, origin mode and wraparound from the register saved at `?1049h`. - Bracket the buffer switch with the baseline: before, so `?1049h` banks grounded state rather than the gap's; after, so `?1049l`'s restore cannot reapply it. - Ground everything a serialized payload is diffed against, not just the pen: SGR, GL, all four G-sets, origin, autowrap, insert, the per-buffer scroll region, and the saved-cursor register. - Switch buffers only when the pane is actually on the other one. `?1049` is not a no-op otherwise — it still swaps the kitty flag registers, which would park the flags of an agent that negotiated them on the normal screen. - Return to the normal buffer when the gap ate the TUI's exit sequence; the restored history was painting into the alt buffer with scrollback left empty. - Restore the CAN #14241 dropped, so a control string the gap truncated is discarded instead of committed by the next ESC. - Ground the abandon path exactly once instead of twice. - Derive the parity/fuzz preambles from the same builder; they had drifted and were asserting against bytes production no longer emits. |
||
  Langning ZhangandJinwoo-H
|
92b6ffd17d |
Terminate renderer graph reload generations and contain disposed-frame notifications (#14070)
* fix(runtime): terminate renderer graph reload generations * fix(runtime): harden renderer reload teardown * fix(runtime): fence renderer graph publication ownership * test(runtime): register renderer graph reload gate * test(runtime): record live reload validation * fix(runtime): ignore cancelled renderer navigations * chore: preserve main formatting during branch sync * chore: satisfy changed-code quality gate * fix(runtime): restore cancelled renderer reloads * fix(runtime): preserve committed reload fencing * test(runtime): prove cancelled reload timeout * docs(reliability): record reload cancellation oracle --------- Co-authored-by: Jinwoo-H <jinwoo0825@gmail.com> |
||
 NeilandOrca
|
46e1c3eee4 |
fix(terminal): strip the captured shim dir across trailing-separator spellings (#14439)
* fix(terminal): strip the captured shim dir across trailing-separator spellings The scrub compared the captured ORCA_ATTRIBUTION_SHIM_DIR to PATH entries literally, so a trailing-separator difference left the legacy shim directory on the spawned PATH. Same class already fixed in the generated wrappers. Also drops a dead default in the POSIX filter, consolidates comments that had accumulated across fixes, and normalizes the legacy directory once rather than per PATH entry in the cmd wrapper. The boundary scanner stays: a shim path can contain the PATH delimiter, which splitting would fragment. * fix(terminal): keep the git shim tombstone parseable on Windows The cmd wrapper carried two em dashes in comments. cmd.exe seeks through a batch file in bytes but advances by decoded character count, so those four extra UTF-8 bytes made it drop the first four characters of every line and the wrapper died with "The syntax of the command is incorrect." Also move the legacy-dir trailing-separator strip into a CALL body: cmd expands a whole line before evaluating `if defined`, so inline it ran its substring syntax against an unset variable and mangled the line. Rooted-path checks are shared by a single subroutine, a relative or drive-relative captured ORCA_REAL_* is rejected, and relative PATH entries are dropped from the exported PATH so the cwd cannot select spawned tools. The POSIX tombstone is deleted rather than written when no absolute interpreter can be verified. Verified on Windows 11 (cmd and PowerShell 5.1): normal lookup, relative and drive-relative ORCA_REAL_GIT, relative and drive-relative PATH entries, trailing separators, legacy shim dir, empty PATH, and a cwd-only PATH with a planted git.cmd/git.bat. Co-authored-by: Orca <help@stably.ai> * fix(terminal): stop the git shim tombstone re-expanding PATH data cmd re-expands a CALL command line, so path data handed to a subroutine as an argument got a second round of percent expansion. A PATH entry holding a literal %CD% became the current directory before the rooted-path guard saw it, and the wrapper then ran a planted git.cmd from that directory (exit 66, reproduced on Windows 11). Callers now pass the value in a variable, which is expanded once. Re-verified on Windows 11 across 15 cases: the %CD% entry is now dropped and the real git runs, and a PATH entry spelled 'C:\paren9 (x86)\cmd' still resolves, so the new for-block did not regress paths with parentheses. Co-authored-by: Orca <help@stably.ai> * fix(terminal): keep percent expressions out of the shim tombstone comments cmd expands variables inside rem, so a comment naming the working directory substituted a path into itself. Verified on Windows 11 that rem does not re-parse the result -- a cwd of 'C:\x&pwned&rem' executed nothing and the real git still ran -- so this was not exploitable, but rem handles separators differently inside a parenthesized block and this script now has some. A test now rejects any percent sign in an emitted rem line. Co-authored-by: Orca <help@stably.ai> * fix(terminal): pin shim tombstone shell state and directory identity Three fixes, each proven before and after. Delayed expansion: bare setlocal inherits the caller's state. Under a parent shell started with /V:ON, a literal !CD! PATH entry became the current directory and a planted git.cmd ran (exit 66), and a legitimate directory containing ! stopped resolving (exit 127). Both on Windows 11, both gone with setlocal DisableDelayedExpansion. Directory identity: the POSIX filter compared the legacy shim directory lexically while comparing its own directory with -ef, so a symlink or a <legacy>/../<legacy> spelling escaped the filter and the live attribution wrapper won the lookup. It now tests both. The env scrub had the same gap and now normalizes before its suffix test. Retained POSIX wrappers: with no absolute bash verifiable the wrapper was deleted, which strands a shell that already hashed the path on 127 instead of falling through to PATH. It now reuses the shebang of the wrapper it replaces, which is known to work on that host, and rejects /usr/bin/env so the ambient lookup stays closed. Deleting is the last resort. Two Windows test pins matched the wrong occurrence and stayed green with the guard they claimed to protect removed; they now assert the subroutine body. All five fixes were mutation-tested. Co-authored-by: Orca <help@stably.ai> * fix(terminal): require bash for a reused shebang and exclude slash-spelled dirs The retained-shebang fallback accepted any absolute executable that was not env, but the rendered body needs BASH_SOURCE, [[ and local, so a #!/bin/zsh wrapper was accepted and then exited 1 on 'BASH_SOURCE[0]: parameter not set'. It now requires bash, which also rejects /usr/bin/env as before. The PowerShell filter trimmed only backslashes while its rooted-path regex accepts forward slashes, so a wrapper or legacy directory spelled with a trailing / missed the lexical exclusion. Verified on Windows 11 that both spellings are now excluded and the real git still runs. The test that claimed to cover the shebang fallback only called the resolver directly, so deleting the wiring left the suite green on any host with /bin/bash. It now mocks the resolver to null and asserts through neutralizeLegacyTerminalShimDir that the wrapper survives with the retained shebang, and that a wrapper without a reusable one is still deleted. Both mutations are now killed. The Windows wrapper text assertions move to their own file rather than taking a max-lines exemption. Co-authored-by: Orca <help@stably.ai> * fix(terminal): stop the shim PATH scrub deleting a legitimate directory The previous round collapsed '..' lexically before classifying a PATH entry. That is not the same as resolving it: when <shim>/posix is a symlink, <shim>/posix/../posix lands elsewhere, so a legitimate directory was classified as the shim and removed, leaving git unresolvable. Reproduced with a symlinked shim/posix and a real git behind it. Resolving for real is not available here either -- this env is also built for remote and WSL panes whose paths name nothing on the local filesystem -- so the classifier is lexical again, deliberately. A '..' spelling that slips through costs nothing at runtime: that directory holds the pass-through tombstone, and the tombstone excludes its own directory by -ef, so the lookup still reaches the real git. Separately, pathEntrySpellings can only enumerate one added separator, so a captured directory spelled with two or more survived the literal removal. The split filter now also compares separator-stripped forms, which covers any number. Both changes are mutation-tested. Co-authored-by: Orca <help@stably.ai> * fix(terminal): stop a relative shim dir letting the cwd pick the binary The -ef identity test added two rounds ago resolves a relative right-hand operand against the wrapper's current directory, so a relative ORCA_ATTRIBUTION_SHIM_DIR let the cwd decide which PATH entry counted as the legacy directory and got a legitimate one skipped. Reproduced as SAFE vs LATER purely by changing the cwd. Identity is now attempted only for an absolute target; the lexical compare still covers the rest. The cmd wrapper had the same shape: full-path expansion made a relative captured value absolute against the cwd before PATH filtering. It now requires a rooted value and leaves the normalized form unset otherwise, which makes the reject subroutine a no-op. Verified on Windows 11 that two runs differing only in cwd now agree. Separately, trailing-separator stripping treated a backslash as a separator on POSIX, where it is a legal filename character, so '/tmp/captured\' and '/tmp/captured' compared equal and a real directory was deleted from PATH. The rule is platform-specific now; the cross-platform classifier still understands both styles because a Windows PATH reaches it through the remote env. Both fixes are mutation-tested. Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai> |
||
|
Neil
|
b82a8791f5 |
perf(runtime): resolve an explicit worktree id without scanning every repo (#14399)
`resolveWorktreeSelector` resolved every selector kind from the whole-fleet snapshot, so a targeted `id:<repoId>::<path>` lookup fanned `git worktree list` across every registered repo to answer a question about one of them. With a cold scan cache -- app startup, or the first lookup after a mutation clears the snapshot -- that is one subprocess per repo, ~17ms each, to find a worktree whose owning repo the id already names. Measured on a ten-repo fleet: one `id:` lookup scans 10 repos before and 1 after. Scope only `id:`. Every other selector kind is matched across the fleet and its `selector_ambiguous` contract is defined over all repos, so scoping `branch:`, `name:`, `issue:`, or a bare selector would silently pick a winner where they correctly refuse today. A test pins that: `branch:main` across ten repos still throws `selector_ambiguous` and still scans all ten. Lineage stays correct because edges are intra-repo by construction. The scoped path returns null and falls back whenever that does not hold: a repo id registered on several execution hosts, an unknown repo id, or a worktree the scoped scan does not contain. A warm fleet snapshot always wins. Row resolution moves out of orca-runtime.ts into repo-worktree-row-resolution.ts, which owns no state -- the cache-aware scan and folder-workspace stamping are injected. orca-runtime.ts ends up 65 lines shorter than before despite the added feature. |
||
|
Jinjing
|
a6a64439a0 |
fix(terminal): keep split error when rejected cleanup throws (#14463)
Wrap kill and retireRejectedPty so a cleanup failure cannot replace the original split-authority error or skip the remaining teardown. |
||
 dngur6344
|
78ca45e2ae |
fix(renderer): remove duplicate git history tooltips (#14453)
* fix(renderer): remove duplicate git history tooltips * test(renderer): harden tooltip regression coverage |
||
|
Jinjing
|
d03eb3218e |
Revert "fix(renderer): stop git history hover from showing two tooltips (#14468)" (#14485)
This reverts commit
|
||
|
NeilandOrca
|
2100fb2553 |
fix(runtime): cap remote git.diff and file previews at the transport budget (#14160)
* fix(runtime): cap remote git.diff and file previews at the transport budget A remote or mobile user who opens the diff of a large image loses their whole WebSocket, not just that request: the E2EE channel closes with 1013 when a reply exceeds the 4 MiB outbound envelope. Two producers can exceed it unaided. git.diff/branchDiff/commitDiff cap text with MAX_RENDERED_DIFF_COMBINED_CHARACTERS (6M chars) -- a *renderer* budget that sits above the transport limit -- and return base64 for previewable binaries bounded only by MAX_GIT_SHOW_BYTES, so a 10 MiB PNG changed in place is ~26.7 MiB in one envelope. files.readPreview inlines base64 up to 10 MiB, and mobile calls it for every image tab. Both now measure against a budget derived from the outbound limit. The check sits in orca-runtime-git.ts, downstream of the dedupe and of both the SSH-provider and local branches, so a payload forwarded verbatim by an old relay is covered by the same code and src/relay needs no change. Local and in-process callers pass no budget and keep full fidelity. Measuring raw bytes would not work, which is the whole reason this needs a module. JSON escaping turns one control byte into six (\u00XX), and binary-buffer.ts sniffs only for NUL in the first 8 KiB -- so a NUL-free file of 0x01-0x1f bytes is classified as *text*, would pass a raw-byte cap, and would then blow the envelope. The budget is escape-aware, with a three-branch fast path that keeps normal diffs at two native byteLength calls and scans only the ambiguous band. The SSH branch of readFileExplorerPreview had the same raw-vs-escaped gap: its stat gate sizes base64 binaries, but text crossed unbounded. It now honours the same decoded-text limit the local branch already enforced. No wire change: GitDiffResult is untouched -- no third kind, no new field. Old clients see an error for one request instead of a dropped connection. diff_too_large joins the structured passthrough codes and lands on an existing error arm in both mobile consumers and the desktop remote path; file_too_large was already handled on both. Instruments the 1013 close, which nothing measured before, so the incidence this cap is meant to drive to zero is finally observable. `emitter` separates a producer size bug from a wedged link. Known regression: remote image previews between ~3.096 and ~3.146 MB now return file_too_large. They only intermittently worked before -- above ~3.0 MB they killed the socket -- so this trades intermittent connection loss for a consistent error. Test: 10281 passed in src/main/runtime + src/shared + src/main/git; mobile 3427 passed. Each of the six budget-enforcement sites is independently mutation-killed. Escaping fixtures cover newline-dense, control-char, CJK, lone-surrogate and base64 content against native JSON.stringify. tsc clean for node, web and cli; oxlint clean. Co-authored-by: Orca <help@stably.ai> * fix(runtime): harden remote reply transport budgets * test(runtime): cover desktop remote preview budgets * test(runtime): close telemetry review gaps * chore(shared): repoint budget imports after the shared/types barrel removal Upstream #14447 dropped the shared/types barrel; GitDiffResult now lives in git-diff-compare-types and GlobalSettings in global-settings-types. Co-authored-by: Orca <help@stably.ai> * fix(ssh): surface an over-cap preview read as file_too_large The stream reader aborts an over-cap read with StreamProtocolError, whose numeric code falls through mapRuntimeError to a generic runtime_error carrying the raw "Reported totalSize N exceeds client cap M" string. Neither preview client recognizes that: runtime-file-client.ts and mobile-file-preview-response.ts both key on file_too_large. It also made the two file_too_large guards directly below the read unreachable on the streaming path. Gives the cap its own error type so the caller can translate it, keeping the bandwidth saving the cap exists for. A genuine protocol fault still propagates unmasked. Found by the readiness review. Mutation-verified: removing the translation fails exactly the new test. Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai> |
||
  Brennan BensonandBrennan Benson
|
d137bb93e1 |
fix(agent-status): stop start-less child stops from minting phantom working (#14375)
* fix(agent-status): stop start-less child stops from minting phantom working buildClaudeCachedLeadStatusPayload fell back to 'working' whenever the pane had no cached lead-turn state. That default is right for a spawn or a child tool call, but the same helper serves SubagentStop and TeammateIdle, which end work and prove the opposite. claudeLeadStateByPaneKey is in-memory only, so every app restart empties it. A Claude session that outlives the restart reports its next child event into an empty map and the pane latches 'working' with an empty roster -- no Stop ever clears it, and the 30-minute window only decays the sidebar dot, never the stored state. Fall back by the event's evidence: terminating child events resolve to 'done', which still gates up through resolveClaudePaneState when the roster or background work proves the pane is busy. * fix(agent-status): require evidence for child completion * fix(agent-status): publish matched teammate idle * fix(agent-status): preserve confirmed child work * fix(agent-status): retain live restored teammates * fix(agent-status): reap unconfirmed siblings after child drain * fix(agent-status): preserve unmatched restored children * fix(agent-status): wait for lead completion after child stop * fix(agent-status): persist restored child transitions --------- Co-authored-by: Brennan Benson <brennan@stably.ai> |
||
|
Neil
|
b6d36c26f4 |
fix(terminal): resolve Ctrl chords positionally on a non-Latin layout (#14469)
macOS maps a control chord by physical key regardless of layout: measured with UCKeyTranslate, physical A/U under Control produce U+0001/U+0015 on 2SetHangul, Russian and Greek exactly as on ABC, though unmodified those keys give ㅁ/ㅕ, ф/г, α/θ. A native terminal inherits this by passing the OS characters through. The browser does not expose that translation. xterm's legacy encoder works anyway because it reads keyCode, which Chromium reports from the physical key, but its kitty encoder derives the key number from `key` and only consults `code` when Shift or Option is held. Ctrl is not in that gate, so a pane with the kitty protocol negotiated reports CSI-u for U+3141 rather than 'a' and the chord does nothing. Ctrl+C escaped this only via its hand-written ETX bypass. Recover the byte from `code` when `key` is non-ASCII, which reproduces the OS control table. An ASCII `key` stays authoritative so a Dvorak remap is honoured. KeyC is excluded: the interrupt policy owns it, and off macOS that policy declines to a selection so the copy binding wins. Fixes #13331 |
||
|
Jinjing
|
b3efd48c7f |
test(e2e): stabilize terminal failure coverage (#14466)
* test(e2e): stabilize terminal failure coverage * docs(e2e): classify failed-run findings * rm report |
||
|
Brennan Benson
|
9e5ee5ef8e |
feat(workspaces): rework cleanup discovery and dialog (#13413)
* fix(workspaces): support full cleanup scans * feat(workspaces): persist cleanup snapshots * feat(workspaces): add cleanup filter model * refactor(workspaces): remove cleanup presets * feat(workspaces): rework cleanup dialog * fix(workspaces): keep cleanup row ordering render-pure * refactor(workspaces): simplify cleanup browsing * refactor(workspaces): show cleanup facts * refactor(workspaces): surface cleanup row facts * fix(workspaces): remove misleading cleanup count * fix(workspaces): preserve full scan semantics * fix(workspaces): scope snapshot persistence * fix(workspaces): preserve cleanup browse compatibility * fix(workspaces): reconcile cleanup dialog state * test(workspaces): update snapshot store fixtures * test(workspaces): preserve cleanup scan modes * perf(workspace-cleanup): stream scan progress and size results * fix(workspace-cleanup): address review feedback * fix(workspace-cleanup): preserve host-scoped cleanup metadata * fix(workspace-cleanup): declare review source dependencies * fix(workspace-cleanup): align size scan banner * fix(workspace-cleanup): shorten scan action * perf(workspace-cleanup): avoid redundant scan IO * perf(workspace-cleanup): bound restarted evidence scans * fix(workspace-cleanup): satisfy scan queue lint * perf(workspace-cleanup): bound scan and snapshot work * perf(workspace-cleanup): serialize final enrichment * test(workspace-cleanup): assert final enrichment drain * fix(workspace-cleanup): stop progress after renderer teardown * perf: batch workspace cleanup git evidence scans * perf(workspace-cleanup): stop redundant snapshot and scan work * fix(workspace-cleanup): resolve review findings across scan, store, and dialog Correctness: - Chunk git-evidence dispatches at the shared 500-target limit and exclude queued/in-flight ids from target selection, so fleets past the limit can no longer strand rows permanently mislabeled as checked-but-unknown. - Key destructive selection pruning on the user's filter state instead of the per-tick matched-set identity; streaming reclassification no longer silently deselects rows. - Clamp the facet clock to max(scannedAt, open time): a stale hydrated snapshot no longer misbuckets idle thresholds or keeps dead agents fresh; row labels use the same clock. - Supersede and cancel the previous broad scan when a new one starts (renderer registry and same-sender guard in main) instead of racing two fleet scans. - Gate snapshot persistence on hasTargetedWorkspaceCleanupScan so worktreeIds: [] can never persist an empty fleet snapshot. - Re-apply dismissals at set-time in progress application so a dismissal landing mid-enrichment is not clobbered. - Record a one-off local snapshot prune for single (unbatched) remote deletes so removed workspaces cannot resurrect from cache. - Strip .exe when normalizing foreground process names so Windows agent processes match. Performance: - Cache per-candidate facet and review-info objects on candidate identity; no-op streaming ticks reuse the previous rows array and skip every downstream pass; matched-set identity is stable under equal membership. - Compute facet counts/options only while the filter popover is open. - Equality-bail git-evidence publishes; structural (non-stringify) facet-group comparison memoized in the toolbar. - Identity-token fast path for the enrichment cache (cache hits skip both JSON.stringify signatures); prune viewed/dismissal records on removal and expiry; bound the superseded-scan-id set. - Restore the no-op bail in removeWorkspaceSpaceWorktrees (regression). - Abort main-side scans when the renderer is destroyed; module-scope controller maps survive handler re-registration. - Batch removal preflight into one targeted scan (with refreshActivity) per 500 ids instead of one scan per row. - Scan repos at concurrency 2, report discovered counts upfront for honest progress, share fs-activity probes per path (folder workspaces), read only the reflog tail, and skip the snapshot read-before-write via a remembered scannedAt. Split workspace-cleanup-worktree-listing, workspace-cleanup-facet-row-caches, and workspace-cleanup-selection-model out of files that crossed max-lines. * fix(workspace-cleanup): address verifier findings - Fall back to a full reflog read when the newest record exceeds the 8KB tail window, so an oversized subject cannot hide recent ref activity. - Bound the single-removal snapshot prune batch id with a UUID; embedding the unbounded worktreeId silently failed main's 128-char validation and skipped the prune for long remote ids. - Key the main-side broad-scan supersession by sender AND scan mode so legacy suggestion-only and full-workspace scans stay isolated, matching the renderer registry. * fix(workspace-cleanup): own facet caches with useMemo instead of render-time ref writes React Doctor (CI changed-lines gate) correctly flagged the three cache refs written during render. Each per-candidate cache now lives in one memo with the derived context it is keyed on, so the memo deps are the invalidation and interior fills stay content-addressed; the matched-set identity stabilization is dropped since its only consumer reads through a useEffectEvent and never keys on identity. |
||
|
Jinjing
|
2e91a74deb |
fix(renderer): stop git history hover from showing two tooltips (#14468)
Remove native title attributes from commit rows, subjects, and ref badges so only the managed tooltip appears. |
||
|
Neil
|
fcd99fc032 | fix(sidebar): satisfy extracted hook quality gates (#14467) | ||
|
Neil
|
08027d0d23 |
refactor(sidebar): split WorktreeList into a worktree-list module (#14465)
WorktreeList.tsx was 6.8k lines behind an `eslint-disable max-lines`. Break it into `sidebar/worktree-list/`: the container keeps store wiring and composition, the virtualized viewport keeps layout, and the drag, reveal, virtualization, row-model, and row-render concerns each get their own file. Every file now fits the oxlint budget, so the suppression and its baseline entry are gone. Behaviour-preserving. The only deliberate cleanups are duplicate branches folded into shared helpers (drop-preview state updates, status-hover fallback, the two identical scroll-to-index reveal branches) and a dead sticky-header-index ref. Tests that asserted on WorktreeList.tsx source text or imported its named helpers now point at the module that owns them. |
||
|
Neil
|
e2e7768cc8 |
fix(terminal): deliver Ctrl+C when a non-Latin input source rewrites the key (#14462)
With a non-Latin input source the OS reports the layout's own glyph for `key` — a Hangul jamo on Korean 2-Set, Cyrillic es on Russian — while `code` stays KeyC. The interrupt policy read `key` for identity and only consulted `code` when `key` was empty or Unidentified, so a jamo short-circuited it to false: the press missed the ETX path and was CSI-u encoded instead, leaving a TUI running. Trust `key` only when it is a Latin letter, which keeps a Dvorak remap of C authoritative. Otherwise ask the layout map what the physical key produces unmodified: an IME layered over a Latin layout answers 'c', and over a Dvorak base answers 'j', which correctly declines. When the map is itself non-Latin it cannot answer either, so fall back to physical position — how terminals have always resolved control chords. Fixes #14460 |
||
|
Brennan Benson
|
3035aa9211 |
fix(browser): restore replaced cookies through CDP identities (#14383)
* fix(browser): restore replaced cookies through CDP identities Both remaining callers of the imported-domain replacement rolled back by rebuilding cookies with cookies.set, which silently drops partitionKey. The rollback in importValidatedCookies puts back the user's ORIGINAL cookies that the import already deleted, so a CHIPS cookie came back as an ordinary one and no restart recovered it. Snapshot CDP identities before the first removal and undo through them, the same machinery removeTransplantableCookies already uses. The store type omits 'set' so the lossy reconstruction cannot be reintroduced, and restoreImportedDomainCookies is deleted now that both callers are gone. * fix(browser): skip the CDP rollback when nothing was replaced restoreClearIdentities attaches the debugger before it iterates, so an empty restore set would spin up a hidden BrowserWindow to put nothing back. The old cookies.set restore was a no-op loop in that case. |
||
|
NeilandOrca
|
b9627e91a9 |
perf(renderer): parallelize runtime catalog and worktree refresh (#14138)
Co-authored-by: Orca <help@stably.ai> |
||
|
Neil
|
77f23b013f |
refactor(shared): drop the shared/types barrel and import from the real modules (#14447)
#14397 split `shared/types.ts` into 46 per-domain modules but kept the path as a re-export barrel so the import sites did not have to change. This removes the barrel: every consumer now imports from the module that actually declares the type, and `src/shared/types.ts` is deleted. Barrels hide where a type lives, make every consumer look like it depends on the whole domain, and let an unrelated edit invalidate a module that ~2,000 files transitively import. 2,323 import declarations across 2,321 files. Rewritten mechanically: each specifier was resolved to an absolute path via the TypeScript AST and recomputed, rather than string-substituted, so alias forms (`@/../../shared/ types`) and per-specifier `type` modifiers survive. Four cases the mechanical pass had to handle, each found by a gate rather than by reading the diff: - Modules inside `src/shared` import the barrel as `./types`, not `shared/types`. A pre-filter on the latter string skipped 176 of them and left imports dangling at a deleted file, which surfaced as confusing `Property 'x' is optional in type 'Repo' but required in Pick<Repo, ...>` errors rather than "module not found". - The barrel RENAMED one type on the way through (`WorkspaceSource as WorkspaceCreateTelemetrySource`), so the original name in the owning module has to be re-aliased at each consumer. - Three test files put `;(globalThis as ...)` on the line after the import. TypeScript parses that `;` as the import statement's terminator, so replacing through `statement.getEnd()` deletes it and breaks ASI. The rewrite now stops at the module specifier. - A file that already imported directly from a module got a SECOND import from it, because the barrel re-exported those same names — which trips `import/no-duplicates` under `--deny-warnings`. A post-pass merges declarations sharing a specifier and type-only-ness; the `import type` plus `import` pair from one module is left alone, since that form is allowed. Splitting one barrel import into several genuinely adds lines, which pushed `terminal-layout-pty-ownership.ts` to 301 counted lines: its 107-character import must wrap, and neither local type collapses onto one line (101 and 116 characters). Rather than contort a type declaration to fit a line budget, `collectLeafIds` and `pruneLeaves` move to `terminal-pane-layout-tree.ts` — they are pure structural operations on the layout tree and independent of PTY ownership. `visible-worktrees.ts` similarly loses its own mini-barrel re-export of `isDefaultBranchWorkspace`, with the four real consumers repointed at the declaring module. No `max-lines` bypass added. Verified: cold `tsc --noEmit` green on node, cli, and web (buildinfo deleted first — these projects are `composite: true` and reuse stale caches); the full `pnpm lint` green, not just bare oxlint — the narrower local check is what let the duplicate imports reach CI; max-lines ratchet OK at 344. |
||
 Jinwoo Hong
|
cd6114ab7e | fix(browser): acknowledge paired tab before navigation (#14402) | ||
|
Neil
|
b04da03ffc |
Require an absolute Orca CLI for the agent-teams tmux shim (#14438)
The generated tmux shim fell back to a bare orca / orca.cmd / orca-ide, and cmd.exe resolves an unqualified command against the current directory before PATH (sh does the same via ./empty PATH entries), so a stray orca.cmd in an agent's checkout could run with the agent-teams team id and token in its environment. Resolve only absolute paths, honor the Windows Path env spelling, degrade to in-process teammates when no CLI can be qualified, and make both shims exit 127 instead of guessing. Verified on macOS, Linux (dash + bash), and Windows (cmd.exe + Git Bash). Fixes STA-4215. |
||
|
Neil
|
bac21ee64d |
fix(worktree-scan): keep the admin-fingerprint probe inside the caller's per-repo budget (#14454)
* fix(worktree-scan): keep the admin-fingerprint wait inside the caller's per-repo budget The awaited probe was capped at 10s while `computeResolvedWorktrees` gives each repo 5s, so a slow mount always blew the budget: the caller gave up and republished persisted rows. The resolved snapshot was then stamped from the *start* of the compute, so a compute longer than its 1s TTL published an already-expired entry and the next poll repeated the whole 5s wait — deterministically, on every TTL expiry. Cap the probe at 2s so the remaining budget still covers the fallback `git worktree list`, and stamp the snapshot on completion. * fix(worktree-scan): derive the probe deadline from the caller budget A flat 2s cut reuse for hosts whose probe lands between 2s and 5s, which used to fit the caller's budget — trading the stall for a repeating `git worktree list`. Subtract a fallback-scan allowance from RESOLVED_WORKTREE_REPO_TIMEOUT_MS instead, so the invariant holds by construction and only probes that could not have fitted are cut. Tests now pin both ends: too large fails the budget invariant, too small fails reuse for a slow-but-healthy probe. |
||
|
NeilandOrca
|
5a6837fce4 |
refactor(store): unify the duplicated catalog equality and identity-key helpers (#13804)
* refactor(store): unify the catalog structural-equality walks Three near-identical structural deep-equality walks had landed independently in the same window: areValuesEqual (#13744, repo-identity-reconcile.ts), areCatalogEntriesEqual (#13770, repos.ts — already folded into the first on this branch's base) and catalogValuesEqual (#13662, worktree-catalog-reconciliation.ts). All three walk plain records and arrays and fall back to reference equality for anything exotic. They are not interchangeable. Two axes genuinely differ, and each caller depends on its own side: - Own-key set. #13744/#13770 require equal own-key counts plus hasOwnProperty, so an absent key differs from a key present and holding `undefined`. #13662 compares the union of both sides' keys, so those are equal. The strict side is load-bearing: the repo/project merges branch on `'localWindowsRuntimePreference' in project` (repos-project-runtime.test.ts "clears stale local runtime preferences"), and projects are now reconciled with this comparator. The loose side is test-pinned by worktree-catalog-reconciliation.test.ts "reuses rows with equivalent nested catalog data", where a locally built row carries `optional: undefined` that the host omits. - Leaf comparison. #13744/#13770 use `===` (NaN never equal, 0 equals -0); #13662 uses `Object.is` (the reverse). So instead of picking a winner, src/shared/structural-value-equality.ts holds one walk parameterised by those two axes and exports the two policies as `structuralValuesEqual` and `structuralValuesEqualIgnoringUndefined`. Every caller keeps its exact current semantics; the ~40 duplicated lines and the silent divergence go away. src/shared/persisted-ui-equality.ts (a fourth copy with a Set branch and no plain-object guard) is deliberately left alone: it gates a disk write in main with no direct test coverage. Also folded, all provably behaviour-identical: - The `${hostId}\0${repoId}` composite key had three copies (getRepoHostIdentityForParts, repoOwnerKey, getEntryKey) that must agree or repos silently stop reconciling. Moved to src/shared/repo-host-identity.ts because one of them lives in src/shared; the renderer module re-exports it. - mergeFetchedReposForHost's hand-inlined upsert loop now calls mergeByIdentity. mergeByIdentity additionally skips replacing a structurally equal row, which cannot change the result here: reconcileFetchedRepos runs immediately after over the same identities in the same order and restores exactly those rows. - Renamed repos.ts's `catalogRowsUnchanged` to `arrayElementsUnchanged`. It is a pure element-identity compare, two files away from `catalogRowsEqual`, which is a full structural compare. src/shared/structural-value-equality.test.ts pins both policies over arrays, nested records, null-prototype records, absent-vs-undefined keys, symbol keys, and non-plain objects (Date/Map/Set/class) falling back to reference equality. * fix(store): keep merged sourceRepoIds order host-independent Prefixing the cross-host remainder made a cross-host project's sourceRepoIds order a function of the refreshing host, so the projects reconcile never reused the row. Also pins the repo-derived host-id contribution the new per-project slice feeds the host-id resolvers. Co-authored-by: Orca <help@stably.ai> * refactor(store): migrate call sites that landed after this branch github.ts and ai-vault-session-identity.ts began using areValuesEqual on main while this branch was stale, and repo-identity-reconcile's record reconciler still called its own deleted walker. All three now use structuralValuesEqual; reuseEqualCatalogRows keeps its duplicate-id cap and calls the ignoring-undefined variant, which is the key-union semantics catalogValuesEqual had. --------- Co-authored-by: Orca <help@stably.ai> |