Repository navigation
Never read an unanswerable hasPty probe as proven PTY absence - #16932
brennanb2025 wants to merge 3 commits into
Conversation
|
Push verified: |
|
Warning Review limit reachedNext included review available in 5 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds typed PTY presence verdicts for live, exited, and unverifiable states. Unanswerable probes record an unverifiable verdict instead of confirming absence. Live probes clear stale verdicts, while confirmed exited probes permit demotion. Tests cover terminal listing, worker dispatch, recovery, and inventory-listed PTYs. Merge Risk: ⚪ Minimal · up to The change preserves unverifiable probe failures without false disconnection, while confirmed absence still clears uncertainty and demotes the terminal correctly. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and relevant. It includes the ELI5 explanation, behavior changes, rationale, reliability contract, compatibility considerations, and verification results. It does not include an explicit Linked Issue reference, the required template headings and checklist, or the required Visual Proof section with N/A or attachments. Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (1 skipped: 1 too large.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
Independent adversarial review — 19705c6Reviewed as sole reviewer; every author claim re-tested from scratch in a clean worktree at this exact SHA. Verdict: merge-ready. No P0/P1. Three P2 nits, none blocking. All CI checks green at review time. Pre-fix repro (re-run, not taken on faith)Reverted only Ablation (re-run individually; 12 tests each run)
The last row matters: the original demotion behavior (undefined/probe-less controller → inventory is authority → exited) is still pinned by main's own test, so this PR does not regress the stale-leaf fix. Recovery of stale doubt is double-covered: the inventory refresh ( Consumer enumeration (independently derived, then diffed)No wire shape or type changed anywhere, so every consumer compiles unchanged — the review question is value semantics per consumer. My grep of every My list is a superset — 5 consumers the PR body does not enumerate, all verified unaffected:
Wire claim (the risky part) — proven, not assertedChecked at shipped tag v1.4.190, the realistic old client:
Residual risk (confident-false sibling) — safe to leave openThe non-owning-provider confident-false window is real and reachable today: the degraded daemon provider answers Task-spec correction — verified
Performance (by hand; the perf skill is disable-model-invocation for workers)Inventory-hit path short-circuits before the probe (pinned by the "never consults" test) — zero added work on the hot path. Missed-pane path: same single sync P2 nits (none blocking)
Also noted, by design rather than defect: between the blip and the next inventory refresh, Checklist sweepSSH/remote: verdict vocabulary and rule 2 followed exactly; SSH/ Merge-ready. CI fully green. Never merged/approved/closed by this review, per task. |
…bsence A null hasPty answer means the per-id presence query could not be asked (failed provider lookup, probe-less provider behind the adapter). The list summary's proven-absence rescue treated any non-true answer as confirmation, so a live worker pane that missed one inventory snapshot while its probe was unanswerable published connected:false — and orchestration wrote the worker off as exited. The controller's answer now resolves through an exhaustive live/unverifiable/exited presence observation: only an observed false (or a controller with no per-id probe, where the successful inventory stays the authority) demotes; null records an unverifiable liveness verdict instead, and a later live observation clears the recorded doubt. The typed gate isConfirmedPtyAbsence does not accept the unverifiable arm.
…es live The unlisted-pane rescue cleared the liveness verdict on any `live` answer. For a leaf already marked disconnected by an unverified stop that published `connected:false` with no verdict on record — the exact pair worker observation and federation control read as a confirmed exit. Clear the verdict only while the leaf still publishes as connected.
19705c6 to
a41679f
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Review loop — patched, not supersededRebased onto current One defect found and fixed (
|
| tree | probe |
|---|---|
pre-PR origin/main |
PASS — verdict survives, consumer reads unverifiable |
PR head 19705c6475 |
FAIL — connected:false + verdict erased → consumer reads exited |
| PR head + fix | PASS |
New test keeps recorded doubt for a disconnected leaf the presence query answers live is red against the unfixed head and green with the fix.
Gate ablation table (each gate reverted individually):
| ablation | result |
|---|---|
null → unverifiable arm removed |
RED (3) |
markPtyLivenessUnverifiable removed |
RED (2) |
forgetPtyLivenessVerdict removed |
RED (1) |
hasPtyAnswer === true → live arm removed |
RED (2) |
isConfirmedPtyAbsence(...) → true |
GREEN — see note |
No two gates are only jointly pinned; every load-bearing gate goes red on its own.
Gates: pnpm tc exit 0, oxlint clean on all 4 files, git diff --check clean, PR suites 13/13, plus 9 related liveness suites 94/94.
Does an unanswerable probe still reach a caller as a positive absence?
No. null only originates at the runtime-controller adapter and the pty:hasPty IPC handler — provider-level hasPty is strict boolean (pty-provider-contract.ts:139, ssh-pty-provider.ts:295, local-pty-provider.ts:1098), so the !hasPty(...) sites in write-input.ts:181 and ssh-relay-session.ts:1953 can never see it. Of the two controller-level consumers, the incumbent sweep at :33775 was already tri-state (covered by pty-inventory-liveness-verdict.test.ts), and buildTerminalSummary is this PR plus the fix above. On the renderer side shouldReconcileMissingSession returns early unless isLive === false, so null never tears a pane down.
Notes, not blockers
isConfirmedPtyAbsenceis a compile-time guard, not a runtime gate. Its only call site is insidecase 'exited':, where the switch has already narrowed the value, so it can only ever returntrue— which is why substitutingtruestays green. That is structural, not a missing test, but the docstring calling it a "typed demotion gate" reads as though it has runtime force.- Second tri-state resolution in the same file.
orca-runtime.ts:33775-33799already resolves the same three-wayhasPtyanswer for leafless PTYs, with the sameNO_OBSERVING_PROVIDER_REASONconstant. The two populations are disjoint (leafExistsForPty), so there is no double probe, but they now disagree onundefinedand could drift. resolveUnlistedPtyPresencefoldsundefinedintoexited. That preserves pre-fix!== truesemantics so it is not a weakening, but the exported signature acceptsboolean | null | undefined, and a future caller whoseundefinedmeans "not asked" would silently get a death certificate.
Composition with #16953
They compose, and they are not mutually redundant — reverting either leaves the bug live:
- Never let a non-owning provider answer a PTY presence question false during the daemon swap window #16953 alone: turns the pre-swap provider's fabricated
falseinto an honestnull, but the pre-Never read an unanswerable hasPty probe as proven PTY absence #16932 predicatehasPty(...) !== truereadsfalseandnullidentically, so it still demotes. - Never read an unanswerable hasPty probe as proven PTY absence #16932 alone: inside the swap window the provider still answers a fabricated
false→ still demotes.
That is a mechanistic confirmation of the same-release constraint. No file overlap; git merge-tree origin/main HEAD exits 0.
Two things for #16953's own lane, not this PR: it is based on a stale 4bb337741c and currently conflicts with origin/main in src/main/ipc/pty/ipc/inspect.ts, so it needs a gh pr update-branch. And both PRs stub hasPty directly on the controller, bypassing hasPtyFromRuntimeController — so the producer/consumer seam the two jointly depend on is untested by either.
Not verified
No Electron/CDP run (this change has no rendered surface), nothing on Windows or a real SSH host, and no packaged-build check. The cross-version wire harness covers the terminal stream only, so the connected/writable publication-timing change has no automated old-client coverage — the reasoning above is the only check on it. Behaviour there moves in the safe direction (strictly fewer false connected:false), so no capability gate is needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 29f0ecea-e30a-43cc-9890-fee4ebb2ab0b
📒 Files selected for processing (4)
src/main/runtime/orca-runtime.tssrc/main/runtime/orchestration/coordinator-task-dispatch.test.tssrc/main/runtime/terminal-list-stale-leaf-liveness.test.tssrc/shared/pty-liveness-verdict.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/shared/pty-liveness-verdict.ts
- src/main/runtime/orca-runtime.ts
- src/main/runtime/orchestration/coordinator-task-dispatch.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
…absent The exited arm set provenAbsent but left any verdict an earlier unanswerable probe had recorded, so a null-then-false transition published connected:false while getPtyLivenessVerdict still reported unverifiable — under-reporting a pane the probe positively answered about. The live arm's asymmetry is deliberate and unchanged: it clears doubt only while the leaf still publishes connected, because dropping a verdict under connected:false is what worker observation reads as a confirmed exit. Here that reading is the correct one.
Summary
live | unverifiable | exitedpresence observation instead ofhasPty !== truenull(unanswerable) per-pane presence answer now records anunverifiableliveness verdict instead of demoting the pane toconnected:falseliveobservation clears the recorded doubt; a confirmedfalsestill demotes exactly as beforeAuthor: @BrennanKB5
ELI5
Orca keeps a list of your terminals and asks the terminal engine "does this pane still have a live process?" The engine has three honest answers: "yes", "no", and "I couldn't check right now." Before this fix, "I couldn't check right now" was treated the same as "no" — so a terminal whose agent was alive and working could be shown as disconnected, and the orchestrator would write the worker off. Now "I couldn't check" is kept as its own answer: the pane stays connected, the doubt is recorded, and it's re-checked on the next look.
User-facing before / after
When you'd hit this: you're coordinating worker agents in Orca terminals (or just running
orca terminal list) at a moment when the PTY backend can't answer the per-pane presence probe — e.g. the daemon-backed provider is mid-restart/being swapped, or the pane's provider lookup fails — and the same refresh's inventory snapshot happens to miss that pane.Before: the pane flips to
disconnectedinterminal.list/orca terminal list, the coordinator's availability filter drops that worker terminal from the dispatchable pool, and worker/federation status surfaces read the pane'sconnected:falseas exited — while the agent process is alive and still working. A live worker gets written off from one unanswerable query.After: the pane stays
connectedin the list, and the unanswerable query is recorded as anunverifiableliveness verdict with a reason — the same verdict vocabularyorchestration.workerShow/federationShowalready publish — so observation surfaces say "unverifiable", never "exited", until a confirmed answer arrives. A confirmed "no" (the engine positively observed the pane absent) demotes exactly as before, and a confirmed "yes" clears any recorded doubt.Reliability contract
exitedrequires positive evidence; loss of the ability to ask isunverifiable(docs/reference/ssh-execution-boundary.md rule 2).buildTerminalSummary's rescue readhasPty !== true, so the controller adapter's documentednull("the query could not be answered": failed provider lookup, probe-less provider, probe threw) counted as proof of absence.PtyPresenceObservation(live | exited | unverifiable); the demotion gateisConfirmedPtyAbsence(presence: ConfirmedPtyPresence)excludes theunverifiablearm at the type level — doubt is not assignable where a confirmed answer is expected. No casts or widenings.connected:false, no recorded verdict, andlistAvailableWorkerTerminalsreturning[]for a live worker terminal; the pre-existing inventory-absence, hasPty-rescue, SSH/remote-exclusion, and unavailable-inventory tests all still pass unchanged.nullarm (3 tests), unverifiable-marking arm (2), live-rescue forget arm (1), exited demotion arm (3, including the pre-existing proven-absence test).listAvailableWorkerTerminals(now keeps the terminal dispatchable); CLIterminal listformatter (prints connected truthfully);resolveGroupAddress, pluginlistWorktreeTerminals, federation/topology terminal-effect recorders, and the active-terminal fallback (none readconnected— unaffected); rendererremote-worktree-sleep(readsconnected === truefor ssh panes, which this predicate never demoted); verdict readersinspectWorkerTerminal(workerShow/workerRead/workerStop/release-completion) andinspectRemoteAttachment(federationShow/Read/ReadOutput/Stop) — both branch onunverifiablebefore theconnected === false → exitedfallback;unverifiableStopVerdictkeeps a failed stop unverifiable.terminal.liststops publishing a falseconnected:falseduring an unanswerable blip (old clients readconnected:trueexactly as before), andobservation.status: 'unverifiable'can occur in a window that previously misreported — that value and itsreasonfield already ship on these surfaces, so mixed-version peers parse it today. No capability gating needed.hasPtycall as before plus O(1) writes to the existing 256-capped verdict map. No new polling, timers, subprocesses, or unbounded state.falsefor a pane it does not own (e.g. the pre-swap local provider during daemon startup) is a separate coercion site of this class and is deliberately not touched here, matching the sibling-site split in Preserve legacy daemon ownership when liveness is unavailable #16900/Fix Windows daemon host pruning on unverifiable liveness #16908/Keep relay completion evidence live/unverifiable/exited, never coerced #16926.Verification
pnpm test src/main/runtime/terminal-list-stale-leaf-liveness.test.ts src/main/runtime/orchestration/coordinator-task-dispatch.test.ts(12/12 post-fix; 3 fail pre-fix)pnpm test src/main/runtime/orca-runtime.test.ts(1213 passed) andsrc/main/runtimeliveness +src/main/runtime/rpcsuites (201 files, 1695 passed)pnpm tc(all projects),oxlint+oxfmt --checkon changed files (lint gate proven live with a canary)