Repository navigation
Fix workspace creation snapshot crash from custom shortcut - #2170
Closed
lawrencecchen wants to merge 2 commits into
Closed
lawrencecchen wants to merge 2 commits into
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughRefactored workspace creation to propagate only inherited font size ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 470-500: The test keeps C buffers alive until deinit which doesn't
exercise the snapshot lifetime; after the snapshot is captured in
didCaptureWorkspaceCreationSnapshot(), immediately invalidate the injected C
config created by installInjectedConfig by freeing all retainedCStringPointers
(free each pointer), deinitializing and deallocating retainedEnvVars, and
zeroing or clearing injectedConfig (and set retainedCStringPointers and
retainedEnvVars to nil/empty) so makeWorkspaceForCreation must not rely on the
original buffers remaining valid.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ee3156f-6be8-4e0b-adc6-0af587494c0e
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
Comment on lines
+470
to
+500
| 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 |
There was a problem hiding this comment.
Invalidate the injected C config immediately after snapshot capture.
This regression is aimed at a snapshot-lifetime crash, but the test keeps working_directory / command / env_vars alive until deinit. As written, it only proves makeWorkspaceForCreation receives a sanitized template; it can still pass if workspaceCreationSnapshot() regresses to retaining the raw ghostty_surface_config_s and sanitizes later. Freeing or clearing the injected buffers in didCaptureWorkspaceCreationSnapshot() would exercise the actual lifetime boundary that caused the crash.
🧪 Tighten the regression
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?
+ private func releaseInjectedConfig() {
+ injectedConfig = nil
+ retainedEnvVars?.deinitialize(count: 1)
+ retainedEnvVars?.deallocate()
+ retainedEnvVars = nil
+ for pointer in retainedCStringPointers {
+ free(pointer)
+ }
+ retainedCStringPointers.removeAll()
+ }
+
deinit {
- retainedEnvVars?.deinitialize(count: 1)
- retainedEnvVars?.deallocate()
- for pointer in retainedCStringPointers {
- free(pointer)
- }
+ releaseInjectedConfig()
}
+
+ override func didCaptureWorkspaceCreationSnapshot() {
+ releaseInjectedConfig()
+ }Also applies to: 529-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/WorkspaceUnitTests.swift` around lines 470 - 500, The test keeps C
buffers alive until deinit which doesn't exercise the snapshot lifetime; after
the snapshot is captured in didCaptureWorkspaceCreationSnapshot(), immediately
invalidate the injected C config created by installInjectedConfig by freeing all
retainedCStringPointers (free each pointer), deinitializing and deallocating
retainedEnvVars, and zeroing or clearing injectedConfig (and set
retainedCStringPointers and retainedEnvVars to nil/empty) so
makeWorkspaceForCreation must not rely on the original buffers remaining valid.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag feat-workspace-creation-snapshot-crash(BUILD SUCCEEDEDfor the tagged app bundle; the script still exits later in the standalone Zigcmuxdstep on this macOS 26 host)CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-feat-workspace-creation-snapshot-crash-test-skipzig -only-testing:cmuxTests/WorkspaceCreationConfigSanitizationTests/testAddWorkspacePassesSanitizedInheritedConfigTemplate test(hostedcmux DEVcrashed during startup before the test connected, so this remains an existing test-harness blocker)Issues
TabManager.workspaceCreationSnapshot()EXC_BAD_ACCESS while handling the custom add-workspace shortcutSummary by cubic
Fixes a crash when creating a workspace via a custom shortcut by sanitizing the inherited terminal config. We now only carry the font size and rebuild a fresh
ghostty_surface_config_sto avoid dangling pointers.ghostty_surface_config_sfor new workspaces, preventing EXC_BAD_ACCESS from pointer-backed fields.font_sizeis inherited and all pointer fields are nil/zero.Written for commit cb48f5a. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests