Repository navigation
Conversation
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('keeps a committed host sleep when an automatic spawn reaches the lock', async () => { | ||
| const { runtime } = shellSessionRuntime() | ||
| rememberHostSleep(runtime, 'sleeping') | ||
|
|
||
| await expect( | ||
| runtime.acquireWorktreeTerminalSpawn(TEST_WORKTREE_ID, 'leave') |
There was a problem hiding this comment.
Knowledge Base Used: PTY streaming and recovery
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 50f0b41d2c2bce27dfda932a6a88a65bbd03e91f and 9447a72. 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds host worktree sleep phases and Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Automatic recovery now leaves a slept remote terminal stopped, while explicit opening still wakes it. No actionable merge-blocking risk remains from the supplied review. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens the distinction between automatic recovery and intentional reopening. The reviewed paths preserve stopped state, but external caller behavior and live remote operation remain partly unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description explains the issue, scope, behavior, testing, trade-offs, and linked issue. However, it does not follow the repository template and omits several required sections, including ELI5, What Changed, Why, Visual Proof or an explicit N/A, AI Disclosure, Review, Agent skill upstream boundary, Notes, and the Checklist. Resolution Restructure the description using the repository template. Add the missing required sections, provide visual proof or state N/A with a reason, complete the testing checkboxes, disclose AI use when applicable, and complete the review, notes, boundary, and checklist items.
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two minor suggestions inline.
Reviewed changes
- Host sleep predicate + settle helper — new
worktree-terminal-spawn-sleep-disposition.tscentralizes which phases block automatic recovery (stopping/sleeping/partial) and how a spawn may settle a committed sleep. - Spawn lock honors disposition —
acquireWorktreeTerminalSpawntakes'wake' | 'leave';'leave'keeps a committed sleep and throwsWorktreeTerminalLeftAsleepError. - Automatic materialize refuses a host sleep —
activateMobileSessionTabadds the host sleep phase to the park guard, forwardsleaveWorktreeSleepingonly for automatic activations, and swallowsWorktreeTerminalLeftAsleepErrorwithout hydrating or rethrowing. - Plumbing + tests —
leaveWorktreeSleepingtravels throughTerminalCreateOptions, the runtime-owned create opts, andRuntimePtySpawnArgs; three new unit/integration assertions plus the retained park tests.
ℹ️ Automatic recovery now relies on one call site swallowing a thrown lock error
The lock-level 'leave' fix only works because activateMobileSessionTab's catch recognizes WorktreeTerminalLeftAsleepError via instanceof and returns the pending snapshot. Any other future automatic materialize path that sets leaveWorktreeSleeping must repeat that swallow, or the error surfaces to the user. A comment tying the flag to the required catch (or a helper that both sets the flag and owns the handling) would make the coupling explicit.
Technical details
# leaveWorktreeSleeping and its error are a two-part contract
## Affected sites
- src/main/runtime/orca-runtime-perform-mobile-session-pty-records-refresh.ts:190-215 — sets `leaveWorktreeSleeping` and swallows `WorktreeTerminalLeftAsleepError`.
- src/main/runtime/orca-runtime-stop-terminals-for-worktree.ts:278-287 — throws only for disposition `'leave'`.
## Required outcome
- A caller that opts into `'leave'` cannot accidentally let the error escape.
- Coverage should exercise the interaction, not just each half.
## Open questions for the human
- Is `WorktreeTerminalLeftAsleepError` intended to be caught by exactly one call site, or should the runtime expose a materialize helper that owns both the flag and the catch?
</details>
<!--
Pullfrog review metadata. These findings were written against 50f0b41;
if commits have landed on fix-24399-remote-sleep-replay since, treat every specific bug, file, or
line callout as POTENTIALLY STALE and re-diff before acting on it.
- Mode: Review (initial)
- Files reviewed: 11
- Commits reviewed: 1
- Base: main (c991893)
- Head: fix-24399-remote-sleep-replay (50f0b41)
- Reviewed commits:
- 50f0b41 — fix(runtime): keep a slept remote shell terminal stopped
- Prior pullfrog review: none
-->
<!-- PULLFROG_DIVIDER_DO_NOT_REMOVE_PLZ -->
<sup><a href="https://pullfrog.com"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pullfrog.com/logos/frog-white-full-18px.png"><img src="https://pullfrog.com/logos/frog-green-full-18px.png" width="9px" height="9px" style="vertical-align: middle; " alt="Pullfrog"></picture></a> ｜ [Fix all ➔](https://pullfrog.com/trigger/stablyai/orca/24416?action=fix&review_id=5382363960) ｜ [Fix 👍s ➔](https://pullfrog.com/trigger/stablyai/orca/24416?action=fix-approved&review_id=5382363960) ｜ [View workflow run](https://github.com/stablyai/orca/actions/runs/36891454710/job/110468520944) ｜ Using `deepseek-v4.1-flash` (free via [Pullfrog for OSS](https://pullfrog.com/for-oss)) ｜ [𝕏](https://x.com/pullfrogai)</sup>
Automatic recovery recreated the terminal because the park guard only consults an agent resume record, and the spawn lock treated a sleep that committed while it waited as a user wake. A background materialize now leaves a stopping, sleeping, or partial host sleep in place, and the single spawn acquire uses that leave disposition. Fixes stablyai#24399
Recovery already passes the flag and the spawn path reads it, but the controller options type omitted it. stopping stays out of the leave check because that phase never outlives the exclusive mutation lock. Co-authored-by: Cursor <cursoragent@cursor.com>
50f0b41 to
9447a72
Compare
Sync update (
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The deltas since the prior pullfrog review at 50f0b41 close both prior inline findings; the substantive fix was already reviewed.
- Spawn contract declares
leaveWorktreeSleeping—RuntimePtyController.spawn's opts gained the optional field, so the plumbing no longer type-checks only becauseorca-runtime-create-terminal.tsis@ts-nocheck. stoppingasymmetry documented —settleWorktreeSpawnSleepgained a comment recording thatstoppingis always replaced before its exclusive mutation lock is released, so a shared-lock spawn never observes it.
I confirmed the stopping invariant in sleepResolvedWorktreeTerminals's finally block: the phase is rewritten to sleeping or partial (or the record deleted) before releaseMutation(), so the comment is accurate. pnpm tc:node and the two new sleep-disposition unit suites pass locally.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
This branch has not been deployed
1 participant
Description
Sleeping a paired headless workspace stops its terminal, then automatic recovery creates the same session again about a second later. Refreshing the desktop first, or sleeping a workspace that is not selected, stays stopped. Ordinary shell tabs have no agent resume record, and the park guard only looks at that record. The spawn lock then treats a sleep that became committed while the spawn waited as a user wake and clears it.
Focused fix
In scope: an automatic materialize refuses a host sleep phase of stopping, sleeping, or partial, and the one spawn acquire leaves a committed sleeping or partial phase in place instead of clearing it.
Out of scope: the user opening the tab, which remains the wake gesture. Agent resume records, SSH reattach hydration, and holding a second lock across terminal creation.
Preserves
An explicit user spawn still clears a committed sleep and creates the terminal. A shell tab with no host sleep still materializes. A slept agent pane stays parked. A failed SSH reattach still hydrates the headless snapshot.
Evidence
node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --cache false src/main/runtime/worktree-terminal-spawn-sleep-disposition.test.ts src/main/ipc/pty/runtime/spawn-execute-sleep-disposition.test.ts— 5 passed.node node_modules/vitest/vitest.mjs run --config config/vitest.config.ts --cache false src/main/runtime/orca-runtime.test.ts -t "deliberately parked pane activation"— 18 passed, 1329 skipped.The cases cover a shell tab with no host sleep, automatic recovery for sleeping, partial, and stopping, a user open of a sleeping shell tab, and the spawn acquire passing leave versus wake. A live remote workspace was not rechecked.
User-regression-tradeoffs
Automatic recovery no longer recreates a terminal while the host records that workspace as stopping, sleeping, or partially slept. Opening the tab still wakes it.
Fixes #24399