Repository navigation
Fix workspace creation snapshot crash - #2176
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughTab snapshots are refactored to capture only Changes
Possibly related PRs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Greptile SummaryThis PR fixes a crash during workspace creation by stopping the full
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant AM as addWorkspace()
participant S as workspaceCreationSnapshot()
participant H as inheritedTerminalFontPointsForNewWorkspace()
participant IC as inheritedTerminalConfigForNewWorkspace()
participant Seam as didCaptureWorkspaceCreationSnapshot()
participant T as workspaceCreationConfigTemplate()
participant MWC as makeWorkspaceForCreation()
AM->>S: capture snapshot
S->>H: get font size
H->>IC: get full config (live surface or fallback)
IC-->>H: ghostty_surface_config_s (may have C pointers)
H-->>S: font_size: Float? (only scalar extracted)
S-->>AM: snapshot { inheritedTerminalFontPoints: Float? }
AM->>Seam: (test seam — source surface buffers may now be freed)
AM->>T: workspaceCreationConfigTemplate(fontPoints)
T-->>AM: ghostty_surface_config_new() + font_size only (no C pointers)
AM->>MWC: configTemplate (clean, pointer-safe)
Reviews (1): Last reviewed commit: "Sanitize workspace creation config snaps..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func inheritedTerminalFontPointsForNewWorkspace( | ||
| workspace: Workspace? | ||
| ) -> Float? { | ||
| guard let inheritedConfig = inheritedTerminalConfigForNewWorkspace(workspace: workspace), | ||
| inheritedConfig.font_size > 0 else { | ||
| return nil | ||
| } | ||
| return inheritedConfig.font_size | ||
| } | ||
|
|
||
| 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 | ||
| } |
There was a problem hiding this comment.
internal
inheritedTerminalFontPointsForNewWorkspace and workspaceCreationConfigTemplate are both declared func (internal visibility) but neither is overridden anywhere — the test subclass only overrides inheritedTerminalConfigForNewWorkspace (which does need to be internal for the @testable import override). These two helpers could be private func to keep the internal surface of TabManager minimal.
| func inheritedTerminalFontPointsForNewWorkspace( | |
| workspace: Workspace? | |
| ) -> Float? { | |
| guard let inheritedConfig = inheritedTerminalConfigForNewWorkspace(workspace: workspace), | |
| inheritedConfig.font_size > 0 else { | |
| return nil | |
| } | |
| return inheritedConfig.font_size | |
| } | |
| 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 | |
| } | |
| 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 | |
| } |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/TabManager.swift (1)
2286-2298: Consider explicit initialization for defensive clarity, though the code is currently safe.The test
testAddWorkspacePassesSanitizedInheritedConfigTemplateAfterSourceBuffersAreReleasedconfirms thatghostty_surface_config_new()properly initializes pointer-backed fields (working_directory,command,env_vars,env_var_count) to nil/0. Additionally,contextis always set atGhosttyTerminalView.swift:3501beforeghostty_surface_new()is called, so relying on downstream assignment is safe. Theinitial_inputfield is not used anywhere in the codebase.That said, explicitly nil-ing these fields in the template would be more defensive and clarify intent, especially if Ghostty's initialization contract ever changes. If you add these assignments, extend the regression test to cover
initial_inputandcontextfor consistency (the latter should beGHOSTTY_SURFACE_CONTEXT_UNINITIALIZEDor equivalent before downstream assignment).🤖 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 workspaceCreationConfigTemplate currently returns a config from ghostty_surface_config_new() but should explicitly clear pointer-backed fields for defensive clarity: after creating var config = ghostty_surface_config_new() set config.working_directory = nil, config.command = nil, config.env_vars = nil, config.env_var_count = 0 and also explicitly clear config.initial_input and set config.context = GHOSTTY_SURFACE_CONTEXT_UNINITIALIZED (or the equivalent sentinel) before setting config.font_size and returning; then extend the test testAddWorkspacePassesSanitizedInheritedConfigTemplateAfterSourceBuffersAreReleased to assert that initial_input and context are sanitized in addition to the other pointer-backed fields.cmuxTests/WorkspaceUnitTests.swift (2)
559-577: Run the captured template through the real creation path.Line 574 replaces
configTemplatewithnil, so this regression never executes the workspace-creation code that used to consume the inherited snapshot. Capturing the argument is useful, but I’d still forward the same template intosuper.makeWorkspaceForCreation(...)so a stale-pointer regression fails on the actual runtime path, not only in the intercepted copy.♻️ Suggested change
override func makeWorkspaceForCreation( title: String, workingDirectory: String?, portOrdinal: Int, configTemplate: ghostty_surface_config_s?, initialTerminalCommand: String?, initialTerminalEnvironment: [String: String] ) -> Workspace { capturedConfigTemplate = configTemplate // The assertion is on the captured template; avoid dereferencing any injected // pointer-backed fields here so the test can safely detect unsanitized state. return super.makeWorkspaceForCreation( title: title, workingDirectory: workingDirectory, portOrdinal: portOrdinal, - configTemplate: nil, + configTemplate: configTemplate, initialTerminalCommand: initialTerminalCommand, initialTerminalEnvironment: initialTerminalEnvironment ) }Based on learnings, "Do not add tests that only verify source code text, method signatures, AST fragments, or grep-style patterns. Tests must verify observable runtime behavior through executable paths (unit/integration/e2e/CLI), not implementation shape."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 559 - 577, The override of makeWorkspaceForCreation captures configTemplate into capturedConfigTemplate but then passes nil to super, preventing the real creation path from exercising the original template; modify the override (makeWorkspaceForCreation) to forward the captured configTemplate (the configTemplate parameter) into super.makeWorkspaceForCreation instead of nil so the inherited workspace-creation logic runs with the real snapshot while still preserving capturedConfigTemplate for assertions.
516-520: Add a fallback cleanup for the malloc-backed fixture.If this helper exits before
didCaptureWorkspaceCreationSnapshot()runs, thestrdup/allocatebuffers never get released. A smalldeinitguard makes the test fixture safe for early-failure paths too.🧹 Suggested cleanup
private final class UnsafeConfigSnapshotTabManager: TabManager { + deinit { + invalidateInjectedConfig() + } + 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?Also applies to: 580-591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/WorkspaceUnitTests.swift` around lines 516 - 520, UnsafeConfigSnapshotTabManager currently retains malloc/strdup-backed buffers (retainedCStringPointers, retainedEnvVars, and possibly members of injectedConfig) that are only freed when didCaptureWorkspaceCreationSnapshot() runs; add a deinit to safely free these on early exit: iterate retainedCStringPointers and free() each pointer and clear the array, deallocate retainedEnvVars if non-nil, and release any malloced fields inside injectedConfig (e.g., C string pointers) before dropping it so the fixture doesn't leak if the helper exits early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmuxTests/WorkspaceUnitTests.swift`:
- Around line 559-577: The override of makeWorkspaceForCreation captures
configTemplate into capturedConfigTemplate but then passes nil to super,
preventing the real creation path from exercising the original template; modify
the override (makeWorkspaceForCreation) to forward the captured configTemplate
(the configTemplate parameter) into super.makeWorkspaceForCreation instead of
nil so the inherited workspace-creation logic runs with the real snapshot while
still preserving capturedConfigTemplate for assertions.
- Around line 516-520: UnsafeConfigSnapshotTabManager currently retains
malloc/strdup-backed buffers (retainedCStringPointers, retainedEnvVars, and
possibly members of injectedConfig) that are only freed when
didCaptureWorkspaceCreationSnapshot() runs; add a deinit to safely free these on
early exit: iterate retainedCStringPointers and free() each pointer and clear
the array, deallocate retainedEnvVars if non-nil, and release any malloced
fields inside injectedConfig (e.g., C string pointers) before dropping it so the
fixture doesn't leak if the helper exits early.
In `@Sources/TabManager.swift`:
- Around line 2286-2298: The workspaceCreationConfigTemplate currently returns a
config from ghostty_surface_config_new() but should explicitly clear
pointer-backed fields for defensive clarity: after creating var config =
ghostty_surface_config_new() set config.working_directory = nil, config.command
= nil, config.env_vars = nil, config.env_var_count = 0 and also explicitly clear
config.initial_input and set config.context =
GHOSTTY_SURFACE_CONTEXT_UNINITIALIZED (or the equivalent sentinel) before
setting config.font_size and returning; then extend the test
testAddWorkspacePassesSanitizedInheritedConfigTemplateAfterSourceBuffersAreReleased
to assert that initial_input and context are sanitized in addition to the other
pointer-backed fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6eab8a26-4451-4c3f-8629-f2ab3f5cb094
📒 Files selected for processing (2)
Sources/TabManager.swiftcmuxTests/WorkspaceUnitTests.swift
…creation-snapshot-crash # Conflicts: # Sources/TabManager.swift
Main already has this test class; the branch's version was a duplicate that would cause a compilation error after merge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* Add workspace creation config regression test * Sanitize workspace creation config snapshot * Remove duplicate WorkspaceCreationConfigSanitizationTests class Main already has this test class; the branch's version was a duplicate that would cause a compilation error after merge. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com> Co-authored-by: austinpower1258 <austinwang115@gmail.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
ghostty_surface_config_sacross workspace creationWhy
This follows up on #2157 after earlier attempts still left workspace creation holding onto pointer-backed config data from an existing surface.
Testing
xcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-issue-2157-build-for-testing-final build-for-testing./scripts/reload.sh --tag issue-2157-workspace-creation-snapshot-crashNotes
cmux-unitruntime execution is currently blocked by an unrelated host app bootstrap crash on this machine, including for unchanged baseline tests.Summary by CodeRabbit
Refactor
Tests