From be58ca149da3f542adbe45316e77f286efa17f3e Mon Sep 17 00:00:00 2001 From: Mish Ushakov <10400064+mishushakov@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:44:25 +0200 Subject: [PATCH 1/4] fix(orchestrator): implement Docker COPY merge semantics in template builds COPY of a directory silently dropped any subdirectory that already existed non-empty at the destination (e.g. COPY rootfs/ / dropped everything; COPY rootfs/etc /etc dropped /etc/profile.d contents), because mv cannot merge directories and find -exec swallowed the failure. The copy script also picked the alphabetically-first entry of the source's parent instead of the entry named by the source path. Merge directory contents with cp -a (existing dirs merged, existing files overwritten), select the entry by basename, and fail the build step on copy errors. Add regression tests covering merge-into-existing trees, file overwrite, and entry selection. Co-Authored-By: Claude Fable 5 --- .../pkg/template/build/commands/copy.go | 4 +- .../template/build/commands/copy_script.sh | 17 ++- .../pkg/template/build/commands/copy_test.go | 131 +++++++++++++++++- 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/packages/orchestrator/pkg/template/build/commands/copy.go b/packages/orchestrator/pkg/template/build/commands/copy.go index 828d6d0018..f1b9446ce9 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy.go +++ b/packages/orchestrator/pkg/template/build/commands/copy.go @@ -57,7 +57,9 @@ var copyScriptTemplate = txtTemplate.Must(txtTemplate.New("copy-script-template" // 3) Extracts it (still in the /tmp directory) // 4) Moves the extracted files to the target path in the sandbox // - If the source is a file, it creates the parent directories and moves the file -// - If the source is a directory, it moves all its contents to the target directory +// - If the source is a directory, it merges its contents into the target +// directory (Docker COPY semantics: existing directories are merged into, +// existing files are overwritten) // Note: The temporary files in the /tmp directory are cleaned up automatically on sandbox restart // because the /tmp is mounted as a tmpfs and deleted on restart. diff --git a/packages/orchestrator/pkg/template/build/commands/copy_script.sh b/packages/orchestrator/pkg/template/build/commands/copy_script.sh index 3697c6eb44..fd06352c08 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_script.sh +++ b/packages/orchestrator/pkg/template/build/commands/copy_script.sh @@ -28,11 +28,11 @@ fi cd "$sourceFolder" || exit 1 -# Get the first entry (file, directory, or symlink) -entry=$(ls -A | head -n 1) +# Get the entry (file, directory, or symlink) named by the source path +entry="$(basename "$sourcePath")" -if [ -z "$entry" ]; then - echo "Error: sourceFolder is empty" +if [ ! -e "$entry" ] && [ ! -L "$entry" ]; then + echo "Error: source path does not exist: $sourcePath" exit 1 fi @@ -54,14 +54,17 @@ elif [ -f "$entry" ]; then mkdir -p "$(dirname "$targetPath")" mv "$entry" "$targetPath" elif [ -d "$entry" ]; then - # It's a directory – apply ownership/permissions recursively, then move contents + # It's a directory – apply ownership/permissions recursively, then merge + # its contents into the target (Docker COPY semantics: existing directories + # are merged into, existing files overwritten – mv can't merge into + # non-empty directories, so copy and remove the source instead) chown -R "$owner" "$entry" if [ -n "$permissions" ]; then chmod -R "$permissions" "$entry" fi mkdir -p "$targetPath" - # Move all contents including hidden files - find "$entry" -mindepth 1 -maxdepth 1 -exec mv {} "$targetPath/" \; + cp -a "$entry/." "$targetPath/" || exit 1 + rm -rf "$entry" else echo "Error: entry is neither file, directory, nor symlink" exit 1 diff --git a/packages/orchestrator/pkg/template/build/commands/copy_test.go b/packages/orchestrator/pkg/template/build/commands/copy_test.go index 7a0864c405..e356e08840 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_test.go +++ b/packages/orchestrator/pkg/template/build/commands/copy_test.go @@ -92,6 +92,12 @@ func renderTemplate(t *testing.T, data copyScriptData) string { // createFilesAndDirs creates files, directories, and symlinks from a map // Values: "file", "dir", "symlink" func createFilesAndDirs(t *testing.T, baseDir string, paths map[string]string) { + t.Helper() + createFilesAndDirsWithContent(t, baseDir, paths, "dummy") +} + +// createFilesAndDirsWithContent is createFilesAndDirs with custom file content +func createFilesAndDirsWithContent(t *testing.T, baseDir string, paths map[string]string, content string) { t.Helper() for path, entryType := range paths { fullPath := filepath.Join(baseDir, path) @@ -103,7 +109,7 @@ func createFilesAndDirs(t *testing.T, baseDir string, paths map[string]string) { // Ensure parent dir exists dir := filepath.Dir(fullPath) require.NoError(t, os.MkdirAll(dir, 0o755)) - require.NoError(t, os.WriteFile(fullPath, []byte("dummy"), 0o644)) + require.NoError(t, os.WriteFile(fullPath, []byte(content), 0o644)) case "symlink": // Create symlink target outside the tree dir := filepath.Dir(fullPath) @@ -148,6 +154,10 @@ type testCase struct { // Example: {"app/": "dir", "app/main.js": "file", "link": "symlink"} files map[string]string + // Setup: paths pre-created in the target before the copy runs, + // to simulate copying over an existing filesystem tree + preexistingTargetPaths map[string]string + // Input: the path within the extracted files to copy from // Examples: "." (root), "app/" (subdirectory), "src/main.js" (specific file) copyFrom string @@ -168,6 +178,12 @@ type testCase struct { // Verification: what paths to check in the target with their types expectedPaths map[string]string + + // Verification: paths that must NOT exist in the target + absentPaths []string + + // Verification: file contents to check in the target + expectedContents map[string]string } func TestParseCopyArgs(t *testing.T) { @@ -623,6 +639,101 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why "deep.txt": "file", }, }, + { + name: "merge_directory_into_existing_tree", + description: "COPY rootfs/etc /etc: merge into a target directory that already has content", + files: map[string]string{ + "etc/motd": "file", + "etc/profile.d/test-env.sh": "file", + }, + copyFrom: "etc/", + copyTo: "etc/", + preexistingTargetPaths: map[string]string{ + "etc/profile.d/existing.sh": "file", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "etc/motd": "file", + "etc/profile.d/test-env.sh": "file", + "etc/profile.d/existing.sh": "file", + }, + }, + { + name: "merge_root_directory_into_existing_root", + description: "COPY rootfs/ /: every top-level dir already exists in the target root", + files: map[string]string{ + "rootfs/etc/profile.d/test-env.sh": "file", + "rootfs/usr/local/bin/tool": "file", + }, + copyFrom: "rootfs/", + copyTo: ".", + preexistingTargetPaths: map[string]string{ + "etc/profile.d/00-existing.sh": "file", + "usr/local/bin/existing-tool": "file", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "etc/profile.d/test-env.sh": "file", + "etc/profile.d/00-existing.sh": "file", + "usr/local/bin/tool": "file", + "usr/local/bin/existing-tool": "file", + }, + }, + { + name: "overwrite_existing_file_in_target", + description: "Files that already exist in the target are overwritten", + files: map[string]string{ + "etc/motd": "file", + }, + copyFrom: "etc/", + copyTo: "etc/", + preexistingTargetPaths: map[string]string{ + "etc/motd": "file", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "etc/motd": "file", + }, + expectedContents: map[string]string{ + "etc/motd": "dummy", + }, + }, + { + name: "copy_directory_that_is_not_first_alphabetically", + description: "The entry named by the source path is copied, not the first entry in its parent", + files: map[string]string{ + "project/components/Button.tsx": "file", + "project/utils/helpers.ts": "file", + }, + copyFrom: "project/utils/", + copyTo: "out/", + shouldSucceed: true, + expectedPaths: map[string]string{ + "out/helpers.ts": "file", + }, + absentPaths: []string{ + "out/Button.tsx", + "out/components", + }, + }, + { + name: "copy_file_that_is_not_first_alphabetically", + description: "The file named by the source path is copied, not the first entry in its parent", + files: map[string]string{ + "assets/logo.png": "file", + "config.json": "file", + }, + copyFrom: "config.json", + copyTo: "dest/config.json", + shouldSucceed: true, + expectedPaths: map[string]string{ + "dest/config.json": "file", + }, + absentPaths: []string{ + "dest/logo.png", + "dest/config.json/logo.png", + }, + }, { name: "deeply_nested_folder", description: "Deeply nested folder should be copied correctly", @@ -656,6 +767,11 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why verifyFilesAndDirs(t, unpackDir, tc.files) } + // Pre-populate the target to simulate an existing filesystem tree + if len(tc.preexistingTargetPaths) > 0 { + createFilesAndDirsWithContent(t, targetBaseDir, tc.preexistingTargetPaths, "preexisting") + } + // Internal: construct SourcePath (sbxUnpackPath + user's copyFrom path) // This mimics how copy.go constructs the path: filepath.Join(sbxUnpackPath, sourcePath) sourcePath := filepath.Join(unpackDir, tc.copyFrom) @@ -701,6 +817,19 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why verifyFilesAndDirs(t, targetBaseDir, tc.expectedPaths) } + // Verify paths that must not exist in the target + for _, path := range tc.absentPaths { + assert.NoFileExists(t, filepath.Join(targetBaseDir, path), "Path %s should not exist", path) + assert.NoDirExists(t, filepath.Join(targetBaseDir, path), "Path %s should not exist", path) + } + + // Verify file contents + for path, expectedContent := range tc.expectedContents { + content, err := os.ReadFile(filepath.Join(targetBaseDir, path)) + require.NoError(t, err, "Failed to read file %s", path) + assert.Equal(t, expectedContent, string(content), "File %s content mismatch", path) + } + // Special verification for permissions tests if tc.permissions != "" && tc.shouldSucceed { for path, entryType := range tc.expectedPaths { From e536e8761cb958dd78026ccbe78b11756a0cb02c Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Mon, 13 Jul 2026 18:49:48 +0000 Subject: [PATCH 2/4] chore: auto-commit generated changes --- .../pkg/template/build/commands/copy_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/orchestrator/pkg/template/build/commands/copy_test.go b/packages/orchestrator/pkg/template/build/commands/copy_test.go index e356e08840..c8e2472fe4 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_test.go +++ b/packages/orchestrator/pkg/template/build/commands/copy_test.go @@ -643,7 +643,7 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why name: "merge_directory_into_existing_tree", description: "COPY rootfs/etc /etc: merge into a target directory that already has content", files: map[string]string{ - "etc/motd": "file", + "etc/motd": "file", "etc/profile.d/test-env.sh": "file", }, copyFrom: "etc/", @@ -653,9 +653,9 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why }, shouldSucceed: true, expectedPaths: map[string]string{ - "etc/motd": "file", + "etc/motd": "file", "etc/profile.d/test-env.sh": "file", - "etc/profile.d/existing.sh": "file", + "etc/profile.d/existing.sh": "file", }, }, { @@ -663,7 +663,7 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why description: "COPY rootfs/ /: every top-level dir already exists in the target root", files: map[string]string{ "rootfs/etc/profile.d/test-env.sh": "file", - "rootfs/usr/local/bin/tool": "file", + "rootfs/usr/local/bin/tool": "file", }, copyFrom: "rootfs/", copyTo: ".", @@ -673,7 +673,7 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why }, shouldSucceed: true, expectedPaths: map[string]string{ - "etc/profile.d/test-env.sh": "file", + "etc/profile.d/test-env.sh": "file", "etc/profile.d/00-existing.sh": "file", "usr/local/bin/tool": "file", "usr/local/bin/existing-tool": "file", From 37f212aa7efed297fbd70d58df391e7aebc12129 Mon Sep 17 00:00:00 2001 From: Mish Ushakov <10400064+mishushakov@users.noreply.github.com> Date: Mon, 13 Jul 2026 20:56:19 +0200 Subject: [PATCH 3/4] fix(orchestrator): keep target dir metadata and fix source cleanup in COPY Address review findings: cp -a "src/." applied the source directory's own ownership/permissions to the existing target directory (so COPY rootfs/ / could chmod/chown /), and rm -rf failed for non-root when a read-only permissions argument was applied to the source. Copy children individually so the target directory keeps its metadata, and restore write permissions before removing the source. Add regression tests for both. Co-Authored-By: Claude Fable 5 --- .../template/build/commands/copy_script.sh | 7 ++- .../pkg/template/build/commands/copy_test.go | 46 +++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/packages/orchestrator/pkg/template/build/commands/copy_script.sh b/packages/orchestrator/pkg/template/build/commands/copy_script.sh index fd06352c08..cbce0b4902 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_script.sh +++ b/packages/orchestrator/pkg/template/build/commands/copy_script.sh @@ -63,7 +63,12 @@ elif [ -d "$entry" ]; then chmod -R "$permissions" "$entry" fi mkdir -p "$targetPath" - cp -a "$entry/." "$targetPath/" || exit 1 + # Copy children one by one so the target directory itself keeps its own + # metadata (Docker copies a directory's contents, never the directory) + find "$entry" -mindepth 1 -maxdepth 1 -exec cp -a -t "$targetPath" {} + || exit 1 + # Restore write permissions so cleanup works even when a read-only + # permissions argument was applied and we are not running as root + chmod -R u+rwx "$entry" rm -rf "$entry" else echo "Error: entry is neither file, directory, nor symlink" diff --git a/packages/orchestrator/pkg/template/build/commands/copy_test.go b/packages/orchestrator/pkg/template/build/commands/copy_test.go index c8e2472fe4..70beb8bfd7 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_test.go +++ b/packages/orchestrator/pkg/template/build/commands/copy_test.go @@ -184,6 +184,9 @@ type testCase struct { // Verification: file contents to check in the target expectedContents map[string]string + + // Verification: octal permissions to check in the target + expectedPerms map[string]string } func TestParseCopyArgs(t *testing.T) { @@ -698,6 +701,41 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why "etc/motd": "dummy", }, }, + { + name: "preserve_target_directory_metadata", + description: "Only directory contents are copied; the existing target directory keeps its own permissions", + files: map[string]string{ + "etc/motd": "file", + }, + copyFrom: "etc/", + copyTo: "etc/", + // 700 must apply to the copied contents, not the existing target dir + permissions: "700", + preexistingTargetPaths: map[string]string{ + "etc/": "dir", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "etc/motd": "file", + }, + expectedPerms: map[string]string{ + "etc": "755", + }, + }, + { + name: "directory_with_readonly_permissions", + description: "Read-only permissions on the copied tree do not break source cleanup", + files: map[string]string{ + "app/config.json": "file", + }, + copyFrom: "app/", + copyTo: "dest/", + permissions: "500", + shouldSucceed: true, + expectedPaths: map[string]string{ + "dest/config.json": "file", + }, + }, { name: "copy_directory_that_is_not_first_alphabetically", description: "The entry named by the source path is copied, not the first entry in its parent", @@ -830,6 +868,14 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why assert.Equal(t, expectedContent, string(content), "File %s content mismatch", path) } + // Verify permissions of specific paths + for path, permStr := range tc.expectedPerms { + perms := getFilePermissions(t, filepath.Join(targetBaseDir, path)) + expectedPerms := os.FileMode(0) + fmt.Sscanf(permStr, "%o", &expectedPerms) + assert.Equal(t, expectedPerms, perms, "Path %s should have %s permissions", path, permStr) + } + // Special verification for permissions tests if tc.permissions != "" && tc.shouldSucceed { for path, entryType := range tc.expectedPaths { From b0c22a5dfcbe6bc8a57d8359ba7037885fd94ef2 Mon Sep 17 00:00:00 2001 From: Mish Ushakov <10400064+mishushakov@users.noreply.github.com> Date: Mon, 13 Jul 2026 21:08:11 +0200 Subject: [PATCH 4/4] fix(orchestrator): handle destination symlinks in COPY directory merge Address review findings: cp wrote through pre-existing destination file symlinks (e.g. /etc/resolv.conf), silently modifying the link target instead of replacing the link, and hard-failed on destination directory symlinks (usrmerge /lib -> usr/lib) with "cannot overwrite non-directory with directory". Merge with a tar pipe instead, matching Docker's tar-based COPY: destination file symlinks are replaced, directory symlinks are followed (--keep-directory-symlink), and existing directories keep their metadata (--no-overwrite-dir). Add regression tests for both. Co-Authored-By: Claude Fable 5 --- .../template/build/commands/copy_script.sh | 10 +++- .../pkg/template/build/commands/copy_test.go | 55 +++++++++++++++++++ 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/packages/orchestrator/pkg/template/build/commands/copy_script.sh b/packages/orchestrator/pkg/template/build/commands/copy_script.sh index cbce0b4902..dd414ed65c 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_script.sh +++ b/packages/orchestrator/pkg/template/build/commands/copy_script.sh @@ -1,5 +1,7 @@ #!/bin/bash +set -o pipefail + targetPath="{{ .TargetPath }}" sourcePath="{{ .SourcePath }}" owner="{{ .Owner }}" @@ -63,9 +65,11 @@ elif [ -d "$entry" ]; then chmod -R "$permissions" "$entry" fi mkdir -p "$targetPath" - # Copy children one by one so the target directory itself keeps its own - # metadata (Docker copies a directory's contents, never the directory) - find "$entry" -mindepth 1 -maxdepth 1 -exec cp -a -t "$targetPath" {} + || exit 1 + # Merge via tar, matching Docker's tar-based COPY: unlike cp, it replaces + # destination file symlinks instead of writing through them, follows + # destination directory symlinks (usrmerge, e.g. /lib -> usr/lib), and + # keeps the metadata of the target directory and other existing dirs + (cd "$entry" && tar -cf - .) | tar -xf - -C "$targetPath" --keep-directory-symlink --no-overwrite-dir || exit 1 # Restore write permissions so cleanup works even when a read-only # permissions argument was applied and we are not running as root chmod -R u+rwx "$entry" diff --git a/packages/orchestrator/pkg/template/build/commands/copy_test.go b/packages/orchestrator/pkg/template/build/commands/copy_test.go index 70beb8bfd7..e3d211d13c 100644 --- a/packages/orchestrator/pkg/template/build/commands/copy_test.go +++ b/packages/orchestrator/pkg/template/build/commands/copy_test.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -118,6 +119,13 @@ func createFilesAndDirsWithContent(t *testing.T, baseDir string, paths map[strin require.NoError(t, os.WriteFile(targetFile, []byte("symlink target"), 0o644)) require.NoError(t, os.Symlink(targetFile, fullPath)) default: + // "symlink:" creates a symlink pointing at the given path + if target, ok := strings.CutPrefix(entryType, "symlink:"); ok { + require.NoError(t, os.MkdirAll(filepath.Dir(fullPath), 0o755)) + require.NoError(t, os.Symlink(target, fullPath)) + + continue + } t.Fatalf("Unknown entry type: %s", entryType) } } @@ -138,6 +146,11 @@ func verifyFilesAndDirs(t *testing.T, baseDir string, paths map[string]string) { info, err := os.Lstat(fullPath) require.NoError(t, err, "Symlink %s should exist", path) assert.Equal(t, os.ModeSymlink, info.Mode()&os.ModeSymlink, "%s should be a symlink", path) + case "regular": + // A regular file, NOT a symlink pointing at one + info, err := os.Lstat(fullPath) + require.NoError(t, err, "File %s should exist", path) + assert.True(t, info.Mode().IsRegular(), "%s should be a regular file, got %s", path, info.Mode()) default: t.Fatalf("Unknown entry type: %s", entryType) } @@ -736,6 +749,48 @@ func TestCopyScriptBehavior(t *testing.T) { //nolint:paralleltest // no idea why "dest/config.json": "file", }, }, + { + name: "replace_target_symlink_to_file", + description: "A source file replaces a destination symlink instead of writing through it", + files: map[string]string{ + "etc/resolv.conf": "file", + }, + copyFrom: "etc/", + copyTo: "etc/", + preexistingTargetPaths: map[string]string{ + // resolv.conf points outside the target tree (as on Ubuntu images) + "../real/resolv.conf": "file", + "etc/resolv.conf": "symlink:../../real/resolv.conf", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "etc/resolv.conf": "regular", + }, + expectedContents: map[string]string{ + "etc/resolv.conf": "dummy", + // The symlink's old target must not have been written through + "../real/resolv.conf": "preexisting", + }, + }, + { + name: "follow_target_symlink_to_directory", + description: "A source directory merges through a destination directory symlink (usrmerge layout)", + files: map[string]string{ + "lib/mylib.so": "file", + }, + copyFrom: ".", + copyTo: ".", + preexistingTargetPaths: map[string]string{ + "usr/lib/existing.so": "file", + "lib": "symlink:usr/lib", + }, + shouldSucceed: true, + expectedPaths: map[string]string{ + "lib": "symlink", + "usr/lib/mylib.so": "file", + "usr/lib/existing.so": "file", + }, + }, { name: "copy_directory_that_is_not_first_alphabetically", description: "The entry named by the source path is copied, not the first entry in its parent",