internal/ociartifact/datastore/impl.go (1)

16-17: Make the interface comment state the intent (DI/mocking)

Proposed comment update

-// Impl is the interface for the implementation.
+// Impl exists to allow dependency injection/mocking of image operations in tests.

As per coding guidelines: Add comments explaining 'why' not 'what' in Go code.

internal/ociartifact/datastore/suite_test.go (1)

12-16: Make the TestRun comment explain why it exists

Proposed comment update

-// TestRun runs the created specs.
+// TestRun wires Ginkgo into go test so the datastore suite runs under `go test`.

As per coding guidelines: Add comments explaining 'why' not 'what' in Go code.

internal/ociartifact/datastore/store_test.go (1)

44-83: Consider adding happy-path tests for PullData.

The current tests cover error scenarios well (ParseNormalizedNamed and DockerNewReference failures). Consider adding tests for the successful pull path and other failure scenarios (e.g., artifact size exceeding MaxSize, empty artifact data) to improve coverage.

internal/ociartifact/artifact.go (1)

32-51: Consider accepting context.Context for proper log propagation.

NewArtifact uses context.Background() for logging (line 42), which loses any tracing/logging context the caller might have. Per the coding guidelines, context.Context should be propagated through function calls.

♻️ Suggested change to accept context

-func NewArtifact(art *libartifact.Artifact) *Artifact {
+func NewArtifact(ctx context.Context, art *libartifact.Artifact) *Artifact {
 	artifact := &Artifact{
 		Artifact: art,
 		namedRef: unknownRef{},
 	}

 	if art.Name != "" {
 		namedRef, err := reference.ParseNormalizedNamed(art.Name)
 		if err != nil {
-			log.Warnf(context.Background(), "Failed to parse artifact name %s with the error %s", art.Name, err)
+			log.Warnf(ctx, "Failed to parse artifact name %q: %v", art.Name, err)

 			namedRef = unknownRef{}
 		}

 		artifact.namedRef = namedRef
 	}

 	return artifact
 }

As per coding guidelines: "Propagate context.Context through function calls in Go code".

internal/ociartifact/store.go (1)

102-111: Consider wrapping the error from Remove for consistent error handling.

Other methods in this file wrap errors with fmt.Errorf and %w, but Remove returns the error directly on line 108. This inconsistency may lose context in error chains.

♻️ Suggested fix for consistency

 func (s *Store) Remove(ctx context.Context, nameOrDigest string) error {
 	artRef, err := libart.NewArtifactStorageReference(nameOrDigest)
 	if err != nil {
 		return fmt.Errorf("invalid nameOrDigest: %w", err)
 	}

-	_, err = s.libartifactStore.Remove(ctx, artRef)
-
-	return err
+	if _, err = s.libartifactStore.Remove(ctx, artRef); err != nil {
+		return fmt.Errorf("remove artifact: %w", err)
+	}
+
+	return nil
 }

As per coding guidelines: "Use fmt.Errorf with %w for error wrapping in Go code".

internal/ociartifact/datastore/store.go (1)

228-231: Consider documenting the minimum digest prefix length.

The len(strRef) >= 3 check enforces a minimum prefix length for digest matching but lacks documentation explaining why 3 was chosen. Adding a brief comment would improve maintainability.

📝 Suggested comment

-	// if strRef is a just digest or short digest
+	// if strRef is a digest or short digest prefix (minimum 3 chars to avoid overly ambiguous matches)
 	if idx := slices.IndexFunc(artifacts, func(a *ociartifact.Artifact) bool { return strings.HasPrefix(a.Digest().Encoded(), strRef) }); len(strRef) >= 3 && idx != -1 {

As per coding guidelines: "Add comments explaining 'why' not 'what' in Go code".