internal/ociartifact/impl.go (1)

34-42: Consider wrapping the error returned by GetManifest.

src.GetManifest errors propagate unwrapped. While the caller currently wraps them, adding a wrap here would provide better context if GetManifestFromRef is reused elsewhere.

Suggested improvement

-	return src.GetManifest(ctx, instanceDigest)
+	manifestBytes, mimeType, err := src.GetManifest(ctx, instanceDigest)
+	if err != nil {
+		return nil, "", fmt.Errorf("get manifest: %w", err)
+	}
+	return manifestBytes, mimeType, nil

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/impl.go` around lines 34 - 42, The GetManifestFromRef
implementation returns src.GetManifest errors unwrapped; update the return to
capture the result and error from src.GetManifest(ctx, instanceDigest) and, if
err != nil, return nil, "", fmt.Errorf("get manifest from source: %w", err) so
callers receive contextualized errors; reference the GetManifestFromRef function
and the src.GetManifest call when making the change.

internal/ociartifact/store.go (1)

148-152: Swallowing the OCI1 parse error is intentional but loses diagnostic info.

When manifest.OCI1FromManifest fails, the error is discarded and ErrIsAnImage is returned directly. This is correct behavior (Docker v2s2 manifests will fail here), but a debug log would help troubleshoot unexpected parse failures without changing the control flow.

Suggested improvement

 		ociManifest, err := manifest.OCI1FromManifest(manifestBytes)
 		// Unable to parse an OCI manifest, assume an image
 		if err != nil {
+			log.Debugf(ctx, "Cannot parse as OCI manifest, treating as container image: %v", err)
 			return ErrIsAnImage
 		}

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store.go` around lines 148 - 152, When
manifest.OCI1FromManifest(manifestBytes) fails we currently return ErrIsAnImage
and discard the original error; update the code to log the parse error at
debug/trace level before returning ErrIsAnImage so diagnostics are preserved.
Locate the call to manifest.OCI1FromManifest in internal/ociartifact/store.go
and add a debug log statement that includes the returned error (using the
existing logger in scope, e.g., s.logger, logger, or ctx logger) while keeping
the control flow intact (still return ErrIsAnImage). Ensure the log message
clearly states it was an OCI manifest parse failure and includes the error
value.

internal/ociartifact/store_test.go (1)

25-44: Consider adding a test case for DockerV2Schema1SignedMediaType.

The imageMimeTypes list includes DockerV2Schema1SignedMediaType, but there's no test exercising that path. A schema 1 manifest has a different structure and would exercise the OCI1FromManifest failure → ErrIsAnImage branch distinctly from the Docker v2s2 test.

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store_test.go` around lines 25 - 44, Add a new unit test
that covers the DockerV2Schema1SignedMediaType path: create a schema1-style
manifest bytes (either by extending makeOCIManifest or adding a small helper
that emits the schema1 structure), feed those bytes through the same code path
that uses imageMimeTypes and the manifest-to-OCI conversion (invoking
OCI1FromManifest via the same processing function used by existing tests), and
assert the code returns the ErrIsAnImage error (or the same failure branch)
rather than succeeding; reference makeOCIManifest,
DockerV2Schema1SignedMediaType, imageMimeTypes, OCI1FromManifest, and
ErrIsAnImage to locate where to add the test and assertions.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

internal/ociartifact/store.go (1)

131-135: Missing unsigned Docker v2 schema 1 media type (DockerV2Schema1MediaType).

imageMimeTypes includes DockerV2Schema1SignedMediaType but not DockerV2Schema1MediaType. If a registry returns the unsigned schema 1 variant (application/vnd.docker.distribution.manifest.v1+json), it will slip through detection and be treated as an artifact. Both legacy variants should be rejected.

Proposed fix

 	imageMimeTypes := []string{
 		specs.MediaTypeImageManifest,
 		manifest.DockerV2Schema2MediaType,
 		manifest.DockerV2Schema1SignedMediaType,
+		manifest.DockerV2Schema1MediaType,
 	}

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store.go` around lines 131 - 135, The imageMimeTypes
slice in internal/ociartifact/store.go currently lists
manifest.DockerV2Schema1SignedMediaType but omits the unsigned variant; update
the imageMimeTypes declaration to include manifest.DockerV2Schema1MediaType
alongside DockerV2Schema1SignedMediaType (so both legacy schema1 media types are
present) to ensure unsigned schema1 manifests are detected and rejected when
processing in the relevant code paths that reference imageMimeTypes.

