Refine validate vsa output

[joejstuart]

Refactor fallback image validation

• Walk each data source directory returned by GetPolicy. These are the actual directories (possibly symlinks) where data was downloaded. Walking them directly ensures we find files even if they're symlinks. This was the cause of inconsistent violation reporting.
• Move report writing to shared code under internal/validate. This is so validate image and the fallback validation will show the same result format.

[codecov]

Codecov Report

❌ Patch coverage is 51.29032% with 302 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
cmd/validate/vsa.go	39.16%	278 Missing ⚠️
internal/validate/vsa/validator.go	0.00%	18 Missing ⚠️
internal/evaluator/conftest_evaluator.go	86.36%	3 Missing ⚠️
internal/validate/report.go	97.43%	2 Missing ⚠️
cmd/validate/image.go	96.42%	1 Missing ⚠️

Flag	Coverage Δ
generative	69.00% <51.29%> (+0.20%)	⬆️
integration	69.00% <51.29%> (+0.20%)	⬆️
unit	69.00% <51.29%> (+0.20%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/validate/vsa/result.go	98.18% <100.00%> (+51.95%)	⬆️
internal/validate/vsa/vsa.go	58.94% <ø> (ø)
cmd/validate/image.go	90.07% <96.42%> (+0.92%)	⬆️
internal/validate/report.go	97.43% <97.43%> (ø)
internal/evaluator/conftest_evaluator.go	81.61% <86.36%> (+0.98%)	⬆️
internal/validate/vsa/validator.go	37.58% <0.00%> (-5.51%)	⬇️
cmd/validate/vsa.go	44.44% <39.16%> (-1.77%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joejstuart]

/retest

[robnester-rh]

/analyze

[qodo-code-review]

PR Analysis 🔬

• This screen contains a list of code components that were changed in this PR.
• You can initiate specific actions for each component, by checking the relevant boxes.
• Results will appear as a comment on the PR, typically after 30-60 seconds.

file	Changed components
image.go	Test Docs Improve Similar   validateImageCmd (function)   +36/-74
image_test.go	Test Docs Improve Similar   TestContainsData (function)   +2/-2   Test Docs Improve Similar   TestContainsAttestation (function)   +2/-2
vsa.go	Test Docs Improve Similar   printVSAInfo (function)   +2/-6   Test Docs Improve Similar   validateVSAData (class)   +6/-4   Test Docs Improve Similar   runValidateVSA (function)   +24/-5   Test Docs Improve Similar   validateSnapshotVSAsFromSpec (function)   +18/-0   Test Docs Improve Similar   processComponentsInParallel (function)   +29/-9   Test Docs Improve Similar   ImageStatus (class)   +6/-0   Test Docs Improve Similar   PolicyDiffCounts (class)   +5/-0   Test Docs Improve Similar   AllSectionsData (class)   +31/-0   Test Docs Improve Similar   ComponentResultsDisplay (class)   +7/-0   Test Docs Improve Similar   VSASectionsReport (class)   +6/-0   Test Docs Improve Similar   HeaderReport (class)   +4/-0   Test Docs Improve Similar   ResultReport (class)   +6/-0   Test Docs Improve Similar   VSASummaryReport (class)   +6/-0   Test Docs Improve Similar   PolicyDiffReport (class)   +6/-0   Test Docs Improve Similar   HeaderDisplay (class)   +4/-0   Test Docs Improve Similar   ResultDisplay (class)   +6/-0   Test Docs Improve Similar   VSASummaryDisplay (class)   +6/-0   Test Docs Improve Similar   PolicyDiffDisplay (class)   +6/-0   Test Docs Improve Similar   shortenImageDigest (function)   +20/-0   Test Docs Improve Similar   extractFallbackReason (function)   +16/-0   Test Docs Improve Similar   extractReasonFromMessage (function)   +18/-0   Test Docs Improve Similar   parsePolicyDiffFromMessage (function)   +37/-0   Test Docs Improve Similar   processVSAResult (function)   +38/-0   Test Docs Improve Similar   aggregateAllSectionsData (function)   +58/-0   Test Docs Improve Similar   buildHeaderDisplay (function)   +6/-0   Test Docs Improve Similar   String (method of HeaderDisplay)   +3/-0   Test Docs Improve Similar   buildResultDisplay (function)   +33/-0

[qodo-code-review]

Generated tests for 'validateImageCmd' ✏️️

validateImageCmd (function) ^[+36/-74]



Component signature:

func validateImageCmd(validate imageValidationFunc) *cobra.Command


Tests for code changes in validateImageCmd function:

[edge case] Workers should emit validate_utils.Result produced via PopulateResultFromOutput; when validation is skipped due to a valid VSA (out == nil, err == nil), no panic occurs and the result is still collected properly test_code: package validate_test import ( "context" "errors" "testing" "time" "github.com/spf13/cobra" app "github.com/enterprise-contract/ec-cli/application/v1alpha1" "github.com/enterprise-contract/ec-cli/cmd/validate" "github.com/enterprise-contract/ec-cli/internal/applicationsnapshot" "github.com/enterprise-contract/ec-cli/internal/validate_utils" ) // Given a worker-based RunE pipeline that now uses validate_utils.Result via PopulateResultFromOutput // When validate returns (out == nil, err == nil) to simulate a VSA-skip, and another component returns a normal non-nil out // Then no panic occurs, and both results are collected into allResults, allowing downstream aggregation without error. func TestWorkerUsesPopulateResultAndHandlesVSASkip(t *testing.T) { // Build a minimal command cmd := &cobra.Command{} ctx := context.Background() cmd.SetContext(ctx) // Prepare components comp1 := app.SnapshotComponent{Name: "c1", ContainerImage: "registry.io/foo@sha256:111"} comp2 := app.SnapshotComponent{Name: "c2", ContainerImage: "registry.io/bar@sha256:222"} // Simulate channels like in validateImageCmd numComponents := 2 jobs := make(chan app.SnapshotComponent, numComponents) results := make(chan validate_utils.Result, numComponents) // Minimal worker mirroring the new signature and PopulateResultFromOutput usage. worker := func(id int, jobs <-chan app.SnapshotComponent, results chan<- validate_utils.Result) { for comp := range jobs { // Case switch by image to simulate behaviors: var out *fakeOutput var err error switch comp.ContainerImage { case "registry.io/foo@sha256:111": // VSA skip case: out == nil, err == nil out = nil err = nil case "registry.io/bar@sha256:222": // Normal validation: out != nil, err == nil out = &fakeOutput{ imageURL: comp.ContainerImage, violations: nil, warnings: nil, successes: []string{"ok"}, policyIn: []byte(`{}`), } err = nil default: err = errors.New("unexpected") } // Use the same helper as in PR res := validate_utils.PopulateResultFromOutput(out, err, comp, true, []string{}) results <- res } } // Start a single worker go worker(0, jobs, results) // Enqueue jobs and close jobs <- comp1 jobs <- comp2 close(jobs) // Collect results var allResults []validate_utils.Result for i := 0; i < numComponents; i++ { res := <-results allResults = append(allResults, res) } close(results) // Then: both results should exist, with no panic. if len(allResults) != 2 { t.Fatalf("expected 2 results, got %d", len(allResults)) } // The VSA-skip result must have Err == nil and a Component set for the skipped comp var foundSkip bool var foundNormal bool for _, r := range allResults { switch r.Component.ContainerImage { case "registry.io/foo@sha256:111": foundSkip = true if r.Err != nil { t.Errorf("expected nil error on VSA skip, got %v", r.Err) } case "registry.io/bar@sha256:222": foundNormal = true if r.Err != nil { t.Errorf("expected nil error on normal validation, got %v", r.Err) } } } if !foundSkip || !foundNormal { t.Fatalf("did not collect expected results; skip=%v normal=%v", foundSkip, foundNormal) } } // Minimal fakeOutput to satisfy the PopulateResultFromOutput expectations in this test. // In production code, Output is from internal/output; here we define the subset we need. type fakeOutput struct { imageURL string violations []applicationsnapshot.Result warnings []applicationsnapshot.Result successes []string policyIn []byte } func (f *fakeOutput) Violations() []applicationsnapshot.Result { return f.violations } func (f *fakeOutput) Warnings() []applicationsnapshot.Result { return f.warnings } func (f *fakeOutput) Successes() []string { return f.successes } func (f *fakeOutput) PolicyInput() []byte { return f.policyIn } // Unused members in this focused test // Note: Adjust or extend if PopulateResultFromOutput requires more fields in your project.

[edge case] Results collection now delegates to validate_utils.CollectComponentResults; when a worker returns an error in validate_utils.Result.Err, the collector returns a wrapped error matching the provided formatter test_code: package validate_test import ( "errors" "fmt" "testing" "github.com/enterprise-contract/ec-cli/internal/applicationsnapshot" "github.com/enterprise-contract/ec-cli/internal/validate_utils" ) // Given a set of validate_utils.Result values containing one with Err != nil // When CollectComponentResults is invoked with an error formatter identical to the PR's usage // Then it should return a non-nil error, wrapping the component name and image per the message format. func TestCollectComponentResultsWrapsWorkerErrors(t *testing.T) { results := []validate_utils.Result{ { Component: applicationsnapshot.Component{ SnapshotComponent: applicationsnapshot.SnapshotComponent{ Name: "comp-ok", ContainerImage: "registry.io/ok@sha256:aaa", }, Success: true, }, PolicyInput: []byte(`{}`), Err: nil, }, { Component: applicationsnapshot.Component{ SnapshotComponent: applicationsnapshot.SnapshotComponent{ Name: "comp-bad", ContainerImage: "registry.io/bad@sha256:bbb", }, Success: false, }, Err: errors.New("boom"), }, } formatErr := func(r validate_utils.Result) error { return fmt.Errorf("error validating image %s of component %s: %w", r.Component.ContainerImage, r.Component.Name, r.Err) } components, manyPolicyInput, err := validate_utils.CollectComponentResults(results, formatErr) if err == nil { t.Fatalf("expected an error to be returned from CollectComponentResults, got nil") } // Ensure the error message wraps both image and component identifiers msg := err.Error() if want := "registry.io/bad@sha256:bbb"; !contains(msg, want) { t.Errorf("expected error to contain image %q, got: %s", want, msg) } if want := "comp-bad"; !contains(msg, want) { t.Errorf("expected error to contain component %q, got: %s", want, msg) } // Successful components should still be omitted from outputs because an error occurred if components != nil || manyPolicyInput != nil { t.Errorf("expected no components or policy inputs on error, got %#v %#v", components, manyPolicyInput) } } func contains(s, sub string) bool { return strings.Contains(s, sub) }

[happy path] Report writing now goes through validate_utils.WriteReport using ReportData and ReportOutputOptions; when --output-file is provided, the output format list should be augmented, and WriteReport should receive the augmented output list test_code: package validate_test import ( "bytes" "context" "path/filepath" "testing" "github.com/spf13/cobra" "github.com/enterprise-contract/ec-cli/internal/applicationsnapshot" "github.com/enterprise-contract/ec-cli/internal/validate_utils" ) // Given a command with --output-file set // When the code augments outputs and calls validate_utils.WriteReport // Then the augmented outputs should include json=<path> and be used in WriteReport func TestWriteReportReceivesAugmentedOutput(t *testing.T) { // Arrange a minimal ReportData similar to what validateImageCmd prepares reportData := validate_utils.ReportData{ Snapshot: "mysnap", Components: []applicationsnapshot.Component{ {SnapshotComponent: applicationsnapshot.SnapshotComponent{Name: "c1", ContainerImage: "img@sha256:abc"}, Success: true}, }, PolicyInputs: [][]byte{[]byte(`{}`)}, ShowSuccesses: true, ShowWarnings: false, } // Simulate the behavior of adding an output-file option outputFile := filepath.Join(t.TempDir(), "out.json") output := []string{} // start empty as in default // Code under test path: append "json=<path>" when outputFile provided output = append(output, "json="+outputFile) opts := validate_utils.ReportOutputOptions{ Output: output, NoColor: true, ForceColor: false, } // Run WriteReport with a command context cmd := &cobra.Command{} cmd.SetContext(context.Background()) // Capture output var buf bytes.Buffer cmd.SetOut(&buf) report, err := validate_utils.WriteReport(reportData, opts, cmd) if err != nil { t.Fatalf("WriteReport failed: %v", err) } // Then: report exists and the output configuration contains the augmented item if report == nil { t.Fatalf("expected non-nil report") } found := false for _, o := range opts.Output { if o == "json="+outputFile { found = true break } } if !found { t.Fatalf("expected augmented output 'json=%s' to be present", outputFile) } }

[robnester-rh]

/review

[qodo-code-review]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Possible Issue In processComponentsInParallel, results channel is now closed via a goroutine waiting on a WaitGroup while the collector expects exactly numComponents results; if any worker exits early on ctx.Done() without sending a result, the collector can return "results channel closed prematurely" or block. Verify all code paths in worker ensure one send per job or adjust collection to range over results until wg completion with a count. var allResults []vsa.ComponentResult for i := 0; i < numComponents; i++ { select { case result, ok := <-results: if !ok { // Channel closed, but we haven't received all results // This shouldn't happen normally, but handle gracefully return allResults, fmt.Errorf("results channel closed prematurely (received %d/%d results)", len(allResults), numComponents) } allResults = append(allResults, result) case <-ctx.Done(): // Timeout during collection - return with partial results // Don't wait for workers as they may be blocked trying to send to a full buffer // The context cancellation will signal workers to stop processing return nil, fmt.Errorf("parallel processing timeout after %v: %w", DefaultTimeoutDuration, ctx.Err()) } } // Sort results by component name for consistent display sort.Slice(allResults, func(i, j int) bool { return allResults[i].ComponentName < allResults[j].ComponentName }) return allResults, nil } API Change Risk validateImageFallbackWithWorkerContext signature changed to accept a SnapshotComponent and relies on data.snapshot being set; ensure all call sites (including tests and any external consumers) are updated and that data.snapshot is always initialized via DetermineInputSpec before any fallback path. // This ensures thread safety while reusing evaluators within the worker func validateImageFallbackWithWorkerContext(ctx context.Context, data *validateVSAData, comp app.SnapshotComponent, workerFallbackContext *vsa.WorkerFallbackContext) (*output.Output, error) { log.Debugf("🔄 Starting fallback validation for image: %s", comp.ContainerImage) log.Debugf("🔄 Fallback: Using component name: %s", comp.Name) // Use precomputed fallback context and worker-specific evaluators if data.fallbackContext == nil { return nil, fmt.Errorf("fallback context not initialized - this should not happen") } if workerFallbackContext == nil { return nil, fmt.Errorf("worker fallback context not initialized - this should not happen") } if data.snapshot == nil { return nil, fmt.Errorf("snapshot spec not available - this should not happen") } log.Debugf("🔄 Fallback: Using precomputed policy configuration: %s", data.fallbackContext.PolicyConfiguration) log.Debugf("🔄 Fallback: Using worker evaluators: %d", len(workerFallbackContext.Evaluators)) // Perform image validation using precomputed context and worker-specific evaluators log.Debugf("🔄 Fallback: Starting image validation...") log.Debugf("🔄 Fallback: Using fallback policy: %s", data.fallbackContext.FallbackPolicy) result, err := image.ValidateImage(ctx, comp, data.snapshot, data.fallbackContext.FallbackPolicy, workerFallbackContext.Evaluators, data.info) if err != nil { log.Debugf("🔄 Fallback: Image validation failed with error: %v", err) return nil, err } return result, nil } Symlink Walking prepareDataDirs switched from afero.Walk to fs.WalkDir using a wrapper; confirm all afero filesystems used implement Open with symlink semantics expected on target platforms, and that permission errors while walking dataSourceDirs are handled as intended (currently continued silently per-dir). return results, nil } // prepareDataDirs inspects the data source directories and base data dir to return a list of // directories that contain data files. That list will be passed to the conftest runner. // dataSourceDirs contains the directories returned by GetPolicy for data sources. // These directories may be symlinks (from cached downloads), but we walk them directly // which ensures we find the files regardless of whether they're symlinks or not. func (c conftestEvaluator) prepareDataDirs(ctx context.Context, dataSourceDirs []string) ([]string, error) { // The reason we do this is to avoid having the names of the subdirs under c.dataDir // converted to keys in the data structure. We want the top level keys in the data files // to be at the top level of the data structure visible to the rego rules. dirsWithDataFiles := make(map[string]bool) // Walk each data source directory returned by GetPolicy // These are the actual directories (possibly symlinks) where data was downloaded // Walking them directly ensures we find files even if they're symlinks for _, dataSourceDir := range dataSourceDirs { // IMPORTANT: Use fs.WalkDir instead of afero.Walk because afero.Walk does not follow symlinks. // When cached downloads create symlinks (as in getPolicyThroughCache), afero.Walk won't traverse // into the symlinked directories, causing data files to be missed. // See afero issue https://github.com/spf13/afero/issues/284 and internal/opa/inspect.go for reference. err := fs.WalkDir(opaWrapperFs{afs: c.fs}, dataSourceDir, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } // Only process files, not directories if !d.IsDir() { ext := filepath.Ext(d.Name()) // Check if this is a data file (.json, .yaml, .yml) // Todo: Should probably recognize other supported types of data if ext == ".json" || ext == ".yaml" || ext == ".yml" { // Mark the directory containing this file as having data dir := filepath.Dir(path) dirsWithDataFiles[dir] = true } } return nil }) if err != nil { // Continue with other directories even if one fails continue } } // Also walk the base data directory to find config.json and any other files // This ensures we don't miss files that weren't from GetPolicy sources err := fs.WalkDir(opaWrapperFs{afs: c.fs}, c.dataDir, func(path string, d fs.DirEntry, err error) error { if err != nil { return err } // Skip the root data directory itself if path == c.dataDir { return nil } // Only process files, not directories if !d.IsDir() { ext := filepath.Ext(d.Name()) // Check if this is a data file (.json, .yaml, .yml) if ext == ".json" || ext == ".yaml" || ext == ".yml" { // Mark the directory containing this file as having data dir := filepath.Dir(path)

[st3penta]

As far as i can tell the PR seems fine, but tbh I don't feel like I understood it well, due to its size and to the fact that is a refactoring (harder to understand with respect to 'new stuff' PRs)

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Limited logging: New critical paths (parallel workers, fallback execution, timeout handling, multi-format report writing) add minimal debug logs and info prints but do not consistently log user/action context or outcomes for all branches. Referred Code printVSAInfo(os.Stdout, fmt.Sprintf("=== Processing Components in Parallel (%d workers) ===", numWorkers)) // Add timeout for parallel processing to prevent hanging ctx, cancel := context.WithTimeout(ctx, DefaultTimeoutDuration) defer cancel() // Set up parallel processing infrastructure jobs := make(chan app.SnapshotComponent, numComponents) results := make(chan vsa.ComponentResult, numComponents) // Use WaitGroup to track worker completion var wg sync.WaitGroup // Start worker goroutines for i := 0; i < numWorkers; i++ { wg.Add(1) go func() { defer wg.Done() worker(jobs, results, ctx, data) }() } ... (clipped 38 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Detailed errors: Several returned errors include internal details (e.g., policy parsing, worker context, timeouts) that may be user-facing depending on CLI surfaces and could reveal implementation specifics. Referred Code data.retriever = retriever // Process validation // Normalize input to SnapshotSpec using DetermineInputSpec (same as validate image) // This unifies --images and single identifier handling identifier := extractVSAIdentifier(data, args) // Use DetermineInputSpec to normalize input (supports both --images and single identifier) snapshot, _, err := applicationsnapshot.DetermineInputSpec(ctx, applicationsnapshot.Input{ Image: identifier, // Single VSA identifier Images: data.images, // Snapshot file/JSON string }) if err != nil { return fmt.Errorf("failed to parse input: %w", err) } if len(snapshot.Components) == 0 { return fmt.Errorf("no components found in input") } // Print appropriate message based on input type ... (clipped 9 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Input normalization: Input spec normalization via DetermineInputSpec is used but explicit validation of output destinations from --output (e.g., write paths) and handling of symlinked sources in parallel workers relies on external utils and may need confirmation for safe path handling. Referred Code // toFormat converts VSASectionsReport or ComponentResultsDisplay to the specified format // For text format, uses ComponentResultsDisplay to leverage existing String() methods func (d ComponentResultsDisplay) toFormat(format string) ([]byte, error) { switch format { case "json": report := d.toVSASectionsReport() data, err := json.MarshalIndent(&report, "", " ") if err != nil { return nil, err } return data, nil case "yaml": report := d.toVSASectionsReport() data, err := yaml.Marshal(&report) if err != nil { return nil, err } return data, nil case "text": return d.toText(), nil default: ... (clipped 62 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label