Skip to content

refactor(docker-ci): default to main tag for CI and require maintainer sign-off on pushed builds - #312

Merged
Ryan-Millard merged 15 commits into
mainfrom
fix/ci/docker-images-on-prs
Apr 22, 2026
Merged

refactor(docker-ci): default to main tag for CI and require maintainer sign-off on pushed builds#312
Ryan-Millard merged 15 commits into
mainfrom
fix/ci/docker-images-on-prs

Conversation

@Ryan-Millard

@Ryan-Millard Ryan-Millard commented Apr 18, 2026

Copy link
Copy Markdown
Owner

What was changed & why

CI was changed to fix the build errors introduced in #310.

CI initially defaults to :main image tag since the image for the PR won't have been pushed. Once a maintainer comments /docker-build <most-recent-commit-sha> or /docker-cleanup, things change.

Fixes: none - my prior mistakes.

Changes

Just CI.

Testing & Verification

Additional Resources

Summary by CodeRabbit

  • Chores

    • Switched container publishing to GitHub Container Registry and expanded CI package permissions.
    • Centralized image selection into a single image-resolution step used by downstream CI.
    • Enhanced image-probing and wait behavior with configurable timing, initial delay, and backoff.
  • New Features

    • Comment-driven image build and cleanup commands with authorization checks and comment reactions.
    • Notifications and local-run instructions now reference GHCR image links.

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Centralizes image selection into a new set-image job (exports image), replaces prior wait-for-image usage in CI with set-image output, adds guard gating and GHCR-first Docker workflows, updates reusable wait-for-image inputs/backoff, and broadens workflow permissions for packages/pull-requests.

Changes

Cohort / File(s) Summary
CI workflow
​.github/workflows/ci.yml
Added set-image job that computes/exports image; updated downstream jobs (lint, cmake-build, build-react-app, build-docs) to depend on and consume needs.set-image.outputs.image; added packages: read permission.
Docker build & publish
​.github/workflows/docker-ci.yml
Reworked triggers to include issue_comment and gated runs with a new guard job; comment-driven builds resolve PR/sha and require authorization; switched PR builds/cache to GHCR, release/main produce GHCR + Docker Hub tags; added GHCR cleanup via actions/delete-package-versions; expanded permissions (pull-requests).
Reusable wait workflow
​.github/workflows/wait-for-docker-image.yml
Replaced single image input with dh_image + ghcr_image; added initial_wait_seconds, backoff_multiplier, increased max_attempts; refactored flow to emit parsed outputs, perform initial wait, check GHCR manifest (freshness) then Docker Hub tag timestamp.
Deploy workflow invocation
​.github/workflows/deploy.yml
Now passes explicit dh_image into reusable wait-for-image call (previously no inputs).

Sequence Diagram(s)

sequenceDiagram
  participant User as Comment Author
  participant Guard as Guard Job
  participant SetImage as Set-Image Job
  participant CI as Downstream CI Jobs
  participant Wait as Wait-For-Image Workflow
  participant GHCR as GHCR Registry
  participant DH as Docker Hub

  User->>Guard: /docker-build or /docker-cleanup (issue_comment)
  Guard->>Guard: resolve PR#, validate SHA, check authorization
  Guard-->>CI: allow or block comment-driven run
  SetImage->>SetImage: compute image tag (pr-<n> or :main) -> output `image`
  CI->>SetImage: needs.set-image.outputs.image
  CI->>Wait: call with `ghcr_image`/`dh_image` (when used)
  Wait->>GHCR: HEAD/manifest request (check existence + last-modified)
  alt GHCR PR tag exists & fresh
    Wait-->>CI: GHCR PR image ready
  else
    Wait->>DH: query tags API (tag_last_pushed)
    alt Docker Hub PR tag exists
      Wait-->>CI: Docker Hub PR image ready
    else
      Wait-->>CI: fallback to GHCR :main image
    end
  end
  CI->>GHCR: push/tag (PR builds/cache)
  CI->>DH: push/tag (release/main builds)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇
I hopped through YAML lanes at dawn,
I stitched a tag so builds respawn,
GHCR keeps the cache in sight,
Docker Hub waits for release night,
One rabbit, one image — all set, hop on! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title emphasizes maintainer sign-off and GHCR default tagging, but the substantive changes span multiple workflows beyond docker-ci.yml, including major refactors of ci.yml and wait-for-docker-image.yml that centralize image selection and introduce security gates.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ci/docker-images-on-prs

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

❤️ Share

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

@Ryan-Millard
Ryan-Millard force-pushed the fix/ci/docker-images-on-prs branch from 12a1510 to 4bf3940 Compare April 18, 2026 18:30

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)

1-11: ⚠️ Potential issue | 🔴 Critical

Fork PRs always receive read-only GITHUB_TOKEN regardless of the permissions block, preventing the PR objective.

The PR objective is to unblock CI on PRs from forks (Docker Hub secrets unavailable). However, in public repos, workflows triggered by pull_request from a fork receive a read-only GITHUB_TOKEN, regardless of the permissions: block declared at workflow or job level. This is a security restriction to prevent malicious forks from writing to the base repository's resources.

Consequently:

  • docker-ci.yml would fail when run on fork PRs because packages: write cannot be granted to fork-triggered tokens, even though it's declared in the workflow.
  • wait-for-image in ci.yml would never see a pr-<N> tag and would still time out.

To achieve write permissions for fork PRs, use pull_request_target event instead of pull_request, which grants read/write permissions (configurable via the permissions block) even for forks. However, this requires careful checkout of the PR head SHA to avoid security vulnerabilities. Additionally, confirm whether packages: read is sufficient for fork-triggered tokens to pull private packages.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 1 - 11, Change the workflow trigger
from pull_request to pull_request_target in the CI workflow so fork PRs can
receive configurable write permissions, then update any jobs that expect to act
on the PR head (e.g., the job that runs docker-ci.yml and the wait-for-image
logic) to securely checkout the PR head commit by using actions/checkout with
repository: github.event.pull_request.head.repo.full_name and ref:
github.event.pull_request.head.sha (and fetch-depth: 0) instead of the default
checkout; ensure the workflow permissions block includes packages: write where
required for publishing/pulling and verify jobs that run code from the PR run
only after explicitly checking out the head SHA to avoid executing untrusted
workspace content.
🧹 Nitpick comments (3)
.github/workflows/docker-ci.yml (2)

