From 3e49ed93e204e64abf2e60321203c9362542506b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 24 Sep 2026 16:41:28 -0600 Subject: [PATCH 01/17] fix(devcontainer): persist profile selection --- cmd/ci/ci.go | 2 + cmd/workspace/build.go | 1 + cmd/workspace/up/agent.go | 1 + cmd/workspace/up/up_client.go | 1 + e2e/tests/up-docker-compose/config.go | 22 +++++ .../.devcontainer/compose.yaml | 4 + .../max-nvidia/devcontainer.json | 6 ++ .../.devcontainer/max/devcontainer.json | 6 ++ pkg/devcontainer/config.go | 84 +++++++++++++--- pkg/devcontainer/config_test.go | 99 ++++++++++++++++++- pkg/provider/workspace.go | 4 + pkg/workspace/workspace.go | 36 +++++-- pkg/workspace/workspace_test.go | 39 ++++++++ 13 files changed, 281 insertions(+), 24 deletions(-) create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/compose.yaml create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json create mode 100644 e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json diff --git a/cmd/ci/ci.go b/cmd/ci/ci.go index 2024812b6a..975ffc8418 100644 --- a/cmd/ci/ci.go +++ b/cmd/ci/ci.go @@ -291,6 +291,8 @@ func (cmd *CICmd) resolveWorkspace( ProviderUserOptions: cmd.ProviderOptions, DevContainerImage: cmd.DevContainerImage, DevContainerPath: cmd.DevContainerPath, + DevContainerID: cmd.DevContainerID, + DevContainerSource: cmd.DevContainerSource, SSHConfigPath: sshConfigPath, UID: cmd.UID, Owner: cmd.Owner, diff --git a/cmd/workspace/build.go b/cmd/workspace/build.go index b222148826..2908e7b903 100644 --- a/cmd/workspace/build.go +++ b/cmd/workspace/build.go @@ -158,6 +158,7 @@ func (cmd *BuildCmd) execute(ctx context.Context, args []string) error { ProviderUserOptions: cmd.ProviderOptions, DevContainerImage: cmd.DevContainerImage, DevContainerPath: cmd.DevContainerPath, + DevContainerID: cmd.DevContainerID, SSHConfigPath: sshConfigPath, UID: cmd.UID, Owner: cmd.Owner, diff --git a/cmd/workspace/up/agent.go b/cmd/workspace/up/agent.go index 5a4733945b..e980a4e4a7 100644 --- a/cmd/workspace/up/agent.go +++ b/cmd/workspace/up/agent.go @@ -146,6 +146,7 @@ func (cmd *UpCmd) buildWorkspaceOptions(workspace *provider2.Workspace) provider baseOptions := cmd.CLIOptions baseOptions.ID = workspace.ID baseOptions.DevContainerPath = workspace.DevContainerPath + baseOptions.DevContainerID = workspace.DevContainerID baseOptions.DevContainerImage = workspace.DevContainerImage baseOptions.DevContainerSource = workspace.DevContainerSource baseOptions.IDE = workspace.IDE.Name diff --git a/cmd/workspace/up/up_client.go b/cmd/workspace/up/up_client.go index cda32f8896..544ca93537 100644 --- a/cmd/workspace/up/up_client.go +++ b/cmd/workspace/up/up_client.go @@ -217,6 +217,7 @@ func (cmd *UpCmd) resolveParams( ReconfigureProvider: cmd.Reconfigure, DevContainerImage: cmd.DevContainerImage, DevContainerPath: cmd.DevContainerPath, + DevContainerID: cmd.DevContainerID, DevContainerSource: cmd.DevContainerSource, SSHConfigPath: cmd.SSHConfigPath, SSHConfigIncludePath: devsyConfig.ContextOption( diff --git a/e2e/tests/up-docker-compose/config.go b/e2e/tests/up-docker-compose/config.go index f394fcdc95..76ac7d83a5 100644 --- a/e2e/tests/up-docker-compose/config.go +++ b/e2e/tests/up-docker-compose/config.go @@ -17,6 +17,7 @@ import ( pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/devcontainer/config" docker "github.com/devsy-org/devsy/pkg/docker" + "github.com/devsy-org/devsy/pkg/flags/names" provider2 "github.com/devsy-org/devsy/pkg/provider" "github.com/devsy-org/devsy/pkg/status" "github.com/docker/docker/api/types/container" @@ -142,6 +143,27 @@ var _ = ginkgo.Describe( gomega.Expect(restartIds).To(gomega.HaveLen(1), "1 compose container after restart") }, ginkgo.SpecTimeout(framework.TimeoutLong())) + ginkgo.It("multi-profile lifecycle retains selected profile", func(ctx context.Context) { + tempDir, err := setupWorkspace( + "tests/up-docker-compose/testdata/docker-compose-multi-profile", + tc.initialDir, + tc.f, + ) + framework.ExpectNoError(err) + + err = tc.f.DevsyUp(ctx, names.Flag(names.DevContainer), "id:max", tempDir) + framework.ExpectNoError(err) + + err = tc.f.DevsyWorkspaceStop(ctx, tempDir) + framework.ExpectNoError(err) + + err = tc.f.DevsyUp(ctx, tempDir) + framework.ExpectNoError(err) + + err = tc.f.DevsyWorkspaceDelete(ctx, tempDir) + framework.ExpectNoError(err) + }, ginkgo.SpecTimeout(framework.TimeoutLong())) + ginkgo.It("environment variables", func(ctx context.Context) { _, workspace, err := tc.setupAndStartWorkspace( ctx, diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/compose.yaml b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/compose.yaml new file mode 100644 index 0000000000..2b9da8bdff --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/compose.yaml @@ -0,0 +1,4 @@ +services: + app: + image: ghcr.io/devsy-org/test-images/go:1 + command: sleep infinity diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json new file mode 100644 index 0000000000..66a81b69a5 --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json @@ -0,0 +1,6 @@ +{ + "name": "max-nvidia", + "dockerComposeFile": "../compose.yaml", + "service": "app", + "workspaceFolder": "/workspaces" +} diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json new file mode 100644 index 0000000000..bfc5a6381e --- /dev/null +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json @@ -0,0 +1,6 @@ +{ + "name": "max", + "dockerComposeFile": "../compose.yaml", + "service": "app", + "workspaceFolder": "/workspaces" +} diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index c886bea5ee..9d0b7ac86c 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -43,15 +43,14 @@ func (r *runner) getRawConfigWithContext( ctx context.Context, options provider.CLIOptions, ) (*config.DevContainerConfig, error) { - source := options.DevContainerSource - if source == "" { - source = r.workspaceConfig.Workspace.DevContainerSource + selection := r.effectiveDevContainerSelection(options) + if selection.source != "" { + return r.rawConfigFromSourceWithContext(ctx, selection.source, options) } - if source != "" { - return r.rawConfigFromSourceWithContext(ctx, source, options) - } - if conf := r.rawConfigFromWorkspace(); conf != nil { - return conf, nil + if selection.path == "" && selection.id == "" { + if conf := r.rawConfigFromWorkspace(); conf != nil { + return conf, nil + } } if conf := r.rawConfigFromContainer(); conf != nil { return conf, nil @@ -59,13 +58,67 @@ func (r *runner) getRawConfigWithContext( if crane.ShouldUse(&options) { return r.rawConfigFromCraneWithContext(ctx, options) } - return r.rawConfigFromFilesystemWithContext(ctx, options) + return r.rawConfigFromFilesystemWithContext(ctx, options, selection) +} + +type devContainerSelection struct { + source string + path string + id string +} + +// effectiveDevContainerSelection returns the one effective selector for this +// operation. Current CLI input wins over persisted workspace state, followed +// by the last resolved path for compatibility with older workspaces. +func (r *runner) effectiveDevContainerSelection( + options provider.CLIOptions, +) devContainerSelection { + if selection, ok := newDevContainerSelection( + options.DevContainerSource, + options.DevContainerPath, + options.DevContainerID, + ); ok { + return selection + } + + if r.workspaceConfig != nil && r.workspaceConfig.Workspace != nil { + workspace := r.workspaceConfig.Workspace + if selection, ok := newDevContainerSelection( + workspace.DevContainerSource, + workspace.DevContainerPath, + workspace.DevContainerID, + ); ok { + return selection + } + } + + if r.workspaceConfig != nil && r.workspaceConfig.LastDevContainerConfig != nil { + if path := r.workspaceConfig.LastDevContainerConfig.Path; path != "" { + return devContainerSelection{path: path} + } + } + + return devContainerSelection{} +} + +func newDevContainerSelection(source, path, id string) (devContainerSelection, bool) { + switch { + case source != "": + return devContainerSelection{source: source}, true + case path != "": + return devContainerSelection{path: path}, true + case id != "": + return devContainerSelection{id: id}, true + default: + return devContainerSelection{}, false + } } // rawConfigFromWorkspace returns the config embedded in the workspace metadata, // or nil when none is present. func (r *runner) rawConfigFromWorkspace() *config.DevContainerConfig { - if r.workspaceConfig.Workspace.DevContainerConfig == nil { + if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil || + r.workspaceConfig.Workspace.DevContainerConfig == nil { return nil } @@ -130,12 +183,15 @@ func (r *runner) rawConfigFromCraneWithContext( func (r *runner) rawConfigFromFilesystem( options provider.CLIOptions, ) (*config.DevContainerConfig, error) { - return r.rawConfigFromFilesystemWithContext(context.Background(), options) + return r.rawConfigFromFilesystemWithContext( + context.Background(), options, r.effectiveDevContainerSelection(options), + ) } func (r *runner) rawConfigFromFilesystemWithContext( ctx context.Context, options provider.CLIOptions, + selection devContainerSelection, ) (*config.DevContainerConfig, error) { localWorkspaceFolder := r.localWorkspaceFolder if subPath := r.workspaceConfig.Workspace.Source.GitSubPath; subPath != "" { @@ -143,11 +199,11 @@ func (r *runner) rawConfigFromFilesystemWithContext( } opts := config.ParseOptions{Selector: config.SelectSingle(localWorkspaceFolder)} - if options.DevContainerID != "" { + if selection.id != "" { // An explicit id must not be shadowed by a root config, and a mismatch // must error rather than silently fall back. opts = config.ParseOptions{ - Selector: config.SelectByID(options.DevContainerID), + Selector: config.SelectByID(selection.id), ForceSelect: true, } } @@ -155,7 +211,7 @@ func (r *runner) rawConfigFromFilesystemWithContext( rawConfig, err := config.ParseDevContainerJSONWithOptions( ctx, localWorkspaceFolder, - r.workspaceConfig.Workspace.DevContainerPath, + selection.path, opts, ) // A missing devcontainer.json is not an error: fall back to auto-detection. diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 66b6037ab7..5a58cb31d6 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -3,6 +3,7 @@ package devcontainer import ( "os" "path/filepath" + "strings" "testing" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -10,7 +11,10 @@ import ( "github.com/stretchr/testify/suite" ) -const testWorkspaceFolder = "/workspace" +const ( + testWorkspaceFolder = "/workspace" + testDevContainerProfile = "max" +) type SubstituteTestSuite struct { suite.Suite @@ -478,6 +482,20 @@ func seedAmbiguousProfiles(t *testing.T, folder string) { } } +func seedNamedProfiles(t *testing.T, folder string, ids ...string) { + t.Helper() + for _, id := range ids { + dir := filepath.Join(folder, ".devcontainer", id) + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + body := []byte(`{"image":"` + id + `"}`) + if err := os.WriteFile(filepath.Join(dir, "devcontainer.json"), body, 0o600); err != nil { + t.Fatal(err) + } + } +} + func TestGetRawConfig_SourceImageBypassesDiscovery(t *testing.T) { folder := t.TempDir() seedAmbiguousProfiles(t, folder) @@ -571,6 +589,85 @@ func TestGetRawConfig_CLISourceOverridesPersisted(t *testing.T) { } } +func TestGetRawConfig_PersistedIDSelectsProfile(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile, "max-nvidia", "max-vaapi") + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerID = testDevContainerProfile + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != testDevContainerProfile { + t.Errorf("Image = %q, want %s", conf.Image, testDevContainerProfile) + } +} + +func TestGetRawConfig_ExplicitSelectorOverridesPersistedSelector(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile, "max-nvidia") + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerSource = "image:persisted" + + conf, err := r.getRawConfig(provider2.CLIOptions{DevContainerID: "max-nvidia"}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "max-nvidia" { + t.Errorf("Image = %q, want max-nvidia", conf.Image) + } +} + +func TestGetRawConfig_ExplicitPathOverridesPersistedID(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile, "max-vaapi") + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerID = testDevContainerProfile + + conf, err := r.getRawConfig(provider2.CLIOptions{ + DevContainerPath: ".devcontainer/max-vaapi/devcontainer.json", + }) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "max-vaapi" { + t.Errorf("Image = %q, want max-vaapi", conf.Image) + } +} + +func TestGetRawConfig_LastConfigPathCompatibilityFallback(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, "max", "max-nvidia") + r := newRunnerAt(folder) + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: ".devcontainer/" + testDevContainerProfile + "/devcontainer.json", + } + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != testDevContainerProfile { + t.Errorf("Image = %q, want %s", conf.Image, testDevContainerProfile) + } +} + +func TestGetRawConfig_InvalidPersistedID(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile, "max-nvidia") + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerID = "missing" + + _, err := r.getRawConfig(provider2.CLIOptions{}) + if err == nil { + t.Fatal("expected invalid persisted ID to fail") + } + if !strings.Contains(err.Error(), `devcontainer with ID "missing" not found`) { + t.Fatalf("error = %q, want missing ID error", err) + } +} + func TestGetRawConfig_SourceNoneWithImageBypassesDiscovery(t *testing.T) { folder := t.TempDir() seedAmbiguousProfiles(t, folder) diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index da3ed2d1f5..1a59865157 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -50,6 +50,10 @@ type Workspace struct { // DevContainerPath is the relative path where the devcontainer.json is located. DevContainerPath string `json:"devContainerPath,omitempty"` + // DevContainerID is the selected named devcontainer profile. It is persisted + // so lifecycle operations reuse the same profile instead of rediscovering it. + DevContainerID string `json:"devContainerID,omitempty"` + // DevContainerSource is the devcontainer source override (e.g. "image:" // or "none") that ignores the project's devcontainer.json. It is persisted so // restarts reuse the same override instead of falling back to discovery. diff --git a/pkg/workspace/workspace.go b/pkg/workspace/workspace.go index 21a5abdb67..687f3541fc 100644 --- a/pkg/workspace/workspace.go +++ b/pkg/workspace/workspace.go @@ -54,6 +54,7 @@ type ResolveParams struct { ReconfigureProvider bool DevContainerImage string DevContainerPath string + DevContainerID string DevContainerSource string SSHConfigPath string SSHConfigIncludePath string @@ -142,20 +143,37 @@ func applyDevContainerOverrides(workspace *providerpkg.Workspace, params Resolve } func applyDevContainerFields(workspace *providerpkg.Workspace, params ResolveParams) bool { - changed := false + changed := applyDevContainerSelection(workspace, params) if params.DevContainerImage != "" && workspace.DevContainerImage != params.DevContainerImage { workspace.DevContainerImage = params.DevContainerImage changed = true } - if params.DevContainerPath != "" && workspace.DevContainerPath != params.DevContainerPath { - workspace.DevContainerPath = params.DevContainerPath - changed = true - } - if params.DevContainerSource != "" && - workspace.DevContainerSource != params.DevContainerSource { - workspace.DevContainerSource = params.DevContainerSource - changed = true + return changed +} + +func applyDevContainerSelection(workspace *providerpkg.Workspace, params ResolveParams) bool { + switch { + case params.DevContainerSource != "": + return setDevContainerSelection(workspace, params.DevContainerSource, "", "") + case params.DevContainerPath != "": + return setDevContainerSelection(workspace, "", params.DevContainerPath, "") + case params.DevContainerID != "": + return setDevContainerSelection(workspace, "", "", params.DevContainerID) + default: + return false } +} + +func setDevContainerSelection( + workspace *providerpkg.Workspace, + source, path, id string, +) bool { + changed := workspace.DevContainerSource != source || + workspace.DevContainerPath != path || + workspace.DevContainerID != id + workspace.DevContainerSource = source + workspace.DevContainerPath = path + workspace.DevContainerID = id return changed } diff --git a/pkg/workspace/workspace_test.go b/pkg/workspace/workspace_test.go index b8042055e7..5c4e45b2af 100644 --- a/pkg/workspace/workspace_test.go +++ b/pkg/workspace/workspace_test.go @@ -202,6 +202,7 @@ func TestApplyDevContainerOverrides_NoOp(t *testing.T) { require.NoError(t, applyDevContainerOverrides(ws, ResolveParams{})) assert.Empty(t, ws.DevContainerImage) assert.Empty(t, ws.DevContainerPath) + assert.Empty(t, ws.DevContainerID) } func TestApplyDevContainerOverrides_SetsImageAndPath(t *testing.T) { @@ -239,3 +240,41 @@ func TestApplyDevContainerOverrides_PersistsSource(t *testing.T) { require.NoError(t, err) assert.Equal(t, source, loaded.DevContainerSource) } + +func TestApplyDevContainerOverrides_PersistsIDAndClearsOtherSelectors(t *testing.T) { + setupTestPathManager(t) + + ws := &providerpkg.Workspace{ + ID: "ws-id", + Context: testDefaultContext, + DevContainerPath: ".devcontainer/old/devcontainer.json", + DevContainerSource: "image:old", + } + require.NoError(t, applyDevContainerOverrides(ws, ResolveParams{DevContainerID: "max"})) + + assert.Equal(t, "max", ws.DevContainerID) + assert.Empty(t, ws.DevContainerPath) + assert.Empty(t, ws.DevContainerSource) + + loaded, err := providerpkg.LoadWorkspaceConfig(testDefaultContext, ws.ID) + require.NoError(t, err) + assert.Equal(t, "max", loaded.DevContainerID) + assert.Empty(t, loaded.DevContainerPath) + assert.Empty(t, loaded.DevContainerSource) +} + +func TestApplyDevContainerOverrides_SelectorReplacement(t *testing.T) { + ws := &providerpkg.Workspace{DevContainerID: "max"} + + changed := applyDevContainerFields(ws, ResolveParams{DevContainerPath: testDevContainerPath}) + assert.True(t, changed) + assert.Empty(t, ws.DevContainerID) + assert.Equal(t, testDevContainerPath, ws.DevContainerPath) + assert.Empty(t, ws.DevContainerSource) + + changed = applyDevContainerFields(ws, ResolveParams{DevContainerSource: "image:python"}) + assert.True(t, changed) + assert.Empty(t, ws.DevContainerID) + assert.Empty(t, ws.DevContainerPath) + assert.Equal(t, "image:python", ws.DevContainerSource) +} From 348048c99110474133287d37a42d872dab03c866 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 24 Sep 2026 18:33:31 -0600 Subject: [PATCH 02/17] fix(devcontainer): normalize legacy subpath --- pkg/devcontainer/config.go | 24 +++++++++++++++++++++++- pkg/devcontainer/config_test.go | 13 +++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 9d0b7ac86c..9fc2342271 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -94,13 +94,35 @@ func (r *runner) effectiveDevContainerSelection( if r.workspaceConfig != nil && r.workspaceConfig.LastDevContainerConfig != nil { if path := r.workspaceConfig.LastDevContainerConfig.Path; path != "" { - return devContainerSelection{path: path} + return devContainerSelection{path: r.compatibilityDevContainerPath(path)} } } return devContainerSelection{} } +// compatibilityDevContainerPath converts the last resolved path, which is +// stored relative to the content root, to the workspace folder used by the +// resolver. Remote workspaces with a git subpath otherwise apply the subpath +// twice during a later lifecycle operation. +func (r *runner) compatibilityDevContainerPath(lastPath string) string { + if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil { + return lastPath + } + + subPath := filepath.Clean(filepath.FromSlash(r.workspaceConfig.Workspace.Source.GitSubPath)) + if subPath == "." || subPath == "" { + return lastPath + } + + relativePath, err := filepath.Rel(subPath, filepath.FromSlash(lastPath)) + if err != nil || relativePath == ".." || + strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) { + return lastPath + } + return filepath.ToSlash(relativePath) +} + func newDevContainerSelection(source, path, id string) (devContainerSelection, bool) { switch { case source != "": diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 5a58cb31d6..e9f5402f76 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -653,6 +653,19 @@ func TestGetRawConfig_LastConfigPathCompatibilityFallback(t *testing.T) { } } +func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { + r := newRunnerAt(t.TempDir()) + r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json", + } + + selection := r.effectiveDevContainerSelection(provider2.CLIOptions{}) + if selection.path != ".devcontainer/devcontainer.json" { + t.Fatalf("selection path = %q, want .devcontainer/devcontainer.json", selection.path) + } +} + func TestGetRawConfig_InvalidPersistedID(t *testing.T) { folder := t.TempDir() seedNamedProfiles(t, folder, testDevContainerProfile, "max-nvidia") From d5a409b3baaf7dd78da54be0bd39edac43a6e518 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 24 Sep 2026 19:20:33 -0600 Subject: [PATCH 03/17] fix(devcontainer): normalize persisted subpath --- pkg/devcontainer/config.go | 23 +++++++++++++++++------ pkg/devcontainer/config_test.go | 11 +++++++++++ 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 9fc2342271..dedd5fc2f2 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -82,12 +82,7 @@ func (r *runner) effectiveDevContainerSelection( } if r.workspaceConfig != nil && r.workspaceConfig.Workspace != nil { - workspace := r.workspaceConfig.Workspace - if selection, ok := newDevContainerSelection( - workspace.DevContainerSource, - workspace.DevContainerPath, - workspace.DevContainerID, - ); ok { + if selection, ok := r.persistedDevContainerSelection(); ok { return selection } } @@ -101,6 +96,22 @@ func (r *runner) effectiveDevContainerSelection( return devContainerSelection{} } +func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) { + workspace := r.workspaceConfig.Workspace + switch { + case workspace.DevContainerSource != "": + return devContainerSelection{source: workspace.DevContainerSource}, true + case workspace.DevContainerPath != "": + return devContainerSelection{ + path: r.compatibilityDevContainerPath(workspace.DevContainerPath), + }, true + case workspace.DevContainerID != "": + return devContainerSelection{id: workspace.DevContainerID}, true + default: + return devContainerSelection{}, false + } +} + // compatibilityDevContainerPath converts the last resolved path, which is // stored relative to the content root, to the workspace folder used by the // resolver. Remote workspaces with a git subpath otherwise apply the subpath diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index e9f5402f76..41eadb4f46 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -666,6 +666,17 @@ func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { } } +func TestEffectiveDevContainerSelection_PersistedPathStripsGitSubPath(t *testing.T) { + r := newRunnerAt(t.TempDir()) + r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" + r.workspaceConfig.Workspace.DevContainerPath = "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json" + + selection := r.effectiveDevContainerSelection(provider2.CLIOptions{}) + if selection.path != ".devcontainer/devcontainer.json" { + t.Fatalf("selection path = %q, want .devcontainer/devcontainer.json", selection.path) + } +} + func TestGetRawConfig_InvalidPersistedID(t *testing.T) { folder := t.TempDir() seedNamedProfiles(t, folder, testDevContainerProfile, "max-nvidia") From 0b91ef8d982d79c3a785248691c253056b4c10af Mon Sep 17 00:00:00 2001 From: skevetter Date: Thu, 24 Sep 2026 19:12:05 -0700 Subject: [PATCH 04/17] fix(devcontainer): restore embedded config priority and pin path semantics A workspace with an embedded devcontainer config and a legacy last-resolved path kept reading from the filesystem after profile selection was introduced; the last-path fallback now applies only when no embedded config exists, restoring the previous behavior. Workspace.DevContainerPath is now documented and used consistently as relative to the workspace folder (content root including the git subpath), while the last resolved path stays relative to the content root. The generic subpath-stripping shim corrupted persisted paths whose first segment matched the subpath; the exact conversion now applies only to the legacy last-path fallback. Both regressions are pinned by tests that fail without this change. Also drops the no-op DevContainerID pass-through in the build command (no flag can set it) and adds the missing nil guard on the git subpath lookup. --- cmd/workspace/build.go | 1 - pkg/devcontainer/config.go | 47 +++++++++++++++-------- pkg/devcontainer/config/result.go | 4 +- pkg/devcontainer/config_test.go | 63 ++++++++++++++++++++++++++++--- pkg/provider/workspace.go | 3 +- 5 files changed, 95 insertions(+), 23 deletions(-) diff --git a/cmd/workspace/build.go b/cmd/workspace/build.go index 2908e7b903..b222148826 100644 --- a/cmd/workspace/build.go +++ b/cmd/workspace/build.go @@ -158,7 +158,6 @@ func (cmd *BuildCmd) execute(ctx context.Context, args []string) error { ProviderUserOptions: cmd.ProviderOptions, DevContainerImage: cmd.DevContainerImage, DevContainerPath: cmd.DevContainerPath, - DevContainerID: cmd.DevContainerID, SSHConfigPath: sshConfigPath, UID: cmd.UID, Owner: cmd.Owner, diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index dedd5fc2f2..e73be191c1 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -87,9 +87,14 @@ func (r *runner) effectiveDevContainerSelection( } } - if r.workspaceConfig != nil && r.workspaceConfig.LastDevContainerConfig != nil { + // The last resolved path is a fallback for workspaces created before the + // selection was persisted. A workspace with an embedded config keeps using + // it, as it did before profile selection existed. + hasEmbeddedConfig := r.workspaceConfig != nil && r.workspaceConfig.Workspace != nil && + r.workspaceConfig.Workspace.DevContainerConfig != nil + if !hasEmbeddedConfig && r.workspaceConfig != nil && r.workspaceConfig.LastDevContainerConfig != nil { if path := r.workspaceConfig.LastDevContainerConfig.Path; path != "" { - return devContainerSelection{path: r.compatibilityDevContainerPath(path)} + return devContainerSelection{path: r.workspaceRelativeLastConfigPath(path)} } } @@ -102,9 +107,7 @@ func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) case workspace.DevContainerSource != "": return devContainerSelection{source: workspace.DevContainerSource}, true case workspace.DevContainerPath != "": - return devContainerSelection{ - path: r.compatibilityDevContainerPath(workspace.DevContainerPath), - }, true + return devContainerSelection{path: workspace.DevContainerPath}, true case workspace.DevContainerID != "": return devContainerSelection{id: workspace.DevContainerID}, true default: @@ -112,11 +115,14 @@ func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) } } -// compatibilityDevContainerPath converts the last resolved path, which is -// stored relative to the content root, to the workspace folder used by the -// resolver. Remote workspaces with a git subpath otherwise apply the subpath -// twice during a later lifecycle operation. -func (r *runner) compatibilityDevContainerPath(lastPath string) string { +// workspaceRelativeLastConfigPath converts the last resolved path, which is +// stored relative to the content root (see DevContainerConfigWithPath.Path), +// to the workspace folder the resolver runs against: the content root plus +// the git subpath. Remote workspaces with a subpath otherwise apply the +// subpath twice during a later lifecycle operation. A path outside the +// subpath cannot be expressed relative to the workspace folder and is +// returned unchanged, matching the previous discovery behavior. +func (r *runner) workspaceRelativeLastConfigPath(lastPath string) string { if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil { return lastPath } @@ -134,6 +140,20 @@ func (r *runner) compatibilityDevContainerPath(lastPath string) string { return filepath.ToSlash(relativePath) } +// workspaceFolder returns the folder the devcontainer resolver runs against: +// the content root plus the git subpath, when any. Workspace.DevContainerPath +// and CLI-provided devcontainer paths are relative to this folder. +func (r *runner) workspaceFolder() string { + if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil { + return r.localWorkspaceFolder + } + subPath := r.workspaceConfig.Workspace.Source.GitSubPath + if subPath == "" { + return r.localWorkspaceFolder + } + return filepath.Join(r.localWorkspaceFolder, filepath.FromSlash(subPath)) +} + func newDevContainerSelection(source, path, id string) (devContainerSelection, bool) { switch { case source != "": @@ -157,7 +177,7 @@ func (r *runner) rawConfigFromWorkspace() *config.DevContainerConfig { rawConfig := config.CloneDevContainerConfig(r.workspaceConfig.Workspace.DevContainerConfig) if devContainerPath := r.workspaceConfig.Workspace.DevContainerPath; devContainerPath != "" { - rawConfig.Origin = path.Join(filepath.ToSlash(r.localWorkspaceFolder), devContainerPath) + rawConfig.Origin = path.Join(filepath.ToSlash(r.workspaceFolder()), devContainerPath) } else { rawConfig.Origin = path.Join( filepath.ToSlash(r.localWorkspaceFolder), @@ -226,10 +246,7 @@ func (r *runner) rawConfigFromFilesystemWithContext( options provider.CLIOptions, selection devContainerSelection, ) (*config.DevContainerConfig, error) { - localWorkspaceFolder := r.localWorkspaceFolder - if subPath := r.workspaceConfig.Workspace.Source.GitSubPath; subPath != "" { - localWorkspaceFolder = filepath.Join(localWorkspaceFolder, subPath) - } + localWorkspaceFolder := r.workspaceFolder() opts := config.ParseOptions{Selector: config.SelectSingle(localWorkspaceFolder)} if selection.id != "" { diff --git a/pkg/devcontainer/config/result.go b/pkg/devcontainer/config/result.go index 9fdddf33b4..1f10b51a2f 100644 --- a/pkg/devcontainer/config/result.go +++ b/pkg/devcontainer/config/result.go @@ -42,7 +42,9 @@ type DevContainerConfigWithPath struct { // Config is the devcontainer.json config Config *DevContainerConfig `json:"config,omitempty"` - // Path is the relative path to the devcontainer.json from the workspace folder + // Path is the path to the devcontainer.json relative to the content root + // (the clone root for git workspaces, without the git subpath). Callers + // resolving against the workspace folder must convert for the subpath. Path string `json:"path,omitempty"` } diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 41eadb4f46..b9853b3920 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -666,14 +666,67 @@ func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { } } -func TestEffectiveDevContainerSelection_PersistedPathStripsGitSubPath(t *testing.T) { +// The last resolved path is content-root-relative, so a real nested path +// whose first segment matches the subpath converts exactly once. +func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testing.T) { r := newRunnerAt(t.TempDir()) - r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" - r.workspaceConfig.Workspace.DevContainerPath = "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json" + r.workspaceConfig.Workspace.Source.GitSubPath = "app" + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: "app/app/.devcontainer/devcontainer.json", + } selection := r.effectiveDevContainerSelection(provider2.CLIOptions{}) - if selection.path != ".devcontainer/devcontainer.json" { - t.Fatalf("selection path = %q, want .devcontainer/devcontainer.json", selection.path) + if selection.path != "app/.devcontainer/devcontainer.json" { + t.Fatalf("selection path = %q, want app/.devcontainer/devcontainer.json", selection.path) + } +} + +// A workspace that carries an embedded config (from the provider protocol) +// and a legacy last-resolved path must keep using the embedded config, as it +// did before profile selection was persisted: the last-path fallback exists +// only for workspaces with no other config source. +func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile) + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ + ImageContainer: config.ImageContainer{Image: "embedded"}, + } + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: ".devcontainer/" + testDevContainerProfile + "/devcontainer.json", + } + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "embedded" { + t.Errorf("Image = %q, want embedded (legacy last path must not override the embedded config)", conf.Image) + } +} + +// A persisted --devcontainer-path is relative to the workspace folder (the +// content root including the git subpath), so a leading segment that matches +// the subpath is part of the real path and must not be stripped. +func TestGetRawConfig_PersistedPathRepeatedSubPathSegment(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "app", "app", ".devcontainer") + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "devcontainer.json"), []byte(`{"image":"nested"}`), 0o600); err != nil { + t.Fatal(err) + } + r := newRunnerAt(root) + r.workspaceConfig.Workspace.Source.GitSubPath = "app" + r.workspaceConfig.Workspace.DevContainerPath = "app/.devcontainer/devcontainer.json" + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "nested" { + t.Errorf("Image = %q, want nested", conf.Image) } } diff --git a/pkg/provider/workspace.go b/pkg/provider/workspace.go index 1a59865157..08d2e78297 100644 --- a/pkg/provider/workspace.go +++ b/pkg/provider/workspace.go @@ -47,7 +47,8 @@ type Workspace struct { // DevContainerImage is the container image to use, overriding whatever is in the devcontainer.json DevContainerImage string `json:"devContainerImage,omitempty"` - // DevContainerPath is the relative path where the devcontainer.json is located. + // DevContainerPath is the path to the devcontainer.json relative to the + // workspace folder: the content root including the git subpath, if any. DevContainerPath string `json:"devContainerPath,omitempty"` // DevContainerID is the selected named devcontainer profile. It is persisted From 0af0e67518a9eb1a7f569b239c67c11a73153682 Mon Sep 17 00:00:00 2001 From: skevetter Date: Thu, 24 Sep 2026 19:24:18 -0700 Subject: [PATCH 05/17] refactor(devcontainer): extract last-path fallback and wrap long lines Splits the legacy last-config-path fallback out of effectiveDevContainerSelection to satisfy the cyclomatic complexity limit, names the repeated test subpath, and wraps lines golines flags. --- pkg/devcontainer/config.go | 29 ++++++++++++++++++++--------- pkg/devcontainer/config_test.go | 22 +++++++++++++++------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index e73be191c1..0c318caaf4 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -87,20 +87,31 @@ func (r *runner) effectiveDevContainerSelection( } } - // The last resolved path is a fallback for workspaces created before the - // selection was persisted. A workspace with an embedded config keeps using - // it, as it did before profile selection existed. - hasEmbeddedConfig := r.workspaceConfig != nil && r.workspaceConfig.Workspace != nil && - r.workspaceConfig.Workspace.DevContainerConfig != nil - if !hasEmbeddedConfig && r.workspaceConfig != nil && r.workspaceConfig.LastDevContainerConfig != nil { - if path := r.workspaceConfig.LastDevContainerConfig.Path; path != "" { - return devContainerSelection{path: r.workspaceRelativeLastConfigPath(path)} - } + if selection, ok := r.lastConfigPathSelection(); ok { + return selection } return devContainerSelection{} } +// lastConfigPathSelection falls back to the last resolved path for workspaces +// created before the selection was persisted. A workspace with an embedded +// config keeps using it, as it did before profile selection existed. +func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { + if r.workspaceConfig == nil || r.workspaceConfig.LastDevContainerConfig == nil { + return devContainerSelection{}, false + } + workspace := r.workspaceConfig.Workspace + if workspace != nil && workspace.DevContainerConfig != nil { + return devContainerSelection{}, false + } + path := r.workspaceConfig.LastDevContainerConfig.Path + if path == "" { + return devContainerSelection{}, false + } + return devContainerSelection{path: r.workspaceRelativeLastConfigPath(path)}, true +} + func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) { workspace := r.workspaceConfig.Workspace switch { diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index b9853b3920..4ab462a0e1 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -14,6 +14,7 @@ import ( const ( testWorkspaceFolder = "/workspace" testDevContainerProfile = "max" + testNestedSubPath = "app" ) type SubstituteTestSuite struct { @@ -670,14 +671,17 @@ func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { // whose first segment matches the subpath converts exactly once. func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testing.T) { r := newRunnerAt(t.TempDir()) - r.workspaceConfig.Workspace.Source.GitSubPath = "app" + r.workspaceConfig.Workspace.Source.GitSubPath = testNestedSubPath r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ - Path: "app/app/.devcontainer/devcontainer.json", + Path: testNestedSubPath + "/app/.devcontainer/devcontainer.json", } selection := r.effectiveDevContainerSelection(provider2.CLIOptions{}) if selection.path != "app/.devcontainer/devcontainer.json" { - t.Fatalf("selection path = %q, want app/.devcontainer/devcontainer.json", selection.path) + t.Fatalf( + "selection path = %q, want app/.devcontainer/devcontainer.json", + selection.path, + ) } } @@ -701,7 +705,10 @@ func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { t.Fatalf("getRawConfig: %v", err) } if conf.Image != "embedded" { - t.Errorf("Image = %q, want embedded (legacy last path must not override the embedded config)", conf.Image) + t.Errorf( + "Image = %q, want embedded (last path must not override the embedded config)", + conf.Image, + ) } } @@ -710,15 +717,16 @@ func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { // the subpath is part of the real path and must not be stripped. func TestGetRawConfig_PersistedPathRepeatedSubPathSegment(t *testing.T) { root := t.TempDir() - dir := filepath.Join(root, "app", "app", ".devcontainer") + dir := filepath.Join(root, testNestedSubPath, testNestedSubPath, ".devcontainer") if err := os.MkdirAll(dir, 0o750); err != nil { t.Fatal(err) } - if err := os.WriteFile(filepath.Join(dir, "devcontainer.json"), []byte(`{"image":"nested"}`), 0o600); err != nil { + configFile := filepath.Join(dir, "devcontainer.json") + if err := os.WriteFile(configFile, []byte(`{"image":"nested"}`), 0o600); err != nil { t.Fatal(err) } r := newRunnerAt(root) - r.workspaceConfig.Workspace.Source.GitSubPath = "app" + r.workspaceConfig.Workspace.Source.GitSubPath = testNestedSubPath r.workspaceConfig.Workspace.DevContainerPath = "app/.devcontainer/devcontainer.json" conf, err := r.getRawConfig(provider2.CLIOptions{}) From 85b766def7c388356023045b524bff1341bf7ef0 Mon Sep 17 00:00:00 2001 From: skevetter Date: Thu, 24 Sep 2026 19:36:03 -0700 Subject: [PATCH 06/17] fix(devcontainer): normalize leading slash in stored git subpath The stored subpath can carry a leading slash (@subpath:/x/y) while the last resolved path is always repo-relative, so filepath.Rel failed and the subpath was applied twice on recreate/reset. This is the failure up-workspaces showed on c14b895, now pinned by a test. --- pkg/devcontainer/config.go | 3 +++ pkg/devcontainer/config_test.go | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 0c318caaf4..c4bb19c481 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -138,7 +138,10 @@ func (r *runner) workspaceRelativeLastConfigPath(lastPath string) string { return lastPath } + // The stored subpath can carry a leading slash (@subpath:/x/y); the last + // path never does, so the comparison needs the repo-relative form. subPath := filepath.Clean(filepath.FromSlash(r.workspaceConfig.Workspace.Source.GitSubPath)) + subPath = strings.TrimPrefix(subPath, string(filepath.Separator)) if subPath == "." || subPath == "" { return lastPath } diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 4ab462a0e1..31235d440c 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -685,6 +685,22 @@ func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testin } } +// The stored git subpath can carry a leading slash (@subpath:/x/y); the +// conversion must still strip it from the content-root-relative last path, +// which never has one. +func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T) { + r := newRunnerAt(t.TempDir()) + r.workspaceConfig.Workspace.Source.GitSubPath = "/devsy/jupyter-notebook-hello-world" + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json", + } + + selection := r.effectiveDevContainerSelection(provider2.CLIOptions{}) + if selection.path != ".devcontainer/devcontainer.json" { + t.Fatalf("selection path = %q, want .devcontainer/devcontainer.json", selection.path) + } +} + // A workspace that carries an embedded config (from the provider protocol) // and a legacy last-resolved path must keep using the embedded config, as it // did before profile selection was persisted: the last-path fallback exists From 72c082d56c3082a228dfa9bf08d425dfa48547d2 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 24 Sep 2026 21:08:23 -0700 Subject: [PATCH 07/17] fix(devcontainer): skip stale last-path fallback after content reset The last-path compatibility fallback selected the recorded config path even when the file was gone. `devsy up --reset` deletes and re-clones the content folder, so a synthesized default recorded by the previous up no longer exists, and the explicit-path stat failure aborted up instead of falling back to discovery and the auto-detected default as before. The fallback now applies only while the recorded file exists; the legacy multi-config disambiguation is unchanged because there the recorded file exists in the cloned repo. --- pkg/devcontainer/config.go | 19 +++++++++++-- pkg/devcontainer/config_test.go | 47 ++++++++++++++++++++++++++++++--- 2 files changed, 61 insertions(+), 5 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index c4bb19c481..5fbc48a9c3 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -96,7 +96,11 @@ func (r *runner) effectiveDevContainerSelection( // lastConfigPathSelection falls back to the last resolved path for workspaces // created before the selection was persisted. A workspace with an embedded -// config keeps using it, as it did before profile selection existed. +// config keeps using it, as it did before profile selection existed. The +// fallback applies only while the recorded file exists: `devsy up --reset` +// deletes and re-clones the content folder, so a synthesized default recorded +// by the previous up is gone, and resolution must fall back to discovery +// exactly as it did before this fallback existed. func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { if r.workspaceConfig == nil || r.workspaceConfig.LastDevContainerConfig == nil { return devContainerSelection{}, false @@ -109,7 +113,18 @@ func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { if path == "" { return devContainerSelection{}, false } - return devContainerSelection{path: r.workspaceRelativeLastConfigPath(path)}, true + relativePath := r.workspaceRelativeLastConfigPath(path) + if !devContainerConfigExists(r.workspaceFolder(), relativePath) { + return devContainerSelection{}, false + } + return devContainerSelection{path: relativePath}, true +} + +// devContainerConfigExists reports whether the workspace-folder-relative +// devcontainer path exists on disk. +func devContainerConfigExists(workspaceFolder, relativePath string) bool { + _, err := os.Stat(filepath.Join(workspaceFolder, filepath.FromSlash(relativePath))) + return err == nil } func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) { diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 31235d440c..29c3caa88e 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -483,6 +483,19 @@ func seedAmbiguousProfiles(t *testing.T, folder string) { } } +// seedConfigAt writes a minimal devcontainer.json at the content-root-relative +// path, so tests exercising the last-path fallback have a real file on disk. +func seedConfigAt(t *testing.T, folder, relativePath string) { + t.Helper() + file := filepath.Join(folder, filepath.FromSlash(relativePath)) + if err := os.MkdirAll(filepath.Dir(file), 0o750); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(file, []byte(`{"image":"seed"}`), 0o600); err != nil { + t.Fatal(err) + } +} + func seedNamedProfiles(t *testing.T, folder string, ids ...string) { t.Helper() for _, id := range ids { @@ -655,7 +668,9 @@ func TestGetRawConfig_LastConfigPathCompatibilityFallback(t *testing.T) { } func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { - r := newRunnerAt(t.TempDir()) + folder := t.TempDir() + seedConfigAt(t, folder, "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json") + r := newRunnerAt(folder) r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ Path: "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json", @@ -670,7 +685,9 @@ func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { // The last resolved path is content-root-relative, so a real nested path // whose first segment matches the subpath converts exactly once. func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testing.T) { - r := newRunnerAt(t.TempDir()) + folder := t.TempDir() + seedConfigAt(t, folder, testNestedSubPath+"/app/.devcontainer/devcontainer.json") + r := newRunnerAt(folder) r.workspaceConfig.Workspace.Source.GitSubPath = testNestedSubPath r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ Path: testNestedSubPath + "/app/.devcontainer/devcontainer.json", @@ -689,7 +706,9 @@ func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testin // conversion must still strip it from the content-root-relative last path, // which never has one. func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T) { - r := newRunnerAt(t.TempDir()) + folder := t.TempDir() + seedConfigAt(t, folder, "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json") + r := newRunnerAt(folder) r.workspaceConfig.Workspace.Source.GitSubPath = "/devsy/jupyter-notebook-hello-world" r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ Path: "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json", @@ -701,6 +720,28 @@ func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T } } +// A recorded last path whose file no longer exists must not be selected. +// `devsy up --reset` deletes and re-clones the content folder, so a +// synthesized default config recorded by the previous up is gone; resolution +// must fall back to discovery and the auto-detected default, exactly as it +// did before the last-path fallback existed. +func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { + r := newRunnerAt(t.TempDir()) + r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" + r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ + Path: "devsy/jupyter-notebook-hello-world/.devcontainer.json", + } + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + const defaultImage = "mcr.microsoft.com/devcontainers/base:ubuntu" + if conf.Image != defaultImage { + t.Errorf("Image = %q, want auto-detected default %s", conf.Image, defaultImage) + } +} + // A workspace that carries an embedded config (from the provider protocol) // and a legacy last-resolved path must keep using the embedded config, as it // did before profile selection was persisted: the last-path fallback exists From c6900702b4c722f46aafa1e2c270d21abac1a2ef Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 24 Sep 2026 23:43:57 -0600 Subject: [PATCH 08/17] refactor(devcontainer): trim redundant comments --- pkg/devcontainer/config.go | 27 ++++++++------------------- pkg/devcontainer/config_test.go | 24 +++++------------------- 2 files changed, 13 insertions(+), 38 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 5fbc48a9c3..c9ff17ae73 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -94,13 +94,9 @@ func (r *runner) effectiveDevContainerSelection( return devContainerSelection{} } -// lastConfigPathSelection falls back to the last resolved path for workspaces -// created before the selection was persisted. A workspace with an embedded -// config keeps using it, as it did before profile selection existed. The -// fallback applies only while the recorded file exists: `devsy up --reset` -// deletes and re-clones the content folder, so a synthesized default recorded -// by the previous up is gone, and resolution must fall back to discovery -// exactly as it did before this fallback existed. +// lastConfigPathSelection supports workspaces created before selection was +// persisted. Embedded configs take precedence, and reset-created stale paths +// fall back to discovery when their recorded file no longer exists. func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { if r.workspaceConfig == nil || r.workspaceConfig.LastDevContainerConfig == nil { return devContainerSelection{}, false @@ -120,8 +116,6 @@ func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { return devContainerSelection{path: relativePath}, true } -// devContainerConfigExists reports whether the workspace-folder-relative -// devcontainer path exists on disk. func devContainerConfigExists(workspaceFolder, relativePath string) bool { _, err := os.Stat(filepath.Join(workspaceFolder, filepath.FromSlash(relativePath))) return err == nil @@ -141,13 +135,9 @@ func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) } } -// workspaceRelativeLastConfigPath converts the last resolved path, which is -// stored relative to the content root (see DevContainerConfigWithPath.Path), -// to the workspace folder the resolver runs against: the content root plus -// the git subpath. Remote workspaces with a subpath otherwise apply the -// subpath twice during a later lifecycle operation. A path outside the -// subpath cannot be expressed relative to the workspace folder and is -// returned unchanged, matching the previous discovery behavior. +// workspaceRelativeLastConfigPath converts a content-root-relative last path +// to the workspace folder (the content root plus the git subpath). Paths +// outside the subpath remain unchanged. func (r *runner) workspaceRelativeLastConfigPath(lastPath string) string { if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil { return lastPath @@ -169,9 +159,8 @@ func (r *runner) workspaceRelativeLastConfigPath(lastPath string) string { return filepath.ToSlash(relativePath) } -// workspaceFolder returns the folder the devcontainer resolver runs against: -// the content root plus the git subpath, when any. Workspace.DevContainerPath -// and CLI-provided devcontainer paths are relative to this folder. +// workspaceFolder is the content root plus the git subpath, when present. +// Workspace.DevContainerPath and CLI paths are relative to this folder. func (r *runner) workspaceFolder() string { if r.workspaceConfig == nil || r.workspaceConfig.Workspace == nil { return r.localWorkspaceFolder diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 29c3caa88e..9e073d1d28 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -483,8 +483,6 @@ func seedAmbiguousProfiles(t *testing.T, folder string) { } } -// seedConfigAt writes a minimal devcontainer.json at the content-root-relative -// path, so tests exercising the last-path fallback have a real file on disk. func seedConfigAt(t *testing.T, folder, relativePath string) { t.Helper() file := filepath.Join(folder, filepath.FromSlash(relativePath)) @@ -682,8 +680,7 @@ func TestEffectiveDevContainerSelection_LastPathStripsGitSubPath(t *testing.T) { } } -// The last resolved path is content-root-relative, so a real nested path -// whose first segment matches the subpath converts exactly once. +// Last-path conversion must strip only the applied subpath. func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testing.T) { folder := t.TempDir() seedConfigAt(t, folder, testNestedSubPath+"/app/.devcontainer/devcontainer.json") @@ -702,9 +699,7 @@ func TestEffectiveDevContainerSelection_LastPathRepeatedSubPathSegment(t *testin } } -// The stored git subpath can carry a leading slash (@subpath:/x/y); the -// conversion must still strip it from the content-root-relative last path, -// which never has one. +// Stored git subpaths may have a leading slash, while last paths do not. func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T) { folder := t.TempDir() seedConfigAt(t, folder, "devsy/jupyter-notebook-hello-world/.devcontainer/devcontainer.json") @@ -720,11 +715,7 @@ func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T } } -// A recorded last path whose file no longer exists must not be selected. -// `devsy up --reset` deletes and re-clones the content folder, so a -// synthesized default config recorded by the previous up is gone; resolution -// must fall back to discovery and the auto-detected default, exactly as it -// did before the last-path fallback existed. +// Reset removes the recorded file, so resolution must return to discovery. func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { r := newRunnerAt(t.TempDir()) r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" @@ -742,10 +733,7 @@ func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { } } -// A workspace that carries an embedded config (from the provider protocol) -// and a legacy last-resolved path must keep using the embedded config, as it -// did before profile selection was persisted: the last-path fallback exists -// only for workspaces with no other config source. +// Embedded provider config must retain precedence over the legacy fallback. func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { folder := t.TempDir() seedNamedProfiles(t, folder, testDevContainerProfile) @@ -769,9 +757,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { } } -// A persisted --devcontainer-path is relative to the workspace folder (the -// content root including the git subpath), so a leading segment that matches -// the subpath is part of the real path and must not be stripped. +// Persisted paths are workspace-folder-relative and must not be stripped. func TestGetRawConfig_PersistedPathRepeatedSubPathSegment(t *testing.T) { root := t.TempDir() dir := filepath.Join(root, testNestedSubPath, testNestedSubPath, ".devcontainer") From 847f9066105eadd7d0e3b22be0d57b874e4df44e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 00:24:24 -0700 Subject: [PATCH 09/17] test(e2e): assert the retained profile after selector-free restart Greptile P2: both multi-profile fixtures used the same Compose file and service, so the lifecycle test passed even if the restart resolved the wrong profile. Give each profile a distinct containerEnv marker and assert the selected one is applied after the selector-free restart. --- e2e/tests/up-docker-compose/config.go | 18 ++++++++++++++++++ .../.devcontainer/max-nvidia/devcontainer.json | 5 ++++- .../.devcontainer/max/devcontainer.json | 5 ++++- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/e2e/tests/up-docker-compose/config.go b/e2e/tests/up-docker-compose/config.go index 76ac7d83a5..3b2e1a6f4f 100644 --- a/e2e/tests/up-docker-compose/config.go +++ b/e2e/tests/up-docker-compose/config.go @@ -160,6 +160,24 @@ var _ = ginkgo.Describe( err = tc.f.DevsyUp(ctx, tempDir) framework.ExpectNoError(err) + workspace, err := tc.f.FindWorkspace(ctx, tempDir) + framework.ExpectNoError(err) + + // The selector-free restart must reuse the persisted profile, not + // fall back to another config: the profiles set distinguishable + // container env markers. + err = tc.f.ExecCommand( + ctx, + true, + true, + "max", + []string{ + cmdWorkspace, cmdSSH, flagCommand, + "echo $SELECTED_PROFILE", workspace.ID, + }, + ) + framework.ExpectNoError(err) + err = tc.f.DevsyWorkspaceDelete(ctx, tempDir) framework.ExpectNoError(err) }, ginkgo.SpecTimeout(framework.TimeoutLong())) diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json index 66a81b69a5..855b7ea29a 100644 --- a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max-nvidia/devcontainer.json @@ -2,5 +2,8 @@ "name": "max-nvidia", "dockerComposeFile": "../compose.yaml", "service": "app", - "workspaceFolder": "/workspaces" + "workspaceFolder": "/workspaces", + "containerEnv": { + "SELECTED_PROFILE": "max-nvidia" + } } diff --git a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json index bfc5a6381e..200bc2f735 100644 --- a/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json +++ b/e2e/tests/up-docker-compose/testdata/docker-compose-multi-profile/.devcontainer/max/devcontainer.json @@ -2,5 +2,8 @@ "name": "max", "dockerComposeFile": "../compose.yaml", "service": "app", - "workspaceFolder": "/workspaces" + "workspaceFolder": "/workspaces", + "containerEnv": { + "SELECTED_PROFILE": "max" + } } From 4dab18b8ee788be6da8998572f0ee5dc23843093 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 01:26:11 -0600 Subject: [PATCH 10/17] refactor(e2e): trim profile assertion comment --- e2e/tests/up-docker-compose/config.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/e2e/tests/up-docker-compose/config.go b/e2e/tests/up-docker-compose/config.go index 3b2e1a6f4f..bf2ac4bc48 100644 --- a/e2e/tests/up-docker-compose/config.go +++ b/e2e/tests/up-docker-compose/config.go @@ -163,9 +163,7 @@ var _ = ginkgo.Describe( workspace, err := tc.f.FindWorkspace(ctx, tempDir) framework.ExpectNoError(err) - // The selector-free restart must reuse the persisted profile, not - // fall back to another config: the profiles set distinguishable - // container env markers. + // Distinct markers verify the persisted selector survived restart. err = tc.f.ExecCommand( ctx, true, From cb2dbdc0822008c330cf3faa407adf5b0520bd80 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 03:49:44 -0700 Subject: [PATCH 11/17] fix: embedded devcontainer config outranks persisted path/id selectors CodeRabbit review on #1271 found that a persisted DevContainerPath or DevContainerID won over an embedded DevContainerConfig, inverting the pre-persistence ordering where rawConfigFromWorkspace was consulted before the filesystem path. Gate persisted path/id selection (and the daemon buildWorkspaceOptions copy) on the absence of an embedded config; persisted source still wins, matching CLI semantics. Regression tests prove both failure modes before the fix. Also make the SELECTED_PROFILE assertion exact ("[max]") so it cannot substring-match other profiles such as max-nvidia. --- cmd/workspace/up/agent.go | 8 +++-- e2e/tests/up-docker-compose/config.go | 4 +-- pkg/devcontainer/config.go | 4 +++ pkg/devcontainer/config_test.go | 47 +++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 4 deletions(-) diff --git a/cmd/workspace/up/agent.go b/cmd/workspace/up/agent.go index e980a4e4a7..1ca39b3f1c 100644 --- a/cmd/workspace/up/agent.go +++ b/cmd/workspace/up/agent.go @@ -145,8 +145,12 @@ func (cmd *UpCmd) devsyUpDaemon( func (cmd *UpCmd) buildWorkspaceOptions(workspace *provider2.Workspace) provider2.CLIOptions { baseOptions := cmd.CLIOptions baseOptions.ID = workspace.ID - baseOptions.DevContainerPath = workspace.DevContainerPath - baseOptions.DevContainerID = workspace.DevContainerID + if workspace.DevContainerConfig == nil { + // An embedded config outranks a persisted path or id, so do not + // carry those into the CLI options. + baseOptions.DevContainerPath = workspace.DevContainerPath + baseOptions.DevContainerID = workspace.DevContainerID + } baseOptions.DevContainerImage = workspace.DevContainerImage baseOptions.DevContainerSource = workspace.DevContainerSource baseOptions.IDE = workspace.IDE.Name diff --git a/e2e/tests/up-docker-compose/config.go b/e2e/tests/up-docker-compose/config.go index bf2ac4bc48..f29342a1b5 100644 --- a/e2e/tests/up-docker-compose/config.go +++ b/e2e/tests/up-docker-compose/config.go @@ -168,10 +168,10 @@ var _ = ginkgo.Describe( ctx, true, true, - "max", + "[max]", []string{ cmdWorkspace, cmdSSH, flagCommand, - "echo $SELECTED_PROFILE", workspace.ID, + "echo \"[$SELECTED_PROFILE]\"", workspace.ID, }, ) framework.ExpectNoError(err) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index c9ff17ae73..9474feeb03 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -126,6 +126,10 @@ func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) switch { case workspace.DevContainerSource != "": return devContainerSelection{source: workspace.DevContainerSource}, true + case workspace.DevContainerConfig != nil: + // An embedded config outranks a persisted path or id, exactly as it + // did before the selection was persisted. + return devContainerSelection{}, false case workspace.DevContainerPath != "": return devContainerSelection{path: workspace.DevContainerPath}, true case workspace.DevContainerID != "": diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 9e073d1d28..90cb109e6b 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -716,6 +716,53 @@ func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T } // Reset removes the recorded file, so resolution must return to discovery. +// An embedded config (from the provider protocol) outranks a persisted +// path or id, exactly as it did before the selection was persisted: on main, +// rawConfigFromWorkspace was checked before the filesystem path, so the +// embedded config won. +func TestGetRawConfig_EmbeddedConfigWinsOverPersistedPath(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile) + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ + ImageContainer: config.ImageContainer{Image: "embedded"}, + } + r.workspaceConfig.Workspace.DevContainerPath = + ".devcontainer/" + testDevContainerProfile + "/devcontainer.json" + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "embedded" { + t.Errorf( + "Image = %q, want embedded (persisted path must not override the embedded config)", + conf.Image, + ) + } +} + +func TestGetRawConfig_EmbeddedConfigWinsOverPersistedID(t *testing.T) { + folder := t.TempDir() + seedNamedProfiles(t, folder, testDevContainerProfile) + r := newRunnerAt(folder) + r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ + ImageContainer: config.ImageContainer{Image: "embedded"}, + } + r.workspaceConfig.Workspace.DevContainerID = testDevContainerProfile + + conf, err := r.getRawConfig(provider2.CLIOptions{}) + if err != nil { + t.Fatalf("getRawConfig: %v", err) + } + if conf.Image != "embedded" { + t.Errorf( + "Image = %q, want embedded (persisted id must not override the embedded config)", + conf.Image, + ) + } +} + func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { r := newRunnerAt(t.TempDir()) r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" From 10a76f9b5b99030c189e94c501b6fe769312c6e9 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 03:58:33 -0700 Subject: [PATCH 12/17] fix: honor the effective devcontainer selector on the crane path CodeRabbit review on #1271 (discussion_r4103745548) found that rawConfigFromCraneWithContext parsed Workspace.DevContainerPath only, ignoring the effective selection. The CLI-id case predates this branch, but persisted selectors made it reachable in a new way: an id selection clears the persisted path, so the crane flow would silently parse the default profile instead of the selected one. Thread the selection through and select by id with ForceSelect, as the filesystem path does; with no id the parse options are unchanged. --- pkg/devcontainer/config.go | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 9474feeb03..27022f05cc 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -56,7 +56,7 @@ func (r *runner) getRawConfigWithContext( return conf, nil } if crane.ShouldUse(&options) { - return r.rawConfigFromCraneWithContext(ctx, options) + return r.rawConfigFromCraneWithContext(ctx, options, selection) } return r.rawConfigFromFilesystemWithContext(ctx, options, selection) } @@ -231,12 +231,15 @@ func (r *runner) rawConfigFromContainer() *config.DevContainerConfig { func (r *runner) rawConfigFromCrane( options provider.CLIOptions, ) (*config.DevContainerConfig, error) { - return r.rawConfigFromCraneWithContext(context.Background(), options) + return r.rawConfigFromCraneWithContext( + context.Background(), options, r.effectiveDevContainerSelection(options), + ) } func (r *runner) rawConfigFromCraneWithContext( ctx context.Context, options provider.CLIOptions, + selection devContainerSelection, ) (*config.DevContainerConfig, error) { localWorkspaceFolder, err := crane.PullConfigFromSourceWithContext( ctx, @@ -246,10 +249,20 @@ func (r *runner) rawConfigFromCraneWithContext( if err != nil { return nil, err } - return config.ParseDevContainerJSON( + opts := config.ParseOptions{} + if selection.id != "" { + // As on the filesystem path, an explicit id must not be shadowed by a + // root config, and a mismatch must error rather than fall back. + opts = config.ParseOptions{ + Selector: config.SelectByID(selection.id), + ForceSelect: true, + } + } + return config.ParseDevContainerJSONWithOptions( ctx, localWorkspaceFolder, - r.workspaceConfig.Workspace.DevContainerPath, + selection.path, + opts, ) } From ac08840edac4445b13ec691b5a16d2d2b473cba3 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 04:04:18 -0700 Subject: [PATCH 13/17] test: hoist the repeated embedded image marker into a constant goconst flagged six occurrences of the "embedded" image literal across the embedded-config precedence tests. --- pkg/devcontainer/config_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 90cb109e6b..8264d56856 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -14,7 +14,9 @@ import ( const ( testWorkspaceFolder = "/workspace" testDevContainerProfile = "max" - testNestedSubPath = "app" + // testEmbeddedImage marks the config embedded in workspace metadata. + testEmbeddedImage = "embedded" + testNestedSubPath = "app" ) type SubstituteTestSuite struct { @@ -725,7 +727,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverPersistedPath(t *testing.T) { seedNamedProfiles(t, folder, testDevContainerProfile) r := newRunnerAt(folder) r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ - ImageContainer: config.ImageContainer{Image: "embedded"}, + ImageContainer: config.ImageContainer{Image: testEmbeddedImage}, } r.workspaceConfig.Workspace.DevContainerPath = ".devcontainer/" + testDevContainerProfile + "/devcontainer.json" @@ -734,7 +736,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverPersistedPath(t *testing.T) { if err != nil { t.Fatalf("getRawConfig: %v", err) } - if conf.Image != "embedded" { + if conf.Image != testEmbeddedImage { t.Errorf( "Image = %q, want embedded (persisted path must not override the embedded config)", conf.Image, @@ -747,7 +749,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverPersistedID(t *testing.T) { seedNamedProfiles(t, folder, testDevContainerProfile) r := newRunnerAt(folder) r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ - ImageContainer: config.ImageContainer{Image: "embedded"}, + ImageContainer: config.ImageContainer{Image: testEmbeddedImage}, } r.workspaceConfig.Workspace.DevContainerID = testDevContainerProfile @@ -755,7 +757,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverPersistedID(t *testing.T) { if err != nil { t.Fatalf("getRawConfig: %v", err) } - if conf.Image != "embedded" { + if conf.Image != testEmbeddedImage { t.Errorf( "Image = %q, want embedded (persisted id must not override the embedded config)", conf.Image, @@ -786,7 +788,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { seedNamedProfiles(t, folder, testDevContainerProfile) r := newRunnerAt(folder) r.workspaceConfig.Workspace.DevContainerConfig = &config.DevContainerConfig{ - ImageContainer: config.ImageContainer{Image: "embedded"}, + ImageContainer: config.ImageContainer{Image: testEmbeddedImage}, } r.workspaceConfig.LastDevContainerConfig = &config.DevContainerConfigWithPath{ Path: ".devcontainer/" + testDevContainerProfile + "/devcontainer.json", @@ -796,7 +798,7 @@ func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { if err != nil { t.Fatalf("getRawConfig: %v", err) } - if conf.Image != "embedded" { + if conf.Image != testEmbeddedImage { t.Errorf( "Image = %q, want embedded (last path must not override the embedded config)", conf.Image, From 4c7c6156d1a08fa84f593e7946ab7437df978199 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 12:02:24 -0700 Subject: [PATCH 14/17] fix: treat only not-exist as absent in the last-path staleness check CodeRabbit review on #1271 (discussion_r4107605404) found that devContainerConfigExists treated every os.Stat error as absent, so a permission error or a file where a directory should be would silently skip the recorded path instead of surfacing the failure. Return false only for not-exist; anything else counts as present so the parse step reports the real error, matching the main parse path's convention. --- pkg/devcontainer/config.go | 6 +++++- pkg/devcontainer/config_test.go | 19 +++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 27022f05cc..9b5854d294 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -118,7 +118,11 @@ func (r *runner) lastConfigPathSelection() (devContainerSelection, bool) { func devContainerConfigExists(workspaceFolder, relativePath string) bool { _, err := os.Stat(filepath.Join(workspaceFolder, filepath.FromSlash(relativePath))) - return err == nil + // Only a missing file means absent. Other stat errors (permissions, a + // file where a directory should be, ...) count as present so the parse + // step surfaces the real filesystem error instead of silently skipping + // the recorded path. + return err == nil || !os.IsNotExist(err) } func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) { diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 8264d56856..39796ecc6d 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -765,6 +765,25 @@ func TestGetRawConfig_EmbeddedConfigWinsOverPersistedID(t *testing.T) { } } +func TestDevContainerConfigExists_TreatsOnlyNotExistAsAbsent(t *testing.T) { + folder := t.TempDir() + // A regular file where a directory should be makes stat fail with + // ENOTDIR. That must count as present so the parse step surfaces the + // real filesystem error instead of silently skipping the recorded path. + if err := os.WriteFile(filepath.Join(folder, "cfg"), []byte("x"), 0o600); err != nil { + t.Fatal(err) + } + if !devContainerConfigExists(folder, "cfg/devcontainer.json") { + t.Error("ENOTDIR stat error treated as absent; want present") + } + if devContainerConfigExists(folder, "missing/devcontainer.json") { + t.Error("missing file treated as present; want absent") + } + if !devContainerConfigExists(folder, "cfg") { + t.Error("existing file treated as absent; want present") + } +} + func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { r := newRunnerAt(t.TempDir()) r.workspaceConfig.Workspace.Source.GitSubPath = "devsy/jupyter-notebook-hello-world" From e52154fd3476f48c2b890bb7ee2e95d290a02b9a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 13:29:33 -0600 Subject: [PATCH 15/17] refactor(devcontainer): trim redundant comments --- pkg/devcontainer/config.go | 3 +-- pkg/devcontainer/config_test.go | 11 +++-------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/pkg/devcontainer/config.go b/pkg/devcontainer/config.go index 9b5854d294..4b1c8d074f 100644 --- a/pkg/devcontainer/config.go +++ b/pkg/devcontainer/config.go @@ -131,8 +131,7 @@ func (r *runner) persistedDevContainerSelection() (devContainerSelection, bool) case workspace.DevContainerSource != "": return devContainerSelection{source: workspace.DevContainerSource}, true case workspace.DevContainerConfig != nil: - // An embedded config outranks a persisted path or id, exactly as it - // did before the selection was persisted. + // Preserve pre-persistence precedence for embedded configs. return devContainerSelection{}, false case workspace.DevContainerPath != "": return devContainerSelection{path: workspace.DevContainerPath}, true diff --git a/pkg/devcontainer/config_test.go b/pkg/devcontainer/config_test.go index 39796ecc6d..e450fb825d 100644 --- a/pkg/devcontainer/config_test.go +++ b/pkg/devcontainer/config_test.go @@ -14,9 +14,8 @@ import ( const ( testWorkspaceFolder = "/workspace" testDevContainerProfile = "max" - // testEmbeddedImage marks the config embedded in workspace metadata. - testEmbeddedImage = "embedded" - testNestedSubPath = "app" + testEmbeddedImage = "embedded" + testNestedSubPath = "app" ) type SubstituteTestSuite struct { @@ -717,11 +716,8 @@ func TestEffectiveDevContainerSelection_LastPathLeadingSlashSubPath(t *testing.T } } -// Reset removes the recorded file, so resolution must return to discovery. // An embedded config (from the provider protocol) outranks a persisted -// path or id, exactly as it did before the selection was persisted: on main, -// rawConfigFromWorkspace was checked before the filesystem path, so the -// embedded config won. +// path or id to preserve pre-persistence behavior. func TestGetRawConfig_EmbeddedConfigWinsOverPersistedPath(t *testing.T) { folder := t.TempDir() seedNamedProfiles(t, folder, testDevContainerProfile) @@ -801,7 +797,6 @@ func TestGetRawConfig_LastPathFallbackSkipsMissingFile(t *testing.T) { } } -// Embedded provider config must retain precedence over the legacy fallback. func TestGetRawConfig_EmbeddedConfigWinsOverLastPathFallback(t *testing.T) { folder := t.TempDir() seedNamedProfiles(t, folder, testDevContainerProfile) From a11a6b9b60e2f2d58a3d7f4e9ecb0b1bf8e1b005 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 14:46:02 -0600 Subject: [PATCH 16/17] test(e2e): tolerate transient rootful Podman SSH stalls --- e2e/tests/up/provider_podman_rootful_lifecycle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e/tests/up/provider_podman_rootful_lifecycle.go b/e2e/tests/up/provider_podman_rootful_lifecycle.go index b6ef84a7c8..5552a7c53d 100644 --- a/e2e/tests/up/provider_podman_rootful_lifecycle.go +++ b/e2e/tests/up/provider_podman_rootful_lifecycle.go @@ -124,7 +124,7 @@ var _ = ginkgo.Describe( return "" } return strings.TrimSpace(out) - }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should( + }).WithTimeout(60 * time.Second).WithPolling(2 * time.Second).Should( gomega.Equal("postCreateDone"), ) From 3ca4ed8d6e40a8b203a761f267b2a36c02152560 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Fri, 25 Sep 2026 16:02:07 -0600 Subject: [PATCH 17/17] test(e2e): read lifecycle markers from workspace --- cmd/workspace/up/agent.go | 2 -- e2e/tests/up/helper.go | 9 +++++ e2e/tests/up/provider_docker.go | 36 ++++++------------- .../up/provider_podman_rootful_lifecycle.go | 36 ++++++------------- .../up/provider_podman_rootless_lifecycle.go | 34 +++++------------- .../docker-waitfor/.devcontainer.json | 8 ++--- pkg/devcontainer/config/result.go | 4 +-- 7 files changed, 43 insertions(+), 86 deletions(-) diff --git a/cmd/workspace/up/agent.go b/cmd/workspace/up/agent.go index 1ca39b3f1c..671a07cd53 100644 --- a/cmd/workspace/up/agent.go +++ b/cmd/workspace/up/agent.go @@ -146,8 +146,6 @@ func (cmd *UpCmd) buildWorkspaceOptions(workspace *provider2.Workspace) provider baseOptions := cmd.CLIOptions baseOptions.ID = workspace.ID if workspace.DevContainerConfig == nil { - // An embedded config outranks a persisted path or id, so do not - // carry those into the CLI options. baseOptions.DevContainerPath = workspace.DevContainerPath baseOptions.DevContainerID = workspace.DevContainerID } diff --git a/e2e/tests/up/helper.go b/e2e/tests/up/helper.go index b2ec878c5f..7cd789f04a 100644 --- a/e2e/tests/up/helper.go +++ b/e2e/tests/up/helper.go @@ -116,6 +116,15 @@ func probeSSH( return out, err } +func readLifecycleFile(workspaceDir, name string) (string, error) { + //nolint:gosec // G304: test-controlled path inside workspace + data, err := os.ReadFile(filepath.Join(workspaceDir, name)) + if err != nil { + return "", err + } + return strings.TrimSpace(string(data)), nil +} + // lifecycleMarkerCount reads a marker file in workspaceDir and returns the count // of non-empty lines. If the file does not exist, it returns 0, nil. // diff --git a/e2e/tests/up/provider_docker.go b/e2e/tests/up/provider_docker.go index aee7fedb06..9a0197dc2f 100644 --- a/e2e/tests/up/provider_docker.go +++ b/e2e/tests/up/provider_docker.go @@ -394,47 +394,31 @@ var _ = ginkgo.Describe( err = dtc.f.DevsyUp(ctx, tempDir) framework.ExpectNoError(err) - // onCreateCommand and updateContentCommand should have run (foreground). - out, err := dtc.execSSH(ctx, tempDir, "cat $HOME/on-create.out") + out, err := readLifecycleFile(tempDir, "on-create.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("onCreateDone")) + gomega.Expect(out).To(gomega.Equal("onCreateDone")) - out, err = dtc.execSSH(ctx, tempDir, "cat $HOME/update-content.out") + out, err = readLifecycleFile(tempDir, "update-content.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("updateContentDone")) + gomega.Expect(out).To(gomega.Equal("updateContentDone")) - // postCreateCommand runs as a deferred hook in the background. - // Wait for it to complete and verify the marker file + env substitution. - gomega.Eventually(func() string { - out, err := dtc.execSSH(ctx, tempDir, "cat $HOME/deferred.marker 2>/dev/null") - if err != nil { - return "" - } - return strings.TrimSpace(out) + // Deferred hooks run in the background; verify completion and env substitution. + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "deferred.marker") }).WithTimeout(30*time.Second).WithPolling(2*time.Second).Should( gomega.Equal("postCreateDone"), "deferred postCreateCommand should eventually complete in background", ) - // Verify the deferred hook received substituted env vars, not literals. - envPath, err := dtc.execSSH(ctx, tempDir, "cat $HOME/deferred-env-path.out") + envPath, err := readLifecycleFile(tempDir, "deferred-env-path.out") framework.ExpectNoError(err) gomega.Expect(envPath).To(gomega.ContainSubstring("/usr/local/bin"), "deferred hook should receive resolved PATH, not ${containerEnv:PATH}") gomega.Expect(envPath).NotTo(gomega.ContainSubstring("${containerEnv:"), "deferred hook should not contain literal variable references") - // postStartCommand also deferred — verify it ran. - gomega.Eventually(func() string { - out, err := dtc.execSSH( - ctx, - tempDir, - "cat $HOME/post-start-deferred.out 2>/dev/null", - ) - if err != nil { - return "" - } - return strings.TrimSpace(out) + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "post-start-deferred.out") }).WithTimeout(30*time.Second).WithPolling(2*time.Second).Should( gomega.Equal("postStartDone"), "deferred postStartCommand should eventually complete in background", diff --git a/e2e/tests/up/provider_podman_rootful_lifecycle.go b/e2e/tests/up/provider_podman_rootful_lifecycle.go index 5552a7c53d..583384330e 100644 --- a/e2e/tests/up/provider_podman_rootful_lifecycle.go +++ b/e2e/tests/up/provider_podman_rootful_lifecycle.go @@ -108,45 +108,29 @@ var _ = ginkgo.Describe( err = f.DevsyUp(ctx, tempDir) framework.ExpectNoError(err) - out, err := f.DevsySSH(ctx, tempDir, "cat $HOME/on-create.out") + out, err := readLifecycleFile(tempDir, "on-create.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("onCreateDone")) + gomega.Expect(out).To(gomega.Equal("onCreateDone")) - out, err = f.DevsySSH(ctx, tempDir, "cat $HOME/update-content.out") + out, err = readLifecycleFile(tempDir, "update-content.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("updateContentDone")) + gomega.Expect(out).To(gomega.Equal("updateContentDone")) - gomega.Eventually(func() string { - out, err := probeSSH(f, - ctx, tempDir, "cat $HOME/deferred.marker 2>/dev/null", - ) - if err != nil { - return "" - } - return strings.TrimSpace(out) - }).WithTimeout(60 * time.Second).WithPolling(2 * time.Second).Should( + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "deferred.marker") + }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should( gomega.Equal("postCreateDone"), ) - envPath, err := f.DevsySSH( - ctx, tempDir, "cat $HOME/deferred-env-path.out", - ) + envPath, err := readLifecycleFile(tempDir, "deferred-env-path.out") framework.ExpectNoError(err) gomega.Expect(envPath).To( gomega.ContainSubstring("/usr/local/bin"), ) gomega.Expect(envPath).NotTo(gomega.ContainSubstring("${containerEnv:")) - gomega.Eventually(func() string { - out, err := probeSSH(f, - ctx, - tempDir, - "cat $HOME/post-start-deferred.out 2>/dev/null", - ) - if err != nil { - return "" - } - return strings.TrimSpace(out) + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "post-start-deferred.out") }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should( gomega.Equal("postStartDone"), ) diff --git a/e2e/tests/up/provider_podman_rootless_lifecycle.go b/e2e/tests/up/provider_podman_rootless_lifecycle.go index 2b172d43d1..e0caa0e02b 100644 --- a/e2e/tests/up/provider_podman_rootless_lifecycle.go +++ b/e2e/tests/up/provider_podman_rootless_lifecycle.go @@ -101,45 +101,29 @@ var _ = ginkgo.Describe( err = f.DevsyUp(ctx, tempDir) framework.ExpectNoError(err) - out, err := f.DevsySSH(ctx, tempDir, "cat $HOME/on-create.out") + out, err := readLifecycleFile(tempDir, "on-create.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("onCreateDone")) + gomega.Expect(out).To(gomega.Equal("onCreateDone")) - out, err = f.DevsySSH(ctx, tempDir, "cat $HOME/update-content.out") + out, err = readLifecycleFile(tempDir, "update-content.out") framework.ExpectNoError(err) - gomega.Expect(strings.TrimSpace(out)).To(gomega.Equal("updateContentDone")) + gomega.Expect(out).To(gomega.Equal("updateContentDone")) - gomega.Eventually(func() string { - out, err := probeSSH(f, - ctx, tempDir, "cat $HOME/deferred.marker 2>/dev/null", - ) - if err != nil { - return "" - } - return strings.TrimSpace(out) + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "deferred.marker") }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should( gomega.Equal("postCreateDone"), ) - envPath, err := f.DevsySSH( - ctx, tempDir, "cat $HOME/deferred-env-path.out", - ) + envPath, err := readLifecycleFile(tempDir, "deferred-env-path.out") framework.ExpectNoError(err) gomega.Expect(envPath).To( gomega.ContainSubstring("/usr/local/bin"), ) gomega.Expect(envPath).NotTo(gomega.ContainSubstring("${containerEnv:")) - gomega.Eventually(func() string { - out, err := probeSSH(f, - ctx, - tempDir, - "cat $HOME/post-start-deferred.out 2>/dev/null", - ) - if err != nil { - return "" - } - return strings.TrimSpace(out) + gomega.Eventually(func() (string, error) { + return readLifecycleFile(tempDir, "post-start-deferred.out") }).WithTimeout(30 * time.Second).WithPolling(2 * time.Second).Should( gomega.Equal("postStartDone"), ) diff --git a/e2e/tests/up/testdata/docker-waitfor/.devcontainer.json b/e2e/tests/up/testdata/docker-waitfor/.devcontainer.json index 56c24e1fd3..34467c941b 100644 --- a/e2e/tests/up/testdata/docker-waitfor/.devcontainer.json +++ b/e2e/tests/up/testdata/docker-waitfor/.devcontainer.json @@ -4,8 +4,8 @@ "remoteEnv": { "CONTAINER_ENV_PATH": "${containerEnv:PATH}" }, - "onCreateCommand": "echo onCreateDone > $HOME/on-create.out", - "updateContentCommand": "echo updateContentDone > $HOME/update-content.out", - "postCreateCommand": "echo -n ${CONTAINER_ENV_PATH} > $HOME/deferred-env-path.out && echo postCreateDone > $HOME/deferred.marker", - "postStartCommand": "echo postStartDone > $HOME/post-start-deferred.out" + "onCreateCommand": "echo onCreateDone > on-create.out", + "updateContentCommand": "echo updateContentDone > update-content.out", + "postCreateCommand": "echo -n ${CONTAINER_ENV_PATH} > deferred-env-path.out && echo postCreateDone > deferred.marker", + "postStartCommand": "echo postStartDone > post-start-deferred.out" } diff --git a/pkg/devcontainer/config/result.go b/pkg/devcontainer/config/result.go index 1f10b51a2f..9ca102eff1 100644 --- a/pkg/devcontainer/config/result.go +++ b/pkg/devcontainer/config/result.go @@ -42,9 +42,7 @@ type DevContainerConfigWithPath struct { // Config is the devcontainer.json config Config *DevContainerConfig `json:"config,omitempty"` - // Path is the path to the devcontainer.json relative to the content root - // (the clone root for git workspaces, without the git subpath). Callers - // resolving against the workspace folder must convert for the subpath. + // Path is relative to the content root, before any Git subpath. Path string `json:"path,omitempty"` }