mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 16:02:32 +00:00
stack-structure
11349
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jinwoo Hong
|
4a86b2dc56 |
refactor(mobile): checked reply readers for files, dictation, host-screen and agent-history (step 7) (#21269)
* test(mobile): record main's file-preview and markdown-disk-fallback replies
Four of this branch's read sites had no malformed-reply coverage, so the reader
change would have had nothing to move at them. `familyGoldens` matrixes only the
first scenario of each family, and `files.preview-load`'s base is the grant-refresh
chain while `session.tab-documents`' is the served markdown tab — which left
`files.read` and `files.readPreview` on the worktree preview path, the artifact
image read, and the markdown tab's on-disk fallback recorded on their success path
only. This commit is the before picture, taken from main's own tree with no product
edit in it.
Three new families, five scenarios, ten goldens:
- `files.preview-worktree-text` / `files.preview-worktree-image` — `files.read` and
`files.readPreview` as the preview screen asks them for a worktree file.
- `files.preview-artifact-image` — `files.readTerminalArtifactPreview`.
- `session.markdown-disk-fallback` — the `files.read` leg a headless host's
`renderer_unavailable` sends the markdown tab down. It carries a second scenario
that serves `markdown.readTab`, because a matrix site needs a fulfilled reply
recorded somewhere in its own family to replay as the `normal` partition.
No existing scenario moved to a new family and no adapter changed, so every
pre-existing golden keeps its `adapterSha256` and `scenarioSha256`. Recorded in a
detached worktree at the manifest's pin (`4b876758d3`) with this manifest copied in;
the control is that all 748 pre-existing goldens came back byte-identical to
origin/main's, which `git diff
|
||
|
Jinwoo Hong
|
9de6f2c6cd |
test(terminal): bump the pane hook-order parity pin past #9035 (#21276)
* test(terminal): bump the pane hook-order parity pin past #9035 #9035 added a useRef and a useCallback to use-terminal-pane-foundation (search input ref, focus-search-input) without moving the parity pin, and its own PR run never executed the shard that holds it. Every PR opened since fails `tests node 24 7/8` on `expected 211 to have a length of 209`. The two hooks are in order behind the existing ones and useMemo stays at 8. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(terminal): re-pin the hook-order hash for the two #9035 hooks The count alone was not the pin: the flattened order is hashed too. The new order is the old one with useRef and useCallback inserted at the foundation stage and nothing else moved (diffed before and after #9035). Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
 Brennan Benson
|
b66ef2e8a8 |
fix(agent-launch): resolve a launch scope, not a git worktree record (#21193)
* fix(agent-launch): resolve a launch scope, not a git worktree record `agent.launch` asked the runtime for a managed worktree record and then read exactly one field off it, `.id`. That record does not exist for every workspace a launch can run in, so the request refused launches the method could otherwise run: the floating workspace resolves to a scope with an id and a path but no worktree row, and `showManagedTerminalWorkspace` throws `selector_not_found` rather than hand back the id it had already resolved. A folder workspace survived that only because the resolver fabricates a worktree row for it. The scope is the answer that is real for all three kinds, so the launch asks for that instead. `showManagedTerminalWorkspace` is unchanged - callers that genuinely need the git record still get it, and still get the refusal. With floating now reaching the mode decision, the host must know which kind of workspace it resolved. The kind is derived from the id it resolved itself, never accepted from a caller, and the route module's existing `floating` blocker does the rest: a workspace with nowhere to keep a session runs a terminal agent. Behaviour change, deliberate: a floating-workspace `agent.launch` used to fail with `selector_not_found` and now succeeds as a terminal agent. That is what lets the floating titlebar agent button move onto the shared launch command instead of driving tab startup itself. No wire change: `AgentLaunchTarget` is untouched. * test(agent-launch): cover floating RPC workspace resolution |
||
|
Brennan Benson
|
434365d2de |
Offer to reconnect native chats that were working when Orca restarted (#21096)
* feat(native-chat): resume structured chats that were working at restart
Teardown records a marker for every session this host was genuinely running a
turn for, derived from the LIVE runtime rather than a persisted status row, so
a stale `running` row left by an older crash can never trigger a resume. On the
next launch a modal lists exactly which chats would resume and resumes them via
native continuation (Claude resume/resumeSessionAt, Codex thread id) — never by
re-sending the prompt, which is what makes an agent redo finished work.
A session resumes only when all of these hold: a teardown marker exists and has
not expired, the record's lease is released and reconciled, a provider resume
cursor exists and still matches the marker, the journal's own turn record names
the same turn, and the marker has not already been spent. Markers are consumed
before the resume is submitted, so a crash mid-resume cannot double-fire, and an
admission gate refuses a second concurrent resume for one session. Resumes are
staggered three at a time rather than spawning every provider at once.
The modal's "Don't ask again" checkbox writes the nativeChatResumeWorkOnRestart
setting, which Settings can turn back off; automatic mode runs the identical
predicate and staggering and reports what it did. Declining consumes the markers
so the prompt cannot return every launch — nothing is lost, because opening a
chat still re-acquires it at the same cursor.
* fix(native-chat): compare handle ROOT and turn state when offering a resume
Four defects QA found in the restart-resume offer, fixed together because the
first two interact: shipping the root fix without the state fix would convert a
silent no-op into actively offering finished chats.
1. Claude was never offered (0/4). The marker recorded agentSessionProviderHandleKey,
which embeds Claude's leaf uuid — a branch cursor. The adapter's own close path
appends a `resumed` link with an advanced leaf during the SAME teardown, so the
marker went stale seconds after it was written and the drift guard refused every
Claude session forever. Record and compare agentSessionProviderHandleRoot instead:
the root is the part a resume must preserve, and changing it is a fork, which is
exactly what this guard is for. Codex is unaffected (its thread id is the whole
key) but uses the root too, so the rule is uniform.
2. The predicate compared turn IDENTITY but discarded turn STATE, so a `completed`
turn satisfied it as readily as an interrupted one. Eviction rewrites `running`
to `interrupted` and never to `completed`, so the state is what separates work
that was cut off from work that finished. Require `interrupted` or `unverifiable`.
3. A chat blocked on a pending approval or question was marked as working, because
the teardown reader accepted any `running` turn while the product's own projection
calls that state `attention`. Teardown now defers to that projection: an agent
waiting on the USER is not interrupted work.
4. "Resume all" could silently no-op. The modal fetched candidates at mount; by click
time the chat's own pane may have bound and taken the hold, moving the lease to
`live` so the predicate dropped it and the call returned no results, leaving the
dialog open behind a dead button. Re-derive at click time and settle an
already-live session as resumed — it is running, which is what the user asked for.
Test fakes now model the Claude close path that advances the leaf, which is why no
unit test could previously exhibit defect 1. Ablation covers all eleven guards.
* fix(native-chat): gate the already-live settlement on the full resume predicate
Two follow-ups from re-QA, both cases of a rule stated by intent rather than by
discriminator.
1. The already-live path bypassed the predicate. "Resume all" sends no session
ids, so the fallback's target set was every marker, and it was gated only on
the session having a live provider child. A chat the predicate had refused --
a completed turn, say -- whose pane happened to own the lease was therefore
settled as `already_live` and had its marker spent, inflating the "Resumed N"
count with chats that were never eligible. No provider spawned and no tokens
were spent, but a marker the predicate rejected must never be consumed.
The resumable set now takes an explicit `leaseState`. The already-live path
derives a second set with ONLY the released-lease clause relaxed, and settles
a session just when it is in that set. Every other clause still applies.
2. The `attention` rule was one-sided. Teardown refuses to mint a marker for a
chat blocked on the user, but the set predicate had no equivalent, so a marker
arriving by any other route was offered once eviction rewrote its turn to
`interrupted` -- the same asymmetry the completed-turn case had.
Gated on projectStructuredAgentSessionStatus === 'attention'. That projection
tests for a pending approval or question BEFORE it looks at turn state, so it
still reports `attention` after the turn is settled, which makes it the durable
signal and keeps one source of truth with teardown.
Ablation now covers thirteen guards, including one for each of the above.
* fix(native-chat): capture awaits-user on the marker instead of re-deriving it
The awaits-user clause could never fire. It asked the live projection for
`attention`, which needs a prompt whose resolution is still `pending` -- but
teardown CANCELS that prompt a few phases after it writes the marker. By the next
launch the evidence is gone, for precisely the sessions the clause was written
for. QA measured the injection still being offered and then resumed.
This is the same shape as the leaf-drift bug: state read after teardown is not the
state that justified the marker. The discriminator, now applied across the whole
predicate:
- a fact teardown itself destroys or mutates must be CAPTURED on the marker
while it is still true;
- a fact that evolves on its own must be RE-DERIVED at read time, never
snapshotted.
So `awaitsUser` is now recorded at teardown and the predicate reads the recorded
value. Teardown still declines to mint a marker for such a session, so the
recorded flag is the second line rather than the only one.
Audit of every other clause against the same test:
- turn id (captured) -- teardown rewrites turn STATE but never the id. Correct.
- provider handle root (captured) -- the close path appends a resumed link, and
appendAgentSessionProviderHandleLink refuses one that changes the root, so the
root is invariant under exactly the mutation that broke the key. Correct.
- turn state (re-derived) -- DELIBERATE exception, stated here rather than left
implicit: we are not reading the state that justified the marker, we are
reading teardown's receipt that it settled the turn. A turn still `running`
means eviction never finished, and we refuse. Correct, and intentionally so.
- lease reconciled / released / handoff stage (re-derived) -- these answer a
different, launch-time question: may this host take the lease NOW. The
teardown-time value would be meaningless, and `unreconciled` is cleared by
this launch's own reconciliation. Correct.
- adapter support, marker TTL, marker consumption (re-derived) -- all evolve
independently of teardown. Correct.
Only awaitsUser was on the wrong side.
* fix(native-chat): drop the unreachable awaits-user marker flag
The captured flag was dead code. `awaitsUser` could only be true when the
projected status was `attention`, and `attention` hits the `continue` above the
push -- so every marker teardown can ever write carries `false` (QA measured
22 of 22 across two real teardowns). The predicate clause reading it was
unreachable by any production path.
A flag that is structurally always false is worse than no flag: it reads as a
safeguard, so the next person to touch this trusts it. The asymmetry it was
added to close was only ever reachable by fault injection, because teardown is
the sole writer of markers and already refuses attention sessions.
Removing it also drops an upgrade discontinuity: as a required field it made a
marker written by the previous build fail validation and be silently discarded,
costing a resume offer on precisely the upgrade where the user was mid-turn.
Markers predating the providerHandleRoot rename still will not parse, but those
carry a leaf-sensitive key the predicate would refuse anyway, so nothing usable
is lost.
In its place the teardown gate now states that `status !== 'working'` is the
SINGLE gate for awaiting-user sessions, why a predicate-side mirror would be
unreachable, and why it could not even re-derive the fact -- so the reasoning is
inherited rather than rediscovered.
Ablation is back to twelve guards; every other clause is unchanged.
* fix(native-chat): say reconnect, not resume, and show each offer's age
Two changes, both independent of the parked continuation decision.
1. The copy claimed something QA disproved. "Resuming continues each agent where
it left off" is false: reconnection restores the session at the point it
stopped, with full context and without re-sending the prompt, but the
interrupted reply does not continue on its own. The toast's "Resumed N chats"
implied work had restarted.
Audited every user-facing string against the rule that none may claim work
continues or that a reply resumes -- which caught more than the three strings
the fix started from. The title, the row button, "Resume all", "Resuming...",
the not-now hint ("picks it up where it left off"), the checkbox and its hint
("resume on their own"), the list's aria-label and the Settings row all made
the same claim. The user-facing verb is now reconnect throughout; the body and
update variant state outright that the interrupted reply will not continue.
en.json synced, runtime boot catalog regenerated.
If we later decide to send a continuation instruction, this is one commit to
change back. Shipping text we know to be false was the worse option.
2. Rows now show each offer's age. The TTL is 24 hours and a stale offer looked
identical to a fresh one. The marker already carried `recordedAt`, so this is
a render change plus one field on the renderer's candidate type, formatted
with the existing formatUiRelativeTime helper rather than a new one.
The clock is stamped once when the list arrives rather than read during render:
ages then stay stable across re-renders, and the render stays pure, which the
react(purity) rule requires.
Guards, predicate and RPC are untouched; ablation still covers twelve.
* feat(native-chat): show the workspace name on each reconnect row
A row read `codex · folder:8f3a1c22-… · 8 hours ago`. Recognising which chats
would reconnect is the entire point of the list, and at twenty rows a UUID
identifies nothing.
No RPC or host change was needed: the renderer can already resolve this id.
Resolved the way automation dispatch resolves the same id space
(resolveAutomationDispatchWorkspace) -- a folder workspace by its full
`folder:<uuid>` key via getKnownWorktreeById, a git worktree by its bare
`repoId::path` id via allWorktrees. Both return a Worktree, whose displayName is
a required field, and DetectedWorktree extends Worktree so either shape answers.
Falls back to the id when nothing resolves, which is what the row showed before
and also covers the window before the worktree store has hydrated.
The lookup lives in a per-row subcomponent because a hook cannot run inside
`map`, and its selector returns a primitive string so repeated selector runs
cannot churn referential equality.
* feat(native-chat): group the reconnect modal by worktree and add opt-in continuation
Grouping. Rows are now grouped under a worktree heading with the repo glyph and
an agent count, using the sidebar's own collapse mechanics. Only presentational
pieces are reused -- RepoIconGlyph, CompactAgentExpansion, AgentIcon and
formatShortTimeAgo. The sidebar's agent row cannot be: worktree-card-compact-agent-row
imports DashboardAgentRow, the dashboard's own type, so both surfaces render one
live-agent model requiring a pane, tab and status entry. Every chat offered here
is by definition stopped, so supplying that would mean inventing live state.
Two things I had assumed were reusable and were not:
- DashboardHostBadge returns null unless hostKind is ssh or remote. Structured
chat is local-only, so it would always render nothing. The host line is
omitted rather than faked; the badge is the right element to add if and when
structured chat gains remote support.
- No state dot. Every AgentDotState misleads here: idle and unverifiable both
presuppose a live pane, interrupted renders red like an error, done green,
working a spinner. A missing dot beats one saying these agents are running.
One worktree renders flat with no heading -- a name, count and chevron around a
single group says nothing the dialog has not already said.
The age column now uses formatShortTimeAgo for sidebar consistency. It takes
(timestamp, now) and subtracts internally rather than taking a delta, so the call
is (recordedAt, listedAt); passing the old delta would have rendered plausible
nonsense. The clock is still stamped once into state, so ages stay stable and the
render stays pure.
Continuation. A secondary "Reconnect and continue" action sends one message, from
a single shared constant, identical for both providers. Reconnect is unchanged and
still sends nothing. An info popover quotes the literal message read from that
same constant, so what is shown cannot drift from what is sent.
Ablation now covers fourteen guards. Two are new: continuation only follows a
reconnect that actually happened, and -- inversely -- a send injected into the
reconnect path must turn the test red, since "don't ask again" rests on reconnect
never sending.
* feat(native-chat): say terminal sessions kept running, and clear the quality gate
The modal lists stopped chats with no way to tell that CLI agents are fine, and
the true state of the world is counterintuitive: the terminal sessions survived
the restart and the chats did not. One line now says so, next to the heading
where it frames the list rather than as a footnote at the bottom.
Wording follows the app's own vocabulary rather than inventing a term: the
catalog settles on "terminal sessions" (terminalSessionCount, "Terminal sessions
are grouped by workspace", "No terminal sessions yet"), and UpdateCard already
reassures with "Your terminal sessions won't be interrupted during the update" in
the same text-xs text-muted-foreground treatment. "kept running" rather than
"were restored" -- nothing reconnected them, they never stopped, and the line
says nothing about why.
Also clears check:code-quality:changed, which I had not been running -- oxlint
alone covers neither the design-system nor the casting audit, so 18 findings had
accumulated across the branch.
- design system (4): Button spacing hand-rolled as gap-1/px-2 is just size="xs";
PopoverContent and DialogTitle own their typography and spacing, so the
text-xs moved to the popover's own children and the title's icon gap moved to
a plain wrapper.
- casting (14): production code loses its assertions outright via Reflect.get,
the idiom already used in managed-hook-detection-commands and
worktree-name-retirement. The marker validator reads each field through
Reflect.get and now checks recordedAt is a number rather than asserting it;
the store-file parse uses the existing `file` shape instead of a second
assertion; the runner narrows the admission error's owner with typeof.
Test fixtures keep their assertions behind the line-specific SAFETY:
rationale the repo mandates for exactly this case.
One trap worth recording: the audit reports an assertion at the line its
EXPRESSION OPENS, not where `as` appears, so a disable-next-line above the
closing brace of a multi-line literal is inert and silently changes nothing.
Guards unchanged; ablation re-proved 14/14 at this head.
* fix(native-chat): give the reconnect row's provider icon an accessible name
Every row rendered the provider as a bare AgentIcon, whose svg carries no
aria-label, title or alt. With a Claude chat and a Codex chat in one worktree the
two rows were identical to any non-visual consumer, and the dialog offered
several identically-named "Reconnect" buttons with nothing to tell them apart.
A regression from
|
||
|
Jinwoo Hong
|
09622f0c28 |
feat(relay): add a break-glass override for the same-cap monitor gate (#21270)
* feat(relay): add a break-glass override for the same-cap monitor gate Every mutating same-cap wave consumes a fresh 15-minute aggregate monitor dry-run. When a chronic fault is what the gate freezes on, waiting for a green window means waiting for the condition the wave removes: the gate froze 44 consecutive times on the recurring Cloud SQL stall the rolling image fixes. Add `gate-override-reason` and `gate-override-confirmation` (`SKIP_RELAY_MONITOR_GATE <target-image-digest>`) to the same-cap dispatch. A valid pair skips only the aggregate evidence download, provenance verification, and single-use marker. A partial or mismatched override fails closed before any mutation, in both the caller and the reusable job. Record the actor, reason, and confirmation in the gate run summary and, for a canary, in the sealed artifact. The live per-wave preflight still runs. Give it a `--no-monitor-state` source that takes the expected selector from the dispatch inputs and pins the migration policy to `strict`, rather than synthesising a state file that would claim a dry-run it never ran. Also give `director.instances` the two-consecutive-sample tolerance the cell probes have: Cloud Run replaces an instance in place, so the count leaves the [5, 6] band for one sample roughly twice a day, and a deploy overlap raises it the same way. Min and max share one streak so an alternating count still freezes. * fix(relay): canonicalise the break-glass preflight membership The override path parsed the operator's membership with a bare schema parse, while the live selector read from the director is normalised and the comparison is an ordered `JSON.stringify`. Unsorted dispatch input would therefore read as selector drift on a healthy fleet, and the every-configured-cell-exactly-once check was lost with it. Normalise through the same `normalizeSelectorMembership` call the monitor CLI uses when it seals evidence, against the same durable Terraform cell set. Tests use a collect stub that returns the director's canonical selector rather than echoing the expected one, so the ordering is actually exercised: unsorted input must canonicalise, and a duplicated, missing, or unknown cell must be rejected. |
||
|
Jinwoo Hong
|
0ed2771fa5 |
fix(relay-ops): tolerate a single unreadable monitor sample (#21272)
An unreadable sample (collector_failed) now gets the same two consecutive sample budget per source as an unread signal, so one failed Cloud Monitoring read no longer restarts the continuous window. monitor_gap keeps zero tolerance because it means the run itself stopped sampling. The pre-drain lineage cap moves from 25 to 35 minutes so a 15-minute window plus one restart still reaches a verdict, and the collector error message is now logged instead of being swallowed. |
||
  Jinjingandgum798
|
e6dcb8b938 |
fix(editor): keep preview Add-note controls out of PDF export (#21268)
Exporting Markdown to PDF from Preview printed an Add-note + button above every block. Preview exports the .markdown-body subtree, and its per-block annotation control renders inside that subtree, so the clone-scrub pass never removed it. Mark the controls container with data-orca-export-hide at the source, add the explicit class to UI_ONLY_SELECTORS (attr-strip fallback; generic attr covers renames), and hide it in EXPORT_CSS as a belt-and-suspenders backstop. Review note bodies and open composer drafts are transient review state and are intentionally excluded from the document PDF. Fixes #21198 / STA-7761 Attribution: diagnosis and core scrub entry by @gum798 (PR #21199, closed in favor of this PR) Co-authored-by: gum798 <33922655+gum798@users.noreply.github.com> |
||
|
Jinwoo Hong
|
7e2ebac318 |
chore(mobile): repin the RPC recording baseline to main after #21246 (#21266)
Every step-7 squash leaves the pin guard red on main until the baseline
names a commit main contains. Repin to
|
||
|
Jinwoo Hong
|
6142657d7a |
refactor(mobile): checked reply readers for the tasks domain's board, runtime, search and create (step 7) (#21246)
* test(mobile): record main's agent.launch create receipt before checking it `agent.launch` is the one read site in the tasks domain's project-board, runtime, source-search and workspace create/source files with no recording family at all, so main's answer to a malformed launch receipt was undocumented and a checked reader would have had nothing to move. One family, one scenario, two goldens: `worktree.agent-launch-create` drives `createWorktreeWithNameRetry` down the `agent.launch` arm instead of `worktree.create`, which needs an `agentLaunch` argument on the existing worktree-create-retry adapter. The agent is a constant there on purpose — which agent is picked changes only the params, and the arm under test is which method the create is issued on. A separate family rather than an eighth `worktree.create-retry` scenario: `familyGoldens` drives its reply matrix over the family's FIRST scenario, so adding to that family would have recorded a pilot golden and left the launch receipt with no partitions. As its own base it gets all eleven. Recorded from a detached worktree at the pinned baseline with this branch's `rpc-recording/` and manifest copied in, per the recipe in the recorder README: `mobile/pnpm-lock.yaml` has drifted past `4b876758d3` on main, so `--record` refuses on this branch's tree even though `mobile/src` and `src/shared` are byte-identical to the pin. Thirty-four existing goldens move on `adapterSha256` and nothing else — the six families mounted through the edited adapter module. No body moves. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * refactor(mobile): checked reply readers for the tasks domain's board, runtime, search and create Forty-three unchecked reply readers across five files become checked zod readers, so a malformed host reply surfaces as one readable error naming the method instead of a downstream TypeError, a rendered `undefined`, or a screen left ready over garbage. Deliberately a behaviour change on malformed replies only. Five schema modules, each recording the consumer line behind every requirement and the host handler it was checked against: - `task-project-board-reply-schema.ts` — the sixteen `github.project.*` envelopes. Where a consumer reads a member off BOTH arms unguarded the schema is a union on `ok`; where it guards everything (`result.error?.message ?? '…'`, `result.labels ?? []`) it is a flat passthrough and requires only the container, because a requirement on a member the consumer already defaults would refuse a reply main rendered. - `task-runtime-reply-schema.ts` — the hydration reads. The three preference writes read `z.unknown()`: no call site interprets their body. - `task-source-search-reply-schema.ts` — the provider searches and the pasted single-item lookups. The Linear union replaces the hand reader in linear-mobile-issue-read.ts, whose own copy reached the screen unattributed. - `workspace-source-reply-schema.ts` — SSH state, agent detection, orca.yaml hooks, sparse presets and base-ref search. - `workspace-create-reply-schema.ts` — the create receipt, the launch receipt and the hosted-base union. Requirements are exactly the members a consumer reads unguarded AND a recorded golden shows the host sending. That second half is load-bearing: the recorded GitHub search row is `{ number, title }`, the recorded Linear issue is `{ id }`, the recorded project is missing `id`/`url`/`source` and the recorded sparse preset is missing `repoId`/`createdAt`/`updatedAt` — requiring what the shared types declare would have dropped rows main renders. Where the value therefore stays looser than the screen's own state type, the call site keeps one narrowing cast with that reason on it rather than a default that would fabricate state. Two enum decisions, both pinned: - `ownerType` is CLOSED with no fallback. It is echoed into the next `github.project.listViews` params, and remote-wire-compatibility.md rule 4 forbids a reply-schema fallback from shaping a param; the host's own listing handler answers `validation_error` for any other value. - `ssh` `status` is OPEN and degrades to `disconnected`, main's own answer for a state it did not receive. The readiness gate is an equality test against `connected`, so an arm this build has not heard of can never grant a create, and the record survives with its Connect affordance. - Every other host vocabulary a consumer equality-tests — the project view `layout`, the `setupRunPolicy` — stays `z.string()` for the same rule. Tri-states are preserved, not collapsed: the row detail's `reviewDecision`, a work item's `author` and the SSH record's `error` each keep explicit `null` distinct from absent, with a unit pin on each. `blank-workspace-create.test.ts` splits one `it.each` in two. The two create routes now answer a workspace-less reply differently: `agent.launch` still reports "Failed to create workspace", because its reader guards `worktreeId` itself, while `worktree.create` is named as unreadable, because the create screen reads `result.worktree.id` unguarded into the session route. Both reach the same catch; only the sentence changes. `mobile-tasks-refactor-parity.test.ts` moves four hashes and no count. Hooks hold at 350 with 28 bodies edited and no dependency array moved; statements hold at 417 and declarations at 194; `semantics` loses exactly four lines, all four string literals that lived inside the one deleted inline cast type. No method literal and no `rpc:` call signature moves. The inventory loses its five tasks lines; the boundary test stays green. Goldens are refreshed in the next commit, which is where the disclosed behaviour change is proved. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): repin and re-record the corpus over the tasks domain's checked readers Repins `baseline` to |
||
|
Jinwoo Hong
|
c4917d6e74 |
fix(cloud): retry transient director admin failures in the relay monitor and preflight (#21263)
The director's /v1/admin/cell-status maps any thrown operation error onto HTTP 404, so a Cloud SQL pool connect timeout arrived at the ops tooling as "Relay admin telemetry returned 404" and killed the whole sample. Retry the admin reads that carry a transient database error, and let the live preflight spend one of its existing attempts on a thrown collector instead of failing the wave. |
||
|
Jinwoo Hong
|
1957437005 |
fix(relay): stop admin routes reporting a stalled database as 404 or 409 (#21264)
Every admin handler collapsed a thrown error into one status, so a two-second pool connect timeout answered POST /v1/admin/cell-status with 404. The rollout tooling never retries a 4xx, by design, so the wave failed on a database that was briefly out of reach and recovered on its own. Transient database failures now answer 503 with Retry-After, the shape the public routes and the region catalog already use. Every other error keeps the route's existing 404 or 409 mapping. |
||
|
Jinwoo Hong
|
8b2502fc92 |
fix(cloud): give same-cap waves ten minutes to consume gate evidence (#21259)
* fix(cloud): give same-cap waves ten minutes to consume gate evidence The live preflight rejected monitor evidence older than five minutes, but the same-cap job only reaches that step about five minutes after the monitor completes: runner queue, the gate job, and a full-branch checkout. On 2026-09-17 the first green gate in 44 attempts died at 302 s. The preflight still takes live samples, so the older baseline is safe. * docs(cloud): state the ten-minute preflight evidence bound |
||
|
Brennan Benson
|
7a1f55c52a |
fix(native-chat): give a failed Claude background task a typed row instead of an opcode (#20519)
* fix(native-chat): give a failed Claude background task a typed row instead of an opcode
A failed backgrounded command printed red rows whose visible text was the wire
opcode, and printed one failure twice. All five task lifecycle kinds are
catalogued status-chrome, but the payload sniffer in classifyProviderFrame runs
first and promotes any frame reporting a failure to the generic unknown-frame
fallback, whose sentence lookup has no key for the field Claude puts its own
sentence in. Two frames for one task therefore produced two rows, both of them
the method name.
Suppressing those frames is not the fix: when the last background task settles
the tracker flushes it and the strip unmounts, local_bash is excluded from the
subagent roster, and the status feed publishes only live tasks, so for a lone
backgrounded command the transcript row is the only report of the failure that
exists anywhere.
So the catalogue now binds: kinds a dedicated typed translator owns are named
as covered, and the generic fallback refuses to emit for them in either
direction. A new row owner keeps one durable row per task id, opened by the
announcement, revised in place by the lifecycle frames and closed by the
notification, carrying the provider's summary, error, output path, usage and a
run state. The row is written on the same dual carrier the subagent roster
uses: a frozen text twin plus a typed block, so a client without the block type
reads the sentence rather than nothing.
hasProviderError keeps its authority everywhere else, unchanged.
* fix(native-chat): keep tool attribution across a background-task row
A background task's row is a system message landing mid-turn between the
assistant's tool calls, exactly where the spawn-group roster row lands. Without
the same exemption it ended the run the following tool messages fold into, so a
tool result arriving after one stopped folding into its own assistant turn.
Also syncs the catalog with the row's one new translate key.
* fix(native-chat): harden background task rows
* fix(native-chat): settle background rows on provider end
* fix(native-chat): scope malformed task fallback text
* test(native-chat): assert only eligibility at the disposition layer
The malformed-task-frame test asserted the generic fallback resolves Claude's
`summary` field itself, which was true only while that key sat in the shared
key list. Eligibility is what this layer decides; the sentence the row leads
with is Claude's, supplied through the display-text seam and proven in the
translation test.
* fix(native-chat): gate background-task admission and scope rows per run
Admission now matches the reference on all three gates. Type is the whole gate
and MONITORS ARE NOT ADMITTED: a monitor runs for the life of the session and
has no outcome a row could report, so it never reaches the timeline. On first
admission only, the task's tool_use_id must name a tool call this session
forwarded at the TOP level — a Task spawned inside a subagent's sidechain names
an id that never reached the transcript, and a top-level row for it would claim
an invocation the user never saw. And a task that already exists and has not
finished is not re-opened: a duplicate announcement is a redelivery, not a
second run.
Rows are now keyed per RUN. A provider may reuse a task id for a distinct later
invocation, and a row keyed by the id alone overwrote the first run's transcript
history instead of leaving it standing. Generation 1 keeps the bare key, so
every row already written is unaffected.
The spawning tool call is carried on the row as parentToolUseId. Orca's journal
has no structural parent link for an item — AgentJournalItemIdentity has four
arms and none carries one — so the relationship is data on the item rather than
nesting.
A terminal frame that names NO tool still opens a row. That is a named
deviation, recorded at its call site, and the measurement behind it is in the PR.
* fix(native-chat): read the aggregate roster by membership, not a phantom status
The background-tasks payload types every entry as exactly
{task_id, task_type, description, ambient?}. It has no per-entry status, so the
state this owner derived from one was always undefined and the reopen branch it
guarded was unreachable on every real payload — proven by deriving the state
from an SDK-shaped entry and getting null.
Membership is the only liveness the payload carries: it is the whole live set
after a change, so presence means live and absence means merely "no longer
listed", never an outcome. Presence does not revive a settled row either — the
level's ordering against the start/stop edges is unspecified and it carries no
evidence of a new run, so the task's own frames stay the only thing that opens
or settles one. Only the identity fields it really sends are read, and ambient
housekeeping entries are excluded as the payload asks.
The two helpers that served the dead branch are removed, along with the test
that exercised it through a synthetic status the CLI cannot send.
* fix(native-chat): mirror reference task admission and drop the synthesis path
The forwarded-parent gate is conditional on the field being PRESENT. An
announcement naming a tool this session never forwarded is a nested child and is
refused; one naming no tool at all is admitted, because absence of the field is
not evidence of an unforwarded parent. The previous rule required the field and
so refused every tool-less task.
Terminal frames now match on task_id alone. The forwarded-parent question is
settled once, at admission, and is never re-asked on a notification or a patch.
A frame for a task that was never admitted yields no row, and a patch is folded
into the row it names rather than opening one.
That removes the synthesized-row path entirely, and with it the named deviation
it carried: the captured tool-less failure lands on a row that already exists,
because its own tool-less announcement is admitted. The dead builders go with
it.
Left deliberately stricter than the reference, and flagged rather than changed:
a terminal frame still records its task id as terminal even for a task never
admitted, so a late announcement cannot open a row for work already reported
finished. Two existing tests pin that.
* fix(native-chat): preserve background task ownership across restarts
* chore: restore pnpm-lock.yaml to origin/main
A local pnpm run rewrote the lockfile and the merge commit swept it in. The
branch changes no dependencies, so it must carry no lockfile delta at all.
* fix(claude): harden background task lifecycle
* fix(claude): bound task generation history
* fix(claude): preserve task identity after history eviction
* fix(claude): resolve background task identity after rebind
* fix(claude): isolate queued task runs
* refactor(claude): give the background-task ledgers one bounded owner
The bounded collections behind a background-task row were read out of the
class with `Reflect.get` to prove they stay capped, which the anti-slop
gate rejects. Move them into `ClaudeBackgroundTaskLedgers`, which owns
the caps beside the eviction helpers and reports a typed readonly size
view the tests assert against.
Also replace a `Reflect.get` in the mobile recording proxy with typed
property access.
* fix(native-chat): report a background task failure the transcript never admitted
A terminal `task_notification` for a task no announcement ever admitted rendered
nothing at all. The typed row owner declined the row because its map held no
entry for the id, and reported the frame as handled — which is exactly what
tells the generic provider-frame fallback to stay quiet. Both surfaces declined
the same frame, so a real failed background task was dropped on the floor.
A terminal frame is self-sufficient: it states an outcome, and it carries the
summary, status, error, output path and usage that outcome needs. It now opens
its own row from those fields, with the summary as the label and `unknown` as
the kind when the frame names no task type. The row map enriches a terminal
frame; it never gates one. Every terminal status writes one, not failures alone,
so there is one rule here rather than a third behaviour for failures.
The deliberate hand-offs still win, because they are recorded rather than
implied: ambient, subagent and foreground tasks are claimed in the foreign-owner
ledger the notification path already checks first. A Task spawned inside a
subagent's sidechain now records its refusal there too, under `sidechain`,
instead of leaving no trace and reading as a task nothing ever decided about. A
capacity-refused task whose outcome the generic fallback already printed records
`fallback` the same way, so a redelivery neither prints twice nor mints the row
capacity refused.
The anti-resurrection guard stays and stays scoped to announcements: a late
`task_started` cannot reopen work already reported finished. The restart rule is
now stated once instead of twice — a different parent alias is the provider's
restart signal only when BOTH runs name their parent, which is what the terminal
ledger already required of an evicted row and what the live row now requires too.
* test(native-chat): pin that an orphan task row reopens the provider's turn
* fix(native-chat): stop an orphan task row drawing its sentence twice
An orphan row took its header label from the notification's summary, which
also renders as the row's sentence, so the same string appeared in both slots
of the same collapsed row. The label now stays empty and the header falls back
to the task kind, leaving the sentence to carry the provider's words.
* fix(native-chat): keep Claude task outcomes owned through capacity and redelivery
* fix(native-chat): keep settled overflow notifications from reopening a turn
* fix(native-chat): retain Claude task rows through journal pressure
|
||
|
Brennan Benson
|
abc8386e14 |
fix(mobile): name a create's launch so a lost reply cannot build two workspaces (#21137)
* fix(mobile): name a create's launch so a lost reply cannot build two workspaces `agent.launch` admits a caller-supplied `operationId` through a durable ledger, so exactly one execution happens and every replay returns the recorded answer. No client sent one, so the machinery was inert and the original defect was still live: mobile retries a lost create by design, and a retried launch built a second agent in a second workspace. Mobile now mints an operation id per create candidate and sends it whenever the host advertises `agent.launch.replay.v1`. The invariant is one operation per candidate. `computeAgentLaunchFingerprint` folds `target` whole, so the workspace name is inside the fingerprint; carrying one id across a name-collision bump would meet its own row under a differing fingerprint and refuse `agent_session_operation_conflict`, failing the create outright on the second candidate. The id is therefore minted beside `clientMutationId` at the top of each loop iteration and reused verbatim by every retry arm inside that candidate — never re-minted, since a new id is a new operation. Admission runs ahead of every effect, so `_invalid` / `_expired` / `_capacity` prove nothing launched: those re-send the same candidate unnamed rather than let bookkeeping fail a create the host would have performed. `_unknown` is the one refusal that is not safe to re-send, and it surfaces. Also corrects a false comment: the legacy path caches the whole launch under `clientMutationId`, so inside its 60s window a replay adds neither a workspace nor a surface, and outside it adds both — not "a second surface, never a second workspace". * fix(mobile): preserve launch identity on refusals * fix(mobile): use launch receipts to authorize replay * test: move mobile launch replay coverage outside node project * fix(mobile): enforce replay-safe launch delivery at the host * test: run mobile launch contracts in mobile checks * test: cover mobile launch contract workflow dependencies |
||
|
Jinwoo Hong
|
6b426a8623 |
test(mobile): repin the RPC recording corpus to main after #21176 (#21254)
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb |
||
|
Jinwoo Hong
|
3e32b83522 |
refactor(mobile): checked reply readers for notifications, components, terminal, transport, home, worktree and browser (step 7) (#21176)
* refactor(mobile): checked reply readers for notifications, components, terminal, transport, home, worktree and browser (step 7)
Twenty-one unchecked reply readers across thirteen 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 card left "proven" over a reply that carried no rows. Deliberately a behaviour
change on malformed replies only.
What each domain required, and why it required no more:
- notifications (5 readers). All four call sites read the payload through `?.`,
so every schema is nullish at the top level and no member is required. The
test-push `reason` and the register `reason` become closed enums, because the
two comparisons against them are the whole of what they decide and an arm this
build does not know took the generic copy on main too. The stream unsubscribe
and the unregister read no body at all.
- components (4). `repo.hooks` requires `source` and nothing else: the drawer
assigns it straight into `SetupHookDetails.source`, whose type is
`string | null`, with no guard in between — nullable so the "no hooks file"
answer keeps its explicit null. `setupTrust` is nullable as well as optional
because the `components-setup-ask` fixture sends an explicit null, and
salvaging that would move a `normal` golden. `ui.get`'s trust record salvages
per repo, so one unreadable repo cannot cost the others their approvals. The
Codex redeem reply stays `z.unknown()`: `decodeResetResult` is a real
scope-and-snapshot validator and splitting it would give one reply two refusal
rules.
- terminal (4). The send verdict and the viewport pair keep main's exact
`=== true` projections. `terminalSendAcceptedSchema` moves here from the
session domain, which now re-exports it: terminal is the lower layer and two
identical copies could drift on what "delivered" means.
`terminal-send-rpc-response.ts` is deleted, its projection now being the
schema's.
- transport (3). `status.get` declares its five members and requires the object;
the three callers disagree about what an unreadable status means, so each keeps
its own verdict behind a named reader — the gate wants the failure, and the
probe and the pairing race must not have it, because both call `interpret`
inside a `.then` fulfilment handler where a throw becomes a detached rejection.
`capabilities` salvages whole rather than per element, which is main's own rule
and what `transport-capability-probe-non-string-capabilities-drop` records.
The two pairing readers are the shared credential contract itself, moved off
the four call sites that each ran `.parse()` on the interpreted value; its
`.strict()` is main's shipped rule for that released surface, not a new one.
- home (2), worktree (2), browser (1). The stats row is checked as an object and
nothing more, `totalHomeStats` being the reader that says so itself; its
per-host slot is now typed as the wire row it holds rather than as the computed
total. `worktree.ps` cannot require `worktrees`: the host answers a union whose
unchanged arm carries `{ unchanged, snapshotId }` and no rows. The twelve
browser commands read no body; `browser.goto`'s settled URL stays nullish
because `navigateToAddress` is inline in `MobileBrowserPane.tsx`, which no
adapter mounts, and a move there would ship unevidenced.
Three fixtures were wrong and are corrected, each disclosed rather than worked
around: the runtime-context test kept a content hash directly under a repo key,
which is not a shape `ui.get` sends; and two snapshot-client tests ran their
reply list dry and handed `fetch` an absent result while claiming to model a
transport failure.
`push-test-envelope` is re-anchored at the same defect's new home, the cast
having been deleted. The boundary test's offender floor comes down from 20 to 10
with the list, which is what its own comment says it is for.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): repin the corpus and re-record step 7's checked reply readers
`baseline` moves to this branch's product commit, which is what `--record`
compares the fenced tree against, and every one of the 758 goldens is
re-recorded from it. The repin is what rewrites the `baseline` header on all of
them; nothing else about the corpus moves except the bodies disclosed below.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): mutate the workspace catalog's reader back to unchecked
The step-7 defect evidence needs a scenario whose reply is the one the change
moves. Every pilot scenario in the catalog family scripts a well-formed reply, so
a mutant that only changes how a *malformed* reply reads has nowhere to diverge —
which is why the pilot's own suite passed against an unchecked catalog reader
while its matrix golden failed.
`worktree-catalog-snapshot-unreadable` scripts `worktree.ps` answering
`{ ok: true }` with no result at all, which is what `result-absent` drives at the
matrix site, and records the fetch rejecting with `RpcIncompatibleReplyError`.
`worktree-catalog-unchecked-reader` then swaps the operation's reader for one that
answers `compatible: true` for every payload — main's reader, in one line — and
the recording moves back to a fulfilled fetch carrying
`admission: { kind: 'invalid' }`, which is the answer that let a broken catalog
render as an empty host (STA-3123).
One golden added and none moved: the manifest sits outside the fenced paths, the
family's matrix base is still `worktree-catalog-snapshot`, and the mutation
registry is not part of `recorderSha256`.
Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
* test(mobile): pin the push-test reason arms the closed enum constrains
`pushDeliveryTestResultSchema.reason` closes over the four arms of the host's
`MobilePushTestResult` (src/shared/mobile-push-contract.ts:99), but no scenario
carried the member, so the corpus could not have caught a wrong vocabulary.
Three scenarios on the existing display-test mount carry it now: the two arms
the screen branches on and one arm no build knows.
Each golden was recorded first at the main pin
|
||
|
Jinwoo Hong
|
a3046cd27b |
fix(relay): treat database pool connect failures as transient, not director faults (#21243)
* fix(relay): treat pool connect failures as transient, not director faults pg-pool raises connection-acquire failures as a plain Error with no SQLSTATE, so the transient classifier matched only one of the three messages it can produce. The other two reached the routes unclassified and became HTTP 500s, which is what the rollout safety gate counts. The acquire boundary now marks the errors it produces, so "Connection terminated unexpectedly" counts as transient when the socket died during the handshake and stays a hard failure mid-statement, where a retry could repeat a commit whose outcome is unknown. /v1/regions and /v1/admin/evacuation-status gain the transient handling /v1/assign and /v1/resolve already had. * fix(relay): mirror the pool-connect verdict in failure diagnostics The query-failure event's connectionTimeout boolean matched one of the two messages connectionTimeoutMillis can produce, so the 210 dialling timeouts in the last day logged as false and were invisible to the field meant to find them. The pool-connect vocabulary now lives beside the acquire boundary that owns it, and both the router's classifier and the diagnostics read it from there, so the two cannot drift. The event also carries the routing verdict the caller already computed, making "how much of this burst reached users as a 500" one field. * fix(relay): null-safe transient classification and honest transient docs The classifier now runs inside the query catch, where a thrown null or undefined would have turned a database failure into a TypeError that buried it. The diagnostics doc claimed transient maps to a 503 or a 500. Sweeps, startup reconciliation, and admin routes that answer 409 all emit the same event, so counting the false ones over-states user-facing hard failures. |
||
|
Jinwoo Hong
|
7184b1dc5b |
fix(relay-ops): recalibrate the pre-roll monitor gate to chronic production baselines (#21241)
* fix(relay-ops): let the pre-roll gate ride out chronic production noise The 15-minute pre-drain dry-run froze 39 times out of 39 on conditions that have nothing to do with the roll it gates: - A cell probe is one HTTP round trip from one runner. When the Asia cells' readiness SQL probe times out behind a saturated pool, the load balancer answers "no healthy upstream" for ~30 s and the gate froze on a single sample. Cell probe signals now need more than cellProbeToleranceSamples consecutive failing samples to freeze; absorbed blips are recorded in the state artifact. Director and auth probes keep zero tolerance. - directorErrors 3 -> 15. Measured non-503 5xx per rolling five minutes over the 24 h to 2026-09-17: p90 3 / p95 5 / p99 9 / max 52. The old bar sat on the p90 and froze 29% of gates. - cloudSqlBackends 250 -> 320. Measured latest-sum over the same 24 h: p95 212 / p99 262 / max 282. The old bar sat under the observed peak and froze 22% of gates. Failure codes are unchanged so downstream matchers keep working, and the trusted evidence scripts are untouched. * fix(relay-ops): key probe tolerance by cell and extend it to live preflight Three review findings on the cell-probe tolerance: - The streak was keyed per signal, so a cell alternating between slow (latency over bar) and down (health/ready 0) held every individual streak at one and never reached the tolerance. A continuously unhealthy cell passed the gate. The streak is now keyed by cell id, so one cell's health, ready and latency readings share it. - The live preflight runs one sample before every mutating wave and retried only on freshness codes, so the same Asia blip could still fail a wave there. It now re-samples per-cell probe breaches on the same tolerance, spaced the existing interval. Director and auth probes still fail the wave on the first bad sample, as does any non-probe threshold. - docs/relay-incident-monitor.md still stated the old bars. Updated the threshold table, the 400-connection ceiling text, and the superseded 2026-08-26 and 2026-09-12 entries, and added a dated 2026-09-17 recalibration entry. Also pins the resumed-state case: a state file carrying a full streak now has a test proving it freezes on the next bad sample. Trusted evidence scripts remain untouched. |
||
 github-actions[bot]
|
0d23ea6e68 | Update README downloads badge | ||
  Shahar MorandNeil
|
de15227a1d |
feat(terminal): search match count + Cmd+F focus parity (#9035)
* feat(terminal): show search match count and keep Cmd+F from closing search Bring the terminal search bar to parity with the editor find bars: - Show a live match indicator (0/0, current/total, "No results", or <count>+ past the highlight limit) driven by the xterm SearchAddon onDidChangeResults event. - A repeat Cmd+F while the search is open now re-focuses and selects the query instead of toggling the panel closed; Esc remains the close path. Adds unit coverage for the indicator states and the toggle decision. * Use auto-generated localization key for TerminalSearch no-results (#9035) Replace the hand-written "noResults" i18n key with the SHA1-based auto key (auto.components.TerminalSearch.10e039b591) to match the repo's auto-keying convention, and sync the key across all locale catalogs. Addresses CodeRabbit review feedback. * test(terminal): cover search dispatch after keyboard module split * fix(terminal): refocus search from its input and verify real matches * test(terminal): use portable echo commands for search proof * refactor(terminal): keep search subscription and cleanup together --------- Co-authored-by: Neil <neil@stably.ai> |
||
 Neil
|
96eb97aad6 |
fix(runtime): split the host-contact epoch out of the connection generation (#20359)
* fix(runtime): split the host-contact epoch out of the connection generation `connectionGeneration` carried two meanings and one reader was always wrong. holding the session mirror through an outage leaves its subscriptions stranded and that edge was the only thing left to revive them. But the same value is the mirror's cache key -- use-runtime-session-mirror-environment-key.ts keys the subscription effect on it, every published frame is stamped with it, and web-session-terminal-retirement-proof-ledger.ts drops retained proofs when it moves. So the bump #20085 needed as a resubscribe signal re-keyed and rebuilt the mirror after any brief flap, which is the #19647 symptom #19873/#20059 fix. Measured first: with the reconnect bump deleted, an ended stream followed by recovery issues zero resubscribes, and the mirror's subscribe call registers no `onClose`, so main's terminal close is dropped. #20085's claim is true -- the subscription really is dead after recovery -- so the trigger has to exist. It just must not be the cache key. Give each meaning its own value: - `connectionGeneration` returns to identity only: a new runtime session, a re-pair, an explicit clear. A same-runtime return no longer moves it, so no stamp, fence or retained proof is invalidated by a flap. - `hostContactEpoch` counts "the host answered again after we lost contact". It lives on the store entry and is read only as a dependency of the two subscription effects in use-web-session-tabs-sync.ts -- never passed to an installer, never part of `environmentKey`, so it cannot become a stamp. `useRuntimeSessionMirrorEnvironmentKey` becomes `useRuntimeSessionMirrorEnvironmentKeys`, returning `environmentKey` (identity) and `resubscribeSignal` (the epoch edge) from the one target scan, so the hot ownership scan is not doubled. Each direction is pinned by its own test: removing the resubscribe dependency fails only 'reinstalls both session-tabs subscriptions when the host answers again'; restoring the reconnect bump fails only the two key-stability tests. * test(runtime): pin the mirror hydration verdict across a host flap The generation tests assert the key string; this asserts what the user feels. The mirror's hydration verdict is stamped with the connection generation, so any bump discards it and every mirrored pane re-parks -- the tab-list rebuild. Held across an unverifiable probe, still discarded when the runtime id actually moved. * test(runtime): build real host statuses instead of casting partials |
||
|
Neil
|
851befa929 |
fix(runtime): hold parking and transport reads through an unverifiable probe (#20096)
* fix(runtime): hold parking and transport reads through an unverifiable probe Three remaining sites where a non-verified status probe was read as evidence the host is gone, per docs/reference/ssh-execution-boundary.md. - runtime-status-refresh published its own copy of the "null the status unless verified" rule, then handed the snapshot to applyRuntimeHostStatusSnapshot, which re-derived it. The copy was dead but free to drift; the snapshot branch now calls applyRuntimeHostStatusSnapshot directly, leaving one implementation. - The paired-parking capability reads treated a nulled status as "host cannot park", so a transient probe failure unparked live paired terminals and dropped a parked session's reattach in favour of a fresh cold restore. Both now read lastVerifiedRuntimeStatus; a capability is a fact about the host's build. - runtimeHostConnectionStateForEntry handled transport 'disconnected' and 'ready' and let 'connecting'/'unknown' fall through to the default 'disconnected' — reporting a host mid-handshake as down, a worse verdict than an actually disconnected transport gets. It now passes the snapshot's transport through. * fix(runtime): keep a revoked host out of the parking promise Holding a capability through an unverifiable probe is right; holding it through the host's own refusal is not. `blocked` (auth rejected, protocol mismatch) stops every retry for good, and parking trades the client's only copy of the scrollback for a host-side restore that can then never happen -- the destructive direction. `isRuntimeHostContactRevoked` names that one terminal verdict once, and the connection-state derivation now reads it too so there is a single definition. Also narrows the transport hint to 'connecting'. 'unknown' means no transport was ever attempted, which is the permanent state of an unreachable paired host: as 'checking' its row lost its Connect action and the status bar read "connecting" for the whole session. * test(runtime): pin the parking gate against over-firing on a flap |
||
|
Neil
|
182ff17141 |
fix(runtime): hold four more host reads through an unverifiable probe (#20095)
* fix(runtime): hold the session mirror through an unverifiable probe Two derivations read the same host state and reached opposite verdicts, and the destructive one won. When a status probe came back unverifiable over a still-ready transport, runtimeHostConnectionStateForEntry called the host 'runtime-unavailable' (connected) while getReachableRuntimeSessionMirrorTargets dropped it, tearing down and cold-rebuilding the session-tab mirror while the host's flows were still delivering. The root cause is that applyRuntimeHostStatusSnapshot nulls entry.status for any non-verified probe while the snapshot retains the runtime identity. The connection-state reader consults the snapshot; the mirror-target reader did not. Give both readers one answer: - lastVerifiedRuntimeStatus() in shared/runtime-host-status.ts is now the single definition of "the last identity the host answered with". runtime-status.ts already had this inline as previousVerifiedStatus and now calls it. - The mirror-target reader asks the shared connection verdict instead of entry.status, gated on isDisconnectedRuntimeHostState -- only the one exit verdict earns a destructive read, per docs/reference/ssh-execution-boundary.md. 'checking' and 'reconnecting' are unverifiable, not evidence of an exit. Holding the target through the outage would strand the mirror on its own: the subscription is installed by the effect in use-web-session-tabs-sync.ts keyed on useRuntimeSessionMirrorEnvironmentKey(), a stream 'end' frame is dropped without resubscribing, and the parking layer retries only a rejected subscribe call. The teardown was the recovery. So regaining contact now advances the connection epoch, giving recovery its own "the host is back" trigger rather than leaving the mirror to be restored as a side effect of having been destroyed. The connection epoch is not the runtime session: a same-runtime return fires no restart hook, no provider session bump, and no toast. * test(runtime): drop the redundant status casts the new casting gate rejects * fix(runtime): hold four more host reads through an unverifiable probe Siblings of the session-mirror defect fixed in #20085. runtime-status-snapshot nulls `entry.status` for any non-verified probe while the snapshot retains the host's identity, so a host with a ready transport that is still delivering reads as gone to anything gating on `entry.status != null`. - client-event subscription selection dropped the stream for such a host, and its disconnect edge bumped the SSH generation, rebuilding even the active host's subscription - the web client's active session-tabs stream tore down and cold-rebuilt, twice per blip - landing preflight discarded its whole result - runtime-aware SSH selectors blanked mirrored target rows Each now reads the shared verdict, isConnectedRuntimeHostState of runtimeHostConnectionStateForEntry, or lastVerifiedRuntimeStatus where the read is host identity rather than reachability. No new predicate: every "genuinely gone" case is byte-identical, so nothing gains a retry loop. * test(runtime): build real host statuses instead of casting partials |
||
|
Neil
|
67dda9affe |
fix(runtime): hold the session mirror through an unverifiable probe (#20085)
* fix(runtime): hold the session mirror through an unverifiable probe Two derivations read the same host state and reached opposite verdicts, and the destructive one won. When a status probe came back unverifiable over a still-ready transport, runtimeHostConnectionStateForEntry called the host 'runtime-unavailable' (connected) while getReachableRuntimeSessionMirrorTargets dropped it, tearing down and cold-rebuilding the session-tab mirror while the host's flows were still delivering. The root cause is that applyRuntimeHostStatusSnapshot nulls entry.status for any non-verified probe while the snapshot retains the runtime identity. The connection-state reader consults the snapshot; the mirror-target reader did not. Give both readers one answer: - lastVerifiedRuntimeStatus() in shared/runtime-host-status.ts is now the single definition of "the last identity the host answered with". runtime-status.ts already had this inline as previousVerifiedStatus and now calls it. - The mirror-target reader asks the shared connection verdict instead of entry.status, gated on isDisconnectedRuntimeHostState -- only the one exit verdict earns a destructive read, per docs/reference/ssh-execution-boundary.md. 'checking' and 'reconnecting' are unverifiable, not evidence of an exit. Holding the target through the outage would strand the mirror on its own: the subscription is installed by the effect in use-web-session-tabs-sync.ts keyed on useRuntimeSessionMirrorEnvironmentKey(), a stream 'end' frame is dropped without resubscribing, and the parking layer retries only a rejected subscribe call. The teardown was the recovery. So regaining contact now advances the connection epoch, giving recovery its own "the host is back" trigger rather than leaving the mirror to be restored as a side effect of having been destroyed. The connection epoch is not the runtime session: a same-runtime return fires no restart hook, no provider session bump, and no toast. * test(runtime): drop the redundant status casts the new casting gate rejects |
||
|
Jinjing
|
25dd70e611 |
Test: target question card title by testid instead of text (#21153)
* test: target question card title by testid instead of text Add data-testid to NativeChatQuestionCard's title element and update the e2e test to query by testid with a text filter. The transcript row also renders the question text, so the previous test could match either location, causing flaky results. Gating on the card's own title node ensures the assertion verifies the card is actually rendered. Fixes #20724 * test(browser-history): budget the fastest sample, not p95 The prepare/match budget assertions measure wall clock inside a vitest worker that shares cores with the rest of the shard, so a slow sample records a preemption rather than the matcher. CI shard 6/8 measured a p95 of 3.57 ms against the 2 ms ceiling while the same test passes in isolation; #18788 already records this file failing the same way. Assert the fastest sample instead, matching the estimator the palette matcher budget already uses for the same reason. Ceilings stay at 2 ms. Measured locally (20 samples per batch): the fastest sample moved 0.05 ms -> 0.08 ms between idle and a 3,374-file parallel run, while p95 of those same batches swung 0.09 ms -> 0.50 ms. * test: target question card title by testid instead of text Add data-testid to NativeChatQuestionCard's title element and update the e2e test to query by testid with a text filter. The transcript row also renders the question text, so the previous test could match either location, causing flaky results. Gating on the card's own title node ensures the assertion verifies the card is actually rendered. Fixes #20724 |
||
|
Neil
|
779667c1e7 | refactor(runtime): declare the host-status entry once instead of per consumer (#20262) | ||
|
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. |