cli/cmd/transfer/component-version/cmd_test.go (1)

85-274: Good coverage overall; please add one stdin case.

loadTransferSpec() has a separate path == "-" branch, but the new tests only exercise file-backed specs. One small --transfer-spec - round-trip would protect the documented pipe workflow from regressing.

🤖 Prompt for AI Agents

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

In `@cli/cmd/transfer/component-version/cmd_test.go` around lines 85 - 274, Add a
test exercising the stdin ("-") branch of loadTransferSpec by round-tripping a
generated spec through stdin: generate the spec via the existing dry-run flow
into a buffer, then invoke test.OCM with args "transfer component-version
--transfer-spec -", supplying the buffer as the process stdin (and appropriate
output/log readers), and assert the command succeeds and the transfer behaves as
in the other transfer-spec tests; this ensures loadTransferSpec's path == "-"
branch is covered.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/cmd/transfer/component-version/cmd.go (1)

236-259: Consider validating the loaded spec has required content.

loadTransferSpec unmarshals YAML into TransformationGraphDefinition but doesn't validate the result. Based on context snippet 2 (builder.go:29-87), BuildAndCheck will eventually process the spec, but an empty Transformations slice or nil Environment won't produce a clear error message for the user.

If the user provides an empty file or malformed spec without transformations, the error surfaces later in BuildAndCheck with a less actionable message.

💡 Optional: Add basic validation after unmarshaling

 	tgd := &transformv1alpha1.TransformationGraphDefinition{}
 	if err := yaml.Unmarshal(data, tgd); err != nil {
 		return nil, fmt.Errorf("parsing transfer spec: %w", err)
 	}
+
+	if len(tgd.Transformations) == 0 {
+		return nil, fmt.Errorf("transfer spec contains no transformations")
+	}

 	return tgd, nil

🤖 Prompt for AI Agents

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

In `@cli/cmd/transfer/component-version/cmd.go` around lines 236 - 259,
loadTransferSpec currently unmarshals YAML into a
transformv1alpha1.TransformationGraphDefinition but doesn't validate required
fields, so empty or missing data (e.g., no Transformations or nil Environment)
only fails later in BuildAndCheck with poor context; after yaml.Unmarshal in
loadTransferSpec validate that tgd is non-nil, that len(tgd.Transformations) > 0
and that tgd.Environment (or other required top-level fields used by
BuildAndCheck) is present/initialized, and return a clear fmt.Errorf like
"invalid transfer spec: missing Transformations" (or "missing Environment") so
users get immediate actionable errors referencing loadTransferSpec rather than
surfacing later in BuildAndCheck.

cli/integration/transfer_with_transfer_spec_test.go (2)

88-91: Consider increasing transfer timeout to reduce CI flakiness.

A fixed 30s budget is tight for registry/container-based integration tests. A slightly larger timeout (or shared integration timeout constant) would make this more reliable.

Suggested adjustment

- ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second)
+ ctx, cancel := context.WithTimeout(t.Context(), 2*time.Minute)

🤖 Prompt for AI Agents

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

In `@cli/integration/transfer_with_transfer_spec_test.go` around lines 88 - 91,
The 30s timeout passed to context.WithTimeout around transferCMD.ExecuteContext
is too tight for container/registry integration tests; increase the timeout
(e.g., 2m or a shared integration timeout constant) or introduce a shared
constant like integrationTimeout and use context.WithTimeout(t.Context(),
integrationTimeout) so the test has a larger, configurable budget to reduce CI
flakiness when calling transferCMD.ExecuteContext.

---

42-42: Use narrower file permissions for temp artifacts.

os.ModePerm is broader than needed and can trigger security lint noise. Prefer explicit file modes for test files.

Suggested adjustment

- r.NoError(os.WriteFile(constructorPath, []byte(constructorContent), os.ModePerm))
+ r.NoError(os.WriteFile(constructorPath, []byte(constructorContent), 0o600))

- r.NoError(os.WriteFile(specFile, spec, os.ModePerm))
+ r.NoError(os.WriteFile(specFile, spec, 0o600))

- r.NoError(os.WriteFile(specFile, []byte(modifiedSpec), os.ModePerm))
+ r.NoError(os.WriteFile(specFile, []byte(modifiedSpec), 0o600))

Also applies to: 137-137, 182-182

🤖 Prompt for AI Agents

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

