[RAPTOR-19727] Don't suggest logs command that won't work

cmd/artifact/build/create/cmd.go

@@ -144,22 +144,28 @@ func waitForAllBuilds(
 		fmt.Fprintf(cmd.ErrOrStderr(), "Waiting for build %s...\n", buildID)
 
 		build, werr := waitStreaming(cmd, artifactID, buildID, poll)
-		if werr != nil && firstWaitErr == nil {
-			firstWaitErr = werr
-		}
 
-		if build == nil {
-			summaries = append(summaries, workload.BuildSummary{BuildID: buildID, Status: workload.BuildStatusCLIUnknown})
+		if build != nil {
+			summary, serr := workload.BuildSummaryFor(build, workload.DefaultBuildLogTail)
 
-			continue
-		}
+			// Reuses summary.LogTail (just fetched above) to decide whether
+			// the logs hint is worth showing -- no extra call.
+			if workload.IsBuildErrorStatus(build.Status) {
+				werr = workload.BuildFailureMessage(artifactID, build.ID, build.Status, len(summary.LogTail) > 0)
+			}
 
-		summary, serr := workload.BuildSummaryFor(build, workload.DefaultBuildLogTail)
-		if serr != nil && firstWaitErr == nil {
-			firstWaitErr = serr
+			if serr != nil && firstWaitErr == nil {
+				firstWaitErr = serr
+			}
+
+			summaries = append(summaries, summary)
+		} else {
+			summaries = append(summaries, workload.BuildSummary{BuildID: buildID, Status: workload.BuildStatusCLIUnknown})
 		}
 
-		summaries = append(summaries, summary)
+		if werr != nil && firstWaitErr == nil {
+			firstWaitErr = werr
+		}
 	}
 
 	return summaries, firstWaitErr

cmd/artifact/build/get/cmd.go

@@ -128,17 +128,15 @@ func runGet(
 		return err
 	}
 
-	if waitErr != nil {
-		return waitErr
+	// Covers both "already terminal-error on the first GET" and "failed
+	// during the wait". Reuses summary.LogTail (already fetched above) to
+	// decide whether the logs hint is worth showing -- no extra call.
+	if workload.IsBuildErrorStatus(build.Status) {
+		return workload.BuildFailureMessage(artifactID, build.ID, build.Status, len(summary.LogTail) > 0)
 	}
 
-	// The build may have been already terminal-error on the first GET, in
-	// which case WaitForBuild was skipped and waitErr stays nil. Surface
-	// that explicitly so the process exits non-zero; hint at the logs
-	// command so the user has a one-step recovery to inspect what went
-	// wrong.
-	if workload.IsBuildErrorStatus(build.Status) {
-		return fmt.Errorf("build %s ended with status %s; run 'dr artifact build logs %s' to inspect", build.ID, build.Status, build.ID)
+	if waitErr != nil {
+		return waitErr
 	}
 
 	return nil

internal/workload/build.go

@@ -387,7 +387,13 @@ func WaitForBuild(
 
 		if IsTerminalBuildStatus(build.Status) {
 			if IsBuildErrorStatus(build.Status) {
-				return build, fmt.Errorf("build %s ended with status %s; run 'dr artifact build logs %s' to inspect", buildID, build.Status, buildID)
+				// No 'dr artifact build logs' suggestion here: this shared
+				// primitive doesn't know whether logs exist, and callers
+				// that already fetch them (via BuildSummaryFor) shouldn't be
+				// forced into a second fetch just so this function can
+				// build a hint. Callers construct the final message
+				// themselves via BuildFailureMessage.
+				return build, fmt.Errorf("build %s ended with status %s", buildID, build.Status)
 			}
 
 			return build, nil
@@ -401,6 +407,27 @@ func WaitForBuild(
 	}
 }
 
+// BuildLogsAvailable reports whether at least one log entry exists for the
+// build. Used to decide whether it's worth pointing the user at
+// 'dr artifact build logs' -- suggesting a command that 404s or prints
+// nothing is worse than not suggesting anything.
+func BuildLogsAvailable(artifactID, buildID string) bool {
+	entries, err := GetArtifactBuildLogs(artifactID, buildID)
+
+	return err == nil && len(entries) > 0
+}
+
+// BuildFailureMessage formats the error for a build that ended in
+// FAILED/CANCELLED, pointing at 'dr artifact build logs' only when
+// logsAvailable is true.
+func BuildFailureMessage(artifactID, buildID, status string, logsAvailable bool) error {
+	if !logsAvailable {
+		return fmt.Errorf("build %s ended with status %s; no logs were captured for this build", buildID, status)
+	}
+
+	return fmt.Errorf("build %s ended with status %s; see 'dr artifact build logs %s %s'", buildID, status, artifactID, buildID)
+}
+
 // BuildSummaryFor composes the terminal-state summary RenderBuildSummary
 // renders. Duration comes from the Build timestamps; ImageURI is fetched
 // from the parent artifact's primary container only on COMPLETED (the

internal/workload/build_test.go

@@ -544,6 +544,96 @@ func TestGetArtifactBuildLogs_ReadsTheOTELStream(t *testing.T) {
 	assert.NotEmpty(t, entries[0].Raw, "OTEL record preserved for JSON passthrough")
 }
 
+// An empty OTEL page (no records for this build's external_build_id) is a
+// legitimate empty result, not an error.
+func TestGetArtifactBuildLogs_EmptyPageIsEmpty(t *testing.T) {
+	installSkipAuth(t)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		_, _ = w.Write([]byte(logsPage("")))
+	}))
+
+	defer srv.Close()
+
+	installEndpoint(t, srv.URL)
+
+	entries, err := GetArtifactBuildLogs("art-1", "b-1")
+	require.NoError(t, err)
+	assert.Empty(t, entries)
+}
+
+func TestBuildLogsAvailable(t *testing.T) {
+	t.Run("true when entries exist", func(t *testing.T) {
+		installSkipAuth(t)
+
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = w.Write([]byte(logsPage("", logEntryDoc("INFO", "hi"))))
+		}))
+
+		defer srv.Close()
+
+		installEndpoint(t, srv.URL)
+
+		assert.True(t, BuildLogsAvailable("art-1", "b-1"))
+	})
+
+	t.Run("false on 404", func(t *testing.T) {
+		installSkipAuth(t)
+
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusNotFound)
+		}))
+
+		defer srv.Close()
+
+		installEndpoint(t, srv.URL)
+
+		assert.False(t, BuildLogsAvailable("art-1", "b-1"))
+	})
+
+	t.Run("false on empty page", func(t *testing.T) {
+		installSkipAuth(t)
+
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			_, _ = w.Write([]byte(logsPage("")))
+		}))
+
+		defer srv.Close()
+
+		installEndpoint(t, srv.URL)
+
+		assert.False(t, BuildLogsAvailable("art-1", "b-1"))
+	})
+
+	t.Run("false on server error", func(t *testing.T) {
+		installSkipAuth(t)
+
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusInternalServerError)
+		}))
+
+		defer srv.Close()
+
+		installEndpoint(t, srv.URL)
+
+		assert.False(t, BuildLogsAvailable("art-1", "b-1"))
+	})
+}
+
+func TestBuildFailureMessage(t *testing.T) {
+	t.Run("with logs available", func(t *testing.T) {
+		err := BuildFailureMessage("art-1", "b-1", BuildStatusFailed, true)
+		require.Error(t, err)
+		assert.EqualError(t, err, "build b-1 ended with status FAILED; see 'dr artifact build logs art-1 b-1'")
+	})
+
+	t.Run("without logs available", func(t *testing.T) {
+		err := BuildFailureMessage("art-1", "b-1", BuildStatusCancelled, false)
+		require.Error(t, err)
+		assert.EqualError(t, err, "build b-1 ended with status CANCELLED; no logs were captured for this build")
+	})
+}
+
 func TestWaitForBuild_TerminalCompletedReturnsNil(t *testing.T) {
 	installSkipAuth(t)
 

internal/workload/output.go

@@ -254,11 +254,25 @@ func RenderBuildSummary(format outputformat.OutputFormat, summary BuildSummary)
 	return nil
 }
 
+// RenderBuildLogs prints a build's log entries: one formatted line each in
+// text mode, or a JSON array (always [], never null, when empty). With no
+// entries in text mode, "No logs found." goes to stderr so stdout stays log
+// lines only and a `logs | grep`/pipe is not polluted by a status line.
 func RenderBuildLogs(format outputformat.OutputFormat, entries []BuildLogEntry) error {
 	if format == outputformat.OutputFormatJSON {
+		if len(entries) == 0 {
+			entries = []BuildLogEntry{}
+		}
+
 		return printJSON(entries)
 	}
 
+	if len(entries) == 0 {
+		fmt.Fprintln(os.Stderr, "No logs found.")
+
+		return nil
+	}
+
 	for _, entry := range entries {
 		fmt.Println(formatLogLine(entry))
 	}

internal/workload/output_test.go

@@ -530,6 +530,29 @@ func TestRenderBuildLogs_JSONPassthroughPreservesRaw(t *testing.T) {
 	assert.Equal(t, "raw", got[0]["message"])
 }
 
+func TestRenderBuildLogs_TextEmpty(t *testing.T) {
+	var stderr string
+
+	stdout := captureStdout(t, func() {
+		stderr = captureStderr(t, func() {
+			require.NoError(t, RenderBuildLogs(outputformat.OutputFormatText, nil))
+		})
+	})
+
+	// The hint goes to stderr so stdout stays log lines only (pipe/grep safe).
+	assert.Empty(t, stdout)
+	assert.Equal(t, "No logs found.\n", stderr)
+}
+
+func TestRenderBuildLogs_JSONAlwaysArray(t *testing.T) {
+	output := captureStdout(t, func() {
+		require.NoError(t, RenderBuildLogs(outputformat.OutputFormatJSON, nil))
+	})
+
+	// A regression that emits `null` instead of a JSON array must fail here.
+	assert.JSONEq(t, `[]`, output)
+}
+
 func TestFilterLogsByLevel(t *testing.T) {
 	entries := []BuildLogEntry{
 		{Levelname: "DEBUG", Message: "d"},

internal/workload/up/build.go

@@ -481,10 +481,10 @@ func buildImage(artifactID, attachTo string, opts Options, report *reporter) (st
 	}
 
 	if workload.IsBuildErrorStatus(built.Status) {
-		// Said here rather than left to the wait's own wording, because this
-		// is the only place that knows which artifact the build belongs to.
-		return built.ID, fmt.Errorf("build %s finished as %s; see 'dr artifact build logs %s %s'",
-			built.ID, built.Status, artifactID, built.ID)
+		// Precheck log availability before suggesting the logs command --
+		// this is the only call site with no already-fetched log info, so
+		// it's the one place that needs its own extra fetch.
+		return built.ID, workload.BuildFailureMessage(artifactID, built.ID, built.Status, hasLogsFn(artifactID, built.ID))
 	}
 
 	// A build still running when the wait expires keeps its id too: it is

internal/workload/up/roll_test.go

@@ -1372,6 +1372,8 @@ func TestRun_FailedBuildOnARollLeavesTheOldVersionServing(t *testing.T) {
 		return &workload.Build{ID: id, Status: workload.BuildStatusFailed},
 			fmt.Errorf("build %s ended with status %s", id, workload.BuildStatusFailed)
 	}
+	// Logs exist for this build, so the failure message should point at them.
+	f.hasLogs = func(string, string) bool { return true }
 
 	install(t, f)
 

internal/workload/up/run.go

@@ -46,6 +46,7 @@ var (
 	lockArtifactFn     = workload.LockArtifact
 	triggerBuildFn     = workload.TriggerArtifactBuild
 	waitBuildFn        = workload.WaitForBuild
+	hasLogsFn          = workload.BuildLogsAvailable
 	listBuildsFn       = workload.ListArtifactBuilds
 	getCredentialFn    = workload.GetCredential
 	findCredentialFn   = workload.FindCredentialNamed

internal/workload/up/run_test.go

@@ -193,6 +193,7 @@ type fakes struct {
 	build       func(string) (*workload.BuildTriggerResponse, error)
 	waitBuild   func(string, string, time.Duration, time.Duration, func(*workload.Build)) (*workload.Build, error)
 	builds      func(string, int) ([]workload.Build, error)
+	hasLogs     func(string, string) bool
 
 	// checkEndpoint is the one GET a deploy ends with.
 	checkEndpoint func(string) (int, error)
@@ -287,6 +288,11 @@ func install(t *testing.T, f fakes) {
 	swap(t, &waitBuildFn, f.waitBuild)
 	swap(t, &listBuildsFn, f.builds)
 
+	// Defaults to "no logs" so a test that does not care does not make a
+	// real network call by accident.
+	force(t, &hasLogsFn, func(string, string) bool { return false })
+	swap(t, &hasLogsFn, f.hasLogs)
+
 	// Nothing stands in the way of a rollout unless a test says so, because
 	// the quiet answer is the one every other roll test wants.
 	force(t, &guardReplacementFn, func(string) error { return nil })
@@ -1492,6 +1498,8 @@ func TestRun_FailedBuildStopsAndNamesTheLogs(t *testing.T) {
 		return &workload.Build{ID: id, Status: workload.BuildStatusFailed},
 			fmt.Errorf("build %s ended with status %s", id, workload.BuildStatusFailed)
 	}
+	// Logs exist for this build, so the failure message should point at them.
+	f.hasLogs = func(string, string) bool { return true }
 
 	install(t, f)