From 81bdb17af57895a3bd3ffd2ed6d7118ce45ad1db Mon Sep 17 00:00:00 2001 From: defangdevs Date: Fri, 21 Aug 2026 08:55:56 -0700 Subject: [PATCH] fix: compose up must exit non-zero when the AI debugger runs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit handleComposeUpErr returned the DEBUGGER's error. The debugger returns nil once it has explained the failure, so any fatal ComposeUp error became exit code 0 for accounts that auto-approve the debugger — which is exactly the paid accounts whose CI runs it unattended. The deploy step went green on a deploy that never happened; only a downstream smoke test noticed. Run the debugger for its side effect and always return the deployment error, the same shape session.go already uses for compose load errors. Fixes #2227 Co-Authored-By: Claude Opus 5 --- src/cmd/cli/command/compose.go | 11 ++++- src/cmd/cli/command/compose_test.go | 70 +++++++++++++++++++++++++++++ src/pkg/debug/debug.go | 12 +++++ 3 files changed, 91 insertions(+), 2 deletions(-) diff --git a/src/cmd/cli/command/compose.go b/src/cmd/cli/command/compose.go index b793b9820..45829d54f 100644 --- a/src/cmd/cli/command/compose.go +++ b/src/cmd/cli/command/compose.go @@ -358,9 +358,16 @@ func handleComposeUpErr(ctx context.Context, debugger *debug.Debugger, project * } term.Error("Error:", client.PrettyError(originalErr)) - return debugger.DebugDeploymentError(ctx, debug.DebugConfig{ + // The debugger runs for its side effect only. It returns nil once it has + // explained the failure, so returning its error here would turn a fatal + // deployment error into exit code 0 — a false-green CI deploy for every + // account that auto-approves the debugger. + if debugErr := debugger.DebugDeploymentError(ctx, debug.DebugConfig{ Project: project, - }, originalErr) + }, originalErr); debugErr != nil { + term.Debug("debugger failed:", debugErr) + } + return originalErr } func handleTooManyProjectsError(ctx context.Context, provider client.Provider, originalErr error) error { diff --git a/src/cmd/cli/command/compose_test.go b/src/cmd/cli/command/compose_test.go index c4ed5bde6..56722d00c 100644 --- a/src/cmd/cli/command/compose_test.go +++ b/src/cmd/cli/command/compose_test.go @@ -3,6 +3,7 @@ package command import ( "bytes" "context" + "errors" "os" "strings" "testing" @@ -10,6 +11,8 @@ import ( "connectrpc.com/connect" "github.com/DefangLabs/defang/src/pkg/cli/client" + "github.com/DefangLabs/defang/src/pkg/cli/compose" + "github.com/DefangLabs/defang/src/pkg/debug" "github.com/DefangLabs/defang/src/pkg/term" defangv1 "github.com/DefangLabs/defang/src/protos/io/defang/v1" ) @@ -129,3 +132,70 @@ func TestResolveTTL(t *testing.T) { }) } } + +// stubDebugAgent stands in for the AI agent: it records that it ran and returns whatever the +// test wants the debug session to end with. +type stubDebugAgent struct { + called bool + err error +} + +func (s *stubDebugAgent) StartWithMessage(context.Context, string) error { + s.called = true + return s.err +} + +// Regression for issue 2227: handleComposeUpErr used to return the DEBUGGER's error. The +// debugger returns nil once it has explained the failure, so a fatal compose up error exited 0 +// and CI went green on a deploy that never happened. +func TestHandleComposeUpErrKeepsTheDeploymentError(t *testing.T) { + prevNonInteractive := global.NonInteractive + global.NonInteractive = true + t.Cleanup(func() { global.NonInteractive = prevNonInteractive }) + + originalErr := errors.New(`service "fabric": port 50051: 'target' must be an integer between 1 and 32767`) + + for _, tt := range []struct { + name string + debugErr error + }{ + {name: "debugger succeeds", debugErr: nil}, + {name: "debugger fails", debugErr: errors.New("agent unavailable")}, + } { + t.Run(tt.name, func(t *testing.T) { + agent := &stubDebugAgent{err: tt.debugErr} + // defaultPermission=true is the paid/auto-approve account: the only one that + // reaches the debugger in CI, and so the only one that hit this bug. + debugger := debug.NewDebuggerForTest(agent, true, false) + + err := handleComposeUpErr(context.Background(), debugger, &compose.Project{}, nil, originalErr) + + if !agent.called { + t.Error("expected the debugger to run") + } + if !errors.Is(err, originalErr) { + t.Errorf("expected the original deployment error, got %v", err) + } + }) + } +} + +// A free-tier CI account never reaches the debugger; it must still get the error. +func TestHandleComposeUpErrWithoutAutoApprove(t *testing.T) { + prevNonInteractive := global.NonInteractive + global.NonInteractive = true + t.Cleanup(func() { global.NonInteractive = prevNonInteractive }) + + originalErr := errors.New("boom") + agent := &stubDebugAgent{} + debugger := debug.NewDebuggerForTest(agent, false, false) + + err := handleComposeUpErr(context.Background(), debugger, &compose.Project{}, nil, originalErr) + + if agent.called { + t.Error("the debugger must not auto-run for a free-tier account") + } + if !errors.Is(err, originalErr) { + t.Errorf("expected the original deployment error, got %v", err) + } +} diff --git a/src/pkg/debug/debug.go b/src/pkg/debug/debug.go index b71baafc9..b56b3f94d 100644 --- a/src/pkg/debug/debug.go +++ b/src/pkg/debug/debug.go @@ -106,6 +106,18 @@ func NewDebugger(ctx context.Context, fabricAddr string, stack *stacks.Parameter }, nil } +// NewDebuggerForTest builds a Debugger around a stub agent. NewDebugger needs a live Fabric +// connection, so tests in OTHER packages — the ones exercising how a caller handles what the +// debugger returns — have no other way to get one. +func NewDebuggerForTest(agent DebugAgent, defaultPermission, interactive bool) *Debugger { + return &Debugger{ + agent: agent, + surveyor: &surveyor{}, + defaultPermission: defaultPermission, + interactive: interactive, + } +} + // AutoApprove reports whether the debugger will run without an interactive prompt. This is true // for paid accounts and lets callers decide whether to invoke the debugger in non-interactive // environments (CI) or just print a hint.