chore: fork CI#4051
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplits CI artifact production into trusted (same-repo) and untrusted (fork PR) build paths, adds a callable workflow to aggregate artifact job results, and conditions Changes
Sequence DiagramsequenceDiagram
participant GH as GitHub
participant CI as CI Workflow
participant TA as Trusted Artifacts
participant UA as Untrusted Artifacts
participant AR as Artifacts Pass
participant QS as Quickstart / E2E
GH->>CI: Trigger workflow (PR or non-PR)
alt same-repo / non-PR
CI->>TA: invoke trusted-artifacts
TA->>TA: build images (can reference/push)
TA-->>AR: report status
else fork PR
CI->>UA: invoke untrusted-artifacts
UA->>UA: build images (no push, read-only)
UA-->>AR: report status
end
CI->>AR: invoke workflow-result with aggregated result
AR->>QS: allow or block quickstart/e2e based on result
QS->>QS: use prebuilt image or build inline depending on source
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
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 (2)
.github/workflows/ci.yaml (2)
536-549:⚠️ Potential issue | 🟠 MajorSame push event issue in e2e job.
The e2e job has the same conditional logic problem as quickstart. Push events will incorrectly trigger the local build step.
🛠️ Suggested fix
- name: Create override files for e2e env: DEPOT_IMAGE_URL: ${{ needs.artifacts.outputs.container-image-url-depot }} - if: github.event.pull_request.head.repo.full_name == github.repository + if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name == github.repository # ... existing step content ... - name: Build as part of e2e - if: github.event.pull_request.head.repo.full_name != github.repository + if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 536 - 549, The e2e job's conditional (e.g., the step using if: github.event.pull_request.head.repo.full_name == github.repository and the subsequent "Build as part of e2e" step using !=) lets push events hit the local-build branch; update the conditions to only run on pull_request events by adding an explicit check for the event type (for example combine github.event_name == 'pull_request' with the existing head.repo.full_name comparison or use github.event.pull_request != null) for both the override/write step and the "Build as part de e2e" step so push events no longer trigger the local build path.
425-448:⚠️ Potential issue | 🟠 MajorSame push event issue affects quickstart override selection.
For push events,
github.event.pull_request.head.repo.full_nameis empty, so:
- Line 425 condition (
== github.repository) →false- Line 448 condition (
!= github.repository) →trueThis means pushes to main would trigger the "Build as part of quickstart" step instead of using the depot image, which is probably not what you want.
🛠️ Suggested fix
- name: Create override files for quickstart - if: github.event.pull_request.head.repo.full_name == github.repository + if: github.event_name == 'push' || github.event.pull_request.head.repo.full_name == github.repository # ... existing step content ... - name: Build as part of quickstart - if: github.event.pull_request.head.repo.full_name != github.repository + if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 425 - 448, The if-conditions that compare github.event.pull_request.head.repo.full_name to github.repository are wrong for push events (pull_request payload is absent), so update both conditionals to first ensure the event is a pull_request before comparing (e.g. add github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository) for the branch that writes quickstart/docker-compose.override.yaml with DEPOT_IMAGE_URL, and invert that check (e.g. github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) or fallback to treating non-pull_request events as same-repo as appropriate for the "Build as part of quickstart" step so pushes to main use the depot image path rather than the other branch.
🧹 Nitpick comments (1)
.github/workflows/ci.yaml (1)
464-466: Missing newline before next step.There's no blank line separating the heredoc-generating step from "Launch Docker Compose". Minor formatting nit, but the e2e job (line 559-561) does include the blank line for consistency.
✏️ Suggested fix
EOF + - name: Launch Docker Compose🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 464 - 466, The YAML step "Launch Docker Compose" is missing a blank line separating it from the preceding heredoc-generating step; add a single empty line before the "name: Launch Docker Compose" step in the .github/workflows/ci.yaml file so the steps are consistently separated (matching the e2e job formatting) to avoid formatting inconsistencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/artifacts.yaml:
- Line 99: The workflow uses invalid GitHub Actions expression syntax by using
"not" for negation (e.g., the save entry currently written as "save: ${{ not
inputs.untrusted_source }}"); replace "not" with the "!" operator inside the
expression so it becomes "save: ${{ ! inputs.untrusted_source }}" and apply the
same fix to the other occurrences noted (the similar expressions at the other
save/condition locations referenced in the comment).
- Line 207: The "save" workflow input uses the wrong expression syntax for the
not operator; update the expression so the not operator wraps the operand in
parentheses (i.e., use not(inputs.untrusted_source)) so the "save" key evaluates
correctly; locate the "save" entry referencing inputs.untrusted_source and
replace the current expression with the corrected not(...) form.
- Line 111: Replace the invalid GitHub Actions conditional "if: not
inputs.untrusted_source" with a proper expression wrapper and operator; change
it to use the expression syntax and negation like: if: ${{ !
inputs.untrusted_source }} so the workflow evaluates the condition correctly
(update the conditional where "if: not inputs.untrusted_source" appears).
- Line 219: The workflow step uses raw YAML boolean syntax ("if: not
inputs.untrusted_source") which is invalid for GitHub Actions; update the step
so the if uses a GitHub Actions expression by wrapping the condition in the
expression delimiters and applying the unary not operator to
inputs.untrusted_source (i.e., convert the plain condition into a proper Actions
expression referencing inputs.untrusted_source). Ensure the line remains an if:
key and references inputs.untrusted_source so the runner evaluates it as an
expression.
- Around line 11-14: The new required workflow input "untrusted_source" causes
callers like release.yaml to fail; either make the input optional by changing
the artifacts.yaml input "untrusted_source" to include a default (e.g., default:
false) and remove required: true, or update every caller (e.g., release.yaml)
that uses the artifacts workflow to pass the input (add inputs:
untrusted_source: ${{ <appropriate expression> }} when calling the reusable
workflow); ensure you update all callers consistently so no workflow call is
left without the input.
In @.github/workflows/ci.yaml:
- Around line 373-374: The untrusted_source assignment incorrectly treats push
events as untrusted because github.event.pull_request is undefined; update the
expression for untrusted_source to only perform the pull-request comparison when
the event is a pull_request (use github.event_name or github.event_name ==
'pull_request') and otherwise mark pushes as trusted (false), referencing the
existing untrusted_source variable and the
github.event.pull_request.head.repo.full_name comparison so only PRs are
checked.
---
Outside diff comments:
In @.github/workflows/ci.yaml:
- Around line 536-549: The e2e job's conditional (e.g., the step using if:
github.event.pull_request.head.repo.full_name == github.repository and the
subsequent "Build as part of e2e" step using !=) lets push events hit the
local-build branch; update the conditions to only run on pull_request events by
adding an explicit check for the event type (for example combine
github.event_name == 'pull_request' with the existing head.repo.full_name
comparison or use github.event.pull_request != null) for both the override/write
step and the "Build as part de e2e" step so push events no longer trigger the
local build path.
- Around line 425-448: The if-conditions that compare
github.event.pull_request.head.repo.full_name to github.repository are wrong for
push events (pull_request payload is absent), so update both conditionals to
first ensure the event is a pull_request before comparing (e.g. add
github.event_name == 'pull_request' &&
github.event.pull_request.head.repo.full_name == github.repository) for the
branch that writes quickstart/docker-compose.override.yaml with DEPOT_IMAGE_URL,
and invert that check (e.g. github.event_name == 'pull_request' &&
github.event.pull_request.head.repo.full_name != github.repository) or fallback
to treating non-pull_request events as same-repo as appropriate for the "Build
as part of quickstart" step so pushes to main use the depot image path rather
than the other branch.
---
Nitpick comments:
In @.github/workflows/ci.yaml:
- Around line 464-466: The YAML step "Launch Docker Compose" is missing a blank
line separating it from the preceding heredoc-generating step; add a single
empty line before the "name: Launch Docker Compose" step in the
.github/workflows/ci.yaml file so the steps are consistently separated (matching
the e2e job formatting) to avoid formatting inconsistencies.
🪄 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: 2a32a033-546a-46af-b862-2ea908088dfa
📒 Files selected for processing (2)
.github/workflows/artifacts.yaml.github/workflows/ci.yaml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yaml (1)
688-719:⚠️ Potential issue | 🟠 MajorGuard the e2e fork checks so pushes still test the artifact image.
The e2e conditions don’t check
github.event_name, so push events can fall into the local-build branch instead of usingneeds.artifacts.outputs.container-image-url-depot. That means main pushes stop testing the image that is about to be pushed.🐛 Proposed fix
- name: Create override files for e2e env: DEPOT_IMAGE_URL: ${{ needs.artifacts.outputs.container-image-url-depot }} - if: github.event.pull_request.head.repo.full_name == github.repository + if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} run: | cat > e2e/docker-compose.override.yaml <<EOF services: openmeter: image: $DEPOT_IMAGE_URL @@ - name: Build as part of e2e - if: github.event.pull_request.head.repo.full_name != github.repository + if: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository }} run: | cat > e2e/docker-compose.override.yaml <<EOF services: openmeter:You can verify the mismatch against the quickstart guards with:
#!/bin/bash # Description: Compare quickstart and e2e fork/same-repo guards. # Expected: e2e guards should include the same github.event_name protection as quickstart. python - <<'PY' from pathlib import Path path = Path(".github/workflows/ci.yaml") lines = path.read_text().splitlines() for start, end, label in [ (572, 619, "quickstart guards"), (688, 719, "e2e guards"), ]: print(f"\n--- {label}: {path}:{start}-{end} ---") for lineno in range(start, end + 1): line = lines[lineno - 1] if "if:" in line or "name:" in line: print(f"{lineno}: {line}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yaml around lines 688 - 719, The e2e job conditionals ("Create override files for e2e" and "Build as part of e2e") only compare github.event.pull_request.head.repo.full_name and miss checking the event type, so push events can incorrectly take the local-build branch; update both if: expressions to also require github.event_name == 'pull_request' (e.g., if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository for the override that uses DEPOT_IMAGE_URL, and the inverse for the build step) so pushes use needs.artifacts.outputs.container-image-url-depot while PRs use local build logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yaml:
- Around line 688-719: The e2e job conditionals ("Create override files for e2e"
and "Build as part of e2e") only compare
github.event.pull_request.head.repo.full_name and miss checking the event type,
so push events can incorrectly take the local-build branch; update both if:
expressions to also require github.event_name == 'pull_request' (e.g., if:
github.event_name == 'pull_request' &&
github.event.pull_request.head.repo.full_name == github.repository for the
override that uses DEPOT_IMAGE_URL, and the inverse for the build step) so
pushes use needs.artifacts.outputs.container-image-url-depot while PRs use local
build logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a075c697-6a2e-4673-a8f0-2ec51d7b8a31
📒 Files selected for processing (3)
.github/workflows/artifacts.yaml.github/workflows/ci.yaml.github/workflows/release.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 544-552: The artifacts-pass job currently uses a conditional that
skips when dependencies fail; change its if condition to always() so the job
runs regardless of dependency results, then add an explicit check step that
inspects needs.*.result (or the specific dependent job names) and fails the job
if any dependency equals "failure" — update the artifacts-pass job (the job
block named "artifacts-pass" that uses ./.github/workflows/workflow-result.yaml
and currently sets with: result: "pass") to run unconditionally (if: always())
and compute the final "result" input (or call the workflow-result helper) based
on whether contains(needs.*.result, 'failure') is true, causing the job to exit
non-zero when any artifact job failed.
- Around line 629-648: The Compose override created in the workflow currently
lists services (openmeter, sink-worker, balance-worker, notification-service,
billing-worker, openmeter-jobs) with build keys but does not set pull_policy, so
Compose may still pull published images; update the override generator in the
GitHub Actions step that writes quickstart/docker-compose.override.yaml to add
pull_policy: build under each service (alongside the build entry) so each
service explicitly forces building the forked code rather than pulling, ensuring
correct YAML indentation and alignment with the existing service blocks.
🪄 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: 29008068-42d9-4457-a0ff-576b1e6ab880
📒 Files selected for processing (3)
.github/workflows/ci.yaml.github/workflows/untrusted-artifacts.yaml.github/workflows/workflow-result.yaml
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit