Fix WSL log tail commands#10501
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @kswift1 on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
github-actions
Bot
added
the
external-contributor
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR updates log-tail command construction so Windows host log paths can be converted for WSL/MSYS2-like sessions, and adds tests for WSL and PowerShell command output.
Concerns
- The new call sites use
terminal.sessions(ctx), but that accessor is only compiled for tests/integration builds, so normal app builds will fail. The same call also producesOption<Arc<Session>>, whiletail_command_for_shellnow expectsOption<&Session>.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| let tail_command = tail_command_for_shell(shell_family, log_file_path); | ||
| let session = terminal | ||
| .active_block_session_id() | ||
| .and_then(|session_id| terminal.sessions(ctx).get(session_id)); |
There was a problem hiding this comment.
terminal.sessions(ctx) is gated behind #[cfg(any(test, feature = "integration_tests"))], and Sessions::get returns Arc<Session> rather than &Session, so this production call path will not compile; use sessions_model().as_ref(ctx).get(session_id).as_deref() or change the helper signature.
| let tail_command = tail_command_for_shell(shell_family, log_path); | ||
| let session = terminal | ||
| .active_block_session_id() | ||
| .and_then(|session_id| terminal.sessions(ctx).get(session_id)); |
There was a problem hiding this comment.
terminal.sessions(ctx) is not available outside test/integration builds, and the returned Arc<Session> does not match the new Option<&Session> parameter; use the production sessions model accessor and convert with as_deref().
kswift1
force-pushed
the
fix/mcp-show-logs-wsl-path
branch
from
4f1b2f7 to
5d0c56c
Compare
|
Addressed the review in a production-safe follow-up. The tail-command helper now takes shell identity via |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR introduces session-aware path conversion for tail commands used by MCP and LSP log viewing and adds unit coverage for WSL/MSYS2/PowerShell command output.
Concerns
- MCP and LSP log opening add a fresh terminal pane before querying
active_session_shell; for a newly-created pane the active session may not be bootstrapped yet, so WSL/MSYS2 launch data can be unavailable and the Windows log path remains unconverted.
Verdict
Found: 0 critical, 2 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| terminal_view_handle.update(ctx, |terminal, ctx| { | ||
| let shell_family = terminal.shell_family(ctx); | ||
| let tail_command = tail_command_for_shell(shell_family, log_file_path); | ||
| let shell_launch_data = terminal.active_session_shell(ctx); |
There was a problem hiding this comment.
active_session_shell can be None immediately after creating the log pane, so WSL/MSYS2 sessions can still get the unconverted Windows log path; use the shell launch data chosen for the new pane instead of only the bootstrapped active session.
| terminal_view_handle.update(ctx, |terminal, ctx| { | ||
| let shell_family = terminal.shell_family(ctx); | ||
| let tail_command = tail_command_for_shell(shell_family, log_path); | ||
| let shell_launch_data = terminal.active_session_shell(ctx); |
There was a problem hiding this comment.
active_session_shell can be None immediately after creating the LSP log pane, so WSL/MSYS2 sessions can still get the unconverted Windows log path; use the shell launch data chosen for the new pane instead of only the bootstrapped active session.
|
Addressed the fresh-pane timing issue. MCP/LSP log opening now uses a small TerminalView helper that prefers the active session shell after bootstrap and falls back to the terminal model's pre-bootstrap active shell launch data when the new pane has not joined a session yet. This keeps the session-aware WSL/MSYS2 conversion, but avoids depending on freshly-bootstrapped pane session metadata. |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR updates log-tail command generation so MCP/LSP log paths are converted for WSL/MSYS2 sessions before being inserted into the active terminal.
Concerns
- Manual testing evidence is missing for this manually testable behavioral change. Please include screenshots or a screen recording showing the log-tail flow working end to end in a WSL session, or justify why manual testing is not possible.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
This fix targets a Windows host + WSL shell path conversion issue. I wasn’t able to run an end-to-end manual verification in a Windows + WSL Warp environment from my current setup. I did validate the code path, updated the implementation to use pre-bootstrap shell launch data, and added targeted unit coverage for WSL/MSYS2/PowerShell path handling. If helpful, I’d appreciate a maintainer-side verification in a Windows WSL environment. |
1 participant
Summary
Closes #10169.