Skip to content

fix(control): restore #13877 group set_color/set_icon changes reverted by #14669 - #14677

Closed
teamleaderleo wants to merge 1 commit into
mainfrom
fix/restore-13877-group-color-icon
Closed

teamleaderleo wants to merge 1 commit into
mainfrom
fix/restore-13877-group-color-icon

Conversation

@teamleaderleo

@teamleaderleo teamleaderleo commented Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

#14669 (ci: restore the git object seed before checkout in E2E and iOS macOS jobs, 2509187) was squashed from a stale base. Besides its workflow changes, it put three files back to their pre-#13877 state:

  • CLI/cmux.swift (drops the --color / --icon aliases)
  • Packages/macOS/CmuxControlSocket/.../ControlCommandCoordinator+WorkspaceGroup.swift (drops the color / icon param aliases and hex validation)
  • Packages/macOS/CmuxControlSocket/Tests/.../ControlCommandCoordinatorWorkspaceGroupColorIconTests.swift (deleted)

git diff 4f0ac55da15^ 2509187c6d9 -- <those paths> is empty, so #14669 was an exact revert of #13877 for these files. No commit since touches them.

This PR restores the three files from 4f0ac55 (#13877) with git checkout 4f0ac55da15 -- <paths>. No other changes. #14669's CI changes stay.

Testing

Relies on PR CI (CmuxControlSocket package tests include the restored ControlCommandCoordinatorWorkspaceGroupColorIconTests). Not built locally.

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Restores the workspace group set_color/set_icon changes from #13877 that #14669 accidentally reverted when it was squashed from a stale base. No other changes; #14669's CI workflow changes are kept.

Restored behavior

  • Re-adds the --color/--icon CLI aliases for the hex/symbol options.
  • Re-adds color/icon as RPC parameter aliases, hex color validation, and normalization to the canonical #RRGGBB spelling, with invalid_params errors for malformed values.
  • Re-adds the regression test suite covering aliases, clearing, and rejection of invalid inputs.

Written for commit e89e344. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • New Features
    • Added --color and --icon as aliases for the existing color and icon options in the command-line interface.
    • Added color and icon as aliases for the corresponding workspace-group settings.
  • Bug Fixes
    • Color values are validated and normalized to uppercase #RRGGBB. Invalid values and parameter types now return an error without changing settings.
    • Empty values or explicit null clear color and icon overrides.

…d by #14669

#14669 (ci: restore the git object seed before checkout) was squashed from a
stale base, so besides its workflow changes it put CLI/cmux.swift,
ControlCommandCoordinator+WorkspaceGroup.swift and the color/icon tests back
to their pre-#13877 state, deleting the tests. This restores those three files
from 4f0ac55 (#13877). Nothing merged since touches them.

Co-authored-by: LuckVd <37114923+LuckVd@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 25, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@github-actions

Copy link
Copy Markdown
Contributor

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: b6cc3a9e-9761-4bd3-9113-587a723dce24

📥 Commits

Reviewing files that changed from the base of the PR and between ce2a516 and e89e344.

📒 Files selected for processing (3)
  • CLI/cmux.swift
  • Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swift
  • Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceGroupColorIconTests.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The CLI and control socket now accept aliases for workspace-group color and icon setters. The control socket validates color values, normalizes accepted colors, and handles empty or null values as clears. Regression tests cover aliases, validation, and clearing.

Changes

Workspace group setters

Layer / File(s) Summary
Setter parameter handling and validation
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swift, Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceGroupColorIconTests.swift
The color and icon setters resolve canonical keys and aliases. Color inputs must contain six ASCII hex digits, with an optional leading #, and are stored as uppercase #RRGGBB. Empty or null values clear the setting. Tests cover valid inputs, invalid parameters, and clearing.
CLI option aliases
CLI/cmux.swift
set-color accepts --color as an alias for --hex, and set-icon accepts --icon as an alias for --symbol. The original option takes precedence when both are supplied.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to e89e3

No identified issue prevents merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e89e3

The new option names use the existing group setters, and color input is more tightly validated. No new privilege or broader group access was identified. The socket’s access controls and the behavior of concurrent updates were not fully established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The added spellings let callers of the existing commands request color or icon changes, but do not identify a new target, mutation owner, or downstream authority. Effective access still depends on the existing socket boundary, whose permission policy was not established here.

Trust Boundaries and Controls

  • observed — The coordinator validates request values before delegation, while the application context checks that the requested group is present in the resolved TabManager. These checks do not, by themselves, establish who may connect to or invoke the socket.

Resilience and Maintainability Implications

  • inferred — Repeated values are no-ops in the existing group owner, but concurrent ordering and interruption or recovery behavior were not established. The PR reuses that owner rather than adding a separate transition path.

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (3 errors, 1 inconclusive)

Check name Status Explanation Resolution
Cmux Swift Actor Isolation ❌ Error The production diff adds aliasStringParam and normalizeHexColor as implicitly @MainActor static helpers inside @MainActor ControlCommandCoordinator. Both helpers are pure value-only parsing ut… Mark the pure helper methods nonisolated static. Keep AliasStringParam as a value-only type, or move it to file scope and make it explicitly Sendable if it must cross isolation boundaries. Confirm that callers from worker or backgroun…
Cmux User-Facing Error Privacy ❌ Error The production change copies untrusted request values into user-facing API error bodies. workspaceGroupSetColor returns raw invalid strings and arbitrary JSONValue values in error.data (lines 39… Do not echo raw parameter values in user-facing error data. Return only sanitized metadata, such as the rejected field name and expected type or format. For unknown keys, report the key name only if it is safe and omit its value. For invali…
Cmux Full Internationalization ❌ Error The production diff adds four user-facing control-socket error messages as English Swift string literals in ControlCommandCoordinator+WorkspaceGroup.swift (lines 370, 391, 398, and 424/438). The mes… Route the four new error messages through the existing ControlWorkspaceGroupStrings localization seam. Add stable localization keys and English default values in Sources/TerminalController+ControlWorkspaceGroupContext.swift, add the cor…
Docstring Coverage ❓ Inconclusive Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (21 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the primary change: restoring the workspace-group set_color and set_icon changes reverted by another pull request.
Description check ✅ Passed The description provides a specific problem statement, affected files, restored behavior, testing scope, and the limitation that the change was not built locally. It omits the template's Demo Video an…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Cmux Cloud Persistent Session And Early Input ✅ Passed The pull request does not change Cloud terminal creation or transport behavior. The authoritative diff changes workspace-group color/icon CLI aliases, RPC validation, and regression tests only. No cha…
Cmux Swift Blocking Runtime ✅ Passed PASS: The production diff only adds CLI aliases, parameter resolution, and color validation. Added production lines contain no semaphores, blocking waits, sleeps, delayed dispatch, polling, main-queue…
Cmux Browser Automation Off-Main ✅ Passed PASS. The PR changes only workspace-group color/icon CLI and coordinator code, plus regression tests. The changed lines contain no browser.* command routing, WebKit/AppKit access, waits, mainActor/pro…
Cmux Expensive Synchronous Load ✅ Passed The production diff only adds CLI option aliases and synchronous validation/normalization of String and JSONValue parameters in workspace-group socket handlers. It adds no `RestorableAgentSessionI…
Cmux Cache Substitution Correctness ✅ Passed PASS. The authoritative PR diff changes CLI option parsing and workspace-group color/icon parameter validation, plus adds regression tests. It does not replace any fresh authoritative read with a cach…
Cmux No Hacky Sleeps ✅ Passed PASS. The PR changes only three Swift files: two production files and one test file. The changed lines add option aliases, parameter validation, normalization, and regression tests. No fixed sleeps, t…
Cmux Algorithmic Complexity ✅ Passed PASS. The production diff adds only bounded work. aliasStringParam scans a fixed two-key array, and normalizeHexColor checks at most six characters after the length guard. The CLI parses two fixed…
Cmux Swift Concurrency ✅ Passed The pull request does not introduce or materially expand any flagged Swift concurrency pattern. The added production code contains synchronous workspaceGroupSetColor, workspaceGroupSetIcon, alias …
Cmux Swift @Concurrent ✅ Passed The Swift diff adds or changes only synchronous code. workspaceGroupSetColor, workspaceGroupSetIcon, aliasStringParam, and normalizeHexColor are synchronous, and the CLI changes are synchronou…
Cmux Swift Package Boundaries ✅ Passed The changed workspace-group RPC logic is already behind the CmuxControlSocket SwiftPM boundary. Packages/macOS/CmuxControlSocket/Package.swift defines the CmuxControlSocket library target and it…
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only CLI/source code and adds tests. It does not change a Package.swift dependency, any .gitignore, an Xcode project package reference, or a workflow. Therefore, no SwiftPM or Xcode Pac…
Cmux Swift Logging ✅ Passed PASS. The diff adds no print, debugPrint, dump, NSLog, ad hoc logging, Logger, or stdout/stderr diagnostics. The existing CLI response print is unchanged and is allowed user-facing CLI outpu…
Cmux Swiftui State Layout ✅ Passed PASS — the pull request does not introduce SwiftUI changes. The authoritative diff changes only CLI/cmux.swift, the control-socket workspace-group coordinator, and its tests. The added code contains…
Cmux Architecture Rethink ✅ Passed PASS. The diff adds local parameter alias resolution and hex normalization in the existing ControlCommandCoordinator path. It does not add sleeps, delayed dispatch, polling, locks, observers, caches…
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed PASS: The authoritative PR diff changes only CLI workspace-group option parsing, workspace-group color/icon control logic, and regression tests. It adds or materially changes no NSWindow, NSPanel, NSW…
Cmux Source Artifacts ✅ Passed The diff changes only three intentional product/test paths: CLI/cmux.swift, a CmuxControlSocket source file, and a new CmuxControlSocket regression test. Their contents are hand-written Swift so…
Cmux No Test Or Debug Seam In Production Source ✅ Passed PASS. The only changed production Sources file adds RPC alias and color-normalization logic used directly by workspaceGroupSetColor and workspaceGroupSetIcon. The diff adds no #if DEBUG or tes…
Full details: Docstring Coverage

Explanation

Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 2 files. (1 skipped: 1 too large.)

Full details: Cmux Swift Actor Isolation

Explanation

The production diff adds aliasStringParam and normalizeHexColor as implicitly @MainActor static helpers inside @MainActor ControlCommandCoordinator. Both helpers are pure value-only parsing utilities. They do not access coordinator state. Existing equivalent helpers in ControlCommandCoordinator+Params.swift are explicitly nonisolated. This introduces unnecessary MainActor coupling covered by the actor-isolation rule. The CLI changes and added tests do not add an actor-isolation issue.

Resolution

Mark the pure helper methods nonisolated static. Keep AliasStringParam as a value-only type, or move it to file scope and make it explicitly Sendable if it must cross isolation boundaries. Confirm that callers from worker or background contexts compile without a MainActor hop.

Full details: Cmux User-Facing Error Privacy

Explanation

The production change copies untrusted request values into user-facing API error bodies. workspaceGroupSetColor returns raw invalid strings and arbitrary JSONValue values in error.data (lines 392 and 399), and workspaceGroupSetIcon does the same for arbitrary values (line 439). Unknown response keys are also echoed (lines 371 and 425). ControlResponseEncoder serializes this data directly into the wire error response. A malformed value can therefore expose credentials, tokens, personal data, or an unredacted nested payload through the cmux control API. The changed tests confirm that nested JSON values reach the type-mismatch path.

Resolution

Do not echo raw parameter values in user-facing error data. Return only sanitized metadata, such as the rejected field name and expected type or format. For unknown keys, report the key name only if it is safe and omit its value. For invalid color strings, omit the raw string and return a generic format error.

Full details: Cmux Full Internationalization

Explanation

The production diff adds four user-facing control-socket error messages as English Swift string literals in ControlCommandCoordinator+WorkspaceGroup.swift (lines 370, 391, 398, and 424/438). The messages are returned in invalid_params API responses, so they are user-facing text. They do not use a localized API, and the repository search found no matching localization keys. The PR changes no catalog files. Existing workspace-group localization uses ControlWorkspaceGroupStrings and String(localized:defaultValue:) in Sources/TerminalController+ControlWorkspaceGroupContext.swift; the new messages bypass that path. Tests and comments are exempt, but these production response messages are not.

Resolution

Route the four new error messages through the existing ControlWorkspaceGroupStrings localization seam. Add stable localization keys and English default values in Sources/TerminalController+ControlWorkspaceGroupContext.swift, add the corresponding properties and coordinator usage, and add translated entries to Resources/Localizable.xcstrings for every existing locale: ar, bs, da, de, en, es, fr, it, ja, km, ko, nb, pl, pt-BR, ru, th, tr, uk, zh-Hans, and zh-Hant.

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

Warning

Some tools did not complete. Review the errors below.

🔧 OpenGrep (1.30.0)
CLI/cmux.swift

OpenGrep scan timed out


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

Automatic catch-up couldn't merge main (8c98e64d1586): Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/WorkspaceGroup/ControlCommandCoordinator+WorkspaceGroup.swift (both sides changed the same lines), Packages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceGroupColorIconTests.swift (added on both sides; needs a person). Nothing was pushed; merge it by hand. A new push or /catch-up tries again.

Label no-auto-catch-up to opt out · Catch-up run

@teamleaderleo

Copy link
Copy Markdown
Collaborator Author

Closing as superseded by #15000, which restored the same #13877 set_color/set_icon handling and tests on main (and has since localized the error strings). Merging this branch would only conflict with that.

— Icicle g1 ⚙️ (run_worker_20260927_686a3a99)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant