Repository navigation
fix: route new-workspace --command through workspace.create initial_command - #2433
jasonkuhrt wants to merge 4 commits into
Conversation
|
@jasonkuhrt is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as rgba(60,130,200,0.5) CLI
participant RPC as rgba(120,200,80,0.5) JSON-RPC Server
participant WS as rgba(200,120,80,0.5) Workspace Service
participant TERM as rgba(160,60,200,0.5) Terminal
CLI->>RPC: sendV2 "workspace.create" {cwd, initial_command}
RPC->>WS: create workspace with params
WS->>TERM: start terminal with initial_command (exec directly)
WS-->>RPC: workspace created {wsId, ref}
RPC-->>CLI: response {wsId, ref}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_cli_new_workspace_initial_command.py (1)
138-143: Use subset validation forworkspace.createparams to avoid brittle failures.Line 138 currently enforces exact dict equality, which can break if unrelated optional params are added later. Assert required keys (
cwd,initial_command) instead.♻️ Proposed change
- expected_params = { - "cwd": str(cwd), - "initial_command": command_text, - } - if state.workspace_create_params != expected_params: + expected_params = { + "cwd": str(cwd), + "initial_command": command_text, + } + observed_params = state.workspace_create_params + missing_or_mismatched = { + k: (expected_params[k], observed_params.get(k)) + for k in expected_params + if observed_params.get(k) != expected_params[k] + } + if missing_or_mismatched: print( "FAIL: workspace.create params mismatch " - f"expected={expected_params!r} observed={state.workspace_create_params!r}" + f"expected_subset={expected_params!r} observed={observed_params!r} " + f"diff={missing_or_mismatched!r}" ) return 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli_new_workspace_initial_command.py` around lines 138 - 143, The test currently compares state.workspace_create_params to expected_params with exact equality which is brittle; update the assertion to validate only the required keys by checking that 'cwd' and 'initial_command' exist in state.workspace_create_params and that their values equal expected_params['cwd'] and expected_params['initial_command'] (use state.workspace_create_params and expected_params to locate the data), and update the failure print to show which required key(s) mismatched rather than full-dict equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_cli_new_workspace_initial_command.py`:
- Around line 138-143: The test currently compares state.workspace_create_params
to expected_params with exact equality which is brittle; update the assertion to
validate only the required keys by checking that 'cwd' and 'initial_command'
exist in state.workspace_create_params and that their values equal
expected_params['cwd'] and expected_params['initial_command'] (use
state.workspace_create_params and expected_params to locate the data), and
update the failure print to show which required key(s) mismatched rather than
full-dict equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 43c6aa47-9ac2-44be-8496-0bbdc0577fd4
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_cli_new_workspace_initial_command.py
Greptile SummaryThis PR fixes Key changes:
Confidence Score: 5/5Safe to merge — core logic change is correct and the new protocol path is already exercised by the daemon; only minor test style issues remain. The Swift change is minimal and correct: it removes a fragile two-step send_text approach and replaces it with the already-supported initial_command mechanism. The server-side handling in TerminalController.swift is unchanged and already handles this field properly. The only findings are P2 style concerns in the new test (missing lock, exact dict equality). Neither blocks merge. tests/test_cli_new_workspace_initial_command.py — minor test robustness improvements suggested but not blocking. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant CLI as cmux CLI
participant Daemon as cmux Daemon
Note over User,Daemon: Before this PR (surface.send_text path)
User->>CLI: cmux new-workspace --command "npm test"
CLI->>Daemon: workspace.create {cwd: ...}
Daemon-->>CLI: {workspace_ref: "workspace:1", workspace_id: "..."}
CLI->>Daemon: surface.send_text {text: "npm test\r", workspace_id: "..."}
Note over Daemon: Depends on shell input state (breaks in vi-mode / fish)
Daemon-->>CLI: {ok: true}
CLI-->>User: OK workspace:1
Note over User,Daemon: After this PR (initial_command path)
User->>CLI: cmux new-workspace --command "npm test"
CLI->>Daemon: workspace.create {cwd: ..., initial_command: "npm test"}
Note over Daemon: Command injected at terminal startup, shell-state independent
Daemon-->>CLI: {workspace_ref: "workspace:1", workspace_id: "..."}
CLI-->>User: OK workspace:1
Reviews (1): Last reviewed commit: "fix: route new-workspace command through..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Closing. #9614 (v0.64.23) took a different route: |
2 participants
Summary
cmux new-workspace --commandcurrently creates the workspace first and then injects the command withsurface.send_text.This PR routes
--commandthroughworkspace.create.initial_commandinstead.Why
workspace.createalready supportsinitial_command, and cmux already threads that through the terminal startup path.Using
surface.send_texthere is the wrong primitive:fish_vi_key_bindingsScope
This PR only changes
cmux new-workspace --command.It does not change:
new-surfacenew-splitImplementation
initial_commandin theworkspace.createparams fornew-workspacesurface.send_textcall for this path--commandis described as an initial terminal commandworkspace.createreceivesinitial_commandandsurface.send_textis not usedThe existing runtime coverage in
tests_v2/test_cli_new_workspace_command_queue.pystill exercises the user-visible behavior that the command runs without selecting the new workspace.Fixes #1900
Summary by cubic
Routes
cmux new-workspace --commandthroughworkspace.create.initial_commandto run the initial command reliably across shells, including vi-mode. Fixes #1900 and ignores empty/whitespace commands.initial_commandinworkspace.createonly when--commandis non-empty; stop callingsurface.send_text.--commandas the initial terminal command.workspace.createincludesinitial_commandandsurface.send_textis not used.Written for commit 1a07088. Summary will update on new commits.
Summary by CodeRabbit