Repository navigation
Add per-workspace environment variables inherited by every shell (#5995) - #6116
Merged
Merged
Conversation
Give a workspace a set of user-defined env vars that every shell spawned in it inherits — the initial terminal plus every later pane/surface/split, and every surface recreated on session restore — without editing global shell init or re-exporting per pane. Model & injection - Workspace carries a persistent `workspaceEnvironment` dictionary, folded into the startup environment at the two terminal-creation choke points (`init` for the initial shell, `newTerminalSurface`/`newTerminalSplit` for every later surface and for session-restore, which routes through `newTerminalSurface`). It flows through the existing `additionalEnvironment` / `initialEnvironmentOverrides` channels, so the managed `CMUX_*` and terminal-identity vars (protected keys in `mergedStartupEnvironment`) always win and can never be clobbered. Explicit per-surface env (layout `env`, scrollback replay, SSH startup) overlays the workspace set. Persistence - Stored on `SessionWorkspaceSnapshot.environment` (optional, so older manifests decode cleanly) and restored before surfaces are rebuilt. Entry points - CLI: repeatable `--env KEY=VALUE` and `--env-file <path>` on `new-workspace` / `workspace create` (file values overridden by `--env`). - cmux.json: an `env` object on a workspace definition. - Socket: `workspace_env` (alias `env`) on `workspace.create`. Inspect - `cmux workspace env [<handle>] [--mask] [--json]` via a new worker-lane `workspace.env` control method. `--mask` redacts values; the env set is kept out of `workspace list` so a plain listing never leaks secrets. Tests, docs, localization - WorkspaceEnvironmentTests covers the acceptance paths (initial shell, later pane, explicit-override precedence, restore round-trip, CMUX_* protection, Codable back-compat, config decode), wired into the pbxproj. - docs/cli-contract.md documents the flags, the inspect command, and the precedence vs shell init files and protected CMUX_* vars. - Localizable.xcstrings updated (en/ja/ko/uk) for the workspace usage/error strings and the new empty-output string. Closes #5995 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-workspace user-defined environment variables to cmux. A new ChangesWorkspace environment variables
Sequence DiagramsequenceDiagram
actor User
participant CLI as cmux CLI
participant TerminalController
participant TabManager
participant Workspace
User->>CLI: cmux new-workspace --env KEY=VALUE --env-file .cmux.env
CLI->>CLI: parseWorkspaceEnvOptions + buildWorkspaceEnvironment
CLI->>TerminalController: workspace.create {workspace_env: {...}}
TerminalController->>TerminalController: v2WorkspaceCreate — sanitizedWorkspaceEnvironment
TerminalController->>TabManager: addWorkspace(workspaceEnvironment:)
TabManager->>Workspace: init(workspaceEnvironment:)
Workspace->>Workspace: sanitize + store workspaceEnvironment
User->>CLI: cmux workspace env <id> --mask
CLI->>TerminalController: workspace.env {workspace_id}
TerminalController->>Workspace: resolve + read workspaceEnvironment
TerminalController-->>CLI: {env, count}
CLI-->>User: print masked KEY=*** lines
Note over Workspace: On every shell spawn:<br/>startupEnvironmentMergingWorkspaceEnvironment<br/>(workspace env + surface overrides, CMUX_* protected)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 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 |
…env-vars # Conflicts: # Resources/Localizable.xcstrings # cmux.xcodeproj/project.pbxproj
Contributor
Greptile SummaryThis PR adds per-workspace environment variables that every shell in a workspace inherits — the initial terminal, all later panes/splits, and restored surfaces after session restart. The implementation is thorough: a single sanitizer (
Confidence Score: 5/5Safe to merge. All terminal-creation paths have been updated to thread the workspace environment, the previously flagged splitPaneWithNewTerminal miss is now fixed and test-covered, localization is complete across all four locales, and the NUL/= sanitizer correctly closes the Swift→C boundary bypass. The feature is well-scoped with a single sanitizer choke point, proper actor isolation (nonisolated helpers, v2MainSync for the socket handler), complete back-compat for session manifests, and a thorough test suite covering every acceptance path. The one comment is a cosmetic improvement to the env-file I/O error format. CLI/cmux.swift — minor: String(describing: error) in the --env-file read-failure message emits raw NSError internals; suggest error.localizedDescription instead. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CLI["CLI --env / --env-file"] -->|buildWorkspaceEnvironment| A
JSON["cmux.json env: {}"] -->|CmuxConfigExecutor| A
SOCK["socket workspace.create\nworkspace_env param"] -->|sanitizedWorkspaceEnvironment| A
A["Workspace.sanitizedWorkspaceEnvironment\n(NUL/= guard, trim, drop blanks)"] --> WS["Workspace.workspaceEnvironment"]
WS -->|startupEnvironmentMergingWorkspaceEnvironment| INIT["init — initialTerminalEnvironment\n(workspace base + surface overlay)"]
WS -->|startupEnvironmentMergingWorkspaceEnvironment| NTS["newTerminalSurface / newTerminalSplit\n(additionalEnvironment)"]
WS -->|startupEnvironmentMergingWorkspaceEnvironment| SPL["splitPaneWithNewTerminal\n(additionalEnvironment)"]
WS -->|startupEnvironmentMergingWorkspaceEnvironment| REP["createReplacementTerminalPanel\n(additionalEnvironment)"]
WS -->|startupEnvironmentMergingWorkspaceEnvironment| SSH["SSH surfaces\n(terminalStartupEnvironment)"]
INIT --> SHELL["Shell process\nvia mergedStartupEnvironment\n(CMUX_* protected keys win)"]
NTS --> SHELL
SPL --> SHELL
REP --> SHELL
SSH --> SHELL
WS -->|sessionSnapshot| SNAP["SessionWorkspaceSnapshot\n.environment (optional)"]
SNAP -->|restoreSessionSnapshot\nbefore surface rebuild| WS
Reviews (12): Last reviewed commit: "Merge origin/main into issue-5995-worksp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
CLI/cmux.swift, TerminalController.swift, Workspace.swift, TabManager.swift, SessionPersistence.swift, and CmuxConfigExecutor.swift grew with the per-workspace environment feature (#5995). Accept the new line counts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 7192-7194: In the error handling block for the --env-file reading
operation in the catch block, replace the use of `error.localizedDescription`
with `String(describing: error)` in the CLIError message construction. This
ensures the full error description is displayed rather than potentially generic
messages that can occur with non-LocalizedError types like CocoaError or
NSError, aligning with the repository's error formatting convention.
In `@cmuxTests/WorkspaceEnvironmentTests.swift`:
- Around line 41-47: Add a new test method to the WorkspaceEnvironmentTests file
that covers environment inheritance for the newTerminalSplit function. Create a
test similar to testLaterSurfaceInheritsWorkspaceEnvironment but instead of
calling newTerminalSurface, call newTerminalSplit on a workspace with a
workspaceEnvironment set, and verify that the resulting terminal panel inherits
the workspace environment variables through its respawnAdditionalEnvironment
property, matching the coverage provided for newTerminalSurface.
🪄 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: 3cffdd79-da14-4f22-b21c-1451c26be1f8
📒 Files selected for processing (13)
CLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftPackages/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandExecutionPolicyTests.swiftResources/Localizable.xcstringsSources/CmuxConfigExecutor.swiftSources/CmuxWorkspaceDefinition.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceEnvironmentTests.swiftdocs/cli-contract.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 7607-7613: The issue is in the "join" case where the target
surface lookup incorrectly uses positionals.dropFirst().first when the source
surface is provided via the --surface option instead of as a positional
argument. When --surface is used, positionals contains only the target (since
the source comes from the option), so dropFirst() removes that element and the
target lookup fails. To fix this, check whether the source surface comes from a
positional argument or the --surface option: if --surface is provided via
optionValue, use positionals.first as the target directly instead of
dropFirst().first; otherwise, maintain the current dropFirst() logic for when
both surfaces are positional arguments.
🪄 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: 53ffaa9d-d6a1-41a2-b15e-9aeca4cf9e3c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
CLI/cmux.swift
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 7607-7613: The issue is in the "join" case where the target
surface lookup incorrectly uses positionals.dropFirst().first when the source
surface is provided via the --surface option instead of as a positional
argument. When --surface is used, positionals contains only the target (since
the source comes from the option), so dropFirst() removes that element and the
target lookup fails. To fix this, check whether the source surface comes from a
positional argument or the --surface option: if --surface is provided via
optionValue, use positionals.first as the target directly instead of
dropFirst().first; otherwise, maintain the current dropFirst() logic for when
both surfaces are positional arguments.
🪄 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: 53ffaa9d-d6a1-41a2-b15e-9aeca4cf9e3c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
CLI/cmux.swift
🛑 Comments failed to post (1)
CLI/cmux.swift (1)
7607-7613:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
canvas join --surface <id> <target>drops the target positional.When the source surface comes from
--surface,positionalsonly contains the target. This branch still doespositionals.dropFirst().first, socmux canvas join --surface surface:1 surface:2always falls into the usage error instead of sendingtarget_surface_id.Proposed fix
case "join": - try surfaceParam(positional: positionals.first, required: true) - guard let targetRaw = positionals.dropFirst().first ?? optionValue(rest, name: "--target"), + let sourcePositional: String? + let targetPositional: String? + if optionValue(rest, name: "--surface") != nil { + sourcePositional = nil + targetPositional = positionals.first + } else { + sourcePositional = positionals.first + targetPositional = positionals.dropFirst().first + } + try surfaceParam(positional: sourcePositional, required: true) + guard let targetRaw = targetPositional ?? optionValue(rest, name: "--target"), let targetId = try normalizeSurfaceHandle(targetRaw, client: client) else { throw CLIError(message: "Usage: cmux canvas join <surface> <target-surface>") } params["target_surface_id"] = targetId method = "canvas.join"🤖 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 `@CLI/cmux.swift` around lines 7607 - 7613, The issue is in the "join" case where the target surface lookup incorrectly uses positionals.dropFirst().first when the source surface is provided via the --surface option instead of as a positional argument. When --surface is used, positionals contains only the target (since the source comes from the option), so dropFirst() removes that element and the target lookup fails. To fix this, check whether the source surface comes from a positional argument or the --surface option: if --surface is provided via optionValue, use positionals.first as the target directly instead of dropFirst().first; otherwise, maintain the current dropFirst() logic for when both surfaces are positional arguments.
…framework - WorkspaceUnitTests: update the two `makeWorkspaceForCreation` test overrides to include the new `workspaceEnvironment` parameter (CI `tests` compile fix). - Workspace.sanitizedWorkspaceEnvironment: reject keys containing NUL or `=` and values containing NUL. A key like `CMUX_SOCKET_PATH\0x` would pass the exact-match protected-key check but truncate to `CMUX_SOCKET_PATH` at the Swift→C boundary (strdup/Ghostty) and clobber the managed variable. The sanitizer is the single choke point for every entry point, so the guard cannot be bypassed (autoreview P1). - WorkspaceEnvironmentTests: convert to Swift Testing per repo policy for new non-UI tests, and add regression coverage for the NUL/`=` rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Add a Swift Testing case covering workspace-env inheritance through newTerminalSplit (the second later-surface choke point), matching the newTerminalSurface coverage (CodeRabbit). - Use String(describing:) instead of error.localizedDescription for the --env-file read failure, per CLI error-formatting convention (CodeRabbit). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Workspace.swift (NUL/= key guard) and WorkspaceUnitTests.swift (the two makeWorkspaceForCreation override updates) grew past the budget. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/Workspace.swift (2)
2472-2481: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep
workspaceEnvironmentbehind a sanitizing setter.This new property is still directly writable, so later assignments can bypass
sanitizedWorkspaceEnvironment(...)and reintroduce the NUL/=cases this PR is trying to close. Please make itprivate(set)and funnel mutations through a helper, or sanitize indidSet.🤖 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/Workspace.swift` around lines 2472 - 2481, The workspaceEnvironment property is currently publicly writable, which allows direct assignments to bypass the sanitizedWorkspaceEnvironment(...) sanitization logic and potentially reintroduce NUL and equals-sign vulnerabilities. Make the workspaceEnvironment property private(set) to prevent direct external assignment, then create a public method or computed property setter that routes mutations through the sanitizedWorkspaceEnvironment(...) function before assigning the sanitized value. Alternatively, add a didSet observer to the workspaceEnvironment property that automatically sanitizes any newly assigned value by calling sanitizedWorkspaceEnvironment(...).
5730-5734:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftNot every terminal creation path goes through the workspace-env merge.
The helper is only wired into
newTerminalSplit/newTerminalSurface, but this file still creates terminals directly increateReplacementTerminalPanel()(Lines 9227-9246),splitPaneWithNewTerminal(...)(Lines 10285-10315), and both direct-construction branches insplitTabBar(_:didSplitPane:)(Lines 11633-11712). Those shells will missworkspaceEnvironment, so the “inherit on every shell/pane/split” contract is still broken. Please route allTerminalPanelcreation through one shared constructor/helper or mergeworkspaceEnvironmentin the remaining sites.As per coding guidelines, “When a behavior is exposed through multiple entrypoints ... implement one shared action/model path and verify every entrypoint that should invoke it.”
🤖 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/Workspace.swift` around lines 5730 - 5734, The `startupEnvironmentMergingWorkspaceEnvironment` helper function is only used in `newTerminalSplit` and `newTerminalSurface`, but terminals are also created directly in three other locations: `createReplacementTerminalPanel`, `splitPaneWithNewTerminal`, and the terminal creation branches in `splitTabBar`. To fix this inconsistency, either consolidate all TerminalPanel creation through a single shared constructor/helper that applies workspace environment merging, or explicitly apply the workspace environment merge using `startupEnvironmentMergingWorkspaceEnvironment` in each of the remaining three locations. Ensure every TerminalPanel creation path uses the same environment merging logic so that all shells/panes/splits consistently inherit the workspace environment.Source: Coding guidelines
♻️ Duplicate comments (1)
cmuxTests/WorkspaceEnvironmentTests.swift (1)
60-67: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd test coverage for
newTerminalSplitenvironment inheritance.The PR objectives state that workspace environment variables are injected at "two terminal-creation choke points":
newTerminalSurface(line 6884) andnewTerminalSplit(line 6699). This test file coversnewTerminalSurfaceinheritance at line 65 but has no corresponding test fornewTerminalSplit. Both functions callstartupEnvironmentMergingWorkspaceEnvironment()and pass the result toTerminalPanel, so the mechanism is shared, but the code path throughnewTerminalSplitremains untested.Add a test method that creates a workspace with a
workspaceEnvironment, callsnewTerminalSplit(), and verifies that the resulting terminal panel inherits the workspace environment variables through itsrespawnAdditionalEnvironmentproperty.🤖 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 `@cmuxTests/WorkspaceEnvironmentTests.swift` around lines 60 - 67, The test file currently covers the newTerminalSurface function for workspace environment inheritance but lacks coverage for newTerminalSplit, which also uses the same startupEnvironmentMergingWorkspaceEnvironment() mechanism. Add a new test method similar in structure to laterSurfaceInheritsWorkspaceEnvironment that creates a workspace with a workspaceEnvironment, invokes newTerminalSplit() on a pane, and asserts that the resulting terminal panel inherits the workspace environment variables through its respawnAdditionalEnvironment property.
🤖 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/Workspace.swift`:
- Around line 2472-2481: The workspaceEnvironment property is currently publicly
writable, which allows direct assignments to bypass the
sanitizedWorkspaceEnvironment(...) sanitization logic and potentially
reintroduce NUL and equals-sign vulnerabilities. Make the workspaceEnvironment
property private(set) to prevent direct external assignment, then create a
public method or computed property setter that routes mutations through the
sanitizedWorkspaceEnvironment(...) function before assigning the sanitized
value. Alternatively, add a didSet observer to the workspaceEnvironment property
that automatically sanitizes any newly assigned value by calling
sanitizedWorkspaceEnvironment(...).
- Around line 5730-5734: The `startupEnvironmentMergingWorkspaceEnvironment`
helper function is only used in `newTerminalSplit` and `newTerminalSurface`, but
terminals are also created directly in three other locations:
`createReplacementTerminalPanel`, `splitPaneWithNewTerminal`, and the terminal
creation branches in `splitTabBar`. To fix this inconsistency, either
consolidate all TerminalPanel creation through a single shared
constructor/helper that applies workspace environment merging, or explicitly
apply the workspace environment merge using
`startupEnvironmentMergingWorkspaceEnvironment` in each of the remaining three
locations. Ensure every TerminalPanel creation path uses the same environment
merging logic so that all shells/panes/splits consistently inherit the workspace
environment.
---
Duplicate comments:
In `@cmuxTests/WorkspaceEnvironmentTests.swift`:
- Around line 60-67: The test file currently covers the newTerminalSurface
function for workspace environment inheritance but lacks coverage for
newTerminalSplit, which also uses the same
startupEnvironmentMergingWorkspaceEnvironment() mechanism. Add a new test method
similar in structure to laterSurfaceInheritsWorkspaceEnvironment that creates a
workspace with a workspaceEnvironment, invokes newTerminalSplit() on a pane, and
asserts that the resulting terminal panel inherits the workspace environment
variables through its respawnAdditionalEnvironment property.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecda0fd6-ad31-42e7-8046-9967c4b83a8b
📒 Files selected for processing (3)
Sources/Workspace.swiftcmuxTests/WorkspaceEnvironmentTests.swiftcmuxTests/WorkspaceUnitTests.swift
…ostty bump - Workspace env now folds into every terminal-creation path, not just init/newTerminalSurface/newTerminalSplit: splitPaneWithNewTerminal (session- index drop), createReplacementTerminalPanel (last-panel replacement), and the drag-to-split placeholder/auto-create panels all merge startupEnvironmentMergingWorkspaceEnvironment([:]). Adds tests for the two publicly-callable paths. - `cmux workspace env` now defaults to the caller's workspace ($CMUX_WORKSPACE_ID) before the selected one, matching its help text and the reconnect/disconnect commands. - `cmux workspace env --json` no longer runs the user env map through formatIDs (which strips id/ref and *_id/*_ref keys); the envelope is formatted and the env map reinserted verbatim, so user variables named id/project_id survive. - Drop the unintended ghostty submodule bump: reset the pointer to origin/main (05c3e29). The bump (5697db8) was pulled in by an earlier main merge and main has since reverted it; the feature does not depend on ghostty. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/Workspace.swift (2)
2481-2481: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winKeep
workspaceEnvironmentbehind a sanitizing setter.This property is the raw source of truth for session persistence and
workspace.env, but it is freely writable. Any future direct assignment can bypasssanitizedWorkspaceEnvironment(...)and invalidate the “single choke point” guarantee described at Lines 5698-5699. Make itprivate(set)and funnel updates through a helper that re-sanitizes before storing.♻️ Proposed hardening
- `@Published` var workspaceEnvironment: [String: String] = [:] + `@Published` private(set) var workspaceEnvironment: [String: String] = [:] + + func setWorkspaceEnvironment(_ environment: [String: String]) { + workspaceEnvironment = Self.sanitizedWorkspaceEnvironment(environment) + }- self.workspaceEnvironment = sanitizedWorkspaceEnvironment + self.setWorkspaceEnvironment(sanitizedWorkspaceEnvironment)- workspaceEnvironment = Self.sanitizedWorkspaceEnvironment(snapshot.environment ?? [:]) + setWorkspaceEnvironment(snapshot.environment ?? [:])🤖 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/Workspace.swift` at line 2481, The workspaceEnvironment property is publicly writable and can be directly modified, which bypasses the sanitizedWorkspaceEnvironment method that serves as the single choke point for validation. Change the workspaceEnvironment property to use private(set) to prevent direct external assignment. Then create a private helper method that accepts new environment values, passes them through sanitizedWorkspaceEnvironment(...) for sanitization, and only then assigns the result to workspaceEnvironment. Update all internal assignments to workspaceEnvironment throughout the class to use this new helper method instead of direct assignment, ensuring all modifications go through the sanitizing logic.
5701-5707:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t silently drop empty-string environment values.
KEY=is a valid environment assignment, and shells distinguish “unset” from “set to empty”. The!pair.value.isEmptyguard erases that distinction by removing the entry entirely, so a workspace cannot intentionally clear an inherited variable. If the two startup channels disagree on empty-value handling, align those channels instead of normalizing user input away here.🤖 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/Workspace.swift` around lines 5701 - 5707, The environment variable processing in the reduce(into:) block includes a guard condition `!pair.value.isEmpty` that silently removes environment variables with empty string values. This is incorrect because shells distinguish between "unset" (absent) and "set to empty" (KEY=), and users should be able to intentionally clear inherited variables. Remove the `!pair.value.isEmpty` guard condition from the reduce block so that environment entries with empty values are preserved in the resulting dictionary.
🤖 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/Workspace.swift`:
- Line 2481: The workspaceEnvironment property is publicly writable and can be
directly modified, which bypasses the sanitizedWorkspaceEnvironment method that
serves as the single choke point for validation. Change the workspaceEnvironment
property to use private(set) to prevent direct external assignment. Then create
a private helper method that accepts new environment values, passes them through
sanitizedWorkspaceEnvironment(...) for sanitization, and only then assigns the
result to workspaceEnvironment. Update all internal assignments to
workspaceEnvironment throughout the class to use this new helper method instead
of direct assignment, ensuring all modifications go through the sanitizing
logic.
- Around line 5701-5707: The environment variable processing in the
reduce(into:) block includes a guard condition `!pair.value.isEmpty` that
silently removes environment variables with empty string values. This is
incorrect because shells distinguish between "unset" (absent) and "set to empty"
(KEY=), and users should be able to intentionally clear inherited variables.
Remove the `!pair.value.isEmpty` guard condition from the reduce block so that
environment entries with empty values are preserved in the resulting dictionary.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90dc0713-3056-49a9-ba79-0a1bcd04f5d5
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
CLI/cmux.swiftSources/Workspace.swiftcmuxTests/WorkspaceEnvironmentTests.swift
…env leak - WorkspaceEnvironmentTests imports CmuxTerminal so TerminalSurface resolves in the wired cmuxTests target (P1, was a compile failure). - Fix workspace env becoming sticky across surface moves (P2). The same TerminalPanel travels when a surface is moved to another workspace, so the workspace env baked into its respawn state would re-seed the source workspace's variables on respawn. TerminalPanel now records the env keys it inherited from the workspace (set only at creation via configureNewTerminalPanel, so it survives the move); respawnTerminalSurface strips those keys from the replayed env and re-folds the current workspace's env. Adds coverage for the seeded-keys tracking. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…pace.env target - Respawn previously stripped any key in the seeded workspace key set, which discarded an explicit per-surface override sharing a workspace key (e.g. a layout env AWS_PROFILE=staging in a workspace with AWS_PROFILE=prod would respawn as prod). TerminalPanel now records the seeded workspace key/value pairs and respawn only drops entries whose value still equals the seeded workspace value, preserving per-surface overrides (autoreview P2). - `workspace.env` validated only workspace_id, so a malformed surface_id/ terminal_id/tab_id or a stale pane_id fell through to the selected workspace and could print the wrong workspace's secrets. It now validates every target param and resolves explicit targets strictly, only falling back to the selected workspace when no explicit target is supplied (autoreview P2). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Workspace is @mainactor, so its static sanitizedWorkspaceEnvironment was main-actor-isolated. The nonisolated socket workspace-create parsing path (v2WorkspaceCreate) calls it synchronously, so mark the pure helper `nonisolated` to keep it safe under stricter Swift concurrency checking (autoreview P1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Resolve the only conflict (.github/swift-file-length-budget.tsv) by regenerating from actual post-merge file lengths via swift_file_length_budget.py --write-budget, not by hand-picking a side: the merged CLI/cmux.swift (34074 lines) is longer than either parent (branch 33671, main 33857), so any single-side pick would have failed CI. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
+7477
to
+7489
| private func runWorkspaceEnvCommand( | ||
| commandArgs: [String], | ||
| client: SocketClient, | ||
| jsonOutput: Bool, | ||
| idFormat: CLIIDFormat, | ||
| windowOverride: String? | ||
| ) throws { | ||
| var rest = commandArgs | ||
| let mask = rest.contains("--mask") | ||
| rest.removeAll { $0 == "--mask" } | ||
|
|
||
| let (workspaceArg, rem0) = parseOption(rest, name: "--workspace") | ||
| let (_, rem1) = parseOption(rem0, name: "--window") |
Contributor
There was a problem hiding this comment.
--env KEY= silently accepted and dropped
parseEnvAssignment validates that the key is non-empty but does not validate the value, so --env AWS_PROFILE= is accepted without error. The empty string is forwarded as workspace_env to the server, where Workspace.sanitizedWorkspaceEnvironment silently discards it (the guard requires !pair.value.isEmpty). The user sees a successful workspace create response, but the variable is never set. The same path applies to env-file lines like AWS_PROFILE=. The fix is to add an empty-value guard in parseEnvAssignment, consistent with the empty-key guard that already exists.
Resolve conflicts: - Resources/Localizable.xcstrings: union of both sides' string keys (keep this branch's cli.workspace.env.empty plus main's settings.account.signIn.slowHint / openInBrowser), valid JSON verified. - .github/swift-file-length-budget.tsv: regenerated from actual post-merge Swift file lengths (scripts/swift_file_length_budget.py --write-budget); check mode passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Merged
This was referenced Jun 17, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Give a cmux workspace a set of user-defined environment variables that every shell spawned in it inherits — the initial terminal plus every later pane/surface/split, and every surface recreated on session restore — without editing global
~/.zshrc, sprinklingexports into each pane, or relying on a per-tool.env.Closes #5995
What you can do
How it works
Workspacecarries a persistentworkspaceEnvironmentdictionary, folded into the startup environment at the two terminal-creation choke points:init(initial shell) andnewTerminalSurface/newTerminalSplit(every later surface and session-restore, which routes throughnewTerminalSurface). No per-entrypoint duplication.CMUX_*is protected. Workspace env flows through the existingadditionalEnvironment/initialEnvironmentOverrideschannels, both of which skipprotectedStartupEnvironmentKeysinmergedStartupEnvironment(...). So the managedCMUX_WORKSPACE_ID/CMUX_SOCKET_PATH/TERM… always win and can never be clobbered (re 0.64.10: CMUX_* env vars not propagated to spawned shells; CLI from those shells silently fails #4858/daemon leaks focused workspace IDs in spawn env (CMUX_WORKSPACE_ID, CMUX_SURFACE_ID set to focused pane, not target spawn) #4920).surfaces[].env, scrollback replay, SSH startup) overlays it.SessionWorkspaceSnapshot.environment(optional → old manifests decode cleanly) and restored before surfaces are rebuilt, so restored shells inherit it.workspace.envcontrol method. The env set is deliberately kept out ofworkspace listso a plain listing never leaks secrets;--maskredacts values.Entry points → one path
CLI
--env/--env-file→workspace_envsocket param, andcmux.jsonenv→ both converge onTabManager.addWorkspace(workspaceEnvironment:)→Workspace.Tests
cmuxTests/WorkspaceEnvironmentTests.swift(wired into the pbxproj) covers the acceptance paths:--env;CMUX_*vars;SessionWorkspaceSnapshotCodable back-compat (absent key → nil, nil → omitted);CmuxWorkspaceDefinitiondecodesenv.Plus a
ControlCommandExecutionPolicyassertion thatworkspace.envruns on the socket-worker lane.Docs & localization
docs/cli-contract.md: the flags, theworkspace envcommand, and a Workspace environment variables section documenting inheritance, persistence, precedence vs login-shell init files (~/.zprofile/~/.zshrc), the protectedCMUX_*vars, and the on-disk-plaintext secret caveat.Resources/Localizable.xcstrings: en/ja/ko/uk updated for the workspace usage/error strings and the new empty-output string.Notes / scope
--envonnew-pane/new-surface/new-split" is a follow-up that composes with feat: --command flag for new-split / new-surface / new-pane #2538: those surfaces already inherit the workspace env automatically, and the control socket already accepts a per-surfacestartup_environment/initial_env.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds per-workspace environment variables that every shell in a workspace inherits — the first terminal plus later panes/splits, split-pane creations, replacements, drag-to-split, and restored surfaces. Configure via CLI or
cmux.json; inspect withcmux workspace env(supports--mask/--json). Closes #5995.New Features
Workspacestores anenvmap applied to all shell spawns and session restore; per-surface env overrides; managedCMUX_*and terminal identity vars are protected.=, and values with NUL.new-workspace/workspace createaccept repeatable--env KEY=VALUEand--env-file <path>(files ignore blanks/comments, stripexport, unquote;--envoverrides file values).cmux workspace env [<handle>] [--mask] [--json]defaults to the caller’s workspace, resolves handles, and is kept out ofworkspace list.environmenton the session snapshot and restored early;cmux.jsonsupports anenvobject;workspace.envcontrol method runs on the worker lane.workspace.createrequires aworkspace_envmap; bareenvis not treated as workspace env.Bug Fixes
workspace.envstrictly validates explicit targets and only falls back when none are provided; defaults to the caller’s workspace first.--jsonpreserves user keys/values verbatim (e.g.,id,*_id).nonisolatedso the socketworkspace.createpath can call it without a main-actor hop.Written for commit ef24d8c. Summary will update on new commits.
Summary by CodeRabbit
Release Notes
New Features
workspace envto display a workspace’s environment (--maskto redact;--jsonfor JSON, sortedKEY=VALUEotherwise).new-workspacewith repeatable--env KEY=VALUE/--env=...and--env-file <path>/--env-file=...(supports comments/blank lines and optional leadingexport); invalid entries are rejected.CMUX_*keys are preserved.Documentation
cmux workspaceandnew-workspacehelp/contract docs, including examples and secret-handling.Tests