Conversation
This will theoretically allow updates to happen seamlessly without requiring a separate session.
This reduces a bit of duplication, both now, and for a future change.
WalkthroughThe pull request introduces a series of updates to the WSH (Wave Shell) system, primarily focusing on enhancing server functionality and adding a new update mechanism. Key changes include the introduction of a Additionally, a new 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 (2)
pkg/wshrpc/wshserver/wshserver.go (1)
702-734: Add logging and use constants for WSL prefix.The implementation looks good but could benefit from the following improvements:
- Add logging for better observability
- Use a constant for the "wsl://" prefix
Apply this diff to improve the implementation:
+const wslPrefix = "wsl://" + func (ws *WshServer) ConnUpdateWshCommand(ctx context.Context, updateInfo wshrpc.RemoteInfo) (bool, error) { connName := updateInfo.ConnName + log.Printf("checking if wsh update is needed for %s", connName) // check version number, return now if update not necessary upToDate, _, err := conncontroller.IsWshVersionUpToDate(updateInfo.ClientVersion) if err != nil { return false, fmt.Errorf("unable to compare wsh version: %w", err) } if upToDate { + log.Printf("wsh is up to date for %s", connName) // no need to update return false, nil } + log.Printf("wsh update needed for %s", connName) // todo: need to add user input code here for validation - if strings.HasPrefix(connName, "wsl://") { + if strings.HasPrefix(connName, wslPrefix) { return false, fmt.Errorf("cannot use this command for wsl connections") } connOpts, err := remote.ParseOpts(connName) if err != nil { return false, fmt.Errorf("error parsing connection name: %w", err) } conn := conncontroller.GetConn(ctx, connOpts, false, &wshrpc.ConnKeywords{}) if conn == nil { return false, fmt.Errorf("connection not found: %s", connName) } err = conn.UpdateWsh(ctx, connName, &conncontroller.WshInstallOpts{Force: true, NoUserPrompt: true}) if err != nil { return false, fmt.Errorf("update failed: %w", err) } + log.Printf("successfully updated wsh for %s", connName) // todo: need to add code for modifying configs? return true, nil }Track the TODO items.
There are two TODO items that need to be addressed:
- Add user input code for validation
- Add code for modifying configs
Would you like me to open GitHub issues to track these tasks?
pkg/wshutil/wshutil.go (1)
541-550: Consider adding error handling for environment variable.The function assumes the SHELL environment variable exists. While this is typically true on Unix-like systems, it might not be set in all environments.
Consider this improvement:
func GetInfo(rpcContext wshrpc.RpcContext) wshrpc.RemoteInfo { + shell := os.Getenv("SHELL") + if shell == "" { + shell = "/bin/sh" // provide a sensible default + } return wshrpc.RemoteInfo{ ConnName: rpcContext.Conn, ClientArch: runtime.GOARCH, ClientOs: runtime.GOOS, ClientVersion: wavebase.WaveVersion, - Shell: os.Getenv("SHELL"), + Shell: shell, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/wsh/cmd/wshcmd-connserver.go(4 hunks)frontend/app/store/wshclientapi.ts(1 hunks)frontend/types/gotypes.d.ts(1 hunks)pkg/remote/conncontroller/conncontroller.go(3 hunks)pkg/wshrpc/wshclient/wshclient.go(1 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)pkg/wshutil/wshutil.go(2 hunks)
👮 Files not reviewed due to content moderation or server errors (1)
- cmd/wsh/cmd/wshcmd-connserver.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
pkg/remote/conncontroller/conncontroller.go
382-382: ineffectual assignment to opts
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
pkg/wshrpc/wshrpctypes.go (1)
87-87: LGTM!The command constant follows the established naming pattern and its value is consistent with other commands in the codebase.
pkg/remote/conncontroller/conncontroller.go (1)
231-231: LGTM!Exporting
IsWshVersionUpToDateis appropriate as it's now used by the wshserver package.frontend/app/store/wshclientapi.ts (1)
60-64: Well-structured implementation following established patterns!The new command follows the consistent pattern used throughout the codebase for RPC commands.
pkg/wshrpc/wshclient/wshclient.go (1)
76-80: Clean implementation following the established pattern!The new command function is well-structured and consistent with other RPC command implementations in the file.
| type RemoteInfo struct { | ||
| ConnName string `json:"host"` | ||
| ClientArch string `json:"clientarch"` | ||
| ClientOs string `json:"clientos"` | ||
| ClientVersion string `json:"string"` | ||
| Shell string `json:"shell"` | ||
| } |
There was a problem hiding this comment.
Fix incorrect JSON tag for ClientVersion field.
The JSON tag for ClientVersion is incorrectly set to "string" instead of "clientversion". Also, consider grouping related fields together.
Apply this diff to fix the JSON tag and improve field grouping:
type RemoteInfo struct {
ConnName string `json:"host"`
+ Shell string `json:"shell"`
ClientArch string `json:"clientarch"`
ClientOs string `json:"clientos"`
- ClientVersion string `json:"string"`
- Shell string `json:"shell"`
+ ClientVersion string `json:"clientversion"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type RemoteInfo struct { | |
| ConnName string `json:"host"` | |
| ClientArch string `json:"clientarch"` | |
| ClientOs string `json:"clientos"` | |
| ClientVersion string `json:"string"` | |
| Shell string `json:"shell"` | |
| } | |
| type RemoteInfo struct { | |
| ConnName string `json:"host"` | |
| Shell string `json:"shell"` | |
| ClientArch string `json:"clientarch"` | |
| ClientOs string `json:"clientos"` | |
| ClientVersion string `json:"clientversion"` | |
| } |
| func (conn *SSHConn) UpdateWsh(ctx context.Context, clientDisplayName string, opts *WshInstallOpts) error { | ||
| if opts == nil { | ||
| opts = &WshInstallOpts{} | ||
| } | ||
| client := conn.GetClient() | ||
| if client == nil { | ||
| return fmt.Errorf("client is nil") | ||
| } | ||
| clientOs, clientArch, err := remote.GetClientPlatform(ctx, genconn.MakeSSHShellClient(client)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // should be able to GetClientPlatform in other ways, but this works for now | ||
| err = remote.CpWshToRemote(ctx, client, clientOs, clientArch) | ||
| if err != nil { | ||
| return fmt.Errorf("error installing wsh to remote: %w", err) | ||
| } | ||
| log.Printf("successfully installed wsh on %s\n", conn.GetName()) | ||
| return nil | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix ineffectual assignment and improve error handling.
The function has the following issues:
- The assignment to
optsat line 382 is ineffectual as it's immediately overwritten. - The error message from
GetClientPlatformshould be more descriptive.
Apply this diff to fix the issues:
func (conn *SSHConn) UpdateWsh(ctx context.Context, clientDisplayName string, opts *WshInstallOpts) error {
- if opts == nil {
- opts = &WshInstallOpts{}
- }
client := conn.GetClient()
if client == nil {
return fmt.Errorf("client is nil")
}
clientOs, clientArch, err := remote.GetClientPlatform(ctx, genconn.MakeSSHShellClient(client))
if err != nil {
- return err
+ return fmt.Errorf("failed to detect client platform: %w", err)
}
- // should be able to GetClientPlatform in other ways, but this works for now
err = remote.CpWshToRemote(ctx, client, clientOs, clientArch)
if err != nil {
return fmt.Errorf("error installing wsh to remote: %w", err)
}
log.Printf("successfully installed wsh on %s\n", conn.GetName())
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (conn *SSHConn) UpdateWsh(ctx context.Context, clientDisplayName string, opts *WshInstallOpts) error { | |
| if opts == nil { | |
| opts = &WshInstallOpts{} | |
| } | |
| client := conn.GetClient() | |
| if client == nil { | |
| return fmt.Errorf("client is nil") | |
| } | |
| clientOs, clientArch, err := remote.GetClientPlatform(ctx, genconn.MakeSSHShellClient(client)) | |
| if err != nil { | |
| return err | |
| } | |
| // should be able to GetClientPlatform in other ways, but this works for now | |
| err = remote.CpWshToRemote(ctx, client, clientOs, clientArch) | |
| if err != nil { | |
| return fmt.Errorf("error installing wsh to remote: %w", err) | |
| } | |
| log.Printf("successfully installed wsh on %s\n", conn.GetName()) | |
| return nil | |
| } | |
| func (conn *SSHConn) UpdateWsh(ctx context.Context, clientDisplayName string, opts *WshInstallOpts) error { | |
| client := conn.GetClient() | |
| if client == nil { | |
| return fmt.Errorf("client is nil") | |
| } | |
| clientOs, clientArch, err := remote.GetClientPlatform(ctx, genconn.MakeSSHShellClient(client)) | |
| if err != nil { | |
| return fmt.Errorf("failed to detect client platform: %w", err) | |
| } | |
| err = remote.CpWshToRemote(ctx, client, clientOs, clientArch) | |
| if err != nil { | |
| return fmt.Errorf("error installing wsh to remote: %w", err) | |
| } | |
| log.Printf("successfully installed wsh on %s\n", conn.GetName()) | |
| return nil | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
382-382: ineffectual assignment to opts
(ineffassign)
frontend/types/gotypes.d.ts
Outdated
| // wshrpc.UpdateInfo | ||
| type UpdateInfo = { | ||
| host: string; | ||
| clientarch: string; | ||
| clientos: string; | ||
| string: string; | ||
| }; |
There was a problem hiding this comment.
Fix field name in UpdateInfo type.
The string field should be renamed to clientversion to match the corrected Go struct.
Apply this diff to fix the field name:
type UpdateInfo = {
host: string;
clientarch: string;
clientos: string;
- string: string;
+ clientversion: string;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // wshrpc.UpdateInfo | |
| type UpdateInfo = { | |
| host: string; | |
| clientarch: string; | |
| clientos: string; | |
| string: string; | |
| }; | |
| // wshrpc.UpdateInfo | |
| type UpdateInfo = { | |
| host: string; | |
| clientarch: string; | |
| clientos: string; | |
| clientversion: string; | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/remote/conncontroller/conncontroller.go (1)
380-393: Improve error handling and logging.The function has the following suggestions:
- Add more descriptive error messages
- Use the connection's
Infofmethod for consistent logging- Consider validating the RemoteInfo fields before use
Apply this diff to improve the implementation:
func (conn *SSHConn) UpdateWsh(ctx context.Context, clientDisplayName string, remoteInfo *wshrpc.RemoteInfo) error { - log.Printf("attempting to update wsh for connection %v", remoteInfo) + conn.Infof(ctx, "attempting to update wsh for connection %s (os:%s arch:%s version:%s)\n", + remoteInfo.ConnName, remoteInfo.ClientOs, remoteInfo.ClientArch, remoteInfo.ClientVersion) + + if remoteInfo.ClientOs == "" || remoteInfo.ClientArch == "" { + return fmt.Errorf("invalid remote info: missing os or arch information") + } + client := conn.GetClient() if client == nil { - return fmt.Errorf("client is nil") + return fmt.Errorf("cannot update wsh: ssh client is not connected") } + err := remote.CpWshToRemote(ctx, client, remoteInfo.ClientOs, remoteInfo.ClientArch) if err != nil { return fmt.Errorf("error installing wsh to remote: %w", err) } - log.Printf("successfully installed wsh on %s\n", conn.GetName()) + conn.Infof(ctx, "successfully updated wsh on %s\n", conn.GetName()) return nil }pkg/wshrpc/wshserver/wshserver.go (2)
715-715: Track TODOs with GitHub issues.There are two TODO comments that need to be tracked:
- Add user input code for validation
- Add code for modifying configs
Would you like me to create GitHub issues to track these TODOs?
Also applies to: 733-733
702-735: Improve error handling and add validation.The implementation could be improved with:
- More descriptive error messages
- Validation of RemoteInfo fields
- Consistent logging
Apply this diff to improve the implementation:
func (ws *WshServer) ConnUpdateWshCommand(ctx context.Context, remoteInfo wshrpc.RemoteInfo) (bool, error) { connName := remoteInfo.ConnName + if connName == "" { + return false, fmt.Errorf("invalid remote info: missing connection name") + } + + log.Printf("checking wsh version for connection %s (current: %s)", connName, remoteInfo.ClientVersion) + // check version number, return now if update not necessary upToDate, _, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion) if err != nil { return false, fmt.Errorf("unable to compare wsh version: %w", err) } if upToDate { // no need to update - log.Printf("wsh update is not needed for connection %s", connName) + log.Printf("wsh is already up to date for connection %s", connName) return false, nil } // todo: need to add user input code here for validation if strings.HasPrefix(connName, "wsl://") { - return false, fmt.Errorf("cannot use this command for wsl connections") + return false, fmt.Errorf("wsh update is not supported for WSL connections") } connOpts, err := remote.ParseOpts(connName) if err != nil { return false, fmt.Errorf("error parsing connection name: %w", err) } conn := conncontroller.GetConn(ctx, connOpts, false, &wshrpc.ConnKeywords{}) if conn == nil { return false, fmt.Errorf("connection not found: %s", connName) } + + log.Printf("attempting to update wsh for connection %s", connName) err = conn.UpdateWsh(ctx, connName, &remoteInfo) if err != nil { - return false, fmt.Errorf("update failed: %w", err) + return false, fmt.Errorf("wsh update failed for connection %s: %w", connName, err) } // todo: need to add code for modifying configs? + log.Printf("successfully updated wsh for connection %s", connName) return true, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/remote/conncontroller/conncontroller.go(3 hunks)pkg/wshrpc/wshrpctypes.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
pkg/wshrpc/wshrpctypes.go (3)
87-87: LGTM!The command constant follows the established naming convention and is appropriately placed with other connection-related commands.
167-167: LGTM!The interface method signature is consistent with other connection-related methods.
505-511:⚠️ Potential issueFix incorrect JSON tag and improve field grouping.
The struct has the following issues:
- The JSON tag for
ClientVersionis incorrect, it should be"clientversion"instead of"string".- Related fields should be grouped together.
Apply this diff to fix the JSON tag and improve field grouping:
type RemoteInfo struct { ConnName string `json:"host"` + Shell string `json:"shell"` ClientArch string `json:"clientarch"` ClientOs string `json:"clientos"` - ClientVersion string `json:"string"` - Shell string `json:"shell"` + ClientVersion string `json:"clientversion"` }Likely invalid or redundant comment.
pkg/remote/conncontroller/conncontroller.go (1)
231-231: LGTM!The function is well-structured and uses proper semantic versioning for comparison.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/remote/conncontroller/conncontroller.go (1)
380-394:⚠️ Potential issueAdd validation for
remoteInfofields inUpdateWshEnsure that
remoteInfo.ClientOsandremoteInfo.ClientArchare not empty before proceeding, to prevent potential errors inremote.CpWshToRemote.Apply this diff to add validation:
client := conn.GetClient() if client == nil { return fmt.Errorf("cannot update wsh: ssh client is not connected") } + if remoteInfo.ClientOs == "" || remoteInfo.ClientArch == "" { + return fmt.Errorf("invalid remote info: missing ClientOs or ClientArch") + } err := remote.CpWshToRemote(ctx, client, remoteInfo.ClientOs, remoteInfo.ClientArch) if err != nil { return fmt.Errorf("error installing wsh to remote: %w", err) }
🧹 Nitpick comments (5)
pkg/wshrpc/wshserver/wshserver.go (2)
719-720: Implement user input validationThe
TODOcomment indicates that user input validation needs to be added. This is essential to ensure secure and reliable operation.Would you like me to implement the user input validation code, or open a new GitHub issue to track this task?
737-738: Implement configuration modification codeThe
TODOcomment suggests that code for modifying configurations is needed. Implementing this will ensure that configurations are properly updated after the WSH update.Would you like me to help implement the configuration modification code, or open a new GitHub issue to track this task?
pkg/remote/conncontroller/conncontroller.go (3)
Line range hint
231-242: RefactorIsWshVersionUpToDateto accept version strings directlyThe function
IsWshVersionUpToDatecurrently expects a version line in the format"wsh v0.10.4", which may not be flexible. Consider refactoring the function to accept version strings directly, improving reusability.Apply this diff to modify the function:
func IsWshVersionUpToDate(clientVersion string) (bool, string, error) { - wshVersionLine = strings.TrimSpace(wshVersionLine) - if wshVersionLine == "not-installed" { + clientVersion = strings.TrimSpace(clientVersion) + if clientVersion == "not-installed" || clientVersion == "" { return false, "", nil } - parts := strings.Fields(wshVersionLine) - if len(parts) != 2 { - return false, "", fmt.Errorf("unexpected version format: %s", wshVersionLine) - } - clientVersion := parts[1] expectedVersion := fmt.Sprintf("v%s", wavebase.WaveVersion) if semver.Compare(clientVersion, expectedVersion) < 0 { return false, clientVersion, nilThen, adjust the calls to this function accordingly.
Line range hint
293-299: Adjust call toIsWshVersionUpToDateAfter refactoring
IsWshVersionUpToDate, update the call inStartConnServerto pass the client version directly.Apply this diff:
conn.Infof(ctx, "got connserver version: %s\n", strings.TrimSpace(versionLine)) - isUpToDate, clientVersion, err := IsWshVersionUpToDate(versionLine) + parts := strings.Fields(versionLine) + if len(parts) != 2 { + return false, "", fmt.Errorf("unexpected version format: %s", versionLine) + } + clientVersion := parts[1] + isUpToDate, clientVersion, err := IsWshVersionUpToDate(clientVersion) if err != nil { sshSession.Close() return false, "", fmt.Errorf("error checking wsh version: %w", err)
293-294: Handle potential nilclientVersionafter parsingAfter extracting
clientVersion, ensure it is not empty before proceeding with the version comparison to prevent potential panics.Apply this diff:
clientVersion := parts[1] + if clientVersion == "" { + return false, "", fmt.Errorf("client version is empty") + } isUpToDate, clientVersion, err := IsWshVersionUpToDate(clientVersion)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/types/gotypes.d.ts(1 hunks)pkg/remote/conncontroller/conncontroller.go(3 hunks)pkg/wshrpc/wshserver/wshserver.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/types/gotypes.d.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build for TestDriver.ai
| upToDate, _, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion) | ||
| if err != nil { | ||
| return false, fmt.Errorf("unable to compare wsh version: %w", err) | ||
| } |
There was a problem hiding this comment.
Incorrect usage of IsWshVersionUpToDate function
The IsWshVersionUpToDate function expects a version line in the format "wsh v0.10.4" or "not-installed", but remoteInfo.ClientVersion likely contains only the version string (e.g., "v0.10.4"). Passing remoteInfo.ClientVersion directly may cause incorrect parsing and version comparison.
Apply this diff to adjust the version string before calling IsWshVersionUpToDate:
log.Printf("checking wsh version for connection %s (current: %s)", connName, remoteInfo.ClientVersion)
- upToDate, _, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion)
+ versionLine := fmt.Sprintf("wsh %s", remoteInfo.ClientVersion)
+ upToDate, _, err := conncontroller.IsWshVersionUpToDate(versionLine)
if err != nil {
return false, fmt.Errorf("unable to compare wsh version: %w", err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| upToDate, _, err := conncontroller.IsWshVersionUpToDate(remoteInfo.ClientVersion) | |
| if err != nil { | |
| return false, fmt.Errorf("unable to compare wsh version: %w", err) | |
| } | |
| versionLine := fmt.Sprintf("wsh %s", remoteInfo.ClientVersion) | |
| upToDate, _, err := conncontroller.IsWshVersionUpToDate(versionLine) | |
| if err != nil { | |
| return false, fmt.Errorf("unable to compare wsh version: %w", err) | |
| } |
This adds an RPC command for updating wsh on a remote machine without starting a new session. It is not being used yet, but will be used for connections using a single server in the future.
1 participant
This adds an RPC command for updating wsh on a remote machine without starting a new session. It is not being used yet, but will be used for connections using a single server in the future.