Repository navigation
Show fork commands when context menu can fork - #5200
lawrencecchen wants to merge 6 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR re-sources forkable-agent fallback snapshots from workspace.forkableAgentSnapshot(forPanelId:), requires .supportedWithoutProbe for fallback visibility, records per-panel forked fallback snapshots at fork creation and continuation, alters cache update logic on probe mismatch, and returns fallback snapshots for immediate forks when probe verification fails. Tests updated accordingly. ChangesForkable-Agent Fallback Snapshot Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/ContentView+ForkAgentConversation.swift (1)
44-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the helper-based snapshot source in the execution path.
Line 44 still pulls the fallback from
restoredAgentSnapshotsByPanelId. That leaves visibility and execution on different snapshot sources, so a probe-free fork command can be shown and still fail/beep on first execution whenworkspace.forkableAgentSnapshot(forPanelId:)has data but the dictionary does not.Suggested fix
- let fallbackSnapshot = currentContext.workspace.restoredAgentSnapshotsByPanelId[panelId] + let fallbackSnapshot = currentContext.workspace.forkableAgentSnapshot(forPanelId: panelId)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/ContentView`+ForkAgentConversation.swift around lines 44 - 55, The fallback snapshot must come from the workspace helper to ensure consistent snapshot source; replace the use of currentContext.workspace.restoredAgentSnapshotsByPanelId[panelId] with the helper currentContext.workspace.forkableAgentSnapshot(forPanelId: panelId) so that Self.commandPaletteImmediateForkExecutionSnapshotSelection(...) receives the helper-provided fallbackSnapshot; update the variable assignment for fallbackSnapshot (and any related usages in this block) to call forkableAgentSnapshot(forPanelId:) instead of indexing restoredAgentSnapshotsByPanelId to avoid mismatched visibility/execution sources.cmuxTests/CommandPaletteSearchEngineTests.swift (1)
802-856: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winCover the probe-free OpenCode execution branch too.
Lines 802-856 only prove immediate execution for
.codexand rejection for direct local OpenCode. The changed behavior also depends on probe-free OpenCode snapshots being executable without a verified probe, so a regression in thelauncher == "omo"/.supportedWithoutProbepath would still pass here because that shape is only covered by the visibility test.➕ Suggested regression test
+ func testImmediateForkExecutionUsesProbeFreeOpenCodeFallbackBeforeProbeVerification() { + let workspaceId = UUID() + let panelId = UUID() + let fallback = SessionRestorableAgentSnapshot( + kind: .opencode, + sessionId: "fallback-omo-session", + workingDirectory: "/tmp/opencode repo", + launchCommand: AgentLaunchCommandSnapshot( + launcher: "omo", + executablePath: "/usr/local/bin/cmux", + arguments: ["/usr/local/bin/cmux", "omo"], + workingDirectory: "/tmp/opencode repo", + environment: nil, + capturedAt: 123, + source: "environment" + ) + ) + + let snapshot = ContentView.commandPaletteImmediateForkExecutionSnapshot( + workspaceId: workspaceId, + panelId: panelId, + isRemoteTerminal: false, + supportedPanelKeys: [], + supportedRemoteContextsByPanelKey: [:], + snapshotFingerprintsByPanelKey: [:], + fallbackSnapshot: fallback, + cachedSnapshot: nil + ) + + XCTAssertEqual(snapshot?.sessionId, fallback.sessionId) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/CommandPaletteSearchEngineTests.swift` around lines 802 - 856, Add a test that asserts probe-free OpenCode snapshots (launcher == "omo") are accepted by commandPaletteImmediateForkExecutionSnapshot: create a fallback SessionRestorableAgentSnapshot with kind .opencode and an AgentLaunchCommandSnapshot whose launcher is "omo" (matching the supportedWithoutProbe path), call ContentView.commandPaletteImmediateForkExecutionSnapshot with that fallback and nil cachedSnapshot, then XCTAssertEqual(snapshot?.sessionId, fallback.sessionId); mirror naming and setup from testImmediateForkExecutionUsesProbeFreeFallbackSnapshotBeforeProbeVerification and ensure the test covers the probe-free OpenCode branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmuxTests/CommandPaletteSearchEngineTests.swift`:
- Around line 802-856: Add a test that asserts probe-free OpenCode snapshots
(launcher == "omo") are accepted by
commandPaletteImmediateForkExecutionSnapshot: create a fallback
SessionRestorableAgentSnapshot with kind .opencode and an
AgentLaunchCommandSnapshot whose launcher is "omo" (matching the
supportedWithoutProbe path), call
ContentView.commandPaletteImmediateForkExecutionSnapshot with that fallback and
nil cachedSnapshot, then XCTAssertEqual(snapshot?.sessionId,
fallback.sessionId); mirror naming and setup from
testImmediateForkExecutionUsesProbeFreeFallbackSnapshotBeforeProbeVerification
and ensure the test covers the probe-free OpenCode branch.
In `@Sources/ContentView`+ForkAgentConversation.swift:
- Around line 44-55: The fallback snapshot must come from the workspace helper
to ensure consistent snapshot source; replace the use of
currentContext.workspace.restoredAgentSnapshotsByPanelId[panelId] with the
helper currentContext.workspace.forkableAgentSnapshot(forPanelId: panelId) so
that Self.commandPaletteImmediateForkExecutionSnapshotSelection(...) receives
the helper-provided fallbackSnapshot; update the variable assignment for
fallbackSnapshot (and any related usages in this block) to call
forkableAgentSnapshot(forPanelId:) instead of indexing
restoredAgentSnapshotsByPanelId to avoid mismatched visibility/execution
sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 482ddaed-0106-4e06-94c0-3bd17f046d2b
📒 Files selected for processing (4)
Sources/ContentView+ForkAgentConversation.swiftSources/ContentView.swiftSources/Workspace.swiftcmuxTests/CommandPaletteSearchEngineTests.swift
Greptile SummaryFixes Cmd+Shift+P sometimes hiding fork commands while the right-click tab menu still offered "Fork Conversation" by aligning the command palette's visibility and execution path with
Confidence Score: 4/5Safe to merge with awareness that the fallback snapshot remains live until panel close even after the forked agent dies. The core logic change is well-tested: new unit tests cover chained forks, startup-binding persistence, probe-free visibility, and the probe-required gate. The three-tier lookup is symmetric with the right-click path. The main open concern is that Sources/Workspace.swift — Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CmdPalette as Command Palette
participant CView as ContentView
participant WS as Workspace
participant LiveIndex as SharedLiveAgentIndex
User->>CmdPalette: Open (Cmd+Shift+P)
CmdPalette->>CView: refreshCommandPaletteForkableAgentAvailabilityIfNeeded
CView->>WS: forkableAgentSnapshot(forPanelId:)
WS-->>CView: 1. restoredAgentSnapshotsByPanelId
WS-->>CView: 2. SharedLiveAgentIndex.snapshot()
WS-->>CView: 3. forkedAgentFallbackSnapshotsByPanelId
CView->>CView: commandPaletteSnapshotForkAvailability()
alt supportedWithoutProbe (Codex / remote OpenCode)
CView->>CView: Insert panelKey immediately
CView->>CView: Seed cache with fallback snapshot
CView-->>CmdPalette: Fork commands visible NOW
CView->>CView: Start background probe (upgrades cache)
else requiresProbe (local OpenCode)
CView->>CView: Wait for probe result
CView-->>CmdPalette: Fork commands hidden until probe succeeds
end
User->>CmdPalette: Execute Fork Conversation
CmdPalette->>WS: forkAgentConversation / forkAgentConversationToNewTab
WS->>WS: Create new panel (newPanelId)
WS->>WS: recordForkedAgentFallbackSnapshot(snapshot, panelId: newPanelId)
WS-->>User: New forked panel (agent launching)
Note over WS,LiveIndex: Agent registers with SharedLiveAgentIndex
LiveIndex-->>WS: objectWillChange fires
WS->>WS: forkableAgentSnapshot() returns live snapshot (tier 2)
Reviews (2): Last reviewed commit: "Persist fork startup binding for restore" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 17727-17741: The forkedAgentStartupBinding function currently
forces autoResume: true when creating a SurfaceResumeBindingSnapshot; instead
preserve the saved policy by using the auto-resume flag from the
SessionRestorableAgentSnapshot (e.g. replace autoResume: true with autoResume:
snapshot.autoResume or the appropriate saved property on
SessionRestorableAgentSnapshot), so the synthesized fork binding respects the
original panel/user auto-resume setting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12861c37-5bdb-43f9-97e4-b9ffea794272
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceUnitTests.swift
| if let binding = forkedAgentStartupBinding(snapshot) { | ||
| surfaceResumeBindingsByPanelId[panelId] = binding | ||
| } | ||
| } | ||
|
|
||
| private func forkedAgentStartupBinding(_ snapshot: SessionRestorableAgentSnapshot) -> SurfaceResumeBindingSnapshot? { | ||
| guard let command = snapshot.forkCommand else { return nil } | ||
| return SurfaceResumeBindingSnapshot( | ||
| name: snapshot.agentDisplayName, | ||
| kind: snapshot.kind.rawValue, | ||
| command: command, | ||
| cwd: snapshot.workingDirectory, | ||
| checkpointId: snapshot.sessionId, | ||
| source: "agent-hook", | ||
| autoResume: true |
There was a problem hiding this comment.
Do not force forked panels into auto-resume.
Line 17741 hardcodes autoResume: true for every synthesized fork startup binding. Because this binding is now persisted for restore, any forked panel will relaunch on next app start even when the source panel or user setting had agent auto-resume disabled. Preserve the existing auto-resume policy instead of unconditionally opting in here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/Workspace.swift` around lines 17727 - 17741, The
forkedAgentStartupBinding function currently forces autoResume: true when
creating a SurfaceResumeBindingSnapshot; instead preserve the saved policy by
using the auto-resume flag from the SessionRestorableAgentSnapshot (e.g. replace
autoResume: true with autoResume: snapshot.autoResume or the appropriate saved
property on SessionRestorableAgentSnapshot), so the synthesized fork binding
respects the original panel/user auto-resume setting.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 87933a1. Configure here.
| if let cachedSnapshot = verifiedCachedSnapshot(expectedFingerprint: fallbackFingerprint) { | ||
| ) | ||
| if probeResultMatches, | ||
| let cachedSnapshot = verifiedCachedSnapshot(expectedFingerprint: fallbackFingerprint) { |
There was a problem hiding this comment.
Seeded fallback misreported as verified cached snapshot
Low Severity
In refreshCommandPaletteForkableAgentAvailabilityIfNeeded, the .supportedWithoutProbe case now pre-seeds the cache with the fallback snapshot (inserting panelKey into supportedPanelKeys and setting snapshotsByPanelKey/snapshotFingerprintsByPanelKey). When commandPaletteImmediateForkExecutionSnapshotSelection later runs, probeResultMatches returns true against this pre-seeded data, and verifiedCachedSnapshot finds the seeded fallback, returning it with usedFallbackSnapshot: false. In forkFocusedAgentConversation, this incorrect flag is written to resultHadFallbackByPanelKey, which can prevent future probe re-triggering if the background probe is cancelled before completion (e.g., on palette dismissal).
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 87933a1. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="cmuxTests/WorkspaceUnitTests.swift">
<violation number="1" location="cmuxTests/WorkspaceUnitTests.swift:5786">
P2: Codex hook-state behavior is tested without isolating `CMUX_AGENT_HOOK_STATE_DIR`, so this test can leak into real user hook-state files and become non-hermetic.
(Based on your team's feedback about isolating Codex hook state in tests.) [FEEDBACK_USED].</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| XCTAssertEqual(binding.command, snapshot.forkCommand) | ||
|
|
||
| let restored = Workspace() | ||
| restored.restoreSessionSnapshot(savedSnapshot) |
There was a problem hiding this comment.
P2: Codex hook-state behavior is tested without isolating CMUX_AGENT_HOOK_STATE_DIR, so this test can leak into real user hook-state files and become non-hermetic.
(Based on your team's feedback about isolating Codex hook state in tests.) .
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/WorkspaceUnitTests.swift, line 5786:
<comment>Codex hook-state behavior is tested without isolating `CMUX_AGENT_HOOK_STATE_DIR`, so this test can leak into real user hook-state files and become non-hermetic.
(Based on your team's feedback about isolating Codex hook state in tests.) .</comment>
<file context>
@@ -5739,6 +5739,65 @@ final class WorkspacePanelGitBranchTests: XCTestCase {
+ XCTAssertEqual(binding.command, snapshot.forkCommand)
+
+ let restored = Workspace()
+ restored.restoreSessionSnapshot(savedSnapshot)
+ let restoredPanelId = try XCTUnwrap(restored.focusedPanelId)
+ let restoredPanel = try XCTUnwrap(restored.terminalPanel(for: restoredPanelId))
</file context>
CodeRabbit now posts non-blocking comment reviews (request_changes_workflow=false, #5538).
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Issues
Note
Medium Risk
Touches fork availability, immediate execution, and session resume bindings across palette and workspace lifecycle; probe gating for local OpenCode is preserved but behavior changes for other agent types.
Overview
Fixes Cmd+Shift+P sometimes hiding fork commands while the tab context menu still offered them by routing palette visibility and execution through
workspace.forkableAgentSnapshot(forPanelId:)(restored → live index → per-panel fork fallback cache) instead of only restored snapshots.Probe-free agents (e.g. Codex, remote OpenCode,
omo) now show in the palette and can fork immediately; the palette seeds its cache from the fallback while a background probe still runs. Probe-required local direct OpenCode stays gated until verification succeeds.After any fork (split, new tab, or new workspace), the app records the fork snapshot on the new panel and writes an auto-resume surface binding from the fork command so chained forks and session restore work before a live agent scan lands. Fallback entries are cleared on panel close and session reset.
Reviewed by Cursor Bugbot for commit 87933a1. Bugbot is set up for automated code reviews on this repo. Configure here.
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Aligns the command palette with the tab context menu so fork commands appear and run when a probe‑free forkable snapshot exists. Also persists fork startup bindings so restored sessions auto‑resume and can be forked again immediately.
workspace.forkableAgentSnapshot(forPanelId:)to decide palette visibility and selection, and persist the fork snapshot and startup binding in new panels so they can fork again right away and auto‑resume on restore.OpenCode), with new regression tests covering visibility, immediate execution paths, chained forks, and restore auto‑resume.Written for commit 87933a1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests