validators/chainsaw/inprocess.go (1)

164-185: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Grace window is anchored too early and can prematurely fail transient recreate flows.

absentDeadline is fixed at function start (Line 164), then any later ErrCodeNotFound uses that stale cutoff (Line 184/Line 281). If a resource appears (or returns non-NotFound) and later briefly disappears during rollout, this path can fail immediately even though the resource is not “entirely absent.”
Please make the grace window stateful (only for consecutive initial NotFound, or reset/disable grace after first non-NotFound). Apply the same fix pattern in validators/chainsaw/runner.go Line 169-193.

Also applies to: 280-285

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@validators/chainsaw/inprocess.go` around lines 164 - 185, The current loop
anchors absentDeadline at function start (absentDeadline :=
time.Now().Add(defaults.AbsentResourceGracePeriod)) which causes
notFoundGraceDeadline(lastErr, absentDeadline, deadline) to treat later
transient NotFound errors as still within the original grace window; change the
logic to set/clear the absentDeadline only when you observe a NotFound result
and to reset/disable it when you observe the first non-NotFound (i.e., make the
grace window stateful): remove the fixed absentDeadline initialization,
introduce a nil/zero absentDeadline variable updated inside the loop when
evaluateAssert returns an ErrCodeNotFound, and clear/zero that variable when
evaluateAssert returns any non-NotFound error so subsequent
notFoundGraceDeadline calls use the updated state; apply the same change to the
analogous loop in validators/chainsaw/runner.go (the block around
evaluateAssert/evaluate and notFoundGraceDeadline).

validators/chainsaw/runner.go (1)

169-201: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

NotFound grace is bypassed on raw-resource asserts because fetch errors are re-coded to internal.

At Line 201, the new grace path depends on isNotFoundErr(lastErr), but assertSingleDocument at Line 266 wraps fetcher.Fetch errors as ErrCodeInternal. That clobbers ErrCodeNotFound, so absent resources in raw YAML mode won’t hit the 30s grace and will continue using the full timeout.

💡 Proposed fix (preserve structured codes from fetcher)

diff --git a/validators/chainsaw/runner.go b/validators/chainsaw/runner.go
@@
 	actual, err := fetcher.Fetch(ctx, apiVersion, kind, namespace, name)
 	if err != nil {
-		return errors.Wrap(errors.ErrCodeInternal,
-			fmt.Sprintf("failed to fetch %s %s/%s", kind, namespace, name), err)
+		return errors.PropagateOrWrap(
+			err,
+			errors.ErrCodeInternal,
+			fmt.Sprintf("failed to fetch %s %s/%s", kind, namespace, name),
+		)
 	}

Based on learnings, structured errors should be propagated as-is to preserve their codes, and this grace logic depends on preserving ErrCodeNotFound.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@validators/chainsaw/runner.go` around lines 169 - 201, The NotFound-grace
logic in notFoundGraceDeadline is bypassed because assertSingleDocument
currently re-wraps errors from fetcher.Fetch as ErrCodeInternal, which destroys
ErrCodeNotFound; update assertSingleDocument so it preserves and returns
structured errors from fetcher.Fetch (i.e., if the fetch error already carries a
structured error code such as ErrCodeNotFound, return it as-is) and only wrap
truly unknown/non-structured errors as ErrCodeInternal; this will ensure
assertAllDocuments sees the original ErrCodeNotFound (so isNotFoundErr(lastErr)
and the absentDeadline logic work correctly).

Source: Learnings

validators/chainsaw/runner.go (1)

265-268: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Preserve Fetch error codes on the raw-resource path.

assertRawResources() now decides both the short absent-resource grace and the final surfaced failure code from lastErr, but this wrap collapses every fetcher.Fetch() failure into ErrCodeInternal. That means raw YAML asserts never hit the new ErrCodeNotFound fast-fail path, and transient ErrCodeUnavailable fetch failures are also misclassified as internal failures. In practice, raw-resource checks still burn the full timeout and can report other instead of the intended structured failure.

Suggested fix

 	actual, err := fetcher.Fetch(ctx, apiVersion, kind, namespace, name)
 	if err != nil {
-		return errors.Wrap(errors.ErrCodeInternal,
-			fmt.Sprintf("failed to fetch %s %s/%s", kind, namespace, name), err)
+		return errors.PropagateOrWrap(err, errors.ErrCodeInternal,
+			fmt.Sprintf("failed to fetch %s %s/%s", kind, namespace, name))
 	}

Based on learnings, errors that already carry the correct structured code should be propagated as-is, or via errors.PropagateOrWrap(...), instead of being re-wrapped under a new code.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@validators/chainsaw/runner.go` around lines 265 - 268, The current wrap in
the fetch path (inside the block handling fetcher.Fetch in runner.go) always
converts any fetch error into ErrCodeInternal which prevents
assertRawResources() from seeing original codes like ErrCodeNotFound or
ErrCodeUnavailable; change the error handling so that errors returned by
fetcher.Fetch (used by functions like assertRawResources and assigned to
lastErr) preserve their structured code—either by using
errors.PropagateOrWrap(...) with the descriptive message or by checking the
error's code and returning it directly instead of wrapping into
ErrCodeInternal—so existing ErrCodeNotFound/ErrCodeUnavailable values flow
through unchanged while still annotating the failure with the "failed to fetch
<kind> <namespace>/<name>" context.

Source: Learnings

validators/chainsaw/runner.go (1)

173-205: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Fail fast on terminal assertion-spec errors in the raw-resource retry loop.

assertSingleDocument now classifies malformed/non-evaluable checks as ErrCodeInvalidRequest, but assertRawResources still retries until timeout instead of exiting immediately on terminal errors. This can unnecessarily consume ChainsawMaxParallel slots and prolong runs.

Suggested fix

 for {
 	lastErr = assertAllDocuments(ctx, docs, fetcher)
 	if lastErr == nil {
 		result.Passed = true
 		slog.Info("health check passed", "component", ca.Name)
 		return result
 	}
+	if isTerminalAssertErr(lastErr) {
+		reason := preferSubstantiveErr(lastSubstantiveErr, lastErr)
+		result.Output = reason.Error()
+		result.Error = reason
+		slog.Warn("health check failed", "component", ca.Name, "error", reason)
+		return result
+	}
 
 	// Record the failure seen while the context is still live; after
 	// cancellation assertAllDocuments returns a context / rate-limiter

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@validators/chainsaw/runner.go` around lines 173 - 205, After
assertAllDocuments returns a terminal assertion-spec error (e.g.
ErrCodeInvalidRequest produced by assertSingleDocument), we should stop retrying
and free the ChainsawMaxParallel slot instead of looping to timeout; detect the
terminal code (use the same error-code helper you have for assertions, e.g.
compare assertionErrorCode(lastErr) to ErrCodeInvalidRequest or add an
isTerminalAssertionErr helper), and when true set the result as a clean failure
(populate the result/Error or result.Message similarly to other failure paths),
log the terminal error, and return immediately from the loop (or break to the
same exit path) inside the assertRawResources retry loop that calls
assertAllDocuments so retries do not continue on non-retryable assertion errors.