feat(cli): add aicr diff for configuration drift detection#582
feat(cli): add aicr diff for configuration drift detection#582sanjeevrg89 wants to merge 6 commits intoNVIDIA:mainfrom
Conversation
Add snapshot-vs-recipe comparison that detects constraint violations and component version drift. Includes Prometheus metrics, table output, and comprehensive test coverage. New packages: pkg/diff (core engine, metrics, table renderer) New command: pkg/cli/diff.go registered in root.go
There was a problem hiding this comment.
Welcome — solid first contribution. Good test coverage (190 tests), clean pkg/cli / pkg/diff separation, and smart reuse of constraints.Evaluate. The tactical items (Close errors, missing timeout, dead metrics code) are straightforward fixes — see inline comments.
mchmarny
left a comment
mchmarny
left a comment
There was a problem hiding this comment.
The bigger questions are architectural:
-
Parallel constraint evaluation path —
evaluateConstraintreimplements the loop fromvalidator.checkReadiness(). Consider extracting a shared function so both commands stay in sync. -
Union result type —
Resultserves both modes via optional fields keyed on.Mode. Separate types per mode would be type-safe and easier to schema-validate for CI consumers. -
Component drift matching is heuristic — string-matching
ComponentRef.Nameagainst image keys will produce false "not-observed" in real clusters where image names don't exactly match component names. -
Scope — Snapshot-vs-snapshot comparison is genuinely novel. Recipe-vs-snapshot is a read-only subset of what
validatealready does. Would it be cleaner to ship snapshot-vs-snapshot here, and add recipe drift asaicr validate --drift-only(skip job execution, report constraint state)? That keeps constraint evaluation in one code path.
yuanchen8911
left a comment
yuanchen8911
left a comment
There was a problem hiding this comment.
Two regressions remain in the GPU workflow changes.
The new trigger/filter lists no longer include pkg/validator/checks/conformance/** or pkg/validator/checks/deployment/**. That means a PR that only changes validator checks will stop triggering these GPU workflows on pull-request/* pushes, creating a silent CI gap for the main end-to-end validator path.
The conformance evidence collection step now requires validate-conformance to have run with a success or failure outcome. If chainsaw test or expected-resource verification fails first, validate-conformance is skipped and evidence collection never runs. Previously we still uploaded conformance evidence after those earlier failures, which was useful for debugging common GPU CI breakages. The same regression exists in both the inference and training workflows.
|
I think this is really two different features. Snapshot-vs-snapshot feels like a natural That also reinforces my earlier concern about the union My preference would be to keep |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Welcome, and thanks for the contribution -- configuration drift detection is a useful feature. Mark's review covers the key architectural questions well. The main one to address first is the scope question: whether recipe-vs-snapshot belongs here or under validate --drift-only, since that drives the type design (union Result vs separate types) and avoids the parallel constraint evaluation path.
Once you have a chance to respond to the review feedback, the tactical items (Close errors, missing timeout, unused metrics registration) should be quick fixes. Looking forward to seeing the next iteration.
Address review feedback on NVIDIA#582 by removing recipe-vs-snapshot mode from aicr diff. Reviewers agreed recipe drift belongs under validate --drift-only to keep constraint evaluation in one code path. Changes: - Remove RecipeVsSnapshot path, union Result type, component drift matching - Remove pkg/diff/metrics.go (dead code — promauto was registering gauges at init that nothing populated) - Add context.WithTimeout on CLI I/O using defaults.CLISnapshotTimeout - Fix writable f.Close() and serializer closer.Close() to capture errors Preserved in pkg/diff/_future/ with a README documenting the use cases that justify recipe-vs-snapshot drift detection (airgapped audit, historical compliance, fleet-scale monitoring, CI/CD gating) and a reuse plan for validate --drift-only. Net: -1,004 lines. 176 tests pass with -race. go vet clean.
|
Thanks @mchmarny, @yuanchen8911, and @ArangoGutierrez for the detailed feedback — all well taken. Pushed an update that scopes this PR down to snapshot-vs-snapshot only, which I think addresses the architectural concerns. What changed in this push:
Net change is -1,004 lines on the feature. 196 tests pass with On recipe-vs-snapshot: I agree with the suggestion to put it under
The reviewers' concern was that constraint evaluation should live in one path — they're right. The suggested Let me know if the scope on this PR now feels right. |
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/cli/diff_test.go`:
- Around line 39-51: The variable name requiredFlags in pkg/cli/diff_test.go is
misleading because some entries are optional; rename it to expectedFlags and
update all references in the test (the slice definition and the for-range that
iterates it) so the intent is clear, keeping the rest of the logic (loop over
cmd.Flags and hasFlag) unchanged; ensure the test error message that uses
flagName remains the same.
In `@pkg/cli/diff.go`:
- Around line 138-141: The drift-detection branch currently returns
errors.New(..., errors.ErrCodeInternal) which is the wrong code; update the
error creation in the block that checks cmd.Bool("fail-on-drift") &&
result.HasDrift() to use errors.ErrCodeInvalidRequest (or add a new constant
like ErrCodeConfigurationMismatch in the central error codes and use that)
instead of ErrCodeInternal, and ensure the new/changed error code maps to the
intended exit code (exit code 2) in the error handling/mapping logic.
In `@pkg/diff/_future/cli_diff.go.bak`:
- Around line 251-259: The table branch currently treats any non-empty output as
a filesystem path (creating a file for "--output -"), so in the block guarded by
outFormat == serializer.FormatTable replace the manual os.Create logic with the
same normalization used elsewhere: call serializer.NewFileWriterOrStdout(output)
to obtain the writer, propagate any error, assign the returned writer to w, and
if the returned writer implements io.Closer defer Close() to avoid leaking file
descriptors; reference the symbols outFormat, serializer.FormatTable, output, w
and serializer.NewFileWriterOrStdout when locating and changing the code.
- Around line 258-272: The JSON/YAML output path currently ignores Close()
errors on the serializer (ser) just like the earlier file branch where defer
f.Close() drops errors; change both close paths to capture and propagate Close()
failures: for the file branch replace defer f.Close() with a deferred closure
that checks f.Close() error and returns or wraps it into the function's error
return, and for the serializer branch change the anonymous defer to call
closer.Close(), check its error, and if non-nil wrap and return it (preserve
existing error wrapping style used around serializer.NewFileWriterOrStdout);
reference the variables/functions ser, f, serializer.NewFileWriterOrStdout and
ensure the function returns the Close() error instead of discarding it.
In `@pkg/diff/_future/README.md`:
- Line 1: Replace the current H2 heading "## Recipe-vs-Snapshot Drift Detection
(deferred)" with a top-level H1 heading so the README begins with a single
top-level heading; update the line in pkg/diff/_future/README.md that contains
"Recipe-vs-Snapshot Drift Detection (deferred)" to use an H1 (single leading
hash) instead of H2 to satisfy MD041.
In `@pkg/diff/_future/recipe_diff.go.bak`:
- Around line 267-281: The current loop in the block that assigns
result.ValidationPhases = checkValidationPhases(rec, snap) is aggregating
phase-level constraint outcomes (iterating vp.ConstraintResults) into
result.Summary.Constraints* while those constraint rows are not present in
result.ConstraintResults, causing inconsistent counts. Fix by either (A) keeping
separate phase counters on result (e.g., result.PhaseSummary.* or new fields)
and increment those instead of result.Summary, or (B) append/flatten each
vp.ConstraintResults into result.ConstraintResults before computing
result.Summary.Total and the ConstraintsPassed/Failed/Error counts; update the
code paths that compute result.Summary (referencing result.ValidationPhases,
vp.ConstraintResults, result.ConstraintResults, and result.Summary) so top-level
summary only reflects entries actually present in result.ConstraintResults.
- Around line 230-236: Guard against nil inputs at the start of
RecipeVsSnapshot: check if rec==nil or snap==nil before using rec.Constraints or
dereferencing snap, and return a safe empty Result (or an explicit error Result)
instead of letting the function panic. Concretely, compute a constraintsLen :=
0; if rec != nil { constraintsLen = len(rec.Constraints) } and use that when
allocating ConstraintResults, and if snap == nil or rec == nil return the
initialized Result (Mode "recipe-vs-snapshot") immediately or set an error field
so downstream code in RecipeVsSnapshot, and any uses of rec or snap, do not
dereference nil values.
In `@pkg/diff/diff.go`:
- Around line 86-92: The docstring for Snapshots contradicts its behavior:
instead of requiring non-nil inputs, the function currently returns an empty
*Result when baseline or target is nil; update the comment for Snapshots to
state it is nil-safe and that when either baseline or target is nil the function
returns an empty Result (with Changes initialized empty), referencing the Result
and Change return values and the snapshotter.Snapshot parameters so callers know
they don't need to pre-validate; do not change the function signature.
In `@pkg/diff/table.go`:
- Around line 56-60: The else branch printing "NO DRIFT" is unreachable because
the function returns when len(result.Changes) == 0 and by the time we reach this
block result.HasDrift() will always be true; remove the dead else branch and
simplify the block to always print the drift message (e.g., replace the if/else
using result.HasDrift() with a single fmt.Fprintln(w, "DRIFT DETECTED")). Ensure
references to result.Changes, result.Summary.Total (populated by Snapshots()),
and result.HasDrift() remain correct elsewhere.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 392d5693-49cb-4486-be78-91531e3ee884
📒 Files selected for processing (13)
pkg/cli/diff.gopkg/cli/diff_test.gopkg/cli/root.gopkg/diff/_future/README.mdpkg/diff/_future/cli_diff.go.bakpkg/diff/_future/metrics.go.bakpkg/diff/_future/metrics_test.go.bakpkg/diff/_future/recipe_diff.go.bakpkg/diff/_future/recipe_diff_test.go.bakpkg/diff/_future/table.go.bakpkg/diff/diff.gopkg/diff/diff_test.gopkg/diff/table.go
| if outFormat == serializer.FormatTable { | ||
| w := os.Stdout | ||
| if output != "" { | ||
| f, err := os.Create(output) | ||
| if err != nil { | ||
| return errors.Wrap(errors.ErrCodeInternal, "failed to create output file", err) | ||
| } | ||
| defer f.Close() | ||
| w = f |
There was a problem hiding this comment.
Honor stdout sentinels in the table path.
The table branch treats any non-empty --output as a filesystem path, so --output - (and the stdout URI) will create a file instead of writing to stdout. Reuse the same output normalization as serializer.NewFileWriterOrStdout here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/diff/_future/cli_diff.go.bak` around lines 251 - 259, The table branch
currently treats any non-empty output as a filesystem path (creating a file for
"--output -"), so in the block guarded by outFormat == serializer.FormatTable
replace the manual os.Create logic with the same normalization used elsewhere:
call serializer.NewFileWriterOrStdout(output) to obtain the writer, propagate
any error, assign the returned writer to w, and if the returned writer
implements io.Closer defer Close() to avoid leaking file descriptors; reference
the symbols outFormat, serializer.FormatTable, output, w and
serializer.NewFileWriterOrStdout when locating and changing the code.
| defer f.Close() | ||
| w = f | ||
| } | ||
| return diff.WriteTable(w, result) | ||
| } | ||
|
|
||
| // JSON/YAML use standard serializer | ||
| ser, err := serializer.NewFileWriterOrStdout(outFormat, output) | ||
| if err != nil { | ||
| return errors.Wrap(errors.ErrCodeInternal, "failed to create output writer", err) | ||
| } | ||
| defer func() { | ||
| if closer, ok := ser.(interface{ Close() error }); ok { | ||
| _ = closer.Close() | ||
| } |
There was a problem hiding this comment.
Propagate close failures from output writers.
Both close paths drop errors. That can silently lose the last buffered bytes and still return success. The future copy should keep the same close/error pattern as the live CLI before it is brought back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/diff/_future/cli_diff.go.bak` around lines 258 - 272, The JSON/YAML
output path currently ignores Close() errors on the serializer (ser) just like
the earlier file branch where defer f.Close() drops errors; change both close
paths to capture and propagate Close() failures: for the file branch replace
defer f.Close() with a deferred closure that checks f.Close() error and returns
or wraps it into the function's error return, and for the serializer branch
change the anonymous defer to call closer.Close(), check its error, and if
non-nil wrap and return it (preserve existing error wrapping style used around
serializer.NewFileWriterOrStdout); reference the variables/functions ser, f,
serializer.NewFileWriterOrStdout and ensure the function returns the Close()
error instead of discarding it.
| @@ -0,0 +1,75 @@ | |||
| ## Recipe-vs-Snapshot Drift Detection (deferred) | |||
There was a problem hiding this comment.
Add top-level heading per markdown standards.
Static analysis flags MD041: first line should be a top-level heading. The current line 1 is ## Recipe-vs-Snapshot Drift Detection (deferred) which is H2.
📝 Suggested fix
+# Deferred Implementation
+
## Recipe-vs-Snapshot Drift Detection (deferred)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Recipe-vs-Snapshot Drift Detection (deferred) | |
| # Deferred Implementation | |
| ## Recipe-vs-Snapshot Drift Detection (deferred) |
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/diff/_future/README.md` at line 1, Replace the current H2 heading "##
Recipe-vs-Snapshot Drift Detection (deferred)" with a top-level H1 heading so
the README begins with a single top-level heading; update the line in
pkg/diff/_future/README.md that contains "Recipe-vs-Snapshot Drift Detection
(deferred)" to use an H1 (single leading hash) instead of H2 to satisfy MD041.
| func RecipeVsSnapshot(rec *recipe.RecipeResult, snap *snapshotter.Snapshot) *Result { | ||
| result := &Result{ | ||
| Mode: "recipe-vs-snapshot", | ||
| ConstraintResults: make([]ConstraintResult, 0, len(rec.Constraints)), | ||
| ComponentDrifts: make([]ComponentDrift, 0), | ||
| ValidationPhases: make([]ValidationPhaseSummary, 0), | ||
| } |
There was a problem hiding this comment.
Guard nil recipe/snapshot inputs before constructing the result.
len(rec.Constraints) panics when rec is nil, and a nil snap is dereferenced later during constraint and component evaluation. For a reusable package API, this should return a safe empty result or an explicit error path instead of crashing. Based on learnings: Functional Packages (pkg/oci, pkg/bundler, pkg/recipe, pkg/collector, pkg/validator) must be self-contained, reusable business logic usable independently without CLI/API.
Possible hardening
func RecipeVsSnapshot(rec *recipe.RecipeResult, snap *snapshotter.Snapshot) *Result {
+ if rec == nil || snap == nil {
+ return &Result{
+ Mode: "recipe-vs-snapshot",
+ ConstraintResults: []ConstraintResult{},
+ ComponentDrifts: []ComponentDrift{},
+ ValidationPhases: []ValidationPhaseSummary{},
+ }
+ }
+
result := &Result{
Mode: "recipe-vs-snapshot",
ConstraintResults: make([]ConstraintResult, 0, len(rec.Constraints)),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func RecipeVsSnapshot(rec *recipe.RecipeResult, snap *snapshotter.Snapshot) *Result { | |
| result := &Result{ | |
| Mode: "recipe-vs-snapshot", | |
| ConstraintResults: make([]ConstraintResult, 0, len(rec.Constraints)), | |
| ComponentDrifts: make([]ComponentDrift, 0), | |
| ValidationPhases: make([]ValidationPhaseSummary, 0), | |
| } | |
| func RecipeVsSnapshot(rec *recipe.RecipeResult, snap *snapshotter.Snapshot) *Result { | |
| if rec == nil || snap == nil { | |
| return &Result{ | |
| Mode: "recipe-vs-snapshot", | |
| ConstraintResults: []ConstraintResult{}, | |
| ComponentDrifts: []ComponentDrift{}, | |
| ValidationPhases: []ValidationPhaseSummary{}, | |
| } | |
| } | |
| result := &Result{ | |
| Mode: "recipe-vs-snapshot", | |
| ConstraintResults: make([]ConstraintResult, 0, len(rec.Constraints)), | |
| ComponentDrifts: make([]ComponentDrift, 0), | |
| ValidationPhases: make([]ValidationPhaseSummary, 0), | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/diff/_future/recipe_diff.go.bak` around lines 230 - 236, Guard against
nil inputs at the start of RecipeVsSnapshot: check if rec==nil or snap==nil
before using rec.Constraints or dereferencing snap, and return a safe empty
Result (or an explicit error Result) instead of letting the function panic.
Concretely, compute a constraintsLen := 0; if rec != nil { constraintsLen =
len(rec.Constraints) } and use that when allocating ConstraintResults, and if
snap == nil or rec == nil return the initialized Result (Mode
"recipe-vs-snapshot") immediately or set an error field so downstream code in
RecipeVsSnapshot, and any uses of rec or snap, do not dereference nil values.
| // 3. Summarize validation phase configuration and count phase-level constraints | ||
| result.ValidationPhases = checkValidationPhases(rec, snap) | ||
| for _, vp := range result.ValidationPhases { | ||
| for _, cr := range vp.ConstraintResults { | ||
| if cr.Error != "" { | ||
| result.Summary.ConstraintsError++ | ||
| } else if cr.Passed { | ||
| result.Summary.ConstraintsPassed++ | ||
| } else { | ||
| result.Summary.ConstraintsFailed++ | ||
| } | ||
| } | ||
| } | ||
|
|
||
| result.Summary.Total = len(result.ConstraintResults) |
There was a problem hiding this comment.
Don't mix phase constraint totals into the top-level constraint summary.
This loop folds phase-level constraint outcomes into Summary.Constraints*, but those rows are not added to ConstraintResults. Any consumer that renders ConstraintResults with Summary will show inconsistent counts versus rows. Keep separate phase counters, or flatten phase constraint results into the main slice before summarizing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/diff/_future/recipe_diff.go.bak` around lines 267 - 281, The current loop
in the block that assigns result.ValidationPhases = checkValidationPhases(rec,
snap) is aggregating phase-level constraint outcomes (iterating
vp.ConstraintResults) into result.Summary.Constraints* while those constraint
rows are not present in result.ConstraintResults, causing inconsistent counts.
Fix by either (A) keeping separate phase counters on result (e.g.,
result.PhaseSummary.* or new fields) and increment those instead of
result.Summary, or (B) append/flatten each vp.ConstraintResults into
result.ConstraintResults before computing result.Summary.Total and the
ConstraintsPassed/Failed/Error counts; update the code paths that compute
result.Summary (referencing result.ValidationPhases, vp.ConstraintResults,
result.ConstraintResults, and result.Summary) so top-level summary only reflects
entries actually present in result.ConstraintResults.
|
@sanjeevrg89 looks like some linting issues remain, otherwise LGTM. Sounds like coderabbit though has some other ideas ;) |
Address review feedback on NVIDIA#582 by narrowing aicr diff to just snapshot-vs-snapshot comparison. Reviewers agreed recipe-vs-snapshot drift belongs under validate --drift-only to keep constraint evaluation in one code path. Architectural changes: - Remove RecipeVsSnapshot path, union Result type, component drift matching, and the parallel constraint evaluation loop - Remove pkg/diff/metrics.go (dead code — promauto was registering gauges at init that nothing populated) Tactical review fixes: - Add context.WithTimeout on CLI I/O using defaults.CLISnapshotTimeout - Capture and log errors from writable f.Close() and serializer closer.Close() instead of discarding them - Use ErrCodeInvalidRequest (exit 2) instead of ErrCodeInternal (exit 8) when fail-on-drift triggers — drift is an expected outcome, not an internal failure - Document nil-safe behavior on Snapshots() instead of claiming both inputs must be non-nil - Remove unreachable "NO DRIFT" branch in table.go (early return handles the empty-changes path) - Rename requiredFlags to expectedFlags in diff_test.go Lint fixes: - gocritic unlambda: drop wrapper closure around runDiffCmd - govet shadow: reassign err instead of shadowing at initDataProvider - prealloc: preallocate changes slice in addedMeasurement and removedMeasurement Preserved in pkg/diff/_future/ with a README documenting the use cases (airgapped audit, historical compliance, fleet-scale monitoring, CI/CD gating, pre-flight validate, evidence trail) that justify recipe-vs-snapshot drift detection as a first-class capability under validate --drift-only. golangci-lint v2.10.1: 0 issues. 196 tests pass with -race.
|
Thanks @mchmarny — linting's green now. Just pushed a fix. Resolved all 4 golangci-lint failures from
While I was in there I also took the valid CodeRabbit nits on the live code:
Skipped the CodeRabbit suggestions against files in Verified locally with |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/cli/diff.go (1)
153-161:⚠️ Potential issue | 🟠 MajorReturn
Close()failures for writable outputs instead of only logging them.
Close()is where buffered I/O errors often surface. Logging these failures still letsaicr diffexit 0 after a truncated output file. MakewriteDiffResultuse a namederrreturn and promote close failures when no earlier error exists.💾 Suggested pattern
-func writeDiffResult(ctx context.Context, cmd *cli.Command, outFormat serializer.Format, result *diff.Result) error { +func writeDiffResult(ctx context.Context, cmd *cli.Command, outFormat serializer.Format, result *diff.Result) (err error) { output := cmd.String("output") ... defer func() { - if closeErr := f.Close(); closeErr != nil { - slog.Error("failed to close output file", "error", closeErr) + if closeErr := f.Close(); closeErr != nil && err == nil { + err = errors.Wrap(errors.ErrCodeInternal, "failed to close output file", closeErr) } }() ... defer func() { if closer, ok := ser.(interface{ Close() error }); ok { - if closeErr := closer.Close(); closeErr != nil { - slog.Error("failed to close serializer", "error", closeErr) + if closeErr := closer.Close(); closeErr != nil && err == nil { + err = errors.Wrap(errors.ErrCodeInternal, "failed to close serializer", closeErr) } } }()As per coding guidelines "Always capture and check Close() error on writable file handles (files opened with Create or OpenFile with write modes)".
Also applies to: 172-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/cli/diff.go` around lines 153 - 161, The defer that closes the created file (variable f) must promote Close() failures to the function return instead of only logging them: change writeDiffResult to use a named return variable (err) and in the defer check closeErr := f.Close(); if closeErr != nil && err == nil { err = errors.Wrap(errors.ErrCodeInternal, "failed to close output file", closeErr) } so Close() errors are returned; apply the same pattern for the other writable output file close at the second occurrence (lines around the other defer) to ensure buffered-write errors are propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/diff/diff.go`:
- Around line 152-156: indexMeasurements currently dereferences m.Type without
checking for nil, causing a panic if measurements contains nil entries; update
the function (indexMeasurements in pkg/diff/diff.go) to skip nil measurement
pointers by adding a guard (if m == nil { continue }) before accessing m.Type so
nil entries are ignored (or handled) instead of panicking, ensuring the map
creation remains safe on invalid input.
---
Duplicate comments:
In `@pkg/cli/diff.go`:
- Around line 153-161: The defer that closes the created file (variable f) must
promote Close() failures to the function return instead of only logging them:
change writeDiffResult to use a named return variable (err) and in the defer
check closeErr := f.Close(); if closeErr != nil && err == nil { err =
errors.Wrap(errors.ErrCodeInternal, "failed to close output file", closeErr) }
so Close() errors are returned; apply the same pattern for the other writable
output file close at the second occurrence (lines around the other defer) to
ensure buffered-write errors are propagated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6efb79ea-a4e0-4d31-8e33-44cff404500f
📒 Files selected for processing (4)
pkg/cli/diff.gopkg/cli/diff_test.gopkg/diff/diff.gopkg/diff/table.go
Three fixes from review: - pkg/diff/table.go: wrap writes to the output Writer with an errWriter helper so the first write failure (broken pipe, full disk) surfaces as a structured pkg/errors.Wrap(ErrCodeInternal, ...) return value instead of a silent exit 0. Also wraps the bare err from tw.Flush() that CLAUDE.md's "never return bare err" rule forbids. - pkg/cli/diff.go: convert writeDiffResult to a named-return function and promote Close() failures on the output file and serializer into the return value when no earlier error exists. Buffered-write failures on Close were previously logged-and-lost, allowing aicr diff to exit 0 after a truncated output file. - pkg/diff/diff.go: guard against nil *measurement.Measurement entries in indexMeasurements. A snapshot loaded from malformed YAML could contain nil entries and panic the dereference on m.Type; now skipped cleanly. Added tests for the new failure paths: TestSnapshots_NilMeasurementEntries exercises the nil guard, TestWriteTable_PropagatesWriteErrors exercises both the empty and non-empty output branches with a failing writer. golangci-lint v2.10.1: 0 issues. 200 tests pass with -race.
|
Good catches from CodeRabbit — pushed fixes for all three: 1. 2. 3. Added two tests for the new failure paths:
|
5 participants
Adds
aicr diff— compares a snapshot against a recipe to surface constraint violations and component version drift.What's included:
Scoped to just the diff feature per @xdu31's feedback on #464 — node-validate and node-scoped constraints will follow as separate PRs.
Summary by CodeRabbit
New Features
Documentation
Tests