105-107: Redundant GHCR_IMAGE env redefinition.

GHCR_IMAGE is already defined at workflow level (line 17) and is available to all jobs. Redefining it in the notify job's env: is harmless but duplicative; removing it keeps the single source of truth.

♻️ Suggested tidy-up
     env:
-      GHCR_IMAGE: ghcr.io/${{ github.repository_owner }}/img2num-dev
       CLEAN_CORE_TAG: ${{ needs.build.outputs.clean_core_tag }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/docker-ci.yml around lines 105 - 107, Redundant GHCR_IMAGE
env in the notify job: remove the GHCR_IMAGE line from the notify job's env
block and keep only CLEAN_CORE_TAG so the job inherits the GHCR_IMAGE value from
the workflow-level env; update the notify job's env to reference only
CLEAN_CORE_TAG (leave GHCR_IMAGE defined at workflow root) to ensure a single
source of truth.

91-94: PR builds will rarely get a cache hit from GHCR_IMAGE:buildcache.

cache-from now lists both cache_ref (GHCR:cache-pr-<N> for PRs) and GHCR_IMAGE:buildcache. For the first build of any PR, cache-pr-<N> doesn't exist yet, so you fall back to buildcache — but buildcache is only ever written by non-PR builds (line 72, 77), and PR builds use cache-to: ref=cache-pr-<N> (line 94), never refreshing the shared cache. That's fine intentionally (avoids PRs poisoning main's cache), just noting that:

  1. The first run on every PR pays full cold-build cost unless main's buildcache has overlapping layers for the current Dockerfile.dev.
  2. The cache-pr-<N> ref will be orphaned after PR close — your cleanup step handles this via the cache-pr-<N> tag match (once the regex above is fixed).

No action required if this is the intended trade-off; worth a comment in the workflow so future maintainers don't "fix" it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/docker-ci.yml around lines 91 - 94, Add a clarifying
comment in the workflow near the cache configuration (referencing cache-from,
cache-to, steps.meta.outputs.cache_ref and GHCR_IMAGE:buildcache) explaining
that PR builds intentionally use a per-PR cache ref (cache-pr-<N>) and do not
update the shared GHCR_IMAGE:buildcache, so the first PR run will be a cold
build unless main's buildcache layers overlap; also note that cache-pr-<N> refs
are cleaned up after PR close and this behavior is intentional to avoid
poisoning main's cache.
.github/workflows/ci.yml (1)

22-27: Hardcoded image owner diverges from docker-ci.yml.

docker-ci.yml builds/pushes to ghcr.io/${{ github.repository_owner }}/img2num-dev (line 17), but here you hardcode ghcr.io/ryanmillard/img2num-dev. They happen to match today, but if the repo is ever transferred, forked-and-renamed, or used as a template, the two workflows will silently pull different images. Prefer a single source of truth.

♻️ Suggested fix
       - name: Set image
         id: set
         run: |
+          OWNER="${{ github.repository_owner }}"
           if [[ "${{ github.event_name }}" == "pull_request" ]]; then
-            echo "image=ghcr.io/ryanmillard/img2num-dev:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
+            echo "image=ghcr.io/${OWNER,,}/img2num-dev:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
           else
-            echo "image=ghcr.io/ryanmillard/img2num-dev:main" >> $GITHUB_OUTPUT
+            echo "image=ghcr.io/${OWNER,,}/img2num-dev:main" >> $GITHUB_OUTPUT
           fi

(GHCR paths must be lowercase; ${OWNER,,} handles that safely.)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 22 - 27, Replace the hardcoded GHCR
owner in the run block echo lines by reading github.repository_owner into a
shell variable and lowercasing it before composing the image name; e.g., set
OWNER=${{ github.repository_owner }}, then use ${OWNER,,} when building the
image string in the two echo "image=..." lines inside the run block so the
workflow pulls the same repository owner as docker-ci.yml and remains correct if
the repo owner changes.
🤖 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/docker-ci.yml:
- Around line 141-156: The ignore-versions regex currently `^(?!pr-${{
github.event.pull_request.number }}|cache-pr-${{
github.event.pull_request.number }}).*` can falsely match prefixes like pr-50
when PR=5; update the value passed to ignore-versions to anchor the alternatives
to the end (e.g. `^(?!pr-${{ github.event.pull_request.number }}$|cache-pr-${{
github.event.pull_request.number }}$).*`) so only exact tag names are exempted,
and add `dry-run: true` to the Delete PR images step to verify whether the
action matches tags or manifest digests before enabling destructive behavior.

In @.github/workflows/wait-for-docker-image.yml:
- Line 18: The workflow docstring claims it checks both Docker Hub and GHCR but
the implementation still always hits Docker Hub using the URL pattern
https://hub.docker.com/v2/repositories/${IMAGE_NAME}/tags/${TAG}/ (IMAGE_NAME,
TAG at top of the file), which will 404 for ghcr.io images; update the
wait-for-image logic to detect images whose IMAGE_NAME begins with "ghcr.io/"
and, for those, call the correct GHCR registry API (e.g., query the container
manifest endpoint under GHCR or fallback to the GitHub Packages versions API for
timestamps) instead of the Docker Hub endpoint, preserving the existing
retry/backoff behavior and staleness semantics.

---

Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 1-11: Change the workflow trigger from pull_request to
pull_request_target in the CI workflow so fork PRs can receive configurable
write permissions, then update any jobs that expect to act on the PR head (e.g.,
the job that runs docker-ci.yml and the wait-for-image logic) to securely
checkout the PR head commit by using actions/checkout with repository:
github.event.pull_request.head.repo.full_name and ref:
github.event.pull_request.head.sha (and fetch-depth: 0) instead of the default
checkout; ensure the workflow permissions block includes packages: write where
required for publishing/pulling and verify jobs that run code from the PR run
only after explicitly checking out the head SHA to avoid executing untrusted
workspace content.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 22-27: Replace the hardcoded GHCR owner in the run block echo
lines by reading github.repository_owner into a shell variable and lowercasing
it before composing the image name; e.g., set OWNER=${{ github.repository_owner
}}, then use ${OWNER,,} when building the image string in the two echo
"image=..." lines inside the run block so the workflow pulls the same repository
owner as docker-ci.yml and remains correct if the repo owner changes.

In @.github/workflows/docker-ci.yml:
- Around line 105-107: Redundant GHCR_IMAGE env in the notify job: remove the
GHCR_IMAGE line from the notify job's env block and keep only CLEAN_CORE_TAG so
the job inherits the GHCR_IMAGE value from the workflow-level env; update the
notify job's env to reference only CLEAN_CORE_TAG (leave GHCR_IMAGE defined at
workflow root) to ensure a single source of truth.
- Around line 91-94: Add a clarifying comment in the workflow near the cache
configuration (referencing cache-from, cache-to, steps.meta.outputs.cache_ref
and GHCR_IMAGE:buildcache) explaining that PR builds intentionally use a per-PR
cache ref (cache-pr-<N>) and do not update the shared GHCR_IMAGE:buildcache, so
the first PR run will be a cold build unless main's buildcache layers overlap;
also note that cache-pr-<N> refs are cleaned up after PR close and this behavior
is intentional to avoid poisoning main's cache.
🪄 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