internal/ociartifact/store.go (1)

148-153: Docker v2 schema 2 detection relies on OCI parse failure — consider adding an explicit comment.

When mimeType is DockerV2Schema2MediaType, the code enters this branch and calls manifest.OCI1FromManifest, which will fail because Docker v2 schema 2 is not a valid OCI manifest. The parse error path then correctly returns ErrIsAnImage. While this produces the right result, it's relying on a non-obvious side effect. A brief comment would prevent future maintainers from "fixing" the parse error handling and inadvertently breaking Docker schema 2 detection.

Suggested clarifying comment

 	if slices.Contains(imageMimeTypes, mimeType) && slices.Contains(configMediaTypes, mediaType) {
 		ociManifest, err := manifest.OCI1FromManifest(manifestBytes)
-		// Unable to parse an OCI manifest, assume an image
+		// Unable to parse as OCI manifest (e.g. Docker v2 schema 2), assume an image
 		if err != nil {
 			return ErrIsAnImage
 		}

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store.go` around lines 148 - 153, The branch that calls
manifest.OCI1FromManifest when mimeType and mediaType match (using
imageMimeTypes and configMediaTypes) intentionally treats a parse error as
Docker v2 schema 2 detection and returns ErrIsAnImage; add an explicit comment
above this block explaining that when mimeType == DockerV2Schema2MediaType
OCI1FromManifest is expected to fail for Docker v2 schema 2, that the error path
is used intentionally to classify the artifact, and that ErrIsAnImage is the
deliberate outcome so future maintainers won't remove or "fix" the parse-failure
logic.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

internal/ociartifact/store.go (2)

149-163: Logic gap: manifest.OCI1FromManifest error is silently promoted to ErrIsAnImage.

When manifest.OCI1FromManifest(manifestBytes) fails for a reason other than "not an OCI manifest" (e.g., malformed JSON, I/O error), the original error is silently discarded and ErrIsAnImage is returned. A caller receiving ErrIsAnImage has no way to distinguish a legitimate Docker schema 2 image from a transient parse failure, potentially blocking valid artifact pulls.

Consider logging the parse error at debug level before returning ErrIsAnImage to preserve observability without changing the callers' contract:

💡 Proposed improvement

-		ociManifest, err := manifest.OCI1FromManifest(manifestBytes)
-		// Unable to parse as OCI manifest (e.g. Docker v2 schema 2), assume an image
-		if err != nil {
-			return ErrIsAnImage
-		}
+		ociManifest, ociErr := manifest.OCI1FromManifest(manifestBytes)
+		// Unable to parse as OCI manifest (e.g. Docker v2 schema 2), assume an image.
+		if ociErr != nil {
+			log.Debugf(ctx, "Cannot parse as OCI manifest (assuming container image): %v", ociErr)
+			return ErrIsAnImage
+		}

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store.go` around lines 149 - 163, The call to
manifest.OCI1FromManifest(manifestBytes) currently swallows all parse errors and
returns ErrIsAnImage; update the error path so the original parse error is
logged (e.g., via log.Debugf or similar) before returning ErrIsAnImage to
preserve observability without changing the function contract—change the block
handling err from manifest.OCI1FromManifest to log the error context (including
err and maybe manifest MIME/mediatype) then return ErrIsAnImage, referencing
manifest.OCI1FromManifest, the err variable, ociManifest, and ErrIsAnImage to
locate the code.

---

63-84: Double manifest fetch on the pull path.

EnsureNotContainerImage opens 1–2 ImageSource connections to fetch the manifest(s), then libartifactStore.Pull internally opens its own connection and re-fetches the same manifest. For large deployments or slow registries this adds measurable latency on every artifact pull.

If the underlying library exposes a way to pass a pre-fetched manifest or if the pull path can be restructured to share the initial fetch, consider doing so. As-is, correctness is unaffected.

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/store.go` around lines 63 - 84, EnsureNotContainerImage
currently fetches manifests, then libartifactStore.Pull re-fetches them causing
double manifest fetch; modify EnsureNotContainerImage (or add a new variant
EnsureNotContainerImageWithManifest) to return the already-fetched manifest or
ImageSource/descriptor instead of discarding it, then add/use a matching
libartifactStore.Pull variant that accepts the pre-fetched manifest/ImageSource
(e.g., s.libartifactStore.PullWithManifest or Pull(ctx, artRef, opts, manifest))
and update the call in the pull path in this file to pass the returned manifest
to avoid a second fetch.

internal/ociartifact/impl.go (1)

34-42: src.Close() error silently discarded.

defer src.Close() drops the cleanup error. While benign for read-only manifest fetches (the manifest bytes are already copied), it can hide transport-level teardown failures in edge cases. Consider at least logging it at debug level to aid diagnostics.

💡 Optional improvement

-	defer src.Close()
+	defer func() {
+		if closeErr := src.Close(); closeErr != nil {
+			log.Debugf(ctx, "close image source: %v", closeErr)
+		}
+	}()

(This requires importing "github.com/cri-o/cri-o/internal/log" in this file, or you can simply ignore it if the project convention is to drop close errors from read-only sources.)

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/impl.go` around lines 34 - 42, GetManifestFromRef
currently defers src.Close() and discards any returned error; change the defer
to capture the Close() error and log it at debug level so transport teardown
failures aren't silently dropped. Update the defer in GetManifestFromRef to call
src.Close(), check its error, and call the project logger (import
"github.com/cri-o/cri-o/internal/log") e.g. log.Debugf("closing image source:
%v", err) when non-nil; keep the original return behavior for manifest and
errors unchanged.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

internal/ociartifact/impl.go (1)

34-42: Consider wrapping the error from GetManifest for consistency.

The error from ref.NewImageSource is wrapped with context (line 37), but src.GetManifest errors are returned unwrapped. For consistent error tracing across the codebase, consider wrapping this error as well.

Also, src.Close() error is discarded. While this is a common pattern for read-only sources, logging or propagating a close error can help diagnose resource cleanup issues.

♻️ Proposed fix

 func (*defaultImpl) GetManifestFromRef(ctx context.Context, ref types.ImageReference, sys *types.SystemContext, instanceDigest *digest.Digest) (manifestBlob []byte, mimeType string, err error) {
 	src, err := ref.NewImageSource(ctx, sys)
 	if err != nil {
 		return nil, "", fmt.Errorf("create image source: %w", err)
 	}
-	defer src.Close()
+	defer func() {
+		if closeErr := src.Close(); closeErr != nil && err == nil {
+			err = fmt.Errorf("close image source: %w", closeErr)
+		}
+	}()

-	return src.GetManifest(ctx, instanceDigest)
+	manifestBlob, mimeType, err = src.GetManifest(ctx, instanceDigest)
+	if err != nil {
+		return nil, "", fmt.Errorf("get manifest: %w", err)
+	}
+	return manifestBlob, mimeType, nil
 }

🤖 Prompt for AI Agents

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

In `@internal/ociartifact/impl.go` around lines 34 - 42, In GetManifestFromRef on
defaultImpl, wrap the error returned by src.GetManifest with context (similar to
how ref.NewImageSource is wrapped) so callers get consistent error traces, and
handle the deferred src.Close() error instead of discarding it — either log it
via the package logger or wrap/append it to the returned error before returning;
update references to ref.NewImageSource, src.GetManifest, and src.Close
accordingly to implement these changes.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro