Fix copy-on-select: override to clipboard and fix ghostty UAF#2715
Fix copy-on-select: override to clipboard and fix ghostty UAF#2715wzoom wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
|
@palootcenas-outreach is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughAdds an inline Ghostty config override Changes
Sequence Diagram(s)sequenceDiagram
participant TerminalView as TerminalView
participant Ghostty as Ghostty
participant Clipboard as macOS_Clipboard
TerminalView->>Ghostty: loadDefaultConfigFilesWithLegacyFallback()
TerminalView->>Ghostty: loadInlineGhosttyConfig(prefix:"cmux-copy-on-select","copy-on-select = clipboard")
Ghostty-->>TerminalView: config loaded (override present)
Note over Ghostty: Selection fix: compare new vs prev BEFORE freeing old pins
TerminalView->>Ghostty: mouse-drag selection events
Ghostty->>Ghostty: eql(prev, new) then free old pins if different
TerminalView->>Clipboard: on mouse-up -> copySelectionToClipboards()
Clipboard-->>TerminalView: clipboard updated
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 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 compounding issues that broke copy-on-select after PR #2420: a use-after-free in Ghostty's
Confidence Score: 4/5Safe to merge once manaflow-ai/ghostty#36 is confirmed merged to fork main and docs/ghostty-fork.md is updated with the new merge commit SHA. One P1 finding: the submodule pointer to 75ec7bf89 may violate the CLAUDE.md submodule safety policy if the ghostty fork branch hasn't landed on fork main yet, and the docs still reference a stale reachability merge commit. The Swift change itself is small, correct, and follows the established inline-config override pattern. Once the dependency is confirmed merged and docs updated, this should be a 5. docs/ghostty-fork.md and the ghostty submodule pointer need verification that 75ec7bf89 is reachable from fork main.
|
| Filename | Overview |
|---|---|
| Sources/GhosttyTerminalView.swift | Adds a loadInlineGhosttyConfig("copy-on-select = clipboard") override after all user config loading; placement and pattern are correct but silently overrides any user-set copy-on-select value. |
| docs/ghostty-fork.md | Adds section 8 documenting the UAF fix and updates the pinned fork head, but the fork-main reachability sentence still references the old section-7 merge commit 5c781d710, not a new merge commit for 75ec7bf89. |
| ghostty | Submodule pointer bumped to 75ec7bf89; per CLAUDE.md policy this commit must be on fork main before the parent-repo pointer is committed — the PR description flags this as a dependency on manaflow-ai/ghostty#36 being merged first. |
Sequence Diagram
sequenceDiagram
participant User
participant Ghostty as Ghostty Surface (fork)
participant cmux as cmux Config Loader
participant NSPasteboard
User->>Ghostty: drag-select text (mouse down → drag)
User->>Ghostty: mouse button release
Note over Ghostty: OLD: setSelection() frees pins,<br/>eql() reads freed ptr → returns true,<br/>copySelectionToClipboards SKIPPED
Note over Ghostty: NEW (75ec7bf89): read existing<br/>tracked selection directly,<br/>call copySelectionToClipboards
cmux-->>Ghostty: config override: copy-on-select = clipboard<br/>(loaded after user config files)
Ghostty->>NSPasteboard: write selected text to NSPasteboard.general
User->>NSPasteboard: Cmd+V → paste works ✓
Reviews (1): Last reviewed commit: "Fix copy-on-select: override to clipboar..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Current cmux pinned fork head: `75ec7bf89` (`fix-copy-on-select-uaf`). | ||
| Fork `main` keeps this pin reachable via merge commit `5c781d710` | ||
| (`Retain layer-background pin ancestry on main`). |
There was a problem hiding this comment.
The Fork main keeps this pin reachable via merge commit 5c781d710 (Retain layer-background pin ancestry on main) sentence was not updated for the new pin. That merge commit describes a section-7 (layer-background) operation, not the section-8 copy-on-select fix. If 75ec7bf89 has been merged to fork main via a new merge commit, this line should reference that new SHA; if it hasn't been merged yet, the submodule pointer is pointing at an off-main commit, which violates the CLAUDE.md submodule safety policy ("always push the submodule commit to its remote main branch BEFORE committing the updated pointer in the parent repo").
The new pinned head is described as fix-copy-on-select-uaf (a branch name rather than a version tag like the previous v1.3.1-473-gae3cc5d29), and the PR description states "The ghostty fork PR (manaflow-ai/ghostty#36) should be merged first" — both signal that the merge to main may still be pending. Please update this reference once the fork PR lands on main.
| loadInlineGhosttyConfig( | ||
| "copy-on-select = clipboard", | ||
| into: config, | ||
| prefix: "cmux-copy-on-select", | ||
| logLabel: "copy-on-select override" | ||
| ) |
There was a problem hiding this comment.
copy-on-select setting
Because loadInlineGhosttyConfig is called after loadCmuxAppSupportGhosttyConfigIfNeeded (and ghostty_config_load_recursive_files), this inline config wins over any user-set value. A user who explicitly sets copy-on-select = false in their Ghostty config will have the preference silently reinstated to clipboard with no cmux-level knob to opt back out.
This is the same architectural pattern used for macos-background-from-layer (a cmux-internal flag), but copy-on-select is a standard, documented Ghostty option users may intentionally set. Worth considering whether a cmux settings key should gate this override, or at minimum adding a note in release notes / docs that the value is managed by cmux.
There was a problem hiding this comment.
Good point. A few notes on why this is an intentional forced override for now:
-
Ghostty's default
copy-on-select = truewrites to a private named pasteboard (com.mitchellh.ghostty.selection), not the system clipboard. This means Cmd+V doesn't paste selections — only middle-click within Ghostty works. That's surprising behavior on macOS where users expect selected text to be available via Cmd+V. -
The UAF bug this PR fixes was in the
copy-on-select = truecode path. Until the ghostty fork fix lands, the default value is actively broken (intermittently copies only the first character). -
This mirrors the existing pattern for
macos-background-from-layer— cmux overrides it because the Ghostty default doesn't match cmux's rendering model. Similarly,copy-on-select = clipboardis the only value that gives cmux users the expected macOS clipboard behavior.
Adding a cmux settings key to let users opt out (back to false or true) is reasonable as a follow-up if users request it. For now, shipping with clipboard as the forced default is the safest path.
There was a problem hiding this comment.
Ghostty's default copy-on-select = true writes to a private named pasteboard (com.mitchellh.ghostty.selection), not the system clipboard. Cmd+V doesn't paste selections — only middle-click within Ghostty works. That's surprising on macOS where people expect selected text in the system clipboard.
The UAF this PR fixes was also in this code path, so before the fix the default was actively broken too.
Same pattern as the macos-background-from-layer override — Ghostty's default doesn't match what cmux needs. Adding a cmux settings key to let users opt out is reasonable as a follow-up if anyone asks for it, but clipboard is the right default to ship with.
![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 `@Sources/GhosttyTerminalView.swift`:
- Around line 1711-1721: The copy-on-select override call to
loadInlineGhosttyConfig("copy-on-select = clipboard", into: config, prefix:
"cmux-copy-on-select", logLabel: "copy-on-select override") is only executed on
the primary config path; update the fallback config branch (the code that
constructs/uses the fallback config variable around the fallback-loading logic)
to also apply the same loadInlineGhosttyConfig call after the fallback config is
created, or factor the override into a shared post-load helper that both the
primary load path and the fallback load path call so that config always receives
the clipboard override regardless of which path is taken.
🪄 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: c1b1290b-956f-4ff4-a3e4-0e2b51b41e68
📒 Files selected for processing (3)
Sources/GhosttyTerminalView.swiftdocs/ghostty-fork.mdghostty
![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 3 files
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="docs/ghostty-fork.md">
<violation number="1" location="docs/ghostty-fork.md:16">
P2: The pinned fork head was updated to `75ec7bf89` but the merge-commit reachability line still references `5c781d710` (`Retain layer-background pin ancestry on main`), which is the section-7 merge commit. This line needs to be updated to reference the merge commit that makes the new section-8 pin reachable on fork `main`. The branch-name label (`fix-copy-on-select-uaf`) rather than a version tag also suggests the fork PR may not have landed on `main` yet — if so, the submodule pointer is pointing at an off-`main` commit.</violation>
</file>
<file name="Sources/GhosttyTerminalView.swift">
<violation number="1" location="Sources/GhosttyTerminalView.swift:1716">
P2: This `copy-on-select = clipboard` override is only applied in the primary config load path. If the fallback config path is taken (e.g., when the primary config fails to load), the override will be missing and copy-on-select will regress to Ghostty's default (private pasteboard), breaking Cmd+V paste of selections. Apply the same override into `fallbackConfig` in the fallback path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
wzoom
force-pushed
the
fix-copy-on-select
branch
2 times, most recently
from
eb2adfc to
84c10b5
Compare
- Override copy-on-select = clipboard in both primary and fallback config paths so selecting text always copies to NSPasteboard.general (Cmd+V). The fallback path was previously missing this override. - Update ghostty submodule to 353d9b518 which fixes a use-after-free in setSelection() that silently skipped clipboard copies. - Update docs/ghostty-fork.md with new commit SHA and description. Fixes manaflow-ai#2664
wzoom
force-pushed
the
fix-copy-on-select
branch
from
84c10b5 to
ca436be
Compare
1 participant
Summary
copy-on-select = clipboardso selecting text copies to the system clipboard (Cmd+V works)docs/ghostty-fork.mdwith section 8 documenting the ghostty fork changeProblem
Two issues combined to break copy-on-select after PR #2420:
PR Remove copy-on-select setting and override #2420 removed the
copy-on-select = falseoverride, activating Ghostty's default of.true. This exposed a latent use-after-free bug in Ghostty'ssetSelection()whereScreen.select()frees old tracked pins, theneql()dereferences the freed pointer. Since freed memory usually retains old values,eql()returnstrueandcopySelectionToClipboardsis silently skipped.Even when copy-on-select fires,
.truewrites to a private named pasteboard (com.mitchellh.ghostty.selection), notNSPasteboard.general. Cmd+V doesn't paste the selection.Fix
setSelection()in the mouse release handler and callcopySelectionToClipboardsdirectly, fixing the use-after-free.copy-on-select = clipboardso selections write to the system clipboard.Dependencies
This PR includes the ghostty submodule SHA update pointing to the fix commit. The ghostty fork PR (manaflow-ai/ghostty#36) should be merged first.
Fixes #2664
Summary by cubic
Force copy-on-select to use the macOS system clipboard in both primary and fallback init paths, and update the
ghosttyfork with a use-after-free fix so selections always copy. Cmd+V now reliably pastes selected text.Bug Fixes
copy-on-select = clipboardin both config init paths so selections write toNSPasteboard.generalinstead ofcom.mitchellh.ghostty.selection.Dependencies
ghosttysubmodule to include the copy-on-select UAF fix and a final copy on mouse-up, preventing silent clipboard skips.Written for commit ca436be. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Documentation