Repository navigation
Conversation
…lank terminal Clicking an emptied workspace left it with no surface: the async activation gate verified the host empty (or could not answer), but the seeder still refused on an unverifiable workspace-sync verdict, and a blocked gate never reseeded at all. Let a gate-verified empty reseed proceed for unverifiable hosts, and fall back to the local authority check on blocked gates. Live (runtime-owned) workspaces still never seed client-side, and startup hydration still honours the closed-last-tab tombstone. Fixes stablyai#22015
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change renames the gate option to Priority: ➖ Normal Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Important
gateVerifiedEmpty widens the seed exception to every unverifiable workspace, but unverifiable also covers a runtime host whose owner cannot be named. That path now seeds a client terminal beside the host's own — the #15556 double-write the PR body says cannot happen. The first inline comment has the reproduction.
Reviewed changes
gateVerifiedEmptyoption — a newInitialTerminalOptionsflag threaded throughreseedGatedEmptyWorkspaceintoensureWorktreeHasInitialTerminal.emptyauthorizes a seed —gateAndReseedEmptyWorkspacenow passesgateVerifiedEmpty: truefor theemptyoutcome, extendingshouldAutoCreatebeyondhostAuthority === 'none'tounverifiable.blockedreseeds — theblockedoutcome falls back toreseedGatedEmptyWorkspacewithout the flag, so onlynone-authority workspaces seed.- Tests — a direct-SSH
unverifiablepredicate test and a localblockedreseed test.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…cked fallback Address review findings on the empty-workspace awakening: - A runtime host that cannot be named also reports unverifiable, and the gate reports empty there via its unscoped fallback. Gate-approved reseeds now require an explicit direct-SSH workspace check, so the client never seeds beside a paired host's own terminal (stablyai#15556). - A blocked gate also covers unrestored sessions and inconsistent structured ownership, where the renderer does not yet know what surfaces exist. The blocked fallback now only fires with no known sleeping sessions, instead of adding the stray shell the deferral was avoiding. - Rename gateVerifiedEmpty to gateApprovedReseed: empty is the gate's produced-no-surface directive, not proof the host holds nothing. - Add an activation-level SSH test through the mocked empty gate (the wiring the unit test alone did not guard), a runtime-unnameable exclusion test, and blocked with/without sleeping tests.
|
Addressed the pullfrog review findings in c987d22:
|
There was a problem hiding this comment.
Important
The blocked fallback is narrower than before, but it still reseeds a local shell when the gate blocked on inconsistent structured ownership — the stray terminal the deferral exists to prevent. The new guard only covers sleeping sessions; a reproduction is in the inline comment.
Reviewed changes
- Gate-approved reseed scoped to direct-SSH —
gateVerifiedEmptybecamegateApprovedReseed, andshouldAutoCreatenow also requiresisDirectSshWorkspace, so a runtime host whose owner cannot be named stays unseeded (#15556). blockedfallback narrowed — the fallback now runs only whenworkspaceHasSleepingAgentSessionsis false; a workspace with a sleeping agent stays deferred.- Activation-level wiring coverage — a mocked
emptygate test now drivesgateAndReseedEmptyWorkspace, so dropping the flag fails the suite. - New regression tests — runtime-unnameable + gate-approved reseed stays unseeded;
blocked+ sleeping sessions leaves the row empty.
ℹ️ Nitpicks
worktree-activation-store-contract.ts:85saysgateApprovedReseedis "Never set forliveor runtime-unnameable hosts", but theemptybranch passestruefor every host kind and only the predicate ignores it there. "Never honored for" would match the code.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Follow-up to the gate-approved reseed: a plain blocked gate also covers unrestored sessions and inconsistent structured ownership, where the renderer does not yet know what surfaces exist. Seeding there plants a stray shell beside a chat that appears moments later (e.g. an ownerless agent-session the runtime is about to project). Expose the census cause from the gate as blocked-census-unavailable and reseed only for it (still gated on no known sleeping sessions). Plain blocked never reseeds. Also fixes the contract doc nitpick: the empty branch passes the flag for every host kind; the predicate honors it only for direct-SSH.
|
Follow-up in 7f8ceff addressing the second review round:
Verified: gate/activation suites green (76 + 68 tests across the touched areas), |
There was a problem hiding this comment.
ℹ️ No critical issues — one test-coverage gap and a stale comment inline.
Reviewed changes
- Split
blockedinto two verdicts — the gate now returnsblocked-census-unavailableonly whendeps.listSessions()itself throws; every other blocked cause (restore never ready, structured read throwing, inconsistent structured ownership, unbound live PTY) stays plainblocked. - Narrowed the reseed fallback —
gateAndReseedEmptyWorkspacenow enters the fallback only forblocked-census-unavailable, so a plainblockedleaves the workspace empty instead of planting the stray shell beside a chat about to appear. Both prior review threads are addressed by this. - Tests — a gate test asserts the census-throw flavor, and an activation-level test asserts plain
blockedleaves the row empty.
ℹ️ Nitpicks
worktree-activation-emptied-workspace-reseed.test.ts:211— the comment "blockedmeans the census could not answer, not that the workspace has a surface" now describes the opposite of the split; that sentence belongs toblocked-census-unavailable.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
![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: Advanced
Run ID: 51d53795-a83a-48a2-994e-02791e431031
📒 Files selected for processing (5)
src/renderer/src/lib/worktree-activation-emptied-workspace-reseed.test.tssrc/renderer/src/lib/worktree-activation-gated-empty-reseed.tssrc/renderer/src/lib/worktree-activation-store-contract.tssrc/renderer/src/lib/worktree-agent-activation-gate.test.tssrc/renderer/src/lib/worktree-agent-activation-gate.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
A rejected PTY census with structured owners on record now stays plain blocked instead of becoming the reseedable flavor: TUI terminals need the census to bind, and skipping it would also blind adoption to coexisting live shells. Only a census with no structured context may reseed. Covers the sleeping-guard mock with the census-unavailable flavor and fixes the stale split comment.
|
Follow-up in 7264781 covering the latest pullfrog + CodeRabbit notes:
Verified: 177 tests across gate/activation/seam/reactivation/store suites green, |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Census-throw path fails closed on structured owners — when
deps.listSessions()rejects and a structured runtime inventory is on record, the gate now returns plainblockedinstead of the reseedableblocked-census-unavailable, so a shell cannot land beside a structured surface the runtime already knows about. - New gate tests — a native-only structured inventory resolves
structured(no seed) while the census still runs, and a rejected census with native or TUI owners on record staysblocked. - Prior coverage gap closed — the sleeping-workspace activation test now mocks
blocked-census-unavailablerather thanblocked, so the fallback'sworkspaceHasSleepingAgentSessionsguard is actually exercised, and the staleblockedcomment was corrected.
I also checked whether the guard should test structured (which includes store-only unifiedTabsByWorktree agent-session tabs) instead of structuredInventory. It should not: a store-only structured tab already suppresses seeding through renderableTabCount > 0 in shouldAutoCreateInitialTerminal, so structuredInventory is precisely the runtime-knows/renderer-has-not-projected case that needs the conservative verdict.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
This goes the opposite way from #21753, #17682 and #19591, which ask that selecting an emptied workspace leave it empty. It's scoped to direct-SSH and unverifiable workspaces, where today the click does nothing at all, and it never seeds runtime-owned (#18513) or startup-restored workspaces. Before this gets a full review, could a maintainer decide whether an explicit click on an emptied workspace should seed one blank shell? |
|
@mmarabel thanks for filing and putting this together. Your diagnosis was right: the activation census already knows the host has nothing running, and seeding was ignoring it. #23213 lands the same core idea in a smaller change, and you're credited as a co-author on the commit. The one place it deliberately differs is the Closing this in favour of #23213. If you think the |
ELI5
Closing the last tab in a workspace puts it to rest, which is fine. But clicking that emptied workspace to wake it back up did nothing — no terminal appeared, so the workspace stayed dead until you opened the new-tab menu and created one by hand. Now an explicit re-open seeds one blank terminal so the workspace wakes up. Nothing old is resumed; it is a fresh shell.
What Changed
Before: activating an emptied workspace (e.g. an SSH-backed worktree whose host had not answered workspace sync) left the center empty/landing with no surface. The async activation gate could verify the host held no live PTYs, or fail to answer at all, and in both cases no terminal was seeded.
After: the user clicks the empty workspace and one blank terminal appears, making the workspace usable immediately.
Mechanism:
gateVerifiedEmptyflag to the initial-terminal options. When the activation gate already censused the owning host and found no live PTYs, an explicit empty-workspace reseed may proceed even while workspace-sync hydration still reportsunverifiable.live(runtime-owned) hosts are still never seeded client-side — the host owns creation there.blockedgate (census could not answer) now falls back to the local authority check, sonone-authority workspaces still reseed instead of stranding an explicitly opened empty workspace.unverifiablestays unseeded without the gate's positive empty verdict (STA-4658).Why
Stranded-with-no-surface is worse than one fresh shell for an explicit user gesture. Seeding a fresh shell does not fork a second writer onto a live agent transcript (no resume happens on this path); the resumed-vs-fresh distinction the reporter asked for is preserved. Alternatives considered: seeding unconditionally on activation (would reintroduce per-launch duplicates per STA-4658 and the runtime double-seed in #17681), and leaving empty workspaces empty (the direction of #21753 / #17682 / PRs #18513 and #19591) — rejected here because the reported bug is the workspace never waking at all.
Linked Issue
Fixes #22015
Visual Proof
N/A — no screenshots or video attached. Verified via automated tests below; manual UI capture not run in this environment.
Testing
New/updated tests:
workspace-terminal-host-authority.test.ts: gate-verified empty seeds an explicitly reopened empty SSH (unverifiable) workspace;liveruntime workspace never seeds even when gate-verified.worktree-activation-emptied-workspace-reseed.test.ts: ablockedgate still reseeds a local empty workspace.Runs: targeted activation suites (119 tests) and store suites (103 tests) pass;
pnpm tcpasses;check:code-quality:changedpasses with 0 findings. Full suite and build left to CI.Platforms actually tested: none manually (unit tests only, Linux host).
Review
Self-reviewed. Cross-platform: change is platform-neutral (store logic, no shell/path/shortcut/menu code). SSH/remote/local: SSH
unverifiablenow seeds only after the gate verified the host empty;liveruntime never seeds client-side; local behavior unchanged. Agents/integrations: no agent-specific logic; fresh shell only, no resume/clone of sessions. Performance: one extra boolean through an activation-only path, no hot-path work. Security: no new inputs, IPC, or permissions; creates a normal local tab like the manual New Terminal flow.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Security, cross-platform (Linux, Windows, Mac), remote SSH, mobile, backwards compatibility, performance considered above. Known tension: open items #21753, #17682, #11108 and PRs #18513/#19591 prefer keeping empty workspaces empty on selection; this PR takes the opposite stance for explicitly re-opened workspaces that would otherwise have no surface at all.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)