feat: improve SSH host UX and model profile validation#13
feat: improve SSH host UX and model profile validation#13Keith-CY merged 6 commits intolay2dev:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ada9cb6549
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let entries = pool | ||
| .sftp_list(host_id, "~/.openclaw/agents") | ||
| .await | ||
| .map_err(|e| format!("Failed to list remote agents directory: {e}"))?; |
There was a problem hiding this comment.
Treat missing remote agents dir as no auth profiles
resolve_remote_key_from_agent_auth_profiles turns any failure from listing ~/.openclaw/agents into a hard error, so remote_test_model_profile aborts before provider-env fallback can run. On remotes that don't have an agents directory (a common setup when credentials come from environment variables), profile tests will fail even when a valid fallback key like ${PROVIDER}_API_KEY is present. Missing-path errors here should be treated as Ok(None) instead of failing the whole resolution path.
Useful? React with 👍 / 👎.
| std::env::temp_dir() | ||
| .join(format!("clawpal-askpass-{}.bat", uuid::Uuid::new_v4())), | ||
| ); | ||
| let script = "@echo off\r\necho %CLAWPAL_SSH_PASSPHRASE%\r\n"; |
There was a problem hiding this comment.
Escape passphrase in Windows askpass helper
The generated .bat askpass script uses echo %CLAWPAL_SSH_PASSPHRASE%, but cmd.exe treats characters such as &, |, >, <, and parentheses as metacharacters. If a key passphrase contains any of these, the echoed value is altered and ssh-add receives the wrong passphrase, so passphrase-based connection fails for valid credentials on Windows. The helper should output the env value using a quoting/escaping-safe approach.
Useful? React with 👍 / 👎.
dev01lay2
left a comment
There was a problem hiding this comment.
Solid feature PR — SSH config auto-detect, passphrase-first UX, model profile testing, and remote key resolution are all welcome additions. A few observations:
Backend (commands.rs)
SSH config parser — thorough handling of quotes, comments, and multi-alias Host lines. The dedup via BTreeMap at the end is a nice touch.
run_provider_probe — clean implementation. A few notes:
- The 20s timeout is reasonable for a user-initiated test action.
truncate_error_textcorrectly handles multi-byte chars viachar_indices. 👍- Consider whether the Anthropic branch should also treat 4xx with specific error types (e.g.
overloaded_error) differently from auth failures, so the toast message is more actionable. Not blocking.
resolve_remote_profile_api_key — the fallback chain (direct key → auth_ref as env var → auth_ref from agent auth-profiles → provider convention env vars) is well-structured. The is_valid_env_var_name guard before printenv is good defense.
is_remote_missing_path_error — the string matching is pragmatic but fragile across locales/SSH implementations. Since this is best-effort fallback, it's acceptable, but a comment noting this would help future maintainers.
SSH (ssh.rs)
Passphrase flow — the RAII AskpassScript with Drop cleanup is good practice. Two notes:
- The Unix askpass script has a stray
\\nat the end:"#!/bin/sh\nprintf '%s\\n' \"\\"\\n"— that literal\nafter the printf will be written into the script file. Should just be\n(newline), not\\n. Worth double-checking the generated script content. - The Windows
.batversion usesecho %CLAWPAL_SSH_PASSPHRASE%which will break if the passphrase contains&,|,>, etc. Considerecho(%CLAWPAL_SSH_PASSPHRASE%or a PowerShell wrapper. Not critical for v1 but worth a TODO.
Duplication: add_key_to_agent_with_passphrase is implemented twice (Unix + Windows inner modules) with slightly different script content. Could extract the shared logic into a helper that takes a script template string, but it's manageable at this size.
Frontend
Passphrase dialog — clean promise-based flow with passphraseResolveRef. The Enter-to-submit on the password input is a nice UX touch.
Duplicate name validation — hasDuplicateInputName using useMemo is correct. The inline error clears on input change. Good.
SSH config preset dropdown — auto-fills host/user/port from ~/.ssh/config, which is exactly the UX improvement described. The useEffect fetch on mount is fine since this is a dialog-level component.
Settings profile actions — Test and Enable/Disable buttons with toast feedback via sonner integrate well. The testingProfileId loading state prevents double-clicks.
i18n
Both en.json and zh.json are in sync — all new keys present in both. ✅
Minor
.gitignorenow excludesbun.lockb/bun.lock/bun.dlock— makes sense with the bun migration. But if bun.lock is the new lockfile, should it actually be committed for reproducible installs? Worth confirming intent.
Overall: well-scoped, good error handling, no security concerns. The askpass script content on Unix is the one thing I'd double-check before merge.
Summary
Scope
Source and Target