mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 00:02:31 +00:00
560c42e1d196f53299b49bf248bdb807f2ffe45b
11123
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jinwoo Hong
|
560c42e1d1 |
fix(cloud): pin the asia cell database pool in the same-cap plan validator (#21171)
* fix(cloud): pin the asia cell database pool in the same-cap plan validator Raising `database_pool_max` from 10 to 16 for production-gce-c27, c28 and c29 made every same-cap roll of those three cells fail closed at plan validation. The cell startup template emits `ORCA_RELAY_DATABASE_POOL_MAX` only for a cell whose region differs from the root region or whose pool is off the default, so the asia cells carry that line while the us-central1 cells do not. The plan validator requires the before and after startup scripts to normalize to the same text, masking only the lines it independently pins to a reviewed value. The pool line was neither masked nor pinned, so the live template's `'10'` and the plan's `'16'` were read as unreviewed drift. The validator gains an optional `--database-pool-max`, accepted in `same-cap-cell` mode alone. When it is supplied the after-script must contain exactly that pool line and the line is masked from the equality check; when it is not supplied the after-script must contain no pool line at all. Masking without the pin would have removed the guard rather than moved it. The same-cap job resolves the expected pool next to the hard cap, cross-checks it against the committed `relay_gce_cells` map (asserting the default 10 for the us-central1 cells), and passes the flag to both validator invocations only for the cells that emit the line. * test(cloud): require the pool pin for a line the live template already carries |
||
|
Jinwoo Hong
|
f949d5fcc4 |
ci(mobile): fail CI when the RPC recording pin leaves main's history or the corpus does not reproduce (#21156)
* test(mobile): fail CI when the RPC recording pin leaves main's history `mobile/rpc-foundation/pilot-scenarios.json` carries the commit every golden claims it was recorded from, and `--record` refuses on any other tree. A behaviour-change branch pins its own last fenced commit, which stops being reachable the moment the branch squash-merges: nobody can record on main again until a hand-made repin lands, and until now only a human noticed. #21123 was that, and so was the repin after #20954. `scripts/rpc-recording-pin-guard.mts ancestry` fails when the pin is not an ancestor of the commit under test, and prints the repin recipe. It refuses to answer on a shallow clone rather than trusting grafted history, so the job checks out with `fetch-depth: 0`. Ordinary product drift past a reachable pin is not a failure. `reproduce` makes the other claim the corpus header makes, which the recording suites do not: they replay the goldens against the CURRENT tree, so a golden recorded somewhere other than the pin -- a merge that auto-merged golden JSON, a refresh copied back from a scratch directory -- passes them and is what the header exists to deny. It checks the pin out detached, lays this tree's recorder and manifest over it, and lets the same suites compare in place, so the comparison is `compareGolden` with lockfile and platform masked as ever. It runs unconditionally on a push to main, which has no `verify` job and is where a squash lands a spliced corpus. On a pull request it runs only when the corpus, the manifest or the recorder moved: nothing else can move the verdict away from the one the base commit published, and `verify` replays the corpus against the branch tree meanwhile. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): judge the recording pin against the tree it was read from Round-1 review of the pin guard. The pull_request ancestry check read the pin out of the merge preview and judged it against the branch head. Those differ whenever main repins after the branch point, so ordinary stale branches failed, and the instruction told the author to repin to their own head -- which creates the unreachable pin the guard exists to catch. Judge the checked-out tree instead. `git worktree prune` in the reproduce teardown was repository-wide. This git directory is shared by every worktree on the machine (611 registered here), so it could deregister an unrelated one whose directory was momentarily missing. `worktree remove --force` alone is enough; a failure to remove is now reported rather than papered over. Also: the concurrency group is per commit on main, because GitHub cancels a pending run in a group whatever `cancel-in-progress` says; the skip gate fails closed when a provenance path stops matching instead of skipping forever; the census-boundary comment states the rule the code uses; and five exports with no consumer are now module-private. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): let an untracked golden and the guard itself buy a reproduction Two bot findings on the skip gate. `git diff` sees tracked paths only, but the reproduction's overlay copy and its census both read the corpus directory as it sits on disk, so an untracked golden or manifest is input to the verdict and used to skip the run that would judge it. Enumerate untracked entries under the provenance paths the way the recorder already does, and run rather than skip: an unjudged local addition is the case the reproduction exists for. The guard script is now a provenance path of its own, so a change to it re-runs the reproduction it implements. Left alone deliberately: run-process.ts and the workflow's `paths:` scope over src/shared, which is a pre-existing gap for the whole mobile workflow rather than this job's. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): refuse to reproduce when the suite list has drifted from the files Round-2 review. The suite names reach vitest as positional filename filters, and vitest exits 0 when only some of them match. A renamed census suite therefore dropped out of the reproduction silently and the guard still printed that the corpus reproduces: three files and 761 tests instead of four and 762, exit 0. Resolve every name under the recorder overlay before spawning, and throw naming the drifted entry. The unit case walks the list and omits each name in turn, so no single rename can slip past it. This is the same fail-open shape as the renamed-pathspec finding. Also: pass an explicit directory type to `symlink`, since Windows needs one and a junction needs no privilege where a real symlink does; and build the throwaway test repositories with `symbolic-ref` rather than `--initial-branch`, which needs git 2.28 against a declared baseline of 2.25. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Jinwoo Hong
|
229dd62cab |
test(mobile): repin the RPC recording corpus to main after #21169 (#21173)
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
 Brennan Benson
