refactor: code review fixes — dead code, error wrapping, deduplication

pkg/bundler/checksum/checksum.go

@@ -49,19 +49,18 @@ func GenerateChecksums(ctx context.Context, bundleDir string, files []string) er
 	checksums := make([]string, 0, len(files))
 
 	for _, file := range files {
-		data, err := os.ReadFile(file)
+		digest, err := SHA256Raw(file)
 		if err != nil {
-			return errors.Wrap(errors.ErrCodeInternal, fmt.Sprintf("failed to read %s for checksum", file), err)
+			return errors.Wrap(errors.ErrCodeInternal, fmt.Sprintf("failed to compute checksum for %s", file), err)
 		}
 
-		hash := sha256.Sum256(data)
 		relPath, err := filepath.Rel(bundleDir, file)
 		if err != nil {
 			// If relative path fails, use absolute path
 			relPath = file
 		}
 
-		checksums = append(checksums, fmt.Sprintf("%s  %s", hex.EncodeToString(hash[:]), relPath))
+		checksums = append(checksums, fmt.Sprintf("%s  %s", hex.EncodeToString(digest), relPath))
 	}
 
 	checksumPath := filepath.Join(bundleDir, ChecksumFileName)
@@ -135,14 +134,13 @@ func VerifyChecksumsFromData(bundleDir string, data []byte) []string {
 			continue
 		}
 
-		fileData, readErr := os.ReadFile(filePath)
+		digest, readErr := SHA256Raw(filePath)
 		if readErr != nil {
 			errs = append(errs, fmt.Sprintf("failed to read %s: %v", relativePath, readErr))
 			continue
 		}
 
-		hash := sha256.Sum256(fileData)
-		actualDigest := hex.EncodeToString(hash[:])
+		actualDigest := hex.EncodeToString(digest)
 		if actualDigest != expectedDigest {
 			errs = append(errs, fmt.Sprintf("checksum mismatch: %s (expected %s, got %s)", relativePath, expectedDigest, actualDigest))
 		}

pkg/bundler/handler.go

@@ -239,11 +239,10 @@ func streamZipResponse(w http.ResponseWriter, dir string, output *result.Output)
 		if err != nil {
 			return aicrerrors.Wrap(aicrerrors.ErrCodeInternal, "failed to open file", err)
 		}
