RemoteInfo Rpc #1719
RemoteInfo Rpc #1719
Conversation
This adds an rpc command to get info on the remote system.
The SoftQuote function makes it unnecessary to fully expand the home directory path. This replaces those cases with a ~ instead.
This removes the need to create a new session for detecting the shell
WalkthroughThe pull request introduces a comprehensive set of changes across multiple files, focusing on remote shell information retrieval and connection management. The modifications primarily involve updating the mechanism for obtaining remote shell and system information, removing the Key changes include adding a new method Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
pkg/shellexec/shellexec.go (3)
Line range hint
22-32: Organize Import Statements for ReadabilityConsider grouping related import statements together and removing any unused imports to improve code readability and maintainability. Specifically, verify if all newly added imports are necessary.
292-299: Error Handling Enhancement When Obtaining Remote InfoCurrently, if
wshclient.RemoteGetInfoCommandfails, the error is returned directly. Provide more context in the error message to aid debugging.Apply this diff to enhance the error message:
if err != nil { - return nil, fmt.Errorf("unable to obtain client info: %w", err) + return nil, fmt.Errorf("failed to retrieve remote shell information: %w", err) }
308-309: Redundant Log MessageThe log message
"using shell: %s"is already being logged in other parts of the code. Ensure that logging is consistent and necessary to avoid cluttering the logs.Consider removing or consolidating log messages for clarity.
pkg/wshrpc/wshserver/wshserver.go (2)
703-707: LGTM! Good architectural improvement.The change to extract the connection name from RPC context instead of RemoteInfo aligns with the PR's goal and improves the architecture by:
- Maintaining a single source of truth for connection names
- Reducing coupling between RemoteInfo and connection management
705-706: Improve error message clarity.The error message could be more specific about what went wrong.
- return false, fmt.Errorf("could not determine handler from context") + return false, fmt.Errorf("RPC response handler not found in context")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/wsh/cmd/wshcmd-connserver.go(1 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(0 hunks)pkg/remote/conncontroller/conncontroller.go(1 hunks)pkg/remote/connutil.go(0 hunks)pkg/shellexec/shellexec.go(4 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshremote/wshremote.go(2 hunks)pkg/wshrpc/wshrpctypes.go(2 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wshutil/wshutil.go(3 hunks)
💤 Files with no reviewable changes (2)
- frontend/types/gotypes.d.ts
- pkg/remote/connutil.go
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CodeQL
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (11)
pkg/shellexec/shellexec.go (2)
324-325: Potential Issue with Fish Shell InitializationThe command constructed for Fish shell may not set the environment variables as intended. Verify that the
-Coption works correctly in the remote environment.Run the following script to verify Fish shell initialization commands:
✅ Verification successful
Fish Shell Initialization Verified as Correct
The command construction for Fish shell using
-Coption andset -x PATHis implemented correctly and consistently across the codebase. The path handling and environment variable setting follow Fish shell conventions.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the Fish shell correctly processes the initialization commands. # Test: Look for Fish shell initialization in the codebase. rg 'if isFishShell' -A 5Length of output: 1688
321-322: Ensure Correct Quoting of Bash RC PathWhen constructing the
bashPath, ensure thatgenconn.SoftQuoteproperly handles paths with spaces or special characters, especially in remote environments.Run the following script to check for any issues with path quoting:
✅ Verification successful
Path Quoting Implementation is Secure and Robust
The
genconn.SoftQuoteimplementation properly handles paths with spaces and special characters, including proper escaping and home directory expansion. The implementation is consistent across all shell integrations (bash, zsh, fish, and powershell).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that genconn.SoftQuote correctly quotes the bash RC path. # Test: Search for occurrences of genconn.SoftQuote usage. rg 'genconn\.SoftQuote\(' -A 2Length of output: 1134
Script:
#!/bin/bash # Find genconn.SoftQuote implementation ast-grep --pattern 'func SoftQuote($$$) $$$' # Check shellutil directory constants rg 'BashIntegrationDir|WaveHomeBinDir|PwshIntegrationDir|ZshIntegrationDir' -A 1Length of output: 5749
cmd/wsh/cmd/wshcmd-connserver.go (1)
193-193: Update Function Call AfterGetInfoSignature ChangeThe
GetInfofunction no longer accepts parameters. Ensure that the call towshutil.GetInfo()reflects this change and that any dependent code is updated accordingly.pkg/wshrpc/wshremote/wshremote.go (2)
21-21: Missing Import ofwshutilPackageThe
wshutilpackage is now required due to the addition ofRemoteGetInfoCommand. Ensure that the import statement is correctly added.
391-394: NewRemoteGetInfoCommandMethod AddedThe
RemoteGetInfoCommandmethod provides essential functionality for retrieving remote information. Ensure that this integration aligns with existing conventions and that any potential security implications are considered.Please confirm that:
- The method correctly retrieves and returns the necessary remote information.
- Proper authentication and authorization checks are in place to prevent unauthorized access.
If you need assistance verifying the security aspects, I can help draft additional tests or suggest best practices.
pkg/wshutil/wshutil.go (1)
543-553: LGTM! Well-structured shell detection implementation.The
getShell()function provides a robust way to determine the user's shell with proper platform-specific handling for macOS and sensible fallbacks.frontend/app/store/wshclientapi.ts (1)
245-249: LGTM! Clean and consistent RPC command implementation.The
RemoteGetInfoCommandimplementation follows the established pattern and properly handles the RPC call.pkg/wshrpc/wshclient/wshclient.go (1)
298-302: LGTM! Consistent with other RPC command implementations.The
RemoteGetInfoCommandimplementation follows the established pattern and properly handles the RPC call and error cases.pkg/wshrpc/wshrpctypes.go (2)
76-76: LGTM! Command constant follows naming convention.The
Command_RemoteGetInfoconstant is properly defined and follows the established naming pattern.
183-183: LGTM! Interface method properly defined.The
RemoteGetInfoCommandmethod is correctly added to the interface with proper context handling.pkg/remote/conncontroller/conncontroller.go (1)
382-382: LGTM! The logging change aligns with the PR objectives.The change to use
conn.GetName()instead ofremoteInfo.ConnNameis consistent with the PR's goal to update the remote shell identification mechanism.
| zshDir := genconn.SoftQuote(fmt.Sprintf("~/.waveterm/%s", shellutil.ZshIntegrationDir)) | ||
| cmdCombined = fmt.Sprintf(`ZDOTDIR=%s %s`, zshDir, cmdCombined) |
There was a problem hiding this comment.
Environment Variable ZDOTDIR May Not Be Set Correctly
Setting ZDOTDIR after the session is created may not affect the Zsh initialization as expected. Consider setting ZDOTDIR before the session starts to ensure that the custom Zsh configuration is loaded.
Apply this diff to set ZDOTDIR before the session starts:
for envKey, envVal := range cmdOpts.Env {
// note these might fail depending on server settings, but we still try
session.Setenv(envKey, envVal)
}
+ if isZshShell(shellPath) {
+ zshDir := genconn.SoftQuote(fmt.Sprintf("~/.waveterm/%s", shellutil.ZshIntegrationDir))
+ session.Setenv("ZDOTDIR", zshDir)
+ }
jwtToken, ok := cmdOpts.Env[wshutil.WaveJwtTokenVarName]
if !ok {
return nil, fmt.Errorf("no jwt token provided to connection")Committable suggestion skipped: line range outside the PR's diff.
| remoteInfo := wshutil.GetInfo() | ||
| needsRestartRaw, err := RpcClient.SendRpcRequest(wshrpc.Command_ConnUpdateWsh, remoteInfo, &wshrpc.RpcOpts{Timeout: 60000}) | ||
| if err != nil { | ||
| return fmt.Errorf("could not update: %w", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle Potential Errors Gracefully
After modifying the GetInfo function call, ensure that any potential errors are handled appropriately, and provide informative error messages to aid in troubleshooting.
Consider checking for errors and updating the error handling logic if necessary.
| func GetInfo() wshrpc.RemoteInfo { | ||
| return wshrpc.RemoteInfo{ | ||
| ConnName: rpcContext.Conn, | ||
| ClientArch: runtime.GOARCH, | ||
| ClientOs: runtime.GOOS, | ||
| ClientVersion: wavebase.WaveVersion, | ||
| Shell: os.Getenv("SHELL"), | ||
| Shell: getShell(), | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Shell path existence verification needed
The review comment is valid. The code assumes shell paths exist without verification:
- For non-Darwin: uses $SHELL or falls back to "/bin/bash"
- For Darwin: uses UserShell from dscl or falls back to "/bin/bash"
- No existence check is performed at any point in the chain
Suggested locations to add verification:
pkg/wshutil/wshutil.go:getShell(): Verify returned path existspkg/util/shellutil/shellutil.go:GetMacUserShell(): Verify shell path before returning
🔗 Analysis chain
Verify shell path existence.
The shell path returned by getShell() should be verified to exist before being used in GetInfo().
Run this script to verify the shell paths:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the default shell paths exist
# Test: Check if common shell paths exist
echo "Checking default shell paths..."
for shell in "/bin/bash" "/bin/sh" "/bin/zsh"; do
if [ -f "$shell" ]; then
echo "Found: $shell"
else
echo "Missing: $shell"
fi
done
# Test: Check if GetMacUserShell is properly implemented
echo "Checking for shellutil.GetMacUserShell implementation..."
rg -A 5 "func GetMacUserShell"
Length of output: 911
Script:
#!/bin/bash
# Find getShell implementation and its dependencies
ast-grep --pattern 'func getShell()' -l go
rg -A 10 'func getShell\(\)'
# Check for shell path validation
rg -i 'exists|stat|isfile|filepath' pkg/wshutil/
Length of output: 1817
Script:
#!/bin/bash
# Check GetMacUserShell implementation
rg -A 15 'func internalMacUserShell' pkg/util/shellutil/
Length of output: 1094
Script:
#!/bin/bash
# Check DefaultShellPath constant
rg "DefaultShellPath\s*=" pkg/util/shellutil/
Length of output: 116
Adds an Rpc Command for getting RemoteInfo. This is used to replace the session that was used to determine the shell on remote machines.