Add custom command shortcut spawning - #3021
austinywang wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds JSON-configurable custom command shortcuts that are live-reloaded and, when triggered, create new surfaces/splits/tabs/workspaces and run the configured shell command inside them with resolved working directory and environment variables. Changes
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant CustomCommandStore
participant TabManager
participant Workspace
participant Terminal
User->>AppDelegate: press shortcut (NSEvent)
AppDelegate->>CustomCommandStore: matchingCommand(for: event)
CustomCommandStore-->>AppDelegate: CustomCommandBinding?
alt binding found
AppDelegate->>TabManager: openSurfaceAndRunCommand(target,cwd,command,id)
TabManager->>TabManager: resolve cwd & env
TabManager->>Workspace: create surface/split/new workspace (with startupEnvironment, overrideWorkingDirectory)
Workspace->>Terminal: instantiate terminal panel (with env)
Terminal-->>TabManager: ready
TabManager->>Terminal: sendTerminalInputWhenReady(command + "\n")
Terminal->>Terminal: execute command and render output
Terminal-->>User: display output
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 adds a live-reloaded
Confidence Score: 4/5Safe to merge after addressing the all-or-nothing JSON decode failure that silently wipes valid bindings when any single entry has a bad target value. One P1 issue remains: a malformed target in any single binding silently disables every custom command shortcut. The rest of the implementation is solid — FSEvents watcher, per-entry validation in resolveBindings, environment injection, and the E2E regression test all look correct. Sources/KeybindingsConfigFile.swift — the non-optional CustomCommandTarget field causes whole-array decode failures that need to be isolated to the offending entry. Important Files Changed
Sequence DiagramsequenceDiagram
participant FS as Filesystem
participant SW as ShortcutSettingsFileWatcher
participant CS as CustomCommandStore
participant KS as KeyboardShortcutSettings
participant AD as AppDelegate
participant TM as TabManager
participant WS as Workspace
FS->>SW: FSEvent (keybindings.json changed)
SW->>CS: onChange callback
CS->>CS: DispatchQueue.main.async reload()
CS->>CS: JSONCParser.preprocess + JSONDecoder.decode
CS->>CS: resolveBindings → [ResolvedBinding]
Note over AD: User presses shortcut key
AD->>KS: matchingCustomCommand(for: event)
KS->>CS: matchingCommand(for: event)
CS-->>KS: CustomCommandBinding?
KS-->>AD: CustomCommandBinding?
AD->>TM: openSurfaceAndRunCommand(target, cwd, command, id)
TM->>TM: customCommandLaunchContext()
TM->>TM: resolvedCustomCommandWorkingDirectory()
TM->>TM: customCommandEnvironment()
alt target = split_right / split_down
TM->>WS: newTerminalSplit(from:, orientation:, workingDirectory:, startupEnvironment:)
else target = new_surface
TM->>WS: newTerminalSurface(inPane:, workingDirectory:, startupEnvironment:)
else target = new_tab / new_workspace
TM->>TM: addWorkspace(workingDirectory:, initialTerminalEnvironment:)
end
WS-->>TM: TerminalPanel
TM->>WS: sendTerminalInputWhenReady(command + newline, to: panel)
Reviews (1): Last reviewed commit: "Implement custom command shortcut spawni..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| struct CustomCommandBinding: Codable, Equatable, Identifiable { | ||
| var id: String | ||
| var shortcut: String | ||
| var command: String | ||
| var label: String? | ||
| var target: CustomCommandTarget | ||
| var cwd: CustomCommandWorkingDirectory? | ||
|
|
||
| var resolvedWorkingDirectory: CustomCommandWorkingDirectory { | ||
| cwd ?? .workspace | ||
| } | ||
| } |
There was a problem hiding this comment.
CustomCommandTarget and CustomCommandWorkingDirectory are non-optional, non-defaulting Codable types. If any entry in custom_commands has an unknown target string (e.g. a typo like "split_left") or a missing required field, JSONDecoder will throw before resolveBindings even runs, and the catch block clears bindings = [] — wiping every previously-working shortcut. The per-entry guard logic in resolveBindings never has a chance to isolate the bad entry.
A user who adds a second binding with a typo in target will silently lose all their other bindings with only an NSLog message to explain it. Consider decoding custom_commands as [AnyCodable] (or wrapping each element decode in a try-catch) so that one bad entry is skipped and the rest continue to work.
| guard !shortcutString.isEmpty, | ||
| let shortcut = StoredShortcut.parse(rawValue: shortcutString), | ||
| !shortcut.hasChord else { | ||
| NSLog("[CustomCommandStore] ignoring custom command '%@' with invalid shortcut '%@' in %@", id, binding.shortcut, path) | ||
| continue |
There was a problem hiding this comment.
hasChord guard is dead code
StoredShortcut.parse(rawValue:) calls StoredShortcut.parse(strokes: [rawValue]) which always produces a single-stroke shortcut, making hasChord always false. The guard !shortcut.hasChord can never trigger here, so users don't get a clear log message when they accidentally write a chord like "cmd+k cmd+j" — the shortcut string is just silently accepted as-is (or rejected earlier if the second stroke can't be parsed from the +-split logic). Consider either documenting that chord syntax is unsupported at the config level, or parsing strokes by splitting on space and passing an array, so the hasChord guard actually functions.
| try: | ||
| previous_keybindings = _write_keybindings_config(config) | ||
| time.sleep(0.5) | ||
|
|
There was a problem hiding this comment.
time.sleep(0.5) is the only guarantee that CustomCommandStore has re-read keybindings.json before the test simulates the shortcut. On slow CI runners or under heavy load this window can be missed, causing the shortcut to fire before the store has the binding and producing a spurious miss. Consider either polling for the binding to be active (via a CLI/socket query, if the store exposes a reload-notification) or, if no such hook exists, using a longer poll loop with a reasonable timeout instead of a bare sleep.
| case .newTab, .newWorkspace: | ||
| let workspace = addWorkspace( | ||
| workingDirectory: resolvedWorkingDirectory, | ||
| initialTerminalEnvironment: environment, | ||
| select: true | ||
| ) | ||
| guard let panel = workspace.focusedTerminalPanel else { return false } | ||
| workspace.sendTerminalInputWhenReady(input, to: panel) | ||
| scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command") | ||
| return true | ||
| } |
There was a problem hiding this comment.
newTab and newWorkspace targets are identical
Both cases call addWorkspace(...) with the same arguments, so a user who writes "target": "new_tab" gets exactly the same behaviour as "new_workspace". Given that cmux has a Tab = Workspace alias this may be intentional, but it means one of the two documented target values is misleading. If they are truly synonyms, consider removing newTab from CustomCommandTarget (or aliasing it in docs) to avoid user confusion. If there is a planned distinction (e.g. new_surface in the current workspace's focused pane vs. a brand-new workspace), the implementation should reflect it.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
953-979:⚠️ Potential issue | 🟠 MajorUse a dedicated custom-command launcher instead of raw terminal input.
The new shortcut path calls this with
trimmedCommand + "\n"fromSources/TabManager.swift, so configured commands run in the user’s interactive shell rather than via the required/bin/sh -c. It also relies on the initial cwd only; shell rc files cancdbefore this delayed injection runs, and the fixed 3s timeout can silently drop slow-start commands.Suggested direction
- func sendTerminalInputWhenReady(_ text: String, to panel: TerminalPanel) { + func sendTerminalInputWhenReady(_ text: String, to panel: TerminalPanel, timeout: TimeInterval = 3.0) { if panel.surface.surface != nil { panel.sendInput(text) return } @@ - DispatchQueue.main.asyncAfter(deadline: .now() + 3.0) { + DispatchQueue.main.asyncAfter(deadline: .now() + timeout) { guard !resolved else { return } resolved = true if let observer { NotificationCenter.default.removeObserver(observer) } NSLog("[CmuxConfig] surface not ready after 3s, dropping command (%d chars)", text.count) } } + + func sendCustomCommandWhenReady(_ command: String, cwd: String?, to panel: TerminalPanel) { + let trimmedCommand = command.trimmingCharacters(in: .whitespacesAndNewlines) + guard !trimmedCommand.isEmpty else { return } + + let shellCommand = "/bin/sh -c \(Self.shellSingleQuoted(trimmedCommand))" + let guardedCommand: String + if let cwd = cwd?.trimmingCharacters(in: .whitespacesAndNewlines), !cwd.isEmpty { + guardedCommand = "cd \(Self.shellSingleQuoted(cwd)) && \(shellCommand)" + } else { + guardedCommand = shellCommand + } + sendTerminalInputWhenReady(guardedCommand + "\n", to: panel, timeout: 30.0) + } + + private static func shellSingleQuoted(_ value: String) -> String { + "'" + value.replacingOccurrences(of: "'", with: "'\"'\"'") + "'" + }Then update
Sources/TabManager.swiftto callsendCustomCommandWhenReady(..., cwd: resolvedWorkingDirectory, to: panel)instead of passing rawtrimmedCommand + "\n".Based on learnings: In this repo’s shell session resume flow, always build resume commands with a guaranteed cwd guard so fresh shells and rc files cannot land outside the intended directory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 953 - 979, Replace the raw-injection helper sendTerminalInputWhenReady with a new sendCustomCommandWhenReady(_ command: String, cwd: String, to panel: TerminalPanel) that still waits for panel.surface readiness (reuse the observer pattern in sendTerminalInputWhenReady) but instead of sending text injects a dedicated custom-command launch API on the terminal (invoke the terminal’s command-launcher rather than panel.sendInput). Construct the launched command to include a cwd-guard (e.g., prefix the shell invocation with a guaranteed cd to the provided cwd) and run via the proper shell execution path (use /bin/sh -c or the repo’s launcher contract) so rc files cannot escape the intended directory; remove the fragile "trimmedCommand + \"\n\"" raw-input usage and the fixed 3s drop behavior, and update TabManager to call sendCustomCommandWhenReady(..., cwd: resolvedWorkingDirectory, to: panel) instead of sendTerminalInputWhenReady with the newline-injected string.
🧹 Nitpick comments (3)
Sources/KeybindingsConfigFile.swift (2)
36-53: Tighten absolute-path validation and consider~expansion.A couple of edge cases worth addressing in the
defaultbranch:
"/"alone passeshasPrefix("/")and becomes.absolutePath("/")— probably not what users intend. Consider requiring the path to contain at least one additional character (and rejecting empty strings up front).- The PR spec is "absolute path", but users commonly write
~/projects/fooin JSON config. Either document that~is not supported, or expand it here (e.g.,NSString(string: raw).expandingTildeInPath) before storing. Silent rejection of~-prefixed paths with only the generic error message will confuse users.Also, the error message mentions the valid sentinel values but not the offending input; including
rawValueindebugDescriptionwould make config errors much easier to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeybindingsConfigFile.swift` around lines 36 - 53, The decoder init (init(from decoder: Decoder)) currently treats any string starting with "/" as a valid .absolutePath; change it to first reject empty rawValue and rawValue == "/" (require at least one more path character), expand leading "~" by calling NSString(string: rawValue).expandingTildeInPath before validation (or explicitly reject "~" if you choose not to support it), then validate the expanded string starts with "/" and is longer than 1 before assigning .absolutePath(...); when throwing DecodingError.dataCorruptedError include the offending rawValue in the debugDescription to aid diagnostics (refer to rawValue, .absolutePath(rawValue), container, and DecodingError.dataCorruptedError).
4-7: Use camelCase withCodingKeysinstead of snake_case stored property.
var custom_commandsviolates Swift naming conventions. Prefer acustomCommandsproperty mapped viaCodingKeysso the JSON key stayscustom_commandswhile the Swift API remains idiomatic.♻️ Proposed refactor
struct Schema: Codable { var version: Int? - var custom_commands: [CustomCommandBinding]? + var customCommands: [CustomCommandBinding]? + + private enum CodingKeys: String, CodingKey { + case version + case customCommands = "custom_commands" + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeybindingsConfigFile.swift` around lines 4 - 7, The Schema struct uses a snake_case stored property `custom_commands`; rename it to `customCommands` and add a CodingKeys enum to map the JSON key to the new property name (e.g., enum CodingKeys: String, CodingKey { case version, customCommands = "custom_commands" }) so Codable still decodes/encodes the `custom_commands` JSON key; update any references to `Schema.custom_commands` to `Schema.customCommands` throughout the codebase.Sources/KeyboardShortcutSettingsFileStore.swift (1)
1530-1530: No module-scope name collisions found after visibility widening.
JSONCParserandShortcutSettingsFileWatcherare now module-internal. Verification confirms:
- No duplicate type declarations exist (each is unique in the module).
ShortcutSettingsFileWatcheris actively used across multiple stores (KeyboardShortcutSettingsFileStoreandCustomCommandStore), confirming broader scope. Consider renaming toCmuxConfigFileWatcherorSettingsFileWatcherto better reflect its role monitoring multiple configuration file types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/KeyboardShortcutSettingsFileStore.swift` at line 1530, The symbol ShortcutSettingsFileWatcher has been widened to module scope but its name no longer reflects its multi-store role; rename ShortcutSettingsFileWatcher (and any matching file/class declarations) to a clearer name such as CmuxConfigFileWatcher or SettingsFileWatcher, update all references (e.g., in KeyboardShortcutSettingsFileStore and CustomCommandStore and any other consumers), and ensure the JSONCParser symbol remains unchanged; adjust initializer signatures and documentation comments if present so callers compile after the rename and run-time behavior is preserved.
🤖 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/CustomCommandStore.swift`:
- Around line 53-96: The resolveBindings function currently only deduplicates
ids but not shortcut collisions; add a Set to track seen shortcuts (e.g.,
seenShortcuts: Set<StoredShortcut> or Set<String> using the parsed
shortcut.rawValue) and, after successfully parsing StoredShortcut in
resolveBindings, check whether the parsed shortcut is already in seenShortcuts;
if it is, log a message similar to the existing NSLog that you are ignoring the
duplicate shortcut for the later CustomCommandBinding (include id,
binding.shortcut, path) and continue, otherwise insert the shortcut into
seenShortcuts and proceed to append the ResolvedBinding; update references
around ResolvedBinding, CustomCommandBinding and StoredShortcut.parse
accordingly.
In `@Sources/TabManager.swift`:
- Around line 5001-5050: Add DEBUG-only dlog() calls surrounding the launch
success/failure branches for the switch on target: for the
.splitRight/.splitDown path (around workspace.newTerminalSplit and before
returning) and for .newSurface (around
bonsplitController.focusedPaneId/newTerminalSurface) and .newTab/.newWorkspace
(around addWorkspace and after obtaining workspace.focusedTerminalPanel). Wrap
each dlog() in `#if` DEBUG / `#endif`, log only metadata such as target enum value,
workspace.id, paneId or panel.id, and a short status string ("launch_success" /
"launch_failed"), and explicitly do NOT log the command/input contents or
environment; place the success log before
scheduleInitialWorkspaceGitMetadataRefreshIfPossible and the failure log where
the guard returns false.
- Line 4999: The current code sends trimmedCommand directly to the terminal
(creating input = trimmedCommand + "\n") which bypasses /bin/sh -c semantics;
update TabManager to wrap the command in a /bin/sh -c invocation and properly
single-quote the command string: add a nonisolated helper
TabManager.shellSingleQuoted(_:) that escapes single quotes, then construct
input as "/bin/sh -c " + TabManager.shellSingleQuoted(trimmedCommand) + "\n" (or
equivalent string concatenation) so the terminal receives the command executed
via /bin/sh -c with correct quoting.
- Around line 5040-5048: Split the combined case so .newTab and .newWorkspace
are handled separately: keep the current behavior for .newWorkspace, but for
.newTab create the workspace via addWorkspace and explicitly disable welcome
injection for command-created workspaces (e.g. set the workspace flag that
prevents auto welcome or avoid calling autoWelcomeIfNeeded on the new workspace)
before sending input with workspace.sendTerminalInputWhenReady(...); still call
scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id,
panelId: panel.id, reason: "custom_command") for the workspace as before.
In `@Sources/Workspace.swift`:
- Around line 8840-8843: The code validates overrideWorkingDirectory by trimming
whitespace but then returns the original untrimmed string; update the return to
use the trimmed value (e.g., compute let trimmed =
overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines) and
return trimmed) so the function returns a normalized/validated working directory
when overrideWorkingDirectory is present.
In `@tests_v2/test_custom_command_shortcut.py`:
- Around line 90-104: The test currently writes KEYBINDINGS_PATH in-place which
can leave a partially-written file; change _write_keybindings_config to write
JSON to a same-directory temporary file (use KEYBINDINGS_PATH.parent and a
unique temp filename) and then atomically replace the target with
os.replace(temp_path, KEYBINDINGS_PATH) after the write is complete; likewise
update _restore_keybindings_config to restore by writing the saved bytes to a
same-directory temp file and os.replace it into KEYBINDINGS_PATH (or unlink
KEYBINDINGS_PATH if previous is None, handling FileNotFoundError), ensuring
UTF-8 encoding when creating the temp file and preserving previous bytes in the
saved variable.
---
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 953-979: Replace the raw-injection helper
sendTerminalInputWhenReady with a new sendCustomCommandWhenReady(_ command:
String, cwd: String, to panel: TerminalPanel) that still waits for panel.surface
readiness (reuse the observer pattern in sendTerminalInputWhenReady) but instead
of sending text injects a dedicated custom-command launch API on the terminal
(invoke the terminal’s command-launcher rather than panel.sendInput). Construct
the launched command to include a cwd-guard (e.g., prefix the shell invocation
with a guaranteed cd to the provided cwd) and run via the proper shell execution
path (use /bin/sh -c or the repo’s launcher contract) so rc files cannot escape
the intended directory; remove the fragile "trimmedCommand + \"\n\"" raw-input
usage and the fixed 3s drop behavior, and update TabManager to call
sendCustomCommandWhenReady(..., cwd: resolvedWorkingDirectory, to: panel)
instead of sendTerminalInputWhenReady with the newline-injected string.
---
Nitpick comments:
In `@Sources/KeybindingsConfigFile.swift`:
- Around line 36-53: The decoder init (init(from decoder: Decoder)) currently
treats any string starting with "/" as a valid .absolutePath; change it to first
reject empty rawValue and rawValue == "/" (require at least one more path
character), expand leading "~" by calling NSString(string:
rawValue).expandingTildeInPath before validation (or explicitly reject "~" if
you choose not to support it), then validate the expanded string starts with "/"
and is longer than 1 before assigning .absolutePath(...); when throwing
DecodingError.dataCorruptedError include the offending rawValue in the
debugDescription to aid diagnostics (refer to rawValue, .absolutePath(rawValue),
container, and DecodingError.dataCorruptedError).
- Around line 4-7: The Schema struct uses a snake_case stored property
`custom_commands`; rename it to `customCommands` and add a CodingKeys enum to
map the JSON key to the new property name (e.g., enum CodingKeys: String,
CodingKey { case version, customCommands = "custom_commands" }) so Codable still
decodes/encodes the `custom_commands` JSON key; update any references to
`Schema.custom_commands` to `Schema.customCommands` throughout the codebase.
In `@Sources/KeyboardShortcutSettingsFileStore.swift`:
- Line 1530: The symbol ShortcutSettingsFileWatcher has been widened to module
scope but its name no longer reflects its multi-store role; rename
ShortcutSettingsFileWatcher (and any matching file/class declarations) to a
clearer name such as CmuxConfigFileWatcher or SettingsFileWatcher, update all
references (e.g., in KeyboardShortcutSettingsFileStore and CustomCommandStore
and any other consumers), and ensure the JSONCParser symbol remains unchanged;
adjust initializer signatures and documentation comments if present so callers
compile after the rename and run-time behavior is preserved.
🪄 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: 7005ff6f-825c-47b4-a7e6-5b7cbb351a1b
📒 Files selected for processing (9)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/CustomCommandStore.swiftSources/KeybindingsConfigFile.swiftSources/KeyboardShortcutSettings.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/TabManager.swiftSources/Workspace.swifttests_v2/test_custom_command_shortcut.py
| private func resolveBindings(_ rawBindings: [CustomCommandBinding]) -> [ResolvedBinding] { | ||
| var seenIDs = Set<String>() | ||
| var resolved: [ResolvedBinding] = [] | ||
|
|
||
| for binding in rawBindings { | ||
| let id = binding.id.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let shortcutString = binding.shortcut.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let command = binding.command.trimmingCharacters(in: .whitespacesAndNewlines) | ||
|
|
||
| guard !id.isEmpty else { | ||
| NSLog("[CustomCommandStore] ignoring custom command with empty id in %@", path) | ||
| continue | ||
| } | ||
| guard seenIDs.insert(id).inserted else { | ||
| NSLog("[CustomCommandStore] ignoring duplicate custom command id '%@' in %@", id, path) | ||
| continue | ||
| } | ||
| guard !shortcutString.isEmpty, | ||
| let shortcut = StoredShortcut.parse(rawValue: shortcutString), | ||
| !shortcut.hasChord else { | ||
| NSLog("[CustomCommandStore] ignoring custom command '%@' with invalid shortcut '%@' in %@", id, binding.shortcut, path) | ||
| continue | ||
| } | ||
| guard !command.isEmpty else { | ||
| NSLog("[CustomCommandStore] ignoring custom command '%@' with empty command in %@", id, path) | ||
| continue | ||
| } | ||
|
|
||
| resolved.append( | ||
| ResolvedBinding( | ||
| binding: CustomCommandBinding( | ||
| id: id, | ||
| shortcut: shortcutString, | ||
| command: command, | ||
| label: binding.label?.trimmingCharacters(in: .whitespacesAndNewlines), | ||
| target: binding.target, | ||
| cwd: binding.cwd | ||
| ), | ||
| shortcut: shortcut | ||
| ) | ||
| ) | ||
| } | ||
|
|
||
| return resolved |
There was a problem hiding this comment.
Reject duplicate custom-command shortcuts.
matchingCommand returns the first match, so two entries with different IDs but the same shortcut make the later command unreachable without any load-time warning. Track resolved shortcuts and ignore duplicates explicitly.
Proposed fix
private func resolveBindings(_ rawBindings: [CustomCommandBinding]) -> [ResolvedBinding] {
var seenIDs = Set<String>()
+ var seenShortcuts: [StoredShortcut] = []
var resolved: [ResolvedBinding] = []
@@
guard !shortcutString.isEmpty,
let shortcut = StoredShortcut.parse(rawValue: shortcutString),
!shortcut.hasChord else {
NSLog("[CustomCommandStore] ignoring custom command '%@' with invalid shortcut '%@' in %@", id, binding.shortcut, path)
continue
}
+ guard !seenShortcuts.contains(shortcut) else {
+ NSLog("[CustomCommandStore] ignoring custom command '%@' with duplicate shortcut '%@' in %@", id, shortcutString, path)
+ continue
+ }
guard !command.isEmpty else {
NSLog("[CustomCommandStore] ignoring custom command '%@' with empty command in %@", id, path)
continue
}
+ seenShortcuts.append(shortcut)
resolved.append(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CustomCommandStore.swift` around lines 53 - 96, The resolveBindings
function currently only deduplicates ids but not shortcut collisions; add a Set
to track seen shortcuts (e.g., seenShortcuts: Set<StoredShortcut> or Set<String>
using the parsed shortcut.rawValue) and, after successfully parsing
StoredShortcut in resolveBindings, check whether the parsed shortcut is already
in seenShortcuts; if it is, log a message similar to the existing NSLog that you
are ignoring the duplicate shortcut for the later CustomCommandBinding (include
id, binding.shortcut, path) and continue, otherwise insert the shortcut into
seenShortcuts and proceed to append the ResolvedBinding; update references
around ResolvedBinding, CustomCommandBinding and StoredShortcut.parse
accordingly.
| workspaceDirectory: workspaceDirectory, | ||
| paneDirectory: paneDirectory | ||
| ) | ||
| let input = trimmedCommand + "\n" |
There was a problem hiding this comment.
Run custom commands through /bin/sh -c, not the interactive shell.
Line 4999 sends the configured command directly to whatever shell the terminal starts. That misses the required /bin/sh -c semantics and lets user shell aliases/functions/rc state change behavior.
🐛 Proposed fix
- let input = trimmedCommand + "\n"
+ let input = "/bin/sh -c \(Self.shellSingleQuoted(trimmedCommand))\n"Add a small quoting helper on TabManager:
private nonisolated static func shellSingleQuoted(_ value: String) -> String {
"'\(value.replacingOccurrences(of: "'", with: "'\"'\"'"))'"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` at line 4999, The current code sends trimmedCommand
directly to the terminal (creating input = trimmedCommand + "\n") which bypasses
/bin/sh -c semantics; update TabManager to wrap the command in a /bin/sh -c
invocation and properly single-quote the command string: add a nonisolated
helper TabManager.shellSingleQuoted(_:) that escapes single quotes, then
construct input as "/bin/sh -c " + TabManager.shellSingleQuoted(trimmedCommand)
+ "\n" (or equivalent string concatenation) so the terminal receives the command
executed via /bin/sh -c with correct quoting.
| switch target { | ||
| case .splitRight, .splitDown: | ||
| guard let workspace = launchContext?.workspace, | ||
| let sourcePanelId = launchContext?.sourcePanelId else { | ||
| return false | ||
| } | ||
| workspace.clearSplitZoom() | ||
| let orientation: SplitOrientation = (target == .splitRight) ? .horizontal : .vertical | ||
| guard let panel = workspace.newTerminalSplit( | ||
| from: sourcePanelId, | ||
| orientation: orientation, | ||
| insertFirst: false, | ||
| focus: true, | ||
| workingDirectory: resolvedWorkingDirectory, | ||
| startupEnvironment: environment | ||
| ) else { | ||
| return false | ||
| } | ||
| workspace.sendTerminalInputWhenReady(input, to: panel) | ||
| scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command") | ||
| return true | ||
|
|
||
| case .newSurface: | ||
| guard let workspace = launchContext?.workspace else { return false } | ||
| let paneId = workspace.bonsplitController.focusedPaneId ?? workspace.bonsplitController.allPaneIds.first | ||
| guard let paneId else { return false } | ||
| workspace.clearSplitZoom() | ||
| guard let panel = workspace.newTerminalSurface( | ||
| inPane: paneId, | ||
| focus: true, | ||
| workingDirectory: resolvedWorkingDirectory, | ||
| startupEnvironment: environment | ||
| ) else { | ||
| return false | ||
| } | ||
| workspace.sendTerminalInputWhenReady(input, to: panel) | ||
| scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command") | ||
| return true | ||
|
|
||
| case .newTab, .newWorkspace: | ||
| let workspace = addWorkspace( | ||
| workingDirectory: resolvedWorkingDirectory, | ||
| initialTerminalEnvironment: environment, | ||
| select: true | ||
| ) | ||
| guard let panel = workspace.focusedTerminalPanel else { return false } | ||
| workspace.sendTerminalInputWhenReady(input, to: panel) | ||
| scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command") | ||
| return true | ||
| } |
There was a problem hiding this comment.
Add DEBUG-only launch logging without command contents.
This path creates splits/surfaces/workspaces but has no dlog() event. Add metadata-only logs around launch success/failure; avoid logging command because it can contain secrets. As per coding guidelines, all debug events for keys, focus, splits, and tabs must be logged using dlog() inside #if DEBUG / #endif.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 5001 - 5050, Add DEBUG-only dlog()
calls surrounding the launch success/failure branches for the switch on target:
for the .splitRight/.splitDown path (around workspace.newTerminalSplit and
before returning) and for .newSurface (around
bonsplitController.focusedPaneId/newTerminalSurface) and .newTab/.newWorkspace
(around addWorkspace and after obtaining workspace.focusedTerminalPanel). Wrap
each dlog() in `#if` DEBUG / `#endif`, log only metadata such as target enum value,
workspace.id, paneId or panel.id, and a short status string ("launch_success" /
"launch_failed"), and explicitly do NOT log the command/input contents or
environment; place the success log before
scheduleInitialWorkspaceGitMetadataRefreshIfPossible and the failure log where
the guard returns false.
| case .newTab, .newWorkspace: | ||
| let workspace = addWorkspace( | ||
| workingDirectory: resolvedWorkingDirectory, | ||
| initialTerminalEnvironment: environment, | ||
| select: true | ||
| ) | ||
| guard let panel = workspace.focusedTerminalPanel else { return false } | ||
| workspace.sendTerminalInputWhenReady(input, to: panel) | ||
| scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command") |
There was a problem hiding this comment.
Keep new_tab distinct and disable welcome injection for command workspaces.
case .newTab, .newWorkspace currently makes both targets create a workspace. It also leaves autoWelcomeIfNeeded enabled, so first-run cmux welcome can be sent into the same terminal as the custom command.
🐛 Proposed direction
- case .newTab, .newWorkspace:
+ case .newTab:
+ guard let workspace = launchContext?.workspace else { return false }
+ let paneId = workspace.bonsplitController.focusedPaneId ?? workspace.bonsplitController.allPaneIds.first
+ guard let paneId else { return false }
+ workspace.clearSplitZoom()
+ guard let panel = workspace.newTerminalSurface(
+ inPane: paneId,
+ focus: true,
+ workingDirectory: resolvedWorkingDirectory,
+ startupEnvironment: environment
+ ) else {
+ return false
+ }
+ workspace.sendTerminalInputWhenReady(input, to: panel)
+ scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id, panelId: panel.id, reason: "custom_command")
+ return true
+
+ case .newWorkspace:
let workspace = addWorkspace(
workingDirectory: resolvedWorkingDirectory,
initialTerminalEnvironment: environment,
- select: true
+ select: true,
+ autoWelcomeIfNeeded: false
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 5040 - 5048, Split the combined case
so .newTab and .newWorkspace are handled separately: keep the current behavior
for .newWorkspace, but for .newTab create the workspace via addWorkspace and
explicitly disable welcome injection for command-created workspaces (e.g. set
the workspace flag that prevents auto welcome or avoid calling
autoWelcomeIfNeeded on the new workspace) before sending input with
workspace.sendTerminalInputWhenReady(...); still call
scheduleInitialWorkspaceGitMetadataRefreshIfPossible(workspaceId: workspace.id,
panelId: panel.id, reason: "custom_command") for the workspace as before.
| if let overrideWorkingDirectory, | ||
| !overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return overrideWorkingDirectory | ||
| } |
There was a problem hiding this comment.
Return the normalized override cwd.
Line 8841 validates the trimmed value, but Line 8842 returns the original string. A JSON cwd with accidental surrounding whitespace/newlines will pass validation and then fail as an invalid working directory.
Suggested fix
- if let overrideWorkingDirectory,
- !overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
- return overrideWorkingDirectory
+ if let overrideWorkingDirectory {
+ let trimmedOverride = overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines)
+ if !trimmedOverride.isEmpty {
+ return trimmedOverride
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 8840 - 8843, The code validates
overrideWorkingDirectory by trimming whitespace but then returns the original
untrimmed string; update the return to use the trimmed value (e.g., compute let
trimmed = overrideWorkingDirectory.trimmingCharacters(in:
.whitespacesAndNewlines) and return trimmed) so the function returns a
normalized/validated working directory when overrideWorkingDirectory is present.
| def _write_keybindings_config(payload: dict) -> bytes | None: | ||
| KEYBINDINGS_PATH.parent.mkdir(parents=True, exist_ok=True) | ||
| previous = KEYBINDINGS_PATH.read_bytes() if KEYBINDINGS_PATH.exists() else None | ||
| KEYBINDINGS_PATH.write_text(json.dumps(payload, indent=2), encoding="utf-8") | ||
| return previous | ||
|
|
||
|
|
||
| def _restore_keybindings_config(previous: bytes | None) -> None: | ||
| if previous is None: | ||
| try: | ||
| KEYBINDINGS_PATH.unlink() | ||
| except FileNotFoundError: | ||
| pass | ||
| return | ||
| KEYBINDINGS_PATH.write_bytes(previous) |
There was a problem hiding this comment.
Write and restore keybindings atomically.
Line 93 truncates the live-reloaded config in place, so the watcher can parse an empty/partial file and clear the binding before Line 160 simulates the shortcut. Use a same-directory temp file plus os.replace for both test setup and restore.
Proposed fix
+def _atomic_write_keybindings(contents: bytes) -> None:
+ KEYBINDINGS_PATH.parent.mkdir(parents=True, exist_ok=True)
+ fd, temp_name = tempfile.mkstemp(
+ prefix=f".{KEYBINDINGS_PATH.name}.",
+ suffix=".tmp",
+ dir=str(KEYBINDINGS_PATH.parent),
+ )
+ temp_path = Path(temp_name)
+ try:
+ with os.fdopen(fd, "wb") as handle:
+ handle.write(contents)
+ handle.flush()
+ os.fsync(handle.fileno())
+ os.replace(temp_path, KEYBINDINGS_PATH)
+ finally:
+ try:
+ temp_path.unlink()
+ except FileNotFoundError:
+ pass
+
+
def _write_keybindings_config(payload: dict) -> bytes | None:
- KEYBINDINGS_PATH.parent.mkdir(parents=True, exist_ok=True)
previous = KEYBINDINGS_PATH.read_bytes() if KEYBINDINGS_PATH.exists() else None
- KEYBINDINGS_PATH.write_text(json.dumps(payload, indent=2), encoding="utf-8")
+ _atomic_write_keybindings(json.dumps(payload, indent=2).encode("utf-8"))
return previous
def _restore_keybindings_config(previous: bytes | None) -> None:
@@
except FileNotFoundError:
pass
return
- KEYBINDINGS_PATH.write_bytes(previous)
+ _atomic_write_keybindings(previous)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_custom_command_shortcut.py` around lines 90 - 104, The test
currently writes KEYBINDINGS_PATH in-place which can leave a partially-written
file; change _write_keybindings_config to write JSON to a same-directory
temporary file (use KEYBINDINGS_PATH.parent and a unique temp filename) and then
atomically replace the target with os.replace(temp_path, KEYBINDINGS_PATH) after
the write is complete; likewise update _restore_keybindings_config to restore by
writing the saved bytes to a same-directory temp file and os.replace it into
KEYBINDINGS_PATH (or unlink KEYBINDINGS_PATH if previous is None, handling
FileNotFoundError), ensuring UTF-8 encoding when creating the temp file and
preserving previous bytes in the saved variable.
![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 91d4d59. Configure here.
| command: customCommand.command, | ||
| customCommandID: customCommand.id | ||
| ) ?? false | ||
| } |
There was a problem hiding this comment.
Custom commands silently shadow later built-in shortcuts
Medium Severity
The custom command shortcut check is inserted in the middle of the built-in shortcut processing chain — after core shortcuts like newSurface but before many others including openBrowser, find, focusBrowserAddressBar, browser navigation, developer tools, zoom, and more. A user-defined custom command whose shortcut collides with any of those later built-in shortcuts will silently shadow the built-in, while a collision with an earlier shortcut silently ignores the custom command. There is no conflict detection or warning, and the user has no way to predict which built-in shortcuts are "overridable" since the priority depends entirely on internal evaluation order.
Reviewed by Cursor Bugbot for commit 91d4d59. Configure here.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 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="tests_v2/test_custom_command_shortcut.py">
<violation number="1" location="tests_v2/test_custom_command_shortcut.py:139">
P2: Capture the existing keybindings backup before writing, so a write failure cannot lose the original file during cleanup.</violation>
</file>
<file name="Sources/CustomCommandStore.swift">
<violation number="1" location="Sources/CustomCommandStore.swift:41">
P1: A single malformed entry in `custom_commands` (e.g. a typo like `"split_left"` in the `target` field) causes `JSONDecoder().decode` to throw for the entire array, and the `catch` block wipes `bindings = []`. This means one bad entry silently disables every other working custom-command shortcut. Decode each entry individually (e.g. decode as `[AnyCodable]` or wrap per-element decode in try/catch) so that one invalid entry is skipped while the rest continue to work.</violation>
</file>
<file name="Sources/Workspace.swift">
<violation number="1" location="Sources/Workspace.swift:8842">
P2: The trimmed value is validated but the original (potentially whitespace-padded) `overrideWorkingDirectory` is returned. Return the trimmed string instead to avoid passing a path with leading/trailing whitespace as a working directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| do { | ||
| let sanitized = try JSONCParser.preprocess(data: data) | ||
| let schema = try JSONDecoder().decode(KeybindingsConfigFile.Schema.self, from: sanitized) |
There was a problem hiding this comment.
P1: A single malformed entry in custom_commands (e.g. a typo like "split_left" in the target field) causes JSONDecoder().decode to throw for the entire array, and the catch block wipes bindings = []. This means one bad entry silently disables every other working custom-command shortcut. Decode each entry individually (e.g. decode as [AnyCodable] or wrap per-element decode in try/catch) so that one invalid entry is skipped while the rest continue to work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CustomCommandStore.swift, line 41:
<comment>A single malformed entry in `custom_commands` (e.g. a typo like `"split_left"` in the `target` field) causes `JSONDecoder().decode` to throw for the entire array, and the `catch` block wipes `bindings = []`. This means one bad entry silently disables every other working custom-command shortcut. Decode each entry individually (e.g. decode as `[AnyCodable]` or wrap per-element decode in try/catch) so that one invalid entry is skipped while the rest continue to work.</comment>
<file context>
@@ -0,0 +1,103 @@
+
+ do {
+ let sanitized = try JSONCParser.preprocess(data: data)
+ let schema = try JSONDecoder().decode(KeybindingsConfigFile.Schema.self, from: sanitized)
+ bindings = resolveBindings(schema.custom_commands ?? [])
+ } catch {
</file context>
| baseline_workspace = client.current_workspace() | ||
| created_workspace = "" | ||
| try: | ||
| previous_keybindings = _write_keybindings_config(config) |
There was a problem hiding this comment.
P2: Capture the existing keybindings backup before writing, so a write failure cannot lose the original file during cleanup.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests_v2/test_custom_command_shortcut.py, line 139:
<comment>Capture the existing keybindings backup before writing, so a write failure cannot lose the original file during cleanup.</comment>
<file context>
@@ -0,0 +1,212 @@
+ baseline_workspace = client.current_workspace()
+ created_workspace = ""
+ try:
+ previous_keybindings = _write_keybindings_config(config)
+ time.sleep(0.5)
+
</file context>
| let splitWorkingDirectory: String? = { | ||
| if let overrideWorkingDirectory, | ||
| !overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty { | ||
| return overrideWorkingDirectory |
There was a problem hiding this comment.
P2: The trimmed value is validated but the original (potentially whitespace-padded) overrideWorkingDirectory is returned. Return the trimmed string instead to avoid passing a path with leading/trailing whitespace as a working directory.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Workspace.swift, line 8842:
<comment>The trimmed value is validated but the original (potentially whitespace-padded) `overrideWorkingDirectory` is returned. Return the trimmed string instead to avoid passing a path with leading/trailing whitespace as a working directory.</comment>
<file context>
@@ -8835,6 +8837,10 @@ final class Workspace: Identifiable, ObservableObject {
let splitWorkingDirectory: String? = {
+ if let overrideWorkingDirectory,
+ !overrideWorkingDirectory.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty {
+ return overrideWorkingDirectory
+ }
if let panelDirectory = panelDirectories[panelId]?.trimmingCharacters(in: .whitespacesAndNewlines),
</file context>
…-3019-preset-command-shortcut # Conflicts: # GhosttyTabs.xcodeproj/project.pbxproj
|
This functionality is already mostly available via command actions in cmux.json, see my reply to your issue here: #3019 (comment) |
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
4 participants
Summary
~/.config/cmux/keybindings.jsoncustom command store for preset shortcut bindingsCloses #3019
Note
Medium Risk
Adds a new config-driven path that spawns terminal surfaces and injects user-defined commands, which can impact shortcut routing and terminal startup/cwd/env handling. Live-reloading file parsing and new surface creation logic increases the chance of edge-case regressions but is scoped and guarded with validation plus a regression test.
Overview
Adds support for custom command shortcuts loaded from a live-reloaded
~/.config/cmux/keybindings.json, allowing a single-stroke shortcut to open a target surface (split/surface/tab/workspace) and run a configured shell command.Implements a new
CustomCommandStore+KeybindingsConfigFileschema, wires shortcut matching intoAppDelegate’s key handling, and addsTabManager.openSurfaceAndRunCommand(...)to resolve CWD policy, injectCMUX_WORKSPACE_CWD/CMUX_PANE_CWD/CMUX_CUSTOM_COMMAND_ID, and send input once the PTY is ready.Refactors shortcut-string parsing into
StoredShortcut.parse/ShortcutStroke.parse(reused by settings file parsing), expandsWorkspace.newTerminalSplitto accept an override working directory and extra environment, and adds a Python regression test that simulates the shortcut and verifies split creation, cwd/env propagation, and command execution.Reviewed by Cursor Bugbot for commit 97adb98. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds custom command shortcuts via a live‑reloaded
~/.config/cmux/keybindings.json. Press a matching single‑stroke shortcut to open a split/tab/surface/workspace and run the configured command with the correct CWD and env. Addresses #3019.New Features
CustomCommandStorereads and live‑reloads~/.config/cmux/keybindings.json(JSON/JSONC).id,shortcut,command,target(split_right,split_down,new_surface,new_tab,new_workspace), optionallabel, andcwd(workspace,pane, or absolute path).StoredShortcut.parse/ShortcutStroke.parse; matched before chord handling; chords are disallowed.TabManager.openSurfaceAndRunCommand(...)spawns target and injects input when PTY is ready; setsCMUX_WORKSPACE_CWD,CMUX_PANE_CWD,CMUX_CUSTOM_COMMAND_ID; resolves CWD per policy.Workspace.newTerminalSplit(...)accepts a working‑directory override and extra environment.Migration
~/.config/cmux/keybindings.jsonwith acustom_commandsarray; changes are picked up automatically.settings.jsonshortcuts are unchanged; custom commands fire only on single‑stroke shortcuts not used in chords.Written for commit 97adb98. Summary will update on new commits.
Summary by CodeRabbit