Repository navigation
Parameterized custom commands: {{variable}} prompts + folder organization - #6898
Closed
austinywang wants to merge 27 commits into
Closed
austinywang wants to merge 27 commits into
austinywang wants to merge 27 commits into
Conversation
Implements the two improvements requested in #6557 for cmux.json `commands`: 1. Variables — `{{variable}}` (and `{{variable=default}}`) placeholders in a command's `command` string. cmux prompts for each value before running and substitutes them back in. Wired into the single `CmuxConfigExecutor.prepareShellInputIfAuthorized` choke point so every entrypoint (Command Palette, surface tab-bar buttons, dock) gets prompting without per-surface duplication. New `CmuxCommandVariableParser` does the shell-safe parsing/substitution; `CmuxCommandVariablePrompt` shows the inline NSAlert prompt. 2. Folder organization — a `folder` key (e.g. "Project/Linting") groups a command in the Command Palette. The breadcrumb shows as the row's trailing badge and feeds the search corpus/keywords so you can filter by folder. Also updates the JSON schema, the custom-commands docs page (all web locales), and adds Localizable.xcstrings entries for the prompt across all locales. Tests: CmuxCommandVariableTests covers parsing, de-duplication, shell-safety, substitution, folder normalization, and palette resolution. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 command-variable prompting, folder-aware command identity and metadata, palette kind labels, and matching schema, docs, localization, tests, and project wiring. ChangesCommand Variables and Folder Organization
Sequence Diagram(s)sequenceDiagram
participant CmuxConfigExecutor
participant CmuxCommandTemplate
participant authorizeProjectActionIfNeeded
participant CmuxCommandVariablePrompt
participant onAuthorized
CmuxConfigExecutor->>CmuxCommandTemplate: init(rawCommand)
CmuxCommandTemplate-->>CmuxConfigExecutor: variables
CmuxConfigExecutor->>authorizeProjectActionIfNeeded: authorizeProjectActionIfNeeded(presentingWindow)
authorizeProjectActionIfNeeded-->>CmuxConfigExecutor: authorized
alt no variables
CmuxConfigExecutor->>onAuthorized: displayCommand + "\n"
else variables present
CmuxConfigExecutor->>CmuxCommandVariablePrompt: present(in:completion:)
CmuxCommandVariablePrompt-->>CmuxConfigExecutor: substitutions
CmuxConfigExecutor->>onAuthorized: resolved command + "\n"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (22 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 SummaryThis PR adds two complementary features to
Confidence Score: 5/5Safe to merge; the new features are well-scoped, fully tested, and wired through a single authorization choke point that preserves the existing trust/confirm model. The variable-substitution and folder-grouping changes are additive and backward-compatible: folderless command IDs keep the same format as before, trust decisions persist correctly against the template string, and workspace dispatch now resolves by ID-then-name with a correct fail-closed fallback for ambiguous names. The parser handles all documented edge cases (quotes, here-docs, backslash escapes, comments) with direct test coverage. The one observation is cosmetic — the folder breadcrumb shows up in both the palette subtitle and the trailing badge for folder-only commands — and does not affect correctness. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant U as User
participant E as CmuxConfigExecutor
participant T as CmuxCommandTemplate
participant A as CmuxActionTrust
participant P as CmuxCommandVariablePrompt
participant S as Shell/Terminal
U->>E: trigger command (palette / button / dock)
E->>T: CmuxCommandTemplate(rawValue:)
T-->>E: template
E->>T: template.variables
T-->>E: [CmuxCommandVariable] parsed from unquoted placeholders
E->>E: sanitizeForDisplay(rawCommand) displays command
E->>A: isTrusted(descriptor) / confirm dialog
A-->>E: authorized
alt variables is empty
E->>S: displayCommand + newline
else variables present
E->>P: CmuxCommandVariablePrompt.present(in:completion:)
P-->>U: NSAlert with per-variable text fields
U->>P: fill values + click Run
P->>T: template.substituting(values) shellQuotes each value
T-->>P: resolved command string
P->>E: sanitizeForDisplay(resolved)
E->>S: resolved + newline
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant U as User
participant E as CmuxConfigExecutor
participant T as CmuxCommandTemplate
participant A as CmuxActionTrust
participant P as CmuxCommandVariablePrompt
participant S as Shell/Terminal
U->>E: trigger command (palette / button / dock)
E->>T: CmuxCommandTemplate(rawValue:)
T-->>E: template
E->>T: template.variables
T-->>E: [CmuxCommandVariable] parsed from unquoted placeholders
E->>E: sanitizeForDisplay(rawCommand) displays command
E->>A: isTrusted(descriptor) / confirm dialog
A-->>E: authorized
alt variables is empty
E->>S: displayCommand + newline
else variables present
E->>P: CmuxCommandVariablePrompt.present(in:completion:)
P-->>U: NSAlert with per-variable text fields
U->>P: fill values + click Run
P->>T: template.substituting(values) shellQuotes each value
T-->>P: resolved command string
P->>E: sanitizeForDisplay(resolved)
E->>S: resolved + newline
end
Reviews (19): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// `_ - .`. Anything else (`$`, `(`, `/`, `|`, …) makes the braces literal so | ||
| /// ordinary shell snippets that happen to use `{{` are never mistaken for a | ||
| /// variable. | ||
| enum CmuxCommandVariableParser { |
Contributor
There was a problem hiding this comment.
CmuxCommandVariableParser is a caseless enum whose entire API is static functions — exactly the shape the cmux-no-ambient-global-state rule flags. Because it has no access modifier it is internal, so it is exported across the module as a static-only namespace. The rule requires this logic to live as methods on a constructable, injectable type or as private/fileprivate file-scope helpers; a caseless enum is explicitly called out as a pattern to avoid. The same concern applies to CmuxCommandVariablePrompt in CmuxCommandVariablePrompt.swift. Prefer a struct CmuxCommandVariableParser with instance methods (or promote the helpers to private file-scope free functions), and correspondingly make CmuxCommandVariablePrompt.present a static method on CmuxConfigExecutor where the caller already lives.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Contributor
Author
There was a problem hiding this comment.
Addressed in 505009d: refactored both new types away from static-only namespaces. CmuxCommandVariableParser (caseless enum) is now CmuxCommandTemplate(rawValue:), a constructable value type with instance members (variables, containsVariables, substituting(_:)); CmuxCommandVariablePrompt is now a struct with stored variables/displayTitle and an instance present(in:completion:). No ambient/global state is introduced.
— Claude Code
CmuxCommandVariables.swift and CmuxCommandVariablePrompt.swift were added to the worktree but not registered in project.pbxproj, so the app target failed to compile with "Cannot find 'CmuxCommandVariableParser'/'CmuxCommandVariablePrompt' in scope". Add the four pbxproj entries for each file (build file, file reference, group child, Sources build phase). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Address review: the initial parser treated any {{...}} with letters/dots/spaces
as a cmux variable, which would hijack existing custom commands that embed
Go/Handlebars-style templates such as {{ .Env.FOO }}.
- Narrow the grammar to bare identifiers ([A-Za-z_][A-Za-z0-9_-]*): template
expressions with leading dots, internal spaces, pipes, or functions are now
left completely untouched and run as-is.
- Add a backslash escape: \{{name}} forces a literal {{name}} (the backslash is
stripped at run time, no prompt). The executor's no-variable path now strips
escapes too.
- Document the identifier rule and escape in the JSON schema.
- Docs messages rendered via plain next-intl t() must be ICU-safe, so the
command-variable doc strings are brace-free prose (the {{...}} syntax is shown
in the adjacent CodeBlock, which is raw JSX); add a template/escape note across
all web locales.
- Expand tests: template expressions stay literal, backslash escape, mixed
escaped/unescaped occurrences.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The backslash-escape handling added three lines (577 -> 580). Refresh just that file's entry in the length budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Sources/CmuxCommandVariablePrompt.swift`:
- Around line 37-52: The `CmuxCommandVariablePrompt` alert uses the reused
`common.cancel` localization key, but the Khmer locale is missing from
`Resources/Localizable.xcstrings`. Add the `km` translation entry for
`common.cancel` so the Cancel button is localized consistently with the new
`dialog.cmuxConfig.commandVariables.*` strings.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 137-183: prepareShellInputIfAuthorized currently reports success
inconsistently when command variables are involved: the no-variable path returns
the real authorization result, but the variable path always returns the result
of CmuxCommandVariablePrompt.present even when the prompt is cancelled. Update
the control flow so the Bool reflects actual execution state, either by making
CmuxCommandVariablePrompt.present return false on cancel, or by changing
prepareShellInputIfAuthorized/its callers (such as Workspace) to only treat the
closure path as executed after confirmation.
In `@web/data/cmux.schema.json`:
- Around line 39-70: Enforce the mutual exclusivity between the command and
workspace entries in the cmux schema so validation matches the runtime contract.
Update the schema around the `properties.command` and `properties.workspace`
definitions to require exactly one of them (and reject objects that contain both
or neither), keeping the existing descriptions intact. Use the same schema
object that currently defines `name`, `command`, `folder`, and `workspace` so
editor validation catches invalid command entries.
🪄 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: d733f955-20c2-464f-9c55-015cfd992ba1
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (31)
Packages/macOS/CmuxCommandPalette/Sources/CmuxCommandPalette/Handling/CommandPaletteCommandContribution.swiftResources/Localizable.xcstringsSources/CmuxCommandVariablePrompt.swiftSources/CmuxCommandVariables.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/ContentView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxCommandVariableTests.swiftweb/app/[locale]/docs/custom-commands/page.tsxweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
Address review (P1): substitution inserted the entered value directly into the
shell command, so a value containing shell metacharacters (e.g. a pasted
branch/label like "main; rm -rf /") could break out of the intended command.
Each substituted value is now wrapped as a single POSIX-quoted shell argument
(single quotes with embedded `'` escaped as `'\''`), so whatever the user types
is passed literally. This covers every documented example
(`--env {{environment}}` -> `--env 'staging'`, `git checkout {{branch}}` ->
`git checkout 'main'`). Escaped `\{{…}}` literals and unrecognized template
expressions are left unquoted.
Document the quoting in the JSON schema and add tests for metacharacter
neutralization and single-quote escaping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Refactor the static `CmuxCommandVariableParser` enum and `CmuxCommandVariablePrompt` static enum into constructable value types (`CmuxCommandTemplate(rawValue:)` with instance members, and a `CmuxCommandVariablePrompt` struct with an instance `present(in:)`), per the no-ambient-global-state guidance — no static-only namespaces. - Enforce command/workspace exclusivity in the JSON schema with `oneOf`, matching the runtime decoder which requires exactly one. - Add the missing Khmer (`km`) translation for the reused `common.cancel` key. - Document the `prepareShellInputIfAuthorized` return contract: `true` means the request was accepted/presented, not that it executed (consistent with the existing async confirm-dialog path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (P1 + P2): the previous approach shell-quoted substituted values
but could still be unsafe when the author wrapped a placeholder in quotes
(`deploy '{{branch}}'` -> the value could break out), and it intercepted bare
template placeholders like `helm ... '{{tag}}'`.
Scan the command tracking single/double shell-quote state and treat a
`{{identifier}}` as a cmux variable only when it appears at an unquoted
position. One rule fixes both:
- Quoted placeholders (`'{{tag}}'`, `"{{x}}"`, `'{{ .Env.FOO }}'`) are template
text and run unchanged — existing template commands are not intercepted.
- Variables only ever substitute at unquoted positions, where the value is
POSIX-quoted as one argument and cannot break out of the command.
Drops the backslash escape (quoting a placeholder is now the way to keep it
literal). Updates schema/docs and replaces the escape tests with quote-position
and mixed quoted/unquoted coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@Sources/CmuxCommandVariables.swift`:
- Around line 69-103: The doc comment for substituting(_:) claims that \{{…}} is
treated as a literal placeholder, but scan() and the .variable handling
currently do not implement that escape behavior. Update
CmuxCommandVariables.scan() and/or the substitution loop so backslash-escaped
placeholders are emitted literally and not passed through shellQuote, or remove
the unsupported claim from the docstring if you are not adding the escape
support. Make sure the behavior and documentation match for the
CmuxCommandVariables.substituting(_:) and scan() paths.
🪄 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: 4029649f-19c9-4e6d-acb0-0b04ba8f0eeb
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (26)
Resources/Localizable.xcstringsSources/CmuxCommandVariablePrompt.swiftSources/CmuxCommandVariables.swiftSources/CmuxConfigExecutor.swiftcmuxTests/CmuxCommandVariableTests.swiftweb/data/cmux.schema.jsonweb/messages/ar.jsonweb/messages/bs.jsonweb/messages/da.jsonweb/messages/de.jsonweb/messages/en.jsonweb/messages/es.jsonweb/messages/fr.jsonweb/messages/it.jsonweb/messages/ja.jsonweb/messages/km.jsonweb/messages/ko.jsonweb/messages/no.jsonweb/messages/pl.jsonweb/messages/pt-BR.jsonweb/messages/ru.jsonweb/messages/th.jsonweb/messages/tr.jsonweb/messages/uk.jsonweb/messages/zh-CN.jsonweb/messages/zh-TW.json
Address review:
- [P1] The quote scanner toggled state on every quote, so a backslash-escaped
quote (`echo "... \" {{x}} ..."`) made a still-quoted placeholder look
unquoted and get substituted, defeating the quoting safety. The scanner now
models shell escaping: backslash escapes the next character outside single
quotes (so `\"` does not close a double-quoted span), and single quotes are
literal-only. A placeholder after an escaped quote stays quoted and is not a
variable.
- [P2] The variable prompt ran before the project-action trust/confirm dialog,
letting an untrusted .cmux/cmux.json solicit values first. Authorization now
happens first, keyed on the command template (with its {{…}} placeholders) and
shown before any values are collected; only after the user approves does cmux
prompt for variables, substitute, and dispatch. Trust now also persists across
values since it is keyed on the template.
Adds an escaped-quote regression test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The optional already defaults to nil in Swift's synthesized memberwise initializer (the app target compiles and CI's test build succeeded with it), but spell the `= nil` out so the source-compatibility of the existing call sites is obvious to readers and reviewers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
+69
to
+96
| /// Resolves the command for execution: replaces every recognized | ||
| /// placeholder whose name is present in `values` with its value as a single | ||
| /// POSIX-quoted shell argument, leaves placeholders whose name is missing | ||
| /// from `values` untouched, leaves non-identifier `{{…}}` template | ||
| /// expressions untouched, and strips the escaping backslash from any | ||
| /// `\{{…}}` so it becomes a literal `{{…}}`. | ||
| /// | ||
| /// Values are shell-quoted so that whatever the user types is passed as one | ||
| /// literal argument — a value like `main; rm -rf /` cannot break out of the | ||
| /// command and run as separate shell words. | ||
| func substituting(_ values: [String: String]) -> String { | ||
| guard rawValue.contains("{{") else { return rawValue } | ||
| var result = "" | ||
| result.reserveCapacity(rawValue.count) | ||
| for token in scan() { | ||
| switch token { | ||
| case .literal(let text): | ||
| result.append(text) | ||
| case .variable(let variable, let raw): | ||
| if let value = values[variable.name] { | ||
| result.append(Self.shellQuote(value)) | ||
| } else { | ||
| result.append(raw) | ||
| } | ||
| } | ||
| } | ||
| return result | ||
| } |
Contributor
There was a problem hiding this comment.
\{{name}} escape is documented but not implemented
The substituting(_:) docstring promises "strips the escaping backslash from any \{{…}} so it becomes a literal {{…}}", but the scanner does not strip the backslash. When the backslash handler fires at position i, it jumps index to i+2, which breaks the {{ detection (the remaining single { doesn't trigger the double-brace check). However, literalStart is never updated, so flushLiteral later emits the original text verbatim — including the backslash.
A user who writes deploy \{{template_var}} production expecting deploy {{template_var}} production to reach the shell (e.g. to pass a literal Mustache placeholder to a downstream tool) will instead send deploy \{{template_var}} production. In bash/zsh, \{{ is interpreted as { + {, so the shell ultimately receives {template_var}} production — a broken command, not {{template_var}} as the doc promises. There is no test covering this escape path.
Contributor
Author
There was a problem hiding this comment.
Fixed in 6849901. The backslash-escape mechanism was removed during the quote-position rewrite (a literal {{name}} is now kept by quoting it or placing it in a here-doc/comment, not by escaping). The substituting() docstring was stale; it now describes the actual behavior and no longer claims to strip a backslash.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Addressed in the current head. CmuxCommandTemplate.substituting(_:) now tracks escaped literal placeholders separately from variables and strips the leading backslash for unquoted {{name}} without prompting or shell-quoting it. Added Swift Testing coverage for mixed escaped-literal and real variable substitution.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Verified on current head cb787ed: escaped unquoted placeholders are implemented in Sources/CmuxCommandTemplate.swift by marking {{name}} template matches with stripEscape and emitting the placeholder without the leading backslash or shell quoting. cmuxTests/CmuxCommandVariableTests.swift includes escapedUnquotedPlaceholderIsLiteral coverage.
— Claude Code
The new oneOf left command/workspace entries valid without a name, but CmuxCommandDefinition.init(from:) requires a non-blank name. Add required: ["name"] and a \S pattern so editor validation matches what the app actually accepts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/CmuxCommandVariables.swift (1)
69-74: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring still claims a
\{{…}}escape thatscan()doesn't implement.Lines 73-74 say
substituting(_:)"strips the escaping backslash from any\{{…}}so it becomes a literal{{…}}". Tracingscan()for\{{name}}: the backslash branch (Line 144) skips past\and the first{; the remaining{name}}begins with a single{, so no placeholder is recognized andliteralStartis never advanced. The output therefore keeps the backslash verbatim (\{{name}}), not{{name}}. The shell-escape work in this PR only fixed quote-state tracking, not backslash stripping before{{. Either implement the strip or drop the claim from the doc comment.🤖 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/CmuxCommandVariables.swift` around lines 69 - 74, The doc comment on substituting(_:) in CmuxCommandVariables.swift promises that escaped template text like \{{...}} has its backslash stripped, but scan() does not implement that behavior. Either update scan() to recognize \{{...}} and emit the unescaped literal {{...}}, or revise the docstring so it matches the current behavior of leaving the backslash intact.
🤖 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/CmuxCommandVariables.swift`:
- Around line 69-74: The doc comment on substituting(_:) in
CmuxCommandVariables.swift promises that escaped template text like \{{...}} has
its backslash stripped, but scan() does not implement that behavior. Either
update scan() to recognize \{{...}} and emit the unescaped literal {{...}}, or
revise the docstring so it matches the current behavior of leaving the backslash
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e2d274a6-d83b-47bd-a18a-8464b6018776
📒 Files selected for processing (5)
Sources/CmuxCommandVariables.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftcmuxTests/CmuxCommandVariableTests.swiftweb/data/cmux.schema.json
Per the repo testing policy, new non-UI test suites use Swift Testing rather than XCTest. Convert CmuxCommandVariableTests to `@Suite`/`@Test`/`#expect`/ `#require` (the folder suite stays `@MainActor` for the CmuxConfigStore case). No behavior change; same coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Split the CmuxCommandVariable DTO into its own file and rename CmuxCommandVariables.swift -> CmuxCommandTemplate.swift so each new source file declares a single major type. - Drop the nested FlippedView helper from CmuxCommandVariablePrompt by laying the accessory rows out in AppKit's native bottom-left coordinate space. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (P1): the resolved command is delivered as interactive terminal
input, so line-editor control bytes (Ctrl-U, newline, ESC, DEL, C1) are
interpreted by readline/zle before the shell parses the quotes. A value like
"\u{15}rm -rf ~ #" could clear the quoted prefix and run as its own command
despite the single-quoting.
shellQuote now drops C0/C1 control characters and DEL from the value before
quoting, so quoting is actually sufficient. Legitimate argument values never
need control bytes. Adds a regression test for the Ctrl-U clear-line vector.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (P2): the scanner recognized `{{name}}` anywhere unquoted,
including inside here-doc bodies, so a command embedding a template via a
heredoc (`cat <<'EOF'\n{{name}}\nEOF`) would prompt and rewrite the template
body. Complete the "unquoted shell-word position" rule by also skipping:
- here-doc bodies (`<<EOF`/`<<-EOF`, quoted or bare delimiter, multiple per
line), while still substituting variables on the operator line and after the
terminator; `<<<` here-strings remain normal words
- `#` comments at a word boundary
Adds heredoc/comment/here-string regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The scanner emitted a private `Token` enum (the file's second type, flagged by the file-organization policy). Replace it with a scan that returns the variable placeholders and their character ranges; `substituting` reconstructs the literal gaps from those ranges. One major type per file, no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review (P2): command ids were derived from name only, so two foldered commands with the same leaf name (e.g. Frontend/Build and Backend/Build) shared an id and the registration loop silently dropped the second from the Command Palette. Fold the normalized folder path into the id so they stay distinct; folderless commands keep their historical cmux.config.command.<name> id. Adds identity + end-to-end "same leaf name, different folders" tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the folder-identity fix: CmuxConfigStore.loadAll() still de-duped commands by name, so foldered same-leaf-name commands (Frontend/Build, Backend/Build) were dropped at load time before the folder-aware ids were ever used. Key the dedup set by command.id (folder + name) so both load while local config still overrides the matching global command. This is the path the sameLeafNameInDifferentFolders regression test exercises. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment on lines
1627
to
+1634
| var id: String { | ||
| "cmux.config.command." + (name.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? name) | ||
| // Fold the folder path into the identity so same-named commands in | ||
| // different folders (e.g. `Frontend/Build` and `Backend/Build`) stay | ||
| // distinct rather than colliding. Folderless commands keep their | ||
| // historical `cmux.config.command.<name>` id. | ||
| let key = (folderComponents + [name]).joined(separator: "/") | ||
| return "cmux.config.command." + (key.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? key) | ||
| } |
Contributor
There was a problem hiding this comment.
/. The current logic joins folder components and name with / and then percent-encodes the whole string — but that means folder="Frontend", name="Build" and folder=nil, name="Frontend/Build" both produce the key "Frontend/Build", which encodes identically to "Frontend%2FBuild". In loadAll() the second command is silently dropped via the seenCommandIDs check, giving the user no error or warning. The fix is to encode each part individually before joining so the separator / is distinguishable from a literal / in the name.
Suggested change
| var id: String { | |
| "cmux.config.command." + (name.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? name) | |
| // Fold the folder path into the identity so same-named commands in | |
| // different folders (e.g. `Frontend/Build` and `Backend/Build`) stay | |
| // distinct rather than colliding. Folderless commands keep their | |
| // historical `cmux.config.command.<name>` id. | |
| let key = (folderComponents + [name]).joined(separator: "/") | |
| return "cmux.config.command." + (key.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? key) | |
| } | |
| var id: String { | |
| // Fold the folder path into the identity so same-named commands in | |
| // different folders (e.g. `Frontend/Build` and `Backend/Build`) stay | |
| // distinct rather than colliding. Folderless commands keep their | |
| // historical `cmux.config.command.<name>` id. | |
| // | |
| // Each part is encoded individually *before* joining so that a literal | |
| // "/" in a command name (e.g. `name: "Frontend/Build"`) is represented | |
| // as "%2F" and stays distinct from the "/" path separator. | |
| let parts = folderComponents + [name] | |
| let encoded = parts | |
| .map { $0.addingPercentEncoding(withAllowedCharacters: .alphanumerics) ?? $0 } | |
| .joined(separator: "/") | |
| return "cmux.config.command." + encoded | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in b416023: the id now percent-encodes each component (folder segments and name) before joining with /, so folder="Frontend", name="Build" -> .../Frontend/Build and name="Frontend/Build" -> .../Frontend%2FBuild are distinct. Covered by the folderIsPartOfCommandIdentity test.
— Claude Code
Contributor
Author
There was a problem hiding this comment.
Addressed in the current code and covered by tests. CmuxCommandDefinition.id percent-encodes each folder/name component before joining, so a folderless name like Project/Linting produces Project%2FLinting while folder: Project + name: Linting produces Project/Linting. The folderIsPartOfCommandIdentity test covers this collision case.
— Claude Code
Address review: - [P1] Custom workspace-command palette actions stored only the command name, so two foldered workspace commands with the same name dispatched whichever loaded first. The custom action now carries the folder-aware command id, and the executor resolves `.workspaceCommand` by id first, falling back to name (user config still references commands by name). - [P2] The identity key joined folder components and name with `/` before encoding, so `folder:"A/B", name:"C"` and `folder:"A", name:"B/C"` collided. Percent-encode each component before joining so distinct (folder, name) pairs always produce distinct ids; folderless commands keep their historical id. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/CmuxCommandTemplate.swift (1)
53-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocstring still claims backslash-escape stripping that isn't implemented.
Lines 57-58 state
substituting(_:)"strips the escaping backslash from any\{{…}}so it becomes a literal{{…}}". Tracingscan()/substituting(_:), a\{{name}}is left untouched (no match), but the backslash is not removed — the output is still\{{name}}, not{{name}}. Either implement the strip or drop the claim so the doc matches behavior.🤖 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/CmuxCommandTemplate.swift` around lines 53 - 58, The doc comment for `substituting(_:)` / `scan()` incorrectly claims that `\{{…}}` escapes have their backslash stripped, but the current implementation leaves them unchanged. Update the implementation to actually remove the escape backslash for matched literal template forms, or revise the `substituting(_:)` documentation to match the existing behavior so the contract is accurate.
🤖 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 `@Sources/CmuxConfig.swift`:
- Around line 2178-2181: Workspace command loading currently allows same-named
commands from different folders, but command execution still resolves by
command.name, so the selected foldered action can invoke the wrong command.
Update the workspace-command flow in CmuxConfig to carry command.id through the
action and resolution path, or keep the dedupe in the workspace command loader
name-based until all command lookup paths are ID-aware. Make sure the fix
touches the workspace command action construction and the later command
resolution logic so the chosen folder-specific command is preserved.
- Around line 1627-1633: The command identity in CmuxConfig’s id accessor is
still collision-prone because folderComponents and name are concatenated before
encoding, so a legacy command like Project/Linting can match a folder-aware
Project + Linting entry. Update the id generation in the id property to encode
or tag each path component separately before joining, while keeping the existing
cmux.config.command.<name> format for folderless commands. Use the existing
folderComponents and name symbols to make the identity unambiguous and preserve
backwards compatibility.
---
Duplicate comments:
In `@Sources/CmuxCommandTemplate.swift`:
- Around line 53-58: The doc comment for `substituting(_:)` / `scan()`
incorrectly claims that `\{{…}}` escapes have their backslash stripped, but the
current implementation leaves them unchanged. Update the implementation to
actually remove the escape backslash for matched literal template forms, or
revise the `substituting(_:)` documentation to match the existing behavior so
the contract is accurate.
🪄 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: 4ee1baec-c297-459d-8a49-332393fdeab8
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (6)
Sources/CmuxCommandTemplate.swiftSources/CmuxCommandVariable.swiftSources/CmuxCommandVariablePrompt.swiftSources/CmuxConfig.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxCommandVariableTests.swift
Follow-up: generated workspace custom actions now carry the folder-aware command id, but the shared new-workspace and surface-button resolvers (resolvedConfiguredNewWorkspaceCommand / resolvedWorkspaceCommand) still matched by name only, so action-reference entrypoints (ui.newWorkspace.action, context menu, tab-bar buttons) would drop those actions. Both resolvers now match the command id first and fall back to the display name, matching the executor — covering every workspace-command resolution path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Final piece of the folder-aware workspace identity: the configured new-workspace action rebuild stored .workspaceCommand(command.command.name), so even after the resolver selected the right same-named command, execution looked it up by name and could open the wrong one. Store command.command.id, matching the generated palette action and the id-then-name resolvers. Self-generated actions now consistently carry ids; user config references stay name-based. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The quote-position rewrite dropped the backslash-escape mechanism, but the
substituting() docstring still claimed it "strips the escaping backslash from
any \{{…}}". Describe the actual behavior: recognized placeholders are
substituted (POSIX-quoted) and everything else — unknown placeholders,
non-identifier template expressions, and {{…}} inside quotes/here-docs/comments
— is left untouched. (Greptile.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/CmuxCommandTemplate.swift (1)
179-196: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winKeep malformed template scanning linear.
For malformed input with repeated
{{and no valid placeholder, this block callsindexOfCloseBracesand then advances only toinnerStart, causing overlapping rescans of the remaining command on the main execution path. Skip the whole invalid{{...}}span when a close exists, and stop scanning when no close exists. As per path instructions, command template parsing should avoid repeated full scans and stay linear in user-provided input.Proposed fix
if character == "{", index + 1 < count, chars[index + 1] == "{" { let innerStart = index + 2 if let close = indexOfCloseBraces(chars, from: innerStart) { let inner = chars[innerStart..<close] let innerHasIllegalCharacter = inner.contains { c in c == "{" || c == "}" || c == "\n" || c == "\r" } if !innerHasIllegalCharacter, let parsed = parse(inner: String(inner)) { matches.append(( CmuxCommandVariable(name: parsed.name, defaultValue: parsed.defaultValue), index..<(close + 2) )) index = close + 2 continue } + index = close + 2 + continue } - // Not a recognized placeholder; skip past "{{". - index = innerStart - continue + // No closing braces remain, so no later placeholder can start. + break }🤖 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/CmuxCommandTemplate.swift` around lines 179 - 196, Keep malformed template scanning linear in CmuxCommandTemplate parsing: in the placeholder handling branch that checks for "{{" and calls indexOfCloseBraces, avoid rescanning overlapping ranges when the inner content is invalid. Update the parsing flow so that when a closing "}}" is found but the placeholder is not recognized, the scanner advances past the entire invalid "{{...}}" span, and when no closing braces exist it stops scanning instead of continuing from innerStart. Apply this in the main scan loop around parse(inner:) and indexOfCloseBraces to prevent repeated full passes over user input.Source: Path instructions
🤖 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 `@Sources/CmuxConfigExecutor.swift`:
- Around line 79-84: The command lookup in CmuxConfigExecutor and the matching
fallbacks in CmuxConfig currently fall back to the first name match, which can
pick the wrong foldered workspace command when display names are duplicated.
Update the shared command resolution logic so it resolves by id first, then
returns a name match only when there is exactly one match, otherwise returns nil
to fail closed. Apply the same helper to the workspace-command lookup paths
around the workspace command handling in CmuxConfigExecutor and the fallback
references in CmuxConfig so all command identity checks behave consistently.
---
Outside diff comments:
In `@Sources/CmuxCommandTemplate.swift`:
- Around line 179-196: Keep malformed template scanning linear in
CmuxCommandTemplate parsing: in the placeholder handling branch that checks for
"{{" and calls indexOfCloseBraces, avoid rescanning overlapping ranges when the
inner content is invalid. Update the parsing flow so that when a closing "}}" is
found but the placeholder is not recognized, the scanner advances past the
entire invalid "{{...}}" span, and when no closing braces exist it stops
scanning instead of continuing from innerStart. Apply this in the main scan loop
around parse(inner:) and indexOfCloseBraces to prevent repeated full passes over
user input.
🪄 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: fd622cd8-a74c-4c64-a5ce-d95265c22d36
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
Sources/CmuxCommandTemplate.swiftSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftcmuxTests/CmuxCommandVariableTests.swift
…zed-workflow-support-with-variables # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings
…zed-workflow-support-with-variables
…zed-workflow-support-with-variables
lawrencecchen
added
the
stale-revisit
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
3 participants
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.
Fixes #6557
Implements the two complementary improvements requested in the issue for the
commandsarray incmux.json.1. Variables in commands
Support
{{variable}}placeholders (and{{variable=default}}) in a command'scommandstring. When a command with placeholders runs, cmux shows an inline prompt asking for each value, then substitutes them before executing.{ "name": "Deploy", "folder": "Project/Deploy", "command": "bin/deploy --env {{environment}} --branch {{branch}}" }CmuxConfigExecutor.prepareShellInputIfAuthorizedchoke point, so every entrypoint that runs a custom command (Command Palette, surface tab-bar buttons, dock) gets variable prompting with no per-surface duplication (per the shared-behavior policy).CmuxCommandVariableParser(new, pure/testable) does the parsing + substitution. Grammar is deliberately shell-safe: only clean identifier-like names (letters, digits, spaces,_ - .) are treated as variables, so ordinary shell snippets that happen to use{{(awk, arithmetic,$( )) are left untouched.CmuxCommandVariablePrompt(new) renders the prompt as anNSAlertwith one text field per variable, matching the existing project-action confirm dialog. Defaults pre-fill the fields.2. Folder organization
A
folderkey (e.g."Project/Linting") groups a command in the Command Palette:Project / Linting) shows as the palette row's trailing badge.This delivers the issue's "organize + discover" goal within the existing palette. A dedicated always-visible side panel (the screenshot in the issue) is a larger UI surface and a natural follow-up; this PR lands the config model, palette visibility, schema, and docs that it would build on.
Other changes
web/data/cmux.schema.json: documents the command item fields, includingfolderand the{{variable}}convention.folderfield on the custom-commands page, across allweb/messages/*locales.Resources/Localizable.xcstrings: prompt strings added for every locale in the catalog (Cancel reusescommon.cancel).Tests
cmuxTests/CmuxCommandVariableTests.swift(wired into the test target):folderComponents/folderBreadcrumb)🤖 Generated with Claude Code
Summary by cubic
Adds parameterized
{{variable}}prompts and afolderfield for custom commands, with quote-aware, shell-safe substitution and Command Palette folder badges. Command identity, de-dup, and workspace command resolution now use folder-aware IDs with safe encoding so same-named commands stay distinct (addresses #6557). Also converts command regression tests to Swift Testing.New Features
{{name}}/{{name=default}}only at unquoted bare identifiers; quoted{{…}}, here-doc bodies, and#comments stay literal, and escaped quotes are respected so quoted placeholders are never substituted. Entered values are inserted as single POSIX-quoted args with control bytes removed; trust/confirm runs before prompting viaCmuxConfigExecutor.prepareShellInputIfAuthorized(parser:CmuxCommandTemplate, prompt:CmuxCommandVariablePrompt).folder(e.g. "Project/Linting") groups commands; the breadcrumb shows as a trailing badge (when no shortcut) and is added to the subtitle and keywords.Bug Fixes
/collision cases.Written for commit cb787ed. Summary will update on new commits.
Summary by CodeRabbit
/-separated folder grouping for custom commands in the command palette, including folder-aware badges/labels.