Repository navigation
Fix ARC workspace inheritance crash and native Zig helper builds - #2283
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughWraps Workspace/TerminalSurface/TerminalPanel lifetimes with withExtendedLifetime during workspace creation and snapshotting to avoid ARC premature deallocation of pointer-backed surface data; switches inheritance to cached font-point extraction and rebuilds configs from that; the Zig build helper skips explicit -Dtarget for native-arch builds. (50 words) Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
Greptile SummaryThis PR fixes two distinct issues: (1) ARC-optimizer-induced use-after-free crashes when opening new workspaces under Xcode 16+ whole-module optimization, and (2) cross-linker failures when building the native Zig helper on macOS Tahoe. ARC lifetime fixes (Swift)
Native Zig build optimisation (shell)
Confidence Score: 5/5Safe to merge; the Swift ARC fixes are correct and the one shell-script concern is an edge case that degrades only to the pre-fix cross-compilation behaviour, never to a build failure. All P0/P1 findings are absent. The single comment is P2: a fat/universal zig binary (uncommon in practice — Homebrew and the Zig project both ship arch-specific binaries) would cause the native-compilation optimisation to silently not apply, reverting to cross-compilation. No crash, no data loss, no build failure. scripts/build-ghostty-cli-helper.sh — the ZIG_ARCH detection via grep|head -1 misidentifies the running arch for universal zig binaries. Important Files Changed
Sequence DiagramsequenceDiagram
participant TM as TabManager.addWorkspace
participant SW as selectedWorkspace (Workspace)
participant TP as TerminalPanel
participant TS as TerminalSurface
participant C as ghostty C API
TM->>SW: pin via withExtendedLifetime(sourceWorkspace)
TM->>TM: preferredWorkingDirectoryForNewTab(workspace:)
TM->>SW: read .currentDirectory / .panelDirectories
TM->>TM: inheritedTerminalFontPointsForNewWorkspace(workspace:)
TM->>TM: inheritedTerminalConfigForNewWorkspace(workspace:)
note over TM,SW: withExtendedLifetime(workspace)
TM->>TP: terminalPanelForWorkspaceConfigInheritanceSource
TM->>TS: pin surface = panel.surface
TM->>TS: read surface.surface → ghostty_surface_t*
TM->>C: cmuxInheritedSurfaceConfig(sourceSurface)
TM->>TS: withExtendedLifetime((panel, surface)) — keeps TS alive through C call
TM-->>TM: return CmuxSurfaceConfigTemplate
note over TM,SW: withExtendedLifetime block ends — SW may now be released
Reviews (1): Last reviewed commit: "Fix ARC workspace inheritance crash and ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ZIG_ARCH="$(file "$(command -v zig)" 2>/dev/null | grep -oE '(arm64|x86_64)' | head -1)" | ||
| case "$TARGET_TRIPLE" in | ||
| aarch64-macos) [[ "$ZIG_ARCH" == "arm64" ]] && TARGET_TRIPLE="" ;; | ||
| x86_64-macos) [[ "$ZIG_ARCH" == "x86_64" ]] && TARGET_TRIPLE="" ;; | ||
| esac |
There was a problem hiding this comment.
When zig is a universal binary, file outputs both architectures on the same line, e.g.:
/opt/homebrew/bin/zig: Mach-O universal binary with 2 architectures: [x86_64:...] [arm64:...]
grep -oE '(arm64|x86_64)' | head -1 will always return x86_64 because it appears first in that format. On an Apple Silicon host where zig would run natively as arm64, the native-compilation optimisation (clearing TARGET_TRIPLE for aarch64-macos) would silently never trigger, and the build would fall back to cross-compilation — defeating the purpose of this fix.
A more robust approach is to intersect the binary's slices with the host arch (uname -m), or to use lipo -archs to enumerate slices and pick the one that matches the current hardware:
# Prefer lipo so fat binaries are handled correctly
_ZIG_BIN="$(command -v zig)"
if [[ -x "$_ZIG_BIN" ]]; then
_SLICES="$(lipo -archs "$_ZIG_BIN" 2>/dev/null || true)"
_HOST_ARCH="$(uname -m)" # arm64 or x86_64 on the metal
if echo "$_SLICES" | grep -qw "$_HOST_ARCH"; then
ZIG_ARCH="$_HOST_ARCH"
else
ZIG_ARCH="$(echo "$_SLICES" | awk '{print $1}')"
fi
fiThe same concern applies to the duplicated detection in the --universal branch (line 135).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build-ghostty-cli-helper.sh`:
- Around line 82-90: The arch-detection code uses command -v zig and a grep
pipeline that can fail under set -euo pipefail, and it calls command -v zig
before the explicit existence check; fix by validating the zig binary once
before any use (capture command -v zig into a variable like zig_path and check
it is non-empty), and make the ZIG_ARCH assignment resilient to no-match by
ensuring the grep/file pipeline cannot cause a non-zero exit (e.g., use "command
-v zig || true" or capture file output into a variable and run grep with "||
true", or use awk to extract arm64|x86_64 safely), then set ZIG_ARCH to an empty
string when detection fails; apply the same robust pattern for both occurrences
of ZIG_ARCH and ensure TARGET_TRIPLE logic (case matching with TARGET_TRIPLE and
ZIG_ARCH) still runs after zig existence and arch detection.
In `@Sources/Workspace.swift`:
- Around line 7406-7408: The call to
rememberTerminalConfigInheritanceSource(terminalPanel) (and the subsequent if
config.fontSize > 0 { ... } block that reads terminalPanel.surface.surface and
calls cmuxCurrentSurfaceFontSizePoints()) is executed after the
withExtendedLifetime((terminalPanel, surface)) {} scope ends, risking ARC
deallocation and a use-after-free; move
rememberTerminalConfigInheritanceSource(terminalPanel) and the following
font-size handling into the body of withExtendedLifetime((terminalPanel,
surface)) { ... } so terminalPanel and surface remain pinned while their C
pointers are accessed.
🪄 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: 23d7d105-8176-47c8-a713-cd6f8b0caae3
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/Workspace.swiftscripts/build-ghostty-cli-helper.sh
| # When the requested target matches zig's native output arch, drop -Dtarget | ||
| # so zig uses native compilation. This avoids cross-linker issues on newer | ||
| # SDKs (e.g., macOS Tahoe + zig 0.15.x). Note: zig may run under Rosetta, | ||
| # so we detect native output arch from the zig binary itself, not uname -m. | ||
| ZIG_ARCH="$(file "$(command -v zig)" 2>/dev/null | grep -oE '(arm64|x86_64)' | head -1)" | ||
| case "$TARGET_TRIPLE" in | ||
| aarch64-macos) [[ "$ZIG_ARCH" == "arm64" ]] && TARGET_TRIPLE="" ;; | ||
| x86_64-macos) [[ "$ZIG_ARCH" == "x86_64" ]] && TARGET_TRIPLE="" ;; | ||
| esac |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="$(fd -p 'build-ghostty-cli-helper.sh' | head -1)"
echo "Inspecting: $f"
nl -ba "$f" | sed -n '80,100p;132,148p'
echo
echo "Reproducing Bash behavior under set -euo pipefail (expect non-zero):"
if bash -c 'set -euo pipefail; x="$(printf "unknown\n" | grep -oE "(arm64|x86_64)" | head -1)"; echo survived'; then
echo "Unexpected: command substitution survived."
exit 1
else
echo "Expected: shell exits when grep has no match."
fiRepository: manaflow-ai/cmux
Length of output: 167
🏁 Script executed:
# Find and read the build script
fd -p 'build-ghostty-cli-helper.sh' | head -1 | xargs wc -lRepository: manaflow-ai/cmux
Length of output: 99
🏁 Script executed:
# Read the critical sections (lines 75-100 and 130-150)
f="$(fd -p 'build-ghostty-cli-helper.sh' | head -1)"
echo "=== File: $f ==="
echo "Lines 75-100:"
sed -n '75,100p' "$f" | cat -n
echo ""
echo "Lines 130-150:"
sed -n '130,150p' "$f" | cat -nRepository: manaflow-ai/cmux
Length of output: 2179
🏁 Script executed:
# Test the bash pipefail behavior with grep
bash -c 'set -euo pipefail; x="$(printf "unknown\n" | grep -oE "(arm64|x86_64)" | head -1)"; echo "survived: [$x]"' 2>&1 || echo "Exit code: $?"Repository: manaflow-ai/cmux
Length of output: 72
🏁 Script executed:
# Test with || true to suppress
bash -c 'set -euo pipefail; x="$(printf "unknown\n" | grep -oE "(arm64|x86_64)" | head -1 || true)"; echo "survived: [$x]"' 2>&1Repository: manaflow-ai/cmux
Length of output: 72
Prevent false hard-fail in arch detection and validate zig before first use.
At lines 86 and 135, ZIG_ARCH="$(... | grep ... | head -1)" terminates the script under set -euo pipefail when grep finds no match, so the intended fallback path is unreachable. Additionally, line 86 uses command -v zig before the explicit check at lines 93-96.
Proposed fix
@@
-if [[ -n "$TARGET_TRIPLE" ]]; then
+if ! command -v zig >/dev/null 2>&1; then
+ echo "error: zig is required to build the Ghostty CLI helper" >&2
+ exit 1
+fi
+
+detect_zig_arch() {
+ local zig_path file_out
+ zig_path="$(command -v zig 2>/dev/null || true)"
+ [[ -z "$zig_path" ]] && return 1
+
+ file_out="$(file "$zig_path" 2>/dev/null || true)"
+ case "$file_out" in
+ *arm64* )
+ [[ "$file_out" != *x86_64* ]] && { echo "arm64"; return 0; }
+ ;;
+ *x86_64* )
+ [[ "$file_out" != *arm64* ]] && { echo "x86_64"; return 0; }
+ ;;
+ esac
+ return 1
+}
+
+if [[ -n "$TARGET_TRIPLE" ]]; then
@@
- ZIG_ARCH="$(file "$(command -v zig)" 2>/dev/null | grep -oE '(arm64|x86_64)' | head -1)"
+ ZIG_ARCH="$(detect_zig_arch || true)"
@@
-fi
-
-if ! command -v zig >/dev/null 2>&1; then
- echo "error: zig is required to build the Ghostty CLI helper" >&2
- exit 1
fi
@@
- ZIG_ARCH="$(file "$(command -v zig)" 2>/dev/null | grep -oE '(arm64|x86_64)' | head -1)"
+ ZIG_ARCH="$(detect_zig_arch || true)"Also applies to: lines 93-96, 135-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build-ghostty-cli-helper.sh` around lines 82 - 90, The arch-detection
code uses command -v zig and a grep pipeline that can fail under set -euo
pipefail, and it calls command -v zig before the explicit existence check; fix
by validating the zig binary once before any use (capture command -v zig into a
variable like zig_path and check it is non-empty), and make the ZIG_ARCH
assignment resilient to no-match by ensuring the grep/file pipeline cannot cause
a non-zero exit (e.g., use "command -v zig || true" or capture file output into
a variable and run grep with "|| true", or use awk to extract arm64|x86_64
safely), then set ZIG_ARCH to an empty string when detection fails; apply the
same robust pattern for both occurrences of ZIG_ARCH and ensure TARGET_TRIPLE
logic (case matching with TARGET_TRIPLE and ZIG_ARCH) still runs after zig
existence and arch detection.
| // Prevent ARC from releasing panel/surface before the C calls above complete. | ||
| withExtendedLifetime((terminalPanel, surface)) {} | ||
| rememberTerminalConfigInheritanceSource(terminalPanel) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that the lifetime guard currently ends before rememberTerminalConfigInheritanceSource(...)
# Expected (current code): one match showing withExtendedLifetime(...) followed by remember call.
rg -n -U 'withExtendedLifetime\(\(terminalPanel, surface\)\)\s*\{\s*\}\s*\n\s*rememberTerminalConfigInheritanceSource\(terminalPanel\)' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 198
🏁 Script executed:
# Read the full context around the flagged lines
sed -n '7395,7420p' Sources/Workspace.swift | cat -nRepository: manaflow-ai/cmux
Length of output: 1486
🏁 Script executed:
# Find the definition of rememberTerminalConfigInheritanceSource to verify it accesses pointer-backed data
rg -n 'func rememberTerminalConfigInheritanceSource' Sources/Workspace.swiftRepository: manaflow-ai/cmux
Length of output: 155
🏁 Script executed:
# Look at the implementation of rememberTerminalConfigInheritanceSource
rg -A 20 'func rememberTerminalConfigInheritanceSource\(' Sources/Workspace.swift | head -40Repository: manaflow-ai/cmux
Length of output: 1141
Keep rememberTerminalConfigInheritanceSource inside the same lifetime pin
At Line 7408, you call rememberTerminalConfigInheritanceSource(terminalPanel) after the withExtendedLifetime block ends. This function reads terminalPanel.surface.surface (a C pointer) and calls cmuxCurrentSurfaceFontSizePoints() with it. ARC can deallocate terminalPanel or surface between lines 7407 and 7408, causing use-after-free. Move both rememberTerminalConfigInheritanceSource(terminalPanel) and the subsequent if config.fontSize > 0 { ... } block into the same lifetime scope.
Suggested patch
- // Prevent ARC from releasing panel/surface before the C calls above complete.
- withExtendedLifetime((terminalPanel, surface)) {}
- rememberTerminalConfigInheritanceSource(terminalPanel)
- if config.fontSize > 0 {
- lastTerminalConfigInheritanceFontPoints = config.fontSize
- }
+ // Prevent ARC from releasing panel/surface before all pointer-backed reads complete.
+ withExtendedLifetime((terminalPanel, surface)) {
+ rememberTerminalConfigInheritanceSource(terminalPanel)
+ if config.fontSize > 0 {
+ lastTerminalConfigInheritanceFontPoints = config.fontSize
+ }
+ }
return config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 7406 - 7408, The call to
rememberTerminalConfigInheritanceSource(terminalPanel) (and the subsequent if
config.fontSize > 0 { ... } block that reads terminalPanel.surface.surface and
calls cmuxCurrentSurfaceFontSizePoints()) is executed after the
withExtendedLifetime((terminalPanel, surface)) {} scope ends, risking ARC
deallocation and a use-after-free; move
rememberTerminalConfigInheritanceSource(terminalPanel) and the following
font-size handling into the body of withExtendedLifetime((terminalPanel,
surface)) { ... } so terminalPanel and surface remain pinned while their C
pointers are accessed.
![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 1 file (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="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:2334">
P2: `inheritedTerminalFontPointsForNewWorkspace` now bypasses the overridable `inheritedTerminalConfigForNewWorkspace` path, so subclass/test-injected inherited config is ignored during `addWorkspace`. This breaks the existing config-sanitization seam and can cause regressions in workspace creation behavior that relies on overrides.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| return nil | ||
| } | ||
| return inheritedConfig.fontSize | ||
| cachedInheritedTerminalFontPointsForNewWorkspace(workspace: workspace) |
There was a problem hiding this comment.
P2: inheritedTerminalFontPointsForNewWorkspace now bypasses the overridable inheritedTerminalConfigForNewWorkspace path, so subclass/test-injected inherited config is ignored during addWorkspace. This breaks the existing config-sanitization seam and can cause regressions in workspace creation behavior that relies on overrides.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 2334:
<comment>`inheritedTerminalFontPointsForNewWorkspace` now bypasses the overridable `inheritedTerminalConfigForNewWorkspace` path, so subclass/test-injected inherited config is ignored during `addWorkspace`. This breaks the existing config-sanitization seam and can cause regressions in workspace creation behavior that relies on overrides.</comment>
<file context>
@@ -2290,48 +2296,42 @@ class TabManager: ObservableObject {
- return nil
- }
- return inheritedConfig.fontSize
+ cachedInheritedTerminalFontPointsForNewWorkspace(workspace: workspace)
}
</file context>
| cachedInheritedTerminalFontPointsForNewWorkspace(workspace: workspace) | |
| guard let inheritedConfig = inheritedTerminalConfigForNewWorkspace(workspace: workspace), | |
| inheritedConfig.fontSize > 0 else { | |
| return nil | |
| } | |
| return inheritedConfig.fontSize |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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`:
- Around line 1246-1248: The default workspace title is currently hard-coded as
"Terminal \(nextTabCount)" when calling makeWorkspaceForCreation (see the title
argument), which must be localized; replace that literal with String(localized:
"workspace.defaultTerminalTitle", defaultValue: "Terminal \(nextTabCount)") (or
similar key) so the UI uses localized text, and add the corresponding key
"workspace.defaultTerminalTitle" with the English value to
Resources/Localizable.xcstrings (and provide translations as needed); ensure the
substitution of nextTabCount remains inside the defaultValue string so the
numeric suffix is preserved.
- Around line 2299-2324: The new logic reduces inheritance to only an optional
font size and drops other fields; restore full-template inheritance by either
(A) having Workspace persist a Swift-owned snapshot of the full
CmuxSurfaceConfigTemplate (not just lastTerminalConfigInheritanceFontPoints) and
expose it (e.g. add or use a method like
lastRememberedTerminalConfigForInheritance()), or (B) if you prefer to keep
Workspace storage minimal, populate and return a full CmuxSurfaceConfigTemplate
from inheritedTerminalConfigForNewWorkspace instead of only setting fontSize
(copy command, environmentVariables, initialInput, waitAfterCommand,
workingDirectory as appropriate) so that
cachedInheritedTerminalFontPointsForNewWorkspace and
inheritedTerminalConfigForNewWorkspace do not lose fields; update references to
cachedInheritedTerminalFontPointsForNewWorkspace,
inheritedTerminalConfigForNewWorkspace,
Workspace.lastTerminalConfigInheritanceFontPoints and addWorkspace to use the
full-template API to preserve parity with the previous behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| let newWorkspace = makeWorkspaceForCreation( | ||
| title: title ?? "Terminal \(nextTabCount)", | ||
| workingDirectory: workingDirectory, |
There was a problem hiding this comment.
Localize the default workspace title.
Terminal \(nextTabCount) is user-visible, so this will show up untranslated in the sidebar/window title path. Please switch it to String(localized:..., defaultValue: ...) and add the xcstrings entry.
🌐 Suggested change
- title: title ?? "Terminal \(nextTabCount)",
+ title: title ?? String(
+ localized: "workspace.defaultTitle",
+ defaultValue: "Terminal \(nextTabCount)"
+ ),As per coding guidelines, "All user-facing strings must be localized using String(localized: "key.name", defaultValue: "English text") for every string shown in the UI. Keys must go in Resources/Localizable.xcstrings with translations for all supported languages."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 1246 - 1248, The default workspace
title is currently hard-coded as "Terminal \(nextTabCount)" when calling
makeWorkspaceForCreation (see the title argument), which must be localized;
replace that literal with String(localized: "workspace.defaultTerminalTitle",
defaultValue: "Terminal \(nextTabCount)") (or similar key) so the UI uses
localized text, and add the corresponding key "workspace.defaultTerminalTitle"
with the English value to Resources/Localizable.xcstrings (and provide
translations as needed); ensure the substitution of nextTabCount remains inside
the defaultValue string so the numeric suffix is preserved.
…aflow-ai#2283) * Fix ARC workspace inheritance crash and native Zig helper builds * Fix Nightly Cmd+N workspace creation crash * Restore safe terminal config snapshots for Intel Nightly
This branch was successfully deployed
1 participant
Summary
Verification
PATH="/opt/homebrew/bin:$PATH" ./scripts/reload.sh --tag cmd-n-intel-pr --launchxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -destination 'platform=macOS,arch=x86_64' -derivedDataPath ~/Library/Developer/Xcode/DerivedData/cmux-intel-mac-transfer buildSummary by cubic
Fixes Nightly Cmd+N and split-workspace crashes by pinning object lifetimes and switching config inheritance to a Swift-owned template instead of live terminal pointers. Also builds the
ghosttyhelper natively withzigwhen possible, choosing native slices in universal builds to avoid macOS Tahoe cross-link failures.withExtendedLifetimeacross creation/snapshot; capture the selected tab from the pinned workspace; build snapshots via an explicit loop to retain each workspace during copy.CmuxSurfaceConfigTemplate: seed only font size from the workspace’s cached lineage for new-workspace creation, pin panel/TerminalSurface wrappers while reading rawghostty_surface_tfor splits, pass a sanitized template (font size only) into new workspaces, and resolve working directory/command/env/initial input at surface creation.Written for commit 256fce1. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Chores
Tests