Repository navigation
Fix nightly startup crash - #4318
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds StartupBreadcrumbLog and instruments app startup/teardown and single-instance logic with breadcrumbs; adapts Ghostty runtime callbacks to per-instance routing; adds smoke-launch and dylib-verification scripts and integrates them into build/sign/notarize CI steps. ChangesStartup Instrumentation and Verification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (13 passed)
✨ 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 |
Contributor
Greptile SummaryFixes the nightly instant-startup crash caused by
Confidence Score: 5/5Safe to merge — crash fix is correctly scoped to the re-entry window and the packaging changes are additive CI guards. The registry+in-flight-fallback design for callback routing is sound for a single process-lifetime GhosttyApp: wakeup_cb resolves the app directly from its userdata pointer, and action_cb uses the locked registry with a deliberate fallback to initializingRuntimeApp only during ghostty_app_new, cleared by defer when init exits. StartupBreadcrumbLog is nightly/debug-gated, uses flock for cross-process serialization, and all previous review concerns have been addressed. Packaging changes are isolated to build and CI scripts with no effect on app logic. No files require special attention. GhosttyTerminalView.swift carries the most risk as the crash fix, but the logic is well-contained and the CI smoke-launch guard will catch regressions. Important Files Changed
Reviews (5): Last reviewed commit: "Address final launch review feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-launch-macos-app.sh`:
- Around line 24-27: The cleanup() function and PID handling currently may pick
an unrelated running app because APP_PID is obtained globally; change the launch
logic to capture and use the child PID returned by the local background launch
(use the shell $! equivalent) and store that in APP_PID immediately after
starting the app so cleanup() always targets the instance you just launched;
update every place that assigns or checks APP_PID (the other launch/wait blocks
and any pgrep/pidof-based checks) to rely on the stored launch PID rather than
querying processes globally, and keep the existing kill -0 / kill calls but only
referencing that scoped APP_PID in cleanup().
- Around line 40-50: The current loop polls with sleep to detect the launched
app (using APP_PID, find_app_pid, OPEN_PID, SECONDS/STARTUP_TIMEOUT_SECONDS),
violating the no-sleep policy; replace it with blocking/wait-based
synchronization by removing the while/sleep loop and using wait "$OPEN_PID" to
block until the open/launcher process finishes, then call find_app_pid once (or
a small bounded non-sleep retry using a timeout wrapper) to obtain APP_PID;
enforce the overall startup timeout by running the open command under a timeout
utility (e.g., timeout/gtimeout) or by using a trap/alarm-based timeout instead
of polling with SECONDS and sleep.
In `@Sources/App/StartupBreadcrumbLog.swift`:
- Around line 10-11: The NSLock usage (lock.lock()/defer { lock.unlock() }) only
protects within one process; replace per-process NSLock protection in
StartupBreadcrumbLog write/append methods with an inter-process file lock: open
the breadcrumb file (or create it) and acquire an exclusive POSIX lock (flock or
fcntl F_SETLKW) on its file descriptor before seeking/appending JSONL, then
release the lock (LOCK_UN) in a defer; update all places that use the local
`lock` (including the other write/append blocks referenced) to use this fd-based
lock around the file writes so concurrent processes cannot corrupt the shared
file. Ensure the lock is taken on the same file descriptor used for the write
and that errors opening/locking are handled/logged.
- Around line 22-24: The loop that merges caller-provided `fields` into
`payload` (the code using `for (key, value) in fields { payload[key] =
sanitized(value) }`) must ignore or namespace reserved breadcrumb keys so
callers cannot overwrite built-in metadata; update the merge to skip keys in a
reserved set (e.g., "timestamp", "event", "pid", and any version keys) or rename
them (e.g., prefix with "custom_") before calling `sanitized(value)`, ensuring
the `payload` and any related types in StartupBreadcrumbLog keep the original
built-in keys intact.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 3619-3621: The code currently calls applyBackgroundToKeyWindow()
on the global key window (mutating cmux.main*), which lets runtime A repaint
runtime B; change the call so the background is applied only to windows/surfaces
owned by this GhosttyTerminalView instance (e.g., add/use a method on
GhosttyTerminalView like applyBackgroundToOwnedWindows() or a variant that
accepts the specific window(s)/surface(s) for self) and invoke that instead of
applyBackgroundToKeyWindow(); update both occurrences (the one inside
DispatchQueue.main.async near applyBackgroundToKeyWindow and the other
occurrence around lines 3851–3853) to scope changes to self’s windows.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 99002098-8736-40ac-84f6-0144d36e979e
📒 Files selected for processing (11)
.github/workflows/nightly.yml.github/workflows/release.ymlSources/App/StartupBreadcrumbLog.swiftSources/AppDelegate.swiftSources/GhosttyTerminalView.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojscripts/build-command-palette-nucleo-ffi.shscripts/sign-cmux-bundle.shscripts/smoke-launch-macos-app.shscripts/verify-command-palette-nucleo-ffi-artifact.sh
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-launch-macos-app.sh`:
- Around line 65-67: The script currently prints raw launcher and system logs
unconditionally (e.g., the cat "$OPEN_LOG" block and the app stability check and
the /usr/bin/log show invocation); change these to only dump raw logs when an
explicit debug env flag is set (use CMUX_SMOKE_DEBUG_LOGS=1) by wrapping the cat
"$OPEN_LOG", the app-stability log dump, and the /usr/bin/log show call in a
conditional that checks CMUX_SMOKE_DEBUG_LOGS, and when the flag is not set
replace the raw output with a short sanitized message (e.g., "Detailed logs
available with CMUX_SMOKE_DEBUG_LOGS=1") so default error output does not leak
internal system/framework details.
In `@Sources/GhosttyTerminalView.swift`:
- Around line 2010-2012: The runtimeConfig.action_cb assignment can be nil
during app construction because appRegistry isn't populated until
ghostty_app_new returns; to fix, register an init-time fallback on
runtimeConfig.action_cb that safely handles the startup window by first
attempting GhosttyApp.runtimeApp(for:) and calling
runtimeApp.handleAction(target:action:) if present, but if runtimeApp is nil
either queue the action for replay or return a defined fallback response (e.g.,
false) until ghostty_app_new completes; update the closure at the sites
referencing runtimeConfig.action_cb (the current block using
GhosttyApp.runtimeApp(for:) and handleAction, and the other similar blocks
around the indicated ranges) so they use this fallback/queue mechanism and
ensure the queued actions are flushed once ghostty_app_new has populated the
registry.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 468db082-fb67-4fc2-a235-476e893048a1
📒 Files selected for processing (3)
Sources/App/StartupBreadcrumbLog.swiftSources/GhosttyTerminalView.swiftscripts/smoke-launch-macos-app.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/GhosttyTerminalView.swift (1)
1672-1676:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
action_cbstill has an init-time hole.The new registry is only populated after
ghostty_app_new(...)returns, but the added safety comment explicitly says Ghostty callbacks can run during singleton initialization. Ifaction_cbfires in that window,runtimeApp(for:)isniland the callback is dropped by returningfalse. This still leaves startup behavior dependent on callback timing. Please add an init-time fallback for the constructing instance (or queue actions untilregisterRuntimeAppruns) instead of relying solely on the post-construction registry.Also applies to: 2011-2013, 2100-2103, 2149-2161
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/GhosttyTerminalView.swift` around lines 1672 - 1676, The action_cb can fire before registerRuntimeApp populates appRegistry, so update the startup path to provide an init-time fallback: add a temporary construct-time holder (e.g. a static optional like constructingApp keyed by the same UInt id) and set it during GhosttyApp initialization, then have runtimeApp(for:) check that constructingApp when appRegistry lookup returns nil; alternatively make action_cb enqueue the incoming actions into a short-lived per-id queue (protected by appRegistryLock) that registerRuntimeApp drains once it sets appRegistry. Concretely, set the constructing instance before calling ghostty_app_new, clear it inside registerRuntimeApp when moving into appRegistry, and modify runtimeApp(for:) (and action_cb) to consult the constructingApp or the per-id queue as a fallback so callbacks are not dropped during initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Sources/GhosttyTerminalView.swift`:
- Around line 1672-1676: The action_cb can fire before registerRuntimeApp
populates appRegistry, so update the startup path to provide an init-time
fallback: add a temporary construct-time holder (e.g. a static optional like
constructingApp keyed by the same UInt id) and set it during GhosttyApp
initialization, then have runtimeApp(for:) check that constructingApp when
appRegistry lookup returns nil; alternatively make action_cb enqueue the
incoming actions into a short-lived per-id queue (protected by appRegistryLock)
that registerRuntimeApp drains once it sets appRegistry. Concretely, set the
constructing instance before calling ghostty_app_new, clear it inside
registerRuntimeApp when moving into appRegistry, and modify runtimeApp(for:)
(and action_cb) to consult the constructingApp or the per-id queue as a fallback
so callbacks are not dropped during initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 58f43dce-68fc-475f-be88-774a582206b8
📒 Files selected for processing (2)
Sources/App/StartupBreadcrumbLog.swiftSources/GhosttyTerminalView.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7ee42f6. Configure here.
| appRegistryLock.lock() | ||
| defer { appRegistryLock.unlock() } | ||
| return appRegistry[key] | ||
| } |
There was a problem hiding this comment.
Unused private method runtimeApp(for:) is dead code
Low Severity
The newly added runtimeApp(for app: ghostty_app_t?) -> GhosttyApp? static method is never called anywhere in the codebase. The wakeup_cb uses runtimeApp(from: UnsafeMutableRawPointer?) (a different overload), and the action_cb uses runtimeAppForActionCallback(_:). This method is dead code that adds confusion alongside the two actually-used lookup methods.
Reviewed by Cursor Bugbot for commit 7ee42f6. Configure here.
lawrencecchen
dismissed stale reviews from coderabbitai[bot] and coderabbitai[bot]
May 19, 2026 00:40
Stale bot change request. Inline findings were addressed in later commits and resolved on the PR.
This was referenced May 19, 2026
This was referenced May 22, 2026
Closed
This was referenced Jun 12, 2026
This was referenced Jun 29, 2026
This was referenced Jul 16, 2026
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
1 participant
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
Verification
Note
Medium Risk
Touches macOS startup/termination paths and Ghostty runtime callback routing, plus adds new CI smoke-launch and signing/linkage verification; failures could impact app launch or release pipelines if assumptions differ across environments.
Overview
Fixes a macOS nightly instant-launch crash by routing Ghostty C runtime callbacks (
wakeup_cb/action_cb) to the correctGhosttyAppinstance viauserdataplus a lockedghostty_app_t→GhosttyAppregistry, avoidingGhosttyApp.sharedre-entry during initialization.Adds lightweight startup breadcrumbs (
StartupBreadcrumbLog) and instruments key launch/single-instance/termination milestones incmuxAppandAppDelegate(enabled by default for nightly/debug or via env flags) to aid post-mortem startup debugging.Hardens packaging/release automation: nightly + release workflows now smoke-launch the signed/notarized app; the Nucleo command-palette dylib build normalizes its install name to
@rpath, and signing verifies the dylib’s install name and rejects CI/source-tree absolute load paths.Reviewed by Cursor Bugbot for commit 7ee42f6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes the nightly instant startup crash by hardening Ghostty runtime callback routing to prevent
GhosttyAppsingleton re-entry during init. Adds env-toggleable startup breadcrumbs and a CI smoke launch step, and enforces an@rpathinstall name for the bundled Nucleo FFI dylib with signing-time verification.Bug Fixes
wakeup_cbnow targets the app fromuserdata;action_cbresolves via a locked registry keyed byghostty_app_tand falls back to the in-flight app during runtime init, eliminating singleton re-entry crashes.libcmux_command_palette_nucleo_ffi.dylibinstall name to@rpath/...during build and verifies it during signing (including checks for absolute load paths).New Features
cmuxApp/AppDelegate, including single-instance and termination paths.scripts/smoke-launch-macos-app.shto launch the signed app, wait for PID registration, filter pre-existing processes, ensure brief liveness, and print breadcrumbs (system logs are optional viaCMUX_SMOKE_DEBUG_LOGS).Written for commit 7ee42f6. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Improvements
Chores