Working on Test Harness for Remote Connections #2829
Conversation
WalkthroughAdds a CLI test harness under cmd/test-conn/ including main-test-conn.go (CLI entry, flags, commands), cliprovider.go (CLIProvider implementing user input), and testutil.go (environment setup and connection/shell test helpers). Introduces a pluggable user input provider pattern in pkg/userinput (UserInputProvider, FrontendProvider, SetUserInputProvider). Adds verbosity constants in pkg/aiusechat/uctypes and updates default verbosity usage. Adds ai:verbosity fields to default wave AI configs. Adds nil checks for SwapToken in shellexec and relaxes filewatcher error handling; adds telemetry for two terminal commands. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/shellexec/shellexec.go`:
- Around line 335-337: StartWslShellProc and StartLocalShellProc currently
dereference cmdOpts.SwapToken without a nil check; add the same guard used
earlier (check if cmdOpts.SwapToken == nil and return a descriptive error like
"SwapToken is required in CommandOptsType") before any dereference in
StartWslShellProc and StartLocalShellProc so they don't panic, and ensure any
subsequent uses (e.g., in functions/methods called within those routines) can
safely assume SwapToken is non-nil.
🧹 Nitpick comments (8)
pkg/wconfig/filewatcher.go (1)
54-59: Silencing not-found errors for optional subdirectories looks correct.Gracefully skipping non-existent config subdirectories is a sensible change. One minor note: consider using
errors.Is(err, fs.ErrNotExist)instead ofos.IsNotExist(err)— it's the preferred idiom since Go 1.13 and correctly unwraps wrapped errors.♻️ Suggested change
Add to imports:
"errors" "io/fs"- if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, fs.ErrNotExist) {This would also allow removing the
"os"import if it's not used elsewhere in the file.pkg/userinput/userinput.go (1)
148-149: Consider guarding against nil provider.If
SetUserInputProvider(nil)is called, subsequentGetUserInputcalls will panic. A nil check would be a cheap safety net.Suggested guard
func SetUserInputProvider(provider UserInputProvider) { + if provider == nil { + panic("userinput: SetUserInputProvider called with nil provider") + } defaultProvider = provider }cmd/test-conn/cliprovider.go (1)
20-55: Implementation handles confirm-type prompts well; text-type input not handled.The method always prompts with "Accept? [y/n]" regardless of
request.ResponseType. If a request expects free-form text input (e.g., a password prompt), this will return "yes"/"no" instead of the actual text. Fine if the test harness only encounters confirm-type prompts, but worth noting for future extensibility.cmd/test-conn/testutil.go (4)
49-57: Non-macOS platforms requireWAVETERM_DATA_HOMEto be set manually.On Linux, the conventional XDG default would be
~/.local/share/waveterm. Returning an error here means the test harness won't work out-of-the-box on Linux without extra env setup. If this is intentional (macOS-only for now), consider documenting it in the usage text. Otherwise:Suggested Linux default
dataHome := os.Getenv("WAVETERM_DATA_HOME") if dataHome == "" { if runtime.GOOS == "darwin" { dataHome = filepath.Join(homeDir, "Library", "Application Support", "waveterm"+devSuffix) os.Setenv("WAVETERM_DATA_HOME", dataHome) + } else if runtime.GOOS == "linux" { + dataHome = filepath.Join(homeDir, ".local", "share", "waveterm"+devSuffix) + os.Setenv("WAVETERM_DATA_HOME", dataHome) } else { return fmt.Errorf("WAVETERM_DATA_HOME must be set on non-macOS systems") } }
63-105: Config watcher is started but never stopped.
wconfig.GetWatcher().Start()at line 101 is called, but there's no corresponding cleanup (e.g.,defer wconfig.GetWatcher().Stop()or similar). For a short-lived CLI tool this likely won't cause issues in practice, but it may leave goroutines/file watchers lingering until process exit.
223-236: SwapToken is missing a JWT token entry.
StartRemoteShellProc(shellexec.go line 453) readscmdOpts.SwapToken.Env[wavebase.WaveJwtTokenVarName]to optionally inject a JWT into the shell environment. The token is created here without a JWT entry, andForceJwtis false incmdOpts, so the current flow is safe. However, if this test is meant to exercise the full wsh exec path including JWT-authenticated communication, you may want to generate and include a JWT token:rpcCtx := wshrpc.RpcContext{Conn: connName} jwtToken, _ := wshutil.MakeClientJWTToken(rpcCtx) swapToken.Env[wavebase.WaveJwtTokenVarName] = jwtToken
175-187: Sleep-then-read is inherently racy for capturing command output.The 500ms sleep may not be sufficient on slow connections or loaded systems, and a single
Readcall may not capture all output. This is acceptable for a test/debug tool, but be aware it can produce flaky results. A read loop with a short deadline would be more robust.cmd/test-conn/main-test-conn.go (1)
26-27: Usage text: "connect" and "ssh" descriptions are potentially misleading.Both
connectandsshmap to the sametestBasicConnectfunction, but the descriptions suggest different behavior ("with wsh" vs. plain). SincetestBasicConnectalways attempts wsh setup (viaconn.Connect), the distinction in the help text may confuse users.Suggested clarification
Commands: - connect <user@host> - Test basic SSH connection with wsh - ssh <user@host> - Test basic SSH connection + connect, ssh <user@host> - Test basic SSH connection (wsh enabled by config)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/app/view/term/termwrap.ts`:
- Around line 253-262: The telemetry checks using
decodedCmd.startsWith("claude") and startsWith("opencode") can produce false
positives for commands that only share the prefix; update these checks to match
whole words only (e.g., require a trailing space or use a word-boundary regex)
before calling recordTEvent("action:term", { "action:type": "claude" }) /
recordTEvent("action:term", { "action:type": "opencode" }); specifically,
replace the bare startsWith calls on decodedCmd with either
decodedCmd.startsWith("claude ") / decodedCmd.startsWith("opencode ") or a regex
like /^\bclaude\b/ and /^\bopencode\b/ so only exact tokens trigger the
telemetry.
|
|
||
| if (decodedCmd.startsWith("claude")) { | ||
| recordTEvent("action:term", { "action:type": "claude" }); | ||
| return; | ||
| } | ||
|
|
||
| if (decodedCmd.startsWith("opencode")) { | ||
| recordTEvent("action:term", { "action:type": "opencode" }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Missing word boundary — may produce false-positive telemetry hits.
The existing checks in this function use either a trailing space ("ssh ") or a \b word boundary in a regex to avoid matching unrelated commands that share a prefix. These new checks use bare startsWith("claude") / startsWith("opencode"), which would also match commands like claude-export, claudetest, opencode-lint, etc.
Consider aligning with the existing pattern:
Suggested fix
- if (decodedCmd.startsWith("claude")) {
+ if (/^claude\b/.test(decodedCmd)) {
recordTEvent("action:term", { "action:type": "claude" });
return;
}
- if (decodedCmd.startsWith("opencode")) {
+ if (/^opencode\b/.test(decodedCmd)) {
recordTEvent("action:term", { "action:type": "opencode" });
return;
}📝 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.
| if (decodedCmd.startsWith("claude")) { | |
| recordTEvent("action:term", { "action:type": "claude" }); | |
| return; | |
| } | |
| if (decodedCmd.startsWith("opencode")) { | |
| recordTEvent("action:term", { "action:type": "opencode" }); | |
| return; | |
| } | |
| if (/^claude\b/.test(decodedCmd)) { | |
| recordTEvent("action:term", { "action:type": "claude" }); | |
| return; | |
| } | |
| if (/^opencode\b/.test(decodedCmd)) { | |
| recordTEvent("action:term", { "action:type": "opencode" }); | |
| return; | |
| } |
🤖 Prompt for AI Agents
In `@frontend/app/view/term/termwrap.ts` around lines 253 - 262, The telemetry
checks using decodedCmd.startsWith("claude") and startsWith("opencode") can
produce false positives for commands that only share the prefix; update these
checks to match whole words only (e.g., require a trailing space or use a
word-boundary regex) before calling recordTEvent("action:term", { "action:type":
"claude" }) / recordTEvent("action:term", { "action:type": "opencode" });
specifically, replace the bare startsWith calls on decodedCmd with either
decodedCmd.startsWith("claude ") / decodedCmd.startsWith("opencode ") or a regex
like /^\bclaude\b/ and /^\bopencode\b/ so only exact tokens trigger the
telemetry.
1 participant
This pull request introduces a new
test-conncommand-line tool for testing SSH connection flows and user input handling, along with improvements to user input provider extensibility and several configuration and validation enhancements.test-connCLI ToolUserInputProviderinterface, with a defaultFrontendProviderand the ability to set a custom providerSwapTokenmust be present inCommandOptsTypewhen starting a remote shell with wsh, improving validation and error handling. (pkg/shellexec/shellexec.go)