Skip to content

Fix Cmd+N crash from workspace creation config snapshots #2178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
austinywang merged 2 commits into from
Mar 26, 2026
Merged

Fix Cmd+N crash from workspace creation config snapshots #2178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2ea997
Add workspace config sanitization regression test
austinywang Mar 26, 2026
a00e617
Sanitize workspace creation config snapshots
lawrencecchen Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions Sources/TabManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ class TabManager: ObservableObject {
let selectedTabId: UUID?
let selectedTabWasPinned: Bool
let preferredWorkingDirectory: String?
let inheritedTerminalConfig: ghostty_surface_config_s?
let inheritedTerminalFontPoints: Float?
}
private var agentPIDSweepTimer: DispatchSourceTimer?
private var workspaceGitMetadataPollTimer: DispatchSourceTimer?
Expand Down Expand Up @@ -1218,7 +1218,9 @@ class TabManager: ObservableObject {
sentryBreadcrumb("workspace.create", data: ["tabCount": nextTabCount])
let explicitWorkingDirectory = normalizedWorkingDirectory(overrideWorkingDirectory)
let workingDirectory = explicitWorkingDirectory ?? snapshot.preferredWorkingDirectory
let inheritedConfig = snapshot.inheritedTerminalConfig
let inheritedConfig = workspaceCreationConfigTemplate(
inheritedTerminalFontPoints: snapshot.inheritedTerminalFontPoints
)
// Resolve placement against the pre-creation snapshot before Workspace init
// boots terminal state. The ssh/new-workspace path can otherwise crash while
// reading @Published placement state from existing workspaces mid-creation.
Expand Down Expand Up @@ -2191,7 +2193,7 @@ class TabManager: ObservableObject {
selectedTabId: currentSelectedTabId,
selectedTabWasPinned: selectedTabSnapshot?.isPinned ?? false,
preferredWorkingDirectory: preferredWorkingDirectoryForNewTab(workspace: selectedWorkspace),
inheritedTerminalConfig: inheritedTerminalConfigForNewWorkspace(workspace: selectedWorkspace)
inheritedTerminalFontPoints: inheritedTerminalFontPointsForNewWorkspace(workspace: selectedWorkspace)
)
}

Expand Down Expand Up @@ -2251,7 +2253,7 @@ class TabManager: ObservableObject {
inheritedTerminalConfigForNewWorkspace(workspace: selectedWorkspace)
}

