ci: allow prs from others#180
Conversation
📝 WalkthroughWalkthroughThis PR reworks the CI pipeline to support fork pull requests safely. The docker-build composite action gains conditional login, push, load, and cache-to inputs. ci.yml splits container image building into trusted and PR paths with selector jobs, routes lint/test/helm/e2e through docker run in the selected container, tightens job permissions, and switches Codecov uploads to OIDC. CONTRIBUTING.md documents the resulting fork PR CI behavior. ChangesFork-safe CI container pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
🤖 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 @.github/actions/docker-build/action.yml:
- Around line 30-40: When custom Buildx outputs are provided via the action’s
outputs input, stop forwarding the default push and load inputs so the build
runs in a single output mode. Update the docker-build action’s input wiring and
any buildx invocation logic in the action to conditionally omit push/load
whenever outputs is non-empty, preserving the documented digest/output-digest
behavior.
In @.github/workflows/ci.yml:
- Line 214: The cache export condition in the workflow is incorrect because the
current github.event_name == 'pull_request' && '' || ... pattern still falls
through to a non-empty cache-to on PRs. Update the cache-to expression in the
workflow to use a != comparison so pull requests resolve to an empty value, and
keep the existing non-PR cache export value intact; verify the change with task
lint-actions before pushing.
- Around line 913-914: The publish-helm job is still using a mutable container
reference from ci-container, which should be replaced with an immutable digest
pin. Update the ci-container workflow output to export the built image digest,
then change the container image reference in the publish-helm job to use
image@sha256:... instead of the tag-based needs.ci-container.outputs.image
value. Use the ci-container output symbol and the publish-helm container image
field to locate the change.
- Around line 38-39: The Checkout code step is leaving GitHub token credentials
persisted in .git/config even though these jobs never push changes. Update each
actions/checkout use in the workflow to set persist-credentials to false, and
apply it consistently to the Checkout code steps in the affected jobs (including
the PR/container-related ones) so credentials are not retained after checkout.
In `@CONTRIBUTING.md`:
- Around line 163-177: The “Container-based jobs reuse the published CI image”
paragraph is out of sync with the PR workflows and should be rewritten to match
the actual selector/artifact flow used by the CI jobs. Update the
CONTRIBUTING.md section by referencing the relevant workflow behavior (the CI
image artifact and per-commit image selection) instead of claiming fork PRs
always run against ghcr.io/...:latest, and keep the package visibility note only
if a remaining job truly requires public access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a084c39-1e09-4e47-9b8c-40bf16ff2f6f
📒 Files selected for processing (3)
.github/actions/docker-build/action.yml.github/workflows/ci.ymlCONTRIBUTING.md
| description: 'Build outputs (e.g., type=image,push=true or type=image,name=...,push-by-digest=true). Overrides push/load when set.' | ||
| required: false | ||
| default: '' | ||
| push: | ||
| description: 'Push the built image to the registry. Set to false for fork PRs whose read-only token cannot write packages.' | ||
| required: false | ||
| default: 'true' | ||
| load: | ||
| description: 'Load the built image into the local Docker daemon (single-platform only). Used to hand a fork-built image to later jobs via an artifact instead of a registry push.' | ||
| required: false | ||
| default: 'false' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== action.yml (relevant range) =="
nl -ba .github/actions/docker-build/action.yml | sed -n '1,140p'
echo
echo "== Search for inputs.outputs / push / load usage in repo =="
rg -n "outputs:|push:|load:" .github -g '*.yml' -g '*.yaml' -g '*.md' -g '*.sh' -g '*.ts' -g '*.js' || trueRepository: ConfigButler/gitops-reverser
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== action.yml (relevant range) =="
sed -n '1,140p' .github/actions/docker-build/action.yml
echo
echo "== Call sites of the composite action =="
rg -n "\.github/actions/docker-build|uses:\s*\.\/?\.github/actions/docker-build" .github -g '*.yml' -g '*.yaml' || true
echo
echo "== Inputs at those call sites =="
rg -n "outputs:|push:|load:" .github/workflows .github/actions -g '*.yml' -g '*.yaml' || trueRepository: ConfigButler/gitops-reverser
Length of output: 4549
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docker-build action call sites =="
rg -n -C 4 "docker-build/action.yml|\.github/actions/docker-build|uses:\s*\.\/\.github/actions/docker-build" .github/workflows .github/actions -g '*.yml' -g '*.yaml' || true
echo
echo "== ci.yml around outputs usage `#1` =="
sed -n '460,490p' .github/workflows/ci.yml
echo
echo "== ci.yml around outputs usage `#2` =="
sed -n '800,820p' .github/workflows/ci.yml
echo
echo "== ci.yml around push/load examples =="
sed -n '50,70p' .github/workflows/ci.yml
echo "---"
sed -n '125,140p' .github/workflows/ci.yml
echo "---"
sed -n '205,215p' .github/workflows/ci.yml
echo "---"
sed -n '420,430p' .github/workflows/ci.ymlRepository: ConfigButler/gitops-reverser
Length of output: 4890
🌐 Web query:
docker/build-push-action outputs overrides push load documentation
💡 Result:
In the docker/build-push-action, the push and load inputs function as shorthand configurations for the --output build argument [1][2][3]. Specifically: - load is shorthand for --output=type=docker [1][3]. - push is shorthand for --output=type=registry [1][3]. Because these inputs effectively define output behavior, using them in conjunction with the explicit outputs input can lead to conflicts or overrides [4][5]. Documentation and behavior notes: 1. Shorthand Logic: The action translates the boolean inputs load and push into specific --output flags [1][4]. If you define custom configurations in the outputs input while also setting push or load to true, the behavior depends on how the underlying Docker Buildx command processes these flags [5]. 2. Mutually Exclusive Nature: Historically, and in related Buildx contexts (such as bake), push and load have been treated as mutually exclusive operations because they direct the build result to different destinations (the local Docker daemon vs. a registry) [5]. 3. Overrides: If both are specified, the explicit --output configuration defined via the outputs input may be ignored or overridden by the shorthands depending on the specific version of Buildx being utilized [4][5]. It is generally recommended to use either outputs (for custom destinations) or the push/load flags, but not to mix them in a way that creates conflicting output types [1][5]. For complex build requirements, it is best practice to use the outputs input to explicitly define all desired output destinations (e.g., type=registry,ref=...,type=local,dest=...) rather than relying on the push or load shorthands [1][3].
Citations:
- 1: https://github.com/docker/build-push-action/blob/v6.12.0/README.md
- 2: https://github.com/docker/build-push-action/blob/1fde16337d494a70a6a0d6c3f24486b201a6a873/action.yml
- 3: https://github.com/docker/build-push-action/blob/v5.2.0/README.md
- 4: https://github.com/docker/build-push-action/blob/10e90e36/src/context.ts
- 5: bake: fix output handling for push docker/buildx#2330
Suppress push/load when outputs is set. Forwarding the defaults alongside custom Buildx outputs leaves callers with mixed output modes, so the digest/output-digest path can behave differently than the action docs promise. .github/actions/docker-build/action.yml:72-79
🤖 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 @.github/actions/docker-build/action.yml around lines 30 - 40, When custom
Buildx outputs are provided via the action’s outputs input, stop forwarding the
default push and load inputs so the build runs in a single output mode. Update
the docker-build action’s input wiring and any buildx invocation logic in the
action to conditionally omit push/load whenever outputs is non-empty, preserving
the documented digest/output-digest behavior.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/ci.yml (3)
253-267: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCI-container/project-image login+download+load steps are duplicated verbatim across four jobs.
lint-helm,lint,test, ande2eeach repeat the identical "Login to Docker registry (non-PR)" / "Download CI container image (PR)" / "Load CI container image (PR)" step trio (ande2eadds a fourth "project image" variant of the same pattern). Any change to this contract (artifact name, registry auth mechanism) now needs four synchronized edits. Consider extracting a composite action (e.g..github/actions/load-container/action.yml) that takes the artifact name and image ref and performs the conditional login-or-download-and-load logic once.Also applies to: 312-326, 377-391, 605-633
🤖 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 @.github/workflows/ci.yml around lines 253 - 267, The Docker registry login and CI image download/load sequence is duplicated across multiple jobs, so centralize the repeated conditional logic into a reusable composite action (for example, a shared action that accepts the artifact name and image ref). Update the affected job blocks to call that action instead of inlining the same three-step pattern, and do the same for the project-image variant used by e2e so changes to auth or artifact handling only need one edit.
28-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
build-ci-container-prandbuild-ci-container-trustedduplicate the conditional login/push/load/cache-to logic that the new compositedocker-buildaction was built to encapsulate.Per the stack context,
.github/actions/docker-build/action.ymlwas specifically extended with alogininput to gate registry login andpush/load/cache-toinputs — yet these two ~65-line jobs still hand-roll the same conditional structure withdocker/build-push-action@v7directly. Consolidating onto the composite action would remove this duplication and keep the conditional-build logic in one place for future changes (e.g., if the digest-pinning fix from the earlier review is applied, it only needs to change once).🤖 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 @.github/workflows/ci.yml around lines 28 - 156, The build jobs for build-ci-container-pr and build-ci-container-trusted are duplicating the conditional registry login, push/load, and cache handling that the composite docker-build action already encapsulates. Replace the direct docker/build-push-action@v7 usage in these jobs with the shared .github/actions/docker-build composite action, wiring its login, push, load, and cache inputs to preserve the current PR vs trusted behavior. Keep the existing image naming and validation steps, but centralize the build logic so future changes only need to be made in one place.
68-68: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueMove
steps.image.outputs.nameinto anenv:block before interpolating into the run script.Flagged by zizmor (template-injection) at both occurrences. The image name is currently attacker-uncontrolled (built from
REGISTRY+github.sha), so actual risk is low, but the canonical fix (per zizmor's own remediation docs) is to bind it viaenv:and reference$IMAGEinstead of interpolating${{ }}directly into the script — this avoids the general anti-pattern regardless of current safety.🔧 Proposed fix (apply to both occurrences)
- name: Validate CI container tools + env: + CI_IMAGE: ${{ steps.image.outputs.name }} run: | - docker run --rm ${{ steps.image.outputs.name }} bash -lc " + docker run --rm "${CI_IMAGE}" bash -lc " set -eAlso applies to: 141-141
🤖 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 @.github/workflows/ci.yml at line 68, The workflow has template-injection risk because docker run interpolates steps.image.outputs.name directly inside the bash script in both occurrences. Move that value into an env block first, bind it to a variable like IMAGE, and update the run script to reference the shell variable instead of using ${} interpolation. Apply the same env-based pattern consistently in both docker run steps so the workflow uses the canonical zizmor-safe approach.Source: Linters/SAST tools
🤖 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 `@docs/proud.md`:
- Around line 32-40: The markdown table in the documentation is missing the
required blank line before the next heading, triggering MD058. Update the
section containing the port table so there is an empty line after the last row
and before the “4. Tilt turns the whole thing into a playground” heading,
keeping the table structure otherwise unchanged.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 253-267: The Docker registry login and CI image download/load
sequence is duplicated across multiple jobs, so centralize the repeated
conditional logic into a reusable composite action (for example, a shared action
that accepts the artifact name and image ref). Update the affected job blocks to
call that action instead of inlining the same three-step pattern, and do the
same for the project-image variant used by e2e so changes to auth or artifact
handling only need one edit.
- Around line 28-156: The build jobs for build-ci-container-pr and
build-ci-container-trusted are duplicating the conditional registry login,
push/load, and cache handling that the composite docker-build action already
encapsulates. Replace the direct docker/build-push-action@v7 usage in these jobs
with the shared .github/actions/docker-build composite action, wiring its login,
push, load, and cache inputs to preserve the current PR vs trusted behavior.
Keep the existing image naming and validation steps, but centralize the build
logic so future changes only need to be made in one place.
- Line 68: The workflow has template-injection risk because docker run
interpolates steps.image.outputs.name directly inside the bash script in both
occurrences. Move that value into an env block first, bind it to a variable like
IMAGE, and update the run script to reference the shell variable instead of
using ${} interpolation. Apply the same env-based pattern consistently in both
docker run steps so the workflow uses the canonical zizmor-safe approach.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 086d7e8c-0b4b-4a90-84ec-d2b931d25ecb
📒 Files selected for processing (4)
.devcontainer/Dockerfile.github/workflows/ci.ymldocs/proud.mdinvestigate.md
💤 Files with no reviewable changes (1)
- investigate.md
| | Port | Service | Why it's there | | ||
| |---|---|---| | ||
| | 13000 | **Gitea** | A real Git server for the reversed repos | | ||
| | 10350 | **Tilt** | The live development loop UI | | ||
| | 19081 | **Allure** | The e2e test report | | ||
| | 19090 | **Prometheus** | Controller metrics + history | | ||
| | 19080 | **Flux Operator UI** | The GitOps side of the house | | ||
| | 16379 | **Valkey (Redis-compatible)** | Watch-resume state | | ||
| ## 4. Tilt turns the whole thing into a playground |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a blank line after the table (MD058).
markdownlint flags the table (lines 32-39) as missing a trailing blank line before the next heading.
📝 Proposed fix
| 16379 | **Valkey (Redis-compatible)** | Watch-resume state |
+
## 4. Tilt turns the whole thing into a playground📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Port | Service | Why it's there | | |
| |---|---|---| | |
| | 13000 | **Gitea** | A real Git server for the reversed repos | | |
| | 10350 | **Tilt** | The live development loop UI | | |
| | 19081 | **Allure** | The e2e test report | | |
| | 19090 | **Prometheus** | Controller metrics + history | | |
| | 19080 | **Flux Operator UI** | The GitOps side of the house | | |
| | 16379 | **Valkey (Redis-compatible)** | Watch-resume state | | |
| ## 4. Tilt turns the whole thing into a playground | |
| | Port | Service | Why it's there | | |
| |---|---|---| | |
| | 13000 | **Gitea** | A real Git server for the reversed repos | | |
| | 10350 | **Tilt** | The live development loop UI | | |
| | 19081 | **Allure** | The e2e test report | | |
| | 19090 | **Prometheus** | Controller metrics + history | | |
| | 19080 | **Flux Operator UI** | The GitOps side of the house | | |
| | 16379 | **Valkey (Redis-compatible)** | Watch-resume state | | |
| ## 4. Tilt turns the whole thing into a playground |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 39-39: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 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 `@docs/proud.md` around lines 32 - 40, The markdown table in the documentation
is missing the required blank line before the next heading, triggering MD058.
Update the section containing the port table so there is an empty line after the
last row and before the “4. Tilt turns the whole thing into a playground”
heading, keeping the table structure otherwise unchanged.
Sources: Coding guidelines, Linters/SAST tools
Restructure the single ci.yml into three workflows split by trust zone, keeping one copy of every validation job: - ci.yml: validation only (containers, lint, unit, e2e, image scan). Runs directly for PRs (fork-safe: no secrets, no registry writes, images travel between jobs as artifacts) and is called as a reusable workflow from release.yml for pushes to main. The duplicated -pr/-trusted job pairs and their selector jobs collapse into single jobs whose steps branch on the event. - release.yml: push to main only. Calls ci.yml first, then release-please, then publishes: multi-arch image with cosign keyless signature, SLSA build provenance and SPDX SBOM attestations, signed Helm chart, verify instructions in the release notes. - scorecard.yml: OpenSSF Scorecard, weekly + on main. Supply-chain hardening: all actions pinned to commit SHAs, base images pinned by digest (Dependabot keeps both moving), Trivy image scan job (fails on fixable CRITICALs), persist-credentials: false everywhere, minimal per-job token permissions, new Trivy/Scorecard/attest actions. e2e: the image-refresh suite now skips when PROJECT_IMAGE is provided externally (no local build -> load chain to validate) and the CI quickstart lane runs it with PROJECT_IMAGE unset + load delivery, so the chain is validated for real - also on fork PRs (fixes the image_refresh_test.go:192 failures on this branch). Docs: docs/ci-overview.md describes the trust zones and how to verify release signatures; CONTRIBUTING.md fork section updated to match the artifact-handoff reality; README badges point at release.yml + Scorecard. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… gate The first CI execution of the image-refresh suite surfaced two real issues: - The image-scan gate caught sops v3.11.0 shipping fixable CRITICALs (grpc CVE-2026-33186, Go stdlib CVE-2025-68121 via an old build toolchain). Bump to v3.13.2, built with go1.26.4 + grpc v1.81.1. - S6 compared the running pod's CRI-reported imageID with the digest recorded in the stamp at import time. Those two identities are not comparable in every environment: load-image.sh falls back to Docker's image id when containerd has not yet materialized the sha256 alias record (it only appears once a pod uses the image), while CRI reports containerd's identity. S6 now waits for the S5 rollout to settle and asserts the environment-independent invariant directly on the k3d node: the pod's image digest must be among the containerd records for the currently imported tag. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
S6's node-truth assertion exposed a real coherence gap: the quickstart lane installs with the provided (ghcr) image ref, and the install stamps do not invalidate when the image ref changes, while load-mode deploys only rollout-restart. The refresh chain therefore kept restarting a deployment still pointing at the provided tag - the local rebuilds were imported into k3d but never run. Reset the install between the helm quickstart and the refresh suite so prepare-e2e renders a fresh install against the locally built tag. Follow-up worth considering: make the install tasks fingerprint the image ref so the DAG re-renders the install when PROJECT_IMAGE changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Allow external contributors to also validate their changes (but at a low trust level, off course not getting a GH token that has other access than read).