|
73b33302b9 |
Show the Source Control AI CLI arguments box only where it actually works (#21149)
* Show the Source Control AI CLI arguments field only where it applies * Fix Source Control arguments on remote launches |
||
|
Jinwoo Hong
|
01a1b6b024 |
refactor(mobile): checked reply readers for the tasks item and list domain (step 7) (#21169)
* refactor(mobile): checked reply readers for the tasks item and list domain (step 7)
Thirty-eight unchecked reply readers across four tasks files become checked zod
readers, so a malformed host reply surfaces as one `RpcIncompatibleReplyError`
naming the method instead of a downstream `TypeError`, a rendered `undefined`,
or a sheet left ready over garbage. Deliberately a behaviour change on malformed
replies only; nothing on the wire moves.
mobile-task-item-state-operations.ts 17
mobile-task-item-detail-operations.ts 8
mobile-task-item-comment-operations.ts 7
mobile-task-list-operations.ts 6
Two rules decide every schema, and both are stated in
task-provider-entity-reply-schema.ts:
1. A member is required only where a tasks consumer reads it with no guard.
Everything reached through `?.`, `??` or a `typeof` test stays optional,
because a reply without it rendered the same fallback then and now.
2. No member is required that the site's own recorded `normal` reply lacks. The
corpus is the only evidence of what a host really sends at each site, and
requiring a member absent from that control would turn a good reply into an
incompatible one.
Rule 2 holds two schemas at the container: `github.prFileContents`, whose
recorded reply is `{ oldContent, newContent, truncated }` where
`getPRFileContents` returns `{ original, modified, ... }`, and `gitlab.todos`,
whose recorded row is not a `GitLabTodo` and whose `normal` partition therefore
records main crashing in `actionName.replace`. Both still gain their container,
which is what names a reply that is not an object or not a list. Correcting
those two scenarios is the follow-up that unlocks narrowing the rows.
Nine writes share one envelope reader and five comment writes share another:
`ok === false` and `error` are one host convention across them, and no input
would make two of them want different answers. The acceptance, the name and the
recorded family stay per operation. Three readers are reused rather than
re-declared — the session domain's boolean confirmation for `setPRFileViewed`
and `resolveReviewThread`, and its salvaged-member combinators throughout.
Three call-site shape tests the reader now answers for are deleted: both
`Array.isArray(payload)` guards on the checks read and the
`typeof count === 'number'` fallback on the item count. `GitHubPRFileContents`
is widened to optional members, which is what the reader can promise, and
`buildGitHubPrFileDiffPreview` takes the widened sides — `splitContentLines`
already treated a falsy side as no content, so no runtime behaviour moves.
The tasks source-parity hashes are refreshed: hook, statement, declaration and
render-token counts are unchanged, the render-token hash does not move at all,
and `semantics` is a pure deletion of ten lines.
Inventory: 137 unchecked readers over 30 files becomes 99 over 26.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the RPC recording corpus and re-record the tasks reply deltas
`baseline` moves to
|
||
|
Brennan Benson
|
e42f7c00bd |
feat(native-chat): render a proposed plan as a plan, not a generic approval (#21090)
* feat(native-chat): render a proposed plan as a plan, not a generic approval A finished plan arrives as an ExitPlanMode tool call. With no handling for it, the generic approval path serialized the tool input, so a plan appeared as thousands of characters of escaped JSON. A plan is content to read, not a privilege to grant. Classify the plan in the permission callback and carry it as a typed subject on the approval item, keeping the existing approval kind so the prompt still reaches every consumer. Mobile filters pending approvals on that kind, so introducing a new one would have made the prompt vanish there silently. Classification runs before registration, so a future permission-mode short-circuit cannot swallow a plan proposal. The assistant tool-use stream is a second ingress and is pinned by its own test, because neither path can be assumed to fire on its own. Rather than adding a second card, the plan renders inside the approval card's existing bounded content region. It inherits the height cap, the scrolling, the keyboard focus and the pinned action row that region already provides, and a typed plan replaces the raw detail instead of rendering both. Buttons read as plan decisions. Mobile renders the same subject through its own markdown component in the same region. * fix(native-chat): preserve plan review semantics * fix(native-chat): keep plan approval one-turn |
||
|
Jinwoo Hong
|
5947d6b269 |
infra(relay): raise asia-east2 cell pools to 16 and record the measured connection ceiling (#21163)
* infra(relay): raise asia-east2 cell pools to 16 and retire four idle cells The three asia-east2 cells sit 176 ms from the Cloud SQL instance in us-central1. Server-side statement time there is 0.2 ms, so a pool slot is held by the round trip, not by the query. At a pool of 10 they measured 94-156 waiters and 2 s waits, and client accepts ran a ~4 s p95 against 222-646 ms in us-central1. Raising those three pools to 16 is the agreed first step; every other cell stays at 10. c4 and c5 join the committed fence set. Both are existing-only capacity the admission selector can never place on again, they carried ~1 connection each on 40-day-old images, and each still holds 10 Postgres connections. The fence set is the prerequisite the fence-source workflow confirms before it drains and attests a cell; it is not itself the resize. c17 and c18 are not fenced here. They are migration-only, and the runbook requires retire-migration-cell to move a migration-only cell to existing-only through a generation-bound selector CAS before it can be fenced. Terraform cannot express that step. The Cloud SQL consumer contract carried two stale numbers: auth at 2 instances when production has run a cap of 20 since 2026-09-04, and a 400-connection ceiling when the live instance reports 500. Both are corrected, and the budget now asserts its headroom in two named gates instead of one aggregate boolean. Those gates fail: auth alone accounts for 200 configured connections and a 215-connection rollout overlap, so the operating maximum is 713 against a usable ceiling of 490. Nothing here caused that, and no pool was lowered to hide it. * infra(relay): move the Cloud SQL contract correction out of this branch The contract correction (auth at its real 20-instance cap, the measured 500-connection ceiling) makes the budget gate fail for reasons that have nothing to do with asia pools or fenced cells, and it held this branch red. It moves to its own branch where the failure is the subject. production-cloud-sql-app-consumers.json returns to main unchanged. The budget test keeps main's single gate and only repins the cell figure that this branch genuinely moves: 230 -> 228, being +18 for three asia pools at 16 and -20 for fencing c4 and c5. Against main's 400-connection model that leaves an operating maximum of 383 under a usable ceiling of 390. * infra(relay): move the c4/c5 fence entries out of this branch Terraform now sets a cell's MIG target size directly from relay_gce_fenced_cells (relay-gce-cells.tf); the lifecycle ignore that used to protect operational target_size drift is gone. So a fence entry sitting on main ahead of its fence-source run is a standing instruction that any apply reaching that cell may execute without the documented drain and attestation. Keeping the entry in the same merge as an unrelated pool change widens that blast radius for no reason. The two entries move to their own branch, to be merged immediately before fence-source runs for c4 and then c5. This branch keeps the multi-line reflow of the list, which makes that later diff two added lines instead of a rewritten one. The cell figure in the budget test follows: 230 + 18 for the three asia-east2 pools at 16, with no fenced-cell subtraction. That is 403 operating against a usable ceiling of 390, so the headroom gate now fails by 13. It fails against a ceiling of 400 that is itself wrong; the instance reports 500. See the PR body. * infra(cloud-sql): record the measured 500-connection ceiling The budget's usable ceiling came from maxConnections: 400, described as the tier default. It is a tier default, since no max_connections flag is set, but the instance does not report 400. SHOW max_connections on it returns 500, measured 2026-09-16. On main the model sat at 385 against a usable ceiling of 390, five connections of margin, so raising the three asia-east2 pools by 18 failed the gate by 13 against a ceiling that was never checked. Against the measured one it is 403 against 490, clearing by 87. Only the ceiling and its source note change here. auth stays recorded at 2 instances, which is also wrong; PR #21165 corrects it, and with the true auth figure the budget is over by 225 for reasons that have nothing to do with these pools. * test(cloud): state the cell pool arithmetic literally in the budget pin comment |
||
|
Brennan Benson
|
68ea3b92e3 |
fix(native-chat): stop a collapsed run claiming success when a tool call failed (#21151)
* fix(native-chat): stop a collapsed run claiming success when a tool call failed A settled activity group drew its completion mark whenever no call in it was `running`. That is not a success test: a tool call is `running`, `completed` or `failed`, so a run whose call failed had nothing running, took the mark, and asserted success over a failure the reader could only find by expanding the run. Success is now stated rather than inferred. `nativeChatToolRunSucceeded` grants the mark only to a run that is settled, has nothing still running, and has no failed call — a call's own `failed` verdict or an error result, the same composite test the task-list, edit-card and ask-row readers already use. A call with no lifecycle state is neither, so legacy transcripts still settle. A collapsed run that did contain failures now says so in the header, as a quiet `N failed` in the header's own mono type with a spoken `Failed tool calls: N`. Text only: a tool error is routine work, so no destructive tint and no swapped glyph. The count is taken over every call in the run, not the latest. * fix(native-chat): count failed tool calls without result mispairing |
||
|
Jinwoo Hong
|
69787e763a |
fix(relay): serve readiness from last-known-good during auth or SQL blips (#21161)
* fix(relay): serve readiness from last-known-good during auth or SQL blips
The load balancer health check hits /ready, which re-probed the auth JWKS
endpoint and Postgres on every poll and reported not-ready on the first
failure. On 2026-09-16 an auth outage therefore took every cell out of the
load balancer within ~30s and dropped every connected host, even though the
token verifier caches keys in process and kept verifying tokens.
/ready now remembers when each dependency last answered and keeps reporting
ready while the failed one stays inside a grace window
(ORCA_RELAY_READINESS_GRACE_MS, default 15 minutes, 0 disables). A process
that has never succeeded still gates on the real dependencies, so cold boot
is unchanged. Grace answers carry degraded plus the failure reason on the
existing readiness observation, and entering or leaving grace logs once.
MIG autohealing still uses the dependency-free /health endpoint.
* fix(relay): split readiness grace per dependency and probe both every poll
Review follow-ups on the last-known-good readiness window.
An unset environment variable arrives as an empty string, which z.coerce
reads as 0, so the single ORCA_RELAY_READINESS_GRACE_MS would have switched
the window off instead of falling back to its default. The two replacement
variables preprocess '' to undefined.
JWKS and SQL now get separate windows and separate clocks:
ORCA_RELAY_READINESS_JWKS_GRACE_MS defaults to 15 minutes, and
ORCA_RELAY_READINESS_SQL_GRACE_MS to 3 minutes. Each cell is its own load
balancer backend, so failing readiness never re-routes a host, it only makes
that hostname unreachable, and a host that lands on a SQL-dead cell gets
WRONG_CELL and is re-placed by the director. Three minutes rides a Cloud SQL
failover without hiding a per-cell fault for a quarter of an hour.
Both dependencies are probed on every poll. A JWKS failure used to
short-circuit the SQL probe, which let the SQL clock age with no evidence
behind it. Grace transitions are emitted per dependency, so JWKS recovering
while SQL fails logs both sides instead of nothing.
/ready keeps its 200 and its {ok:true} body when healthy, and adds
degraded plus the dependency list when the answer comes from a window.
|
||
 Brennan BensonandMerge Sim
|
5287c5cdbc |
fix(mobile): stop a created tab from jumping when the host snapshot lands (#20069)
* fix(mobile): stop a created tab from jumping when the host snapshot lands
Creating a tab from the mobile session strip painted the new tab at the end
of the strip and then visibly jumped it to a different slot a beat later.
The client asked the host to insert the tab after the active tab, but then
predicted a different placement for its own optimistic paint:
afterTabId: activeSessionTabId ?? undefined // host: splice(insertAfter + 1)
...
return [...prev, { ...created, isActive: true }] // client: append
Two independent placements that disagree, so the optimistic frame is wrong by
construction and the tab snaps to its real slot on the next published snapshot.
The disagreement dates to
|
||
  
|
0e3b71f605 |
fix(session): give an SSH workspace one owning partition so its tabs stop round-tripping as deletions (#19572)
* fix(session): give an SSH workspace one owning partition so its tabs stop round-tripping as deletions `workspaceSessionPartitionHostId` answered differently depending on who asked: the renderer mapped an SSH worktree's session to the `local` blob, the main-process runtime read-modify-wrote `ssh:<targetId>`. One workspace's session lived in two stores and no reader reunited them, so whatever landed on the unread side did not read as unknown — it round-tripped as absence. The remote-workspace upload is a `replace-session` patch, which turned that absence into deletion on the host, and the next pull applied the deletion locally and re-poisoned the snapshot. Collapse the two answers into one: every non-'local' host owns its partition. Boot hydration and the export fallback now read the SSH partition, and rows a shipping build left in `local` are folded back in once, gap-filling only — an empty tab row is a gap, never proof that anything was closed. Folder workspaces deliberately keep their existing 'local' routing: boot discovers SSH partitions from the repo catalog, so an SSH target that owns only a folder workspace has no partition any reader enumerates. They are still adopted back out of an SSH partition when a repo does name the host. Fixes #12721 Supersedes #12722 Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * test(session): pin the old-client empty-publish skew direction * fix(session): adopt every workspace the host partition names, not only tabbed ones Review caught that gating adoption on `host.tabsByWorktree[key].length > 0` traded the #12721 deletion for a narrower one. The write path routes EVERY worktree-scoped field to the owning partition, so an SSH workspace with open editor files or browser tabs and no terminals had all of it dropped on every restart — and unlike terminal state it cannot be recovered from the host snapshot, which carries terminal fields only, so an unsaved `dirtyDraftContent` was destroyed outright. The defect was not a missing field. It was a hand-maintained field list deciding what the read recovers while the write used the ownership table, so the two could disagree. Adoption now walks `WORKSPACE_SESSION_FIELD_OWNERSHIP` with an exhaustive switch, and a new ownership kind is a compile-time decision rather than a silent omission. Session keys are normalized through the shared `normalizeWorkspaceSessionKeyToWorkspaceId` so host-qualified visit recency (`ssh:target|worktreeId`) reaches its workspace, and the regression is pinned by feeding the shipping split's own output back through the real boot read rather than a hand-built fixture. Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> * fix(session): stop adoption overwriting rows it was never told about Three losses, one cause: the reader walks its own description of the partition layout while the writer walks another, so the two agree on which ownership kinds exist and not on what a kind means. - An empty host row replaced a populated base row, destroying an unsaved dirtyDraftContent the header comment says must never be destroyed. The host holding nothing is not evidence the base is wrong. - A contested bare id was adopted as if local and ssh:<target> were one workspace written twice, which is exactly the id where that premise is false. The read already reached that verdict and adoption could not ask for it, so it is passed in; contested keys are gap-filled, never replaced. mergeWorkspaceSessionsWithHostShadow now reports the real contested set, which primaryHostBySessionKey never was. - Tab-, pane- and file-keyed rows are adopted through the split's own indexes, so unified-only tabs come back and the pane key is parsed once. - A bare lastVisitedAtByWorktreeId key only fills a gap; the split has a dedicated branch for that field and the reader had none. * test(session): pin the tombstone/gap boundary the two readings meet at An explicit empty tabsByWorktree row means the user closed the last terminal; adoption reads an empty base row as a gap to fill. Same value, opposite readings, so the boundary is asserted rather than argued: the tombstone lands in the owning partition, restores as a present empty row rather than a deleted key, is declined by the real seeding predicate, is published as an empty list, and the legacy-transition resurrection happens once and cannot recur. * docs(reliability): record the adoption guards and the tombstone boundary in the gate * test(e2e): read the SSH restart assertions from the partition that owns them ssh-cold-activation-restore asserted persistence through session.get() with no host, which is the local partition an SSH worktree's rows no longer live in. The invariant it means to check is that the state is persisted where the boot read will find it, so it now unions local and ssh:<targetId> and stays correct on both layouts. Confirmed the product invariant separately rather than by the edit: the behavioural half of both tests - the full app restart, the active worktree, the eager terminal remount and the PTY-owner reclaim against a real Docker OpenSSH host - runs after this check and passes. 2 passed in 48.9s. * test(e2e): read ssh-restart-tab-accumulation from the owning partition too Same layout-coupled read as ssh-cold-activation-restore: the pre-quit flush asserted through session.get() with no host. Verified against a real Docker OpenSSH target - both repeated quit/relaunch cycles keep exactly the restored SSH tabs, no accumulation and no loss. 2 passed in 52.9s. * fix(lint): clear the casting gate on the partition adoption main tightened typescript/consistent-type-assertions to assertionStyle: never, which the rebase brings onto these added lines. Most of the round-trip fixtures did not need a cast at all -- three were hiding wrong-shaped literals (a browser workspace keyed 'name', a unified tab keyed 'type', a layout keyed 'direction'), now written as the types they stand for. The adoption reads narrow through an isRecord predicate instead of casting, which also stops a null entry throwing out of Object.keys. What is left is dynamic-field writes and unknown-typed IPC returns, each with its own SAFETY rationale. * fix(session): give an SSH folder workspace one owning partition boot can find The partition owner rule already names `ssh:<targetId>` for a repo-backed worktree, but `getFolderWorkspacePartitionHostId` still answered 'local' for a folder workspace while main's `RuntimeWorkspaceSessionController.getPreferredHostId` answered `ssh:<targetId>` for the same key. That is #12723 unfixed for folder workspaces, and once the renderer started writing `ssh:*` at all it got worse: a save's field-level patch carries only the rows routed to that partition, so a `tabsByWorktree` write without the folder row erased the row main had put there. The reason the renderer could not route there was real - boot discovered SSH partitions from the repo catalog, which cannot name a target whose only workspace is a folder. So persistence now answers that directly over `session:list-host-ids`, and boot reads the partitions that exist rather than the ones a catalog implies. Removing a folder workspace prunes its rows from the owning partition too, or the census would adopt them back on the next launch as a workspace the user already deleted. Adoption now decides from the repo catalog instead of from co-presence. Two partitions holding one bare `repoId::path` is not evidence of a collision - that is the exact shape the repair exists for - so the verdict comes from `resolveWorktreeExecutionHost`: a repo id registered on more than one host is contested and may only be gap-filled, and one the catalog positively resolves to a different host is residue this partition does not own and is not adopted at all. Without the second rule a stale partition sorting first won the read and was then written into the live one. Nothing is deleted either way; the rows stay where they are. Finally, a workspace adopted out of a partition now routes back to that partition. Routing used to re-derive an owner from the catalog, so a boot whose repos had not hydrated moved the rows it had just reunited back into 'local' and re-stranded them. Contested ids are withheld from that override, because routing the whole bare id to one host is the loss the gap-fill prevents. The publish path resolves each workspace's owner once for the whole publish, shared with the projection, so the per-target catalog attribution does not repeat it per connected host. * fix(session): drop a deleted workspace from every partition, not just the local blob Adversarial review of the previous commit found three ways the partition census - which now reads whatever persistence holds rather than what the repo catalog implies - keeps rows alive that nothing should keep alive. `deleteProjectGroup` pruned only the local blob, so every folder workspace under a deleted group left its rows in `ssh:<targetId>`; the next boot adopted them back, named that partition their owner and wrote them there again, forever. `removeFolderWorkspace` had the same hole for a workspace whose partition its host expression could not name: main never persists a folder workspace's `executionHostId`, and `RuntimeWorkspaceSessionController` can infer a connection from the group's repos that the workspace row itself does not carry. Deriving the partition at delete time is the wrong question - a deleted workspace owns nothing anywhere - so both paths now remove it from every partition. The third is on the read side. A contested id is deliberately withheld from the read-source override so the write cannot carry one host's rows into another's partition, but the routing that then re-derives an owner answers 'local' for an id the catalog cannot name. Adopting such a row moved it out of the partition that owns it and into the blob: the two-store split this change exists to remove. A contested id the assembled session holds no row for is therefore not adopted at all. Gap-filling stays available for a contested id the session already names, since that row's own partition is what the write follows. Declining to adopt leaves a row invisible for one boot; it never deletes one. Also: the folder-key guard in both catalog attributions was dead, because `getRepoIdFromWorktreeId` hands back the whole key rather than nothing when there is no `::`. The verdict was right and the resolution wasted; it now skips by shape. And the two type assertions the changed-code casting gate rejected are gone rather than suppressed. * fix(session): park the rows a partition read declines instead of letting the next write erase them A partition write replaces each field with exactly what the unified session routed there. So a row the read left out of that session is erased from its own partition the moment any sibling workspace writes the same one - and with SSH partitions now the owning store, that row is then in no partition at all. Three separate decisions produce such rows: residue the catalog attributes to another host, a contested id withheld so the write cannot carry one host's rows into another's partition, and a workspace the base already holds the live copy of. Declining to show a row was quietly deleting it. The machinery for this already exists. `attachHostSessionShadow` writes a contested runtime co-claimant's parked rows straight back into its own slice before the write, so the primary's write cannot erase them; the ssh partitions simply were not among the slices the contention split arbitrates. The read now parks everything it is not returning to an ssh partition into that same shadow, and the existing re-attach puts it back. Leak, never kill - docs/reference/ssh-execution- boundary.md - and a row no partition holds is unrecoverable. Second, the contested branch of the tab adoption read `Object.hasOwn` as "the base has tabs here". An empty list satisfies it, so whenever a legacy id happened to be contested, #12721's empty local row won over the host's real one - the exact reading the module's own header, and the gate invariant it is pinned by, say is wrong. An empty row is the gap this repair fills, so it is now treated as one. * test(session): pin the empty-base-row gap for a contested id Mutation testing found the assertion missing: reverting the gate to `Object.hasOwn` left all 39 assertions passing, which makes the fix that reads an empty base tab row as a gap unguarded. The #12721 shape does not stop being a gap because the id happens to be contested. --------- Co-authored-by: Robert Nisipeanu <github@nisipeanu.com> Co-authored-by: Jinwoo-H <Jinwoo-H@users.noreply.github.com> |
||
|
Neil
|
a46b5b15ec |
fix(worktree): ask the execution host whose home a remote delete would take (#19865)
* fix(worktree): ask the execution host whose home a remote delete would take
`isDangerousWorktreeRemovalPath` read `os.homedir()` — the machine running
Orca — and then applied POSIX-only shape rules. SSH orphan cleanup feeds it
remote paths, so a Windows host profile (`C:\Users\bob`) was unrecognised from
a macOS/Linux desktop and the recursive delete lost its last guard, while a
coincidental client-home prefix could refuse a legitimate remote delete.
The removal route already resolves one execution host for the whole removal;
it now resolves one home authority the same way. `WorktreeRemovalHomeAuthority`
is `{ kind: 'client' }` or `{ kind: 'executionHost'; homePath }`, required at
every guard entry point, so the ambient read is unreachable from a remote
removal. The host's answer is the `$HOME` the SSH session already read on the
host during relay deploy — no new probe. Unresolved stays `null`, meaning
unknown, never "same as this client's".
Path-shape rules now cover Windows profiles (`C:\Users`, `C:\Users\<name>`,
any drive or UNC root, case-insensitively) and WSL UNC aliases, which front a
Linux filesystem and so take the POSIX shapes.
Fixes #18275
* fix(worktree): merge the duplicated removal-route import
The focused code-quality plugins deny `import/no-duplicates`.
* test(worktree): pin the IPC removal call site and the unknown-host-home refusal
Mutation testing found four survivors in the home guard:
- Swapping the IPC unregistered-removal call site to the client's home passed
every suite while the remote delete could reach the host's own home. Only
the runtime call site was pinned. Add the mirror test for the IPC path.
- Falling back to os.homedir() when the execution host reported nothing was
indistinguishable from refusing; the client home never coincided with the
probed path. Assert the fallback stays off with homedir pinned to the path.
- Comparing an execution-host home across path syntaxes survived because no
row exercised the win32 home under POSIX ops: path.resolve manufactures
<cwd>/C:/Users/bob, which every ancestor of the cwd contains.
- Dropping the bare /Users rule survived; add the row.
Also cover the forward-slash C:/Users/bob form normalizeRemoteHome reports
for a Windows host, which no existing row used.
* ci: re-run after an unrelated Electron probe startup timeout
* fix(lint): clear the casting and max-lines gates on the home guard
Rebasing onto main brings two gates this branch predates:
typescript/consistent-type-assertions at assertionStyle: never, and the
300-line ceiling that the added home lookup pushed
orca-runtime-remove-managed-worktree.ts past. The fixture casts carry
per-site SAFETY rationales; the route's git-options-and-listing step
moves into its own module, which also stops the local/SSH branch being
spelled twice in one expression.
* fix(lint): name the home predicate for what it matches
main enabled anti-slop/no-shape-in-symbol-names (#20785) after this
branch was written. The predicate answers whether a path IS a home root,
not whether it resembles one.
* fix(worktree): refuse a removal the execution host cannot vouch for
Review of the home guard found three ways it still let a delete proceed on
evidence about the wrong machine, or on no evidence at all.
`getPathOps` switches to win32 as soon as EITHER the worktree path or the repo
path looks Windows-absolute, and `//nas/share/repo` does. A POSIX worktree path
was then judged by Windows-only shape rules, which recognise `<root>\Users\<name>`
and nothing else, so `/home/alice` — and any client home outside `\Users` —
stopped matching and the last guard in front of a recursive delete went quiet.
The home question involves the worktree path and a home, never the repo path, so
the predicate now reads the path in its own syntax as well and refuses if either
reading names a home. A union of refusals can only ever refuse more.
An execution host that never reported its `$HOME` is `unverifiable`, and
`unverifiable` does not authorise a delete. `isRemovalHomeAuthorityResolved`
gates the two paths that recursively delete a directory —
`canSafelyRemoveOrphanedWorktreeDirectory` and
`canCleanupUnregisteredOrcaLeftoverDirectory` — because the orphan proof they
accept, a `.git` file at the top of a directory, is also what a bare-repo
dotfiles `$HOME` looks like, and there the guard is the only evidence there is.
`git worktree remove` is deliberately not gated: the host's own Git registry
already established that the path is a linked worktree of that repo, and a
missing second opinion does not retract a first one. An empty `$HOME` is
normalised to unanswered rather than read as a resolved home.
The IPC entry point spelled its host two ways. The metadata prune, the
archive-hook route and now the home authority came from
`getRepoExecutionHostId(repo)`, while the `git worktree list` and every delete
came from raw `repo.connectionId`. A row carrying only
`executionHostId: 'ssh:<target>'` therefore listed a remote checkout on this
client and deleted a same-named local path while the guards vouched for the
remote one; the mirror row did the reverse (#11163, previously fixed on the
runtime path only). Neither spelling is evidence about the other, so a row that
carries two host names is refused before anything is listed or deleted. Both
sides are spelled by `getRepoExecutionHostId`, so they can differ on content but
never on normalisation.
A `runtime:<env>` row refuses here for the same reason. It is not reachable
through this handler today — the renderer sends environment targets to
`worktree.rm`, and the host-qualified catalog refuses to list a runtime host —
so that arm closes a door rather than changing a flow.
Fixtures that register an SSH provider now report a host home, because a
connected relay session always has one: `remoteCliBridgeEnv` is assigned before
`registerSshGitProvider`, is never cleared, and providers are unregistered
before the session leaves `activeSessions`. The wiring lives in its own module
called from the harness rather than in `worktrees-test-module-mocks`, which
`vi.mock` factories import: reaching the production route module from there
pulls in `providers/ssh-git-dispatch` while it is being mocked, and the module
runner deadlocks.
* fix(worktree): compare removal host names after decoding, not as stored text
`getRepoExecutionHostId` returns a row's `executionHostId` as stored, while the
same row's `connectionId` is re-spelled through `toSshExecutionHostId`, which
percent-encodes. A byte compare of the two would refuse a perfectly consistent
row over a `%20`, so the two host ids are now compared after `parseExecutionHostId`
has decoded the target id out of each.
`runtime:<env>` and an unparseable id decode to no machine at all and match
nothing, including each other — a runtime-owned row has a null `connectionId`
and would otherwise read as local, which is a delete on this client.
* fix(lint): clear the static-analysis gates on the removal home authority
The type-aware audit rejects a `default` arm on a discriminated switch, so the
host-kind switch names `runtime` and `undefined` outright — which also makes a
host kind added later a compile error here rather than a silent fallthrough.
The two test casts the changed-code gate flagged are gone: the leftover-cleanup
meta is typed instead of asserted, and the unparseable-host-id case narrows to
`ExecutionHostId` with the SAFETY rationale the gate asks for.
* docs(worktree): say why an unroutable removal host is refused by a plain compare
The comparison refuses `runtime:<env>` because only the left operand can name
no machine — `repoRowHostId` comes from `connectionId` and is always `local` or
an `ssh:` id. That invariant was doing the work silently; an explicit null test
in its place was a branch no input can reach, so the reason is written down
instead.
* fix(worktree): gate the registered removal on the host home answer too
I argued `git worktree remove --force` did not need the host's home answer,
because the host's own Git registry had already established that the path is a
linked worktree of that repo. That is true and it is not enough: `git worktree
add` accepts a pre-existing empty directory, and that directory can afterwards
be somebody's `$HOME` — a build account's home, a container's `HOME=/workspace`.
Being a linked worktree proves provenance, not that the path is not a home, and
the remove deletes the checkout either way.
With the host's answer that case is already caught by containment. Without it
only the path shapes remain, and a home at a non-standard location
(`/var/home/<u>`, `/export/home/<u>`, `D:\\Profiles\\<u>`) has no shape to match.
So `findRegisteredDeletableWorktree` now requires the answer as well, and every
gate that authorises a delete is on the same rule.
The fixture that models a connected relay session moves out of `ipc/` and is
shared: four runtime specs register an SSH provider without one, and a live
provider implies a reported home in production.
|
||
|
Neil
|
e45cf438bc |
fix(runtime): park a mirrored pane's resume until its PTY handle lands (#19882)
* test(repro): #19735 resumes a published mirrored pane before its handle lands * fix(runtime): park a mirrored pane's resume until its PTY handle lands Mirror hydration means the host's tab rows arrived, not that a given pane's liveness is decidable: the PTY handle lands one relay round trip later. On that frame the pane read as not-live and the sweep resumed a session the host was still running, producing a duplicate resume tab. An empty handle map for a published row is unverifiable, never exited. Park the pane on a per-pane wait with three bounded exits, each replaying the sweep: its own handle lands, the row is retracted, or a deadline expires. The deadline decides resume rather than an indefinite hold, and is scoped to the connection generation so a reconnect re-arms it. Closes #19735 * fix(runtime): bound the handle-gap expiry map to the current connection * fix(runtime): void a handle-gap verdict the reconnect made stale The per-pane park bounds itself with one deadline per connection, but the waiter never recorded WHICH connection it was armed on. A wait armed on generation 0 that fires after a reconnect stamps its expiry against the current generation, so hasHostMirrorHandleWaitExpired agrees, the mirror lookup returns null, and the pane is resumed after 1ms on a connection that has had no chance to publish the handle. That is #19735's fork with an extra step, reached through the guard that exists to prevent it. The module's own doc comment claims the opposite -- "a reconnect bumps the connection generation and arms a fresh wait" -- and that is true only for a wait which had ALREADY expired, which is precisely the case the existing test covered. The test and the comment agreed with each other and both were wrong about the live case. The waiter now carries the generation it was armed on and records no verdict when the generation has moved; the replay re-parks through the existing machinery and the new connection gets its own full budget. Still bounded per connection generation, which is what was documented all along. Also pins the three sibling attacks on the same window: two panes in one environment where only one handle lands, a handle published by a foreign environment, and an environment tearing its rows down mid-park (which leaves no waiter and no scheduled timer). The test file now leads with how to assert on this module at all, because the obvious shape cannot fail. "Did the waiter release" is not an observable here -- a waiter released for the wrong reason is re-parked by the replayed sweep, so the store reads identically one tick later, and a mutation releasing every waiter on any tab's handle survived twelve assertions written that way. What a spurious release costs is the deadline, so the assertions advance the clock and require the pane to decide on the ORIGINAL schedule. * fix(terminal): a live pane owns its transcript in any workspace The resume dedup was scoped to the record's own workspace on both terms -- the entry's tab had to be in worktreeTabIds AND entry.worktreeId had to match -- and additionally required entry.state !== 'done'. A record whose peer pane has finished a turn and still holds a live PTY therefore matched nothing, and the sweep launched a second agent onto a transcript the peer is still writing. Cross-workspace, it matched nothing even while the peer was mid-turn. The two ids really do drift. canonicalizeTerminalSessionWorktreeId re-keys tabsByWorktree, tabGroups, tabGroupLayouts, activeTabIdByWorktree and activeGroupIdByWorktree onto the canonical worktree id, and does NOT re-key sleepingAgentSessionsByPaneKey, whose records carry worktreeId inside them. So adopting an orphaned terminal is a direct producer of a record naming one workspace while its pane and status row name another. Split into two arms rather than widening the existing condition. The new arm carries no workspace scope but demands hard evidence: a provider session id names one transcript, so a pane whose exact PTY is live right now already owns it wherever that pane sits, and no workspace boundary makes a live PTY less live. The scoped arm keeps its scope and its state !== 'done' term, because a status row with no live PTY is a claim about the past and must not reach across workspaces. Relationship to #19736: that PR fixes the SAME-workspace half of this in the same function, by relaxing only the status term. This arm covers that cell too -- measured both ways on this branch, which does not carry #19736: its thirty `checks exact live ownership before resuming` cases all pass with this change alone, and ten of them fail without it. So this supersedes #19736 rather than sitting beside it, and #19736's one-line `export` of stablePaneHasLivePty is carried here because this arm needs it. If #19736 lands first this becomes a pure widening and its tests should be kept. Both cells are pinned here either way. * fix(runtime): isolate one pane's replay from the handle-gap drain One store write releases every due pane, and the drain runs synchronously inside a zustand subscriber. `waiter.run()` was unguarded, so a single pane's replay reached two things it has no business touching: - the throw escapes out of `useAppStore.setState`, meaning the mirror apply that published the PTY handle throws at its own call site; - every pane queued behind the thrower is stranded — waiter still parked, deadline still armed — and then decides on a connection whose evidence landed long ago. The deadline path fans out the same way, so a throwing replay also escaped the timer callback. Reachable: `resumeSleepingAgentSessionsForWorktree` reaches `state.createTab` with no guard of its own. The panes in a drain are strangers to each other and to the frame that released them; none of them should be able to see another's failure. The new tests live in their own file because host-mirror-handle-gap-resume.test.ts drives the waiter through the real resume sweep and so cannot choose what a replay DOES. Note for anyone extending that file: per its header, "did the waiter release" is not an observable here — a spurious release is re-parked immediately and reads identically one tick later. These tests assert on timer count and on the deadline instead. Also records two findings next to the code, so they are not rediscovered: `expiredGenerationByPane` is never pruned for a removed environment (bounded and inert, since removal advances the generation, but it does not drain — and a DIFFERENT leak in that same map is being fixed concurrently, so reconcile rather than patch around it); and sustained reconnect churn holding a pane parked indefinitely is CORRECT, not the latch-that-never-releases defect, because under churn liveness genuinely is unverifiable and ssh-execution-boundary.md forbids resolving that to `exited`. It has the shape of the defect and will eventually be "fixed" by someone who does not know that. Mutation: dropping the guard kills exactly the three new assertions and leaves all twelve existing waiter tests passing. * fix(runtime): drain a removed environment's handle-gap verdicts on teardown `expiredGenerationByPane` is pruned only by rules that run when a verdict is RECORDED — the stale-generation sweep here, and the tab-death sweep added separately ( |
||
|
Neil
|
ea01cd0ccd |
fix(windows): reject a node-pty addon that predates the MSYS breakaway denial (#20047)
* docs(windows): record the measured MSYS job-breakaway mechanism The per-PTY job already denies JOB_OBJECT_LIMIT_BREAKAWAY_OK for Cygwin/MSYS shells (#19068), but nothing records why, and a conpty.node built before that commit fails windows-msys-job.win32.test.ts in a way that reads as a source defect. Measured on a real Windows 11 host: both the plain and the exec- replacement Git Bash shapes leak, the escape is the MSYS runtime's own spawn/exec (fork keeps membership), and a single-variable A/B on usesCygwinRuntime flips the result 0/2 -> 4/4. Also names the gap the failure hid behind: node-pty-job-ownership.cjs asserts symbol presence, which cannot distinguish patch revisions. * fix(windows): reject a node-pty addon that predates the MSYS breakaway denial The native-runtime gate asserted only that terminateJob, listJobProcessIds and assignCurrentProcessToJob were exported. All three predate the Cygwin/MSYS breakaway denial, so an addon built before it passes every gate, isPtyJobOwnershipAvailable() returns true, and windows-pty-job.win32.test.ts passes 6/6 -- while every Git Bash child is created outside its pane's job and survives terminatePtyJob. Read the resolved .node and require the wide msys-2.0.dll literal that usesCygwinRuntime holds, the way stagedRelayAddonIsUnpatched() already tells a patched windows-process-tree addon from a published one. An addon the caller cannot name is refused rather than skipped: a gate that cannot see its subject is not a gate. Verified against real binaries on a Windows 11 host: the shared checkout's pre-#19068 build errors, a build from current patched source passes, a missing path errors. Also closes the cross-host packaging skip. The export half has to load the addon so it cannot run when the packaging host is not the target, which is how a Windows release built elsewhere could ship this. The marker is a file read and needs neither; an unrecognised layout warns rather than fails a release that was packaging fine. * fix(windows): check the MSYS breakaway denial on the rebuild path too The Electron probe carried the marker check, but it lives inside probeElectronNativeModules, which returns early whenever the Electron package binary is unusable. Covered by another path is not this path checks -- and the defect this whole change closes was a gate that looked like it checked. Reading the binary needs neither a loadable Electron nor an executable target arch, so assert it after the rebuild, beside the windows-process-tree assertion that exists for the same reason: this is the addon copied into the packaged app. Absent warns (a cross-platform rebuild need not leave a win32 addon on this disk); present and unmarked is fatal. The fixtures now write a real addon file, because the gate reads the binary it was told about rather than trusting the exports. Verified against the two real binaries measured on the Windows host: the pre-#19068 build fails this path, the build from current patched source passes. * fix(windows): check the marker on every ConPTY path the packaged app can load The packaged marker check read one hard-coded path, `build/Release/conpty.node`, and warned when it was absent. `loadNativeModule` tries `build/Release`, then `build/Debug`, then `prebuilds/win32-<arch>`, swallowing each failure, and `prunePackagedNodePty` drops the published prebuild only when a same-arch `build/Release` exists to replace it. So the two packages the check was added for were the two it could not see: - cross-host: no host but Windows can build conpty.node, so there is no `build/Release` and the prebuild is what ships. The check warned and returned. - cross-arch: `build/Release` is the packaging host's own arch, patched and marked, so the check printed OK -- while the target app cannot load it and falls through to the unmarked prebuild underneath. Measured, not assumed: both published Windows prebuilds in the node-pty tarball contain neither `msys-2.0.dll` nor `cygwin1.dll` in any encoding. They are the binary that leaks every MSYS pane child out of its job. It now sweeps every candidate present for the *target* arch and refuses a package with no candidate at all, which is a package with no ConPTY backend rather than a layout to shrug at. It runs for every Windows slice instead of only the branch the export check skips, so deleting the export check cannot silently take it too. A stale source build keeps the rebuild advice; the prebuild gets the advice that actually works, which is to package the slice on a Windows host of that arch. Also: the marker constant was re-typed in four places and was tied to the C++ literal that produces it by nothing at all, so editing the patch would have left a gate that fails every correctly rebuilt addon and tells the developer to do the one thing that cannot help. The fixtures now take the constant from the gate, and a test asserts the patch still adds `L"msys-2.0.dll"` to conpty.cc. And the rebuild path treated a missing addon as a warning even on the host that will run the install, where node-pty would fall through to that same prebuild. The verdict is now a value, so it is tested without a platform gate. * fix(windows): resolve the packaged ConPTY the way its loader does Sweeping every candidate and demanding the marker on all of them was wrong in the one case it was meant to make safe. `beforeBuild` runs `rebuild-native-deps.mjs --platform=win32 --arch=<target>`, so a cross-arch slice normally does get a patched `build/Release` for the target; `prunePackagedNodePty` keeps the prebuild anyway because its guard is `electronArch === process.arch` rather than the arch of the binary. That package is correct and its leftover prebuild is never reached, and the sweep failed it -- telling whoever ran it to package on a Windows arm64 host, which is both the wrong remedy and one no runner here can offer. Presence cannot separate that package from the one whose cross-arch rebuild quietly emitted the host's architecture, because the only difference is the arch of `build/Release`. So the gate now resolves the addon the way `loadNativeModule` does -- first candidate whose PE `IMAGE_FILE_HEADER.Machine` matches the target, walking root-then-lib for each layout in node-pty's own order -- and checks the marker on the one that will actually run. A package with no candidate, or none of the target's architecture, is refused: it has no ConPTY backend either way, and the second is exactly what a silently host-arch cross-build looks like. The PE machine reader already existed, privately, in the relay addon builder that needed the same "a cross-build cannot silently emit host arch" guarantee. It is now shared rather than copied. Two seams were unreachable from anything but Windows, so nothing tested them: - the afterPack hook's win32 block was an inline if/else that only a source-text assertion could inspect, and that assertion could not tell the difference between the check running and the check being wrapped in `try {} catch {}`. It is now `verifyPackagedWindowsNodePty`, and "the marker check runs even where the export check cannot" is four spied assertions instead of a string match. - the rebuild path's verdict read `process` directly, so the branch that fires only on the host being rebuilt for was dead on every other host. It now takes the host as arguments, and the fs checks, the warning and the failure are all exercised from macOS. Fixtures write a real PE header rather than `MZ fake addon`, since the gate now reads one. The machine table is pinned to the documented IMAGE_FILE_MACHINE values, because every fixture builds its header from that table and a table wrong in both entries would otherwise agree with itself. * fix(windows): say why the packaged ConPTY fell back, not just that it did The previous commit resolved the addon by architecture but still had one message for every way the resolution could land on the published prebuild. Those ways want opposite remedies, and the one it printed was the remedy the commit before it had just called wrong: - no source build in the package at all — the slice has to be built somewhere that can build node-pty for the target arch. - a source build that is there but is the packaging host's architecture, because the cross-arch rebuild did not honour `--arch` — re-running that rebuild is the fix, and "package on a Windows arm64 host" is neither necessary nor possible. The second is the common one, since node-pty publishes a prebuild for both Windows arches and prune keeps the target's on every cross-arch package. So the old text fired mostly on the case it described least. It now reports which source builds were skipped and the machine field each carried, and names the rebuild command. "Nothing the target can load" had the same problem in reverse: a zero-length or truncated `conpty.node` got a cross-architecture diagnosis. Every candidate is now named with what was actually read, including "not a PE image". The rebuild path asserts the architecture too. A rebuild that ignored `--arch` was otherwise only visible at packaging, two steps from the command that fixes it. Arches with no known machine value are left unjudged rather than guessed at. Two things the extraction broke or nearly broke, both found by mutation: - the shared PE reader answers `null` where the relay builder's private copy returned a number, which would have turned its "node-gyp ignored --arch" error into a `TypeError`. Both callers now go through `describePeMachine`. - the rebuild fixtures stage a script's co-located modules by walking its imports, and the walker only understood `from '...'` — so the gate's new `require('./windows-pe-machine.cjs')` was left behind and every subprocess test failed with a resolution error, which is the exact failure its own comment warns about. It now follows `require` and bare side-effect `import` as well, and has tests; the fixture stages the gate by walking it rather than by naming one file. Fixtures write real PE headers through one shared builder instead of three hand-rolled ones. * fix(windows): run the node-pty addon gates on the Windows job that can `rebuild-native-deps-node-pty.test.mjs` carries four `skipIf(platform !== 'win32')` tests. The full suite runs on ubuntu, and the Windows PR job runs an explicit file list that never named this file -- so those tests were skipped on Linux and never reached anywhere else. Three of them predate this branch. The Windows job is added the four node-pty addon suites plus the module-walker one; the comment above that list already says why it is the right place, which is that the addon assertions only hold once natives have been rebuilt. Running the path-joining suites there also covers the separator this gate's candidate list is built from. The rest is round-three review: - the rebuild-time arch assertion told a reader "node-gyp did not honour --arch" about a file that was not a PE image at all, which is a truncated or quarantined artifact and a different command to run. The two now read differently, and neither claims the other's cause. Same fix the packaged gate had one commit ago, in the place that had not had it yet. - the missing-addon error said node-pty "would load" a prebuild without checking it is there. It says "fall through to" now, which is true either way. - `isLoadableByArch` had no caller left once the packaged gate started needing the raw machine field for its message. Removed rather than kept warm. - each candidate's header is read once instead of up to three times. - the module walker's comment claimed every shape that reaches a co-located module; it does not follow `projectRequire`/`requireLocal`, and it must not -- those specifiers resolve against the project root, so following one stages the wrong path and the copy fails. Proven by trying: widening the pattern to require-shaped names broke nine tests on `projectRequire('./config/scripts/...')`. The comment now says what it follows and why it stops there. - a new test resolved a file URL with `.pathname`, which keeps the drive-letter slash on Windows -- the very job this commit adds it to. * docs(windows): put the superseded export-only gate in the past tense It describes what used to pass a broken addon, so present tense reads as a description of the gate the same document then explains replacing it. * fix(windows): repair what running the node-pty suites on Windows exposed Putting these files on the Windows job turned four assertions red on the first run. Three of them were in tests that carried `skipIf(platform !== 'win32')` and had therefore never executed anywhere, on any branch. - `writeFakeElectronRebuild` emitted the `windows-process-tree` addon a real rebuild leaves but never node-pty's, so every Windows test of the rebuild path ran against a tree no real rebuild can produce: node-pty "rebuilt" with nothing in `build/Release`. The new same-host check reads that state correctly and said so. The fake rebuild now writes `build/Release/conpty.node` when it was asked to rebuild node-pty for win32, with the marker and the target machine. - `mkTempProject` never staged `windows-process-tree-creation-time.cjs`. The rebuild script reaches it through `projectRequire`, which resolves against the project root, so the module walker cannot follow it and must not try. Staged by name, with a comment saying which of the two it is. Without it the windows-process-tree probe failed to load its own checker and the module joined `modulesToRebuild`, which is the second and third red assertion. - the two `nodePtyAddonPath` cases compared against a literal POSIX string. `resolve` returns a drive letter and backslashes on Windows, so they could only ever pass off it. Built from segments now, which still pins the `..` traversal that is the point of the test. Verified on macOS: ensure-native-runtime-job-ownership, verify-packaged-node-pty-job-ownership, windows-pe-machine, script-module-dependencies, rebuild-native-deps-node-pty, rebuild-native-deps, rebuild-native-deps-windows-process-tree, ensure-native-runtime -- 109 passed, 6 skipped. The 6 are the Windows-gated rebuild tests, which is the job this change is aimed at; Windows CI is the arbiter. * fix(windows): give the packaged fallback a third verdict, for a file that is no image The packaged gate had two remedies for landing on the published prebuild and picked between them on `!prebuilt`, which puts a truncated, empty or quarantined `build/Release/conpty.node` in the cross-arch bucket: "the source build beside it is the wrong architecture ... re-run with --arch". It is not the wrong architecture, it is not an architecture, and `--arch` is not the command. The rebuild-path gate was split for exactly this a commit ago; this is the same split in the place that had not had it. Also from review of the settled state: - the stale-source-build branch ended in a call that happened to throw, so a reader could not see it was terminal and the file was read twice to get there. The verdict is now an Error the caller throws, built once from the read it already did, and shared with `assertCygwinBreakawayDenied` rather than copied. - four injection seams had no consumer in production or in tests (`deniesBreakaway`, `peMachine`, and `exists`/`peMachine` on the rebuild verdict). An unused seam is a way for the tested path and the real one to drift apart; the tests drive both with real files. Removed. - the loader table existed in a docblock and in the reference doc, already disagreeing about row four. The docblock cites the doc now. - `peImage` stamped machine `0x0000` for an arch it had no value for, because `writeUInt16LE(undefined)` coerces to zero. A fixture that quietly invents the field the gates read is the same species of silent lie the gates exist to catch; it throws, and a test holds it to that. - a test named for refusing an unreadable candidate asserted only that something threw. Renamed to what it proves. * fix(windows): make the rebuild fixtures represent a tree that can exist Second round of what running these suites on Windows exposed. The module the walker could not stage is now staged, so the probe reached its own checker and the real reasons surfaced: - `writeFakeWindowsProcessTree` exported `{}`. The creation-time gate reads `supportedProcessDataFlags` off the addon and calls its absence "the tarball prebuilt, not a build of the patched source" — correctly. The fixture predates that gate and, being Windows-only, never met it. The healthy fake now reports the flag, taken from the gate's own constant. Two tests were failing on this, the second only because the module then joined `modulesToRebuild`. - `rebuilds a loadable ConPTY native that lacks Orca job ownership` asked for a node-pty rebuild in a tree where node-pty had none of the payload its package ships. It gets `writeFakeNodePtyConptyPayload` like its two siblings. I also tried making the fake rebuild emit `build/Release/conpty.node` the way a real one does, and backed it out: `restoreNodePtyWindowsConptyRuntime` keys off that file and then reads `third_party/conpty`, so emitting it in a tree without the package payload turns one honest gap into an ENOENT two steps away. The payload fixture is where "node-pty has its addon" belongs. macOS: ensure-native-runtime-job-ownership, verify-packaged-node-pty-job-ownership, windows-pe-machine, script-module-dependencies, rebuild-native-deps-node-pty, rebuild-native-deps, rebuild-native-deps-windows-process-tree, ensure-native-runtime — 112 passed, 6 skipped. The 6 are the Windows-gated rebuild tests; Windows CI is the arbiter and is why they are on that job now. * fix(windows): register the node-pty addon suites in the scope list too Putting the five suites in the Windows lane's vitest argv gets them run once the job starts; `WINDOWS_PACKAGE_TESTS` in `pr-code-change-scope.mjs` is what decides whether the job starts at all. Only the argv was updated, so a PR touching just `rebuild-native-deps-node-pty.test.mjs` would not have started the Windows job, and its four Windows-only cases — including the same-host-absent one added here — would have run on no machine for that PR. Exactly the shape of gap this branch is about. Both lists now name all five, and `windows-pe-machine`, `windows-pe-image-fixture` and `script-module-dependencies` join `NATIVE_RUNTIME_PREFIXES` so a change to the modules themselves starts it too. `win32-test-lane-registration.test.mjs` exists to catch precisely this and did not, because its matcher only recognises suite-level gates (`describe.runIf` / `describe.skipIf`) and a `.win32.` filename. These tests gate per `it`. Widening it is not this branch's change to make: about thirty files across the repo carry per-`it` Windows gates and are unregistered, so the ratchet would move far beyond node-pty. Flagged rather than done. Message repairs from the same review: - the non-PE arm of the rebuild-time arch error read "... is not a PE image, so nothing can load it, so node-pty would fall back ...". The shared consequence clause already opens with ", so". - the no-source-build packaging error ended "Package this Windows slice on such a host", which is wrong advice for the case where the host IS such a host and the rebuild simply left nothing — reachable when the artifact is removed before prune runs. It now names both readings and points at the beforeBuild output. - the relay-addon builder blamed `--arch` for a build output that is not a PE at all, the same guess the node-pty gate was taught to stop making. - the patch-drift assertion was a bare `toBe(true)`, so a real drift read as "expected false to be true". It now names the two things that can have drifted and what happens until they agree. |
||
|
Neil
|
2531dc9d5a |
fix(runtime): bound the connect phase against an unreachable host, at the transport (#20053)
* fix(runtime): bound the remote-runtime connect against an unreachable host A host that is powered off or firewalled black-holes the TCP SYN, so the remote-runtime WebSocket neither opens nor errors. The Node-side transports set no connect bound, leaving the caller's whole-request timeout as the only one: every `orca <cmd> --environment <unreachable>` sat silent for 60s before failing with a generic `runtime_timeout`. Measured on an unreachable paired host (win-lowspec, SYNs dropped): terminal list / worktree list / repo list / status each took 60.19-60.26s; the same command against a reachable host answered in 0.24s. So this was the shared transport, not one command. Pass `handshakeTimeout` at the three shared remote-runtime WebSocket construction sites, which `ws` applies across TCP connect and the HTTP upgrade. The value matches the bound the browser transport already used. The failure keeps code `remote_runtime_unavailable` so the existing transport-loss classification in terminal-process-inspection still applies, and the message names the endpoint and stops at "unverifiable" — per docs/reference/ssh-execution-boundary.md, loss of contact is never evidence that the host's work stopped. * fix(relay): bound the control socket's connect phase at the transport The relay control socket was constructed with no `handshakeTimeout`, the same gap fixed for the remote-runtime transports. It was not a live defect: the class-level `connectDeadlineMs` (15s) also covers a stalled connect, and that deadline does fire — its `unref()` is safe because the pending TCP connect is itself a ref'd libuv handle that holds the event loop open. Measured in a bare Node process: unref'd timer with an empty loop never fires (exit at 0ms), but the same timer alongside a black-holed connect fired at 2003ms. It was a defect waiting on a refactor. The two bounds cover different phases, and the class deadline covers the connect phase only incidentally. DO NOT REMOVE EITHER BOUND AS REDUNDANT. They are not. Proven by mutation: - Remove the transport bound -> a stalled *connect* falls through to the class deadline, rejecting with `relay_control_connect_timeout` after the full deadline instead of the transport error. - Remove the class deadline -> a stall during the *proving* phase (socket open, host proof never answered) is unbounded; the incumbent test hangs 30s. `handshakeTimeout` cannot see that phase at all. Reuses `remoteRuntimeConnectOptions` rather than forking a second helper, and moves the construction into `relay-control-socket-factory.ts` so a caller that needs a relay control socket gets the bound instead of re-deriving an unbounded one. `handshakeTimeoutMs` is settable apart from `connectDeadlineMs` so a test can stall the connect alone and assert which bound produced the rejection — error identity, not elapsed time. The connect-bound ratchet now covers the relay site and asserts the site still resolves, so an allowlist that silently stopped matching cannot pass vacuously. * fix(lint): carry SAFETY rationales for the connect-bound casts main tightened typescript/consistent-type-assertions to assertionStyle: never, which the rebase brings onto these added lines. Dropping the generic default is not typeable, so each cast keeps its own rationale. * fix(runtime): keep the bounded connect failure inside both message gates The connect bound's new wording dropped out of the two gates that classify remote-transport failures by message text, and those gates are the only ones that run on the path the bound made reachable. `subscribeRemoteRuntimeTransport` reports a connect failure by *rejecting* the subscribe promise, and that rejection crosses `ipcMain.handle`, which keeps only the message. The renderer then classifies it with `RECOVERABLE_MESSAGE_FRAGMENTS`. `Could not reach the remote Orca runtime at …` matched no fragment, so it read as fatal: `recovery.cancel()` and a red banner instead of a retry. Before the bound existed this case reached the 15s subscription-start timer, whose message did match a fragment, so introducing a 12s bound turned an auto-recovering pane into a dead-ended one — the #12650 shape. The same wording also fell outside `REMOTE_RUNTIME_UNREACHABLE_RE`, so the Tailscale remedy was dropped for precisely the unreachable-host failure it exists for. Keep the canonical phrase both gates already recognise rather than teaching each gate a second synonym for one condition, and pin it: the phrase is now a named constant, the corpus in `remote-runtime-transport-error-agreement.test.ts` grows the coded, hinted and code-stripped producers derived from the real helper, and a new subscribe-path test proves the connect bound (not the start timer) is what fires and that its message still classifies as recoverable once the code is gone. Verdict wording is unchanged: `unverifiable`, never a synonym for exited. Also states the bound in seconds, corrects the module comment (`handshakeTimeout` is a socket inactivity timer, so a slow-but-answering host is not cut off), and splits the subscription contract types out to stay under `max-lines`. * fix(relay): drop the duplicate connect bound on the control socket The claim that `connectDeadlineMs` cannot see a black-holed connect is false. `RelayControlClient.connect()` constructs the socket and arms `connectTimer` in the same synchronous call — `new WebSocket()` never blocks — and `expireConnect` fires from `opening` as well as `proving`. The class deadline was already a strict superset of a transport `handshakeTimeout` on that socket. It was also inert. Production passes neither option, so the transport bound was derived from `connectDeadlineMs` and both timers were 15_000, armed in the same tick; the ws timer is an inactivity timer armed on the later `socket` event, so it could not win. Its only reachable effect was changing which string a stalled relay connect rejects with, and it narrowed an existing test's 20ms deadline into a handshake bound it could race. So this removes the factory, the test-only `handshakeTimeoutMs` option and the source-grep test whose premise was wrong, and replaces them with a test that holds the real ground: a connect whose upgrade is never answered expires on the class deadline. Moving the timer arm after `open`, or narrowing `expireConnect` to `proving`, both turn it red — which is what a future reader needs before concluding the phase is uncovered and adding a second bound again. No behaviour change for a reachable relay, and none for the verdict: a stalled connect still rejects and still reaches `unverifiable`, never `exited`. * fix(runtime): stop the endpoint in the failure message from undoing the fix Putting the endpoint into the message created three problems the message itself caused. The Tailscale hint is idempotent by testing whether "tailscale" already appears anywhere in the message. That held while the message was fixed copy. Now a host called `tailscale-box` puts the word there itself, and the hint — the only actionable remedy on an unreachable host — is suppressed for it. Key the guard on the two hints instead of the word. The endpoint comes from a pasted pairing code, which is only length-capped; `normalizePairingUrl` rejects userinfo but nothing re-validates a stored offer. Render scheme, host and port only, so a pasted `wss://user:secret@host` cannot reach a surface the user reads. And drop the elapsed time from the wording. `handshakeTimeout` is a socket inactivity timer, so a `wss://` host that completes TCP and then goes silent re-arms it once and fails at about twice the bound; measured at 2008ms against a 1000ms bound. "within 12s" would have been wrong there, and the endpoint is the actionable part regardless. Also refuse a non-positive or non-finite bound: `ws` and `net` both gate on a truthy timeout, so `0` left the connect completely unbounded while still satisfying the connect-bound ratchet. * fix(runtime): keep the endpoint from smuggling a verdict into the message `isRemoteTerminalGoneMessage` in the pty transport substring-matches `terminal_gone` / `terminal_exited` / `no_connected_pty`, and it runs before the recoverable-connection gate: a match retires the pane's terminal id and cancels recovery. WHATWG URL accepts `_` in a special-scheme host, so once the failure message carried the endpoint, `ws://terminal_gone.example:6768` turned loss of contact into a terminal-gone verdict — the one conclusion `docs/reference/ssh-execution-boundary.md` forbids. Render the host only when it matches a hostname or IP-literal grammar that cannot carry such a token, and fall back to naming no endpoint at all. A well-formed host, including a bracketed IPv6 literal, is still shown. * docs(runtime): say why this connect bound is not the relay's removed duplicate |
||
|
Neil
|
6c3b97b950 |
fix(mobile): a scope refusal is not a missing method on the Relay pairing probes (#19952)
* fix(mobile): a scope refusal is not a missing method on the Relay pairing probes
The desktop's mobile allowlist gate runs before its RPC dispatcher, so a method an
older desktop predates is absent from both and the phone is answered `forbidden`,
never `method_not_found`. Keying the "too old for Relay, stay on LAN" fallback on
`method_not_found` alone therefore never fired against the exact desktop it exists
for: first-time pairing threw instead of committing a LAN host.
`isPairingRelayRpcUnavailable` accepts both codes at the three pairing probe sites.
It is pairing-scoped on purpose - `isMethodNotFoundRefusal` has four other consumers
that must keep reading `forbidden` as a refusal, not as absence.
The main-side test pins the claim the fallback rests on: the dispatcher really does
answer `forbidden` to a mobile-scoped device and `method_not_found` to a runtime one,
and this build allowlists both probes, so `forbidden` on either can only mean an
older desktop.
* fix(mobile): leave a breadcrumb when a desktop refuses relay pairing
The LAN fallback now commits a host instead of throwing, so the refusal code
was the only record of why a phone ended up without a relay endpoint and
nothing wrote it down. Log it on the path that swallows it.
Narrow `isPairingRelayRpcUnavailable` to the two codes it matches rather than
to `RpcFailure`: a plain failure guard would collapse the *false* branch to
`RpcSuccess`, which a refusal carrying any other code still reaches.
Rename the `'method-not-found'` sentinel in the direct-upgrade reader, which
stopped describing what it covers, and correct two comments that named a
`method_not_found` mechanism the desktop cannot produce for these methods:
both probes have been allowlisted and registered by the same commit since
Relay landed, and an unwired pairing provider answers `runtime_error`.
* docs(wire): record that the mobile surface refuses by scope, not by absence
Two comments cited this page for "a scope refusal is not a missing method" and
the page did not say it — the only nearby statement says the opposite, because
it describes the runtime-scoped surface, where the dispatcher does answer
`method_not_found`. The allowlist gate makes the mobile surface the exception,
and the harness does not run that surface, so this note is the only record.
* docs(mobile): name the pairing site the scope refusal actually reached
The comments and the wire-compat note said this fixed first-time QR pairing.
It cannot: the `relay` block on the pairing offer, both RPC handlers and both
allowlist entries all landed in
|
||
|
Jinwoo Hong
|
0699d73fd6 |
fix(relay): skip boot-time DDL when the catalog already has the object (#21147)
* fix(relay): skip boot-time DDL when the catalog already has the object CREATE INDEX IF NOT EXISTS and ALTER TABLE ADD COLUMN IF NOT EXISTS take their relation lock before the server evaluates the existence test, so a boot on an already-migrated database still joins the lock queue. Relation locks are granted in queue order, so every writer queues behind it. The shared runner now asks pg_catalog whether the index or column is already there and skips the statement when a row comes back, and 55P03 is no longer retried by default: with the pre-check ahead of it, a lock timeout means the object is genuinely missing and each retry re-enters the queue. Push keeps the old retry behind an explicit option. * fix(relay): tie the index pre-check to its table and fail on an unreadable target Three defects found in review of the auth reference implementation: - The catalog query matched an index by name inside the table's namespace without checking it belonged to that table. Index names are unique per schema, not per table, so a same-named index on a sibling table answered yes and the real index was skipped forever. Added i.indrelid = t.oid. - Lock-target derivation read a keyword sitting in an identifier position as the object name: CREATE UNIQUE INDEX CONCURRENTLY ON t(c) yielded the name CONCURRENTLY, and ADD COLUMN IF NOT EXISTS with no column yielded IF. A wrong target is worse than none, so keywords are now excluded and an index or column statement whose target cannot be read throws at boot with the statement text instead of falling through to the lock path. - A concurrent-create collision retried the CREATE INDEX, taking SHARE on the table again for an object another director had just finished creating. The catalog is re-asked instead and a present object counts as skipped. * fix(relay): pre-check constraint swaps so a warm boot sends no DDL at all The two ALTER TABLE constraint statements were the last lock-taking statements without a pre-check, so every boot still took ACCESS EXCLUSIVE on relay_region_rehome_attempts twice. A lock target now carries the catalog answer that means there is nothing left to do. ADD CONSTRAINT skips when pg_constraint already names it; DROP CONSTRAINT IF EXISTS is the inverse and skips when it does not, because nothing to drop is nothing to do. The match is by name only: the CHECK body is generated from RELAY_REGIONS, so comparing it would re-run the swap on every region change. Changing a definition under the same name is an operator migration, and the rule comment beside SCHEMA says so. A bare DROP CONSTRAINT gets no target and throws at boot, because skipping it would swallow the undefined_object the server is supposed to raise. The census invariant is now that every lock-taking statement has a pre-check, with no exceptions, and the warm-boot Postgres test asserts zero statements sent rather than two. * fix(relay): refuse a multi-action ALTER TABLE instead of pre-checking its first action `ALTER TABLE t ADD COLUMN IF NOT EXISTS a TEXT, ADD COLUMN IF NOT EXISTS b TEXT` derived the target for `a` alone, so once `a` existed the whole statement was skipped and `b` was never added. The first subcommand parses, so neither the parse throw nor the census caught it. A lock-taking ALTER TABLE with a comma outside parentheses, quotes and comments now throws at boot. One action per statement, or no pre-check is possible. Commas inside a parenthesised type, a CHECK body, a quoted default or a comment are unaffected, and push's 18 statements still parse. * fix(relay): strip every comment before classifying, fold catalog names, count brackets Four findings from the bot reviews on #21147: - A comment between two keywords (ALTER TABLE t ADD /* note */ COLUMN c TEXT) was invisible to both the classification regexes and the must-parse shapes, so the statement got no target AND no throw and ran with no pre-check. Every comment is now stripped quote-aware before classification, nested block comments included. The server is still sent the original text. - hasTopLevelComma counted parentheses but not square brackets, so ADD COLUMN c bigint[] DEFAULT ARRAY[1, 2] read as two subcommands and failed the boot. - bareIdentifier split a qualified name on '.' regardless of quoting, so "a.b" became b", and it kept the written case while Postgres folds an unquoted identifier to lower case before storing it in relname, attname and conname. The name is now tokenised quote-aware and folded, with the qualified table text still passed to to_regclass as written. - sqlWithoutLeadingComments is renamed sqlWithoutComments to match. Relay's 74 statements and push's 18 all still parse, and no relay target name changed: every identifier there was already lower case. * fix(relay): treat a dollar-quoted body as opaque in both scanners A comment marker, comma, parenthesis or bracket inside `$$...$$` or `$tag$...$tag$` is text. The closing delimiter has to match the opening tag exactly, so an inner `$$` inside a `$tag$` body is more text rather than the end, and a tag cannot start with a digit, which keeps a `$1` placeholder from reading as an opener. Relay's pg_stat_statements DO block is the only dollar-quoted statement in the schema, and it now survives the stripper byte-identical. A test asserts that against the real statement. |
||
|
Brennan Benson
|
28a2b628bc |
fix(native-chat): open the message rail panel on the current message (#21143)
* fix(native-chat): open the message rail panel on the current message
The rail's hover panel mounts fresh at scrollTop 0 every time it opens, so
in a long thread it showed the top of the conversation instead of where the
reader actually is. It already knew which row was current — activeId drives
the highlight — it just never scrolled to it.
Attach a ref to the current row that calls scrollIntoView({ block: 'nearest' }).
Radix unmounts popover content on close, so ref attachment is the open edge;
it also re-fires when a different row goes active under an open panel.
* fix(native-chat): keep current rail item focused
* fix(native-chat): resync rail after list changes
* fix(native-chat): own focus across retained rail opens
|
||
|
Brennan Benson
|
fbe7b194b8 |
fix(quality-gate): let the changed-code gate see the focused import plugins (#20912)
import/no-duplicates was reachable only through the repo-wide CI audit, so an author's first signal was a red static analysis job after push. |
||
|
Brennan Benson
|
c2962a765a |
feat(desktop): let the renderer reach agent.launch on its own main process (#21132)
* feat(desktop): let the renderer reach agent.launch on its own main process The desktop renderer aimed at a remote host was admitted to `agent.launch`; the same renderer aimed at its own main process was refused `agent_launch_unsupported`. Main sends `ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES` on the remote path, which carries the capability, while `runtime:call` built its own hardcoded list that did not. Collapse the two hand-maintained copies in `runtime.ts` — the unary and the streaming path held separate literals — into one constant, add the capability to it, and pin its divergence from the remote Electron list so the next capability cannot drift the same way. No caller is migrated: this makes the call possible and changes no behaviour. * docs(test): mark which ledger rationales are grouped rather than audited |
||
 Neil
|
2569a71ce8 | fix(deps): update vulnerable dependencies without new overrides | ||
|
Jinwoo Hong
|
631b51f508 |
perf(usage): run the Claude/Codex/OpenCode usage scans on a worker thread (#21114)
* perf(codex-usage): resume rollout scans at the last parsed byte Codex rollout files are append-only and grow all day, but any append changed both mtime and size, so `canReuse` discarded the cached entry and the scanner re-read the whole file from byte 0 on the Electron main process. On one real corpus that was 6.59 GB re-read per cycle across 26.63 GB / 21,110 files. Each parsed file now persists a resume point: the offset just past the last newline-terminated line, the parse context at that offset (session id, cwd, model, running totals), a sha256 of the 4 KiB before it, and the file's dev:ino. A grown file resumes there and merges the appended rollup into the cached one; anything unproven falls back to a full reparse — truncation, an in-place rewrite, rotation, a counted tail with no trailing newline, a legacy copied-session suffix offset, or a file that must reclaim deferred fork claims. Resume never depends on mtime equality, so a coarse-mtime filesystem cannot hide an append. Fixture: a 75,737-byte rollout with a 758-byte append re-read 76,495 bytes before and 8,950 after (the append plus two bounded 4 KiB boundary windows). Also bounds the automation-attribution force predicate for both Codex and Claude: it keyed on `lastScanError`, so a persistently failing scan forced a fresh full rescan on every single lookup. It now keys on the most recent scan attempt, which is one forced scan per run regardless of outcome. * perf(usage): run the Claude/Codex/OpenCode usage scans on a worker thread The three first-party usage scans walk whole rollout and transcript corpora and read OpenCode's SQLite synchronously, all on the Electron main process. They rarely produce a long stall — the JSONL reader streams, so it yields to the loop between chunks — but they pin the main-process event loop at ~95% utilization for the scan's whole duration, which is what every IPC message, timer and window event then queues behind. Move that work to one lazily-spawned, unref'd worker thread shared by all three providers, following the OpenCode SQLite scanner precedent (#8864). Measured on a synthetic 4,000-rollout corpus (25.8 MB cache): a cold scan drops from 2,147 ms of main-thread time to 31 ms, and a steady-state incremental scan from 165 ms to 64 ms. The worker is stateless and the cache crosses the boundary both ways. That costs ~64 ms of structured clone at this corpus size, against 2,147 ms saved on the cold path, and it keeps the persisted cache the single source of truth — a worker-owned copy would need an invalidation protocol and a second resident copy of the same multi-MB array. Failure is closed, never a silent empty result: a worker that cannot spawn, times out, or crash-loops rejects, and the store records the scan error and keeps the previous projection. Two clients already carried the same FIFO/timeout/crash-cap machinery, so extract it once as WorkerThreadRequestQueue (with the packaged entry-path resolver as worker-thread-entry-path) and move all three onto it, rather than adding a third copy. Their existing tests pass unchanged. The oracle is event-loop utilization on the calling thread, not a stopwatch: usage-scan-worker-event-loop.test.ts runs the same scan both ways and asserts the worker leg leaves the caller idle while the main-thread leg does not, so CI load moves both legs together (#18788). * test(usage): compare the two scan arms instead of two fixed thresholds The event-loop oracle claimed to be self-calibrating — its header said "the ratio is self-calibrating, so CI load moves both legs together (#18788) instead of tipping a fixed millisecond threshold." It computed no ratio. Two separate `it()` blocks each asserted an absolute threshold against its own arm, run separately, so load moved them independently. The comment described a test nobody wrote, and the flake it promised was impossible is the one that landed: `activeRatio > 0.8` on the calling-thread arm measured 0.764 on an ubuntu runner. Fixing the comment is not enough, because the fraction is the wrong quantity. CPU contention drags the calling-thread arm's active/wall fraction *down* toward the worker's, since the loop parks waiting on a contended libuv pool. A 4-vCPU Linux container measured that arm at 0.175-0.756 across twenty runs, idle and loaded — never once above 0.8. Active *milliseconds* move the other way: contention stretches the caller's JS time far more than it stretches the worker arm's fixed post-and-deserialize cost, so the gap widens under load. Merge the two arms into one case over one corpus and assert the worker arm costs the caller under a fifth of the inline arm's active milliseconds. Same twenty Linux runs: 10.9x-83.6x, passing throughout. Keep the presence preconditions on both arms — an arm that silently scanned nothing satisfies the comparison trivially — and extend them to the calling-thread arm, which previously checked only file and session counts. * fix(ports): name the dropped command when the probe queue is full The shared-queue extraction turned `Port scan command queue is full; dropped ${command}.` into a constant string, because `describeFull` was given no way to see the request. Pile-up is per-probe, so the name is the only thing in that log that identifies which of lsof/ps/netstat was shed. Pass the rejected request to `describeFull` and restore the name. The request is built before the cap check so it exists to be named; the id it burns is a correlation token, so a gap costs nothing. The existing overflow test asserted only the error class, which is why the regression escaped a 29-test suite. It now dispatches the overflow under a different command than the accepted ones and asserts the message text, so a message that names the wrong request fails too. Also add a direct WorkerThreadRequestQueue test. Three subsystems share the queue and each client test only sees the parts its own protocol exercises, with `queueCap` reachable from port-scan alone. Covers one-at-a-time FIFO dispatch, the deadline starting at dispatch rather than enqueue, the consecutive-death cap, and both points where that count clears. And record the child-process hazard at the usage worker entry. `terminate()` reaps nothing the thread spawned, and OpenCode discovery reaches a fork today: `wslGated*` forks the WSL transcript sidecar for a `\\wsl$\...` path, which a Windows `OPENCODE_DB` or `XDG_DATA_HOME` can be. One scan through that entry with a UNC `OPENCODE_DB` forked a sidecar that outlived `terminate()`. * test(ai-vault): assert the OpenCode worker messages exactly, not by fragment Checked every message string in the two clients the shared-queue extraction rewrote against origin/main. Only the port-scan queue-full one regressed (fixed in the previous commit); the OpenCode SQLite client's four messages render identically, the remaining source diffs being renames — `error.message` to `lastError`, `call.timeoutMs` and `CALL_DEADLINE_MS` to `timeoutMs`. `session-scanner-worker-client.ts` was not touched by the extraction. But its suite could not have caught it either. `/timed out/`, `/exited with code/` and a bare `rejects.toThrow()` all still match a message that has lost its interpolated value, which is the same blind spot that let the port-scan regression through. Assert the rendered text instead: the timeout names its deadline, the exit names its code, and the crash-loop drain still carries the text of the fault that killed the run. * fix(usage): correct the worker entry's child-process note The previous note said `worker.terminate()` leaves a forked sidecar orphaned. It does not, and the reproduction that appeared to show it used a stub sidecar missing the `process.on('disconnect', () => process.exit(0))` the real entry has. With a faithful one: the sidecar lives exactly as long as the thread and is gone within 2s of `terminate()`, because tearing the thread down closes the IPC channel it owned. Two worker lifecycles forked two sidecars and leaked neither, and the pre-worker main-thread path reaps its sidecar the same way, on host exit. What is true and worth recording: a fork is reachable from this bundle at all, which is easy to miss; it survives only as long as the channel does; and the sidecar is now re-forked per worker lifecycle instead of pooled for the app's life. State those, and warn that a future child which does not exit on channel close would not get the same free cleanup. * fix(usage): kill a wedged scan worker on no progress, not on wall clock `USAGE_SCAN_TIMEOUT_MS` was a 10-minute deadline on the whole scan. A cold scan of a real history is legitimately minutes — 637 s measured on a 30 GB corpus with 300 worktrees before the per-cwd memo, ~51 s after — so a larger corpus or a slower disk crosses it. Crossing it killed the worker, recorded a scan error and left the cache unadvanced, so the next refresh started cold and died at the same point, forever. The deadline is now a no-progress window. The worker posts a file counter as it walks the corpus (`UsageScanWorkerProgress`, rate-limited to one message a second), and `WorkerThreadRequestQueue` re-arms the active call's timer on each one via the new optional `isProgress`. Clients that do not pass it keep the plain wall-clock deadline. `MAX_CONSECUTIVE_DEATHS` and idle teardown are unchanged. * refactor(usage): report scan progress as a file count, not one call per file Claude's scanner walks batches, so a per-file callback made it loop just to bump a counter. |
||
|
Jinwoo Hong
|
f36a7cecf2 |
perf(codex-usage): resume rollout scans at the last parsed byte (#21102)
* perf(codex-usage): resume rollout scans at the last parsed byte
Codex rollout files are append-only and grow all day, but any append
changed both mtime and size, so `canReuse` discarded the cached entry and
the scanner re-read the whole file from byte 0 on the Electron main
process. On one real corpus that was 6.59 GB re-read per cycle across
26.63 GB / 21,110 files.
Each parsed file now persists a resume point: the offset just past the
last newline-terminated line, the parse context at that offset (session
id, cwd, model, running totals), a sha256 of the 4 KiB before it, and the
file's dev:ino. A grown file resumes there and merges the appended
rollup into the cached one; anything unproven falls back to a full
reparse — truncation, an in-place rewrite, rotation, a counted tail with
no trailing newline, a legacy copied-session suffix offset, or a file
that must reclaim deferred fork claims. Resume never depends on mtime
equality, so a coarse-mtime filesystem cannot hide an append.
Fixture: a 75,737-byte rollout with a 758-byte append re-read 76,495
bytes before and 8,950 after (the append plus two bounded 4 KiB boundary
windows).
Also bounds the automation-attribution force predicate for both Codex and
Claude: it keyed on `lastScanError`, so a persistently failing scan forced
a fresh full rescan on every single lookup. It now keys on the most recent
scan attempt, which is one forced scan per run regardless of outcome.
* fix(codex-usage): verify the head of a resumed rollout prefix
The resume guard proved only the 4 KiB before the resume offset, and leaned
on dev:ino to catch a rollout that was replaced at the same path. ext4 and
overlayfs hand a recreated file the inode the old one freed, so on Linux that
check passes and a same-length prefix swap resumes over changed history.
Measured 20/20 inode reuse on ext4 and overlayfs, 0/20 on APFS and tmpfs --
which is why the case only failed in CI.
An in-place prefix rewrite kept no inode change on any platform, so that
variant was missed on macOS too.
Digest a bounded window at the start of the parsed prefix as well. When the
two windows meet, one read covers the whole prefix and leaves no gap. The
head window is carried across a resume rather than re-read, so a resumed scan
reads the appended bytes plus three 4 KiB windows.
* test(codex-usage): cover the resume window layout switch
* test(codex-usage): cover the boundary window in isolation
* test(codex-usage): isolate the boundary window with disjoint windows
* fix(codex-usage): restart a rollout parse when its verified prefix is gone
The scanner verifies a rollout's prefix in its first pass and reads it in
the second, so a truncation in between left the merged projection holding
the whole pre-truncation history while `processedFile` was re-stat'd to the
new, smaller size. Size and mtime then matched disk with no resume state
left to reject, so the reuse path served the stale total on every later
scan. The resume-state builder returns null only on a short read, which is
exactly that signal; on it, drop the merge and reparse the file from zero.
Also covers three guards that no test was holding: the unterminated-tail
resume suppression (a tail that is valid JSON minus its newline is counted,
so resuming over it double-counts), the short-read check in
`readWindowDigest` (without it a resume point past EOF verifies against
itself), and the legacy-suffix exclusion in the scanner's resume guard
(bridge markers can appear on a file that already has a resume state).
* fix(codex-usage): re-verify a rollout resume point at the point of use
The scanner verified each resume point while walking the sessions
directory, then parsed the files afterwards, so every file discovered or
parsed in between widened the gap between the check and the read. A
rollout replaced in that gap resumed at the old offset into unrelated
bytes: the cached session id, cwd, model and running totals were stitched
onto another file's records, and because the projection was then re-stat'd
to the new size, the reuse path froze the corrupted numbers. A shrink was
the visible half of this; a replacement larger than the recorded offset
never short-reads and corrupts instead of going stale.
Re-run the full check — inode, head window and boundary window — inside
the parse, against the file about to be read. The short-read fallback
added alongside it still covers the narrower case of a truncation landing
after that check, during the read itself.
Cost, measured on the existing byte oracle: a resumed file now reads
`appended + 5 * 4096` rather than `appended + 3 * 4096`, paid only by
files that changed since the last scan; untouched rollouts still read
nothing. Two byte-total assertions that a 15 KB rollout can no longer
satisfy now assert their intent directly — that the parse read did not
reopen at byte 0 — via a stream oracle that records each read's offset.
* test(codex-usage): pin mid-scan replacement on attribution, not totals
The mid-scan replacement case was written with a heavier replacement so
the token totals diverged, which overstated how visible the defect is.
Rebuilt on the variant where the stale prefix contributes exactly as many
events as the resumed read skips: daily aggregates and token totals then
match a cold scan byte for byte, and the misattribution — 60 records of
one session recorded against another — is the only remaining signal.
Oracle is now the session shape. Removing the point-of-use re-verification
fails it with `session-grower` in place of `session-other`; every
totals-based assertion still passes under that mutation.
* perf(codex-usage): stop resuming a rollout prefix too short to pay for it
Point-of-use re-verification made a resumed scan cost five bounded windows,
which is more than re-reading a small rollout outright. Measured against a
cold reparse of the same file, resuming lost below a 12,288 B prefix and
lost badly under 8 KiB, where the coalesced-window layout rehashed the
whole prefix on each of the three verification passes.
Set the floor at that break-even — 3 * 4096, the point where two
verification passes plus the recorded boundary stop being cheaper than
reading the prefix once — and refuse to record or accept a resume point
below it. Measured: a 12,568 B prefix now reads 21,234 B resumed against
21,514 B cold, and a 76,484 B rollout reads 21,238 B against 84,676 B. No
size band reads more than a cold scan any more; under the floor the
windows are skipped entirely and a scan reads exactly the file.
With every offset past the floor the two windows can no longer overlap, so
the coalesced-layout branch and the empty-window branch are gone. The
floor is also input validation: a persisted offset below it would put the
boundary window at a negative start and throw ERR_OUT_OF_RANGE.
Tests that meant to exercise the resume path were silently reparsing whole
once the floor landed — the suite stayed green while three guards lost
their only coverage. They now size their rollouts off RESUMABLE_RECORDS
and assert the offsets their parse reads actually opened at, so a test
that stops resuming fails instead of passing quietly.
* test(codex-usage): cover the reuse gate's own legacy-bridge check
`scanner.ts` carries the same `legacySourceSkipBytes === 0` term twice and
they are different guards: line 83 gates resuming, line 71 gates reuse.
Only the first had a test, so dropping the second left the suite green.
It is load-bearing. A cached entry can predate the bridge marker while the
source file is untouched, so size and mtime still match and nothing else
stops the scan serving a full-history projection for a file that is now
parsed suffix-only. With a total-only record after the copy point the two
readings diverge — baseline worth nothing against a delta worth three —
and the reused entry reports 18 tokens where a cold scan reports 15.
* fix(codex-usage): annotate the mid-scan seam instead of asserting it
The changed-code quality gate rejects any non-const type assertion, and
`onStreamOpen: { current: null as (...) | null }` is one, so `static
analysis` failed on this PR. A typed local carries the same intent.
* fix(usage): force an automation lookup onto a scan already in flight
`shouldForceAutomationUsageScan` keyed on `max(lastScanStartedAt,
lastScanCompletedAt)`, so a scan that started after the run completed but
is still running counted as a finished attempt. The lookup then called
`refresh(false)`, which returns early inside the 5-minute staleness
window instead of joining the scan, and the run's usage read
`unavailable`. Forcing instead just awaits the shared `scanPromise`.
While a scan is in flight its start time is no longer treated as an
attempt, so the once-per-run bound still holds: a failed scan leaves
`lastScanStartedAt` past the run and stops re-forcing.
The two providers' copies of the predicate were byte-identical, so it now
lives in `src/main/usage/automation-usage-scan-forcing.ts`.
|
||
|
Jinwoo Hong
|
2c2d068b26 |
perf(usage): resolve each cwd's worktree once per scan (#21130)
* perf(usage): resolve each cwd's worktree once per scan Codex and OpenCode attribution ran the worktree containment search for every parsed event, so a cold scan cost events x worktrees. On 745 MB of real rollouts (~20k events) that is 1.2s with 0 worktrees, 5.0s with 100, 12.8s with 300 and 39.8s with 1000; a full corpus with hundreds of remembered worktrees is where the STA-7724 reparse burned minutes of main-thread CPU. A scan holds only a few hundred distinct cwds, so both scanners now build one memoized resolver per scan and thread it through parsing instead of passing the worktree list to every event. * refactor(usage): make the worktree resolver own canonicalization `createUsageWorktreeResolver` now takes raw worktree refs and canonicalizes them itself, so each scanner has one entry point and neither keeps a private `buildWorktreesWithCanonicalPaths` or `canonicalizePath`. The resolver unit test counts comparisons through the same `areWorktreePathsEqual` mock the scanner-level test uses instead of a property getter. |
||
|
Jinwoo Hong
|
77cd61df39 |
fix(relay): keep pool pressure a per-cell rehome exclusion, not a fleet stop (#21126)
The fleet safety gate returned database_pool_pressure whenever the Math.max of database_pool_waiters_max or database_pool_wait_ms_max across every general cell crossed 16 waiters or 250ms. Measured 2026-09-16, the asia-east2 cells breach continuously at 94-156 waiters and ~2000ms while their server-side execution is 0.2ms, which is a client pool too narrow for a 176ms round trip rather than database distress, and the us-central1 cells breach in bursts on about a third of polls. Worse, the bar flaps: the pre-check passes, the commit re-check reads fresh rows seconds later and trips, and that path durably disables the control instead of merely deferring. Drop the pool check from the fleet gate. Pool pressure stays a per-cell exclusion in regionalRehomeCellSafetyIsClean, which already drops a breaching cell as both source and target on selection and again on the commit path. The fleet bars that remain (stale monitoring, sql failure storms, control-recovery failures, reconnect storms) all signal database-wide distress. Nothing cells publish, no stored row and no exported constant changes. |
||
|
Jinwoo Hong
|
f2e4d2fdb0 |
test(mobile): repin the RPC recording corpus to main after #21089 (#21123)
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Jinwoo Hong
|
4b876758d3 |
refactor(mobile): checked reply readers for the session domain (step 7) (#21089)
* test(mobile): record main's session reply behaviour at every unrecorded read site Step 7 for the session domain changes how 51 RPC readers read a *malformed* reply. Eleven of the session read sites had no recording family, so main's answer to a malformed reply at those sites was undocumented and the reader change would have had nothing to move. This commit is the before picture, taken from main's own tree with no product edit in it. Ten new families, twelve scenarios, twenty-five goldens: - `session.review-file-diff` / `session.review-branch-diff` — `git.diff` and `git.branchDiff` read through the review projection, which the Changes screen's verbatim readers do not cover. - `session.review-git-mutations` — the single-file `git.stage` / `git.discard` and the bulk stage sweep's second `git.stage`. - `session.review-send-sheet` — `session.tabs.list` read for the agent terminals the send sheet lists, the third reader on that method. Needs an `open-send-sheet` action on the review-action adapter, which re-digests that family's eight goldens on `adapterSha256` and nothing else. - `session.browser-tab-create` — `browser.tabCreate`. - `agentSession.structured-create` — `agentSession.create`, whose family base only ever covered the support probe. - `session.tab-rename` / `session.tab-close-session` — `terminal.rename` and `session.tabs.close`. - `settings.new-tab-local-agents` — `preflight.detectAgents`, the arm the new-tab loader takes for a workspace with no connection. `baseline` is repinned to main's tip because two commits (#20659, #21004) touched a fenced path after the pilot's pin, so `--record` refuses on main's own tree until it moves. The repin is what rewrites `baseline` on all 705 existing goldens; nothing else about them moves. Decoded against origin/main through the value pool: 705 header-only (`baseline` on every one, `adapterSha256` on the eight review-action goldens), 0 body-moved, 25 added, 0 deleted. Not covered, with the reason: the chunked clipboard upload's `appendImageUploadChunk`, `commitImageUpload` and `abortImageUpload` cannot be matrixed, because `replyMatrixSites` takes every completion in the base scenario and the chain's later params carry the `uploadId` the start reply named. Driving `clipboard.startImageUpload#1` therefore makes main send an append whose params no scripted step matches, and the recorder raises `Request params mismatch: clipboard.appendImageUploadChunk#1` instead of recording. The two families were written, probed and removed. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): checked reply readers for the session domain (step 7) Fifty-one unchecked reply readers across nine files become checked zod readers, so a malformed host reply surfaces as one readable error at the operation boundary instead of a downstream `TypeError`, a rendered `undefined`, or a screen left ready over garbage. Deliberately a behaviour change on malformed replies only. Eight schema modules, one per reply family, each recording the consumer line behind every requirement and the host handler that publishes it: - `clipboard-image-reply-schema.ts` — the upload slot's `uploadId`, the commit and single-frame path strings, and the two legs whose body nothing reads. - `github-pr-mutation-reply-schema.ts` — the `{ ok, error }` status envelope as two variants, and the bare-boolean confirmation. - `github-pr-entity-reply-schema.ts` / `github-pr-read-reply-schema.ts` — the seven PR sidebar reads. Every identity requirement the hand parsers had is kept, so a payload that degraded to null still degrades to null; what changes is a payload that is not the declared container at all. - `diff-review-reply-schema.ts` — the normalized branch compare, the review notes on the worktree record, the three file-diff arms, and the file-level git mutations. - `review-terminal-reply-schema.ts`, `session-launch-reply-schema.ts`, `session-read-reply-schema.ts`, `session-write-reply-schema.ts` — the review send sheet, the launch paths, the session screen's reads and its writes. Requirements are exactly the members a consumer reads unguarded, everything else is a salvaged optional with main's own default applied in the transform, and no schema is `.strict()`: a member a newer host adds passes through untouched. Enum arm sets that a reader compares against pass through or degrade to the arm the reader handles most conservatively; the two closed sets — the committed change status and the diff kind — are closed because main *dropped* an arm it did not know rather than passing it through, and degrading them would draw a row or render a diff main never did. No member is coerced on the way back to the host. `github-pr-parsers.ts`, `github-pr-comment-parsers.ts` and `github-pr-value-readers.ts` are gone; their suite is now the parity record for the schemas that replaced them, with the four cases that refuse rather than degrade marked as such. Twelve call-site casts are deleted, and three dead "response was invalid" branches with them: the reader refuses those replies now, so the error names its method. The nine session files come off `unchecked-rpc-reader-inventory.ts` entirely rather than being lowered. `git show --stat` on this commit touches nothing under `mobile/rpc-foundation`. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): unit-pin every session reply schema's decision Three kinds of case, one per kind of decision the schemas encode: a member a consumer reads unguarded is required and its absence refuses, an arm set a reader compares against degrades to the arm that reader handles most conservatively, and a reply whose arms need different members is declared as variants and each arm is read. The last suite is the wire-compatibility claim: a member no reader knows passes straight through, on the markdown document, the upload slot and the terminal inventory alike, so a newer host is never refused for a field mobile does not read. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): refresh the corpus for the session domain's checked readers Repins `baseline` to the last commit touching a fenced path and re-records all 730 goldens, which is the disclosed behaviour change taken as an observation. Decoded through the value pool against the pre-refactor tree on this branch: 688 header-only with `baseline` the only key that moved, 42 body-moved, 0 added, 0 deleted. The 42 are seven named scenarios and thirty-five matrix goldens, and every moved checkpoint's own reply is malformed or refused. Three `normal` partitions appear in the list and none of them reads a well-formed reply differently: the review file-diff family's base scenario drives three legs and its third is scripted `{ kind: 'unknown' }`, so that leg's checkpoint moves in every variant, the varied leg included. The same append-only-history effect puts `pr-read-upstream-error`'s `no-pr` checkpoint in the list for the malformed PR recorded before it. What the corpus now records, in one sentence: a property read on null, a V8 destructuring message shown to the user, and four hand-written "response was invalid" strings are replaced by one message that names the method, and four screens that published a malformed payload as ready state now show an error instead. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): split the expanded check run out of the PR read schemas `github-pr-read-reply-schema.ts` was 328 code lines against the 300-line cap. The expanded check run and the annotations, jobs and steps listed under it are one reply with no reader in common with the other six, so they move to `github-pr-check-reply-schema.ts` whole. A move, not an edit: no schema changes and no golden moves. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the corpus to the branch's last fenced-path commit The schema-module split touched `mobile/src`, so `--record` refuses on the pin the previous refresh left behind. Repins to that commit and re-records. Decoded against the previous corpus: 730 header-only with `baseline` the only key that moved, 0 body-moved, 0 added, 0 deleted — the split is a move, and the corpus says so. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): drop the worktree display-name cast's type import The live-title read is typed by its schema now, so the cast it annotated is gone and the import it needed with it. oxlint flags the leftover. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the corpus to the branch tip The unused-import removal touched a fenced path, so the pin moves with it. Decoded against the previous corpus: 730 header-only on `baseline` alone, 0 body-moved, 0 added, 0 deleted. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): contain a refused prChecks reply to the checks section The checks read was the one phase-1 dependency that could take the whole PR sidebar down. `loadPrSidebarData` routed `!checksOutcome.ok` through `failureState`, so a host whose `github.prChecks` shape drifted cost the user the title, body, comments, reviewers and merge controls — everything they opened the sidebar for — over a section that renders a row of icons. Main never noticed because its unchecked reader answered `[]` for the same reply; this branch's reader refuses it, which is correct, and which is what makes the containment necessary. Contained the way phase 2 already is: a failed read keeps `kind: 'ready'`, empties `checks`, and carries the message in a new `checksError` so the checks section can say what happened. The sidebar can no longer reach `error` or `blocked` on the checks read alone. Also pins the enum departure this PR makes deliberately. The degrading arm sets go through `salvagedOptional(name, z.enum(...))` rather than `openEnum` because `openEnum` refuses a non-string where main mapped it to the conservative arm; nothing held that, and all 2477 tests stayed green against the swap. Six cases now hold both halves: a non-string degrades on the three open sets, and an unknown arm drops the row on the closed ones. Four deletions the reviewer found: a reaction-token alias with no importers, the `errorType`/`fetchedAt` the branch-lookup reader fabricated to satisfy a type whose only consumer reads neither, two bare schema aliases, and a quick-commands pass-through with two callers. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the corpus to the containment commit `--record` refuses unless the product tree equals `baseline`, so the fix above moves the pin. The corpus re-recorded in place against it: 730 goldens, every one header-only on `baseline`, no observation moved. No observation moved because no family reaches the code the fix changed. The `github.pr-read` family calls the seven wrapper reads directly and records their `{ ok, error }` outcomes; `loadPrSidebarData` sits a layer above that and no scenario mounts it. The prChecks outcome is identical before and after — what changed is what the sidebar does with it — so the unit suite is the only oracle for the containment. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): record the PR sidebar's checks containment The containment landed with no golden: no scenario mounted `loadPrSidebarData`, so the row in the delta table rested on unit tests alone. `PrSidebarLoadDeps` is five client-taking functions, so a new adapter drives phase 1 directly and records the `PrSidebarState` it resolves to — no React host, and no edit to an existing adapter, so no recorded golden moves. Two scenarios: a normal load, and one whose checks leg answers a shape the reader refuses. The matrix over the base then drives all eleven partitions at `github.prChecks#1`, and every one of them records `ready` with a `checksError` where main took the whole sidebar to `error`. `pr-sidebar-checks-failure-state` is the mutant that routes the refusal back through `failureState`; it moves both `pr-sidebar-checks-refused` and the prChecks matrix golden. Also pins two closed-and-required enum decisions that were free to become defaults — an unknown check-summary state drops the summary block, an unknown reaction content drops the reaction — deletes four exported type aliases and five enum constants with no reader outside their own file, makes `PRChecksSection`'s `checksError` required so a second caller cannot silently lose the message, and stops the header reading "No checks" when the checks were unreadable rather than absent. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the corpus to the pr-sidebar family commit Six new goldens — two pilots and the four matrix sites the base scenario scripts — and `baseline` on the 730 that already existed. No body moved and no `adapterSha256`: the family is a new adapter module, so nothing recorded through another one re-digests. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): re-record the corpus against the merged main Repins `baseline` to the merge commit and re-records all 736 goldens in place. Against `origin/main` the 705 shared goldens move only on `baseline` (672 of them header-only), leaving the same 33 body moves and the same partitions the branch carried before the merge, plus its 31 added goldens. Every body also takes main's recorder shape from #21088: `sent` becomes `ordinal` over one interleaved write counter, subscriptions record a cleanup checkpoint, and a salvaging read now reports a `reply-salvage` effect naming what it dropped. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * fix(mobile): keep an explicit null on the two tri-state PR flags `autoMergeAllowed` and `mergeQueueRequired` carry three answers, not two: `null` is GitHub saying auto-merge is not allowed, `undefined` is the host not carrying the member at all. The readers coalesced the null away, so a well-formed reply read differently from the parsers they replaced, which preserved it explicitly. Both shared types already declare `boolean | null`. No consumer separates the two today — `pull-request-auto-merge-availability` compares with `=== true` and `!== false` — so this is parity, not a visible fix, which is exactly why it needed a test. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin the corpus to the tri-state flag commit All 736 goldens move on `baseline` alone: no scenario scripts an explicit null on either flag, so preserving it changes no recorded screen. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): check the two session-write readers #21083 brought Step 7 empties the session block of the unchecked-reader inventory, and #21083 landed two readers into it after that: the New Tab create's member read of `tab`, and the display-mode toggle's payload. Converting them is what keeps the claim true — a session line reappearing would mean the domain is not migrated. `created-terminal-tab` requires `tab.id` and `tab.type === 'terminal'`, because the strip keys the new tab on the id and spreads the rest into a union whose arm `type` picks. `terminal`, `title` and `terminalTheme` stay optional behind main's own guards, and unknown members pass through. `terminal-display-mode-set` reads nothing, so it takes the same `z.unknown()` the other five unread writes take. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin and re-record over #21083's corpus All 736 goldens this branch already had move on `baseline` alone, and #21083's 22 arrive beside them. One of the 22 moves against main's own recording: `matrix-session.create-terminal-session.tabs.createterminal-1`, where the New Tab create's five malformed partitions read `Cannot read properties of undefined (reading 'tab')` and now read the method's own message. Two of them also stop unsubscribing the terminal the user was watching before the property read threw, so a create that never happened no longer costs the live pane its subscription. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * docs(mobile): say what carries a refused create reply to the catch Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Brennan Benson
|
7f5141ae2d |
Make the Agent Permissions toggle apply to Codex chat (#20977)
* fix(structured-chat): deliver the permission posture through each transport's own contract Codex posture moves off app-server argv onto typed `thread/start` and `thread/resume` params. Manual states `on-request` / `workspace-write` explicitly instead of omitting the fields, which app-server resolved through the mirrored config.toml — a Manual thread on a home carrying `approval_policy = "never"` never prompted. Claude keeps its owned `--dangerously-skip-permissions` flag through SDK `extraArgs`; the SDK's typed bypass option emits a newer allow flag that older user-installed binaries reject. Posture is re-derived from current settings on every session acquisition. * fix(structured-chat): parse permission arguments as argv * fix(structured-chat): keep permission policy authoritative |
||
|
Brennan Benson
|
0bf815a480 |
fix(agent-launch): make a lost launch safe to retry (#21106)
* feat(agent-launch): make a lost launch safe to retry `agent.launch` could not be retried safely. Only a create-worktree target carrying a clientMutationId got any idempotency at all, and that was a 60s in-memory cache with no caller partition that dies with the process; an existing-workspace launch got none. Mobile retries a lost create by design, so the retry is the ordinary case — and a retry past that cache meant a second worktree and a second agent. A caller may now name its launch with an optional `operationId` and get one execution, the recorded answer on every replay, and a truthful refusal when the outcome is unknown. Admission runs before the worktree selector is resolved, so a replay answers from the record rather than re-deciding against today's world. The core is an atomic claim. Admission alone cannot decide who runs: two replays both read `pending`, and settling `unknown` replaces the outcome blind, so two serialized writes are not a compare-and-swap and both callers execute. A conditional current-state swap now reports which caller won, and settlement is monotone so a late `unknown` cannot erase a recorded success. Also here: a host-computed fingerprint over the launch intent that excludes mutable settings, the full launch result persisted so a replay returns the receipt and warning that cannot be recomputed once settings move, and a derived child operation id for the inner attach — the ledger key carries no method, so forwarding the launch id would make the attach conflict with its own launch. Safety, not recovery. Nothing here probes for a surface a dead attempt left behind, adopts one, or finishes an interrupted publication. Callers that send no `operationId` keep today's behaviour exactly, which is why the field is optional and the host advertises `agent.launch.replay.v1`: an older host strips an unknown param and launches anyway, so a client may only treat a retry as safe once the host has said it enforces the ledger. * fix(agent-launch): keep an unreadable launch payload from costing the store Review follow-ups on the replay-safety ledger. A recorded `launch` payload must not gate row validity. `isAgentLaunchResult` is a hand-maintained mirror of a result type later work will edit, and `isAgentSessionOperationRow` is consulted by the store loader, where one rejected row makes the whole file unparseable — a primary and backup that both fail to parse raise `agent_session_store_corrupt` and the profile loses every lease. That is the same argument the row already makes for keeping `sessionId` required, applied to the field this PR added. The payload is now typed `unknown`, left out of the row guard, and narrowed where it is read, so a payload this build cannot read refuses exactly one replay. A recorded failure now replays as the code the launch raised. Narrowing it through the closed `agentSession.*` refusal list answered `worktree_not_found` with `agent_session_operation_invalid` — the ledger's "your id is malformed" signal, which invites a client to mint a fresh id when the truthful answer is that this launch definitively did not run and the same id is safe to retry. The persisted failure code is bounded on the way in. A code is an identifier, but `error.message` is free text: an errno sentence carrying an absolute path arrived here as one and was written into a file re-serialized whole on every later operation. Bounded on write only — a length check in the row validator would reject rows this same build wrote, which is the hazard above. Comments: the caller key does not give one client a single namespace across surfaces, because the structured attach this launch performs partitions under `structuredCallerFor`; the two coincide only for a bearer-identity caller with no paired device, which is exactly when the derived child id is load-bearing. Recorded as a known limit that a `lost` claim cannot tell a sibling executing now from one a restart abandoned; telling them apart needs execution-generation tagging, which is recovery. Tests: the store-level ablation was inert — it defined a local stand-in and passed identically with and without the guard. It now substitutes the non-atomic composition into the handler's own store and watches one tap create two workspaces. Each of the four new guards was watched failing against the unfixed code: `agent_session_store_corrupt` on reopen, `expected false to be true` on the row guard, `agent_session_operation_invalid` in place of `worktree_not_found`, and a 6042-character code where 128 is the bound. * fix(agent-launch): keep live retries in one execution * docs(agent-launch): clarify failed replay guidance |
||
|
Jinwoo Hong
|
de4dab93cb |
test(shared): drop the duplicated separator-only git grep test (#21116)
|
||
|
Brennan Benson
|
aad41b1a40 |
fix(native-chat): render approvals from the harness presentation, not serialized tool input (#21087)
* fix(native-chat): render approvals from the harness presentation, not serialized tool input The approval card built its title from the tool name and rendered JSON.stringify(input) into an element with no height bound. Any large payload - a file write's contents, a proposed plan - pushed the action buttons past the viewport with no way to scroll to them, leaving the prompt unanswerable without zooming the pane out. Thread the agent SDK's own presentation fields through the prompt registry into the journal item: title, displayName, description, decisionReason, blockedPath and matchedAskRule. The SDK documents its title as the prompt text to use instead of reconstructing one, and warns that the decision reason may carry terminal escapes, so those are stripped before rendering. The card now also shows why a request was raised rather than only what it was. Bound the detail in a scrollable region that is reachable by keyboard, and cap it main-side with the existing shared tool-detail limit rather than the far looser journal payload bound. Focus moves to the card when a prompt appears and Escape resolves it, which previously did nothing because the composer owning that handler is unmounted while a prompt is pending. Mobile rendered the same unbounded detail and is fixed alongside. * fix(native-chat): keep approval actions reachable |
||
|
Neil
|
55ae3b393c | fix: make git grep directory filters recursive | ||
|
Jinwoo Hong
|
ccb4d2044b |
refactor(mobile): send the last session-route raw-port calls as operations (step 6, migration 2) (#21083)
* test(mobile): record the session startup, create and display-mode families
Three mount adapters and ten scenarios for the last raw-port sends in the
session route, recorded at the pinned main baseline before any product edit.
The three hooks were listed as blocked on a WebView-ref substitute. They are
not: none imports the terminal WebView, and all three send with no ref. The
display-mode toggle reads a `{cols, rows}` cell and a device-token cell; the
create path calls scope callbacks; the startup effect drives scope callbacks
only. Each stub is an effect sink, shapes no param and swallows no throw.
One scenario reaches both `worktree.activate` sites the way the product does:
the auto-create clears `created` off the route, the effect re-runs on the same
mount and takes the other branch, so the reply matrix drives both.
The create adapter mounts in its factory rather than as a scripted step. React
draws one `Math.random()` lazily the first time `enqueueTask` runs, and the
runner flushes through `await act` after every step, so a scripted mount would
make `clientMutationId` the second draw of the seeded sequence on the first
recording in a process and the first on every later one. The two determinism
runs caught it.
Recorded through the pinned-baseline worktree recipe, because main has moved
past `a28085adbf` in `src/shared` and this branch does not repin. 705 existing
goldens byte-identical, 15 added, 0 moved, 0 deleted.
Mutation census against the raw-port code, applied and reverted by hand, all
twelve killed: wrong method at each of the four sites; acceptance verdict
swapped at each of the four verdict-reading sites; dropped `unsubscribeTerminal`
on replace; the two activation branches swapped; a delayed `fetchTerminals` pass
dropped; the viewport pair not forwarded on `terminal.setDisplayMode`.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* refactor(mobile): send the last session-route raw-port calls as operations
Four references, three files, no behaviour change. Proven by replay: the
fifteen goldens recorded at the pin before this commit pass unchanged, so no
re-record.
- `use-mobile-session-startup.ts` both `worktree.activate` sends reuse
host-screen's `worktreeActivate`. Its skip verdict was never read before;
the startup effect is its first reader, and it reads exactly what main read
off the envelope — whether an accepted reply says the host is headless.
- `use-mobile-session-terminal-create-actions.ts` `session.tabs.createTerminal`
gets `sessionTabCreateTerminal`, a single-reader operation beside the other
session-screen writes. `require-result-or-throw-message` replaces the
`if (response.ok)` branch because the throw lands in the catch that already
reported the host's message, character for character, including the empty
message falling back to the screen's own copy. The reader stays the unguarded
`.tab` read, because that policy rethrows a reader's exception rather than
converting it, which is what keeps a null or absent result failing where it
failed before.
- `use-mobile-session-terminal-stream-display.ts` `terminal.setDisplayMode` gets
`terminalDisplayModeSet`, a skip whose verdict the caller does not read, the
way `terminalBufferClear` already works: the server does the resize and
reports it on the terminal's existing subscription, so main looked at nothing
in the envelope and only a transport rejection was ever a failure.
The prompt `terminal.send` in the create path stays on the raw port. It is the
only `terminal.send` caller that falls back to its own copy when the host
refuses with an empty message, so no existing operation carries its acceptance
and a new one is a fourth method outside this migration's scope. It is recorded
either way.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): lower the raw-port inventory and refresh the session route pins
Pending raw-port inventory: two entries deleted and one lowered, 12 files / 21
references to 10 / 17. The startup and display-mode entries reach zero; the
create entry keeps the prompt `terminal.send` and states its own reason.
Three stale comments corrected. The startup, create and display-mode entries
claimed a WebView-ref or subscription wall that measurement did not find: none
of the three hooks imports the terminal WebView, the display-mode write is not
gated on an open subscription, and the create path's `subscribeToTerminal` is a
scope callback rather than a `client.subscribe`. The accounts screen's entry
said the runner is request-only, which stopped being true when `ScenarioStep`
gained `frame`; what actually blocks it is that no scenario has been written for
`accounts.subscribe`, so its entry now says that instead.
Unchecked-reader inventory: `mobile-session-write-operations.ts` 8 to 10 for the
two readers the migration added, named in the header the way #20954's three are.
Route parity: four pins refreshed with their reasons — the callback bodies for
the display-mode toggle, the effects for the startup activation pair, the nested
function bodies for the create, and the runtime strings, whose count falls 535 to
531 as four more method literals move to their operations' definitions. The
startup source pins now name `worktreeActivate` and still hold what they held:
the plain activation is fired rather than awaited, and it goes out before the tab
load.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* docs(mobile): say which part of the display-mode operation no golden holds
Post-refactor census survivor, measured rather than assumed: swapping
`terminalDisplayModeSet`'s acceptance for `require-result-or-throw-message`
moves none of the fifteen goldens. The call site reads no verdict and its own
`catch` swallows a throw either way, so no policy is observable there. The
method, the params and the viewport pair are what the goldens hold at that site.
The six other operation-level mutations all kill.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): record the empty cells the session guards are written for
Three session sends are gated on a cell every existing scenario filled: the
display-mode toggle carries `viewport` only once a surface has measured one and
`client` only once the phone holds a device token, and the startup sequence
swallows a refused tab load before loading terminals behind it. Every recording
declared those cells full, so the arm each guard exists for was never on the
wire and dropping the guard moved no golden.
The two device cells become scenario arguments rather than adapter constants, so
a scenario can declare them empty; the tab load may now be declared to reject,
which is the only way a refused scope callback is reachable at all. Declared, not
shaped: the stubs build no param and swallow no throw.
Three scenarios take the empty arm. The token and viewport ones send `auto`,
which is the direction both members ride, and the startup one records that the
terminal loads and the activation timer still run behind a refused tab load.
Recorded at the pinned baseline through the detached-pin worktree recipe, since
this branch may not repin. 705 goldens identical, 0 body moved, 3 added, 0
deleted; the 15 header-only moves are `adapterSha256` on the three edited
families and `scenarioSha256` on the four scenarios that now declare their token.
The create adapter's determinism comment now names the draw it works around:
React's lazy `("require" + Math.random())` in `enqueueTask`, the scheduler line
that seeds the sequence, and the mismatch a misplaced mount reports. #21088
retires the workaround.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): witness the three session guards the recordings had not pinned
Each mutation is the guard deleted: the display-mode send carries `client` with
an empty id, carries `viewport` before anything measured one, and the startup
sequence lets a refused tab load reject it so the terminal loads and activation
timer behind it never run. All three survived the whole suite before the
scenarios above; the witness asserts each is killed by its scenario and that
every other scenario of the same family still cannot see it.
A mutation that changes a param the scenario completes aborts at the transport's
params assertion instead of producing a divergent recording. That is the
scenario detecting it, so the witness reads that one message as a kill, narrowed
to it and taken only after the anchor is proved applied.
The README gains the class as its fifth bounding fact: a value an adapter holds
as a constant is a cell no scenario can empty, so the arm that reads it empty is
unreachable until the constant becomes an argument. Corpus counts refreshed to
what the suite measures.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* refactor(mobile): drop the terminal-create result type nothing reads
`TerminalCreateResult` wrapped the created tab for the old `sendRequest` reply
shape. The migrated call site reads the tab off the operation and names the tab
type directly, leaving the wrapper with zero readers repo-wide. Using it at the
cast site would have kept the cast and only renamed it, so it goes.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): let the create scenarios declare what the create puts on the wire
The terminal-create adapter decided four of the members its own goldens hold:
the worktree, the tab a new one is inserted after, and every launch option but
the prompt and its two toasts. A value an adapter supplies itself is a cell no
scenario can empty, so `afterTabId`'s omission arm — the arm a fresh session and
a last-tab close both take — was unreachable, and the quick-command members were
recorded only as absent. All of it now comes from the scenario, and the mount
moves to the first action so the arguments are in place before the hook reads
them. It stays out of a scripted mount step for the determinism reason above it.
Four scenarios follow the new arguments: a create with no active tab, a shell
quick command, an agent quick command, and a second tap while the host is still
answering the first. The refused scenario stops declaring an `errorToast` the
adapter dropped: forwarding the toast independently of the prompt is what the
product does, so that golden now records the failure toast it always showed.
Recorded at the branch's pin, so 705 goldens stay byte-identical to the merge
base; five headers move on adapter and scenario digests and one body moves, the
refused create's new toast effect.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): witness the three create guards the recordings had not pinned
Each of the three new create scenarios closes a mutation that survived all 853
tests before it: putting the active tab on the wire as `null` instead of
omitting it, swapping the `command` and `agentPrompt` members the host reads,
and dropping the in-flight guard so a second tap opens a terminal nobody asked
for. The witness asserts the hole and the closure together, as the others do.
The params-mismatch abort the witness reads as a kill now rests on an assertion
rather than on an argument: no scenario in the manifest completes a request
after its last checkpoint, so a send whose params stopped matching always
suppressed an observation a golden holds.
Known-open holes loses its prose count and becomes a list that names the site,
the mutant and why no scenario can see it. Two entries join it: the display-mode
acceptance, which no call site reads, and the startup timer's attached-terminal
guard, which needs an adapter that can attach a terminal mid-scenario.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* refactor(mobile): interpret the activation reply where it is reported
`reportActivationOutcome` took a verdict, which left the timer site hand-building
`{ accepted: false }` for the case where there is no reply to interpret at all.
Taking `RpcResponse | null` and interpreting inside puts the operation's own
policy at both sites and spells the absent reply as absence. Nothing is lost:
`worktreeActivate` reads an unchecked payload and admits every success, so its
`interpret` cannot throw on a reply either site can receive.
No golden moves; the effect digest is repinned.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): give the create family its mount step back
The create adapter mounted inside its first action so the create would run
ahead of the flush that made React pay its one lazy `Math.random()` draw.
#21088 pays that draw in the scheduler before it installs the seed, so the
position of the mount no longer decides which seeded value `clientMutationId`
reads, and the family goes back to the shape every other one uses: a declared
`mount` step carrying the cells the hook reads as it renders — the worktree,
the active tab, the device token — and a `create` step carrying the launch
options it passes.
The display-mode family keeps mounting from its `mount` action, which is that
same declared shape and never was the workaround.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): re-record the corpus at main's pin
|
||
|
Brennan Benson
|
4b87bc718e |
refactor(agent-launch): redefine the agent.launch contract (#20999)
* refactor(agent-launch): redefine the agent.launch contract
`agent.launch` has no clients yet, so the contract is redefined in place
rather than versioned.
- params require `operation.id`, pinned to the shipped operation-id mint so
the host can read the embedded timestamp back. No caller-supplied
fingerprint: the host derives its own.
- the result carries `disposition` ('created' | 'replayed', the same
vocabulary `RuntimeCreateAgentSessionResult` already uses) and a single
top-level `warning` instead of one on the terminal arm only.
- the prompt receipt becomes an outcome enum, so a receipt can under-claim
instead of reporting a bare `delivered: false`.
- the dead `customization` field is deleted, and the mode-reason union and
receipt are declared once in shared with main re-exporting.
- `clientMutationId` joins the reserved create fields, with a test pinning
the list to the create schema in both directions.
Contract only; no behaviour change and no ledger wiring.
* docs(agent-launch): stop calling the stripped set "agent fields"
`clientMutationId` joined AGENT_LAUNCH_RESERVED_CREATE_FIELDS, so three
comments describing the stripped set as agent fields now teach the wrong
model — including a SAFETY rationale, where a reader is trusting it most.
The rationale's claim is unchanged and still sound: deleting keys from a
parsed object leaves the rest the parsed shape.
* refactor(agent-launch): make the attempt id the launch's only idempotency key
Review follow-ups on the contract redefinition.
`operation: { id }` becomes a flat `clientOperationId`, spelled the way
`terminal.createAgentSession` and the structured mutation envelope already
spell the same concept, and admitted by the shipped
`parseAgentSessionOperationTimestamp` rather than a second copy of its
pattern — so `agent-session-host-authority` keeps the regex private.
The handler now dedupes on that id instead of the create payload's
`clientMutationId`. That field is optional, so keying on it left any launch
that omitted one with no idempotency at all, while the required attempt id
did nothing. Reserving `clientMutationId` is still right, but for the reason
the comments now give: `createManagedWorktree` never reads it, so a copy left
in the forwarded payload is inert while still reading as a guarantee. The
previous rationale — that it was a second live dedupe key — was not true.
`messageId` moves onto the prompt receipt's `journaled` arm so a producer
cannot report the text as committed without saying where, and `rpcCallerKey`
picks up the `terminal.create` call site it was lifted from instead of
shipping with no callers.
* docs(agent-launch): record why disposition is two-valued only for now
The ledger admits attempts whose outcome was never recorded, and neither
`created` nor `replayed` can say "I cannot tell you" — a caller handed
`created` for an unresolved attempt starts a second agent. Noted at the type
rather than in review, so whoever wires the ledger reads it where they edit.
* fix(agent-launch): keep contract within implemented guarantees
|
||
|
Brennan Benson
|
2fbdada551 |
docs(native-chat): correct why a slash command is inert in the answer row (#21111)
The previous note said running a command from the question card's free-text row could only answer with command text or abandon the prompt. That is wrong about skills, and silent on the real cause. Verified against a live structured session: the typed answer is delivered verbatim as the AskUserQuestion tool result, so it reaches the model but never the command parser. A client-side command is therefore inert; a skill name can still be acted on because the model simply reads it. |
||
|
Jinwoo Hong
|
2e3a24c30f |
fix(cloud-auth): keep Sign in clickable during a pending browser wait (#21078)
* fix(cloud-auth): keep Sign in clickable during a pending browser wait Closing the cloud sign-in tab used to leave every Sign in button disabled as "Signing in…" until the 5-minute loopback timeout. A second click now starts another wait, the first tab can still complete, and the first successful callback wins. STA-7610 * fix(cloud-auth): satisfy typecheck and localization after Sign in unlock Keep the account-pane mock able to represent a missing auth status, and drop unused Signing in catalog entries now that the wait no longer relabels the button. * fix(cloud-auth): ignore a stale sign-in after a later wait succeeds A second Sign in click still starts a new loopback wait. Completing that newer wait links the session; finishing the older tab afterwards is cancelled instead of overwriting the linked identity or toasting again. * test(cloud-auth): cover post-exchange stale connect and pending Sign in Pin the branch that discards an earlier token exchange after a later wait has already linked, keep Sign in enabled while connect is still pending, and suppress a failed toast when auth is already connected. * fix(cloud-auth): do not relink an in-flight sign-in after sign-out Signing out now invalidates outstanding PKCE attempts in main and the renderer so a later browser tab cannot restore the session. * fix(cloud-auth): do not wipe a newer connect that finishes during sign-out If sign-in completes while revoke is still in flight, skip session clear and unlink so the new session survives. Do not toast signed-out when auth is already connected again. |
||
|
Brennan Benson
|
533b0bd02e |
fix(native-chat): count a turn from the send that opened it (#21086)
* fix(native-chat): count a turn from the send that opened it The live turn indicator switched on at the submission but anchored its clock at the provider turn-open, so it jumped back by exactly the dispatch latency the moment the turn opened. Measured on a real Claude session: the counter climbed to "Working for 25s", reset to "Working for 0s", then settled "Worked for 26s" — three readings of one turn, from two different instants. The host now resolves the send that opened a turn and publishes it as an additive optional `requestedAt` on the turn lifecycle row. `startedAt` keeps its exact meaning, the provider turn-open, and is never rewritten, so clients that cannot be upgraded see no change to any value they already read. Both providers write it; it is omitted when no send can be named (provider-resumed turns, replayed history). Readers take one origin, `requestedAt ?? startedAt`, for both the live counter and the settled host interval, so the two cannot disagree. The provider's own reported duration keeps outranking the host interval, unchanged. The host-to-local clock conversion is now latched once per turn rather than re-derived per render. `receivedAt - hostNow` carries that sample's one-way delivery latency as well as skew, and the reducer replaces the sample on every frame, so re-deriving imported fresh jitter and could move the anchor later — the same class of backwards jump this change removes. With the conversion fixed, an origin that improves moves the anchor earlier by exactly that much, so displayed elapsed only grows. No monotonicity guard is added; the ordering is structural. Desktop and mobile drove byte-identical copies of the timing hook, so both are collapsed onto one React-free helper in shared. Regression tests drive the origin resolution rather than an already-resolved anchor, assert in milliseconds because second-flooring hides the sub-second case, and include a deliberate host/client skew so a raw timestamp assignment cannot pass on a machine where the two clocks agree. * fix(native-chat): correlate Codex turn origins by echo * fix(native-chat): preserve causal turn timing ownership * fix(native-chat): keep settled turn timing continuous |
||
|
Jinwoo Hong
|
1c4f271478 |
test(mobile): repin the recording baseline to main after #21088 (#21105)
#21088 landed product changes on two fenced paths — the mobile hosted-review create params and the shared hosted-review contract — without moving the manifest baseline, so the corpus stayed pinned to |
||
|
Neil
|
52b53c5bda |
feat(settings): choose the default terminal shell (#21085)
* feat(settings): configure default terminal shell * test(terminal): cover default shell selection * fix(terminal): apply shell setting to daemon PTYs * test(terminal): provide PTY dimensions in shell cases * fix(settings): clarify default shell behavior * feat(settings): make shell choice explicit * fix(settings): keep shell control testable without preload * fix(settings): slim terminal shell control * chore(i18n): allow terminal shell setting labels * chore(i18n): mark dynamic shell label |
||
|
Jinwoo Hong
|
9add08bb59 |
test(mobile): recorder follow-ups — write ordinal, teardown streams, context anchor, salvage observation, provider pass-through, React draw (#21088)
* refactor(mobile-recorder): one shared write ordinal for requests, payloads and effects
`sent` stamped each payload and effect with the number of requests sent at
write time, which orders those two lists against sends but never against each
other. A family that sends no requests therefore had every stamp at `0`:
moving `host-worktree-refresh.ts`'s two initial snapshot reads from after
`client.subscribe` to before it moved none of the 705 goldens.
One monotonic counter per recording now stamps requests, payloads and effects
alike at the moment each is written, so the three append-only lists are ordered
against each other. The same reorder now fails five goldens. A request is
stamped at the logical `sendRequest` call rather than when its physical payload
is published, so a send that waited for connected carries two distinct stamps.
Full re-record from the pinned baseline: 699 bodies moved, 6 header-only,
0 added, 0 deleted; the only moved JSON paths are `sent` leaving and `ordinal`
arriving on `sender`, `payloads` and `effects`. Decoding with those two fields
stripped leaves all 705 header-only.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* feat(mobile-recorder): observe streams still registered at teardown
Closing a stream only writes to the wire when its method has an unsubscribe
builder. `notifications.subscribe` has none, so a cleanup that forgets its
local `unsubscribeStream()` leaks a live registry record and nothing on the
wire changes. Until now that class was covered by one hand-written scenario
per method, which stops the stream and cuts over so the leak reappears as a
second subscribe payload.
Teardown now asks each session's `RpcClientStreamRegistry` what it still holds,
after the product's cleanup and before the transport disposes it, and records a
non-empty answer as a `streams-registered-at-teardown` effect carrying each
stream's method, subscribe payload and cancelled flag. The set is read off the
registry's own map: a mirror kept by the recorder would reproduce the product's
bookkeeping rather than observe it. Deleting `unsubscribeStream()` from
`mobile-notifications.ts` fails 7 goldens now, against 1 before.
Re-record: 4 bodies moved, 701 header-only, 0 added, 0 deleted. All four are
the two `runtime.clientEvents.subscribe` matrices, on partitions whose subscribe
reply is not a well-formed `ready`: with no subscription id to unsubscribe with,
the registry deliberately holds the cancelled record, which is why the
observation carries `cancelled`.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* refactor(mobile-recorder): one host-client context exposure, anchored on the product source
Five adapter modules each carried `exports.recorderHostClientContext = Ctx;`
inside a source string appended to `client-context.tsx`. `Ctx` is a
module-private local, so the reference lives in a string no type checker
follows: renaming it typechecks clean and fails a recording with a
`ReferenceError` a hundred seconds in, five times over.
`hostClientContextExposure` and `loadHostClientContext` are the one copy, and
`adapter-seam.test.ts` asserts the declaration the exposure names still exists
exactly once in `client-context.tsx` and refuses a sixth inline copy. A rename
remains invisible to `tsc` — nothing but editing the fenced product module
makes a private local checkable — so the anchor is what turns it into one
failure that says what moved.
Also splits the subscription tests out of `recording-runner.test.ts`, which
items 1 and 2 had pushed past `max-lines`.
Re-record: 705 header-only, 0 bodies moved, 0 added, 0 deleted; `recorderSha256`
on all 705 and `adapterSha256` on the 23 goldens mounted through the five
modules.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* feat(mobile-recorder): record what a checked read salvaged
`collectSalvageDrops` builds a report on every decoded reply — which array
elements a `salvagingArray` threw away, which members a `salvagedOptional`
read as absent — and `classifyRpcReply` puts it on the outcome, where nothing
reads it. Which rows a reply lost was therefore visible nowhere, including in
a golden.
The recorder wraps `classifyRpcReply` on the mounted module, the one seam every
checked read passes through and the only one that knows the operation the drop
happened under, and records a non-empty report as a `reply-salvage` effect. No
product code changes; the report was already being built and discarded.
No golden carries one. All 19,384 checked reads in the corpus decode their reply
whole, because the reply matrix varies the envelope a host sends rather than the
shape of a row inside a result. The observation pins that absence, and moves the
first time a narrowed element or member schema drops a recorded row — including
where nothing downstream reads it. `salvage-observation.test.ts` is what keeps
the observation honest, driving a malformed row and a malformed optional through
the real `git.status` reply schema.
Re-record: 705 header-only on `recorderSha256`, 0 bodies moved, 0 added,
0 deleted.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(source-control): let hostedReview.create carry a provider token this build does not list
`HostedReviewCreate.provider` was a closed `z.enum`, so a client repeating back
a provider a newer host named in its own eligibility reply had its create
rejected at params validation. Mobile worked around it with a SAFETY-annotated
assertion: narrowing to `'unsupported'` before sending would have made the host
refuse its own provider, so the token was cast through instead.
The schema member is now `z.string()`, and both create handlers narrow through
`supportsHostedReviewCreation` before calling the runtime, so an arm this build
does not know answers `unsupported_provider` with readable copy rather than a
params error the client cannot act on. `createHostedReview`'s own refusal is
the single source of that copy. The mobile assertion is deleted.
Product change on a fenced path, so the goldens are not re-recorded: the whole
recording suite replays green against the corpus committed in the previous
commit, 825 passed, zero golden movement.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(source-control): annotate the runtime stub cast in the provider refusal test
The changed-code quality gate counts a new `as unknown as OrcaRuntimeService`
as a finding. A narrower stand-in does not exist: the interface has 1047
members and `Pick` of the three this test uses is not assignable.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile-recorder): pay React's lazy Math.random draw before the seeded run
React resolves `enqueueTask` by reading `module['require' + Math.random()]` and memoizes the
result, so a process draws exactly one `Math.random()` the first time it awaits `act`. The runner
drains through `act` after every step, so that draw landed inside whichever recording ran first and
ate the seeded sequence's first value: a family recording a `Math.random()`-derived param recorded
one value when it ran alone and a different one when it ran after any other family, and an adapter
could only dodge it by drawing in its factory ahead of the first drain.
The scheduler now pays that draw once per process, before it installs the seeded generator, so the
seeded sequence starts at the same value for every recording. Priming is awaited, which makes
`start` async.
Goldens re-recorded: 705 header-only, `recorderSha256` alone. No golden carried a first-in-process
value, so nothing moved in a body.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* fix(mobile-recorder): drain before reading the streams left at teardown
The teardown observation read the registry after `dispose()` returned but before the scheduler
drained, so a cleanup that closes its stream on a due 0ms timer had not run yet and was recorded as
an uncancelled registration — the one shape this observation reserves for a cleanup that never ran.
A deferred close and a stream nobody ever closed were byte-identical.
The drain now runs before the read, with the transport still disposed after it. A second drain stays
after disposal: tearing the registries down rejects what the product still awaited, and an unhandled
rejection is an effect the cleanup checkpoint has to see.
Also: the registry size comparison in `registeredStreams()` could never fire, because `size()`
returns `this.streams.size` on the same object; `RECORDER_HOST_CLIENT_CONTEXT` is used only in its
own module and no longer exported; and `streamPayloads` now says what it holds, which is every frame
the registry publishes rather than only subscribes.
Goldens are stale in this commit and are re-recorded in the next one.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): re-record the corpus after the baseline repin and the teardown drain
Recorded from a detached worktree pinned at
|
||
 Jinjing
|
852ee907ee |
fix(e2e): stabilize flaky E2E tests against timing races (#20900)
* fix(e2e): stabilize flaky E2E tests against timing races - Paired terminal: use stable cold activation assertion instead of racy one-shot read; background tabs park eagerly. - Native chat: scope hydration assertions to transcript subtree to avoid false positives from UI chrome (worktree rows, tab titles). - Onboarding: inject verified status snapshot with max sequence to prevent hydration from downgrading host health during skip-to- project-setup. - Paired web: encode host health faults in snapshots with high sequence so real hydrations cannot outbid injected state. - Quick open: clear prior tooltips and increase hover timeouts to handle streaming result remounting. - Terminal attention: pass 'terminal-bell' to unread marker to match production contract (reads marker value, not presence). * fix one last test |
||
 Gon SongandNeil
|
85d1ffc072 |
fix: accept enterprise managed GitHub owner logins (#20450)
Unify owner validation across project pickers and repository overrides. Preserve EMU usernames in API and auth-status branch-prefix resolution, with regression coverage. Co-authored-by: Neil <neil@stably.ai> |
||
  OrcaWinandm4air
|
6101f0169f |
Make CLI reveal labels translatable (#21079)
* fix(i18n): make reveal labels translatable in CliSection
Platform-specific reveal labels ("Show in Finder", "Show in Explorer",
"Show in File Manager") are now wrapped with translate() for i18n
support. Also backfills missing translations in non-English locales.
* add trams;atopm foxes
---------
Co-authored-by: m4air <m4air@Mac.localdomain>
|
||
|
Jinwoo Hong
|
66a894d913 |
test(mobile): repin the recording baseline to main after #19850 (#21092)
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Brennan Benson
|
e6a3b5d019 |
docs(native-chat): record why the question answer row has no slash grammar (#21084)
The free-text row on an AskUserQuestion card is a plain input on purpose, but nothing said so, and its absence reads as a missing picker rather than a decision. Note the reason at the input. |
||
|
Brennan BensonandMerge Sim
|
97aa5ff19b |
fix(mobile): open native chat when a new worktree launches a default agent (#19850)
* refactor(agent-launch): make the launch-mode decision surface-neutral
`decideWorkerStartMode` was the only shared answer to "structured chat session
or terminal agent?", but it lived in an orchestration-named module and spoke
orchestration's vocabulary, so the other launch surfaces could not call it.
Move the decision to `main/agent-launch/agent-launch-mode` unchanged and leave
`orchestration-worker-start-mode` as the adapter that supplies the noun.
A worker is not a special kind of launch; it is the same launch with a dispatch
attached. Naming the receipt's subject is the only thing orchestration actually
contributed, so that is the only thing the adapter keeps: "worker" in both
sentences, plus the `--terminal` wording, which reads as nonsense anywhere a
`--terminal` flag does not exist. Both are pinned, because they are asserted.
No behavior change. The receipts are byte-identical for every reachable case,
proven by running the new pin against both implementations.
Also pins the wording, which nothing was holding. The existing suites assert
`toContain` fragments ('terminal agent', 'cannot create') and the CLI suite
asserts a receipt handed to it by a mock rather than one this code produced;
all six files stayed green against a deliberately corrupted vocabulary. A
dispatch receipt is the only place a structured-to-terminal downgrade explains
itself, so the whole sentence is the contract, not a fragment of it.
* feat(agent-launch): add the launch intent and the one executor that runs it
The sequencing around the launch decision was duplicated per surface, and the
duplicate is where the bug lives. A new worktree was created agent-first, so
its startup terminal WAS the agent and the structured branch below it could
never be reached — every new-worktree launch was a PTY regardless of the user's
default. Orchestration fixed that for itself in #19431; mobile and the CLI
still have it.
`executeAgentLaunch` inverts the order once, for everyone. When the preference
is structured the worktree is created with NO startup agent, the executing host
is then asked whether it can host a session for the workspace that now exists,
and only then is a surface created. The host verdict cannot be hoisted above
creation: `agentSession.createSupport` only answers for a workspace it can
resolve, which is why the decision stays in two halves.
Agent-first creation is deliberately preserved for PTY launches — it is what
sequences the agent's startup command behind the setup runner, so wait-for-setup
comes for free there.
What actually differs per surface is only how a surface is built (an
orchestration worker's session takes a dispatch hold and a mailbox a plain
launch must not take), so that is injected as a factory rather than branched on.
The intent also strips the reserved agent fields from a migrated create payload:
a caller moving off `worktree.create` passes its existing params, and a stale
`startupAgent` in there would re-create the very path this replaces.
Tests assert order and arguments, not just the resulting mode. Reintroducing
agent-first creation reddens 4 of 11.
* feat(agent-launch): expose the launch executor as the agent.launch RPC
Adds `agent.launch` — one host-side method that decides structured-vs-terminal and
creates the surface — wired to the real runtime factories: `createManagedWorktree`
for the workspace, forking on `startupAgent` exactly as the orchestration worker
path does; `createStructuredAgentSessionForWorktree` for a chat session; and
`createTerminal` for a PTY agent. Allowlisted for mobile, which is the surface the
routing gap was reported on.
`worktree.create` is untouched. Its `startupAgent` keeps meaning "spawn a PTY agent"
verbatim, because it answers with `agentTerminalHandle` only on that path: a host
that quietly routed it to a structured session would hand every older client a
response with no handle and no error. All new behaviour sits behind
`agent.launch.v1`, which the host now advertises and a remote client must negotiate,
so a client that does not gets today's behaviour unchanged.
* feat(mobile): route workspace creates through agent.launch
Picking an agent on the mobile create sheet always produced a terminal, even
when the user's default was native chat, because all three create paths put
`startupAgent` on `worktree.create`. That means "create the worktree
agent-first", so its startup terminal IS the agent and the structured branch
below it is unreachable — while the same phone's in-workspace "+" button opened
a chat.
The blank, branch and new-branch creates now send the same payload through
`agent.launch` and let the host settle the surface. `worktree.create` is
untouched, and a host that does not advertise `agent.launch.v1` (read from the
existing `status.get` probe) keeps today's path exactly.
Work-item creates stay on `worktree.create`: they pre-fill the issue/PR URL as
an unsent `startupDraft`, which a structured session cannot hold yet, so routing
them would submit the URL as a first turn.
* fix(agent-launch): drop the deleted draft-prompt blocker from the reason map
main removed the draft-prompt blocker in #19681 (a structured session now holds
an unsent draft), so the exhaustive Record no longer typechecks.
* chore(agent-launch): carry a SAFETY rationale on the agent placement cast
The type-assertion gate landed after this branch's base, so the new file's
copy of the worker-start cast is now a changed-code finding.
* chore(agent-launch): carry agent.launch through main's RPC typing and casting gates
The typed-method contract, the generated params catalog and the
`assertionStyle: never` casting scan all landed after this branch's base.
- AGENT_LAUNCH_METHODS kept an `RpcMethod[]` annotation, which widened its
method name to `string` and broke assignability; every sibling infers instead.
- `agent.launch` binds a schema under src/main, so it joins the catalog's
RPC_METHODS_WITHOUT_SHARED_PARAMS and the parity gate's hand-listed twin.
- The now-typed methods make most test casts unnecessary; the few that remain
carry the line-specific SAFETY rationale the casting gate requires.
* test(mobile): supply the agent-launch fixture the create-submit recording needs
The golden RPC recordings landed upstream while this branch was out, so they
first met agent.launch here. Three things had to happen, and only one of them is
a fixture bump.
1. workspace-settings-mounts.ts mounts useNewWorkspaceCreateSubmit against a
fixture model that throws on any member it was not given. This PR added a
required getAgentLaunchSupport, so the submit aborted with "Missing model
fixture" before it ever issued the create, and three cleanup checkpoints
vanished. That read like a product regression and was not one. Supplying the
member restores the recording byte-for-byte; it is pinned false for the same
reason the cutover probe is, so the baseline stays on worktree.create.
2. Editing that adapter moves adapterSha256 for the twelve settings goldens it
mounts. Their recordings are unchanged - header only, by design: the digest
is per-golden so editing a module fails exactly the goldens that mounted it.
3. Five goldens changed behaviourally, and both changes are this PR's:
the capability probe now reports agentLaunch, and a create whose reply
carries no worktree returns "Failed to create workspace" instead of throwing
a TypeError off an unguarded result.worktree read. The launch route needs
that guard, since a receipt can arrive without a worktreeId.
* refactor(mobile): decode the launch receipt instead of asserting its shape
The changed-code quality gate refuses type assertions, and the eight it flagged
were worth removing rather than suppressing.
The production one was the point. readAgentLaunchCreateOutcome asserted the RPC
payload into Partial<AgentLaunchResult> and then runtime-checked it anyway, so
the assertion bought nothing and claimed a contract the host had not proven. It
now narrows with `in` and validates each hop, which is the same nullability
question readCreateResult already answers on the sibling path - a launch receipt
can legitimately arrive without a worktreeId. AgentLaunchCreateOutcome ties
worktreeId to the shared contract so a change there fails this reader's
typecheck rather than passing a differently-typed field through.
The test fakes claimed a whole RpcClient via `as unknown as RpcClient` while
implementing one member. They now build a typed literal, matching the pattern in
use-mobile-structured-agent-options.test.ts. The read sites cast params and then
read one field; they now assert the payload with toMatchObject, which removes
the cast and pins more of the shape than the cast did.
Also pins the warning passthrough, which nothing covered: a terminal launch that
seats the workspace but cannot start the pty reports why, and the absent, blank,
non-string and structured-surface cases report nothing. Writing that test caught
a real drop I had introduced in the reader.
* ci(mobile): re-run Mobile Checks when a shared capability changes
Mobile Checks is path-filtered to mobile/**, but mobile imports the negotiated
capability names straight from src/shared/protocol-version.ts and records the
whole capability read verbatim in its goldens. So a capability added desktop-side
rewrites a mobile fixture while never triggering the suite that would catch it.
That is what happened here: #19849 introduced agent.launch.v1 and Mobile Checks
never ran on it. Verified at the run level rather than by check name - the
window-free check-runs API on
|
||
|
Brennan Benson
|
0cd05bc3d9 |
docs(contributing): state what a PR description must cover (#21080)
AGENTS.md said nothing about writing PRs, and the template's section comments could be satisfied without ever telling a reviewer what changed for the user or which mechanism moved. Name the same four requirements in both places: no jargon, user-facing before/after, the mechanism, and why over the alternatives. |
||
|
Jinwoo Hong
|
12d744f253 |
fix(skills): keep computer-use off filesystem and shell tasks (#21069)
* fix(skills): keep computer-use off filesystem and shell tasks STA-7615: "On my desktop create a folder" was matching computer-use because discovery copy said OS/window-level and neighboring skills advertised desktop UI. Scope the trigger to visible GUI with no CLI path, and exclude files/folders/git/shell. * fix(skills): prefer programmatic paths over computer-use State the last-resort rule in discovery copy instead of enumerating files/folders/git/shell. computer-use prefers shell, filesystem, git, HTTP, CLIs, and Playwright/CDP; neighboring skills route to Computer Use only when a visible window needs GUI control those cannot do. * fix(skills): stop advertising computer-use from orchestration Orchestration coordinates workers; it does not drive a GUI. Drop Computer Use and Playwright/embedded-browser routing from its discovery description so those tools are not pulled in from a coordination skill. * fix(skills): drop Playwright from orca-cli discovery orca-cli should not prescribe Playwright or CDP. Those tools may not be installed, and page automation is not this skill's job. * fix(skills): drop the page-only ban from computer-use discovery Page automation is a preference, not a prohibition. If Playwright or CDP is not available, a visible browser window is valid Computer Use. Keep the hard split for Orca's embedded browser (`orca-cli`) only. |
||
|
Jinwoo Hong
|
71e308e574 |
feat(relay): count failed cell-inventory lock acquisitions (#21067)
* feat(relay): count failed cell-inventory lock acquisitions The cell inventory lock is taken NOWAIT, so contention errors with 55P03 and retries instead of waiting. CellInventoryHoldSamples.record only runs after a successful acquisition, so the hold metrics were structurally blind to the dominant failure mode: production showed ~65 failed fleet-wide acquisitions per minute while cellInventoryHoldMsMax read a benign 53ms mean. Count failures next to the holds and publish them as cellInventoryLockUnavailable in orca_relay_runtime_metrics. Drained on both the commit and the rollback path, since a 55P03 rolls its transaction back. * fix(relay): separate request-path lock timeouts from sweep deferrals Review caught that the first counter only incremented under failIfUnavailable, which is the sweep mode. Background sweeps take the inventory NOWAIT and re-derive a skipped candidate next tick, so those deferrals are by design and already reported as orca_relay_sweep_cell_inventory_busy. The request path uses a bounded lock_timeout instead, whose expiry raises the same 55P03 without NOWAIT and was not counted at all -- so the metric measured only the benign population and missed the user-visible one. Split them: cellInventoryLockUnavailable for NOWAIT deferrals, cellInventoryLockTimeouts for expired bounded waits. Production over 30 minutes shows why the distinction matters -- roughly 1,200 fleet-wide sweep deferrals against roughly 10/min request-path timeouts. Adds transaction-path coverage for both drains, which were previously unpinned. Timeouts count per attempt, not per request, since 55P03 is retryable. * fix(relay): publish the cell-inventory lock metrics to Cloud Monitoring google_logging_metric.relay_snapshot only creates metrics for fields listed in relay_runtime_metrics, and the cellInventoryHold* fields were never added when the hold telemetry landed. They have been log-only since, so nothing could alert on the lock and the contention stayed invisible in exactly the way the telemetry was meant to prevent. Maps the three hold fields and both new failure counters. Also corrects the field comment: the split is by wait policy, not by caller. assignOnce takes the inventory fail-fast on its first placement attempt, so request-reachable sites land in cellInventoryLockUnavailable too; that lane reads as contention pressure, and the expired bounded wait is the stall lane. |
||
|
Brennan Benson
|
f02d09c1ba |
fix(native-chat): deliver queued messages while the chat pane is hidden (#20659)
* fix(native-chat): deliver queued messages while the chat pane is hidden With two or more messages queued, everything behind the head waited on the user's attention. The drain only inspected the head and returned unless it was `queued`, and a `pending` send deliberately leaves the head `dispatching`. An entry only leaves that state through the journal subscription, which is torn down when the pane goes hidden -- and a worktree switch hides it. Two changes, both needed: - One shared admission rule now says what the queue does next, and the drain takes its `dispatch`: the first `queued` entry, skipping entries the host has already acknowledged. It still stops at an `unconfirmed` entry or a refusal the user must act on. Order is not the outbox's to keep -- the host appends the submission inside the per-session serialize chain before dispatching, so journal order is arrival order. Holding the tail bought no ordering guarantee and cost delivery. Single-flight still keeps sends strictly sequential, and a launch prompt's in-flight send, which runs outside it, still stops the queue. - The journal subscription now stays open while a session has undelivered outbox entries, published from the `writeOutbox` choke point. The subscription's retaining hold is what also keeps the host from evicting the session 15s after the last turn, which would otherwise turn the stall into a blocked head refusing `agent_session_ownership_unknown`. An acknowledged entry stays in the outbox rather than retiring on `pending`: the text is safe either way, since the journal upserts a render item from the submission's own body, but a `pending` can still settle `rejected` or `unknown` and only the entry carries the retry state that answer needs. Follow-on corrections the head-only assumption had hidden: - Single-flight is released where the disposition is applied, not in a later `.finally`. That state write is what re-runs the drain, so the release has to land first or the queue has no trigger left. - One ref now holds the in-flight entry's id instead of a bare boolean, and the reconcile effect keys its release on that, not on the head, so a journal update about the head can no longer discard a still-unsettled send of the tail. - A refusal blocks the entry it refused, read back by index so a rotated id is preserved. - The automatic unknown probe and the Retry affordance both read the blocker at whatever index it sits, the Retry through the same shared rule as the drain. `raises no delivery notice for a stuck message behind a healthy head` asserted that a message behind an admitted head raises nothing, because a Retry could not act on it. It now can, so that guard is rewritten to assert the notice names that entry and its Retry sends that entry. * fix(native-chat): resume outbox after journal admission and scope subscriptions * test: name outbox send request by domain role |