Repository navigation
Add customizable Jupyter directory mode - #5222
lawrencecchen wants to merge 10 commits into
Conversation
|
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 configurable "directory tools" (vscodeServeWeb, shellWebServer): config model, schema, defaults, runtime availability and executable resolution, web-server launch+URL extraction, command-palette integration, UI/menu wiring for inline VS Code, and tests covering config and runtime flows. ChangesDirectory Tools Integration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (6 errors, 1 warning, 1 inconclusive)
✅ Passed checks (10 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7edf331ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let result = self.launchWebServer(tool: tool, directoryURL: normalizedDirectoryURL) | ||
| self.queue.async { | ||
| if let result { | ||
| self.serversByKey[key] = result |
There was a problem hiding this comment.
When a shellWebServer tool such as the built-in Jupyter entry is launched, the spawned Process is retained here and reused, but the new controller has no stop/termination path and applicationWillTerminate still only stops VSCodeServeWebController. In the scenario where a user opens Jupyter from the command palette and then quits cmux, the child server can be orphaned and keep listening/running after the app exits, unlike the existing VS Code inline server lifecycle.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces a configurable
Confidence Score: 3/5Mergeable once the open localization gaps and three previously flagged bugs (progressController deallocation, premature workspace creation, blocking semaphore on launchQueue) are resolved. All 25 new user-facing string keys in the xcstrings catalog carry only English and Japanese translations, leaving 18 other locales without coverage for the entire Jupyter / directory-tool UI surface. Three additional bugs flagged in prior review rounds remain open: the DirectoryToolLaunchPanelController is deallocated before the user can interact with it, a blank workspace is created before trust authorization is granted, and the launchQueue thread is blocked for up to the full startup timeout waiting for the server URL. Resources/Localizable.xcstrings (missing 18 locales for all new keys), Sources/ContentView.swift (progressController lifetime and workspace creation ordering), Sources/App/TerminalDirectoryOpenSupport.swift (blocking semaphore on launchQueue) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant ContentView
participant DirectoryToolLaunchPanelController
participant CmuxConfigExecutor
participant DirectoryToolWebServerController
participant Process as zsh Process
User->>ContentView: Invoke tool command palette entry
ContentView->>CmuxConfigExecutor: authorizeProjectAutomationIfNeeded (if local config)
CmuxConfigExecutor-->>ContentView: onAuthorized callback
ContentView->>DirectoryToolLaunchPanelController: "show (requiresApproval=true)"
User->>DirectoryToolLaunchPanelController: Click Allow
DirectoryToolLaunchPanelController->>DirectoryToolWebServerController: ensureWebServerURL(tool, directoryURL)
DirectoryToolWebServerController->>Process: launch zsh -lc command
Process-->>DirectoryToolWebServerController: stdout/stderr output
DirectoryToolWebServerController-->>DirectoryToolLaunchPanelController: progress(output)
Process-->>DirectoryToolWebServerController: prints local URL
DirectoryToolWebServerController-->>ContentView: completion(.opened(url))
ContentView->>DirectoryToolLaunchPanelController: close()
ContentView->>ContentView: openBrowser(inWorkspace:url:)
Reviews (10): Last reviewed commit: "Refine Jupyter launch controls" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var deduped: [String] = [] | ||
| for value in values where seen.insert(value).inserted { | ||
| deduped.append(value) | ||
| } | ||
| return deduped |
There was a problem hiding this comment.
waitForURL calls semaphore.wait(timeout: .now() + 60) on the serial launchQueue thread inside launchWebServer. This holds the launchQueue thread hostage for up to 60 s while waiting for the server process to print a URL — the exact blocking-synchronization pattern flagged by the cmux blocking-runtime rule. If the server is slow to start or silently crashes, every subsequent call to ensureWebServerURL queues behind the blocked thread and can't proceed until the timeout fires. The existing ServeWebOutputCollector uses the same pattern; new code introduced here should move to a real-signal approach (e.g., AsyncStream/continuation.yield signalled from the readabilityHandler) rather than perpetuating the blocking semaphore design.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
| "command.openFolderInJupyter.subtitle": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Jupyter" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Jupyter" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
The new command.openFolderInJupyter.subtitle entry only covers en and ja, but the immediately adjacent command.openFolderInVSCodeInline.subtitle also carries a ko entry. Likewise, menu.openInJupyter only includes en and ja, while the comparable menu.openInVSCode (VS Code Inline menu string just above it) is fully translated across all locales the catalog supports — including ar, bs, da, de, es, fr, it, km, ko, nb, pl, pt-BR, ru, th, tr, uk. Both new keys are user-facing production strings, so the full-internationalization rule requires entries for every locale already in the catalog.
Rule Used: Flag production user-facing text that is not fully... (source)
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 10 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="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:377">
P3: New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.</violation>
<violation number="2" location="Sources/App/TerminalDirectoryOpenSupport.swift:905">
P1: Concurrent opens can launch duplicate web-server processes for the same tool/directory, leaving earlier processes orphaned.</violation>
<violation number="3" location="Sources/App/TerminalDirectoryOpenSupport.swift:987">
P2: Timeout handling can leak background server processes because it terminates without ensuring exit or escalation.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:1041">
P2: Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| guard let configStore = AppDelegate.shared?.mainWindowContexts.values | ||
| .first(where: { $0.tabManager === activeTabManager })? | ||
| .cmuxConfigStore else { | ||
| return TerminalDirectoryOpenTarget.vscodeInline.applicationURL() |
There was a problem hiding this comment.
P2: Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/cmuxApp.swift, line 1041:
<comment>Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.</comment>
<file context>
@@ -1032,6 +1034,17 @@ struct cmuxApp: App {
+ guard let configStore = AppDelegate.shared?.mainWindowContexts.values
+ .first(where: { $0.tabManager === activeTabManager })?
+ .cmuxConfigStore else {
+ return TerminalDirectoryOpenTarget.vscodeInline.applicationURL()
+ }
+ return configStore.loadedDirectoryTools
</file context>
| return TerminalDirectoryOpenTarget.vscodeInline.applicationURL() | |
| return TerminalDirectoryOpenTarget.vscodeInline.isAvailable() | |
| ? TerminalDirectoryOpenTarget.vscodeInline.applicationURL() | |
| : nil |
| } | ||
| } | ||
|
|
||
| private func directoryToolUniquePreservingOrder(_ values: [String]) -> [String] { |
There was a problem hiding this comment.
P3: New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/App/TerminalDirectoryOpenSupport.swift, line 377:
<comment>New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.</comment>
<file context>
@@ -293,6 +293,96 @@ enum TerminalDirectoryOpenTarget: String, CaseIterable {
+ }
+}
+
+private func directoryToolUniquePreservingOrder(_ values: [String]) -> [String] {
+ var seen: Set<String> = []
+ var deduped: [String] = []
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e91d2f626
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "items": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["id"], |
There was a problem hiding this comment.
Because CmuxDirectoryToolDefinition.init(from:) throws when an enabled tool lacks a nonblank title, configs that satisfy this schema (for example { "id": "notebook", "kind": "shellWebServer", "command": "..." }) are still rejected by the app. Please require title for enabled directory tools here or make the decoder/defaulting match the schema so editor validation does not green-light unusable configs.
Useful? React with 👍 / 👎.
| self.launchQueue.async { | ||
| let result = self.launchWebServer(tool: tool, directoryURL: normalizedDirectoryURL) |
There was a problem hiding this comment.
When the same directory tool is invoked again while the first launchWebServer call is still waiting for a URL, serversByKey is still empty, so this path enqueues another launch for the same key. With slow Jupyter startup or a double invocation, two servers can start and only the later process is retained for reuse, leaving the earlier one untracked/running; track a pending launch per key or recheck on launchQueue before launching.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/data/cmux.schema.json (1)
5-5:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the root schema description for project-local
directoryTools.The intro text still says project-local configs support actions, commands, notification hooks, and UI action wiring, but the Swift config loader now also reads
localConfig?.directoryTools. That makes the editor/help text immediately stale.🤖 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 `@web/data/cmux.schema.json` at line 5, Update the root "description" string in the JSON schema (the "description" property) to mention that project-local .cmux/cmux.json also supports directoryTools in addition to actions, commands, notification hooks, and UI action wiring; reference the Swift loader symbol localConfig?.directoryTools when composing the new text so the editor/help text stays 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 `@cmuxTests/CmuxConfigTests.swift`:
- Around line 136-162: Add an assertion in testDecodeDirectoryTool to verify the
decoded urlRegex is correct: after obtaining tool from config.directoryTools
(and the existing assertions), assert that tool.urlRegex equals the JSON value
"(http://127\\\\.0\\\\.0\\\\.1:[^\\\\s]+)" so the test covers decoding of the
urlRegex field.
In `@Sources/App/TerminalDirectoryOpenSupport.swift`:
- Around line 873-914: DirectoryToolWebServerController lacks lifecycle APIs to
stop/restart servers and to clean up on app termination; add methods stop(key:
ServerKey) (or stop(toolID: String, directoryPath: String)) to terminate the
Process, remove the entry from serversByKey and wait for exit, restart(...) to
call stop then relaunch via launchWebServer, and a shutdownAll() to iterate
serversByKey, terminate each process and clear the dictionary; wire
shutdownAll() to the app termination hook and ensure ensureWebServerURL uses the
same key semantics when updating serversByKey.
- Around line 873-875: The class DirectoryToolWebServerController exposes a
public initializer allowing bypass of the shared singleton; add a private init()
to enforce singleton usage (matching the pattern used by
VSCodeServeWebController) so only DirectoryToolWebServerController.shared can be
used, and optionally add a test factory pattern (e.g., a makeForTesting or init
with a launchProcessOverride closure) if you later need injected behavior for
tests; update DirectoryToolWebServerController by making its initializer private
and mirror VSCodeServeWebController’s test-factory approach if test injection is
required.
In `@Sources/cmuxApp.swift`:
- Around line 1037-1045: The helper configuredInlineVSCodeApplicationURL
currently picks the config by matching activeTabManager against
AppDelegate.shared?.mainWindowContexts, which can return the wrong context when
an auxiliary window is key; change it to resolve the main-window context the
same way the open-folder flow does by calling
AppDelegate.shared?.preferredMainWindowContextForWorkspaceCreation(...) and then
read cmuxConfigStore.loadedDirectoryTools from that resolved context; if that
context or its cmuxConfigStore is nil, fall back to
TerminalDirectoryOpenTarget.vscodeInline.applicationURL(), otherwise return the
first loadedDirectoryTools entry where kind == .vscodeServeWeb and isAvailable()
and call .applicationURL() on it (keep the function name
configuredInlineVSCodeApplicationURL and use the same
cmuxConfigStore.loadedDirectoryTools lookup logic).
In `@Sources/CmuxConfig.swift`:
- Around line 462-492: defaultDefinitions currently uses
String(localized:defaultValue:) when stored in a static let, which freezes the
locale at first initialization; change defaultDefinitions to be a computed
static var (or lazily create each CmuxDirectoryToolDefinition with closures that
call String(localized:defaultValue:) on access) so titles/subtitles are resolved
at runtime, i.e. replace the static let defaultDefinitions with a static var
defaultDefinitions: [CmuxDirectoryToolDefinition] { ... } (or otherwise ensure
CmuxDirectoryToolDefinition’s title/subtitle are computed properties) so the
localized titles for the definitions update when the app locale changes.
In `@web/data/cmux.schema.json`:
- Around line 42-120: The schema adds new user-facing strings under
"directoryTools" but doesn't expose them to the localization pipeline; add
descriptionKey entries for the directoryTools block and for each property that
currently has a "description" (e.g., the parent object "directoryTools" and
properties "id", "title", "subtitle", "keywords", "enabled", "kind",
"applicationBundlePathCandidates", "executablePathCandidates", "command", "cwd",
"urlRegex") so the web docs can look up translations in
web/messages/<locale>.json; also add descriptionKey values for any enum labels
you want localized (e.g., the "kind" enum values) to match keys in
web/messages/en.json and web/messages/ja.json, ensuring keys follow the existing
naming convention used elsewhere in the schema.
---
Outside diff comments:
In `@web/data/cmux.schema.json`:
- Line 5: Update the root "description" string in the JSON schema (the
"description" property) to mention that project-local .cmux/cmux.json also
supports directoryTools in addition to actions, commands, notification hooks,
and UI action wiring; reference the Swift loader symbol
localConfig?.directoryTools when composing the new text so the editor/help text
stays 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: 9153032f-b8b1-4eca-b95a-c12f8acbf883
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/App/TerminalDirectoryOpenSupport.swiftSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/ContentView.swiftSources/cmuxApp.swiftcmuxTests/CmuxConfigTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftdocs/configuration.mdweb/data/cmux.schema.json
| final class DirectoryToolWebServerController { | ||
| static let shared = DirectoryToolWebServerController() | ||
| private static let startupTimeoutSeconds: TimeInterval = 60 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding private init() for singleton consistency.
VSCodeServeWebController uses private init(...) with a #if DEBUG factory method (makeForTesting) to ensure the singleton pattern while allowing test injection. This class allows direct instantiation which bypasses the shared singleton. If test injection is needed later, consider adding:
final class DirectoryToolWebServerController {
static let shared = DirectoryToolWebServerController()
private static let startupTimeoutSeconds: TimeInterval = 60
+
+ private init() {}If test injection is required, follow the VSCodeServeWebController pattern with a launchProcessOverride closure parameter.
🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 873-873: Classes should have an explicit deinit method
(required_deinit)
🤖 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/App/TerminalDirectoryOpenSupport.swift` around lines 873 - 875, The
class DirectoryToolWebServerController exposes a public initializer allowing
bypass of the shared singleton; add a private init() to enforce singleton usage
(matching the pattern used by VSCodeServeWebController) so only
DirectoryToolWebServerController.shared can be used, and optionally add a test
factory pattern (e.g., a makeForTesting or init with a launchProcessOverride
closure) if you later need injected behavior for tests; update
DirectoryToolWebServerController by making its initializer private and mirror
VSCodeServeWebController’s test-factory approach if test injection is required.
| private func configuredInlineVSCodeApplicationURL() -> URL? { | ||
| guard let configStore = AppDelegate.shared?.mainWindowContexts.values | ||
| .first(where: { $0.tabManager === activeTabManager })? | ||
| .cmuxConfigStore else { | ||
| return TerminalDirectoryOpenTarget.vscodeInline.applicationURL() | ||
| } | ||
| return configStore.loadedDirectoryTools | ||
| .first { $0.kind == .vscodeServeWeb && $0.isAvailable() }? | ||
| .applicationURL() |
There was a problem hiding this comment.
Resolve the config from the same main-window context as the open-folder flow.
This helper derives the directory-tool config by matching activeTabManager, which is still driven by NSApp.keyWindow ?? NSApp.mainWindow. When an auxiliary window is key, that can miss the intended main-window context and fall back to TerminalDirectoryOpenTarget.vscodeInline.applicationURL(), so the File menu can stay enabled or open the hard-coded VS Code target even when the focused workspace’s config disables or overrides inline VS Code. Use the same preferredMainWindowContextForWorkspaceCreation(...) path that the open-folder panel uses, then read cmuxConfigStore.loadedDirectoryTools from that resolved context. Based on learnings: “In AppDelegate.showOpenFolderPanel(), seed NSOpenPanel.directoryURL using preferredMainWindowContextForWorkspaceCreation(debugSource: …) rather than NSApp.keyWindow… handles auxiliary-key-window cases.”
🤖 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/cmuxApp.swift` around lines 1037 - 1045, The helper
configuredInlineVSCodeApplicationURL currently picks the config by matching
activeTabManager against AppDelegate.shared?.mainWindowContexts, which can
return the wrong context when an auxiliary window is key; change it to resolve
the main-window context the same way the open-folder flow does by calling
AppDelegate.shared?.preferredMainWindowContextForWorkspaceCreation(...) and then
read cmuxConfigStore.loadedDirectoryTools from that resolved context; if that
context or its cmuxConfigStore is nil, fall back to
TerminalDirectoryOpenTarget.vscodeInline.applicationURL(), otherwise return the
first loadedDirectoryTools entry where kind == .vscodeServeWeb and isAvailable()
and call .applicationURL() on it (keep the function name
configuredInlineVSCodeApplicationURL and use the same
cmuxConfigStore.loadedDirectoryTools lookup logic).
| static let defaultDefinitions: [CmuxDirectoryToolDefinition] = [ | ||
| CmuxDirectoryToolDefinition( | ||
| id: "vscode-inline", | ||
| title: String(localized: "menu.openInVSCode", defaultValue: "Open Current Directory in VS Code (Inline)"), | ||
| subtitle: String(localized: "command.openFolderInVSCodeInline.subtitle", defaultValue: "VS Code Inline"), | ||
| keywords: ["terminal", "directory", "open", "ide", "vs", "code", "visual", "studio", "inline", "browser", "serve-web"], | ||
| kind: .vscodeServeWeb, | ||
| applicationBundlePathCandidates: defaultVSCodeApplicationBundlePathCandidates | ||
| ), | ||
| CmuxDirectoryToolDefinition( | ||
| id: "jupyter", | ||
| title: String(localized: "menu.openInJupyter", defaultValue: "Open Current Directory in Jupyter"), | ||
| subtitle: String(localized: "command.openFolderInJupyter.subtitle", defaultValue: "Jupyter"), | ||
| keywords: ["terminal", "directory", "open", "notebook", "jupyter", "lab", "python", "browser"], | ||
| kind: .shellWebServer, | ||
| executablePathCandidates: [ | ||
| "/opt/homebrew/bin/jupyter", | ||
| "/usr/local/bin/jupyter", | ||
| "/usr/bin/jupyter", | ||
| ], | ||
| command: """ | ||
| TOOL="${CMUX_TOOL_EXECUTABLE:-$(command -v jupyter || true)}"; \ | ||
| if [ -z "$TOOL" ]; then TOOL="$(command -v jupyter-lab || true)"; fi; \ | ||
| if [ -z "$TOOL" ]; then exit 127; fi; \ | ||
| if [ "$(basename "$TOOL")" = "jupyter-lab" ]; then exec "$TOOL" --no-browser --ip=127.0.0.1 --port=0; fi; \ | ||
| exec "$TOOL" lab --no-browser --ip=127.0.0.1 --port=0 | ||
| """, | ||
| cwd: "{directory}", | ||
| urlRegex: "(http://127\\.0\\.0\\.1:[^\\s]+)" | ||
| ), | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Swift, are static letproperty initializers evaluated only once per process, and would aString(localized:) created there remain stale if the app's language changes later at runtime?
💡 Result:
In Swift, static let properties are lazily initialized upon their first access and are guaranteed by the language runtime to be initialized exactly once per process [1][2]. Once initialized, the stored value remains fixed for the lifetime of that process [2]. Regarding String(localized:), this initializer evaluates at the time of assignment to produce a String instance based on the locale active at that moment [3][4]. Because static properties are evaluated only once, a String(localized:) call performed within a static let initializer captures the localized text only once [1][2]. It does not automatically re-evaluate or update if the system language changes at runtime [5]. If you require dynamic localization that updates when the app's language changes, you should avoid storing the result in a static let constant [5]. Instead: 1. Use a computed property (static var) or a function that performs the String(localized:) call every time it is accessed [5]. 2. In SwiftUI, prefer passing a LocalizedStringResource or LocalizedStringKey directly to views (such as Text), allowing the framework to handle localization updates automatically when the environment's locale changes [5].
Citations:
- 1: https://stackoverflow.com/questions/34667134/implicitly-lazy-static-members-in-swift
- 2: https://stackoverflow.com/questions/43374222/in-swift-why-does-assigning-to-a-static-variable-also-invoke-its-getter
- 3: https://developer.apple.com/documentation/swift/string/init(localized:defaultvalue:options:table:bundle:locale:comment:)?changes=_8
- 4: https://developer.apple.com/documentation/swift/string?changes=_6_4
- 5: https://stackoverflow.com/questions/77698781/swiftui-update-localizedstringresource-interpolated-value-after-change-of-enviro
Avoid freezing localized directory-tool labels in static let
static let is initialized only once per process, so these String(localized:defaultValue:) titles/subtitles capture the locale from first access; they won’t update if the app’s language changes later at runtime.
Proposed fix
- static let defaultDefinitions: [CmuxDirectoryToolDefinition] = [
- CmuxDirectoryToolDefinition(
- id: "vscode-inline",
- title: String(localized: "menu.openInVSCode", defaultValue: "Open Current Directory in VS Code (Inline)"),
- subtitle: String(localized: "command.openFolderInVSCodeInline.subtitle", defaultValue: "VS Code Inline"),
- keywords: ["terminal", "directory", "open", "ide", "vs", "code", "visual", "studio", "inline", "browser", "serve-web"],
- kind: .vscodeServeWeb,
- applicationBundlePathCandidates: defaultVSCodeApplicationBundlePathCandidates
- ),
- CmuxDirectoryToolDefinition(
- id: "jupyter",
- title: String(localized: "menu.openInJupyter", defaultValue: "Open Current Directory in Jupyter"),
- subtitle: String(localized: "command.openFolderInJupyter.subtitle", defaultValue: "Jupyter"),
- keywords: ["terminal", "directory", "open", "notebook", "jupyter", "lab", "python", "browser"],
- kind: .shellWebServer,
- executablePathCandidates: [
- "/opt/homebrew/bin/jupyter",
- "/usr/local/bin/jupyter",
- "/usr/bin/jupyter",
- ],
- command: """
- TOOL="${CMUX_TOOL_EXECUTABLE:-$(command -v jupyter || true)}"; \
- if [ -z "$TOOL" ]; then TOOL="$(command -v jupyter-lab || true)"; fi; \
- if [ -z "$TOOL" ]; then exit 127; fi; \
- if [ "$(basename "$TOOL")" = "jupyter-lab" ]; then exec "$TOOL" --no-browser --ip=127.0.0.1 --port=0; fi; \
- exec "$TOOL" lab --no-browser --ip=127.0.0.1 --port=0
- """,
- cwd: "{directory}",
- urlRegex: "(http://127\\.0\\.0\\.1:[^\\s]+)"
- ),
- ]
+ static var defaultDefinitions: [CmuxDirectoryToolDefinition] {
+ [
+ CmuxDirectoryToolDefinition(
+ id: "vscode-inline",
+ title: String(localized: "menu.openInVSCode", defaultValue: "Open Current Directory in VS Code (Inline)"),
+ subtitle: String(localized: "command.openFolderInVSCodeInline.subtitle", defaultValue: "VS Code Inline"),
+ keywords: ["terminal", "directory", "open", "ide", "vs", "code", "visual", "studio", "inline", "browser", "serve-web"],
+ kind: .vscodeServeWeb,
+ applicationBundlePathCandidates: defaultVSCodeApplicationBundlePathCandidates
+ ),
+ CmuxDirectoryToolDefinition(
+ id: "jupyter",
+ title: String(localized: "menu.openInJupyter", defaultValue: "Open Current Directory in Jupyter"),
+ subtitle: String(localized: "command.openFolderInJupyter.subtitle", defaultValue: "Jupyter"),
+ keywords: ["terminal", "directory", "open", "notebook", "jupyter", "lab", "python", "browser"],
+ kind: .shellWebServer,
+ executablePathCandidates: [
+ "/opt/homebrew/bin/jupyter",
+ "/usr/local/bin/jupyter",
+ "/usr/bin/jupyter",
+ ],
+ command: """
+ TOOL="${CMUX_TOOL_EXECUTABLE:-$(command -v jupyter || true)}"; \
+ if [ -z "$TOOL" ]; then TOOL="$(command -v jupyter-lab || true)"; fi; \
+ if [ -z "$TOOL" ]; then exit 127; fi; \
+ if [ "$(basename "$TOOL")" = "jupyter-lab" ]; then exec "$TOOL" --no-browser --ip=127.0.0.1 --port=0; fi; \
+ exec "$TOOL" lab --no-browser --ip=127.0.0.1 --port=0
+ """,
+ cwd: "{directory}",
+ urlRegex: "(http://127\\.0\\.0\\.1:[^\\s]+)"
+ ),
+ ]
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static let defaultDefinitions: [CmuxDirectoryToolDefinition] = [ | |
| CmuxDirectoryToolDefinition( | |
| id: "vscode-inline", | |
| title: String(localized: "menu.openInVSCode", defaultValue: "Open Current Directory in VS Code (Inline)"), | |
| subtitle: String(localized: "command.openFolderInVSCodeInline.subtitle", defaultValue: "VS Code Inline"), | |
| keywords: ["terminal", "directory", "open", "ide", "vs", "code", "visual", "studio", "inline", "browser", "serve-web"], | |
| kind: .vscodeServeWeb, | |
| applicationBundlePathCandidates: defaultVSCodeApplicationBundlePathCandidates | |
| ), | |
| CmuxDirectoryToolDefinition( | |
| id: "jupyter", | |
| title: String(localized: "menu.openInJupyter", defaultValue: "Open Current Directory in Jupyter"), | |
| subtitle: String(localized: "command.openFolderInJupyter.subtitle", defaultValue: "Jupyter"), | |
| keywords: ["terminal", "directory", "open", "notebook", "jupyter", "lab", "python", "browser"], | |
| kind: .shellWebServer, | |
| executablePathCandidates: [ | |
| "/opt/homebrew/bin/jupyter", | |
| "/usr/local/bin/jupyter", | |
| "/usr/bin/jupyter", | |
| ], | |
| command: """ | |
| TOOL="${CMUX_TOOL_EXECUTABLE:-$(command -v jupyter || true)}"; \ | |
| if [ -z "$TOOL" ]; then TOOL="$(command -v jupyter-lab || true)"; fi; \ | |
| if [ -z "$TOOL" ]; then exit 127; fi; \ | |
| if [ "$(basename "$TOOL")" = "jupyter-lab" ]; then exec "$TOOL" --no-browser --ip=127.0.0.1 --port=0; fi; \ | |
| exec "$TOOL" lab --no-browser --ip=127.0.0.1 --port=0 | |
| """, | |
| cwd: "{directory}", | |
| urlRegex: "(http://127\\.0\\.0\\.1:[^\\s]+)" | |
| ), | |
| ] | |
| static var defaultDefinitions: [CmuxDirectoryToolDefinition] { | |
| [ | |
| CmuxDirectoryToolDefinition( | |
| id: "vscode-inline", | |
| title: String(localized: "menu.openInVSCode", defaultValue: "Open Current Directory in VS Code (Inline)"), | |
| subtitle: String(localized: "command.openFolderInVSCodeInline.subtitle", defaultValue: "VS Code Inline"), | |
| keywords: ["terminal", "directory", "open", "ide", "vs", "code", "visual", "studio", "inline", "browser", "serve-web"], | |
| kind: .vscodeServeWeb, | |
| applicationBundlePathCandidates: defaultVSCodeApplicationBundlePathCandidates | |
| ), | |
| CmuxDirectoryToolDefinition( | |
| id: "jupyter", | |
| title: String(localized: "menu.openInJupyter", defaultValue: "Open Current Directory in Jupyter"), | |
| subtitle: String(localized: "command.openFolderInJupyter.subtitle", defaultValue: "Jupyter"), | |
| keywords: ["terminal", "directory", "open", "notebook", "jupyter", "lab", "python", "browser"], | |
| kind: .shellWebServer, | |
| executablePathCandidates: [ | |
| "/opt/homebrew/bin/jupyter", | |
| "/usr/local/bin/jupyter", | |
| "/usr/bin/jupyter", | |
| ], | |
| command: """ | |
| TOOL="${CMUX_TOOL_EXECUTABLE:-$(command -v jupyter || true)}"; \ | |
| if [ -z "$TOOL" ]; then TOOL="$(command -v jupyter-lab || true)"; fi; \ | |
| if [ -z "$TOOL" ]; then exit 127; fi; \ | |
| if [ "$(basename "$TOOL")" = "jupyter-lab" ]; then exec "$TOOL" --no-browser --ip=127.0.0.1 --port=0; fi; \ | |
| exec "$TOOL" lab --no-browser --ip=127.0.0.1 --port=0 | |
| """, | |
| cwd: "{directory}", | |
| urlRegex: "(http://127\\.0\\.0\\.1:[^\\s]+)" | |
| ), | |
| ] | |
| } |
🤖 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/CmuxConfig.swift` around lines 462 - 492, defaultDefinitions
currently uses String(localized:defaultValue:) when stored in a static let,
which freezes the locale at first initialization; change defaultDefinitions to
be a computed static var (or lazily create each CmuxDirectoryToolDefinition with
closures that call String(localized:defaultValue:) on access) so
titles/subtitles are resolved at runtime, i.e. replace the static let
defaultDefinitions with a static var defaultDefinitions:
[CmuxDirectoryToolDefinition] { ... } (or otherwise ensure
CmuxDirectoryToolDefinition’s title/subtitle are computed properties) so the
localized titles for the definitions update when the app locale changes.
| "directoryTools": { | ||
| "title": "directoryTools", | ||
| "description": "Browser-backed tools that open the focused terminal directory inside cmux. Built-ins provide VS Code Inline and Jupyter; entries with the same id override defaults, and enabled false disables a default.", | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "additionalProperties": false, | ||
| "required": ["id"], | ||
| "properties": { | ||
| "id": { | ||
| "type": "string", | ||
| "pattern": "^[A-Za-z0-9._-]+$", | ||
| "description": "Stable id used for Command Palette commands. Use vscode-inline or jupyter to override built-in tools." | ||
| }, | ||
| "title": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "Command Palette title, for example Open Current Directory in Notebook." | ||
| }, | ||
| "subtitle": { | ||
| "type": "string", | ||
| "description": "Optional Command Palette subtitle." | ||
| }, | ||
| "keywords": { | ||
| "type": "array", | ||
| "items": { "type": "string" }, | ||
| "default": [], | ||
| "description": "Extra search keywords for the Command Palette." | ||
| }, | ||
| "enabled": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Set false to remove a built-in or configured tool." | ||
| }, | ||
| "kind": { | ||
| "type": "string", | ||
| "enum": ["vscodeServeWeb", "shellWebServer"], | ||
| "default": "shellWebServer", | ||
| "description": "vscodeServeWeb runs VS Code's code-tunnel serve-web backend. shellWebServer runs command and opens the first local URL matched in output." | ||
| }, | ||
| "applicationBundlePathCandidates": { | ||
| "type": "array", | ||
| "items": { "type": "string" }, | ||
| "default": [], | ||
| "description": "Application bundle candidates for vscodeServeWeb. /Applications entries are also checked under ~/Applications." | ||
| }, | ||
| "executablePathCandidates": { | ||
| "type": "array", | ||
| "items": { "type": "string" }, | ||
| "default": [], | ||
| "description": "Executable candidates for shellWebServer availability. The resolved path is exposed to command as CMUX_TOOL_EXECUTABLE." | ||
| }, | ||
| "command": { | ||
| "type": "string", | ||
| "description": "Shell command for shellWebServer. cmux sets CMUX_DIRECTORY, CMUX_TOOL_ID, and CMUX_TOOL_EXECUTABLE." | ||
| }, | ||
| "cwd": { | ||
| "type": "string", | ||
| "default": "{directory}", | ||
| "description": "Working directory for shellWebServer. Supports {directory}; relative paths resolve from the focused directory." | ||
| }, | ||
| "urlRegex": { | ||
| "type": "string", | ||
| "description": "Optional regular expression used to capture the browser URL from stdout/stderr. If it has a capture group, group 1 is used." | ||
| } | ||
| }, | ||
| "allOf": [ | ||
| { | ||
| "if": { | ||
| "properties": { | ||
| "kind": { "const": "shellWebServer" }, | ||
| "enabled": { "not": { "const": false } } | ||
| } | ||
| }, | ||
| "then": { "required": ["command"] } | ||
| } | ||
| ] | ||
| }, | ||
| "default": [] |
There was a problem hiding this comment.
Wire the new schema descriptions into localized docs.
This block adds a lot of new user-facing schema copy, but none of it is keyed for the web docs localization pipeline. As written, directoryTools will stay English-only in localized configuration docs instead of pulling translations from web/messages/en.json and web/messages/ja.json.
Based on learnings, descriptionKey is the hook the web docs use for localized schema copy via web/app/[locale]/docs/configuration and web/messages/<locale>.json.
🤖 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 `@web/data/cmux.schema.json` around lines 42 - 120, The schema adds new
user-facing strings under "directoryTools" but doesn't expose them to the
localization pipeline; add descriptionKey entries for the directoryTools block
and for each property that currently has a "description" (e.g., the parent
object "directoryTools" and properties "id", "title", "subtitle", "keywords",
"enabled", "kind", "applicationBundlePathCandidates",
"executablePathCandidates", "command", "cwd", "urlRegex") so the web docs can
look up translations in web/messages/<locale>.json; also add descriptionKey
values for any enum labels you want localized (e.g., the "kind" enum values) to
match keys in web/messages/en.json and web/messages/ja.json, ensuring keys
follow the existing naming convention used elsewhere in the schema.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55d51189c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case .vscodeServeWeb: | ||
| guard let vscodeApplicationURL = tool.applicationURL() else { return false } | ||
| return openFocusedDirectoryInInlineVSCode(directoryURL, vscodeApplicationURL: vscodeApplicationURL) |
There was a problem hiding this comment.
When a project-local .cmux/cmux.json defines a vscodeServeWeb directory tool, this branch launches the configured app's Contents/Resources/app/bin/code-tunnel directly via openDirectoryInInlineVSCode without the project automation trust prompt. In that context tool.sourcePath is non-nil and applicationBundlePathCandidates comes from the untrusted project config, while the shellWebServer branch below goes through authorizeDirectoryToolIfNeeded; a malicious project can therefore point at an executable fake .app path and get code execution when the user selects the tool or the inline VS Code folder action.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| private func openFocusedDirectoryInInlineVSCode(_ directoryURL: URL) -> Bool { | ||
| AppDelegate.shared?.openDirectoryInInlineVSCode(directoryURL, tabManager: tabManager) ?? false | ||
| private func openFocusedDirectory(_ directoryURL: URL, with tool: CmuxResolvedDirectoryTool) -> Bool { | ||
| guard tool.isAvailable() else { return false } | ||
| switch tool.kind { | ||
| case .vscodeServeWeb: | ||
| guard let vscodeApplicationURL = tool.applicationURL() else { return false } | ||
| return openFocusedDirectoryInInlineVSCode(directoryURL, vscodeApplicationURL: vscodeApplicationURL) | ||
| case .shellWebServer: | ||
| return openFocusedDirectoryInShellDirectoryTool(directoryURL, tool: tool) |
There was a problem hiding this comment.
tabManager.addWorkspace(select: true) is called to capture targetWorkspaceId before authorizeDirectoryToolIfNeeded runs. For the third branch (no selected workspace and no open tabs), this immediately creates and selects a new blank workspace — before the user sees any trust prompt. If the user denies authorization, the blank workspace is left behind selected in the UI. Moving targetWorkspaceId computation inside the authorized closure fixes the side effect, or restricting the fallback to tabManager.selectedWorkspace?.id ?? tabManager.tabs.first?.id and opening the browser in whatever workspace is active at callback time avoids it entirely.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
Sources/CmuxConfig.swift (2)
400-403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the schema’s
directoryTools.idpattern in the decoder.
parseConfig(at:)decodes this file directly, so configs never pass through JSON Schema enforcement here. Right now any non-blankidis accepted, even though the schema contract requires^[A-Za-z0-9._-]+$. That leaks invalid values intocommandPaletteCommandIdand the override/disable merge path, so malformed IDs can create commands the rest of the stack does not expect. Reject invalid IDs inCmuxDirectoryToolDefinition.init(from:), not just in the schema.As per coding guidelines,
web/data/cmux.schema.jsonrequires directory tool ids to match^[A-Za-z0-9._-]+$.🤖 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/CmuxConfig.swift` around lines 400 - 403, CmuxDirectoryToolDefinition.init(from decoder:) currently accepts any non-blank id; update the decoder to enforce the schema pattern ^[A-Za-z0-9._-]+$ by validating the id string (retrieved via Self.requiredTrimmedString(forKey: .id, in: container)) against that regex and throwing a DecodingError.dataCorruptedError(forKey: .id, in: container, debugDescription: "...") when it doesn’t match; ensure the error message names the invalid id and that validation occurs immediately after decoding id so invalid values never propagate to commandPaletteCommandId or merge paths.
431-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
startupTimeoutSecondson decode, not only in the schema.This field is decoded verbatim, so
0or negative values are currently accepted even though the schema says the minimum is1. SinceCmuxConfigStoredoes not run schema validation before decoding, an invalid config can make shell web-server tools fail immediately or behave unpredictably downstream. Mirror the notification-hook timeout validation here and reject non-finite or< 1values.🤖 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/CmuxConfig.swift` around lines 431 - 448, The decoded startupTimeoutSeconds is accepted verbatim but must be validated like the notification-hook timeout; after decoding startupTimeoutSeconds via container.decodeIfPresent(Double.self, forKey: .startupTimeoutSeconds) (the startupTimeoutSeconds local), check if the value is non-nil, isFinite, and >= 1.0 and if not, throw a DecodingError.dataCorruptedError(forKey: .startupTimeoutSeconds, in: container, debugDescription: "startupTimeoutSeconds must be a finite number >= 1"); place this validation immediately after the decode and before the switch that uses startupTimeoutSeconds so invalid configs are rejected during decode.Sources/App/TerminalDirectoryOpenSupport.swift (2)
995-1017:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not return raw process output in surfaced startup failures.
These branches copy
error.localizedDescriptionand raw stdout/stderr intoLaunchFailure.output. For shell-backed tools, that can expose tokenized localhost URLs, filesystem paths, and vendor-specific diagnostics directly in user-facing error copy. Return localized/generic failure text here and keep raw output redacted or debug-only. As per coding guidelines, "For production user-facing errors, alerts, command output, API error bodies, and recovery copy, flag implementation leaks such as upstream vendor names, internal provider names, environment variables, database or migration details, raw upstream messages, internal billing ids, or unredacted payloads."🤖 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/App/TerminalDirectoryOpenSupport.swift` around lines 995 - 1017, The catch and timeout branches currently propagate raw process output (error.localizedDescription and collector.outputSnippet) into the user-facing LaunchFailure.output; change these returns to use a generic/localized message (e.g., "Failed to launch tool" / a localized .genericLaunchFailure string) and redact raw output from the LaunchFailure. Keep the existing cleanup (stdoutPipe/stderrPipe readabilityHandler removal and process termination) but route the raw error.localizedDescription and collector.outputSnippet to debug logging or a non-user-facing diagnostic store instead of returning them in LaunchFailure; update the return sites that construct LaunchFailure(reason:..., output: ...) to supply the sanitized message and ensure any code that logs the raw output uses a debug log API rather than the LaunchFailure object.
1065-1087:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBound the collector buffer instead of rescanning an ever-growing string.
append(_:)keeps all emitted output inoutputBuffer, then reruns URL extraction over the full buffer on every chunk. A noisy server that never prints a URL turns this into repeated full-buffer scans with unbounded memory growth until timeout. Keep only a bounded suffix and reuse a compiled matcher or incremental scan. As per coding guidelines, "flag nested full-collection scans, per-target rescans for batch actions, repeated sort/filter/map work in hot paths, in-memory joins that belong in the data store, and unbenchmarked algorithm choices for paths expected to handle roughly 1000 workspaces or similar records."🤖 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/App/TerminalDirectoryOpenSupport.swift` around lines 1065 - 1087, append(_:) currently appends every chunk to outputBuffer and rescans the entire growing string for a URL on each call; change it to keep only a bounded suffix (e.g., last N characters where N >= max URL length + overlap) instead of unbounded growth, and run the URL extraction only against the new bounded buffer so you avoid repeated full-buffer scans and unbounded memory use. Also introduce a reusable compiled matcher (store the compiled regex/matcher used by DirectoryToolWebServerURLBuilder.extractURL as a property referenced by urlPattern or wrap extractURL to accept a precompiled matcher) and reuse it across append calls, keep the existing lock/resolvedURL/didSignal/semaphore semantics, and ensure you trim or truncate outputBuffer under the lock immediately after appending to enforce the bound and still detect URLs that span chunk boundaries.
♻️ Duplicate comments (2)
web/data/cmux.schema.json (1)
42-134:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWire the new
directoryToolsschema copy into the localized docs pipeline.This adds a full set of new user-facing descriptions, but the
directoryToolsblock and its nested properties still only have rawdescriptiontext. The localized configuration docs read schema copy throughdescriptionKeyandweb/messages/<locale>.json, so this section will remain English-only in non-English docs until those keys are added.Based on learnings,
descriptionKeyis the hook the web docs use vianext-intlandweb/messages/<locale>.json, and the coding guidelines require matching locale coverage for user-facing schema copy.🤖 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 `@web/data/cmux.schema.json` around lines 42 - 134, The new "directoryTools" schema block contains user-facing "description" strings but isn't wired into the localization pipeline; replace those description fields (for "directoryTools" itself and its nested properties: id, title, subtitle, keywords, enabled, kind, applicationBundlePathCandidates, executablePathCandidates, command, cwd, urlRegex, failureMessage, installCommand, startupTimeoutSeconds) with corresponding "descriptionKey" entries and add matching keys/values into web/messages/<locale>.json for each locale (following the existing next-intl key naming convention used elsewhere) so the docs generator reads localized copy via descriptionKey; ensure keys exactly match the strings you add in the schema and include all locales required by the coding guidelines.Sources/CmuxConfig.swift (1)
480-519:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t freeze localized built-in tool strings in
static let.These
String(localized:defaultValue:)calls run whendefaultDefinitionsis first initialized, so the built-in title/subtitle/failureMessage strings stay stuck to that first locale for the rest of the process. If the user changesapp.languageat runtime, directory tool labels will not update with the rest of the UI. MakedefaultDefinitionscomputed instead of stored.🤖 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/CmuxConfig.swift` around lines 480 - 519, The built-in localized strings in static let defaultDefinitions are evaluated once at initialization and get frozen to the first locale; change defaultDefinitions to a computed property so String(localized:defaultValue:) is evaluated on each access. Replace the stored property declaration static let defaultDefinitions: [CmuxDirectoryToolDefinition] = [...] with a computed property static var defaultDefinitions: [CmuxDirectoryToolDefinition] { return [...] } (keeping the same CmuxDirectoryToolDefinition entries including ids like "vscode-inline" and "jupyter" and keys such as title, subtitle, failureMessage) so localization updates when app.language changes at runtime.
🤖 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/App/TerminalDirectoryOpenSupport.swift`:
- Around line 903-935: The method ensureWebServerURL can start duplicate
launches because serversByKey is only set after launch completes; add a
pending-launches map (e.g. pendingLaunches: [ServerKey: [ (LaunchResult) -> Void
] ]) keyed by ServerKey and modify ensureWebServerURL to, inside the queue.async
block, if no running server exists then check pendingLaunches: if an entry
exists append the completion and return, else create a new array with the
completion, start launchWebServer(tool:directoryURL) as before, and when that
launch finishes (inside the existing queue/Dispatch flow) iterate the pending
completions for that ServerKey, invoke each with the single LaunchResult, then
remove the pendingLaunches entry and only then update serversByKey if opened;
keep existing synchronization using queue and launchQueue.
In `@Sources/ContentView.swift`:
- Around line 9968-9977: Add an inline non-sensitive debug event log in the
ContentView branch handling the alert response: when response ==
.alertFirstButtonReturn, log a UI event (using the same debug event logging
facility used elsewhere in ContentView) recording the tool identifier (e.g.,
tool.id) and the action "run_install_command" immediately before calling
runDirectoryToolInstallCommand(tool:directoryURL:); when response ==
.alertSecondButtonReturn, log the tool identifier and the action
"copy_install_command" immediately before clearing/setting NSPasteboard and do
NOT include installCommand or other sensitive content in the log.
- Around line 10013-10020: The code currently appends raw failure.output into
the user-facing message (in the block that builds outputFormat and returns
String(format: outputFormat, baseMessage, output)), which may leak sensitive
upstream/tool output; modify the logic that uses failure.output to sanitize or
omit raw output before embedding it into informativeText: either remove
inclusion of failure.output from the returned string (return baseMessage only)
or replace the second format argument with a redacted/normalized summary (e.g.,
a short sanitizedMessage derived from failure.output that strips paths, tokens,
and long logs). Update the symbols failure.output, outputFormat, and the return
expression so the alert only contains baseMessage plus a safe, trimmed summary
(or nothing) instead of the raw output.
---
Outside diff comments:
In `@Sources/App/TerminalDirectoryOpenSupport.swift`:
- Around line 995-1017: The catch and timeout branches currently propagate raw
process output (error.localizedDescription and collector.outputSnippet) into the
user-facing LaunchFailure.output; change these returns to use a
generic/localized message (e.g., "Failed to launch tool" / a localized
.genericLaunchFailure string) and redact raw output from the LaunchFailure. Keep
the existing cleanup (stdoutPipe/stderrPipe readabilityHandler removal and
process termination) but route the raw error.localizedDescription and
collector.outputSnippet to debug logging or a non-user-facing diagnostic store
instead of returning them in LaunchFailure; update the return sites that
construct LaunchFailure(reason:..., output: ...) to supply the sanitized message
and ensure any code that logs the raw output uses a debug log API rather than
the LaunchFailure object.
- Around line 1065-1087: append(_:) currently appends every chunk to
outputBuffer and rescans the entire growing string for a URL on each call;
change it to keep only a bounded suffix (e.g., last N characters where N >= max
URL length + overlap) instead of unbounded growth, and run the URL extraction
only against the new bounded buffer so you avoid repeated full-buffer scans and
unbounded memory use. Also introduce a reusable compiled matcher (store the
compiled regex/matcher used by DirectoryToolWebServerURLBuilder.extractURL as a
property referenced by urlPattern or wrap extractURL to accept a precompiled
matcher) and reuse it across append calls, keep the existing
lock/resolvedURL/didSignal/semaphore semantics, and ensure you trim or truncate
outputBuffer under the lock immediately after appending to enforce the bound and
still detect URLs that span chunk boundaries.
In `@Sources/CmuxConfig.swift`:
- Around line 400-403: CmuxDirectoryToolDefinition.init(from decoder:) currently
accepts any non-blank id; update the decoder to enforce the schema pattern
^[A-Za-z0-9._-]+$ by validating the id string (retrieved via
Self.requiredTrimmedString(forKey: .id, in: container)) against that regex and
throwing a DecodingError.dataCorruptedError(forKey: .id, in: container,
debugDescription: "...") when it doesn’t match; ensure the error message names
the invalid id and that validation occurs immediately after decoding id so
invalid values never propagate to commandPaletteCommandId or merge paths.
- Around line 431-448: The decoded startupTimeoutSeconds is accepted verbatim
but must be validated like the notification-hook timeout; after decoding
startupTimeoutSeconds via container.decodeIfPresent(Double.self, forKey:
.startupTimeoutSeconds) (the startupTimeoutSeconds local), check if the value is
non-nil, isFinite, and >= 1.0 and if not, throw a
DecodingError.dataCorruptedError(forKey: .startupTimeoutSeconds, in: container,
debugDescription: "startupTimeoutSeconds must be a finite number >= 1"); place
this validation immediately after the decode and before the switch that uses
startupTimeoutSeconds so invalid configs are rejected during decode.
---
Duplicate comments:
In `@Sources/CmuxConfig.swift`:
- Around line 480-519: The built-in localized strings in static let
defaultDefinitions are evaluated once at initialization and get frozen to the
first locale; change defaultDefinitions to a computed property so
String(localized:defaultValue:) is evaluated on each access. Replace the stored
property declaration static let defaultDefinitions:
[CmuxDirectoryToolDefinition] = [...] with a computed property static var
defaultDefinitions: [CmuxDirectoryToolDefinition] { return [...] } (keeping the
same CmuxDirectoryToolDefinition entries including ids like "vscode-inline" and
"jupyter" and keys such as title, subtitle, failureMessage) so localization
updates when app.language changes at runtime.
In `@web/data/cmux.schema.json`:
- Around line 42-134: The new "directoryTools" schema block contains user-facing
"description" strings but isn't wired into the localization pipeline; replace
those description fields (for "directoryTools" itself and its nested properties:
id, title, subtitle, keywords, enabled, kind, applicationBundlePathCandidates,
executablePathCandidates, command, cwd, urlRegex, failureMessage,
installCommand, startupTimeoutSeconds) with corresponding "descriptionKey"
entries and add matching keys/values into web/messages/<locale>.json for each
locale (following the existing next-intl key naming convention used elsewhere)
so the docs generator reads localized copy via descriptionKey; ensure keys
exactly match the strings you add in the schema and include all locales required
by the coding guidelines.
🪄 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: 40b309fa-2625-4283-a82a-e1579388c1c5
📒 Files selected for processing (8)
Resources/Localizable.xcstringsSources/App/TerminalDirectoryOpenSupport.swiftSources/CmuxConfig.swiftSources/ContentView.swiftcmuxTests/CmuxConfigTests.swiftcmuxTests/OmnibarAndToolsTests.swiftdocs/configuration.mdweb/data/cmux.schema.json
| func ensureWebServerURL( | ||
| tool: CmuxResolvedDirectoryTool, | ||
| directoryURL: URL, | ||
| completion: @escaping (LaunchResult) -> Void | ||
| ) { | ||
| let normalizedDirectoryURL = directoryURL.standardizedFileURL | ||
| let key = ServerKey(toolID: tool.id, directoryPath: normalizedDirectoryURL.path) | ||
| queue.async { | ||
| if let server = self.serversByKey[key], | ||
| server.process.isRunning { | ||
| DispatchQueue.main.async { | ||
| completion(.opened(server.url)) | ||
| } | ||
| return | ||
| } | ||
| self.serversByKey.removeValue(forKey: key) | ||
| self.launchQueue.async { | ||
| let result = self.launchWebServer(tool: tool, directoryURL: normalizedDirectoryURL) | ||
| self.queue.async { | ||
| if case .opened(let process, let url) = result { | ||
| self.serversByKey[key] = (process, url) | ||
| } | ||
| DispatchQueue.main.async { | ||
| switch result { | ||
| case .opened(_, let url): | ||
| completion(.opened(url)) | ||
| case .failed(let failure): | ||
| completion(.failed(failure)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Coalesce in-flight launches per ServerKey.
Two callers can reach this method for the same (tool.id, directoryURL) before the first launchWebServer(...) finishes. Because serversByKey is only populated after launch completion, the second request still starts a second process instead of joining the first one. That can spawn duplicate Jupyter/web-server instances for one directory and race which URL gets cached. Track a pending-launch/completion list per ServerKey and fan out the single result.
🤖 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/App/TerminalDirectoryOpenSupport.swift` around lines 903 - 935, The
method ensureWebServerURL can start duplicate launches because serversByKey is
only set after launch completes; add a pending-launches map (e.g.
pendingLaunches: [ServerKey: [ (LaunchResult) -> Void ] ]) keyed by ServerKey
and modify ensureWebServerURL to, inside the queue.async block, if no running
server exists then check pendingLaunches: if an entry exists append the
completion and return, else create a new array with the completion, start
launchWebServer(tool:directoryURL) as before, and when that launch finishes
(inside the existing queue/Dispatch flow) iterate the pending completions for
that ServerKey, invoke each with the single LaunchResult, then remove the
pendingLaunches entry and only then update serversByKey if opened; keep existing
synchronization using queue and launchQueue.
| if response == .alertFirstButtonReturn { | ||
| runDirectoryToolInstallCommand( | ||
| installCommand, | ||
| tool: tool, | ||
| directoryURL: directoryURL | ||
| ) | ||
| } else if response == .alertSecondButtonReturn { | ||
| NSPasteboard.general.clearContents() | ||
| NSPasteboard.general.setString(installCommand, forType: .string) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Log alert button actions to the debug event log.
The new “Run Install Command” / “Copy Install Command” user actions perform side effects but do not emit inline UI-event debug logs. Add non-sensitive action logging (e.g., tool id + selected action only).
As per coding guidelines: Sources/**/ContentView.swift should log mouse and UI events inline in views to the debug event log.
🤖 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/ContentView.swift` around lines 9968 - 9977, Add an inline
non-sensitive debug event log in the ContentView branch handling the alert
response: when response == .alertFirstButtonReturn, log a UI event (using the
same debug event logging facility used elsewhere in ContentView) recording the
tool identifier (e.g., tool.id) and the action "run_install_command" immediately
before calling runDirectoryToolInstallCommand(tool:directoryURL:); when response
== .alertSecondButtonReturn, log the tool identifier and the action
"copy_install_command" immediately before clearing/setting NSPasteboard and do
NOT include installCommand or other sensitive content in the log.
There was a problem hiding this comment.
2 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:10013">
P1: User-facing launch failure alert includes raw shell stdout/stderr, which can leak sensitive process output to the UI.</violation>
</file>
<file name="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:1000">
P2: Raw `error.localizedDescription` is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning `LaunchFailure`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } catch { | ||
| stdoutPipe.fileHandleForReading.readabilityHandler = nil | ||
| stderrPipe.fileHandleForReading.readabilityHandler = nil | ||
| return .failed(LaunchFailure(reason: .launchFailed, output: error.localizedDescription)) |
There was a problem hiding this comment.
P2: Raw error.localizedDescription is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning LaunchFailure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/App/TerminalDirectoryOpenSupport.swift, line 1000:
<comment>Raw `error.localizedDescription` is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning `LaunchFailure`.</comment>
<file context>
@@ -970,20 +997,27 @@ final class DirectoryToolWebServerController {
stdoutPipe.fileHandleForReading.readabilityHandler = nil
stderrPipe.fileHandleForReading.readabilityHandler = nil
- return nil
+ return .failed(LaunchFailure(reason: .launchFailed, output: error.localizedDescription))
}
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e5927db65
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let descriptor = CmuxActionTrustDescriptor( | ||
| actionID: "directoryTool.\(tool.id)", | ||
| kind: "directoryTool", | ||
| command: tool.displayCommand, |
There was a problem hiding this comment.
For a project-local shellWebServer, the trusted fingerprint and prompt only cover tool.displayCommand; execution also depends on executablePathCandidates (via CMUX_TOOL_EXECUTABLE) and cwd. If a user clicks “Trust and Run” for a benign project tool, that project can later keep the same command string but change the candidate executable path or working directory, and the next launch will skip the trust prompt while running different code/scope. Include these execution-affecting fields in the descriptor/display used for project-local directory tools.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/CmuxConfig.swift (2)
433-448:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
startupTimeoutSecondsin the decoder, not just in the schema.Line 433 accepts
0, negative, and non-finite values even thoughweb/data/cmux.schema.jsondeclares a minimum of1.cmux.jsonis parsed byJSONDecoder, so invalid values can still reach runtime and produce immediate timeout/failure behavior.Suggested fix
installCommand = try Self.optionalTrimmedString(forKey: .installCommand, in: container) startupTimeoutSeconds = try container.decodeIfPresent(Double.self, forKey: .startupTimeoutSeconds) + if let startupTimeoutSeconds, + !startupTimeoutSeconds.isFinite || startupTimeoutSeconds < 1 { + throw DecodingError.dataCorruptedError( + forKey: .startupTimeoutSeconds, + in: container, + debugDescription: "startupTimeoutSeconds must be greater than or equal to 1" + ) + } switch kind {🤖 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/CmuxConfig.swift` around lines 433 - 448, After decoding startupTimeoutSeconds from the container, validate its value (in CmuxConfig decoder) to enforce the schema minimum: if let secs = startupTimeoutSeconds, ensure secs.isFinite and secs >= 1.0; if the check fails throw DecodingError.dataCorruptedError(forKey: .startupTimeoutSeconds, in: container, debugDescription: "startupTimeoutSeconds must be a finite number >= 1"). Place this validation immediately after the startupTimeoutSeconds = try ... line so invalid 0, negative or non-finite values are rejected during decoding.
400-448:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBuilt-in
directoryToolsoverrides currently replace defaults instead of overriding them.Reusing
id: "jupyter"orid: "vscode-inline"with only the changed fields is whatdocs/configuration.mdandweb/data/cmux.schema.jsonadvertise, butresolvedDirectoryToolsoverwrites the prior entry wholesale. That means a partial override either fails decode (title/commandbecome required again) or silently drops the built-in metadata. Merge matching IDs field-by-field before validation, or tighten the schema/docs to require full replacement objects.Also applies to: 2713-2762
🤖 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/CmuxConfig.swift` around lines 400 - 448, The directoryTools decoding currently replaces built-in entries instead of merging overrides, causing partial overrides (e.g., id "jupyter"/"vscode-inline") to lose defaults or fail validation; update the decoding path that builds resolvedDirectoryTools so that when decoding a CmuxDirectoryTool (init(from decoder: Decoder)) you first look up an existing built-in entry by id, start from that default object, then overlay only the non-nil/non-empty decoded fields (title, command, keywords, applicationBundlePathCandidates, executablePathCandidates, kind, etc.) onto it before running the validation in init(from:); in short, perform a field-by-field merge of the decoded tool into the matched built-in tool instead of wholesale replacement so partial config objects correctly inherit defaults.
♻️ Duplicate comments (2)
Sources/CmuxConfig.swift (1)
480-520:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't freeze localized default tool labels in a
static let.
defaultDefinitionsis initialized once, so theString(localized:defaultValue:)titles/subtitles are captured in the first locale and reused for the rest of the process.loadedDirectoryToolsis then populated from that array and consumed by the menu/palette wiring inSources/cmuxApp.swift:1008-1078, so changing the app language later will keep the old built-in labels until restart/reload. This is the same issue raised earlier and still appears unresolved.In Swift, are static let property initializers evaluated only once per process, and would String(localized:defaultValue:) values created there stay stale if the app locale changes later at runtime?🤖 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/CmuxConfig.swift` around lines 480 - 520, defaultDefinitions is freezing localized titles/subtitles at process-start because it's a static let; change it so localization is evaluated at runtime (e.g., make defaultDefinitions a computed static var or a factory method that returns [CmuxDirectoryToolDefinition]) and stop embedding String(localized:defaultValue:) directly in a compile-time static initializer. Update the code that populates loadedDirectoryTools to call the new computed/factory defaultDefinitions (or re-localize title/subtitle there) so menu/palette labels are generated using the current locale whenever loadedDirectoryTools is built or refreshed; refer to CmuxDirectoryToolDefinition, defaultDefinitions, and loadedDirectoryTools to locate the affected symbols. Ensure no other static let initializers contain localized Strings to avoid similar staleness.web/data/cmux.schema.json (1)
42-133:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftWire the new schema copy into the localized docs pipeline.
This block adds a full set of user-facing descriptions for
directoryTools, but none of them exposedescriptionKey, so localized configuration docs will keep showing this section in English. Please adddescriptionKeyentries for the new top-level block and nested properties, then populate the correspondingweb/messages/en.jsonandweb/messages/ja.jsonentries. Based on learnings,descriptionKeyis the hook the web docs use for localized schema copy viaweb/app/[locale]/docs/configurationandweb/messages/<locale>.json.🤖 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 `@web/data/cmux.schema.json` around lines 42 - 133, The schema's directoryTools block and its nested properties (directoryTools and its properties id, title, subtitle, keywords, enabled, kind, applicationBundlePathCandidates, executablePathCandidates, command, cwd, urlRegex, failureMessage, installCommand, startupTimeoutSeconds) need descriptionKey entries added so localized docs pick up translations; add a descriptionKey for the top-level "directoryTools" object and for each nested property in the schema, then add corresponding keys and localized strings into the messages JSON for English and Japanese (the en and ja messages files used by the docs pipeline) so the documentation renders localized descriptions.
🤖 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.
Outside diff comments:
In `@Sources/CmuxConfig.swift`:
- Around line 433-448: After decoding startupTimeoutSeconds from the container,
validate its value (in CmuxConfig decoder) to enforce the schema minimum: if let
secs = startupTimeoutSeconds, ensure secs.isFinite and secs >= 1.0; if the check
fails throw DecodingError.dataCorruptedError(forKey: .startupTimeoutSeconds, in:
container, debugDescription: "startupTimeoutSeconds must be a finite number >=
1"). Place this validation immediately after the startupTimeoutSeconds = try ...
line so invalid 0, negative or non-finite values are rejected during decoding.
- Around line 400-448: The directoryTools decoding currently replaces built-in
entries instead of merging overrides, causing partial overrides (e.g., id
"jupyter"/"vscode-inline") to lose defaults or fail validation; update the
decoding path that builds resolvedDirectoryTools so that when decoding a
CmuxDirectoryTool (init(from decoder: Decoder)) you first look up an existing
built-in entry by id, start from that default object, then overlay only the
non-nil/non-empty decoded fields (title, command, keywords,
applicationBundlePathCandidates, executablePathCandidates, kind, etc.) onto it
before running the validation in init(from:); in short, perform a field-by-field
merge of the decoded tool into the matched built-in tool instead of wholesale
replacement so partial config objects correctly inherit defaults.
---
Duplicate comments:
In `@Sources/CmuxConfig.swift`:
- Around line 480-520: defaultDefinitions is freezing localized titles/subtitles
at process-start because it's a static let; change it so localization is
evaluated at runtime (e.g., make defaultDefinitions a computed static var or a
factory method that returns [CmuxDirectoryToolDefinition]) and stop embedding
String(localized:defaultValue:) directly in a compile-time static initializer.
Update the code that populates loadedDirectoryTools to call the new
computed/factory defaultDefinitions (or re-localize title/subtitle there) so
menu/palette labels are generated using the current locale whenever
loadedDirectoryTools is built or refreshed; refer to
CmuxDirectoryToolDefinition, defaultDefinitions, and loadedDirectoryTools to
locate the affected symbols. Ensure no other static let initializers contain
localized Strings to avoid similar staleness.
In `@web/data/cmux.schema.json`:
- Around line 42-133: The schema's directoryTools block and its nested
properties (directoryTools and its properties id, title, subtitle, keywords,
enabled, kind, applicationBundlePathCandidates, executablePathCandidates,
command, cwd, urlRegex, failureMessage, installCommand, startupTimeoutSeconds)
need descriptionKey entries added so localized docs pick up translations; add a
descriptionKey for the top-level "directoryTools" object and for each nested
property in the schema, then add corresponding keys and localized strings into
the messages JSON for English and Japanese (the en and ja messages files used by
the docs pipeline) so the documentation renders localized descriptions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cc88603-9dfd-4c98-bd79-55248a92b6c5
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/CmuxConfig.swiftSources/ContentView.swiftcmuxTests/TerminalAndGhosttyTests.swiftdocs/configuration.mdweb/data/cmux.schema.json
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/CmuxConfig.swift">
<violation number="1" location="Sources/CmuxConfig.swift:517">
P2: The new install command executes a remote script via `curl | sh`, which is unsafe for a one-click in-app install action.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| localized: "directoryTool.jupyter.failureMessage", | ||
| defaultValue: "Jupyter is not installed, uvx is not available, or the tool did not print a local URL. Install uv, then run this command again." | ||
| ), | ||
| installCommand: "if command -v brew >/dev/null 2>&1; then brew install uv; else curl -LsSf https://astral.sh/uv/install.sh | sh; fi", |
There was a problem hiding this comment.
P2: The new install command executes a remote script via curl | sh, which is unsafe for a one-click in-app install action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CmuxConfig.swift, line 517:
<comment>The new install command executes a remote script via `curl | sh`, which is unsafe for a one-click in-app install action.</comment>
<file context>
@@ -503,17 +503,18 @@ struct CmuxDirectoryToolDefinition: Codable, Sendable, Hashable {
+ defaultValue: "Jupyter is not installed, uvx is not available, or the tool did not print a local URL. Install uv, then run this command again."
),
- installCommand: "python3 -m pip install --user jupyterlab",
+ installCommand: "if command -v brew >/dev/null 2>&1; then brew install uv; else curl -LsSf https://astral.sh/uv/install.sh | sh; fi",
startupTimeoutSeconds: 20
),
</file context>
| installCommand: "if command -v brew >/dev/null 2>&1; then brew install uv; else curl -LsSf https://astral.sh/uv/install.sh | sh; fi", | |
| installCommand: "if command -v brew >/dev/null 2>&1; then brew install uv; else echo 'Please install uv manually: https://docs.astral.sh/uv/getting-started/installation/'; fi", |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2faf33dbde
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| AppDelegate.shared?.showOpenFolderInInlineVSCodePanel( | ||
| vscodeApplicationURL: configuredInlineVSCodeApplicationURL() | ||
| ) |
There was a problem hiding this comment.
When the active workspace has a project-local .cmux/cmux.json overriding vscodeServeWeb, this File menu path now resolves that project-local tool and passes only its application URL into showOpenFolderInInlineVSCodePanel, so the later launch has no sourcePath/tool descriptor to run through the project automation trust prompt. In that scenario a malicious repo can point applicationBundlePathCandidates at a fake .app containing Contents/Resources/app/bin/code-tunnel, and choosing File > Open Folder in VS Code (Inline)… executes it without the trust gate used for shell directory tools. Fresh evidence beyond the existing command-palette finding is that this menu entry was changed in this diff to source the configured tool URL directly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
cmux/Sources/AppDelegate.swift
Line 7284 in 5839742
Honor the requested VS Code application
When a configured vscodeServeWeb tool points at a different VS Code bundle after an inline server is already running, this call still goes through the singleton VSCodeServeWebController. I checked ensureServeWebURL, and it returns the cached serveWebURL whenever serveWebProcess is running before comparing the requested vscodeApplicationURL, so the new preferred bundle is ignored until the user manually stops/restarts the server. This makes custom/project-local VS Code directory tools open folders in the previously launched backend instead of the configured one.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:377">
P3: New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.</violation>
<violation number="2" location="Sources/App/TerminalDirectoryOpenSupport.swift:1000">
P2: Raw `error.localizedDescription` is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning `LaunchFailure`.</violation>
<violation number="3" location="Sources/App/TerminalDirectoryOpenSupport.swift:1109">
P3: Final progress output is derived after clearing the buffer, so exit-time URL detection emits an empty progress update.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:1041">
P2: Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:10013">
P1: User-facing launch failure alert includes raw shell stdout/stderr, which can leak sensitive process output to the UI.</violation>
</file>
<file name="Sources/CmuxConfig.swift">
<violation number="1" location="Sources/CmuxConfig.swift:517">
P2: The new install command executes a remote script via `curl | sh`, which is unsafe for a one-click in-app install action.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8826491009
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| var stopCommandPaletteCommandId: String { | ||
| "\(commandPaletteCommandId).stop" | ||
| } |
There was a problem hiding this comment.
Because the stop command id is derived by appending .stop, a configured tool whose id already uses that suffix can collide with another tool's stop command; for example, the schema allows id: "jupyter.stop", which makes its launch command id identical to the built-in Jupyter stop command. In that setup the command palette registers both contributions under the same id and the later handler overwrites the earlier one, so selecting “Stop Jupyter” can launch the custom tool instead of stopping the server.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:377">
P3: New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.</violation>
<violation number="2" location="Sources/App/TerminalDirectoryOpenSupport.swift:1000">
P2: Raw `error.localizedDescription` is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning `LaunchFailure`.</violation>
<violation number="3" location="Sources/App/TerminalDirectoryOpenSupport.swift:1083">
P2: Manual locking was introduced in this file’s new launch handle, which violates the file’s concurrency rule and adds blocking synchronization to process lifecycle state.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:1041">
P2: Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.</violation>
</file>
<file name="Sources/CmuxConfig.swift">
<violation number="1" location="Sources/CmuxConfig.swift:517">
P2: The new install command executes a remote script via `curl | sh`, which is unsafe for a one-click in-app install action.</violation>
<violation number="2" location="Sources/CmuxConfig.swift:580">
P2: The stop command ID is derived by appending `.stop` to `commandPaletteCommandId`. This creates a collision risk: if tool A has id `"jupyter"`, its stop command becomes `palette.terminalDirectoryTool.jupyter.stop`, which is identical to the launch command of a tool B with id `"jupyter.stop"`. Since the schema allows arbitrary tool IDs, the later `registry.register` call would overwrite the earlier handler, causing unexpected behavior in the command palette. Consider using a separator that cannot appear in tool IDs (e.g., `__stop`) or validating that no tool ID ends with `.stop`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| "palette.terminalDirectoryTool.\(id)" | ||
| } | ||
|
|
||
| var stopCommandPaletteCommandId: String { |
There was a problem hiding this comment.
P2: The stop command ID is derived by appending .stop to commandPaletteCommandId. This creates a collision risk: if tool A has id "jupyter", its stop command becomes palette.terminalDirectoryTool.jupyter.stop, which is identical to the launch command of a tool B with id "jupyter.stop". Since the schema allows arbitrary tool IDs, the later registry.register call would overwrite the earlier handler, causing unexpected behavior in the command palette. Consider using a separator that cannot appear in tool IDs (e.g., __stop) or validating that no tool ID ends with .stop.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/CmuxConfig.swift, line 580:
<comment>The stop command ID is derived by appending `.stop` to `commandPaletteCommandId`. This creates a collision risk: if tool A has id `"jupyter"`, its stop command becomes `palette.terminalDirectoryTool.jupyter.stop`, which is identical to the launch command of a tool B with id `"jupyter.stop"`. Since the schema allows arbitrary tool IDs, the later `registry.register` call would overwrite the earlier handler, causing unexpected behavior in the command palette. Consider using a separator that cannot appear in tool IDs (e.g., `__stop`) or validating that no tool ID ends with `.stop`.</comment>
<file context>
@@ -577,6 +577,10 @@ struct CmuxResolvedDirectoryTool: Sendable, Hashable, Identifiable {
"palette.terminalDirectoryTool.\(id)"
}
+ var stopCommandPaletteCommandId: String {
+ "\(commandPaletteCommandId).stop"
+ }
</file context>
| } | ||
|
|
||
| final class DirectoryToolWebServerLaunchHandle { | ||
| private let lock = NSLock() |
There was a problem hiding this comment.
P2: Manual locking was introduced in this file’s new launch handle, which violates the file’s concurrency rule and adds blocking synchronization to process lifecycle state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/App/TerminalDirectoryOpenSupport.swift, line 1083:
<comment>Manual locking was introduced in this file’s new launch handle, which violates the file’s concurrency rule and adds blocking synchronization to process lifecycle state.</comment>
<file context>
@@ -1061,6 +1079,45 @@ final class DirectoryToolWebServerController {
}
+final class DirectoryToolWebServerLaunchHandle {
+ private let lock = NSLock()
+ private var process: Process?
+ private var isCancelled = false
</file context>
| } | ||
| } | ||
|
|
||
| private func openFocusedDirectoryInInlineVSCode(_ directoryURL: URL) -> Bool { | ||
| AppDelegate.shared?.openDirectoryInInlineVSCode(directoryURL, tabManager: tabManager) ?? false | ||
| private func openFocusedDirectory(_ directoryURL: URL, with tool: CmuxResolvedDirectoryTool) -> Bool { | ||
| guard tool.isAvailable() else { return false } | ||
| switch tool.kind { | ||
| case .vscodeServeWeb: | ||
| guard let vscodeApplicationURL = tool.applicationURL() else { return false } | ||
| return openFocusedDirectoryInInlineVSCode(directoryURL, vscodeApplicationURL: vscodeApplicationURL) | ||
| case .shellWebServer: | ||
| return openFocusedDirectoryInShellDirectoryTool(directoryURL, tool: tool) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
DirectoryToolLaunchProgressController deallocated before user interaction
progressController is a local var declared inside the @escaping onAuthorized closure. The onAllow: { progressController in … } argument uses a parameter named progressController, which shadows the outer variable — so no inner closure captures the outer reference strongly. The onStop closure only captures launchHandle. panel.delegate, allowButton.target, and stopButton.target are all weak in AppKit.
Consequence: when onAuthorized returns after progressController.show(…), the outer variable goes out of scope with no remaining strong reference, and ARC deallocates the controller. The panel stays visible but all button targets zero out; clicking Allow or Cancel does nothing, and windowWillClose is never delivered. The progress dialog is permanently frozen.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1d3fce566
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func windowWillClose(_ notification: Notification) { | ||
| if !isClosed && !didAllow { | ||
| onStop() |
There was a problem hiding this comment.
After a user clicks Allow for a shell directory tool, closing the launch panel with the window close control no longer calls onStop because didAllow is true. In that scenario the child server keeps starting in the background and can still open a browser split even though the user dismissed the progress window; the close control should behave like Stop once the launch has begun.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/App/TerminalDirectoryOpenSupport.swift">
<violation number="1" location="Sources/App/TerminalDirectoryOpenSupport.swift:377">
P3: New helper duplicates existing dedupe logic in the same file instead of reusing a shared utility.</violation>
<violation number="2" location="Sources/App/TerminalDirectoryOpenSupport.swift:493">
P2: `ensureServeWebURL` returns the cached `serveWebURL` whenever any serve-web process is already running, without verifying that the running server was launched for the same `vscodeApplicationURL`. With multiple `vscodeServeWeb` directory tools configured (e.g., VS Code vs. VS Code Insiders), the user could get a URL routed to the wrong VS Code instance.</violation>
<violation number="3" location="Sources/App/TerminalDirectoryOpenSupport.swift:1000">
P2: Raw `error.localizedDescription` is propagated to user-facing failure text, which can leak internal path/upstream details; sanitize this field before returning `LaunchFailure`.</violation>
<violation number="4" location="Sources/App/TerminalDirectoryOpenSupport.swift:1083">
P2: Manual locking was introduced in this file’s new launch handle, which violates the file’s concurrency rule and adds blocking synchronization to process lifecycle state.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:1041">
P2: Fallback now checks only VS Code app presence, which can enable “Open Folder in VS Code (Inline)” even when inline serve-web is unavailable.</violation>
</file>
<file name="Sources/CmuxConfig.swift">
<violation number="1" location="Sources/CmuxConfig.swift:517">
P2: The new install command executes a remote script via `curl | sh`, which is unsafe for a one-click in-app install action.</violation>
<violation number="2" location="Sources/CmuxConfig.swift:580">
P2: The stop command ID is derived by appending `.stop` to `commandPaletteCommandId`. This creates a collision risk: if tool A has id `"jupyter"`, its stop command becomes `palette.terminalDirectoryTool.jupyter.stop`, which is identical to the launch command of a tool B with id `"jupyter.stop"`. Since the schema allows arbitrary tool IDs, the later `registry.register` call would overwrite the earlier handler, causing unexpected behavior in the command palette. Consider using a separator that cannot appear in tool IDs (e.g., `__stop`) or validating that no tool ID ends with `.stop`.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| #endif | ||
|
|
||
| func ensureServeWebURL(vscodeApplicationURL: URL, completion: @escaping (URL?) -> Void) { | ||
| func ensureServeWebURL( |
There was a problem hiding this comment.
P2: ensureServeWebURL returns the cached serveWebURL whenever any serve-web process is already running, without verifying that the running server was launched for the same vscodeApplicationURL. With multiple vscodeServeWeb directory tools configured (e.g., VS Code vs. VS Code Insiders), the user could get a URL routed to the wrong VS Code instance.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/App/TerminalDirectoryOpenSupport.swift, line 493:
<comment>`ensureServeWebURL` returns the cached `serveWebURL` whenever any serve-web process is already running, without verifying that the running server was launched for the same `vscodeApplicationURL`. With multiple `vscodeServeWeb` directory tools configured (e.g., VS Code vs. VS Code Insiders), the user could get a URL routed to the wrong VS Code instance.</comment>
<file context>
@@ -490,7 +490,11 @@ final class VSCodeServeWebController {
#endif
- func ensureServeWebURL(vscodeApplicationURL: URL, completion: @escaping (URL?) -> Void) {
+ func ensureServeWebURL(
+ vscodeApplicationURL: URL,
+ progress: ((String) -> Void)? = nil,
</file context>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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 8c3c801. Configure here.
| return openFocusedDirectoryInInlineVSCode(directoryURL, vscodeApplicationURL: vscodeApplicationURL) | ||
| case .shellWebServer: | ||
| return openFocusedDirectoryInShellDirectoryTool(directoryURL, tool: tool) | ||
| } |
There was a problem hiding this comment.
Project VS Code tools skip trust
Medium Severity
Project-local shellWebServer directory tools go through authorizeDirectoryToolIfNeeded, but vscodeServeWeb tools call inline VS Code directly with no project trust check, despite the same local sourcePath semantics.
Reviewed by Cursor Bugbot for commit 8c3c801. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8c3c80154c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| VSCodeServeWebController.shared.ensureServeWebURL( | ||
| vscodeApplicationURL: vscodeApplicationURL, |
There was a problem hiding this comment.
When a configured vscodeServeWeb tool points at a different VS Code bundle, this call still goes through the singleton VSCodeServeWebController, whose cache hit path in ensureServeWebURL only checks that some serve-web process is running and returns its URL without comparing the requested vscodeApplicationURL. In a setup with multiple VS Code tools (or after changing the configured bundle from Stable to Insiders), invoking the new tool reuses the old server/version/extensions instead of launching the configured app, so the new customization is silently ignored until the global server is stopped.
Useful? React with 👍 / 👎.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Summary
Testing
Issues
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Runs user-configurable shell commands and subprocesses to bind localhost services and open URLs in-app; mitigated by trust prompts and localhost-focused URL parsing, but misconfiguration or command injection in config remains a concern.
Overview
Introduces configurable
directoryToolsso terminal “open directory” actions can launch browser splits via VS Codeserve-webor arbitrary shell web servers (built-in Jupyter plus overrides incmux.json).Config & registry:
CmuxConfig/CmuxConfigStoreload and merge default + global/local tool definitions; the command palette registers open/stop commands per tool and drops VS Code Inline from the legacyTerminalDirectoryOpenTargetshortcut list in favor of the tool registry.Launch UX: New
DirectoryToolLaunchPanelControllershows command review (Allow), live log output, and Stop/Cancel; inline VS Code and shell tools stream stdout/stderr while waiting for a local URL. Failures surfacefailureMessage, captured output, and optional Run/Copy install commands; project-local tools use the existing automation trust flow.Runtime:
DirectoryToolWebServerControllerrunszsh -lcwithCMUX_DIRECTORY/CMUX_TOOL_*, parses URLs via regex, caches one server per tool+directory, and reusesServeWebOutputCollectorprogress hooks for VS Code.Reviewed by Cursor Bugbot for commit 8c3c801. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds customizable directory tools to open the focused terminal directory in a browser split, with built‑in
vscode-inlineandjupyter. Aligns Jupyter mode with inline VS Code via a customizable abstraction.directoryToolsincmux.json(kinds:vscodeServeWeb,shellWebServer); built-ins:vscode-inline,jupyter. Override or disable defaults by id.shellWebServerruns a command in the directory, captures a local URL via default/customurlRegex, exportsCMUX_DIRECTORY,CMUX_TOOL_ID,CMUX_TOOL_EXECUTABLE, reuses servers per tool+directory, and supportsstartupTimeoutSeconds.jupytertries localjupyter/jupyter-lab, falls back touvx --from jupyterlab jupyter lab, includes aninstallCommandforuv, and has refined launch controls (clearer progress and Stop behavior).vscodeServeWeb. The old.vscodeInlinetarget is removed from generic open-directory shortcuts, and the menu item is disabled unless configured.Written for commit 8c3c801. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Localization
Tests