feat: introduce --reproducible and image insepct#275
Merged
Conversation
📝 WalkthroughWalkthroughAdds a reproducible build mode (API/CLI/controller/validation), a new ChangesReproducible Build + Inspect
Sequence DiagramsequenceDiagram
actor User
participant CLI as caib CLI
participant API as Build API
participant Registry as OCI Registry
participant Tekton as Tekton
participant Scripts as Build/Push Scripts
User->>CLI: caib image inspect <ref>
CLI->>Registry: fetch manifest & annotations
Registry-->>CLI: manifest + annotations
CLI->>Registry: discover referrers by digest tag
Registry-->>CLI: referrer index (artifact types + digests)
CLI->>Registry: pull known referrer artifacts (if OutputDir)
Registry-->>CLI: artifact blobs
CLI-->>User: print provenance (table or JSON/YAML + rebuild command)
User->>CLI: caib image build --reproducible --secure ...
CLI->>API: POST /build {Reproducible: true, SecureBuild: true, ...}
API->>Tekton: create PipelineRun with reproducible param
Tekton->>Scripts: run build-image (reproducible)
Scripts->>Scripts: add reproducible define, archive sources, produce manifest
Tekton->>Scripts: run push-artifact (reproducible)
Scripts->>Registry: push image and attach OCI referrers
Registry-->>API: provide referrer digests/result
API-->>User: build complete (provenance artifacts recorded)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/caib/buildcmd/build.go (1)
147-169: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd regression coverage for the new reproducible flag paths.
This adds new validation branches and new
BuildRequest.Reproducibleserialization in both build modes, but there isn't matching coverage in this diff. A small table-driven handler test aroundvalidateReproducibleFlags()plus request construction would catch flag/API drift before it only fails on-cluster.As per coding guidelines,
**/*.go: Add failing tests before starting implementation.Also applies to: 470-493, 662-748
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/caib/buildcmd/build.go` around lines 147 - 169, Add table-driven unit tests that cover the new validation branches around reproducible handling: test Handler.validateReproducibleFlags() for combinations of opts.Reproducible, opts.SecureBuild, and opts.UseInternalRegistry (e.g., reproducible without secure -> expect error via common.ValidateReproducibleRequiresSecure, reproducible with internal registry -> expect the specific "--reproducible cannot be used with --internal-registry" error, and valid combos -> no error). Also add tests that exercise request construction/serialization to ensure BuildRequest.Reproducible is set correctly in both build modes (the code paths that set BuildRequest.Reproducible referenced in the handler that builds requests for disk-image vs container builds), using table-driven cases to catch flag/API drift and failing tests before implementation changes land.
🤖 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/imagebuild_types.go`:
- Around line 105-109: The CRD does not enforce the documented invariant that
Reproducible implies SecureBuild; add an XValidation rule to the ImageBuildSpec
type so the OpenAPI v3 schema rejects specs where reproducible is true but
secureBuild is false or omitted. Locate the ImageBuildSpec definition (reference
symbols: ImageBuildSpec, Reproducible, SecureBuild) and add an XValidation
expression that requires secureBuild == true when reproducible == true, then run
`make generate manifests` to regenerate the CRD manifests.
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 339-383: The code unconditionally appends " --reproducible" which
produces invalid rebuild commands for artifacts not recorded as reproducible;
change the logic around the parts slice so that you only append "
--reproducible" when the artifact metadata actually marks it reproducible (e.g.,
check get("reproducible") == "true" or another reproducibility marker in your
metadata), leaving the rest of the push handling (isDevBuild / --push /
--push-disk) unchanged; update the block that currently does parts =
append(parts, " --reproducible") to be conditional using get("reproducible")
(or the appropriate reproducibility referrer/type) so non-reproducible artifacts
do not get the flag.
In `@internal/buildapi/server.go`:
- Around line 1344-1346: In createBuild, validate at the API boundary that a
request cannot set Spec.Reproducible=true while Spec.SecureBuild=false: detect
this condition (req.Spec.Reproducible && !req.Spec.SecureBuild) and return a
clear validation error instead of proceeding; remove the current silent behavior
where annotations[automotivev1alpha1.AnnotationTaskBundleRef] may be omitted
(the code that sets AnnotationTaskBundleRef should only run when both
SecureBuild is true and Reproducible is true), and ensure any callers relying on
createBuild receive the error so inconsistent requests fail fast.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 850-858: The inline Python heredoc that handles missing PyYAML
(the block invoked by python3 - "$MANIFEST_FILE"
"$WORKSPACE_PATH/aib-manifest.yml" <<'PYEOF') must fail closed instead of
copying the raw manifest; in the ImportError branch remove shutil.copy2 and
instead print a clear stderr error and exit with non-zero (e.g., sys.exit(1)) so
the shell script fails and does not publish the unsanitized MANIFEST_FILE to
WORKSPACE_PATH/aib-manifest.yml; ensure the surrounding shell script will
propagate that non-zero exit (set -e or check the python exit code) so the build
aborts when PyYAML is unavailable.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 523-547: attach_referrer is using relative paths
("./aib-manifest.yml", "./build-sources.tar.gz") so when REPRODUCIBLE=true the
script misses files written to /workspace/shared by build_image.sh; change the
calls to attach_referrer to point to the workspace paths (e.g.
/workspace/shared/aib-manifest.yml and /workspace/shared/build-sources.tar.gz)
and, when REPRODUCIBLE is "true" and DISK_DIGEST is set, treat missing referrers
as fatal (exit non-zero) instead of just warning so the attach step cannot
silently succeed without the promised artifacts; keep the attach_referrer
function and DISK_DIGEST/REPRODUCIBLE checks intact while only updating the file
path arguments and the missing-file behavior in that reproducible branch.
In `@internal/common/tasks/tasks.go`:
- Around line 318-353: Add a generation unit test that fails first, then
implements checks to lock these Tekton params: "reproducible",
"task-bundle-ref", "custom-defines", and "aib-extra-args" are present on the
generated Task/Pipeline and are correctly wired (bound) into the steps named
"build-image" and "push-disk-artifact"; locate the generator you use to produce
the Tekton objects in internal/common/tasks (the code that constructs the Param
slices) and write assertions that the produced Task/Pipeline's Spec.Params
contains those Param.Name values and that the TaskSpec/Step/Args for the
"build-image" and "push-disk-artifact" steps reference the corresponding param
substitutions, then add similar tests covering the other affected regions
(around the other ranges noted) so the test suite fails if any of these param
names or bindings are renamed or removed.
---
Outside diff comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 147-169: Add table-driven unit tests that cover the new validation
branches around reproducible handling: test Handler.validateReproducibleFlags()
for combinations of opts.Reproducible, opts.SecureBuild, and
opts.UseInternalRegistry (e.g., reproducible without secure -> expect error via
common.ValidateReproducibleRequiresSecure, reproducible with internal registry
-> expect the specific "--reproducible cannot be used with --internal-registry"
error, and valid combos -> no error). Also add tests that exercise request
construction/serialization to ensure BuildRequest.Reproducible is set correctly
in both build modes (the code paths that set BuildRequest.Reproducible
referenced in the handler that builds requests for disk-image vs container
builds), using table-driven cases to catch flag/API drift and failing tests
before implementation changes land.
🪄 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: 1924f7ac-f417-43fe-9193-a86df464457d
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (16)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/common/build_validation.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
Comment on lines
+318
to
+353
| { | ||
| Name: "reproducible", | ||
| Type: tektonv1.ParamTypeString, | ||
| Description: "Attach RPMs and AIB manifest as OCI referrers for reproducibility (true/false)", | ||
| Default: &tektonv1.ParamValue{ | ||
| Type: tektonv1.ParamTypeString, | ||
| StringVal: "false", | ||
| }, | ||
| }, | ||
| { | ||
| Name: "task-bundle-ref", | ||
| Type: tektonv1.ParamTypeString, | ||
| Description: "Digest-pinned Tekton Bundle reference used for this build", | ||
| Default: &tektonv1.ParamValue{ | ||
| Type: tektonv1.ParamTypeString, | ||
| StringVal: "", | ||
| }, | ||
| }, | ||
| { | ||
| Name: "custom-defines", | ||
| Type: tektonv1.ParamTypeString, | ||
| Description: "Newline-separated custom build definitions (key=value pairs)", | ||
| Default: &tektonv1.ParamValue{ | ||
| Type: tektonv1.ParamTypeString, | ||
| StringVal: "", | ||
| }, | ||
| }, | ||
| { | ||
| Name: "aib-extra-args", | ||
| Type: tektonv1.ParamTypeString, | ||
| Description: "Newline-separated extra arguments passed to AIB", | ||
| Default: &tektonv1.ParamValue{ | ||
| Type: tektonv1.ParamTypeString, | ||
| StringVal: "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Please lock the new Tekton param wiring down with a generation test.
A typo here only shows up when the Pipeline runs. A focused unit test that asserts these params exist on the generated Task/Pipeline and are bound into build-image / push-disk-artifact would keep the new reproducibility plumbing from drifting silently.
As per coding guidelines, **/*.go: Add failing tests before starting implementation.
Also applies to: 575-583, 1101-1136, 1313-1451
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/common/tasks/tasks.go` around lines 318 - 353, Add a generation unit
test that fails first, then implements checks to lock these Tekton params:
"reproducible", "task-bundle-ref", "custom-defines", and "aib-extra-args" are
present on the generated Task/Pipeline and are correctly wired (bound) into the
steps named "build-image" and "push-disk-artifact"; locate the generator you use
to produce the Tekton objects in internal/common/tasks (the code that constructs
the Param slices) and write assertions that the produced Task/Pipeline's
Spec.Params contains those Param.Name values and that the TaskSpec/Step/Args for
the "build-image" and "push-disk-artifact" steps reference the corresponding
param substitutions, then add similar tests covering the other affected regions
(around the other ranges noted) so the test suite fails if any of these param
names or bindings are renamed or removed.
bennyz
force-pushed
the
reproducible-builds
branch
from
f641393 to
5ddc775
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/buildapi/types.go`:
- Around line 166-167: The new BuildRequest.Reproducible boolean field is not
preserved when reconstructing templates; update the template round-trip code
(e.g., getBuildTemplate and any template/introspection reconstruction functions
that build a BuildRequest) to read and set BuildRequest.Reproducible from the
source request/input so it’s included in the reconstructed template. Locate
usages that create or marshal a BuildRequest (functions named getBuildTemplate,
any BuildRequest builders or serializers) and add logic to copy the Reproducible
value into the rebuilt request and include it in any JSON/encoding paths so
replaying a template retains the reproducible flag.
🪄 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: 99639eab-5f7b-43b0-93bc-964b6b5b024b
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (16)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/common/build_validation.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (3)
- cmd/caib/main.go
- api/v1alpha1/labels.go
- cmd/caib/inspectcmd/inspect_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- cmd/caib/registryauth/credentials.go
- cmd/caib/common/build_validation.go
- api/v1alpha1/imagebuild_types.go
- internal/common/tasks/scripts/build_image.sh
- internal/controller/imagebuild/controller.go
- internal/common/tasks/scripts/push_artifact.sh
- cmd/caib/buildcmd/build.go
- cmd/caib/image/image.go
- cmd/caib/inspectcmd/inspect.go
- cmd/caib/runtime_wiring.go
bennyz
force-pushed
the
reproducible-builds
branch
from
5ddc775 to
03d9daf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/caib/inspectcmd/inspect_test.go (1)
81-97: ⚡ Quick win
TestBuildRebuildCommand_DevBuilddoesn't assert--securebehavior.
fullAnnotations()includestask-bundle-ref, sobuildRebuildCommandmay or may not emit--securefor a dev build. The test neither asserts it's present nor absent, leaving a behavioral gap. Add an explicit assertion matching the intended semantics (e.g.,--secureshould/should not appear inbuild-devoutput).♻️ Suggested addition
if strings.Contains(cmd, "--push-disk") { t.Errorf("build-dev should not have --push-disk, got: %s", cmd) } + // dev builds should still honour --secure when task-bundle-ref is present + if !strings.Contains(cmd, "--secure") { + t.Errorf("build-dev with task-bundle-ref should have --secure, got: %s", cmd) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect_test.go` around lines 81 - 97, TestBuildRebuildCommand_DevBuild currently omits asserting whether the generated cmd includes "--secure"; update the test (TestBuildRebuildCommand_DevBuild) to explicitly check the cmd returned by buildRebuildCommand (using the local variable cmd) for the intended "--secure" behavior (e.g., add a strings.Contains assertion that fails if "--secure" is present when it should not be, or vice versa depending on the intended semantics) so the test documents and enforces the expected secure-flag policy for dev builds.cmd/caib/buildcmd/build.go (1)
78-80: ⚡ Quick winAdd tests for the constraint validations in
validateReproducibleFlags.The
validateReproducibleFlags()method (lines 161-168) validates two constraint combinations: reproducible requires secure build, and reproducible cannot be used with internal registry. These validations lack test coverage. Per coding guidelines, add failing tests before implementation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build.go` around lines 78 - 80, Add unit tests that cover the constraint validations in validateReproducibleFlags: create test cases that set Reproducible=true with SecureBuild=false and assert the function returns an error, and set Reproducible=true while using an internal registry (simulate the code path that marks internal registry usage) and assert an error is returned; also include a positive case where Reproducible=true and SecureBuild=true with a non-internal registry that expects no error. Reference the validateReproducibleFlags function and the SecureBuild/Reproducible flag fields when constructing the test inputs and error assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/caib/inspectcmd/inspect_test.go`:
- Around line 150-167: The captureStdout helper currently does a single r.Read
which can return a partial read causing flaky JSON/YAML tests; change
captureStdout (the function named captureStdout in inspect_test.go) to call
io.ReadAll(r) after closing the write end and return the full byte slice as a
string, and ensure you close r to avoid leaking the file descriptor; also add
"io" to the test imports.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 857-866: The redact function currently uses substring matching
(any(s in k.lower() for s in SENSITIVE_KEYS)) which causes over-redaction (e.g.,
"gpgkeys"); update redact(obj) to perform segment-based matching instead:
normalize keys and SENSITIVE_KEYS to lowercase, split each dict key on
separators like '-' and '_' (e.g., segments = re.split(r'[-_]', k.lower())), and
redact only when any segment equals an entry in SENSITIVE_KEYS; keep existing
recursion for nested dicts/lists and ensure SENSITIVE_KEYS remains a set of
lowercase tokens to speed lookups (refer to SENSITIVE_KEYS and redact).
---
Nitpick comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 78-80: Add unit tests that cover the constraint validations in
validateReproducibleFlags: create test cases that set Reproducible=true with
SecureBuild=false and assert the function returns an error, and set
Reproducible=true while using an internal registry (simulate the code path that
marks internal registry usage) and assert an error is returned; also include a
positive case where Reproducible=true and SecureBuild=true with a non-internal
registry that expects no error. Reference the validateReproducibleFlags function
and the SecureBuild/Reproducible flag fields when constructing the test inputs
and error assertions.
In `@cmd/caib/inspectcmd/inspect_test.go`:
- Around line 81-97: TestBuildRebuildCommand_DevBuild currently omits asserting
whether the generated cmd includes "--secure"; update the test
(TestBuildRebuildCommand_DevBuild) to explicitly check the cmd returned by
buildRebuildCommand (using the local variable cmd) for the intended "--secure"
behavior (e.g., add a strings.Contains assertion that fails if "--secure" is
present when it should not be, or vice versa depending on the intended
semantics) so the test documents and enforces the expected secure-flag policy
for dev builds.
🪄 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: 2757146a-6da2-4aff-9759-55c5afc78f95
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (17)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/common/build_validation.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (4)
- api/v1alpha1/labels.go
- cmd/caib/registryauth/credentials.go
- cmd/caib/main.go
- cmd/caib/inspectcmd/inspect.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/caib/runtime_wiring.go
- internal/buildapi/server.go
- internal/controller/imagebuild/controller.go
- cmd/caib/image/image.go
- internal/common/tasks/scripts/push_artifact.sh
- internal/buildapi/types.go
bennyz
force-pushed
the
reproducible-builds
branch
from
03d9daf to
d9f99df
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
internal/common/tasks/scripts/build_image.sh (1)
850-851:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid publishing the raw input manifest as a referrer artifact.
Line 850 copies the manifest verbatim, and that file is later attached as a downloadable OCI referrer. If the manifest contains credentials/secrets, this leaks them.
Suggested hardening
- cp "$MANIFEST_FILE" "$WORKSPACE_PATH/aib-manifest.yml" + # Sanitize before publishing as referrer artifact. + python3 - "$MANIFEST_FILE" "$WORKSPACE_PATH/aib-manifest.yml" <<'PYEOF' +import sys, yaml +from pathlib import Path + +SENSITIVE = {"password","secret","token","auth","credential","passphrase","key"} + +def redact(o): + if isinstance(o, dict): + for k, v in o.items(): + if str(k).lower() in SENSITIVE: + o[k] = "REDACTED" + else: + redact(v) + elif isinstance(o, list): + for i in o: + redact(i) + +data = yaml.safe_load(Path(sys.argv[1]).read_text()) +redact(data) +Path(sys.argv[2]).write_text(yaml.safe_dump(data, sort_keys=False)) +PYEOF🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/common/tasks/scripts/build_image.sh` around lines 850 - 851, The script copies the raw input manifest ($MANIFEST_FILE) into the workspace as aib-manifest.yml which can leak credentials when published; instead, replace the direct copy with creation of a redacted manifest: read $MANIFEST_FILE, remove or redact sensitive keys (e.g., auth, credentials, password, token, secret, registry.auth) and any entries under image.manifest.annotations using a safe parser (jq or yaml sanitizer) and write the sanitized output to $WORKSPACE_PATH/aib-manifest.yml; if sanitization isn’t possible, do not export the manifest at all and instead write only non-sensitive metadata (image digest, created date, non-secret labels). Ensure subsequent steps use the sanitized file or metadata and that MANIFEST_FILE is never attached raw.internal/common/tasks/scripts/push_artifact.sh (1)
527-529:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail reproducible mode when required referrer files are missing.
With
REPRODUCIBLE=true, Line 528 only warns and continues. That allows a “reproducible” publish without the required artifacts.Suggested fix
attach_referrer() { local file="$1" artifact_type="$2" label="$3" if [ ! -f "$file" ]; then - echo "WARNING: $label not found at $file" - return + echo "ERROR: $label not found at $file" + exit 1 fiAlso applies to: 541-548
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 527 - 529, The current check only warns when a required referrer file is missing (echo "WARNING: $label not found at $file"; return), which allows a reproducible publish to proceed; update the missing-file handling to honor the REPRODUCIBLE flag: if REPRODUCIBLE is true, print an error (use a clear message that includes $label and $file) and exit with a non-zero status (exit 1); otherwise keep the current warning/return behavior. Apply this change to the same missing-file checks that appear around the other block (the one covering lines 541-548) so both places enforce failure when REPRODUCIBLE=true.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 346-369: The CLI parts are built from OCI annotations via get(...)
and appended directly (see the blocks that append to parts for "distro",
"target", "arch", "automotive-image-builder", "builder-image", "export-format"
and the loop over "custom-defines"), which can break or be unsafe if values
contain spaces or quotes; update these append sites to shell-quote values before
formatting (introduce a small helper like quoteForShell(s string) that returns a
safely single-quoted string, escaping embedded single quotes, and use it in
fmt.Sprintf calls and inside the custom-defines loop so each appended argument
uses the quoted value).
- Around line 246-248: The code silently swallows json.Unmarshal errors on
rawManifest into index; change this so malformed referrers indexes are surfaced
instead of treated as “no referrers”: when json.Unmarshal(rawManifest, &index)
returns an error, wrap or return that error (e.g., return nil,
fmt.Errorf("failed to parse referrers index: %w", err)) or at minimum log a
warning including rawManifest and err before returning; update the error
handling where json.Unmarshal, rawManifest, and index are used so callers of the
surrounding function receive the parse failure instead of nil, nil.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Line 542: The script currently runs "cd /workspace/shared" unguarded; if that
cd fails subsequent steps will run in the wrong directory. Update the
push_artifact.sh logic around the "cd /workspace/shared" invocation (the cd
command near the referrer attachment section) to check the result and abort on
failure — e.g., perform the cd and if it fails log an error and exit non‑zero
(or ensure set -e is enabled before this step) so the script will not continue
in the wrong directory.
---
Duplicate comments:
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 850-851: The script copies the raw input manifest ($MANIFEST_FILE)
into the workspace as aib-manifest.yml which can leak credentials when
published; instead, replace the direct copy with creation of a redacted
manifest: read $MANIFEST_FILE, remove or redact sensitive keys (e.g., auth,
credentials, password, token, secret, registry.auth) and any entries under
image.manifest.annotations using a safe parser (jq or yaml sanitizer) and write
the sanitized output to $WORKSPACE_PATH/aib-manifest.yml; if sanitization isn’t
possible, do not export the manifest at all and instead write only non-sensitive
metadata (image digest, created date, non-secret labels). Ensure subsequent
steps use the sanitized file or metadata and that MANIFEST_FILE is never
attached raw.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 527-529: The current check only warns when a required referrer
file is missing (echo "WARNING: $label not found at $file"; return), which
allows a reproducible publish to proceed; update the missing-file handling to
honor the REPRODUCIBLE flag: if REPRODUCIBLE is true, print an error (use a
clear message that includes $label and $file) and exit with a non-zero status
(exit 1); otherwise keep the current warning/return behavior. Apply this change
to the same missing-file checks that appear around the other block (the one
covering lines 541-548) so both places enforce failure when REPRODUCIBLE=true.
🪄 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: 5221b690-9533-4d2a-98e8-a7cbddd5c209
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (17)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/common/build_validation.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (5)
- cmd/caib/registryauth/credentials.go
- cmd/caib/main.go
- api/v1alpha1/labels.go
- cmd/caib/inspectcmd/inspect_test.go
- internal/buildapi/server.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/controller/imagebuild/controller.go
- cmd/caib/common/build_validation.go
- cmd/caib/image/image.go
- cmd/caib/buildcmd/build.go
Comment on lines
+346
to
+369
| if v := get("distro"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --distro %s", v)) | ||
| } | ||
| if v := get("target"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --target %s", v)) | ||
| } | ||
| if v := get("arch"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --arch %s", v)) | ||
| } | ||
| if v := get("automotive-image-builder"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --aib-image %s", v)) | ||
| } | ||
| if v := get("builder-image"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --builder-image %s", v)) | ||
| } | ||
| if v := get("export-format"); v != "" { | ||
| parts = append(parts, fmt.Sprintf(" --format %s", v)) | ||
| } | ||
| if v := get("custom-defines"); v != "" { | ||
| for _, def := range strings.Split(v, "\n") { | ||
| def = strings.TrimSpace(def) | ||
| if def != "" { | ||
| parts = append(parts, fmt.Sprintf(" --define %s", def)) | ||
| } |
There was a problem hiding this comment.
Quote annotation-derived CLI values before composing the rebuild command.
These values come from OCI annotations and are appended raw; special chars/spaces can produce unsafe or broken copy-paste commands.
Suggested fix
import (
"context"
"encoding/json"
"fmt"
"os"
"path/filepath"
+ "strconv"
"strings"
@@
func buildRebuildCommand(annotations map[string]string, referrerTypes map[string]bool) string {
get := func(key string) string { return annotations[annotationPrefix+key] }
+ q := func(v string) string { return strconv.Quote(v) }
@@
if v := get("distro"); v != "" {
- parts = append(parts, fmt.Sprintf(" --distro %s", v))
+ parts = append(parts, fmt.Sprintf(" --distro %s", q(v)))
}
if v := get("target"); v != "" {
- parts = append(parts, fmt.Sprintf(" --target %s", v))
+ parts = append(parts, fmt.Sprintf(" --target %s", q(v)))
}
@@
def = strings.TrimSpace(def)
if def != "" {
- parts = append(parts, fmt.Sprintf(" --define %s", def))
+ parts = append(parts, fmt.Sprintf(" --define %s", q(def)))
}
}
}Also applies to: 355-363
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/caib/inspectcmd/inspect.go` around lines 346 - 369, The CLI parts are
built from OCI annotations via get(...) and appended directly (see the blocks
that append to parts for "distro", "target", "arch", "automotive-image-builder",
"builder-image", "export-format" and the loop over "custom-defines"), which can
break or be unsafe if values contain spaces or quotes; update these append sites
to shell-quote values before formatting (introduce a small helper like
quoteForShell(s string) that returns a safely single-quoted string, escaping
embedded single quotes, and use it in fmt.Sprintf calls and inside the
custom-defines loop so each appended argument uses the quoted value).
d9f99df to
f985bd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
cmd/caib/buildcmd/build_disk_test.go (2)
121-192: 💤 Low value
TestRunDiskDefaultsToInternalRegistryLGTM — one minor gap worth notingThe table-driven structure and coverage of the six combinations look correct. The one gap is that success-path subtests do not assert
capturedErr == nil; for the two cases withwantInternal: falsean unexpected error that fires before the defaulting logic would leaveUseInternalRegistryat its initialfalsevalue and silently pass. This is low-impact given the other assertions, but worth a quick guard:if *opts.UseInternalRegistry != tc.wantInternal { t.Errorf("UseInternalRegistry = %v, want %v", *opts.UseInternalRegistry, tc.wantInternal) } +if capturedErr != nil { + t.Errorf("unexpected error: %v", capturedErr) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build_disk_test.go` around lines 121 - 192, The test TestRunDiskDefaultsToInternalRegistry may silently pass when an unexpected error is captured; after calling h.RunDisk(...) in each subtest where tc.wantErrContains is empty assert that capturedErr is nil (e.g. fail the test if capturedErr != nil) before checking *opts.UseInternalRegistry so failures in opts.HandleError are surfaced; locate the test function and add this nil-check immediately after h.RunDisk and before the UseInternalRegistry assertion.
49-49: ⚡ Quick winNo test covers
TaskBundleRef/--reproduciblebehaviour inRunDisk
taskBundleRefis only wired intonewTestDiskOpts()to prevent a nil-dereference panic in existing tests. No test in this file exercises whatRunDiskactually does differently whenTaskBundleRefis a non-empty, digest-pinned OCI reference (e.g., that the correct annotation is emitted, or that an invalid reference is rejected).Per coding guidelines, failing tests should be added before (or alongside) the implementation. A minimal test table entry such as the one below would satisfy the guideline:
✅ Suggested test skeleton for
TaskBundleRef+func TestRunDiskTaskBundleRef(t *testing.T) { + tests := []struct { + name string + taskBundleRef string + wantErrContains string + }{ + { + name: "empty ref is accepted", + taskBundleRef: "", + }, + { + name: "valid digest-pinned ref is accepted", + taskBundleRef: "quay.io/org/task-bundle@sha256:abcdef1234567890", + }, + { + name: "non-digest ref is rejected", + taskBundleRef: "quay.io/org/task-bundle:latest", + wantErrContains: "digest", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + opts := newTestDiskOpts() + *opts.TaskBundleRef = tc.taskBundleRef + + var capturedErr error + opts.HandleError = func(err error) { capturedErr = err } + + h := NewHandler(opts) + h.RunDisk(&cobra.Command{}, []string{"quay.io/test/image:latest"}) + + if tc.wantErrContains != "" { + if capturedErr == nil { + t.Fatalf("expected error containing %q, got nil", tc.wantErrContains) + } + if !strings.Contains(capturedErr.Error(), tc.wantErrContains) { + t.Fatalf("expected error containing %q, got %q", tc.wantErrContains, capturedErr) + } + return + } + if capturedErr != nil { + t.Fatalf("unexpected error: %v", capturedErr) + } + }) + } +}As per coding guidelines (
**/*.go): "Add failing tests before starting implementation."Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build_disk_test.go` at line 49, Add tests that exercise TaskBundleRef/--reproducible behavior for RunDisk: create table-driven cases in build_disk_test.go that call newTestDiskOpts to set TaskBundleRef to an OCI digest-pinned reference and to invalid references, then invoke RunDisk and assert the expected outcomes (e.g., that the correct annotation is emitted on success and that invalid references are rejected with an error). Use the existing test harness helpers in build_disk_test.go, add one failing test case for a valid digest-pinned ref and one for an invalid ref, and verify logs/returned error values from RunDisk to ensure behavior is covered before implementing fixes. Ensure references to TaskBundleRef, newTestDiskOpts, and RunDisk are used so the test targets the right code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 140-141: downloadReferrers is using only username/password for
registry auth, so it ignores sysCtx.AuthFilePath (the --registry-auth-file) used
elsewhere; update downloadReferrers to accept and use the same auth context (or
an auth file path) instead of falling back to username/password only, passing
h.opts.RegistryAuthFile / sysCtx.AuthFilePath into downloadReferrers and using
it when creating remote/registry clients; apply the same change for the other
call sites you noted (the block around lines 391-416) so referrer downloads
honor --registry-auth-file consistently.
- Around line 364-380: The rebuild command omits recorded "aib-extra-args" so
add handling for get("aib-extra-args") and append it to parts as a
"--extra-args" entry; specifically, after the custom-defines loop (or before the
secure/reproducible logic) call v := get("aib-extra-args"), trim it, skip if
empty, and append parts with fmt.Sprintf(" --extra-args %s", v) (or
quote/escape if necessary) so the suggested command includes any extra AIB
flags.
In `@internal/controller/imagebuild/controller.go`:
- Around line 1084-1089: Only include the "task-bundle-ref" param when the build
is being executed as a reproducible (secure) build: change the PipelineRun
params construction that currently adds Name:"task-bundle-ref" with
StringVal:imageBuild.Spec.TaskBundleRef to only add that param when the
reproducible/secure gating is true (the same condition used to choose
PipelineRef from buildConfig.TaskBundleRef). Apply the identical gating to the
legacy push TaskRun params (the block that builds the TaskRun param list), so
both PipelineRun and TaskRun only snapshot imageBuild.Spec.TaskBundleRef when
reproducible builds are enabled.
---
Nitpick comments:
In `@cmd/caib/buildcmd/build_disk_test.go`:
- Around line 121-192: The test TestRunDiskDefaultsToInternalRegistry may
silently pass when an unexpected error is captured; after calling h.RunDisk(...)
in each subtest where tc.wantErrContains is empty assert that capturedErr is nil
(e.g. fail the test if capturedErr != nil) before checking
*opts.UseInternalRegistry so failures in opts.HandleError are surfaced; locate
the test function and add this nil-check immediately after h.RunDisk and before
the UseInternalRegistry assertion.
- Line 49: Add tests that exercise TaskBundleRef/--reproducible behavior for
RunDisk: create table-driven cases in build_disk_test.go that call
newTestDiskOpts to set TaskBundleRef to an OCI digest-pinned reference and to
invalid references, then invoke RunDisk and assert the expected outcomes (e.g.,
that the correct annotation is emitted on success and that invalid references
are rejected with an error). Use the existing test harness helpers in
build_disk_test.go, add one failing test case for a valid digest-pinned ref and
one for an invalid ref, and verify logs/returned error values from RunDisk to
ensure behavior is covered before implementing fixes. Ensure references to
TaskBundleRef, newTestDiskOpts, and RunDisk are used so the test targets the
right code paths.
🪄 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: 8f3d8f70-1dd0-447a-a297-8972fb57aecf
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (19)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/common/build_validation.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (6)
- api/v1alpha1/labels.go
- cmd/caib/main.go
- internal/common/tasks/scripts/build_image.sh
- cmd/caib/registryauth/credentials.go
- cmd/caib/inspectcmd/inspect_test.go
- cmd/caib/common/build_validation.go
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/buildapi/types.go
- internal/common/tasks/tasks.go
- cmd/caib/runtime_wiring.go
- internal/buildapi/server.go
- cmd/caib/buildcmd/build.go
- internal/common/tasks/scripts/push_artifact.sh
Comment on lines
+1084
to
+1089
| { | ||
| Name: "task-bundle-ref", | ||
| Value: tektonv1.ParamValue{ | ||
| Type: tektonv1.ParamTypeString, | ||
| StringVal: imageBuild.Spec.TaskBundleRef, | ||
| }, |
There was a problem hiding this comment.
Only snapshot task-bundle-ref for reproducible builds.
PipelineRef already resolves from buildConfig.TaskBundleRef in secure mode, so sending the same ref through task params affects provenance only. As written, secure-but-non-reproducible builds can still be tagged with a bundle ref and later look reproducible to inspect.
Suggested fix
+ taskBundleRef := ""
+ if imageBuild.Spec.Reproducible {
+ taskBundleRef = imageBuild.Spec.TaskBundleRef
+ }
+
{
Name: "task-bundle-ref",
Value: tektonv1.ParamValue{
Type: tektonv1.ParamTypeString,
- StringVal: imageBuild.Spec.TaskBundleRef,
+ StringVal: taskBundleRef,
},
},Apply the same gating in both the build PipelineRun params and the legacy push TaskRun params.
Also applies to: 1677-1696
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/imagebuild/controller.go` around lines 1084 - 1089, Only
include the "task-bundle-ref" param when the build is being executed as a
reproducible (secure) build: change the PipelineRun params construction that
currently adds Name:"task-bundle-ref" with
StringVal:imageBuild.Spec.TaskBundleRef to only add that param when the
reproducible/secure gating is true (the same condition used to choose
PipelineRef from buildConfig.TaskBundleRef). Apply the identical gating to the
legacy push TaskRun params (the block that builds the TaskRun param list), so
both PipelineRun and TaskRun only snapshot imageBuild.Spec.TaskBundleRef when
reproducible builds are enabled.
f985bd7 to
a06b38c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/caib/buildcmd/build.go (1)
594-610:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RunDisksilently drops--reproducibleinstead of validating or propagating it.Unlike
RunBuildandRunBuildDev, which both callvalidateReproducibleFlags()and setReproducibleon theBuildRequest,RunDiskhere only forwardsTaskBundleRefand never:
- Calls
validateReproducibleFlags(), so--reproduciblewithout--secureand--reproducibletogether with--internal-registryare not rejected at the CLI boundary.- Sets
req.Reproducible = *h.opts.Reproducible, so even if the user passes--reproducible, the flag is silently lost on the way to the server. The server then seesReproducible=falseand never enforces its own checks.If reproducibility is intentionally unsupported for
diskmode (since it builds from an existing container, not an AIB manifest), reject--reproducibleexplicitly with a clear error. Otherwise, propagate it the same way as the other handlers.♻️ Suggested fix (explicit rejection)
if *h.opts.UseInternalRegistry && *h.opts.ExportOCI != "" { h.handleError(fmt.Errorf("--internal-registry cannot be used with --push")) return } + + if *h.opts.Reproducible { + h.handleError(fmt.Errorf("--reproducible is not supported for disk builds")) + return + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build.go` around lines 594 - 610, RunDisk currently omits validation and propagation of the --reproducible flag: call validateReproducibleFlags() from RunDisk (same as RunBuild/RunBuildDev) and set req.Reproducible = *h.opts.Reproducible before submitting the BuildRequest so the server receives the flag; alternatively, if disk mode should not support reproducible builds, explicitly detect *h.opts.Reproducible in RunDisk and return an error explaining that --reproducible is not supported for disk mode (instead of silently dropping it). Ensure the change references the RunDisk handler, validateReproducibleFlags(), the BuildRequest struct, and h.opts.Reproducible.
♻️ Duplicate comments (1)
cmd/caib/inspectcmd/inspect.go (1)
349-382:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAnnotation-derived CLI values are still inserted unquoted into the rebuild command.
A previous review flagged that values from OCI annotations (e.g.,
distro,target,arch,automotive-image-builder,builder-image,export-format, items incustom-defines, items inaib-extra-args) should be shell-quoted before being composed into the suggested rebuild command. The handling here still appends raw values viafmt.Sprintf, so any annotation containing whitespace, quotes,$, backticks, etc. will produce a broken or unsafe command when copy-pasted.♻️ Suggested helper-based fix
+import ( + "strconv" +) + func buildRebuildCommand(annotations map[string]string, referrerTypes map[string]bool) string { get := func(key string) string { return annotations[annotationPrefix+key] } + q := func(v string) string { return strconv.Quote(v) } @@ - if v := get("distro"); v != "" { - parts = append(parts, fmt.Sprintf(" --distro %s", v)) - } - if v := get("target"); v != "" { - parts = append(parts, fmt.Sprintf(" --target %s", v)) - } - if v := get("arch"); v != "" { - parts = append(parts, fmt.Sprintf(" --arch %s", v)) - } - if v := get("automotive-image-builder"); v != "" { - parts = append(parts, fmt.Sprintf(" --aib-image %s", v)) - } - if v := get("builder-image"); v != "" { - parts = append(parts, fmt.Sprintf(" --builder-image %s", v)) - } - if v := get("export-format"); v != "" { - parts = append(parts, fmt.Sprintf(" --format %s", v)) - } + if v := get("distro"); v != "" { + parts = append(parts, fmt.Sprintf(" --distro %s", q(v))) + } + if v := get("target"); v != "" { + parts = append(parts, fmt.Sprintf(" --target %s", q(v))) + } + if v := get("arch"); v != "" { + parts = append(parts, fmt.Sprintf(" --arch %s", q(v))) + } + if v := get("automotive-image-builder"); v != "" { + parts = append(parts, fmt.Sprintf(" --aib-image %s", q(v))) + } + if v := get("builder-image"); v != "" { + parts = append(parts, fmt.Sprintf(" --builder-image %s", q(v))) + } + if v := get("export-format"); v != "" { + parts = append(parts, fmt.Sprintf(" --format %s", q(v))) + } @@ - if def != "" { - parts = append(parts, fmt.Sprintf(" --define %s", def)) + if def != "" { + parts = append(parts, fmt.Sprintf(" --define %s", q(def))) } @@ - if arg != "" { - parts = append(parts, fmt.Sprintf(" --extra-args %s", arg)) + if arg != "" { + parts = append(parts, fmt.Sprintf(" --extra-args %s", q(arg))) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect.go` around lines 349 - 382, The rebuilt command inserts raw annotation values (via get(...) and fmt.Sprintf into parts) which can break or allow injection; add a helper function (e.g., shellQuote or quoteForShell) that safely quotes/escapes a string for POSIX shells (wrap in single quotes and escape existing single quotes using the '\'' pattern) and then replace all uses of fmt.Sprintf that embed v, def, or arg (the occurrences that handle "distro", "target", "arch", "automotive-image-builder", "builder-image", "export-format", items in "custom-defines", and items in "aib-extra-args") to call the helper (e.g., fmt.Sprintf(" --distro %s", shellQuote(v))) so every annotation-derived value appended to parts is shell-quoted.
🧹 Nitpick comments (5)
cmd/caib/buildcmd/build_disk_test.go (2)
49-94: ⚡ Quick winAdd a test verifying
TaskBundleRefis forwarded into theBuildRequest.
newTestDiskOptsnow allocates aTaskBundleRefpointer, but no test in this file actually sets it to a non-empty value and asserts that it lands in the resultingbuildapitypes.BuildRequest.TaskBundleRef. Since the new wiring is the whole point of the change, a focused test would catch silent regressions where the value is dropped (similar to the issue I flagged incmd/caib/buildcmd/build.gofor--reproducible).Per coding guidelines, failing tests should be added before/with the implementation for
**/*.go.As per coding guidelines: "
**/*.go: Add failing tests before starting implementation".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build_disk_test.go` around lines 49 - 94, Add a failing unit test in cmd/caib/buildcmd/build_disk_test.go that sets the TaskBundleRef field on the Options produced by newTestDiskOpts (assign a non-empty string to the TaskBundleRef pointer) and asserts that the resulting buildapitypes.BuildRequest returned by the code path that consumes these options contains the same TaskBundleRef value in BuildRequest.TaskBundleRef; locate where newTestDiskOpts is used to produce a BuildRequest (or call the function that converts Options -> buildapitypes.BuildRequest) and add an assertion there to catch regressions where TaskBundleRef is dropped.
13-94: 💤 Low value
Reproduciblefield not initialized innewTestDiskOpts.
opts.Reproducible *boolis left as the zero value (nil) here.RunDiskcurrently doesn't dereference it (see the related issue I flagged incmd/caib/buildcmd/build.go), but if that handler is updated to validate/forward the flag, this test helper will start panicking with a nil-pointer dereference. Initializing it now is a one-line preemptive fix.♻️ Suggested fix
secureBuild bool + reproducible bool taskBundleRef string @@ SecureBuild: &secureBuild, + Reproducible: &reproducible, TaskBundleRef: &taskBundleRef,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build_disk_test.go` around lines 13 - 94, newTestDiskOpts leaves the Options.Reproducible (*bool) nil which can cause a nil-pointer dereference later; fix by declaring a local bool (e.g. reproducible := false) in newTestDiskOpts and set the returned Options.Reproducible to its address (Reproducible: &reproducible) so the pointer is always non-nil; locate this change in the newTestDiskOpts function and the Options struct usage in build_disk_test.go.cmd/caib/inspectcmd/inspect.go (2)
277-287: 💤 Low valueColor setup is duplicated across CLI handlers.
This
bold/green/yellow/cyaninitialization pattern (no-op funcs, then conditionally replaced withcolor.New(...).SprintFunc()) is also present incmd/caib/buildcmd/build.go(displayBuildResults,displayBuildLogsCommand). Consider extracting a tiny helper incmd/caib/common(e.g.,ColorFuncs(supportsColor bool) struct { Bold, Green, Yellow, Cyan func(...any) string }) to centralize the styling and reduce drift.Not a blocker — only worth doing if more handlers add similar styling.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect.go` around lines 277 - 287, The duplicated color setup in Handler.printProvenance (the local bold/green/yellow/cyan no-op initializers and conditional replacement using h.supportsColor()) should be extracted into a small reusable helper in cmd/caib/common; create e.g. a ColorFuncs(supports bool) returning a struct with Bold, Green, Yellow, Cyan func(...any) string, move the logic currently in printProvenance into that helper, and update printProvenance (and similar functions like displayBuildResults and displayBuildLogsCommand) to call the new ColorFuncs(h.supportsColor()) and use the returned fields instead of in-function duplication.
217-262: ⚖️ Poor tradeoffConsider implementing the Referrers API with tag-schema fallback, but add unit tests first.
The current implementation derives the referrers tag (
sha256-<hex>) and fetches the manifest at that tag only. Per OCI Distribution Spec v1.1.0, registries supporting the Referrers API serve referrers viaGET /v2/<name>/referrers/<digest>and may not maintain the tag-schema fallback. On such registries, this code will silently produce an empty referrer list.Additionally, the
discoverReferrersfunction currently lacks unit tests. Per the coding guidelines for Go files, failing tests should be added before implementation or refactoring.If the target registries are guaranteed to push the referrers tag (as confirmed by the artifact-push implementation), the current approach is acceptable—but this should be documented. Alternatively, implement the Referrers API with automatic fallback to tag-schema, with corresponding test coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect.go` around lines 217 - 262, discoverReferrers currently only fetches the tag-schema referrers (built via referrersTag/referrersRef) which fails on registries that implement the Referrers API; add a proper Referrers API call (GET /v2/<name>/referrers/<digest>) and have discoverReferrers try that first, then fall back to the existing tag-schema lookup (keep repo, referrersTag, referrersRef, ref.NewImageSource and src.GetManifest usage for the fallback path); before changing logic, add unit tests covering: (1) registry that returns referrers via the Referrers API, (2) registry that only exposes tag-schema, and (3) empty/no-referrers case, then implement the API call and fallback behavior and ensure tests pass.cmd/caib/common/oci_artifact.go (1)
21-21: 💤 Low valueVariadic for a single optional value is misleading.
The variadic
authFilePaths ...stringis only ever read at index 0 and silently ignores additional entries. This is unusual idiomatically — callers might reasonably expect that passing multiple paths is meaningful (e.g., fallback chain). Either:
- Convert to a regular
authFilePath stringparameter (and update all call sites — backward compatibility was clearly the motivation here), or- Use a functional-options pattern, or
- Document the contract on the function comment.
Given the breadth of call sites, option 3 is the lowest-effort path:
♻️ Suggested doc-comment fix
-// PullOCIArtifact pulls and extracts an OCI artifact to local destination. +// PullOCIArtifact pulls and extracts an OCI artifact to local destination. +// authFilePaths is treated as an optional single value: only the first non-empty +// element is honored as types.SystemContext.AuthFilePath; additional elements +// are ignored. func PullOCIArtifact(ociRef, destPath, username, password string, insecureSkipTLS bool, authFilePaths ...string) error {Also applies to: 33-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/common/oci_artifact.go` at line 21, The variadic authFilePaths ...string on PullOCIArtifact is misleading because only authFilePaths[0] is used; update the API contract by adding a concise doc comment above PullOCIArtifact explaining that only the first element (authFilePaths[0]) is honored and additional entries are ignored, and do the same for the other functions in this file that use the variadic authFilePaths parameter so callers know this is intentional (alternatively rename to authFilePath string if you prefer to change the signature across call sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/buildapi/server.go`:
- Around line 1183-1195: Add failing unit tests for resolveTaskBundleRef in
server_test.go before modifying implementation: write tests that (1) assert a
user-supplied digest-pinned TaskBundleRef (e.g., image@sha256:<64 hex>) is
accepted and returned unchanged; (2) assert that when req.TaskBundleRef is empty
the function falls back to OperatorConfig.spec.osBuilds.taskBundleRef (mocking a
k8s client to return an OperatorConfig with that field); and (3) assert error
cases: invalid format returns http.StatusBadRequest and missing OperatorConfig
returns a non-nil error; reference resolveTaskBundleRef, BuildRequest struct,
and validateSecureBuild behavior when building your test inputs/mocks.
---
Outside diff comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 594-610: RunDisk currently omits validation and propagation of the
--reproducible flag: call validateReproducibleFlags() from RunDisk (same as
RunBuild/RunBuildDev) and set req.Reproducible = *h.opts.Reproducible before
submitting the BuildRequest so the server receives the flag; alternatively, if
disk mode should not support reproducible builds, explicitly detect
*h.opts.Reproducible in RunDisk and return an error explaining that
--reproducible is not supported for disk mode (instead of silently dropping it).
Ensure the change references the RunDisk handler, validateReproducibleFlags(),
the BuildRequest struct, and h.opts.Reproducible.
---
Duplicate comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 349-382: The rebuilt command inserts raw annotation values (via
get(...) and fmt.Sprintf into parts) which can break or allow injection; add a
helper function (e.g., shellQuote or quoteForShell) that safely quotes/escapes a
string for POSIX shells (wrap in single quotes and escape existing single quotes
using the '\'' pattern) and then replace all uses of fmt.Sprintf that embed v,
def, or arg (the occurrences that handle "distro", "target", "arch",
"automotive-image-builder", "builder-image", "export-format", items in
"custom-defines", and items in "aib-extra-args") to call the helper (e.g.,
fmt.Sprintf(" --distro %s", shellQuote(v))) so every annotation-derived value
appended to parts is shell-quoted.
---
Nitpick comments:
In `@cmd/caib/buildcmd/build_disk_test.go`:
- Around line 49-94: Add a failing unit test in
cmd/caib/buildcmd/build_disk_test.go that sets the TaskBundleRef field on the
Options produced by newTestDiskOpts (assign a non-empty string to the
TaskBundleRef pointer) and asserts that the resulting buildapitypes.BuildRequest
returned by the code path that consumes these options contains the same
TaskBundleRef value in BuildRequest.TaskBundleRef; locate where newTestDiskOpts
is used to produce a BuildRequest (or call the function that converts Options ->
buildapitypes.BuildRequest) and add an assertion there to catch regressions
where TaskBundleRef is dropped.
- Around line 13-94: newTestDiskOpts leaves the Options.Reproducible (*bool) nil
which can cause a nil-pointer dereference later; fix by declaring a local bool
(e.g. reproducible := false) in newTestDiskOpts and set the returned
Options.Reproducible to its address (Reproducible: &reproducible) so the pointer
is always non-nil; locate this change in the newTestDiskOpts function and the
Options struct usage in build_disk_test.go.
In `@cmd/caib/common/oci_artifact.go`:
- Line 21: The variadic authFilePaths ...string on PullOCIArtifact is misleading
because only authFilePaths[0] is used; update the API contract by adding a
concise doc comment above PullOCIArtifact explaining that only the first element
(authFilePaths[0]) is honored and additional entries are ignored, and do the
same for the other functions in this file that use the variadic authFilePaths
parameter so callers know this is intentional (alternatively rename to
authFilePath string if you prefer to change the signature across call sites).
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 277-287: The duplicated color setup in Handler.printProvenance
(the local bold/green/yellow/cyan no-op initializers and conditional replacement
using h.supportsColor()) should be extracted into a small reusable helper in
cmd/caib/common; create e.g. a ColorFuncs(supports bool) returning a struct with
Bold, Green, Yellow, Cyan func(...any) string, move the logic currently in
printProvenance into that helper, and update printProvenance (and similar
functions like displayBuildResults and displayBuildLogsCommand) to call the new
ColorFuncs(h.supportsColor()) and use the returned fields instead of in-function
duplication.
- Around line 217-262: discoverReferrers currently only fetches the tag-schema
referrers (built via referrersTag/referrersRef) which fails on registries that
implement the Referrers API; add a proper Referrers API call (GET
/v2/<name>/referrers/<digest>) and have discoverReferrers try that first, then
fall back to the existing tag-schema lookup (keep repo, referrersTag,
referrersRef, ref.NewImageSource and src.GetManifest usage for the fallback
path); before changing logic, add unit tests covering: (1) registry that returns
referrers via the Referrers API, (2) registry that only exposes tag-schema, and
(3) empty/no-referrers case, then implement the API call and fallback behavior
and ensure tests pass.
🪄 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: ba1a57ce-49d6-4b9f-b5bb-b1a4798b07f1
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/common/build_validation.gocmd/caib/common/oci_artifact.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (5)
- cmd/caib/registryauth/credentials.go
- cmd/caib/main.go
- api/v1alpha1/labels.go
- internal/common/tasks/scripts/build_image.sh
- cmd/caib/inspectcmd/inspect_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/buildapi/types.go
- cmd/caib/common/build_validation.go
- cmd/export-tasks/main.go
- internal/common/tasks/tasks.go
- internal/common/tasks/pipeline_test.go
- internal/common/tasks/scripts/push_artifact.sh
- cmd/caib/image/image.go
- internal/controller/imagebuild/controller.go
Comment on lines
+1183
to
+1195
| func resolveTaskBundleRef(ctx context.Context, k8sClient client.Client, namespace string, req *BuildRequest) (string, int, error) { | ||
| if !req.SecureBuild { | ||
| return "", 0, nil | ||
| } | ||
| if req.TaskBundleRef != "" { | ||
| ref := strings.TrimSpace(req.TaskBundleRef) | ||
| if !digestPinnedRef.MatchString(ref) { | ||
| return "", http.StatusBadRequest, fmt.Errorf("taskBundleRef must be digest-pinned (image@sha256:<64 hex>), got %q", ref) | ||
| } | ||
| return ref, 0, nil | ||
| } | ||
| return validateSecureBuild(ctx, k8sClient, namespace) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
OCI Tekton bundle pinning best practices for supply-chain security: should clients be allowed to override an operator-defined task bundle ref?
💡 Result:
No, clients should not be allowed to override operator-defined task bundle refs for supply-chain security in OCI Tekton bundles. Best practices emphasize pinning bundles by digest for immutability and reproducibility [1][2][3]. Operator-defined bundles (e.g., via TektonConfig or operator bundles) ensure controlled, verified resources [4][5][6]. Allowing overrides introduces supply-chain risks by enabling unverified or malicious bundles, undermining pinning [7][8]. Use Tekton's Trusted Resources feature with VerificationPolicies to enforce signature verification on remote refs (bundles, git, hub); set trusted-resources-verification-no-match-policy to "fail" or "warn" in feature-flags ConfigMap to block untrusted refs [7][8][9]. Chains provides separate provenance signing but doesn't prevent ref overrides [10][11]. Disable unnecessary resolvers (enable-bundles-resolver: false) if strict control needed [6][12]. In OpenShift Pipelines (OCI context), operator manages secure defaults; custom overrides risk compliance [13][6].
Citations:
- 1: https://github.com/tektoncd/community/blob/main/teps/0005-tekton-oci-bundles.md
- 2: https://mintlify.com/tektoncd/pipeline/resolvers/bundle-resolver
- 3: https://tekton.dev/docs/pipelines/bundle-resolver
- 4: https://github.com/tektoncd/operator/blob/main/operatorhub/tools/CONFIG-DEFINITION.md
- 5: https://github.com/tektoncd/operator/blob/main/docs/TektonConfig.md
- 6: https://docs.redhat.com/en/documentation/red_hat_openshift_pipelines/1.15/html/installing_and_configuring/customizing-configurations-in-the-tektonconfig-cr
- 7: https://tekton.dev/docs/pipelines/trusted-resources
- 8: https://github.com/tektoncd/community/blob/main/teps/0091-trusted-resources.md
- 9: https://mintlify.com/tektoncd/pipeline/api/beta/verificationpolicy
- 10: https://tekton.dev/docs/chains/
- 11: https://docs.redhat.com/en/documentation/red_hat_openshift_pipelines/1.11/html/securing_openshift_pipelines/using-tekton-chains-for-openshift-pipelines-supply-chain-security
- 12: https://tekton.dev/docs/pipelines/additional-configs
- 13: https://docs.openshift.com/container-platform/4.13/cicd/pipelines/using-tekton-chains-for-openshift-pipelines-supply-chain-security.html
🏁 Script executed:
# Search for RBAC or permission checks related to TaskBundleRef
rg -i "taskbundleref|task-bundle" --type go -A 3 -B 3 | head -100Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 6828
🏁 Script executed:
# Find validateSecureBuild function to understand fallback behavior
ast-grep --pattern $'func validateSecureBuild($$$) {
$$$
}'Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Search for OperatorConfig and how it's used for bundle validation
rg "OperatorConfig|\.spec\.osBuilds" --type go -A 2 -B 2 | head -80Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 5242
🏁 Script executed:
# Check if there's any comparison/validation of user-provided vs operator-defined bundles
rg "taskBundleRef.*OperatorConfig|OperatorConfig.*taskBundleRef" --type go -A 5 -B 5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 7384
🏁 Script executed:
# Search for comments or documentation about TaskBundleRef override behavior
rg "override|bypass|allow.*taskBundleRef" --type go -i -A 2 -B 2Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 50404
🏁 Script executed:
# Search for tests related to resolveTaskBundleRef or TaskBundleRef
find . -name "*test.go" -type f | xargs grep -l "resolveTaskBundleRef\|TaskBundleRef" 2>/dev/null | head -20Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 171
🏁 Script executed:
# Check if resolveTaskBundleRef has any tests
rg "resolveTaskBundleRef|TestResolveTaskBundleRef" --type go -A 5 -B 2Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1211
🏁 Script executed:
# Look for git changes to understand if this is new code
git log --oneline --all internal/buildapi/server.go 2>/dev/null | head -5Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 363
🏁 Script executed:
# Check line count of the server.go file to understand its size
wc -l internal/buildapi/server.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 124
🏁 Script executed:
# Search for test files in buildapi directory
find ./internal/buildapi -name "*test.go" -type fRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 850
🏁 Script executed:
# Search for any tests that might test createBuild (which calls resolveTaskBundleRef)
rg "func Test.*[Cc]reate.*[Bb]uild|func Test.*[Bb]uild" ./internal/buildapi --type goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 249
🏁 Script executed:
# Check if there are any test files in buildapi at all
ls -la ./internal/buildapi/ | grep -i testRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 1348
🏁 Script executed:
# Check server_test.go for tests related to resolveTaskBundleRef or TaskBundleRef
rg "resolveTaskBundleRef|TestTaskBundleRef|TestSecureBuild" ./internal/buildapi/server_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Look at what tests are in server_test.go
rg "^func Test" ./internal/buildapi/server_test.go | head -30Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check if there are tests for the createBuild function
rg "createBuild|TestCreateBuild" ./internal/buildapi/server_test.go -A 3Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# List all tests in server_test.go
grep "^func Test" ./internal/buildapi/server_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Check the content of server_test.go to understand its scope
head -100 ./internal/buildapi/server_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 2939
🏁 Script executed:
# Search for SecureBuild or TaskBundleRef tests in server_test.go
grep -n "SecureBuild\|TaskBundleRef\|resolveTaskBundle" ./internal/buildapi/server_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 73
🏁 Script executed:
# Count total test functions in server_test.go
grep -c "^\s*It(" ./internal/buildapi/server_test.goRepository: centos-automotive-suite/automotive-dev-operator
Length of output: 94
🏁 Script executed:
# Get all context/describe blocks to understand test coverage
grep -E "^\s*(Context|Describe|It)\(" ./internal/buildapi/server_test.go | head -40Repository: centos-automotive-suite/automotive-dev-operator
Length of output: 1860
Add failing tests for resolveTaskBundleRef before implementation (coding guidelines).
The function was added without corresponding tests in server_test.go. Per coding guidelines for **.go files, add failing tests before starting implementation. Tests should cover:
- User-supplied digest-pinned
TaskBundleRefvalidation - Fallback to
OperatorConfig.spec.osBuilds.taskBundleRefwhen user ref is empty - Error cases (invalid format, missing OperatorConfig)
Secondary note on security design
The function allows user-supplied digest-pinned bundles to override operator-mandated ones. While this enables reproducible rebuilds, it contradicts Tekton supply-chain security best practices (Trusted Resources, verification policies). If this override is intentional, document the security implications and consider gating it behind explicit RBAC/allow-list checks per your organization's supply-chain policy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/buildapi/server.go` around lines 1183 - 1195, Add failing unit tests
for resolveTaskBundleRef in server_test.go before modifying implementation:
write tests that (1) assert a user-supplied digest-pinned TaskBundleRef (e.g.,
image@sha256:<64 hex>) is accepted and returned unchanged; (2) assert that when
req.TaskBundleRef is empty the function falls back to
OperatorConfig.spec.osBuilds.taskBundleRef (mocking a k8s client to return an
OperatorConfig with that field); and (3) assert error cases: invalid format
returns http.StatusBadRequest and missing OperatorConfig returns a non-nil
error; reference resolveTaskBundleRef, BuildRequest struct, and
validateSecureBuild behavior when building your test inputs/mocks.
18c4cdd to
563b9bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/common/tasks/scripts/push_artifact.sh (1)
320-339:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't persist raw
custom-defines/aib-extra-argsin OCI annotations.These fields are arbitrary user input, and this change now stores them in artifact metadata for every push. That permanently exposes anything passed via
--defineor--extra-argsto anyone who can read the artifact, and in the multi-layer path the same values also get echoed into task logs via the annotation file dump. Please whitelist only known-safe values or move rebuild-only details into a separate downloaded artifact instead of manifest annotations.Also applies to: 401-420
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/common/tasks/scripts/push_artifact.sh` around lines 320 - 339, The Python snippet in push_artifact.sh currently stores raw custom_defs and extra_args into OCI annotations (keys "automotive.sdv.cloud.redhat.com/custom-defines" and "automotive.sdv.cloud.redhat.com/aib-extra-args"), which exposes arbitrary user input; modify the code so it does not persist raw values: either remove adding those keys entirely or only add them after validating against an allowlist of safe keys/values, or alternatively move the full raw strings into a separate downloadable artifact used for rebuilds (create a task-bundle/aux-artifact) and keep only a non-sensitive reference in annotations; update the variables custom_defs and extra_args handling in the Python block that builds dict a accordingly and ensure any remaining annotation contains only sanitized/whitelisted data or a reference token.cmd/caib/buildcmd/build.go (1)
596-612:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
RunDiskdrops--reproducibleand skips its validation path.This request literal never copies
*h.opts.Reproducible, socaib image disksilently creates a non-reproducible build even when the flag is set. Because this path also never callsvalidateReproducibleFlags(), the implicit internal-registry default can mask an invalid reproducible request instead of failing fast.💡 Minimal fix
func (h *Handler) RunDisk(cmd *cobra.Command, args []string) { h.applyWaitFollowDefaults(cmd, false, false) @@ if *h.opts.UseInternalRegistry && *h.opts.ExportOCI != "" { h.handleError(fmt.Errorf("--internal-registry cannot be used with --push")) return } + + if err := h.validateReproducibleFlags(); err != nil { + h.handleError(err) + return + } @@ req := buildapitypes.BuildRequest{ Name: *h.opts.BuildName, ContainerRef: containerRef, Distro: buildapitypes.Distro(*h.opts.Distro), Target: buildapitypes.Target(*h.opts.Target), Architecture: buildapitypes.Architecture(*h.opts.Architecture), ExportFormat: buildapitypes.ExportFormat(*h.opts.DiskFormat), Mode: buildapitypes.ModeDisk, AutomotiveImageBuilder: *h.opts.AutomotiveImageBuilder, StorageClass: *h.opts.StorageClass, AIBExtraArgs: *h.opts.AIBExtraArgs, Compression: buildapitypes.Compression(*h.opts.CompressionAlgo), ExportOCI: *h.opts.ExportOCI, SecureBuild: *h.opts.SecureBuild, + Reproducible: *h.opts.Reproducible, TaskBundleRef: *h.opts.TaskBundleRef, RestoreSourcesRef: *h.opts.RestoreSourcesRef, TTL: *h.opts.TTL, }Please add a
RunDiskregression case for--reproduciblewhile touching this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/buildcmd/build.go` around lines 596 - 612, RunDisk is not propagating the --reproducible flag into the BuildRequest and it also skips validateReproducibleFlags(), causing reproducible builds to be silently disabled and validation to be skipped; fix by adding Reproducible: *h.opts.Reproducible to the buildapitypes.BuildRequest literal in RunDisk and invoke validateReproducibleFlags(h.opts) (or the existing validateReproducibleFlags call used elsewhere) before creating the request so invalid combinations fail fast, and add a RunDisk regression test that runs the command with --reproducible (and cases for the implicit internal-registry default) to assert the request.Reproducible is true and validation behavior is exercised.internal/buildapi/server.go (1)
1663-1680:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBuild templates drop the pinned reproducibility refs.
createBuildstores bothTaskBundleRefandRestoreSourcesRefinImageBuildSpec, but this template reconstruction only returnsReproducible. Any rebuild driven from/templateorcaib image inspectwill lose those pinned refs and can no longer reproduce the original inputs exactly.💡 Minimal fix
BuildRequest: BuildRequest{ Name: build.Name, Manifest: manifest, ManifestFileName: manifestFileName, Distro: Distro(build.Spec.GetDistro()), Target: Target(build.Spec.GetTarget()), Architecture: Architecture(build.Spec.Architecture), ExportFormat: ExportFormat(build.Spec.GetExportFormat()), Mode: Mode(build.Spec.GetMode()), AutomotiveImageBuilder: build.Spec.GetAIBImage(), CustomDefs: build.Spec.GetCustomDefs(), AIBExtraArgs: build.Spec.GetAIBExtraArgs(), Compression: Compression(build.Spec.GetCompression()), SecureBuild: build.Spec.SecureBuild, Reproducible: build.Spec.Reproducible, + TaskBundleRef: build.Spec.TaskBundleRef, + RestoreSourcesRef: build.Spec.RestoreSourcesRef, TTL: build.Spec.GetTTL(), },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/buildapi/server.go` around lines 1663 - 1680, The template response omits pinned reproducibility refs so rebuilds lose exact inputs; update the BuildRequest construction in the BuildTemplateResponse (inside server.go where BuildTemplate is assembled) to include the ImageBuildSpec's TaskBundleRef and RestoreSourcesRef (e.g., pull build.Spec.TaskBundleRef and build.Spec.RestoreSourcesRef into corresponding fields on BuildRequest) so the /template and caib image inspect output preserve those pinned refs for reproducibility.
♻️ Duplicate comments (3)
internal/common/tasks/scripts/build_image.sh (1)
904-915:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t publish the build manifest without sanitizing it first.
This
cpreintroduces the raw-manifest leak: any secrets or credentials embedded in the input manifest will end up inaib-manifest.ymland get attached as a reproducibility referrer. Please route the manifest through the same sanitization/redaction step here and fail closed if sanitization cannot run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/common/tasks/scripts/build_image.sh` around lines 904 - 915, Replace the direct copy of the raw manifest (cp "$MANIFEST_FILE" "$WORKSPACE_PATH/aib-manifest.yml") with a call to the manifest sanitization/redaction step and write the sanitized output to "$WORKSPACE_PATH/aib-manifest.yml"; ensure you invoke the existing sanitizer (or a new sanitize_manifest utility) on MANIFEST_FILE, check its exit status and fail closed (exit non-zero) if sanitization fails, and do not proceed to save or publish the untranslated MANIFEST_FILE. Use the same variable names (MANIFEST_FILE and "$WORKSPACE_PATH/aib-manifest.yml") so reviewers can locate and verify the change.internal/controller/imagebuild/controller.go (1)
1084-1089:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly propagate
task-bundle-refwhen the build is actually reproducible.
internal/common/tasks/scripts/push_artifact.shsnapshots this param into artifact annotations, andcmd/caib/inspectcmd/inspect.gotreats any recordedtask-bundle-refas enough to emit--secure --reproducible. With the current unconditional wiring, secure-but-non-reproducible builds will be reported bycaib image inspectas reproducible even though no reproducibility snapshot was requested. Gate this param onSpec.Reproduciblein both paths.Also applies to: 1698-1703
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/imagebuild/controller.go` around lines 1084 - 1089, The code currently always injects the Tekton Param named "task-bundle-ref" using imageBuild.Spec.TaskBundleRef; change both sites (the block creating the Param at the current location and the similar block around lines 1698-1703) to only append/emit that param when imageBuild.Spec.Reproducible is true. Concretely, wrap the creation/assignment of the "task-bundle-ref" tektonv1.ParamValue (and any code that reads imageBuild.Spec.TaskBundleRef) in a conditional check for imageBuild.Spec.Reproducible so the Param is not added for non-reproducible builds (keep the existing field names imageBuild.Spec.TaskBundleRef and the Param.Name "task-bundle-ref").cmd/caib/inspectcmd/inspect.go (1)
349-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShell-quote annotation-derived CLI values in the rebuild command.
Values from OCI annotations are still appended raw here. Any spaces, quotes, or leading
-indistro,target,--define,--extra-args, or--task-bundle-refwill produce a broken or misleading copy-paste command.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect.go` around lines 349 - 387, The code appends annotation-derived values into the rebuild command raw (see uses of get("distro"), get("target"), get("arch"), get("automotive-image-builder"), get("builder-image"), get("export-format"), the loop handling get("custom-defines") and get("aib-extra-args"), and taskBundleRef) which can break shell parsing; update the assembly logic to run every annotation value through a proper shell-quoting routine before appending to parts (including per-define and per-extra-arg entries) so spaces, quotes, or leading "-" are safely quoted and preserved when copied as a command, and replace the raw fmt.Sprintf calls that append to parts with ones that use the quoted value.
🧹 Nitpick comments (1)
internal/common/tasks/pipeline_test.go (1)
293-319: ⚡ Quick winAdd
restore-sources-refto this generation-test matrix.
internal/common/tasks/tasks.gonow exposes and forwardsrestore-sources-ref, but this suite only locks downreproducibleand the push-task params. A rename/removal ofrestore-sources-refwould still pass here, so it’s worth asserting its presence, default, andbuild-imagebinding alongside the other new params.As per coding guidelines,
**/*.go: Add failing tests before starting implementation.Also applies to: 347-353, 395-439
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/common/tasks/pipeline_test.go` around lines 293 - 319, Update the pipeline generation tests to include the new "restore-sources-ref" param: add "restore-sources-ref" to the required params slice in TestReproducibleParams_Pipeline (function GenerateTektonPipeline, type BuildConfig) and in TestReproducibleParams_BuildImageBinding also assert that build-image task has a binding for "restore-sources-ref" with value "$(params.restore-sources-ref)" using findPipelineTask and taskParamBinding helpers; replicate analogous assertions in the other related test blocks mentioned (around the other ranges) so presence, default and build-image binding for restore-sources-ref are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/caib/common/build_validation.go`:
- Around line 51-54: Update the error returned by
ValidateReproducibleRequiresSecure to accurately state the invariant: when
reproducible is true and secureBuild is false return an error with the message
"--reproducible requires --secure" (replace the current longer message about
task bundle pinning). Locate the function ValidateReproducibleRequiresSecure and
change the fmt.Errorf call to use the shorter, precise message.
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 223-252: discoverReferrers currently skips the OCI 1.1 Referrers
API and always uses the tag-schema fallback; change discoverReferrers to first
call the OCI Referrers API (the registry /v2/<name>/referrers?digest=<digest>
path) and parse its response into the index/Manifests structure, and only when
that API returns a 404 fall back to the existing tag-schema logic that builds
referrersTag/referrersRef and calls
docker.ParseReference/NewImageSource/GetManifest; ensure other HTTP errors
propagate as errors. Also add failing unit tests for discoverReferrers per
project guidelines: one test that mocks a registry returning a valid OCI 1.1
referrers JSON (assert manifests are returned) and one that mocks a 404 from the
referrers API to assert the code falls back to the tag schema behavior.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 147-149: The tar extraction currently trusts the user-controlled
SOURCES_ARCHIVE (restore-sources-ref) and can write absolute or ../ paths into
BUILD_DIR/osbuild_store; before running tar -xzf, inspect the archive (tar -tzf
"$SOURCES_ARCHIVE") and validate every member does not start with '/' and does
not contain '..' segments (reject or fail on any invalid entry), then extract
into RESTORE_TMPDIR (already used) and move/sync only validated files into
"$BUILD_DIR/osbuild_store"; keep the final rm -rf "$RESTORE_TMPDIR" cleanup.
Ensure checks reference SOURCES_ARCHIVE, RESTORE_TMPDIR and
BUILD_DIR/osbuild_store so reviewers can find the changes.
---
Outside diff comments:
In `@cmd/caib/buildcmd/build.go`:
- Around line 596-612: RunDisk is not propagating the --reproducible flag into
the BuildRequest and it also skips validateReproducibleFlags(), causing
reproducible builds to be silently disabled and validation to be skipped; fix by
adding Reproducible: *h.opts.Reproducible to the buildapitypes.BuildRequest
literal in RunDisk and invoke validateReproducibleFlags(h.opts) (or the existing
validateReproducibleFlags call used elsewhere) before creating the request so
invalid combinations fail fast, and add a RunDisk regression test that runs the
command with --reproducible (and cases for the implicit internal-registry
default) to assert the request.Reproducible is true and validation behavior is
exercised.
In `@internal/buildapi/server.go`:
- Around line 1663-1680: The template response omits pinned reproducibility refs
so rebuilds lose exact inputs; update the BuildRequest construction in the
BuildTemplateResponse (inside server.go where BuildTemplate is assembled) to
include the ImageBuildSpec's TaskBundleRef and RestoreSourcesRef (e.g., pull
build.Spec.TaskBundleRef and build.Spec.RestoreSourcesRef into corresponding
fields on BuildRequest) so the /template and caib image inspect output preserve
those pinned refs for reproducibility.
In `@internal/common/tasks/scripts/push_artifact.sh`:
- Around line 320-339: The Python snippet in push_artifact.sh currently stores
raw custom_defs and extra_args into OCI annotations (keys
"automotive.sdv.cloud.redhat.com/custom-defines" and
"automotive.sdv.cloud.redhat.com/aib-extra-args"), which exposes arbitrary user
input; modify the code so it does not persist raw values: either remove adding
those keys entirely or only add them after validating against an allowlist of
safe keys/values, or alternatively move the full raw strings into a separate
downloadable artifact used for rebuilds (create a task-bundle/aux-artifact) and
keep only a non-sensitive reference in annotations; update the variables
custom_defs and extra_args handling in the Python block that builds dict a
accordingly and ensure any remaining annotation contains only
sanitized/whitelisted data or a reference token.
---
Duplicate comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 349-387: The code appends annotation-derived values into the
rebuild command raw (see uses of get("distro"), get("target"), get("arch"),
get("automotive-image-builder"), get("builder-image"), get("export-format"), the
loop handling get("custom-defines") and get("aib-extra-args"), and
taskBundleRef) which can break shell parsing; update the assembly logic to run
every annotation value through a proper shell-quoting routine before appending
to parts (including per-define and per-extra-arg entries) so spaces, quotes, or
leading "-" are safely quoted and preserved when copied as a command, and
replace the raw fmt.Sprintf calls that append to parts with ones that use the
quoted value.
In `@internal/common/tasks/scripts/build_image.sh`:
- Around line 904-915: Replace the direct copy of the raw manifest (cp
"$MANIFEST_FILE" "$WORKSPACE_PATH/aib-manifest.yml") with a call to the manifest
sanitization/redaction step and write the sanitized output to
"$WORKSPACE_PATH/aib-manifest.yml"; ensure you invoke the existing sanitizer (or
a new sanitize_manifest utility) on MANIFEST_FILE, check its exit status and
fail closed (exit non-zero) if sanitization fails, and do not proceed to save or
publish the untranslated MANIFEST_FILE. Use the same variable names
(MANIFEST_FILE and "$WORKSPACE_PATH/aib-manifest.yml") so reviewers can locate
and verify the change.
In `@internal/controller/imagebuild/controller.go`:
- Around line 1084-1089: The code currently always injects the Tekton Param
named "task-bundle-ref" using imageBuild.Spec.TaskBundleRef; change both sites
(the block creating the Param at the current location and the similar block
around lines 1698-1703) to only append/emit that param when
imageBuild.Spec.Reproducible is true. Concretely, wrap the creation/assignment
of the "task-bundle-ref" tektonv1.ParamValue (and any code that reads
imageBuild.Spec.TaskBundleRef) in a conditional check for
imageBuild.Spec.Reproducible so the Param is not added for non-reproducible
builds (keep the existing field names imageBuild.Spec.TaskBundleRef and the
Param.Name "task-bundle-ref").
---
Nitpick comments:
In `@internal/common/tasks/pipeline_test.go`:
- Around line 293-319: Update the pipeline generation tests to include the new
"restore-sources-ref" param: add "restore-sources-ref" to the required params
slice in TestReproducibleParams_Pipeline (function GenerateTektonPipeline, type
BuildConfig) and in TestReproducibleParams_BuildImageBinding also assert that
build-image task has a binding for "restore-sources-ref" with value
"$(params.restore-sources-ref)" using findPipelineTask and taskParamBinding
helpers; replicate analogous assertions in the other related test blocks
mentioned (around the other ranges) so presence, default and build-image binding
for restore-sources-ref are enforced.
🪄 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: 12c4b962-8db6-46bc-bbe4-1fdfb284e2f5
⛔ Files ignored due to path filters (1)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**
📒 Files selected for processing (20)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/common/build_validation.gocmd/caib/common/oci_artifact.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gointernal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/caib/inspectcmd/inspect_test.go
Comment on lines
+51
to
+54
| func ValidateReproducibleRequiresSecure(reproducible, secureBuild bool) error { | ||
| if reproducible && !secureBuild { | ||
| return fmt.Errorf("--reproducible requires --secure for task bundle pinning") | ||
| } |
There was a problem hiding this comment.
Make the error describe the actual invariant.
This helper rejects every reproducible build without secure mode, but the current text makes it sound like only task-bundle pinning is blocked. A shorter message like --reproducible requires --secure is less misleading for CLI users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/caib/common/build_validation.go` around lines 51 - 54, Update the error
returned by ValidateReproducibleRequiresSecure to accurately state the
invariant: when reproducible is true and secureBuild is false return an error
with the message "--reproducible requires --secure" (replace the current longer
message about task bundle pinning). Locate the function
ValidateReproducibleRequiresSecure and change the fmt.Errorf call to use the
shorter, precise message.
Comment on lines
+147
to
+149
| mkdir -p "$BUILD_DIR/osbuild_store" | ||
| tar -xzf "$SOURCES_ARCHIVE" -C "$BUILD_DIR/osbuild_store" | ||
| rm -rf "$RESTORE_TMPDIR" |
There was a problem hiding this comment.
Validate archive paths before extracting referrer contents.
restore-sources-ref is user-controlled input, and tar -xzf will happily honor absolute paths or ../ entries. In this privileged task, a malicious sources artifact can write outside $BUILD_DIR/osbuild_store before the build even starts.
Suggested guard
if [ -z "$SOURCES_ARCHIVE" ]; then
echo "ERROR: No archive found after pulling sources referrer" >&2
exit 1
fi
+ if tar -tzf "$SOURCES_ARCHIVE" | grep -Eq '(^/|(^|/)\.\.(/|$))'; then
+ echo "ERROR: Sources archive contains unsafe paths" >&2
+ exit 1
+ fi
+
mkdir -p "$BUILD_DIR/osbuild_store"
tar -xzf "$SOURCES_ARCHIVE" -C "$BUILD_DIR/osbuild_store"
rm -rf "$RESTORE_TMPDIR"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/common/tasks/scripts/build_image.sh` around lines 147 - 149, The tar
extraction currently trusts the user-controlled SOURCES_ARCHIVE
(restore-sources-ref) and can write absolute or ../ paths into
BUILD_DIR/osbuild_store; before running tar -xzf, inspect the archive (tar -tzf
"$SOURCES_ARCHIVE") and validate every member does not start with '/' and does
not contain '..' segments (reject or fail on any invalid entry), then extract
into RESTORE_TMPDIR (already used) and move/sync only validated files into
"$BUILD_DIR/osbuild_store"; keep the final rm -rf "$RESTORE_TMPDIR" cleanup.
Ensure checks reference SOURCES_ARCHIVE, RESTORE_TMPDIR and
BUILD_DIR/osbuild_store so reviewers can find the changes.
563b9bd to
e3a7a7b
Compare
Contributor
Author
|
@ambient-code please review |
![ambient-code[bot]](https://avatars.githubusercontent.com/u/231559327?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good feature addition — reproducible builds with OCI referrers and the inspect subcommand are well designed. The validation is layered correctly (CLI, API, CRD CEL rule), test coverage for inspect is thorough, and the pipelinePassthroughParams() refactor is a clean reduction of boilerplate. A few items to consider below.
Minor: PR title has a typo — "insepct" should be "inspect". Worth fixing before merge since it'll be in git history.
| ORAS_BASE_URL="https://github.com/oras-project/oras/releases/download/v${ORAS_VERSION}" | ||
| ORAS_CHECKSUMS="oras_${ORAS_VERSION}_checksums.txt" | ||
| curl -sLO "${ORAS_BASE_URL}/${ORAS_TARBALL}" | ||
| curl -sLO "${ORAS_BASE_URL}/${ORAS_CHECKSUMS}" |
There was a problem hiding this comment.
Downloading oras from GitHub at build time introduces a runtime dependency on an external network resource. If GitHub is unreachable, rate-limited, or the release is removed/yanked, all restore-sources builds will fail.
Consider pre-installing oras in the builder image (or the build-automotive-image task's base image) instead. That way the binary is baked in and verified once at image-build time rather than on every build invocation.
Not a blocker for the initial merge, but worth tracking as a follow-up to harden the flow.
Contributor
There was a problem hiding this comment.
I tend to agree on this one, it can be a bit flaky adding the oras hardcoded like this, but I'm not sure if we can add it to the image somehow...
Contributor
Author
There was a problem hiding this comment.
we can't it would have to be in automotive-image-builder...
Contributor
There was a problem hiding this comment.
yeah, I wasn't sure about that, so I guess we don't have other options...
--reproducible saves a snapshot of the RPMs used for the build, so they can be reused later. `caib image inspect` would show the details of an OCI artifact with rebuild instructions Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com> Assisted-by: claude-opus-4.6
bennyz
force-pushed
the
reproducible-builds
branch
from
e3a7a7b to
875ac34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/caib/inspectcmd/inspect.go (1)
248-256: 💤 Low valueSet
MinVersion: tls.VersionTLS12on the insecure transport.Per static-analysis hint (
missing-ssl-minversion-go). The user has opted out of certificate verification, but there's no reason to also let the connection negotiate TLS 1.0/1.1. A one-line addition closes that downgrade window.♻️ Proposed fix
if sysCtx.DockerInsecureSkipTLSVerify == types.OptionalBoolTrue { authClient.Client = &http.Client{ Transport: &http.Transport{ TLSClientConfig: &tls.Config{ + MinVersion: tls.VersionTLS12, InsecureSkipVerify: true, //nolint:gosec // user explicitly opted in }, }, } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/caib/inspectcmd/inspect.go` around lines 248 - 256, The insecure TLS transport created when sysCtx.DockerInsecureSkipTLSVerify == types.OptionalBoolTrue should explicitly set a minimum TLS version to prevent downgrades: update the tls.Config used for authClient.Client's http.Transport (the block that assigns authClient.Client) to include MinVersion: tls.VersionTLS12 in addition to InsecureSkipVerify so the connection cannot negotiate TLS 1.0/1.1.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/common/tasks/pipeline_test.go`:
- Around line 395-416: TestPipelineParamSpec_Defaults (and the similar test
around lines 418-439) only checks params that exist today; add a seen map keyed
by the expected param names (the wantDefaults map) and mark entries as seen when
iterating pipeline.Spec.Params (using p.Name), then after the loop fail the test
if any expected key in wantDefaults was not seen so removal of a param causes
the test to fail; update the same pattern in the companion test that covers the
other default values to ensure missing params are detected.
---
Nitpick comments:
In `@cmd/caib/inspectcmd/inspect.go`:
- Around line 248-256: The insecure TLS transport created when
sysCtx.DockerInsecureSkipTLSVerify == types.OptionalBoolTrue should explicitly
set a minimum TLS version to prevent downgrades: update the tls.Config used for
authClient.Client's http.Transport (the block that assigns authClient.Client) to
include MinVersion: tls.VersionTLS12 in addition to InsecureSkipVerify so the
connection cannot negotiate TLS 1.0/1.1.
🪄 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: ed3c7b50-cfd6-4833-96ee-c864b55a006d
⛔ Files ignored due to path filters (55)
config/crd/bases/automotive.sdv.cloud.redhat.com_imagebuilds.yamlis excluded by!config/crd/bases/**go.sumis excluded by!**/*.sum,!**go.sumvendor/modules.txtis excluded by!vendor/**vendor/oras.land/oras-go/v2/LICENSEis excluded by!vendor/**vendor/oras.land/oras-go/v2/content/descriptor.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/content/graph.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/content/limitedstorage.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/content/reader.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/content/resolver.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/content/storage.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/errdef/errors.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/cas/memory.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/cas/proxy.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/descriptor/descriptor.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/docker/mediatype.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/httputil/seek.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/ioutil/io.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/spec/artifact.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/syncutil/limit.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/syncutil/limitgroup.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/syncutil/merge.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/syncutil/once.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/internal/syncutil/pool.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/reference.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/registry.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/auth/cache.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/auth/challenge.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/auth/client.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/auth/credential.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/auth/scope.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/file_store.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/internal/config/config.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/internal/executer/executer.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/internal/ioutil/ioutil.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/memory_store.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/native_store.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/native_store_darwin.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/native_store_generic.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/native_store_linux.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/native_store_windows.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/registry.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/store.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/credentials/trace/trace.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/errcode/errors.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/internal/errutil/errutil.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/manifest.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/referrers.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/registry.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/repository.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/retry/client.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/retry/policy.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/url.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/utils.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/remote/warning.gois excluded by!vendor/**vendor/oras.land/oras-go/v2/registry/repository.gois excluded by!vendor/**
📒 Files selected for processing (21)
api/v1alpha1/imagebuild_types.goapi/v1alpha1/labels.gocmd/caib/buildcmd/build.gocmd/caib/buildcmd/build_disk_test.gocmd/caib/common/build_validation.gocmd/caib/common/oci_artifact.gocmd/caib/image/image.gocmd/caib/inspectcmd/inspect.gocmd/caib/inspectcmd/inspect_test.gocmd/caib/main.gocmd/caib/registryauth/credentials.gocmd/caib/runtime_wiring.gocmd/export-tasks/main.gogo.modinternal/buildapi/server.gointernal/buildapi/types.gointernal/common/tasks/pipeline_test.gointernal/common/tasks/scripts/build_image.shinternal/common/tasks/scripts/push_artifact.shinternal/common/tasks/tasks.gointernal/controller/imagebuild/controller.go
✅ Files skipped from review due to trivial changes (2)
- cmd/caib/main.go
- internal/buildapi/types.go
🚧 Files skipped from review as they are similar to previous changes (14)
- api/v1alpha1/labels.go
- cmd/caib/common/build_validation.go
- api/v1alpha1/imagebuild_types.go
- cmd/caib/common/oci_artifact.go
- cmd/caib/buildcmd/build_disk_test.go
- internal/common/tasks/scripts/build_image.sh
- cmd/caib/runtime_wiring.go
- cmd/export-tasks/main.go
- internal/controller/imagebuild/controller.go
- internal/common/tasks/scripts/push_artifact.sh
- cmd/caib/image/image.go
- cmd/caib/buildcmd/build.go
- internal/buildapi/server.go
- cmd/caib/inspectcmd/inspect_test.go
bkhizgiy
approved these changes
May 7, 2026
10 tasks
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--reproducible saves a snapshot of the RPMs used for the build, so they can be reused later.
caib image inspectwould show the details of an OCI artifact with rebuild instructionsSummary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
New Features
Bug Fixes / Validation
Tests