-		defer file.Close()
-
-		_, err = io.Copy(writer, file)
-		if err != nil {
-			return aicrerrors.Wrap(aicrerrors.ErrCodeInternal, "failed to copy file content", err)
+		_, copyErr := io.Copy(writer, file)
+		file.Close()
+		if copyErr != nil {
+			return aicrerrors.Wrap(aicrerrors.ErrCodeInternal, "failed to copy file content", copyErr)
 		}
 
 		return nil

pkg/bundler/validations/checks.go

@@ -152,8 +152,8 @@ func checkConditions(recipeResult *recipe.RecipeResult, conditions map[string][]
 		// Check if actualValue matches any of the expected values (OR matching)
 		found := false
 		for _, expectedStr := range expectedValues {
-			// Use matchesCriteriaField for consistent matching logic
-			if matchesCriteriaField(actualValue, expectedStr) {
+			// Use recipe.MatchesCriteriaField for consistent matching logic
+			if recipe.MatchesCriteriaField(actualValue, expectedStr) {
 				found = true
 				break
 			}
@@ -165,20 +165,3 @@ func checkConditions(recipeResult *recipe.RecipeResult, conditions map[string][]
 
 	return true
 }
-
-// matchesCriteriaField implements matching for a single criteria field.
-// Reuses the logic from recipe/criteria.go for consistency.
-// Returns true if the actual value matches the expected value.
-// For validation conditions, we use simple equality matching (not asymmetric like recipe matching).
-func matchesCriteriaField(actualValue, expectedValue string) bool {
-	// Treat empty as "any" for consistency with criteria matching
-	expectedIsAny := expectedValue == "" || expectedValue == "any"
-
-	// If expected is "any", it matches any actual value
-	if expectedIsAny {
-		return true
-	}
-
-	// Expected has a specific value - actual must match exactly
-	return actualValue == expectedValue
-}

pkg/collector/file/file.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 	"unicode/utf8"
 
+	"github.com/NVIDIA/aicr/pkg/defaults"
 	"github.com/NVIDIA/aicr/pkg/errors"
 )
 
@@ -99,7 +100,7 @@ func WithSkipEmptyValues(skip bool) Option {
 func NewParser(opts ...Option) *Parser {
 	p := &Parser{
 		delimiter:       "\n",
-		maxSize:         1 << 20, // 1MB default
+		maxSize:         defaults.FileParserMaxSize,
 		skipComments:    true,
 		kvDelimiter:     "=",
 		vDefault:        "",

pkg/collector/gpu/gpu.go

@@ -64,7 +64,7 @@ func (s *Collector) Collect(ctx context.Context) (*measurement.Measurement, erro
 
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "GPU collection cancelled", err)
 	}
 
 	data, err := executeCommand(ctx, nvidiaSMICommand, "-q", "-x")

pkg/collector/k8s/image.go

@@ -51,7 +51,7 @@ func (k *Collector) collectContainerImages(ctx context.Context) (map[string]meas
 	for _, pod := range pods.Items {
 		// Check for context cancellation
 		if err := ctx.Err(); err != nil {
-			return nil, err
+			return nil, errors.Wrap(errors.ErrCodeTimeout, "image collection cancelled", err)
 		}
 
 		for _, container := range pod.Spec.Containers {

pkg/collector/k8s/node.go

@@ -29,7 +29,7 @@ import (
 func (k *Collector) collectNode(ctx context.Context) (map[string]measurement.Reading, error) {
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "node collection cancelled", err)
 	}
 
 	// Get the current node name from environment
@@ -99,10 +99,6 @@ func parseProvider(providerID string) string {
 
 	// Split by "://" to get the provider prefix
 	parts := strings.SplitN(providerID, "://", 2)
-	if len(parts) < 1 {
-		slog.Warn("invalid providerID format", slog.String("providerID", providerID))
-		return ""
-	}
 
 	// Normalize provider names
 	provider := strings.ToLower(strings.TrimSpace(parts[0]))

pkg/collector/k8s/policy.go

@@ -57,7 +57,7 @@ func (k *Collector) collectClusterPolicies(ctx context.Context) (map[string]meas
 	// Find all ClusterPolicy resources across all API groups
 	for _, apiResourceList := range apiResourceLists {
 		if err := ctx.Err(); err != nil {
-			return nil, err
+			return nil, errors.Wrap(errors.ErrCodeTimeout, "policy collection cancelled", err)
 		}
 
 		if apiResourceList == nil {
@@ -105,7 +105,7 @@ func (k *Collector) collectClusterPolicies(ctx context.Context) (map[string]meas
 			for _, policy := range policies.Items {
 				// Check for context cancellation
 				if err := ctx.Err(); err != nil {
-					return nil, err
+					return nil, errors.Wrap(errors.ErrCodeTimeout, "policy collection cancelled", err)
 				}
 
 				// Extract spec for detailed information

pkg/collector/k8s/server.go

@@ -27,7 +27,7 @@ import (
 func (k *Collector) collectServer(ctx context.Context) (map[string]measurement.Reading, error) {
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "server collection cancelled", err)
 	}
 
 	// Server Version

pkg/collector/os/grub.go

@@ -39,7 +39,7 @@ var (
 func (c *Collector) collectGRUB(ctx context.Context) (*measurement.Subtype, error) {
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "grub collection cancelled", err)
 	}
 
 	parser := file.NewParser(

pkg/collector/os/kmod.go

@@ -33,7 +33,7 @@ var (
 func (c *Collector) collectKMod(ctx context.Context) (*measurement.Subtype, error) {
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "kmod collection cancelled", err)
 	}
 
 	parser := file.NewParser()

pkg/collector/os/release.go

@@ -42,7 +42,7 @@ var (
 func (c *Collector) collectRelease(ctx context.Context) (*measurement.Subtype, error) {
 	// Check if context is canceled
 	if err := ctx.Err(); err != nil {
-		return nil, err
+		return nil, errors.Wrap(errors.ErrCodeTimeout, "release collection cancelled", err)
 	}
 
 	// Try primary location first, fall back to alternative per freedesktop.org spec

pkg/collector/os/sysctl.go

@@ -50,8 +50,8 @@ func (c *Collector) collectSysctl(ctx context.Context) (*measurement.Subtype, er
 		}
 
 		// Check if context is canceled
-		if ctx.Err() != nil {
-			return ctx.Err()
+		if ctxErr := ctx.Err(); ctxErr != nil {
+			return errors.Wrap(errors.ErrCodeTimeout, "sysctl collection cancelled", ctxErr)
 		}
 
 		// Skip symlinks to prevent directory traversal attacks
@@ -111,7 +111,7 @@ func (c *Collector) collectSysctl(ctx context.Context) (*measurement.Subtype, er
 // parseMultiLineKeyValue attempts to parse lines as space-separated key-value pairs.
 // Returns true if all non-empty lines were successfully parsed as key-value pairs.
 func (c *Collector) parseMultiLineKeyValue(path string, lines []string, params map[string]measurement.Reading) bool {
-	allParsed := true
+	tmp := make(map[string]measurement.Reading)
 
 	for _, line := range lines {
 		if line == "" {
@@ -125,13 +125,16 @@ func (c *Collector) parseMultiLineKeyValue(path string, lines []string, params m
 			key := parts[0]
 			value := strings.Join(parts[1:], " ")
 			extendedPath := path + "/" + key
-			params[extendedPath] = measurement.Str(value)
+			tmp[extendedPath] = measurement.Str(value)
 		} else {
 			// Not a key-value pair format
-			allParsed = false
-			break
+			return false
 		}
 	}
 
-	return allParsed
+	for k, v := range tmp {
+		params[k] = v
+	}
+
+	return true
 }

pkg/component/base.go

@@ -237,10 +237,10 @@ func GetRecipeBundlerVersion(m map[string]string) string {
 	return "unknown"
 }
 
-// BuildBaseConfigMap creates a configuration map with common bundler settings.
+// buildBaseConfigMap creates a configuration map with common bundler settings.
 // Returns a map containing bundler version.
 // Bundlers can extend this map with their specific values.
-func (b *BaseBundler) BuildBaseConfigMap() map[string]string {
+func (b *BaseBundler) buildBaseConfigMap() map[string]string {
 	config := make(map[string]string)
 
 	config[bundlerVersionKey] = b.Config.Version()
@@ -252,7 +252,7 @@ func (b *BaseBundler) BuildBaseConfigMap() map[string]string {
 // This includes base config from bundler settings plus recipe version.
 // Use this when working with RecipeResult (new format) instead of Recipe.
 func (b *BaseBundler) BuildConfigMapFromInput(input interface{ GetVersion() string }) map[string]string {
-	config := b.BuildBaseConfigMap()
+	config := b.buildBaseConfigMap()
 
 	// Add recipe version if available
 	if version := input.GetVersion(); version != "" {

pkg/component/base_test.go

@@ -449,13 +449,13 @@ func TestBaseBundler_AddError(t *testing.T) {
 	}
 }
 
-func TestBaseBundler_BuildBaseConfigMap(t *testing.T) {
+func TestBaseBundler_buildBaseConfigMap(t *testing.T) {
 	cfg := config.NewConfig(
 		config.WithVersion("v1.2.3"),
 	)
 
 	b := NewBaseBundler(cfg, types.BundleType("gpu-operator"))
-	configMap := b.BuildBaseConfigMap()
+	configMap := b.buildBaseConfigMap()
 
 	// Test bundler version is set
 	if configMap["bundler_version"] != "v1.2.3" {

pkg/component/helpers.go

@@ -26,8 +26,8 @@ import (
 	"github.com/NVIDIA/aicr/pkg/errors"
 )
 
-// ComputeChecksum computes the SHA256 checksum of the given content.
-func ComputeChecksum(content []byte) string {
+// computeChecksum computes the SHA256 checksum of the given content.
+func computeChecksum(content []byte) string {
 	hash := sha256.Sum256(content)
 	return hex.EncodeToString(hash[:])
 }
@@ -83,8 +83,8 @@ func GetConfigValue(config map[string]string, key, defaultValue string) string {
 	return defaultValue
 }
 
-// ExtractCustomLabels extracts custom labels from config map with "label_" prefix.
-func ExtractCustomLabels(config map[string]string) map[string]string {
+// extractCustomLabels extracts custom labels from config map with "label_" prefix.
+func extractCustomLabels(config map[string]string) map[string]string {
 	labels := make(map[string]string)
 	for k, v := range config {
 		if len(k) > 6 && k[:6] == "label_" {
@@ -94,8 +94,8 @@ func ExtractCustomLabels(config map[string]string) map[string]string {
 	return labels
 }
 
-// ExtractCustomAnnotations extracts custom annotations from config map with "annotation_" prefix.
-func ExtractCustomAnnotations(config map[string]string) map[string]string {
+// extractCustomAnnotations extracts custom annotations from config map with "annotation_" prefix.
+func extractCustomAnnotations(config map[string]string) map[string]string {
 	annotations := make(map[string]string)
 	for k, v := range config {
 		if len(k) > 11 && k[:11] == "annotation_" {
@@ -111,17 +111,8 @@ const (
 	StrFalse = "false"
 )
 
-// BoolToString converts a boolean to "true" or "false" string.
-// Use this for Helm values that require string booleans.
-func BoolToString(b bool) string {
-	if b {
-		return StrTrue
-	}
-	return StrFalse
-}
-
-// ParseBoolString parses a string boolean value.
+// parseBoolString parses a string boolean value.
 // Returns true if the value is "true" or "1", false otherwise.
-func ParseBoolString(s string) bool {
+func parseBoolString(s string) bool {
 	return s == StrTrue || s == "1"
 }