Skip to content

fix(remote): stop a disclosure list latching the mirror completeness gate - #18619

Merged
nwparker merged 3 commits into
mainfrom
nwparker/fix-18595-runtime-hostscope
Sep 4, 2026
Merged

nwparker merged 3 commits into
mainfrom
nwparker/fix-18595-runtime-hostscope

Conversation

@nwparker

@nwparker nwparker commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​330 $\color{#cf222e}{\Huge{\mathbf{−}}}$​6 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​324
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​40 $\color{#cf222e}{\Huge{\mathbf{−}}}$​7 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​33

Fixes #18595.

Replaces an earlier version of this PR that filtered runtime: hosts out of listKnownExecutionHostIds. That approach was sound but did not reach the reporter's own configuration — their peer runtime is named in omittedHostIds by a mirrored row, not by a session partition, so the filter left it in place. The branch was reset to base and rebuilt.

Root cause: one field, two jobs, opposite requirements

hostScope.omittedHostIds serves two purposes that pull in opposite directions.

As disclosure, it must over-name. src/cli/omitted-host-scope-selectors.ts:33-42 deliberately keeps ids for servers that are no longer paired, because docs/reference/ssh-execution-boundary.md requires a listing to name its gaps, and an agent completing a partial listing needs to see them.

As a completeness gate, it must name only coverage that was owed and not delivered — otherwise it latches.

It latched. Two sources put a peer runtime in that list permanently:

  • workspaceSessionsByHostId keeps a partition for every runtime a machine has ever paired with, and nothing prunes it (session-host-partitions.ts:70-80).
  • A mirrored remote:<env>@@handle row contributes terminal.executionHostId (orca-runtime-get-runtime-id.ts:133-137).

Neither can ever be covered, because a runtime: host can never enter queriedHostIds — there is no paired-runtime PTY provider (src/main/ipc/pty/provider/registry.ts:18-23, src/main/ipc/pty/runtime/inventory-operations.ts:24-52). So any client that has ever paired outward publishes a permanently non-empty omittedHostIds, probeHostLiveTerminals returns unverifiable forever, markHostSessionMirrorHydrated never fires, and panes parked on parkUntilHostSessionMirrorHydrates never drain. Blank remote tabs.

The change

hostScopeCensusIsComplete() in src/shared/runtime-listing-host-scope.ts gives the gate its own answer and leaves the disclosure list untouched. Three branches, all load-bearing:

  1. scope === undefined → not complete. A host too old to publish a scope cannot claim one; absence of the claim is never the claim. This stays first.
  2. hostIds.length === 0 → not complete. A listing that answered for no host proves nothing.
  3. hostIds carries no id parseExecutionHostId can read → not complete. An unreadable coverage claim is not a claim.
  4. Otherwise, complete iff every omitted host is kind === 'runtime'.

Branch 3 is the fix. A paired runtime is a peer with its own control plane, reached with --environment; this runtime never queried it and never owed it coverage, so its presence in the list is disclosure rather than a gap. This mirrors a rule the host already applies to itself — isCompleteSessionTabsPtyCensus discounts runtime kinds identically (orca-runtime-collect-mobile-visible-graph-changed-worktrees.ts:188-197).

Applied at the two gates: host-live-terminal-probe.ts and web-session-terminal-orphan-recovery-inventory.ts. Deliberately not applied at worktree-live-terminal-surface-owners.ts:118 — that is a worktree-scoped listing where every other host including ssh: is omitted by design, so the predicate would null its owner index permanently on any machine with an SSH target.

Branch 2 is defence in depth, not a live path: listKnownExecutionHostIds always seeds local, so a real scope with hostIds: [] also omits local and is refused by branch 3 anyway. It exists so a scope that covered nothing can never read complete if that stops holding.

Review finding: covered ids were trusted without being read

CodeRabbit found a real asymmetry introduced by the first version of this gate. The predicate refused an omitted id it could not parse, but accepted an unparseable covered id as proof of coverage — isTerminalListResult validates only that hostIds is an array. So {hostIds: ['runtime:'], omittedHostIds: ['runtime:env-7']} was unverifiable before this PR and would have become complete after it. Narrow, but a new exposure created by the fix.

Taken as at least one legible covered host, not the suggested "every id parses". The strict form is a forward-compatibility trap: a host that later gains a host kind this client cannot parse would report an incomplete census forever — which is this bug in a new coat, and exactly the failure mode the predicate exists to prevent. A test pins that distinction.

Why four pre-existing tests changed

Adding the check turned four tests red, and they are corrected rather than accommodated. They published hostScope: { hostIds: ['remote-runtime'] } — a bare environment id that parseExecutionHostId rejects.

No host emits that shape. A runtime answering terminal.list names the execution hosts it covered, which is local; verified against a live paired runtime (see the hardware section above, where the real wire answer is hostIds: ["local"]). The fixtures asserted a wire shape that does not exist, which is why the assertions move.

"I changed pre-existing tests so my change would pass" deserves scrutiny, so the reasoning is stated here rather than left implicit. Two of the four resolved from one line in the shared fixture (web-session-terminal-orphan-recovery-regression-fixtures.ts); the other two were inline literals in web-session-terminal-pending-handle-recovery.test.ts.

Wire compatibility: no wire change

Rules 1, 2 and 3 of docs/reference/remote-wire-compatibility.md are not engaged. No src/main production file is touched; the host publishes byte-identical content and only the client's reading moves.

behaviour
new client + pre-v1.4.187 host (no hostScope) unchanged — probe skips the gate on hostScope &&, recovery reads it as unverifiable
new client + v1.4.187+ host fixed
old client + any host unchanged, still latches

Being client-side is the point: it reaches the reporter by updating their M1 alone, without waiting for their remote to update. Mixed versions are the normal state. It also avoids a new field's fallback rule, where "absent means complete" would recreate this bug with the polarity flipped.

Hardware verification, on the reporter's shape

The real headless runtime (out/orcad/orcad.js) built from this branch, seeded with the reporter's own environment id from the issue, paired to the real CLI over the real RPC path:

LIVE terminal.list hostScope:
  {"hostIds":["local"],
   "omittedHostIds":["runtime:77ab217c-d04f-41bc-919d-709185f60308"],
   "omittedHostSelectors":[{"hostId":"runtime:77ab217c-...","selector":null}]}

That is the shape reported in the issue. Feeding that live wire answer to the real probeHostLiveTerminals from each tree — the host output is identical either side, so this isolates exactly what moved:

BASE  637dc30a321: unverifiable  -> mirror never settles, panes park
SPLIT this branch: none          -> mirror settles

Also measured: this defect does not require a stale pairing or an aged profile. A brand-new client profile has its paired runtimes in workspaceSessionsByHostId after the first connect, so it fires on first use.

Version check on the reporter's host

Worth recording, because it decides whether this reaches them. hostScope on terminal.list — the field this gate reads — shipped in v1.4.187; the reporter runs 1.4.197, and a 1.4.195 remote here publishes it. Confirmed on hardware.

hostScope on worktree.list is a different, unreleased path (#18417, added 2026-09-03, in no release tag). A 1.4.195 remote correctly omits it, which is why orca worktree list --environment <remote> returns no hostScope today. That is not a defect and not what the gate reads.

Testing

test with the fix removed
reads a peer runtime as disclosure, not as coverage this host owed FAILS
settles on a peer runtime it disclosed but never owed coverage for FAILS
reports a live terminal through a peer-runtime disclosure FAILS
prunes a twice-absent binding when the only omission is a peer runtime FAILS (gate 2)
refuses a listing that covered no host at all FAILS (branch 2 only)
SSH-gap, covered-nothing and absent-scope cases at both gates guards

Reverting each branch independently: branch 3 → 3 failures; branch 2 → 1 failure; gate 2 alone → 1 failure. That last one matters: a readiness review caught that the first version of this PR left gate 2 unpinned — reverting it alone kept the entire renderer suite green, because every pre-existing fixture passes omittedHostIds: []. The commit claimed two gates and proved one.

One fixture trap worth naming: listResult substitutes its default scope when handed undefined, so the absent-scope case deletes the key instead. Routing through the fixture there silently tests the default.

  • pnpm tc clean
  • src/renderer + src/cli + src/shared — 4273 files, 37279 passed
  • src/main — 3093 passed; 5 failures under full-suite load (node-pty-master-fd-retirement, ssh-remote-commands) that pass 26/26 in isolation, plus structured-tui-transcript-catchup which is known-broken on main. None touch this change.
  • oxlint exit 0 on all changed files, oxfmt applied, no max-lines disable

Deliberately not addressed

CodeRabbit's pre-merge checks pass 4 of 5. The failure is Docstring Coverage at 50% against an 80% threshold, over four analyzed functions. Overridden: the production function (hostScopeCensusIsComplete) carries a full docstring, the remainder are test helpers, and AGENTS.md asks for concise non-obvious comments rather than blanket ones. Meeting the threshold would mean adding boilerplate that violates the repo's own style rule to satisfy a bot heuristic.

Not covered

  • Nothing prunes workspaceSessionsByHostId on unpair. This makes the entry harmless rather than absent. Separate follow-up.
  • A configured-but-unreachable SSH target still latches the probe, correctly: that host was owed coverage. Pre-existing and out of scope, but the same blank-tab symptom will persist for that topology.
  • worktree list returning [] on a pure-remote client was investigated separately and is not a second defect: four independent guards prevent a runtime:-hosted repo from entering a client store, and the symptom reproduces exactly on a fresh client with zero runtime repos. It is this bug plus having no locally registered repos.
  • The reporter's blank metadata is addressed by their own PR Keep mirrored remote terminal metadata #18602, which is complementary to this one.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: dc44f6ac-1830-453f-958e-a042d05117a5

📥 Commits

Reviewing files that changed from the base of the PR and between 637dc30 and 2797c6f.

📒 Files selected for processing (9)
  • src/main/ipc/pty/runtime/queried-host-kinds.test.ts
  • src/renderer/src/runtime/__fixtures__/web-session-terminal-orphan-recovery-regression-fixtures.ts
  • src/renderer/src/runtime/host-live-terminal-probe.ts
  • src/renderer/src/runtime/host-session-mirror-empty-inventory-settle.test.ts
  • src/renderer/src/runtime/web-session-terminal-orphan-recovery-host-scope-gate.test.ts
  • src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts
  • src/renderer/src/runtime/web-session-terminal-pending-handle-recovery.test.ts
  • src/shared/runtime-listing-host-scope.test.ts
  • src/shared/runtime-listing-host-scope.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The change adds hostScopeCensusIsComplete to centralize host-scope validation. It treats omitted peer runtimes as disclosure, while omitted SSH hosts, empty coverage, missing scopes, and malformed host IDs remain incomplete. Terminal probing and orphan recovery now use this helper. Tests cover runtime controller host reporting, census semantics, terminal probe results, and orphan binding pruning.

Merge Risk: 🟡 Moderate · up to 2797c

This change allows peer-runtime omissions to settle terminal inventories, but a malformed covered host identifier can still make an incomplete empty inventory authoritative, potentially hiding or pruning remote terminal metadata. Resolve this validation gap before merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The changes directly address the reported remote terminal inventory hydration failure by treating peer-runtime omissions as disclosure rather than missing coverage. The context does not show implement… Add or reference code changes and regression tests that verify remote worktree/project inventory hydration and UI-created remote tabs route to the correct remote worktree, or narrow the linked issue scope to terminal mirror hydration only.
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 12 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary fix: preventing a disclosed remote runtime from latching the mirror completeness gate.
Description check ✅ Passed The description provides detailed change rationale, linked issue, implementation scope, compatibility analysis, and test results. It does not reproduce all template headings or checklist items, but th…
Out of Scope Changes check ✅ Passed The production changes, tests, and fixture updates remain focused on host-scope census completeness and the two affected mirror recovery gates. No unrelated code changes are evident.
Full details: Linked Issues check

Explanation

The changes directly address the reported remote terminal inventory hydration failure by treating peer-runtime omissions as disclosure rather than missing coverage. The context does not show implementation for all linked issue objectives, especially worktree/project inventory behavior and UI-created tab routing.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

A single-commit fix for #18595: a leftover runtime: session partition in workspaceSessionsByHostId was making every terminal.list / worktree.list answer with a permanently non-empty hostScope.omittedHostIds, which the renderer reads as unverifiable and parks remote panes on forever.

  • listKnownExecutionHostIds narrowing — session-partition host ids whose parsed kind is runtime are no longer added to the "known" set, so a bare pairing partition stops appearing as an omission.
  • Four focused tests — two revert-tests (unscoped and worktree-scoped listings drop the phantom omission) and two guards (a mirrored row still omits its runtime host; an SSH partition still omits).

I traced the change against every consumer of listKnownExecutionHostIds and the omittedHostIds field and found the narrowing consistent and safe:

  • Repos (getRepoExecutionHostId), folder workspaces, resolved worktrees, and terminal rows with a runtime: executionHostId all still disclose a paired runtime through the loops in buildTerminalListHostScope and the includeConfiguredHosts branch, so only the evidence-free partition is dropped — the original #14973 justification is preserved.
  • queriedHostIds is built only from listProcessesWithHostScopeFromRuntimeController (local + registered SSH providers) and refreshPtyWorktreeRecordsWithControllerInventory (adds only ssh: hosts), so it never contains a runtime: id; the untouched covered filter at orca-runtime-get-runtime-id.ts:171 remains inert and no host is made to look covered.
  • isCompleteSessionTabsPtyCensus already excluded runtime: hosts from its completeness check, so its behavior is unchanged (and now more consistent) under the narrowing.
  • The new assertions are exact (toEqual([]) / toEqual(['runtime:env-7'])), and the revert/guard split in the PR description matches what the tests actually pin.

Wire compatibility is handled correctly: the frame shape is unchanged and the host publishes different content, which can only turn a phantom unverifiable into a real verdict for old clients.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

One new commit since the prior pullfrog review (8dbbfcd → 2ab22ca): a documentation-only update to the resolveOmittedHostScopeSelectors JSDoc in src/cli/omitted-host-scope-selectors.ts.

  • Doc comment corrected for the #18595 narrowing — the provenance list now reads "repos, folder workspaces, mirrored rows, and resolved worktrees" instead of "…and workspace sessions", and a new paragraph explains the distinction the fix introduced: content-bearing runtime hosts stay named as gaps, while a bare workspaceSessionsByHostId partition is not.

The comment is accurate against the code: a runtime: host still enters omittedHostIds through repos (getRepoExecutionHostId), folder workspaces, resolved worktrees, and mirrored terminal rows in buildTerminalListHostScope, and only the evidence-free session partition is dropped. No behavioral surface changed.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

One new commit since the prior pullfrog review (2ab22ca → 6e3a1c9): two tests added to src/main/runtime/terminal-list-execution-host-scope.test.ts that close the last open coverage gap.

  • Worktree-listing host scope now exercised through the real pipeline — listManagedWorktrees() reaches listKnownExecutionHostIds via managedWorktreeQueries.listKnownHostIds (not the injected dependency worktree-list-host-scope.test.ts uses), so these tests pin the actual sourcing rule on the second surface the fix governs.
  • drops a partition-only paired runtime from the worktree listing scope, keeping SSH — asserts a runtime:env-7 partition contributes nothing while ssh:box-9 (and the honest local gap from zero rows) stays named.
  • keeps a repo-known paired runtime in the worktree listing scope — asserts a repo carrying executionHostId: 'runtime:env-9' still surfaces that host, guarding against over-broadening.

Both assertions are exact (toEqual), and I traced each through listKnownExecutionHostIds → buildWorktreeListingPage → buildWorktreeListingHostScope: reverting the kind !== 'runtime' filter re-admits runtime:env-7 and fails both, so they are genuine revert-tests, not coverage theatre. Verified locally — the file's 25 tests pass.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

…gate

`hostScope.omittedHostIds` was doing two jobs with opposite requirements. As
disclosure it must over-name: `omitted-host-scope-selectors.ts` deliberately
keeps ids for servers that are no longer paired so a caller can still see the
gap, and `docs/reference/ssh-execution-boundary.md` requires a listing to name
what it did not cover. As a completeness gate it must name only coverage that
was owed and not delivered, or it latches.

It latched. `workspaceSessionsByHostId` keeps a partition for every runtime a
machine has ever paired with and nothing prunes it, and a mirrored `remote:` row
names its peer too — so any client that has ever paired outward publishes a
permanently non-empty `omittedHostIds`. `probeHostLiveTerminals` read that as
`unverifiable`, `markHostSessionMirrorHydrated` never fired, and panes parked on
`parkUntilHostSessionMirrorHydrates` never drained.

`hostScopeCensusIsComplete` gives the gate its own answer and leaves the
disclosure list alone. A `runtime:` host is never owed coverage by the runtime
answering: a paired runtime is a peer with its own control plane reached with
`--environment`, and there is no paired-runtime PTY provider for this runtime to
have queried. Two other branches stay load-bearing — an absent scope is a host
too old to claim one, and a listing that covered no host proves nothing.

No wire change: the host publishes byte-identical content and only the client's
reading moves, so this reaches the reporter by updating their client alone
rather than waiting for their remote. That also avoids a new field's fallback
rule, where "absent means complete" would recreate the bug with the polarity
flipped.

`queried-host-kinds.test.ts` pins the invariant the predicate rests on at its
source, because the consolidation moving the SSH path onto orcad is the change
most likely to introduce a runtime-backed PTY provider and quietly invalidate it.

Fixes #18595
@nwparker
nwparker force-pushed the nwparker/fix-18595-runtime-hostscope branch from 6e3a1c9 to f49bc28 Compare September 4, 2026 11:16
…invariant claim

The readiness review found the second gate unpinned: reverting
`web-session-terminal-orphan-recovery-inventory.ts` alone to the pre-PR
expression left the whole renderer suite green, because every existing fixture
passes `omittedHostIds: []`. The commit claimed two gates and proved one.

Four cases now drive `resolveTerminalOrphanInventory` through a non-empty scope.
Reverting that gate alone fails the peer-runtime case. Note the absent-scope case
deletes the key rather than passing `undefined`, because `listResult` substitutes
its default for `undefined` — routing through the fixture there silently tests
the default instead.

`queried-host-kinds.test.ts` also claimed more than it caught: a runtime-backed
transport registered under an SSH connection id reports as `ssh:` and passes,
which is the shape the orcad consolidation is expected to take. It pins the
spelling this function emits, which is what the gate keys on, and now says so.
@nwparker nwparker changed the title fix(remote): stop a leftover pairing partition making every listing incomplete fix(remote): stop a disclosure list latching the mirror completeness gate Sep 4, 2026

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The PR description still documents the reverted host-side fix, not the client-side gate that actually ships — it should be rewritten before merge.

Reviewed changes

This commit rewrites the fix from the previously reviewed host-side approach (narrowing listKnownExecutionHostIds so the host stops publishing phantom runtime: omissions) to a client-side completeness gate. The host now publishes omittedHostIds exactly as before; the renderer decides what counts as incomplete.

  • Added hostScopeCensusIsComplete in src/shared/runtime-listing-host-scope.ts — a census is complete only when a scope is present, at least one host was covered, and every omitted host is a runtime: peer (disclosure) rather than an ssh:/local gap.
  • Wired it into the two renderer readers — host-live-terminal-probe.ts and web-session-terminal-orphan-recovery-inventory.ts now call the gate instead of omittedHostIds.length > 0, so a leftover runtime: partition no longer forces unverifiable.
  • Added tests — a source-invariant test (queried-host-kinds.test.ts) pins that the PTY inventory only ever names local/ssh hosts, plus probe-level and unit tests that fail without the gate.

⚠️ PR description documents the reverted approach

The prose below the (auto-regenerated) LoC table still describes the earlier host-side fix: "The change" says listKnownExecutionHostIds no longer admits a partition-only runtime: host, and "Testing" points at six tests in terminal-list-execution-host-scope.test.ts. None of that is in this diff — orca-runtime-get-runtime-id.ts and that test file are unchanged from base. The implementation is now entirely client-side, so the "Wire compatibility" and "Hardware verification" sections are also inverted: the host publishes the same content as before, and only updated clients change behavior. Rewrite the description around hostScopeCensusIsComplete so a reviewer is not misled about where the fix lives and whether an old client is fixed.

Technical details
# Stale PR description vs. client-side implementation

## Affected sites
- PR #18619 description ("The change", "Testing", "Wire compatibility", "Hardware verification") — documents the reverted `listKnownExecutionHostIds` narrowing.

## Required outcome
- Rewrite the description to describe `src/shared/runtime-listing-host-scope.ts#hostScopeCensusIsComplete` and its two renderer call sites, and correct the wire-compat claim (host content unchanged; fix is client-only).

## Open questions for the human
- Is a client-only rollout acceptable (old clients keep the bug until they update), or does the fix also need the host-side narrowing the earlier commits had?

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

One new commit since the prior pullfrog review (f49bc28 → 1e3c6e9): the orphan-recovery gate is now pinned with its own test file, the invariant guard's claim is narrowed to what it actually proves, and the PR description was rewritten to match the client-side approach.

  • Added web-session-terminal-orphan-recovery-host-scope-gate.test.ts — four tests drive recoverWebSessionTerminalOrphansBeforeApply twice (pruning needs two consecutive authoritative inventories) and pin the second consumer of hostScopeCensusIsComplete: a peer-only omission prunes the ghost binding, while an SSH gap, a covered-nothing listing, and an absent scope all retain it.
  • Narrowed the queried-host-kinds.test.ts invariant claim — the comment now spells out what the test does and does not catch (a runtime-backed transport registered under an SSH id still reports ssh:), and the assertion hardened to a UUID-shaped connection id that must still come back ssh:.
  • Rewrote the PR body — "Root cause", "Wire compatibility" (now correctly "no wire change"), hardware verification, and the testing table all describe the client-side gate, resolving the stale-description note from the prior review.

The new gate test is a genuine revert test, not coverage theatre: tracing resolveTerminalOrphanInventory → confirmSurfaceInventoryAbsence, a peer-runtime-only omittedHostIds is the only shape that reaches the remove path, so reverting the gate would turn "prunes a twice-absent binding" into a retained binding and fail. I ran both changed test files locally — 6 tests pass.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker

nwparker commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

Note for reviewers: this PR was force-pushed and rebuilt. Earlier reviews covered an abandoned host-side approach (listKnownExecutionHostIds filtering). The current head 1e3c6e91aeb is a client-side completeness gate — hostScopeCensusIsComplete in src/shared/runtime-listing-host-scope.ts, applied at host-live-terminal-probe.ts and web-session-terminal-orphan-recovery-inventory.ts. No src/main production file is touched.

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

@nwparker I will review the rebuilt PR at 1e3c6e91aeb. I will focus on the client-side completeness gate and its two integration points.

⚠️ Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nwparker

nwparker commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Team

Run ID: 3d198d40-aaee-4b5b-be88-e7e9eb2ad424

📥 Commits

Reviewing files that changed from the base of the PR and between 2ab22ca and 1e3c6e9.

📒 Files selected for processing (7)
  • src/main/ipc/pty/runtime/queried-host-kinds.test.ts
  • src/renderer/src/runtime/host-live-terminal-probe.ts
  • src/renderer/src/runtime/host-session-mirror-empty-inventory-settle.test.ts
  • src/renderer/src/runtime/web-session-terminal-orphan-recovery-host-scope-gate.test.ts
  • src/renderer/src/runtime/web-session-terminal-orphan-recovery-inventory.ts
  • src/shared/runtime-listing-host-scope.test.ts
  • src/shared/runtime-listing-host-scope.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

return false
}
// A listing that covered no host at all proves nothing, whatever it omitted.
if (scope.hostIds.length === 0) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Validate covered host identifiers before declaring the census complete.

hostIds.length accepts an unparseable covered host as proof of coverage. For example, { hostIds: ['runtime:'], omittedHostIds: [] } returns true. The probe validates only that hostIds is an array, so it can convert this malformed inventory into none instead of unverifiable.

Reject the scope unless every hostIds entry parses successfully. Add a matching regression test.

Proposed fix
-  if (scope.hostIds.length === 0) {
+  if (
+    scope.hostIds.length === 0 ||
+    scope.hostIds.some((hostId) => parseExecutionHostId(hostId) === null)
+  ) {
     return false
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (scope.hostIds.length === 0) {
if (
scope.hostIds.length === 0 ||
scope.hostIds.some((hostId) => parseExecutionHostId(hostId) === null)
) {

CodeRabbit found a real asymmetry: the predicate refused an omitted host id it
could not parse, but accepted an unparseable *covered* id as proof of coverage.
`isTerminalListResult` validates only that `hostIds` is an array, so
`{hostIds: ['runtime:'], omittedHostIds: ['runtime:env-7']}` was `unverifiable`
before this PR and would have become `complete` after it.

Taken as "at least one legible covered host" rather than the suggested "every id
parses". A host that later gains a kind this client cannot parse would otherwise
report an incomplete census forever — which is this bug in a new coat, and the
failure mode the predicate exists to prevent.

The check exposed four tests publishing `hostIds: ['remote-runtime']`, a bare
environment id that `parseExecutionHostId` rejects. No host emits that: a runtime
answering `terminal.list` names the execution hosts it covered, which is `local`
— verified against a live paired runtime. Those fixtures are corrected to the
shape the wire actually carries, which is why the assertions move.
@nwparker

nwparker commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor
⚠️ Action not completed

Already reviewed the last commit. Use @coderabbitai full review to rerun a review of the entire changeset.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@nwparker

nwparker commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

One new commit since the prior pullfrog review (1e3c6e9 → 2797c6f) that tightens branch 2 of the completeness gate and fixes a latent fixture bug the tightening exposed.

  • hostScopeCensusIsComplete now rejects a coverage claim with no legible covered host — branch 2 changed from scope.hostIds.length === 0 to !scope.hostIds.some(parseExecutionHostId), so a listing whose covered set is entirely unreadable ids (['runtime:'], garbage) is no longer believed on the strength of an empty or all-runtime: omission list. Deliberately "at least one legible" rather than "all legible", so a host that later gains a kind this client cannot parse does not report an incomplete census forever — the same latch this predicate exists to stop, in a new coat.
  • Corrected the orphan-recovery regression fixture default scope — the default hostScope named the environment id (remote-runtime) as a covered host, which is not a legible execution-host id; the new branch would have rejected it. It now names local, matching what a live host actually reports, with matching 'local' literal updates in the sibling settle/pending-handle/gate tests.
  • Added two legibility unit tests — refuses a coverage claim with no legible host in it (a genuine revert test: with branch 2 reverted to the length check, ['runtime:'] parses to null and would still return complete) and accepts a coverage claim carrying one legible host beside an unreadable one (pins the "at least one" choice against an over-tightening to every).

I verified the changed test files locally (52 tests pass), and the covered-side leniency / omitted-side strictness split is sound: an unparseable covered id is irrelevant to completeness, while an unparseable omitted id still correctly reads as a gap.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@nwparker
nwparker merged commit 0d2375a into main Sep 4, 2026
31 of 32 checks passed
@nwparker nwparker mentioned this pull request Sep 4, 2026
8 of 9 tasks
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
…gate (stablyai#18619)

* fix(remote): stop a disclosure list latching the mirror completeness gate

`hostScope.omittedHostIds` was doing two jobs with opposite requirements. As
disclosure it must over-name: `omitted-host-scope-selectors.ts` deliberately
keeps ids for servers that are no longer paired so a caller can still see the
gap, and `docs/reference/ssh-execution-boundary.md` requires a listing to name
what it did not cover. As a completeness gate it must name only coverage that
was owed and not delivered, or it latches.

It latched. `workspaceSessionsByHostId` keeps a partition for every runtime a
machine has ever paired with and nothing prunes it, and a mirrored `remote:` row
names its peer too — so any client that has ever paired outward publishes a
permanently non-empty `omittedHostIds`. `probeHostLiveTerminals` read that as
`unverifiable`, `markHostSessionMirrorHydrated` never fired, and panes parked on
`parkUntilHostSessionMirrorHydrates` never drained.

`hostScopeCensusIsComplete` gives the gate its own answer and leaves the
disclosure list alone. A `runtime:` host is never owed coverage by the runtime
answering: a paired runtime is a peer with its own control plane reached with
`--environment`, and there is no paired-runtime PTY provider for this runtime to
have queried. Two other branches stay load-bearing — an absent scope is a host
too old to claim one, and a listing that covered no host proves nothing.

No wire change: the host publishes byte-identical content and only the client's
reading moves, so this reaches the reporter by updating their client alone
rather than waiting for their remote. That also avoids a new field's fallback
rule, where "absent means complete" would recreate the bug with the polarity
flipped.

`queried-host-kinds.test.ts` pins the invariant the predicate rests on at its
source, because the consolidation moving the SSH path onto orcad is the change
most likely to introduce a runtime-backed PTY provider and quietly invalidate it.

Fixes stablyai#18595

* test(remote): pin the orphan-recovery host-scope gate and narrow the invariant claim

The readiness review found the second gate unpinned: reverting
`web-session-terminal-orphan-recovery-inventory.ts` alone to the pre-PR
expression left the whole renderer suite green, because every existing fixture
passes `omittedHostIds: []`. The commit claimed two gates and proved one.

Four cases now drive `resolveTerminalOrphanInventory` through a non-empty scope.
Reverting that gate alone fails the peer-runtime case. Note the absent-scope case
deletes the key rather than passing `undefined`, because `listResult` substitutes
its default for `undefined` — routing through the fixture there silently tests
the default instead.

`queried-host-kinds.test.ts` also claimed more than it caught: a runtime-backed
transport registered under an SSH connection id reports as `ssh:` and passes,
which is the shape the orcad consolidation is expected to take. It pins the
spelling this function emits, which is what the gate keys on, and now says so.

* fix(remote): require a legible covered host before believing a census

CodeRabbit found a real asymmetry: the predicate refused an omitted host id it
could not parse, but accepted an unparseable *covered* id as proof of coverage.
`isTerminalListResult` validates only that `hostIds` is an array, so
`{hostIds: ['runtime:'], omittedHostIds: ['runtime:env-7']}` was `unverifiable`
before this PR and would have become `complete` after it.

Taken as "at least one legible covered host" rather than the suggested "every id
parses". A host that later gains a kind this client cannot parse would otherwise
report an incomplete census forever — which is this bug in a new coat, and the
failure mode the predicate exists to prevent.

The check exposed four tests publishing `hostIds: ['remote-runtime']`, a bare
environment id that `parseExecutionHostId` rejects. No host emits that: a runtime
answering `terminal.list` names the execution hosts it covered, which is `local`
— verified against a live paired runtime. Those fixtures are corrected to the
shape the wire actually carries, which is why the assertions move.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

M1 remote inventory omits connected Intel runtime, blanking remote tabs after 1.4.197

1 participant