Run ID: d9db4e6d-ab07-443b-b4e0-8964b314c48f

📥 Commits

Reviewing files that changed from the base of the PR and between 79903d3 and 12a1510.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • .github/workflows/docker-ci.yml
  • .github/workflows/wait-for-docker-image.yml

Comment thread .github/workflows/docker-ci.yml Outdated
Comment thread .github/workflows/wait-for-docker-image.yml Outdated
@Ryan-Millard Ryan-Millard changed the title refactor(docker-ci): switch to use GitHub packages alongside Docker Hub (better PR integration) refactor(docker-ci): switch to use GHCR alongside Docker Hub (better PR integration) Apr 18, 2026
Repository owner deleted a comment from github-actions Bot Apr 18, 2026
Repository owner deleted a comment from github-actions Bot Apr 18, 2026
@Ryan-Millard
Ryan-Millard force-pushed the fix/ci/docker-images-on-prs branch from 265a917 to 7f2582e Compare April 18, 2026 19:50

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

14-27: Hardcoded image repo duplicates docker-ci.yml — consider deriving it.

ghcr.io/ryanmillard/img2num-dev is hardcoded here and also constructed in .github/workflows/docker-ci.yml. If the repo/owner is ever renamed (or this is forked), both files must be kept in sync. Consider deriving from github.repository_owner / github.event.repository.name (lowercased), e.g.:

♻️ Suggested derivation
       - name: Set image
         id: set
         run: |
-          if [[ "${{ github.event_name }}" == "pull_request" ]]; then
-            echo "image=ghcr.io/ryanmillard/img2num-dev:pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
-          else
-            echo "image=ghcr.io/ryanmillard/img2num-dev:main" >> $GITHUB_OUTPUT
-          fi
+          OWNER="${GITHUB_REPOSITORY_OWNER,,}"
+          REPO="ghcr.io/${OWNER}/img2num-dev"
+          if [[ "${{ github.event_name }}" == "pull_request" ]]; then
+            echo "image=${REPO}:pr-${PR_NUMBER}" >> "$GITHUB_OUTPUT"
+          else
+            echo "image=${REPO}:main" >> "$GITHUB_OUTPUT"
+          fi
+        env:
+          PR_NUMBER: ${{ github.event.pull_request.number }}

Also a minor hardening nit: moving github.event.pull_request.number into env: avoids GitHub Actions script-injection patterns, even though the PR number is numeric and safe in practice.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 14 - 27, Replace the hardcoded image
repo in the "Set image" step (step id "set", output "image") by deriving owner
and repository from GitHub context: use github.repository_owner and
github.event.repository.name (lowercased) to construct the image name instead of
"ghcr.io/ryanmillard/img2num-dev"; also move github.event.pull_request.number
into the step's env to reference it as an environment variable (avoids inline
expression injection) so the step builds either
"ghcr.io/<owner>/<repo-lowercase>:pr-<PR_NUMBER>" for pull requests or
"ghcr.io/<owner>/<repo-lowercase>:main" otherwise.
🤖 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.yml:
- Around line 28-33: The reusable workflow wait-for-docker-image.yml currently
always queries Docker Hub and will never succeed for GHCR images; update the
workflow to detect the registry from IMAGE_NAME (look for prefixes like ghcr.io)
and branch: for Docker Hub keep the existing Docker Hub v2 tags curl using
IMAGE_NAME/TAG, and for GHCR implement the GHCR flow by first requesting an auth
token from https://ghcr.io/token?scope=repository:<name>:pull then calling
GHCR's manifest endpoint https://ghcr.io/v2/<name>/manifests/<tag> with that
Bearer token; ensure the variables IMAGE_NAME, TAG and retry loop governed by
MAX_ATTEMPTS are reused and that failures from the wrong-registry curl do not
exhaust attempts (i.e., choose the correct API based on registry prefix before
attempting requests).

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 14-27: Replace the hardcoded image repo in the "Set image" step
(step id "set", output "image") by deriving owner and repository from GitHub
context: use github.repository_owner and github.event.repository.name
(lowercased) to construct the image name instead of
"ghcr.io/ryanmillard/img2num-dev"; also move github.event.pull_request.number
into the step's env to reference it as an environment variable (avoids inline
expression injection) so the step builds either
"ghcr.io/<owner>/<repo-lowercase>:pr-<PR_NUMBER>" for pull requests or
"ghcr.io/<owner>/<repo-lowercase>:main" otherwise.
🪄 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

Run ID: a7b8a42c-924d-44d4-8253-a37e8804814f

📥 Commits

Reviewing files that changed from the base of the PR and between 12a1510 and 7f2582e.

📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • .github/workflows/docker-ci.yml
  • .github/workflows/wait-for-docker-image.yml
✅ Files skipped from review due to trivial changes (1)
  • .github/workflows/wait-for-docker-image.yml
