Conversation
WalkthroughThis pull request introduces several significant enhancements to the Wave Terminal's codebase, focusing on connection management, shell command execution, and infrastructure improvements. The changes span multiple packages and introduce new functionality for handling SSH and WSL connections, implementing generic connection utilities, and improving error handling across various components. Key additions include a new 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 (
|
…ric RunSimple. create a new SyncBuffer struct.
… in the CpWsh func
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
pkg/genconn/wsl-impl.go (1)
18-24: ConstructingWSLShellClient
This constructor is straightforward and correctly assigns the incoming distribution. Consider logging or validating thedistroargument if there's any chance it might be nil or invalid at runtime.pkg/remote/conncontroller/conncontroller.go (1)
319-321: Version check logic
Usingsemver.Compare(clientVersion, expectedVersion) >= 0means the installation is skipped if the installed version is the same or newer. If it’s intentional to let a newer version stand, this is correct. Otherwise, consider using a strictly-greater check (> 0) to prompt upgrades if needed.cmd/wsh/cmd/wshcmd-conn.go (2)
90-103: Separate concerns between SSH and WSL retrievalThe consolidated approach of fetching SSH and WSL statuses in one function is clear and maintainable. However, consider logging errors where they occur before returning them, to provide visibility into which connection type failed.
153-165: Validate concurrency safety when disconnecting multiple connectionsLooping through all connections and disconnecting each individually is generally fine. However, in environments with numerous connections, consider whether concurrent disconnections might be beneficial. Should you later decide on asynchronous disconnect calls, ensure concurrency safety on shared resources.
pkg/wavebase/wavebase.go (2)
55-63: Add documentation for supported binariesThe map-based approach is effective for quick membership lookups. Consider adding a brief comment explaining the purpose of each supported key and how new platform keys can be added or removed. This can reduce confusion for future maintainers.
278-283: Include case normalization on inputsIf there is any chance the OS or architecture might be passed with varied casing (e.g., "Linux" vs. "linux"), consider normalizing case before lookup to avoid false negatives. Otherwise, looks good.
pkg/wsl/wsl.go (1)
332-335: Surface more context for debuggingWhen returning the error from
shellutil.GetWshBinaryPath, consider wrapping it with contextual information (e.g., which OS or architecture) to aid debugging. For instance:- return err + return fmt.Errorf("failed to get wsh binary path for %s-%s: %w", clientOs, clientArch, err)ROADMAP.md (1)
32-32: Status update aligns with ongoing connection improvements.The status change from Planned to In Progress accurately reflects the current work on connection-related improvements, which is consistent with this PR's objectives.
Consider updating the v0.11 release timeline since we're past January 13th, 2024.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
ROADMAP.md(1 hunks)cmd/wsh/cmd/wshcmd-conn.go(2 hunks)frontend/app/store/keymodel.ts(0 hunks)go.mod(1 hunks)pkg/blockcontroller/blockcontroller.go(8 hunks)pkg/genconn/genconn.go(1 hunks)pkg/genconn/quote.go(1 hunks)pkg/genconn/quote_test.go(1 hunks)pkg/genconn/ssh-impl.go(1 hunks)pkg/genconn/wsl-impl.go(1 hunks)pkg/remote/conncontroller/conncontroller.go(4 hunks)pkg/remote/connutil.go(4 hunks)pkg/util/shellutil/shellutil.go(5 hunks)pkg/util/syncbuf/syncbuf.go(1 hunks)pkg/wavebase/wavebase.go(2 hunks)pkg/wsl/wsl-util.go(1 hunks)pkg/wsl/wsl.go(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/app/store/keymodel.ts
🔇 Additional comments (38)
pkg/genconn/wsl-impl.go (5)
1-5: Build Constraint and License Notice
These lines establish the Windows-only build constraint and the license. Everything here looks properly defined.
43-61:MakeWSLProcessControllerError Handling
The function correctly handles potential errors fromBuildShellCommand. Usingfmt.Errorf()with"%w"to wrap the underlying error is a good practice for better context. Also, returning early on error is clean. Nice work.
63-77: Safe Start Sequence
Lock usage and the check forw.startedeffectively prevent starting the same command multiple times. The error messages are clear.
79-93: Single Wait Execution & Graceful Kill
Usingsync.OnceinWait()avoids multiple waits. TheKill()method is straightforward. However, note thatProcess.Kill()does not provide an error return; we rely on the process to terminate properly. This is acceptable if partial knowledge of kill success is acceptable.
95-146: Pipe Management
Stdin, Stdout, and Stderr pipe methods properly guard against calling afterStart(), preventing usage conflicts. Also, the checks for multiple calls on the same pipe are good. Overall, concurrency handling here looks solid.pkg/genconn/genconn.go (4)
1-17: Package Introduction
The file sets up a sound foundation for generic shell execution, definingCommandSpec,ShellClient, andShellProcessController. The design seems flexible for different backends (e.g., SSH, WSL).
39-57:RunSimpleCommandPipeline Setup
Creates a process, retrieves stdout/stderr pipes, and starts the command. Good use of aWaitGroupto handle concurrent copying from output streams. The error wrapping adds clarity when diagnosing failures.
58-77: Concurrent Stream Copy & Context Wait
io.Copyfrom stdout/stderr intosyncbufis a straightforward approach.ProcessContextWaitis an elegant way to combine process waiting with a context cancellation. The final returns of combined outputs are neatly done.
110-128:BuildShellCommandand Environment Handling
Building shell commands with environment variables is properly validated. TheHardQuoteusage ensures safer command construction. Be mindful that quoting logic must be carefully tested to avoid edge cases with special characters.pkg/genconn/ssh-impl.go (5)
16-22:SSHShellClientConstruction
Simple structure wraps anssh.Client. Building around an existing SSH client is a nice approach for modularity. No issues here.
42-55:MakeSSHCmdClientSession Initialization
The creation of anssh.Sessionis properly checked for errors, returning a wrapped error if session creation fails. This approach is clear and easy to maintain.
57-80:StartMethod
Locks ensure that the command isn’t started more than once. The fallback logic for unpiped input/output is a nice detail, avoiding corner cases. This is well-executed.
82-98:WaitandKillMethods
Similar to the WSL version,sync.Onceensures a consistent wait. TheKill()method (session.Close()) is safe, though you might consider clarifying in docstrings that closing the session terminates the remote process.
100-145: Pipe Setup
Each pipe method checksstartedand whether the pipe was already created. This prevents race conditions and pipe misuse. Error messages are clear, and concurrency is properly managed with the lock.pkg/util/shellutil/shellutil.go (3)
96-101: Windows Shell Detection
Here,exec.LookPath("pwsh")and thenexec.LookPath("powershell")is a sensible fallback chain, returning"powershell.exe"if neither is found. This logic ensures a best-effort approach for Windows environments.
Line range hint
222-237:GetWshBinaryPath
The function normalizes architecture names, checks for unsupported platforms, then constructs a filename. This is a good pattern. Just ensure calls towavebase.SupportedWshBinariesremain updated for newly supported OS/arch combos.
Line range hint
311-327: Graceful Handling of Missing Binaries
initCustomShellStartupFilesInternal()logs a non-fatal error if the WSH binary path is missing, then returns early. This approach avoids halting the entire initialization. This is user-friendly and robust.pkg/remote/connutil.go (6)
8-8: Context-Based Refactor
Introducingcontextis a great move toward graceful cancellation and timeouts.
Line range hint
58-81:GetWshVersion
Validates that thewsh versionoutput has exactly two fields and performs semver checks. The explicit error for unexpected output is helpful for debugging.
155-158:normalizeOs
Lowercasing and trimming OS strings ensures consistent comparisons. This is a simple but important detail to avoid subtle mismatches.
160-168:normalizeArch
Maps known architecture synonyms (x86_64,amd64→x64;arm64,aarch64→arm64). This is straightforward, but stay in sync if new arch naming arises.
171-190:GetClientPlatform
Runsuname -smremotely, splitting the result into OS and arch. Checkingwavebase.ValidateWshSupportedArchis a good integration that ensures the rest of the code doesn’t proceed on unsupported platforms.
200-255:CpWshToRemoteFlow
- Retrieves the local WSH path using
GetWshBinaryPath.- Streams the binary to the remote host using an install template with final naming and permissions.
- Uses concurrency (
copyDonechannel) to track data copying.- Properly checks both the process error and the copy error.
This is cleanly structured code with robust error handling.
pkg/remote/conncontroller/conncontroller.go (3)
23-23: New import forgenconn
The newly added import forgithub.com/wavetermdev/waveterm/pkg/genconnindicates integration with generic connection utilities. This looks appropriate given the usage ofgenconn.MakeSSHShellClientlater in the file.
36-36: Usinggolang.org/x/mod/semver
Introducing semver comparison is beneficial for flexible and robust version checks. This import nicely aligns with the updated logic inCheckAndInstallWsh.
373-379: Installation flow for Wsh
After callingCpWshToRemote, the code returns an error if the operation fails. If partial installation could leave the remote target in an inconsistent state, consider adding optional cleanup or rollback steps. Otherwise, this error handling is sufficient.
[approve]pkg/blockcontroller/blockcontroller.go (5)
276-281: Sequential shell command orchestration
This newDoRunShellCommandmethod cleanly delegates shell process setup tosetupAndStartShellProcessand lifecycle management tomanageRunningShellProcess, improving clarity and separation of concerns.
Line range hint
283-423: Refactored shell process initialization
- Block file creation (lines 287-291): Good approach using a circular buffer to limit log size. Resetting terminal state upon
fs.ErrExistensures a fresh environment.- Status check (line 297): Early return if already running helps avoid duplicate processes.
- Command resolution (lines 303-324): Separates logic for shell-based vs. command-based controllers. Clear error returns for unknown controllers.
- Remote vs. local logic (lines 335-414): The branching structure is clean and explicit for WSL, SSH, or local execution. Encouraging clarifying logs or metrics for each path might aid debugging.
- Handling Wsh fallback (lines 373-386): If Wsh fails, you gracefully disable it and attempt a no-Wsh scenario, increasing resilience.
- Final update and status assignment (lines 414-422): Attaching the new
ShellProcto the controller with a running state is neat.Overall, this function is well-structured. A minor note: consider factoring out helper functions or sub-blocks if it grows larger in the future.
Line range hint
425-525: Shell process lifecycle management
- Concurrent read/write loops (lines ~439-482): Each goroutine handles a distinct data flow path. Carefully performed defers ensure cleanup in error conditions.
- Input channel usage (lines ~444-455): The
shellInputChdesign is logical. You guard references by storing it locally within the goroutine.- Final wait and exit code handling (lines ~494-515): Storing the exit code and updating status to
Status_Doneis consistent. Unregistering the route to avoid stale references is also a plus.This method is a robust approach to orchestrating the shell I/O. Good job.
526-535: NewupdateTermSizeutility
The function sets term size in the database, then updates the shell’s PTY dimensions. Good abstraction that ensures both the DB model and the shell environment remain synchronized.
584-599: Persisting terminal dimensions
SettingtermSizein the DB (lines 584–599) is concise and has appropriate error handling. If partial updates are a concern, you already store changes inRuntimeOptsand send broker updates on success. This is coherent.pkg/util/syncbuf/syncbuf.go (1)
1-41: Thread-safe buffer implementation
- Structure & Locking (lines 12-16): Straightforward design with a mutex-protected
bytes.Buffer.- Instantiation (lines 17-22):
MakeSyncBuffercorrectly returns a pointer, and re-initializes a freshbytes.Buffer.- Async copying (lines 24-29):
MakeSyncBufferFromReaderelegantly streams data into the buffer, ensuring concurrency safety.- Write and String methods (lines 31-41): Both hold the lock during operations, maintaining concurrency safety.
This is a clear and usable concurrency utility.
pkg/genconn/quote.go (1)
1-78: String quoting helpers
HardQuote(lines 19-40): Simple, direct solution. Good check of empty strings. Proper escaping of special characters ensures safe shell usage.SoftQuote(lines 42-78): The special handling for~expansions is a thoughtful addition, letting certain paths remain unquoted. Skipping$escaping fosters environment variable expansion.The approach is consistent with typical shell quoting practices. Watch for corner cases (e.g., multi-byte unicode). Otherwise, it’s well done.
pkg/genconn/quote_test.go (1)
1-110: Comprehensive quote testing
The test table covers various edge cases (empty strings, tilde patterns, variables, embedded quotes/backslashes, etc.). This breadth of coverage is commendable. Consider adding a test for multi-byte characters (e.g., Unicode) if that’s relevant to your user base.cmd/wsh/cmd/wshcmd-conn.go (1)
105-109: Graceful handling of no connection scenarioThe logic is straightforward and gracefully handles the “no connections” case. This looks good.
pkg/wsl/wsl-util.go (1)
16-16: Switch to text-based templatingSwitching from HTML-based templates to
text/templateis sensible for command generation. Ensure any previously relied-upon escaping or sanitization logic is still enforced where needed.go.mod (2)
66-66: New indirect dependency looks good.The addition of
golang.org/x/mod v0.22.0as an indirect dependency is appropriate as it's a well-maintained part of the Go ecosystem.
Line range hint
77-79: Verify the security implications of using forked dependencies.The use of forked versions of
ssh_configandptypackages requires careful consideration:
- Please document the rationale for using these specific forks in the codebase.
- Ensure there's a process to keep track of upstream security fixes.
Let's verify the changes in these forks:
* use pwsh over powershell if installed (on windows) for default shell * refactor blockcontroller.DoRunShellCommand into a "setup" and "manage" phase * fix wshcmd-conn to also disconnect wsl connections * new genconn interfaces to make a standardized environment to run SSH/WSL commands via `sh -c`. also create better quoting functions that are composable * replace html/template with text/template for shell command templating (avoids special chars getting turned into HTML entities, breaking the commands) * do not reinstall wsh if the installed version has a higher version (prevents flip-flopping on shared systems) * simplify clientOs/clientArch detection. use `uname -sm`. also validate the os/arch combo as compatible with our builds. * replace CpHostToRemote with CpWshToRemote. hard codes wsh paths inside of the function instead of having them passed in (quoting restrictions) * new SyncBuffer class to use with commands that properly synchronizes Writes/String output * fix setTermSize to actually update DB with terminal size
1 participant
No description provided.