pkg/config/config.go (1)

425-427: Consider adding path validation for AdditionalArtifactStores.

The new AdditionalArtifactStores field lacks validation in the RuntimeConfig.Validate() method. Similar configuration fields like HooksDir (lines 1364-1385) include checks to ensure paths are valid directories. Consider adding validation to:

• Check if paths are absolute
• Verify paths exist or can be created
• Log warnings for invalid paths rather than failing startup (to match the behavior in internal/ociartifact/store.go which ignores invalid stores)

[scratchpad_end] -->

♻️ Example validation pattern

You could add validation in the RuntimeConfig.Validate() method similar to the HooksDir pattern:

// Validate additional artifact stores
for _, storePath := range c.AdditionalArtifactStores {
	if !filepath.IsAbs(storePath) {
		logrus.Warnf("Additional artifact store path is not absolute: %s", storePath)
		continue
	}
	if err := os.MkdirAll(storePath, 0o755); err != nil {
		logrus.Warnf("Cannot create additional artifact store directory %s: %v", storePath, err)
	}
}

Note: Since internal/ociartifact/store.go already silently ignores invalid stores, this validation would be informational and help users catch configuration errors early.

internal/storage/image.go (1)

904-904: Pass AdditionalArtifactStores to the artifact fallback path for consistency.

The OCI artifact store created in the fallback path (line 904) uses nil for additional stores, while the main image pull path in server/server.go:461 explicitly passes config.AdditionalArtifactStores. To ensure consistent artifact resolution across all code paths, consider threading the additional artifact stores through ImageCopyOptions to pullImageImplementation. This would allow the fallback artifact retrieval to check additional read-only stores, matching behavior with the primary pull path.

internal/ociartifact/store.go (4)

57-69: Consider logging errors when ignoring invalid additional stores.

While silently ignoring invalid stores prevents CRI-O startup failures, users may not realize their configured additional stores are being skipped. Adding a warning log would improve observability.

📋 Proposed improvement

 	for _, path := range additionalPaths {
 		// Assume the pattern is to have an "artifacts" directory within the configured path,
 		// similar to how additionalimagestores works in containers/storage.
 		addPath := filepath.Join(path, "artifacts")
 
 		addStore, err := libartStore.NewArtifactStore(addPath, systemContext)
 		if err != nil {
-			// Just ignore invalid stores to prevent breaking CRI-O startup
+			// Just ignore invalid stores to prevent breaking CRI-O startup
+			log.Warnf(context.Background(), "Ignoring invalid additional artifact store %q: %v", path, err)
 			continue
 		}
 		additionalStores = append(additionalStores, RealLibartifactStore{addStore})
 		validAdditionalPaths = append(validAdditionalPaths, addPath)
 	}

As per coding guidelines, use logrus with structured fields for logging.

---

110-110: Use structured logging fields instead of string formatting.

The logging statement uses string formatting, but coding guidelines recommend using logrus with structured fields.

📋 Proposed improvement

-	log.Infof(ctx, "Checking/Pulling OCI artifact from ref: %s", ref)
+	log.WithFields(log.Fields{
+		"ref": ref,
+	}).Infof(ctx, "Checking/Pulling OCI artifact")

As per coding guidelines, use logrus with structured fields for logging.

---

166-172: Consider logging errors when listing additional stores.

Errors from additional stores are silently ignored, which could hide issues like permission problems or corrupted stores. Adding warning logs would improve observability without breaking functionality.

📋 Proposed improvement

 	for _, addStore := range s.additionalStores {
 		addArts, err := addStore.List(ctx)
 		if err != nil {
+			log.Warnf(ctx, "Failed to list artifacts from additional store: %v", err)
 			continue
 		}
 		arts = append(arts, addArts...)
 	}

As per coding guidelines, use logrus with structured fields for logging.

---

473-488: Consider logging errors during path resolution.

The resolveRootPath function silently ignores errors from List() calls (lines 475, 481). While this might be intentional for robustness with read-only stores, logging these errors would help diagnose configuration or permission issues.

📋 Proposed improvement

 func (s *Store) resolveRootPath(ctx context.Context, nameOrDigest string) (string, error) {
-	arts, _ := s.LibartifactStore.List(ctx)
+	arts, err := s.LibartifactStore.List(ctx)
+	if err != nil {
+		log.Debugf(ctx, "Failed to list artifacts from main store during path resolution: %v", err)
+	}
 	if s.containsArtifact(arts, nameOrDigest) {
 		return s.rootPath, nil
 	}
 
 	for i, store := range s.additionalStores {
-		arts, _ := store.List(ctx)
+		arts, err := store.List(ctx)
+		if err != nil {
+			log.Debugf(ctx, "Failed to list artifacts from additional store during path resolution: %v", err)
+			continue
+		}
 		if s.containsArtifact(arts, nameOrDigest) {
 			return s.additionalPaths[i], nil
 		}
 	}
 
 	return "", fmt.Errorf("artifact %s not found in any store", nameOrDigest)
 }

As per coding guidelines, use logrus with structured fields for logging.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: cri-o/cri-o PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-03T18:27:19.593Z
Learning: When adding/changing features, update related repositories: cri-o.io website and packaging repositories

Learnt from: bitoku
Repo: cri-o/cri-o PR: 9667
File: server/container_create.go:1233-1236
Timestamp: 2025-12-17T13:38:34.646Z
Learning: In the cri-o/cri-o repository, protobuf-generated Get* methods for k8s.io/cri-api types are nil-safe: if the receiver is nil, GetX() returns the zero value instead of panicking. Do not add explicit nil checks before chaining calls on such getters. Apply this guidance to all Go code that uses these generated getters across the codebase.

Learnt from: bitoku
Repo: cri-o/cri-o PR: 9676
File: internal/lib/stats/cgroup_stats_unsupported.go:1-7
Timestamp: 2025-12-18T13:28:24.244Z
Learning: In the cri-o/cri-o repository, for platform-specific types guarded by Go build tags (for example //go:build !linux), implement empty structs for unsupported platforms to permit compilation and clearly indicate the feature is not available rather than mirroring the Linux struct with unpopulated fields. Apply this pattern to all relevant platform-specific files across the codebase (i.e., any file under build-taged sections that should compile on all targets but lacks full implementation for some platforms).

internal/factory/container/device_test.go (1)

299-303: Consider using octal notation for file mode clarity.

os.FileMode(432) is correct but opaque. Using os.FileMode(0o660) makes the intent (owner+group read-write) immediately obvious to reviewers.

🔧 Suggested change

-						FileMode: func() *os.FileMode { m := os.FileMode(432); return &m }(),
+						FileMode: func() *os.FileMode { m := os.FileMode(0o660); return &m }(),

Apply similarly at lines 310, 390, and 397.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/factory/container/device_test.go` around lines 299 - 303, Replace
opaque decimal filemode literals with octal notation for clarity: change
instances like FileMode: func() *os.FileMode { m := os.FileMode(432); return &m
}() to FileMode: func() *os.FileMode { m := os.FileMode(0o660); return &m }(),
and apply the same replacement for the other FileMode occurrences in the test
(the FileMode struct fields shown in the device test cases) so each uses the
equivalent 0o... octal literal instead of a decimal number.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro