fix(cli): address workspace review feedback

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

Author: matthewgrossman

.agents/skills/openshell-cli/SKILL.md

@@ -223,17 +223,19 @@ openshell sandbox ssh-config my-sandbox >> ~/.ssh/config
 # Upload local files to the sandbox working directory
 openshell sandbox upload my-sandbox ./src
 
-# Download files from sandbox (replace /workspace with the path from `pwd -P`)
-openshell sandbox download my-sandbox /workspace/output ./local-output
+# Download a path relative to the sandbox working directory
+openshell sandbox download my-sandbox output ./local-output
 ```
 
 Uploads honor `.gitignore` by default. Add `--no-git-ignore` only when ignored files are intentionally in scope.
 
 Uploads preserve symlinks, including dangling symlinks, instead of dereferencing their targets. A symlink source bypasses Git-aware filtering so the link itself is archived.
 
 When the upload destination is omitted, the CLI discovers the remote working
-directory. Downloads must use an absolute sandbox path within that same
-canonical working directory.
+directory. Uploading a named directory merges it into an existing directory of
+the same name, overwriting matching entries without deleting unrelated entries.
+Downloads accept paths relative to that working directory or absolute paths
+within it.
 
 ### Execute a non-interactive command
 

.agents/skills/openshell-cli/cli-reference.md

@@ -258,11 +258,11 @@ Open an interactive SSH shell. The name defaults to the last-used sandbox. `--ed
 
 ### `openshell sandbox upload <name> <path> [dest]`
 
-Upload files using tar-over-SSH. The CLI discovers the canonical remote working directory when the destination is omitted. `.gitignore` filtering is enabled unless `--no-git-ignore` is passed.
+Upload files using tar-over-SSH. The CLI discovers the canonical remote working directory when the destination is omitted. A named directory merges into an existing directory of the same name, overwriting matching entries without deleting unrelated entries. `.gitignore` filtering is enabled unless `--no-git-ignore` is passed.
 
 ### `openshell sandbox download <name> <path> [dest]`
 
-Download files using tar-over-SSH. The sandbox source must be an absolute path within the canonical remote working directory. The local destination defaults to `.`.
+Download files using tar-over-SSH. The sandbox source may be relative to the canonical remote working directory or an absolute path within it. The local destination defaults to `.`.
 
 ### `openshell sandbox ssh-config [name]`
 

crates/openshell-cli/src/ssh.rs

@@ -7,7 +7,6 @@ use crate::tls::{TlsOptions, grpc_client};
 use miette::{IntoDiagnostic, Result, WrapErr};
 #[cfg(unix)]
 use nix::sys::signal::{SaFlags, SigAction, SigHandler, SigSet, Signal, sigaction};
-use openshell_core::ObjectId;
 use openshell_core::forward::{
     ForwardSpec, build_proxy_command, format_gateway_url, resolve_ssh_gateway, shell_escape,
     validate_ssh_session_response, write_forward_pid,
@@ -16,6 +15,7 @@ use openshell_core::proto::{
     CreateSshSessionRequest, GetSandboxRequest, SshRelayTarget, TcpForwardFrame, TcpForwardInit,
     tcp_forward_init,
 };
+use openshell_core::{ObjectId, driver_mounts};
 use owo_colors::OwoColorize;
 use std::fs;
 use std::future::Future;
@@ -867,36 +867,36 @@ fn lexical_clean_absolute_path(path: &str) -> Option<String> {
     Some(out)
 }
 
-/// Validate that a sandbox-side source path passed to `sandbox download`
-/// resolves under the sandbox writable root.
+/// Resolve a sandbox-side source path passed to `sandbox download` under the
+/// sandbox writable root.
 ///
 /// Returns the cleaned, traversal-resolved path on success. Refuses any
-/// path that lexically escapes the discovered workspace root with a
-/// user-facing error.
+/// path that lexically escapes the discovered workspace root with a user-facing
+/// error. Relative paths are interpreted from the workspace root.
 ///
 /// This is a lexical guard only — it does not follow symlinks. Call
 /// `resolve_sandbox_source_path` after this on any path that will be passed
 /// to a subsequent SSH I/O operation, so a workspace symlink to `/etc` cannot
 /// leak files outside the workspace.
-fn validate_sandbox_source_path(path: &str, workspace_root: &str) -> Result<String> {
+fn validate_sandbox_source_path(workspace_root: &str, path: &str) -> Result<String> {
     if path.is_empty() {
         return Err(miette::miette!("sandbox source path is empty"));
     }
-    let cleaned = lexical_clean_absolute_path(path)
-        .ok_or_else(|| miette::miette!("sandbox source path must be absolute (got '{path}')"))?;
-    if !is_under_workspace(&cleaned, workspace_root) {
+    let candidate = if path.starts_with('/') {
+        path.to_string()
+    } else {
+        format!("{workspace_root}/{path}")
+    };
+    let cleaned = lexical_clean_absolute_path(&candidate)
+        .ok_or_else(|| miette::miette!("sandbox source path is invalid (got '{path}')"))?;
+    if !driver_mounts::path_is_or_under(Path::new(&cleaned), Path::new(workspace_root)) {
         return Err(miette::miette!(
             "sandbox source path '{path}' is outside the sandbox workspace ({workspace_root})"
         ));
     }
     Ok(cleaned)
 }
 
-/// Pure helper: is `path` equal to the workspace root or a descendant of it?
-fn is_under_workspace(path: &str, workspace_root: &str) -> bool {
-    path == workspace_root || path.starts_with(&format!("{workspace_root}/"))
-}
-
 /// Discover the workspace root and resolve every symlink in `sandbox_path` in
 /// one SSH probe, then refuse the result if it lands outside the workspace.
 ///
@@ -928,13 +928,13 @@ async fn resolve_sandbox_source_path(
     }
 
     let workspace_root = validate_discovered_workspace_root(workspace_root)?;
-    validate_sandbox_source_path(sandbox_path, &workspace_root)?;
+    validate_sandbox_source_path(&workspace_root, sandbox_path)?;
     if resolved.is_empty() {
         return Err(miette::miette!(
             "sandbox source path '{sandbox_path}' does not exist"
         ));
     }
-    if !is_under_workspace(resolved, &workspace_root) {
+    if !driver_mounts::path_is_or_under(Path::new(resolved), Path::new(&workspace_root)) {
         return Err(miette::miette!(
             "sandbox source path '{sandbox_path}' resolves to '{resolved}', outside the sandbox workspace ({workspace_root})"
         ));
@@ -1199,8 +1199,8 @@ async fn probe_sandbox_source_kind(
 ///   behaviour for the directory-source case.
 ///
 /// The sandbox source path is also subjected to a workspace-boundary check
-/// before any SSH command is issued; paths that lexically resolve outside
-/// the discovered workspace root are refused.
+/// before any file probe or archive command is issued; paths that resolve
+/// outside the discovered workspace root are refused.
 pub async fn sandbox_sync_down(
     server: &str,
     name: &str,
@@ -2006,89 +2006,70 @@ mod tests {
     fn validate_sandbox_source_path_accepts_workspace_paths() {
         let workspace_root = "/workspace/project";
         assert_eq!(
-            validate_sandbox_source_path("/workspace/project/file.txt", workspace_root).unwrap(),
+            validate_sandbox_source_path(workspace_root, "/workspace/project/file.txt").unwrap(),
             "/workspace/project/file.txt"
         );
         assert_eq!(
             validate_sandbox_source_path(
-                "/workspace/project/.agent/workspace/hello.txt",
-                workspace_root
+                workspace_root,
+                "/workspace/project/.agent/workspace/hello.txt"
             )
             .unwrap(),
             "/workspace/project/.agent/workspace/hello.txt"
         );
         assert_eq!(
-            validate_sandbox_source_path("/workspace/project", workspace_root).unwrap(),
+            validate_sandbox_source_path(workspace_root, "/workspace/project").unwrap(),
             "/workspace/project"
         );
         assert_eq!(
-            validate_sandbox_source_path("/workspace/project/", workspace_root).unwrap(),
+            validate_sandbox_source_path(workspace_root, "/workspace/project/").unwrap(),
             "/workspace/project"
         );
         assert_eq!(
-            validate_sandbox_source_path("/workspace/project/sub/../file", workspace_root).unwrap(),
+            validate_sandbox_source_path(workspace_root, "/workspace/project/sub/../file").unwrap(),
             "/workspace/project/file"
         );
+        assert_eq!(
+            validate_sandbox_source_path(workspace_root, "output/file.txt").unwrap(),
+            "/workspace/project/output/file.txt"
+        );
+        assert_eq!(
+            validate_sandbox_source_path(workspace_root, "./output/../file.txt").unwrap(),
+            "/workspace/project/file.txt"
+        );
     }
 
     #[test]
     fn validate_sandbox_source_path_rejects_traversal_and_escapes() {
         let workspace_root = "/workspace/project";
-        let traversal = validate_sandbox_source_path("/etc/passwd", workspace_root).unwrap_err();
+        let traversal = validate_sandbox_source_path(workspace_root, "/etc/passwd").unwrap_err();
         assert!(
             format!("{traversal}").contains("outside the sandbox workspace"),
             "unexpected error: {traversal}"
         );
 
         let parent_escape =
-            validate_sandbox_source_path("/workspace/project/../../etc/passwd", workspace_root)
+            validate_sandbox_source_path(workspace_root, "/workspace/project/../../etc/passwd")
                 .unwrap_err();
         assert!(
             format!("{parent_escape}").contains("outside the sandbox workspace"),
             "unexpected error: {parent_escape}"
         );
 
         let prefix_only =
-            validate_sandbox_source_path("/workspace/projected/secrets", workspace_root)
+            validate_sandbox_source_path(workspace_root, "/workspace/projected/secrets")
                 .unwrap_err();
         assert!(
             format!("{prefix_only}").contains("outside the sandbox workspace"),
             "unexpected error: {prefix_only}"
         );
 
-        let empty = validate_sandbox_source_path("", workspace_root).unwrap_err();
+        let empty = validate_sandbox_source_path(workspace_root, "").unwrap_err();
         assert!(format!("{empty}").contains("empty"));
 
-        let relative =
-            validate_sandbox_source_path("workspace/project/file", workspace_root).unwrap_err();
-        assert!(format!("{relative}").contains("must be absolute"));
-    }
-
-    #[test]
-    fn is_under_workspace_accepts_root_and_descendants() {
-        assert!(is_under_workspace(
-            "/workspace/project",
-            "/workspace/project"
-        ));
-        assert!(is_under_workspace(
-            "/workspace/project/file",
-            "/workspace/project"
-        ));
-        assert!(is_under_workspace(
-            "/workspace/project/sub/nested",
-            "/workspace/project"
-        ));
-    }
-
-    #[test]
-    fn is_under_workspace_rejects_outside_paths_and_prefix_collisions() {
-        assert!(!is_under_workspace("/etc/passwd", "/workspace/project"));
-        assert!(!is_under_workspace(
-            "/workspace/projected/secrets",
-            "/workspace/project"
-        ));
-        assert!(!is_under_workspace("/", "/workspace/project"));
-        assert!(!is_under_workspace("", "/workspace/project"));
+        let relative_escape =
+            validate_sandbox_source_path(workspace_root, "../../etc/passwd").unwrap_err();
+        assert!(format!("{relative_escape}").contains("outside the sandbox workspace"));
     }
 
     #[test]

crates/openshell-core/src/driver_mounts.rs

@@ -97,6 +97,9 @@ pub fn validate_mount_subpath(subpath: &str) -> Result<(), String> {
 }
 
 /// Validate a container-side mount target for user-supplied driver mounts.
+///
+/// Workspace collisions depend on the inspected image's resolved working
+/// directory and are checked separately by `validate_workspace_mount_target`.
 pub fn validate_container_mount_target(target: &str) -> Result<(), String> {
     if target.is_empty() {
         return Err("mount target must not be empty".to_string());

crates/openshell-driver-docker/src/lib.rs

@@ -2436,8 +2436,8 @@ fn build_container_create_body_for_image(
         working_dir: Some("/".to_string()),
         env: Some(build_environment_for_oci_user(sandbox, config, &image.user)),
         entrypoint: Some(vec![SUPERVISOR_MOUNT_PATH.to_string()]),
-        // Clear the image CMD so Docker does not append inherited args to the
-        // supervisor entrypoint.
+        // Replace the image CMD with the supervisor's resolved workspace
+        // argument so Docker cannot append inherited image arguments.
         cmd: Some(vec!["--workdir".to_string(), workspace_root]),
         labels: Some(labels),
         host_config: Some(HostConfig {

crates/openshell-driver-kubernetes/src/driver.rs

@@ -4066,7 +4066,8 @@ mod tests {
             "init container must not depend on a shell"
         );
 
-        // Agent container command should be overridden to the emptyDir path
+        // `--workdir` is optional for standalone supervisor invocations and
+        // has no implicit default, so Kubernetes must pass its fixed workspace.
         let command = pod_template["spec"]["containers"][0]["command"]
             .as_array()
             .expect("command should be set");

crates/openshell-sandbox/src/lib.rs

@@ -189,20 +189,20 @@ pub async fn run_sandbox(
     // OpenShift retain their authoritative numeric pair; Docker and Podman
     // fill only omitted policy fields from OCI Config.User.
     #[cfg(unix)]
-    let (resolved_process_identity, workspace_home) = {
+    let (resolved_process_identity, workdir_is_home) = {
         let driver_identity = openshell_supervisor_process::identity::DriverIdentity::from_env()?;
-        let workspace_home = matches!(
+        let workdir_is_home = matches!(
             &driver_identity,
             openshell_supervisor_process::identity::DriverIdentity::OciUser { .. }
         );
         let resolved = openshell_supervisor_process::identity::resolve_process_identity(
             &mut policy,
             &driver_identity,
         )?;
-        (resolved, workspace_home)
+        (resolved, workdir_is_home)
     };
     #[cfg(not(unix))]
-    let (resolved_process_identity, workspace_home) = (
+    let (resolved_process_identity, workdir_is_home) = (
         openshell_supervisor_process::process::ResolvedProcessIdentity::default(),
         false,
     );
@@ -691,7 +691,7 @@ pub async fn run_sandbox(
             program,
             args,
             workdir.as_deref(),
-            workspace_home,
+            workdir_is_home,
             timeout_secs,
             interactive,
             sandbox_id.as_deref(),
@@ -1510,7 +1510,7 @@ mod baseline_tests {
     }
 
     #[test]
-    fn baseline_read_write_uses_dynamic_workdir_instead_of_sandbox() {
+    fn baseline_read_write_does_not_hardcode_sandbox() {
         let (_ro, rw) = baseline_enrichment_paths();
         assert!(rw.contains(&"/tmp".to_string()));
         assert!(!rw.contains(&"/sandbox".to_string()));

crates/openshell-supervisor-process/src/process.rs

@@ -530,7 +530,7 @@ impl ProcessHandle {
         program: &str,
         args: &[String],
         workdir: Option<&str>,
-        workspace_home: bool,
+        workdir_is_home: bool,
         interactive: bool,
         policy: &SandboxPolicy,
         resolved_identity: ResolvedProcessIdentity,
@@ -543,7 +543,7 @@ impl ProcessHandle {
             program,
             args,
             workdir,
-            workspace_home,
+            workdir_is_home,
             interactive,
             policy,
             resolved_identity,
@@ -565,7 +565,7 @@ impl ProcessHandle {
         program: &str,
         args: &[String],
         workdir: Option<&str>,
-        workspace_home: bool,
+        workdir_is_home: bool,
         interactive: bool,
         policy: &SandboxPolicy,
         resolved_identity: ResolvedProcessIdentity,
@@ -577,7 +577,7 @@ impl ProcessHandle {
             program,
             args,
             workdir,
-            workspace_home,
+            workdir_is_home,
             interactive,
             policy,
             resolved_identity,
@@ -593,7 +593,7 @@ impl ProcessHandle {
         program: &str,
         args: &[String],
         workdir: Option<&str>,
-        workspace_home: bool,
+        workdir_is_home: bool,
         interactive: bool,
         policy: &SandboxPolicy,
         resolved_identity: ResolvedProcessIdentity,
@@ -620,7 +620,7 @@ impl ProcessHandle {
 
         if let Some(dir) = workdir {
             cmd.current_dir(dir);
-            if workspace_home {
+            if workdir_is_home {
                 cmd.env("HOME", dir);
             }
         }
@@ -752,7 +752,7 @@ impl ProcessHandle {
         program: &str,
         args: &[String],
         workdir: Option<&str>,
-        workspace_home: bool,
+        workdir_is_home: bool,
         interactive: bool,
         policy: &SandboxPolicy,
         resolved_identity: ResolvedProcessIdentity,
@@ -776,7 +776,7 @@ impl ProcessHandle {
 
         if let Some(dir) = workdir {
             cmd.current_dir(dir);
-            if workspace_home {
+            if workdir_is_home {
                 cmd.env("HOME", dir);
             }
         }