From d940311a14f112194377bb98defe5a1f5acd92c6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Thu, 28 May 2026 11:20:24 -0500 Subject: [PATCH] fix(ide): propagate in-container setup failures instead of panicking When an IDE install inside the container failed (e.g. Jupyter on a Node image with no pip), the inner agent exited non-zero without sending a structured result, the host received a half-populated Result with a nil SubstitutionContext, and openIDE nil-derefed on ContainerWorkspaceFolder. Three-layer defense: - Inner agent now sends Result{Error: ...} via the tunnel on setup failure, so the underlying cause survives the SSH boundary. - Outer agent surfaces a forwarded result.Error as a non-zero exit. - Host validates SubstitutionContext and opener guards against nil Result before dispatching to any per-IDE handler. Send-failure logs upgraded from Debug to Error and include the original cause so a failure-on-failure leaves a breadcrumb. --- cmd/agent/container/setup.go | 19 ++++++++++++++++-- cmd/agent/workspace/up.go | 14 +++++++++++-- cmd/up/up.go | 11 ++++++++++ pkg/ide/opener/opener.go | 18 +++++++++++++++++ pkg/ide/opener/opener_test.go | 38 +++++++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 4 deletions(-) diff --git a/cmd/agent/container/setup.go b/cmd/agent/container/setup.go index 9802723eb..d6fb3d6ce 100644 --- a/cmd/agent/container/setup.go +++ b/cmd/agent/container/setup.go @@ -210,18 +210,33 @@ func (cmd *SetupContainerCmd) finalizeSetup(sctx *setupContext) error { deferred, err := setup.SetupContainerPreAttach(sctx.ctx, cfg) if err != nil { - return err + return cmd.reportSetupFailure(sctx, err) } if !cmd.Prebuild { if err := cmd.setupPostAttach(sctx, deferred); err != nil { - return err + return cmd.reportSetupFailure(sctx, err) } } return cmd.sendSetupResult(sctx.ctx, sctx.setupInfo, sctx.tunnelClient) } +// reportSetupFailure forwards a structured error result through the tunnel +// before returning the original error. Without this, the outer agent only +// sees the SSH exit code and the underlying cause (e.g. an IDE install +// failure) gets lost to a generic wrapper on the host side. +func (cmd *SetupContainerCmd) reportSetupFailure(sctx *setupContext, cause error) error { + errResult := &config.Result{Error: cause.Error()} + if sendErr := cmd.sendSetupResult(sctx.ctx, errResult, sctx.tunnelClient); sendErr != nil { + // Failure-on-failure: the host will see only the SSH exit code, so + // log the original cause alongside the send failure to leave a + // breadcrumb for debugging. + log.Errorf("failed to forward setup error %q to host: %v", cause, sendErr) + } + return cause +} + func (cmd *SetupContainerCmd) setupPostAttach( sctx *setupContext, deferred setup.DeferredHooks, diff --git a/cmd/agent/workspace/up.go b/cmd/agent/workspace/up.go index 96a739b56..bb1e185e4 100644 --- a/cmd/agent/workspace/up.go +++ b/cmd/agent/workspace/up.go @@ -153,12 +153,22 @@ func (cmd *UpCmd) up( // receive a result back from agent" fallback. errResult := &config2.Result{Error: err.Error()} if sendErr := cmd.sendResult(ctx, errResult, tunnelClient); sendErr != nil { - log.Debugf("failed to send error result back to host: %v", sendErr) + log.Errorf("failed to forward up error %q to host: %v", err, sendErr) } return err } - return cmd.sendResult(ctx, result, tunnelClient) + // runner.Up can return (result, nil) where result carries a structured + // Error forwarded from the inner container-setup step. Treat that as a + // failure so the agent process exits non-zero and the host doesn't try + // to proceed with a half-populated result. + if err := cmd.sendResult(ctx, result, tunnelClient); err != nil { + return err + } + if result != nil && result.Error != "" { + return fmt.Errorf("%s", result.Error) + } + return nil } func (cmd *UpCmd) sendResult( diff --git a/cmd/up/up.go b/cmd/up/up.go index 2b43c8943..c2ba648e3 100644 --- a/cmd/up/up.go +++ b/cmd/up/up.go @@ -258,6 +258,17 @@ func (cmd *UpCmd) executeDevsyUp( if cmd.Platform.Enabled { return nil, nil } + // Guard against a result that lacks the substitution context — that + // indicates the agent returned a half-populated result (e.g. an inner + // container-setup failure that didn't carry through as result.Error). + // Without this, downstream openIDE would nil-deref on + // SubstitutionContext.ContainerWorkspaceFolder. + if result.SubstitutionContext == nil { + return nil, fmt.Errorf( + "agent returned an incomplete result (missing substitution context); " + + "check earlier logs for the underlying setup failure", + ) + } user := config2.GetRemoteUser(result) workdir := "" diff --git a/pkg/ide/opener/opener.go b/pkg/ide/opener/opener.go index 582c40dd2..af6fd1d48 100644 --- a/pkg/ide/opener/opener.go +++ b/pkg/ide/opener/opener.go @@ -62,6 +62,10 @@ func Open( return "", nil } + if err := validateOpenParams(ideName, params); err != nil { + return "", err + } + if fn, ok := browserIDEOpener(ideName); ok { return fn(ctx, ideOptions, params) } @@ -69,6 +73,20 @@ func Open( return openDesktopIDE(ctx, ideName, ideOptions, params) } +// validateOpenParams ensures the fields downstream openers depend on are +// populated. A nil Result or SubstitutionContext means workspace setup did +// not complete and we should surface a clear error instead of panicking on +// a field access deep inside an IDE opener. +func validateOpenParams(ideName string, params IDEParams) error { + if params.Result == nil || params.Result.SubstitutionContext == nil { + return fmt.Errorf( + "cannot open IDE %q: workspace setup did not complete (missing substitution context)", + ideName, + ) + } + return nil +} + // browserIDEOpener returns a handler for browser-based IDEs if ideName matches. func browserIDEOpener( ideName string, diff --git a/pkg/ide/opener/opener_test.go b/pkg/ide/opener/opener_test.go index 2a7fab7db..9ac6495ec 100644 --- a/pkg/ide/opener/opener_test.go +++ b/pkg/ide/opener/opener_test.go @@ -2,9 +2,11 @@ package opener import ( "context" + "strings" "testing" "github.com/devsy-org/devsy/pkg/config" + config2 "github.com/devsy-org/devsy/pkg/devcontainer/config" ) // TestIDEParams_Launch_OpenBrowser asserts the mapping that browser IDE openers @@ -62,6 +64,42 @@ func TestOpenDesktopIDE_HeadlessShortCircuits(t *testing.T) { } } +// TestOpen_NilResultRejected pins the fix for the panic that occurred when +// the agent returned a half-populated result (no SubstitutionContext) after +// an in-container IDE install failure. Previously Open would nil-deref on +// params.Result.SubstitutionContext.ContainerWorkspaceFolder; now it must +// return a descriptive error before dispatching to any per-IDE handler. +func TestOpen_NilResultRejected(t *testing.T) { + tests := []struct { + name string + params IDEParams + }{ + {"nil Result", IDEParams{Result: nil}}, + {"nil SubstitutionContext", IDEParams{Result: &config2.Result{}}}, + } + ides := []string{ + string(config.IDEJupyterNotebook), + string(config.IDEOpenVSCode), + string(config.IDEVSCode), + } + for _, tt := range tests { + for _, ide := range ides { + t.Run(tt.name+"/"+ide, func(t *testing.T) { + url, err := Open(context.Background(), ide, nil, tt.params) + if err == nil { + t.Fatalf("expected error, got nil (url=%q)", url) + } + if !strings.Contains(err.Error(), ide) { + t.Errorf("expected error to name IDE %q, got %q", ide, err.Error()) + } + if url != "" { + t.Errorf("expected empty url, got %q", url) + } + }) + } + } +} + func TestParseAddressAndPort_Empty(t *testing.T) { addr, p, err := ParseAddressAndPort("", 10000) if err != nil {