mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-09-01 00:59:33 +00:00
Fix skills system: enable by default, fix registry and install (#300)
* feat: add Docker detection module with platform guidance Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: add Docker sandbox step to setup wizard Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: show Docker status in boot screen Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: check Docker availability at startup When SANDBOX_ENABLED=true, proactively detect whether Docker is installed and running before creating the ContainerJobManager. If Docker is unavailable, log a warning with platform-specific guidance and disable the sandbox for the session. Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: enable sandbox by default, improve wizard explanation, document detection limits - SandboxConfig defaults to enabled=true (startup check disables gracefully if Docker is unavailable) - Wizard step explains why Docker matters: isolation for LLM-generated code vs running directly on the host - Document detection confidence per platform in detect.rs module docs: high on macOS/Linux, medium on Windows (named pipe edge cases) Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: cargo fmt + update test_builder_defaults for enabled-by-default Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: deduplicate wizard Docker status handling per review Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: fix skills system - enable by default, fix registry connectivity and install - Enable skills system by default (SKILLS_ENABLED no longer required) - Bypass Vercel TLS fingerprint blocking by pointing DEFAULT_REGISTRY_URL directly at the Convex backend (wry-manatee-359.convex.site) - Handle ZIP archives from ClawHub download API - the registry returns ZIP files containing SKILL.md, not raw text. Uses flate2 (existing dep) to extract SKILL.md from the archive. - Surface catalog search errors in the UI with a yellow warning banner instead of silently returning empty results - Handle both {"results":[...]} envelope and bare [...] array JSON formats from the search API - Add ClawHub links and metadata to search result cards (clickable skill names linking to clawhub.ai, relevance score, "updated X ago" recency) - Fix 3 pre-existing clippy warnings in tests/html_to_markdown.rs Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: address security review feedback on ZIP extraction and SSRF - Cap download size to 10 MB before reading response body - Guard against ZIP bombs: cap uncompressed_size at 1 MB, wrap DeflateDecoder with .take() read limit - Use checked_add for ZIP header offset arithmetic to prevent overflow - Remove .unwrap() on try_into() -- use direct array construction - Handle IPv4-mapped IPv6 addresses (::ffff:192.168.x.x) in SSRF checks - Don't leak internal registry URLs in user-facing catalog_error messages - Fix non-ASCII panic in catalog response debug logging (use .get() instead of byte slicing) Co-Authored-By: Claude Opus 4.6 <[email protected]> * feat: add /skills command and enrich search results with ClawHub metadata - Parse /skills and /skills search <query> as SystemCommands in submission.rs - Add skill_catalog to AgentDeps and wire it through main.rs - Handle "skills" command in commands.rs: list installed skills and search ClawHub - Add /skills and /skills search <q> entries to /help output - Add SkillDetail, SkillStats, SkillOwner structs to catalog.rs - Add fetch_skill_detail() calling GET /api/v1/skills/{slug} on Convex backend - Add enrich_search_results() to fetch stars/downloads/owner for top 5 results in parallel - Fix SkillDetailResponse wrapper struct to match actual API shape: {"skill":{...},"owner":{...}} - Surface stars, downloads, owner in web UI skill search cards (app.js) - Surface enriched data in skills web handler and skill_search tool output Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix: cargo fmt after merge conflict resolution Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix: separate installed_skills dir for correct trust on restart, remove duplicate handlers Trust level bug: skills installed from ClawHub were written to user_dir (~/.ironclaw/skills/) which is discovered as Trusted on restart. Now installs go to ~/.ironclaw/installed_skills/ which is discovered as Installed, matching the documented skill directory layout. Changes: - SkillsConfig: add installed_dir field (SKILLS_INSTALLED_DIR env var, default ~/.ironclaw/installed_skills/) - SkillRegistry: add with_installed_dir() builder, installed_dir()/ install_target_dir() accessors, and discover installed_dir with SkillTrust::Installed in discover_all() - All install paths (web handler, skill tool) use install_target_dir() instead of user_dir() so new installs land in the correct directory - 3 new registry tests: test_installed_dir_uses_installed_trust, test_install_target_dir_prefers_installed_dir, test_user_dir_stays_trusted_with_installed_dir Duplicate handler cleanup: handlers/skills.rs was the canonical implementation but the handlers module was never compiled (not declared in web/mod.rs), so server.rs had its own duplicate inline definitions that the router used. Wire up the handlers module, delete the 260-line duplicate in server.rs, and have server.rs import skills handlers from handlers::skills. Fix pre-existing compile error in handlers/extensions.rs (missing needs_setup field). Add #[allow(dead_code)] on not-yet-migrated handler modules to suppress warnings. Co-Authored-By: Claude Opus 4.6 <[email protected]> * fix: probe more Docker socket paths on macOS Docker Desktop 4.13+ (stabilised in 4.18) no longer creates the /var/run/docker.sock symlink by default. The API socket lives at ~/.docker/run/docker.sock, which bollard's connect_with_local_defaults() does not try. Add a fallback probe list covering the common macOS container runtimes: - ~/.docker/run/docker.sock — Docker Desktop 4.13+ - ~/.colima/default/docker.sock — Colima - ~/.rd/docker.sock — Rancher Desktop Remove the bogus ~/.docker/desktop/docker.sock path that was added previously; it is not an API socket on any known Docker installation. Fixes the false-negative "Docker is installed but not running" warning reported by Illia on macOS with Docker Desktop 4.18+. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Harden Docker detection for rootless Linux and Windows fallback --------- Co-authored-by: Claude Opus 4.6 <[email protected]>
This commit is contained in:
Zaki Manian
co-authored by
Claude Opus 4.6
Claude Opus 4.6
parent
f4ba85ffa2
commit
4e2dd76ae5
@@ -28,7 +28,7 @@ pub struct SandboxConfig {
|
||||
impl Default for SandboxConfig {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
enabled: false, // Disabled by default until Docker is confirmed available
|
||||
enabled: true, // Startup check disables gracefully if Docker unavailable
|
||||
policy: SandboxPolicy::ReadOnly,
|
||||
timeout: Duration::from_secs(120),
|
||||
memory_limit_mb: 2048,
|
||||
|
||||
+83
-15
@@ -26,7 +26,7 @@
|
||||
//! ```
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
use std::path::{Path, PathBuf};
|
||||
use std::time::Duration;
|
||||
|
||||
use bollard::Docker;
|
||||
@@ -490,40 +490,108 @@ impl ContainerRunner {
|
||||
///
|
||||
/// Tries these locations in order:
|
||||
/// 1. `DOCKER_HOST` env var (bollard default)
|
||||
/// 2. `/var/run/docker.sock` (Linux default)
|
||||
/// 3. `~/.docker/run/docker.sock` (Docker Desktop on macOS)
|
||||
/// 2. `/var/run/docker.sock` (Linux default; also used by OrbStack and Podman Desktop on macOS)
|
||||
/// 3. `~/.docker/run/docker.sock` (Docker Desktop 4.13+ on macOS — primary user-owned socket)
|
||||
/// 4. `~/.colima/default/docker.sock` (Colima — popular lightweight Docker Desktop alternative)
|
||||
/// 5. `~/.rd/docker.sock` (Rancher Desktop on macOS)
|
||||
/// 6. `$XDG_RUNTIME_DIR/docker.sock` (common rootless Docker socket on Linux)
|
||||
/// 7. `/run/user/$UID/docker.sock` (rootless Docker fallback on Linux)
|
||||
pub async fn connect_docker() -> Result<Docker> {
|
||||
// First try bollard defaults (checks DOCKER_HOST, then /var/run/docker.sock)
|
||||
// First try bollard defaults (checks DOCKER_HOST env var, then /var/run/docker.sock).
|
||||
// This covers Linux, OrbStack (updates the /var/run symlink), and any user with
|
||||
// DOCKER_HOST set to their runtime's socket.
|
||||
if let Ok(docker) = Docker::connect_with_local_defaults()
|
||||
&& docker.ping().await.is_ok()
|
||||
{
|
||||
return Ok(docker);
|
||||
}
|
||||
|
||||
// Try Docker Desktop socket (macOS)
|
||||
if let Some(home) = std::env::var_os("HOME") {
|
||||
let desktop_sock = std::path::Path::new(&home).join(".docker/run/docker.sock");
|
||||
if desktop_sock.exists() {
|
||||
let sock_str = desktop_sock.to_string_lossy();
|
||||
if let Ok(docker) =
|
||||
Docker::connect_with_socket(&sock_str, 120, bollard::API_DEFAULT_VERSION)
|
||||
&& docker.ping().await.is_ok()
|
||||
{
|
||||
return Ok(docker);
|
||||
#[cfg(unix)]
|
||||
{
|
||||
// Try well-known user-owned socket locations for desktop and rootless runtimes.
|
||||
// Docker Desktop 4.13+ (stabilised in 4.18) stopped creating the
|
||||
// /var/run/docker.sock symlink by default and moved the API socket
|
||||
// to ~/.docker/run/docker.sock.
|
||||
for sock in unix_socket_candidates() {
|
||||
if sock.exists() {
|
||||
let sock_str = sock.to_string_lossy();
|
||||
if let Ok(docker) =
|
||||
Docker::connect_with_socket(&sock_str, 120, bollard::API_DEFAULT_VERSION)
|
||||
&& docker.ping().await.is_ok()
|
||||
{
|
||||
return Ok(docker);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Err(SandboxError::DockerNotAvailable {
|
||||
reason: "Could not connect to Docker. Tried: default socket, ~/.docker/run/docker.sock"
|
||||
reason: "Could not connect to Docker daemon. Tried: $DOCKER_HOST, \
|
||||
/var/run/docker.sock, ~/.docker/run/docker.sock, \
|
||||
~/.colima/default/docker.sock, ~/.rd/docker.sock, \
|
||||
$XDG_RUNTIME_DIR/docker.sock, /run/user/$UID/docker.sock"
|
||||
.to_string(),
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn unix_socket_candidates() -> Vec<PathBuf> {
|
||||
unix_socket_candidates_from_env(
|
||||
std::env::var_os("HOME").map(PathBuf::from),
|
||||
std::env::var_os("XDG_RUNTIME_DIR").map(PathBuf::from),
|
||||
std::env::var("UID").ok(),
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
fn unix_socket_candidates_from_env(
|
||||
home: Option<PathBuf>,
|
||||
xdg_runtime_dir: Option<PathBuf>,
|
||||
uid: Option<String>,
|
||||
) -> Vec<PathBuf> {
|
||||
let mut candidates = Vec::new();
|
||||
let mut push_unique = |path: PathBuf| {
|
||||
if !candidates.iter().any(|existing| existing == &path) {
|
||||
candidates.push(path);
|
||||
}
|
||||
};
|
||||
|
||||
if let Some(home) = home {
|
||||
push_unique(home.join(".docker/run/docker.sock")); // Docker Desktop 4.13+
|
||||
push_unique(home.join(".colima/default/docker.sock")); // Colima
|
||||
push_unique(home.join(".rd/docker.sock")); // Rancher Desktop
|
||||
}
|
||||
|
||||
if let Some(xdg_runtime_dir) = xdg_runtime_dir {
|
||||
push_unique(xdg_runtime_dir.join("docker.sock"));
|
||||
}
|
||||
|
||||
if let Some(uid) = uid.filter(|value| !value.is_empty()) {
|
||||
push_unique(PathBuf::from(format!("/run/user/{uid}/docker.sock")));
|
||||
}
|
||||
|
||||
candidates
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn test_unix_socket_candidates_include_rootless_paths() {
|
||||
let candidates = unix_socket_candidates_from_env(
|
||||
Some(PathBuf::from("/home/tester")),
|
||||
Some(PathBuf::from("/run/user/1000")),
|
||||
Some("1000".to_string()),
|
||||
);
|
||||
|
||||
assert!(candidates.contains(&PathBuf::from("/home/tester/.docker/run/docker.sock")));
|
||||
assert!(candidates.contains(&PathBuf::from("/home/tester/.colima/default/docker.sock")));
|
||||
assert!(candidates.contains(&PathBuf::from("/home/tester/.rd/docker.sock")));
|
||||
assert!(candidates.contains(&PathBuf::from("/run/user/1000/docker.sock")));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_docker_connection() {
|
||||
// This test requires Docker to be running
|
||||
|
||||
@@ -0,0 +1,233 @@
|
||||
//! Proactive Docker detection with platform-specific guidance.
|
||||
//!
|
||||
//! Checks whether Docker is both installed (binary on PATH) and running
|
||||
//! (daemon responding to ping), and provides platform-appropriate
|
||||
//! installation or startup instructions when it is not.
|
||||
//!
|
||||
//! # Detection Limitations
|
||||
//!
|
||||
//! - **macOS**: High confidence. Detects both standard Docker Desktop socket
|
||||
//! (`~/.docker/run/docker.sock`) and the default `/var/run/docker.sock`.
|
||||
//!
|
||||
//! - **Linux**: High confidence for standard installs. Rootless Docker uses
|
||||
//! a different socket path (`/run/user/$UID/docker.sock`) which is now
|
||||
//! checked by the fallback in `connect_docker()`. If `DOCKER_HOST` is set,
|
||||
//! bollard's default connection still takes precedence.
|
||||
//!
|
||||
//! - **Windows**: Medium confidence. Binary detection uses `where.exe` which
|
||||
//! works reliably. Daemon detection relies on bollard's default named pipe
|
||||
//! connection (`//./pipe/docker_engine`) which works with Docker Desktop.
|
||||
//! The Unix socket fallback in `connect_docker()` is a no-op on Windows,
|
||||
//! so detection also probes `docker version`/`docker info` via CLI if the
|
||||
//! named pipe is unavailable.
|
||||
|
||||
/// Docker daemon availability status.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum DockerStatus {
|
||||
/// Docker binary found on PATH and daemon responding to ping.
|
||||
Available,
|
||||
/// `docker` binary not found on PATH.
|
||||
NotInstalled,
|
||||
/// Binary found but daemon not responding.
|
||||
NotRunning,
|
||||
/// Sandbox feature not enabled (no check performed).
|
||||
Disabled,
|
||||
}
|
||||
|
||||
impl DockerStatus {
|
||||
/// Returns true if Docker is available and ready.
|
||||
pub fn is_ok(&self) -> bool {
|
||||
matches!(self, DockerStatus::Available)
|
||||
}
|
||||
|
||||
/// Human-readable status string.
|
||||
pub fn as_str(&self) -> &'static str {
|
||||
match self {
|
||||
DockerStatus::Available => "available",
|
||||
DockerStatus::NotInstalled => "not installed",
|
||||
DockerStatus::NotRunning => "not running",
|
||||
DockerStatus::Disabled => "disabled",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Host platform for install guidance.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum Platform {
|
||||
MacOS,
|
||||
Linux,
|
||||
Windows,
|
||||
}
|
||||
|
||||
impl Platform {
|
||||
/// Detect the current platform.
|
||||
pub fn current() -> Self {
|
||||
match std::env::consts::OS {
|
||||
"macos" => Platform::MacOS,
|
||||
"windows" => Platform::Windows,
|
||||
_ => Platform::Linux,
|
||||
}
|
||||
}
|
||||
|
||||
/// Installation instructions for Docker on this platform.
|
||||
pub fn install_hint(&self) -> &'static str {
|
||||
match self {
|
||||
Platform::MacOS => {
|
||||
"Install Docker Desktop: https://docs.docker.com/desktop/install/mac-install/"
|
||||
}
|
||||
Platform::Linux => "Install Docker Engine: https://docs.docker.com/engine/install/",
|
||||
Platform::Windows => {
|
||||
"Install Docker Desktop: https://docs.docker.com/desktop/install/windows-install/"
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Instructions to start the Docker daemon on this platform.
|
||||
pub fn start_hint(&self) -> &'static str {
|
||||
match self {
|
||||
Platform::MacOS => "Start Docker Desktop from Applications, or run: open -a Docker",
|
||||
Platform::Linux => "Start the Docker daemon: sudo systemctl start docker",
|
||||
Platform::Windows => "Start Docker Desktop from the Start menu",
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Result of a Docker detection check.
|
||||
pub struct DockerDetection {
|
||||
pub status: DockerStatus,
|
||||
pub platform: Platform,
|
||||
}
|
||||
|
||||
/// Check whether Docker is installed and running.
|
||||
///
|
||||
/// 1. Checks if `docker` binary exists on PATH
|
||||
/// 2. If found, tries to connect and ping the Docker daemon via `connect_docker()`
|
||||
/// 3. Returns `Available`, `NotInstalled`, or `NotRunning`
|
||||
pub async fn check_docker() -> DockerDetection {
|
||||
let platform = Platform::current();
|
||||
|
||||
// Step 1: Check if docker binary is on PATH
|
||||
if !docker_binary_exists() {
|
||||
return DockerDetection {
|
||||
status: DockerStatus::NotInstalled,
|
||||
platform,
|
||||
};
|
||||
}
|
||||
|
||||
// Step 2: Try to connect to the daemon
|
||||
if crate::sandbox::connect_docker().await.is_ok() {
|
||||
return DockerDetection {
|
||||
status: DockerStatus::Available,
|
||||
platform,
|
||||
};
|
||||
}
|
||||
|
||||
// Windows fallback: if the named pipe probe fails but docker CLI can still
|
||||
// reach the daemon/server, treat Docker as available.
|
||||
#[cfg(windows)]
|
||||
if docker_cli_daemon_reachable() {
|
||||
return DockerDetection {
|
||||
status: DockerStatus::Available,
|
||||
platform,
|
||||
};
|
||||
}
|
||||
|
||||
DockerDetection {
|
||||
status: DockerStatus::NotRunning,
|
||||
platform,
|
||||
}
|
||||
}
|
||||
|
||||
/// Check if the `docker` binary exists on PATH.
|
||||
fn docker_binary_exists() -> bool {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
std::process::Command::new("which")
|
||||
.arg("docker")
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.is_ok_and(|s| s.success())
|
||||
}
|
||||
#[cfg(windows)]
|
||||
{
|
||||
std::process::Command::new("where")
|
||||
.arg("docker")
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.is_ok_and(|s| s.success())
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
fn docker_cli_daemon_reachable() -> bool {
|
||||
let stdout = std::process::Stdio::null();
|
||||
let stderr = std::process::Stdio::null();
|
||||
|
||||
// `docker version` requires daemon reachability for server fields.
|
||||
let version_ok = std::process::Command::new("docker")
|
||||
.args(["version", "--format", "{{.Server.Version}}"])
|
||||
.stdout(stdout)
|
||||
.stderr(stderr)
|
||||
.status()
|
||||
.is_ok_and(|s| s.success());
|
||||
|
||||
if version_ok {
|
||||
return true;
|
||||
}
|
||||
|
||||
// Fallback for environments where `docker version --format` behaves differently.
|
||||
std::process::Command::new("docker")
|
||||
.args(["info", "--format", "{{.ServerVersion}}"])
|
||||
.stdout(std::process::Stdio::null())
|
||||
.stderr(std::process::Stdio::null())
|
||||
.status()
|
||||
.is_ok_and(|s| s.success())
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn test_detect_platform() {
|
||||
let platform = Platform::current();
|
||||
match platform {
|
||||
Platform::MacOS | Platform::Linux | Platform::Windows => {}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_install_hint_not_empty() {
|
||||
for platform in [Platform::MacOS, Platform::Linux, Platform::Windows] {
|
||||
assert!(!platform.install_hint().is_empty());
|
||||
assert!(!platform.start_hint().is_empty());
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_docker_status_display() {
|
||||
assert_eq!(DockerStatus::Available.as_str(), "available");
|
||||
assert_eq!(DockerStatus::NotInstalled.as_str(), "not installed");
|
||||
assert_eq!(DockerStatus::NotRunning.as_str(), "not running");
|
||||
assert_eq!(DockerStatus::Disabled.as_str(), "disabled");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_docker_status_is_ok() {
|
||||
assert!(DockerStatus::Available.is_ok());
|
||||
assert!(!DockerStatus::NotInstalled.is_ok());
|
||||
assert!(!DockerStatus::NotRunning.is_ok());
|
||||
assert!(!DockerStatus::Disabled.is_ok());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn test_check_docker_returns_valid_status() {
|
||||
let result = check_docker().await;
|
||||
match result.status {
|
||||
DockerStatus::Available | DockerStatus::NotInstalled | DockerStatus::NotRunning => {}
|
||||
DockerStatus::Disabled => panic!("check_docker should never return Disabled"),
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -460,7 +460,7 @@ mod tests {
|
||||
#[test]
|
||||
fn test_builder_defaults() {
|
||||
let manager = SandboxManagerBuilder::new().build();
|
||||
assert!(!manager.config.enabled); // Disabled by default
|
||||
assert!(manager.config.enabled); // Enabled by default (startup check disables if Docker unavailable)
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -87,12 +87,14 @@
|
||||
|
||||
pub mod config;
|
||||
pub mod container;
|
||||
pub mod detect;
|
||||
pub mod error;
|
||||
pub mod manager;
|
||||
pub mod proxy;
|
||||
|
||||
pub use config::{ResourceLimits, SandboxConfig, SandboxPolicy};
|
||||
pub use container::{ContainerOutput, ContainerRunner, connect_docker};
|
||||
pub use detect::{DockerDetection, DockerStatus, Platform, check_docker};
|
||||
pub use error::{Result, SandboxError};
|
||||
pub use manager::{ExecOutput, SandboxManager, SandboxManagerBuilder};
|
||||
pub use proxy::{
|
||||
|
||||
Reference in New Issue
Block a user