Repository navigation
Fix Release-only Cmd+N workspace snapshot UAF - #2181
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCapture Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Line 1240: Replace the bare title literal used where the workspace/tab is
created (the title: "Terminal \(nextTabCount)" occurrence) with a localized
string lookup using String(localized: ...) and format the number, e.g. use
String(localized: "workspace.title.default", defaultValue: "Terminal
%d").formatted(nextTabCount) (referencing the existing nextTabCount and the
title: parameter), and add the key "workspace.title.default" with the English
default "Terminal %d" to your Localizable.xcstrings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a107ebdd-63ba-487f-9d05-4692a3639c50
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
| let ordinal = Self.nextPortOrdinal | ||
| Self.nextPortOrdinal += 1 | ||
| let newWorkspace = makeWorkspaceForCreation( | ||
| title: "Terminal \(nextTabCount)", |
There was a problem hiding this comment.
Localize the default workspace title at Line 1240.
"Terminal \(nextTabCount)" is user-visible and should use a localization key.
🌐 Proposed fix
- title: "Terminal \(nextTabCount)",
+ title: String(
+ localized: "workspace.title.default",
+ defaultValue: "Terminal \(nextTabCount)"
+ ),Also add workspace.title.default to Resources/Localizable.xcstrings.
As per coding guidelines: “All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") … Never use bare string literals … or other UI elements”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` at line 1240, Replace the bare title literal used
where the workspace/tab is created (the title: "Terminal \(nextTabCount)"
occurrence) with a localized string lookup using String(localized: ...) and
format the number, e.g. use String(localized: "workspace.title.default",
defaultValue: "Terminal %d").formatted(nextTabCount) (referencing the existing
nextTabCount and the title: parameter), and add the key
"workspace.title.default" with the English default "Terminal %d" to your
Localizable.xcstrings.
Greptile SummaryThis PR fixes a Release-mode use-after-free crash triggered by the Cmd+N workspace creation path: the Swift ARC optimizer could release Confidence Score: 4/5Production fix is correct and well-reasoned; the regression test has a compile error that blocks the test target build but does not affect app behavior. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant TabManager
participant ARC as Swift ARC (Release)
participant Workspace
Caller->>TabManager: addWorkspace(placementOverride:)
TabManager->>TabManager: capturedTabs = tabs (strong copy)
TabManager->>TabManager: capturedSelectedTabId = selectedTabId
Note over TabManager,ARC: withExtendedLifetime(capturedTabs) begins — ARC blocked from releasing array
TabManager->>TabManager: workspaceCreationSnapshot(currentTabs: capturedTabs, ...)
TabManager->>TabManager: didCaptureWorkspaceCreationSnapshot()
Note over Workspace: (mid-creation: external close may remove workspace from tabs)
ARC--xWorkspace: [blocked] early release of captured Workspace prevented
TabManager->>TabManager: makeWorkspaceForCreation(...)
TabManager->>TabManager: var updatedTabs = tabs (live, post-close)
TabManager->>TabManager: updatedTabs.insert(newWorkspace, at: insertIndex)
TabManager->>TabManager: tabs = updatedTabs
Note over TabManager,ARC: withExtendedLifetime ends — capturedTabs released
ARC->>Workspace: release captured Workspace (ref count may drop to 0)
TabManager-->>Caller: return newWorkspace
Reviews (1): Last reviewed commit: "fix: retain snapshot workspaces through ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard let closingWorkspace else { | ||
| XCTFail("Expected secondary workspace") | ||
| return | ||
| } | ||
|
|
||
| let closingWorkspaceId = closingWorkspace.id | ||
| weak var weakClosingWorkspace = closingWorkspace | ||
| XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id]) | ||
| closingWorkspace = nil |
There was a problem hiding this comment.
guard let shadow makes closingWorkspace = nil a compile error
After guard let closingWorkspace (SE-0345 shorthand) on line 462, the name closingWorkspace in the continuation scope refers to a new non-optional let constant (Workspace, not Workspace?), shadowing the original var Workspace? declared on line 458. closingWorkspace = nil on line 470 therefore fails to compile on two counts: the binding is immutable (let) and the type doesn't accept nil.
Because the assignment cannot compile, the test target would not build, meaning the regression test can never actually run — and the intended strong-reference drop never happens anyway, so weakClosingWorkspace would remain non-nil through the whole function regardless of withExtendedLifetime.
The simplest fix is to avoid the shorthand guard and nil-out the original var directly:
XCTAssertNotNil(closingWorkspace, "Expected secondary workspace")
guard closingWorkspace != nil else { return }
let closingWorkspaceId = closingWorkspace!.id
weak var weakClosingWorkspace = closingWorkspace
XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id])
closingWorkspace = nil // drops the local strong ref; original var is still Workspace?Or use a distinct holder name so both the unwrapped let and the nullable var coexist:
var closingWorkspaceHolder: Workspace? = manager.addWorkspace()
let third = manager.addWorkspace()
manager.selectWorkspace(third)
guard let closingWorkspace = closingWorkspaceHolder else {
XCTFail("Expected secondary workspace")
return
}
let closingWorkspaceId = closingWorkspace.id
weak var weakClosingWorkspace: Workspace? = closingWorkspace
XCTAssertEqual(manager.tabs.map(\.id), [first.id, closingWorkspaceId, third.id])
closingWorkspaceHolder = nil // drop the holder's strong ref
Ingests all upstream fixes since 2026-03-22 including: - Fix Cmd+N crash: retain snapshot workspaces (manaflow-ai#2183, manaflow-ai#2181, manaflow-ai#2178, manaflow-ai#2173) - Fix browser pane restore after reopen (manaflow-ai#2141) - Fix Ghostty resize_split keybind (manaflow-ai#1899) - Reduce shell integration prompt latency (manaflow-ai#2109) - Fix command palette focus after terminal find (manaflow-ai#2089) - Add Codex CLI hooks (manaflow-ai#2103) - Add cmux.json custom commands (manaflow-ai#2011) - Fix window position restore on relaunch (manaflow-ai#2129) Conflict resolution: - BrowserPanel.swift: accepted upstream configureWebViewConfiguration() refactor (already includes our forMainFrameOnly:true CAPTCHA fix from PR manaflow-ai#1877) Fork-specific files preserved: - Sources/Panels/WebAuthn{Coordinator,BridgeJavaScript}.swift - Sources/FIDO2/module.modulemap - vendor/ctap2 submodule - cmux.entitlements (with camera/audio-input removed) - cmux.embedded.entitlements - .github/workflows/fork-{ci,release}.yml
* test: reproduce Cmd+N snapshot workspace lifetime race * fix: retain snapshot workspaces through Cmd+N creation * fix: repair workspace lifetime regression test
This branch was successfully deployed
1 participant
Closes #2180
Summary
tabsarray alive for the fulladdWorkspace()path so Release ARC cannot drop capturedWorkspacereferences before insertionTesting
./scripts/reload.sh --tag cmd-n-retain-fix --launch./scripts/reloads.sh --tag cmd-n-retain-fixSummary by CodeRabbit
Bug Fixes
Tests