bindings/go/transfer/internal/helpers.go (2)

99-101: Improve error message clarity.

Line 100 uses imageRef (the parsed struct) in the error message. Depending on its String() implementation, this may not produce a user-friendly message. Consider using the original imageReference string instead.

♻️ Proposed fix

 	if imageRef.Repository == "" {
-		return "", fmt.Errorf("invalid image reference %q: repository is required", imageRef)
+		return "", fmt.Errorf("invalid image reference %q: repository is required", imageReference)
 	}

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/helpers.go` around lines 99 - 101, The error
message uses the parsed imageRef struct which may not render nicely; update the
validation in helpers.go so the fmt.Errorf uses the original imageReference
input string (not imageRef) when returning "invalid image reference ...:
repository is required" — locate the check that references imageRef.Repository
and change the error formatting to include imageReference for clearer user
output (preserve the same error text and formatting otherwise).

---

15-25: Consider returning an error instead of panicking.

The panic() calls in asUnstructured will crash the program if conversion fails. While this may be acceptable for internal use where inputs are guaranteed valid, returning an error would make the function safer and more reusable.

♻️ Proposed refactor to return error

-func asUnstructured(typed runtime.Typed) *runtime.Unstructured {
+func asUnstructured(typed runtime.Typed) (*runtime.Unstructured, error) {
 	var raw runtime.Raw
 	if err := runtime.NewScheme(runtime.WithAllowUnknown()).Convert(typed, &raw); err != nil {
-		panic(fmt.Sprintf("cannot convert to raw: %v", err))
+		return nil, fmt.Errorf("cannot convert to raw: %w", err)
 	}
 	var unstructured runtime.Unstructured
 	if err := runtime.NewScheme(runtime.WithAllowUnknown()).Convert(&raw, &unstructured); err != nil {
-		panic(fmt.Sprintf("cannot convert to unstructured: %v", err))
+		return nil, fmt.Errorf("cannot convert to unstructured: %w", err)
 	}
-	return &unstructured
+	return &unstructured, nil
 }

This would require updating all call sites to handle the returned error.

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/helpers.go` around lines 15 - 25, Replace the
panics in asUnstructured with proper error propagation: change the signature of
asUnstructured(typed runtime.Typed) to return (*runtime.Unstructured, error),
replace both panic(fmt.Sprintf(...)) calls with returning nil and the wrapped
error, and update all callers of asUnstructured to handle the returned error
(propagate or handle appropriately). Ensure you import/return errors as needed
and keep the conversion logic using
runtime.NewScheme(runtime.WithAllowUnknown()) unchanged.

bindings/go/transfer/internal/localblob.go (1)

15-15: Consider removing or using the unused parameter.

The second parameter _ *descriptorv2.LocalBlob is unused. If it's reserved for future use, consider adding a brief comment explaining why, or remove it to keep the signature clean.

♻️ Proposed fix

