From b590c4550b24df10a016eed8420b205d6587b948 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 16:48:14 -0500 Subject: [PATCH 1/3] feat(devcontainer): import external --devcontainer configs into the workspace Passing --devcontainer with a path to a config outside the repo previously flattened it to a single synthesized file at the repo root, which broke self-contained configs: a relative Dockerfile/feature/compose path resolves against the config's directory, so the sibling assets were lost. Import the external config into the workspace as a native named profile (.devcontainer//) instead: - A self-contained "/devcontainer.json" is copied whole so its sibling assets travel with it; a bare file is copied on its own. - The profile is discovered natively (.devcontainer//devcontainer.json), so config-relative paths resolve. - The imported dir is added to .git/info/exclude so it does not dirty git status, and carries a marker file so teardown can identify it. A collision with a project-owned .devcontainer// falls back to a suffixed dir. - runner.Delete removes the imported profile for local-folder sources (alongside cleanupDeliveryVolume), so both up-delete and ci teardown clean up through one path. --- .../up/up_devcontainer_source_test.go | 11 ++ cmd/workspace/up/up_validate.go | 9 +- pkg/devcontainer/config.go | 183 ++++++++++++++++++ pkg/devcontainer/delete.go | 16 ++ pkg/devcontainer/delete_test.go | 36 ++++ pkg/devcontainer/import_external_test.go | 162 ++++++++++++++++ 6 files changed, 415 insertions(+), 2 deletions(-) create mode 100644 pkg/devcontainer/import_external_test.go diff --git a/cmd/workspace/up/up_devcontainer_source_test.go b/cmd/workspace/up/up_devcontainer_source_test.go index 99f560e50..98e5898fa 100644 --- a/cmd/workspace/up/up_devcontainer_source_test.go +++ b/cmd/workspace/up/up_devcontainer_source_test.go @@ -28,6 +28,17 @@ func TestResolveDevContainerSource_Path(t *testing.T) { assert.Empty(t, cmd.DevContainerSource) } +func TestResolveDevContainerSource_ExternalPathPassThrough(t *testing.T) { + // An external absolute path stays in the source so the runner imports it. + cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} + cmd.DevContainerSource = "/abs/external/devcontainer.json" + + require.NoError(t, cmd.resolveDevContainerSource()) + assert.Equal(t, "/abs/external/devcontainer.json", cmd.DevContainerSource) + assert.Empty(t, cmd.DevContainerPath) + assert.Empty(t, cmd.DevContainerID) +} + func TestResolveDevContainerSource_NoneAndImagePassThrough(t *testing.T) { for _, spec := range []string{srcNone, "image:python"} { cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} diff --git a/cmd/workspace/up/up_validate.go b/cmd/workspace/up/up_validate.go index a576b1140..9ee440137 100644 --- a/cmd/workspace/up/up_validate.go +++ b/cmd/workspace/up/up_validate.go @@ -61,8 +61,13 @@ func (cmd *UpCmd) resolveDevContainerSource() error { cmd.DevContainerID = spec.ID cmd.DevContainerSource = "" case devcontainer.SourcePath: - cmd.DevContainerPath = spec.Path - cmd.DevContainerSource = "" + // An in-repo relative path is just a config path; an external absolute + // path is left in the source so the runner imports it (bringing its + // sibling assets — Dockerfile, features — into the workspace). + if !filepath.IsAbs(spec.Path) { + cmd.DevContainerPath = spec.Path + cmd.DevContainerSource = "" + } case devcontainer.SourceNone, devcontainer.SourceImage: } return nil diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 9165217fa..09affef90 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -9,14 +9,33 @@ import ( "path" "path/filepath" "slices" + "strings" pkgconfig "github.com/devsy-org/devsy/pkg/config" + copypkg "github.com/devsy-org/devsy/pkg/copy" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/devcontainer/crane" "github.com/devsy-org/devsy/pkg/flags/names" "github.com/devsy-org/devsy/pkg/language" "github.com/devsy-org/devsy/pkg/log" "github.com/devsy-org/devsy/pkg/provider" + "github.com/devsy-org/devsy/pkg/random" +) + +// External --devcontainer configs are imported into the workspace as a named +// devcontainer profile (.devcontainer//), which the config discovery +// already recognizes and which keeps config-relative assets (Dockerfile, +// features, compose) resolving. importedProfileMarker is dropped inside the +// imported dir so teardown can identify and remove it regardless of any +// collision suffix. +var ( + importedProfileParent = filepath.ToSlash(devcontainerProfileParent) + importedProfileMarker = "." + pkgconfig.BinaryName + "-imported" +) + +const ( + devcontainerProfileParent = ".devcontainer" + importedProfileName = pkgconfig.BinaryName ) // getRawConfig resolves the raw devcontainer config for the workspace, trying @@ -180,11 +199,175 @@ func (r *runner) rawConfigFromSource( defaultConfig = language.DefaultConfig(r.localWorkspaceFolder) } return r.saveSynthesizedConfig(defaultConfig) + case SourcePath: + return r.importExternalDevContainer(spec.Path) + case SourceID: + return nil, fmt.Errorf("devcontainer id source must be resolved before build") default: return nil, fmt.Errorf("unsupported devcontainer source kind %q", spec.Kind) } } +// importExternalDevContainer copies a devcontainer config that lives outside the +// workspace into it as a named profile (.devcontainer//), so config +// discovery finds it and config-relative assets (Dockerfile, features, compose) +// resolve. A self-contained "/devcontainer.json" has its whole folder +// copied; a bare file is copied on its own. The profile dir is added to the +// repo's local git exclude so it doesn't dirty git status, and is marked so +// teardown can remove it. +func (r *runner) importExternalDevContainer(srcPath string) (*config.DevContainerConfig, error) { + srcPath, err := filepath.Abs(srcPath) + if err != nil { + return nil, fmt.Errorf("resolve devcontainer path %s: %w", srcPath, err) + } + if _, err := os.Stat(srcPath); err != nil { + return nil, fmt.Errorf("devcontainer path %s does not exist: %w", srcPath, err) + } + + relDir := r.importedProfileRelDir() + destDir := filepath.Join(r.localWorkspaceFolder, filepath.FromSlash(relDir)) + if err := copyExternalDevContainer(srcPath, destDir); err != nil { + return nil, err + } + if err := os.WriteFile(filepath.Join(destDir, importedProfileMarker), nil, 0o600); err != nil { + return nil, fmt.Errorf("mark imported devcontainer: %w", err) + } + if err := r.excludeFromGit(relDir); err != nil { + log.Debugf("could not add imported devcontainer to git exclude: %v", err) + } + + origin := filepath.Join(destDir, filepath.Base(srcPath)) + rawConfig, err := config.ParseDevContainerJSONFile(context.Background(), origin) + if err != nil { + return nil, fmt.Errorf("parse imported devcontainer.json: %w", err) + } + return rawConfig, nil +} + +// importedProfileRelDir returns the workspace-relative profile directory to +// import into: .devcontainer/, or .devcontainer/_ when +// the plain name already exists in the repo (so an imported profile never +// clobbers a config the project ships). +func (r *runner) importedProfileRelDir() string { + base := path.Join(importedProfileParent, importedProfileName) + plain := filepath.Join(r.localWorkspaceFolder, filepath.FromSlash(base)) + if isImportedProfileDir(plain) || !dirExists(plain) { + return base // reusable (ours) or free + } + return base + "_" + random.String(6) +} + +// copyExternalDevContainer copies the external config into destDir, replacing +// any previous import. A self-contained config (its parent dir holds the +// devcontainer.json plus sibling assets) is imported whole; otherwise only the +// file is copied. +func copyExternalDevContainer(srcPath, destDir string) error { + if err := os.RemoveAll(destDir); err != nil { + return fmt.Errorf("clear imported devcontainer dir: %w", err) + } + if isSelfContainedDevContainer(srcPath) { + if err := copypkg.Directory(filepath.Dir(srcPath), destDir); err != nil { + return fmt.Errorf("copy devcontainer folder: %w", err) + } + return nil + } + if err := copypkg.CreateIfNotExists(destDir, 0o755); err != nil { + return fmt.Errorf("create imported devcontainer dir: %w", err) + } + dest := filepath.Join(destDir, filepath.Base(srcPath)) + if err := copypkg.File(srcPath, dest, 0o644); err != nil { + return fmt.Errorf("copy devcontainer file: %w", err) + } + return nil +} + +// dirExists reports whether path is an existing directory. +func dirExists(path string) bool { + info, err := os.Stat(path) + return err == nil && info.IsDir() +} + +// isSelfContainedDevContainer reports whether the config at srcPath lives in a +// dedicated folder alongside build assets (Dockerfile, features, compose), in +// which case the whole folder must travel with it. +func isSelfContainedDevContainer(srcPath string) bool { + dir := filepath.Dir(srcPath) + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + for _, e := range entries { + if e.Name() == filepath.Base(srcPath) { + continue + } + return true // any sibling file/dir means the config isn't standalone + } + return false +} + +// isImportedProfileDir reports whether dir is a previously imported profile +// (identified by the marker file), meaning it is safe for devsy to overwrite. +func isImportedProfileDir(dir string) bool { + _, err := os.Stat(filepath.Join(dir, importedProfileMarker)) + return err == nil +} + +// excludeFromGit adds relPath to the workspace repo's .git/info/exclude so the +// imported files don't show up in git status. It is best-effort: a workspace +// that isn't a git repo (no .git/info) is simply skipped. +func (r *runner) excludeFromGit(relPath string) error { + excludePath := filepath.Join(r.localWorkspaceFolder, ".git", "info", "exclude") + if _, err := os.Stat(filepath.Dir(excludePath)); err != nil { + return nil // not a git repo (or no info dir); nothing to do + } + // #nosec G304 -- path is under the workspace .git dir + existing, err := os.ReadFile(excludePath) + if err != nil && !os.IsNotExist(err) { + return err + } + entry := "/" + filepath.ToSlash(relPath) + for line := range strings.SplitSeq(string(existing), "\n") { + if strings.TrimSpace(line) == entry { + return nil // already excluded + } + } + // #nosec G304 -- path is under the workspace .git dir + f, err := os.OpenFile(excludePath, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + if err != nil { + return err + } + defer func() { _ = f.Close() }() + _, err = fmt.Fprintf(f, "%s\n", entry) + return err +} + +// CleanupImportedDevContainers removes any devcontainer profiles that devsy +// imported into workspaceFolder from an external --devcontainer path (those +// carrying importedProfileMarker). It is safe to call when none exist. +func CleanupImportedDevContainers(workspaceFolder string) error { + parent := filepath.Join(workspaceFolder, filepath.FromSlash(importedProfileParent)) + entries, err := os.ReadDir(parent) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + for _, e := range entries { + if !e.IsDir() { + continue + } + dir := filepath.Join(parent, e.Name()) + if !isImportedProfileDir(dir) { + continue + } + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("remove imported devcontainer %s: %w", dir, err) + } + } + return nil +} + func (r *runner) saveSynthesizedConfig( c *config.DevContainerConfig, ) (*config.DevContainerConfig, error) { diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 137759e9c..4a9c32fbb 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -15,6 +15,7 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { return fmt.Errorf("find dev container: %w", err) } defer r.cleanupDeliveryVolume(ctx) + defer r.cleanupImportedDevContainer() if containerDetails == nil { return nil } @@ -50,6 +51,21 @@ func (r *runner) cleanupDeliveryVolume(ctx context.Context) { } } +// cleanupImportedDevContainer removes any devcontainer profile the runner +// imported from an external --devcontainer path into a local-folder source. +// Other sources keep their content under a devsy-managed folder that the caller +// deletes wholesale, so there is nothing to clean there. Best-effort. +func (r *runner) cleanupImportedDevContainer() { + if r.workspaceConfig == nil || + r.workspaceConfig.Workspace == nil || + r.workspaceConfig.Workspace.Source.LocalFolder == "" { + return + } + if err := CleanupImportedDevContainers(r.localWorkspaceFolder); err != nil { + log.Debugf("best-effort imported devcontainer cleanup: %v", err) + } +} + func (r *runner) Stop(ctx context.Context) error { containerDetails, err := r.driver.FindDevContainer(ctx, r.id) if err != nil { diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 8f881bf45..b2163664c 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "io" + "path/filepath" "testing" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -175,3 +176,38 @@ func TestCleanupDeliveryVolume_DoesNotPanic(t *testing.T) { r.cleanupDeliveryVolume(context.Background()) } + +func TestDelete_RemovesImportedDevContainer(t *testing.T) { + // A local-folder workspace with an imported profile: Delete must remove it. + ws := t.TempDir() + external := filepath.Join(t.TempDir(), "devcontainer.json") + writeFile(t, external, `{"image":"alpine"}`) + + r := newTestRunner(&mockDriver{findResult: nil}) + r.localWorkspaceFolder = ws + r.workspaceConfig.Workspace = &provider.Workspace{ + Source: provider.WorkspaceSource{LocalFolder: ws}, + } + + if _, err := r.importExternalDevContainer(external); err != nil { + t.Fatalf("import failed: %v", err) + } + if !dirExists(importedProfilePath(ws)) { + t.Fatal("expected imported profile before delete") + } + + if err := r.Delete(context.Background(), DeleteOptions{}); err != nil { + t.Fatalf("Delete failed: %v", err) + } + if dirExists(importedProfilePath(ws)) { + t.Error("imported profile should be removed after Delete") + } +} + +func TestDelete_NonLocalSource_KeepsNothingToClean(t *testing.T) { + // No Workspace/local folder: cleanup must be a safe no-op. + r := newTestRunner(&mockDriver{findResult: nil}) + if err := r.Delete(context.Background(), DeleteOptions{}); err != nil { + t.Fatalf("Delete failed: %v", err) + } +} diff --git a/pkg/devcontainer/import_external_test.go b/pkg/devcontainer/import_external_test.go new file mode 100644 index 000000000..d1d85aef8 --- /dev/null +++ b/pkg/devcontainer/import_external_test.go @@ -0,0 +1,162 @@ +package devcontainer + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// writeFile is a small test helper. +func writeFile(t *testing.T, path, body string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) +} + +// importedProfilePath is the default profile dir devsy imports into. +func importedProfilePath(ws string) string { + return filepath.Join(ws, filepath.FromSlash(importedProfileParent), importedProfileName) +} + +func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { + // External, self-contained config: devcontainer.json + sibling Dockerfile. + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), + `{"name":"ext","build":{"dockerfile":"Dockerfile"}}`) + writeFile(t, filepath.Join(external, "Dockerfile"), "FROM alpine\n") + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + cfg, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + + // The whole folder is imported under .devcontainer// so the + // Dockerfile travels with the config. + imported := importedProfilePath(ws) + assert.FileExists(t, filepath.Join(imported, "devcontainer.json")) + assert.FileExists(t, filepath.Join(imported, "Dockerfile")) + + // Origin points inside the workspace so a relative Dockerfile resolves. + assert.Equal(t, filepath.Join(imported, "devcontainer.json"), cfg.Origin) + assert.Equal(t, "Dockerfile", cfg.GetDockerfile()) + dockerfile := filepath.Join(filepath.Dir(cfg.Origin), cfg.GetDockerfile()) + assert.FileExists(t, dockerfile) +} + +func TestImportExternalDevContainer_BareFile(t *testing.T) { + // A lone devcontainer.json (no sibling assets) is copied on its own. + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + cfg, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + assert.FileExists(t, filepath.Join(importedProfilePath(ws), "devcontainer.json")) + assert.Equal(t, "alpine", cfg.Image) +} + +func TestImportExternalDevContainer_WritesMarker(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + assert.True(t, isImportedProfileDir(importedProfilePath(ws)), + "imported profile must carry the marker file") +} + +func TestImportExternalDevContainer_CollisionGetsSuffix(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + // The project already ships its own .devcontainer// (no marker). + ws := t.TempDir() + writeFile(t, filepath.Join(importedProfilePath(ws), "devcontainer.json"), + `{"image":"project-owned"}`) + r := &runner{localWorkspaceFolder: ws} + + cfg, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + + // The import must land in a suffixed sibling, not the project's dir. + assert.Equal(t, "alpine", cfg.Image) + base := filepath.Base(filepath.Dir(cfg.Origin)) + assert.True(t, strings.HasPrefix(base, importedProfileName+"_"), + "expected a suffixed dir, got %q", base) +} + +func TestImportExternalDevContainer_ReusesOwnProfile(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + // Two imports in a row must reuse the same (devsy-owned) profile dir. + first, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + second, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + assert.Equal(t, first.Origin, second.Origin) +} + +func TestImportExternalDevContainer_AddsGitExclude(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(ws, ".git", "info"), 0o750)) + r := &runner{localWorkspaceFolder: ws} + + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + + // #nosec G304 -- test path + exclude, err := os.ReadFile(filepath.Join(ws, ".git", "info", "exclude")) + require.NoError(t, err) + assert.Contains(t, string(exclude), importedProfileParent+"/"+importedProfileName) +} + +func TestImportExternalDevContainer_NonGitRepoSkipsExclude(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() // no .git + r := &runner{localWorkspaceFolder: ws} + + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err, "import must succeed even outside a git repo") +} + +func TestCleanupImportedDevContainers(t *testing.T) { + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + + ws := t.TempDir() + // A project-owned profile (no marker) must survive cleanup. + writeFile(t, filepath.Join(ws, ".devcontainer", "app", "devcontainer.json"), `{}`) + + r := &runner{localWorkspaceFolder: ws} + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + require.DirExists(t, importedProfilePath(ws)) + + require.NoError(t, CleanupImportedDevContainers(ws)) + assert.NoDirExists(t, importedProfilePath(ws), "imported profile must be removed") + assert.DirExists(t, filepath.Join(ws, ".devcontainer", "app"), + "project-owned profile must be preserved") +} + +func TestCleanupImportedDevContainers_NoDevcontainerDir(t *testing.T) { + assert.NoError(t, CleanupImportedDevContainers(t.TempDir())) +} From 3d21663f89cab85e596a91c92e2502139cfbda1a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 17:05:11 -0500 Subject: [PATCH 2/3] chore: cleanup comments --- .../up/up_devcontainer_source_test.go | 1 - cmd/workspace/up/up_validate.go | 3 -- pkg/devcontainer/config.go | 39 ++----------------- pkg/devcontainer/delete.go | 10 +---- pkg/devcontainer/delete_test.go | 2 - pkg/devcontainer/import_external_test.go | 11 ------ 6 files changed, 5 insertions(+), 61 deletions(-) diff --git a/cmd/workspace/up/up_devcontainer_source_test.go b/cmd/workspace/up/up_devcontainer_source_test.go index 98e5898fa..41abb6791 100644 --- a/cmd/workspace/up/up_devcontainer_source_test.go +++ b/cmd/workspace/up/up_devcontainer_source_test.go @@ -29,7 +29,6 @@ func TestResolveDevContainerSource_Path(t *testing.T) { } func TestResolveDevContainerSource_ExternalPathPassThrough(t *testing.T) { - // An external absolute path stays in the source so the runner imports it. cmd := &UpCmd{GlobalFlags: &flags.GlobalFlags{}} cmd.DevContainerSource = "/abs/external/devcontainer.json" diff --git a/cmd/workspace/up/up_validate.go b/cmd/workspace/up/up_validate.go index 9ee440137..88795ab34 100644 --- a/cmd/workspace/up/up_validate.go +++ b/cmd/workspace/up/up_validate.go @@ -61,9 +61,6 @@ func (cmd *UpCmd) resolveDevContainerSource() error { cmd.DevContainerID = spec.ID cmd.DevContainerSource = "" case devcontainer.SourcePath: - // An in-repo relative path is just a config path; an external absolute - // path is left in the source so the runner imports it (bringing its - // sibling assets — Dockerfile, features — into the workspace). if !filepath.IsAbs(spec.Path) { cmd.DevContainerPath = spec.Path cmd.DevContainerSource = "" diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 09affef90..e25f0c65b 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -22,12 +22,6 @@ import ( "github.com/devsy-org/devsy/pkg/random" ) -// External --devcontainer configs are imported into the workspace as a named -// devcontainer profile (.devcontainer//), which the config discovery -// already recognizes and which keeps config-relative assets (Dockerfile, -// features, compose) resolving. importedProfileMarker is dropped inside the -// imported dir so teardown can identify and remove it regardless of any -// collision suffix. var ( importedProfileParent = filepath.ToSlash(devcontainerProfileParent) importedProfileMarker = "." + pkgconfig.BinaryName + "-imported" @@ -208,13 +202,6 @@ func (r *runner) rawConfigFromSource( } } -// importExternalDevContainer copies a devcontainer config that lives outside the -// workspace into it as a named profile (.devcontainer//), so config -// discovery finds it and config-relative assets (Dockerfile, features, compose) -// resolve. A self-contained "/devcontainer.json" has its whole folder -// copied; a bare file is copied on its own. The profile dir is added to the -// repo's local git exclude so it doesn't dirty git status, and is marked so -// teardown can remove it. func (r *runner) importExternalDevContainer(srcPath string) (*config.DevContainerConfig, error) { srcPath, err := filepath.Abs(srcPath) if err != nil { @@ -244,23 +231,15 @@ func (r *runner) importExternalDevContainer(srcPath string) (*config.DevContaine return rawConfig, nil } -// importedProfileRelDir returns the workspace-relative profile directory to -// import into: .devcontainer/, or .devcontainer/_ when -// the plain name already exists in the repo (so an imported profile never -// clobbers a config the project ships). func (r *runner) importedProfileRelDir() string { base := path.Join(importedProfileParent, importedProfileName) plain := filepath.Join(r.localWorkspaceFolder, filepath.FromSlash(base)) if isImportedProfileDir(plain) || !dirExists(plain) { - return base // reusable (ours) or free + return base } return base + "_" + random.String(6) } -// copyExternalDevContainer copies the external config into destDir, replacing -// any previous import. A self-contained config (its parent dir holds the -// devcontainer.json plus sibling assets) is imported whole; otherwise only the -// file is copied. func copyExternalDevContainer(srcPath, destDir string) error { if err := os.RemoveAll(destDir); err != nil { return fmt.Errorf("clear imported devcontainer dir: %w", err) @@ -281,15 +260,11 @@ func copyExternalDevContainer(srcPath, destDir string) error { return nil } -// dirExists reports whether path is an existing directory. func dirExists(path string) bool { info, err := os.Stat(path) return err == nil && info.IsDir() } -// isSelfContainedDevContainer reports whether the config at srcPath lives in a -// dedicated folder alongside build assets (Dockerfile, features, compose), in -// which case the whole folder must travel with it. func isSelfContainedDevContainer(srcPath string) bool { dir := filepath.Dir(srcPath) entries, err := os.ReadDir(dir) @@ -300,25 +275,20 @@ func isSelfContainedDevContainer(srcPath string) bool { if e.Name() == filepath.Base(srcPath) { continue } - return true // any sibling file/dir means the config isn't standalone + return true } return false } -// isImportedProfileDir reports whether dir is a previously imported profile -// (identified by the marker file), meaning it is safe for devsy to overwrite. func isImportedProfileDir(dir string) bool { _, err := os.Stat(filepath.Join(dir, importedProfileMarker)) return err == nil } -// excludeFromGit adds relPath to the workspace repo's .git/info/exclude so the -// imported files don't show up in git status. It is best-effort: a workspace -// that isn't a git repo (no .git/info) is simply skipped. func (r *runner) excludeFromGit(relPath string) error { excludePath := filepath.Join(r.localWorkspaceFolder, ".git", "info", "exclude") if _, err := os.Stat(filepath.Dir(excludePath)); err != nil { - return nil // not a git repo (or no info dir); nothing to do + return nil // nothing to do } // #nosec G304 -- path is under the workspace .git dir existing, err := os.ReadFile(excludePath) @@ -341,9 +311,6 @@ func (r *runner) excludeFromGit(relPath string) error { return err } -// CleanupImportedDevContainers removes any devcontainer profiles that devsy -// imported into workspaceFolder from an external --devcontainer path (those -// carrying importedProfileMarker). It is safe to call when none exist. func CleanupImportedDevContainers(workspaceFolder string) error { parent := filepath.Join(workspaceFolder, filepath.FromSlash(importedProfileParent)) entries, err := os.ReadDir(parent) diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index 4a9c32fbb..2b1b61adf 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -43,18 +43,12 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { return nil } -// cleanupDeliveryVolume removes the devsy-managed volumes created for this -// workspace. Best-effort: failures are logged, not returned. func (r *runner) cleanupDeliveryVolume(ctx context.Context) { if err := r.newAgentDelivery().Cleanup(ctx, r.id); err != nil { - log.Debugf("best-effort delivery volume cleanup: %v", err) + log.Debugf("delivery volume cleanup: %v", err) } } -// cleanupImportedDevContainer removes any devcontainer profile the runner -// imported from an external --devcontainer path into a local-folder source. -// Other sources keep their content under a devsy-managed folder that the caller -// deletes wholesale, so there is nothing to clean there. Best-effort. func (r *runner) cleanupImportedDevContainer() { if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil || @@ -62,7 +56,7 @@ func (r *runner) cleanupImportedDevContainer() { return } if err := CleanupImportedDevContainers(r.localWorkspaceFolder); err != nil { - log.Debugf("best-effort imported devcontainer cleanup: %v", err) + log.Debugf("imported devcontainer cleanup: %v", err) } } diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index b2163664c..21229eae6 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -178,7 +178,6 @@ func TestCleanupDeliveryVolume_DoesNotPanic(t *testing.T) { } func TestDelete_RemovesImportedDevContainer(t *testing.T) { - // A local-folder workspace with an imported profile: Delete must remove it. ws := t.TempDir() external := filepath.Join(t.TempDir(), "devcontainer.json") writeFile(t, external, `{"image":"alpine"}`) @@ -205,7 +204,6 @@ func TestDelete_RemovesImportedDevContainer(t *testing.T) { } func TestDelete_NonLocalSource_KeepsNothingToClean(t *testing.T) { - // No Workspace/local folder: cleanup must be a safe no-op. r := newTestRunner(&mockDriver{findResult: nil}) if err := r.Delete(context.Background(), DeleteOptions{}); err != nil { t.Fatalf("Delete failed: %v", err) diff --git a/pkg/devcontainer/import_external_test.go b/pkg/devcontainer/import_external_test.go index d1d85aef8..3cfc2581f 100644 --- a/pkg/devcontainer/import_external_test.go +++ b/pkg/devcontainer/import_external_test.go @@ -10,20 +10,17 @@ import ( "github.com/stretchr/testify/require" ) -// writeFile is a small test helper. func writeFile(t *testing.T, path, body string) { t.Helper() require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o750)) require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) } -// importedProfilePath is the default profile dir devsy imports into. func importedProfilePath(ws string) string { return filepath.Join(ws, filepath.FromSlash(importedProfileParent), importedProfileName) } func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { - // External, self-contained config: devcontainer.json + sibling Dockerfile. external := t.TempDir() writeFile(t, filepath.Join(external, "devcontainer.json"), `{"name":"ext","build":{"dockerfile":"Dockerfile"}}`) @@ -35,13 +32,10 @@ func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { cfg, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) require.NoError(t, err) - // The whole folder is imported under .devcontainer// so the - // Dockerfile travels with the config. imported := importedProfilePath(ws) assert.FileExists(t, filepath.Join(imported, "devcontainer.json")) assert.FileExists(t, filepath.Join(imported, "Dockerfile")) - // Origin points inside the workspace so a relative Dockerfile resolves. assert.Equal(t, filepath.Join(imported, "devcontainer.json"), cfg.Origin) assert.Equal(t, "Dockerfile", cfg.GetDockerfile()) dockerfile := filepath.Join(filepath.Dir(cfg.Origin), cfg.GetDockerfile()) @@ -49,7 +43,6 @@ func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { } func TestImportExternalDevContainer_BareFile(t *testing.T) { - // A lone devcontainer.json (no sibling assets) is copied on its own. external := t.TempDir() writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) @@ -79,7 +72,6 @@ func TestImportExternalDevContainer_CollisionGetsSuffix(t *testing.T) { external := t.TempDir() writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) - // The project already ships its own .devcontainer// (no marker). ws := t.TempDir() writeFile(t, filepath.Join(importedProfilePath(ws), "devcontainer.json"), `{"image":"project-owned"}`) @@ -88,7 +80,6 @@ func TestImportExternalDevContainer_CollisionGetsSuffix(t *testing.T) { cfg, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) require.NoError(t, err) - // The import must land in a suffixed sibling, not the project's dir. assert.Equal(t, "alpine", cfg.Image) base := filepath.Base(filepath.Dir(cfg.Origin)) assert.True(t, strings.HasPrefix(base, importedProfileName+"_"), @@ -102,7 +93,6 @@ func TestImportExternalDevContainer_ReusesOwnProfile(t *testing.T) { ws := t.TempDir() r := &runner{localWorkspaceFolder: ws} - // Two imports in a row must reuse the same (devsy-owned) profile dir. first, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) require.NoError(t, err) second, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) @@ -143,7 +133,6 @@ func TestCleanupImportedDevContainers(t *testing.T) { writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) ws := t.TempDir() - // A project-owned profile (no marker) must survive cleanup. writeFile(t, filepath.Join(ws, ".devcontainer", "app", "devcontainer.json"), `{}`) r := &runner{localWorkspaceFolder: ws} From c04f654f576b382f0b63ee347b0ab3ea93ec7f61 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 22 Jul 2026 17:38:30 -0500 Subject: [PATCH 3/3] fix(devcontainer): scope self-contained import to .devcontainer folders Address review feedback: - Only treat an external config as self-contained (folder copy) when it lives in a dedicated .devcontainer directory or a .devcontainer/ profile, matching the spec's layout. A config at a project root now copies just the file, so it can't pull in arbitrary sibling trees (secrets, .git, ...). - Preserve the original srcPath in the filepath.Abs error message instead of the empty string Abs returns on failure. --- pkg/devcontainer/config.go | 21 ++++++------ pkg/devcontainer/import_external_test.go | 42 +++++++++++++++++++++++- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index e25f0c65b..d6e32de50 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -203,10 +203,11 @@ func (r *runner) rawConfigFromSource( } func (r *runner) importExternalDevContainer(srcPath string) (*config.DevContainerConfig, error) { - srcPath, err := filepath.Abs(srcPath) + absPath, err := filepath.Abs(srcPath) if err != nil { return nil, fmt.Errorf("resolve devcontainer path %s: %w", srcPath, err) } + srcPath = absPath if _, err := os.Stat(srcPath); err != nil { return nil, fmt.Errorf("devcontainer path %s does not exist: %w", srcPath, err) } @@ -265,19 +266,19 @@ func dirExists(path string) bool { return err == nil && info.IsDir() } +// isSelfContainedDevContainer reports whether srcPath lives in a dedicated +// devcontainer directory whose sibling assets (Dockerfile, features, compose) +// belong with the config and must be copied alongside it. To avoid pulling in +// arbitrary trees, this is limited to the spec's own layout: the config's +// parent directory is ".devcontainer", or a profile directory nested directly +// under ".devcontainer" (".devcontainer//devcontainer.json"). A config +// that sits anywhere else (e.g. a project root) is treated as a bare file. func isSelfContainedDevContainer(srcPath string) bool { dir := filepath.Dir(srcPath) - entries, err := os.ReadDir(dir) - if err != nil { - return false - } - for _, e := range entries { - if e.Name() == filepath.Base(srcPath) { - continue - } + if filepath.Base(dir) == devcontainerProfileParent { return true } - return false + return filepath.Base(filepath.Dir(dir)) == devcontainerProfileParent } func isImportedProfileDir(dir string) bool { diff --git a/pkg/devcontainer/import_external_test.go b/pkg/devcontainer/import_external_test.go index 3cfc2581f..26eddb069 100644 --- a/pkg/devcontainer/import_external_test.go +++ b/pkg/devcontainer/import_external_test.go @@ -21,7 +21,8 @@ func importedProfilePath(ws string) string { } func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { - external := t.TempDir() + // A dedicated .devcontainer/ folder with a sibling Dockerfile. + external := filepath.Join(t.TempDir(), ".devcontainer") writeFile(t, filepath.Join(external, "devcontainer.json"), `{"name":"ext","build":{"dockerfile":"Dockerfile"}}`) writeFile(t, filepath.Join(external, "Dockerfile"), "FROM alpine\n") @@ -42,6 +43,45 @@ func TestImportExternalDevContainer_SelfContainedFolder(t *testing.T) { assert.FileExists(t, dockerfile) } +func TestImportExternalDevContainer_ProjectRootCopiesOnlyConfig(t *testing.T) { + // A config that lives at a project root (parent is NOT .devcontainer) must + // copy only the config file — never the surrounding tree (secrets, .git, + // node_modules, ...). + external := t.TempDir() + writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`) + writeFile(t, filepath.Join(external, "secret.env"), "TOKEN=shh") + writeFile(t, filepath.Join(external, ".git", "config"), "[core]") + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + + imported := importedProfilePath(ws) + assert.FileExists(t, filepath.Join(imported, "devcontainer.json")) + assert.NoFileExists(t, filepath.Join(imported, "secret.env"), + "must not copy sibling files from a non-.devcontainer parent") + assert.NoDirExists(t, filepath.Join(imported, ".git"), + "must not copy sibling dirs from a non-.devcontainer parent") +} + +func TestImportExternalDevContainer_ProfileFolderIsSelfContained(t *testing.T) { + // A named profile (.devcontainer//devcontainer.json) is self-contained + // and its siblings travel with it. + external := filepath.Join(t.TempDir(), ".devcontainer", "backend") + writeFile(t, filepath.Join(external, "devcontainer.json"), + `{"build":{"dockerfile":"Dockerfile"}}`) + writeFile(t, filepath.Join(external, "Dockerfile"), "FROM alpine\n") + + ws := t.TempDir() + r := &runner{localWorkspaceFolder: ws} + + _, err := r.importExternalDevContainer(filepath.Join(external, "devcontainer.json")) + require.NoError(t, err) + assert.FileExists(t, filepath.Join(importedProfilePath(ws), "Dockerfile")) +} + func TestImportExternalDevContainer_BareFile(t *testing.T) { external := t.TempDir() writeFile(t, filepath.Join(external, "devcontainer.json"), `{"image":"alpine"}`)