mirror of
https://github.com/replicatedhq/troubleshoot.git
synced 2026-08-27 00:37:20 +00:00
Kris ColemanandClaude Opus 4.7
2192aecf40
* 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>