-func processLocalBlob(resource descriptorv2.Resource, _ *descriptorv2.LocalBlob, id string, val *discoveryValue, tgd *transformv1alpha1.TransformationGraphDefinition, toSpec runtime.Typed, resourceTransformIDs map[int]string, i int, uploadAsOCIArtifact bool) error {
+func processLocalBlob(resource descriptorv2.Resource, id string, val *discoveryValue, tgd *transformv1alpha1.TransformationGraphDefinition, toSpec runtime.Typed, resourceTransformIDs map[int]string, i int, uploadAsOCIArtifact bool) error {

Note: This would require updating the call site in graph.go as well.

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/localblob.go` at line 15, The function
processLocalBlob currently has an unused parameter `_ *descriptorv2.LocalBlob`;
either remove this parameter from the signature or use it if intended; update
all callers (notably the call site in graph.go) to match the new signature; if
you keep the parameter for future use, add a brief comment above the parameter
(e.g., "// reserved for future use") to explain why it's intentionally unused
and prevent linters from flagging it.

bindings/go/transfer/internal/graph.go (1)

85-89: Consider adding a type assertion check for safety.

The type assertion on line 87 could panic if the attribute is missing or has an unexpected type. While this is internal code with controlled inputs, a checked assertion would be more defensive.

🛡️ Proposed defensive assertion

 for _, v := range d.Vertices {
-	val := v.Attributes[dagsync.AttributeValue].(*discoveryValue)
+	valAttr, ok := v.Attributes[dagsync.AttributeValue].(*discoveryValue)
+	if !ok {
+		return fmt.Errorf("unexpected attribute type for vertex %s", v.ID)
+	}
+	val := valAttr
 	component := val.Descriptor.Component.Name

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/graph.go` around lines 85 - 89, The loop in
fillGraphDefinitionWithPrefetchedComponents uses an unchecked type assertion
v.Attributes[dagsync.AttributeValue].(*discoveryValue) which can panic if the
attribute is missing or the type differs; change this to a checked assertion
(e.g., raw, ok := v.Attributes[dagsync.AttributeValue]; dv, ok2 :=
raw.(*discoveryValue)) and if either the attribute is absent or ok2 is false,
return a descriptive error (or skip/continue as appropriate) referencing
dagsync.AttributeValue and discoveryValue so callers can diagnose
missing/invalid attributes.

bindings/go/transfer/internal/discovery_test.go (1)

59-64: Minor: Non-deterministic map iteration in mock.

The fallback logic iterating over m.repos map (lines 60-63) has non-deterministic ordering. While current tests either set sharedRepo or have single-entry maps, this could cause flaky tests if future tests configure multiple repos without setting sharedRepo.

♻️ Suggested improvement

Consider documenting the single-repo assumption or making the mock more explicit:

 	// Return the first repo (for simple tests)
+	// WARNING: Map iteration is non-deterministic; ensure only one repo is configured
+	// or use sharedRepo for multi-entry scenarios.
 	for _, r := range m.repos {
 		return r, nil
 	}

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/discovery_test.go` around lines 59 - 64, The
mock currently returns the "first" repo by iterating m.repos which is
non-deterministic; update the repo-returning logic in the mock (the code that
iterates m.repos and returns r, nil) to be deterministic: if m.sharedRepo (or a
similar explicit shared repo field) is set return that; else if len(m.repos)==1
return that single entry; otherwise either return a clear error indicating
multiple repos are configured or select a deterministic entry by sorting the map
keys and returning the first key’s repo. Ensure you modify the function that
currently loops "for _, r := range m.repos { return r, nil }" to use one of
these deterministic strategies.

bindings/go/transfer/integration/integration_test.go (1)

232-260: Add an explicit timeout context for graph execution and target verification.

Using t.Context() alone can leave this integration test hanging longer than necessary on network/container issues. A local deadline makes failures deterministic.

Suggested refactor

-	ctx := t.Context()
+	ctx, cancel := context.WithTimeout(t.Context(), 2*time.Minute)
+	defer cancel()
 	credResolver := &staticCredResolver{address: registryAddr, username: user, password: password}

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/integration/integration_test.go` around lines 232 - 260,
The test currently uses t.Context() (ctx) which can hang; wrap t.Context() with
a local deadline via context.WithTimeout and defer cancel, then pass that timed
ctx into graph.Process(ctx) and targetRepo.GetComponentVersion(ctx) (and any
other network calls in this block) so the test fails fast on network/container
issues; update the variable used by those calls (ctx) to the timed context and
ensure cancel() is deferred.

bindings/go/transfer/internal/discovery_test.go (3)

165-225: Consider adding tests for additional error paths.

The resolver tests cover key format errors and GetRepositorySpecificationForComponent failures, but the implementation (per context snippet 1) has additional failure points:

1. GetComponentVersionRepositoryForSpecification returning an error
2. repo.GetComponentVersion returning an error
3. Digest verification failure (when expectedDigest returns a non-nil value)

Adding these would improve coverage of the resolver's error handling paths.

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/discovery_test.go` around lines 165 - 225,
Tests currently miss covering three resolver error paths:
GetComponentVersionRepositoryForSpecification failures, repo.GetComponentVersion
failures, and digest verification failures in resolver.Resolve. Add separate
unit tests that instantiate resolver with a mockCVRepoResolver that returns an
error from GetComponentVersionRepositoryForSpecification to assert the error is
propagated; another test where mockCVRepo (the repo value returned) has
GetComponentVersion return an error and assert resolver.Resolve returns that
error; and a test where expectedDigest returns a non-nil descriptor differing
from the actual descriptor digest to assert a digest verification failure is
raised. Use the existing mock types (mockCVRepoResolver, mockCVRepo) and the
resolver.Resolve call to exercise these code paths and assert error messages
contain the expected cause.

---

52-64: Non-deterministic map iteration may cause flaky behavior.

When sharedRepo is nil and multiple repos are configured, iterating over m.repos with for _, r := range m.repos returns an arbitrary entry due to Go's randomized map iteration order. Current tests only configure one repo, so this works, but future tests with multiple repos could become flaky.

Consider using sharedRepo consistently in tests or keying the lookup by the specification:

♻️ Suggested improvement

 func (m *mockCVRepoResolver) GetComponentVersionRepositoryForSpecification(_ context.Context, _ runtime.Typed) (repository.ComponentVersionRepository, error) {
 	if m.err != nil {
 		return nil, m.err
 	}
 	if m.sharedRepo != nil {
 		return m.sharedRepo, nil
 	}
-	// Return the first repo (for simple tests)
-	for _, r := range m.repos {
-		return r, nil
-	}
-	return nil, fmt.Errorf("no repos configured")
+	return nil, fmt.Errorf("no sharedRepo configured for GetComponentVersionRepositoryForSpecification")
 }

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/discovery_test.go` around lines 52 - 64, The
GetComponentVersionRepositoryForSpecification method on mockCVRepoResolver can
return a non-deterministic repo because iterating m.repos (a map) yields
randomized order; change the logic to be deterministic by either (a) preferring
and returning m.sharedRepo when present in all tests, or (b) selecting by a
stable key derived from the incoming specification (use the runtime.Typed
argument) or (c) deterministically picking the first map entry by sorting the
map keys before selecting. Update GetComponentVersionRepositoryForSpecification
to implement one of these deterministic choices (reference: mockCVRepoResolver,
GetComponentVersionRepositoryForSpecification, sharedRepo, repos) so future
tests won’t be flaky.

---

181-195: Consider using sharedRepo for clarity.

This test relies on the map iteration in GetComponentVersionRepositoryForSpecification returning the single configured repo. Using sharedRepo would make the test intent clearer and avoid coupling to the mock's fallback behavior:

♻️ Suggested improvement

 	r := &resolver{
 		repoResolver: &mockCVRepoResolver{
 			specs: map[string]runtime.Typed{
 				"ocm.software/test:1.0.0": repoSpec,
 			},
-			repos: map[string]repository.ComponentVersionRepository{
-				"ocm.software/test:1.0.0": &mockCVRepo{
-					descriptors: map[string]*descriptor.Descriptor{
-						"ocm.software/test:1.0.0": desc,
-					},
-				},
+			sharedRepo: &mockCVRepo{
+				descriptors: map[string]*descriptor.Descriptor{
+					"ocm.software/test:1.0.0": desc,
+				},
 			},
 		},
 		expectedDigest: func(_ runtime.Identity) *descriptor.Digest { return nil },
 	}

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/discovery_test.go` around lines 181 - 195, The
test should create a single shared repo instance and use it in the mock resolver
instead of relying on map iteration/fallback: create sharedRepo :=
&mockCVRepo{descriptors:
map[string]*descriptor.Descriptor{"ocm.software/test:1.0.0": desc}} and then set
the mockCVRepoResolver.repos entry for the spec key to that sharedRepo (keep
repoSpec in the specs map as-is); update the resolver initialization to
reference sharedRepo so resolver, mockCVRepoResolver, mockCVRepo, desc and
repoSpec are clearly linked and the test no longer depends on
GetComponentVersionRepositoryForSpecification iteration order.

bindings/go/transfer/internal/helpers.go (1)

15-25: Consider reusing the scheme instance instead of creating new ones on each call.

asUnstructured creates two new runtime.Scheme instances per invocation. Since this function may be called frequently during graph building, reusing the package-level scheme variable (or a dedicated unstructured-conversion scheme) would reduce allocation overhead.

Also, the panic calls assume conversion can never fail for valid inputs. If this assumption is violated, it will crash the process. Consider returning an error instead, or add a doc comment explaining when panics are acceptable.

♻️ Suggested improvement

+var unstructuredScheme = runtime.NewScheme(runtime.WithAllowUnknown())
+
-func asUnstructured(typed runtime.Typed) *runtime.Unstructured {
+func asUnstructured(typed runtime.Typed) (*runtime.Unstructured, error) {
 	var raw runtime.Raw
-	if err := runtime.NewScheme(runtime.WithAllowUnknown()).Convert(typed, &raw); err != nil {
-		panic(fmt.Sprintf("cannot convert to raw: %v", err))
+	if err := unstructuredScheme.Convert(typed, &raw); err != nil {
+		return nil, fmt.Errorf("cannot convert to raw: %w", err)
 	}
 	var unstructured runtime.Unstructured
-	if err := runtime.NewScheme(runtime.WithAllowUnknown()).Convert(&raw, &unstructured); err != nil {
-		panic(fmt.Sprintf("cannot convert to unstructured: %v", err))
+	if err := unstructuredScheme.Convert(&raw, &unstructured); err != nil {
+		return nil, fmt.Errorf("cannot convert to unstructured: %w", err)
 	}
-	return &unstructured
+	return &unstructured, nil
 }

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/helpers.go` around lines 15 - 25, The
asUnstructured function currently allocates two runtime.Scheme instances per
call; change it to reuse a package-level scheme (e.g., a pre-initialized
variable named scheme or a dedicated unstructuredScheme) for Convert operations
instead of calling runtime.NewScheme every time, and handle conversion failures
without unguarded panics by returning an error from asUnstructured (or, if you
must keep panics, add a clear doc comment stating the function panics on
conversion failure). Update callers to handle the new error return if you choose
the error approach and reference asUnstructured, runtime.Scheme, and the
package-level scheme/unstructuredScheme when making the change.

bindings/go/transfer/internal/oci.go (1)

102-104: Minor: Inconsistent spacing in CEL expressions.

The CEL expressions have double spaces before the colon (e.g., "labels :"). While this likely doesn't affect functionality, it's inconsistent with typical formatting.

♻️ Formatting fix

-				"digest":        fmt.Sprintf("${%s.output.resource.digest}", getResourceID),
-				"labels":        fmt.Sprintf("${has(%s.output.resource.labels) ? %s.output.resource.labels  : []}", getResourceID, getResourceID),
-				"extraIdentity": fmt.Sprintf("${has(%s.output.resource.extraIdentity) ? %s.output.resource.extraIdentity  : {}}", getResourceID, getResourceID),
-				"srcRefs":       fmt.Sprintf("${has(%s.output.resource.srcRefs) ? %s.output.resource.srcRefs  : []}", getResourceID, getResourceID),
+				"digest":        fmt.Sprintf("${%s.output.resource.digest}", getResourceID),
+				"labels":        fmt.Sprintf("${has(%s.output.resource.labels) ? %s.output.resource.labels : []}", getResourceID, getResourceID),
+				"extraIdentity": fmt.Sprintf("${has(%s.output.resource.extraIdentity) ? %s.output.resource.extraIdentity : {}}", getResourceID, getResourceID),
+				"srcRefs":       fmt.Sprintf("${has(%s.output.resource.srcRefs) ? %s.output.resource.srcRefs : []}", getResourceID, getResourceID),

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/oci.go` around lines 102 - 104, Fix
inconsistent spacing in the CEL expressions used in the map entries built with
fmt.Sprintf (look for the "labels", "extraIdentity", and "srcRefs" entries that
use getResourceID) by removing the extra space before the colon so the
ternary-like CEL snippets read "... ? %s.output.resource.labels : []" (and
similarly for extraIdentity and srcRefs); adjust the string literals passed to
fmt.Sprintf accordingly to ensure consistent single-space formatting around the
colon.

bindings/go/transfer/internal/graph_test.go (1)

157-252: Add a regression that covers descriptor metadata plus a transformed resource.

These cases exercise the resource wiring well, but they still don't cover a descriptor that has component-level metadata (for example labels) and then goes through the transformed-resource branch. A focused case there would lock in the fix for buildDescriptorSpec and prevent silent descriptor truncation from regressing.

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/graph_test.go` around lines 157 - 252, Add a
regression test that constructs a descriptor with component-level metadata
(e.g., labels) using testDescriptor and a resource that triggers the
transformed-resource path (e.g., helmResource), then call BuildGraphDefinition
(same flags as existing helm tests) and assert that the resulting
graph/component spec preserves the original metadata; this will exercise the
transformed-resource branch and lock in the fix for buildDescriptorSpec by
ensuring component metadata is not dropped during transformation.

bindings/go/transfer/internal/localblob.go (1)

15-15: Consider using or removing the unused *descriptorv2.LocalBlob parameter.

The second parameter _ *descriptorv2.LocalBlob is passed from the call site in graph.go (where it's the typed access) but never used. If it provides useful metadata for future enhancements, consider keeping it with a comment; otherwise, removing it simplifies the interface.

🤖 Prompt for AI Agents

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

In `@bindings/go/transfer/internal/localblob.go` at line 15, The parameter list
for processLocalBlob includes an unused second parameter (_
*descriptorv2.LocalBlob); either remove this parameter from processLocalBlob and
its callers (including the call in graph.go that currently passes the typed
access) to simplify the signature, or keep it and add a brief comment inside the
function signature explaining it's reserved for future metadata use so
linters/readers know it's intentionally unused; update all call sites (e.g., the
graph.go invocation) to match the new signature if you remove it, or leave them
as-is if you keep it with the comment.