🚧 Files skipped from review as they are similar to previous changes (1)
  • .github/workflows/docker-ci.yml

Comment thread .github/workflows/ci.yml Outdated
@Ryan-Millard

Copy link
Copy Markdown
Owner Author

@coderabbitai, you said:

.github/workflows/ci.yml (1)
1-11: ⚠️ Potential issue | 🔴 Critical

Fork PRs always receive read-only GITHUB_TOKEN regardless of the permissions block, preventing the PR objective.

The PR objective is to unblock CI on PRs from forks (Docker Hub secrets unavailable). However, in public repos, workflows > triggered by pull_request from a fork receive a read-only GITHUB_TOKEN, regardless of the permissions: block declared at > workflow or job level. This is a security restriction to prevent malicious forks from writing to the base repository's resources.

Consequently:

docker-ci.yml would fail when run on fork PRs because packages: write cannot be granted to fork-triggered tokens, even though it's declared in the workflow.
wait-for-image in ci.yml would never see a pr- tag and would still time out.
To achieve write permissions for fork PRs, use pull_request_target event instead of pull_request, which grants read/write permissions (configurable via the permissions block) even for forks. However, this requires careful checkout of the PR head SHA to avoid security vulnerabilities. Additionally, confirm whether packages: read is sufficient for fork-triggered tokens to pull private packages.

But the workflow pushes to the GHCR just fine with the current permissions and it appears in the package. What is the problem?

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard, you're right that the workflow pushes to GHCR just fine — but that's because this PR is from your own branch (fix/ci/docker-images-on-prs) in the same repository, not from a fork. Same-repo branch PRs receive a token that can honour the declared packages: write permission.

The original concern was about what happens when a contributor opens a PR from a fork of your repo — which is what this PR's objective states it is solving. For those:

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception is where an admin has selected the "Send write tokens to workflows from pull requests" option in the GitHub Actions settings.

And more directly: Pull requests from public forks are still considered a special case and will receive a read token regardless of these settings.

So in practice:

PR source packages: write effective? GHCR push works?
Your own branch (this PR) ✅ Yes ✅ Yes
A fork contributor's PR ❌ No (read-only token) ❌ No

The workflow successfully pushes the pr-<N> tag from this PR, but a fork-contributed PR would still fail at the GHCR login/push step in docker-ci.yml for the same reason Docker Hub secrets were unavailable. The PR objective of unblocking fork PRs may not be fully achieved without either:

  1. Enabling "Send write tokens to workflows from pull requests" in repo settings (security trade-off), or
  2. Using a pull_request_target event with careful head-SHA checkout to retain write permissions for forks.

Would you like me to open a tracking issue for this, or does your repo not currently receive fork-based PRs and this was just a future-proofing concern?


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard, you're right that the workflow pushes to GHCR just fine — but that's because this PR is from your own branch (fix/ci/docker-images-on-prs) in the same repository, not from a fork. Same-repo branch PRs receive a token that can honour the declared packages: write permission.

The original concern was about what happens when a contributor opens a PR from a fork of your repo — which is what this PR's objective states it is solving. For those:

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception is where an admin has selected the "Send write tokens to workflows from pull requests" option in the GitHub Actions settings.

And more directly: Pull requests from public forks are still considered a special case and will receive a read token regardless of these settings.

So in practice:

PR source packages: write effective? GHCR push works?
Your own branch (this PR) ✅ Yes ✅ Yes
A fork contributor's PR ❌ No (read-only token) ❌ No
The workflow successfully pushes the pr-<N> tag from this PR, but a fork-contributed PR would still fail at the GHCR login/push step in docker-ci.yml for the same reason Docker Hub secrets were unavailable. The PR objective of unblocking fork PRs may not be fully achieved without either:

  1. Enabling "Send write tokens to workflows from pull requests" in repo settings (security trade-off), or
  2. Using a pull_request_target event with careful head-SHA checkout to retain write permissions for forks.

Would you like me to open a tracking issue for this, or does your repo not currently receive fork-based PRs and this was just a future-proofing concern?

🧠 Learnings used

@coderabbitai, which job in ci.yml requires the packages: write permission?

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat .github/workflows/ci.yml

Length of output: 2363


@Ryan-Millard — that's the key point, and you're right to push back: no job in ci.yml requires packages: write. The workflow only declares packages: read at the top level, which is used solely to pull the pre-built container image for the lint, cmake-build, build-react-app, and build-docs jobs. packages: read is available to fork-triggered tokens, so ci.yml itself has no permission problem with fork PRs.

My original concern about packages: write being blocked for fork PRs belongs entirely to docker-ci.yml (the workflow that builds and pushes the pr-<N> image). The indirect consequence for ci.yml is:

  1. Fork PR triggers both workflows simultaneously.
  2. docker-ci.yml fails to push the image (no packages: write on fork token) → image never appears in GHCR.
  3. ci.yml's wait-for-image job times out waiting for an image that was never pushed.

So ci.yml's permissions are correctly scoped — the fork-PR gap is purely a docker-ci.yml authoring concern, not a ci.yml one. I should have been more precise in my original comment by pointing to docker-ci.yml rather than framing it as a ci.yml issue.


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain
🏁 Script executed:

#!/bin/bash
cat .github/workflows/ci.yml

Length of output: 2363

@Ryan-Millard — that's the key point, and you're right to push back: no job in ci.yml requires packages: write. The workflow only declares packages: read at the top level, which is used solely to pull the pre-built container image for the lint, cmake-build, build-react-app, and build-docs jobs. packages: read is available to fork-triggered tokens, so ci.yml itself has no permission problem with fork PRs.

My original concern about packages: write being blocked for fork PRs belongs entirely to docker-ci.yml (the workflow that builds and pushes the pr-<N> image). The indirect consequence for ci.yml is:

  1. Fork PR triggers both workflows simultaneously.
  2. docker-ci.yml fails to push the image (no packages: write on fork token) → image never appears in GHCR.
  3. ci.yml's wait-for-image job times out waiting for an image that was never pushed.

So ci.yml's permissions are correctly scoped — the fork-PR gap is purely a docker-ci.yml authoring concern, not a ci.yml one. I should have been more precise in my original comment by pointing to docker-ci.yml rather than framing it as a ci.yml issue.

