Repository navigation
Fix cmux terminal environment injection - #4728
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors TerminalSurface startup environment assembly to build and apply a per-surface CmuxContextEnvironment (workspaceId, surfaceId, socketPath) into a protected environment, consolidates surface creation to use the helper, and adds a test validating CMUX_* environment variables are present. ChangesCMUX Context Environment Injection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c87cc6f to
b0106ea
Compare
Greptile SummaryThis PR fixes a regression where
Confidence Score: 5/5Safe to merge — the change correctly aligns the pre-spawn lookup with the surface-creation path, actor isolation is properly declared, and the regression test covers all five affected keys. The refactor is narrow: it replaces five individual managed-env assignments with a shared static helper, adds @mainactor to the lookup method, and contributes a regression test. No new mutable shared state is introduced, the CmuxContextEnvironment struct is a plain Sendable value type, and the merge logic in mergedStartupEnvironment was already well-tested. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant TS as TerminalSurface
participant TSE as TerminalStartupEnvironment
participant TC as TerminalController
Note over TS,TSE: Pre-spawn lookup (startupEnvironmentValue)
Caller->>TS: "startupEnvironmentValue(key) [@MainActor]"
TS->>TC: activeSocketPath(preferredPath:)
TC-->>TS: socketPath
TS->>TSE: "applyManagedCmuxContextEnvironment(context, &env, &protectedKeys)"
TSE-->>TS: env populated with 5 CMUX keys
TS->>TSE: mergedStartupEnvironment(base:protectedKeys:additionalEnv:overrides:)
TSE-->>TS: merged[String:String]
TS-->>Caller: merged[key]
Note over TS,TSE: Surface creation (createSurface)
Caller->>TS: createSurface()
TS->>TC: activeSocketPath(preferredPath:)
TC-->>TS: socketPath
TS->>TSE: "applyManagedCmuxContextEnvironment(context, &env, &protectedKeys)"
TSE-->>TS: env populated with 5 CMUX keys (same path)
Reviews (3): Last reviewed commit: "fix: centralize cmux terminal context en..." | Re-trigger Greptile |
| private func cmuxContextEnvironment(socketPath: String) -> CmuxContextEnvironment { | ||
| CmuxContextEnvironment( | ||
| workspaceId: tabId, | ||
| surfaceId: id, | ||
| socketPath: socketPath | ||
| ) | ||
| } |
There was a problem hiding this comment.
Missing
@MainActor on cmuxContextEnvironment accessing mutable tabId
cmuxContextEnvironment reads tabId, which is a private(set) var on a class that is not globally @MainActor-isolated. Without the annotation, the method is nonisolated by default in Swift 6 strict concurrency, and any future caller outside @MainActor would compile cleanly while producing a data race on the mutable tabId. Adding @MainActor here makes the isolation contract explicit and matches every call site.
Rule Used: Flag new or materially worsened Swift 6 actor isol... (source)
There was a problem hiding this comment.
Fixed by making the cmux context helper a pure static constructor that receives workspace ID, surface ID, and socket path from the caller, so it no longer reads mutable surface state outside the caller's actor context.
— Claude Code
| @MainActor | ||
| func startupEnvironmentValue(_ key: String) -> String? { | ||
| additionalEnvironment[key] ?? initialEnvironmentOverrides[key] | ||
| let socketPath = TerminalController.shared.activeSocketPath( | ||
| preferredPath: SocketControlSettings.socketPath() | ||
| ) | ||
| var environment: [String: String] = [:] | ||
| var protectedKeys: Set<String> = [] | ||
| Self.applyManagedCmuxContextEnvironment( | ||
| cmuxContextEnvironment(socketPath: socketPath), | ||
| to: &environment, | ||
| protectedKeys: &protectedKeys | ||
| ) | ||
| return Self.mergedStartupEnvironment( | ||
| base: environment, | ||
| protectedKeys: protectedKeys, | ||
| additionalEnvironment: additionalEnvironment, | ||
| initialEnvironmentOverrides: initialEnvironmentOverrides | ||
| )[key] | ||
| } |
There was a problem hiding this comment.
startupEnvironmentValue is a partial and undocumented view of the startup environment
The method is named as a generic key lookup over the startup environment, but it only materialises five CMUX context keys plus the additionalEnvironment/initialEnvironmentOverrides dicts. Managed keys applied in createSurface — TERM, COLORTERM, CMUX_BUNDLED_CLI_PATH, CMUX_PORT, etc. — return nil here. A future call like startupEnvironmentValue("CMUX_BUNDLED_CLI_PATH") will silently return nil even though the running terminal has it set. The fix centralises the CMUX context path correctly, but without a comment or a narrower name (e.g. cmuxContextEnvironmentValue) the method's scope is still easy to misread.
Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed by documenting startupEnvironmentValue as a pre-spawn lookup for managed context keys and explicit startup overrides, with full runtime-only environment assembly still owned by createSurface.
— Claude Code
b0106ea to
25eaca4
Compare
Summary
Fixes #4714.
Verification
Commit structure
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Low Risk
Scoped to terminal startup environment assembly and protected keys; no auth or data-path changes, with regression test coverage.
Overview
CMUX terminal context env vars (
CMUX_WORKSPACE_ID,CMUX_SURFACE_ID,CMUX_PANEL_ID,CMUX_TAB_ID,CMUX_SOCKET_PATH) are now built in one place viaapplyManagedCmuxContextEnvironmentand used both when a Ghostty surface is created and in pre-spawnstartupEnvironmentValuelookups (which previously only merged overrides and could miss managed context keys).A small
CmuxContextEnvironmenttype carries workspace/surface IDs and the active socket path. A new regression test asserts all five keys are present and correct onTerminalSurface.startupEnvironmentValue.Reviewed by Cursor Bugbot for commit 25eaca4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Refactor
Tests