fix(analyze/secret): guard nil deref when spec omits fail: outcome (#2053) (#2054)

* fix(analyze/secret): guard nil deref when spec omits fail: outcome (#2053)

When a Preflight spec defines only warn: and/or pass: outcomes and the
target Secret is missing, analyzeSecret dereferenced a nil failOutcome and
panicked. Reproduction from replicatedhq/troubleshoot#2053:

  outcomes:
    - warn:
        when: "notFound"
        message: "secret missing (warn)"
    - pass:
        message: "secret found"

This change:
- Collects fail, warn, and warn-when-notFound outcomes up front.
- Routes a missing secret (or missing key) through a single resolver:
  prefer warn(when=notFound), fall back to fail, then any warn,
  then synthesize a benign warn result. Never panics.
- Adds table-driven tests covering the three new shapes.

Same defect shape as #263 (imagePullSecret). The sibling analyzers
configmap.go and image.go have the same pattern but are out of scope
for this PR.

Refs: replicatedhq/troubleshoot#2053

* refactor(analyze/secret): drop warn handling per analyzer contract

Per review feedback (banjoh): the secret analyzer only supports fail
(not found) and pass (found) outcomes — `warn:` and `when:` are not
part of the analyzer's contract (https://troubleshoot.sh/docs/analyze/secrets).

Drop the warn / notFoundWarn branches added in the previous commit and
collapse the nil-fail fallback onto IsFail with a default message. The
core fix — guarding the nil deref at the previous secret.go:72 — stays
in place.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* refactor(analyze/secret): mirror image_pull_secret pattern; return nil when spec has neither outcome

Per banjoh review: collect failOutcome and passOutcome with non-nil
checks; when the spec contains neither, return nil and let the framework
surface the missing-outcome error rather than fabricating a result.

Structure now mirrors pkg/analyze/image_pull_secret.go: default to
IsFail with fail-outcome message (if set), flip to IsPass with
pass-outcome message when the secret/key check succeeds, fill default
messages at the end only when none were configured. Analyze() now
forwards a nil result through cleanly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(analyze/secret): assign outcome messages inside their own branch

Cursor bugbot caught: when the spec defined only a fail outcome and the
secret was found, the result showed IsPass=true with the fail outcome's
message. Pre-assigning the fail message at the top before flipping
IsPass meant the stale message leaked through whenever no pass outcome
was configured.

Move both message assignments into their respective branches so a pass
result never carries a fail message. Default messages still fill in
when no outcome is set on the active branch. Added a regression test
covering fail-only spec + secret found.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(analyze/secret): address Greptile P1s — combined outcome + empty configured messages

Two fixes flagged by review on #2054:

1. Combined pass outcome dropped: the outcome loop used 'else if' for pass, so a
   single outcome object containing both fail and pass silently dropped the pass.
   Capture fail and pass independently.

2. Empty configured messages overwritten: the default-message fallback fired
   whenever result.Message was empty, clobbering a configured outcome that
   intentionally has an empty message (e.g. URI-only). Track whether the matched
   branch had a configured outcome and only fall back to a default when none was
   supplied, preserving the configured message and URI verbatim.

Adds tests: combined fail+pass captured on both found/not-found paths, and
URI-only pass/fail outcomes preserved without default-message overwrite.

* fix(analyze/secret): drop fabricated default messages; error on missing outcome

Per maintainer review: analyzers do not fabricate default messages — an empty
outcome message is intentional (e.g. a URI-only outcome), so remove the default
message fallback and preserve the configured outcome verbatim. When a matched
branch has no configured outcome, the message stays empty.

Also address the missing-outcome case: when a spec defines neither a pass nor a
fail outcome, analyzeSecret returned (nil, nil), which the Analyze wrapper
swallowed into an empty result slice — the user saw neither a result nor a
config error. Return an explicit error so the framework surfaces the
misconfiguration.

Tests updated: the no-fail-outcome and only-fail-outcome cases now assert an
empty message rather than a fabricated one, and the neither-outcome case asserts
an error.

* fix(analyze/secret): default message only when no outcome is configured for the matched branch

Distinguish an absent matching outcome from an intentionally empty configured
message. A configured outcome with an empty message (e.g. a URI-only outcome) is
still preserved verbatim. But when the matched branch has no configured outcome at
all — a pass-only spec that took the fail path, or a fail-only spec that passed —
fall back to a default diagnostic instead of emitting an empty message.

Addresses the greptile P1 (endorsed by banjoh): dropping the default entirely
conflated the two cases and left users with a pass/fail result and no context.
Tests restore the default-message expectations for the no-configured-outcome
branches; URI-only (configured-empty) and neither-outcome (error) cases unchanged.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

pkg/analyze/secret.go

@@ -30,6 +30,9 @@ func (a *AnalyzeSecret) Analyze(getFile getCollectedFileContents, findFiles getC
 	if err != nil {
 		return nil, err
 	}
+	if result == nil {
+		return nil, nil
+	}
 	result.Strict = a.analyzer.Strict.BoolOrDefaultFalse()
 	return []*AnalyzeResult{result}, nil
 }
@@ -54,42 +57,68 @@ func (a *AnalyzeSecret) analyzeSecret(analyzer *troubleshootv1beta2.AnalyzeSecre
 		return nil, err
 	}
 
+	// The secret analyzer only supports fail (not found) and pass (found) outcomes
+	// per https://troubleshoot.sh/docs/analyze/secrets. If the spec contains
+	// neither, return an explicit error: returning (nil, nil) is swallowed by the
+	// Analyze wrapper into an empty result slice, so the misconfiguration would
+	// surface as neither a result nor an error.
+	// Capture fail and pass independently: a single outcome object may set both,
+	// so an else-if here would silently drop the second one.
+	var failOutcome, passOutcome *troubleshootv1beta2.SingleOutcome
+	for _, outcome := range analyzer.Outcomes {
+		if outcome.Fail != nil {
+			failOutcome = outcome.Fail
+		}
+		if outcome.Pass != nil {
+			passOutcome = outcome.Pass
+		}
+	}
+	if failOutcome == nil && passOutcome == nil {
+		return nil, fmt.Errorf("secret analyzer %s/%s must define at least one pass or fail outcome", analyzer.Namespace, analyzer.SecretName)
+	}
+
 	result := AnalyzeResult{
 		Title:   a.Title(),
 		IconKey: "kubernetes_analyze_secret",
 		IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
 	}
 
-	var failOutcome *troubleshootv1beta2.Outcome
-	for _, outcome := range analyzer.Outcomes {
-		if outcome.Fail != nil {
-			failOutcome = outcome
-		}
+	secretFound := foundSecret.SecretExists
+	if secretFound && analyzer.Key != "" {
+		secretFound = foundSecret.Key == analyzer.Key && foundSecret.KeyExists
 	}
-
-	if !foundSecret.SecretExists {
+	// Use the matched branch's configured outcome verbatim, tracking whether one
+	// was actually present. A configured outcome with an intentionally empty
+	// message (e.g. a URI-only outcome) is preserved as-is. But when the matched
+	// branch has NO configured outcome at all — e.g. a pass-only spec that took
+	// the fail path, or a fail-only spec that passed — the empty message is not
+	// an intentional choice, so fall back to a default diagnostic. An absent
+	// outcome is not the same as an intentionally empty one.
+	outcomeConfigured := false
+	if secretFound {
+		result.IsPass = true
+		if passOutcome != nil {
+			result.Message = passOutcome.Message
+			result.URI = passOutcome.URI
+			outcomeConfigured = true
+		}
+	} else {
 		result.IsFail = true
-		result.Message = failOutcome.Fail.Message
-		result.URI = failOutcome.Fail.URI
-
-		return &result, nil
-	}
-
-	if analyzer.Key != "" {
-		if foundSecret.Key != analyzer.Key || !foundSecret.KeyExists {
-			result.IsFail = true
-			result.Message = failOutcome.Fail.Message
-			result.URI = failOutcome.Fail.URI
-
-			return &result, nil
+		if failOutcome != nil {
+			result.Message = failOutcome.Message
+			result.URI = failOutcome.URI
+			outcomeConfigured = true
 		}
 	}
 
-	result.IsPass = true
-	for _, outcome := range analyzer.Outcomes {
-		if outcome.Pass != nil {
-			result.Message = outcome.Pass.Message
-			result.URI = outcome.Pass.URI
+	if !outcomeConfigured {
+		switch {
+		case result.IsPass:
+			result.Message = fmt.Sprintf("Secret %s was found in namespace %s", analyzer.SecretName, analyzer.Namespace)
+		case analyzer.Key != "" && foundSecret.SecretExists:
+			result.Message = fmt.Sprintf("Key %s was not found in secret %s/%s", analyzer.Key, analyzer.Namespace, analyzer.SecretName)
+		default:
+			result.Message = fmt.Sprintf("Secret %s was not found in namespace %s", analyzer.SecretName, analyzer.Namespace)
 		}
 	}
 

pkg/analyze/secret_test.go

@@ -166,6 +166,118 @@ func Test_analyzeSecret(t *testing.T) {
 				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
 			},
 		},
+		{
+			name: "not found with no fail outcome falls back to a default message (no configured outcome for this branch)",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				AnalyzeMeta: troubleshootv1beta2.AnalyzeMeta{
+					CheckName: "Optional Secret",
+				},
+				Namespace:  "default",
+				SecretName: "does-not-exist",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							Message: "secret found",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/default/does-not-exist.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "default",
+					Name:         "does-not-exist",
+					SecretExists: false,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsFail:  true,
+				Message: "Secret does-not-exist was not found in namespace default",
+				Title:   "Optional Secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "key not found with no fail outcome falls back to a default message (no configured outcome for this branch)",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				AnalyzeMeta: troubleshootv1beta2.AnalyzeMeta{
+					CheckName: "Optional Secret Key",
+				},
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Key:        "missing-key",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							Message: "key found",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret/missing-key.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					Key:          "missing-key",
+					SecretExists: true,
+					KeyExists:    false,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsFail:  true,
+				Message: "Key missing-key was not found in secret test-namespace/test-secret",
+				Title:   "Optional Secret Key",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "found with only fail outcome configured falls back to the default pass message, not the fail outcome's message",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							Message: "Not found",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					SecretExists: true,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsPass:  true,
+				Message: "Secret test-secret was found in namespace test-namespace",
+				Title:   "Secret test-secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "spec with neither fail nor pass outcome returns an error so the framework surfaces the misconfiguration",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				AnalyzeMeta: troubleshootv1beta2.AnalyzeMeta{
+					CheckName: "Misconfigured",
+				},
+				Namespace:  "default",
+				SecretName: "does-not-exist",
+				Outcomes:   []*troubleshootv1beta2.Outcome{},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/default/does-not-exist.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "default",
+					Name:         "does-not-exist",
+					SecretExists: false,
+				}),
+			},
+			wantErr: true,
+		},
 		{
 			name: "key not found secret not found",
 			analyzer: &troubleshootv1beta2.AnalyzeSecret{
@@ -190,6 +302,128 @@ func Test_analyzeSecret(t *testing.T) {
 			},
 			wantErr: true, // TODO: should this be a not found error? This will not work with selectors.
 		},
+		{
+			name: "combined fail and pass in a single outcome, secret found uses the pass outcome",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							Message: "Not found",
+						},
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							Message: "Found",
+							URI:     "https://example.com/found",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					SecretExists: true,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsPass:  true,
+				Message: "Found",
+				URI:     "https://example.com/found",
+				Title:   "Secret test-secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "combined fail and pass in a single outcome, secret not found uses the fail outcome",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							Message: "Not found",
+						},
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							Message: "Found",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					SecretExists: false,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsFail:  true,
+				Message: "Not found",
+				Title:   "Secret test-secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "secret found with URI-only pass outcome preserves the empty message and URI",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Pass: &troubleshootv1beta2.SingleOutcome{
+							URI: "https://example.com/pass",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					SecretExists: true,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsPass:  true,
+				Message: "",
+				URI:     "https://example.com/pass",
+				Title:   "Secret test-secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
+		{
+			name: "secret not found with URI-only fail outcome preserves the empty message and URI",
+			analyzer: &troubleshootv1beta2.AnalyzeSecret{
+				Namespace:  "test-namespace",
+				SecretName: "test-secret",
+				Outcomes: []*troubleshootv1beta2.Outcome{
+					{
+						Fail: &troubleshootv1beta2.SingleOutcome{
+							URI: "https://example.com/fail",
+						},
+					},
+				},
+			},
+			mockFiles: map[string][]byte{
+				"secrets/test-namespace/test-secret.json": mustJSONMarshalIndent(t, collect.SecretOutput{
+					Namespace:    "test-namespace",
+					Name:         "test-secret",
+					SecretExists: false,
+				}),
+			},
+			want: &AnalyzeResult{
+				IsFail:  true,
+				Message: "",
+				URI:     "https://example.com/fail",
+				Title:   "Secret test-secret",
+				IconKey: "kubernetes_analyze_secret",
+				IconURI: "https://troubleshoot.sh/images/analyzer-icons/secret.svg?w=13&h=16",
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {