Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions cmd/agent/container/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
14 changes: 12 additions & 2 deletions cmd/agent/workspace/up.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
11 changes: 11 additions & 0 deletions cmd/up/up.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 := ""
Expand Down
18 changes: 18 additions & 0 deletions pkg/ide/opener/opener.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,31 @@ func Open(
return "", nil
}

if err := validateOpenParams(ideName, params); err != nil {
return "", err
}

if fn, ok := browserIDEOpener(ideName); ok {
return fn(ctx, ideOptions, params)
}

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,
Expand Down
38 changes: 38 additions & 0 deletions pkg/ide/opener/opener_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
Loading