🧠 Learnings used

@coderabbitai, why did you say there was a problem then? ci.yml doesn't need to write to packages. I'm confused. Did I misunderstand you?

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard — you didn't misunderstand me; my original comment was wrong in its attribution. I raised the concern against ci.yml (Lines 1-11) when it had no business being there. ci.yml only needs packages: read to pull the pre-built container image, and packages: read is perfectly available to fork-triggered tokens — so there is no permission problem in ci.yml at all.

What I should have done is raise the concern directly on docker-ci.yml, since that's the workflow that needs packages: write to push the pr-<N> image. The only knock-on effect on ci.yml is indirect: if docker-ci.yml fails to push (because a fork PR can't get packages: write), wait-for-image will time out waiting for an image that was never published. But that's a consequence of a docker-ci.yml problem, not a ci.yml problem.

Sorry for the confusion — the original critical comment on ci.yml was misdirected and should be disregarded as it applies to ci.yml itself.


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard — you didn't misunderstand me; my original comment was wrong in its attribution. I raised the concern against ci.yml (Lines 1-11) when it had no business being there. ci.yml only needs packages: read to pull the pre-built container image, and packages: read is perfectly available to fork-triggered tokens — so there is no permission problem in ci.yml at all.

What I should have done is raise the concern directly on docker-ci.yml, since that's the workflow that needs packages: write to push the pr-<N> image. The only knock-on effect on ci.yml is indirect: if docker-ci.yml fails to push (because a fork PR can't get packages: write), wait-for-image will time out waiting for an image that was never published. But that's a consequence of a docker-ci.yml problem, not a ci.yml problem.

Sorry for the confusion — the original critical comment on ci.yml was misdirected and should be disregarded as it applies to ci.yml itself.

🧠 Learnings used

@coderabbitai, does that mean that everything is okay for now because docker-ci.yml also has the correct permissions?

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat .github/workflows/docker-ci.yml

Length of output: 5845


@Ryan-Millard — unfortunately, no. Declaring packages: write in the permissions block of docker-ci.yml is not sufficient for fork PRs. Here's why:@Ryan-MillardNo, docker-ci.yml does not have effective permissions for fork PRs, for exactly the same reason as described before.

The GITHUB_TOKEN has write permissions to a number of API endpoints, except in the case of pull requests from forks which are always read. Pull requests from public forks are still considered a special case and will receive a read token regardless of the permissions settings in the workflow YAML.

So even though docker-ci.yml correctly declares:

permissions:
  packages: write
  pull-requests: write

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception is where an admin user has selected the "Send write tokens to workflows from pull requests" option in the GitHub Actions settings.

In practical terms for docker-ci.yml on a fork PR:

Step Works for fork PR?
GHCR login (GITHUB_TOKEN) ✅ Login succeeds (read token is enough to authenticate)
GHCR push (packages: write required) ❌ Fails — token is forced read-only
PR comment (pull-requests: write required) ❌ Fails — token is forced read-only

So the image never gets pushed, and ci.yml's wait-for-image times out as a consequence.

The root issue is in docker-ci.yml using the pull_request event. To truly unblock fork PRs you'd need either:

  1. Enable "Send write tokens to workflows from pull requests" in Settings → Actions → General (security trade-off), or
  2. Refactor to use pull_request_target with a carefully pinned head-SHA checkout (more involved, but the standard secure approach).

Would you like me to open a tracking issue for this?


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain
🏁 Script executed:

#!/bin/bash
cat .github/workflows/docker-ci.yml

Length of output: 5845

@Ryan-Millard — unfortunately, no. Declaring packages: write in the permissions block of docker-ci.yml is not sufficient for fork PRs. Here's why:@Ryan-MillardNo, docker-ci.yml does not have effective permissions for fork PRs, for exactly the same reason as described before.

The GITHUB_TOKEN has write permissions to a number of API endpoints, except in the case of pull requests from forks which are always read. Pull requests from public forks are still considered a special case and will receive a read token regardless of the permissions settings in the workflow YAML.

So even though docker-ci.yml correctly declares:

permissions:
  packages: write
  pull-requests: write

You can use the permissions key to add and remove read permissions for forked repositories, but typically you can't grant write access. The exception is where an admin user has selected the "Send write tokens to workflows from pull requests" option in the GitHub Actions settings.

In practical terms for docker-ci.yml on a fork PR:

Step Works for fork PR?
GHCR login (GITHUB_TOKEN) ✅ Login succeeds (read token is enough to authenticate)
GHCR push (packages: write required) ❌ Fails — token is forced read-only
PR comment (pull-requests: write required) ❌ Fails — token is forced read-only
So the image never gets pushed, and ci.yml's wait-for-image times out as a consequence.

The root issue is in docker-ci.yml using the pull_request event. To truly unblock fork PRs you'd need either:

  1. Enable "Send write tokens to workflows from pull requests" in Settings → Actions → General (security trade-off), or
  2. Refactor to use pull_request_target with a carefully pinned head-SHA checkout (more involved, but the standard secure approach).

Would you like me to open a tracking issue for this?

🧠 Learnings used

@coderabbitai, is there no way around this? I wanted to use GHCR to avoid the problem where secrets weren't allowed in fork workflows related to Docker Hub.

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard — the fundamental issue is that switching from Docker Hub to GHCR does not actually sidestep the fork permission problem. GitHub's standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. This applies equally to GHCR and Docker Hub — both require write access that fork-triggered tokens cannot get. The difference is:

  • Docker Hub: blocked because fork tokens can't access your stored secret credentials.
  • GHCR: blocked because fork tokens cannot be granted packages: write, even with it declared in the permissions: block.

So the GHCR approach works perfectly for this PR (same-repo branch), but would hit the same wall as Docker Hub for a genuine fork contributor's PR.


The proper workaround: workflow_run

The standard pattern for this problem is a two-workflow split:

  1. A pull_request-triggered workflow that runs in the fork context (read-only), does the actual build, and uploads the image as an artifact (tar file).
  2. A workflow_run-triggered workflow that runs in the base repository context (has write permissions), downloads the artifact, and pushes it to GHCR.

