Fix Python runtime PATH for skill shell commands (#3483)

src/openhuman/runtime_python/bootstrap.rs

@@ -27,6 +27,9 @@ pub enum PythonSource {
 /// Fully-resolved Python interpreter.
 #[derive(Debug, Clone)]
 pub struct ResolvedPython {
+    /// Directory that should be prepended to `PATH` for child processes so
+    /// `python`, `python3`, `pip`, and `pip3` resolve to the same toolchain.
+    pub bin_dir: std::path::PathBuf,
     /// Absolute path to the Python executable.
     pub python_bin: std::path::PathBuf,
     /// Normalized interpreter version, e.g. `3.12.4`.
@@ -187,6 +190,7 @@ fn resolve_from_system(system: SystemPython) -> ResolvedPython {
         "[runtime_python::bootstrap] reusing compatible system python"
     );
     ResolvedPython {
+        bin_dir: python_bin_dir(&system.path),
         python_bin: system.path,
         version: system.version,
         source: PythonSource::System,
@@ -207,12 +211,20 @@ fn probe_managed_install(install_dir: &Path) -> Option<ResolvedPython> {
     let version = super::resolver::probe_python_version_public(&python_bin)?;
     let version_info = super::resolver::parse_python_version(&version)?;
     Some(ResolvedPython {
+        bin_dir: python_bin_dir(&python_bin),
         python_bin,
         version: version_info.display(),
         source: PythonSource::Managed,
     })
 }
 
+fn python_bin_dir(python_bin: &Path) -> PathBuf {
+    python_bin
+        .parent()
+        .map(Path::to_path_buf)
+        .unwrap_or_else(|| PathBuf::from("."))
+}
+
 fn find_python_binary(install_dir: &Path) -> Option<PathBuf> {
     let candidates = [
         install_dir.join("bin").join("python3.12"),

src/openhuman/tools/impl/system/shell.rs

@@ -1,5 +1,6 @@
 use crate::openhuman::agent::host_runtime::RuntimeAdapter;
 use crate::openhuman::javascript::NodeBootstrap;
+use crate::openhuman::runtime_python::PythonBootstrap;
 use crate::openhuman::security::{AuditLogger, CommandExecutionLog, GateDecision, SecurityPolicy};
 use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolResult};
 use async_trait::async_trait;
@@ -50,6 +51,12 @@ pub struct ShellTool {
     /// toolchain. Non-blocking: never triggers a download for unrelated
     /// commands (we use `try_cached()`).
     node_bootstrap: Option<Arc<NodeBootstrap>>,
+    /// Optional managed Python bootstrap. Unlike Node PATH injection, Python
+    /// shell support is the primary execution surface for skills, so
+    /// Python-looking commands resolve this lazily before spawn. That keeps
+    /// `pip install foo` and `python3 -m foo` on one interpreter instead of
+    /// mixing arbitrary host `pip` and `python3` binaries.
+    python_bootstrap: Option<Arc<PythonBootstrap>>,
 }
 
 impl ShellTool {
@@ -63,6 +70,7 @@ impl ShellTool {
             runtime,
             audit,
             node_bootstrap: None,
+            python_bootstrap: None,
         }
     }
 
@@ -80,6 +88,27 @@ impl ShellTool {
             runtime,
             audit,
             node_bootstrap: Some(bootstrap),
+            python_bootstrap: None,
+        }
+    }
+
+    /// Attach managed language runtimes used by shell-invoked skills. Node is
+    /// injected only after a dedicated node/npm tool resolved it; Python is
+    /// resolved lazily for python/pip commands because shell is currently the
+    /// user-facing Python skill execution path.
+    pub fn with_language_bootstraps(
+        security: Arc<SecurityPolicy>,
+        runtime: Arc<dyn RuntimeAdapter>,
+        audit: Arc<AuditLogger>,
+        node_bootstrap: Option<Arc<NodeBootstrap>>,
+        python_bootstrap: Option<Arc<PythonBootstrap>>,
+    ) -> Self {
+        Self {
+            security,
+            runtime,
+            audit,
+            node_bootstrap,
+            python_bootstrap,
         }
     }
 
@@ -264,25 +293,17 @@ impl ShellTool {
             }
         }
 
-        // If a managed Node.js install has already been resolved, transparently
-        // prepend its bin dir to PATH so this shell sees the managed toolchain.
-        // `try_cached()` never blocks and never triggers a download — unrelated
-        // commands (e.g. `ls`) stay fast and byte-identical to before.
-        if let Some(bootstrap) = self.node_bootstrap.as_ref() {
-            if let Some(resolved) = bootstrap.try_cached() {
-                let host_path = std::env::var("PATH").unwrap_or_default();
-                let sep = if cfg!(windows) { ";" } else { ":" };
-                let prepended = if host_path.is_empty() {
-                    resolved.bin_dir.to_string_lossy().into_owned()
-                } else {
-                    format!("{}{}{}", resolved.bin_dir.display(), sep, host_path)
-                };
-                tracing::debug!(
-                    bin_dir = %resolved.bin_dir.display(),
-                    version = %resolved.version,
-                    "[shell] prepending managed node bin to PATH"
+        match self.runtime_path_for_command(command).await {
+            Ok(Some(path)) => {
+                tracing::debug!(path = %path, "[shell] applying managed runtime PATH");
+                cmd.env("PATH", path);
+            }
+            Ok(None) => {}
+            Err(error) => {
+                return (
+                    true,
+                    ToolResult::error(format!("Failed to resolve command runtime: {error}")),
                 );
-                cmd.env("PATH", prepended);
             }
         }
 
@@ -350,16 +371,16 @@ impl ShellTool {
         );
 
         let mut extra_env = std::collections::HashMap::new();
-        if let Some(bootstrap) = self.node_bootstrap.as_ref() {
-            if let Some(resolved) = bootstrap.try_cached() {
-                let host_path = std::env::var("PATH").unwrap_or_default();
-                let sep = if cfg!(windows) { ";" } else { ":" };
-                let prepended = if host_path.is_empty() {
-                    resolved.bin_dir.to_string_lossy().into_owned()
-                } else {
-                    format!("{}{}{}", resolved.bin_dir.display(), sep, host_path)
-                };
-                extra_env.insert("PATH".to_string(), prepended);
+        match self.runtime_path_for_command(command).await {
+            Ok(Some(path)) => {
+                extra_env.insert("PATH".to_string(), path);
+            }
+            Ok(None) => {}
+            Err(error) => {
+                return (
+                    true,
+                    ToolResult::error(format!("Failed to resolve command runtime: {error}")),
+                );
             }
         }
 
@@ -402,6 +423,109 @@ impl ShellTool {
             ),
         }
     }
+
+    async fn runtime_path_for_command(&self, command: &str) -> anyhow::Result<Option<String>> {
+        let mut prepend_dirs = Vec::new();
+
+        // Node injection preserves the existing contract: shell only sees the
+        // managed Node bin directory after a previous node/npm tool resolved it.
+        if let Some(bootstrap) = self.node_bootstrap.as_ref() {
+            if let Some(resolved) = bootstrap.try_cached() {
+                tracing::debug!(
+                    bin_dir = %resolved.bin_dir.display(),
+                    version = %resolved.version,
+                    "[shell] prepending managed node bin to PATH"
+                );
+                prepend_dirs.push(resolved.bin_dir);
+            }
+        }
+
+        if shell_command_needs_python_runtime(command) {
+            if let Some(bootstrap) = self.python_bootstrap.as_ref() {
+                let resolved = bootstrap.resolve().await?;
+                tracing::debug!(
+                    bin_dir = %resolved.bin_dir.display(),
+                    python_bin = %resolved.python_bin.display(),
+                    version = %resolved.version,
+                    source = ?resolved.source,
+                    "[shell] prepending python runtime bin to PATH"
+                );
+                prepend_dirs.push(resolved.bin_dir);
+            }
+        }
+
+        if prepend_dirs.is_empty() {
+            Ok(None)
+        } else {
+            Ok(Some(prepend_path_dirs(
+                prepend_dirs.iter().map(|p| p.as_path()),
+                &std::env::var("PATH").unwrap_or_default(),
+            )))
+        }
+    }
+}
+
+fn prepend_path_dirs<'a>(
+    dirs: impl IntoIterator<Item = &'a std::path::Path>,
+    host_path: &str,
+) -> String {
+    let sep = if cfg!(windows) { ";" } else { ":" };
+    let mut parts: Vec<String> = dirs
+        .into_iter()
+        .map(|path| path.to_string_lossy().into_owned())
+        .collect();
+    if !host_path.is_empty() {
+        parts.push(host_path.to_string());
+    }
+    parts.join(sep)
+}
+
+fn shell_command_needs_python_runtime(command: &str) -> bool {
+    let lower = command.to_ascii_lowercase();
+    lower
+        .split(|ch| matches!(ch, ';' | '&' | '|' | '\n' | '\r'))
+        .any(segment_starts_with_python_command)
+}
+
+fn segment_starts_with_python_command(segment: &str) -> bool {
+    let mut tokens = segment.split_whitespace().peekable();
+    while let Some(token) = tokens.next() {
+        let token = token.trim_matches(|ch| matches!(ch, '(' | ')' | '<' | '>'));
+        if token.is_empty() {
+            continue;
+        }
+        if token.contains('=') && !token.starts_with('-') {
+            continue;
+        }
+        if matches!(token, "sudo" | "command" | "time" | "env") {
+            continue;
+        }
+        return is_python_executable_token(token);
+    }
+    false
+}
+
+fn is_python_executable_token(token: &str) -> bool {
+    let executable = token.rsplit('/').next().unwrap_or(token);
+    matches!(
+        executable,
+        "python"
+            | "python3"
+            | "py"
+            | "pip"
+            | "pip3"
+            | "python.exe"
+            | "python3.exe"
+            | "pip.exe"
+            | "pip3.exe"
+    ) || versioned_executable(executable, "python3.")
+        || versioned_executable(executable, "pip3.")
+}
+
+fn versioned_executable(executable: &str, prefix: &str) -> bool {
+    executable
+        .strip_prefix(prefix)
+        .is_some_and(|suffix| !suffix.is_empty() && suffix.chars().all(|c| c.is_ascii_digit()))
 }
 
 #[cfg(test)]
@@ -894,6 +1018,54 @@ mod tests {
         }
     }
 
+    #[test]
+    fn shell_detects_python_runtime_commands() {
+        for command in [
+            "python3 -m pyfiglet hello",
+            "python -m pip install pyfiglet",
+            "pip install pyfiglet",
+            "pip3.13 show pyfiglet",
+            "/opt/openhuman/python/bin/python3 script.py",
+            "echo hi && python3 -V",
+        ] {
+            assert!(
+                shell_command_needs_python_runtime(command),
+                "expected python runtime detection for {command}"
+            );
+        }
+
+        for command in [
+            "echo python3",
+            "ls",
+            "cat ./pipelines.txt",
+            "node script.js",
+        ] {
+            assert!(
+                !shell_command_needs_python_runtime(command),
+                "did not expect python runtime detection for {command}"
+            );
+        }
+    }
+
+    #[test]
+    fn shell_runtime_path_prepends_managed_dirs_before_host_path() {
+        let python = std::path::Path::new("/opt/openhuman/python/bin");
+        let node = std::path::Path::new("/opt/openhuman/node/bin");
+        let joined = prepend_path_dirs([python, node], "/usr/local/bin:/usr/bin");
+        let sep = if cfg!(windows) { ";" } else { ":" };
+        assert_eq!(
+            joined,
+            format!(
+                "{}{}{}{}{}",
+                python.display(),
+                sep,
+                node.display(),
+                sep,
+                "/usr/local/bin:/usr/bin"
+            )
+        );
+    }
+
     #[tokio::test]
     async fn shell_blocks_rate_limited() {
         let security = Arc::new(SecurityPolicy {

src/openhuman/tools/ops.rs

@@ -4,6 +4,7 @@ use crate::openhuman::agent::host_runtime::{NativeRuntime, RuntimeAdapter};
 use crate::openhuman::config::{Config, DelegateAgentConfig};
 use crate::openhuman::javascript::NodeBootstrap;
 use crate::openhuman::memory::Memory;
+use crate::openhuman::runtime_python::PythonBootstrap;
 use crate::openhuman::security::{AuditLogger, SecurityPolicy};
 use std::collections::HashMap;
 use std::sync::Arc;
@@ -113,22 +114,30 @@ pub fn all_tools_with_runtime(
         );
         None
     };
-
-    let shell: Box<dyn Tool> = if let Some(bootstrap) = node_bootstrap.as_ref() {
-        Box::new(ShellTool::with_node_bootstrap(
-            security.clone(),
-            Arc::clone(&runtime),
-            Arc::clone(&audit),
-            Arc::clone(bootstrap),
-        ))
+    let python_bootstrap: Option<Arc<PythonBootstrap>> = if root_config.runtime_python.enabled {
+        tracing::debug!(
+            minimum_version = %root_config.runtime_python.minimum_version,
+            prefer_system = root_config.runtime_python.prefer_system,
+            "[tools::ops] python runtime enabled — constructing shared PythonBootstrap"
+        );
+        Some(Arc::new(PythonBootstrap::new(
+            root_config.runtime_python.clone(),
+        )))
     } else {
-        Box::new(ShellTool::new(
-            security.clone(),
-            Arc::clone(&runtime),
-            Arc::clone(&audit),
-        ))
+        tracing::debug!(
+            "[tools::ops] python runtime disabled — shell python/pip PATH injection suppressed"
+        );
+        None
     };
 
+    let shell: Box<dyn Tool> = Box::new(ShellTool::with_language_bootstraps(
+        security.clone(),
+        Arc::clone(&runtime),
+        Arc::clone(&audit),
+        node_bootstrap.as_ref().map(Arc::clone),
+        python_bootstrap.as_ref().map(Arc::clone),
+    ));
+
     let mut tools: Vec<Box<dyn Tool>> = vec![
         shell,
         Box::new(FileReadTool::new(security.clone())),