From b30d34199aeee58ead90a987647118f2502365d0 Mon Sep 17 00:00:00 2001
From: Mikhail Chusavitin <mchusavitin@mchusmbp.local>
Date: Thu, 9 Jul 2026 15:48:12 +0300
Subject: [PATCH] app/webui: replace generic "see summary.txt" SAT failure text
 with the real reason
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Every place that surfaced a FAILED SAT result — task error messages,
component-status.json detail, and the hardware snapshot's ErrorDescription/
StatusHistory — used to say only "SAT overall_status=FAILED (see
summary.txt)" or "<label> failed", forcing an engineer to go dig through the
run directory to find out what actually broke.

nvidia-config's summary.txt now carries a "warnings" field with the specific
GPU/NVLink finding. A new SATFailureDetail/satFailureDetailFromKV in
component_status_db.go reads that field, or falls back to naming whichever
generic SAT sub-job(s) reported non-OK/UNSUPPORTED status along with their
exit code. This feeds both the task-runner error message and the component
status DB. sat_overlay.go's satKeyStatus (which drives ErrorDescription on
the exported hardware snapshot) now does the same, with storage kept
per-device so one drive's rc doesn't get attributed to another's card.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---
 audit/internal/app/component_status_db.go     | 88 +++++++++++++++++--
 .../internal/app/component_status_db_test.go  | 58 ++++++++++++
 audit/internal/app/sat_overlay.go             | 32 +++++--
 audit/internal/app/sat_overlay_test.go        | 61 +++++++++++++
 .../internal/platform/nvidia_config_check.go  |  5 ++
 .../platform/nvidia_config_check_test.go      | 22 +++++
 audit/internal/webui/task_runner.go           |  6 +-
 audit/internal/webui/tasks.go                 |  6 +-
 8 files changed, 265 insertions(+), 13 deletions(-)