The workflow_run event executes workflows after another workflow completes — while useful for privileged operations after validation, it carries security risks. Specifically, attackers can modify triggering workflows in pull requests, which then trigger existing workflow_run workflows with elevated permissions. This means you must treat all artifacts from the triggering workflow as untrusted and validate them carefully. The GitHub Security Lab recommendation is to assign repository privileges only where needed explicitly through pull_request and workflow_run.

The split looks roughly like this:

docker-build.yml (triggered by pull_request, runs in fork context):

on: pull_request
jobs:
  build:
    runs-on: ubuntu-latest
    permissions: {}
    steps:
      - uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
      - name: Build image
        run: docker build -t img2num:pr-${{ github.event.pull_request.number }} .
      - name: Export image
        run: docker save img2num:pr-${{ github.event.pull_request.number }} > /tmp/image.tar
      - uses: actions/upload-artifact@v4
        with:
          name: docker-image-pr-${{ github.event.pull_request.number }}
          path: /tmp/image.tar

docker-push.yml (triggered by workflow_run, runs in base repo context with write permissions):

on:
  workflow_run:
    workflows: ["Build Docker image"]
    types: [completed]
permissions:
  packages: write
jobs:
  push:
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    runs-on: ubuntu-latest
    steps:
      - uses: actions/download-artifact@v4
        with:
          name: docker-image-pr-${{ github.event.workflow_run.pull_requests[0].number }}
          github-token: ${{ secrets.GITHUB_TOKEN }}
          run-id: ${{ github.event.workflow_run.id }}
      - name: Load image
        run: docker load < image.tar
      - name: Login to GHCR
        uses: docker/login-action@v3
        with:
          registry: ghcr.io
          username: ${{ github.actor }}
          password: ${{ secrets.GITHUB_TOKEN }}
      - name: Push to GHCR
        run: docker tag img2num:... ghcr.io/... && docker push ghcr.io/...

pull_request_target — what to avoid

Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise. If you need pull_request_target, never check out the PR head — only use it for labeling or commenting on the base branch code.


Summary for your situation

If fork PRs are uncommon (or non-existent) in this repo, the current GHCR approach is a genuine improvement — it removes the Docker Hub secret dependency and works correctly for all same-repo branch PRs. The fork gap only matters if external contributors actually open PRs. If you want to fully support that case, the workflow_run split is the correct path, but it's meaningfully more complex to implement securely. Would you like me to open a tracking issue for that future work?


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard — the fundamental issue is that switching from Docker Hub to GHCR does not actually sidestep the fork permission problem. GitHub's standard pull_request workflow trigger by default prevents write permissions and secrets access to the target repository. This applies equally to GHCR and Docker Hub — both require write access that fork-triggered tokens cannot get. The difference is:

  • Docker Hub: blocked because fork tokens can't access your stored secret credentials.
  • GHCR: blocked because fork tokens cannot be granted packages: write, even with it declared in the permissions: block.

So the GHCR approach works perfectly for this PR (same-repo branch), but would hit the same wall as Docker Hub for a genuine fork contributor's PR.

The proper workaround: workflow_run

The standard pattern for this problem is a two-workflow split:

  1. A pull_request-triggered workflow that runs in the fork context (read-only), does the actual build, and uploads the image as an artifact (tar file).
  2. A workflow_run-triggered workflow that runs in the base repository context (has write permissions), downloads the artifact, and pushes it to GHCR.

The workflow_run event executes workflows after another workflow completes — while useful for privileged operations after validation, it carries security risks. Specifically, attackers can modify triggering workflows in pull requests, which then trigger existing workflow_run workflows with elevated permissions. This means you must treat all artifacts from the triggering workflow as untrusted and validate them carefully. The GitHub Security Lab recommendation is to assign repository privileges only where needed explicitly through pull_request and workflow_run.

The split looks roughly like this:

docker-build.yml (triggered by pull_request, runs in fork context):

on: pull_request
jobs:
  build:
    runs-on: ubuntu-latest
    permissions: {}
    steps:
      - uses: actions/checkout@v4
        with:
          ref: ${{ github.event.pull_request.head.sha }}
      - name: Build image
        run: docker build -t img2num:pr-${{ github.event.pull_request.number }} .
      - name: Export image
        run: docker save img2num:pr-${{ github.event.pull_request.number }} > /tmp/image.tar
      - uses: actions/upload-artifact@v4
        with:
          name: docker-image-pr-${{ github.event.pull_request.number }}
          path: /tmp/image.tar

docker-push.yml (triggered by workflow_run, runs in base repo context with write permissions):

on:
  workflow_run:
    workflows: ["Build Docker image"]
    types: [completed]
permissions:
  packages: write
jobs:
  push:
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    runs-on: ubuntu-latest
    steps:
      - uses: actions/download-artifact@v4
        with:
          name: docker-image-pr-${{ github.event.workflow_run.pull_requests[0].number }}
          github-token: ${{ secrets.GITHUB_TOKEN }}
          run-id: ${{ github.event.workflow_run.id }}
      - name: Load image
        run: docker load < image.tar
      - name: Login to GHCR
        uses: docker/login-action@v3
        with:
          registry: ghcr.io
          username: ${{ github.actor }}
          password: ${{ secrets.GITHUB_TOKEN }}
      - name: Push to GHCR
        run: docker tag img2num:... ghcr.io/... && docker push ghcr.io/...

pull_request_target — what to avoid

Combining pull_request_target workflow trigger with an explicit checkout of an untrusted PR is a dangerous practice that may lead to repository compromise. If you need pull_request_target, never check out the PR head — only use it for labeling or commenting on the base branch code.

Summary for your situation

If fork PRs are uncommon (or non-existent) in this repo, the current GHCR approach is a genuine improvement — it removes the Docker Hub secret dependency and works correctly for all same-repo branch PRs. The fork gap only matters if external contributors actually open PRs. If you want to fully support that case, the workflow_run split is the correct path, but it's meaningfully more complex to implement securely. Would you like me to open a tracking issue for that future work?

🧠 Learnings used

@coderabbitai, what if I do this before pushing:

