Repository navigation
remote-tmux: let a failed socket request say what failed - #17134
Conversation
…line Remote tmux requests share a socket wrapper with Cloud VM requests. Its fallback answers any error it does not recognise with "The Cloud VM request failed", so every remote tmux failure reached the CLI as that.
The wrapper remote tmux requests share with Cloud VM requests answered any error it did not recognise with the Cloud VM line. RemoteTmuxError carries app-owned wording with remote text already flattened and capped, so it is returned as it is.
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (2)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughError formatting now returns ChangesRemote tmux error text
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~8 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Remote tmux failures retain their intended error messages, with no identified issue blocking the change from merging. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 3 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 unsupported.) Full details: Cmux User-Facing Error PrivacyExplanation The diff exposes raw remote command errors to cmux end users. Resolution Change the production socket formatter so it does not return remote stderr or other unreviewed upstream text in user-facing errors. Use app-authored, safe error copy for
✨ 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 |
CI failure attributionCI failed on
Matched log linesNot re-run automatically: Written by |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot is paused — on-demand spend limit reachedBugbot 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. |
|
|
|
Merged, thank you @ejc3! :D |
|
Merge receipt for
Labeled |
main no longer compiles after this merge@teamleaderleo @ejc3: after Evidence: https://github.com/manaflow-ai/cmux/actions/runs/37156042656/job/111299626496 Nothing blocks merging meanwhile. A fix-forward (or, failing that, a revert) is attempted automatically unless an open pull request already fixes this. main_compile_attribution.py: post-merge, nothing here gates a merge. |
00f182f Set Claude idle after reentrant stop without work (manaflow-ai#16635) d8ef7e7 Reject unknown and valueless options in cmux hooks setup (manaflow-ai#17183) 46b5f9c remote-tmux: let a failed socket request say what failed (manaflow-ai#17134) e88d636 remote-tmux: re-read a pane from tmux when its width changes, not only when it grows (manaflow-ai#17140) 984baea remote-tmux: close a window emptied by gathering its mirrors into a new one (manaflow-ai#17141) 9c41d59 fix: recover hidden terminal renderer after window attach (manaflow-ai#16548) 70854a5 ci: publish dogfood artifacts from red CI runs (manaflow-ai#17210) db77e58 fix(cli): reject missing notify text values (manaflow-ai#16778) 76ccbfc ci: activate org-member dogfood artifact publisher (manaflow-ai#17206)
Summary
Every remote tmux request that fails reports the same thing from the CLI, whatever went wrong:
The host is an ssh host and no Cloud VM is involved. The real error in that run was "host unreachable: app not ready".
Remote tmux socket methods run through
v2VmCall, the wrapper Cloud VM requests use. Its last fallback,cloudVMSafeErrorMessage, returns the Cloud VM line for any error it does not recognise, andRemoteTmuxErroris not one it recognises.RemoteTmuxError.messageis app-owned wording, and it already flattens and caps any remote text it includes, so the fallback now returns it as it is. The comment onRemoteTmuxError'sCustomStringConvertibleconformance says the socket path was meant to surface that message.Testing
RemoteTmuxSocketErrorTextTeststhrows threeRemoteTmuxErrorcases throughv2VmCalland reads the message out of the encoded reply.Run with
xcodebuild test-without-building -scheme cmux-unit -only-testing:cmuxTests/RemoteTmuxSocketErrorTextTests:RemoteTmuxMirrorCloseDetachTests,RemoteTmuxAuthTestsandRemoteTmuxMissingTmuxTests(46 tests in 4 suites).I also checked it from the CLI against a live tmux server on a local build that carries this change on top of other branches of mine. A failing
cmux ssh-tmuxprinted the Cloud VM line before and its own reason after.Changelog
Fixed: A failed
cmux ssh-tmuxnow reports why it failed instead of a Cloud VM error.Checklist
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixed failed remote tmux requests reporting the Cloud VM error line instead of their own reason.
Remote tmux socket methods run through
v2VmCall, the wrapper Cloud VM requests use, and its fallback returns "The Cloud VM request failed" for any error it does not recognize.RemoteTmuxErrorwas unrecognized, so a failingcmux ssh-tmuxpointed users atcmux vm lseven when no Cloud VM was involved. The wrapper now returnsRemoteTmuxError.messageas-is, since it already flattens and caps any remote text it includes.RemoteTmuxSocketErrorTextTestscovering threeRemoteTmuxErrorcases routed throughv2VmCall.Written for commit 30da6b1. Summary will update on new commits.
Summary by CodeRabbit