diff --git a/audit/internal/app/component_status_db.go b/audit/internal/app/component_status_db.go
index 5450aff..8eab2b1 100644
--- a/audit/internal/app/component_status_db.go
+++ b/audit/internal/app/component_status_db.go
@@ -2,8 +2,10 @@ package app
 
 import (
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -231,6 +233,12 @@ func ApplySATResultToDB(db *ComponentStatusDB, target, archivePath string) {
 
 	source := "sat:" + target
 	dbStatus := satStatusToDBStatus(overall)
+	detail := target + " SAT: " + overall
+	if overall != "OK" {
+		if reason := satFailureDetailFromKV(kv); reason != "" {
+			detail += " — " + reason
+		}
+	}
 
 	// Map SAT target to component keys. GPU targets are keyed by vendor, not
 	// by the raw target string: "nvidia" (Check tier) and "nvidia-stress" /
@@ -244,13 +252,13 @@ func ApplySATResultToDB(db *ComponentStatusDB, target, archivePath string) {
 	switch target {
 	case "nvidia", "nvidia-targeted-stress", "nvidia-compute", "nvidia-targeted-power", "nvidia-pulse",
 		"nvidia-interconnect", "nvidia-bandwidth", "nvidia-stress", "nvidia-config":
-		db.Record("pcie:gpu:nvidia", source, dbStatus, target+" SAT: "+overall)
+		db.Record("pcie:gpu:nvidia", source, dbStatus, detail)
 	case "amd", "amd-stress", "amd-mem", "amd-bandwidth":
-		db.Record("pcie:gpu:amd", source, dbStatus, target+" SAT: "+overall)
+		db.Record("pcie:gpu:amd", source, dbStatus, detail)
 	case "memory", "memory-stress", "sat-stress":
-		db.Record("memory:all", source, dbStatus, target+" SAT: "+overall)
+		db.Record("memory:all", source, dbStatus, detail)
 	case "cpu", "platform-stress":
-		db.Record("cpu:all", source, dbStatus, target+" SAT: "+overall)
+		db.Record("cpu:all", source, dbStatus, detail)
 	case "storage":
 		// Try to record per-device if available in summary.
 		recordedAny := false
@@ -265,11 +273,19 @@ func ApplySATResultToDB(db *ComponentStatusDB, target, archivePath string) {
 			}
 			devName := base[:idx]
 			devStatus := satStatusToDBStatus(strings.ToUpper(strings.TrimSpace(val)))
-			db.Record("storage:"+devName, source, devStatus, "storage SAT: "+val)
+			devDetail := "storage SAT: " + val
+			if strings.ToUpper(strings.TrimSpace(val)) != "OK" {
+				if rc, ok := kv[base+"_rc"]; ok {
+					devDetail = fmt.Sprintf("storage SAT job %q: %s (rc=%s)", base, val, rc)
+				} else {
+					devDetail = fmt.Sprintf("storage SAT job %q: %s", base, val)
+				}
+			}
+			db.Record("storage:"+devName, source, devStatus, devDetail)
 			recordedAny = true
 		}
 		if !recordedAny {
-			db.Record("storage:all", source, dbStatus, "storage SAT: "+overall)
+			db.Record("storage:all", source, dbStatus, detail)
 		}
 	}
 }
@@ -309,6 +325,66 @@ func ReadSATOverallStatus(archivePath string) string {
 	return strings.ToUpper(strings.TrimSpace(kv["overall_status"]))
 }
 
+// SATFailureDetail explains *why* a SAT run's overall_status isn't OK, read
+// from the summary.txt next to archivePath. "SAT overall_status=FAILED (see
+// summary.txt)" tells an engineer nothing without opening the run directory
+// themselves; this pulls the specific reason out so it can be surfaced
+// directly in the task's error message and in the component status DB.
+// Returns "" if summary.txt is unreadable or carries no identifiable reason.
+func SATFailureDetail(archivePath string) string {
+	if strings.TrimSpace(archivePath) == "" {
+		return ""
+	}
+	runDir := strings.TrimSuffix(extractArchivePath(archivePath), ".tar.gz")
+	data, err := os.ReadFile(filepath.Join(runDir, "summary.txt"))
+	if err != nil {
+		return ""
+	}
+	return satFailureDetailFromKV(parseSATKV(string(data)))
+}
+
+// satFailureDetailFromKV inspects an already-parsed summary.txt for the
+// reason behind a non-OK overall_status.
+//
+//   - Checks that write structured findings (nvidia-config's GPU config /
+//     NVLink topology check) put the human-readable reason straight into a
+//     "warnings" field — return that verbatim.
+//   - Generic SAT acceptance packs (nvidia, amd, memory, cpu, storage, ...)
+//     instead record one "<job>_status"/"<job>_rc" pair per sub-job; walk
+//     those and report whichever job(s) didn't come back OK/UNSUPPORTED.
+func satFailureDetailFromKV(kv map[string]string) string {
+	if w := strings.TrimSpace(kv["warnings"]); w != "" {
+		return w
+	}
+
+	keys := make([]string, 0, len(kv))
+	for k := range kv {
+		keys = append(keys, k)
+	}
+	sort.Strings(keys)
+
+	var failed []string
+	for _, k := range keys {
+		if k == "overall_status" || !strings.HasSuffix(k, "_status") {
+			continue
+		}
+		v := strings.ToUpper(strings.TrimSpace(kv[k]))
+		if v == "" || v == "OK" || v == "UNSUPPORTED" {
+			continue
+		}
+		job := strings.TrimSuffix(k, "_status")
+		if rc, ok := kv[job+"_rc"]; ok && strings.TrimSpace(rc) != "" {
+			failed = append(failed, fmt.Sprintf("%s=%s (rc=%s)", job, v, rc))
+		} else {
+			failed = append(failed, fmt.Sprintf("%s=%s", job, v))
+		}
+	}
+	if len(failed) == 0 {
+		return ""
+	}
+	return "failed sub-job(s): " + strings.Join(failed, ", ")
+}
+
 func extractArchivePath(s string) string {
 	s = strings.TrimSpace(s)
 	if rest, ok := strings.CutPrefix(s, "Archive written to "); ok {
diff --git a/audit/internal/app/component_status_db_test.go b/audit/internal/app/component_status_db_test.go
index 81bd9ff..c440d88 100644
--- a/audit/internal/app/component_status_db_test.go
+++ b/audit/internal/app/component_status_db_test.go
@@ -98,6 +98,64 @@ func TestApplyComponentStatusDBMatchesGPUByVendor(t *testing.T) {
 
 func strPtr(s string) *string { return &s }
 
+// TestSATFailureDetailPrefersWarningsField guards the real bug this
+// exercises: an engineer looking at a failed nvidia-config task used to see
+// only "SAT overall_status=FAILED (see summary.txt)" — no indication of
+// which GPU or NVLink pair was the problem. nvidia-config's summary.txt
+// (nvidia_config_check.go's renderNvidiaConfigCheckSummary) writes the
+// specific reason into a "warnings" field; SATFailureDetail must surface it
+// verbatim instead of falling through to the generic sub-job scan.
+func TestSATFailureDetailPrefersWarningsField(t *testing.T) {
+	runDir := t.TempDir()
+	summary := "run_at_utc=2026-07-09T18:40:57Z\n" +
+		"nvlink_pairs_checked=1\n" +
+		"nvlink_pairs_with_issues=1\n" +
+		"overall_status=FAILED\n" +
+		"warnings=NVLink GPU0<->GPU1: 2/36 NVLinks inactive on a bonded pair\n"
+	if err := os.WriteFile(filepath.Join(runDir, "summary.txt"), []byte(summary), 0644); err != nil {
+		t.Fatal(err)
+	}
+	want := "NVLink GPU0<->GPU1: 2/36 NVLinks inactive on a bonded pair"
+	if got := SATFailureDetail(runDir); got != want {
+		t.Fatalf("SATFailureDetail() = %q, want %q", got, want)
+	}
+}
+
+// TestSATFailureDetailFallsBackToFailedSubJobs guards generic SAT acceptance
+// packs (nvidia, memory, cpu, storage, ...), which have no "warnings" field
+// — they record one "<job>_status"/"<job>_rc" pair per sub-job instead.
+// SATFailureDetail must name the specific job(s) that failed rather than
+// telling the reader nothing beyond "see summary.txt".
+func TestSATFailureDetailFallsBackToFailedSubJobs(t *testing.T) {
+	runDir := t.TempDir()
+	summary := "run_at_utc=2026-07-09T18:40:57Z\n" +
+		"nvidia-smi-q_rc=0\n" +
+		"nvidia-smi-q_status=OK\n" +
+		"bee-gpu-burn_rc=1\n" +
+		"bee-gpu-burn_status=FAILED\n" +
+		"job_ok=1\n" +
+		"job_failed=1\n" +
+		"overall_status=FAILED\n"
+	if err := os.WriteFile(filepath.Join(runDir, "summary.txt"), []byte(summary), 0644); err != nil {
+		t.Fatal(err)
+	}
+	want := "failed sub-job(s): bee-gpu-burn=FAILED (rc=1)"
+	if got := SATFailureDetail(runDir); got != want {
+		t.Fatalf("SATFailureDetail() = %q, want %q", got, want)
+	}
+}
+
+// TestSATFailureDetailEmptyWhenNoReasonFound guards the ultimate fallback:
+// when summary.txt carries no identifiable per-job or warnings detail (e.g.
+// unreadable or from an older binary version), callers must get "" so they
+// know to fall back to their own generic message rather than silently
+// printing an empty explanation.
+func TestSATFailureDetailEmptyWhenNoReasonFound(t *testing.T) {
+	if got := SATFailureDetail(filepath.Join(t.TempDir(), "does-not-exist")); got != "" {
+		t.Fatalf("SATFailureDetail() = %q, want empty for missing summary.txt", got)
+	}
+}
+
 // TestApplySATResultToDBCoversAllHealthCheckTargets guards against a target
 // silently falling through ApplySATResultToDB's switch with no matching
 // case — exactly what happened to the old "confidential-computing" target
diff --git a/audit/internal/app/sat_overlay.go b/audit/internal/app/sat_overlay.go
index 50f888d..211de80 100644
--- a/audit/internal/app/sat_overlay.go
+++ b/audit/internal/app/sat_overlay.go
@@ -1,6 +1,7 @@
 package app
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"sort"
@@ -61,7 +62,7 @@ func applyNvidiaPerGPUStatus(devs []schema.HardwarePCIeDevice, baseDir string) {
 		if !ok {
 			continue
 		}
-		status, description, ok := satKeyStatus(st.runStatus, firstNonEmpty(strings.TrimSpace(st.reason), "nvidia GPU SAT"))
+		status, description, ok := satKeyStatus(st.runStatus, firstNonEmpty(strings.TrimSpace(st.reason), "nvidia GPU SAT"), nil)
 		if !ok {
 			continue
 		}
@@ -221,10 +222,20 @@ func parseStorageSATStatus(summary satSummary) map[string]satStatusResult {
 		}
 		devName := base[:idx]
 		step := strings.ReplaceAll(base[idx+1:], "_", "-")
-		stepStatus, desc, ok := satKeyStatus(strings.ToUpper(strings.TrimSpace(value)), "storage "+step)
+		label := "storage " + step
+		// Storage steps are per-device, per-job — satFailureDetailFromKV(summary.kv)
+		// would combine every device's failures into one description here.
+		// Build the reason from this one job's own rc instead so a failing
+		// nvme1n1 self-test doesn't get another drive's error attributed to it.
+		stepStatus, desc, ok := satKeyStatus(strings.ToUpper(strings.TrimSpace(value)), label, nil)
 		if !ok {
 			continue
 		}
+		if stepStatus == "Critical" {
+			if rc, hasRC := summary.kv[base+"_rc"]; hasRC && strings.TrimSpace(rc) != "" {
+				desc = fmt.Sprintf("%s failed (rc=%s)", label, rc)
+			}
+		}
 		current := result[devName]
 		if !current.ok || statusSeverity(stepStatus) > statusSeverity(current.status) {
 			result[devName] = satStatusResult{status: stepStatus, description: desc, ok: true}
@@ -234,10 +245,17 @@ func parseStorageSATStatus(summary satSummary) map[string]satStatusResult {
 }
 
 func satSummaryStatus(summary satSummary, label string) (string, string, bool) {
-	return satKeyStatus(summary.overall, label)
+	return satKeyStatus(summary.overall, label, summary.kv)
 }
 
-func satKeyStatus(rawStatus, label string) (string, string, bool) {
+// satKeyStatus maps a raw SAT overall_status to a hardware component status
+// and a human-readable description. For FAILED, "<label> failed" alone
+// tells an engineer nothing they didn't already know from the Critical
+// badge — kv (the parsed summary.txt) is consulted via
+// satFailureDetailFromKV for the actual reason (which GPU/NVLink pair, or
+// which sub-job and its exit code) so ErrorDescription/StatusHistory carry
+// something actionable instead of a dead end.
+func satKeyStatus(rawStatus, label string, kv map[string]string) (string, string, bool) {
 	switch strings.ToUpper(strings.TrimSpace(rawStatus)) {
 	case "OK":
 		// No error description on success — error_description is for problems only.
@@ -246,7 +264,11 @@ func satKeyStatus(rawStatus, label string) (string, string, bool) {
 		// Tool couldn't run or test was incomplete — we can't assert hardware health.
 		return "Unknown", "", true
 	case "FAILED":
-		return "Critical", label + " failed", true
+		desc := label + " failed"
+		if reason := satFailureDetailFromKV(kv); reason != "" {
+			desc = label + " failed: " + reason
+		}
+		return "Critical", desc, true
 	default:
 		return "", "", false
 	}
diff --git a/audit/internal/app/sat_overlay_test.go b/audit/internal/app/sat_overlay_test.go
index 85ad8ad..2e3f76c 100644
--- a/audit/internal/app/sat_overlay_test.go
+++ b/audit/internal/app/sat_overlay_test.go
@@ -3,6 +3,7 @@ package app
 import (
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"bee/audit/internal/collector"
@@ -63,6 +64,66 @@ func TestApplyLatestSATStatusesMarksAMDGPUs(t *testing.T) {
 	}
 }
 
+// TestApplyLatestSATStatusesGPUFailureDescriptionNamesFailingSubJob guards
+// against ErrorDescription regressing to a bare "amd GPU SAT failed" — an
+// engineer reading the hardware snapshot (e.g. in a support bundle) needs to
+// know which sub-job failed and its exit code, not just that something did.
+func TestApplyLatestSATStatusesGPUFailureDescriptionNamesFailingSubJob(t *testing.T) {
+	baseDir := t.TempDir()
+	runDir := filepath.Join(baseDir, "gpu-amd-20260325-161436")
+	if err := os.MkdirAll(runDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	raw := "run_at_utc=2026-03-25T16:14:36Z\n" +
+		"rocm-bandwidth-test_rc=1\n" +
+		"rocm-bandwidth-test_status=FAILED\n" +
+		"overall_status=FAILED\n"
+	if err := os.WriteFile(filepath.Join(runDir, "summary.txt"), []byte(raw), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	class := "DisplayController"
+	amdVendorID := collector.AMDVendorID
+	snap := schema.HardwareSnapshot{
+		PCIeDevices: []schema.HardwarePCIeDevice{{DeviceClass: &class, VendorID: &amdVendorID}},
+	}
+
+	applyLatestSATStatuses(&snap, baseDir, nil)
+
+	desc := snap.PCIeDevices[0].ErrorDescription
+	if desc == nil || !strings.Contains(*desc, "rocm-bandwidth-test=FAILED (rc=1)") {
+		t.Fatalf("ErrorDescription=%v, want it to name the failing sub-job and rc", desc)
+	}
+}
+
+// TestApplyLatestSATStatusesStorageFailureDescriptionIncludesRC guards the
+// per-device storage path, which must attribute the rc of *that device's*
+// own failing job rather than any other device's.
+func TestApplyLatestSATStatusesStorageFailureDescriptionIncludesRC(t *testing.T) {
+	baseDir := t.TempDir()
+	runDir := filepath.Join(baseDir, "storage-20260325-161151")
+	if err := os.MkdirAll(runDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	raw := "run_at_utc=2026-03-25T16:11:51Z\n" +
+		"sda_smartctl_health_rc=2\n" +
+		"sda_smartctl_health_status=FAILED\n" +
+		"overall_status=FAILED\n"
+	if err := os.WriteFile(filepath.Join(runDir, "summary.txt"), []byte(raw), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	sda := schema.HardwareStorage{Telemetry: map[string]any{"linux_device": "/dev/sda"}}
+	snap := schema.HardwareSnapshot{Storage: []schema.HardwareStorage{sda}}
+
+	applyLatestSATStatuses(&snap, baseDir, nil)
+
+	desc := snap.Storage[0].ErrorDescription
+	if desc == nil || !strings.Contains(*desc, "rc=2") {
+		t.Fatalf("ErrorDescription=%v, want it to include the failing job's rc", desc)
+	}
+}
+
 func TestApplyLatestSATStatusesMarksNvidiaGPUByPerGPUStatusFile(t *testing.T) {
 	baseDir := t.TempDir()
 	runDir := filepath.Join(baseDir, "gpu-nvidia-20260407-162123")
diff --git a/audit/internal/platform/nvidia_config_check.go b/audit/internal/platform/nvidia_config_check.go
index 8f14f36..e2c91a4 100644
--- a/audit/internal/platform/nvidia_config_check.go
+++ b/audit/internal/platform/nvidia_config_check.go
@@ -470,6 +470,11 @@ func renderNvidiaConfigCheckSummary(status NvidiaConfigCheckStatus) string {
 		fmt.Fprintln(&b, "overall_status=OK")
 	} else {
 		fmt.Fprintln(&b, "overall_status=FAILED")
+		// warnings carries the actual reason(s) in prose so a reader of
+		// summary.txt (or anything parsing it, like sat.DescribeFailure)
+		// never has to fall back to "see the report" — each entry already
+		// names the specific GPU or NVLink pair and what's wrong with it.
+		fmt.Fprintf(&b, "warnings=%s\n", strings.Join(status.Warnings, "; "))
 	}
 	return b.String()
 }
diff --git a/audit/internal/platform/nvidia_config_check_test.go b/audit/internal/platform/nvidia_config_check_test.go
index ec5c4b2..a0bf6ef 100644
--- a/audit/internal/platform/nvidia_config_check_test.go
+++ b/audit/internal/platform/nvidia_config_check_test.go
@@ -166,3 +166,25 @@ func TestRenderNvidiaConfigCheckSummaryOverallStatus(t *testing.T) {
 		t.Fatalf("status with warnings missing overall_status=FAILED:\n%s", got)
 	}
 }
+
+// TestRenderNvidiaConfigCheckSummaryIncludesWarningsField guards that a
+// failed run's summary.txt carries the actual reason in prose (a "warnings"
+// field), not just a bare overall_status=FAILED — app.SATFailureDetail reads
+// this field so a failed task's error message names the specific GPU/NVLink
+// problem instead of telling the engineer to go read summary.txt themselves.
+func TestRenderNvidiaConfigCheckSummaryIncludesWarningsField(t *testing.T) {
+	status := NvidiaConfigCheckStatus{Warnings: []string{
+		"GPU 0 (H100): ECC is disabled",
+		"NVLink GPU0<->GPU1: 2/36 NVLinks inactive on a bonded pair",
+	}}
+	got := renderNvidiaConfigCheckSummary(status)
+	want := "warnings=GPU 0 (H100): ECC is disabled; NVLink GPU0<->GPU1: 2/36 NVLinks inactive on a bonded pair\n"
+	if !strings.Contains(got, want) {
+		t.Fatalf("summary missing combined warnings field:\ngot:\n%s\nwant substring:\n%s", got, want)
+	}
+
+	clean := NvidiaConfigCheckStatus{}
+	if got := renderNvidiaConfigCheckSummary(clean); strings.Contains(got, "warnings=") {
+		t.Fatalf("clean status summary should omit warnings field entirely:\n%s", got)
+	}
+}
diff --git a/audit/internal/webui/task_runner.go b/audit/internal/webui/task_runner.go
index 4f4b8a0..355d4ee 100644
--- a/audit/internal/webui/task_runner.go
+++ b/audit/internal/webui/task_runner.go
@@ -437,7 +437,11 @@ func executeTaskWithOptions(opts *HandlerOptions, t *Task, j *jobState, ctx cont
 	if archive != "" {
 		archivePath := app.ExtractArchivePath(archive)
 		if err == nil && app.ReadSATOverallStatus(archivePath) == "FAILED" {
-			err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)")
+			if reason := app.SATFailureDetail(archivePath); reason != "" {
+				err = fmt.Errorf("SAT FAILED: %s", reason)
+			} else {
+				err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)")
+			}
 		}
 		// See tasks.go's identical guard: a user-aborted run must not
 		// overwrite the component status DB with a partial result.
diff --git a/audit/internal/webui/tasks.go b/audit/internal/webui/tasks.go
index ea1333b..93a3adc 100644
--- a/audit/internal/webui/tasks.go
+++ b/audit/internal/webui/tasks.go
@@ -1150,7 +1150,11 @@ func (q *taskQueue) runTask(t *Task, j *jobState, ctx context.Context) {
 		archivePath := app.ExtractArchivePath(archive)
 		if err == nil {
 			if app.ReadSATOverallStatus(archivePath) == "FAILED" {
-				err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)")
+				if reason := app.SATFailureDetail(archivePath); reason != "" {
+					err = fmt.Errorf("SAT FAILED: %s", reason)
+				} else {
+					err = fmt.Errorf("SAT overall_status=FAILED (see summary.txt)")
+				}
 			}
 		}
 		// A user-aborted run (ctx canceled) may still have produced a partial