fix(app-platform): deployment-level component enum + visible log-fetch errors in Diagnostic

CHANGELOG.md

@@ -4,6 +4,12 @@ All notable changes to workflow-plugin-digitalocean are documented here.
 
 ## [Unreleased]
 
+### Fixed
+
+- **Troubleshoot deploy/build log fetch** — Fixed two issues that prevented log blocks from appearing in operator-facing Diagnostic output:
+  - `deploymentComponents` now reads component names from `dep.Services / .StaticSites / .Workers / .Jobs / .Functions` (deployment-level arrays populated by both ListDeployments and GetDeployment) before falling back to `dep.Spec.*` and finally `[""]` aggregate. Previously only Spec was inspected, which is nil from ListDeployments, so the empty-aggregate fallback was always hit and DO API returned no logs.
+  - GetLogs API errors, HTTP-fetch errors, empty HistoricURLs, and empty-body responses now append a brief failure note to `Diagnostic.Detail` (in addition to the existing stderr log, which is captured at hashicorp/go-plugin TRACE level and not surfaced to operators). Operators now see the failure mode in the same Troubleshoot block as the rest of the diagnostic output.
+
 ### Added
 
 - **Troubleshoot fetches DO deploy/build logs** (PR-E2) — `AppPlatformDriver.Troubleshoot`

internal/drivers/app_platform.go

@@ -442,19 +442,34 @@ func isTerminalErrorPhase(phase godo.DeploymentPhase) bool {
 
 // attachDeployLogs fetches DO deploy (or build) logs for a failed deployment
 // and appends delimited log blocks to diag.Detail. One block per component.
-// Failures are best-effort: errors are logged to stderr but diag is never
-// withheld.
+// All four failure modes (GetLogs API error, HTTP-fetch error, empty
+// HistoricURLs, empty body) also append a brief failure note to diag.Detail so
+// the cause is visible in operator-facing output. The existing stderr writes
+// are preserved for plugin-debug but are captured at TRACE level by
+// hashicorp/go-plugin and not surfaced to operators by default.
 func (d *AppPlatformDriver) attachDeployLogs(ctx context.Context, appID string, dep *godo.Deployment, diag *interfaces.Diagnostic) {
 	logType := chooseLogType(dep)
 	components := deploymentComponents(dep)
 
+	header := "Deploy logs"
+	if logType == godo.AppLogTypeBuild {
+		header = "Build logs"
+	}
+
 	for _, comp := range components {
+		label := comp
+		if label == "" {
+			label = "<all>"
+		}
+
 		logs, _, err := d.client.GetLogs(ctx, appID, dep.ID, comp, logType, false, troubleshootLogTailLines)
 		if err != nil {
 			fmt.Fprintf(os.Stderr, "troubleshoot: GetLogs app=%s dep=%s component=%q: %v\n", appID, dep.ID, comp, err)
+			diag.Detail += fmt.Sprintf("\n\n---\n%s — component %q: log fetch unavailable (GetLogs API error: %v)\n---", header, label, err)
 			continue
 		}
 		if logs == nil || len(logs.HistoricURLs) == 0 {
+			diag.Detail += fmt.Sprintf("\n\n---\n%s — component %q: no historic logs returned by DO API\n---", header, label)
 			continue
 		}
 		tail, err := fetchLogTail(ctx, logs.HistoricURLs, troubleshootLogTailLines)
@@ -463,19 +478,13 @@ func (d *AppPlatformDriver) attachDeployLogs(ctx context.Context, appID string,
 			// presigned URL, leaking AWS-style signed credentials to the
 			// operator's terminal log. Surface only the redacted shape.
 			fmt.Fprintf(os.Stderr, "troubleshoot: log HTTP fetch app=%s dep=%s component=%q: %s (presigned URL not logged)\n", appID, dep.ID, comp, redactURLError(err))
+			diag.Detail += fmt.Sprintf("\n\n---\n%s — component %q: log fetch failed (%s)\n---", header, label, redactURLError(err))
 			continue
 		}
 		if tail == "" {
+			diag.Detail += fmt.Sprintf("\n\n---\n%s — component %q: log fetch returned empty body\n---", header, label)
 			continue
 		}
-		label := comp
-		if label == "" {
-			label = "<all>"
-		}
-		header := "Deploy logs"
-		if logType == godo.AppLogTypeBuild {
-			header = "Build logs"
-		}
 		diag.Detail += fmt.Sprintf("\n\n---\n%s — component %q (last %d lines):\n%s\n---", header, label, troubleshootLogTailLines, tail)
 	}
 }
@@ -514,41 +523,82 @@ func chooseLogType(dep *godo.Deployment) godo.AppLogType {
 }
 
 // deploymentComponents returns the list of component names to fetch logs for.
-// Components are collected from Services, Jobs, Workers, and Functions (if the
-// AppSpec is present). If the spec is nil or all four arrays are empty, a
-// single empty string is returned — the DO API treats component="" as "all
-// components aggregated".
+// It prefers the deployment-level arrays (Services, StaticSites, Workers, Jobs,
+// Functions) which are populated by both ListDeployments and GetDeployment.
+// If those are all empty it falls back to dep.Spec.* (populated by richer
+// GetDeployment calls that include the full spec), and finally to [""] — the
+// DO API treats component="" as "all components aggregated".
 func deploymentComponents(dep *godo.Deployment) []string {
-	if dep.Spec == nil {
-		// No spec available — fall back to aggregate ("" = all components).
-		return []string{""}
-	}
 	var names []string
-	for _, svc := range dep.Spec.Services {
+
+	// 1. Prefer deployment-level arrays: populated by ListDeployments and
+	// GetDeployment alike. dep.Spec may be nil from ListDeployments, but these
+	// fields are always present when the deployment has components.
+	for _, svc := range dep.Services {
 		if svc != nil && svc.Name != "" {
 			names = append(names, svc.Name)
 		}
 	}
-	for _, job := range dep.Spec.Jobs {
-		if job != nil && job.Name != "" {
-			names = append(names, job.Name)
+	for _, ss := range dep.StaticSites {
+		if ss != nil && ss.Name != "" {
+			names = append(names, ss.Name)
 		}
 	}
-	for _, w := range dep.Spec.Workers {
+	for _, w := range dep.Workers {
 		if w != nil && w.Name != "" {
 			names = append(names, w.Name)
 		}
 	}
-	for _, fn := range dep.Spec.Functions {
+	for _, job := range dep.Jobs {
+		if job != nil && job.Name != "" {
+			names = append(names, job.Name)
+		}
+	}
+	for _, fn := range dep.Functions {
 		if fn != nil && fn.Name != "" {
 			names = append(names, fn.Name)
 		}
 	}
-	if len(names) == 0 {
-		// Spec present but no components enumerated — fall back to aggregate.
-		return []string{""}
+	if len(names) > 0 {
+		return names
 	}
-	return names
+
+	// 2. Fallback: Spec-level arrays populated by a richer GetDeployment call
+	// that includes the full AppSpec. Present when dep came from CreateDeployment
+	// or a single-resource Get, absent from ListDeployments.
+	if dep.Spec != nil {
+		for _, svc := range dep.Spec.Services {
+			if svc != nil && svc.Name != "" {
+				names = append(names, svc.Name)
+			}
+		}
+		for _, ss := range dep.Spec.StaticSites {
+			if ss != nil && ss.Name != "" {
+				names = append(names, ss.Name)
+			}
+		}
+		for _, job := range dep.Spec.Jobs {
+			if job != nil && job.Name != "" {
+				names = append(names, job.Name)
+			}
+		}
+		for _, w := range dep.Spec.Workers {
+			if w != nil && w.Name != "" {
+				names = append(names, w.Name)
+			}
+		}
+		for _, fn := range dep.Spec.Functions {
+			if fn != nil && fn.Name != "" {
+				names = append(names, fn.Name)
+			}
+		}
+		if len(names) > 0 {
+			return names
+		}
+	}
+
+	// 3. Last resort: aggregate ("" = DO API returns logs across all components).
+	return []string{""}
 }
 
 // fetchLogTail fetches log content from the most recent historic URL (index 0)

internal/drivers/app_platform_test.go

@@ -1506,7 +1506,7 @@ func TestTroubleshoot_AttachesBuildLogsForBuildErroredDeployment(t *testing.T) {
 
 // TestTroubleshoot_GracefulOnGetLogsError verifies that when GetLogs returns
 // an error, the Diagnostic is still produced (no panic, no error return), and
-// the Detail does not contain a log block.
+// the Detail contains a visible failure note so operators know why logs are absent.
 func TestTroubleshoot_GracefulOnGetLogsError(t *testing.T) {
 	dep := &godo.Deployment{
 		ID:    "dep-getlogs-err",
@@ -1532,15 +1532,15 @@ func TestTroubleshoot_GracefulOnGetLogsError(t *testing.T) {
 	if len(diags) == 0 {
 		t.Fatal("expected Diagnostic to be produced even when GetLogs fails")
 	}
-	// Diagnostic must not contain a log block.
-	if strings.Contains(diags[0].Detail, "Deploy logs") {
-		t.Errorf("Detail should not contain log block when GetLogs fails; got: %q", diags[0].Detail)
+	// Detail must contain a visible failure note (not silently empty).
+	if !strings.Contains(diags[0].Detail, "log fetch unavailable") {
+		t.Errorf("Detail should contain 'log fetch unavailable' failure note; got: %q", diags[0].Detail)
 	}
 }
 
 // TestTroubleshoot_GracefulOnHTTPFetchError verifies that when GetLogs returns
 // a URL but the HTTP server returns 500, the Diagnostic is still produced
-// without a log block (no panic).
+// and Detail contains a visible failure note so operators know why logs are absent.
 func TestTroubleshoot_GracefulOnHTTPFetchError(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusInternalServerError)
@@ -1571,8 +1571,9 @@ func TestTroubleshoot_GracefulOnHTTPFetchError(t *testing.T) {
 	if len(diags) == 0 {
 		t.Fatal("expected Diagnostic to be produced even when HTTP fetch fails")
 	}
-	if strings.Contains(diags[0].Detail, "Deploy logs") {
-		t.Errorf("Detail should not contain log block when HTTP fetch fails; got: %q", diags[0].Detail)
+	// Detail must contain a visible failure note (not silently empty).
+	if !strings.Contains(diags[0].Detail, "log fetch failed") {
+		t.Errorf("Detail should contain 'log fetch failed' failure note; got: %q", diags[0].Detail)
 	}
 }
 

internal/drivers/app_platform_troubleshoot_test.go

@@ -7,6 +7,8 @@ import (
 	"context"
 	"errors"
 	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -21,6 +23,8 @@ type stubAppClient struct {
 	appErr     error
 	deps       []*godo.Deployment
 	depsErr    error
+	logs       *godo.AppLogs
+	logsErr    error
 }
 
 func (s *stubAppClient) Create(_ context.Context, _ *godo.AppCreateRequest) (*godo.App, *godo.Response, error) {
@@ -45,7 +49,7 @@ func (s *stubAppClient) Delete(_ context.Context, _ string) (*godo.Response, err
 	return nil, errors.New("not implemented in stub")
 }
 func (s *stubAppClient) GetLogs(_ context.Context, _, _, _ string, _ godo.AppLogType, _ bool, _ int) (*godo.AppLogs, *godo.Response, error) {
-	return nil, nil, nil
+	return s.logs, nil, s.logsErr
 }
 
 // ── Troubleshoot tests ─────────────────────────────────────────────────────
@@ -503,3 +507,155 @@ func TestDeploymentCauseAndPhase_LeafStepFallback_NoCause(t *testing.T) {
 		t.Errorf("expected phase from dep.Phase, got %q", phase)
 	}
 }
+
+// ── deploymentComponents tests ─────────────────────────────────────────────
+
+func TestDeploymentComponents_PrefersDeploymentLevelOverSpec(t *testing.T) {
+	dep := &godo.Deployment{
+		// Deployment-level arrays (populated by ListDeployments).
+		Services: []*godo.DeploymentService{{Name: "svc1"}},
+		// Spec-level arrays (populated by richer GetDeployment).
+		Spec: &godo.AppSpec{
+			Services: []*godo.AppServiceSpec{{Name: "spec-svc1"}},
+		},
+	}
+	got := deploymentComponents(dep)
+	if len(got) != 1 || got[0] != "svc1" {
+		t.Errorf("expected [\"svc1\"] (deployment-level wins), got %v", got)
+	}
+}
+
+func TestDeploymentComponents_FallsBackToSpecWhenDeploymentLevelEmpty(t *testing.T) {
+	dep := &godo.Deployment{
+		// No deployment-level arrays.
+		Spec: &godo.AppSpec{
+			Services: []*godo.AppServiceSpec{{Name: "spec-svc"}},
+		},
+	}
+	got := deploymentComponents(dep)
+	if len(got) != 1 || got[0] != "spec-svc" {
+		t.Errorf("expected [\"spec-svc\"] (spec fallback), got %v", got)
+	}
+}
+
+func TestDeploymentComponents_FallsBackToAggregateWhenAllEmpty(t *testing.T) {
+	dep := &godo.Deployment{}
+	got := deploymentComponents(dep)
+	if len(got) != 1 || got[0] != "" {
+		t.Errorf("expected [\"\"] (aggregate fallback), got %v", got)
+	}
+}
+
+func TestDeploymentComponents_IncludesStaticSitesDeploymentLevel(t *testing.T) {
+	dep := &godo.Deployment{
+		StaticSites: []*godo.DeploymentStaticSite{{Name: "static1"}},
+	}
+	got := deploymentComponents(dep)
+	if len(got) != 1 || got[0] != "static1" {
+		t.Errorf("expected [\"static1\"] from StaticSites, got %v", got)
+	}
+}
+
+// ── attachDeployLogs visibility tests ─────────────────────────────────────
+
+// errorDeployment is a helper that builds a minimal ERROR-phase deployment
+// with one service component (deployment-level) so attachDeployLogs runs.
+func errorDeployment(serviceName string) *godo.Deployment {
+	return &godo.Deployment{
+		ID:    "dep-test",
+		Phase: godo.DeploymentPhase_Error,
+		Services: []*godo.DeploymentService{
+			{Name: serviceName},
+		},
+	}
+}
+
+func TestAttachDeployLogs_VisibleErrorWhenGetLogsErrors(t *testing.T) {
+	dep := errorDeployment("web")
+	diag := &interfaces.Diagnostic{ID: dep.ID}
+
+	stub := &stubAppClient{logsErr: errors.New("DO API 503")}
+	d := NewAppPlatformDriverWithClient(stub, "nyc3")
+	d.attachDeployLogs(context.Background(), "app-id", dep, diag)
+
+	if !strings.Contains(diag.Detail, "log fetch unavailable") {
+		t.Errorf("Detail should contain 'log fetch unavailable'; got: %q", diag.Detail)
+	}
+	if !strings.Contains(diag.Detail, "GetLogs API error") {
+		t.Errorf("Detail should contain 'GetLogs API error'; got: %q", diag.Detail)
+	}
+}
+
+func TestAttachDeployLogs_VisibleNoteWhenEmptyHistoricURLs(t *testing.T) {
+	dep := errorDeployment("web")
+	diag := &interfaces.Diagnostic{ID: dep.ID}
+
+	stub := &stubAppClient{logs: &godo.AppLogs{HistoricURLs: nil}}
+	d := NewAppPlatformDriverWithClient(stub, "nyc3")
+	d.attachDeployLogs(context.Background(), "app-id", dep, diag)
+
+	if !strings.Contains(diag.Detail, "no historic logs returned") {
+		t.Errorf("Detail should contain 'no historic logs returned'; got: %q", diag.Detail)
+	}
+}
+
+func TestAttachDeployLogs_VisibleErrorWhenHTTPFetchErrors(t *testing.T) {
+	// Serve a 500 to trigger fetchLogTail error path.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+	defer srv.Close()
+
+	dep := errorDeployment("web")
+	diag := &interfaces.Diagnostic{ID: dep.ID}
+
+	stub := &stubAppClient{logs: &godo.AppLogs{HistoricURLs: []string{srv.URL}}}
+	d := NewAppPlatformDriverWithClient(stub, "nyc3")
+	d.attachDeployLogs(context.Background(), "app-id", dep, diag)
+
+	if !strings.Contains(diag.Detail, "log fetch failed") {
+		t.Errorf("Detail should contain 'log fetch failed'; got: %q", diag.Detail)
+	}
+}
+
+func TestAttachDeployLogs_VisibleNoteWhenEmptyBody(t *testing.T) {
+	// Return 200 OK with empty body.
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	dep := errorDeployment("web")
+	diag := &interfaces.Diagnostic{ID: dep.ID}
+
+	stub := &stubAppClient{logs: &godo.AppLogs{HistoricURLs: []string{srv.URL}}}
+	d := NewAppPlatformDriverWithClient(stub, "nyc3")
+	d.attachDeployLogs(context.Background(), "app-id", dep, diag)
+
+	if !strings.Contains(diag.Detail, "empty body") {
+		t.Errorf("Detail should contain 'empty body'; got: %q", diag.Detail)
+	}
+}
+
+func TestAttachDeployLogs_HappyPath_AppendsLogBlock(t *testing.T) {
+	const logBody = "line1\nline2\nError: something broke\n"
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		_, _ = w.Write([]byte(logBody))
+	}))
+	defer srv.Close()
+
+	dep := errorDeployment("web")
+	diag := &interfaces.Diagnostic{ID: dep.ID}
+
+	stub := &stubAppClient{logs: &godo.AppLogs{HistoricURLs: []string{srv.URL}}}
+	d := NewAppPlatformDriverWithClient(stub, "nyc3")
+	d.attachDeployLogs(context.Background(), "app-id", dep, diag)
+
+	if !strings.Contains(diag.Detail, "Deploy logs") {
+		t.Errorf("Detail should contain 'Deploy logs' header; got: %q", diag.Detail)
+	}
+	if !strings.Contains(diag.Detail, "Error: something broke") {
+		t.Errorf("Detail should contain log content; got: %q", diag.Detail)
+	}
+}