private func inheritedTerminalConfigForNewWorkspace(
func inheritedTerminalConfigForNewWorkspace(
workspace: Workspace?
) -> ghostty_surface_config_s? {
Comment on lines +2256 to 2258

@greptile-apps greptile-apps Bot Mar 26, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 private widened to internal for test subclassing

inheritedTerminalConfigForNewWorkspace(workspace:) is now internal (no explicit access modifier) so UnsafeConfigSnapshotTabManager in the test target can override it. This is a pragmatic and functional approach given the test harness, but it permanently exposes this method to all callers within the module, not just the test.

An alternative that keeps the method private and avoids a permanent API surface increase is injecting the config source as a closure or a small protocol:

// In TabManager, keep private:
private var inheritedConfigProvider: (Workspace?) -> ghostty_surface_config_s? = { [weak self] in
    self?.inheritedTerminalConfigForNewWorkspace(workspace: $0)
}

The test then just swaps out manager.inheritedConfigProvider instead of subclassing. Not a blocker — the current approach is correct and testable — but worth considering if the codebase discourages widening private to internal solely for tests.

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!

if let panel = terminalPanelForWorkspaceConfigInheritanceSource(workspace: workspace),
Expand All @@ -2271,6 +2273,30 @@ class TabManager: ObservableObject {
return nil
}

private func inheritedTerminalFontPointsForNewWorkspace(
workspace: Workspace?
) -> Float? {
guard let inheritedConfig = inheritedTerminalConfigForNewWorkspace(workspace: workspace),
inheritedConfig.font_size > 0 else {
return nil
}
return inheritedConfig.font_size
}

private func workspaceCreationConfigTemplate(
inheritedTerminalFontPoints: Float?
) -> ghostty_surface_config_s? {
guard let inheritedTerminalFontPoints, inheritedTerminalFontPoints > 0 else {
return nil
}
// ghostty_surface_config_s can carry raw C pointers owned by the source surface.
// New workspace creation only needs the inherited zoom level, so rebuild a clean
// config instead of snapshotting pointer-backed fields across workspace creation.
var config = ghostty_surface_config_new()
config.font_size = inheritedTerminalFontPoints
return config
}
Comment on lines +2286 to +2298

@coderabbitai coderabbitai Bot Mar 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Check if TabManager.swift exists and read the specified lines
if [ -f "Sources/TabManager.swift" ]; then
  wc -l Sources/TabManager.swift
  echo "---"
  sed -n '2276,2298p' Sources/TabManager.swift
else
  # Try to locate TabManager.swift
  find . -name "TabManager.swift" -type f
fi

Repository: manaflow-ai/cmux

Length of output: 1086

🏁 Script executed:

# Search for ghostty_surface_config definition and usage
rg "ghostty_surface_config" -A 3 -B 1 --max-count 20

Repository: manaflow-ai/cmux

Length of output: 9223

🏁 Script executed:

# Look for the Ghostty header file in the repo
find . -name "ghostty.h" -o -name "*ghostty*.h" | head -20

Repository: manaflow-ai/cmux

Length of output: 71

🏁 Script executed:

# Read the ghostty.h header to understand the struct and constructor
cat -n ghostty.h | grep -A 40 "ghostty_surface_config_s" | head -60

Repository: manaflow-ai/cmux

Length of output: 2266

🏁 Script executed:

# Look for the implementation or documentation of ghostty_surface_config_new()
# Check if there's a build directory or any implementation hints
rg "ghostty_surface_config_new" -B 3 -A 10

Repository: manaflow-ai/cmux

Length of output: 6034

🏁 Script executed:

# Check how the config is actually used across the codebase to understand the risk
rg "ghostty_surface_config_new\(\)" -A 5 | head -80

Repository: manaflow-ai/cmux

Length of output: 2722

🏁 Script executed:

# Search for any comments or documentation about field initialization
rg "zero.init|zero-init|zero.initialize" -i -C 2

Repository: manaflow-ai/cmux

Length of output: 1237

🏁 Script executed:

# Get the exact struct definition to see all pointer fields
sed -n '400,453p' ghostty.h | cat -n

Repository: manaflow-ai/cmux

Length of output: 1575

Explicitly clear uninitialized pointer fields before returning.

The Ghostty header doesn't document that ghostty_surface_config_new() zero-initializes all members, and C struct constructors across FFI boundaries don't guarantee initialization. Since this config is passed to ghostty_surface_new(), uninitialized pointer values could introduce undefined behavior. The test code (WorkspaceUnitTests.swift) explicitly sets all pointer fields after construction, establishing the expected pattern. Clear the fields you don't use.

Suggested hardening 
     var config = ghostty_surface_config_new()
+    config.working_directory = nil
+    config.command = nil
+    config.env_vars = nil
+    config.env_var_count = 0
+    config.initial_input = nil
     config.font_size = inheritedTerminalFontPoints
     return config
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Sources/TabManager.swift` around lines 2286 - 2298, The function
workspaceCreationConfigTemplate creates a ghostty_surface_config_s via
ghostty_surface_config_new() but currently only sets font_size, leaving other
pointer fields uninitialized; before returning from
workspaceCreationConfigTemplate, explicitly zero or nil out all
pointer/reference members of the ghostty_surface_config_s (the fields that could
hold C pointers) so the struct contains no indeterminate pointer values when
later passed to ghostty_surface_new(); update workspaceCreationConfigTemplate to
set those pointer fields to nil/0 (or otherwise clear them) after calling
ghostty_surface_config_new() and before returning the config.


private func normalizedWorkingDirectory(_ directory: String?) -> String? {
guard let directory else { return nil }
let normalized = normalizeDirectory(directory)
Expand Down
86 changes: 86 additions & 0 deletions cmuxTests/WorkspaceUnitTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,92 @@ final class WorkspaceCreationPlacementTests: XCTestCase {
}
}

@MainActor
final class WorkspaceCreationConfigSanitizationTests: XCTestCase {
private final class UnsafeConfigSnapshotTabManager: TabManager {
private var retainedCStringPointers: [UnsafeMutablePointer<CChar>] = []
private var retainedEnvVars: UnsafeMutablePointer<ghostty_env_var_s>?
private var injectedConfig: ghostty_surface_config_s?
var capturedConfigTemplate: ghostty_surface_config_s?

deinit {
retainedEnvVars?.deinitialize(count: 1)
retainedEnvVars?.deallocate()
for pointer in retainedCStringPointers {
free(pointer)
}
}

func installInjectedConfig(fontSize: Float) {
let workingDirectory = strdup("/tmp/cmux-workspace-snapshot")
let command = strdup("echo snapshot")
let envKey = strdup("CMUX_INHERITED_ENV")
let envValue = strdup("1")
let envVars = UnsafeMutablePointer<ghostty_env_var_s>.allocate(capacity: 1)
envVars.initialize(
to: ghostty_env_var_s(
key: UnsafePointer(envKey),
value: UnsafePointer(envValue)
)
)

retainedCStringPointers = [workingDirectory, command, envKey, envValue].compactMap { $0 }
retainedEnvVars = envVars

var config = ghostty_surface_config_new()
config.font_size = fontSize
config.working_directory = UnsafePointer(workingDirectory)
config.command = UnsafePointer(command)
config.env_vars = envVars
config.env_var_count = 1
injectedConfig = config
}

override func inheritedTerminalConfigForNewWorkspace(
workspace: Workspace?
) -> ghostty_surface_config_s? {
injectedConfig ?? super.inheritedTerminalConfigForNewWorkspace(workspace: workspace)
}

override func makeWorkspaceForCreation(
title: String,
workingDirectory: String?,
portOrdinal: Int,
configTemplate: ghostty_surface_config_s?,
initialTerminalCommand: String?,
initialTerminalEnvironment: [String: String]
) -> Workspace {
capturedConfigTemplate = configTemplate
return super.makeWorkspaceForCreation(
title: title,
workingDirectory: workingDirectory,
portOrdinal: portOrdinal,
configTemplate: configTemplate,
initialTerminalCommand: initialTerminalCommand,
initialTerminalEnvironment: initialTerminalEnvironment
)
}
}

func testAddWorkspacePassesSanitizedInheritedConfigTemplate() {
let manager = UnsafeConfigSnapshotTabManager()
manager.installInjectedConfig(fontSize: 19)

_ = manager.addWorkspace()

guard let capturedConfig = manager.capturedConfigTemplate else {
XCTFail("Expected captured config template for new workspace")
return
}

XCTAssertEqual(capturedConfig.font_size, 19, accuracy: 0.001)
XCTAssertNil(capturedConfig.working_directory)
XCTAssertNil(capturedConfig.command)
XCTAssertNil(capturedConfig.env_vars)
XCTAssertEqual(capturedConfig.env_var_count, 0)
}
}


final class WorkspaceTabColorSettingsTests: XCTestCase {
func testNormalizedHexAcceptsAndNormalizesValidInput() {
Expand Down
Loading