In `@cli/integration/transfer_with_transfer_spec_test.go` at line 42, Replace the
permissive os.ModePerm usage when writing temp test files with a narrower
explicit mode (e.g., 0o600) to avoid security lint noise; update the
os.WriteFile calls (see the r.NoError(os.WriteFile(constructorPath,
[]byte(constructorContent), os.ModePerm)) call and the similar os.WriteFile
invocations around the other noted lines) to use the explicit file mode constant
instead of os.ModePerm.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/cmd/transfer/component-version/cmd.go (1)

184-196: ⚠️ Potential issue | 🟠 Major

renderTGD() still needs a hard error check before io.ReadAll().

The nil-guard on Close() fixed only half of the earlier issue. If renderTGD() returns (nil, rerr), Line 191 still reads from a nil reader, and any io.ReadAll() failure is still dropped, so the original BuildAndCheck() error can still get masked.

🔧 Suggested change

 	if err != nil {
 		reader, rerr := renderTGD(tgd, output)
-		defer func() {
-			if reader != nil {
-				_ = reader.Close()
-			}
-		}()
-		raw, _ := io.ReadAll(reader)
-		return errors.Join(
-			err,
-			rerr,
-			fmt.Errorf("%s", raw),
-		)
+		if rerr != nil {
+			return errors.Join(err, rerr)
+		}
+		defer func() {
+			if cerr := reader.Close(); cerr != nil {
+				slog.WarnContext(ctx, "closing transformation graph reader failed", "error", cerr)
+			}
+		}()
+		raw, readErr := io.ReadAll(reader)
+		if readErr != nil {
+			return errors.Join(err, readErr)
+		}
+		if len(raw) == 0 {
+			return err
+		}
+		return errors.Join(err, fmt.Errorf("%s", raw))
 	}

🤖 Prompt for AI Agents

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

In `@cli/cmd/transfer/component-version/cmd.go` around lines 184 - 196, The error
handling around renderTGD() is incomplete: ensure you check rerr and that reader
is non-nil before calling io.ReadAll(), and capture any error from io.ReadAll()
instead of discarding it; specifically, after calling renderTGD(tgd, output)
check if rerr != nil and return or include it, only set up the deferred Close()
when reader != nil, then call raw, readErr := io.ReadAll(reader) and include
readErr in the errors.Join alongside err and rerr (or return early if reader is
nil and rerr is set) so the original BuildAndCheck error isn't masked and
ReadAll failures are propagated.

cli/cmd/transfer/component-version/cmd.go (1)

231-252: Reject blank / empty transfer specs before returning a zero-value graph.

yaml.Unmarshal accepts an empty file or stdin into TransformationGraphDefinition{}. Since the bindings type leaves both top-level fields zero-valued by default, this path currently depends on downstream behavior to catch a truncated spec. A small upfront validation here would make the new hand-edited workflow much easier to diagnose.

🔧 Suggested change

 func loadTransferSpec(path string, stdin io.Reader) (*transformv1alpha1.TransformationGraphDefinition, error) {
 	var data []byte
 	var err error
 
 	if path == "-" {
 		data, err = io.ReadAll(stdin)
 		if err != nil {
 			return nil, fmt.Errorf("reading transfer spec from stdin: %w", err)
 		}
 	} else {
 		data, err = os.ReadFile(path)
 		if err != nil {
 			return nil, fmt.Errorf("reading transfer spec file %q: %w", path, err)
 		}
 	}
+	if len(bytes.TrimSpace(data)) == 0 {
+		return nil, fmt.Errorf("transfer spec is empty")
+	}
 
 	tgd := &transformv1alpha1.TransformationGraphDefinition{}
 	if err := yaml.Unmarshal(data, tgd); err != nil {
 		return nil, fmt.Errorf("parsing transfer spec: %w", err)
 	}
+	if len(tgd.Transformations) == 0 {
+		return nil, fmt.Errorf("transfer spec must contain at least one transformation")
+	}
 
 	return tgd, nil
 }

🤖 Prompt for AI Agents

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

In `@cli/cmd/transfer/component-version/cmd.go` around lines 231 - 252, In
loadTransferSpec, reject empty or blank specs and zero-value parsed graphs
before returning: after reading data, validate that bytes.TrimSpace(data) is
non-empty and after yaml.Unmarshal into
transformv1alpha1.TransformationGraphDefinition (tgd) check that at least one
required top-level field on tgd is set (i.e., not the zero-value) — if not,
return a descriptive error like "empty transfer spec" so callers don't get a
zero-value TransformationGraphDefinition; update loadTransferSpec to perform
these checks and return errors instead of returning a zeroed tgd.