- name: Login to GHCR
  uses: docker/login-action@v3
  with:
    registry: ghcr.io
    username: ${{ github.actor }}
    password: ${{ secrets.GITHUB_TOKEN }}

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

@Ryan-Millard — that login step is already present in docker-ci.yml (as added by this PR), and it's not the bottleneck. Here's the distinction:

  • docker login (authentication) — succeeds fine with a fork PR token. It just proves your identity to the registry.
  • docker push (authorization) — requires packages: write scope on the token. GitHub mints the GITHUB_TOKEN with that scope stripped for fork-triggered pull_request workflows, server-side. No action you take in the workflow YAML — including the login step — can add that scope back.

Think of it like having a door key cut to only open the front door: logging in is inserting the key, but trying to push is opening a back-door lock your key was never cut for. The login action itself has no way to re-cut the key.

So the sequence for a fork PR would be:

Step Result
GHCR login with GITHUB_TOKEN ✅ Succeeds (auth doesn't need write)
docker push to GHCR ❌ Fails — token has no packages: write

The workflow_run split I described earlier is the correct way to solve this, because the docker-push.yml workflow runs in the base repository context where packages: write is genuinely available.


🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-09T18:50:40.161Z
Learning: Preference for this repo: When asked to open a tracking issue, automatically create it via GitHub CLI (assign to Ryan-Millard) rather than asking for manual copy/paste, and include backlinks to the PR and the request comment.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.732Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.

Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-18T17:26:42.385Z
Learning: In the Ryan-Millard/Img2Num repository, the `.coderabbit.yaml` includes a `pre_merge_checks.custom_checks` entry named "CodeRabbit Config Needs Update" (mode: warning) that checks if a PR introduces new languages, linter config files, new file extensions, or new CI tooling without a corresponding update to `.coderabbit.yaml`.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)

14-45: Consider short-circuiting the probe on push events.

On push to main, github.event.pull_request.number is empty, so PR_TAG becomes "pr-" and both curls are guaranteed-to-404 round trips to GHCR and Docker Hub before falling through to MAIN_IMAGE. Harmless but wasteful, and if either registry ever rate-limits anonymous HEADs this could flap. Either:

  • gate the job body on if: github.event_name == 'pull_request' and skip the probe entirely for push (returning :main directly), or
  • check [[ -z "${{ github.event.pull_request.number }}" ]] at the top of the script and short-circuit.

Minor; not blocking.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml around lines 14 - 45, The set-image job wastes
network probes on non-PR events because PR_TAG is built from
github.event.pull_request.number; modify the job to short-circuit for
non-pull_request events by checking the event type or empty PR number at the top
of the step: inside the step with id "set" (where MAIN_IMAGE, PR_TAG,
GHCR_STATUS and DH_STATUS are defined) detect if github.event_name !=
'pull_request' or PR number is empty and immediately emit MAIN_IMAGE to
GITHUB_OUTPUT and exit, otherwise run the existing GHCR_STATUS/DH_STATUS probes;
alternatively gate the entire job with if: github.event_name == 'pull_request'
so the probes never run on push.
🤖 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.yml:
- Around line 23-45: The CI uses a single REPO var with the wrong Docker Hub
namespace ("ryan-millard/img2num-dev") causing the DH probe to fail and risking
drift; define separate repo variables (e.g.,
GHCR_REPO="ryan-millard/img2num-dev" and DH_REPO="ryanmillard/img2num-dev"),
keep PR_TAG and MAIN_IMAGE derived from the correct GHCR_REPO/GHCR image, build
DH_IMAGE and GHCR_IMAGE from their respective repo variables, and update the
curl checks (GHCR_STATUS and DH_STATUS) and the echo/GITHUB_OUTPUT lines to use
GHCR_IMAGE and DH_IMAGE so the Docker Hub probe queries the correct namespace
and names aren’t duplicated/hardcoded.

In @.github/workflows/docker-ci.yml:
- Around line 58-71: The awk usage on COMMENT_BODY captures field 2 from every
line, causing PROVIDED_SHA to contain trailing lines; change extraction to take
the second whitespace-separated token only from the first line of COMMENT_BODY
(so PROVIDED_SHA is derived from the first line/token), then validate
PROVIDED_SHA against a git-SHA hex regex (e.g. 7–40 hex chars) before
proceeding; update the logic around PROVIDED_SHA, COMMENT_BODY, CURRENT_SHA and
the subsequent [[ "$CURRENT_SHA" != "$PROVIDED_SHA"* ]] check to use the single
validated token and return the same user-facing error when validation fails.
- Around line 217-241: The GHCR deep-link includes the tag and will 404; update
the comment body to link to the package overview instead of a version-specific
path. In the comment body where you build the package URL using
steps.img.outputs.ghcr_image and env.CLEAN_CORE_TAG, remove the trailing "/${{
env.CLEAN_CORE_TAG }}" so the link points to https://github.com/${{
steps.img.outputs.lower_owner_name }}/${{ github.event.repository.name
}}/packages/container/img2num-dev (or, if you need a deep link, resolve the
numeric version via the Packages API and append ?tag=${{ env.CLEAN_CORE_TAG }}).
Ensure the markdown link and any references in the body (the one under "###
Image") use steps.img.outputs.lower_owner_name and steps.img.outputs.ghcr_image
accordingly.

---

Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 14-45: The set-image job wastes network probes on non-PR events
because PR_TAG is built from github.event.pull_request.number; modify the job to
short-circuit for non-pull_request events by checking the event type or empty PR
number at the top of the step: inside the step with id "set" (where MAIN_IMAGE,
PR_TAG, GHCR_STATUS and DH_STATUS are defined) detect if github.event_name !=
'pull_request' or PR number is empty and immediately emit MAIN_IMAGE to
GITHUB_OUTPUT and exit, otherwise run the existing GHCR_STATUS/DH_STATUS probes;
alternatively gate the entire job with if: github.event_name == 'pull_request'
so the probes never run on push.
🪄 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

Run ID: 948c2c07-57a6-4978-9702-d41f632e7584

📥 Commits

Reviewing files that changed from the base of the PR and between b1d6c91 and 13b8e51.

📒 Files selected for processing (2)
  • .github/workflows/ci.yml
  • .github/workflows/docker-ci.yml

Comment thread .github/workflows/ci.yml
Comment thread .github/workflows/docker-ci.yml
Comment thread .github/workflows/docker-ci.yml
@Ryan-Millard
Ryan-Millard force-pushed the fix/ci/docker-images-on-prs branch from 13b8e51 to c0f3e29 Compare April 21, 2026 22:47
@Ryan-Millard

Copy link
Copy Markdown
Owner Author

/docker-build 13b8e51

@Ryan-Millard
Ryan-Millard force-pushed the fix/ci/docker-images-on-prs branch from c0f3e29 to 29ad493 Compare April 21, 2026 22:56
@Ryan-Millard
Ryan-Millard force-pushed the fix/ci/docker-images-on-prs branch from 29ad493 to 7d6dae9 Compare April 21, 2026 23:33
@Ryan-Millard

Ryan-Millard commented Apr 21, 2026

Copy link
Copy Markdown
Owner Author

@Krasner, please check this out.

It first checks if the container registry has an image for this pr and defaults to :main if there isn't one.

For safety, we do not automatically push anything. Maintainers will need to run the build and push command.

New commands for us to comment:

  1. /docker-build 13b8e51 -> use the commit hash on the PR (must be the most the recent for safety)
  2. /docker-cleanup -> deletes the image from the registry (GHCR)

You can't see it work here because GitHub is stupid and the workflow file needs to be inside the base branch.

I sent you a collaborator request for another repository. Please accept it and look at this PR:

https://github.com/Ryan-Millard/Workflow-Tester/pull/31

The failures on it don't matter because they're unrelated.

@Ryan-Millard

Copy link
Copy Markdown
Owner Author

@Krasner, I also just updated it to only allow you and I to build and push the files (not other maintainers) for safety in the future because a future maintainer might not reason about the dangers of pushing an image without verifying what's going on.

Repository owner deleted a comment from coderabbitai Bot Apr 21, 2026
@Krasner

Krasner commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

ok looks like this could work. we can try merging to main, updating my python branch, and then verifying that this works even for a PR coming from a fork

@Ryan-Millard Ryan-Millard changed the title refactor(docker-ci): switch to use GHCR alongside Docker Hub (better PR integration) refactor(docker-ci): default to main tag for CI and require maintainer sign-off on pushed builds Apr 22, 2026
@Ryan-Millard

Copy link
Copy Markdown
Owner Author

ok looks like this could work. we can try merging to main, updating my python branch, and then verifying that this works even for a PR coming from a fork

My fingers are about to break from how hard I have them crossed.😥

@Krasner

Krasner commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

ok looks like this could work. we can try merging to main, updating my python branch, and then verifying that this works even for a PR coming from a fork

My fingers are about to break from how hard I have them crossed.😥

You got this!

@Ryan-Millard
Ryan-Millard merged commit 6edf3f4 into main Apr 22, 2026
17 checks passed
@Ryan-Millard
Ryan-Millard deleted the fix/ci/docker-images-on-prs branch April 22, 2026 00:20
Ryan-Millard added a commit that referenced this pull request Apr 22, 2026
…ner sign-off on pushed builds (#312)

* refactor(docker-ci): switch to use GitHub packages alongside Docker Hub (better PR integration)

* ci(docker): fix notify job comment

* ci(wait-for-docker-image.yml): fix check logic by including GHCR

* ci(wait-for-docker-image): add default initial wait time & split into steps

* fix(wait-for-docker-image): add ghcr & dh checks so it checks both registries

* ci(docker-ci): fix prefix-collision bug in regex

* ci(wait-for-docker-image): check GHCR push time to verify newness of build

* test - don't worry, CodeRabbit

* ci(docker): update docker-ci.yml to require commands to build and push image

* fix(docker-ci): resolve code injection risk

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* ci(docker): cleanup docker-ci.yml

* fix(docker-ci): accidental image name error

* fix(docker-ci): Checkout of untrusted code in trusted context

* ci(docker): restrict to only Krasner & Ryan-Millard with permission to build

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Ryan-Millard added a commit that referenced this pull request Apr 22, 2026
…ner sign-off on pushed builds (#312)

* refactor(docker-ci): switch to use GitHub packages alongside Docker Hub (better PR integration)

* ci(docker): fix notify job comment

* ci(wait-for-docker-image.yml): fix check logic by including GHCR

* ci(wait-for-docker-image): add default initial wait time & split into steps

* fix(wait-for-docker-image): add ghcr & dh checks so it checks both registries

* ci(docker-ci): fix prefix-collision bug in regex

* ci(wait-for-docker-image): check GHCR push time to verify newness of build

* test - don't worry, CodeRabbit

* ci(docker): update docker-ci.yml to require commands to build and push image

* fix(docker-ci): resolve code injection risk

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* ci(docker): cleanup docker-ci.yml

* fix(docker-ci): accidental image name error

* fix(docker-ci): Checkout of untrusted code in trusted context

* ci(docker): restrict to only Krasner & Ryan-Millard with permission to build

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@Ryan-Millard

Copy link
Copy Markdown
Owner Author

/docker-cleanup

Ryan-Millard added a commit that referenced this pull request Apr 22, 2026
…ner sign-off on pushed builds (#312)

* refactor(docker-ci): switch to use GitHub packages alongside Docker Hub (better PR integration)

* ci(docker): fix notify job comment

* ci(wait-for-docker-image.yml): fix check logic by including GHCR

* ci(wait-for-docker-image): add default initial wait time & split into steps

* fix(wait-for-docker-image): add ghcr & dh checks so it checks both registries

* ci(docker-ci): fix prefix-collision bug in regex

* ci(wait-for-docker-image): check GHCR push time to verify newness of build

* test - don't worry, CodeRabbit

* ci(docker): update docker-ci.yml to require commands to build and push image

* fix(docker-ci): resolve code injection risk

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* ci(docker): cleanup docker-ci.yml

* fix(docker-ci): accidental image name error

* fix(docker-ci): Checkout of untrusted code in trusted context

* ci(docker): restrict to only Krasner & Ryan-Millard with permission to build

---------

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants